fix: 12 logical issues across HTTP handlers and GraphQL modules#602
Merged
lakhansamani merged 6 commits intomainfrom Apr 9, 2026
Merged
fix: 12 logical issues across HTTP handlers and GraphQL modules#602lakhansamani merged 6 commits intomainfrom
lakhansamani merged 6 commits intomainfrom
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
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
Systematic code review of all HTTP handlers and GraphQL modules uncovered 12 logical issues ranging from critical (broken hybrid flow code exchange) to low (spec-compliance nits). All fixed with tests.
Critical / High
FingerPrint) instead of AES-encrypted session (FingerPrintHash) in authorize.go — code exchange at/oauth/tokenalways failed for hybrid flows (OIDC Core §3.3)len(scope)(always 3) instead oflen(params.Scope)— empty scope list would strip all scopes from tokens(nil, nil)— conditionotp == nil && err != nilmisses the(nil, nil)caseMedium
params.Emailfor OTP lookup instead of the trimmed/lowercasedemailvariable_update_userwas commented out (TODO) — admins could assign duplicate phones!=for token comparison instead ofsubtle.ConstantTimeCompare(RFC 7009 §2.1)json.Unmarshalerrors silently dropped in 5+ locations in oauth_callback.goLow
@@separator in token.go nonce for refresh_token grants (code is empty)post_logout_redirect_uriinstead of silently ignoring per OIDC spec (RP-Initiated Logout §2)Test plan
/oauth/token)@@, revoke with wrong token returns 200