OAuth refresh-token grant — durable claude.ai sessions (Spec: ACE-033) - #88
Conversation
…login) Spec: ACE-033 The server issued a 1h access JWT and nothing else — no refresh token, the token endpoint only accepted authorization_code, and the metadata advertised only that grant. So claude.ai had to redo the full OAuth login every time the access token expired (~hourly), on every self-hosted deploy. - Issue a refresh_token on the authorization_code grant; add the refresh_token grant (RFC 6749 §6) that mints a fresh access JWT and ROTATES the refresh token. - Reuse detection: replaying an already-revoked (rotated/stolen) refresh token revokes the whole family (OAuth 2.1 posture for public clients). - Storage: new oauth_refresh_token table (migration 010), sha256 hash only — never plaintext; bound to username + issuing client_id; revocable; auto-migrated on boot. - Metadata advertises "refresh_token"; access tokens stay short-lived (1h). - Token lifetimes are env-configurable (AGAMI_ACCESS_TOKEN_TTL / AGAMI_REFRESH_TOKEN_TTL, seconds) with defaults when unset (access 1h, refresh 30-day idle) — closes the "baked in, no knob" gap; a bad value fails safe to the default. Tests: 8 assertions in test_oauth_server.py (issue-on-code, rotation+renew, reuse→family revoke, missing/unknown/wrong-client, expiry, hash-not-plaintext storage, metadata, TTL override+fail-safe). Full gate green (1314).
There was a problem hiding this comment.
Pull request overview
Adds OAuth 2.0 refresh-token support to the self-hosted Agami OAuth provider so connected clients (e.g., claude.ai) can silently renew the short-lived access JWT without forcing hourly re-authentication, including rotation and reuse-detection plus new persistence for refresh tokens.
Changes:
- Implement
refresh_tokenissuance onauthorization_codeexchange and addrefresh_tokengrant handling with rotation + family revocation on reuse. - Add
oauth_refresh_tokenpersistence via migrationmigrations/core/010_oauth_refresh.sql(hash-at-rest, indexes) and advertise the new grant in OAuth server metadata. - Make access/refresh token lifetimes env-configurable and add targeted tests + env-example documentation + changelog entry.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_oauth_server.py | Adds refresh-token grant integration tests (issue/rotate/reuse-detect/expiry/hash-at-rest/metadata/TTL override). |
| packages/agami-core/src/oauth_server.py | Implements refresh-token hashing/storage, token endpoint dispatch, refresh grant logic, and env-configurable TTLs. |
| packages/agami-core/src/mcp_http.py | Updates OAuth metadata to advertise refresh_token grant; minor formatting changes. |
| migrations/core/010_oauth_refresh.sql | Adds the oauth_refresh_token table and indexes for rotation lineage and lookup. |
| deploy/agami.env.example | Documents TTL env vars for access/refresh tokens. |
| plugins/agami/skills/agami-deploy/bundle/agami.env.example | Same TTL env var documentation for the deploy bundle template. |
| CHANGELOG.md | Adds an Unreleased entry documenting refresh tokens + rotation/reuse detection + TTL knobs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| seconds = int(raw) | ||
| except ValueError: | ||
| return default | ||
| return timedelta(seconds=seconds) if seconds > 0 else default |
There was a problem hiding this comment.
Fixed in 4572a23 — good catch. _ttl_from_env now also catches OverflowError (an absurdly large positive value would raise when constructing timedelta(seconds=...)), so every bad value fails closed to the default as documented. Simplified the branching, and added the overflow value to the fail-safe test.
| if row["revoked"]: | ||
| # A revoked token being presented == a rotated/stolen token replayed → burn the whole family | ||
| # so a thief and the victim both lose it (one re-login is the accepted cost of theft detection). | ||
| store.execute( | ||
| "UPDATE oauth_refresh_token SET revoked = 1 WHERE family = ?", (row["family"],) | ||
| ) | ||
| store.commit() | ||
| return _oauth_error("invalid_grant", "refresh token has been revoked") | ||
| if _now().isoformat() > row["expires_at"]: |
There was a problem hiding this comment.
This is the intended, documented tradeoff (ACE-033 Decisions), and the two comments aren't actually in conflict — they cover different timings, which I've made explicit in 4572a23. A truly-concurrent double-use where BOTH requests read revoked=0 never reaches the family-revoke branch: both fall through to the atomic UPDATE ... WHERE revoked=0, and the loser gets rowcount=0 → plain invalid_grant, no family kill. The family is only burned when a token is presented that was ALREADY revoked when read — a replay after the rotation committed, or a client that lost the rotation response and retries the old token. That last case costing one re-login is the accepted price of OAuth 2.1 reuse detection (your option (a): accepted + documented). A retry-safe grace window (option (b)) would weaken the theft-detection property the owner chose, so we're keeping strict rotation.
… (Copilot #88) - _ttl_from_env now also catches OverflowError, so an absurdly large positive AGAMI_*_TTL (which would raise when constructing timedelta) falls back to the default instead of crashing token issuance — the fail-safe now holds for ALL bad values. Simplified the branching. Test covers the overflow value. - Sharpened the reuse-detection comment: a truly-concurrent double-use (both read revoked=0) is caught by the rowcount guard with a plain invalid_grant (no family kill); only a replay AFTER a committed rotation (or a lost-response retry) burns the family — the documented ACE-033 tradeoff. No behavior change.
| # Single-use, atomically: only the request that flips used 0→1 may issue a token. The | ||
| # conditional UPDATE + rowcount check closes the read-then-write race two concurrent exchanges | ||
| # would otherwise win together (double-issued tokens for one code). Committed with the refresh | ||
| # insert in _token_response, so code-burn and token issuance are one transaction. | ||
| burned = store.execute( | ||
| "UPDATE oauth_state SET used = 1 WHERE code = ? AND used = 0", (row["code"],) | ||
| ) | ||
| if burned.rowcount != 1: | ||
| store.commit() | ||
| return _oauth_error("invalid_grant", "code is invalid or already used") | ||
| return _token_response( | ||
| store, username=row["username"], client_id=row["client_id"] or "", family=None | ||
| ) |
There was a problem hiding this comment.
Good find — real edge (an authorize can complete without a client_id, so the stored client_id can be blank, and a refresh that sends one would then spuriously mismatch). Fixed in a fast-follow, #89: the bind check is now symmetric — enforced only when BOTH sides present a client_id, skipped when either is blank (the token secret + hash-at-rest are the gate). I went with that over tightening authorize() to require a client_id, since that changes the auth-code flow and could break a client that legitimately authorizes without one. Regression test added. Rolls into v0.3.8.
Summary
Spec: ACE-033 (F2 app-auth). The self-hosted server issued a 1h access JWT and nothing else — no refresh token, the token endpoint only accepted
authorization_code, and the metadata advertised only that grant. So claude.ai had to redo the full OAuth login every hour. This adds the refresh-token grant (RFC 6749 §6) so it silently renews.Changes
refresh_tokenon theauthorization_codegrant; add therefresh_tokengrant — mints a fresh access JWT and rotates the refresh token (revoke presented, issue successor in the same family).oauth_refresh_tokentable (migration010) — sha256 hash only, never plaintext; bound tousername+ issuingclient_id; revocable; auto-migrated on boot (no manual step).["authorization_code", "refresh_token"]; access tokens stay short-lived (1h).AGAMI_ACCESS_TOKEN_TTL/AGAMI_REFRESH_TOKEN_TTL, seconds) with defaults when unset (access 1h, refresh 30-day idle) — closes the "baked-in, no knob" gap; a bad value fails safe to the default. Documented in bothagami.env.examplecopies.Test plan
8 assertions in
tests/test_oauth_server.py, mapping the spec's acceptance criteria: issue-on-code · rotation+renew (new JWT,subpreserved, new≠old refresh) · reuse→family revocation (sibling dies) · missing/unknown/wrong-client →invalid_grant· expiry enforced · hash-not-plaintext storage · metadata · TTL override + fail-safe-to-default. Theauthorization_codepath (single-use, PKCE, expiry, redirect-match, signing precheck) is unchanged — existing OAuth tests pass untouched. Full gate green (1314 passed).Review
Ran
/reviewincl. mandatory security-review (high lane): 0 must-fix. Confirmed rotation atomicity, family-revocation (WHERE family=?, committed), hash-at-rest, check ordering, full parameterization, and grant isolation on both sqlite + postgres; the auth-code path is byte-for-byte preserved. Two review nits (docstring accuracy, one test assertion) folded in.Decisions (from the spec)
client_idbinding lenient on refresh (RFC 6749 §6 doesn't require it for public clients) so claude.ai's refresh can't break on a missing param; the token secret + hash-at-rest are the real gate.Checklist
Ships in v0.3.8; the self-hosted server picks it up via
./deploy.sh(table migrates in automatically).