domain: authorization code redemption is non-atomic, concurrent redeems bypass single-use #49

New issue

Closed

opened 2026-07-03 01:41:43 +00:00 by rosa · 1 comment

rosa commented

2026-07-03 01:41:43 +00:00

Owner

Found during a code review of the domain crate.

Location: crates/domain/src/services.rs:824 (redeem_authorization_code)
Severity: Security — ADR-0008 single-use bypass

Problem

redeem_authorization_code performs a non-atomic find then in-memory redeem then delete, with no row lock, and delete_authorization_code ignores rows_affected. Two concurrent redemptions of the same code can both pass verification before either DELETE lands, and each mints an access token.

Failure scenario

An attacker who intercepts a code (or a client that double-submits) fires two POST /token requests concurrently. Both pass find_authorization_code_by_id and record.redeem, then both DELETE (idempotent), and two bearer tokens are issued from one single-use code.

Suggested fix

Make consumption atomic: a conditional DELETE ... RETURNING (or UPDATE ... WHERE not-yet-redeemed) that the first redemption wins, and treat a zero-row result as invalid_grant.

Found during a code review of the `domain` crate. **Location:** `crates/domain/src/services.rs:824` (`redeem_authorization_code`) **Severity:** Security — ADR-0008 single-use bypass ## Problem `redeem_authorization_code` performs a non-atomic find then in-memory `redeem` then delete, with no row lock, and `delete_authorization_code` ignores `rows_affected`. Two concurrent redemptions of the same code can both pass verification before either DELETE lands, and each mints an access token. ## Failure scenario An attacker who intercepts a code (or a client that double-submits) fires two `POST /token` requests concurrently. Both pass `find_authorization_code_by_id` and `record.redeem`, then both DELETE (idempotent), and two bearer tokens are issued from one single-use code. ## Suggested fix Make consumption atomic: a conditional DELETE ... RETURNING (or UPDATE ... WHERE not-yet-redeemed) that the first redemption wins, and treat a zero-row result as invalid_grant.

rosa referenced this issue

2026-07-03 01:42:23 +00:00

domain: redeem_authorization_code deletes the code before loading the user, burning it on transient errors #61

rosa added the

needs-triage

label

2026-07-03 01:43:52 +00:00

rosa commented

2026-07-03 20:55:18 +00:00

Author

Owner

This was generated by AI during triage.

Verified during triage by code inspection: redemption is a load, an in-memory check, and a separate unconditional DELETE whose affected-row count is ignored — nothing serializes two concurrent redemptions of the same code. This brief also covers issue #61: the DELETE precedes the user load, so a transient failure after the consume burns the code with nothing granted. Both defects live in the same function and the same fix must resolve both orderings, so they share this brief.

Agent Brief

Category: bug
Summary: Make authorization-code consumption atomic (single-use under concurrency, per ADR-0008) and consume the code only when a grant is actually issued (covers issues #49 and #61)

Current behavior:
redeem_authorization_code loads the AuthorizationCodeRecord, runs the binding checks (AuthorizationCodeRecord::redeem — client, redirect URI, PKCE, expiry, verifier), deletes the code through the repository, then loads the user and returns the grant. Two defects:

The find → check → delete sequence is not atomic and the repository delete ignores how many rows it removed. Two concurrent redemptions of the same code can both pass the checks before either delete lands, and each mints an access token — violating ADR-0008's single-use guarantee.
The delete happens before the user lookup, so a transient repository error (or a missing user) after the delete permanently burns a valid code with nothing granted; the client's retry gets invalid_grant and must restart the whole authorization flow.

Desired behavior:
Consuming a code is a single atomic claim that exactly one caller can win; every concurrent or subsequent attempt fails with AuthorizationError::InvalidGrant. A code is consumed only once nothing fallible stands between the claim and returning the grant: a failure loading the user (or any other transient error before the claim) leaves the code intact and redeemable. The binding checks and their error behavior are unchanged.

Key interfaces:

The repository port's authorization-code deletion must become (or be replaced by) a conditional consume whose result distinguishes "I claimed it" from "someone else already did" — e.g. a conditional DELETE … RETURNING-style operation, with the zero-row case mapped to InvalidGrant in the service.
redeem_authorization_code reorders so fallible loads (the user) happen before the claim.
The in-memory mock repository must implement the same consume semantics so service-level tests exercise the contract (issue #63 tracks broader mock divergence; keep this method in sync here regardless).

Acceptance criteria:

Redeeming a code that has already been consumed yields InvalidGrant (the claim reports zero rows; no second token is minted).
A repository conformance test covers the consume operation: first consume succeeds, second consume of the same code reports "already consumed".
When the user lookup fails transiently during redemption, the code is not consumed and a subsequent redemption of the same code succeeds (testable against the mock repository).
A successful redemption still deletes the code — a replay after success gets InvalidGrant.
The ci mise task passes.

Out of scope:

Access-token issuance, refresh, or revocation semantics (issue #62 covers revocation).
Changing ADR-0008's rules (PKCE, expiry, single-use policy).
The consent or authorization-request half of the flow.
Fixing the broader in-memory mock divergence beyond this consume method (issue #63).

> *This was generated by AI during triage.* Verified during triage by code inspection: redemption is a load, an in-memory check, and a separate unconditional DELETE whose affected-row count is ignored — nothing serializes two concurrent redemptions of the same code. This brief also covers issue #61: the DELETE precedes the user load, so a transient failure after the consume burns the code with nothing granted. Both defects live in the same function and the same fix must resolve both orderings, so they share this brief. ## Agent Brief **Category:** bug **Summary:** Make authorization-code consumption atomic (single-use under concurrency, per ADR-0008) and consume the code only when a grant is actually issued (covers issues #49 and #61) **Current behavior:** `redeem_authorization_code` loads the `AuthorizationCodeRecord`, runs the binding checks (`AuthorizationCodeRecord::redeem` — client, redirect URI, PKCE, expiry, verifier), deletes the code through the repository, then loads the user and returns the grant. Two defects: 1. The find → check → delete sequence is not atomic and the repository delete ignores how many rows it removed. Two concurrent redemptions of the same code can both pass the checks before either delete lands, and each mints an access token — violating ADR-0008's single-use guarantee. 2. The delete happens before the user lookup, so a transient repository error (or a missing user) after the delete permanently burns a valid code with nothing granted; the client's retry gets `invalid_grant` and must restart the whole authorization flow. **Desired behavior:** Consuming a code is a single atomic claim that exactly one caller can win; every concurrent or subsequent attempt fails with `AuthorizationError::InvalidGrant`. A code is consumed only once nothing fallible stands between the claim and returning the grant: a failure loading the user (or any other transient error before the claim) leaves the code intact and redeemable. The binding checks and their error behavior are unchanged. **Key interfaces:** - The repository port's authorization-code deletion must become (or be replaced by) a conditional consume whose result distinguishes "I claimed it" from "someone else already did" — e.g. a conditional `DELETE … RETURNING`-style operation, with the zero-row case mapped to `InvalidGrant` in the service. - `redeem_authorization_code` reorders so fallible loads (the user) happen before the claim. - The in-memory mock repository must implement the same consume semantics so service-level tests exercise the contract (issue #63 tracks broader mock divergence; keep this method in sync here regardless). **Acceptance criteria:** - [ ] Redeeming a code that has already been consumed yields `InvalidGrant` (the claim reports zero rows; no second token is minted). - [ ] A repository conformance test covers the consume operation: first consume succeeds, second consume of the same code reports "already consumed". - [ ] When the user lookup fails transiently during redemption, the code is not consumed and a subsequent redemption of the same code succeeds (testable against the mock repository). - [ ] A successful redemption still deletes the code — a replay after success gets `InvalidGrant`. - [ ] The `ci` mise task passes. **Out of scope:** - Access-token issuance, refresh, or revocation semantics (issue #62 covers revocation). - Changing ADR-0008's rules (PKCE, expiry, single-use policy). - The consent or authorization-request half of the flow. - Fixing the broader in-memory mock divergence beyond this consume method (issue #63).

rosa referenced this issue

2026-07-03 20:55:23 +00:00

domain: redeem_authorization_code deletes the code before loading the user, burning it on transient errors #61

rosa added

bug

ready-for-agent

and removed

needs-triage

labels

2026-07-03 20:55:31 +00:00

rosa referenced this issue from a commit

2026-07-03 21:14:20 +00:00