fix: derive oauth token exchange redirect_uri from callback request - #6575
Open
feitianbubu wants to merge 1 commit into
Open
fix: derive oauth token exchange redirect_uri from callback request#6575feitianbubu wants to merge 1 commit into
feitianbubu wants to merge 1 commit into
Conversation
Contributor
WalkthroughOAuth and OIDC token exchanges now build callback redirect URIs from incoming request data. OIDC response handling now reports body-read failures and includes truncated response content when no access token is returned. ChangesOAuth callback flow
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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
🤖 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/oidc.go`:
- Around line 103-104: Update the empty-token error handling in the OIDC token
exchange flow around AccessToken and logger.LogError so it never logs the raw
body response. Log only safe error fields, or redact refresh_token, id_token,
and other token fields before producing any bounded preview, while preserving
the existing failure handling.
- Around line 90-94: Update the response-reading logic in ExchangeToken to read
at most the configured fixed maximum response size plus one byte, using a
bounded reader around res.Body. Detect and reject responses exceeding the
maximum immediately after reading, before unmarshalling, and return the existing
OAuth error path with an appropriate size-limit error.
In `@oauth/redirect_uri.go`:
- Around line 20-24: Update callbackRedirectURI’s X-Forwarded-Proto handling to
trim and normalize the first header value, accepting only non-empty “http” or
“https” schemes. For empty or unsupported values, fall back to the TLS-aware
scheme, using “https” when the request is TLS-enabled and “http” otherwise,
without altering the existing behavior when no forwarded header is present.
🪄 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 Plus
Run ID: 0e122b16-2da3-44ce-823c-4cdb02f86cd3
📒 Files selected for processing (3)
oauth/generic.gooauth/oidc.gooauth/redirect_uri.go
Comment on lines
+90
to
+94
| bodyBytes, err := io.ReadAll(res.Body) | ||
| if err != nil { | ||
| logger.LogError(ctx, fmt.Sprintf("[OAuth-OIDC] ExchangeToken read body error: %s", err.Error())) | ||
| return nil, NewOAuthErrorWithRaw(i18n.MsgOAuthConnectFailed, map[string]any{"Provider": "OIDC"}, err.Error()) | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the OIDC token response before reading it.
io.ReadAll(res.Body) has no byte limit. A faulty or compromised token endpoint can return an oversized response during login and force a large allocation. Read at most a fixed maximum plus one byte. Reject an oversized response before unmarshalling.
Proposed fix
+const maxOIDCTokenResponseBytes = 1 << 20
+
-bodyBytes, err := io.ReadAll(res.Body)
+bodyBytes, err := io.ReadAll(
+ io.LimitReader(res.Body, maxOIDCTokenResponseBytes+1),
+)
if err != nil {
...
}
+if len(bodyBytes) > maxOIDCTokenResponseBytes {
+ return nil, NewOAuthError(i18n.MsgOAuthTokenFailed, map[string]any{
+ "Provider": "OIDC",
+ })
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| bodyBytes, err := io.ReadAll(res.Body) | |
| if err != nil { | |
| logger.LogError(ctx, fmt.Sprintf("[OAuth-OIDC] ExchangeToken read body error: %s", err.Error())) | |
| return nil, NewOAuthErrorWithRaw(i18n.MsgOAuthConnectFailed, map[string]any{"Provider": "OIDC"}, err.Error()) | |
| } | |
| const maxOIDCTokenResponseBytes = 1 << 20 | |
| bodyBytes, err := io.ReadAll( | |
| io.LimitReader(res.Body, maxOIDCTokenResponseBytes+1), | |
| ) | |
| if err != nil { | |
| logger.LogError(ctx, fmt.Sprintf("[OAuth-OIDC] ExchangeToken read body error: %s", err.Error())) | |
| return nil, NewOAuthErrorWithRaw(i18n.MsgOAuthConnectFailed, map[string]any{"Provider": "OIDC"}, err.Error()) | |
| } | |
| if len(bodyBytes) > maxOIDCTokenResponseBytes { | |
| return nil, NewOAuthError(i18n.MsgOAuthTokenFailed, map[string]any{ | |
| "Provider": "OIDC", | |
| }) | |
| } |
🤖 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/oidc.go` around lines 90 - 94, Update the response-reading logic in
ExchangeToken to read at most the configured fixed maximum response size plus
one byte, using a bounded reader around res.Body. Detect and reject responses
exceeding the maximum immediately after reading, before unmarshalling, and
return the existing OAuth error path with an appropriate size-limit error.
Comment on lines
103
to
+104
| if oidcResponse.AccessToken == "" { | ||
| logger.LogError(ctx, "[OAuth-OIDC] ExchangeToken failed: empty access token") | ||
| logger.LogError(ctx, fmt.Sprintf("[OAuth-OIDC] ExchangeToken failed: empty access token, response: %s", string(bodyBytes[:min(len(bodyBytes), 500)]))) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not log the raw OIDC response.
When AccessToken is empty, the body can still contain refresh_token or id_token. Line 104 sends the raw body to logger.LogError. The supplied logger/logger.go Lines 84-86 context shows that this message goes directly to the shared log helper. Log only safe error fields, or redact all token fields before creating a bounded preview.
Proposed fix
type oidcOAuthResponse struct {
AccessToken string `json:"access_token"`
IDToken string `json:"id_token"`
RefreshToken string `json:"refresh_token"`
TokenType string `json:"token_type"`
ExpiresIn int `json:"expires_in"`
Scope string `json:"scope"`
+ Error string `json:"error"`
+ ErrorDescription string `json:"error_description"`
}
- logger.LogError(ctx, fmt.Sprintf("[OAuth-OIDC] ExchangeToken failed: empty access token, response: %s", string(bodyBytes[:min(len(bodyBytes), 500)])))
+ logger.LogError(ctx, fmt.Sprintf(
+ "[OAuth-OIDC] ExchangeToken failed: status=%d, error=%s, error_description=%s",
+ res.StatusCode, oidcResponse.Error, oidcResponse.ErrorDescription,
+ ))🤖 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/oidc.go` around lines 103 - 104, Update the empty-token error handling
in the OIDC token exchange flow around AccessToken and logger.LogError so it
never logs the raw body response. Log only safe error fields, or redact
refresh_token, id_token, and other token fields before producing any bounded
preview, while preserving the existing failure handling.
Comment on lines
+20
to
+24
| if proto := c.Request.Header.Get("X-Forwarded-Proto"); proto != "" { | ||
| scheme = strings.ToLower(strings.TrimSpace(strings.Split(proto, ",")[0])) | ||
| } else if c.Request.TLS == nil { | ||
| scheme = "http" | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 4 \
'X-Forwarded-Proto|SetTrustedProxies|TrustedPlatform|RemoteIPHeaders|proxy_set_header' \
--glob '*.go' --glob '*.md' --glob '*.yaml' --glob '*.yml' --glob '*.conf' .Repository: QuantumNous/new-api
Length of output: 6975
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '--- files ---\n'
fd -a 'redirect_uri.go|passkey/service.go|trusted_proxies.go|web.go|main.go' . | sed 's#^\./##'
printf '\n--- oauth/redirect_uri.go ---\n'
cat -n oauth/redirect_uri.go
printf '\n--- middleware/trusted_proxies.go ---\n'
cat -n middleware/trusted_proxies.go
printf '\n--- search SetTrustedProxies usages ---\n'
rg -n -C 3 'SetTrustedProxies' --glob '*.go' .
printf '\n--- detectScheme behavioral probe ---\n'
python3 - <<'PY'
import urllib.parse, re
def detect(proto, tls, r_scheme):
if proto != "":
scheme = strings_split_comma(proto)[0].lower().strip()
elif tls is None:
scheme = "http"
elif r_scheme:
scheme = r_scheme.lower()
else:
scheme = "https"
return scheme
def strings_split_comma(s):
return s.split(",")
for proto in ["ftp", ",https", "http,https", "https,,http", "http "]:
print(f"{proto!r}: scheme={detect(proto, None, '')} callback={('https://'+proto+'://example.ignored/redirect').startswith('https://ftp://') if False else ''}")
# Gin Forwarded headers are populated after TrustedProxies call. Here we match local code semantics.
print("comma-first-value examples:", [(f"https://example/auth/callback?redirect_uri={detect(p,None,'')%3A//example/callback}")] for p in ["ftp", ",https"])
PYRepository: QuantumNous/new-api
Length of output: 248
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- files ---'
fd -a 'redirect_uri.go|passkey/service.go|trusted_proxies.go|web.go|main.go|gin.go' . | sed 's#^\./##'
printf '%s\n' ''
printf '%s\n' '--- oauth/redirect_uri.go ---'
cat -n oauth/redirect_uri.go
printf '%s\n' ''
printf '%s\n' '--- middleware/trusted_proxies.go ---'
cat -n middleware/trusted_proxies.go
printf '%s\n' ''
printf '%s\n' '--- search SetTrustedProxies usages ---'
rg -n -C 3 'SetTrustedProxies' --glob '*.go' .
printf '%s\n' ''
printf '%s\n' '--- header parsing behavioral probe ---'
python3 - <<'PY'
def parse_scheme(proto, tls):
if proto != "":
scheme = proto.split(',')[0].lower().strip()
elif tls is None:
scheme = "http"
else:
scheme = "https"
return scheme
cases = ["ftp", ",https", "-", "http,https", "https,,http", "http "]
for proto in cases:
print(f"proto={proto!r:12} TLS={None!r:4} => scheme={parse_scheme(proto, None)!r}")
PYRepository: QuantumNous/new-api
Length of output: 6557
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- common/gin.go ---'
cat -n common/gin.go
printf '%s\n' ''
printf '%s\n' '--- main.go relevant sections ---'
cat -n main.go | sed -n '1,220p'
printf '%s\n' ''
printf '%s\n' '--- router/main.go ---'
cat -n router/main.go | sed -n '1,220p'
printf '%s\n' ''
printf '%s\n' '--- setTrustedProxies availability/version hints ---'
rg -n 'func .*SetTrustedProxies|SetTrustedProxies\(|github.com/gin-gonic/gin|SetTrustedIPs|TrustedPlatform' .Repository: QuantumNous/new-api
Length of output: 40615
Reject unsupported X-Forwarded-Proto values.
callbackRedirectURI copies the first comma-separated header value into the callback redirect_uri path. Values such as ftp or an empty first value like ,https can produce an unsupported URI. Accept only http or https; for unsupported values fall back to the TLS-aware https scheme.
🤖 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_uri.go` around lines 20 - 24, Update callbackRedirectURI’s
X-Forwarded-Proto handling to trim and normalize the first header value,
accepting only non-empty “http” or “https” schemes. For empty or unsupported
values, fall back to the TLS-aware scheme, using “https” when the request is
TLS-enabled and “http” otherwise, without altering the existing behavior when no
forwarded header is present.
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.
修复多域名部署时, 非主域名无法使用oidc登录的问题
因标准的oidc提供商会验证请求和回调的域名必须相同
注意：反代部署需要传
X-Forwarded-Proto（nginx 标准配置都有）；没有该头且后端是明文 HTTP 时按 http 处理。🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
Bug fix，我已提交或关联对应 Issue，且不会将设计取舍、预期不一致或理解偏差直接归类为 bug。📸 运行证明 / Proof of Work
本地多域名部署验证：同一实例分别从两个域名走 OIDC 登录和自定义 OAuth 登录。修复前非 ServerAddress 域名一律
invalid_grant；修复后两个域名均登录成功，原 ServerAddress 域名不受影响。Summary by CodeRabbit