Skip to content

security(oauth): make revocation visible at introspect and revoke - #774

Merged
lakhansamani merged 1 commit into
mainfrom
security/token-revocation-visibility
Aug 15, 2026
Merged

security(oauth): make revocation visible at introspect and revoke#774
lakhansamani merged 1 commit into
mainfrom
security/token-revocation-visibility

Conversation

@lakhansamani

Copy link
Copy Markdown
Contributor

Found by a full-repo security/protocol review. Two defects, both on the
revocation path, both proven by a test that fails without the fix.

The shared cause

The memory-store session entry is this codebase's only revocation record.
Logout, password reset, admin session-wipe and /oauth/revoke all revoke by
deleting it, and validateStatefulAccessToken reads it on every authenticated
request. Neither token endpoint did.

F1 — introspection reported revoked tokens as active

/oauth/introspect validated signature, exp, iss, aud and the user's
RevokedTimestamp — never the session entry. A token ValidateAccessToken
already rejected answered active:true and disclosed sub, scope and aud
with it. Any resource server trusting the endpoint accepted a logged-out or
explicitly revoked token for the rest of its TTL. RFC 7662 §2.2 defines
active as "has not been revoked".

The handler's own doc comment stated the property it broke: "Always returns
{"active": false} for any inactive, invalid, or unknown token."

F2 — revocation silently ignored access tokens

/oauth/revoke accepted token_type_hint=access_token as supported, then only
ever looked up refresh_token_<nonce>. An access token matched nothing and got
a 200 {} with nothing revoked. RFC 7009 §2.2 mandates that 200 either way, so
no client could tell. The hint now orders the lookup rather than restricting it,
per §2.1 ("MAY ignore the hint").

Shape of the fix

One helper (session_lookup.go) read by both endpoints, so the two cannot
drift. id_token is carved out by token type — it is never registered in the
store, so requiring an entry would report every one inactive.

Reviewer notes — things this deliberately does not do

  • Store outage answers inactive. Matches validateStatefulAccessToken.
    subjectLiveness argues the other way for exactly this reason: an
    unreachable DB must not become "not active" and deny the fleet at once.
    Distinguishing absent from unavailable needs GetUserSession to grow a
    known flag across every provider — out of scope, worth a follow-up issue.
  • Resource-bound access tokens stay unrevocable here. The ownership guard
    rejects them before the lookup, unchanged. "Access tokens are now revocable"
    would be a false changelog line.
  • Revoking an access token drops the browser session too.
    DeleteUserSession clears all three entry types for the nonce. Already true
    for refresh tokens; per-type deletion needs a memory-store interface change
    across all six providers.

Test expectations corrected

  • setupIntrospectTest never registered a session, so
    TestIntrospectActiveAccessToken was asserting an unregistered token is
    active. Fixed to mirror the real login flow.
  • TestDeprovisionedUserRevocation deletes sessions before introspecting, so
    after this change its inactive result is satisfied by the session check and
    no longer proves the RevokedTimestamp branch runs. That assertion moves to
    the companion test that keeps the session live.

Verification

Each new test confirmed failing with the source reverted and the tests kept.

go build ./...   OK
go vet ./...     OK
make test        exit 0 — 43 packages, 0 FAIL
make lint        exit 0
make smoke       exit 0

make test-all-db not run: no storage provider touched, memory-store interface
used rather than changed.

The session entry is this codebase's only revocation record — logout,
password reset, admin session-wipe and /oauth/revoke all revoke by
deleting it — and neither token endpoint consulted it.

/oauth/introspect checked signature, exp, iss, aud and the user's
RevokedTimestamp, so a token ValidateAccessToken already rejected still
answered active:true and disclosed sub, scope and aud with it. Any
resource server trusting the endpoint accepted a logged-out token for
the rest of its TTL (RFC 7662 §2.2).

/oauth/revoke accepted token_type_hint=access_token as supported, then
only ever looked up refresh_token_<nonce>; an access token matched
nothing and got a 200 with nothing revoked. RFC 7009 §2.2 mandates that
200 either way, so no client could tell. The hint now orders the lookup
rather than restricting it, per §2.1.

Both endpoints read the entry through one helper so the two cannot
drift, and id_token is carved out by token type: it is never registered
in the store, so requiring an entry would report every one inactive.

Notes for reviewers:
- A store outage now answers inactive, matching
  validateStatefulAccessToken. subjectLiveness argues the other way for
  exactly this reason; distinguishing absent from unavailable needs
  GetUserSession to grow a known-flag across every provider. Follow-up.
- Resource-bound access tokens stay unrevocable here: the ownership
  guard rejects them before the lookup, as before.
- Revoking an access token drops the browser session too —
  DeleteUserSession clears all three entry types for the nonce. Already
  true for refresh tokens; per-type deletion needs an interface change.
- setupIntrospectTest never registered a session, so
  TestIntrospectActiveAccessToken was asserting an unregistered token is
  active. Corrected.
- TestDeprovisionedUserRevocation deletes sessions before introspecting,
  so its inactive result no longer proves the RevokedTimestamp branch
  runs. That assertion moves to the companion test that keeps the
  session live.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant