Bug fix oauth redirect issue#4954
Open
dingjianrui-crypto wants to merge 1 commit into
Open
Conversation
Contributor
WalkthroughThis PR refactors OAuth redirect URI construction to derive the public origin from incoming request headers instead of relying solely on static server configuration. Three OAuth providers (Discord, Generic, OIDC) now use a new ChangesOAuth Redirect URI Refactoring
Auth Store localStorage Cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🧹 Nitpick comments (1)
oauth/redirect_test.go (1)
11-26: ⚡ Quick winAdd coverage for
Forwarded-header host fallback.Current tests don’t cover the
Forwarded: proto=...;host=...path, which is a key reverse-proxy branch inrequestPublicOrigin.🧪 Suggested test case
func TestCallbackRedirectURIUsesRequestOrigin(t *testing.T) { @@ require.Equal(t, "https://www.modelsphere.net/oauth/google", callbackRedirectURI(c, "/oauth/google")) } + +func TestCallbackRedirectURIUsesForwardedHeaderHostAndProto(t *testing.T) { + req := httptest.NewRequest("GET", "http://internal:3000/api/oauth/google", nil) + req.Header.Set("Forwarded", `for=1.2.3.4;proto=https;host=www.modelsphere.net`) + + c := &gin.Context{Request: req} + + require.Equal(t, "https://www.modelsphere.net/oauth/google", callbackRedirectURI(c, "/oauth/google")) +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@oauth/redirect_test.go` around lines 11 - 26, Add a test that covers the Forwarded-header fallback in requestPublicOrigin: create an httptest request with an internal URL (e.g., http://internal:3000/api/oauth/google), set the Forwarded header to include proto and host (e.g., "proto=https;host=www.modelsphere.net"), build a gin.Context with that request and assert callbackRedirectURI(c, "/oauth/google") returns "https://www.modelsphere.net/oauth/google"; this ensures callbackRedirectURI and requestPublicOrigin correctly parse the Forwarded header host fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@oauth/redirect.go`:
- Around line 24-35: The code that determines host uses
firstForwardedValue(req.Header.Get("X-Forwarded-Host")) and falls back to
req.Host, but it doesn't parse the host from the standard Forwarded header;
update the host resolution to also extract host from req.Header.Get("Forwarded")
when X-Forwarded-Host is empty by reusing or adding a parser (similar to
forwardedProto) to pull the host=... token (e.g., call
forwardedHost(forwardedHeader) or extend firstForwardedValue to check the
Forwarded header), then assign that parsed host to the host variable before
falling back to req.Host so redirect_uri uses the correct proxy-provided host.
In `@web/default/src/stores/auth-store.ts`:
- Line 97: initUser currently restores only the persisted user but not the uid
that reset() clears; update the initUser initialization to also read
window.localStorage.getItem('uid') alongside 'user' and return/assign the uid
into the store (same place where the user is restored), and in the catch block
ensure you remove both 'user' and 'uid' from localStorage; reference the
initUser initializer and the reset() method and the keys 'user' and 'uid' so the
OAuth-set uid (web/default/src/routes/oauth/$provider.tsx) is available for the
API module (web/default/src/lib/api.ts) after page reloads.
---
Nitpick comments:
In `@oauth/redirect_test.go`:
- Around line 11-26: Add a test that covers the Forwarded-header fallback in
requestPublicOrigin: create an httptest request with an internal URL (e.g.,
http://internal:3000/api/oauth/google), set the Forwarded header to include
proto and host (e.g., "proto=https;host=www.modelsphere.net"), build a
gin.Context with that request and assert callbackRedirectURI(c, "/oauth/google")
returns "https://www.modelsphere.net/oauth/google"; this ensures
callbackRedirectURI and requestPublicOrigin correctly parse the Forwarded header
host fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3e1445ca-7930-4dce-a6cf-cd6401d36297
📒 Files selected for processing (6)
oauth/discord.gooauth/generic.gooauth/oidc.gooauth/redirect.gooauth/redirect_test.goweb/default/src/stores/auth-store.ts
Comment on lines
+24
to
+35
| host := firstForwardedValue(req.Header.Get("X-Forwarded-Host")) | ||
| if host == "" { | ||
| host = strings.TrimSpace(req.Host) | ||
| } | ||
| if host == "" { | ||
| return "" | ||
| } | ||
|
|
||
| proto := firstForwardedValue(req.Header.Get("X-Forwarded-Proto")) | ||
| if proto == "" { | ||
| proto = forwardedProto(req.Header.Get("Forwarded")) | ||
| } |
Contributor
There was a problem hiding this comment.
Parse host= from Forwarded when X-Forwarded-Host is absent.
Line 24 only checks X-Forwarded-Host; if a proxy provides only Forwarded: proto=...;host=..., Line 26 falls back to internal req.Host, which can cause OAuth redirect_uri mismatch and failed token exchange.
💡 Suggested fix
func requestPublicOrigin(c *gin.Context) string {
@@
req := c.Request
host := firstForwardedValue(req.Header.Get("X-Forwarded-Host"))
+ if host == "" {
+ host = forwardedHost(req.Header.Get("Forwarded"))
+ }
if host == "" {
host = strings.TrimSpace(req.Host)
}
@@
return strings.TrimSpace(proto) + "://" + host
}
@@
func forwardedProto(value string) string {
@@
}
+
+func forwardedHost(value string) string {
+ value = firstForwardedValue(value)
+ if value == "" {
+ return ""
+ }
+ for _, part := range strings.Split(value, ";") {
+ part = strings.TrimSpace(part)
+ key, rawValue, ok := strings.Cut(part, "=")
+ if !ok || !strings.EqualFold(strings.TrimSpace(key), "host") {
+ continue
+ }
+ return strings.Trim(strings.TrimSpace(rawValue), `"`)
+ }
+ return ""
+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@oauth/redirect.go` around lines 24 - 35, The code that determines host uses
firstForwardedValue(req.Header.Get("X-Forwarded-Host")) and falls back to
req.Host, but it doesn't parse the host from the standard Forwarded header;
update the host resolution to also extract host from req.Header.Get("Forwarded")
when X-Forwarded-Host is empty by reusing or adding a parser (similar to
forwardedProto) to pull the host=... token (e.g., call
forwardedHost(forwardedHeader) or extend firstForwardedValue to check the
Forwarded header), then assign that parsed host to the host variable before
falling back to req.Host so redirect_uri uses the correct proxy-provided host.
| set((state) => { | ||
| if (typeof window !== 'undefined') { | ||
| window.localStorage.removeItem('user') | ||
| window.localStorage.removeItem('uid') |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find where uid is written to and read from localStorage to determine if it should be restored
# Search for localStorage.setItem('uid') or localStorage.getItem('uid')
echo "=== Searching for uid localStorage writes ==="
rg -n --type=ts --type=tsx -C3 "localStorage\.setItem\(['\"]uid['\"]"
echo -e "\n=== Searching for uid localStorage reads ==="
rg -n --type=ts --type=tsx -C3 "localStorage\.getItem\(['\"]uid['\"]"
echo -e "\n=== Searching for other uid localStorage operations ==="
rg -n --type=ts --type=tsx -C3 "localStorage\.(removeItem|clear).*uid"Repository: QuantumNous/new-api
Length of output: 314
🏁 Script executed:
#!/bin/bash
# Re-run search with corrected syntax (remove problematic tsx type)
echo "=== Searching for uid localStorage writes ==="
rg -n "localStorage\.setItem\(['\"]uid['\"]" --max-count=20
echo -e "\n=== Searching for uid localStorage reads ==="
rg -n "localStorage\.getItem\(['\"]uid['\"]" --max-count=20
echo -e "\n=== Searching for alternative localStorage patterns for uid ==="
rg -n "localStorage\[.uid" --max-count=20
echo -e "\n=== Broader search for 'uid' in localStorage context ==="
rg -n "localStorage.*uid" -A2 -B2 --max-count=30Repository: QuantumNous/new-api
Length of output: 2512
🏁 Script executed:
#!/bin/bash
# Read the full auth-store.ts file to understand context
echo "=== Full auth-store.ts file ==="
cat -n web/default/src/stores/auth-store.tsRepository: QuantumNous/new-api
Length of output: 3688
Add uid restoration during initialization to match the reset cleanup.
The change removes uid from localStorage in the reset() method, but the uid value is not restored during initialization in initUser(). This creates an asymmetry: user is initialized, persisted, and cleared (full lifecycle), while uid is only cleared.
Based on the codebase search, uid is set by the OAuth flow (web/default/src/routes/oauth/$provider.tsx) and read by the API module (web/default/src/lib/api.ts) on every request. Since the reset() method now claims responsibility for clearing this state, the initUser() function should also restore it to maintain a complete lifecycle consistent with the coding guideline: "Persist state to localStorage within the store and restore it during initialization."
Consider restoring uid during initialization alongside user:
Suggested approach
const initUser = (() => {
try {
if (typeof window !== 'undefined') {
const saved = window.localStorage.getItem('user')
// Also check uid to ensure it's available after page reload
window.localStorage.getItem('uid')
return saved ? JSON.parse(saved) : null
}
} catch {
if (typeof window !== 'undefined') {
window.localStorage.removeItem('user')
window.localStorage.removeItem('uid')
}
}
return null
})()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/default/src/stores/auth-store.ts` at line 97, initUser currently restores
only the persisted user but not the uid that reset() clears; update the initUser
initialization to also read window.localStorage.getItem('uid') alongside 'user'
and return/assign the uid into the store (same place where the user is
restored), and in the catch block ensure you remove both 'user' and 'uid' from
localStorage; reference the initUser initializer and the reset() method and the
keys 'user' and 'uid' so the OAuth-set uid
(web/default/src/routes/oauth/$provider.tsx) is available for the API module
(web/default/src/lib/api.ts) after page reloads.
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.
Important
📝 变更描述 / Description
(简述：做了什么？为什么这样改能生效？请基于你对代码逻辑的理解来写，避免粘贴未经整理的内容)
🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
Bug fix，我已提交或关联对应 Issue，且不会将设计取舍、预期不一致或理解偏差直接归类为 bug。📸 运行证明 / Proof of Work
(请在此粘贴截图、关键日志或测试报告，以证明变更生效)
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests