From 82d9f0384c71cb429244ae17878eae587010cb24 Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Tue, 21 Jul 2026 13:10:16 +0530 Subject: [PATCH 1/4] security(oauth): OAuth 2.1 / MCP hardening MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Refresh-token reuse response (OAuth 2.1 §6.1 / RFC 9700 §4.14.2): a genuine, already-rotated refresh token replayed is now detected and revokes the whole live session family, not a no-op invalid_grant. Rotation itself already retired the old token; this adds the breach response. Reuse is only flagged for a signed, client-bound refresh token whose session record is gone, so a forged/foreign token can't force a revocation. - RFC 8707 resource indicator on the authorization_code flow: optional single `resource` at /authorize is bound to the code, must be echoed and matched at /oauth/token (invalid_grant on mismatch), and sets the access token `aud`. id_token / refresh_token audiences stay the client. - RFC 8414: /.well-known/oauth-authorization-server alias of the OIDC discovery handler; advertise resource_indicators_supported. - --oauth21-strict flag (default false, opt-in). When on, reject the implicit/hybrid-with-token response types and PKCE plain (S256 only), reflected in code_challenge_methods_supported. Off is unchanged. Refs #516 --- cmd/root.go | 1 + internal/config/config.go | 6 + internal/http_handlers/authorize.go | 43 +- internal/http_handlers/openid_config.go | 14 +- internal/http_handlers/token.go | 58 ++- .../integration_tests/oauth21_mcp_test.go | 371 ++++++++++++++++++ internal/server/http_routes.go | 4 + internal/service/auth_response.go | 7 +- internal/service/login.go | 7 +- internal/service/session.go | 7 +- internal/service/signup.go | 7 +- internal/token/auth_token.go | 51 ++- 12 files changed, 566 insertions(+), 10 deletions(-) create mode 100644 internal/integration_tests/oauth21_mcp_test.go diff --git a/cmd/root.go b/cmd/root.go index 862b247b..5e205aa8 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -133,6 +133,7 @@ func init() { f.StringVar(&rootArgs.config.ClientSecret, "client-secret", "", "Client secret for the OAuth") f.StringVar(&rootArgs.config.DefaultAuthorizeResponseMode, "default-authorize-response-mode", constants.ResponseModeQuery, "Default response mode for the authorize endpoint") f.StringVar(&rootArgs.config.DefaultAuthorizeResponseType, "default-authorize-response-type", constants.ResponseTypeToken, "Default response type for the authorize endpoint") + f.BoolVar(&rootArgs.config.OAuth21Strict, "oauth21-strict", false, "Enforce OAuth 2.1 restrictions: reject the implicit/hybrid-with-token response types (token, id_token token) and PKCE plain (require S256). Breaking; opt-in") // Admin flags f.StringVar(&rootArgs.config.AdminSecret, "admin-secret", "", "Secret for the admin (REQUIRED, must not be empty)") diff --git a/internal/config/config.go b/internal/config/config.go index f018ab57..439b0dd5 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -249,6 +249,12 @@ type Config struct { DefaultAuthorizeResponseMode string // Default Authorize response type DefaultAuthorizeResponseType string + // OAuth21Strict opts into OAuth 2.1 breaking restrictions (default false). + // When true the authorization endpoint rejects the implicit/hybrid-with-token + // response types (response_type=token and id_token token) and the PKCE + // "plain" code_challenge_method (S256 required). Off keeps all current + // behavior unchanged. + OAuth21Strict bool // Twilio Configurations // TwilioAPISecret is the API secret for Twilio diff --git a/internal/http_handlers/authorize.go b/internal/http_handlers/authorize.go index d81bf825..3b1663d6 100644 --- a/internal/http_handlers/authorize.go +++ b/internal/http_handlers/authorize.go @@ -95,6 +95,14 @@ func (h *httpProvider) AuthorizeHandler() gin.HandlerFunc { nonce := param("nonce") screenHint := param("screen_hint") + // RFC 8707 resource indicator: optional. When present it binds the + // issued authorization code (and, at exchange time, the access token's + // `aud`) to the target resource server. RFC 8707 §2 permits multiple + // resource values, but this flow issues a single-audience access token + // (matching token_exchange.go), so exactly one is allowed — reject a + // repeated parameter rather than silently picking one. + resource := param("resource") + // OIDC Core §3.1.2.1 standard authorization request parameters. loginHint := param("login_hint") uiLocales := param("ui_locales") @@ -175,6 +183,13 @@ func (h *httpProvider) AuthorizeHandler() gin.HandlerFunc { } } + // RFC 8707: reject a repeated resource parameter (single audience only). + // redirect_uri is validated above, so redirectErrorToRP is safe here. + if vals := gc.Request.Form["resource"]; len(vals) > 1 { + redirectErrorToRP(gc, responseMode, redirectURI, state, "invalid_request", "only one resource parameter is supported") + return + } + // OIDCC-3.1.2.6 (JAR, RFC 9101): the server must either process a // `request`/`request_uri` object or reject it with // request_not_supported / request_uri_not_supported. We don't parse @@ -221,6 +236,14 @@ func (h *httpProvider) AuthorizeHandler() gin.HandlerFunc { }) return } + // OAuth 2.1 strict mode: PKCE "plain" is removed — S256 only. + if h.Config.OAuth21Strict && codeChallengeMethod == "plain" && codeChallenge != "" { + gc.JSON(http.StatusBadRequest, gin.H{ + "error": "invalid_request", + "error_description": "code_challenge_method=plain is not allowed; use S256", + }) + return + } canonical, ok := supportedResponseTypeSet(responseType) if !ok { @@ -260,6 +283,16 @@ func (h *httpProvider) AuthorizeHandler() gin.HandlerFunc { } } + // OAuth 2.1 strict mode: the implicit grant is removed. Reject the + // response types that return an access token directly from the + // authorization endpoint (response_type=token and id_token token). + // Pure id_token and the code-based hybrids are left untouched — only + // the access-token-in-the-fragment cases are dropped. + if h.Config.OAuth21Strict && (responseType == constants.ResponseTypeToken || responseType == "id_token token") { + redirectErrorToRP(gc, responseMode, redirectURI, state, "unsupported_response_type", "response_type="+responseType+" is not supported") + return + } + if errCode, errDesc := h.validateAuthorizeRequest(responseType, responseMode, state); errCode != "" { log.Debug().Str("error", errCode).Str("error_description", errDesc).Msg("Invalid request") gc.JSON(http.StatusBadRequest, gin.H{ @@ -332,7 +365,10 @@ func (h *httpProvider) AuthorizeHandler() gin.HandlerFunc { if codeChallenge != "" { challengeData = codeChallenge + "::" + codeChallengeMethod } - if err := h.MemoryStoreProvider.SetState(state, code+"@@"+challengeData+"@@"+nonce+"@@"+url.QueryEscape(redirectURI)); err != nil { + // [4] carries the RFC 8707 resource (url-escaped, empty when absent) + // so the login/signup/session/auth_response services can rebind it + // to the code state they persist after a fresh login. + if err := h.MemoryStoreProvider.SetState(state, code+"@@"+challengeData+"@@"+nonce+"@@"+url.QueryEscape(redirectURI)+"@@"+url.QueryEscape(resource)); err != nil { log.Debug().Err(err).Msg("Error setting temp code") gc.JSON(http.StatusInternalServerError, gin.H{"error": "internal server error"}) return @@ -715,7 +751,10 @@ func (h *httpProvider) AuthorizeHandler() gin.HandlerFunc { if codeChallenge != "" { codeChallengeData = codeChallenge + "::" + codeChallengeMethod } - if err := h.MemoryStoreProvider.SetState(code, codeChallengeData+"@@"+newSessionToken+"@@"+nonce+"@@"+url.QueryEscape(redirectURI)); err != nil { + // [4] binds the RFC 8707 resource to the code (url-escaped, empty + // when absent); the token endpoint enforces the echoed resource + // matches this and sets the access token `aud` to it. + if err := h.MemoryStoreProvider.SetState(code, codeChallengeData+"@@"+newSessionToken+"@@"+nonce+"@@"+url.QueryEscape(redirectURI)+"@@"+url.QueryEscape(resource)); err != nil { log.Debug().Err(err).Msg("Error setting temp code") handleResponse(gc, responseMode, authURL, redirectURI, loginError, http.StatusOK) return diff --git a/internal/http_handlers/openid_config.go b/internal/http_handlers/openid_config.go index 29263d34..c0328903 100644 --- a/internal/http_handlers/openid_config.go +++ b/internal/http_handlers/openid_config.go @@ -36,6 +36,13 @@ func (h *httpProvider) OpenIDConfigurationHandler() gin.HandlerFunc { // configured a backchannel_logout_uri — avoids lying to RPs. backchannelSupported := h.Config.BackchannelLogoutURI != "" + // OAuth 2.1 strict mode drops PKCE "plain" (S256 only). RFC 7636 §4.2 + // otherwise requires advertising both. + codeChallengeMethods := []string{"S256", "plain"} + if h.Config.OAuth21Strict { + codeChallengeMethods = []string{"S256"} + } + resp := gin.H{ // REQUIRED fields (OIDC Discovery §3) "issuer": issuer, @@ -62,8 +69,11 @@ func (h *httpProvider) OpenIDConfigurationHandler() gin.HandlerFunc { // code_verifier (PKCE) or client_secret — "none" without PKCE is // rejected with invalid_request. "private_key_jwt" advertises the // RFC 7523 client_assertion (JWT-bearer) workload-identity path. - "token_endpoint_auth_methods_supported": []string{"client_secret_basic", "client_secret_post", "none", "private_key_jwt"}, - "code_challenge_methods_supported": []string{"S256", "plain"}, + "token_endpoint_auth_methods_supported": []string{"client_secret_basic", "client_secret_post", "none", "private_key_jwt"}, + "code_challenge_methods_supported": codeChallengeMethods, + // RFC 8707 resource indicators are honored on the authorization_code + // flow (resource query param → access token aud) and token-exchange. + "resource_indicators_supported": true, "revocation_endpoint": issuer + "/oauth/revoke", "revocation_endpoint_auth_methods_supported": []string{"client_secret_basic", "client_secret_post"}, "introspection_endpoint": issuer + "/oauth/introspect", diff --git a/internal/http_handlers/token.go b/internal/http_handlers/token.go index 6fb793b4..00006151 100644 --- a/internal/http_handlers/token.go +++ b/internal/http_handlers/token.go @@ -177,6 +177,7 @@ func (h *httpProvider) TokenHandler() gin.HandlerFunc { var roles, scope []string loginMethod := "" sessionKey := "" + resource := "" // RFC 8707 resource bound to the auth code; becomes the access token aud oidcNonce := "" // OIDC nonce from the original /authorize request authTime := int64(0) // End-User's actual last authentication (OIDC Core §2 auth_time); 0 = unknown, CreateIDToken falls back to time.Now() @@ -233,6 +234,28 @@ func (h *httpProvider) TokenHandler() gin.HandlerFunc { } } + // RFC 8707 resource binding: sessionDataSplit[4] holds the resource + // (url-escaped) the client bound at /authorize, empty when none was + // supplied. When bound, the token request MUST echo exactly one + // matching resource (same enforcement shape as the PKCE + // code_verifier and redirect_uri checks). When present it becomes + // the access token's `aud` (see CreateAuthToken / AuthTokenConfig). + // When unbound the request `resource` is ignored — clients that + // don't use resource indicators are unaffected. + if len(sessionDataSplit) > 4 { + resource, _ = url.QueryUnescape(sessionDataSplit[4]) + } + if resource != "" { + requestResources := gc.PostFormArray("resource") + if len(requestResources) != 1 || strings.TrimSpace(requestResources[0]) != resource { + gc.JSON(http.StatusBadRequest, gin.H{ + "error": "invalid_grant", + "error_description": "The resource parameter does not match the authorization request", + }) + return + } + } + // Parse code_challenge and method from stored state. // Format: "challenge::method" or just "challenge" (legacy, defaults to plain per RFC 7636 §4.2) // or empty string (no PKCE — confidential client). @@ -243,6 +266,18 @@ func (h *httpProvider) TokenHandler() gin.HandlerFunc { storedChallenge = storedChallenge[:idx] } + // OAuth 2.1 strict mode: reject a "plain" PKCE code. /authorize + // already refuses to mint one under strict mode; this closes the + // window where the flag was flipped on between authorize and + // exchange for a code already in flight. + if h.Config.OAuth21Strict && storedChallenge != "" && storedMethod == "plain" { + gc.JSON(http.StatusBadRequest, gin.H{ + "error": "invalid_request", + "error_description": "code_challenge_method=plain is not allowed; use S256", + }) + return + } + // RFC 7636 §4.5: If PKCE was used at /authorize, the token request // MUST include code_verifier. This is orthogonal to client authentication. if storedChallenge != "" && codeVerifier != "" { @@ -365,7 +400,27 @@ func (h *httpProvider) TokenHandler() gin.HandlerFunc { // HTTP 400, not 401. claims, err := h.TokenProvider.ValidateRefreshToken(gc, refreshToken, resolvedClient.ClientID) if err != nil { - log.Debug().Err(err).Msg("Error validating refresh token") + // OAuth 2.1 §6.1 / RFC 9700 §4.14.2 refresh-token reuse + // response: a genuine, already-rotated refresh token was + // replayed. Revoke the whole live session family for this + // user so the (possibly stolen) live tokens are killed too — + // not just a no-op invalid_grant. DeleteAllUserSessions with + // the bare user id matches every login-method-namespaced + // session key (substring match), mirroring the existing + // revocation call sites (reset_password, deactivate_account). + if errors.Is(err, token.ErrRefreshTokenReuse) { + if sub, _ := claims["sub"].(string); sub != "" { + if delErr := h.MemoryStoreProvider.DeleteAllUserSessions(sub); delErr != nil { + log.Debug().Err(delErr).Str("user_id", sub).Msg("Failed to revoke sessions on refresh token reuse") + } + } + metrics.RecordSecurityEvent("refresh_token_reuse", "token_endpoint") + log.Warn().Msg("refresh token reuse detected — revoked user session family") + } else { + log.Debug().Err(err).Msg("Error validating refresh token") + } + // Same opaque response either way — never reveal to the caller + // whether reuse was detected (no oracle). gc.JSON(http.StatusBadRequest, gin.H{ "error": "invalid_grant", "error_description": "The refresh token is invalid or has expired", @@ -510,6 +565,7 @@ func (h *httpProvider) TokenHandler() gin.HandlerFunc { HostName: hostname, AuthTime: authTime, ClientID: resolvedClient.ClientID, + Resource: resource, }) if err != nil { log.Debug().Err(err).Msg("Error creating auth token") diff --git a/internal/integration_tests/oauth21_mcp_test.go b/internal/integration_tests/oauth21_mcp_test.go new file mode 100644 index 00000000..c23b3e64 --- /dev/null +++ b/internal/integration_tests/oauth21_mcp_test.go @@ -0,0 +1,371 @@ +package integration_tests + +import ( + "crypto/sha256" + "encoding/base64" + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "testing" + + "github.com/gin-gonic/gin" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/authorizerdev/authorizer/internal/constants" + "github.com/authorizerdev/authorizer/internal/graph/model" + "github.com/authorizerdev/authorizer/internal/token" +) + +// mcpSession signs up a fresh user, registers a browser session, and returns a +// router with /authorize + /oauth/token mounted plus the session cookie value. +// Tests build their own /authorize query (response_type, resource, PKCE method) +// on top of it. +func mcpSession(t *testing.T, ts *testSetup, scope []string) (http.Handler, string) { + t.Helper() + _, ctx := createContext(ts) + + email := "mcp_" + uuid.New().String() + "@authorizer.dev" + password := "Password@123" + _, err := ts.GraphQLProvider.SignUp(ctx, &model.SignUpRequest{ + Email: &email, + Password: password, + ConfirmPassword: password, + }) + require.NoError(t, err) + + user, err := ts.StorageProvider.GetUserByEmail(ctx, email) + require.NoError(t, err) + + nonce := uuid.New().String() + sessionData, sessionToken, sessionExpiresAt, err := ts.TokenProvider.CreateSessionToken(&token.AuthTokenConfig{ + User: user, + Nonce: nonce, + Roles: ts.Config.DefaultRoles, + Scope: scope, + LoginMethod: constants.AuthRecipeMethodBasicAuth, + }) + require.NoError(t, err) + + sessionKey := constants.AuthRecipeMethodBasicAuth + ":" + user.ID + require.NoError(t, ts.MemoryStoreProvider.SetUserSession( + sessionKey, constants.TokenTypeSessionToken+"_"+sessionData.Nonce, sessionToken, sessionExpiresAt)) + + router := gin.New() + router.GET("/authorize", ts.HttpProvider.AuthorizeHandler()) + router.POST("/oauth/token", ts.HttpProvider.TokenHandler()) + return router, sessionToken +} + +func doAuthorizeGET(router http.Handler, qs url.Values, sessionToken string) *httptest.ResponseRecorder { + w := httptest.NewRecorder() + req, _ := http.NewRequest(http.MethodGet, "/authorize?"+qs.Encode(), nil) + req.AddCookie(&http.Cookie{Name: constants.AppCookieName + "_session", Value: sessionToken}) + router.ServeHTTP(w, req) + return w +} + +func codeFromRedirect(t *testing.T, w *httptest.ResponseRecorder) string { + t.Helper() + require.Equal(t, http.StatusFound, w.Code, "authorize should redirect: %s", w.Body.String()) + loc, err := url.Parse(w.Header().Get("Location")) + require.NoError(t, err) + code := loc.Query().Get("code") + require.NotEmpty(t, code, "authorization code must be present") + return code +} + +// Item 2: RFC 8707 resource indicator on the primary authorization_code flow. +// A resource bound at /authorize must become the access token's `aud`, while +// the id_token audience stays the requesting client. +func TestOAuth21_ResourceIndicator_BindsAccessTokenAudience(t *testing.T) { + cfg := getTestConfig() + ts := initTestSetup(t, cfg) + registerTestClient(t, ts, "mcp-client", "mcp-secret") + + const resource = "https://mcp.example.com" + scope := []string{"openid", "profile", "email"} + + verifier := uuid.New().String() + uuid.New().String() + sum := sha256.Sum256([]byte(verifier)) + challenge := base64.RawURLEncoding.EncodeToString(sum[:]) + + router, cookie := mcpSession(t, ts, scope) + + qs := url.Values{} + qs.Set("client_id", "mcp-client") + qs.Set("response_type", "code") + qs.Set("redirect_uri", "http://localhost:3000/callback") + qs.Set("code_challenge", challenge) + qs.Set("code_challenge_method", "S256") + qs.Set("state", uuid.New().String()) + qs.Set("response_mode", "query") + qs.Set("scope", "openid profile email") + qs.Set("resource", resource) + + code := codeFromRedirect(t, doAuthorizeGET(router, qs, cookie)) + + form := url.Values{} + form.Set("grant_type", "authorization_code") + form.Set("code", code) + form.Set("code_verifier", verifier) + form.Set("redirect_uri", "http://localhost:3000/callback") + form.Set("resource", resource) + + w := exchangeCode(router, form, []string{"mcp-client", "mcp-secret"}) + require.Equal(t, http.StatusOK, w.Code, "code exchange body: %s", w.Body.String()) + + var body map[string]interface{} + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + + accessToken, _ := body["access_token"].(string) + require.NotEmpty(t, accessToken) + assert.Equal(t, resource, decodeJWTPayloadUnverified(t, accessToken)["aud"], + "access_token aud must be the bound resource (RFC 8707)") + + // The id_token audience must remain the OAuth client (OIDC), not the resource. + idToken, _ := body["id_token"].(string) + require.NotEmpty(t, idToken) + assert.Equal(t, "mcp-client", decodeJWTPayloadUnverified(t, idToken)["aud"], + "id_token aud must stay the requesting client, not the resource") +} + +// Item 2: a resource bound at /authorize must be echoed and matched at +// /oauth/token — a mismatch (or an omitted resource) is rejected invalid_grant, +// exactly like a PKCE code_verifier mismatch. +func TestOAuth21_ResourceIndicator_MismatchRejected(t *testing.T) { + cfg := getTestConfig() + ts := initTestSetup(t, cfg) + registerTestClient(t, ts, "mcp-client", "mcp-secret") + + scope := []string{"openid", "profile", "email"} + + authorizeForCode := func() (http.Handler, string, string) { + verifier := uuid.New().String() + uuid.New().String() + sum := sha256.Sum256([]byte(verifier)) + challenge := base64.RawURLEncoding.EncodeToString(sum[:]) + router, cookie := mcpSession(t, ts, scope) + qs := url.Values{} + qs.Set("client_id", "mcp-client") + qs.Set("response_type", "code") + qs.Set("redirect_uri", "http://localhost:3000/callback") + qs.Set("code_challenge", challenge) + qs.Set("code_challenge_method", "S256") + qs.Set("state", uuid.New().String()) + qs.Set("response_mode", "query") + qs.Set("scope", "openid profile email") + qs.Set("resource", "https://mcp.example.com") + return router, codeFromRedirect(t, doAuthorizeGET(router, qs, cookie)), verifier + } + + baseForm := func(code, verifier string) url.Values { + form := url.Values{} + form.Set("grant_type", "authorization_code") + form.Set("code", code) + form.Set("code_verifier", verifier) + form.Set("redirect_uri", "http://localhost:3000/callback") + return form + } + + t.Run("wrong resource rejected", func(t *testing.T) { + router, code, verifier := authorizeForCode() + form := baseForm(code, verifier) + form.Set("resource", "https://evil.example.com") + w := exchangeCode(router, form, []string{"mcp-client", "mcp-secret"}) + assert.Equal(t, http.StatusBadRequest, w.Code, "body: %s", w.Body.String()) + var errBody map[string]interface{} + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &errBody)) + assert.Equal(t, "invalid_grant", errBody["error"]) + }) + + t.Run("omitted resource rejected", func(t *testing.T) { + router, code, verifier := authorizeForCode() + form := baseForm(code, verifier) // no resource echoed + w := exchangeCode(router, form, []string{"mcp-client", "mcp-secret"}) + assert.Equal(t, http.StatusBadRequest, w.Code, "body: %s", w.Body.String()) + var errBody map[string]interface{} + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &errBody)) + assert.Equal(t, "invalid_grant", errBody["error"]) + }) +} + +// Item 1: OAuth 2.1 refresh-token rotation with reuse invalidation. After a +// refresh rotates the token, replaying the OLD refresh token must be rejected +// invalid_grant AND trigger the breach response — the whole live session +// family is revoked, so the freshly-issued refresh token stops working too. +func TestOAuth21_RefreshTokenReuse_RevokesFamily(t *testing.T) { + cfg := getTestConfig() + ts := initTestSetup(t, cfg) + registerTestClient(t, ts, "reuse-client", "reuse-secret") + + router, code, verifier := loginForOfflineAccess(t, ts, "reuse-client") + + exchangeForm := url.Values{} + exchangeForm.Set("grant_type", "authorization_code") + exchangeForm.Set("code", code) + exchangeForm.Set("code_verifier", verifier) + exchangeForm.Set("redirect_uri", "http://localhost:3000/callback") + + w := exchangeCode(router, exchangeForm, []string{"reuse-client", "reuse-secret"}) + require.Equal(t, http.StatusOK, w.Code, "code exchange body: %s", w.Body.String()) + var body map[string]interface{} + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + oldRefresh, _ := body["refresh_token"].(string) + require.NotEmpty(t, oldRefresh, "offline_access scope must yield a refresh_token") + + // Rotate once — a new refresh token is issued and the old one is retired. + refresh := func(rt string) *httptest.ResponseRecorder { + f := url.Values{} + f.Set("grant_type", "refresh_token") + f.Set("refresh_token", rt) + return exchangeCode(router, f, []string{"reuse-client", "reuse-secret"}) + } + + w1 := refresh(oldRefresh) + require.Equal(t, http.StatusOK, w1.Code, "first refresh body: %s", w1.Body.String()) + var body1 map[string]interface{} + require.NoError(t, json.Unmarshal(w1.Body.Bytes(), &body1)) + newRefresh, _ := body1["refresh_token"].(string) + require.NotEmpty(t, newRefresh) + require.NotEqual(t, oldRefresh, newRefresh, "rotation must mint a fresh refresh token") + + // Replay the OLD (already-rotated) refresh token: reuse detected. + w2 := refresh(oldRefresh) + assert.Equal(t, http.StatusBadRequest, w2.Code, "reuse body: %s", w2.Body.String()) + var errBody map[string]interface{} + require.NoError(t, json.Unmarshal(w2.Body.Bytes(), &errBody)) + assert.Equal(t, "invalid_grant", errBody["error"]) + + // Breach response: the reuse revoked the whole family, so the token that + // was legitimately rotated in is now dead too. + w3 := refresh(newRefresh) + assert.Equal(t, http.StatusBadRequest, w3.Code, + "the live refresh token must be revoked after reuse of its rotated predecessor: %s", w3.Body.String()) + var errBody3 map[string]interface{} + require.NoError(t, json.Unmarshal(w3.Body.Bytes(), &errBody3)) + assert.Equal(t, "invalid_grant", errBody3["error"]) +} + +// Item 4: the --oauth21-strict flag. Default (false) leaves the implicit grant +// and PKCE "plain" working exactly as today; true rejects both. +func TestOAuth21_StrictMode_Gating(t *testing.T) { + cfg := getTestConfig() + ts := initTestSetup(t, cfg) + registerTestClient(t, ts, "strict-client", "strict-secret") + + implicitQS := func() url.Values { + qs := url.Values{} + qs.Set("client_id", "strict-client") + qs.Set("response_type", "token") + qs.Set("redirect_uri", "http://localhost:3000/callback") + qs.Set("state", uuid.New().String()) + qs.Set("response_mode", "fragment") + qs.Set("scope", "openid profile email") + return qs + } + + plainPKCEQS := func(verifier string) url.Values { + qs := url.Values{} + qs.Set("client_id", "strict-client") + qs.Set("response_type", "code") + qs.Set("redirect_uri", "http://localhost:3000/callback") + // plain: code_challenge == code_verifier (RFC 7636 §4.2). + qs.Set("code_challenge", verifier) + qs.Set("code_challenge_method", "plain") + qs.Set("state", uuid.New().String()) + qs.Set("response_mode", "query") + qs.Set("scope", "openid profile email") + return qs + } + + t.Run("default false: implicit response_type=token still works", func(t *testing.T) { + ts.Config.OAuth21Strict = false + router, cookie := mcpSession(t, ts, []string{"openid", "profile", "email"}) + w := doAuthorizeGET(router, implicitQS(), cookie) + require.Equal(t, http.StatusFound, w.Code, "body: %s", w.Body.String()) + assert.Contains(t, w.Header().Get("Location"), "access_token=", + "implicit flow must return an access token in the fragment when strict is off") + }) + + t.Run("strict true: implicit response_type=token rejected", func(t *testing.T) { + ts.Config.OAuth21Strict = true + router, cookie := mcpSession(t, ts, []string{"openid", "profile", "email"}) + w := doAuthorizeGET(router, implicitQS(), cookie) + require.Equal(t, http.StatusFound, w.Code, "body: %s", w.Body.String()) + loc := w.Header().Get("Location") + assert.Contains(t, loc, "unsupported_response_type") + assert.NotContains(t, loc, "access_token=") + }) + + t.Run("default false: PKCE plain still works", func(t *testing.T) { + ts.Config.OAuth21Strict = false + verifier := uuid.New().String() + uuid.New().String() + router, cookie := mcpSession(t, ts, []string{"openid", "profile", "email"}) + code := codeFromRedirect(t, doAuthorizeGET(router, plainPKCEQS(verifier), cookie)) + + form := url.Values{} + form.Set("grant_type", "authorization_code") + form.Set("code", code) + form.Set("code_verifier", verifier) + form.Set("redirect_uri", "http://localhost:3000/callback") + w := exchangeCode(router, form, []string{"strict-client", "strict-secret"}) + require.Equal(t, http.StatusOK, w.Code, "plain PKCE exchange body: %s", w.Body.String()) + }) + + t.Run("strict true: PKCE plain rejected at authorize", func(t *testing.T) { + ts.Config.OAuth21Strict = true + verifier := uuid.New().String() + uuid.New().String() + router, cookie := mcpSession(t, ts, []string{"openid", "profile", "email"}) + w := doAuthorizeGET(router, plainPKCEQS(verifier), cookie) + assert.Equal(t, http.StatusBadRequest, w.Code, "body: %s", w.Body.String()) + assert.Contains(t, w.Body.String(), "invalid_request") + assert.Contains(t, w.Body.String(), "plain") + }) + + // Reset so a shared cfg pointer can't leak strict mode into later tests. + ts.Config.OAuth21Strict = false +} + +// Item 3 / Item 4: the RFC 8414 metadata handler advertises resource indicators +// and reflects strict-mode PKCE. The route alias itself is a one-line wrapper of +// this same handler (see internal/server/http_routes.go). +func TestOAuth21_AuthServerMetadata(t *testing.T) { + cfg := getTestConfig() + ts := initTestSetup(t, cfg) + + router := gin.New() + router.GET("/.well-known/oauth-authorization-server", ts.HttpProvider.OpenIDConfigurationHandler()) + + fetch := func() map[string]interface{} { + w := httptest.NewRecorder() + req, _ := http.NewRequest(http.MethodGet, "/.well-known/oauth-authorization-server", nil) + router.ServeHTTP(w, req) + require.Equal(t, http.StatusOK, w.Code) + var doc map[string]interface{} + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &doc)) + return doc + } + + methodsOf := func(doc map[string]interface{}) []string { + raw, _ := doc["code_challenge_methods_supported"].([]interface{}) + out := make([]string, 0, len(raw)) + for _, m := range raw { + out = append(out, m.(string)) + } + return out + } + + ts.Config.OAuth21Strict = false + doc := fetch() + assert.Equal(t, true, doc["resource_indicators_supported"], + "metadata must advertise resource_indicators_supported") + assert.ElementsMatch(t, []string{"S256", "plain"}, methodsOf(doc)) + + ts.Config.OAuth21Strict = true + assert.ElementsMatch(t, []string{"S256"}, methodsOf(fetch()), + "strict mode must advertise S256 only") + + ts.Config.OAuth21Strict = false +} diff --git a/internal/server/http_routes.go b/internal/server/http_routes.go index 62b0c0d6..394ee112 100644 --- a/internal/server/http_routes.go +++ b/internal/server/http_routes.go @@ -94,6 +94,10 @@ func (s *server) NewRouter() *gin.Engine { router.GET("/api/v1/org-discovery", s.Dependencies.HTTPProvider.OrgDiscoveryHandler()) // OPEN ID routes router.GET("/.well-known/openid-configuration", s.Dependencies.HTTPProvider.OpenIDConfigurationHandler()) + // RFC 8414 OAuth 2.0 Authorization Server Metadata. MCP clients probe this + // path first; the OIDC discovery document already satisfies RFC 8414's MUSTs, + // so serve the same handler as a thin alias (cheap interop win). + router.GET("/.well-known/oauth-authorization-server", s.Dependencies.HTTPProvider.OpenIDConfigurationHandler()) router.GET("/.well-known/jwks.json", s.Dependencies.HTTPProvider.JWKsHandler()) // RFC 6749 §3.1 / OIDC Core §3.1.2.1: the authorization endpoint MUST // support GET and MAY support POST. diff --git a/internal/service/auth_response.go b/internal/service/auth_response.go index c0074a90..8f36dfa9 100644 --- a/internal/service/auth_response.go +++ b/internal/service/auth_response.go @@ -36,6 +36,7 @@ func (p *provider) issueAuthResponse(ctx context.Context, meta RequestMetadata, nonce := "" oidcNonce := "" authorizeRedirectURI := "" + authorizeResource := "" if state != nil { authorizeState, _ := p.MemoryStoreProvider.GetState(refs.StringValue(state)) if authorizeState != "" { @@ -49,6 +50,10 @@ func (p *provider) issueAuthResponse(ctx context.Context, meta RequestMetadata, if len(authorizeStateSplit) > 3 { authorizeRedirectURI = authorizeStateSplit[3] } + // RFC 8707 resource (url-escaped) bound at /authorize, rebound to the code below. + if len(authorizeStateSplit) > 4 { + authorizeResource = authorizeStateSplit[4] + } } else { nonce = authorizeState } @@ -76,7 +81,7 @@ func (p *provider) issueAuthResponse(ctx context.Context, meta RequestMetadata, // Code challenge could be optional if PKCE flow is not used if code != "" { - if err := p.MemoryStoreProvider.SetState(code, codeChallenge+"@@"+authToken.FingerPrintHash+"@@"+oidcNonce+"@@"+authorizeRedirectURI); err != nil { + if err := p.MemoryStoreProvider.SetState(code, codeChallenge+"@@"+authToken.FingerPrintHash+"@@"+oidcNonce+"@@"+authorizeRedirectURI+"@@"+authorizeResource); err != nil { log.Debug().Err(err).Msg("Failed to set state") return nil, err } diff --git a/internal/service/login.go b/internal/service/login.go index 5c5864b1..10fb0023 100644 --- a/internal/service/login.go +++ b/internal/service/login.go @@ -491,6 +491,7 @@ func (p *provider) Login(ctx context.Context, meta RequestMetadata, params *mode nonce := "" oidcNonce := "" authorizeRedirectURI := "" + authorizeResource := "" if params.State != nil { // Get state from store authorizeState, _ := p.MemoryStoreProvider.GetState(refs.StringValue(params.State)) @@ -506,6 +507,10 @@ func (p *provider) Login(ctx context.Context, meta RequestMetadata, params *mode if len(authorizeStateSplit) > 3 { authorizeRedirectURI = authorizeStateSplit[3] } + // RFC 8707 resource (url-escaped) bound at /authorize, rebound to the code below. + if len(authorizeStateSplit) > 4 { + authorizeResource = authorizeStateSplit[4] + } } else { nonce = authorizeState } @@ -540,7 +545,7 @@ func (p *provider) Login(ctx context.Context, meta RequestMetadata, params *mode // Code challenge could be optional if PKCE flow is not used if code != "" { - if err := p.MemoryStoreProvider.SetState(code, codeChallenge+"@@"+authToken.FingerPrintHash+"@@"+oidcNonce+"@@"+authorizeRedirectURI); err != nil { + if err := p.MemoryStoreProvider.SetState(code, codeChallenge+"@@"+authToken.FingerPrintHash+"@@"+oidcNonce+"@@"+authorizeRedirectURI+"@@"+authorizeResource); err != nil { log.Debug().Msg("Failed to set state") return nil, nil, err } diff --git a/internal/service/session.go b/internal/service/session.go index 32f51cb1..9670f19f 100644 --- a/internal/service/session.go +++ b/internal/service/session.go @@ -78,6 +78,7 @@ func (p *provider) Session(ctx context.Context, meta RequestMetadata, params *mo codeChallenge := "" oidcNonce := "" authorizeRedirectURI := "" + authorizeResource := "" if params != nil && params.State != nil { authorizeState, _ := p.MemoryStoreProvider.GetState(refs.StringValue(params.State)) if authorizeState != "" { @@ -91,6 +92,10 @@ func (p *provider) Session(ctx context.Context, meta RequestMetadata, params *mo if len(parts) > 3 { authorizeRedirectURI = parts[3] } + // RFC 8707 resource (url-escaped) bound at /authorize, rebound to the code below. + if len(parts) > 4 { + authorizeResource = parts[4] + } } _ = p.MemoryStoreProvider.RemoveState(refs.StringValue(params.State)) } @@ -115,7 +120,7 @@ func (p *provider) Session(ctx context.Context, meta RequestMetadata, params *mo } if code != "" { - if err := p.MemoryStoreProvider.SetState(code, codeChallenge+"@@"+authToken.FingerPrintHash+"@@"+oidcNonce+"@@"+authorizeRedirectURI); err != nil { + if err := p.MemoryStoreProvider.SetState(code, codeChallenge+"@@"+authToken.FingerPrintHash+"@@"+oidcNonce+"@@"+authorizeRedirectURI+"@@"+authorizeResource); err != nil { log.Debug().Err(err).Msg("Failed to set code state") return nil, nil, err } diff --git a/internal/service/signup.go b/internal/service/signup.go index af76527b..62ce7326 100644 --- a/internal/service/signup.go +++ b/internal/service/signup.go @@ -299,6 +299,7 @@ func (p *provider) SignUp(ctx context.Context, meta RequestMetadata, params *mod nonce := "" oidcNonce := "" authorizeRedirectURI := "" + authorizeResource := "" if params.State != nil { // Get state from store authorizeState, _ := p.MemoryStoreProvider.GetState(refs.StringValue(params.State)) @@ -313,6 +314,10 @@ func (p *provider) SignUp(ctx context.Context, meta RequestMetadata, params *mod if len(authorizeStateSplit) > 3 { authorizeRedirectURI = authorizeStateSplit[3] } + // RFC 8707 resource (url-escaped) bound at /authorize, rebound to the code below. + if len(authorizeStateSplit) > 4 { + authorizeResource = authorizeStateSplit[4] + } } else { nonce = authorizeState } @@ -391,7 +396,7 @@ func (p *provider) SignUp(ctx context.Context, meta RequestMetadata, params *mod // Code challenge could be optional if PKCE flow is not used if code != "" { - if err := p.MemoryStoreProvider.SetState(code, codeChallenge+"@@"+authToken.FingerPrintHash+"@@"+oidcNonce+"@@"+authorizeRedirectURI); err != nil { + if err := p.MemoryStoreProvider.SetState(code, codeChallenge+"@@"+authToken.FingerPrintHash+"@@"+oidcNonce+"@@"+authorizeRedirectURI+"@@"+authorizeResource); err != nil { log.Debug().Err(err).Msg("SetState failed") return nil, nil, err } diff --git a/internal/token/auth_token.go b/internal/token/auth_token.go index 2494851d..8f2f1bd2 100644 --- a/internal/token/auth_token.go +++ b/internal/token/auth_token.go @@ -5,6 +5,7 @@ import ( "crypto/subtle" "encoding/base64" "encoding/json" + "errors" "fmt" "strings" "time" @@ -21,6 +22,16 @@ import ( "github.com/authorizerdev/authorizer/internal/utils" ) +// ErrRefreshTokenReuse signals that a genuine, correctly-signed and +// client-bound refresh token was presented but no live session record backs +// it — i.e. it was already rotated away (or the session was logged out / +// expired) and is now being replayed. OAuth 2.1 §6.1 / RFC 9700 §4.14.2 treat +// this as a compromise signal: the caller MUST revoke the token family. It is +// returned instead of a plain unauthorized so the token endpoint can trigger +// that revocation. A malformed / foreign / wrong-type token yields a plain +// unauthorized (no side effect) — only a genuine reuse trips this. +var ErrRefreshTokenReuse = errors.New("refresh token reuse detected") + // reservedClaims are security-critical JWT claims that custom scripts must not override. var reservedClaims = map[string]bool{ "sub": true, @@ -84,6 +95,15 @@ type AuthTokenConfig struct { // to the same client (RFC 6749 §6). Empty is valid (client_credentials // / machine paths that never mint a refresh token). ClientID string + // Resource is the RFC 8707 resource indicator the client bound to the + // authorization request (the target MCP/API server). When non-empty it + // becomes the ACCESS token's `aud` claim so the token cannot be replayed + // at a different resource server (audience restriction). It intentionally + // does NOT change the id_token or refresh_token audience: the id_token + // audience is the client (OIDC), and the refresh_token audience is the + // client too (RFC 6749 §6 client binding). Empty preserves existing + // behavior — aud defaults to the requesting client / bootstrap client_id. + Resource string } // loginMethodToAMR maps an internal LoginMethod value to the OIDC Core §2 @@ -246,6 +266,19 @@ func (p *provider) audience(cfg *AuthTokenConfig) string { return p.config.ClientID } +// accessTokenAudience returns the "aud" claim for an ACCESS token. When the +// client supplied an RFC 8707 resource indicator it is the audience — the +// token is bound to that resource server and is not replayable elsewhere. +// Otherwise it falls back to audience() (the requesting client), so callers +// that don't use resource indicators are unaffected. Only the access token +// uses this; id_token / refresh_token audiences remain the client. +func (p *provider) accessTokenAudience(cfg *AuthTokenConfig) string { + if cfg.Resource != "" { + return cfg.Resource + } + return p.audience(cfg) +} + // CreateRefreshToken util to create JWT token func (p *provider) CreateRefreshToken(cfg *AuthTokenConfig) (string, int64, error) { // Lifetime is configurable via --refresh-token-expires-in (seconds). @@ -301,7 +334,7 @@ func (p *provider) CreateAccessToken(cfg *AuthTokenConfig) (string, int64, error expiresAt := time.Now().Add(expiryBound).Unix() customClaims := jwt.MapClaims{ "iss": cfg.HostName, - "aud": p.audience(cfg), + "aud": p.accessTokenAudience(cfg), "nonce": cfg.Nonce, "sub": cfg.User.ID, "exp": expiresAt, @@ -490,6 +523,22 @@ func (p *provider) ValidateRefreshToken(gc *gin.Context, refreshToken string, ex token, err := p.dependencies.MemoryStoreProvider.GetUserSession(sessionKey, constants.TokenTypeRefreshToken+"_"+nonce) if nonce == "" || err != nil { p.dependencies.Log.Debug().Err(err).Msgf("invalid refresh token: %v, key: %s", err, sessionKey+":"+constants.TokenTypeRefreshToken+"_"+nonce) + // Reuse detection (OAuth 2.1 §6.1 / RFC 9700 §4.14.2): ParseJWTToken + // above already verified this token's signature, so it is genuinely + // one we issued. If it carries a nonce and is a refresh token bound to + // the presenting client, yet no live session record backs the nonce, + // it was already rotated away and is being replayed — a compromise + // signal. Surface it distinctly so the token endpoint revokes the + // family. Anything else (missing nonce, wrong token_type, or a token + // bound to a different client) is an ordinary unauthorized with no + // side effect, so a forged/foreign token cannot force a revocation. + if nonce != "" { + tokenType, _ := res["token_type"].(string) + aud, _ := res["aud"].(string) + if tokenType == constants.TokenTypeRefreshToken && aud == expectedClientID { + return res, ErrRefreshTokenReuse + } + } return res, fmt.Errorf(`unauthorized`) } From f40d3de53ebc826515d182cec407947aa44e73cf Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Tue, 21 Jul 2026 16:14:36 +0530 Subject: [PATCH 2/4] security(oauth): scope refresh-token reuse revocation to the token family MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix four findings from the adversarial review of PR #693. Fix 1 (HIGH) — refresh-token reuse was an unauthenticated forced-logout DoS. On reuse detection the token endpoint called DeleteAllUserSessions(userID), wiping every session of the user across every login method. Since the refresh grant needs no client secret, anyone holding any retired refresh token (logs, history, backups) could keep a victim perpetually logged out. Approach taken: full family-scoping via a per-lineage family record — NOT the grace-window+cooldown fallback. Investigating the memory_store backends showed the subkey-encoding sketch (refresh_token__) would force family_id into access/session tokens + validation and ~40 Set/Get/Delete call sites — far more invasive than described. Instead: - Refresh tokens carry a stable family_id claim, minted once at first issuance and carried forward unchanged across every rotation (nonce still rotates). Generation lives in CreateRefreshToken, so every issuance path gets one for free; family_id is reserved against custom-script forgery. - On rotation the handler writes a family index record ("refresh_family_" -> {live, prev, rotated_at}) via the existing SetUserSession — no new interface method, works uniformly across the in-memory, redis and db backends. - On reuse, revocation deletes only that family's live session via the existing DeleteUserSession(sessionKey, liveNonce); other lineages and login-methods of the same user are untouched. A family-less (legacy) or live-lineage-absent replay revokes nothing — never the whole user. - Grace window (10s): a replay of the immediate predecessor within the window is a benign double-submit (multi-tab/retry), not a breach — the live session is not revoked. Any older nonce, or a predecessor replayed after the window, is genuine reuse and revokes the family. Fix 2 (MEDIUM) — strict mode now rejects every response_type delivering a bearer token into the fragment. The check is component-exact on the canonical response_type, catching the front-channel-token hybrids "code token" and "code id_token token" alongside "token"/"id_token token", while leaving pure "id_token" (substring only) untouched. Fix 3 (LOW) — the /authorize resource parameter is validated per RFC 8707 §2 (absolute URI, no fragment); an invalid value is rejected with invalid_target. Fix 4 (LOW/MEDIUM) — ValidateAccessToken rejects a token whose aud is a resource indicator (absolute URI other than the default audience) at Authorizer's own protected endpoints (/userinfo, GraphQL, gRPC). Introspection keeps its own ParseJWTToken path and is unaffected, so an external RS can still validate resource-bound tokens. Tests: family-scoped revocation kills only its lineage (unrelated same-user session survives); grace window benign within / breach after; strict-mode rejection of the two hybrids plus pure-id_token allowance; resource validation; and the aud restriction at /userinfo. All pre-existing OAuth 2.1 tests updated and passing. --- internal/http_handlers/authorize.go | 66 ++- internal/http_handlers/token.go | 180 +++++++- .../oauth21_hardening_test.go | 403 ++++++++++++++++++ .../integration_tests/oauth21_mcp_test.go | 54 ++- internal/token/auth_token.go | 42 ++ 5 files changed, 695 insertions(+), 50 deletions(-) create mode 100644 internal/integration_tests/oauth21_hardening_test.go diff --git a/internal/http_handlers/authorize.go b/internal/http_handlers/authorize.go index 3b1663d6..603c9d50 100644 --- a/internal/http_handlers/authorize.go +++ b/internal/http_handlers/authorize.go @@ -190,6 +190,14 @@ func (h *httpProvider) AuthorizeHandler() gin.HandlerFunc { return } + // RFC 8707 §2: the resource indicator MUST be an absolute URI and MUST + // NOT include a fragment component. The RFC-conventional error code for + // a rejected resource is invalid_target. + if resource != "" && !isValidResourceIndicator(resource) { + redirectErrorToRP(gc, responseMode, redirectURI, state, "invalid_target", "resource must be an absolute URI without a fragment") + return + } + // OIDCC-3.1.2.6 (JAR, RFC 9101): the server must either process a // `request`/`request_uri` object or reject it with // request_not_supported / request_uri_not_supported. We don't parse @@ -283,12 +291,17 @@ func (h *httpProvider) AuthorizeHandler() gin.HandlerFunc { } } - // OAuth 2.1 strict mode: the implicit grant is removed. Reject the - // response types that return an access token directly from the - // authorization endpoint (response_type=token and id_token token). - // Pure id_token and the code-based hybrids are left untouched — only - // the access-token-in-the-fragment cases are dropped. - if h.Config.OAuth21Strict && (responseType == constants.ResponseTypeToken || responseType == "id_token token") { + // OAuth 2.1 strict mode: the implicit grant is removed. Reject EVERY + // response type that delivers a bearer access token into the URL + // fragment — not just "token" and "id_token token" but also the + // front-channel-token hybrids "code token" and "code id_token token", + // which equally return an access_token in the fragment (see the + // hasAccessToken dispatch below). responseType is already canonical + // (sorted/deduped/lowercased), so a bearer access token is present iff + // the space-separated components include the exact component "token". + // The check is component-exact (not a substring) so "id_token" — which + // contains "token" only as a substring — is NOT matched. + if h.Config.OAuth21Strict && responseTypeHasBareToken(responseType) { redirectErrorToRP(gc, responseMode, redirectURI, state, "unsupported_response_type", "response_type="+responseType+" is not supported") return } @@ -975,6 +988,47 @@ func supportedResponseTypeSet(raw string) (string, bool) { return "", false } +// isValidResourceIndicator enforces RFC 8707 §2 on a resource indicator: it +// MUST be an absolute URI (scheme + hierarchical part) and MUST NOT contain a +// fragment component. A relative reference, an opaque non-URI string, or any +// value carrying a "#" is rejected. Callers must pass a non-empty value. +func isValidResourceIndicator(resource string) bool { + // A fragment delimiter is forbidden outright — check the raw string so a + // trailing "#" (empty fragment) is also rejected, not just a populated one. + if strings.Contains(resource, "#") { + return false + } + u, err := url.Parse(resource) + if err != nil { + return false + } + // Absolute URI: has a scheme and is not just an opaque scheme:string. Require + // a scheme and either a host or a rooted path so bare "mailto:" style opaque + // forms and relative references are rejected. + if !u.IsAbs() { + return false + } + if u.Host == "" && !strings.HasPrefix(u.Path, "/") { + return false + } + return true +} + +// responseTypeHasBareToken reports whether the (already canonicalized, +// space-separated) response_type includes the exact component "token" — i.e. +// the flow delivers a bearer access token into the front channel (URL +// fragment). It splits on whitespace and compares each component exactly, so +// "id_token" (which contains "token" only as a substring) is NOT matched. +// Matches: "token", "code token", "id_token token", "code id_token token". +func responseTypeHasBareToken(responseType string) bool { + for _, c := range strings.Fields(responseType) { + if c == constants.ResponseTypeToken { + return true + } + } + return false +} + // validateAuthorizeRequest validates the authorize request parameters and // returns RFC 6749 §4.1.2.1 compliant error code and description on failure. // Returns empty strings when validation passes. diff --git a/internal/http_handlers/token.go b/internal/http_handlers/token.go index 00006151..ace475ae 100644 --- a/internal/http_handlers/token.go +++ b/internal/http_handlers/token.go @@ -4,6 +4,7 @@ import ( "crypto/sha256" "crypto/subtle" "encoding/base64" + "encoding/json" "errors" "net/http" "net/url" @@ -64,6 +65,114 @@ func formURLDecodeOrKeep(s string) string { return s } +// refreshFamilyKeyPrefix namespaces the per-lineage reuse-detection index in +// the session store. The full sub-key is refreshFamilyKeyPrefix+, +// stored under the same session key (":") as the token +// entries. It can never collide with a token entry (those are +// "_", e.g. "refresh_token_") because the literal prefix +// differs ("refresh_family_" vs "refresh_token_"). DeleteAllUserSessions still +// sweeps it (substring/glob match on the user id), so logout/reset clear it. +const refreshFamilyKeyPrefix = "refresh_family_" + +// refreshReuseGraceWindow is how long after a rotation a replay of the +// just-rotated-away (immediate-predecessor) token is treated as a benign +// double-submit (multi-tab SPA, network retry) rather than a breach. Kept +// short: long enough to absorb a client retrying with the token it already +// swapped, short enough that a stolen predecessor replayed later still trips +// revocation. +// ponytail: fixed 10s window; make it a CLI flag only if a real client needs longer. +const refreshReuseGraceWindow = 10 * time.Second + +// refreshFamilyRecord is the reuse-detection index for one refresh-token +// lineage. LiveNonce is the current live token's nonce (the one revoked on a +// genuine reuse); PrevNonce is the nonce it was just rotated from (the benign +// double-submit candidate); RotatedAt bounds the grace window. +type refreshFamilyRecord struct { + LiveNonce string `json:"live"` + PrevNonce string `json:"prev"` + RotatedAt int64 `json:"rotated_at"` +} + +// setRefreshFamilyRecord persists (overwrites) the family index for a lineage. +// Best-effort: a failure only degrades reuse detection for this family, it must +// not fail the token issuance the caller just completed. +func (h *httpProvider) setRefreshFamilyRecord(sessionKey, familyID string, rec refreshFamilyRecord, expiresAt int64) { + b, err := json.Marshal(rec) + if err != nil { + return + } + if err := h.MemoryStoreProvider.SetUserSession(sessionKey, refreshFamilyKeyPrefix+familyID, string(b), expiresAt); err != nil { + h.Log.Debug().Err(err).Str("session_key", sessionKey).Msg("Failed to persist refresh family record") + } +} + +// handleRefreshTokenReuse applies OAuth 2.1 §6.1 / RFC 9700 §4.14.2 reuse +// revocation, scoped to the compromised refresh token's own lineage (family) +// rather than the whole user. This closes the unauthenticated forced-logout +// DoS: replaying any single retired token can no longer wipe a user's other +// sessions/login-methods. It also applies the benign double-submit grace +// window so a legitimate multi-tab / retry race does not self-revoke. +func (h *httpProvider) handleRefreshTokenReuse(gc *gin.Context, claims map[string]interface{}) { + log := h.Log.With().Str("func", "handleRefreshTokenReuse").Logger() + + sub, _ := claims["sub"].(string) + if sub == "" { + return + } + sessionKey := sub + if lm, _ := claims["login_method"].(string); lm != "" { + sessionKey = lm + ":" + sub + } + familyID, _ := claims["family_id"].(string) + replayedNonce, _ := claims["nonce"].(string) + + // Legacy tokens (issued before family tracking) carry no family_id, so there + // is no lineage to scope to. Do NOT fall back to nuking the whole user — + // that is exactly the DoS this fix removes. A family-less replay revokes + // nothing; the token is still rejected (invalid_grant) by the caller. + if familyID == "" { + metrics.RecordSecurityEvent("refresh_token_reuse_no_family", "token_endpoint") + log.Warn().Msg("refresh token reuse detected on a family-less (legacy) token — no lineage to revoke") + return + } + + recRaw, err := h.MemoryStoreProvider.GetUserSession(sessionKey, refreshFamilyKeyPrefix+familyID) + if err != nil || recRaw == "" { + // No live lineage backing this family: it was never rotated, or was + // logged out / expired. Nothing live to revoke — and crucially, no other + // session or login-method for this user is touched. + metrics.RecordSecurityEvent("refresh_token_reuse_no_live_family", "token_endpoint") + return + } + var rec refreshFamilyRecord + if err := json.Unmarshal([]byte(recRaw), &rec); err != nil { + log.Debug().Err(err).Msg("Failed to decode refresh family record") + return + } + + // Benign double-submit grace window: only when the replayed token is the + // IMMEDIATE predecessor of the current live token (rec.PrevNonce) AND the + // rotation was within the window. An immediate-predecessor replay means the + // live token has not itself been used since (had it been used, the chain + // would have advanced and rec.PrevNonce would no longer equal the replayed + // nonce) — that is a multi-tab/retry race, not theft. Any older nonce, or a + // predecessor replayed after the window, is treated as genuine reuse. + if replayedNonce == rec.PrevNonce && time.Now().Unix()-rec.RotatedAt <= int64(refreshReuseGraceWindow.Seconds()) { + metrics.RecordSecurityEvent("refresh_token_double_submit", "token_endpoint") + log.Debug().Msg("refresh token double-submit within grace window — treated as race, family not revoked") + return + } + + // Genuine reuse: revoke only this family's live session (session + access + + // refresh entries for the current live nonce). DeleteUserSession is scoped to + // this exact (sessionKey, nonce), so other lineages/login-methods survive. + if err := h.MemoryStoreProvider.DeleteUserSession(sessionKey, rec.LiveNonce); err != nil { + log.Debug().Err(err).Str("session_key", sessionKey).Msg("Failed to revoke live session on refresh token reuse") + } + metrics.RecordSecurityEvent("refresh_token_reuse", "token_endpoint") + log.Warn().Msg("refresh token reuse detected — revoked the compromised token family only") +} + // TokenHandler to handle /oauth/token requests // grant type required func (h *httpProvider) TokenHandler() gin.HandlerFunc { @@ -180,6 +289,12 @@ func (h *httpProvider) TokenHandler() gin.HandlerFunc { resource := "" // RFC 8707 resource bound to the auth code; becomes the access token aud oidcNonce := "" // OIDC nonce from the original /authorize request authTime := int64(0) // End-User's actual last authentication (OIDC Core §2 auth_time); 0 = unknown, CreateIDToken falls back to time.Now() + // Refresh-token family tracking for reuse detection (OAuth 2.1 §6.1). + // Populated only on the refresh_token grant: refreshFamilyID is the + // lineage id carried forward into the rotated token, oldRefreshNonce is + // the nonce being rotated away (recorded as the family's prev_nonce). + refreshFamilyID := "" + oldRefreshNonce := "" if isAuthorizationCodeGrant { if code == "" { @@ -402,20 +517,13 @@ func (h *httpProvider) TokenHandler() gin.HandlerFunc { if err != nil { // OAuth 2.1 §6.1 / RFC 9700 §4.14.2 refresh-token reuse // response: a genuine, already-rotated refresh token was - // replayed. Revoke the whole live session family for this - // user so the (possibly stolen) live tokens are killed too — - // not just a no-op invalid_grant. DeleteAllUserSessions with - // the bare user id matches every login-method-namespaced - // session key (substring match), mirroring the existing - // revocation call sites (reset_password, deactivate_account). + // replayed. Revoke ONLY the compromised token's lineage + // (family) — not the whole user — so replaying one retired + // token cannot force-log-out the user's other sessions and + // login-methods (an unauthenticated DoS). handleRefreshTokenReuse + // also applies the benign double-submit grace window. if errors.Is(err, token.ErrRefreshTokenReuse) { - if sub, _ := claims["sub"].(string); sub != "" { - if delErr := h.MemoryStoreProvider.DeleteAllUserSessions(sub); delErr != nil { - log.Debug().Err(delErr).Str("user_id", sub).Msg("Failed to revoke sessions on refresh token reuse") - } - } - metrics.RecordSecurityEvent("refresh_token_reuse", "token_endpoint") - log.Warn().Msg("refresh token reuse detected — revoked user session family") + h.handleRefreshTokenReuse(gc, claims) } else { log.Debug().Err(err).Msg("Error validating refresh token") } @@ -507,6 +615,17 @@ func (h *httpProvider) TokenHandler() gin.HandlerFunc { return } + // Carry the refresh-token family forward across this rotation. A + // legacy token issued before family tracking has no family_id claim — + // mint a fresh one so the rotated token (and its family record) are + // consistent from here on. oldRefreshNonce becomes the family's + // prev_nonce, used by the reuse grace window. + refreshFamilyID, _ = claims["family_id"].(string) + if refreshFamilyID == "" { + refreshFamilyID = uuid.New().String() + } + oldRefreshNonce = nonce + // RFC 6749 §6: "the authorization server MUST verify the binding // between the refresh token and client identity whenever // possible." Enforced above via ValidateRefreshToken's audience @@ -556,16 +675,17 @@ func (h *httpProvider) TokenHandler() gin.HandlerFunc { hostname := parsers.GetHost(gc) nonce := uuid.New().String() authToken, err := h.TokenProvider.CreateAuthToken(gc, &token.AuthTokenConfig{ - User: user, - Roles: roles, - Scope: scope, - LoginMethod: loginMethod, - Nonce: nonce, - OIDCNonce: oidcNonce, - HostName: hostname, - AuthTime: authTime, - ClientID: resolvedClient.ClientID, - Resource: resource, + User: user, + Roles: roles, + Scope: scope, + LoginMethod: loginMethod, + Nonce: nonce, + OIDCNonce: oidcNonce, + HostName: hostname, + AuthTime: authTime, + ClientID: resolvedClient.ClientID, + Resource: resource, + RefreshTokenFamilyID: refreshFamilyID, }) if err != nil { log.Debug().Err(err).Msg("Error creating auth token") @@ -628,6 +748,20 @@ func (h *httpProvider) TokenHandler() gin.HandlerFunc { }) return } + // Record/refresh the family index on rotation so reuse detection can + // (a) find and revoke only this lineage's live session and (b) tell a + // benign double-submit of the just-rotated token from genuine theft. + // Only written on the refresh grant — first issuance needs no record + // (reuse can only be detected after a rotation or expiry, and a + // replay of a never-rotated / logged-out token has no live lineage to + // protect, which is exactly when the old whole-user nuke did harm). + if isRefreshTokenGrant && refreshFamilyID != "" { + h.setRefreshFamilyRecord(sessionKey, refreshFamilyID, refreshFamilyRecord{ + LiveNonce: authToken.FingerPrint, + PrevNonce: oldRefreshNonce, + RotatedAt: time.Now().Unix(), + }, authToken.RefreshToken.ExpiresAt) + } } if isRefreshTokenGrant { metrics.RecordAuthEvent(metrics.EventTokenRefresh, metrics.StatusSuccess) diff --git a/internal/integration_tests/oauth21_hardening_test.go b/internal/integration_tests/oauth21_hardening_test.go new file mode 100644 index 00000000..717655f2 --- /dev/null +++ b/internal/integration_tests/oauth21_hardening_test.go @@ -0,0 +1,403 @@ +package integration_tests + +import ( + "context" + "crypto/sha256" + "encoding/base64" + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/gin-gonic/gin" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/authorizerdev/authorizer/internal/constants" + "github.com/authorizerdev/authorizer/internal/graph/model" + "github.com/authorizerdev/authorizer/internal/token" +) + +// refreshFamilyKeyPrefix mirrors the (unexported) constant in +// internal/http_handlers/token.go. The reuse-detection family record for a +// lineage is stored under ":" at this sub-key + +// . Tests reach into it to simulate elapsed time for the grace +// window without sleeping. +const refreshFamilyKeyPrefix = "refresh_family_" + +// offlineAccessSessionForEmail logs an ALREADY-signed-up user in for a fresh +// offline_access session and returns a router plus an authorization code + +// PKCE verifier ready to exchange. Calling it twice for the same email yields +// two independent refresh-token lineages (families) under the same session key +// — the setup needed to prove reuse revocation is scoped to one family. +func offlineAccessSessionForEmail(t *testing.T, ts *testSetup, clientID, email string) (http.Handler, string, string) { + t.Helper() + _, ctx := createContext(ts) + + user, err := ts.StorageProvider.GetUserByEmail(ctx, email) + require.NoError(t, err) + + nonce := uuid.New().String() + scope := []string{"openid", "profile", "email", "offline_access"} + sessionData, sessionToken, sessionExpiresAt, err := ts.TokenProvider.CreateSessionToken(&token.AuthTokenConfig{ + User: user, + Nonce: nonce, + Roles: ts.Config.DefaultRoles, + Scope: scope, + LoginMethod: constants.AuthRecipeMethodBasicAuth, + }) + require.NoError(t, err) + + sessionKey := constants.AuthRecipeMethodBasicAuth + ":" + user.ID + require.NoError(t, ts.MemoryStoreProvider.SetUserSession( + sessionKey, constants.TokenTypeSessionToken+"_"+sessionData.Nonce, sessionToken, sessionExpiresAt)) + + verifier := uuid.New().String() + uuid.New().String() + sum := sha256.Sum256([]byte(verifier)) + challenge := base64.RawURLEncoding.EncodeToString(sum[:]) + + router := gin.New() + router.GET("/authorize", ts.HttpProvider.AuthorizeHandler()) + router.POST("/oauth/token", ts.HttpProvider.TokenHandler()) + + qs := url.Values{} + qs.Set("client_id", clientID) + qs.Set("response_type", "code") + qs.Set("redirect_uri", "http://localhost:3000/callback") + qs.Set("code_challenge", challenge) + qs.Set("code_challenge_method", "S256") + qs.Set("state", uuid.New().String()) + qs.Set("response_mode", "query") + qs.Set("scope", strings.Join(scope, " ")) + + code := codeFromRedirect(t, doAuthorizeGET(router, qs, sessionToken)) + return router, code, verifier +} + +func signupUser(t *testing.T, ts *testSetup) string { + t.Helper() + _, ctx := createContext(ts) + email := "hardening_" + uuid.New().String() + "@authorizer.dev" + password := "Password@123" + _, err := ts.GraphQLProvider.SignUp(ctx, &model.SignUpRequest{ + Email: &email, + Password: password, + ConfirmPassword: password, + }) + require.NoError(t, err) + return email +} + +func exchangeForRefresh(t *testing.T, router http.Handler, code, verifier string, basic []string) string { + t.Helper() + form := url.Values{} + form.Set("grant_type", "authorization_code") + form.Set("code", code) + form.Set("code_verifier", verifier) + form.Set("redirect_uri", "http://localhost:3000/callback") + w := exchangeCode(router, form, basic) + require.Equal(t, http.StatusOK, w.Code, "code exchange body: %s", w.Body.String()) + var body map[string]interface{} + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + rt, _ := body["refresh_token"].(string) + require.NotEmpty(t, rt, "offline_access must yield a refresh_token") + return rt +} + +func doRefresh(router http.Handler, rt string, basic []string) *httptest.ResponseRecorder { + f := url.Values{} + f.Set("grant_type", "refresh_token") + f.Set("refresh_token", rt) + return exchangeCode(router, f, basic) +} + +func refreshOK(t *testing.T, router http.Handler, rt string, basic []string) string { + t.Helper() + w := doRefresh(router, rt, basic) + require.Equal(t, http.StatusOK, w.Code, "refresh body: %s", w.Body.String()) + var body map[string]interface{} + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + rt2, _ := body["refresh_token"].(string) + require.NotEmpty(t, rt2) + return rt2 +} + +// Item 1 (the HIGH fix): refresh-token reuse revocation MUST be scoped to the +// compromised token's own family — replaying one retired token must NOT log the +// user out of their OTHER live sessions. Before the fix, the token endpoint +// called DeleteAllUserSessions(userID), so any single leaked/retired refresh +// token was an unauthenticated forced-logout DoS across every session of that +// user. This test builds TWO independent refresh lineages for the SAME user +// (same session key), triggers genuine reuse on one, and asserts the other +// survives. +func TestOAuth21_RefreshTokenReuse_ScopedToFamily(t *testing.T) { + cfg := getTestConfig() + ts := initTestSetup(t, cfg) + registerTestClient(t, ts, "family-client", "family-secret") + basic := []string{"family-client", "family-secret"} + + email := signupUser(t, ts) + + // Family A and family B: two separate logins of the same user. + routerA, codeA, verifierA := offlineAccessSessionForEmail(t, ts, "family-client", email) + familyARefresh := exchangeForRefresh(t, routerA, codeA, verifierA, basic) + + routerB, codeB, verifierB := offlineAccessSessionForEmail(t, ts, "family-client", email) + familyBRefresh := exchangeForRefresh(t, routerB, codeB, verifierB, basic) + + // Advance family A twice so its original token is genuinely stale (not the + // immediate predecessor covered by the grace window), then replay it. + aRotated := refreshOK(t, routerA, familyARefresh, basic) + aLive := refreshOK(t, routerA, aRotated, basic) + + reuse := doRefresh(routerA, familyARefresh, basic) + assert.Equal(t, http.StatusBadRequest, reuse.Code, "reuse body: %s", reuse.Body.String()) + + // Family A's live token is revoked (breach response, scoped to family A). + deadA := doRefresh(routerA, aLive, basic) + assert.Equal(t, http.StatusBadRequest, deadA.Code, + "family A live token must be revoked after reuse in family A: %s", deadA.Body.String()) + + // The DoS fix: family B — an unrelated session of the SAME user — is + // untouched and still refreshes. Under the old whole-user revocation this + // would have been wiped too. + survivor := doRefresh(routerB, familyBRefresh, basic) + assert.Equal(t, http.StatusOK, survivor.Code, + "an unrelated session of the same user must survive reuse in another family: %s", survivor.Body.String()) +} + +// Item 1 grace window: a benign double-submit (multi-tab SPA / network retry) +// replays the just-rotated-away token against its own still-live successor. +// Within the short grace window that is treated as an ordinary race — the +// replay fails invalid_grant but the live session is NOT revoked. After the +// window the SAME replay is treated as genuine reuse and revokes the family. +func TestOAuth21_RefreshTokenReuse_GraceWindow(t *testing.T) { + cfg := getTestConfig() + ts := initTestSetup(t, cfg) + registerTestClient(t, ts, "grace-client", "grace-secret") + basic := []string{"grace-client", "grace-secret"} + + t.Run("within window: benign double-submit does not revoke", func(t *testing.T) { + email := signupUser(t, ts) + router, code, verifier := offlineAccessSessionForEmail(t, ts, "grace-client", email) + original := exchangeForRefresh(t, router, code, verifier, basic) + + live := refreshOK(t, router, original, basic) // rotate once: original -> live + + // Immediately replay the just-rotated-away token (the immediate + // predecessor) — a double-submit race. + replay := doRefresh(router, original, basic) + assert.Equal(t, http.StatusBadRequest, replay.Code, "the replayed token itself is invalid: %s", replay.Body.String()) + + // The live session must NOT have been revoked — it still refreshes. + stillAlive := doRefresh(router, live, basic) + assert.Equal(t, http.StatusOK, stillAlive.Code, + "a benign double-submit within the grace window must not revoke the live session: %s", stillAlive.Body.String()) + }) + + t.Run("after window: same replay is genuine reuse and revokes", func(t *testing.T) { + email := signupUser(t, ts) + router, code, verifier := offlineAccessSessionForEmail(t, ts, "grace-client", email) + original := exchangeForRefresh(t, router, code, verifier, basic) + + live := refreshOK(t, router, original, basic) // rotate once: original -> live + + // Simulate the grace window having elapsed by ageing the family record's + // rotated_at far into the past (no sleep). familyID + owner come from the + // live refresh token's claims. + ageFamilyRecordPast(t, ts, live) + + replay := doRefresh(router, original, basic) + assert.Equal(t, http.StatusBadRequest, replay.Code, "reuse body: %s", replay.Body.String()) + + // Now the live session IS revoked — reuse after the window is a breach. + revoked := doRefresh(router, live, basic) + assert.Equal(t, http.StatusBadRequest, revoked.Code, + "reuse of the immediate predecessor after the grace window must revoke the family: %s", revoked.Body.String()) + }) +} + +// ageFamilyRecordPast rewrites the reuse-detection family record for the given +// live refresh token so its rotated_at is far in the past, simulating an +// elapsed grace window without sleeping. +func ageFamilyRecordPast(t *testing.T, ts *testSetup, liveRefresh string) { + t.Helper() + claims := decodeJWTPayloadUnverified(t, liveRefresh) + familyID, _ := claims["family_id"].(string) + require.NotEmpty(t, familyID, "live refresh token must carry a family_id") + sub, _ := claims["sub"].(string) + loginMethod, _ := claims["login_method"].(string) + sessionKey := sub + if loginMethod != "" { + sessionKey = loginMethod + ":" + sub + } + + raw, err := ts.MemoryStoreProvider.GetUserSession(sessionKey, refreshFamilyKeyPrefix+familyID) + require.NoError(t, err) + require.NotEmpty(t, raw) + var rec map[string]interface{} + require.NoError(t, json.Unmarshal([]byte(raw), &rec)) + rec["rotated_at"] = int64(1) // far in the past + out, err := json.Marshal(rec) + require.NoError(t, err) + // Keep it live in the store (far-future expiry) so the reuse path finds it. + require.NoError(t, ts.MemoryStoreProvider.SetUserSession( + sessionKey, refreshFamilyKeyPrefix+familyID, string(out), 1<<62)) +} + +// Item 2 (MEDIUM): --oauth21-strict must reject EVERY response_type that +// delivers a bearer access token into the URL fragment, including the +// front-channel-token hybrids "code token" and "code id_token token" — not +// only "token" / "id_token token". The check is component-exact so pure +// "id_token" (which contains "token" only as a substring) is unaffected. +func TestOAuth21_StrictMode_RejectsFrontChannelTokenHybrids(t *testing.T) { + cfg := getTestConfig() + ts := initTestSetup(t, cfg) + registerTestClient(t, ts, "hybrid-client", "hybrid-secret") + + authorize := func(responseType string) *httptest.ResponseRecorder { + router, cookie := mcpSession(t, ts, []string{"openid", "profile", "email"}) + qs := url.Values{} + qs.Set("client_id", "hybrid-client") + qs.Set("response_type", responseType) + qs.Set("redirect_uri", "http://localhost:3000/callback") + qs.Set("state", uuid.New().String()) + qs.Set("response_mode", "fragment") + qs.Set("nonce", uuid.New().String()) // required whenever id_token is present + qs.Set("scope", "openid profile email") + return doAuthorizeGET(router, qs, cookie) + } + + rejected := []string{"code token", "code id_token token", "token", "id_token token"} + for _, rt := range rejected { + t.Run("strict rejects "+rt, func(t *testing.T) { + ts.Config.OAuth21Strict = true + w := authorize(rt) + require.Equal(t, http.StatusFound, w.Code, "body: %s", w.Body.String()) + loc := w.Header().Get("Location") + assert.Contains(t, loc, "unsupported_response_type", "loc: %s", loc) + assert.NotContains(t, loc, "access_token=", "no access token may leak into the fragment: %s", loc) + }) + } + + // Pure id_token (contains "token" only as a substring) is NOT a + // front-channel access-token flow — strict mode must still allow it. + t.Run("strict allows pure id_token", func(t *testing.T) { + ts.Config.OAuth21Strict = true + w := authorize("id_token") + require.Equal(t, http.StatusFound, w.Code, "body: %s", w.Body.String()) + loc := w.Header().Get("Location") + assert.NotContains(t, loc, "unsupported_response_type", "pure id_token must not be rejected by strict mode: %s", loc) + }) + + ts.Config.OAuth21Strict = false +} + +// Item 3 (LOW): RFC 8707 §2 requires the resource indicator to be an absolute +// URI with no fragment. An invalid resource is rejected at /authorize with the +// RFC-conventional invalid_target error code. +func TestOAuth21_ResourceIndicator_Validation(t *testing.T) { + cfg := getTestConfig() + ts := initTestSetup(t, cfg) + registerTestClient(t, ts, "res-client", "res-secret") + + authorizeWithResource := func(resource string) *httptest.ResponseRecorder { + router, cookie := mcpSession(t, ts, []string{"openid", "profile", "email"}) + verifier := uuid.New().String() + uuid.New().String() + sum := sha256.Sum256([]byte(verifier)) + challenge := base64.RawURLEncoding.EncodeToString(sum[:]) + qs := url.Values{} + qs.Set("client_id", "res-client") + qs.Set("response_type", "code") + qs.Set("redirect_uri", "http://localhost:3000/callback") + qs.Set("code_challenge", challenge) + qs.Set("code_challenge_method", "S256") + qs.Set("state", uuid.New().String()) + qs.Set("response_mode", "query") + qs.Set("scope", "openid profile email") + qs.Set("resource", resource) + return doAuthorizeGET(router, qs, cookie) + } + + invalidTargetRejected := func(t *testing.T, w *httptest.ResponseRecorder) { + t.Helper() + require.Equal(t, http.StatusFound, w.Code, "body: %s", w.Body.String()) + loc, err := url.Parse(w.Header().Get("Location")) + require.NoError(t, err) + assert.Equal(t, "invalid_target", loc.Query().Get("error"), + "invalid resource must be rejected with invalid_target: %s", w.Header().Get("Location")) + } + + t.Run("relative reference rejected", func(t *testing.T) { + invalidTargetRejected(t, authorizeWithResource("/api/resource")) + }) + t.Run("non-URI string rejected", func(t *testing.T) { + invalidTargetRejected(t, authorizeWithResource("not-a-uri")) + }) + t.Run("fragment rejected", func(t *testing.T) { + invalidTargetRejected(t, authorizeWithResource("https://mcp.example.com/api#frag")) + }) + + t.Run("valid absolute URI accepted", func(t *testing.T) { + w := authorizeWithResource("https://mcp.example.com/api") + code := codeFromRedirect(t, w) // redirects with a code, not an error + require.NotEmpty(t, code) + }) +} + +// Item 4 (LOW/MEDIUM): RFC 8707 audience restriction must be enforced at +// Authorizer's OWN protected endpoints. A token minted for an external resource +// (aud = resource URI) must be rejected at /userinfo, while a normal +// client-bound token still works. Introspection keeps its own path and is +// unaffected (see /oauth/introspect — it uses ParseJWTToken directly). +func TestOAuth21_ResourceBoundToken_RejectedAtUserInfo(t *testing.T) { + cfg := getTestConfig() + ts := initTestSetup(t, cfg) + _, ctx := createContext(ts) + + email := signupUser(t, ts) + + // A resource-bound access token (aud = external resource URI). + resourceToken := issueAccessTokenWithResource(t, ts, ctx, email, "https://mcp.example.com") + assert.Equal(t, "https://mcp.example.com", decodeJWTPayloadUnverified(t, resourceToken)["aud"], + "sanity: token aud must be the resource") + code, _ := callUserInfo(t, ts, resourceToken) + assert.Equal(t, http.StatusUnauthorized, code, + "a resource-bound token must not authenticate at authorizer's own /userinfo") + + // A normal client-bound token (aud = default audience) still works. + clientToken := issueAccessTokenWithScopes(t, ts, ctx, email, []string{"openid", "profile", "email"}) + code2, body := callUserInfo(t, ts, clientToken) + assert.Equal(t, http.StatusOK, code2, "a client-bound token must still work at /userinfo: %v", body) +} + +// issueAccessTokenWithResource mints and persists an access token whose `aud` +// is the given RFC 8707 resource indicator, mirroring what /oauth/token does +// for a resource-bound exchange. +func issueAccessTokenWithResource(t *testing.T, ts *testSetup, ctx context.Context, email, resource string) string { + t.Helper() + user, err := ts.StorageProvider.GetUserByEmail(ctx, email) + require.NoError(t, err) + nonce := "nonce-" + uuid.New().String() + authToken, err := ts.TokenProvider.CreateAuthToken(nil, &token.AuthTokenConfig{ + User: user, + Roles: []string{"user"}, + Scope: []string{"openid", "profile", "email"}, + LoginMethod: constants.AuthRecipeMethodBasicAuth, + Nonce: nonce, + HostName: "http://localhost", + Resource: resource, + }) + require.NoError(t, err) + require.NotNil(t, authToken.AccessToken) + + sessionStoreKey := constants.AuthRecipeMethodBasicAuth + ":" + user.ID + require.NoError(t, ts.MemoryStoreProvider.SetUserSession( + sessionStoreKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, authToken.FingerPrintHash, authToken.SessionTokenExpiresAt)) + require.NoError(t, ts.MemoryStoreProvider.SetUserSession( + sessionStoreKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, authToken.AccessToken.Token, authToken.AccessToken.ExpiresAt)) + return authToken.AccessToken.Token +} diff --git a/internal/integration_tests/oauth21_mcp_test.go b/internal/integration_tests/oauth21_mcp_test.go index c23b3e64..efdf1dff 100644 --- a/internal/integration_tests/oauth21_mcp_test.go +++ b/internal/integration_tests/oauth21_mcp_test.go @@ -191,10 +191,14 @@ func TestOAuth21_ResourceIndicator_MismatchRejected(t *testing.T) { }) } -// Item 1: OAuth 2.1 refresh-token rotation with reuse invalidation. After a -// refresh rotates the token, replaying the OLD refresh token must be rejected -// invalid_grant AND trigger the breach response — the whole live session -// family is revoked, so the freshly-issued refresh token stops working too. +// Item 1: OAuth 2.1 refresh-token rotation with reuse invalidation, scoped to +// the compromised token's own lineage (family). Genuine reuse — replaying a +// stale token AFTER the chain has advanced past it, so it is not the immediate +// predecessor caught by the double-submit grace window — must be rejected +// invalid_grant AND revoke the family's live token. (That reuse revokes ONLY +// this family and not the user's other sessions is proved by +// TestOAuth21_RefreshTokenReuse_ScopedToFamily; the benign double-submit grace +// window by TestOAuth21_RefreshTokenReuse_GraceWindow.) func TestOAuth21_RefreshTokenReuse_RevokesFamily(t *testing.T) { cfg := getTestConfig() ts := initTestSetup(t, cfg) @@ -215,7 +219,6 @@ func TestOAuth21_RefreshTokenReuse_RevokesFamily(t *testing.T) { oldRefresh, _ := body["refresh_token"].(string) require.NotEmpty(t, oldRefresh, "offline_access scope must yield a refresh_token") - // Rotate once — a new refresh token is issued and the old one is retired. refresh := func(rt string) *httptest.ResponseRecorder { f := url.Values{} f.Set("grant_type", "refresh_token") @@ -223,29 +226,38 @@ func TestOAuth21_RefreshTokenReuse_RevokesFamily(t *testing.T) { return exchangeCode(router, f, []string{"reuse-client", "reuse-secret"}) } + // Rotate TWICE so oldRefresh is no longer the immediate predecessor of the + // live token — its replay is unambiguous theft-after-advance, not a race. w1 := refresh(oldRefresh) require.Equal(t, http.StatusOK, w1.Code, "first refresh body: %s", w1.Body.String()) var body1 map[string]interface{} require.NoError(t, json.Unmarshal(w1.Body.Bytes(), &body1)) - newRefresh, _ := body1["refresh_token"].(string) - require.NotEmpty(t, newRefresh) - require.NotEqual(t, oldRefresh, newRefresh, "rotation must mint a fresh refresh token") - - // Replay the OLD (already-rotated) refresh token: reuse detected. - w2 := refresh(oldRefresh) - assert.Equal(t, http.StatusBadRequest, w2.Code, "reuse body: %s", w2.Body.String()) + refresh2, _ := body1["refresh_token"].(string) + require.NotEmpty(t, refresh2) + require.NotEqual(t, oldRefresh, refresh2, "rotation must mint a fresh refresh token") + + w2 := refresh(refresh2) + require.Equal(t, http.StatusOK, w2.Code, "second refresh body: %s", w2.Body.String()) + var body2 map[string]interface{} + require.NoError(t, json.Unmarshal(w2.Body.Bytes(), &body2)) + liveRefresh, _ := body2["refresh_token"].(string) + require.NotEmpty(t, liveRefresh) + + // Replay the ORIGINAL (now two rotations stale) refresh token: genuine reuse. + w3 := refresh(oldRefresh) + assert.Equal(t, http.StatusBadRequest, w3.Code, "reuse body: %s", w3.Body.String()) var errBody map[string]interface{} - require.NoError(t, json.Unmarshal(w2.Body.Bytes(), &errBody)) + require.NoError(t, json.Unmarshal(w3.Body.Bytes(), &errBody)) assert.Equal(t, "invalid_grant", errBody["error"]) - // Breach response: the reuse revoked the whole family, so the token that - // was legitimately rotated in is now dead too. - w3 := refresh(newRefresh) - assert.Equal(t, http.StatusBadRequest, w3.Code, - "the live refresh token must be revoked after reuse of its rotated predecessor: %s", w3.Body.String()) - var errBody3 map[string]interface{} - require.NoError(t, json.Unmarshal(w3.Body.Bytes(), &errBody3)) - assert.Equal(t, "invalid_grant", errBody3["error"]) + // Breach response: the reuse revoked this family's live session, so the + // token that was legitimately rotated in is now dead too. + w4 := refresh(liveRefresh) + assert.Equal(t, http.StatusBadRequest, w4.Code, + "the live refresh token must be revoked after genuine reuse in its family: %s", w4.Body.String()) + var errBody4 map[string]interface{} + require.NoError(t, json.Unmarshal(w4.Body.Bytes(), &errBody4)) + assert.Equal(t, "invalid_grant", errBody4["error"]) } // Item 4: the --oauth21-strict flag. Default (false) leaves the implicit grant diff --git a/internal/token/auth_token.go b/internal/token/auth_token.go index 8f2f1bd2..21cc6ea3 100644 --- a/internal/token/auth_token.go +++ b/internal/token/auth_token.go @@ -7,11 +7,13 @@ import ( "encoding/json" "errors" "fmt" + "net/url" "strings" "time" "github.com/gin-gonic/gin" "github.com/golang-jwt/jwt/v4" + "github.com/google/uuid" "github.com/robertkrimen/otto" "github.com/authorizerdev/authorizer/internal/constants" @@ -56,6 +58,10 @@ var reservedClaims = map[string]bool{ // (RFC 9068) identifies the actor and is likewise reserved. "act": true, "client_id": true, + // family_id binds a refresh token to its rotation lineage for reuse + // detection (OAuth 2.1 §6.1). A script that could forge it would let a + // stolen token masquerade as a different family and dodge revocation. + "family_id": true, } // AuthTokenConfig is the configuration for auth token @@ -104,6 +110,16 @@ type AuthTokenConfig struct { // client too (RFC 6749 §6 client binding). Empty preserves existing // behavior — aud defaults to the requesting client / bootstrap client_id. Resource string + // RefreshTokenFamilyID is the stable identifier of the refresh-token + // lineage (family). It is established once at first issuance (a fresh UUID + // when empty) and carried forward UNCHANGED across every rotation of the + // same lineage — each rotation still gets a fresh Nonce, but the family_id + // stays constant. It scopes refresh-token-reuse revocation (OAuth 2.1 §6.1 / + // RFC 9700 §4.14.2) to the compromised lineage instead of the whole user, + // so replaying one retired token cannot force-log-out a user's other + // sessions/login-methods. CreateRefreshToken embeds it as the "family_id" + // claim. Empty on non-refresh paths (client_credentials / machine tokens). + RefreshTokenFamilyID string } // loginMethodToAMR maps an internal LoginMethod value to the OIDC Core §2 @@ -293,6 +309,14 @@ func (p *provider) CreateRefreshToken(cfg *AuthTokenConfig) (string, int64, erro if authTime == 0 { authTime = time.Now().Unix() } + // Refresh-token family (OAuth 2.1 §6.1 / RFC 9700 §4.14.2). Established once + // at first issuance (fresh UUID when the caller supplies none) and carried + // forward unchanged by the refresh grant across every rotation, so reuse + // detection can revoke exactly this lineage instead of the whole user. + familyID := cfg.RefreshTokenFamilyID + if familyID == "" { + familyID = uuid.NewString() + } customClaims := jwt.MapClaims{ "iss": cfg.HostName, "aud": p.audience(cfg), @@ -307,6 +331,7 @@ func (p *provider) CreateRefreshToken(cfg *AuthTokenConfig) (string, int64, erro "login_method": cfg.LoginMethod, "allowed_roles": strings.Split(cfg.User.Roles, ","), "client_id": cfg.ClientID, + "family_id": familyID, } token, err := p.SignJWTToken(customClaims) @@ -477,6 +502,23 @@ func (p *provider) ValidateAccessToken(gc *gin.Context, accessToken string) (map // the JWT signature) as the expected value; the checks above already // establish the token is a genuine, unexpired, unrevoked Authorizer token. aud, _ := res["aud"].(string) + + // RFC 8707 audience restriction. A token minted with a resource indicator + // carries that resource (an absolute URI, e.g. "https://mcp.example.com") as + // its `aud` so it is usable ONLY at that external resource server — which + // validates it via /oauth/introspect or JWKS, NOT here. This path guards + // Authorizer's OWN protected resources (/userinfo, GraphQL, gRPC). Accepting + // a resource-bound token here would defeat the audience restriction the token + // was issued with, so reject any `aud` that is an absolute URI (resource + // indicator form) other than the configured default audience. Legitimate + // client-bound tokens carry an opaque client_id `aud` (not a URI) and are + // unaffected. Introspection uses ParseJWTToken directly and is untouched. + if aud != "" && aud != p.config.ClientID { + if u, err := url.Parse(aud); err == nil && u.IsAbs() { + p.dependencies.Log.Debug().Str("aud", aud).Msg("access token rejected: resource-bound audience not valid at authorizer's own endpoints") + return res, fmt.Errorf(`unauthorized: token audience is a resource indicator`) + } + } hostname := parsers.GetHost(gc) if ok, err := p.ValidateJWTClaims(res, &AuthTokenConfig{ HostName: hostname, From 9679bea578d83e1a64c3e216e9e489eccb88111e Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Tue, 21 Jul 2026 17:21:42 +0530 Subject: [PATCH 3/4] chore(oauth): rename --oauth21-strict flag to --oauth2-1-strict "oauth21" reads ambiguously (looks like a made-up version 21); the dash between "2" and "1" makes it unambiguously OAuth 2.1. Go field name (OAuth21Strict) and test file names are unaffected - Go identifiers can't carry dots/hyphens anyway. --- cmd/root.go | 2 +- internal/integration_tests/oauth21_hardening_test.go | 2 +- internal/integration_tests/oauth21_mcp_test.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index 5e205aa8..79f97720 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -133,7 +133,7 @@ func init() { f.StringVar(&rootArgs.config.ClientSecret, "client-secret", "", "Client secret for the OAuth") f.StringVar(&rootArgs.config.DefaultAuthorizeResponseMode, "default-authorize-response-mode", constants.ResponseModeQuery, "Default response mode for the authorize endpoint") f.StringVar(&rootArgs.config.DefaultAuthorizeResponseType, "default-authorize-response-type", constants.ResponseTypeToken, "Default response type for the authorize endpoint") - f.BoolVar(&rootArgs.config.OAuth21Strict, "oauth21-strict", false, "Enforce OAuth 2.1 restrictions: reject the implicit/hybrid-with-token response types (token, id_token token) and PKCE plain (require S256). Breaking; opt-in") + f.BoolVar(&rootArgs.config.OAuth21Strict, "oauth2-1-strict", false, "Enforce OAuth 2.1 restrictions: reject the implicit/hybrid-with-token response types (token, id_token token) and PKCE plain (require S256). Breaking; opt-in") // Admin flags f.StringVar(&rootArgs.config.AdminSecret, "admin-secret", "", "Secret for the admin (REQUIRED, must not be empty)") diff --git a/internal/integration_tests/oauth21_hardening_test.go b/internal/integration_tests/oauth21_hardening_test.go index 717655f2..6d2d22bf 100644 --- a/internal/integration_tests/oauth21_hardening_test.go +++ b/internal/integration_tests/oauth21_hardening_test.go @@ -248,7 +248,7 @@ func ageFamilyRecordPast(t *testing.T, ts *testSetup, liveRefresh string) { sessionKey, refreshFamilyKeyPrefix+familyID, string(out), 1<<62)) } -// Item 2 (MEDIUM): --oauth21-strict must reject EVERY response_type that +// Item 2 (MEDIUM): --oauth2-1-strict must reject EVERY response_type that // delivers a bearer access token into the URL fragment, including the // front-channel-token hybrids "code token" and "code id_token token" — not // only "token" / "id_token token". The check is component-exact so pure diff --git a/internal/integration_tests/oauth21_mcp_test.go b/internal/integration_tests/oauth21_mcp_test.go index efdf1dff..9a5d8c96 100644 --- a/internal/integration_tests/oauth21_mcp_test.go +++ b/internal/integration_tests/oauth21_mcp_test.go @@ -260,7 +260,7 @@ func TestOAuth21_RefreshTokenReuse_RevokesFamily(t *testing.T) { assert.Equal(t, "invalid_grant", errBody4["error"]) } -// Item 4: the --oauth21-strict flag. Default (false) leaves the implicit grant +// Item 4: the --oauth2-1-strict flag. Default (false) leaves the implicit grant // and PKCE "plain" working exactly as today; true rejects both. func TestOAuth21_StrictMode_Gating(t *testing.T) { cfg := getTestConfig() From 0a741abb70853b29f4175a5f8298ba0c1eb2f98f Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Tue, 21 Jul 2026 20:01:09 +0530 Subject: [PATCH 4/4] security(oauth): don't revoke on a transient live-token lookup failure Found in a fresh adversarial re-review of the family-scoped reuse fix. handleRefreshTokenReuse couldn't tell "this nonce was genuinely rotated away" from "the store had a transient blip looking up the CURRENT live token's own session entry" - GetUserSession collapses both to a plain error. Without a guard, a transient lookup failure on a still-legitimate live token fell straight into the genuine-reuse branch and revoked the user's own session over what should have been a retryable fault. Fix: if the presented nonce equals the family record's LiveNonce, this can't be a rotated-away token by definition - skip revocation and let it fail as an ordinary invalid_grant instead. Regression test simulates the failure by expiring just the refresh_token_ store entry (not via DeleteUserSession, which cascades to the sibling session/access token entries for that nonce and would mask the bug) and asserts the sibling access_token_ entry survives. Verified the test actually catches the regression: temporarily disabled the guard, confirmed the test fails (access token wiped), restored it. --- internal/http_handlers/token.go | 14 ++++ .../oauth21_hardening_test.go | 64 +++++++++++++++++++ 2 files changed, 78 insertions(+) diff --git a/internal/http_handlers/token.go b/internal/http_handlers/token.go index ace475ae..c36198fd 100644 --- a/internal/http_handlers/token.go +++ b/internal/http_handlers/token.go @@ -150,6 +150,20 @@ func (h *httpProvider) handleRefreshTokenReuse(gc *gin.Context, claims map[strin return } + // The presented nonce IS the family's current live token: this is not a + // rotated-away token being replayed at all — ValidateRefreshToken's own + // session lookup for this exact live nonce must have hit a transient store + // error (a timeout, a momentary blip) rather than a genuine "key absent" + // (the store layer can't distinguish the two, see GetUserSession). Treat it + // as an ordinary failed validation, not a breach: revoking here would nuke + // the user's own still-legitimate session over a retryable fault, which is + // strictly worse than the plain invalid_grant the caller already returns. + if replayedNonce == rec.LiveNonce { + metrics.RecordSecurityEvent("refresh_token_reuse_transient", "token_endpoint") + log.Debug().Msg("presented nonce matches the family's live token — likely a transient session-store error, not reuse; not revoking") + return + } + // Benign double-submit grace window: only when the replayed token is the // IMMEDIATE predecessor of the current live token (rec.PrevNonce) AND the // rotation was within the window. An immediate-predecessor replay means the diff --git a/internal/integration_tests/oauth21_hardening_test.go b/internal/integration_tests/oauth21_hardening_test.go index 6d2d22bf..fcfac26b 100644 --- a/internal/integration_tests/oauth21_hardening_test.go +++ b/internal/integration_tests/oauth21_hardening_test.go @@ -248,6 +248,70 @@ func ageFamilyRecordPast(t *testing.T, ts *testSetup, liveRefresh string) { sessionKey, refreshFamilyKeyPrefix+familyID, string(out), 1<<62)) } +// A transient failure looking up the CURRENT live refresh token's own session +// record (a store timeout/blip — GetUserSession cannot distinguish that from +// "key genuinely absent") must NOT be treated as reuse. Simulated here by +// deleting the live token's session entry directly (without a second +// rotation), so the presented nonce still equals the family record's +// LiveNonce — the exact signal handleRefreshTokenReuse uses to recognize +// "this is not a rotated-away token, something else went wrong" and skip +// revocation. +// +// One real rotation happens first and is load-bearing for the test, not +// incidental: a family record only exists once a refresh has actually +// happened (the initial authorization_code exchange writes none), so the +// LiveNonce-guard path can't be reached at all without it. +func TestOAuth21_RefreshTokenReuse_TransientLookupFailureNotRevoked(t *testing.T) { + cfg := getTestConfig() + ts := initTestSetup(t, cfg) + registerTestClient(t, ts, "transient-client", "transient-secret") + basic := []string{"transient-client", "transient-secret"} + + email := signupUser(t, ts) + router, code, verifier := offlineAccessSessionForEmail(t, ts, "transient-client", email) + original := exchangeForRefresh(t, router, code, verifier, basic) + live := refreshOK(t, router, original, basic) // rotate once: establishes the family record + + claims := decodeJWTPayloadUnverified(t, live) + nonce, _ := claims["nonce"].(string) + sub, _ := claims["sub"].(string) + loginMethod, _ := claims["login_method"].(string) + require.NotEmpty(t, nonce) + sessionKey := sub + if loginMethod != "" { + sessionKey = loginMethod + ":" + sub + } + + // Simulate the transient blip: ONLY the live token's own refresh_token_ + // session entry vanishes out from under it (a store timeout mid-lookup) — + // nothing rotated, so this is still the family's LiveNonce. Overwrite just + // that one sub-key's expiry to the past (the db-backed store's + // GetUserSession treats an expired row as "not found" and deletes it on + // read) rather than DeleteUserSession, which cascades to session_token_/ + // access_token_/refresh_token_ TOGETHER for the nonce and would erase the + // very sibling entry this test needs intact to prove the point below. + refreshTokenKey := "refresh_token_" + nonce + accessTokenKey := "access_token_" + nonce + require.NoError(t, ts.MemoryStoreProvider.SetUserSession(sessionKey, refreshTokenKey, live, 1)) + preAccess, err := ts.MemoryStoreProvider.GetUserSession(sessionKey, accessTokenKey) + require.NoError(t, err) + require.NotEmpty(t, preAccess, "test setup: the access token entry must still be present before the replay") + + // Presenting the (now-orphaned-but-still-live-per-the-family-record) token + // fails validation as an ordinary invalid_grant... + replay := doRefresh(router, live, basic) + assert.Equal(t, http.StatusBadRequest, replay.Code, "replay body: %s", replay.Body.String()) + + // ...but must NOT have been treated as a breach. If handleRefreshTokenReuse + // incorrectly ran the genuine-reuse branch, DeleteUserSession(sessionKey, + // rec.LiveNonce) would have wiped this SAME nonce's access token too — even + // though nothing about it actually rotated or was stolen. + postAccess, err := ts.MemoryStoreProvider.GetUserSession(sessionKey, accessTokenKey) + require.NoError(t, err) + assert.NotEmpty(t, postAccess, + "a transient lookup failure on the live token must not revoke the live session (access token entry was wiped)") +} + // Item 2 (MEDIUM): --oauth2-1-strict must reject EVERY response_type that // delivers a bearer access token into the URL fragment, including the // front-channel-token hybrids "code token" and "code id_token token" — not