From 728da28bf8c1c69ce54f7b2fed66fadcfc042ccb Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Sat, 15 Aug 2026 15:13:17 +0530 Subject: [PATCH] security(oauth): make revocation visible at introspect and revoke MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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_; 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. --- internal/http_handlers/introspect.go | 46 ++++++ .../http_handlers/revoke_refresh_token.go | 26 +++- internal/http_handlers/session_lookup.go | 52 +++++++ .../oauth_revoke_token_type_test.go | 137 ++++++++++++++++++ .../integration_tests/oidc_introspect_test.go | 77 +++++++++- .../scim_deprovision_test.go | 25 ++++ 6 files changed, 357 insertions(+), 6 deletions(-) create mode 100644 internal/http_handlers/session_lookup.go create mode 100644 internal/integration_tests/oauth_revoke_token_type_test.go diff --git a/internal/http_handlers/introspect.go b/internal/http_handlers/introspect.go index d68bcbeb8..7884b4bf8 100644 --- a/internal/http_handlers/introspect.go +++ b/internal/http_handlers/introspect.go @@ -8,6 +8,7 @@ import ( "github.com/gin-gonic/gin" + "github.com/authorizerdev/authorizer/internal/constants" "github.com/authorizerdev/authorizer/internal/parsers" "github.com/authorizerdev/authorizer/internal/service/clientauth" ) @@ -110,6 +111,21 @@ func (h *httpProvider) IntrospectHandler() gin.HandlerFunc { return } + // RFC 7662 §2.2: "active" means the token "has not been revoked". In this + // codebase revocation IS the deletion of the memory-store session entry — + // logout, password reset, admin session-wipe and /oauth/revoke all work + // that way — so that entry is what has to be consulted. Signature, exp, + // iss and aud cannot see any of it, which is why a logged-out token used + // to introspect as active (with its sub, scope and aud disclosed) for the + // remainder of its TTL, and why a resource server trusting this endpoint + // kept accepting it. + // + // Placed before the user lookup below so a revoked token costs no DB read. + if !h.tokenSessionIsLive(claims, tokenValue) { + gc.JSON(http.StatusOK, gin.H{"active": false}) + return + } + // Revocation awareness: for a first-party user token (sub == user id), a // revoked/deprovisioned user's token must introspect as inactive even // before the short access-token TTL elapses (SCIM active:false, account @@ -187,6 +203,36 @@ func respondResourceClientAuthError(gc *gin.Context, err error, hasBasicAuth boo }) } +// tokenSessionIsLive reports whether the session entry backing this token still +// exists and still holds it. See sessionEntryMatches: that entry is the +// revocation record, which is why exp/iss/aud alone could never see a logout. +// +// Only STATEFUL token types are checked. An id_token is never registered in the +// store — it is an assertion, not a credential, and nothing revokes one — so +// requiring an entry would make every id_token introspect as inactive. +// TestIntrospectActiveIDToken guards that. +// +// An RFC 8693 delegated token needs no special case here. It is stateless and +// carries no `nonce`, so it falls on the guard below and reports inactive, which +// is the documented contract (see CreateDelegatedAccessToken: "NOT via +// /oauth/introspect"). It also never reaches this far in practice — its `aud` is +// a resource URI, which the audience check above already rejects. +func (h *httpProvider) tokenSessionIsLive(claims map[string]interface{}, presented string) bool { + tokenType, _ := claims["token_type"].(string) + if tokenType != constants.TokenTypeAccessToken && tokenType != constants.TokenTypeRefreshToken { + return true + } + nonce, _ := claims["nonce"].(string) + sessionKey := claimsSessionKey(claims) + if nonce == "" || sessionKey == "" { + // Every stateful token this server mints carries both, so something that + // cannot be checked must not report active. Same rule as + // validateStatefulAccessToken's `nonce == ""` guard. + return false + } + return h.sessionEntryMatches(sessionKey, tokenType+"_"+nonce, presented) +} + // audienceMatchesIntrospect accepts either a string aud or a []interface{} // aud claim and returns true if it contains the expected client ID. func audienceMatchesIntrospect(audClaim interface{}, expected string) bool { diff --git a/internal/http_handlers/revoke_refresh_token.go b/internal/http_handlers/revoke_refresh_token.go index 72b7b5b6d..815954bb2 100644 --- a/internal/http_handlers/revoke_refresh_token.go +++ b/internal/http_handlers/revoke_refresh_token.go @@ -6,7 +6,6 @@ import ( "github.com/authorizerdev/authorizer/internal/audit" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/metrics" "github.com/authorizerdev/authorizer/internal/service/clientauth" "github.com/authorizerdev/authorizer/internal/utils" @@ -143,10 +142,27 @@ func (h *httpProvider) RevokeRefreshTokenHandler() gin.HandlerFunc { return } - existingToken, err := h.MemoryStoreProvider.GetUserSession(sessionToken, constants.TokenTypeRefreshToken+"_"+nonce) - // RFC 7009 §2.1: use constant-time comparison to prevent timing attacks - // Dual-read against the stored digest — see crypto.VerifySessionValue. - if err != nil || !crypto.VerifySessionValue(tokenValue, existingToken) { + // RFC 7009 §2.1: the hint is an optimisation, not a filter — the server + // "MAY ignore" it and MUST still find the token when the hint is wrong or + // absent. Only the refresh entry used to be consulted, so an access token + // presented here matched nothing and the handler answered 200 having + // revoked precisely nothing; §2.2 mandates that 200 either way, so the + // caller could not tell. Try the hinted type first, then the other. + // + // sessionEntryMatches keeps the constant-time, digest-dual-read comparison + // this line always did — see internal/http_handlers/session_lookup.go. + candidates := []string{constants.TokenTypeRefreshToken, constants.TokenTypeAccessToken} + if tokenTypeHint == constants.TokenTypeAccessToken { + candidates = []string{constants.TokenTypeAccessToken, constants.TokenTypeRefreshToken} + } + matched := false + for _, tokenType := range candidates { + if h.sessionEntryMatches(sessionToken, tokenType+"_"+nonce, tokenValue) { + matched = true + break + } + } + if !matched { // RFC 7009 §2.2: Token not found or mismatch - return 200 log.Debug().Msg("Token not found or mismatch, returning 200 per RFC 7009") gc.JSON(http.StatusOK, gin.H{}) diff --git a/internal/http_handlers/session_lookup.go b/internal/http_handlers/session_lookup.go new file mode 100644 index 000000000..965ae8522 --- /dev/null +++ b/internal/http_handlers/session_lookup.go @@ -0,0 +1,52 @@ +package http_handlers + +import ( + "github.com/authorizerdev/authorizer/internal/crypto" +) + +// The memory-store session entry IS the revocation record in this codebase. +// Logout, password reset, admin session-wipe and /oauth/revoke all revoke by +// deleting it, and nothing else records revocation anywhere. Every surface that +// needs to answer "is this token still live?" therefore has to read it: +// token.validateStatefulAccessToken does so on every authenticated request, and +// the two token-facing endpoints in this package do so through the helpers here. +// +// They exist as one definition rather than two inline copies because +// /oauth/introspect and /oauth/revoke now ask exactly the same question, and a +// second copy is a second place for the answer to drift. + +// sessionEntryMatches reports whether the memory store still holds this exact +// token under the given entry key. +// +// The VALUE is compared, not merely the entry's existence, to stay identical to +// token.validateStatefulAccessToken — one definition of "this token is the live +// one", not two. +// +// Note it does NOT demote a merely superseded token: the refresh grant mints a +// fresh nonce and leaves the previous nonce's entries alone, so a still-unexpired +// predecessor legitimately keeps its own live entry until it expires. +func (h *httpProvider) sessionEntryMatches(sessionKey, entryKey, presented string) bool { + stored, err := h.MemoryStoreProvider.GetUserSession(sessionKey, entryKey) + if err != nil { + return false + } + // Dual-read against the stored digest — see crypto.VerifySessionValue. The + // client_credentials path writes the raw token rather than a digest, which + // that function's legacy branch handles. + return crypto.VerifySessionValue(presented, stored) +} + +// claimsSessionKey derives the memory-store session key a token addresses. +// Mirrors token.validateStatefulAccessToken and RevokeRefreshTokenHandler: the +// shape is ":", or a bare "" when the token carries no +// login_method. +func claimsSessionKey(claims map[string]interface{}) string { + sub, _ := claims["sub"].(string) + if sub == "" { + return "" + } + if lm, _ := claims["login_method"].(string); lm != "" { + return lm + ":" + sub + } + return sub +} diff --git a/internal/integration_tests/oauth_revoke_token_type_test.go b/internal/integration_tests/oauth_revoke_token_type_test.go new file mode 100644 index 000000000..bc3be6791 --- /dev/null +++ b/internal/integration_tests/oauth_revoke_token_type_test.go @@ -0,0 +1,137 @@ +package integration_tests + +import ( + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/authorizerdev/authorizer/internal/constants" +) + +// RFC 7009 §2.1 makes token_type_hint an OPTIMISATION, not a filter: the server +// "MAY ignore" it and MUST still find the token when the hint is wrong or absent. +// +// /oauth/revoke used to consult only the refresh entry, while separately +// accepting token_type_hint=access_token as a supported hint. An access token +// presented there matched nothing and the handler returned 200 having revoked +// nothing at all — and because §2.2 mandates that 200 either way, no client could +// tell. These tests pin both halves of the rule: the hinted type is found, and a +// wrong hint does not hide the other one. + +// postRevoke drives the real /oauth/revoke handler. +func postRevoke(t *testing.T, ts *testSetup, tokenValue, hint string) *httptest.ResponseRecorder { + t.Helper() + router := gin.New() + router.POST("/oauth/revoke", ts.HttpProvider.RevokeRefreshTokenHandler()) + + form := url.Values{} + form.Set("token", tokenValue) + form.Set("client_id", ts.Config.ClientID) + if hint != "" { + form.Set("token_type_hint", hint) + } + + w := httptest.NewRecorder() + req, _ := http.NewRequest(http.MethodPost, "/oauth/revoke", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + // Host must match the iss claim baked into tokens at creation time. + req.Host = "localhost" + router.ServeHTTP(w, req) + return w +} + +// accessTokenStillValidates reports whether the token is still accepted by +// Authorizer's own request-serving auth — the observable definition of "was it +// really revoked?". +func accessTokenStillValidates(t *testing.T, ts *testSetup, accessToken string) bool { + t.Helper() + req, _ := http.NewRequest(http.MethodGet, "/userinfo", nil) + req.Host = "localhost" + _, err := ts.TokenProvider.ValidateAccessToken(&gin.Context{Request: req}, accessToken) + return err == nil +} + +// TestRevokeAccessTokenInvalidatesSession is the regression test: an access token +// presented at /oauth/revoke must actually be revoked, not silently ignored. +func TestRevokeAccessTokenInvalidatesSession(t *testing.T) { + ts, _, authToken := setupIntrospectTest(t) + + require.True(t, accessTokenStillValidates(t, ts, authToken.AccessToken.Token), + "baseline: the access token must validate before revocation") + + w := postRevoke(t, ts, authToken.AccessToken.Token, constants.TokenTypeAccessToken) + require.Equal(t, http.StatusOK, w.Code, "RFC 7009 §2.2: revocation always answers 200") + + assert.False(t, accessTokenStillValidates(t, ts, authToken.AccessToken.Token), + "an access token presented at /oauth/revoke MUST be revoked, not silently ignored") +} + +// TestRevokeAccessTokenWithoutHintIsFound covers the same path with no hint at +// all, which RFC 7009 §2.1 permits and which is what a client that does not know +// the token's type will send. +func TestRevokeAccessTokenWithoutHintIsFound(t *testing.T) { + ts, _, authToken := setupIntrospectTest(t) + + w := postRevoke(t, ts, authToken.AccessToken.Token, "") + require.Equal(t, http.StatusOK, w.Code) + + assert.False(t, accessTokenStillValidates(t, ts, authToken.AccessToken.Token), + "an access token MUST be found even when no token_type_hint is supplied") +} + +// TestRevokeRefreshTokenWithWrongHintStillRevokes pins the half of RFC 7009 §2.1 +// that a naive "switch on the hint" fix would break: the hint only orders the +// lookup, it never restricts it. +func TestRevokeRefreshTokenWithWrongHintStillRevokes(t *testing.T) { + ts, _, authToken := setupIntrospectTest(t) + + claims, err := ts.TokenProvider.ParseJWTToken(authToken.AccessToken.Token) + require.NoError(t, err) + userID, _ := claims["sub"].(string) + require.NotEmpty(t, userID) + + // setupIntrospectTest asks for no offline_access, so it registers only the + // session and access entries. Register a refresh entry under the same nonce, + // as a login with offline_access would. + refreshValue := "refresh-token-value-" + userID + require.NoError(t, ts.MemoryStoreProvider.SetUserSession( + introspectSessionKey(userID), + constants.TokenTypeRefreshToken+"_"+authToken.FingerPrint, + refreshValue, + authToken.AccessToken.ExpiresAt, + )) + + // A refresh token presented with the WRONG hint must still be found. The + // handler parses the presented value as a JWT before the lookup, so present + // the access token's own JWT while asserting on the refresh entry being the + // one the loop can reach: the access candidate is tried FIRST under this + // hint, so a match here proves the loop does not stop at a miss. + w := postRevoke(t, ts, authToken.AccessToken.Token, constants.TokenTypeRefreshToken) + require.Equal(t, http.StatusOK, w.Code) + + assert.False(t, accessTokenStillValidates(t, ts, authToken.AccessToken.Token), + "a wrong token_type_hint MUST NOT prevent the token from being found") +} + +// TestRevokeUnregisteredTokenIsNoop guards the negative half: a syntactically +// valid, correctly-audienced token that was never registered (or was already +// revoked) still answers 200 and revokes nothing. +func TestRevokeUnregisteredTokenIsNoop(t *testing.T) { + ts, _, authToken := setupIntrospectTest(t) + + claims, err := ts.TokenProvider.ParseJWTToken(authToken.AccessToken.Token) + require.NoError(t, err) + userID, _ := claims["sub"].(string) + + // Drop the session first, so the token is genuinely unregistered. + require.NoError(t, ts.MemoryStoreProvider.DeleteUserSession(introspectSessionKey(userID), authToken.FingerPrint)) + + w := postRevoke(t, ts, authToken.AccessToken.Token, "") + assert.Equal(t, http.StatusOK, w.Code, "RFC 7009 §2.2: an unknown token still answers 200") +} diff --git a/internal/integration_tests/oidc_introspect_test.go b/internal/integration_tests/oidc_introspect_test.go index 3d0f3df18..1c9ef1177 100644 --- a/internal/integration_tests/oidc_introspect_test.go +++ b/internal/integration_tests/oidc_introspect_test.go @@ -13,6 +13,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/authorizerdev/authorizer/internal/constants" "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/authorizerdev/authorizer/internal/token" @@ -45,14 +46,39 @@ func setupIntrospectTest(t *testing.T) (*testSetup, string, *token.AuthToken) { User: user, Roles: []string{"user"}, Scope: []string{"openid", "profile", "email"}, - LoginMethod: "basic_auth", + LoginMethod: constants.AuthRecipeMethodBasicAuth, Nonce: "nonce-" + uuid.New().String(), HostName: "http://localhost", }) require.NoError(t, err) + + // Mirror the real login flow: persist the session + access token in the + // memory store. Introspection reports a token whose session entry is gone as + // inactive (RFC 7662 §2.2), so a token minted without one is a REVOKED token, + // not a live one — this helper used to hand every test that shape and then + // assert it was active. + require.NoError(t, ts.MemoryStoreProvider.SetUserSession( + introspectSessionKey(user.ID), + constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, + authToken.FingerPrintHash, + authToken.SessionTokenExpiresAt, + )) + require.NoError(t, ts.MemoryStoreProvider.SetUserSession( + introspectSessionKey(user.ID), + constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, + authToken.AccessToken.Token, + authToken.AccessToken.ExpiresAt, + )) + return ts, email, authToken } +// introspectSessionKey is the memory-store session key for a basic_auth login, +// matching what token.validateStatefulAccessToken derives from the claims. +func introspectSessionKey(userID string) string { + return constants.AuthRecipeMethodBasicAuth + ":" + userID +} + func postIntrospect(t *testing.T, ts *testSetup, form string, basicAuth ...string) *httptest.ResponseRecorder { t.Helper() router := gin.New() @@ -90,6 +116,12 @@ func TestIntrospectActiveAccessToken(t *testing.T) { assert.Equal(t, cfg.ClientID, body["aud"], "active response MUST include aud") } +// TestIntrospectActiveIDToken is the guard on introspection's id_token carve-out, +// not merely a happy-path case. An id_token is never registered in the memory +// store — it is an assertion, not a credential, and nothing revokes one — so the +// session-liveness check in tokenSessionIsLive skips it by token type. Widen that +// check to every token type and this test is what fails; do not delete it as a +// duplicate of TestIntrospectActiveAccessToken. func TestIntrospectActiveIDToken(t *testing.T) { ts, _, authToken := setupIntrospectTest(t) cfg := ts.Config @@ -102,6 +134,49 @@ func TestIntrospectActiveIDToken(t *testing.T) { assert.Equal(t, true, body["active"]) } +// TestIntrospectRevokedSessionIsInactive is the regression test for the RFC 7662 +// §2.2 gap: introspection validated signature, exp, iss, aud and the user's +// RevokedTimestamp, but never the memory-store session entry — which is the only +// place revocation is recorded. Logout, password reset, admin session-wipe and +// /oauth/revoke all revoke by deleting it, so a token Authorizer's own API +// rejected still introspected as active, disclosing sub/scope/aud to any resource +// server that trusted the answer. +func TestIntrospectRevokedSessionIsInactive(t *testing.T) { + ts, _, authToken := setupIntrospectTest(t) + cfg := ts.Config + + claims, err := ts.TokenProvider.ParseJWTToken(authToken.AccessToken.Token) + require.NoError(t, err) + userID, _ := claims["sub"].(string) + require.NotEmpty(t, userID) + + form := "token=" + authToken.AccessToken.Token + "&client_id=" + cfg.ClientID + "&client_secret=" + cfg.ClientSecret + + w := postIntrospect(t, ts, form) + require.Equal(t, http.StatusOK, w.Code) + var before map[string]interface{} + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &before)) + require.Equal(t, true, before["active"], "baseline: a live token must introspect as active") + + // Revoke exactly the way logout and RevokeRefreshTokenHandler do. + require.NoError(t, ts.MemoryStoreProvider.DeleteUserSession(introspectSessionKey(userID), authToken.FingerPrint)) + + // The token is now dead at Authorizer's own API surface... + vReq, _ := http.NewRequest(http.MethodGet, "/userinfo", nil) + vReq.Host = "localhost" + _, vErr := ts.TokenProvider.ValidateAccessToken(&gin.Context{Request: vReq}, authToken.AccessToken.Token) + require.Error(t, vErr, "baseline: the revoked token must no longer validate") + + // ...so introspection must not report it live either. + w = postIntrospect(t, ts, form) + require.Equal(t, http.StatusOK, w.Code) + var after map[string]interface{} + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &after)) + assert.Equal(t, false, after["active"], "a revoked token MUST introspect as inactive (RFC 7662 §2.2)") + assert.Nil(t, after["sub"], "inactive response MUST NOT leak sub") + assert.Nil(t, after["scope"], "inactive response MUST NOT leak scope") +} + func TestIntrospectInactiveReturnsOnlyActiveFalse(t *testing.T) { cfg := getTestConfig() ts := initTestSetup(t, cfg) diff --git a/internal/integration_tests/scim_deprovision_test.go b/internal/integration_tests/scim_deprovision_test.go index 5694eab1f..ea3123891 100644 --- a/internal/integration_tests/scim_deprovision_test.go +++ b/internal/integration_tests/scim_deprovision_test.go @@ -152,4 +152,29 @@ func TestDeprovisionedUserRevocation_AccessTokenBlockedEvenIfSessionDeleteMissed // The still-live session-store entry alone must not be enough: the same // access token must now be rejected purely on the RevokedTimestamp check. assert.Error(t, callProfile(), "a revoked user's access token must stop authenticating requests immediately") + + // Introspection's RevokedTimestamp branch is asserted HERE, not in + // TestDeprovisionedUserRevocation above, and the reason is the session check + // introspection now performs (RFC 7662 §2.2, see tokenSessionIsLive). That + // test calls DeleteAllUserSessions before introspecting, so its "inactive" + // result is satisfied by the session check alone and no longer proves the + // RevokedTimestamp branch runs at all. This test deliberately leaves the + // session entry live, so the only thing that can produce inactive here is + // RevokedTimestamp — which keeps that branch covered. + router := gin.New() + router.POST("/oauth/introspect", ts.HttpProvider.IntrospectHandler()) + form := url.Values{} + form.Set("token", *signupRes.AccessToken) + form.Set("client_id", cfg.ClientID) + form.Set("client_secret", cfg.ClientSecret) + w := httptest.NewRecorder() + introspectReq, _ := http.NewRequest(http.MethodPost, "/oauth/introspect", strings.NewReader(form.Encode())) + introspectReq.Header.Set("Content-Type", "application/x-www-form-urlencoded") + introspectReq.Header.Set("X-Authorizer-URL", "http://"+ts.HttpServer.Listener.Addr().String()) + router.ServeHTTP(w, introspectReq) + require.Equal(t, http.StatusOK, w.Code) + var body map[string]interface{} + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + assert.Equal(t, false, body["active"], + "a revoked user's token must introspect as inactive on RevokedTimestamp alone, with its session still live") }