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

A token a person deletes stays deleted: its OAuth refresh token ends with it #8436

Merged opened this

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

Merged

When a person deletes a token, its OAuth refresh token is now deleted with it. Before this change, revoking a harness on Settings › Tokens deleted only the access token. Because `oauth_refresh.token_id` is `ON DELETE SET NULL`, the unused refresh token could still get a fresh access token from `POST /oauth/token`.

shipped production is serving it deploy 47e6991

opened this

When a person deletes a token, its OAuth refresh token is now deleted with it. Before this change, revoking a harness on Settings › Tokens deleted only the access token. Because oauth_refresh.token_id is ON DELETE SET NULL, the unused refresh token could still get a fresh access token from POST /oauth/token.

What changed

  • src/core/tokens.ts: deleteToken and revokeToken now both go through one private helper, deleteWithRefreshFamily. In one transaction it reads the token's refresh family before the delete (the delete clears token_id), deletes the token, and, only if a token was actually deleted, deletes every oauth_refresh row of that family.

- It is plain SQL, so tokens.ts still does not import oauth.ts and there is no import cycle. - A delete by someone other than the owner does nothing and leaves the family working.

  • Tokens that simply expire are not affected. The expiry sweep pruneTokens does not go through the helper, so an expired access token can still be refreshed. The refresh rotation in oauth.ts deletes the old access token with its own SQL, so a normal refresh does not end its family.
  • Because every place that deletes a token uses these two functions, the fix covers Settings › Tokens, DELETE /api/v1/tokens/:id, the terminal's sign-ins screen, app tokens and run tokens.
  • docs/features/oauth-refresh.md now says that deleting ends the family and that expiry does not.

Tests (in test/oauth-refresh.test.ts)

  • After a code exchange, deleting the access token with deleteToken as its owner makes the refresh grant answer invalid_grant. The same happens with revokeToken, and no oauth_refresh rows of that family are left.
  • A delete by someone else returns false, and the family still refreshes.
  • An access token that expires and is swept by pruneTokens still refreshes, and the new access token works.

Checks

  • With the source change reverted, the new test fails; with it, all 15 tests in the file pass.
  • bun scripts/gate.ts and bun x tsc --noEmit are clean.
  • All 47 token, OAuth and sign-in test files pass (321 tests).
  • scripts/fast.ts named 1,342 test files, more than its 5-minute budget, so I ran only those token, OAuth and sign-in files rather than its list.

Not done: the ON DELETE SET NULL in the migration is unchanged. A cascade would also end the family whenever an access token simply expires, which the issue says must not happen.

To try it: get a token with a lifetime through the code exchange, delete it on Settings › Tokens, then send POST /oauth/token with grant_type=refresh_token. It answers invalid_grant.

🤖 Generated with Claude Code

Closes #8428

commented

When a person deletes a token, its OAuth refresh token is now deleted with it. Before this change, revoking a harness on Settings › Tokens deleted only the access token. Because oauth_refresh.token_id is ON DELETE SET NULL, the unused refresh token could still get a fresh access token from POST /oauth/token.

What changed

  • src/core/tokens.ts: deleteToken and revokeToken now both go through one private helper, deleteWithRefreshFamily. In one transaction it reads the token's refresh family before the delete (the delete clears token_id), deletes the token, and, only if a token was actually deleted, deletes every oauth_refresh row of that family.

- It is plain SQL, so tokens.ts still does not import oauth.ts and there is no import cycle. - A delete by someone other than the owner does nothing and leaves the family working.

  • Tokens that simply expire are not affected. The expiry sweep pruneTokens does not go through the helper, so an expired access token can still be refreshed. The refresh rotation in oauth.ts deletes the old access token with its own SQL, so a normal refresh does not end its family.
  • Because every place that deletes a token uses these two functions, the fix covers Settings › Tokens, DELETE /api/v1/tokens/:id, the terminal's sign-ins screen, app tokens and run tokens.
  • docs/features/oauth-refresh.md now says that deleting ends the family and that expiry does not.

Tests (in test/oauth-refresh.test.ts)

  • After a code exchange, deleting the access token with deleteToken as its owner makes the refresh grant answer invalid_grant. The same happens with revokeToken, and no oauth_refresh rows of that family are left.
  • A delete by someone else returns false, and the family still refreshes.
  • An access token that expires and is swept by pruneTokens still refreshes, and the new access token works.

Checks

  • With the source change reverted, the new test fails; with it, all 15 tests in the file pass.
  • bun scripts/gate.ts and bun x tsc --noEmit are clean.
  • All 47 token, OAuth and sign-in test files pass (321 tests).
  • scripts/fast.ts named 1,342 test files, more than its 5-minute budget, so I ran only those token, OAuth and sign-in files rather than its list.

Not done: the ON DELETE SET NULL in the migration is unchanged. A cascade would also end the family whenever an access token simply expires, which the issue says must not happen.

To try it: get a token with a lifetime through the code exchange, delete it on Settings › Tokens, then send POST /oauth/token with grant_type=refresh_token. It answers invalid_grant.

🤖 Generated with Claude Code

Approved

Clean security fix. deleteToken and revokeToken share one helper. It reads the family before the delete, runs in a transaction, and only ends the family when a row was actually deleted, so a non-owner delete does nothing. Expiry through pruneTokens is untouched, as the issue requires. The SQL is parameterised and there is no new dependency or import cycle. The tests cover owner delete, revoke, non-owner delete and expiry, and the how-to is in docs/features/oauth-refresh.md, not README. I did not fetch the preview. Nothing to change.

commented

Left #8428 open: 3 unticked items remain (Deleting a token through deleteToken or revokeToken also deletes every oauth_refresh row of that token's family (the same work endFamily in src/core/oauth.ts does); one helper, called from both.; An access token that merely expires (the sweep at src/core/tokens.ts:174) does NOT end its family: refreshing after expiry keeps working.; Tests (test/oauth-refresh.test.ts): mint through the code exchange, delete the access token as its owner, then the refresh grant answers invalid_grant; an expired access token still refreshes.).

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs