From 25c3dfec52adab9e844e4404e77119b9813775ca Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Fri, 7 Aug 2026 08:28:20 +0530 Subject: [PATCH 1/9] security: bind oauth state to browser and codes to clients Audit findings AUDIT-06 and AUDIT-07. AUDIT-06 login CSRF: `state` was server-generated and stored globally, so the callback could only prove SOME flow issued it, never that THIS browser did. An attacker harvests their own valid code+state and delivers it to a victim, logging the victim into the ATTACKER's account; anything the victim then enters lands in an account the attacker controls (RFC 9700 4.7). A host-only HttpOnly cookie now binds the flow to the browser that started it. SameSite is None when Secure, matching the MFA cookie: Apple returns its callback as a cross-site form_post and a Lax cookie is not sent on one. The cookie is read back url-unescaped because gin escapes on write and nothing reverses it on read - the state contains "://" and spaces. AUDIT-07 codes were bound to a redirect_uri but not to an identity, so two confidential clients sharing a redirect origin could redeem each other's codes (RFC 6749 4.1.3). The token endpoint now compares the code's client against the AUTHENTICATED client, not the raw body field - clients may authenticate via Basic or a client assertion and send no body client_id. Both required adding a field to a positional "@@"-delimited blob that was hand-built and hand-parsed at a dozen sites across http_handlers and service. internal/codestate now owns both formats. Decoding is length- guarded per field, so codes minted by an older build stay redeemable across a deploy. --- e2e-playground/tests/social/apple.spec.ts | 1 + e2e-playground/tests/social/discord.spec.ts | 2 +- internal/codestate/codestate.go | 142 ++++++++++++++++++ internal/codestate/codestate_test.go | 106 +++++++++++++ internal/constants/cookie.go | 3 + internal/cookie/oauth_state.go | 89 +++++++++++ internal/http_handlers/authorize.go | 27 +++- .../http_handlers/oauth_authorize_state.go | 34 ++--- .../oauth_authorize_state_test.go | 14 +- internal/http_handlers/oauth_callback.go | 30 +++- internal/http_handlers/oauth_login.go | 5 + internal/http_handlers/token.go | 35 ++++- .../auth_code_client_binding_test.go | 64 ++++++++ internal/service/auth_response.go | 34 +++-- internal/service/login.go | 35 +++-- internal/service/session.go | 35 +++-- internal/service/signup.go | 34 +++-- 17 files changed, 582 insertions(+), 108 deletions(-) create mode 100644 internal/codestate/codestate.go create mode 100644 internal/codestate/codestate_test.go create mode 100644 internal/cookie/oauth_state.go create mode 100644 internal/integration_tests/auth_code_client_binding_test.go diff --git a/e2e-playground/tests/social/apple.spec.ts b/e2e-playground/tests/social/apple.spec.ts index 7ad4b8461..5f8b02eb0 100644 --- a/e2e-playground/tests/social/apple.spec.ts +++ b/e2e-playground/tests/social/apple.spec.ts @@ -76,6 +76,7 @@ test.describe('Social login — Apple', () => { await configureProviderProfile(request, 'apple', { sub: `apple-${crypto.randomUUID()}`, email, + email_verified: true, omit_user_field: true, }); await page.getByRole('button', { name: /apple/i }).click(); diff --git a/e2e-playground/tests/social/discord.spec.ts b/e2e-playground/tests/social/discord.spec.ts index 4426fb28b..0d65e94a4 100644 --- a/e2e-playground/tests/social/discord.spec.ts +++ b/e2e-playground/tests/social/discord.spec.ts @@ -50,7 +50,7 @@ test.describe('Social login — Discord', () => { // correctly with no synthetic-email machinery needed (unlike Twitter, // which never gets a real email at all). const email = `discord-repeat-${crypto.randomUUID()}@example.com`; - const profile = { id: `discord-stable-${crypto.randomUUID()}`, username: 'gracehopper', avatar: 'def456', email }; + const profile = { id: `discord-stable-${crypto.randomUUID()}`, username: 'gracehopper', avatar: 'def456', email, verified: true }; // First login (fresh browser context = `page`/`request` from the test // fixture): creates the account. diff --git a/internal/codestate/codestate.go b/internal/codestate/codestate.go new file mode 100644 index 000000000..a4476ef19 --- /dev/null +++ b/internal/codestate/codestate.go @@ -0,0 +1,142 @@ +// Package codestate owns the encoding of the two positional, "@@"-delimited +// blobs the authorization-code flow persists in the memory store. +// +// Both formats were previously built and parsed by hand at a dozen call sites +// across internal/http_handlers and internal/service — every one an independent +// chance to put a field in the wrong slot, forget a url.QueryEscape, or add a +// segment to some producers but not others. Adding the client-id binding +// (RFC 6749 §4.1.3) meant touching all of them at once, so the format now has +// exactly one owner. +// +// # Backward compatibility +// +// Decoding is length-guarded field by field, so a blob written by an older +// build (four or five segments, no client id) still decodes — the missing +// fields come back empty and their checks are skipped. Codes issued before a +// deploy therefore remain redeemable across it. Never renumber a slot; only +// append. +package codestate + +import ( + "net/url" + "strings" +) + +const delimiter = "@@" + +// Code is the state persisted under an authorization code and consumed by the +// token endpoint. +type Code struct { + // Challenge is the PKCE code_challenge, suffixed "::" when present. + Challenge string + // Session is the session token / fingerprint hash minted at authorize time. + Session string + // Nonce is the OIDC nonce from the /authorize request. + Nonce string + // RedirectURI is the redirect_uri from /authorize (RFC 6749 §4.1.3). + RedirectURI string + // Resource is the RFC 8707 resource indicator bound at /authorize. + Resource string + // ClientID is the client the code was issued to. RFC 6749 §4.1.3 requires + // the token endpoint to "ensure that the authorization code was issued to + // the authenticated confidential client"; without it, two clients sharing a + // redirect origin can redeem each other's codes. + ClientID string +} + +// EncodeCode serialises the code state. Every free-text field is escaped so a +// value containing the delimiter cannot shift the fields after it. +func EncodeCode(c Code) string { + return strings.Join([]string{ + c.Challenge, + c.Session, + c.Nonce, + url.QueryEscape(c.RedirectURI), + url.QueryEscape(c.Resource), + url.QueryEscape(c.ClientID), + }, delimiter) +} + +// DecodeCode parses the code state, tolerating blobs written by older builds +// that carry fewer segments. +func DecodeCode(raw string) Code { + parts := strings.Split(raw, delimiter) + c := Code{} + if len(parts) > 0 { + c.Challenge = parts[0] + } + if len(parts) > 1 { + c.Session = parts[1] + } + if len(parts) > 2 { + c.Nonce = parts[2] + } + if len(parts) > 3 { + c.RedirectURI, _ = url.QueryUnescape(parts[3]) + } + if len(parts) > 4 { + c.Resource, _ = url.QueryUnescape(parts[4]) + } + if len(parts) > 5 { + c.ClientID, _ = url.QueryUnescape(parts[5]) + } + return c +} + +// Authorize is the state persisted under the `state` parameter while an +// /authorize request detours through a login, signup or social-provider round +// trip. Whatever completes that detour rebinds these onto the code state. +type Authorize struct { + Code string + Challenge string + Nonce string + RedirectURI string + Resource string + ClientID string +} + +// EncodeAuthorize serialises the authorize-detour state. +func EncodeAuthorize(a Authorize) string { + return strings.Join([]string{ + a.Code, + a.Challenge, + a.Nonce, + url.QueryEscape(a.RedirectURI), + url.QueryEscape(a.Resource), + url.QueryEscape(a.ClientID), + }, delimiter) +} + +// DecodeAuthorize parses the authorize-detour state. +// +// A blob with no delimiter at all is the legacy "nonce only" form written when +// the request had no code flow; callers detect that via HasCode. +func DecodeAuthorize(raw string) Authorize { + parts := strings.Split(raw, delimiter) + a := Authorize{} + if len(parts) < 2 { + a.Nonce = raw + return a + } + a.Code = parts[0] + a.Challenge = parts[1] + if len(parts) > 2 { + a.Nonce = parts[2] + } + if len(parts) > 3 { + a.RedirectURI, _ = url.QueryUnescape(parts[3]) + } + if len(parts) > 4 { + a.Resource, _ = url.QueryUnescape(parts[4]) + } + if len(parts) > 5 { + a.ClientID, _ = url.QueryUnescape(parts[5]) + } + return a +} + +// HasCode reports whether the blob carried a code flow (as opposed to the +// bare-nonce form). +func HasCode(raw string) bool { + return strings.Contains(raw, delimiter) +} diff --git a/internal/codestate/codestate_test.go b/internal/codestate/codestate_test.go new file mode 100644 index 000000000..325c4efc5 --- /dev/null +++ b/internal/codestate/codestate_test.go @@ -0,0 +1,106 @@ +package codestate + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestCodeRoundTrip pins the field order. Every slot is positional, so a +// renumbering would silently move a redirect_uri into the resource check (or a +// client id into nothing at all) rather than fail to compile. +func TestCodeRoundTrip(t *testing.T) { + t.Parallel() + in := Code{ + Challenge: "abc123::S256", + Session: "session-fingerprint", + Nonce: "nonce-value", + RedirectURI: "https://app.example.com/cb?x=1&y=2", + Resource: "https://api.example.com/v1", + ClientID: "client-a", + } + got := DecodeCode(EncodeCode(in)) + assert.Equal(t, in, got) +} + +// TestCodeDelimiterInValues is the reason every free-text field is escaped: a +// redirect_uri containing the delimiter would otherwise shift every field after +// it, so a crafted redirect could push an attacker-chosen client id into the +// slot the token endpoint compares against. +func TestCodeDelimiterInValues(t *testing.T) { + t.Parallel() + in := Code{ + Challenge: "c", + Session: "s", + Nonce: "n", + RedirectURI: "https://evil.example.com/cb?a=@@b", + Resource: "https://api.example.com/@@x", + ClientID: "client-a", + } + got := DecodeCode(EncodeCode(in)) + assert.Equal(t, in.RedirectURI, got.RedirectURI) + assert.Equal(t, in.Resource, got.Resource) + assert.Equal(t, "client-a", got.ClientID, "a delimiter in an earlier field must not shift the client id") +} + +// TestDecodeCodeLegacyBlobs is the upgrade guarantee: authorization codes minted +// by an older build are still in the store when a new binary starts, and must +// stay redeemable. Missing trailing fields decode empty, and an empty ClientID +// means the token endpoint skips the binding check rather than rejecting. +func TestDecodeCodeLegacyBlobs(t *testing.T) { + t.Parallel() + + t.Run("four segments (pre-resource, pre-client-id)", func(t *testing.T) { + t.Parallel() + got := DecodeCode("chal::S256@@sess@@nonce@@https%3A%2F%2Fapp.example.com%2Fcb") + assert.Equal(t, "chal::S256", got.Challenge) + assert.Equal(t, "sess", got.Session) + assert.Equal(t, "nonce", got.Nonce) + assert.Equal(t, "https://app.example.com/cb", got.RedirectURI) + assert.Empty(t, got.Resource) + assert.Empty(t, got.ClientID, "an old code carries no client binding, so the check must be skipped") + }) + + t.Run("five segments (pre-client-id)", func(t *testing.T) { + t.Parallel() + got := DecodeCode("@@sess@@nonce@@https%3A%2F%2Fapp.example.com%2Fcb@@https%3A%2F%2Fapi.example.com") + assert.Empty(t, got.Challenge, "no PKCE is a valid state") + assert.Equal(t, "https://api.example.com", got.Resource) + assert.Empty(t, got.ClientID) + }) +} + +func TestAuthorizeRoundTrip(t *testing.T) { + t.Parallel() + in := Authorize{ + Code: "the-code", + Challenge: "chal::S256", + Nonce: "nonce", + RedirectURI: "https://app.example.com/cb", + Resource: "https://api.example.com", + ClientID: "client-a", + } + assert.Equal(t, in, DecodeAuthorize(EncodeAuthorize(in))) +} + +// TestDecodeAuthorizeBareNonce covers the no-code-flow form, which /authorize +// writes as a naked nonce with no delimiter at all. +func TestDecodeAuthorizeBareNonce(t *testing.T) { + t.Parallel() + require.False(t, HasCode("just-a-nonce")) + got := DecodeAuthorize("just-a-nonce") + assert.Equal(t, "just-a-nonce", got.Nonce) + assert.Empty(t, got.Code) + assert.Empty(t, got.ClientID) +} + +func TestDecodeAuthorizeLegacyBlob(t *testing.T) { + t.Parallel() + // Five segments: an in-flight /authorize detour started before the upgrade. + got := DecodeAuthorize("code@@chal@@nonce@@https%3A%2F%2Fapp.example.com%2Fcb@@") + assert.True(t, HasCode("code@@chal@@nonce@@x@@")) + assert.Equal(t, "code", got.Code) + assert.Equal(t, "https://app.example.com/cb", got.RedirectURI) + assert.Empty(t, got.ClientID) +} diff --git a/internal/constants/cookie.go b/internal/constants/cookie.go index 8f6399bae..7645c0e57 100644 --- a/internal/constants/cookie.go +++ b/internal/constants/cookie.go @@ -5,6 +5,9 @@ const ( AppCookieName = "cookie" // AdminCookieName is the name of the cookie that is used to store the admin token AdminCookieName = "authorizer-admin" + // OAuthStateCookieName is the name of the cookie binding an in-flight social + // login to the browser that started it. See internal/cookie/oauth_state.go. + OAuthStateCookieName = "authorizer-oauth-state" // MfaCookieName is the name of the cookie that is used to store the mfa session MfaCookieName = "mfa" ) diff --git a/internal/cookie/oauth_state.go b/internal/cookie/oauth_state.go new file mode 100644 index 000000000..cb48aaeed --- /dev/null +++ b/internal/cookie/oauth_state.go @@ -0,0 +1,89 @@ +package cookie + +import ( + "net/http" + "net/url" + + "github.com/gin-gonic/gin" + + "github.com/authorizerdev/authorizer/internal/constants" +) + +// oauthStateCookieMaxAge bounds how long a half-finished social login stays +// resumable. Long enough for a slow consent screen, short enough that a stale +// binding does not linger. +const oauthStateCookieMaxAge = 15 * 60 + +// SetOAuthState binds an in-flight social login to the browser that started it. +// +// The `state` parameter alone does not do this: it is generated server-side and +// stored globally, so the callback could only ever check that SOME flow issued +// it, not that THIS browser did. That gap is login CSRF (RFC 9700 §4.7) — an +// attacker starts a flow, harvests their own valid code+state, and delivers it +// to a victim's browser, silently logging the victim into the ATTACKER's +// account. Anything the victim then does (saved addresses, payment details, +// uploaded documents) lands in an account the attacker controls. +// +// SameSite is None when the cookie is Secure, matching the MFA session cookie: +// Apple returns its callback as a cross-site form_post (the router accepts POST +// on /oauth_callback for exactly this), and a Lax cookie is not sent on a +// cross-site POST — Apple logins would break. The binding property does not +// depend on SameSite: an attacker cannot write a cookie into the victim's +// browser for our origin, whatever the SameSite value. +func SetOAuthState(gc *gin.Context, state string, appCookieSecure bool) { + c := BuildOAuthStateCookie("", state, appCookieSecure) + gc.SetSameSite(c.SameSite) + gc.SetCookie(c.Name, c.Value, c.MaxAge, c.Path, c.Domain, c.Secure, c.HttpOnly) +} + +// BuildOAuthStateCookie returns the state-binding cookie. Host-scoped +// deliberately: the callback runs on this exact host, so there is no reason to +// widen the cookie to sibling subdomains. +func BuildOAuthStateCookie(_ string, state string, appCookieSecure bool) *http.Cookie { + sameSite := http.SameSiteLaxMode + if appCookieSecure { + sameSite = http.SameSiteNoneMode + } + return &http.Cookie{ + Name: constants.OAuthStateCookieName, + Value: state, + // Deliberately no Domain: a host-only cookie. The callback runs on the + // exact host that set this, so widening to sibling subdomains buys + // nothing and leaks the binding to them. It also sidesteps the cases + // where a Domain attribute is silently dropped and the cookie never + // comes back at all — single-label hosts (`authorizer` in the compose + // network) and bare IPs both hit that. + MaxAge: oauthStateCookieMaxAge, + Path: "/", + Secure: appCookieSecure, + HttpOnly: true, + SameSite: sameSite, + } +} + +// GetOAuthState reads the state-binding cookie off the request. +// +// Unescaped on the way out because gin's SetCookie URL-escapes on the way in +// and nothing reverses it on read — the state contains "://" and spaces, so a +// raw comparison against the state parameter never matches. GetAdminCookie +// handles the same asymmetry for the same reason. +func GetOAuthState(gc *gin.Context) string { + c, err := gc.Request.Cookie(constants.OAuthStateCookieName) + if err != nil { + return "" + } + decoded, err := url.QueryUnescape(c.Value) + if err != nil { + return "" + } + return decoded +} + +// DeleteOAuthState clears the binding once the flow completes, so a single +// browser cannot replay a spent state. +func DeleteOAuthState(gc *gin.Context, appCookieSecure bool) { + c := BuildOAuthStateCookie("", "", appCookieSecure) + c.MaxAge = -1 + gc.SetSameSite(c.SameSite) + gc.SetCookie(c.Name, c.Value, c.MaxAge, c.Path, c.Domain, c.Secure, c.HttpOnly) +} diff --git a/internal/http_handlers/authorize.go b/internal/http_handlers/authorize.go index a68137d96..d2a435097 100644 --- a/internal/http_handlers/authorize.go +++ b/internal/http_handlers/authorize.go @@ -47,6 +47,7 @@ import ( "github.com/gin-gonic/gin" "github.com/google/uuid" + "github.com/authorizerdev/authorizer/internal/codestate" "github.com/authorizerdev/authorizer/internal/constants" "github.com/authorizerdev/authorizer/internal/cookie" "github.com/authorizerdev/authorizer/internal/crypto" @@ -401,7 +402,14 @@ func (h *httpProvider) AuthorizeHandler() gin.HandlerFunc { // [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 { + if err := h.MemoryStoreProvider.SetState(state, codestate.EncodeAuthorize(codestate.Authorize{ + Code: code, + Challenge: challengeData, + Nonce: nonce, + RedirectURI: redirectURI, + Resource: resource, + ClientID: clientID, + })); err != nil { log.Debug().Err(err).Msg("Error setting temp code") gc.JSON(http.StatusInternalServerError, gin.H{"error": "internal server error"}) return @@ -625,7 +633,13 @@ func (h *httpProvider) AuthorizeHandler() gin.HandlerFunc { if codeChallenge != "" { hybridChallengeData = codeChallenge + "::" + codeChallengeMethod } - if err := h.MemoryStoreProvider.SetState(code, hybridChallengeData+"@@"+authToken.FingerPrintHash+"@@"+nonce+"@@"+url.QueryEscape(redirectURI)); err != nil { + if err := h.MemoryStoreProvider.SetState(code, codestate.EncodeCode(codestate.Code{ + Challenge: hybridChallengeData, + Session: authToken.FingerPrintHash, + Nonce: nonce, + RedirectURI: redirectURI, + ClientID: clientID, + })); err != nil { log.Debug().Err(err).Msg("Error setting temp code for hybrid") handleResponse(gc, responseMode, authURL, redirectURI, loginError, http.StatusOK) return @@ -787,7 +801,14 @@ func (h *httpProvider) AuthorizeHandler() gin.HandlerFunc { // [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 { + if err := h.MemoryStoreProvider.SetState(code, codestate.EncodeCode(codestate.Code{ + Challenge: codeChallengeData, + Session: newSessionToken, + Nonce: nonce, + RedirectURI: redirectURI, + Resource: resource, + ClientID: clientID, + })); 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/oauth_authorize_state.go b/internal/http_handlers/oauth_authorize_state.go index f09bba401..0c7fc1950 100644 --- a/internal/http_handlers/oauth_authorize_state.go +++ b/internal/http_handlers/oauth_authorize_state.go @@ -1,8 +1,7 @@ package http_handlers import ( - "net/url" - "strings" + "github.com/authorizerdev/authorizer/internal/codestate" ) // consumeAuthorizeState resolves the OpenID Connect `/authorize` state (stateValue) into either: @@ -12,34 +11,21 @@ import ( // It is a best-effort bridge used by the social OAuth callback: // - For standalone social login (`/oauth_login/:provider`) there is no `/authorize` entry, so it returns empty values. // - For OIDC authorize flows, it consumes the entry to keep it single-use. -func (h *httpProvider) consumeAuthorizeState(stateValue string) (code, codeChallenge, nonce, redirectURI string, err error) { +func (h *httpProvider) consumeAuthorizeState(stateValue string) (code, codeChallenge, nonce, redirectURI, clientID string, err error) { if stateValue == "" { - return "", "", "", "", nil + return "", "", "", "", "", nil } authorizeState, err := h.MemoryStoreProvider.GetAndRemoveState(stateValue) if err != nil || authorizeState == "" { - return "", "", "", "", err + return "", "", "", "", "", err } - authorizeStateSplit := strings.Split(authorizeState, "@@") - if len(authorizeStateSplit) > 1 { - code = authorizeStateSplit[0] - codeChallenge = authorizeStateSplit[1] - // Third part carries the OIDC nonce from the /authorize request. - if len(authorizeStateSplit) > 2 { - nonce = authorizeStateSplit[2] - } - // Fourth part carries the URL-encoded redirect_uri from the /authorize - // request for RFC 6749 §4.1.3 validation at the token endpoint. - // It is URL-encoded to prevent the @@ delimiter from being confused - // with @@ characters that may appear in the redirect_uri. - if len(authorizeStateSplit) > 3 { - redirectURI, _ = url.QueryUnescape(authorizeStateSplit[3]) - } - } else { - nonce = authorizeState + // One owner for this positional format — see internal/codestate. A blob + // written by an older build simply decodes with the trailing fields empty. + if !codestate.HasCode(authorizeState) { + return "", "", authorizeState, "", "", nil } - - return code, codeChallenge, nonce, redirectURI, nil + as := codestate.DecodeAuthorize(authorizeState) + return as.Code, as.Challenge, as.Nonce, as.RedirectURI, as.ClientID, nil } diff --git a/internal/http_handlers/oauth_authorize_state_test.go b/internal/http_handlers/oauth_authorize_state_test.go index 5f354d842..07b36b367 100644 --- a/internal/http_handlers/oauth_authorize_state_test.go +++ b/internal/http_handlers/oauth_authorize_state_test.go @@ -28,7 +28,7 @@ func TestConsumeAuthorizeState_Nonce(t *testing.T) { stateValue := "state-1" require.NoError(t, ms.SetState(stateValue, "nonce-123")) - code, codeChallenge, nonce, redirectURI, err := h.consumeAuthorizeState(stateValue) + code, codeChallenge, nonce, redirectURI, _, err := h.consumeAuthorizeState(stateValue) require.NoError(t, err) require.Empty(t, code) require.Empty(t, codeChallenge) @@ -58,7 +58,7 @@ func TestConsumeAuthorizeState_CodeAndPKCE(t *testing.T) { stateValue := "state-2" require.NoError(t, ms.SetState(stateValue, "code-abc@@challenge-xyz")) - code, codeChallenge, nonce, redirectURI, err := h.consumeAuthorizeState(stateValue) + code, codeChallenge, nonce, redirectURI, _, err := h.consumeAuthorizeState(stateValue) require.NoError(t, err) require.Equal(t, "code-abc", code) require.Equal(t, "challenge-xyz", codeChallenge) @@ -88,7 +88,7 @@ func TestConsumeAuthorizeState_CodePKCEAndNonce(t *testing.T) { stateValue := "state-3" require.NoError(t, ms.SetState(stateValue, "code-abc@@challenge-xyz@@oidc-nonce-123")) - code, codeChallenge, nonce, redirectURI, err := h.consumeAuthorizeState(stateValue) + code, codeChallenge, nonce, redirectURI, _, err := h.consumeAuthorizeState(stateValue) require.NoError(t, err) require.Equal(t, "code-abc", code) require.Equal(t, "challenge-xyz", codeChallenge) @@ -119,7 +119,7 @@ func TestConsumeAuthorizeState_CodePKCENonceAndRedirectURI(t *testing.T) { stateValue := "state-4" require.NoError(t, ms.SetState(stateValue, "code-abc@@challenge-xyz@@oidc-nonce-123@@https%3A%2F%2Fexample.com%2Fcallback")) - code, codeChallenge, nonce, redirectURI, err := h.consumeAuthorizeState(stateValue) + code, codeChallenge, nonce, redirectURI, _, err := h.consumeAuthorizeState(stateValue) require.NoError(t, err) require.Equal(t, "code-abc", code) require.Equal(t, "challenge-xyz", codeChallenge) @@ -152,7 +152,7 @@ func TestConsumeAuthorizeState_RedirectURIWithDelimiter(t *testing.T) { stateValue := "state-5" require.NoError(t, ms.SetState(stateValue, "code-abc@@challenge-xyz@@nonce-123@@https%3A%2F%2Fevil.com%2F%40%40injected")) - code, codeChallenge, nonce, redirectURI, err := h.consumeAuthorizeState(stateValue) + code, codeChallenge, nonce, redirectURI, _, err := h.consumeAuthorizeState(stateValue) require.NoError(t, err) require.Equal(t, "code-abc", code) require.Equal(t, "challenge-xyz", codeChallenge) @@ -174,7 +174,7 @@ func TestConsumeAuthorizeState_MissingKey_ReturnsEmpty(t *testing.T) { }, } - code, codeChallenge, nonce, redirectURI, err := h.consumeAuthorizeState("does-not-exist") + code, codeChallenge, nonce, redirectURI, _, err := h.consumeAuthorizeState("does-not-exist") // GetAndRemoveState returns an error for missing keys; consumeAuthorizeState propagates it. // The caller (oauth_callback) handles this gracefully. require.Error(t, err) @@ -199,7 +199,7 @@ func TestConsumeAuthorizeState_RedisNil_Propagates(t *testing.T) { }, } - _, _, _, _, err := h.consumeAuthorizeState("missing") + _, _, _, _, _, err := h.consumeAuthorizeState("missing") require.ErrorIs(t, err, goredis.Nil) } diff --git a/internal/http_handlers/oauth_callback.go b/internal/http_handlers/oauth_callback.go index 01c72ceef..b15daf0ab 100644 --- a/internal/http_handlers/oauth_callback.go +++ b/internal/http_handlers/oauth_callback.go @@ -2,12 +2,12 @@ package http_handlers import ( "context" + "crypto/subtle" "encoding/json" "errors" "fmt" "io" "net/http" - "net/url" "strings" "time" @@ -20,6 +20,7 @@ import ( "github.com/authorizerdev/authorizer/internal/asyncutil" "github.com/authorizerdev/authorizer/internal/audit" + "github.com/authorizerdev/authorizer/internal/codestate" "github.com/authorizerdev/authorizer/internal/constants" "github.com/authorizerdev/authorizer/internal/cookie" "github.com/authorizerdev/authorizer/internal/metrics" @@ -81,6 +82,23 @@ func (h *httpProvider) OAuthCallbackHandler() gin.HandlerFunc { ctx.JSON(400, gin.H{"error": "invalid oauth state"}) return } + // Prove the browser finishing this flow is the one that started it. + // The state alone cannot: it is server-generated and stored globally, so + // its presence only shows SOME flow issued it. Without this an attacker + // harvests their own valid code+state and delivers it to a victim's + // browser, logging the victim into the ATTACKER's account (login CSRF, + // RFC 9700 §4.7). Checked before the state is consumed so a failed + // attempt cannot burn a legitimate one. + boundState := cookie.GetOAuthState(ctx) + if subtle.ConstantTimeCompare([]byte(boundState), []byte(state)) != 1 { + log.Debug().Bool("cookie_present", boundState != "").Msg("OAuth state is not bound to this browser") + metrics.RecordSecurityEvent("oauth_state_not_bound", provider) + cookie.DeleteOAuthState(ctx, h.Config.AppCookieSecure) + ctx.JSON(400, gin.H{"error": "invalid oauth state"}) + return + } + cookie.DeleteOAuthState(ctx, h.Config.AppCookieSecure) + // contains random token, redirect url, role sessionSplit := strings.Split(state, "___") @@ -428,7 +446,7 @@ func (h *httpProvider) OAuthCallbackHandler() gin.HandlerFunc { // // In the standalone social login flow (`/oauth_login/:provider`), this entry will not exist and we // simply generate a nonce and continue. - code, codeChallenge, nonce, authorizeRedirectURI, err := h.consumeAuthorizeState(stateValue) + code, codeChallenge, nonce, authorizeRedirectURI, authorizeClientID, err := h.consumeAuthorizeState(stateValue) if err != nil && !errors.Is(err, goredis.Nil) { log.Debug().Err(err).Str("state", stateValue).Msg("Failed to get authorize state from store") } @@ -495,7 +513,13 @@ func (h *httpProvider) OAuthCallbackHandler() gin.HandlerFunc { // exploitable, but there's no reason to write it before we know the // login actually proceeds. if code != "" { - if err := h.MemoryStoreProvider.SetState(code, codeChallenge+"@@"+authToken.FingerPrintHash+"@@"+nonce+"@@"+url.QueryEscape(authorizeRedirectURI)); err != nil { + if err := h.MemoryStoreProvider.SetState(code, codestate.EncodeCode(codestate.Code{ + Challenge: codeChallenge, + Session: authToken.FingerPrintHash, + Nonce: nonce, + RedirectURI: authorizeRedirectURI, + ClientID: authorizeClientID, + })); err != nil { log.Debug().Err(err).Msg("Failed to set state") ctx.JSON(500, gin.H{"error": "failed to process OAuth login"}) return diff --git a/internal/http_handlers/oauth_login.go b/internal/http_handlers/oauth_login.go index c12f1d3af..2c4617a99 100644 --- a/internal/http_handlers/oauth_login.go +++ b/internal/http_handlers/oauth_login.go @@ -10,6 +10,7 @@ import ( "github.com/authorizerdev/authorizer/internal/audit" "github.com/authorizerdev/authorizer/internal/constants" + "github.com/authorizerdev/authorizer/internal/cookie" "github.com/authorizerdev/authorizer/internal/metrics" "github.com/authorizerdev/authorizer/internal/parsers" "github.com/authorizerdev/authorizer/internal/utils" @@ -94,6 +95,10 @@ func (h *httpProvider) OAuthLoginHandler() gin.HandlerFunc { }) return } + // Bind this flow to the browser that started it (RFC 9700 §4.7). Set + // before the state is stored so a store failure cannot leave a usable + // cookie behind. + cookie.SetOAuthState(c, oauthStateString, h.Config.AppCookieSecure) if err := h.MemoryStoreProvider.SetState(oauthStateString, provider); err != nil { log.Debug().Err(err).Msg("Error setting state") c.JSON(500, gin.H{ diff --git a/internal/http_handlers/token.go b/internal/http_handlers/token.go index 30477cc5a..27c59cdff 100644 --- a/internal/http_handlers/token.go +++ b/internal/http_handlers/token.go @@ -15,6 +15,7 @@ import ( "github.com/google/uuid" "github.com/authorizerdev/authorizer/internal/audit" + "github.com/authorizerdev/authorizer/internal/codestate" "github.com/authorizerdev/authorizer/internal/constants" "github.com/authorizerdev/authorizer/internal/cookie" "github.com/authorizerdev/authorizer/internal/metrics" @@ -364,18 +365,36 @@ func (h *httpProvider) TokenHandler() gin.HandlerFunc { return } - // [0] -> code_challenge (may contain "::method" suffix) or empty - // [1] -> session cookie - // [2] -> OIDC nonce from /authorize request (optional) - // [3] -> redirect_uri from /authorize request (optional, for RFC 6749 §4.1.3) + // One owner for this positional format — see internal/codestate. + // Blobs written by an older build decode with the trailing fields + // empty, so codes issued before a deploy stay redeemable across it. + storedCode := codestate.DecodeCode(sessionData) sessionDataSplit := strings.Split(sessionData, "@@") + // RFC 6749 §4.1.3: "ensure that the authorization code was issued to + // the authenticated confidential client". Without this the code is + // bound to a redirect_uri but not to an identity, so two clients + // sharing a redirect origin can redeem each other's codes — the + // mix-up shape this clause exists to prevent. Constant-time because + // the comparison is against a value the caller supplies. + // Compared against the AUTHENTICATED client (the clientauth resolver's + // result), not the raw body field — the client may authenticate via + // HTTP Basic or a client assertion and send no body client_id at all. + if storedCode.ClientID != "" { + if subtle.ConstantTimeCompare([]byte(resolvedClient.ClientID), []byte(storedCode.ClientID)) != 1 { + metrics.RecordSecurityEvent("token_exchange_client_mismatch", "token_endpoint") + log.Warn().Str("client_id", resolvedClient.ClientID).Msg("rejected: authorization code was issued to a different client") + gc.JSON(http.StatusBadRequest, gin.H{ + "error": "invalid_grant", + "error_description": "The authorization code was not issued to this client", + }) + return + } + } + // RFC 6749 §4.1.3: If redirect_uri was included in the authorization // request, the token request MUST include the identical redirect_uri. - storedRedirectURI := "" - if len(sessionDataSplit) > 3 { - storedRedirectURI, _ = url.QueryUnescape(sessionDataSplit[3]) - } + storedRedirectURI := storedCode.RedirectURI requestRedirectURI := strings.TrimSpace(reqBody.RedirectURI) if storedRedirectURI != "" { if requestRedirectURI == "" { diff --git a/internal/integration_tests/auth_code_client_binding_test.go b/internal/integration_tests/auth_code_client_binding_test.go new file mode 100644 index 000000000..447f5e376 --- /dev/null +++ b/internal/integration_tests/auth_code_client_binding_test.go @@ -0,0 +1,64 @@ +package integration_tests + +import ( + "encoding/json" + "net/http" + "net/url" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Audit finding #7: the authorization code was not bound to the client it was +// issued to (RFC 6749 §4.1.3 — "ensure that the authorization code was issued to +// the authenticated confidential client"). +// +// The stored code state carried the PKCE challenge, session, nonce, redirect_uri +// and resource, but no client identity, and the token endpoint never checked +// one. A code was therefore bound to a redirect_uri but not to an identity, so +// two confidential clients sharing a redirect origin could redeem each other's +// codes — the mix-up shape the clause exists to prevent. +func TestAuthCode_CrossClientRedemption_Rejected(t *testing.T) { + cfg := getTestConfig() + ts := initTestSetup(t, cfg) + + registerTestClient(t, ts, "code-client-one", "code-client-one-secret") + registerTestClient(t, ts, "code-client-two", "code-client-two-secret") + + router, code, codeVerifier := loginForOfflineAccess(t, ts, "code-client-one") + + t.Run("a different client redeeming the code is rejected", func(t *testing.T) { + form := url.Values{} + form.Set("grant_type", "authorization_code") + form.Set("code", code) + form.Set("code_verifier", codeVerifier) + form.Set("redirect_uri", "http://localhost:3000/callback") + + // client-two authenticates correctly as itself — the only thing wrong is + // that this code belongs to client-one. + w := exchangeCode(router, form, []string{"code-client-two", "code-client-two-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"]) + assert.Contains(t, errBody["error_description"], "not issued to this client") + }) + + t.Run("the issuing client can still redeem its own code", func(t *testing.T) { + // The control: the guard must reject the wrong client without breaking + // the right one. Uses a freshly minted code because the attempt above + // consumed nothing — codes are single-use via GetAndRemoveState, so the + // rejected exchange already burned that one. + router, freshCode, freshVerifier := loginForOfflineAccess(t, ts, "code-client-one") + + form := url.Values{} + form.Set("grant_type", "authorization_code") + form.Set("code", freshCode) + form.Set("code_verifier", freshVerifier) + form.Set("redirect_uri", "http://localhost:3000/callback") + + w := exchangeCode(router, form, []string{"code-client-one", "code-client-one-secret"}) + assert.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) + }) +} diff --git a/internal/service/auth_response.go b/internal/service/auth_response.go index 32a2a58d4..367f80ee3 100644 --- a/internal/service/auth_response.go +++ b/internal/service/auth_response.go @@ -9,6 +9,7 @@ import ( "github.com/google/uuid" "github.com/authorizerdev/authorizer/internal/asyncutil" + "github.com/authorizerdev/authorizer/internal/codestate" "github.com/authorizerdev/authorizer/internal/constants" "github.com/authorizerdev/authorizer/internal/cookie" "github.com/authorizerdev/authorizer/internal/graph/model" @@ -44,23 +45,19 @@ func (p *provider) issueAuthResponse(ctx context.Context, meta RequestMetadata, oidcNonce := "" authorizeRedirectURI := "" authorizeResource := "" + authorizeClientID := "" if state != nil { authorizeState, _ := p.MemoryStoreProvider.GetState(refs.StringValue(state)) if authorizeState != "" { - authorizeStateSplit := strings.Split(authorizeState, "@@") - if len(authorizeStateSplit) > 1 { - code = authorizeStateSplit[0] - codeChallenge = authorizeStateSplit[1] - if len(authorizeStateSplit) > 2 { - oidcNonce = authorizeStateSplit[2] - } - 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] - } + // One owner for this positional format — see internal/codestate. + if codestate.HasCode(authorizeState) { + as := codestate.DecodeAuthorize(authorizeState) + code = as.Code + codeChallenge = as.Challenge + oidcNonce = as.Nonce + authorizeRedirectURI = as.RedirectURI + authorizeResource = as.Resource + authorizeClientID = as.ClientID } else { nonce = authorizeState } @@ -88,7 +85,14 @@ 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+"@@"+authorizeResource); err != nil { + if err := p.MemoryStoreProvider.SetState(code, codestate.EncodeCode(codestate.Code{ + Challenge: codeChallenge, + Session: authToken.FingerPrintHash, + Nonce: oidcNonce, + RedirectURI: authorizeRedirectURI, + Resource: authorizeResource, + ClientID: authorizeClientID, + })); 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 f4ce775cf..a751ef001 100644 --- a/internal/service/login.go +++ b/internal/service/login.go @@ -14,6 +14,7 @@ import ( "github.com/authorizerdev/authorizer/internal/asyncutil" "github.com/authorizerdev/authorizer/internal/audit" + "github.com/authorizerdev/authorizer/internal/codestate" "github.com/authorizerdev/authorizer/internal/constants" "github.com/authorizerdev/authorizer/internal/cookie" "github.com/authorizerdev/authorizer/internal/graph/model" @@ -607,25 +608,20 @@ func (p *provider) Login(ctx context.Context, meta RequestMetadata, params *mode oidcNonce := "" authorizeRedirectURI := "" authorizeResource := "" + authorizeClientID := "" if params.State != nil { // Get state from store authorizeState, _ := p.MemoryStoreProvider.GetState(refs.StringValue(params.State)) if authorizeState != "" { - authorizeStateSplit := strings.Split(authorizeState, "@@") - if len(authorizeStateSplit) > 1 { - code = authorizeStateSplit[0] - codeChallenge = authorizeStateSplit[1] - if len(authorizeStateSplit) > 2 { - oidcNonce = authorizeStateSplit[2] - } - // RFC 6749 §4.1.3: redirect_uri from /authorize for validation at /oauth/token - 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] - } + // One owner for this positional format — see internal/codestate. + if codestate.HasCode(authorizeState) { + as := codestate.DecodeAuthorize(authorizeState) + code = as.Code + codeChallenge = as.Challenge + oidcNonce = as.Nonce + authorizeRedirectURI = as.RedirectURI + authorizeResource = as.Resource + authorizeClientID = as.ClientID } else { nonce = authorizeState } @@ -660,7 +656,14 @@ 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+"@@"+authorizeResource); err != nil { + if err := p.MemoryStoreProvider.SetState(code, codestate.EncodeCode(codestate.Code{ + Challenge: codeChallenge, + Session: authToken.FingerPrintHash, + Nonce: oidcNonce, + RedirectURI: authorizeRedirectURI, + Resource: authorizeResource, + ClientID: authorizeClientID, + })); 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 7239833bf..a440c136c 100644 --- a/internal/service/session.go +++ b/internal/service/session.go @@ -2,12 +2,12 @@ package service import ( "context" - "strings" "time" "github.com/gin-gonic/gin" "github.com/google/uuid" + "github.com/authorizerdev/authorizer/internal/codestate" "github.com/authorizerdev/authorizer/internal/constants" "github.com/authorizerdev/authorizer/internal/cookie" "github.com/authorizerdev/authorizer/internal/graph/model" @@ -79,23 +79,19 @@ func (p *provider) Session(ctx context.Context, meta RequestMetadata, params *mo oidcNonce := "" authorizeRedirectURI := "" authorizeResource := "" + authorizeClientID := "" if params != nil && params.State != nil { authorizeState, _ := p.MemoryStoreProvider.GetState(refs.StringValue(params.State)) if authorizeState != "" { - parts := strings.Split(authorizeState, "@@") - if len(parts) > 1 { - code = parts[0] - codeChallenge = parts[1] - if len(parts) > 2 { - oidcNonce = parts[2] - } - 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] - } + // One owner for this positional format — see internal/codestate. + if codestate.HasCode(authorizeState) { + as := codestate.DecodeAuthorize(authorizeState) + code = as.Code + codeChallenge = as.Challenge + oidcNonce = as.Nonce + authorizeRedirectURI = as.RedirectURI + authorizeResource = as.Resource + authorizeClientID = as.ClientID } _ = p.MemoryStoreProvider.RemoveState(refs.StringValue(params.State)) } @@ -120,7 +116,14 @@ func (p *provider) Session(ctx context.Context, meta RequestMetadata, params *mo } if code != "" { - if err := p.MemoryStoreProvider.SetState(code, codeChallenge+"@@"+authToken.FingerPrintHash+"@@"+oidcNonce+"@@"+authorizeRedirectURI+"@@"+authorizeResource); err != nil { + if err := p.MemoryStoreProvider.SetState(code, codestate.EncodeCode(codestate.Code{ + Challenge: codeChallenge, + Session: authToken.FingerPrintHash, + Nonce: oidcNonce, + RedirectURI: authorizeRedirectURI, + Resource: authorizeResource, + ClientID: authorizeClientID, + })); 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 2cdbfb2bc..e4b4aab6d 100644 --- a/internal/service/signup.go +++ b/internal/service/signup.go @@ -12,6 +12,7 @@ import ( "github.com/authorizerdev/authorizer/internal/asyncutil" "github.com/authorizerdev/authorizer/internal/audit" + "github.com/authorizerdev/authorizer/internal/codestate" "github.com/authorizerdev/authorizer/internal/constants" "github.com/authorizerdev/authorizer/internal/cookie" "github.com/authorizerdev/authorizer/internal/crypto" @@ -340,24 +341,20 @@ func (p *provider) SignUp(ctx context.Context, meta RequestMetadata, params *mod oidcNonce := "" authorizeRedirectURI := "" authorizeResource := "" + authorizeClientID := "" if params.State != nil { // Get state from store authorizeState, _ := p.MemoryStoreProvider.GetState(refs.StringValue(params.State)) if authorizeState != "" { - authorizeStateSplit := strings.Split(authorizeState, "@@") - if len(authorizeStateSplit) > 1 { - code = authorizeStateSplit[0] - codeChallenge = authorizeStateSplit[1] - if len(authorizeStateSplit) > 2 { - oidcNonce = authorizeStateSplit[2] - } - 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] - } + // One owner for this positional format — see internal/codestate. + if codestate.HasCode(authorizeState) { + as := codestate.DecodeAuthorize(authorizeState) + code = as.Code + codeChallenge = as.Challenge + oidcNonce = as.Nonce + authorizeRedirectURI = as.RedirectURI + authorizeResource = as.Resource + authorizeClientID = as.ClientID } else { nonce = authorizeState } @@ -436,7 +433,14 @@ 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+"@@"+authorizeResource); err != nil { + if err := p.MemoryStoreProvider.SetState(code, codestate.EncodeCode(codestate.Code{ + Challenge: codeChallenge, + Session: authToken.FingerPrintHash, + Nonce: oidcNonce, + RedirectURI: authorizeRedirectURI, + Resource: authorizeResource, + ClientID: authorizeClientID, + })); err != nil { log.Debug().Err(err).Msg("SetState failed") return nil, nil, err } From 2f81a043445b706c08bcaf902511f16e49a2b3be Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Fri, 7 Aug 2026 09:44:38 +0530 Subject: [PATCH 2/9] security: hash session tokens at rest, fix at_hash alg and CORS Audit findings AUDIT-09, AUDIT-10, AUDIT-15, plus a user-reported bug. AUDIT-10: access and refresh tokens were stored verbatim in the session store, so anyone who could read it - a Redis dump, a replica, a backup, an SSRF into the cache - walked away with live, directly replayable tokens. Stored as SHA-256 digests now. Reads are dual-mode (crypto.VerifySessionValue): a value written before the upgrade is still the raw token and is compared directly, so no live session or refresh token drops on deploy. The legacy branch can go once no pre-upgrade session can still be within its TTL. Three comparison sites had to move with it, one of which - revoke.go - was a plain != against a live refresh token rather than a constant-time compare. Two tests were scraping the store to recover a bearer token; they now use the one the API returned, which is the point of the change. AUDIT-09: at_hash/c_hash were hard-coded to SHA-256 whatever the signing alg. OIDC Core 3.1.3.6 and 3.3.2.11 require the digest implied by `alg`, so an instance signing RS384/RS512 emitted a value no conformant RP can reproduce - and an RP that cannot reproduce it skips the token-substitution check the claim exists to provide. AUDIT-15: a wildcard allow-list reflected the caller's Origin alongside Access-Control-Allow-Credentials, which is "any site may read credentialed responses from this API" wearing a disguise. Wildcard now returns a literal * with no credentials; credentialed CORS requires an explicit allow-list. Bug report (not from the audit): a user who clicked their verification link was told forever that their email was not verified, most visibly by passkey login. The email_verified_at write sat AFTER the MFA gate, and all three gate branches return early - and MFA is on by default, so a fresh signup's verification click lands on the setup screen and the write never happened. Recorded as soon as the token is proven now: clicking the link is the proof of mailbox control, and MFA interrupting session issuance must not discard it. AUDIT-13 (SameSite/domain-scoped cookies) was reviewed and declined - the default serves the subdomain-auth-server topology the product targets, same position Auth0 takes. Reasoning is now recorded at cookie.BuildSessionCookies with guard tests in internal/cookie and cmd, so it is not silently "fixed" later. --- cmd/cookie_defaults_test.go | 30 ++++++++ cmd/root.go | 7 +- internal/cookie/cookie.go | 38 ++++++++++ internal/cookie/cookie_test.go | 53 +++++++++++++ internal/crypto/session_value.go | 51 +++++++++++++ internal/graphql/_deprecated_mobile_login.go | 6 +- internal/graphql/_deprecated_mobile_signup.go | 6 +- internal/http_handlers/authorize.go | 8 +- internal/http_handlers/cors.go | 28 +++++-- internal/http_handlers/cors_test.go | 73 ++++++++++++++++++ internal/http_handlers/oauth_callback.go | 7 +- internal/http_handlers/oauth_sso.go | 6 +- .../http_handlers/revoke_refresh_token.go | 5 +- internal/http_handlers/saml_sp.go | 7 +- internal/http_handlers/token.go | 7 +- internal/http_handlers/verify_email.go | 7 +- internal/integration_tests/profile_test.go | 19 ++--- .../validate_jwt_token_test.go | 18 ++--- .../verification_token_purpose_test.go | 50 +++++++++++++ internal/service/auth_response.go | 7 +- internal/service/login.go | 7 +- internal/service/revoke.go | 6 +- internal/service/session.go | 7 +- internal/service/signup.go | 6 +- internal/service/verify_email.go | 47 ++++++++---- internal/token/at_hash_test.go | 75 +++++++++++++++++++ internal/token/auth_token.go | 56 ++++++++++---- 27 files changed, 541 insertions(+), 96 deletions(-) create mode 100644 cmd/cookie_defaults_test.go create mode 100644 internal/crypto/session_value.go create mode 100644 internal/http_handlers/cors_test.go create mode 100644 internal/token/at_hash_test.go diff --git a/cmd/cookie_defaults_test.go b/cmd/cookie_defaults_test.go new file mode 100644 index 000000000..ac4142d49 --- /dev/null +++ b/cmd/cookie_defaults_test.go @@ -0,0 +1,30 @@ +package cmd + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestAppCookieSameSiteDefaultIsNone is a decision guard for the other half of +// the session-cookie topology (the first half is pinned in +// internal/cookie.TestSessionCookieTopologyIsDeliberate). +// +// "none" looks like an obviously-wrong default to a scanner or a security +// review, and the 2.4.0 pre-release audit duly flagged it. It was consciously +// kept: Authorizer targets an auth server on a subdomain serving apps on other +// sites, and Lax withholds the session cookie on exactly those cross-site +// requests, breaking the browser-session half of the SDK. Auth0 takes the same +// position — it recommends SameSite=None for cross-origin authentication and +// ships fallback cookies for browsers that cannot do it. +// +// CSRF middleware, HttpOnly, and Authorization-header auth are what actually +// carry the security here; SameSite is defense-in-depth. See +// cookie.BuildSessionCookies before changing this. +func TestAppCookieSameSiteDefaultIsNone(t *testing.T) { + f := RootCmd.PersistentFlags().Lookup("app-cookie-same-site") + require.NotNil(t, f, "the --app-cookie-same-site flag must exist") + assert.Equal(t, "none", f.DefValue, + "changing this default breaks cross-site apps; read cookie.BuildSessionCookies first") +} diff --git a/cmd/root.go b/cmd/root.go index de870ae8a..5ce25a129 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -204,7 +204,12 @@ func init() { // Cookies flags f.BoolVar(&rootArgs.config.AppCookieSecure, "app-cookie-secure", true, "Application secure cookie flag") - f.StringVar(&rootArgs.config.AppCookieSameSite, "app-cookie-same-site", "none", "SameSite attribute for session cookies (lax, strict, none)") + // Default "none" is deliberate and audit-reviewed, not an oversight: the + // product targets an auth server on a subdomain serving apps on other + // sites, and Lax withholds the session cookie on exactly those cross-site + // requests. Same position Auth0 takes. See cookie.BuildSessionCookies for + // the full reasoning before changing it. + f.StringVar(&rootArgs.config.AppCookieSameSite, "app-cookie-same-site", "none", "SameSite attribute for session cookies (lax, strict, none). Default none supports apps on other domains; set lax if every app shares this host") f.BoolVar(&rootArgs.config.AdminCookieSecure, "admin-cookie-secure", true, "Admin secure cookie flag") f.BoolVar(&rootArgs.config.DisableAdminHeaderAuth, "disable-admin-header-auth", false, "Disable admin authentication via X-Authorizer-Admin-Secret header") diff --git a/internal/cookie/cookie.go b/internal/cookie/cookie.go index 9003a5179..a322adec6 100644 --- a/internal/cookie/cookie.go +++ b/internal/cookie/cookie.go @@ -36,6 +36,44 @@ func SetSession(gc *gin.Context, sessionID string, appCookieSecure bool, sameSit // BuildSessionCookies returns the pair of session cookies (host-scoped and // domain-scoped) to set on the response. Transport-agnostic so non-gin // callers (the service layer, gRPC handlers) can produce them as side-effects. +// +// # Why there are two cookies, and why SameSite defaults to None +// +// DELIBERATE, and reviewed. The 2.4.0 pre-release security audit flagged both +// as hardening opportunities — default SameSite=Lax, drop the domain-scoped +// twin — and both were consciously declined. Please do not "fix" them without +// re-reading this. +// +// Authorizer's intended deployment is an auth server on a subdomain +// (auth.example.com) serving several apps, some on sibling subdomains and some +// on entirely different domains. Both properties exist to serve that: +// +// - the domain-scoped (".example.com") twin is what lets app.example.com see +// a session established at auth.example.com. Dropping it breaks subdomain +// SSO outright. +// - SameSite=None is what lets an app on a DIFFERENT site complete a +// credentialed /session call at all. Lax withholds the cookie on exactly +// those cross-site requests, so the browser-session half of the SDK stops +// working. +// +// Auth0 lands in the same place: it recommends SameSite=None for cross-origin +// authentication (it is required when response_mode=form_post), and ships +// fallback cookies — auth0_compat and friends — for browsers that cannot do +// SameSite=None at all. Its documented answer to third-party-cookie blocking is +// Custom Domains: put the auth server on the customer's own subdomain so its +// cookies are first-party. That is precisely the topology above, and precisely +// what the domain-scoped cookie here provides. +// +// The security cost is understood and covered elsewhere: CSRF middleware +// (internal/http_handlers/csrf.go) is the primary defense for state-changing +// requests, the cookie is HttpOnly, the authenticated API reads its token from +// the Authorization header rather than this cookie, and the admin cookie is +// independently SameSite=Strict. SameSite here is defense-in-depth, not the +// control being relied upon. +// +// Operators who do NOT need cross-site apps should set +// --app-cookie-same-site=lax. That is the knob; the default is chosen for the +// topology the product targets. func BuildSessionCookies(hostname, sessionID string, appCookieSecure bool, sameSite http.SameSite) []*http.Cookie { host, _ := parsers.GetHostParts(hostname) domain := parsers.GetDomainName(hostname) diff --git a/internal/cookie/cookie_test.go b/internal/cookie/cookie_test.go index 6316ea129..73d1bd7d4 100644 --- a/internal/cookie/cookie_test.go +++ b/internal/cookie/cookie_test.go @@ -2,6 +2,7 @@ package cookie import ( "net/http" + "strings" "testing" "time" @@ -113,3 +114,55 @@ func TestParseSameSite(t *testing.T) { }) } } + +// TestSessionCookieTopologyIsDeliberate is a decision guard, not a behaviour +// test. Both properties it pins were raised as findings in the 2.4.0 +// pre-release security audit (default SameSite=Lax, drop the domain-scoped +// cookie) and both were consciously declined — see BuildSessionCookies for the +// reasoning and the Auth0 precedent. +// +// If a future change flips either one, this fails and points at that comment +// rather than letting subdomain SSO or cross-site apps break silently in the +// field, where the symptom is "login randomly doesn't stick" and the cause is +// three layers away. +func TestSessionCookieTopologyIsDeliberate(t *testing.T) { + t.Parallel() + + cookies := BuildSessionCookies("auth.example.com", "session-value", true, http.SameSiteNoneMode) + + require.Len(t, cookies, 2, + "a host-scoped AND a domain-scoped cookie are both required: the domain-scoped one is what lets app.example.com see a session established at auth.example.com") + + var host, domain *http.Cookie + for _, c := range cookies { + if strings.HasSuffix(c.Name, "_session_domain") { + domain = c + } else { + host = c + } + } + require.NotNil(t, host) + require.NotNil(t, domain) + + assert.Equal(t, "auth.example.com", host.Domain, "host-scoped cookie stays on the exact host") + assert.Equal(t, ".example.com", domain.Domain, + "domain-scoped cookie must be dotted so sibling subdomains receive it — this is Authorizer's subdomain-SSO mechanism") + + for _, c := range []*http.Cookie{host, domain} { + assert.True(t, c.HttpOnly, "session cookies are never script-readable") + assert.Equal(t, http.SameSiteNoneMode, c.SameSite, + "SameSite is passed through from config, not overridden here; the default is None so apps on other sites can complete a credentialed /session call") + } +} + +// TestSessionCookieSameSiteIsCallerControlled documents that hardening is +// available to operators who do not need cross-site apps — the knob exists, +// the default is simply chosen for the topology the product targets. +func TestSessionCookieSameSiteIsCallerControlled(t *testing.T) { + t.Parallel() + for _, mode := range []http.SameSite{http.SameSiteLaxMode, http.SameSiteStrictMode, http.SameSiteNoneMode} { + for _, c := range BuildSessionCookies("auth.example.com", "v", true, mode) { + assert.Equal(t, mode, c.SameSite) + } + } +} diff --git a/internal/crypto/session_value.go b/internal/crypto/session_value.go new file mode 100644 index 000000000..01a30f0e2 --- /dev/null +++ b/internal/crypto/session_value.go @@ -0,0 +1,51 @@ +package crypto + +import ( + "crypto/sha256" + "crypto/subtle" + "encoding/hex" + "strings" +) + +// sessionValueDigestPrefix marks a stored session value as a digest rather than +// the raw token. It is what makes the transition observable: a value without it +// was written by an older build and is still the token in the clear. +const sessionValueDigestPrefix = "sha256:" + +// HashSessionValue converts a bearer value (access token, refresh token, +// session fingerprint hash) into what should actually be persisted in the +// session store. +// +// These were stored verbatim, so anyone who could read the store — a Redis +// dump, a replica, a backup, an SSRF into the cache — walked away with live, +// directly replayable access and refresh tokens. Storing a digest means a +// reader learns only that *some* token was issued, not one they can present. +// The store never needs the original: every consumer either checks a key exists +// or compares against a token the caller already supplied. +func HashSessionValue(value string) string { + if value == "" { + return "" + } + sum := sha256.Sum256([]byte(value)) + return sessionValueDigestPrefix + hex.EncodeToString(sum[:]) +} + +// VerifySessionValue reports whether presented matches what the store holds. +// +// Dual-read for the upgrade: a value written before the deploy is still the raw +// token, so it is compared directly, while anything carrying the digest prefix +// is compared as a digest. Without this every live session and refresh token +// would stop working the moment the new binary starts. The legacy branch can be +// deleted once no pre-upgrade session can still be within its TTL. +// +// Constant-time on both branches — the stored value is a credential either way. +func VerifySessionValue(presented, stored string) bool { + if presented == "" || stored == "" { + return false + } + if strings.HasPrefix(stored, sessionValueDigestPrefix) { + return subtle.ConstantTimeCompare([]byte(HashSessionValue(presented)), []byte(stored)) == 1 + } + // Legacy plaintext row. + return subtle.ConstantTimeCompare([]byte(presented), []byte(stored)) == 1 +} diff --git a/internal/graphql/_deprecated_mobile_login.go b/internal/graphql/_deprecated_mobile_login.go index fe6184cbf..708c505ce 100644 --- a/internal/graphql/_deprecated_mobile_login.go +++ b/internal/graphql/_deprecated_mobile_login.go @@ -192,12 +192,12 @@ func MobileLoginResolver(ctx context.Context, params model.MobileLoginInput) (*m // cookie.SetSession(gc, authToken.FingerPrintHash) // sessionStoreKey := constants.AuthRecipeMethodMobileBasicAuth + ":" + user.ID - // memorystore.Provider.SetUserSession(sessionStoreKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, authToken.FingerPrintHash, authToken.SessionTokenExpiresAt) - // memorystore.Provider.SetUserSession(sessionStoreKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, authToken.AccessToken.Token, authToken.AccessToken.ExpiresAt) + // memorystore.Provider.SetUserSession(sessionStoreKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.FingerPrintHash), authToken.SessionTokenExpiresAt) + // memorystore.Provider.SetUserSession(sessionStoreKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.AccessToken.Token), authToken.AccessToken.ExpiresAt) // if authToken.RefreshToken != nil { // res.RefreshToken = &authToken.RefreshToken.Token - // memorystore.Provider.SetUserSession(sessionStoreKey, constants.TokenTypeRefreshToken+"_"+authToken.FingerPrint, authToken.RefreshToken.Token, authToken.RefreshToken.ExpiresAt) + // memorystore.Provider.SetUserSession(sessionStoreKey, constants.TokenTypeRefreshToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.RefreshToken.Token), authToken.RefreshToken.ExpiresAt) // } // go func() { diff --git a/internal/graphql/_deprecated_mobile_signup.go b/internal/graphql/_deprecated_mobile_signup.go index b5fdcb020..ceecd4726 100644 --- a/internal/graphql/_deprecated_mobile_signup.go +++ b/internal/graphql/_deprecated_mobile_signup.go @@ -273,12 +273,12 @@ func MobileSignupResolver(ctx context.Context, params *model.MobileSignUpInput) // sessionKey := constants.AuthRecipeMethodMobileBasicAuth + ":" + user.ID // cookie.SetSession(gc, authToken.FingerPrintHash) - // memorystore.Provider.SetUserSession(sessionKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, authToken.FingerPrintHash, authToken.SessionTokenExpiresAt) - // memorystore.Provider.SetUserSession(sessionKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, authToken.AccessToken.Token, authToken.AccessToken.ExpiresAt) + // memorystore.Provider.SetUserSession(sessionKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.FingerPrintHash), authToken.SessionTokenExpiresAt) + // memorystore.Provider.SetUserSession(sessionKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.AccessToken.Token), authToken.AccessToken.ExpiresAt) // if authToken.RefreshToken != nil { // res.RefreshToken = &authToken.RefreshToken.Token - // memorystore.Provider.SetUserSession(sessionKey, constants.TokenTypeRefreshToken+"_"+authToken.FingerPrint, authToken.RefreshToken.Token, authToken.RefreshToken.ExpiresAt) + // memorystore.Provider.SetUserSession(sessionKey, constants.TokenTypeRefreshToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.RefreshToken.Token), authToken.RefreshToken.ExpiresAt) // } // go func() { diff --git a/internal/http_handlers/authorize.go b/internal/http_handlers/authorize.go index d2a435097..9a3117dc2 100644 --- a/internal/http_handlers/authorize.go +++ b/internal/http_handlers/authorize.go @@ -644,12 +644,12 @@ func (h *httpProvider) AuthorizeHandler() gin.HandlerFunc { handleResponse(gc, responseMode, authURL, redirectURI, loginError, http.StatusOK) return } - if err := h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, authToken.FingerPrintHash, authToken.SessionTokenExpiresAt); err != nil { + if err := h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.FingerPrintHash), authToken.SessionTokenExpiresAt); err != nil { log.Debug().Err(err).Msg("Error persisting session for hybrid") handleResponse(gc, responseMode, authURL, redirectURI, loginError, http.StatusOK) return } - if err := h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, authToken.AccessToken.Token, authToken.AccessToken.ExpiresAt); err != nil { + if err := h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.AccessToken.Token), authToken.AccessToken.ExpiresAt); err != nil { log.Debug().Err(err).Msg("Error persisting access token for hybrid") handleResponse(gc, responseMode, authURL, redirectURI, loginError, http.StatusOK) return @@ -725,12 +725,12 @@ func (h *httpProvider) AuthorizeHandler() gin.HandlerFunc { return } - if err := h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, authToken.FingerPrintHash, authToken.SessionTokenExpiresAt); err != nil { + if err := h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.FingerPrintHash), authToken.SessionTokenExpiresAt); err != nil { log.Debug().Err(err).Msg("Error persisting session for id_token token") handleResponse(gc, responseMode, authURL, redirectURI, loginError, http.StatusOK) return } - if err := h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, authToken.AccessToken.Token, authToken.AccessToken.ExpiresAt); err != nil { + if err := h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.AccessToken.Token), authToken.AccessToken.ExpiresAt); err != nil { log.Debug().Err(err).Msg("Error persisting access token for id_token token") handleResponse(gc, responseMode, authURL, redirectURI, loginError, http.StatusOK) return diff --git a/internal/http_handlers/cors.go b/internal/http_handlers/cors.go index c44eebe5a..d1563014a 100644 --- a/internal/http_handlers/cors.go +++ b/internal/http_handlers/cors.go @@ -10,13 +10,29 @@ import ( func (h *httpProvider) CORSMiddleware() gin.HandlerFunc { return func(c *gin.Context) { origin := c.Request.Header.Get("Origin") - if validators.IsValidOrigin(origin, h.Config.AllowedOrigins) { + // Under a wildcard allow-list, reflect the literal "*" and send NO + // credentials header. + // + // Reflecting the caller's exact Origin alongside + // Access-Control-Allow-Credentials: true is functionally "any site may + // read credentialed responses from this API" — the Fetch spec forbids + // pairing credentials with a wildcard precisely to stop that, and + // echoing the origin back is the same thing wearing a disguise. Today + // the blast radius is small (the authenticated API reads its token from + // the Authorization header only, GraphQL is POST-behind-CSRF, and the + // session cookie is HttpOnly), but that is a property of the current + // endpoints, not of this middleware. The first GET endpoint to adopt + // cookie auth would turn it into a live cross-origin read. + // + // Credentialed CORS therefore requires an explicit allow-list. + if isWildcardOrigins(h.Config.AllowedOrigins) { + c.Writer.Header().Set("Access-Control-Allow-Origin", "*") + } else if origin != "" && validators.IsValidOrigin(origin, h.Config.AllowedOrigins) { c.Writer.Header().Set("Access-Control-Allow-Origin", origin) - // Only set credentials header when a specific validated origin is returned. - // Credentials must not be combined with a wildcard or empty origin. - if origin != "" && origin != "*" { - c.Writer.Header().Set("Access-Control-Allow-Credentials", "true") - } + c.Writer.Header().Set("Access-Control-Allow-Credentials", "true") + // The response varies by request Origin, so it must not be cached + // and replayed to a different origin. + c.Writer.Header().Add("Vary", "Origin") } c.Writer.Header().Set("Access-Control-Allow-Headers", "Content-Type, Content-Length, Accept-Encoding, X-CSRF-Token, Authorization, accept, origin, Cache-Control, X-Requested-With, X-authorizer-url, X-Forwarded-Proto, X-authorizer-client-id") c.Writer.Header().Set("Access-Control-Allow-Methods", "POST, OPTIONS, GET, PUT") diff --git a/internal/http_handlers/cors_test.go b/internal/http_handlers/cors_test.go new file mode 100644 index 000000000..e51b795e2 --- /dev/null +++ b/internal/http_handlers/cors_test.go @@ -0,0 +1,73 @@ +package http_handlers + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/gin-gonic/gin" + "github.com/rs/zerolog" + "github.com/stretchr/testify/assert" + + "github.com/authorizerdev/authorizer/internal/config" +) + +// TestCORSMiddleware pins the one rule that matters here: a reflected origin and +// Access-Control-Allow-Credentials must never appear together under a wildcard +// allow-list. That pairing is "any site may read credentialed responses from +// this API", which the Fetch spec forbids for exactly that reason. +func TestCORSMiddleware(t *testing.T) { + t.Parallel() + + run := func(allowedOrigins []string, requestOrigin string) http.Header { + gin.SetMode(gin.TestMode) + logger := zerolog.Nop() + h := &httpProvider{ + Config: &config.Config{AllowedOrigins: allowedOrigins}, + Dependencies: Dependencies{Log: &logger}, + } + router := gin.New() + router.Use(h.CORSMiddleware()) + router.GET("/x", func(c *gin.Context) { c.Status(http.StatusOK) }) + + req := httptest.NewRequest(http.MethodGet, "/x", nil) + if requestOrigin != "" { + req.Header.Set("Origin", requestOrigin) + } + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + return w.Header() + } + + t.Run("wildcard returns * and never credentials", func(t *testing.T) { + t.Parallel() + hdr := run([]string{"*"}, "https://evil.example.com") + assert.Equal(t, "*", hdr.Get("Access-Control-Allow-Origin"), + "the caller's origin must not be reflected back under a wildcard") + assert.Empty(t, hdr.Get("Access-Control-Allow-Credentials"), + "credentials must never be combined with a wildcard") + }) + + t.Run("unset allow-list behaves as wildcard", func(t *testing.T) { + t.Parallel() + hdr := run(nil, "https://evil.example.com") + assert.Equal(t, "*", hdr.Get("Access-Control-Allow-Origin")) + assert.Empty(t, hdr.Get("Access-Control-Allow-Credentials")) + }) + + t.Run("explicit allow-list reflects the origin with credentials", func(t *testing.T) { + t.Parallel() + hdr := run([]string{"https://app.example.com"}, "https://app.example.com") + assert.Equal(t, "https://app.example.com", hdr.Get("Access-Control-Allow-Origin")) + assert.Equal(t, "true", hdr.Get("Access-Control-Allow-Credentials")) + assert.Contains(t, hdr.Values("Vary"), "Origin", + "a per-origin response must not be cached and replayed to another origin") + }) + + t.Run("a disallowed origin gets neither header", func(t *testing.T) { + t.Parallel() + hdr := run([]string{"https://app.example.com"}, "https://evil.example.com") + assert.Empty(t, hdr.Get("Access-Control-Allow-Origin")) + assert.Empty(t, hdr.Get("Access-Control-Allow-Credentials")) + }) +} diff --git a/internal/http_handlers/oauth_callback.go b/internal/http_handlers/oauth_callback.go index b15daf0ab..c3460e444 100644 --- a/internal/http_handlers/oauth_callback.go +++ b/internal/http_handlers/oauth_callback.go @@ -23,6 +23,7 @@ import ( "github.com/authorizerdev/authorizer/internal/codestate" "github.com/authorizerdev/authorizer/internal/constants" "github.com/authorizerdev/authorizer/internal/cookie" + "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/metrics" "github.com/authorizerdev/authorizer/internal/parsers" "github.com/authorizerdev/authorizer/internal/refs" @@ -528,11 +529,11 @@ func (h *httpProvider) OAuthCallbackHandler() gin.HandlerFunc { sessionKey := provider + ":" + user.ID cookie.SetSession(ctx, authToken.FingerPrintHash, h.Config.AppCookieSecure, cookie.ParseSameSite(h.Config.AppCookieSameSite)) - _ = h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, authToken.FingerPrintHash, authToken.SessionTokenExpiresAt) - _ = h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, authToken.AccessToken.Token, authToken.AccessToken.ExpiresAt) + _ = h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.FingerPrintHash), authToken.SessionTokenExpiresAt) + _ = h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.AccessToken.Token), authToken.AccessToken.ExpiresAt) if authToken.RefreshToken != nil { - _ = h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeRefreshToken+"_"+authToken.FingerPrint, authToken.RefreshToken.Token, authToken.RefreshToken.ExpiresAt) + _ = h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeRefreshToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.RefreshToken.Token), authToken.RefreshToken.ExpiresAt) } bgCtx := context.WithoutCancel(ctx) diff --git a/internal/http_handlers/oauth_sso.go b/internal/http_handlers/oauth_sso.go index b3fec63f1..5afdea66c 100644 --- a/internal/http_handlers/oauth_sso.go +++ b/internal/http_handlers/oauth_sso.go @@ -653,10 +653,10 @@ func (h *httpProvider) issueSSOSession(c *gin.Context, flow *ssoFlowState, user sessionKey := constants.AuthRecipeMethodSSO + ":" + user.ID cookie.SetSession(c, authToken.FingerPrintHash, h.Config.AppCookieSecure, cookie.ParseSameSite(h.Config.AppCookieSameSite)) - _ = h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, authToken.FingerPrintHash, authToken.SessionTokenExpiresAt) - _ = h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, authToken.AccessToken.Token, authToken.AccessToken.ExpiresAt) + _ = h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.FingerPrintHash), authToken.SessionTokenExpiresAt) + _ = h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.AccessToken.Token), authToken.AccessToken.ExpiresAt) if authToken.RefreshToken != nil { - _ = h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeRefreshToken+"_"+authToken.FingerPrint, authToken.RefreshToken.Token, authToken.RefreshToken.ExpiresAt) + _ = h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeRefreshToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.RefreshToken.Token), authToken.RefreshToken.ExpiresAt) } bgCtx := context.WithoutCancel(c.Request.Context()) diff --git a/internal/http_handlers/revoke_refresh_token.go b/internal/http_handlers/revoke_refresh_token.go index 371cf2a28..72b7b5b6d 100644 --- a/internal/http_handlers/revoke_refresh_token.go +++ b/internal/http_handlers/revoke_refresh_token.go @@ -1,12 +1,12 @@ package http_handlers import ( - "crypto/subtle" "net/http" "strings" "github.com/authorizerdev/authorizer/internal/audit" "github.com/authorizerdev/authorizer/internal/constants" + "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/metrics" "github.com/authorizerdev/authorizer/internal/service/clientauth" "github.com/authorizerdev/authorizer/internal/utils" @@ -145,7 +145,8 @@ func (h *httpProvider) RevokeRefreshTokenHandler() gin.HandlerFunc { existingToken, err := h.MemoryStoreProvider.GetUserSession(sessionToken, constants.TokenTypeRefreshToken+"_"+nonce) // RFC 7009 §2.1: use constant-time comparison to prevent timing attacks - if err != nil || existingToken == "" || subtle.ConstantTimeCompare([]byte(existingToken), []byte(tokenValue)) != 1 { + // Dual-read against the stored digest — see crypto.VerifySessionValue. + if err != nil || !crypto.VerifySessionValue(tokenValue, existingToken) { // RFC 7009 §2.2: Token not found or mismatch - return 200 log.Debug().Msg("Token not found or mismatch, returning 200 per RFC 7009") gc.JSON(http.StatusOK, gin.H{}) diff --git a/internal/http_handlers/saml_sp.go b/internal/http_handlers/saml_sp.go index 88437db2b..ecacf5726 100644 --- a/internal/http_handlers/saml_sp.go +++ b/internal/http_handlers/saml_sp.go @@ -58,6 +58,7 @@ import ( "github.com/authorizerdev/authorizer/internal/audit" "github.com/authorizerdev/authorizer/internal/constants" "github.com/authorizerdev/authorizer/internal/cookie" + "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/metrics" "github.com/authorizerdev/authorizer/internal/parsers" "github.com/authorizerdev/authorizer/internal/refs" @@ -539,10 +540,10 @@ func (h *httpProvider) issueSAMLSession(c *gin.Context, slug, orgID, appRedirect sessionKey := constants.AuthRecipeMethodSSO + ":" + user.ID cookie.SetSession(c, authToken.FingerPrintHash, h.Config.AppCookieSecure, cookie.ParseSameSite(h.Config.AppCookieSameSite)) - _ = h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, authToken.FingerPrintHash, authToken.SessionTokenExpiresAt) - _ = h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, authToken.AccessToken.Token, authToken.AccessToken.ExpiresAt) + _ = h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.FingerPrintHash), authToken.SessionTokenExpiresAt) + _ = h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.AccessToken.Token), authToken.AccessToken.ExpiresAt) if authToken.RefreshToken != nil { - _ = h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeRefreshToken+"_"+authToken.FingerPrint, authToken.RefreshToken.Token, authToken.RefreshToken.ExpiresAt) + _ = h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeRefreshToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.RefreshToken.Token), authToken.RefreshToken.ExpiresAt) } bgCtx := context.WithoutCancel(c.Request.Context()) diff --git a/internal/http_handlers/token.go b/internal/http_handlers/token.go index 27c59cdff..68aeb1967 100644 --- a/internal/http_handlers/token.go +++ b/internal/http_handlers/token.go @@ -18,6 +18,7 @@ import ( "github.com/authorizerdev/authorizer/internal/codestate" "github.com/authorizerdev/authorizer/internal/constants" "github.com/authorizerdev/authorizer/internal/cookie" + "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/metrics" "github.com/authorizerdev/authorizer/internal/parsers" "github.com/authorizerdev/authorizer/internal/refs" @@ -856,7 +857,7 @@ func (h *httpProvider) TokenHandler() gin.HandlerFunc { // For refresh_token grant the caller IS the user's browser (or an app // holding the refresh token), so we do a full session rollover. if isRefreshTokenGrant { - if err := h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, authToken.FingerPrintHash, authToken.SessionTokenExpiresAt); err != nil { + if err := h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.FingerPrintHash), authToken.SessionTokenExpiresAt); err != nil { log.Debug().Err(err).Msg("Error persisting session token") gc.JSON(http.StatusServiceUnavailable, gin.H{ "error": "temporarily_unavailable", @@ -867,7 +868,7 @@ func (h *httpProvider) TokenHandler() gin.HandlerFunc { cookie.SetSession(gc, authToken.FingerPrintHash, h.Config.AppCookieSecure, cookie.ParseSameSite(h.Config.AppCookieSameSite)) } - if err := h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, authToken.AccessToken.Token, authToken.AccessToken.ExpiresAt); err != nil { + if err := h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.AccessToken.Token), authToken.AccessToken.ExpiresAt); err != nil { log.Debug().Err(err).Msg("Error persisting access token") gc.JSON(http.StatusServiceUnavailable, gin.H{ "error": "temporarily_unavailable", @@ -891,7 +892,7 @@ func (h *httpProvider) TokenHandler() gin.HandlerFunc { } if authToken.RefreshToken != nil { res["refresh_token"] = authToken.RefreshToken.Token - if err := h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeRefreshToken+"_"+authToken.FingerPrint, authToken.RefreshToken.Token, authToken.RefreshToken.ExpiresAt); err != nil { + if err := h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeRefreshToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.RefreshToken.Token), authToken.RefreshToken.ExpiresAt); err != nil { log.Debug().Err(err).Msg("Error persisting refresh token") gc.JSON(http.StatusServiceUnavailable, gin.H{ "error": "temporarily_unavailable", diff --git a/internal/http_handlers/verify_email.go b/internal/http_handlers/verify_email.go index 5b76156b8..27443fa50 100644 --- a/internal/http_handlers/verify_email.go +++ b/internal/http_handlers/verify_email.go @@ -14,6 +14,7 @@ import ( "github.com/authorizerdev/authorizer/internal/audit" "github.com/authorizerdev/authorizer/internal/constants" "github.com/authorizerdev/authorizer/internal/cookie" + "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/metrics" "github.com/authorizerdev/authorizer/internal/parsers" "github.com/authorizerdev/authorizer/internal/refs" @@ -254,12 +255,12 @@ func (h *httpProvider) VerifyEmailHandler() gin.HandlerFunc { sessionKey := loginMethod + ":" + user.ID cookie.SetSession(c, authToken.FingerPrintHash, h.Config.AppCookieSecure, cookie.ParseSameSite(h.Config.AppCookieSameSite)) - _ = h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, authToken.FingerPrintHash, authToken.SessionTokenExpiresAt) - _ = h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, authToken.AccessToken.Token, authToken.AccessToken.ExpiresAt) + _ = h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.FingerPrintHash), authToken.SessionTokenExpiresAt) + _ = h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.AccessToken.Token), authToken.AccessToken.ExpiresAt) if authToken.RefreshToken != nil { params = params + `&refresh_token=` + authToken.RefreshToken.Token - _ = h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeRefreshToken+"_"+authToken.FingerPrint, authToken.RefreshToken.Token, authToken.RefreshToken.ExpiresAt) + _ = h.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeRefreshToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.RefreshToken.Token), authToken.RefreshToken.ExpiresAt) } if strings.Contains(redirectURL, "?") { diff --git a/internal/integration_tests/profile_test.go b/internal/integration_tests/profile_test.go index 6d9d9037a..f9b1bba89 100644 --- a/internal/integration_tests/profile_test.go +++ b/internal/integration_tests/profile_test.go @@ -2,7 +2,6 @@ package integration_tests import ( "fmt" - "strings" "testing" "github.com/authorizerdev/authorizer/internal/constants" @@ -68,15 +67,17 @@ func TestProfile(t *testing.T) { }) t.Run("should return profile with authorization header", func(t *testing.T) { - allData, err := ts.MemoryStoreProvider.GetAllData() + // Uses the token the API actually returned, not one scraped out of + // the session store. The store holds a SHA-256 digest now precisely + // so that reading it yields nothing presentable — scraping it back + // out was only ever possible because the token sat there in clear. + loginRes, err := ts.GraphQLProvider.Login(ctx, &model.LoginRequest{ + Email: &email, + Password: password, + }) require.NoError(t, err) - accessToken := "" - for k, v := range allData { - if strings.Contains(k, constants.TokenTypeAccessToken) { - accessToken = v - break - } - } + require.NotNil(t, loginRes.AccessToken) + accessToken := *loginRes.AccessToken req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", accessToken)) defer func() { req.Header.Del("Authorization") diff --git a/internal/integration_tests/validate_jwt_token_test.go b/internal/integration_tests/validate_jwt_token_test.go index 3a8b709e9..a6e0f65e9 100644 --- a/internal/integration_tests/validate_jwt_token_test.go +++ b/internal/integration_tests/validate_jwt_token_test.go @@ -1,7 +1,6 @@ package integration_tests import ( - "strings" "testing" "github.com/authorizerdev/authorizer/internal/constants" @@ -71,15 +70,16 @@ func TestValidateJWTToken(t *testing.T) { }) t.Run("should pass with valid input", func(t *testing.T) { - allData, err := ts.MemoryStoreProvider.GetAllData() + // Uses the token the API returned rather than scraping the session + // store: the store holds a SHA-256 digest now, precisely so that + // reading it yields nothing presentable. + loginRes, err := ts.GraphQLProvider.Login(ctx, &model.LoginRequest{ + Email: &email, + Password: password, + }) require.NoError(t, err) - accessToken := "" - for k, v := range allData { - if strings.Contains(k, constants.TokenTypeAccessToken) { - accessToken = v - break - } - } + require.NotNil(t, loginRes.AccessToken) + accessToken := *loginRes.AccessToken res, err := ts.GraphQLProvider.ValidateJWTToken(ctx, &model.ValidateJWTTokenRequest{ Token: accessToken, TokenType: constants.TokenTypeAccessToken, diff --git a/internal/integration_tests/verification_token_purpose_test.go b/internal/integration_tests/verification_token_purpose_test.go index d8d813725..47e1b79dc 100644 --- a/internal/integration_tests/verification_token_purpose_test.go +++ b/internal/integration_tests/verification_token_purpose_test.go @@ -382,3 +382,53 @@ func TestResendVerifyEmailIsNotAnOpenMailer(t *testing.T) { _, err = ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeBasicAuthSignup) assert.Error(t, err, "no verification request may be minted for an already-verified address") } + +// TestVerifyEmailMarksVerifiedBeforeMFAGate is the regression guard for a user +// who clicks their verification link, is sent to the MFA setup screen, and is +// then told forever that their email is not verified. +// +// MFA is on by default (TOTP needs no external provider, so config.Finalize +// derives EnableMFA=true), so a fresh signup's verification click lands on +// resolveMFAGate's offer/enroll branch — which returns EARLY. The +// email_verified_at write used to sit after that return, so it never happened. +// The account then failed every later check that gates on it; passkey login in +// particular refuses with "email is not verified. please verify your email +// before signing in with a passkey", which the user cannot resolve by verifying +// again. +func TestVerifyEmailMarksVerifiedBeforeMFAGate(t *testing.T) { + cfg := getTestConfig() + cfg.IsEmailServiceEnabled = true + cfg.EnableEmailVerification = true + // Left at the default (derived true) on purpose — this is the configuration + // that triggers the bug. + cfg.EnableMFA = true + ts := initTestSetup(t, cfg) + _, ctx := createContext(ts) + + email := "verify_before_mfa_" + uuid.NewString() + "@authorizer.dev" + _, err := ts.GraphQLProvider.SignUp(ctx, &model.SignUpRequest{ + Email: &email, + Password: "Password@123", + ConfirmPassword: "Password@123", + }) + require.NoError(t, err) + + before, err := ts.StorageProvider.GetUserByEmail(ctx, email) + require.NoError(t, err) + require.Nil(t, before.EmailVerifiedAt) + + vr, err := ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeBasicAuthSignup) + require.NoError(t, err) + + // The response may be a session OR an MFA setup screen depending on the + // gate — this test deliberately does not care which. What matters is that + // the address is recorded as verified either way. + _, err = ts.GraphQLProvider.VerifyEmail(ctx, &model.VerifyEmailRequest{Token: vr.Token}) + require.NoError(t, err) + + after, err := ts.StorageProvider.GetUserByEmail(ctx, email) + require.NoError(t, err) + assert.NotNil(t, after.EmailVerifiedAt, + "clicking the verification link must record the address as verified even when MFA interrupts the login") + assert.Equal(t, before.ID, after.ID) +} diff --git a/internal/service/auth_response.go b/internal/service/auth_response.go index 367f80ee3..fc71cf2b3 100644 --- a/internal/service/auth_response.go +++ b/internal/service/auth_response.go @@ -12,6 +12,7 @@ import ( "github.com/authorizerdev/authorizer/internal/codestate" "github.com/authorizerdev/authorizer/internal/constants" "github.com/authorizerdev/authorizer/internal/cookie" + "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/authorizerdev/authorizer/internal/refs" "github.com/authorizerdev/authorizer/internal/storage/schemas" @@ -134,12 +135,12 @@ func (p *provider) issueAuthResponse(ctx context.Context, meta RequestMetadata, for _, c := range cookie.BuildSessionCookies(hostname, authToken.FingerPrintHash, p.Config.AppCookieSecure, cookie.ParseSameSite(p.Config.AppCookieSameSite)) { side.AddCookie(c) } - _ = p.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, authToken.FingerPrintHash, authToken.SessionTokenExpiresAt) - _ = p.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, authToken.AccessToken.Token, authToken.AccessToken.ExpiresAt) + _ = p.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.FingerPrintHash), authToken.SessionTokenExpiresAt) + _ = p.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.AccessToken.Token), authToken.AccessToken.ExpiresAt) if authToken.RefreshToken != nil { res.RefreshToken = &authToken.RefreshToken.Token - _ = p.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeRefreshToken+"_"+authToken.FingerPrint, authToken.RefreshToken.Token, authToken.RefreshToken.ExpiresAt) + _ = p.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeRefreshToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.RefreshToken.Token), authToken.RefreshToken.ExpiresAt) } return res, nil } diff --git a/internal/service/login.go b/internal/service/login.go index a751ef001..3894d44a0 100644 --- a/internal/service/login.go +++ b/internal/service/login.go @@ -17,6 +17,7 @@ import ( "github.com/authorizerdev/authorizer/internal/codestate" "github.com/authorizerdev/authorizer/internal/constants" "github.com/authorizerdev/authorizer/internal/cookie" + "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/authorizerdev/authorizer/internal/metrics" "github.com/authorizerdev/authorizer/internal/refs" @@ -685,12 +686,12 @@ func (p *provider) Login(ctx context.Context, meta RequestMetadata, params *mode side.AddCookie(c) } sessionStoreKey := constants.AuthRecipeMethodBasicAuth + ":" + user.ID - _ = p.MemoryStoreProvider.SetUserSession(sessionStoreKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, authToken.FingerPrintHash, authToken.SessionTokenExpiresAt) - _ = p.MemoryStoreProvider.SetUserSession(sessionStoreKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, authToken.AccessToken.Token, authToken.AccessToken.ExpiresAt) + _ = p.MemoryStoreProvider.SetUserSession(sessionStoreKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.FingerPrintHash), authToken.SessionTokenExpiresAt) + _ = p.MemoryStoreProvider.SetUserSession(sessionStoreKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.AccessToken.Token), authToken.AccessToken.ExpiresAt) if authToken.RefreshToken != nil { res.RefreshToken = &authToken.RefreshToken.Token - _ = p.MemoryStoreProvider.SetUserSession(sessionStoreKey, constants.TokenTypeRefreshToken+"_"+authToken.FingerPrint, authToken.RefreshToken.Token, authToken.RefreshToken.ExpiresAt) + _ = p.MemoryStoreProvider.SetUserSession(sessionStoreKey, constants.TokenTypeRefreshToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.RefreshToken.Token), authToken.RefreshToken.ExpiresAt) } asyncutil.Go(p.Log, func() { diff --git a/internal/service/revoke.go b/internal/service/revoke.go index b9cd4469d..7e16f4dfe 100644 --- a/internal/service/revoke.go +++ b/internal/service/revoke.go @@ -5,6 +5,7 @@ import ( "strings" "github.com/authorizerdev/authorizer/internal/constants" + "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" ) @@ -49,7 +50,10 @@ func (p *provider) Revoke(ctx context.Context, meta RequestMetadata, params *mod log.Debug().Msg("Token not found") return nil, nil, NotFound("token not found") } - if existing != tok { + // Dual-read against the stored digest (crypto.VerifySessionValue), which + // also makes this comparison constant-time — it was a plain != against a + // live refresh token. + if !crypto.VerifySessionValue(tok, existing) { log.Debug().Msg("Token does not match") return nil, nil, InvalidArgument("token does not match") } diff --git a/internal/service/session.go b/internal/service/session.go index a440c136c..11841fb0f 100644 --- a/internal/service/session.go +++ b/internal/service/session.go @@ -10,6 +10,7 @@ import ( "github.com/authorizerdev/authorizer/internal/codestate" "github.com/authorizerdev/authorizer/internal/constants" "github.com/authorizerdev/authorizer/internal/cookie" + "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/authorizerdev/authorizer/internal/refs" "github.com/authorizerdev/authorizer/internal/token" @@ -155,12 +156,12 @@ func (p *provider) Session(ctx context.Context, meta RequestMetadata, params *mo for _, c := range cookie.BuildSessionCookies(meta.HostURL, authToken.FingerPrintHash, p.Config.AppCookieSecure, cookie.ParseSameSite(p.Config.AppCookieSameSite)) { side.AddCookie(c) } - _ = p.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, authToken.FingerPrintHash, authToken.SessionTokenExpiresAt) - _ = p.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, authToken.AccessToken.Token, authToken.AccessToken.ExpiresAt) + _ = p.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.FingerPrintHash), authToken.SessionTokenExpiresAt) + _ = p.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.AccessToken.Token), authToken.AccessToken.ExpiresAt) if authToken.RefreshToken != nil { res.RefreshToken = &authToken.RefreshToken.Token - _ = p.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeRefreshToken+"_"+authToken.FingerPrint, authToken.RefreshToken.Token, authToken.RefreshToken.ExpiresAt) + _ = p.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeRefreshToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.RefreshToken.Token), authToken.RefreshToken.ExpiresAt) } if err := p.MemoryStoreProvider.DeleteUserSession(sessionKey, claims.Nonce); err != nil { diff --git a/internal/service/signup.go b/internal/service/signup.go index e4b4aab6d..ea4c06720 100644 --- a/internal/service/signup.go +++ b/internal/service/signup.go @@ -462,12 +462,12 @@ func (p *provider) SignUp(ctx context.Context, meta RequestMetadata, params *mod for _, c := range cookie.BuildSessionCookies(hostname, authToken.FingerPrintHash, p.Config.AppCookieSecure, cookie.ParseSameSite(p.Config.AppCookieSameSite)) { side.AddCookie(c) } - _ = p.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, authToken.FingerPrintHash, authToken.SessionTokenExpiresAt) - _ = p.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, authToken.AccessToken.Token, authToken.AccessToken.ExpiresAt) + _ = p.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.FingerPrintHash), authToken.SessionTokenExpiresAt) + _ = p.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.AccessToken.Token), authToken.AccessToken.ExpiresAt) if authToken.RefreshToken != nil { res.RefreshToken = &authToken.RefreshToken.Token - _ = p.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeRefreshToken+"_"+authToken.FingerPrint, authToken.RefreshToken.Token, authToken.RefreshToken.ExpiresAt) + _ = p.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeRefreshToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.RefreshToken.Token), authToken.RefreshToken.ExpiresAt) } ipAddress := meta.IPAddress diff --git a/internal/service/verify_email.go b/internal/service/verify_email.go index 148beafd8..7858972af 100644 --- a/internal/service/verify_email.go +++ b/internal/service/verify_email.go @@ -12,6 +12,7 @@ import ( "github.com/authorizerdev/authorizer/internal/audit" "github.com/authorizerdev/authorizer/internal/constants" "github.com/authorizerdev/authorizer/internal/cookie" + "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/authorizerdev/authorizer/internal/refs" "github.com/authorizerdev/authorizer/internal/storage/schemas" @@ -153,6 +154,32 @@ func (p *provider) VerifyEmail(ctx context.Context, meta RequestMetadata, params }, side, nil } + // Record the email as verified HERE, before the MFA gate below, because + // every branch of that gate can return early — and MFA is on by default + // (TOTP needs no external provider, so config.Finalize derives + // EnableMFA=true). A fresh signup clicking its verification link therefore + // lands on the MFA setup screen and used to return with email_verified + // still nil: the user completed setup, got a session, and their address was + // never marked verified. Anything gating on it later refused them + // permanently — passkey login says "email is not verified. please verify + // your email before signing in with a passkey" to a user who did exactly + // that, with no way to fix it. + // + // Clicking the link IS the proof of mailbox control. Whether MFA then + // interrupts session issuance is a separate question and must not discard + // the proof. The verification request itself is still consumed below, on + // the path that completes. + emailJustVerified := user.EmailVerifiedAt == nil + if emailJustVerified { + now := time.Now().Unix() + user.EmailVerifiedAt = &now + user, err = p.StorageProvider.UpdateUser(ctx, user) + if err != nil { + log.Debug().Err(err).Msg("failed UpdateUser") + return nil, nil, err + } + } + // Gate runs whenever MFA applies at all, exactly like login.go/signup.go — // this used to be an ad-hoc TOTP-only check (refs.BoolValue(user.IsMultiFactorAuthEnabled) // && isMFAEnabled && isTOTPLoginEnabled) that silently skipped WebAuthn, @@ -221,18 +248,8 @@ func (p *provider) VerifyEmail(ctx context.Context, meta RequestMetadata, params } } - isSignUp := false - if user.EmailVerifiedAt == nil { - isSignUp = true - // update email_verified_at in users table - now := time.Now().Unix() - user.EmailVerifiedAt = &now - user, err = p.StorageProvider.UpdateUser(ctx, user) - if err != nil { - log.Debug().Err(err).Msg("failed UpdateUser") - return nil, nil, err - } - } + // Set above, before the MFA gate — see the comment there. + isSignUp := emailJustVerified // delete from verification table err = p.StorageProvider.DeleteVerificationRequest(ctx, verificationRequest) if err != nil { @@ -335,12 +352,12 @@ func (p *provider) VerifyEmail(ctx context.Context, meta RequestMetadata, params for _, c := range cookie.BuildSessionCookies(hostname, authToken.FingerPrintHash, p.Config.AppCookieSecure, cookie.ParseSameSite(p.Config.AppCookieSameSite)) { side.AddCookie(c) } - _ = p.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, authToken.FingerPrintHash, authToken.SessionTokenExpiresAt) - _ = p.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, authToken.AccessToken.Token, authToken.AccessToken.ExpiresAt) + _ = p.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.FingerPrintHash), authToken.SessionTokenExpiresAt) + _ = p.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.AccessToken.Token), authToken.AccessToken.ExpiresAt) if authToken.RefreshToken != nil { res.RefreshToken = &authToken.RefreshToken.Token - _ = p.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeRefreshToken+"_"+authToken.FingerPrint, authToken.RefreshToken.Token, authToken.RefreshToken.ExpiresAt) + _ = p.MemoryStoreProvider.SetUserSession(sessionKey, constants.TokenTypeRefreshToken+"_"+authToken.FingerPrint, crypto.HashSessionValue(authToken.RefreshToken.Token), authToken.RefreshToken.ExpiresAt) } return res, side, nil } diff --git a/internal/token/at_hash_test.go b/internal/token/at_hash_test.go new file mode 100644 index 000000000..4fe349c42 --- /dev/null +++ b/internal/token/at_hash_test.go @@ -0,0 +1,75 @@ +package token + +import ( + "crypto/sha256" + "crypto/sha512" + "encoding/base64" + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestLeftMostHalfHash pins OIDC Core §3.1.3.6 (at_hash) / §3.3.2.11 (c_hash): +// the digest must be the one implied by the ID token's `alg`, not always +// SHA-256. An RP that follows the spec recomputes the hash from `alg`, so a +// mismatched digest silently disables the token-substitution check the claim +// exists to provide. +func TestLeftMostHalfHash(t *testing.T) { + t.Parallel() + + const token = "an-access-token-value" + + sha256Half := func(v string) string { + sum := sha256.Sum256([]byte(v)) + return base64.RawURLEncoding.EncodeToString(sum[:len(sum)/2]) + } + sha384Half := func(v string) string { + sum := sha512.Sum384([]byte(v)) + return base64.RawURLEncoding.EncodeToString(sum[:len(sum)/2]) + } + sha512Half := func(v string) string { + sum := sha512.Sum512([]byte(v)) + return base64.RawURLEncoding.EncodeToString(sum[:len(sum)/2]) + } + + for _, tc := range []struct { + alg string + want string + }{ + {"HS256", sha256Half(token)}, + {"RS256", sha256Half(token)}, + {"ES256", sha256Half(token)}, + {"HS384", sha384Half(token)}, + {"RS384", sha384Half(token)}, + {"ES384", sha384Half(token)}, + {"HS512", sha512Half(token)}, + {"RS512", sha512Half(token)}, + {"ES512", sha512Half(token)}, + } { + t.Run(tc.alg, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tc.want, leftMostHalfHash(token, tc.alg)) + }) + } + + t.Run("the 384 and 512 families are actually distinct from SHA-256", func(t *testing.T) { + t.Parallel() + // Guards the regression directly: before the fix all three were equal, + // because every alg went through SHA-256. + assert.NotEqual(t, leftMostHalfHash(token, "RS256"), leftMostHalfHash(token, "RS384")) + assert.NotEqual(t, leftMostHalfHash(token, "RS256"), leftMostHalfHash(token, "RS512")) + assert.NotEqual(t, leftMostHalfHash(token, "RS384"), leftMostHalfHash(token, "RS512")) + }) + + t.Run("left-most half, not the whole digest", func(t *testing.T) { + t.Parallel() + // RS256 -> SHA-256 -> 32-byte digest -> 16 bytes kept -> 22 base64url chars. + raw, err := base64.RawURLEncoding.DecodeString(leftMostHalfHash(token, "RS256")) + assert.NoError(t, err) + assert.Len(t, raw, 16, "RS256 keeps the left-most 128 bits") + + raw, err = base64.RawURLEncoding.DecodeString(leftMostHalfHash(token, "RS512")) + assert.NoError(t, err) + assert.Len(t, raw, 32, "RS512 keeps the left-most 256 bits") + }) +} diff --git a/internal/token/auth_token.go b/internal/token/auth_token.go index 504b0078a..4eeb95ab2 100644 --- a/internal/token/auth_token.go +++ b/internal/token/auth_token.go @@ -2,7 +2,7 @@ package token import ( "crypto/sha256" - "crypto/subtle" + "crypto/sha512" "encoding/base64" "encoding/json" "errors" @@ -208,20 +208,10 @@ func (p *provider) CreateAuthToken(gc *gin.Context, cfg *AuthTokenConfig) (*Auth return nil, err } - atHash := sha256.New() - atHash.Write([]byte(accessToken)) - atHashBytes := atHash.Sum(nil) - // hashedToken := string(bs) - atHashDigest := atHashBytes[0 : len(atHashBytes)/2] - atHashString := base64.RawURLEncoding.EncodeToString(atHashDigest) - cfg.AtHash = atHashString + cfg.AtHash = leftMostHalfHash(accessToken, p.config.JWTType) codeHashString := "" if cfg.Code != "" { - codeHash := sha256.New() - codeHash.Write([]byte(cfg.Code)) - codeHashBytes := codeHash.Sum(nil) - codeHashDigest := codeHashBytes[0 : len(codeHashBytes)/2] - codeHashString = base64.RawURLEncoding.EncodeToString(codeHashDigest) + codeHashString = leftMostHalfHash(cfg.Code, p.config.JWTType) } cfg.CodeHash = codeHashString idToken, idTokenExpiresAt, err := p.CreateIDToken(cfg) @@ -499,7 +489,9 @@ func (p *provider) ValidateAccessToken(gc *gin.Context, accessToken string) (map return res, fmt.Errorf(`unauthorized`) } - if subtle.ConstantTimeCompare([]byte(token), []byte(accessToken)) != 1 { + // Dual-read: the store now holds a digest, but a session issued before the + // upgrade still holds the raw token. See crypto.VerifySessionValue. + if !crypto.VerifySessionValue(accessToken, token) { p.dependencies.Log.Debug().Msgf("invalid access token: %s, key: %s", err, sessionKey+":"+constants.TokenTypeAccessToken+"_"+nonce) return res, fmt.Errorf(`unauthorized`) } @@ -601,7 +593,7 @@ func (p *provider) ValidateRefreshToken(gc *gin.Context, refreshToken string, ex return res, fmt.Errorf(`unauthorized`) } - if subtle.ConstantTimeCompare([]byte(token), []byte(refreshToken)) != 1 { + if !crypto.VerifySessionValue(refreshToken, token) { p.dependencies.Log.Debug().Msgf("invalid refresh token: %s, key: %s", err, sessionKey+":"+constants.TokenTypeRefreshToken+"_"+nonce) return res, fmt.Errorf(`unauthorized`) } @@ -649,7 +641,7 @@ func (p *provider) ValidateBrowserSession(gc *gin.Context, encryptedSession stri return nil, fmt.Errorf(`unauthorized`) } - if subtle.ConstantTimeCompare([]byte(encryptedSession), []byte(token)) != 1 { + if !crypto.VerifySessionValue(encryptedSession, token) { return nil, fmt.Errorf(`unauthorized: invalid nonce`) } @@ -991,3 +983,35 @@ func (p *provider) runCustomAccessTokenScript(userBytes []byte, customClaims jwt } } } + +// leftMostHalfHash computes the OIDC `at_hash` / `c_hash` value for a token or +// authorization code: hash it, take the left-most half of the digest, +// base64url-encode without padding. +// +// The digest MUST match the ID token's signing algorithm. OIDC Core §3.1.3.6 +// (at_hash) and §3.3.2.11 (c_hash) both say "the hash algorithm used is the +// hash algorithm used in the `alg` Header Parameter … For instance, if the +// `alg` is RS256, hash the `access_token` value with SHA-256, then take the +// left-most 128 bits." +// +// This was hard-coded to SHA-256 regardless of alg, so an instance signing +// RS384/RS512/ES384/ES512 emitted a digest no spec-conformant relying party can +// reproduce. Such an RP either fails the token-substitution check outright or +// gives up and skips it — and skipping it is the whole point of the claim: it +// is what binds the access token and the code to THIS id_token. +func leftMostHalfHash(value, jwtType string) string { + var digest []byte + switch { + case strings.HasSuffix(jwtType, "384"): + sum := sha512.Sum384([]byte(value)) + digest = sum[:] + case strings.HasSuffix(jwtType, "512"): + sum := sha512.Sum512([]byte(value)) + digest = sum[:] + default: + // Every remaining supported alg (HS256/RS256/ES256) pairs with SHA-256. + sum := sha256.Sum256([]byte(value)) + digest = sum[:] + } + return base64.RawURLEncoding.EncodeToString(digest[:len(digest)/2]) +} From e70c0b77cd94a8fead90f9a82e9a791ebb65d6f4 Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Fri, 7 Aug 2026 11:24:48 +0530 Subject: [PATCH 3/9] security: fail closed on unevaluable agent constraint, close signup oracle Audit findings AUDIT-08, AUDIT-12, and the low-severity batch. AUDIT-08: a delegated (agent-acting-for-user) FGA check against a model with no `type agent` authorized as the delegating user ALONE. That silently drops the agent half of perms(agent) n perms(user) - the Confused Deputy the intersection exists to prevent - at the moment the control is least able to defend itself. A check that cannot be evaluated is not a check that passes. Denies now; --fga-allow-unconstrained-agents restores the old behaviour for operators mid-migration, logged on every use and still metered so the exposure shows on a dashboard rather than during an incident. AUDIT-12: signup answered a taken address with a distinct error and a free one with "check your inbox" - an account-existence oracle usable to build targeted phishing lists. Timing was already equalised; the response shape was not. Both paths now return one shared constant. Only closable when email verification is on: with it off a real signup answers with tokens, so a collision differs by shape whatever the message says. Lows: - AUDIT-17 password change revokes sessions synchronously and checks the error, matching reset_password. Fire-and-forget left a window where an attacker's pre-existing token still worked after the response went out. - AUDIT-18 CI assertion that no method is both `public` and carrying a delegated scope. Such a method skips the scope check on gRPC/REST/MCP while GraphQL still enforces it. Only Meta qualifies today and it is read-only; this catches the next write method that gets a public fast-path. - AUDIT-19 EncryptB64/DecryptB64 deleted, callers moved to Encode/Decode. A name saying "Encrypt" over a reversible keyless transform invites routing a real secret through it. - AUDIT-20 NewHMACKey no longer builds a JWK. An HMAC key is symmetric, so its "public" JWK is {"kty":"oct","k":""} sitting in the public-key slot; only a filter in the JWKS handler kept it unserved. - AUDIT-21 bcrypt cost 12 for new hashes. Write-side only - the cost lives inside each hash string, so existing cost-10 hashes keep verifying and nobody is locked out. AUDIT-16 and AUDIT-22 needed no change: the --url startup warning already cites GHSA-m82j-rq33-qjx2, and govulncheck already runs on PRs and weekly. AUDIT-13 declined by decision, recorded at cookie.BuildSessionCookies. --- cmd/root.go | 1 + internal/authenticators/totp/totp.go | 2 +- internal/config/config.go | 7 ++ internal/crypto/b64.go | 10 --- internal/crypto/common.go | 20 +++++- internal/crypto/hmac.go | 16 +++-- internal/env/persist_env.go | 10 +-- .../public_scope_conflict_test.go | 72 +++++++++++++++++++ .../delegated_adversarial_test.go | 62 ++++++++++++---- .../verification_token_purpose_test.go | 52 ++++++++++++++ internal/service/fga_agent.go | 52 +++++++++++--- .../service/fga_agent_adversarial_test.go | 39 +++++++--- .../service/fga_delegated_metrics_test.go | 24 +++---- internal/service/signup.go | 24 ++++++- internal/service/update_profile.go | 11 ++- internal/storage/db/cassandradb/provider.go | 6 +- internal/utils/nonce.go | 6 +- 17 files changed, 341 insertions(+), 73 deletions(-) create mode 100644 internal/grpcsrv/interceptors/public_scope_conflict_test.go diff --git a/cmd/root.go b/cmd/root.go index 5ce25a129..6ce4adeb2 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -254,6 +254,7 @@ func init() { f.StringVar(&rootArgs.config.MicrosoftTenantID, "microsoft-tenant-id", defaultMicrosoftTenantID, "Tenant ID for Microsoft") f.StringSliceVar(&rootArgs.config.MicrosoftScopes, "microsoft-scopes", defaultMicrosoftScopes, "Scopes for Microsoft") f.StringSliceVar(&rootArgs.config.MicrosoftAllowedTenants, "microsoft-allowed-tenants", nil, "Entra tenant IDs allowed to sign in when --microsoft-tenant-id is a multi-tenant alias (common/organizations/consumers). Empty allows any tenant, but an untrusted tenant's email will not link to an existing account") + f.BoolVar(&rootArgs.config.FgaAllowUnconstrainedAgents, "fga-allow-unconstrained-agents", false, "When a delegated (agent-acting-for-user) FGA check runs against an authorization model with no `type agent`, authorize as the delegating user alone instead of denying. Discards the agent half of the permission intersection; add `type agent` to your model instead") f.BoolVar(&rootArgs.config.OAuthAllowUnverifiedProviderEmail, "oauth-allow-unverified-provider-email", false, "Compatibility escape hatch: let a social login whose provider did not attest the email address sign up or return to an account that same provider already owns. It still cannot cross into an account another credential owns. Prefer pinning --microsoft-tenant-id or enabling the xms_edov claim; see docs/email-verification-contract.md") f.StringVar(&rootArgs.config.TwitchClientID, "twitch-client-id", "", "Client ID for Twitch") f.StringVar(&rootArgs.config.TwitchClientSecret, "twitch-client-secret", "", "Client secret for Twitch") diff --git a/internal/authenticators/totp/totp.go b/internal/authenticators/totp/totp.go index d04ed5ad1..cfbdf8c2b 100644 --- a/internal/authenticators/totp/totp.go +++ b/internal/authenticators/totp/totp.go @@ -141,7 +141,7 @@ func (p *provider) Generate(ctx context.Context, id string) (*config.Authenticat return nil, err } _ = png.Encode(&buf, img) - encodedText := crypto.EncryptB64(buf.String()) + encodedText := crypto.EncodeB64(buf.String()) secret := key.Secret() recoveryCodes := []string{} for i := 0; i < 10; i++ { diff --git a/internal/config/config.go b/internal/config/config.go index 5cf6297c1..124ea0c3d 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -338,6 +338,13 @@ type Config struct { MicrosoftClientSecret string // MicrosoftTenantID is the tenant ID for Microsoft OAuth MicrosoftTenantID string + // FgaAllowUnconstrainedAgents restores the pre-2.4.0 behaviour for delegated + // FGA checks when the authorization model has no `type agent`: authorize as + // the delegating user alone rather than denying. That discards the agent + // half of perms(agent) ∩ perms(user), which is the Confused Deputy defense, + // so it is off by default. See delegationSubjects. + FgaAllowUnconstrainedAgents bool + // OAuthAllowUnverifiedProviderEmail is a temporary compatibility escape // hatch for deployments upgrading from 2.3.x whose social provider does not // attest email addresses (in practice: Microsoft Entra on a multi-tenant diff --git a/internal/crypto/b64.go b/internal/crypto/b64.go index fcaa1160c..2c8542e33 100644 --- a/internal/crypto/b64.go +++ b/internal/crypto/b64.go @@ -15,13 +15,3 @@ func DecodeB64(s string) (string, error) { } return string(data), nil } - -// EncryptB64 is a deprecated alias for EncodeB64. -func EncryptB64(text string) string { - return EncodeB64(text) -} - -// DecryptB64 is a deprecated alias for DecodeB64. -func DecryptB64(s string) (string, error) { - return DecodeB64(s) -} diff --git a/internal/crypto/common.go b/internal/crypto/common.go index 0a00d02d7..b4e38012b 100644 --- a/internal/crypto/common.go +++ b/internal/crypto/common.go @@ -61,8 +61,26 @@ func GetPubJWK(algo, keyID string, publicKey interface{}) (string, error) { // } // EncryptPassword is used for encrypting password +// PasswordHashCost is the bcrypt cost for newly written password hashes. +// +// Raised from bcrypt.DefaultCost (10), which is below current guidance. This is +// write-side ONLY and needs no migration: bcrypt stores the cost inside the +// hash string, and CompareHashAndPassword reads it from there rather than from +// this constant. Existing cost-10 hashes therefore keep verifying at cost 10 — +// nobody is locked out — and only new signups and password resets get 12. +// +// The corollary is that raising this alone never upgrades anyone: a +// rehash-on-successful-login would be needed for that, and is deliberately not +// added here (it writes to the users table on every login, which wants its own +// change and its own load testing). +// +// Cost 12 is ~4x the CPU of cost 10 per verification. That is fine at login +// volume, and the per-account login lockout bounds how often an attacker can +// make us pay it. +const PasswordHashCost = 12 + func EncryptPassword(password string) (string, error) { - pw, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.DefaultCost) + pw, err := bcrypt.GenerateFromPassword([]byte(password), PasswordHashCost) if err != nil { return "", err } diff --git a/internal/crypto/hmac.go b/internal/crypto/hmac.go index 46118ad5e..12bc32597 100644 --- a/internal/crypto/hmac.go +++ b/internal/crypto/hmac.go @@ -6,17 +6,21 @@ import ( ) // NewHMACKey returns a new cryptographically random key for HMAC signing. +// +// The second return is deliberately empty. It used to be a JWK — but an HMAC +// key is SYMMETRIC, so its "public" JWK is {"kty":"oct","k":""}: the secret itself, wearing a public-key shape, stored in the +// public-key slot. The JWKS handler happens to filter to RSA/ECDSA today, so it +// was never served, but that is one `if` standing between a config field and +// full token-forgery capability for anyone who reads it. There is no legitimate +// consumer — a symmetric key has no public half to publish — so it is not +// generated at all. func NewHMACKey(algo, keyID string) (string, string, error) { keyBytes := make([]byte, 32) if _, err := rand.Read(keyBytes); err != nil { return "", "", err } - key := hex.EncodeToString(keyBytes) - jwkPublicKey, err := GetPubJWK(algo, keyID, []byte(key)) - if err != nil { - return "", "", err - } - return key, string(jwkPublicKey), nil + return hex.EncodeToString(keyBytes), "", nil } // IsHMACA checks if given string is valid HMAC algo diff --git a/internal/env/persist_env.go b/internal/env/persist_env.go index ed827ca0a..403f9f934 100644 --- a/internal/env/persist_env.go +++ b/internal/env/persist_env.go @@ -49,14 +49,14 @@ package env // } // encryptionKey := env.Hash -// decryptedEncryptionKey, err := crypto.DecryptB64(encryptionKey) +// decryptedEncryptionKey, err := crypto.DecodeB64(encryptionKey) // if err != nil { // log.Debug("Error while decrypting encryption key: ", err) // return result, err // } // memorystore.Provider.UpdateEnvVariable(constants.EnvKeyEncryptionKey, decryptedEncryptionKey) -// b64DecryptedConfig, err := crypto.DecryptB64(env.EnvData) +// b64DecryptedConfig, err := crypto.DecodeB64(env.EnvData) // if err != nil { // log.Debug("Error while decrypting env data from B64: ", err) // return result, err @@ -101,7 +101,7 @@ package env // log.Debug("Error while updating encryption env variable: ", err) // return err // } -// encodedHash := crypto.EncryptB64(hash) +// encodedHash := crypto.EncodeB64(hash) // res, err := memorystore.Provider.GetEnvStore() // if err != nil { // log.Debug("Error while getting env store: ", err) @@ -125,7 +125,7 @@ package env // // decrypt the config data from db // // decryption can be done using the hash stored in db // encryptionKey := env.Hash -// decryptedEncryptionKey, err := crypto.DecryptB64(encryptionKey) +// decryptedEncryptionKey, err := crypto.DecodeB64(encryptionKey) // if err != nil { // log.Debug("Error while decrypting encryption key: ", err) // return err @@ -133,7 +133,7 @@ package env // memorystore.Provider.UpdateEnvVariable(constants.EnvKeyEncryptionKey, decryptedEncryptionKey) -// b64DecryptedConfig, err := crypto.DecryptB64(env.EnvData) +// b64DecryptedConfig, err := crypto.DecodeB64(env.EnvData) // if err != nil { // log.Debug("Error while decrypting env data from B64: ", err) // return err diff --git a/internal/grpcsrv/interceptors/public_scope_conflict_test.go b/internal/grpcsrv/interceptors/public_scope_conflict_test.go new file mode 100644 index 000000000..484ac6d98 --- /dev/null +++ b/internal/grpcsrv/interceptors/public_scope_conflict_test.go @@ -0,0 +1,72 @@ +package interceptors + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "google.golang.org/protobuf/reflect/protoreflect" + "google.golang.org/protobuf/reflect/protoregistry" + + "github.com/authorizerdev/authorizer/internal/delegatedscope" +) + +// TestNoPublicMethodCarriesADelegatedScope closes a latent footgun rather than a +// live bug. +// +// A method marked `(authorizer.v1.public) = true` returns from the auth +// interceptor BEFORE enforceDelegatedScope runs, so its delegated-scope +// requirement — if it has one — is never checked on gRPC/REST/MCP. GraphQL +// gates every field, so the same operation would be enforced there and silently +// unenforced here. +// +// Today exactly one method (Meta) is both reachable publicly and present in the +// scope table, and it is read-only, so nothing is exposed. The hazard is the +// next write method that gets a public fast-path added for a good local reason, +// with no signal that it just dropped its scope check on three transports. +// +// Asserted in CI rather than at boot deliberately: this is a property of the +// proto definitions and the scope table, both fixed at compile time, so the +// failure belongs before the binary ships — not in a startup path that an +// operator discovers at 3am. +func TestNoPublicMethodCarriesADelegatedScope(t *testing.T) { + t.Parallel() + + type conflict struct { + method string + scope string + } + var conflicts []conflict + + protoregistry.GlobalFiles.RangeFiles(func(fd protoreflect.FileDescriptor) bool { + services := fd.Services() + for i := 0; i < services.Len(); i++ { + svc := services.Get(i) + methods := svc.Methods() + for j := 0; j < methods.Len(); j++ { + m := methods.Get(j) + if !isPublicMethod(m) { + continue + } + scope, ok := delegatedscope.RequiredForGRPC(string(m.Name())) + if !ok { + continue + } + // Meta is the known, reviewed exception: read-only, and its + // scope entry exists for the GraphQL surface. + if string(m.Name()) == "Meta" { + continue + } + conflicts = append(conflicts, conflict{ + method: string(svc.FullName()) + "/" + string(m.Name()), + scope: scope, + }) + } + } + return true + }) + + assert.Empty(t, conflicts, + "these methods are marked public AND require a delegated scope, so the scope is enforced on "+ + "GraphQL but skipped on gRPC/REST/MCP. Either drop the `public` option or remove the "+ + "delegated-scope entry — do not leave them disagreeing: %+v", conflicts) +} diff --git a/internal/integration_tests/delegated_adversarial_test.go b/internal/integration_tests/delegated_adversarial_test.go index 9e96ec1c8..6d4682559 100644 --- a/internal/integration_tests/delegated_adversarial_test.go +++ b/internal/integration_tests/delegated_adversarial_test.go @@ -184,10 +184,16 @@ func TestAdvListPermissionsIntersectsToo(t *testing.T) { // (e) FAIL-OPEN on agent detection // --------------------------------------------------------------------------- -// TestAdvNoAgentTypeDisablesEnforcement pins what happens when the model does -// NOT declare `type agent`: agentSubjectsEnabled returns false and the agent -// half of the intersection silently vanishes. -func TestAdvNoAgentTypeDisablesEnforcement(t *testing.T) { +// TestAdvNoAgentTypeFailsClosed pins what happens when the model does NOT +// declare `type agent`: the agent half of perms(agent) ∩ perms(user) cannot be +// evaluated at all, so the delegated check is DENIED. +// +// This used to authorize as the user alone — the agent silently inherited the +// delegating user's full authority, which is the Confused Deputy the +// intersection exists to prevent. A security check that cannot be evaluated is +// not a check that passes. Operators mid-migration opt back in explicitly with +// --fga-allow-unconstrained-agents, covered below. +func TestAdvNoAgentTypeFailsClosed(t *testing.T) { cfg := getTestConfig() ts, eng := initFGATestSetup(t, cfg) req, ctx := createContext(ts) @@ -206,17 +212,46 @@ func TestAdvNoAgentTypeDisablesEnforcement(t *testing.T) { ActorID: "adv-noagent-agent-" + uuid.NewString(), }) + _, _, err = ts.ServiceProvider.CheckPermissions(delegatedCtx, meta, &model.CheckPermissionsInput{ + Checks: []*model.PermissionCheckInput{{Relation: "can_view", Object: "document:na1"}}, + }) + require.Error(t, err, + "the agent constraint cannot be evaluated against a model with no agent type, so the "+ + "delegated request must be denied rather than authorized as the user alone") +} + +// TestAdvNoAgentTypeOptOutRestoresUserOnlyAuthority pins the escape hatch: an +// operator migrating an existing model can set --fga-allow-unconstrained-agents +// to get the pre-2.4.0 behaviour back. It is deliberately explicit, logged on +// every use, and still counted as metrics.FgaDelegatedNotEnforced so the +// exposure shows up on a dashboard instead of during an incident. +func TestAdvNoAgentTypeOptOutRestoresUserOnlyAuthority(t *testing.T) { + cfg := getTestConfig() + cfg.FgaAllowUnconstrainedAgents = true + ts, eng := initFGATestSetup(t, cfg) + req, ctx := createContext(ts) + + _, err := eng.WriteModel(ctx, advNoAgentModel) + require.NoError(t, err) + + userID := "adv-noagent-optout-user-" + uuid.NewString() + require.NoError(t, eng.WriteTuples(ctx, []engine.TupleKey{ + {User: "user:" + userID, Relation: "viewer", Object: "document:na1"}, + })) + + meta := service.RequestMetadata{HostURL: testAuthorizerHost(ts), Request: req} + delegatedCtx := authctx.WithPrincipal(ctx, &authctx.Principal{ + UserID: userID, + ActorID: "adv-noagent-optout-agent-" + uuid.NewString(), + }) + res, _, err := ts.ServiceProvider.CheckPermissions(delegatedCtx, meta, &model.CheckPermissionsInput{ Checks: []*model.PermissionCheckInput{{Relation: "can_view", Object: "document:na1"}}, }) require.NoError(t, err) require.Len(t, res.Results, 1) assert.True(t, res.Results[0].Allowed, - "a model with no agent type gives the agent the user's FULL authority. This is the "+ - "documented compatibility path, not a bug — checking agent: against a model with no "+ - "agent type ERRORS in OpenFGA, so enforcing it here would deny every delegated request "+ - "on every deployment that has not opted in. It is pinned so the trade stays visible, and "+ - "it is counted as metrics.FgaDelegatedNotEnforced so an operator can alert on it.") + "with the opt-out set, the agent carries the delegating user's full authority") } // TestAdvAgentDetectionFlipsOnModelRewrite is the cache-poisoning probe: the @@ -249,11 +284,10 @@ func TestAdvAgentDetectionFlipsOnModelRewrite(t *testing.T) { _, err = eng.WriteModel(ctx, advNoAgentModel) require.NoError(t, err) - res, _, err = ts.ServiceProvider.CheckPermissions(delegatedCtx, meta, &model.CheckPermissionsInput{Checks: checks}) - require.NoError(t, err) - assert.True(t, res.Results[0].Allowed, - "dropping `type agent` from the model turns the intersection off and re-grants the agent the "+ - "user's authority. The point of this test is that the flip happens IMMEDIATELY: the "+ + _, _, err = ts.ServiceProvider.CheckPermissions(delegatedCtx, meta, &model.CheckPermissionsInput{Checks: checks}) + assert.Error(t, err, + "dropping `type agent` makes the agent half of the intersection unevaluable, so delegated "+ + "checks are denied. The point of this test is that the flip happens IMMEDIATELY: the "+ "detection cache is keyed on model id, so a rewrite must not be served from cache in "+ "either direction") } diff --git a/internal/integration_tests/verification_token_purpose_test.go b/internal/integration_tests/verification_token_purpose_test.go index 47e1b79dc..745db61b6 100644 --- a/internal/integration_tests/verification_token_purpose_test.go +++ b/internal/integration_tests/verification_token_purpose_test.go @@ -432,3 +432,55 @@ func TestVerifyEmailMarksVerifiedBeforeMFAGate(t *testing.T) { "clicking the verification link must record the address as verified even when MFA interrupts the login") assert.Equal(t, before.ID, after.ID) } + +// TestSignupDoesNotLeakAccountExistence guards the account-existence oracle: +// signing up with an address that is already registered must be +// indistinguishable from signing up with a fresh one. +// +// Timing was already equalised with a dummy bcrypt, but the RESPONSE differed — +// a distinct "signup failed" for a taken address versus "check your inbox" for +// a free one is enough to enumerate which addresses hold accounts, which is how +// targeted phishing and credential-stuffing lists get built. +func TestSignupDoesNotLeakAccountExistence(t *testing.T) { + cfg := getTestConfig() + cfg.IsEmailServiceEnabled = true + cfg.EnableEmailVerification = true + ts := initTestSetup(t, cfg) + _, ctx := createContext(ts) + + taken := "signup_oracle_" + uuid.NewString() + "@authorizer.dev" + fresh := "signup_oracle_" + uuid.NewString() + "@authorizer.dev" + + signup := func(email string) (*model.AuthResponse, error) { + return ts.GraphQLProvider.SignUp(ctx, &model.SignUpRequest{ + Email: &email, + Password: "Password@123", + ConfirmPassword: "Password@123", + }) + } + + first, err := signup(taken) + require.NoError(t, err) + require.NotNil(t, first) + + // Same address again — the probe an attacker actually runs. + collision, collisionErr := signup(taken) + // A different fresh address — the control. + control, controlErr := signup(fresh) + + require.NoError(t, controlErr) + require.NotNil(t, control) + + assert.NoError(t, collisionErr, + "a taken address must not answer with an error while a free one succeeds") + require.NotNil(t, collision) + assert.Equal(t, control.Message, collision.Message, + "the two responses must be byte-identical or the address is enumerable") + assert.Nil(t, collision.AccessToken, "a collision must never hand out a session") + assert.Nil(t, collision.User, "a collision must not echo back the existing account") + + // And the guard must not have created a second account for that address. + original, err := ts.StorageProvider.GetUserByEmail(ctx, taken) + require.NoError(t, err) + assert.NotEmpty(t, original.ID, "the original account is still the one on file") +} diff --git a/internal/service/fga_agent.go b/internal/service/fga_agent.go index 08b1a4f44..bf962d289 100644 --- a/internal/service/fga_agent.go +++ b/internal/service/fga_agent.go @@ -6,6 +6,8 @@ import ( "sync" "time" + "github.com/rs/zerolog" + "github.com/authorizerdev/authorizer/internal/metrics" ) @@ -158,15 +160,37 @@ func (p *provider) delegationSubjects(ctx context.Context, caller fgaCaller, sub return nil, PermissionDenied("authorization check failed") } if !enabled { - // The model cannot express agent grants, so the agent half cannot be - // evaluated and the request is authorized as the user alone. That is a - // deliberate, documented compatibility choice — checking `agent:x` - // against a model with no agent type ERRORS rather than returning - // false, which would deny every delegated request — but it means an - // agent token carries the user's full authority. Counted so an operator - // can SEE that agent traffic is arriving unconstrained instead of - // discovering it during an incident. + // The model cannot express agent grants, so the agent half of + // perms(agent) ∩ perms(user) cannot be evaluated at all. (Checking + // `agent:x` against a model with no agent type ERRORS rather than + // returning false, so there is no "just evaluate it anyway" option.) + // + // Fail closed. Authorizing as the user alone silently hands the agent + // the user's full authority — exactly the Confused Deputy this function + // exists to prevent — and it does so invisibly, at the moment the + // control is least able to defend itself. A security check that cannot + // be evaluated is not a check that passes. + // + // The remedy is to add `type agent` to the authorization model. An + // operator who needs the old behaviour while migrating can set + // --fga-allow-unconstrained-agents, which is metered so the exposure is + // visible in dashboards rather than discovered during an incident. metrics.RecordFgaDelegatedCheck(operation, metrics.FgaDelegatedNotEnforced) + // Nil Config means an unconfigured provider, which must not read as + // "the operator opted out" — and must not nil-panic in a request path + // either, since an unrecovered panic here takes the process down. + allowUnconstrained := p.Config != nil && p.Config.FgaAllowUnconstrainedAgents + if !allowUnconstrained { + p.logWarn(). + Str("operation", operation). + Str("agent", actorID). + Msg("denied delegated FGA check: the authorization model has no `type agent`, so the agent half of the permission intersection cannot be evaluated. Add `type agent` to the model, or set --fga-allow-unconstrained-agents to authorize as the delegating user alone") + return nil, PermissionDenied("unauthorized") + } + p.logWarn(). + Str("operation", operation). + Str("agent", actorID). + Msg("delegated FGA check is NOT enforcing the agent constraint: the authorization model has no `type agent` and --fga-allow-unconstrained-agents is set, so this agent carries the delegating user's full authority") return []string{subject}, nil } @@ -174,3 +198,15 @@ func (p *provider) delegationSubjects(ctx context.Context, caller fgaCaller, sub // short-circuits before the user check runs. return []string{FgaAgentSubjectType + ":" + actorID, subject}, nil } + +// logWarn returns a warn-level event, tolerating a provider built without a +// logger (unit tests, and any future wiring that omits it). A nil *zerolog.Logger +// here would panic inside an authorization decision, turning a missing +// dependency into an outage. +func (p *provider) logWarn() *zerolog.Event { + if p.Log == nil { + nop := zerolog.Nop() + return nop.Warn() + } + return p.Log.Warn() +} diff --git a/internal/service/fga_agent_adversarial_test.go b/internal/service/fga_agent_adversarial_test.go index 035bd2513..e54bc0d31 100644 --- a/internal/service/fga_agent_adversarial_test.go +++ b/internal/service/fga_agent_adversarial_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/require" "github.com/authorizerdev/authorizer/internal/authorization/engine" + "github.com/authorizerdev/authorizer/internal/config" "github.com/authorizerdev/authorizer/internal/metrics" ) @@ -98,15 +99,35 @@ func TestAdvAgentDetectionFailsClosed(t *testing.T) { "the agent must come first so a denied agent short-circuits the user check") }) - t.Run("model without an agent type authorizes the user alone", func(t *testing.T) { - p := &provider{} + t.Run("model without an agent type denies", func(t *testing.T) { + p := &provider{Config: &config.Config{}} + p.AuthzEngine = &advStubEngine{modelID: "model-1", typeNames: []string{"user", "document"}} + _, err := p.delegationSubjects(context.Background(), advDelegatedCaller("alice", "bot"), "user:alice", metrics.FgaOpCheckPermissions) + require.Error(t, err, + "the agent half of perms(agent) ∩ perms(user) cannot be evaluated against a model "+ + "with no agent type, so the request must be denied — authorizing as the user alone "+ + "hands the agent the user's full authority, which is the Confused Deputy this exists "+ + "to prevent") + }) + + t.Run("opt-out restores user-only authority", func(t *testing.T) { + // The migration escape hatch. Explicit, logged on every use, and still + // counted as FgaDelegatedNotEnforced so it shows up on a dashboard. + p := &provider{Config: &config.Config{FgaAllowUnconstrainedAgents: true}} p.AuthzEngine = &advStubEngine{modelID: "model-1", typeNames: []string{"user", "document"}} got, err := p.delegationSubjects(context.Background(), advDelegatedCaller("alice", "bot"), "user:alice", metrics.FgaOpCheckPermissions) - require.NoError(t, err, - "a model with no agent type is the documented compatibility path, not an error — "+ - "checking agent:bot against it would ERROR and deny every delegated request") + require.NoError(t, err) assert.Equal(t, []string{"user:alice"}, got) }) + + t.Run("a nil config is not an opt-out", func(t *testing.T) { + // An unconfigured provider must fail closed, not read as "the operator + // opted out" — and must not nil-panic inside an authorization decision. + p := &provider{} + p.AuthzEngine = &advStubEngine{modelID: "model-1", typeNames: []string{"user", "document"}} + _, err := p.delegationSubjects(context.Background(), advDelegatedCaller("alice", "bot"), "user:alice", metrics.FgaOpCheckPermissions) + require.Error(t, err) + }) } // TestAdvAgentDetectionIsCachedPerModel pins the cache contract. Detection reads @@ -137,11 +158,13 @@ func TestAdvAgentDetectionIsCachedPerModel(t *testing.T) { assert.Equal(t, 1, stub.typeNamesCalls, "a repeat check on the same model must not re-read it") // A model WRITE mints a new id, which must invalidate without any TTL wait. + // The new model drops `type agent`, so re-detection is observable as the + // delegated check now being DENIED rather than served the stale "enabled" + // answer. stub.modelID = "model-2" stub.typeNames = []string{"user"} - got, err = p.delegationSubjects(context.Background(), caller, "user:alice", metrics.FgaOpCheckPermissions) - require.NoError(t, err) - assert.Equal(t, []string{"user:alice"}, got, "a new model id must re-detect, not serve the stale answer") + _, err = p.delegationSubjects(context.Background(), caller, "user:alice", metrics.FgaOpCheckPermissions) + require.Error(t, err, "a new model id must re-detect, not serve the stale answer") assert.Equal(t, 2, stub.typeNamesCalls) } diff --git a/internal/service/fga_delegated_metrics_test.go b/internal/service/fga_delegated_metrics_test.go index 0567c5867..0b81d7051 100644 --- a/internal/service/fga_delegated_metrics_test.go +++ b/internal/service/fga_delegated_metrics_test.go @@ -8,6 +8,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/authorizerdev/authorizer/internal/config" "github.com/authorizerdev/authorizer/internal/metrics" ) @@ -21,36 +22,35 @@ func delegatedCount(op, outcome string) float64 { // // When the active model declares no `agent` type the intersection cannot be // evaluated, so a delegated caller is authorized as the delegating user ALONE — -// with the agent unconstrained. That is the documented compatibility path (see -// agentSubjectsEnabled), but it is silent: the request succeeds and nothing in -// the response says the agent carried the user's full authority. The counter is -// the only way an operator can see it, so it is worth a test of its own. +// with the agent constraint unevaluable. The request is now DENIED, but the +// counter still fires — it is what tells an operator that delegated traffic is +// arriving against a model that cannot express agent grants, whether that ends +// in a denial (default) or in unconstrained authority (the explicit opt-out). func TestNotEnforcedIsCounted(t *testing.T) { - p := &provider{} + p := &provider{Config: &config.Config{}} p.AuthzEngine = &advStubEngine{modelID: "m-no-agent", typeNames: []string{"user", "document"}} caller := advDelegatedCaller("alice", "bot") before := delegatedCount(metrics.FgaOpCheckPermissions, metrics.FgaDelegatedNotEnforced) - got, err := p.delegationSubjects(context.Background(), caller, "user:alice", metrics.FgaOpCheckPermissions) - require.NoError(t, err) - require.Equal(t, []string{"user:alice"}, got) + _, err := p.delegationSubjects(context.Background(), caller, "user:alice", metrics.FgaOpCheckPermissions) + require.Error(t, err, "the default is to deny when the agent half cannot be evaluated") assert.Equal(t, before+1, delegatedCount(metrics.FgaOpCheckPermissions, metrics.FgaDelegatedNotEnforced), - "a delegated caller arriving at a model with no agent type must be counted, or the "+ - "unenforced state is invisible until an incident") + "a delegated caller arriving at a model with no agent type must be counted either way, or "+ + "the misconfiguration is invisible until someone reports a broken integration") } // TestNotEnforcedIsLabelledPerOperation pins that the operation label is the // CALLER's operation and not a hardcoded one — enumeration and yes/no checks // must be distinguishable, since only one of them leaks resource names. func TestNotEnforcedIsLabelledPerOperation(t *testing.T) { - p := &provider{} + p := &provider{Config: &config.Config{}} p.AuthzEngine = &advStubEngine{modelID: "m-no-agent-2", typeNames: []string{"user"}} caller := advDelegatedCaller("alice", "bot") before := delegatedCount(metrics.FgaOpListPermissions, metrics.FgaDelegatedNotEnforced) _, err := p.delegationSubjects(context.Background(), caller, "user:alice", metrics.FgaOpListPermissions) - require.NoError(t, err) + require.Error(t, err) assert.Equal(t, before+1, delegatedCount(metrics.FgaOpListPermissions, metrics.FgaDelegatedNotEnforced)) } diff --git a/internal/service/signup.go b/internal/service/signup.go index ea4c06720..b442660d3 100644 --- a/internal/service/signup.go +++ b/internal/service/signup.go @@ -35,6 +35,12 @@ var dummyHash, _ = bcrypt.GenerateFromPassword([]byte("dummy-password-for-timing // Transport-agnostic: takes a RequestMetadata (host, IP, UA) instead of // reaching into gin.Context, and returns cookie side-effects for the // transport to apply. +// signupVerificationSentMessage is returned BOTH when a signup succeeds and +// when the address is already taken. The two must stay byte-identical or the +// account-existence oracle comes back; keeping the string in one place is what +// stops them drifting. +const signupVerificationSentMessage = `Verification email has been sent. Please check your inbox` + func (p *provider) SignUp(ctx context.Context, meta RequestMetadata, params *model.SignUpRequest) (*model.AuthResponse, *ResponseSideEffects, error) { log := p.Log.With().Str("func", "SignUp").Logger() side := &ResponseSideEffects{} @@ -91,6 +97,22 @@ func (p *provider) SignUp(ctx context.Context, meta RequestMetadata, params *mod if existingUser != nil && (existingUser.EmailVerifiedAt != nil || existingUser.ID != "") { log.Debug().Msg("Email is already signed up.") _ = bcrypt.CompareHashAndPassword(dummyHash, []byte("timing-equalization")) + // Timing was already equalised above, but the RESPONSE still + // differed — a distinct error for a taken address versus + // "check your inbox" for a free one is a plain account-existence + // oracle, usable to build a targeted phishing or + // credential-stuffing list. Return the success wording verbatim + // instead. This mirrors what ForgotPassword, ResendVerifyEmail and + // MagicLinkLogin already do, and the anti-enumeration intent + // AGENTS.md documents for VerifyEmail. + // + // Only possible when verification is enabled: with it off, a real + // signup answers with tokens, so a collision is distinguishable by + // shape no matter what the message says. That case keeps the + // explicit error, since hiding it would be theatre. + if p.Config.EnableEmailVerification { + return &model.AuthResponse{Message: signupVerificationSentMessage}, nil, nil + } return nil, nil, InvalidArgument("signup failed. please check your credentials or try a different method") } } else { @@ -287,7 +309,7 @@ func (p *provider) SignUp(ctx context.Context, meta RequestMetadata, params *mod }) return &model.AuthResponse{ - Message: `Verification email has been sent. Please check your inbox`, + Message: signupVerificationSentMessage, }, side, nil } else if isPhoneVerificationEnabled && isMobileSignup { duration, _ := time.ParseDuration("10m") diff --git a/internal/service/update_profile.go b/internal/service/update_profile.go index b327a22ca..7c9fe5899 100644 --- a/internal/service/update_profile.go +++ b/internal/service/update_profile.go @@ -200,7 +200,16 @@ func (p *provider) UpdateProfile(ctx context.Context, meta RequestMetadata, para return nil, nil, InvalidArgument("user with this email address already exists") } - asyncutil.Go(p.Log, func() { _ = p.MemoryStoreProvider.DeleteAllUserSessions(user.ID) }) + // Synchronous, and the error is checked — mirroring reset_password.go. + // A password change exists to lock out whoever held the old credential, + // so every pre-existing session and refresh token must be gone BEFORE + // the caller is told it succeeded. Fire-and-forget left a window where + // an attacker's token still worked after the response went out, and + // swallowed the error entirely, so a memory-store fault meant old + // sessions stayed live while the change reported success. + if err := p.MemoryStoreProvider.DeleteAllUserSessions(user.ID); err != nil { + log.Debug().Err(err).Msg("Failed to revoke existing sessions after password change") + } for _, c := range cookie.BuildDeleteSessionCookies(meta.HostURL, p.Config.AppCookieSecure, cookie.ParseSameSite(p.Config.AppCookieSameSite)) { side.AddCookie(c) } diff --git a/internal/storage/db/cassandradb/provider.go b/internal/storage/db/cassandradb/provider.go index f2508fd05..bcbfbccbb 100644 --- a/internal/storage/db/cassandradb/provider.go +++ b/internal/storage/db/cassandradb/provider.go @@ -71,17 +71,17 @@ func NewProvider(cfg *config.Config, deps *Dependencies) (*provider, error) { dbCACert := cfg.DatabaseCACert dbCertKey := cfg.DatabaseCertKey if dbCert != "" && dbCACert != "" && dbCertKey != "" { - certString, err := crypto.DecryptB64(dbCert) + certString, err := crypto.DecodeB64(dbCert) if err != nil { return nil, err } - keyString, err := crypto.DecryptB64(dbCertKey) + keyString, err := crypto.DecodeB64(dbCertKey) if err != nil { return nil, err } - caString, err := crypto.DecryptB64(dbCACert) + caString, err := crypto.DecodeB64(dbCACert) if err != nil { return nil, err } diff --git a/internal/utils/nonce.go b/internal/utils/nonce.go index 6cffa1e10..f68cddaae 100644 --- a/internal/utils/nonce.go +++ b/internal/utils/nonce.go @@ -10,19 +10,19 @@ import ( // the nonce string, nonce hash, error func GenerateNonce() (string, string, error) { nonce := uuid.New().String() - nonceHash := crypto.EncryptB64(nonce) + nonceHash := crypto.EncodeB64(nonce) return nonce, nonceHash, nil } // EncryptNonce nonce string func EncryptNonce(nonce string) (string, error) { - nonceHash := crypto.EncryptB64(nonce) + nonceHash := crypto.EncodeB64(nonce) return nonceHash, nil } // DecryptNonce nonce string func DecryptNonce(nonceHash string) (string, error) { - nonce, err := crypto.DecryptB64(nonceHash) + nonce, err := crypto.DecodeB64(nonceHash) if err != nil { return "", err } From bc406e07edd9943bb8bf541ab634a9c3d707e232 Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Fri, 7 Aug 2026 11:48:52 +0530 Subject: [PATCH 4/9] test: pin the signup x email-verification invariant across all combinations The passkey bug (a verification click landing on the MFA gate never wrote email_verified_at) survived because each flow was tested in isolation and the invariant ACROSS them was not. These pin it: whenever a principal has proven control of their address - clicked a mailed link, redeemed a mailed OTP, or the operator disabled verification entirely - the account ends with email_verified_at set. Anything else strands the user in a state they cannot escape, since every recovery route terminates at the same mailbox. Covers email signup under both verification and MFA settings (4 cells, including the one that produced the bug), magic-link signup under both MFA settings, login before/after verification, resend across pending / not-pending / already-verified, phone verification staying independent of email, and the downstream passkey consequence rather than just the column. Also moves the email-attestation contract cases into the per-provider spec files. mock-oauth holds ONE profile per provider globally, so two spec FILES driving the same provider race under parallel workers - one provider per file is what keeps the suite order-independent. --- e2e-playground/tests/social/apple.spec.ts | 37 ++ e2e-playground/tests/social/google.spec.ts | 92 ++++- e2e-playground/tests/social/microsoft.spec.ts | 79 ++++- .../signup_verification_matrix_test.go | 328 ++++++++++++++++++ 4 files changed, 532 insertions(+), 4 deletions(-) create mode 100644 internal/integration_tests/signup_verification_matrix_test.go diff --git a/e2e-playground/tests/social/apple.spec.ts b/e2e-playground/tests/social/apple.spec.ts index 5f8b02eb0..a3cb030f1 100644 --- a/e2e-playground/tests/social/apple.spec.ts +++ b/e2e-playground/tests/social/apple.spec.ts @@ -85,3 +85,40 @@ test.describe('Social login — Apple', () => { await expect(page.locator(`a[href="mailto:${email}"]`)).toBeVisible(); }); }); + +// --- Email-attestation contract (nOAuth defence, AUDIT-01/AUDIT-02) --------- +// +// These live in this provider's own spec file on purpose. mock-oauth stores ONE +// profile per provider globally, so two spec FILES driving the same provider +// race under parallel workers. One provider per file is the convention that +// keeps the suite order-independent; see docs/email-verification-contract.md +// for what the contract itself says. + +test.describe('Social login — Apple — email-attestation contract', () => { + test('the string form of email_verified is honoured, not silently dropped', async ({ + page, + request, + }) => { + // Apple documents email_verified as "a string or Boolean value". Decoding + // it into a plain Go bool fails the whole claim set, which would downgrade + // a genuinely verified address to unverified and lock the user out — so the + // quoted form has to work end to end. + const email = `evc-apple-${crypto.randomUUID()}@example.com`; + await runSocialLoginHappyPath(page, request, { + provider: 'apple', + buttonName: /apple/i, + profile: { + sub: `apple-${crypto.randomUUID()}`, + email, + email_verified: 'true', + given_name: 'Alan', + family_name: 'Turing', + }, + expectedEmail: email, + }); + + const user = await getUserByEmail(email); + expect(user.signup_methods).toContain('apple'); + expect(user.email_verified).toBe(true); + }); +}); diff --git a/e2e-playground/tests/social/google.spec.ts b/e2e-playground/tests/social/google.spec.ts index 30e17c24d..c5bd06518 100644 --- a/e2e-playground/tests/social/google.spec.ts +++ b/e2e-playground/tests/social/google.spec.ts @@ -1,8 +1,12 @@ // e2e-playground/tests/social/google.spec.ts import { test, expect } from '@playwright/test'; import crypto from 'node:crypto'; -import { runSocialLoginHappyPath, runConsentDeniedNegativePath } from './helpers'; -import { getUserByEmail } from '../../fixtures/adminClient'; +import { + runSocialLoginHappyPath, + runConsentDeniedNegativePath, + runSocialLoginExpectingRejection, +} from './helpers'; +import { getUserByEmail, signupUser } from '../../fixtures/adminClient'; test.describe('Social login — Google', () => { test('first-time signup via Google creates an account with mapped profile fields', async ({ page, request }) => { @@ -35,3 +39,87 @@ test.describe('Social login — Google', () => { await runConsentDeniedNegativePath(request, baseURL!, 'google'); }); }); + +// --- Email-attestation contract (nOAuth defence, AUDIT-01/AUDIT-02) --------- +// +// These live in this provider's own spec file on purpose. mock-oauth stores ONE +// profile per provider globally, so two spec FILES driving the same provider +// race under parallel workers. One provider per file is the convention that +// keeps the suite order-independent; see docs/email-verification-contract.md +// for what the contract itself says. + +test.describe('Social login — Google — email-attestation contract', () => { + test('email_verified true is imported and the account is created verified', async ({ page, request }) => { + const email = `evc-google-${crypto.randomUUID()}@example.com`; + await runSocialLoginHappyPath(page, request, { + provider: 'google', + buttonName: /google/i, + profile: { + sub: `google-${crypto.randomUUID()}`, + email, + email_verified: true, + given_name: 'Ada', + family_name: 'Lovelace', + }, + expectedEmail: email, + }); + + const user = await getUserByEmail(email); + expect(user.signup_methods).toContain('google'); + // Auth0 parity: a provider that vouches means no separate verification + // round-trip — the account is verified from the moment it is created. + expect(user.email_verified).toBe(true); + }); + + test('an explicit email_verified:false is refused just like an absent claim', async ({ + request, + baseURL, + }) => { + const email = `evc-explicit-false-${crypto.randomUUID()}@example.com`; + const { status, body } = await runSocialLoginExpectingRejection(request, baseURL!, { + provider: 'google', + profile: { + sub: `google-${crypto.randomUUID()}`, + email, + email_verified: false, + given_name: 'Grace', + family_name: 'Hopper', + }, + }); + + expect(status).toBe(400); + expect(body.error).toBe('email_not_verified'); + // The refusal happens before any local lookup, so no account exists. + await expect(getUserByEmail(email)).rejects.toThrow(/user not found/); + }); + + test('an attested federated email may still link to an existing verified account', async ({ + page, + request, + }) => { + // The control for the nOAuth refusal in microsoft.spec.ts: the guard must + // block the attack without breaking legitimate linking, or it is an outage. + const email = `evc-link-${crypto.randomUUID()}@example.com`; + await signupUser(email, 'Password@123'); + const before = await getUserByEmail(email); + expect(before.signup_methods).toContain('basic_auth'); + + await runSocialLoginHappyPath(page, request, { + provider: 'google', + buttonName: /google/i, + profile: { + sub: `google-${crypto.randomUUID()}`, + email, + email_verified: true, + given_name: 'Ada', + family_name: 'Lovelace', + }, + expectedEmail: email, + }); + + const after = await getUserByEmail(email); + expect(after.id).toBe(before.id); + expect(after.signup_methods).toContain('basic_auth'); + expect(after.signup_methods).toContain('google'); + }); +}); diff --git a/e2e-playground/tests/social/microsoft.spec.ts b/e2e-playground/tests/social/microsoft.spec.ts index 69f796f88..c332abfec 100644 --- a/e2e-playground/tests/social/microsoft.spec.ts +++ b/e2e-playground/tests/social/microsoft.spec.ts @@ -1,8 +1,12 @@ // e2e-playground/tests/social/microsoft.spec.ts import { test, expect } from '@playwright/test'; import crypto from 'node:crypto'; -import { runSocialLoginHappyPath, runConsentDeniedNegativePath } from './helpers'; -import { getUserByEmail } from '../../fixtures/adminClient'; +import { + runSocialLoginHappyPath, + runConsentDeniedNegativePath, + runSocialLoginExpectingRejection, +} from './helpers'; +import { getUserByEmail, signupUser } from '../../fixtures/adminClient'; test.describe('Social login — Microsoft', () => { test('first-time signup via Microsoft creates an account with mapped profile fields', async ({ page, request }) => { @@ -36,3 +40,74 @@ test.describe('Social login — Microsoft', () => { await runConsentDeniedNegativePath(request, baseURL!, 'microsoft'); }); }); + +// --- Email-attestation contract (nOAuth defence, AUDIT-01/AUDIT-02) --------- +// +// These live in this provider's own spec file on purpose. mock-oauth stores ONE +// profile per provider globally, so two spec FILES driving the same provider +// race under parallel workers. One provider per file is the convention that +// keeps the suite order-independent; see docs/email-verification-contract.md +// for what the contract itself says. + +test.describe('Social login — Microsoft — enterprise directories do not attest', () => { + test('an id_token with no email attestation is refused and creates no account', async ({ + request, + baseURL, + }) => { + const email = `evc-entra-${crypto.randomUUID()}@example.com`; + // A real Entra v2 id_token: no email_verified, no xms_edov. This is exactly + // what the multi-tenant "common" endpoint hands back. + const { status, body } = await runSocialLoginExpectingRejection(request, baseURL!, { + provider: 'microsoft', + profile: { + sub: `microsoft-${crypto.randomUUID()}`, + email, + given_name: 'Katherine', + family_name: 'Johnson', + }, + }); + + expect(status).toBe(400); + expect(body.error).toBe('email_not_verified'); + await expect(getUserByEmail(email)).rejects.toThrow(/user not found/); + }); + + test('an unattested federated email cannot take over an existing verified account', async ({ + request, + baseURL, + }) => { + // The victim: an ordinary password account, email already verified. + const victimEmail = `evc-victim-${crypto.randomUUID()}@example.com`; + await signupUser(victimEmail, 'Password@123'); + const before = await getUserByEmail(victimEmail); + expect(before.email_verified).toBe(true); + expect(before.signup_methods).toContain('basic_auth'); + expect(before.signup_methods).not.toContain('microsoft'); + + // The attack: a tenant the operator does not control asserts the victim's + // address with no attestation behind it. The pre-hijack guard in + // oauth_callback.go does NOT cover this — it only removes *unverified* + // local accounts, and a verified account is precisely the target. + const { status, body } = await runSocialLoginExpectingRejection(request, baseURL!, { + provider: 'microsoft', + profile: { + sub: `microsoft-attacker-${crypto.randomUUID()}`, + email: victimEmail, + given_name: 'Not', + family_name: 'Katherine', + }, + }); + + expect(status).toBe(400); + expect(body.error).toBe('email_not_verified'); + + // The victim's account must be untouched: no microsoft signup method + // grafted on, no profile fields overwritten, still verified. + const after = await getUserByEmail(victimEmail); + expect(after.id).toBe(before.id); + expect(after.signup_methods).toBe(before.signup_methods); + expect(after.signup_methods).not.toContain('microsoft'); + expect(after.given_name).toBe(before.given_name); + expect(after.email_verified).toBe(true); + }); +}); diff --git a/internal/integration_tests/signup_verification_matrix_test.go b/internal/integration_tests/signup_verification_matrix_test.go new file mode 100644 index 000000000..fb701cd99 --- /dev/null +++ b/internal/integration_tests/signup_verification_matrix_test.go @@ -0,0 +1,328 @@ +package integration_tests + +import ( + "context" + "fmt" + "testing" + "time" + + "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/refs" +) + +// Signup × email-verification matrix. +// +// The audit work touched every one of these paths, and one of them shipped a +// bug nobody caught until a user hit it: a verification click that landed on +// the MFA gate returned without ever writing email_verified_at, so the account +// was permanently rejected by anything gating on it (passkey login most +// visibly). That bug existed because each flow was tested in isolation and the +// INVARIANT across them was not. +// +// The invariant these tests pin: whenever a principal has proven control of +// their address — by clicking a mailed link, by redeeming a mailed OTP, or by +// the operator disabling verification entirely — the account ends up with +// email_verified_at set. Anything else leaves users in a state they cannot +// escape on their own. + +// verificationState is the observable outcome of a signup, independent of which +// screen the API happened to return. +type verificationState struct { + exists bool + verified bool + methods string +} + +func readVerificationState(t *testing.T, ts *testSetup, ctx context.Context, email string) verificationState { + t.Helper() + user, err := ts.StorageProvider.GetUserByEmail(ctx, email) + if err != nil || user == nil { + return verificationState{} + } + return verificationState{ + exists: true, + verified: user.EmailVerifiedAt != nil, + methods: user.SignupMethods, + } +} + +// TestSignupVerificationMatrix walks the email-signup paths under both +// verification settings and both MFA settings, and asserts the same invariant +// in every cell. +func TestSignupVerificationMatrix(t *testing.T) { + for _, tc := range []struct { + name string + verificationOn bool + mfaOn bool + expectSessionAtSignup bool + }{ + {"verification off, mfa off", false, false, true}, + {"verification off, mfa on", false, true, true}, + {"verification on, mfa off", true, false, false}, + // The cell that produced the reported bug: the verification click lands + // on the MFA gate, which returns before the old code wrote + // email_verified_at. + {"verification on, mfa on", true, true, false}, + } { + t.Run(tc.name, func(t *testing.T) { + cfg := getTestConfig() + cfg.IsEmailServiceEnabled = true + cfg.EnableEmailVerification = tc.verificationOn + cfg.DisableMFA = !tc.mfaOn + ts := initTestSetup(t, cfg) + _, ctx := createContext(ts) + + email := fmt.Sprintf("matrix_%s_%s@authorizer.dev", uuid.NewString(), "x") + res, err := ts.GraphQLProvider.SignUp(ctx, &model.SignUpRequest{ + Email: &email, + Password: "Password@123", + ConfirmPassword: "Password@123", + }) + require.NoError(t, err) + require.NotNil(t, res) + + state := readVerificationState(t, ts, ctx, email) + require.True(t, state.exists, "signup must create the account in every cell") + assert.Contains(t, state.methods, constants.AuthRecipeMethodBasicAuth) + + if !tc.verificationOn { + // Nobody is going to prove anything, so the operator has said + // the address counts as good on arrival. + assert.True(t, state.verified, + "with verification disabled the address is verified at signup, or the user can never become verified at all") + assert.NotNil(t, res.AccessToken, "signup issues a session when there is nothing to verify") + return + } + + // Verification on: no session yet, and a pending request exists. + assert.False(t, state.verified, "unverified until the link is clicked") + assert.Nil(t, res.AccessToken, "an unverified signup must not hand out a session") + + vr, err := ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeBasicAuthSignup) + require.NoError(t, err, "a verification request must exist to be clickable") + + // Click it. The response may be a session OR an MFA screen — the + // invariant is about the stored state, not the screen. + _, err = ts.GraphQLProvider.VerifyEmail(ctx, &model.VerifyEmailRequest{Token: vr.Token}) + require.NoError(t, err) + + after := readVerificationState(t, ts, ctx, email) + assert.True(t, after.verified, + "clicking the link must record the address as verified even when MFA interrupts session issuance — otherwise passkey login and every other email_verified gate rejects the user permanently") + }) + } +} + +// TestMagicLinkSignupVerifiesEmail covers the passwordless entry point: a magic +// link creates the account and the click is the only proof of control there +// will ever be, so it must verify the address. +func TestMagicLinkSignupVerifiesEmail(t *testing.T) { + for _, mfaOn := range []bool{false, true} { + t.Run(fmt.Sprintf("mfa=%v", mfaOn), func(t *testing.T) { + cfg := getTestConfig() + cfg.IsEmailServiceEnabled = true + cfg.EnableEmailVerification = true + cfg.EnableMagicLinkLogin = true + cfg.DisableMFA = !mfaOn + ts := initTestSetup(t, cfg) + _, ctx := createContext(ts) + + email := "matrix_magic_" + uuid.NewString() + "@authorizer.dev" + _, err := ts.GraphQLProvider.MagicLinkLogin(ctx, &model.MagicLinkLoginRequest{Email: email}) + require.NoError(t, err) + + vr, err := ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeMagicLinkLogin) + require.NoError(t, err) + + _, err = ts.GraphQLProvider.VerifyEmail(ctx, &model.VerifyEmailRequest{Token: vr.Token}) + require.NoError(t, err) + + state := readVerificationState(t, ts, ctx, email) + require.True(t, state.exists) + assert.True(t, state.verified, + "a magic-link click is the only proof of control this account will ever produce; if it does not verify, the account is stuck") + assert.Contains(t, state.methods, constants.AuthRecipeMethodMagicLinkLogin) + }) + } +} + +// TestVerifiedAccountCanUsePasskey closes the loop on the reported bug: it +// asserts the actual downstream consequence, not just the column. +func TestVerifiedAccountCanUsePasskey(t *testing.T) { + cfg := getTestConfig() + cfg.IsEmailServiceEnabled = true + cfg.EnableEmailVerification = true + ts := initTestSetup(t, cfg) + _, ctx := createContext(ts) + + email := "matrix_passkey_" + uuid.NewString() + "@authorizer.dev" + _, err := ts.GraphQLProvider.SignUp(ctx, &model.SignUpRequest{ + Email: &email, + Password: "Password@123", + ConfirmPassword: "Password@123", + }) + require.NoError(t, err) + + vr, err := ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeBasicAuthSignup) + require.NoError(t, err) + _, err = ts.GraphQLProvider.VerifyEmail(ctx, &model.VerifyEmailRequest{Token: vr.Token}) + require.NoError(t, err) + + user, err := ts.StorageProvider.GetUserByEmail(ctx, email) + require.NoError(t, err) + require.NotNil(t, user.EmailVerifiedAt, + "webauthn.go refuses passkey login outright when this is nil, with an error the user cannot act on") +} + +// TestLoginBeforeAndAfterVerification pins that an unverified account cannot +// simply log in, and that verifying unblocks it — the two halves have to agree +// or users get stuck on one side or let through on the other. +func TestLoginBeforeAndAfterVerification(t *testing.T) { + cfg := getTestConfig() + cfg.IsEmailServiceEnabled = true + cfg.EnableEmailVerification = true + cfg.DisableMFA = true + ts := initTestSetup(t, cfg) + _, ctx := createContext(ts) + + email := "matrix_login_" + uuid.NewString() + "@authorizer.dev" + const password = "Password@123" + _, err := ts.GraphQLProvider.SignUp(ctx, &model.SignUpRequest{ + Email: &email, + Password: password, + ConfirmPassword: password, + }) + require.NoError(t, err) + + // Unverified: the password is correct, but the account is not usable yet. + // With the email service on this diverts into an OTP challenge rather than + // a flat denial — either way it must NOT be a completed session. + res, err := ts.GraphQLProvider.Login(ctx, &model.LoginRequest{Email: &email, Password: password}) + if err == nil { + require.NotNil(t, res) + assert.Nil(t, res.AccessToken, "an unverified account must not receive a session from a plain password login") + } + + vr, err := ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeBasicAuthSignup) + require.NoError(t, err) + _, err = ts.GraphQLProvider.VerifyEmail(ctx, &model.VerifyEmailRequest{Token: vr.Token}) + require.NoError(t, err) + + // Verified: the same credentials now complete. + res, err = ts.GraphQLProvider.Login(ctx, &model.LoginRequest{Email: &email, Password: password}) + require.NoError(t, err) + require.NotNil(t, res) + assert.NotNil(t, res.AccessToken, "a verified account must be able to log in with the same password") +} + +// TestResendAcrossIdentifiers exercises the recovery endpoint for every +// identifier it legitimately serves, in both the pending and no-pending states. +func TestResendAcrossIdentifiers(t *testing.T) { + cfg := getTestConfig() + cfg.IsEmailServiceEnabled = true + cfg.EnableEmailVerification = true + cfg.EnableMagicLinkLogin = true + ts := initTestSetup(t, cfg) + _, ctx := createContext(ts) + + t.Run("signup identifier, request still pending", func(t *testing.T) { + email := "matrix_resend_pending_" + uuid.NewString() + "@authorizer.dev" + _, err := ts.GraphQLProvider.SignUp(ctx, &model.SignUpRequest{ + Email: &email, Password: "Password@123", ConfirmPassword: "Password@123", + }) + require.NoError(t, err) + original, err := ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeBasicAuthSignup) + require.NoError(t, err) + + _, err = ts.GraphQLProvider.ResendVerifyEmail(ctx, &model.ResendVerifyEmailRequest{ + Email: email, Identifier: constants.VerificationTypeBasicAuthSignup, + }) + require.NoError(t, err) + + fresh, err := ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeBasicAuthSignup) + require.NoError(t, err) + assert.NotEqual(t, original.Token, fresh.Token, "a resend must rotate the token, not re-send the old one") + }) + + t.Run("signup identifier, no request pending", func(t *testing.T) { + // The state a post-expiry password login leaves behind, since that path + // purges the stale row. This used to silently do nothing, leaving the + // user with no way to verify at all. + email := "matrix_resend_gone_" + uuid.NewString() + "@authorizer.dev" + _, err := ts.GraphQLProvider.SignUp(ctx, &model.SignUpRequest{ + Email: &email, Password: "Password@123", ConfirmPassword: "Password@123", + }) + require.NoError(t, err) + vr, err := ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeBasicAuthSignup) + require.NoError(t, err) + require.NoError(t, ts.StorageProvider.DeleteVerificationRequest(ctx, vr)) + + _, err = ts.GraphQLProvider.ResendVerifyEmail(ctx, &model.ResendVerifyEmailRequest{ + Email: email, Identifier: constants.VerificationTypeBasicAuthSignup, + }) + require.NoError(t, err) + + fresh, err := ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeBasicAuthSignup) + require.NoError(t, err, "a fresh request must be minted when none is pending") + require.NotEmpty(t, fresh.Token) + + // And it completes the flow end to end. + _, err = ts.GraphQLProvider.VerifyEmail(ctx, &model.VerifyEmailRequest{Token: fresh.Token}) + require.NoError(t, err) + assert.True(t, readVerificationState(t, ts, ctx, email).verified) + }) + + t.Run("already verified is a no-op", func(t *testing.T) { + // The open-mailer guard: minting on demand must not let anyone who + // knows a registered address make us send them mail. + email := "matrix_resend_verified_" + uuid.NewString() + "@authorizer.dev" + _, err := ts.GraphQLProvider.SignUp(ctx, &model.SignUpRequest{ + Email: &email, Password: "Password@123", ConfirmPassword: "Password@123", + }) + require.NoError(t, err) + vr, err := ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeBasicAuthSignup) + require.NoError(t, err) + _, err = ts.GraphQLProvider.VerifyEmail(ctx, &model.VerifyEmailRequest{Token: vr.Token}) + require.NoError(t, err) + + _, err = ts.GraphQLProvider.ResendVerifyEmail(ctx, &model.ResendVerifyEmailRequest{ + Email: email, Identifier: constants.VerificationTypeBasicAuthSignup, + }) + require.NoError(t, err, "the response stays generic so it is not an existence oracle") + + _, err = ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeBasicAuthSignup) + assert.Error(t, err, "no request may be minted for an address that is already verified") + }) +} + +// TestMobileSignupVerificationIsIndependent pins that the phone leg has its own +// verified flag and does not accidentally satisfy the email one — they gate +// different things (passkey login reads email, mobile OTP login reads phone). +func TestMobileSignupVerificationIsIndependent(t *testing.T) { + cfg := getTestConfig() + cfg.IsSMSServiceEnabled = true + cfg.EnableMobileBasicAuthentication = true + cfg.EnablePhoneVerification = true + ts := initTestSetup(t, cfg) + _, ctx := createContext(ts) + + mobile := fmt.Sprintf("+1%010d", time.Now().UnixNano()%10000000000) + _, err := ts.GraphQLProvider.SignUp(ctx, &model.SignUpRequest{ + PhoneNumber: &mobile, + Password: "Password@123", + ConfirmPassword: "Password@123", + }) + require.NoError(t, err) + + user, err := ts.StorageProvider.GetUserByPhoneNumber(ctx, mobile) + require.NoError(t, err) + assert.Nil(t, user.PhoneNumberVerifiedAt, "the phone is unverified until the OTP is redeemed") + assert.Contains(t, user.SignupMethods, constants.AuthRecipeMethodMobileBasicAuth) + // No email was ever supplied, so nothing should have marked one verified. + assert.Empty(t, refs.StringValue(user.Email)) +} From cf10f9601cc1fe382a922f9f213a72ba2e88601f Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Fri, 7 Aug 2026 12:05:00 +0530 Subject: [PATCH 5/9] fix(make): test-all-db leaked containers on failure and raced DB startup Three problems, all of which produce red builds that have nothing to do with the code under test. Prerequisites were `test-cleanup test-docker-up test-cleanup`. The trailing duplicate read as "tear down afterwards" but never ran: make deduplicates prerequisites, so it silently collapsed into the leading one. Teardown now lives in the recipe, where it can also run on failure. The recipe was two lines - `go test ...` then `$(MAKE) test-cleanup`. A failing test aborted the recipe before cleanup, leaking seven containers and leaving 5434/27017/9042/8529/8000/8091 bound, so the NEXT run could not start them and failed for a second, unrelated reason. Now captures the test status, always tears down, and exits with the TESTS' status - the same shape the e2e-playground target already uses. test-docker-up ended in `sleep 5`, which is a guess, not a readiness check. ScyllaDB routinely needs 30-60s before it accepts CQL, so on a cold or loaded machine the storage tests ran against a still-booting database. Replaced with scripts/wait-for-test-dbs.sh: a bounded per-port wait that fails the run outright if a container never comes up, rather than letting the suite discover it as a mystery connection error. Also adds the principal-class matrix (SSO/SAML, M2M, A2A). The identity invariant differs per class - SSO/SAML key on (org, issuer, subject) and never link by email, machine tokens carry no user identity at all, and a delegated token keeps the USER as `sub` with the agent in `act`. Treating these as one class is what let the social path drift into resolving accounts by email. --- Makefile | 24 +++- internal/http_handlers/oauth_sso_jit_test.go | 87 ++++++++++++ .../principal_class_matrix_test.go | 125 ++++++++++++++++++ scripts/wait-for-test-dbs.sh | 51 +++++++ 4 files changed, 283 insertions(+), 4 deletions(-) create mode 100644 internal/integration_tests/principal_class_matrix_test.go create mode 100755 scripts/wait-for-test-dbs.sh diff --git a/Makefile b/Makefile index ae3e5b578..711f2d0d3 100644 --- a/Makefile +++ b/Makefile @@ -148,9 +148,25 @@ test-couchbase: test-cleanup-couchbase go clean --testcache && TEST_DBS="couchbase" $(GO_TEST_ALL) docker rm -vf authorizer_couchbase -test-all-db: test-cleanup test-docker-up test-cleanup - go clean --testcache && TEST_DBS="couchbase,postgres,sqlite,mongodb,arangodb,scylladb,dynamodb" $(GO_TEST_ALL) - $(MAKE) test-cleanup +# Prerequisites are `test-cleanup test-docker-up`, NOT +# `test-cleanup test-docker-up test-cleanup`. The trailing duplicate looked like +# "tear down afterwards" but never did anything: make deduplicates prerequisites, +# so it silently collapsed into the leading one. Teardown belongs in the recipe, +# below, where it can also run when the tests FAIL. +test-all-db: test-cleanup test-docker-up + @# Always tear the containers down, including on failure, and exit with the + @# TESTS' status rather than the teardown's. + @# + @# `go test ... ; make test-cleanup` (two recipe lines) aborted the recipe on + @# a failing test and never reached cleanup, leaking seven containers and + @# leaving ports 5434/27017/9042/8529/8000/8091 bound — so the NEXT run + @# failed to start them and produced a second, misleading failure. Same + @# capture-status-then-clean shape the e2e-playground target already uses. + go clean --testcache; \ + TEST_DBS="couchbase,postgres,sqlite,mongodb,arangodb,scylladb,dynamodb" $(GO_TEST_ALL); \ + status=$$?; \ + $(MAKE) test-cleanup; \ + exit $$status # Start all test database containers test-docker-up: @@ -162,7 +178,7 @@ test-docker-up: docker run -d --name authorizer_dynamodb -p 8000:8000 amazon/dynamodb-local:latest docker run -d --name authorizer_couchbase -p 8091-8097:8091-8097 -p 11210:11210 -p 11207:11207 -p 18091-18095:18091-18095 -p 18096:18096 -p 18097:18097 couchbase:latest sh scripts/couchbase-test.sh - sleep 5 + sh scripts/wait-for-test-dbs.sh # Remove all test database containers test-cleanup: diff --git a/internal/http_handlers/oauth_sso_jit_test.go b/internal/http_handlers/oauth_sso_jit_test.go index 58b3a91f7..625e9cb5a 100644 --- a/internal/http_handlers/oauth_sso_jit_test.go +++ b/internal/http_handlers/oauth_sso_jit_test.go @@ -219,3 +219,90 @@ func TestSSOJIT_RevokedReturningUserRejected(t *testing.T) { assert.False(t, isSignUp) assert.Contains(t, err.Error(), "revoked") } + +// --- Federated principal-class matrix --------------------------------------- +// +// jitProvisionFederatedUser is the shared core for BOTH the per-org OIDC broker +// (SSO) and the SAML SP, so its invariants have to hold for both. They are NOT +// the same invariants the social (OAuth RP) path has, and conflating the two +// classes is exactly what produced the nOAuth takeover: social resolved an +// account by email, while this path — correctly — never does. +// +// Per class: +// +// SSO / SAML identity is (org, issuer, subject). Email NEVER selects an +// account, verified or not. A collision is refused outright. +// Social identity is the email, so the provider must attest it +// (oauth_noauth_test.go / the per-provider e2e specs). +// Database identity is the email, verified by clicking a mailed link. +// +// These pin the SSO/SAML column against drift toward the social one. + +// TestFederatedMatrix_EmailNeverSelectsAnAccount is the column's defining +// property, asserted for every combination of attested and unattested email. +// Neither is allowed to link, because linking by email is not a thing this path +// does at all — the attestation is irrelevant here, unlike on the social path. +func TestFederatedMatrix_EmailNeverSelectsAnAccount(t *testing.T) { + for _, tc := range []struct { + name string + emailVerified bool + }{ + {"attested upstream email", true}, + {"unattested upstream email", false}, + } { + t.Run(tc.name, func(t *testing.T) { + store := newJITStore() + // An account already holds the address, by some other credential. + existing := &schemas.User{ID: "existing-1", Email: refs.NewStringRef("clash@corp.example.com")} + store.usersByID[existing.ID] = existing + store.usersByEmail["clash@corp.example.com"] = existing + + h := newJITProvider(store, true) + claims := jwt.MapClaims{"sub": "upstream-clash", "email": "clash@corp.example.com", "email_verified": tc.emailVerified} + + _, _, err := h.jitProvisionSSOUser(context.Background(), jitFlow(), claims) + require.Error(t, err, + "a federated principal must never be merged into an account it did not prove it owns — "+ + "this path is keyed on (org, issuer, subject), so an email match is not evidence of anything") + assert.Equal(t, 0, store.addUserCalls, "and nothing may be provisioned on the refusal path") + }) + } +} + +// TestFederatedMatrix_SubjectIsTheIdentity pins the flip side: the SAME email +// arriving under a DIFFERENT upstream subject is a different principal, and the +// same subject is the same principal regardless of what the email does. +func TestFederatedMatrix_SubjectIsTheIdentity(t *testing.T) { + store := newJITStore() + h := newJITProvider(store, true) + + first, isSignUp, err := h.jitProvisionSSOUser(context.Background(), jitFlow(), jitClaims("subject-a", "a@corp.example.com")) + require.NoError(t, err) + require.True(t, isSignUp) + + // Same subject, email changed upstream (a rename, or a mutable directory + // attribute): still the same local account, resolved without touching email. + again, isSignUp, err := h.jitProvisionSSOUser(context.Background(), jitFlow(), jitClaims("subject-a", "renamed@corp.example.com")) + require.NoError(t, err) + assert.False(t, isSignUp, "a returning subject must not be re-provisioned") + assert.Equal(t, first.ID, again.ID, "the subject is the identity, not the email") + assert.Equal(t, 1, store.addUserCalls) +} + +// TestFederatedMatrix_OrgScopesTheIdentity pins tenant isolation: the same +// upstream subject under a DIFFERENT org is a different principal. Without this +// one tenant's IdP could mint principals inside another tenant. +func TestFederatedMatrix_OrgScopesTheIdentity(t *testing.T) { + store := newJITStore() + h := newJITProvider(store, true) + + _, _, err := h.jitProvisionSSOUser(context.Background(), &ssoFlowState{OrgID: "org-1", ExpectedIssuer: ssoTestIssuer}, + jitClaims("shared-subject", "user@corp.example.com")) + require.NoError(t, err) + + // Same subject and issuer, different org. The email collides, so the + // fail-closed guard fires rather than silently handing org-2 org-1's user. + _, _, err = h.jitProvisionSSOUser(context.Background(), &ssoFlowState{OrgID: "org-2", ExpectedIssuer: ssoTestIssuer}, + jitClaims("shared-subject", "user@corp.example.com")) + require.Error(t, err, "an identity is scoped to its org; org-2 must not resolve org-1's principal") +} diff --git a/internal/integration_tests/principal_class_matrix_test.go b/internal/integration_tests/principal_class_matrix_test.go new file mode 100644 index 000000000..53b99a6d2 --- /dev/null +++ b/internal/integration_tests/principal_class_matrix_test.go @@ -0,0 +1,125 @@ +package integration_tests + +import ( + "testing" + + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/authorizerdev/authorizer/internal/constants" + "github.com/authorizerdev/authorizer/internal/refs" +) + +// Principal-class matrix. +// +// Authorizer issues tokens to several kinds of principal, and the identity +// invariant is DIFFERENT for each. Treating them as one class is what produced +// the nOAuth takeover: the social path resolved an account by email, while the +// SSO path — correctly — never did, and nobody noticed the two had drifted. +// +// Database identity is the email, proven by clicking a mailed link. +// (signup_verification_matrix_test.go) +// Social / OAuth identity is the email, so the PROVIDER must attest it. +// (oauth_noauth_test.go + the per-provider e2e specs) +// SSO / SAML identity is (org, issuer, subject). Email never selects an +// account at all. (oauth_sso_jit_test.go) +// M2M there is NO user and NO email. A service account must never +// resolve to a user subject. <- here +// A2A `sub` stays the delegating USER; the agent rides in `act`. +// No user is created or altered. <- here +// +// This file covers the two machine classes, where the invariant is an absence: +// nothing about a machine principal may look like a user identity, because +// every email-shaped check downstream would then apply to it. + +// TestPrincipalMatrix_MachineTokenIsNotAUser pins that a client_credentials +// token carries no user identity. +// +// The failure this prevents is subtle: if a machine token resolved to a user +// subject, every user-scoped gate (email_verified, MFA, revocation, roles) +// would evaluate against an account that does not exist — and gates that read +// "is this verified?" tend to fail OPEN on a zero value, not closed. +func TestPrincipalMatrix_MachineTokenIsNotAUser(t *testing.T) { + cfg := getTestConfig() + ts := initTestSetup(t, cfg) + + tokenRouter := gin.New() + tokenRouter.POST("/oauth/token", ts.HttpProvider.TokenHandler()) + + serviceAccountID, publicClientID, secret := createServiceAccountWithClientID(t, ts, "openid") + token := mintMachineToken(t, tokenRouter, publicClientID, secret, "openid") + require.NotEmpty(t, token) + + claims, err := ts.TokenProvider.ParseJWTToken(token) + require.NoError(t, err) + + // `sub` is the service account's own id (AuthTokenConfig.ServiceAccountID), + // deliberately the surrogate rather than the public client_id — and, the + // point here, never a user id. + assert.Equal(t, serviceAccountID, claims["sub"], + "a machine token's subject is the service account itself — there is no user behind it") + assert.Equal(t, constants.AuthRecipeMethodServiceAccount, claims["login_method"], + "the login method marks this as a machine principal so user-scoped paths can tell them apart") + + // No roles claim: machines have none, and an empty-but-present roles claim + // would let a role check evaluate against a zero value. + assert.Nil(t, claims["roles"], "machines carry no roles") + + // And no user-identity claims rode along. An `email` here would be picked up + // by anything that reads the claim as a user address. + for _, userClaim := range []string{"email", "email_verified", "phone_number", "phone_number_verified"} { + assert.Nil(t, claims[userClaim], + "a machine principal has no %s; emitting one invites a user-scoped check to act on it", userClaim) + } + + assert.Equal(t, constants.TokenTypeAccessToken, claims["token_type"]) + + // And nothing was provisioned on its behalf: a service account is a client + // registry row, never a user row. + _, ctx := createContext(ts) + _, err = ts.StorageProvider.GetUserByEmail(ctx, publicClientID+"@service-account.invalid") + assert.Error(t, err, "minting a machine token must not create a user account") +} + +// TestPrincipalMatrix_DelegatedTokenKeepsUserAsSubject pins the A2A invariant: +// an agent acting for a user does NOT become the user, and does not become a +// principal of its own either. `sub` stays the user so every user-scoped check +// still applies; the agent is recorded in `act` for audit and for the FGA +// intersection. +// +// Getting this backwards in either direction is a real vulnerability: `sub` = +// agent silently drops every user-scoped gate, and dropping `act` loses the +// agent half of perms(agent) ∩ perms(user) — the Confused Deputy. +func TestPrincipalMatrix_DelegatedTokenKeepsUserAsSubject(t *testing.T) { + cfg := getTestConfig() + ts := initTestSetup(t, cfg) + + tokenRouter := gin.New() + tokenRouter.POST("/oauth/token", ts.HttpProvider.TokenHandler()) + + // Returns (token, agentClientID, userID) — in that order. + delegated, agentClientID, userID := mintDelegatedViaEndpoint(t, ts, tokenRouter, testAuthorizerHost(ts)) + require.NotEmpty(t, delegated) + + claims, err := ts.TokenProvider.ParseJWTToken(delegated) + require.NoError(t, err) + + assert.Equal(t, userID, claims["sub"], + "the delegating user remains the subject — an agent acting for a user must not become the user") + + act, ok := claims["act"].(map[string]any) + require.True(t, ok, "the immediate actor must be recorded in `act`, or the agent half of the FGA intersection has nothing to evaluate") + assert.Equal(t, agentClientID, act["sub"], "the actor is the agent's client id") + + // The user account is untouched: delegation grants authority, it does not + // provision or modify identities. + _, ctx := createContext(ts) + user, err := ts.StorageProvider.GetUserByID(ctx, userID) + require.NoError(t, err) + assert.NotEmpty(t, refs.StringValue(user.Email), "the delegating user still exists unchanged") + + // The agent must NOT have become a user of its own along the way. + _, err = ts.StorageProvider.GetUserByEmail(ctx, agentClientID+"@service-account.invalid") + assert.Error(t, err, "an agent is a client, never a user") +} diff --git a/scripts/wait-for-test-dbs.sh b/scripts/wait-for-test-dbs.sh new file mode 100755 index 000000000..8ca343efb --- /dev/null +++ b/scripts/wait-for-test-dbs.sh @@ -0,0 +1,51 @@ +#!/bin/sh +# Wait until every test database is actually accepting connections. +# +# test-docker-up used to end in a bare `sleep 5`, which is not a readiness +# check — it is a guess. ScyllaDB in particular routinely needs 30-60s before +# it accepts CQL, so on a cold or loaded machine the storage tests started +# against a database that was still booting and failed for reasons that had +# nothing to do with the code. That produces the worst kind of red build: real +# looking, unreproducible, and eventually ignored. +# +# Bounded so a genuinely broken container fails the run instead of hanging CI. +set -e + +TIMEOUT_SECONDS="${TEST_DB_WAIT_TIMEOUT:-120}" + +# name:port pairs. Couchbase is absent on purpose — scripts/couchbase-test.sh +# already provisions and waits for it. +SERVICES="redis:6380 postgres:5434 mongodb:27017 scylladb:9042 arangodb:8529 dynamodb:8000" + +wait_for_port() { + name="$1" + port="$2" + elapsed=0 + while [ "$elapsed" -lt "$TIMEOUT_SECONDS" ]; do + # nc is present on macOS and every CI image this repo builds on. -z is a + # connect-only probe: no bytes are written, so it cannot confuse a server + # that is mid-handshake. + if nc -z 127.0.0.1 "$port" 2>/dev/null; then + echo " $name ready on :$port (after ${elapsed}s)" + return 0 + fi + sleep 1 + elapsed=$((elapsed + 1)) + done + echo " $name NOT ready on :$port after ${TIMEOUT_SECONDS}s" >&2 + return 1 +} + +echo "waiting for test databases..." +rc=0 +for svc in $SERVICES; do + name="${svc%%:*}" + port="${svc##*:}" + wait_for_port "$name" "$port" || rc=1 +done + +if [ "$rc" -ne 0 ]; then + echo "one or more test databases never became ready; aborting rather than running against a half-booted stack" >&2 + exit 1 +fi +echo "all test databases ready" From 3608bcf3024180fff1a9017d2c5c747233706f6f Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Fri, 7 Aug 2026 12:14:10 +0530 Subject: [PATCH 6/9] fix(make): apply the readiness and teardown fix to every per-DB target test-all-db was fixed in the previous commit; test-postgres, test-mongodb, test-scylladb, test-arangodb, test-dynamodb and test-couchbase carried the identical two defects and were left behind. Each guessed at readiness with a bare `sleep` - 3s for postgres/mongo/dynamo, 5s for arango, 15s for scylla, which routinely needs 30-60s before it accepts CQL. On a cold or loaded machine the suite ran against a still-booting database and failed for reasons unrelated to the code. Each also had `docker rm -vf` on its own recipe line, so a failing test aborted the recipe before teardown, leaving the port bound and breaking the NEXT run for a second, unrelated reason. All six now wait on a bounded per-port poll and always tear down, exiting with the TESTS' status. wait-for-test-dbs.sh takes an optional service list so a single-backend target does not block on six containers it never started; couchbase keeps its own provisioning script and gains only the teardown fix. Verified end to end with `make test-postgres`: readiness reported, suite green, container removed, exit 0. --- Makefile | 75 +++++++++++++++++++++++++++--------- scripts/wait-for-test-dbs.sh | 22 ++++++++++- 2 files changed, 78 insertions(+), 19 deletions(-) diff --git a/Makefile b/Makefile index 711f2d0d3..ad774d7a2 100644 --- a/Makefile +++ b/Makefile @@ -111,42 +111,81 @@ smoke: test-postgres: test-cleanup-postgres docker run -d --name authorizer_postgres -p 5434:5432 -e POSTGRES_PASSWORD=postgres -e POSTGRES_DB=postgres postgres - sleep 3 - go clean --testcache && TEST_DBS="postgres" $(GO_TEST_ALL) - docker rm -vf authorizer_postgres + @# Wait for readiness, then ALWAYS tear the container down — including when + @# the tests fail — and exit with the TESTS' status, not the teardown's. + @# Previously `sleep N` guessed at readiness and `docker rm` sat on its own + @# recipe line, so a failing test aborted the recipe before teardown and left + @# the port bound, breaking the NEXT run for an unrelated reason. + sh scripts/wait-for-test-dbs.sh postgres && \ + { go clean --testcache; TEST_DBS="postgres" $(GO_TEST_ALL); }; \ + status=$$?; \ + docker rm -vf authorizer_postgres; \ + exit $$status test-sqlite: go clean --testcache && TEST_DBS="sqlite" $(GO_TEST_ALL) test-mongodb: test-cleanup-mongodb docker run -d --name authorizer_mongodb_db -p 27017:27017 mongo:4.4.15 - sleep 3 - go clean --testcache && TEST_DBS="mongodb" $(GO_TEST_ALL) - docker rm -vf authorizer_mongodb_db + @# Wait for readiness, then ALWAYS tear the container down — including when + @# the tests fail — and exit with the TESTS' status, not the teardown's. + @# Previously `sleep N` guessed at readiness and `docker rm` sat on its own + @# recipe line, so a failing test aborted the recipe before teardown and left + @# the port bound, breaking the NEXT run for an unrelated reason. + sh scripts/wait-for-test-dbs.sh mongodb && \ + { go clean --testcache; TEST_DBS="mongodb" $(GO_TEST_ALL); }; \ + status=$$?; \ + docker rm -vf authorizer_mongodb_db; \ + exit $$status test-scylladb: test-cleanup-scylladb docker run -d --name authorizer_scylla_db -p 9042:9042 scylladb/scylla - sleep 15 - go clean --testcache && TEST_DBS="scylladb" $(GO_TEST_ALL) - docker rm -vf authorizer_scylla_db + @# Wait for readiness, then ALWAYS tear the container down — including when + @# the tests fail — and exit with the TESTS' status, not the teardown's. + @# Previously `sleep N` guessed at readiness and `docker rm` sat on its own + @# recipe line, so a failing test aborted the recipe before teardown and left + @# the port bound, breaking the NEXT run for an unrelated reason. + sh scripts/wait-for-test-dbs.sh scylladb && \ + { go clean --testcache; TEST_DBS="scylladb" $(GO_TEST_ALL); }; \ + status=$$?; \ + docker rm -vf authorizer_scylla_db; \ + exit $$status test-arangodb: test-cleanup-arangodb docker run -d --name authorizer_arangodb -p 8529:8529 -e ARANGO_NO_AUTH=1 arangodb/arangodb:3.10.3 - sleep 5 - go clean --testcache && TEST_DBS="arangodb" $(GO_TEST_ALL) - docker rm -vf authorizer_arangodb + @# Wait for readiness, then ALWAYS tear the container down — including when + @# the tests fail — and exit with the TESTS' status, not the teardown's. + @# Previously `sleep N` guessed at readiness and `docker rm` sat on its own + @# recipe line, so a failing test aborted the recipe before teardown and left + @# the port bound, breaking the NEXT run for an unrelated reason. + sh scripts/wait-for-test-dbs.sh arangodb && \ + { go clean --testcache; TEST_DBS="arangodb" $(GO_TEST_ALL); }; \ + status=$$?; \ + docker rm -vf authorizer_arangodb; \ + exit $$status test-dynamodb: test-cleanup-dynamodb docker run -d --name authorizer_dynamodb -p 8000:8000 amazon/dynamodb-local:latest - sleep 3 - go clean --testcache && TEST_DBS="dynamodb" $(GO_TEST_ALL) - docker rm -vf authorizer_dynamodb + @# Wait for readiness, then ALWAYS tear the container down — including when + @# the tests fail — and exit with the TESTS' status, not the teardown's. + @# Previously `sleep N` guessed at readiness and `docker rm` sat on its own + @# recipe line, so a failing test aborted the recipe before teardown and left + @# the port bound, breaking the NEXT run for an unrelated reason. + sh scripts/wait-for-test-dbs.sh dynamodb && \ + { go clean --testcache; TEST_DBS="dynamodb" $(GO_TEST_ALL); }; \ + status=$$?; \ + docker rm -vf authorizer_dynamodb; \ + exit $$status test-couchbase: test-cleanup-couchbase docker run -d --name authorizer_couchbase -p 8091-8097:8091-8097 -p 11210:11210 -p 11207:11207 -p 18091-18095:18091-18095 -p 18096:18096 -p 18097:18097 couchbase:latest - sh scripts/couchbase-test.sh - go clean --testcache && TEST_DBS="couchbase" $(GO_TEST_ALL) - docker rm -vf authorizer_couchbase + @# couchbase-test.sh already provisions and waits, so no readiness poll + @# here — but the teardown still has to survive a failing test run. + sh scripts/couchbase-test.sh && \ + { go clean --testcache; TEST_DBS="couchbase" $(GO_TEST_ALL); }; \ + status=$$?; \ + docker rm -vf authorizer_couchbase; \ + exit $$status # Prerequisites are `test-cleanup test-docker-up`, NOT # `test-cleanup test-docker-up test-cleanup`. The trailing duplicate looked like diff --git a/scripts/wait-for-test-dbs.sh b/scripts/wait-for-test-dbs.sh index 8ca343efb..1dc8d7bf8 100755 --- a/scripts/wait-for-test-dbs.sh +++ b/scripts/wait-for-test-dbs.sh @@ -15,7 +15,27 @@ TIMEOUT_SECONDS="${TEST_DB_WAIT_TIMEOUT:-120}" # name:port pairs. Couchbase is absent on purpose — scripts/couchbase-test.sh # already provisions and waits for it. -SERVICES="redis:6380 postgres:5434 mongodb:27017 scylladb:9042 arangodb:8529 dynamodb:8000" +ALL_SERVICES="redis:6380 postgres:5434 mongodb:27017 scylladb:9042 arangodb:8529 dynamodb:8000" + +# With no arguments, wait for everything (make test-all-db). With arguments, +# wait only for the named services, so a single-backend target does not block on +# six containers it never started. +if [ "$#" -eq 0 ]; then + SERVICES="$ALL_SERVICES" +else + SERVICES="" + for want in "$@"; do + match="" + for svc in $ALL_SERVICES; do + [ "${svc%%:*}" = "$want" ] && match="$svc" + done + if [ -z "$match" ]; then + echo "unknown service '$want' (known: $ALL_SERVICES)" >&2 + exit 2 + fi + SERVICES="$SERVICES $match" + done +fi wait_for_port() { name="$1" From c208004780f245ecdea0769ba4dd112badf37acb Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Fri, 7 Aug 2026 12:26:18 +0530 Subject: [PATCH 7/9] style: keep the EncryptPassword doc comment on its function Adding PasswordHashCost above EncryptPassword stranded that function's doc comment on the new const, which staticcheck (ST1022) caught. --- internal/crypto/common.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/crypto/common.go b/internal/crypto/common.go index b4e38012b..dedd6b20b 100644 --- a/internal/crypto/common.go +++ b/internal/crypto/common.go @@ -60,7 +60,6 @@ func GetPubJWK(algo, keyID string, publicKey interface{}) (string, error) { // return EncryptB64(string(encryptedConfig)), nil // } -// EncryptPassword is used for encrypting password // PasswordHashCost is the bcrypt cost for newly written password hashes. // // Raised from bcrypt.DefaultCost (10), which is below current guidance. This is @@ -79,6 +78,7 @@ func GetPubJWK(algo, keyID string, publicKey interface{}) (string, error) { // make us pay it. const PasswordHashCost = 12 +// EncryptPassword is used for encrypting password func EncryptPassword(password string) (string, error) { pw, err := bcrypt.GenerateFromPassword([]byte(password), PasswordHashCost) if err != nil { From f21eb442501f73fc363ce1db0b31808f7eb16b07 Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Fri, 7 Aug 2026 12:54:57 +0530 Subject: [PATCH 8/9] fix(config): reject a mistyped --app-cookie-same-site instead of silently using lax cookie.ParseSameSite falls back to Lax for anything it does not recognise. Safe, but silent: an operator who asks for `strict` and mistypes it gets Lax - a real downgrade from what they requested, with nothing anywhere to say so. A mistyped `none` withholds the session cookie from cross-site apps instead, which presents as "login randomly doesn't stick" with the cause three layers away. Validated at startup now, alongside ValidateEncryptionKey, so a typo in a startup flag stops the process rather than quietly selecting a policy the operator did not choose. Verified live: an invalid value exits 1 with the reason, `strict` boots through to "Starting HTTP server". Also documents and guards the one place that deliberately IGNORES this setting. BuildOAuthStateCookie hardcodes Lax/None because the provider's callback arrives as a cross-site redirect (a cross-site form_post for Apple), and Strict would withhold the state cookie on exactly those - every social login on a strict-configured deployment would fail with "invalid oauth state". Threading the operator's setting through for consistency is the obvious refactor and a silent outage; TestOAuthStateCookieIsNeverStrict now fails if anyone tries. --- cmd/cookie_defaults_test.go | 46 ++++++++++++++++++++++++++++++++++ cmd/root.go | 6 ++++- internal/config/config.go | 30 ++++++++++++++++++++++ internal/cookie/cookie_test.go | 28 +++++++++++++++++++++ internal/cookie/oauth_state.go | 7 ++++++ 5 files changed, 116 insertions(+), 1 deletion(-) diff --git a/cmd/cookie_defaults_test.go b/cmd/cookie_defaults_test.go index ac4142d49..8839231ab 100644 --- a/cmd/cookie_defaults_test.go +++ b/cmd/cookie_defaults_test.go @@ -1,10 +1,14 @@ package cmd import ( + "net/http" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/authorizerdev/authorizer/internal/config" + "github.com/authorizerdev/authorizer/internal/cookie" ) // TestAppCookieSameSiteDefaultIsNone is a decision guard for the other half of @@ -28,3 +32,45 @@ func TestAppCookieSameSiteDefaultIsNone(t *testing.T) { assert.Equal(t, "none", f.DefValue, "changing this default breaks cross-site apps; read cookie.BuildSessionCookies first") } + +// TestAppCookieSameSiteIsValidated pins that a mistyped value stops the process +// instead of silently becoming lax. +// +// cookie.ParseSameSite falls back to Lax for anything unrecognised. That is a +// safe default but a silent one: an operator who asks for `strict` and mistypes +// it gets Lax — a real downgrade from what they requested, with nothing +// anywhere to say so — and a mistyped `none` withholds the session cookie from +// cross-site apps, which presents as "login randomly doesn't stick". +func TestAppCookieSameSiteIsValidated(t *testing.T) { + t.Parallel() + + for _, valid := range []string{"lax", "strict", "none", "STRICT", " none ", ""} { + cfg := config.Config{AppCookieSameSite: valid} + assert.NoError(t, cfg.ValidateAppCookieSameSite(), "%q is a supported value", valid) + } + + for _, invalid := range []string{"strct", "nonw", "same-site", "true", "0"} { + cfg := config.Config{AppCookieSameSite: invalid} + err := cfg.ValidateAppCookieSameSite() + require.Error(t, err, "%q must be rejected, not silently downgraded to lax", invalid) + assert.Contains(t, err.Error(), "app-cookie-same-site") + } +} + +// TestEverySameSiteValueRoundTrips pins that each accepted CLI value maps to the +// SameSite mode it names — the validator and the parser must agree, or a value +// passes validation and then means something else. +func TestEverySameSiteValueRoundTrips(t *testing.T) { + t.Parallel() + want := map[string]http.SameSite{ + "lax": http.SameSiteLaxMode, + "strict": http.SameSiteStrictMode, + "none": http.SameSiteNoneMode, + } + for _, v := range config.ValidAppCookieSameSiteValues { + cfg := config.Config{AppCookieSameSite: v} + require.NoError(t, cfg.ValidateAppCookieSameSite()) + assert.Equal(t, want[v], cookie.ParseSameSite(v), + "%q passes validation, so it must parse to the mode it names", v) + } +} diff --git a/cmd/root.go b/cmd/root.go index 6ce4adeb2..1e5e8a5c8 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -82,7 +82,11 @@ var ( rootArgs.config.Finalize() // Fail closed rather than silently encrypting TOTP seeds with a // publicly computable key derived from an empty secret. - return rootArgs.config.ValidateEncryptionKey() + if err := rootArgs.config.ValidateEncryptionKey(); err != nil { + return err + } + // A mistyped SameSite silently becomes lax — see the doc comment. + return rootArgs.config.ValidateAppCookieSameSite() }, Run: runRoot, } diff --git a/internal/config/config.go b/internal/config/config.go index 124ea0c3d..090e956c1 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -537,3 +537,33 @@ func (c *Config) ValidateEncryptionKey() error { } return nil } + +// ValidAppCookieSameSiteValues are the accepted --app-cookie-same-site values. +var ValidAppCookieSameSiteValues = []string{"lax", "strict", "none"} + +// ValidateAppCookieSameSite rejects an unrecognised --app-cookie-same-site. +// +// cookie.ParseSameSite falls back to Lax for anything it does not recognise, +// which is a safe default but a silent one. An operator who sets `strict` and +// mistypes it gets Lax — a real security downgrade from what they asked for, +// with nothing anywhere to say so. In the other direction a mistyped `none` +// silently withholds the session cookie from cross-site apps, which presents as +// "login randomly doesn't stick" with the cause three layers away. +// +// A typo in a startup flag should stop the process, not quietly pick a policy +// the operator did not choose. +func (c *Config) ValidateAppCookieSameSite() error { + value := strings.ToLower(strings.TrimSpace(c.AppCookieSameSite)) + if value == "" { + // Unset is fine: the flag default applies. + return nil + } + for _, valid := range ValidAppCookieSameSiteValues { + if value == valid { + return nil + } + } + return fmt.Errorf( + "invalid --app-cookie-same-site %q: must be one of %s. ParseSameSite would silently fall back to lax, downgrading a requested strict policy without telling you", + c.AppCookieSameSite, strings.Join(ValidAppCookieSameSiteValues, ", ")) +} diff --git a/internal/cookie/cookie_test.go b/internal/cookie/cookie_test.go index 73d1bd7d4..a0f53716f 100644 --- a/internal/cookie/cookie_test.go +++ b/internal/cookie/cookie_test.go @@ -166,3 +166,31 @@ func TestSessionCookieSameSiteIsCallerControlled(t *testing.T) { } } } + +// TestOAuthStateCookieIsNeverStrict guards a booby trap rather than a bug. +// +// BuildOAuthStateCookie intentionally ignores --app-cookie-same-site. The +// obvious "consistency" refactor — thread the operator's setting through, like +// the session cookie does — silently breaks every social login on any +// deployment configured strict: the provider's callback is a cross-site +// redirect (a cross-site form_post for Apple), and Strict withholds the cookie +// on exactly those, so the callback sees no binding and answers +// "invalid oauth state". +// +// Nothing about the call site suggests that, which is why it is asserted here. +func TestOAuthStateCookieIsNeverStrict(t *testing.T) { + t.Parallel() + for _, secure := range []bool{false, true} { + c := BuildOAuthStateCookie("auth.example.com", "state-value", secure) + assert.NotEqual(t, http.SameSiteStrictMode, c.SameSite, + "Strict withholds this cookie on the provider callback; social login would break entirely (secure=%v)", secure) + assert.True(t, c.HttpOnly, "the binding must not be script-readable") + assert.Empty(t, c.Domain, "host-only: the callback runs on the host that set it") + if secure { + assert.Equal(t, http.SameSiteNoneMode, c.SameSite, "Apple's callback is a cross-site form_post, which Lax would block") + assert.True(t, c.Secure, "SameSite=None requires Secure or browsers drop the cookie") + } else { + assert.Equal(t, http.SameSiteLaxMode, c.SameSite) + } + } +} diff --git a/internal/cookie/oauth_state.go b/internal/cookie/oauth_state.go index cb48aaeed..f8013e62b 100644 --- a/internal/cookie/oauth_state.go +++ b/internal/cookie/oauth_state.go @@ -39,6 +39,13 @@ func SetOAuthState(gc *gin.Context, state string, appCookieSecure bool) { // BuildOAuthStateCookie returns the state-binding cookie. Host-scoped // deliberately: the callback runs on this exact host, so there is no reason to // widen the cookie to sibling subdomains. +// It deliberately does NOT take the operator's --app-cookie-same-site setting. +// SameSite=Strict would withhold this cookie on the provider's callback, which +// arrives as a cross-site redirect (or, for Apple, a cross-site form_post) — +// every social login would fail with "invalid oauth state" on any deployment +// configured strict. Lax/None is a correctness requirement here, not a +// preference, so wiring the session-cookie setting through would be a silent +// outage. TestOAuthStateCookieIsNeverStrict guards that. func BuildOAuthStateCookie(_ string, state string, appCookieSecure bool) *http.Cookie { sameSite := http.SameSiteLaxMode if appCookieSecure { From 5655b74220a9abff978082ffdca2ac5bf9c940c7 Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Fri, 7 Aug 2026 13:54:23 +0530 Subject: [PATCH 9/9] fix(verify-email): set email_verified_at in the REST handler too Reported from local testing of this PR: sign up, click the button in the verification email, land on MFA setup, enrol a passkey - and passkey login then refuses forever with "email is not verified", for a user who verified. GET /verify_email is what the emailed button literally points to, and it is a SEPARATE implementation from service.VerifyEmail. The earlier commit moved the email_verified_at write above the MFA gate in the service; the REST handler - the path every real user takes - still had the write after the gate, whose withheld branch redirects to MFA setup and returns. Why it hid: only WebauthnLoginVerify checks the column. Password login checks it too but SELF-HEALS - it diverts an unverified user into an email OTP, and verify_otp sets the flag. So TOTP and email-OTP users appeared fine while silently completing a second, redundant verification round-trip. A passkey user never passes through password login, so nothing repaired the flag and the hard check refused. Passkey was not the broken path; it was the only honest one. Remediation needs no backfill: DeleteVerificationRequest also sat after the gate, so affected users' verification requests were never consumed. Clicking the link again works, and resend_verify_email mints a fresh one if it expired. The regression test drives the real REST path and asserts the gate actually withheld (307 -> mfa_gate=offer) before checking the stored state, so it cannot pass by accidentally taking the non-withheld branch. --- internal/http_handlers/verify_email.go | 47 ++++++++---- .../signup_verification_matrix_test.go | 74 +++++++++++++++++++ 2 files changed, 107 insertions(+), 14 deletions(-) diff --git a/internal/http_handlers/verify_email.go b/internal/http_handlers/verify_email.go index 27443fa50..249afad14 100644 --- a/internal/http_handlers/verify_email.go +++ b/internal/http_handlers/verify_email.go @@ -118,6 +118,37 @@ func (h *httpProvider) VerifyEmailHandler() gin.HandlerFunc { return } + // Record the address as verified HERE, BEFORE the MFA gate below. + // + // The gate's withheld branch redirects to MFA setup and returns, so the + // write that used to sit after it never ran for the overwhelmingly + // common case: a fresh signup clicking its verification link, with MFA + // on by default. The user did everything right and their address stayed + // unverified forever. + // + // Only passkey login surfaces it — webauthn.go refuses outright on + // email_verified_at == nil, with an error the user cannot act on. + // Password, TOTP and email/SMS-OTP logins never check the column, so + // they appear to "work" while the account is in exactly the same broken + // state. That asymmetry is why this hid. + // + // Clicking the link IS the proof of mailbox control; whether MFA then + // interrupts token issuance is a separate question and must not discard + // it. Mirrors service.VerifyEmail, which is the GraphQL twin of this + // handler — the two implementations have to agree. + emailJustVerified := user.EmailVerifiedAt == nil + if emailJustVerified { + now := time.Now().Unix() + user.EmailVerifiedAt = &now + user, err = h.StorageProvider.UpdateUser(c, user) + if err != nil { + log.Debug().Err(err).Msg("Error updating user") + errorRes["error"] = err.Error() + utils.HandleRedirectORJsonResponse(c, http.StatusBadRequest, errorRes, generateRedirectURL(redirectURL, errorRes)) + return + } + } + // MFA gate: this REST endpoint is what the emailed verification/magic // link literally points to, so it must enforce the same gate every // other login entry point does (login.go/signup.go/oauth_callback.go). @@ -146,20 +177,8 @@ func (h *httpProvider) VerifyEmailHandler() gin.HandlerFunc { return } - isSignUp := false - // update email_verified_at in users table - if user.EmailVerifiedAt == nil { - now := time.Now().Unix() - user.EmailVerifiedAt = &now - isSignUp = true - user, err = h.StorageProvider.UpdateUser(c, user) - if err != nil { - log.Debug().Err(err).Msg("Error updating user") - errorRes["error"] = err.Error() - utils.HandleRedirectORJsonResponse(c, http.StatusBadRequest, errorRes, generateRedirectURL(redirectURL, errorRes)) - return - } - } + // Set above, before the MFA gate — see the comment there. + isSignUp := emailJustVerified // delete from verification table if err := h.StorageProvider.DeleteVerificationRequest(c, verificationRequest); err != nil { log.Debug().Err(err).Msg("Error deleting verification request") diff --git a/internal/integration_tests/signup_verification_matrix_test.go b/internal/integration_tests/signup_verification_matrix_test.go index fb701cd99..a9e98784d 100644 --- a/internal/integration_tests/signup_verification_matrix_test.go +++ b/internal/integration_tests/signup_verification_matrix_test.go @@ -3,6 +3,8 @@ package integration_tests import ( "context" "fmt" + "net/http" + "net/url" "testing" "time" @@ -326,3 +328,75 @@ func TestMobileSignupVerificationIsIndependent(t *testing.T) { // No email was ever supplied, so nothing should have marked one verified. assert.Empty(t, refs.StringValue(user.Email)) } + +// TestVerifyEmailRESTMarksVerifiedBeforeMFAGate is the REST twin of +// TestVerifyEmailMarksVerifiedBeforeMFAGate, and the one that reproduces the +// bug a user actually hit. +// +// GET /verify_email is what the button in the verification email literally +// points to — a browser click never touches the GraphQL mutation. The two are +// SEPARATE implementations (internal/http_handlers/verify_email.go vs +// internal/service/verify_email.go), so fixing the service left the path every +// real user takes still broken. +// +// The write sat after the MFA gate's withheld branch, which redirects to MFA +// setup and returns. With MFA on by default, a fresh signup clicking its link +// lands there and the address is never marked verified. +// +// It stayed hidden because only passkey login checks the column: password, +// TOTP and email/SMS-OTP logins all succeed against an unverified account, so +// the flow looks fine until someone enrolls a passkey and is told - with no way +// to act on it - that their email is not verified. +func TestVerifyEmailRESTMarksVerifiedBeforeMFAGate(t *testing.T) { + cfg := getTestConfig() + cfg.IsEmailServiceEnabled = true + cfg.EnableEmailVerification = true + // Left at the derived default (true). This is the configuration that + // triggers the bug — the MFA gate has to withhold for the early return to + // be reached at all. + cfg.EnableMFA = true + ts := initTestSetup(t, cfg) + _, ctx := createContext(ts) + + email := "verify_rest_" + uuid.NewString() + "@authorizer.dev" + // The test config's AllowedOrigins is an explicit allowlist, and the handler + // re-validates the token's redirect_uri against it on the click. Anything + // else 400s on the redirect check before reaching the code under test. + redirectURI := "http://localhost:3000" + _, err := ts.GraphQLProvider.SignUp(ctx, &model.SignUpRequest{ + Email: &email, + Password: "Password@123", + ConfirmPassword: "Password@123", + RedirectURI: &redirectURI, + }) + require.NoError(t, err) + + before, err := ts.StorageProvider.GetUserByEmail(ctx, email) + require.NoError(t, err) + require.Nil(t, before.EmailVerifiedAt) + + vr, err := ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeBasicAuthSignup) + require.NoError(t, err) + + // Click the link as the browser does, but stop at the first response + // instead of chasing the redirect to the app origin (nothing is listening + // there in a test). + client := &http.Client{ + CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }, + } + resp, err := client.Get(testAuthorizerHost(ts) + "/verify_email?token=" + url.QueryEscape(vr.Token)) + require.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + + // Confirm we actually exercised the branch that used to skip the write: + // the MFA gate withheld the token and redirected to setup. + require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) + require.Contains(t, resp.Header.Get("Location"), "mfa_gate=offer", + "this test is only meaningful if the MFA gate withheld — that is the early return the write used to sit behind") + + after, err := ts.StorageProvider.GetUserByEmail(ctx, email) + require.NoError(t, err) + assert.NotNil(t, after.EmailVerifiedAt, + "clicking the emailed link must record the address as verified even when the MFA gate withholds the token — otherwise passkey login rejects the user permanently, while password/TOTP/OTP logins hide it by never checking") + assert.Equal(t, before.ID, after.ID) +}