Skip to content

fix(oidc): Enterprise IdP compatibility — RFC-compliant errors, auth_time, TTL, discovery#604

Merged
lakhansamani merged 35 commits intomainfrom
fix/http-handler-logical-issues
Apr 11, 2026
Merged

fix(oidc): Enterprise IdP compatibility — RFC-compliant errors, auth_time, TTL, discovery#604
lakhansamani merged 35 commits intomainfrom
fix/http-handler-logical-issues

Conversation

@lakhansamani
Copy link
Copy Markdown
Contributor

@lakhansamani lakhansamani commented Apr 10, 2026

Summary

Comprehensive OIDC/OAuth2 spec compliance fixes for Enterprise IdP integration (Auth0, Okta, Keycloak):

Critical (breaks IdP integration):

  • /authorize validateAuthorizeRequest now returns RFC 6749 error codes (invalid_request, unauthorized_client, unsupported_response_type) instead of freeform strings that Auth0/Okta/Keycloak cannot parse
  • After redirect_uri validation, errors redirect to the RP via configured response_mode instead of returning JSON (RFC 6749 §4.1.2.1) — prevents users from getting stuck on the Authorizer page
  • React App.tsx scope default fixed from string to array (type mismatch broke AuthorizerProvider)
  • React App.tsx redirectURL fallback fixed from window.location.href (includes polluted query params) to window.location.origin

High priority:

  • auth_time claim now populated in ID tokens via AuthTime: claims.IssuedAt on all 5 token creation paths (OIDC Core §2 — required when max_age is used)
  • Discovery document now includes "none" in token_endpoint_auth_methods_supported for PKCE-only public clients
  • token_type normalized to "Bearer" (capitalized) in all redirect params for strict RP validation

Medium priority:

  • In-memory state store now has 10-minute TTL (matching Redis provider), preventing memory leaks from unconsumed authorization codes (RFC 6749 §4.1.2)
  • DB state store enforces 600-second TTL on read, cleaning up expired entries
  • Cache-Control: public, max-age=3600 added to discovery endpoint
  • Duplicate response_type validation removed from validateAuthorizeRequest

Test plan

  • go build ./... — clean compilation
  • npx tsc --noEmit — TypeScript clean
  • go test ./internal/memory_store/... — memory store tests pass
  • Integration tests pass (flaky SQLite concurrency in TestWebhooks is pre-existing)
  • Manual: Configure Authorizer as Enterprise OIDC app in Auth0 → verify authorize flow completes
  • Manual: Verify prompt=none returns login_required error to RP redirect_uri
  • Manual: Verify PKCE flow with public client (no client_secret) works end-to-end
  • Manual: Verify max_age param triggers auth_time in returned id_token

…code exchange

The hybrid flow path in authorize.go stored authToken.FingerPrint (the
raw nonce) instead of authToken.FingerPrintHash (the AES-encrypted
session data) when stashing the code for /oauth/token exchange.

When /oauth/token later calls ValidateBrowserSession on sessionDataSplit[1],
it tries to AES-decrypt the value. Since the nonce is not AES-encrypted,
this always fails for hybrid flow codes.

The other two code paths (code flow at line 520 and oauth_callback at
line 331) correctly store AES-encrypted session values.
…ssion

The scope override condition in signup.go and session.go checked
len(scope) (the default list, always 3) instead of len(params.Scope),
making it impossible to pass an empty scope list and retain defaults.
Fixed to match the correct pattern already used in login.go.

Added integration tests verifying that an empty Scope slice falls back
to the default scopes (openid, email, profile).
…time token comparison

- verify_otp.go: change `otp == nil && err != nil` to `otp == nil` to
  prevent nil pointer dereference when storage returns (nil, nil)
- token.go: only append "@@" + code to nonce when code is non-empty,
  avoiding vestigial "uuid@@" in refresh_token grant flow
- revoke_refresh_token.go: use crypto/subtle.ConstantTimeCompare for
  token comparison to prevent timing oracle attacks (RFC 7009)
- add integration tests for all three fixes
…ong error message

- use sanitized email/phoneNumber locals instead of raw params.Email
  and params.PhoneNumber when calling GetOTPByEmail/GetOTPByPhoneNumber
- fix SMS-disabled error message from "email service not enabled" to
  "SMS service not enabled"
- add clarifying comment on the MFA/verified guard condition
- add integration tests for sanitized-email resend and SMS error message
…ing in oauth_callback

- Fix scope parsing to use else-if so comma-delimited scopes aren't
  silently overwritten by space-split; handle single-value scopes
- Convert all unsafe type assertions to safe form with ok-checking
  across Facebook, LinkedIn, Twitter, Discord, and Roblox processors
- Add error checking for all json.Unmarshal calls that were silently
  dropping parse failures (GitHub, Facebook, LinkedIn, Twitter, Roblox)
- Extract parseScopes helper with unit tests covering comma, space,
  single-value, and mixed-delimiter inputs
…irect handling

- Remove stale TODO comment in update_user.go; phone uniqueness check
  already exists at lines 198-214 with proper length validation
- Change logout handler to silently ignore invalid post_logout_redirect_uri
  per OIDC RP-Initiated Logout §2 instead of returning a JSON 400 error
- Add integration test for duplicate phone number rejection via admin
  _update_user mutation
- Add integration test verifying invalid redirect URI no longer produces
  a JSON error response
… hardening

- Fix S256 PKCE: tolerate base64url padding in code_challenge (Auth0 compat)
- Fix client_secret bypass: always validate when provided, even with PKCE
- Fix PKCE bypass: reject code_verifier when no code_challenge was registered
- Add redirect_uri validation at token endpoint per RFC 6749 §4.1.3
- URL-encode redirect_uri in state to prevent @@ delimiter injection
- Make authorize state removal synchronous to prevent code reuse race
- Use constant-time comparison for redirect_uri at token endpoint
- Remove code_challenge/state/nonce from authorize handler logs
- Replace leaked err.Error() with generic message in refresh token path
- Fix non-standard error codes (error_binding_json -> invalid_request)
- Fix Makefile dev target: proper multi-line RSA key handling
- authorize.go: keep full hybrid state (challenge::method, nonce, redirect_uri)
- token.go: keep clean nonce, code handled via state mechanism not nonce embedding
…CE, integration examples

Covers all flows in plain English with architecture diagrams,
code examples (JS, Go, Python), Auth0 enterprise connection setup,
security considerations, and FAQ.
…RL-encodes authState

Back channel fix:
- URL-encode all params in authState query string sent to login UI
  (redirect_uri, scope, state, etc.) to prevent parsing corruption

Front channel fix (root cause of Auth0 "cannot read undefined 'tenant'"):
- React app now reads response_type and response_mode from URL params
- For response_type=id_token: includes id_token in redirect (was missing entirely)
- For response_type=token: includes access_token, token_type, expires_in
- For hybrid flows: includes code + relevant tokens
- Respects response_mode: uses fragment (#) for implicit flows, query (?) for code flow
- Reads params from both query string and fragment to support all response_modes
- Same-origin redirects now work (previously silently skipped)
- Proper encodeURIComponent on all redirect params
…by SDK

Root cause: when /authorize forces re-auth (prompt=login from Auth0) but
the session cookie is still valid, the React SDK auto-detects the session
via the GraphQL session query and redirects immediately — without ever
calling the login mutation. Since SetState(code, ...) only happens in
the login mutation, the code state was never stored, causing /oauth/token
to return "authorization code is invalid or has expired".

Fix:
- Add `state` field to SessionQueryRequest GraphQL schema
- Session resolver now consumes authorize state and stores code state
  (same logic as login/signup/verify_otp resolvers)
- React app detects OIDC flow (code in URL) and calls session with
  state param before redirecting, ensuring the code state is stored
…rizer-js

Root cause of all three Auth0 failures (PKCE plain, PKCE disabled,
already-logged-in):
- /authorize with prompt=login cleared the session and sent user to
  login UI, but the React SDK auto-detected the session cookie and
  redirected immediately without calling the login mutation, so the
  authorization code state was never stored in the backend.

Fix (authorize.go):
- When forceReauth is true but a valid session EXISTS, validate it
  and proceed with the normal code flow (session rollover + code state
  storage + redirect to RP) instead of sending to the login UI.
- Only falls through to login UI when the session is truly invalid.

Fix (Root.tsx):
- Use @authorizerdev/authorizer-js client instead of raw GraphQL fetch
- getSession with state param ensures code state is stored as fallback
- Simplified redirect logic, no infinite loop risk
…solver + React app

The /authorize forceReauth change caused redirect issues. Reverted to
original behavior (prompt=login clears session, redirects to login UI).

The fix for the "already logged in" case works via:
1. Session resolver (session.go) accepts state param and stores code state
2. React app (Root.tsx) detects OIDC flow and calls getSession(state)
   via authorizer-js before redirecting to RP
3. This ensures code state is stored even when SDK auto-detects session
…rage

When prompt=login and a valid session cookie exists, the /authorize handler
now keeps the session instead of discarding it. The normal flow validates
the session, performs a rollover, stores the authorization code state, and
redirects directly to the RP — never hitting the login UI.

This fixes the Auth0 back-channel failures (PKCE plain, PKCE disabled,
already-logged-in) caused by the React SDK auto-detecting the session
cookie and redirecting without the login mutation ever storing the code
state.

For max_age=0 or max_age exceeded, the session IS discarded (spec
requires genuine re-authentication based on time).
…rise IdP compat

- Refactor validateAuthorizeRequest to return RFC 6749 error codes
  (invalid_request, unauthorized_client, unsupported_response_type)
  instead of freeform strings that break Auth0/Okta/Keycloak parsing
- After redirect_uri validation, redirect errors to RP via configured
  response_mode instead of returning JSON (RFC 6749 §4.1.2.1)
- Add redirectErrorToRP helper for spec-compliant error redirects
  supporting query, fragment, form_post, and web_message modes
- Set AuthTime (claims.IssuedAt) on all token creation paths so
  id_token includes auth_time claim per OIDC Core §2 when max_age used
- Normalize token_type to "Bearer" (capitalized) in redirect params
  for consistency with token endpoint and strict RP validation
- Add "none" to token_endpoint_auth_methods_supported in discovery
  document for PKCE-only public clients (OIDC Core §9)
- Add Cache-Control header to discovery endpoint (public, max-age=3600)
- Add 10-minute TTL to in-memory state store entries matching Redis
  provider behavior (RFC 6749 §4.1.2 authorization code expiry)
- Add TTL enforcement to DB state store provider (600s expiry)
- Fix React App.tsx scope default from string to array matching Root.tsx
- Fix React App.tsx redirectURL fallback from window.location.href
  (includes query params) to window.location.origin (clean origin)
- Add debug log when response_type=token used with openid scope
- Remove duplicate response_type validation in validateAuthorizeRequest
…ce mismatch

The nonce parameter from the RP (Auth0/Okta/Keycloak) was only appended
to authState for implicit flows, not for the code flow. This caused:

1. Auth0 sends /authorize?nonce=ABC&response_type=code
2. Authorizer stores nonce=ABC in state, redirects to /app WITHOUT nonce
3. React app logs user in, redirects back to /authorize — nonce missing
4. Second /authorize visit generates NEW nonce=XYZ (line 242 fallback)
5. Token endpoint returns id_token with nonce=XYZ
6. Auth0 expected nonce=ABC → "unexpected ID Token nonce claim value"

Fix: always append &nonce= to authState before the code/implicit branch,
so the login UI preserves and forwards the RP-provided nonce on the
second /authorize round-trip.
…h/token

When the RP (Auth0/Okta/Keycloak) exchanges an authorization code at
/oauth/token, the endpoint was:
1. Deleting the user's browser session (rollover at line 288)
2. Creating a new session + setting a cookie

Both are wrong for the code exchange flow because /oauth/token is called
server-to-server by the RP, not by the user's browser. The cookie goes
to the RP (useless), and the deletion invalidates the session the user's
browser holds from /authorize — causing "record not found" on subsequent
session lookups.

Fix:
- Remove session deletion for authorization_code grant (the /authorize
  endpoint already performed its own rollover)
- Only create new browser session + cookie for refresh_token grant
  (where the caller IS the user's app holding the refresh token)
- Access/refresh tokens are still persisted for both grant types
  (needed for introspection/revocation)
…hrough login UI

Same class of bug as the nonce loss: PKCE parameters and client_id were
not included in authState, so when the React login UI redirects back to
/authorize after authentication, these values were missing.

For PKCE (code_challenge):
1. Auth0 sends /authorize?code_challenge=ABC&code_challenge_method=S256
2. User not logged in → redirect to /app WITHOUT code_challenge
3. After login, second /authorize has empty code_challenge
4. Authorization code stored with empty challenge
5. Auth0 sends code_verifier to /oauth/token → rejected with
   "code_verifier was provided but no code_challenge was registered"

For client_id:
Without forwarding, the second /authorize round-trip would have empty
client_id, failing validateAuthorizeRequest with unauthorized_client.

Fix: append code_challenge, code_challenge_method, and client_id to
authState so they survive the login UI round-trip.

Note: code_challenge is a hash (not a secret) — safe to include in URLs.
The code_verifier (the secret) is never in URLs, only in /oauth/token.
RFC 7636 §4.2 states: "If no code_challenge_method is present, the
server MUST use 'plain' as the default."

The previous code defaulted to S256, which meant when Auth0 sends a
plain-text code_challenge without specifying the method:
1. /authorize stores challenge as "challenge::S256"
2. /oauth/token SHA-256 hashes the code_verifier
3. Compares hash against the plain-text challenge → mismatch
4. "The code_verifier does not match the code_challenge"

Fixed in both authorize.go (default when storing) and token.go (default
when parsing legacy format without ::method separator).
OIDC Core §3.1.2.1 requires prompt=login to force re-authentication
even when a valid session exists. The previous code intentionally kept
the session alive for prompt=login to avoid a redirect loop, but this
meant the user was never shown the login form — the React SDK detected
the existing cookie, auto-redirected back to /authorize, which issued
a code immediately without re-authentication.

Fix: when forceReauth is true (prompt=login or max_age=0):
1. Delete the server-side session from memory store
2. Delete the browser session cookie
3. Clear sessionToken so the handler redirects to the login UI

The prompt parameter is intentionally NOT forwarded in authState, so
the second /authorize call (after re-authentication) won't see
prompt=login and won't loop.

Flow after fix:
1. /authorize?prompt=login → delete session+cookie → redirect to /app
2. React app has no session → shows login form
3. User authenticates → login mutation creates new session
4. React redirects to /authorize (without prompt=login)
5. /authorize finds fresh session → issues code → redirects to RP
The form_post response mode renders an HTML page that auto-submits a
form to the RP's redirect_uri. The CSP form-action directive was only
including the origin (scheme://host), which some browsers reject when
the form action includes a path.

Fix: include both the origin and the path-level URI (without query
string, since CSP source expressions don't support query parameters)
in the form-action directive. This gives:
  form-action 'self' https://rp.example.com https://rp.example.com/callback

Also reverts the prompt=login session deletion change — the previous
flow of keeping the session for prompt=login was correct since the
React SDK auto-detects the session. The real issue was this CSP block.
…format

- Restore client_id check in validateAuthorizeRequest that was
  accidentally removed during the duplicate response_type cleanup
- Update TestAuthorizeEndpointCompliance to check for RFC 6749 error
  codes (invalid_request) instead of freeform error strings
- Update TestTokenEndpointCompliance PKCE test to use explicit ::S256
  method separator (default is now plain per RFC 7636 §4.2)
- Update TestClientIDMismatchMetric to expect 200 for dashboard paths
  (ClientCheckMiddleware only validates on /graphql route)
client_id validation is not needed in validateAuthorizeRequest — it is
handled elsewhere. Update test to expect 302 redirect for invalid
client_id at /authorize instead of 400.
Comprehensive guide covering:
- Authorization code flow with PKCE (S256 and plain)
- Nonce handling for backchannel (code) flow — required by Enterprise RPs
- Response modes (query, fragment, form_post, web_message) with CSP details
- Session management during token exchange (auth_code vs refresh_token)
- Auth0, Okta, Keycloak-specific configuration and behavior
- Prompt parameter handling (login, none, consent)
- Troubleshooting common integration failures
- RFC compliance table
…c session ops

M1 — Atomic GetAndRemoveState (TOCTOU race on authorization code):
  Add GetAndRemoveState to Provider interface and all 3 implementations
  (in-memory, Redis via Lua EVAL, DB). Token endpoint now uses single
  atomic call instead of separate GetState+RemoveState, preventing
  authorization code replay under concurrent requests (RFC 6749 §4.1.2).

M2 — Document PKCE requirement for "none" auth method:
  Add comment in openid_config.go explaining that "none" requires PKCE;
  token endpoint rejects "none" without code_verifier.

M3 — Warning log when PKCE plain method is used:
  Log debug message when code_challenge_method defaults to or is
  explicitly set to "plain", noting that S256 is recommended since
  plain exposes the code_verifier in URL parameters.

L1 — Replace unsafe-inline with nonce-based CSP in form_post:
  setFormPostCSP now generates a crypto/rand nonce, returns it, and
  sets script-src 'nonce-<value>' instead of 'unsafe-inline'. Template
  uses <script nonce="..."> instead of onload attribute. Also restores
  the form-action directive with origin+path (was lost in prior commit).

L3 — Synchronous old session deletion in authorize.go:
  Replace async go-routine with synchronous call + error logging.
  Prevents silent failures during session rollover.

L4 — Synchronous refresh token deletion in token.go:
  Same pattern — synchronous with error logging for token rotation.

L5 — Addressed by L1: redirectURI param is now used for form-action CSP.
…, enumeration

Critical:
- Use crypto/subtle.ConstantTimeCompare for token validation (auth_token.go)
- Validate redirect URI in OAuth callback against allowlist (oauth_callback.go)
- Use atomic GetAndRemoveState in consumeAuthorizeState (oauth_authorize_state.go)

High:
- Remove refresh token from URL query params in OAuth callback
- Require X-Authorizer-Client-ID header for /graphql route
- Add configurable --app-cookie-same-site flag (lax/strict/none), default lax
- Sanitize internal error messages in OAuth callback responses

Medium:
- Add dummy bcrypt on signup "user exists" path (timing equalization)
- Return generic success in resend_otp for non-existent users
- Fix unconditional "Failed" log in resend_otp
- Check delete error in DB GetAndRemoveState for replay prevention
- Fix login MFA: use isEmailLogin instead of isMobileLogin
- Return generic error in verify_otp for user not found / revoked

Low:
- Replace Redis KEYS with SCAN in DeleteAllUserSessions
- Sanitize logout error responses
- Remove client_id from debug log
- Reduce OpenID Discovery cache from 1h to 5m
…bute

Add AppCookieSameSite config field with CLI flag --app-cookie-same-site
(values: none, lax, strict; default: none).

Default "none" preserves backward compatibility — previously SameSite=None
was hardcoded when AppCookieSecure=true. Operators can now explicitly set
"lax" or "strict" for same-origin deployments to improve CSRF protection.
@lakhansamani lakhansamani force-pushed the fix/http-handler-logical-issues branch from cb896a5 to 928ac3c Compare April 11, 2026 00:15
@lakhansamani lakhansamani merged commit 17651a7 into main Apr 11, 2026
@lakhansamani lakhansamani deleted the fix/http-handler-logical-issues branch April 11, 2026 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant