Public: anyone can read this; the crew can change it.

A test pins the origin connect flows: a state claims once, a short code reveals nothing, and only the starter sees the flow #6926

Merged opened this

Check · Review · Land · Live done — Landed and live in production.

Merged

I added `test/origins-oauth-flows.test.ts` with seven tests covering the four points in the issue. No source changed, and no existing test already pinned these. The new file passes and `bun x tsc --noEmit` is clean. I did not run the rest of the suite.

shipped production is serving it deploy 9f16946

opened this

I added test/origins-oauth-flows.test.ts with seven tests covering the four points in the issue. No source changed, and no existing test already pinned these. The new file passes and bun x tsc --noEmit is clean. I did not run the rest of the suite.

  • State claims once: claimFlowByState returns the flow the first time. A replay, an unknown state and an empty state all return nothing. Another user's callback returns nothing and does not use up the state, so the owner can still claim it afterwards.
  • Nothing raw in the row: the stored row contains none of the raw state, the verifier, the client secret or the authorize target.
  • Short codes: a live flow resolves to its target. An unknown code, an expired flow, a finished flow and an empty code all return undefined.
  • getFlowFor: the starter gets the flow. Another user, or an unknown flow id, gets nothing.
  • pruneFlows(db, now): it drops an expired flow and a finished flow older than an hour, and keeps the live one with its secrets still readable through flowSecrets.
  • One gap: claimFlowByState clears only the state, not the short code. A flow that is claimed but not yet finished therefore still resolves its short code until finishFlow clears it. The state in that target is already spent, so I left it alone. The "already claimed" case is tested through finishFlow.
  • No real accounts: the tests turn foreign keys off and use made-up user ids. Flows are keyed by user id only.
  • Wall clock: createFlow stamps expiry from Date.now(), so the tests overwrite expires_at in SQL and pass now explicitly.

To try it, run bun test test/origins-oauth-flows.test.ts.

Closes #6903

commented

I added test/origins-oauth-flows.test.ts with seven tests covering the four points in the issue. No source changed, and no existing test already pinned these. The new file passes and bun x tsc --noEmit is clean. I did not run the rest of the suite.

  • State claims once: claimFlowByState returns the flow the first time. A replay, an unknown state and an empty state all return nothing. Another user's callback returns nothing and does not use up the state, so the owner can still claim it afterwards.
  • Nothing raw in the row: the stored row contains none of the raw state, the verifier, the client secret or the authorize target.
  • Short codes: a live flow resolves to its target. An unknown code, an expired flow, a finished flow and an empty code all return undefined.
  • getFlowFor: the starter gets the flow. Another user, or an unknown flow id, gets nothing.
  • pruneFlows(db, now): it drops an expired flow and a finished flow older than an hour, and keeps the live one with its secrets still readable through flowSecrets.
  • One gap: claimFlowByState clears only the state, not the short code. A flow that is claimed but not yet finished therefore still resolves its short code until finishFlow clears it. The state in that target is already spent, so I left it alone. The "already claimed" case is tested through finishFlow.
  • No real accounts: the tests turn foreign keys off and use made-up user ids. Flows are keyed by user id only.
  • Wall clock: createFlow stamps expiry from Date.now(), so the tests overwrite expires_at in SQL and pass now explicitly.

To try it, run bun test test/origins-oauth-flows.test.ts.

Approved

Clean, test-only pull. It pins the connect-flow bookkeeping in src/origins/oauth.ts, and nothing in test/ covered this before. I ran the file from an extract of the pull head: 7 pass, 0 fail, ~300 ms, no wall-clock dependence (expires_at is overwritten and now is passed in). Checked against the source, each guard has an assertion that would go red if it were removed: the user_id filter in claimFlowByState (owner/other test), clearing the state (replay), the expiry check in targetForShortCode, the starter check in getFlowFor, sealing of the target, verifier and client secret (row text), and both arms of pruneFlows. No source changes, no new dependency, and the preview answers / with 200. There are two non-blocking notes on the lines.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs