feat(oidc): security hardening from OIDC review — S256-only PKCE, atomic code redemption, reuse family revocation

Applies the three findings from the 2026-07-11 security review (RFC 9700):

- PKCE accepts S256 only. Authlib's default also allows 'plain', where the
  challenge travels as the cleartext verifier — useless against the
  authorization-request interception PKCE exists for. Discovery no longer
  advertises plain.
- Authorization codes are consumed atomically (single UPDATE ... RETURNING
  on a redeemed flag), closing the TOCTOU race where two concurrent /token
  requests could both redeem one code. Any redemption attempt burns the
  code, including ones that later fail PKCE. Redeemed rows are kept until
  expiry: reuse of a redeemed code signals interception and revokes every
  token issued to that (client, user) grant. Side effect: the nonce-replay
  window now actually spans the code lifetime.
- Replay of a rotated-out refresh token revokes the whole token family, so
  a thief who rotates first no longer keeps a live token while the legit
  client is silently rejected.

Per-client rate-limit buckets (finding 4) deferred — the global in-process
guard stays; noted as follow-up when it matters.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: ClaydeCode

migrations/shard-core-0003-oidc.sql

@@ -21,7 +21,8 @@ CREATE TABLE IF NOT EXISTS oidc_codes (
     code_challenge TEXT,
     code_challenge_method TEXT,
     auth_time BIGINT NOT NULL,
-    expires_at TIMESTAMPTZ NOT NULL
+    expires_at TIMESTAMPTZ NOT NULL,
+    redeemed BOOLEAN NOT NULL DEFAULT FALSE
 );
 
 CREATE TABLE IF NOT EXISTS oidc_tokens (

shard_core/database/oidc.py

@@ -46,22 +46,30 @@ async def insert_code(conn: AsyncConnection, code: dict):
     await conn.execute(sql, code)
 
 
+async def redeem_code(
+    conn: AsyncConnection, code_hash: str, client_id: str
+) -> dict | None:
+    """Atomically consume the code — the single UPDATE makes concurrent
+    redemptions of the same code impossible (only one caller gets the row)."""
+    sql: LiteralString = """UPDATE oidc_codes SET redeemed = TRUE
+        WHERE code_hash = %s AND client_id = %s AND NOT redeemed AND expires_at > now()
+        RETURNING *"""
+    async with conn.cursor(row_factory=dict_row) as cur:
+        await cur.execute(sql, (code_hash, client_id))
+        return await cur.fetchone()
+
+
 async def get_code(
     conn: AsyncConnection, code_hash: str, client_id: str
 ) -> dict | None:
     sql: LiteralString = (
-        "SELECT * FROM oidc_codes WHERE code_hash = %s AND client_id = %s AND expires_at > now()"
+        "SELECT * FROM oidc_codes WHERE code_hash = %s AND client_id = %s"
     )
     async with conn.cursor(row_factory=dict_row) as cur:
         await cur.execute(sql, (code_hash, client_id))
         return await cur.fetchone()
 
 
-async def delete_code(conn: AsyncConnection, code_hash: str):
-    sql: LiteralString = "DELETE FROM oidc_codes WHERE code_hash = %s"
-    await conn.execute(sql, (code_hash,))
-
-
 async def exists_nonce(conn: AsyncConnection, nonce: str, client_id: str) -> bool:
     sql: LiteralString = "SELECT 1 FROM oidc_codes WHERE nonce = %s AND client_id = %s"
     async with conn.cursor() as cur:
@@ -94,9 +102,9 @@ async def get_token_by_access_hash(
 async def get_token_by_refresh_hash(
     conn: AsyncConnection, refresh_token_hash: str
 ) -> dict | None:
-    sql: LiteralString = (
-        "SELECT * FROM oidc_tokens WHERE refresh_token_hash = %s AND NOT revoked"
-    )
+    # revoked rows included on purpose — rotated-token replay must be
+    # distinguishable from an unknown token (reuse detection)
+    sql: LiteralString = "SELECT * FROM oidc_tokens WHERE refresh_token_hash = %s"
     async with conn.cursor(row_factory=dict_row) as cur:
         await cur.execute(sql, (refresh_token_hash,))
         return await cur.fetchone()
@@ -107,3 +115,10 @@ async def revoke_token(conn: AsyncConnection, access_token_hash: str):
         "UPDATE oidc_tokens SET revoked = TRUE WHERE access_token_hash = %s"
     )
     await conn.execute(sql, (access_token_hash,))
+
+
+async def revoke_all_for_grant(conn: AsyncConnection, client_id: str, user_sub: int):
+    sql: LiteralString = (
+        "UPDATE oidc_tokens SET revoked = TRUE WHERE client_id = %s AND user_sub = %s"
+    )
+    await conn.execute(sql, (client_id, user_sub))

shard_core/service/oidc_provider.py

@@ -265,11 +265,31 @@ def save_authorization_code(self, code, request):
         )
 
     def query_authorization_code(self, code, client):
-        row = _run(_with_conn(db_oidc.get_code, hash_secret(code), client.client_id))
+        # atomic burn-on-query: any redemption attempt (even one that later
+        # fails PKCE) consumes the code, and two concurrent requests can't
+        # both get it (RFC 9700 single-use)
+        row = _run(_with_conn(db_oidc.redeem_code, hash_secret(code), client.client_id))
+        if row is None:
+            stale = _run(
+                _with_conn(db_oidc.get_code, hash_secret(code), client.client_id)
+            )
+            if stale and stale["redeemed"]:
+                # reuse of a redeemed code signals interception — kill every
+                # token issued to this (client, user) grant
+                _run(
+                    _with_conn(
+                        db_oidc.revoke_all_for_grant,
+                        client.client_id,
+                        stale["user_sub"],
+                    )
+                )
+            return None
         return AuthorizationCode.from_row(code, row)
 
     def delete_authorization_code(self, authorization_code):
-        _run(_with_conn(db_oidc.delete_code, hash_secret(authorization_code.code)))
+        # already consumed atomically in query_authorization_code; the row is
+        # kept (redeemed) for reuse detection until it expires
+        pass
 
     def authenticate_user(self, authorization_code):
         return _run(_user_from_id_async(authorization_code.user_sub))
@@ -282,7 +302,18 @@ def authenticate_refresh_token(self, refresh_token):
         row = _run(
             _with_conn(db_oidc.get_token_by_refresh_hash, hash_secret(refresh_token))
         )
-        if row and row["issued_at"] + REFRESH_TOKEN_LIFETIME > time.time():
+        if row is None:
+            return None
+        if row["revoked"]:
+            # replay of a rotated-out refresh token — revoke the whole family
+            # so a thief who rotated first doesn't keep a live token
+            _run(
+                _with_conn(
+                    db_oidc.revoke_all_for_grant, row["client_id"], row["user_sub"]
+                )
+            )
+            return None
+        if row["issued_at"] + REFRESH_TOKEN_LIFETIME > time.time():
             return _TokenRecord(row)
         return None
 
@@ -381,6 +412,12 @@ def _query_client(client_id: str):
     return OidcClient.from_row(_run(_with_conn(db_oidc.get_client, client_id)))
 
 
+class S256CodeChallenge(CodeChallenge):
+    # 'plain' (Authlib's default second method) sends challenge == verifier in
+    # cleartext, defeating PKCE's interception protection (RFC 9700 wants S256)
+    SUPPORTED_CODE_CHALLENGE_METHOD = ["S256"]
+
+
 def build_authorization_server(server_cls=AuthorizationServer) -> AuthorizationServer:
     """server_cls lets the web layer pass its framework-adapter subclass."""
     server = server_cls(scopes_supported=SUPPORTED_SCOPES)
@@ -389,7 +426,7 @@ def build_authorization_server(server_cls=AuthorizationServer) -> AuthorizationS
     server.register_token_generator("default", _generate_bearer_token)
     server.register_grant(
         ShardAuthCodeGrant,
-        [CodeChallenge(required=True), ShardOpenIDCode(require_nonce=False)],
+        [S256CodeChallenge(required=True), ShardOpenIDCode(require_nonce=False)],
     )
     server.register_grant(ShardRefreshTokenGrant)
     return server
@@ -460,5 +497,5 @@ def discovery_document() -> dict:
             "client_secret_post",
             "none",
         ],
-        "code_challenge_methods_supported": ["S256", "plain"],
+        "code_challenge_methods_supported": ["S256"],
     }

tests/test_oidc.py

@@ -144,7 +144,7 @@ async def test_discovery_document(app_client: AsyncClient):
     assert "public" in disco["subject_types_supported"]
     assert "RS256" in disco["id_token_signing_alg_values_supported"]
     assert "openid" in disco["scopes_supported"]
-    assert "S256" in disco["code_challenge_methods_supported"]
+    assert disco["code_challenge_methods_supported"] == ["S256"]
     assert {"authorization_code", "refresh_token"} <= set(
         disco["grant_types_supported"]
     )
@@ -313,6 +313,12 @@ async def test_refresh_rotation(app_client: AsyncClient):
     )
     assert r.status_code in (400, 401), r.text
 
+    # ...and its replay is treated as compromise: the whole family dies
+    r = await app_client.get(
+        USERINFO, headers={"Authorization": f"Bearer {new_tok['access_token']}"}
+    )
+    assert r.status_code == 401, "family not revoked after refresh-token reuse"
+
 
 # --- negatives -----------------------------------------------------------------------
 
@@ -408,6 +414,67 @@ async def test_scope_narrowing(app_client: AsyncClient):
     assert {"openid", "profile"} <= granted
 
 
+async def test_plain_pkce_rejected(app_client: AsyncClient):
+    """RFC 9700: only S256 — 'plain' sends the verifier in cleartext."""
+    await pair_new_terminal(app_client)
+    oidc_client = await make_client()
+    verifier = secrets.token_urlsafe(32)
+    params, r = await authorize(
+        app_client,
+        oidc_client,
+        verifier,
+        code_challenge=verifier,
+        code_challenge_method="plain",
+    )
+    if 300 <= r.status_code < 400:
+        query = parse_qs(urlparse(r.headers["location"]).query)
+        assert "code" not in query, "code issued for plain PKCE"
+        assert "error" in query
+    else:
+        assert r.status_code == 400, r.text
+
+
+async def test_failed_redemption_burns_code(app_client: AsyncClient):
+    """Any redemption attempt consumes the code — a PKCE-failing attempt
+    must not leave the code redeemable."""
+    await pair_new_terminal(app_client)
+    oidc_client = await make_client()
+    verifier = secrets.token_urlsafe(32)
+    code = await get_code(app_client, oidc_client, verifier)
+
+    r = await exchange_code(app_client, oidc_client, code, "wrong-" + verifier)
+    assert r.status_code == 400, r.text
+
+    r = await exchange_code(app_client, oidc_client, code, verifier)
+    assert r.status_code == 400, "code survived a failed redemption attempt"
+
+
+async def test_code_reuse_revokes_issued_tokens(app_client: AsyncClient):
+    """Reuse of a redeemed code signals interception — tokens issued to the
+    grant must be revoked (RFC 9700)."""
+    await pair_new_terminal(app_client)
+    oidc_client = await make_client()
+    verifier = secrets.token_urlsafe(32)
+    code = await get_code(app_client, oidc_client, verifier)
+
+    r = await exchange_code(app_client, oidc_client, code, verifier)
+    assert r.status_code == 200, r.text
+    tok = r.json()
+
+    r = await app_client.get(
+        USERINFO, headers={"Authorization": f"Bearer {tok['access_token']}"}
+    )
+    assert r.status_code == 200
+
+    r = await exchange_code(app_client, oidc_client, code, verifier)
+    assert r.status_code == 400, r.text
+
+    r = await app_client.get(
+        USERINFO, headers={"Authorization": f"Bearer {tok['access_token']}"}
+    )
+    assert r.status_code == 401, "tokens not revoked after code reuse"
+
+
 # --- storage hardening -----------------------------------------------------------------