fix: harden Steward checkout callback#7891
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
| .then((session) => { | ||
| if (session.token) { | ||
| writeStoredStewardToken(session.token); | ||
| } | ||
| setIsAuthed(Boolean(session.token) || hasStewardAuthedCookie()); |
There was a problem hiding this comment.
setIsAuthed will silently fail if token is absent and cookie hasn't been written yet
setIsAuthed(Boolean(session.token) || hasStewardAuthedCookie()) sets the user as unauthenticated when session.token is absent and the steward-authed cookie hasn't been applied by the browser. Cookie availability after a cross-origin credentials: "include" response is generally synchronous once .then() fires, but this is a departure from the previous unconditional setIsAuthed(true) on any 2xx. If the server-side gate on shouldReturnClientToken is ever fixed to withhold the token from non-checkout origins, callers from those origins will land on the unauthenticated state after a fully successful code exchange.
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
@Dexploarer hey — quick read, the intent is right (Stripe POST from 1. ...(shouldReturnClientToken(c, isProduction) ? { token } : {}),
expiresAt: ...,
expiresIn: ...,
token, // ← always returned, kills the gateEvery permitted origin still gets the token, which contradicts the "refresh stays cookie-only" claim in the PR description. Either drop the unconditional 2. The Playwright tests + hash-strip + |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Summary
Verification
Greptile Summary
This PR hardens the Steward checkout callback to support the hardware checkout flow on elizaos.ai: it adds a
shouldReturnClientTokengate so the nonce-exchange endpoint can return the bearer to that origin, extends token-param handling to cover bothtoken/refreshTokenandaccess_token/refresh_tokennaming conventions, and strips token-bearing hash fragments before React boots. Playwright regression tests are added for the code-exchange, bearer-propagation, and hash-stripping paths.steward-nonce-exchange/route.ts: newshouldReturnClientTokenhelper gates token exposure by origin; a previously-flagged bug leavestokenunconditionally in the response via a later property that overrides the conditional spread.CheckoutPage.tsx: refactors query/hash token reading behindreadStewardTokenParams/removeStewardTokenParamshelpers that handle both naming conventions; the code-exchange path now writes the returned bearer tolocalStoragesostartStripeCheckoutcan pick it up.gitleaks.yml: tightens secret scanning to diff-range only for PR/push events, replacing the previous whole-tree scan.Confidence Score: 4/5
The checkout token-exchange flow is mostly correct, but the origin gate in the nonce-exchange route has no runtime effect because the unconditional token property overrides the conditional spread — an issue flagged in a prior review that remains unresolved in this revision.
The auth handler still returns the access token unconditionally to all permitted origins, regardless of the shouldReturnClientToken result. Until the later token property is removed, every *.elizacloud.ai caller receives the bearer in the response body, not only the hardware checkout origin as intended.
packages/cloud-api/auth/steward-nonce-exchange/route.ts — the token property on line 415 overrides the conditional spread immediately above it.
Important Files Changed
Comments Outside Diff (2)
packages/cloud-api/auth/steward-nonce-exchange/route.ts, line 408-417 (link)The new
shouldReturnClientTokenspread (line 412) is immediately overridden by the unconditionaltoken,property on line 415. In a JS/TS object literal, a later key always wins, so every permitted origin receives the access token in the response body regardless of origin — the checkout-only restriction described in the PR and in the comment block above this return has no runtime effect.refreshTokenis similarly always exposed, contradicting the stated "refresh stays cookie-only" guarantee.packages/shared/src/steward-session-client/index.ts, line 303-316 (link)tokenproperty inStewardNonceExchangeResponsetoken?: stringis declared twice in the same interface body — once at the top (added by this PR) and again at line 314 (pre-existing with the JSDoc). TypeScript emitsTS2300: Duplicate identifier 'token'for this. Thepackages/sharedtypecheck doesn't appear in the PR's verification steps, so the error may not have been caught. Remove the duplicate declaration at line 304 and keep the annotated one.Reviews (6): Last reviewed commit: "fix: scope secret scan and cloud e2e run..." | Re-trigger Greptile