Code Review Expert

Code Review: POST /api/checkout

Summary

This handler can't safely take real money in its current state, and it isn't ready for Black Friday. There are four independent critical problems:

• SQL injection in the discount lookup.
• A client-controlled charge amount. The server charges whatever total the browser sends, and the computed discount is never used.
• A live Stripe secret key committed in source.
• A check-then-act race that will oversell stock under concurrent load.

On top of that, the Stripe charge and the DB writes are not coordinated, and the handler has no error handling. Any failure after the charge, including a likely order-ID collision, leaves a customer charged with no order recorded.

The core fix is structural. The server must own pricing. Inventory must be reserved atomically in a short transaction. The charge must be idempotent, with a documented recovery path. A corrected reference implementation is at the end.

Issues

1. Live Stripe secret key hardcoded in source

• Severity: Critical
• Location: module scope, new Stripe('sk_live_51Hxxx...')
• Problem: A live secret key in source control gives anyone with repo access (current or former teammates, CI logs, a leaked laptop, a future accidental public push) full control of the Stripe account. That includes issuing refunds, reading customer data, and creating charges. It's also in git history now, so deleting the line doesn't fix it.
• Fix:
  1. Roll the key in the Stripe dashboard immediately. Treat it as compromised regardless of whether the repo is private.
  2. Load the key from the environment and fail fast at boot if it's missing. Consider a restricted key scoped to PaymentIntents only.
  3. Add a secret scanner (gitleaks, trufflehog, or GitHub secret scanning) to CI.

  const stripeKey = process.env.STRIPE_SECRET_KEY;
  if (!stripeKey) throw new Error('STRIPE_SECRET_KEY is not set');
  const stripe = new Stripe(stripeKey, { apiVersion: '2024-06-20' }); // pin the version
  

2. SQL injection in the discount-code lookup

• Severity: Critical

• Location: const query = `SELECT percent_off FROM discounts WHERE code = '${discountCode}' ...`

• Problem: discountCode comes straight from req.body and is interpolated into SQL. Three concrete attacks:

  • discountCode: "' OR 1=1 --" returns the first discount row, active or not.
  • "x' UNION SELECT 100 --" produces a 100% discount once the discount is actually applied (see #3).
  • Depending on DB role privileges, stacked or time-based payloads can exfiltrate or modify data.

  discountCode also doesn't have to be a string. Express's JSON parser will happily pass an object or array, which makes the interpolation unpredictable.

• Fix: Parameterize the query, as the inventory queries already do, and validate the type and length first.

  const { rows } = await client.query<{ percent_off: string }>(
    'SELECT percent_off FROM discounts WHERE code = $1 AND active = true',
    [discountCode]
  );
  

  Also make sure the app's DB role has only the privileges it needs (no DDL, no superuser), so a future injection bug has limited blast radius.

3. Client controls the charge amount; the discount is computed but never applied

• Severity: Critical

• Location: const { ..., total } = req.body; → amount: Math.round(total * 100)

• Problem: The amount charged is exactly what the browser sends. Anyone with DevTools can post total: 0.50 for a $200 cart and get it. Nothing on the server connects total to the items, and no product prices are looked up anywhere.

  Separately, discount is fetched and then never used. So either the frontend computes the discounted total (making the server lookup pointless) or discounts silently do nothing. Either way, discount_code gets written to the order even when the code was invalid, which will corrupt promo reporting.

  Other edge cases:

  • total can be negative, NaN, or a string ("abc" * 100 is NaN, and Stripe rejects it only after the stock checks have run).
  • Floating-point money: 19.99 * 3 is 59.97000000000001.

• Fix:

  • Remove total from the request contract entirely. The server computes it from authoritative prices.
  • Keep all money as integer cents.
  • Validate percent_off is within [0, 100], since a bad row shouldn't produce a negative charge.
  • Record only codes that were actually applied.

  I'm assuming prices live in a products table or on inventory (e.g. price_cents). The snippet doesn't show where prices come from, and that gap is part of the problem.

  const subtotalCents = lines.reduce((s, l) => s + l.priceCents * l.qty, 0);
  const pct = discount ? Number(discount.percent_off) : 0; // pg returns NUMERIC as string
  if (!(pct >= 0 && pct <= 100)) throw new Error(`Invalid percent_off for ${discountCode}`);
  const discountCents = Math.round((subtotalCents * pct) / 100);
  const totalCents = subtotalCents - discountCents;
  

  If the frontend wants to show a total before submit, add a POST /api/checkout/quote endpoint that uses the same server-side function. Never accept the number back.

4. Check-then-decrement race lets concurrent requests oversell stock

• Severity: Critical (for a flash sale specifically)

• Location: the SELECT quantity loop, followed later by the UPDATE ... quantity - $1 loop

• Problem: This is a textbook TOCTOU (time-of-check to time-of-use) bug. Consider a candle with 1 unit left and 20 concurrent requests:

  1. All 20 SELECT and see quantity = 1.
  2. All 20 pass the check.
  3. All 20 get charged.
  4. All 20 decrement, leaving quantity = -19.

  The window between check and update also includes a Stripe network round trip (hundreds of ms), which makes it huge. At Black Friday concurrency this is near-certain to happen.

  The same bug can be triggered by a single request:

  • Duplicate lines: items: [{productId: 7, qty: 1}, {productId: 7, qty: 1}] with stock 1 passes, because each line is checked against full stock independently.
  • Negative quantities: qty: -50 passes quantity < qty, then quantity - (-50) adds 50 units of phantom inventory.

• Fix: Merge duplicate lines, validate qty as a positive integer, and make the check and the decrement a single atomic operation. Either of these works:

  • A conditional update: UPDATE ... SET quantity = quantity - $1 WHERE product_id = $2 AND quantity >= $1, then verify the rowcount.
  • Row locks inside a transaction: SELECT ... FOR UPDATE.

  With multi-item carts, take the locks in a consistent order (ORDER BY product_id). Otherwise two carts {A, B} and {B, A} can deadlock each other.

  // Inside a transaction:
  const { rows } = await client.query(
    `SELECT i.product_id, i.quantity, p.price_cents
       FROM inventory i JOIN products p ON p.id = i.product_id
      WHERE i.product_id = ANY($1::int[])
      ORDER BY i.product_id
      FOR UPDATE OF i`,
    [productIds]
  );
  // ...verify every requested product exists and has enough stock, then:
  await client.query(
    `UPDATE inventory i SET quantity = i.quantity - c.qty
       FROM unnest($1::int[], $2::int[]) AS c(product_id, qty)
      WHERE i.product_id = c.product_id`,
    [productIds, qtys]
  );
  

  Also add CHECK (quantity >= 0) on inventory.quantity as a last line of defense. With it, an oversell becomes a failed transaction instead of silent negative stock.

5. The Stripe charge and the DB writes aren't coordinated, so money and orders can diverge

• Severity: Critical

• Location: everything after stripe.charges.create(...)

• Problem: The sequence is: charge, then N separate updates, then insert, with no transaction and no compensation. Any failure after the charge leaves a paid customer with no order and no inventory decrement. Possible causes include a DB blip, a pool timeout, a process restart during deploy, or the order-ID collision in #7.

  Because each UPDATE is its own autocommit statement, a failure midway also leaves inventory partially decremented. In the other direction, if the charge fails, nothing is recorded at all, which is fine but gives you no audit trail.

  You also can't just wrap the Stripe call inside a DB transaction. That would hold row locks on your hottest products for the full Stripe round trip, which serializes the flash sale and invites lock timeouts. And a DB commit failure after a successful charge would still leave you charged-but-not-recorded.

• Fix: Use a reserve → charge → finalize pattern with short transactions and an idempotency key:

  1. Tx 1 (short): Validate, lock and decrement inventory, and insert the order with status = 'pending' and the server-computed total_cents. Commit. Stock is now reserved.
  2. Charge via a Stripe PaymentIntent with idempotencyKey = order.id and metadata.order_id. Retries can't double-charge.
  3. Tx 2: On success, set status = 'paid' and store payment_intent_id. On a definitive failure (decline), mark the order failed and restore inventory.
  4. Recovery:
     • A Stripe webhook (payment_intent.succeeded / payment_failed) is the source of truth. It covers crashes between steps 2 and 3.
     • A small reconciliation job sweeps pending orders older than ~15 minutes. It checks Stripe by metadata, then finalizes, or releases the stock.

  An alternative is capture_method: 'manual': authorize first, commit the order, then capture. That's also valid. The key point is to have a documented answer for what happens if the process dies right after Stripe says yes.

  Also, charges.create with source: cardToken is Stripe's legacy API. It doesn't support SCA/3-D Secure, which many cards now require, especially EU cards. Migrate to PaymentIntents with a payment_method id. A requires_action status then has to go back to the frontend with the client_secret.

6. No error handling: async rejections hang requests and crash on bad input

• Severity: High
• Location: the whole handler
• Problem: Express 4 does not catch rejected promises from async handlers. Any thrown error becomes an unhandled rejection, and the client's request hangs until timeout. On Node 15+ with default settings, an unhandled rejection terminates the process, taking down every in-flight checkout. Concrete triggers:
  • An unknown productId makes stock.rows[0] undefined, so .quantity throws a TypeError.
  • A missing or non-array items makes for...of throw.
  • A card decline makes Stripe throw StripeCardError, which is the most common failure in normal use.
  • Any DB error.
• Fix: Wrap the handler (or use express-async-errors, or upgrade to Express 5) and map known errors to proper statuses. Declines → 402, stock conflicts → 409, validation → 400, everything else → 500 with a logged correlation id. Don't leak internal error text to the client.

7. Random order IDs will collide within days and are guessable

• Severity: High
• Location: const orderId = Math.floor(Math.random() * 1000000);
• Problem: By the birthday bound, with 1,000,000 possible IDs a collision is likely (~50%) after only about 1,180 orders. At 200 orders a day that's about 6 days, much sooner on Black Friday. Collisions will already have happened unless something else prevents them. Once a primary-key violation happens it fires after the card was charged (see #5). The IDs are also small and enumerable, which matters if any endpoint looks up orders by id.
• Fix: Let Postgres generate the key (id bigint GENERATED ALWAYS AS IDENTITY, or uuid DEFAULT gen_random_uuid()) and use RETURNING id. If you need a customer-facing order number, make it a separate column.

8. No idempotency: a double-click or retry causes a double charge

• Severity: High
• Location: request contract
• Problem: "Place Order" double-clicks, a flaky mobile connection retrying, or the React app re-submitting after a timeout will each create a second charge and a second order. Under Black Friday load, with slow responses, users will click twice.
• Fix: The client generates a UUID when the checkout page loads and sends it as idempotencyKey. Store it on orders with a UNIQUE constraint. On conflict, return the existing order's result instead of processing again. Disable the button on the frontend too, but don't rely on that.

9. No input validation on the request body

• Severity: High
• Location: const { items, discountCode, cardToken, total } = req.body;
• Problem: Nothing checks shape, types, or bounds. Several of the issues above come directly from this: negative or fractional qty, duplicate lines, non-string discountCode, an empty items array (which currently charges total for nothing), and unbounded array length (a 100k-item cart is effectively a DoS).
• Fix: Validate at the boundary with a schema library, then normalize by merging duplicates.

  const CheckoutBody = z.object({
    items: z.array(z.object({
      productId: z.number().int().positive(),   // assumption: integer product ids
      qty: z.number().int().min(1).max(20),
    })).min(1).max(50),
    discountCode: z.string().trim().min(1).max(64).optional(),
    paymentMethodId: z.string().startsWith('pm_'),
    idempotencyKey: z.string().uuid(),
  }).strict();
  

10. N+1 queries against inventory

• Severity: Medium
• Location: both for (const item of items) loops
• Problem: The handler makes 2N+2 sequential round trips per checkout. The individual queries are cheap, but each one holds a pool connection (default pool size is 10) and adds latency. Under a spike this is what exhausts the pool, and once locking is added (#4), longer transactions mean longer lock hold times.
• Fix: Use one SELECT ... WHERE product_id = ANY($1) ORDER BY product_id FOR UPDATE and one set-based UPDATE ... FROM unnest(...), as in #4. That's a constant number of queries regardless of cart size.

11. The discount system is open to abuse

• Severity: Medium

• Location: discount lookup

• Problem: Even with injection fixed:

  • Codes can be brute-forced, because nothing rate-limits attempts.
  • There's no concept of expiry, max redemptions, or per-customer limits. The schema isn't shown, so I'm assuming only code, percent_off, and active exist.
  • An invalid code is silently ignored but still stored on the order.

  For a Black Friday promo code that gets shared on coupon sites, usage caps usually matter.

• Fix:

  • Rate-limit /checkout, and any future /discounts/validate endpoint, per IP and per session.
  • Return an explicit 400 for an invalid code rather than silently charging full price. Users should know their code didn't apply.
  • If you add max_redemptions, increment it atomically inside Tx 1: UPDATE discounts SET redemptions = redemptions + 1 WHERE code = $1 AND redemptions < max_redemptions RETURNING percent_off. Otherwise it has the same race as inventory.
  • Decide and document case sensitivity, e.g. normalize with upper().

12. Order rows store client-supplied data and omit key facts

• Severity: Medium
• Location: INSERT INTO orders ... [orderId, JSON.stringify(items), total, discountCode, charge.id]
• Problem: items is the raw client payload, which can include arbitrary extra fields. There's no price snapshot, so you can't later reconstruct what the customer paid per item after prices change. total is the client's number. There's no status, customer or email, or timestamp (unless the DB defaults one). None of this is visible in the snippet, but I'd flag it for refunds and support.
• Fix: Store server-computed lines (product_id, qty, unit_price_cents) in an order_items table or a validated JSONB column, plus subtotal_cents, discount_cents, total_cents, status, payment_intent_id, and idempotency_key.

13. Maintainability: one function does everything

• Severity: Medium

• Location: whole handler

• Problem: Validation, pricing, discounting, inventory, payment, and persistence are all inline in the route. Gift-card support is coming next, and it touches pricing (a second deduction), payment (a split tender, possibly with a $0 Stripe charge), and compensation (restoring the gift-card balance on failure). Adding it to this structure will make it untestable. Module-level Pool and Stripe construction also makes unit testing require real infrastructure.

• Fix: Split the work into:

  • validate: a schema
  • priceCart(lines, discount): a pure function, trivially unit-testable, and where gift cards plug in
  • reserveInventory(client, lines)
  • chargePayment(order)
  • finalizeOrder / releaseOrder

  Inject pool and stripe (or at least import them from a shared module) so tests can substitute fakes.

14. Minor issues

• Severity: Low
• Location: various
• Problem / Fix:
  • new Pool(...) inside a route module creates a separate pool per module that does this. Share one pool app-wide.
  • Return 201 Created for a new order. The error body should say which item is short ({ error: 'insufficient_stock', productId }) so the UI can show a useful message.
  • currency: 'usd' and description are fine hardcoded for now, but put them in config before international shipping comes up.
  • Pin the Stripe apiVersion so SDK upgrades don't silently change behavior.
  • Stripe rejects charges under $0.50. Decide what happens when a discount (or later a gift card) brings the total below that.

Strengths

• The inventory queries are parameterized ($1, $2). That's the right habit, and the fix for #2 is just to apply it consistently.
• The decrement is relative (quantity = quantity - $1) rather than read-modify-write in JS (SET quantity = ${newQty}). That avoids one class of lost-update bug and is the right foundation for the atomic version.
• The stock check happens before the charge, which shows the right intent. It just needs to be atomic.
• charge_id is persisted on the order, which is essential for refunds and reconciliation. Keep that (as payment_intent_id).
• Math.round(total * 100) shows awareness that Stripe wants integer minor units. Push that further and keep cents everywhere.
• The handler is short and linear, so the intended flow is easy to follow, which made these issues easy to spot.

Recommendations (prioritized)

1. Today, before anything else: Roll the Stripe live key, move it to an env var, and add secret scanning to CI.
2. Before merge (blocking): Fix the SQL injection (#2), remove client total and compute prices server-side (#3), add schema validation (#9), and add async error handling (#6). Each is small and independently shippable.
3. Before Black Friday (blocking): Implement the reserve → charge → finalize flow:
   • atomic, lock-ordered inventory reservation (#4)
   • DB-generated IDs (#7)
   • idempotency keys (#8)
   • PaymentIntents with an idempotency key, plus a webhook and a reconciliation job (#5)
   • the CHECK (quantity >= 0) constraint as a backstop
4. Load-test the concurrency fix. Seed a product with stock 10, fire 100 parallel checkouts with Stripe test mode or a mocked payment layer, and assert exactly 10 succeed, stock ends at 0, and no rows are negative. Include a mixed-order multi-item scenario to shake out deadlocks. This single test would have caught #4 and will guard against regressions.
5. Before gift cards: Extract priceCart as a pure function and the payment step behind an interface. Gift cards then become one more deduction in pricing and one more compensating action in releaseOrder.
6. Add tests for these cases:
   • Security: injection strings in discountCode, a manipulated total being ignored, a non-string discountCode.
   • Input handling: negative, zero, or fractional qty; duplicate product lines; an unknown productId; an empty cart.
   • Discounts: invalid or inactive codes; percent_off of 0, 100, or out of range.
   • Payment failures: a Stripe decline releases inventory and marks the order failed; a Stripe timeout followed by a retry with the same idempotency key does not double-charge.
   • Persistence failures: a DB failure after a successful charge is recovered by the webhook or reconciliation job.
   • Idempotency: a replayed request returns the original order.
   • Concurrency: the load test above.
7. Operational: Add rate limiting on /checkout, structured logging with order id and payment intent id on every step, and alerting on pending orders older than the reconciliation window.

Reference implementation (sketch)

This is illustrative, not drop-in. It assumes a products(id, price_cents) table, and orders columns status, total_cents, idempotency_key UNIQUE, and payment_intent_id. The idempotent-replay lookup and the webhook/reconciler are omitted for brevity.

import { Router, Request, Response, NextFunction } from 'express';
import { PoolClient } from 'pg';
import Stripe from 'stripe';
import { z } from 'zod';
import { pool } from '../db';        // single shared pool
import { stripe } from '../stripe';  // key from env, apiVersion pinned

const router = Router();

const CheckoutBody = z.object({
  items: z.array(z.object({
    productId: z.number().int().positive(),
    qty: z.number().int().min(1).max(20),
  })).min(1).max(50),
  discountCode: z.string().trim().min(1).max(64).optional(),
  paymentMethodId: z.string().startsWith('pm_'),
  idempotencyKey: z.string().uuid(),
}).strict();

class HttpError extends Error {
  constructor(public status: number, public code: string, public details?: object) { super(code); }
}

async function withTx<T>(fn: (c: PoolClient) => Promise<T>): Promise<T> {
  const client = await pool.connect();
  try {
    await client.query('BEGIN');
    const result = await fn(client);
    await client.query('COMMIT');
    return result;
  } catch (err) {
    await client.query('ROLLBACK').catch(() => {});
    throw err;
  } finally {
    client.release();
  }
}

/** Pure: easy to unit test, and where gift cards will plug in. */
export function priceCart(lines: { priceCents: number; qty: number }[], percentOff: number) {
  if (!(percentOff >= 0 && percentOff <= 100)) throw new Error('invalid percent_off');
  const subtotalCents = lines.reduce((s, l) => s + l.priceCents * l.qty, 0);
  const discountCents = Math.round((subtotalCents * percentOff) / 100);
  return { subtotalCents, discountCents, totalCents: subtotalCents - discountCents };
}

router.post('/checkout', async (req: Request, res: Response, next: NextFunction) => {
  try {
    const parsed = CheckoutBody.safeParse(req.body);
    if (!parsed.success) throw new HttpError(400, 'invalid_request');
    const { items, discountCode, paymentMethodId, idempotencyKey } = parsed.data;

    // Merge duplicate lines so per-product stock checks are correct.
    const qtyById = new Map<number, number>();
    for (const i of items) qtyById.set(i.productId, (qtyById.get(i.productId) ?? 0) + i.qty);
    const productIds = [...qtyById.keys()].sort((a, b) => a - b);
    const qtys = productIds.map((id) => qtyById.get(id)!);

    // Tx 1: price, reserve stock, create pending order. Short; no network calls inside.
    const order = await withTx(async (client) => {
      const { rows: stock } = await client.query<{ product_id: number; quantity: number; price_cents: number }>(
        `SELECT i.product_id, i.quantity, p.price_cents
           FROM inventory i JOIN products p ON p.id = i.product_id
          WHERE i.product_id = ANY($1::int[])
          ORDER BY i.product_id
          FOR UPDATE OF i`,
        [productIds]
      );
      const byId = new Map(stock.map((r) => [r.product_id, r]));
      for (const [id, qty] of qtyById) {
        const row = byId.get(id);
        if (!row) throw new HttpError(400, 'unknown_product', { productId: id });
        if (row.quantity < qty) throw new HttpError(409, 'insufficient_stock', { productId: id });
      }

      let percentOff = 0;
      if (discountCode) {
        const { rows } = await client.query<{ percent_off: string }>(
          'SELECT percent_off FROM discounts WHERE code = $1 AND active = true',
          [discountCode]
        );
        if (rows.length === 0) throw new HttpError(400, 'invalid_discount_code');
        percentOff = Number(rows[0].percent_off);
      }

      const lines = productIds.map((id, idx) => ({ productId: id, qty: qtys[idx], priceCents: byId.get(id)!.price_cents }));
      const { totalCents } = priceCart(lines, percentOff);
      if (totalCents < 50) throw new HttpError(400, 'total_below_minimum');

      await client.query(
        `UPDATE inventory i SET quantity = i.quantity - c.qty
           FROM unnest($1::int[], $2::int[]) AS c(product_id, qty)
          WHERE i.product_id = c.product_id`,
        [productIds, qtys]
      );

      const { rows: [o] } = await client.query<{ id: string }>(
        `INSERT INTO orders (items, total_cents, discount_code, status, idempotency_key)
         VALUES ($1, $2, $3, 'pending', $4) RETURNING id`,
        [JSON.stringify(lines), totalCents, discountCode ? discountCode : null, idempotencyKey]
      );
      return { id: o.id, totalCents };
    });
    // NOTE: handle unique-violation on idempotency_key (pg code 23505) by returning the existing order.

    // Charge outside any transaction; idempotent on order id.
    let intent: Stripe.PaymentIntent;
    try {
      intent = await stripe.paymentIntents.create(
        {
          amount: order.totalCents,
          currency: 'usd',
          payment_method: paymentMethodId,
          confirm: true,
          automatic_payment_methods: { enabled: true, allow_redirects: 'never' },
          metadata: { order_id: order.id },
        },
        { idempotencyKey: `order-${order.id}` }
      );
    } catch (err) {
      if (err instanceof Stripe.errors.StripeCardError) {
        await releaseOrder(order.id, productIds, qtys);
        throw new HttpError(402, 'card_declined');
      }
      // Ambiguous failure (timeout/5xx): leave 'pending'; webhook + reconciler resolve it.
      throw err;
    }

    if (intent.status === 'requires_action') {
      return res.status(202).json({ orderId: order.id, clientSecret: intent.client_secret });
    }

    await pool.query(
      `UPDATE orders SET status = 'paid', payment_intent_id = $2 WHERE id = $1 AND status = 'pending'`,
      [order.id, intent.id]
    );
    res.status(201).json({ orderId: order.id });
  } catch (err) {
    next(err); // central error middleware maps HttpError -> status, logs others, returns 500
  }
});

async function releaseOrder(orderId: string, productIds: number[], qtys: number[]) {
  await withTx(async (client) => {
    const { rowCount } = await client.query(
      `UPDATE orders SET status = 'failed' WHERE id = $1 AND status = 'pending'`,
      [orderId]
    );
    if (rowCount === 0) return; // already finalized elsewhere; don't double-restore
    await client.query(
      `UPDATE inventory i SET quantity = i.quantity + c.qty
         FROM unnest($1::int[], $2::int[]) AS c(product_id, qty)
        WHERE i.product_id = c.product_id`,
      [productIds, qtys]
    );
  });
}

export default router;

Once gift cards are added, they slot into priceCart (an extra deduction), Tx 1 (an atomic balance decrement alongside inventory), and releaseOrder (restoring the balance). The handler's shape stays the same.