[alecritson]

Security audit follow-up (Edwardes EDW2-205). Three gaps:

Webhook-first placement on an unpinned session

A customer who has a payment intent can confirm it with Stripe.js without ever calling pay(). The webhook's authorize() places the order, but CompleteSessionOnPaymentSuccess only completed PaymentProcessing sessions. The session stayed Open, so none of complete() ran: no OrderPlacing listeners (PO reference, account reference), no contact email stamp, no meta.payment_method, no confirmation handover.

By the time the webhook arrives the money is taken, so refusing the order would leave a paid customer with nothing. The listener now pins an Open session for that cart the way the pay boundary does, against the live cart the gateway just charged for. It flags the session meta.completed_without_pay_step and logs a warning, then runs complete() like any other webhook-first completion. Expiry is deliberately not checked, because the payment already happened. Failed payments leave the session alone.

storeContact had no operability gate

The signed-in branch goes through the driver's operableCart(). The guest branch wrote customer_email straight onto the session, including PaymentProcessing and terminal sessions. That email is what completion stamps on the order. The guest branch now applies the same gate: CheckoutSessionConflictException('frozen') (409) and CheckoutSessionNotOperableException (410).

Processing poll 403 after a webhook completion

Once the gateway places the order, CartSession::current() mints the browser a fresh cart. If the webhook beat the guest's next processing poll, ensureOwnership() saw a different cart and returned 403, and the guest never got the confirmation handover. pay() now records the pinned session's UUID in the browser session (the last 5 are kept), and ensureOwnership() accepts that for PaymentProcessing and Completed sessions. It checks this before calling current(). A browser that never pinned the session still gets 403.

Review follow-ups

• The pin re-reads the session row under lockForUpdate() and writes the merged meta. A JSON-path write was tried first, but MySQL's JSON_SET on a NULL column returns NULL and dropped the flag.
• Open hold-mode sessions are never adopted. They stay Open on purpose until the confirm page captures at reconcile.
• storePaymentIntent refuses terminal and expired sessions (ensureOperable), and refuses PaymentProcessing sessions with 409 frozen, because replacing the charged intent would strand reconciliation.
• The listener docblock notes that an OrderPlacing listener must not authorise a payment itself (re-entrancy).
• More tests: a replayed success completes once; an expired Open session is still adopted; the chosen payment method survives the pin; hold-mode sessions are left Open; an expired session's contact change gets 410; a payment intent on a completed or pinned session gets 409.
• Verified on MySQL as well as SQLite. All new tests pass on both. The only 2 MySQL failures (OrderDetailsElementTest PO/notes, and the collect point stamp) fail identically on main: they come from JSON key ordering and are unrelated.

Tests

• WebhookFirstPlacementTest: an Open session completes and adopts the gateway's order, with OrderPlacing dispatched once; a failed payment leaves the session Open.
• ContactInformationTest: guest email change refused with 409 when frozen and 410 when terminal.
• PayBoundaryTest: the pinning browser can poll after a webhook completion; a browser that never pinned still gets 403.

Each new test failed before its fix. Package suite: 304 passed, 1 skipped. phpstan and pint are clean.

🤖 Generated with Claude Code

[alecritson]

I've left 6 comments: Pinned-session ownership grant on Completed sessions never expires; Guest contact gate duplicates ensureOperable() and disagrees on status codes; Webhook-first customers never get the pinned-session ownership grant; Listener's session-selection and lost-race branches are untested; Pin snapshot is taken outside the lock and bypasses state transitions; and 1 more.

[alecritson]

Pinned-session ownership grant on Completed sessions never expires
Low · Security

ensureOwnership() now lets any request carrying the browser session through for a Completed checkout session whose UUID is in lunar.checkout.pinned. The grant has no time limit. It only drops off after five newer pins or when the session is invalidated.

Before this change, a completed guest session stopped being reachable once the cart rotated. Now, on a shared or kiosk browser, the next person can open the processing/confirmation routes for an earlier guest's order and see their email, address and order details for as long as the Laravel session lives.

Fix: store a timestamp with each pinned UUID and only honour it for a short window, e.g. ['uuid' => ..., 'at' => now()->timestamp] checked against roughly 1 hour. Or limit the bypass to the processing route instead of every ensureOwnership() caller.

[alecritson]

Fixed in 99b8225. Each pinned UUID is now stored with the time it was recorded, and ensureOwnership() only honours it for PINNED_SESSION_TTL (1 hour). Test: lets a pinned-session ownership grant lapse after an hour.

[alecritson]

Guest contact gate duplicates ensureOperable() and disagrees on status codes
Low · Full review

storeContact() writes its own gate: PaymentProcessing → 409, and non-Open or expired → 410, so Completed gets 410. storePaymentIntent() uses ensureOperable() plus a 409 check, and its new test expects 409 for a Completed session. Two endpoints now answer a completed session with different status codes, and the hand-written copy can drift from ensureOperable() and the driver's operableCart().

Fix: call ensureOperable() in the guest branch, adding the PaymentProcessing → 409 check if needed. Alternatively, pull the frozen-or-gone gate into one helper used by storeContact, storePaymentIntent and operableCart(), so terminal sessions get the same status code everywhere.

[alecritson]

Fixed in 99b8225. storeContact (guest branch) and storePaymentIntent now share one ensureWritable() gate that matches operableCart(): PaymentProcessing gets 409 frozen, and terminal or expired sessions get 410. The completed-session intent test now expects 410. ensureOperable() (the bare 409 used by storeElement and friends) is unchanged.

[alecritson]

Webhook-first customers never get the pinned-session ownership grant
Medium · Full review

rememberPinnedSession() is only called from pay(). In the webhook-first flow this PR adds, the customer confirms with Stripe.js and never calls pay(), so their browser session never gets the UUID.

If the webhook arrives before the Stripe.js return_url redirect or the first processing poll, the listener completes the session and CartSession::current() mints a fresh cart. ensureOwnership() then falls through to the cart check and returns 403. That is the same race the PR fixes for the pay() path, and the PR description lists "no confirmation handover" as one of the symptoms being fixed.

Fix: also call rememberPinnedSession($session) in storePaymentIntent() after the intent is created, since the browser holding the intent is the one that can confirm it. Add a test where the listener completes an Open session and the same browser then polls processing.

[alecritson]

Good catch, fixed in 99b8225. storePaymentIntent now records the grant once the intent is created, since that browser can confirm with the gateway directly. New test: lets the browser that created the intent reach the confirmation after a webhook-first completion. It creates an intent through the route, has the gateway place the order, checks the listener completes the session, and then the same browser's poll redirects to success with the handover. It fails without the change.

[alecritson]

Listener's session-selection and lost-race branches are untested
Low · Tests & coverage

Two new branches in CompleteSessionOnPaymentSuccess have no tests:

• Session selection: the orderByRaw that prefers a PaymentProcessing session over an Open one for the same cart. No test creates both and checks which session is completed and which is left alone.
• Lost race: the $pinned !== 1 path in pinForGatewayPayment(), where another process moved the session first. Add a test that moves the session to PaymentProcessing or Completed between the outer query and the pin (e.g. via a driver double or by calling pinForGatewayPayment on a stale model). Check that complete() runs only when appropriate and that no warning is logged.

[alecritson]

Lost race: covered in 99b8225 by swapping the driver for a proxy whose snapshot() moves the session under the listener. One test has pay() win (session ends Completed, no flag, no warning logged) and one has another process cancel it (left Cancelled, no completion, no warning).

Session selection: dropped instead of tested. active_cart_reference is unique across live sessions, so a cart can never have an Open and a PaymentProcessing session at once. The test I wrote for it hit that constraint. The orderByRaw was dead code, so the query now just carries a comment explaining why it can only match one row.

[alecritson]

Pin snapshot is taken outside the lock and bypasses state transitions
Low · Full review

pinForGatewayPayment() calls $this->driver->snapshot($session) before it opens the transaction and takes the row lock. It then sets the status with a raw query-builder update().

• The amounts and fingerprint come from a cart read made before the lock, so a concurrent cart change can end up stored on the pinned session.
• The raw update skips model events and any state-transition hooks or StateChanged listeners that a normal Open → PaymentProcessing transition fires.

If the pay boundary also uses a raw update, document that this matches it. Otherwise, move the snapshot inside the transaction and go through the same transition code the pay boundary uses.

[alecritson]

Fixed in 99b8225. The snapshot is now taken inside the transaction, after the row lock. The raw guarded update is deliberate: it's the same one-statement Open → PaymentProcessing update the pay boundary uses in LunarCheckoutDriver::assertReadyForPayment() (session states move through these guards, not model transitions), and the comment now says so. complete() also re-verifies the fingerprint against the cart under the cart-row lock.

[alecritson]

Poll-after-webhook test only asserts that some redirect happened
Low · Tests & coverage

->assertRedirect() passes for any 3xx, including a redirect back to the cart or an error page. The point of the test is that the pinning browser reaches the confirmation handover, so it should assert the target, e.g. ->assertRedirect(route('lunar.checkout.confirmation', ...)) or whatever the handover route is. Otherwise a regression that redirects somewhere else would go unnoticed.

[alecritson]

Fixed in 99b8225. It now asserts the redirect goes to the session's success URL and that lunar.checkout.completed.order_reference holds the session's order, so a redirect anywhere else, or one without the handover, fails.