fix(oauth): RFC-compliant PKCE, redirect_uri validation, and security hardening#603
Merged
lakhansamani merged 9 commits intomainfrom Apr 10, 2026
Merged
Conversation
…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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
=) incode_challenge— fixes Auth0 and other providers that send padded challengesclient_secretwhen provided, even when PKCE is used (previously PKCE bypassed client auth entirely)code_verifierwhen nocode_challengewas registered at/authorize— prevents attackers from bypassingclient_secretby sending arbitrary verifiersredirect_uriin@@-delimited state to prevent delimiter injection attacksredirect_uriat/oauth/tokenmatches the one from/authorizeredirect_uricomparison, remove secrets from logs, generic error messages for refresh token failuresmake dev— RSA keys now useprintffor proper multi-line handlingSecurity Audit Findings Addressed
@@delimiter injection viaredirect_uriRemoveState()allows authorize state reusecode_verifierwithoutcode_challengepasses all authcode_challengelogged in plaintext (secret forplainmethod)has_code_challengeredirect_uricomparison not constant-timesubtle.ConstantTimeCompareerr.Error()leaked in refresh token error responseTest plan
TEST_DBS=sqlite)consumeAuthorizeStateincluding delimiter injection testTestHybridFlowCodeExchangeend-to-end passesmake devstarts successfullygo build ./...clean