feat: OAuth redirect URI validation for external frontends#3390
Open
0-don wants to merge 1 commit intoQuantumNous:mainfrom
Open
feat: OAuth redirect URI validation for external frontends#33900-don wants to merge 1 commit intoQuantumNous:mainfrom
0-don wants to merge 1 commit intoQuantumNous:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an env-configurable allowlist for OAuth redirect origins, validates and canonicalizes redirect URIs, implements Redis-backed signed OAuth state and one-time exchange-code flows, updates OAuth handlers to prefer signed states and return a one-time redirect URL, and updates frontend to follow returned redirect URLs. Changes
Sequence DiagramsequenceDiagram
participant Browser as Browser
participant FE as Frontend (OAuth2Callback)
participant API as API (OAuth Controller)
participant Common as common pkg
participant Redis as Redis
participant DB as User DB
Browser->>API: GET /api/oauth/code?redirect_uri=https://example.com
API->>Common: IsAllowedRedirectURI(redirect_uri)
Common-->>API: allowed / denied
alt allowed
API->>Common: CreateOAuthState(redirect_uri, aff)
Common-->>API: signed_state
API-->>Browser: JSON { state: signed_state }
else denied
API-->>Browser: HTTP error (redirect_uri_not_allowed)
end
Note right of Browser: User authorizes at provider
Browser->>API: Callback (code=XYZ&state=signed_state)
API->>Common: RedeemOAuthState(state)
Common-->>API: { RedirectURI, Aff } or nil
alt signed state valid
API->>DB: find/create user (apply aff)
DB-->>API: user
API->>Redis: StoreOAuthExchangeCode(user+token) -> one-time code
Redis-->>API: code
API-->>Browser: HTTP 200 { redirect_url }
FE->>Browser: window.location.replace(redirect_url)
else fallback
API->>API: session-based state validation / bind flow
API-->>Browser: existing login/bind response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
controller/oauth.go (1)
107-116: Consider guarding type assertion to prevent potential panic.Line 109 performs a type assertion
session.Get("oauth_state").(string)without checking if the value is actually a string. While the value is only set as a string in this codebase, a corrupted or tampered session could cause a panic.🔧 Safer type assertion
} else { // Session-based state - if state == "" || session.Get("oauth_state") == nil || state != session.Get("oauth_state").(string) { + savedState, ok := session.Get("oauth_state").(string) + if state == "" || !ok || state != savedState { c.JSON(http.StatusForbidden, gin.H{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/oauth.go` around lines 107 - 116, The session type assertion session.Get("oauth_state").(string) can panic if the stored value is not a string; update the OAuth callback check (the block comparing state with session.Get("oauth_state")) to perform a safe assertion like val, ok := session.Get("oauth_state").(string) (or convert to string via fmt.Sprintf) and treat non-string or missing values as invalid by returning the existing forbidden JSON response; ensure you reference the variables state and session.Get("oauth_state") in the new check so the behavior remains the same when the value is a valid string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controller/oauth.go`:
- Around line 39-46: The state assembly in GenerateOAuthCode currently
concatenates nonce, redirectURI and affCode with '|' which breaks if redirectURI
contains '|'; fix by encoding the redirectURI (and affCode if present) before
building the payload (e.g., base64 or URL-encoding) so the delimiter can't
appear inside values, then compute the HMAC via common.GenerateHMAC and append
the signature as before; update HandleOAuth's state parsing logic (the code
around lines where it splits state in HandleOAuth) to decode the encoded
redirectURI/affCode after splitting (matching the Encode/Decode method you
choose) so extraction of redirect_uri and affCode is robust.
In `@web/src/components/auth/OAuth2Callback.jsx`:
- Around line 59-62: The current direct redirect to data.redirect_url can leak
token-bearing query params; modify OAuth2Callback.jsx so that before calling
window.location.href you detect if data.redirect_url contains token-bearing
params (e.g., access_token, token, id_token) and avoid a simple GET redirect;
instead either (1) call your backend exchange endpoint to swap the token for a
one-time handoff code and then redirect to the redirect_url with only that code,
or (2) as a defensive frontend fallback, build and submit an HTML form
(method="POST") to the redirect origin with the token placed in a hidden input
and using the redirect URL without query token params, so token data is sent in
the request body (not the URL); ensure this logic is applied around the current
redirect branch that references data.redirect_url and preserved when message ===
'bind'.
---
Nitpick comments:
In `@controller/oauth.go`:
- Around line 107-116: The session type assertion
session.Get("oauth_state").(string) can panic if the stored value is not a
string; update the OAuth callback check (the block comparing state with
session.Get("oauth_state")) to perform a safe assertion like val, ok :=
session.Get("oauth_state").(string) (or convert to string via fmt.Sprintf) and
treat non-string or missing values as invalid by returning the existing
forbidden JSON response; ensure you reference the variables state and
session.Get("oauth_state") in the new check so the behavior remains the same
when the value is a valid string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 99145203-0089-492b-81e2-1f81fde5d6c0
📒 Files selected for processing (11)
common/constants.gocommon/init.gocommon/str.gocontroller/oauth.gocontroller/user.godocker-compose.ymli18n/keys.goi18n/locales/en.yamli18n/locales/zh-CN.yamli18n/locales/zh-TW.yamlweb/src/components/auth/OAuth2Callback.jsx
0-don
force-pushed
the
feat/oauth-redirect-uri
branch
from
6df3cff to
a47d402
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@common/str.go`:
- Around line 91-93: The origin check currently allows any scheme for loopback
hosts because it only tests parsed.Hostname(), so change the logic to require
parsed.Scheme == "https" OR (parsed.Scheme == "http" AND parsed.Hostname() is
"localhost" or "127.0.0.1"); i.e., use parsed.Scheme and parsed.Hostname()
together so only http is permitted for loopback while all other hosts must be
https.
In `@controller/oauth.go`:
- Around line 38-47: The signed `state` is vulnerable because the Nonce from
common/str.go is never bound to the browser; change CreateSignedOAuthState to
generate and return a nonce that is stored server-side per user (e.g., in the
session or an HttpOnly, SameSite cookie) and change the callback path that calls
ValidateSignedOAuthState to verify that the returned state's Nonce matches and
is unconsumed in that server-side store, then mark the Nonce as used (one-time)
and reject mismatches or reused nonces; update any relevant handlers that mint
state (CreateSignedOAuthState call) and the callback handler that uses
ValidateSignedOAuthState to implement this store-and-consume check.
- Around line 166-168: The current flow in setupLoginAndRedirect (invoked from
controller/oauth.go) exposes the bearer access_token and user data in the query
string, which leaks tokens via browser history and logs; change
setupLoginAndRedirect (controller/user.go) to never emit access_token in the
query string: instead generate a short-lived one-time authorization code (or
nonce) on the server, persist it mapped to the access token and user session,
and append only that code to the redirect URL; implement a new server endpoint
to exchange the one-time code for the access token (validating single-use and
expiry), and remove any logic that places access_token in the query string in
OAuth2Callback.jsx — if you cannot implement an exchange immediately, at minimum
move the token into the URL fragment (e.g., `#access_token`=...) so it isn’t sent
to servers, but the correct fix is the one-time code exchange.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a3afb8eb-4b59-4dc5-81e0-2344e912e7dd
📒 Files selected for processing (11)
common/constants.gocommon/init.gocommon/str.gocontroller/oauth.gocontroller/user.godocker-compose.ymli18n/keys.goi18n/locales/en.yamli18n/locales/zh-CN.yamli18n/locales/zh-TW.yamlweb/src/components/auth/OAuth2Callback.jsx
✅ Files skipped from review due to trivial changes (5)
- docker-compose.yml
- common/constants.go
- i18n/locales/en.yaml
- i18n/locales/zh-CN.yaml
- i18n/locales/zh-TW.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- web/src/components/auth/OAuth2Callback.jsx
- common/init.go
- i18n/keys.go
- controller/user.go
0-don
force-pushed
the
feat/oauth-redirect-uri
branch
from
a47d402 to
6dba9e0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
common/str.go (1)
151-154: Consider using constant-time comparison for HMAC verification.The string equality comparison
GenerateHMAC(encoded) != sigis theoretically vulnerable to timing attacks. While practical exploitation is mitigated by the short TTL and rate limiting, usingcrypto/subtle.ConstantTimeComparewould be a defense-in-depth improvement.🔒 Suggested improvement
+import "crypto/subtle" func ValidateSignedOAuthState(state string) *OAuthStateResult { idx := strings.LastIndex(state, ".") if idx < 0 { return nil } encoded, sig := state[:idx], state[idx+1:] - if GenerateHMAC(encoded) != sig { + expectedSig := GenerateHMAC(encoded) + if subtle.ConstantTimeCompare([]byte(expectedSig), []byte(sig)) != 1 { return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/str.go` around lines 151 - 154, The HMAC verification uses a regular string inequality which can be timing-attack sensitive; replace the direct comparison of GenerateHMAC(encoded) and sig with a constant-time comparison using crypto/subtle.ConstantTimeCompare: ensure both values are []byte (or convert to []byte), check lengths first, then use ConstantTimeCompare on GenerateHMAC(encoded) and sig and return nil when the comparison result is zero; update the check around state, idx, and GenerateHMAC accordingly.controller/oauth.go (1)
154-158: Consider handling session save error.The
_ = session.Save()silently ignores errors. While affiliate attribution is non-critical, a failed save means the inviter won't receive credit. Consider logging the error at minimum.💡 Suggested improvement
if affCode != "" { session.Set("aff", affCode) - _ = session.Save() + if err := session.Save(); err != nil { + common.SysLog(fmt.Sprintf("Failed to save affiliate code to session: %v", err)) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/oauth.go` around lines 154 - 158, The session.Save() error is being ignored after setting the affiliate code; change the code around session.Set("aff", affCode) and session.Save() to capture the returned error (e.g., err := session.Save()) and log it instead of discarding it so failed saves are visible; use the existing request/controller logger (or the package logger) to emit a warning or error including context like the affCode and the error, while still allowing execution to continue since attribution is non-critical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@common/str.go`:
- Around line 205-218: The RedeemOAuthExchangeCode function currently uses
separate RedisGet and RedisDel calls (RedisGet, RedisDel) causing a TOCTOU race
that allows multiple redemptions; change it to perform an atomic get+delete
using a Redis transaction/pipeline (TxPipeline) similar to the pattern in
RedisHSetObj: start a TxPipeline, queue a GET and a DEL for key :=
"oauth_exchange:"+code, execute the pipeline, check the GET result and unmarshal
into OAuthExchangeData (using Unmarshal) only if GET returned data, and return
nil on errors or missing data; ensure errors from the pipeline and the GET
result are handled and the DEL is still part of the same pipeline to guarantee
single-use semantics.
In `@controller/oauth.go`:
- Around line 43-45: The oauth_nonce cookie is currently set with Secure=true
which breaks local HTTP dev; change the two SetCookie calls for "oauth_nonce"
(the creation c.SetCookie("oauth_nonce", nonce, 600, "/", "", true, true) and
the clearing call that sets it to empty) to set the Secure flag
conditionally—compute a secureFlag boolean based on the incoming request (e.g.,
secureFlag := c.Request.TLS != nil || strings.HasPrefix(c.Request.Host,
"localhost") || strings.HasPrefix(c.Request.Host, "127.0.0.1") or check the
scheme) and pass secureFlag as the sixth parameter to c.SetCookie in both the
creation and clearing places so cookies work over local HTTP while remaining
secure in production.
In `@controller/user.go`:
- Around line 129-143: Race on first-time token generation: wrap the token
creation/update in a transactional, conditional update to avoid overwriting a
concurrently-set token — e.g., start a DB transaction, lock the user row (SELECT
... FOR UPDATE) or use an update with a WHERE that requires access_token IS NULL
(or a version check) so only the first request sets the token; in practice
modify the flow around user.GetAccessToken / GenerateRandomKey /
user.SetAccessToken / user.Update to re-check the current token inside the
transaction and only write if still empty (or use optimistic versioned Update),
then return the final token from the DB.
---
Nitpick comments:
In `@common/str.go`:
- Around line 151-154: The HMAC verification uses a regular string inequality
which can be timing-attack sensitive; replace the direct comparison of
GenerateHMAC(encoded) and sig with a constant-time comparison using
crypto/subtle.ConstantTimeCompare: ensure both values are []byte (or convert to
[]byte), check lengths first, then use ConstantTimeCompare on
GenerateHMAC(encoded) and sig and return nil when the comparison result is zero;
update the check around state, idx, and GenerateHMAC accordingly.
In `@controller/oauth.go`:
- Around line 154-158: The session.Save() error is being ignored after setting
the affiliate code; change the code around session.Set("aff", affCode) and
session.Save() to capture the returned error (e.g., err := session.Save()) and
log it instead of discarding it so failed saves are visible; use the existing
request/controller logger (or the package logger) to emit a warning or error
including context like the affCode and the error, while still allowing execution
to continue since attribution is non-critical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d32225e3-f187-44ad-8961-8fa4948c5755
📒 Files selected for processing (12)
common/constants.gocommon/init.gocommon/str.gocontroller/oauth.gocontroller/user.godocker-compose.ymli18n/keys.goi18n/locales/en.yamli18n/locales/zh-CN.yamli18n/locales/zh-TW.yamlrouter/api-router.goweb/src/components/auth/OAuth2Callback.jsx
✅ Files skipped from review due to trivial changes (5)
- common/init.go
- docker-compose.yml
- i18n/locales/zh-CN.yaml
- i18n/locales/en.yaml
- i18n/locales/zh-TW.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- common/constants.go
- web/src/components/auth/OAuth2Callback.jsx
0-don
force-pushed
the
feat/oauth-redirect-uri
branch
from
6dba9e0 to
bbf9b58
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
common/str.go (1)
146-174: Consider using constant-time comparison for HMAC verification.Line 152 uses direct string comparison (
!=) for HMAC validation. While the risk is mitigated by the 10-minute TTL and rate limiting on the OAuth endpoints, usingcrypto/subtle.ConstantTimeComparewould provide defense-in-depth against timing attacks.♻️ Suggested improvement
+import "crypto/subtle" + func ValidateSignedOAuthState(state string) *OAuthStateResult { idx := strings.LastIndex(state, ".") if idx < 0 { return nil } encoded, sig := state[:idx], state[idx+1:] - if GenerateHMAC(encoded) != sig { + expectedSig := GenerateHMAC(encoded) + if subtle.ConstantTimeCompare([]byte(expectedSig), []byte(sig)) != 1 { return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/str.go` around lines 146 - 174, The HMAC check in ValidateSignedOAuthState currently compares strings with !=; update the verification to use a constant-time comparison by converting the expected MAC from GenerateHMAC(encoded) and the provided sig to byte slices and using crypto/subtle.ConstantTimeCompare (or ConstantTimeEq) to compare them, returning nil on mismatch; ensure you reference ValidateSignedOAuthState and GenerateHMAC when making the change and preserve the existing behavior (return nil on failed verification).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@common/str.go`:
- Around line 146-174: The HMAC check in ValidateSignedOAuthState currently
compares strings with !=; update the verification to use a constant-time
comparison by converting the expected MAC from GenerateHMAC(encoded) and the
provided sig to byte slices and using crypto/subtle.ConstantTimeCompare (or
ConstantTimeEq) to compare them, returning nil on mismatch; ensure you reference
ValidateSignedOAuthState and GenerateHMAC when making the change and preserve
the existing behavior (return nil on failed verification).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75444f8a-015a-42a1-ad47-e0f8cd7a7531
📒 Files selected for processing (12)
common/constants.gocommon/init.gocommon/str.gocontroller/oauth.gocontroller/user.godocker-compose.ymli18n/keys.goi18n/locales/en.yamli18n/locales/zh-CN.yamli18n/locales/zh-TW.yamlrouter/api-router.goweb/src/components/auth/OAuth2Callback.jsx
✅ Files skipped from review due to trivial changes (5)
- docker-compose.yml
- common/init.go
- i18n/locales/en.yaml
- i18n/locales/zh-CN.yaml
- common/constants.go
🚧 Files skipped from review as they are similar to previous changes (2)
- i18n/locales/zh-TW.yaml
- web/src/components/auth/OAuth2Callback.jsx
0-don
force-pushed
the
feat/oauth-redirect-uri
branch
2 times, most recently
from
a65c0c7 to
e1e7316
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@common/str.go`:
- Around line 105-129: Both helpers need to short-circuit when Redis is
disabled: in redisStoreOnce(prefix string, v any, ttl time.Duration) return an
error immediately if RedisEnabled is false (avoid calling RedisSet/Get) and in
redisRedeemOnce[T any](prefix, key string) return nil if RedisEnabled is false;
add these checks at the start of the functions (use the RedisEnabled boolean) so
redisStoreOnce does not attempt Marshal/RedisSet when Redis is off and returns a
descriptive error, and redisRedeemOnce returns nil without calling
RedisGet/RedisDel/Unmarshal when Redis is off.
In `@controller/oauth.go`:
- Around line 22-48: The API router group that registers the OAuth endpoints
(/api/oauth/state and /api/oauth/exchange) is missing CORS, causing cross-origin
requests to fail; update the router where apiRouter is created to apply the CORS
middleware (call apiRouter.Use(middleware.CORS())) so the GenerateOAuthCode and
the OAuth exchange handler can accept cross-origin browser requests, or
alternatively wrap only the OAuth route registrations with middleware.CORS() if
you prefer selective application.
In `@controller/user.go`:
- Around line 117-123: The code currently ignores errors from GenerateRandomKey
and the DB write via user.Update(false), and uses a broad full-row update which
can clobber concurrent changes; change the flow so that after generating the
token (check and handle any error from GenerateRandomKey), call a new or
existing single-column persistence method (e.g., UpdateAccessToken or similar on
the user model) that writes only the access_token and returns an error,
refreshes the in-memory/cache representation of the user if needed, and only
proceed to mint the exchange code when UpdateAccessToken succeeds; remove the
call to Update(false) and ensure all errors from token generation and the narrow
update are checked and handled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9765570b-1b3b-49a4-b873-15b0fe74833a
📒 Files selected for processing (12)
common/constants.gocommon/init.gocommon/str.gocontroller/oauth.gocontroller/user.godocker-compose.ymli18n/keys.goi18n/locales/en.yamli18n/locales/zh-CN.yamli18n/locales/zh-TW.yamlrouter/api-router.goweb/src/components/auth/OAuth2Callback.jsx
✅ Files skipped from review due to trivial changes (6)
- docker-compose.yml
- i18n/locales/zh-CN.yaml
- common/constants.go
- i18n/locales/en.yaml
- router/api-router.go
- i18n/locales/zh-TW.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- i18n/keys.go
- web/src/components/auth/OAuth2Callback.jsx
- common/init.go
0-don
force-pushed
the
feat/oauth-redirect-uri
branch
3 times, most recently
from
b1ef2f8 to
3017174
Compare
Add support for OAuth login flows initiated from external frontends on different domains. When redirect_uri is provided, uses HMAC-signed state tokens instead of session-based state (which won't survive cross-domain redirects). - Add OAUTH_ALLOWED_REDIRECT_ORIGINS env var (comma-separated origins) - Add IsAllowedRedirectURI() to validate redirect URIs against allowlist - Dual-mode state validation: signed token for cross-origin, session for same-origin - setupLoginAndRedirect() generates access token and redirects with user data - Frontend OAuth2Callback handles redirect_url in response - i18n key for redirect URI not allowed (en, zh-CN, zh-TW)
0-don
force-pushed
the
feat/oauth-redirect-uri
branch
from
3017174 to
e7218a2
Compare
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
Adds OAuth redirect URI support for external frontends on different domains. An external app can initiate OAuth login via new-api and receive user credentials securely through a one-time code exchange.
How it works
GET /api/oauth/state?redirect_uri=https://myapp.com/callbackto get a random state key (stored in Redis, 10min TTL)redirect_urlcontaining only?code=...(no tokens in URL)POST /api/oauth/exchangeto getaccess_tokenand user dataSecurity
OAUTH_ALLOWED_REDIRECT_ORIGINSenv var (comma-separated allowlist, HTTPS enforced except localhost)New endpoint
POST /api/oauth/exchangewith body{ "code": "..." }returns{ access_token, user_id, username, display_name, role }.Configuration
Summary by CodeRabbit
New Features
Documentation