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

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 {

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{})

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 "<login_method>:<sub>", or a bare "<sub>" 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
+}

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")
+}