fix: move oidc discovery url fetching to backend proxy #2437
Conversation
WalkthroughAdds a backend OIDC discovery HTTP handler and route, and updates the frontend settings UI to add an Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Frontend (Browser)
participant Server as Backend API
participant Provider as OIDC Provider (.well-known)
Browser->>Server: GET /api/oidc/discovery?well_known_url=<url> (AdminAuth, rate-limited)
Server->>Provider: GET <well_known_url> (5s timeout, SSRF validated)
alt success & valid JSON
Provider-->>Server: 200 + discovery JSON
Server-->>Browser: 200 { success: true, data: <discovery JSON> }
else invalid URL / SSRF / network / non-JSON
Server-->>Browser: 4xx/5xx { success: false, message: <error> }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/settings/SystemSetting.jsx (1)
501-526: Guard on{ success }and avoid mutatinginputsdirectly in OIDC discovery flow.
If the backend returns{ success:false, message }(possibly with HTTP 200), the current code will throw onres.data[...]. Also direct mutation can fail to re-render.- const res = await API.get('/api/oidc/discovery', { + const payload = await API.get('/api/oidc/discovery', { params: { well_known_url: inputs['oidc.well_known'] }, - }).then((response => response.data)); - inputs['oidc.authorization_endpoint'] = - res.data['authorization_endpoint']; - inputs['oidc.token_endpoint'] = res.data['token_endpoint']; - inputs['oidc.user_info_endpoint'] = res.data['userinfo_endpoint']; + }).then((response) => response.data); + if (!payload?.success) { + showError(payload?.message || t('获取 OIDC 配置失败')); + return; + } + const cfg = payload.data || {}; + setInputs((prev) => ({ + ...prev, + 'oidc.authorization_endpoint': cfg['authorization_endpoint'] || '', + 'oidc.token_endpoint': cfg['token_endpoint'] || '', + 'oidc.user_info_endpoint': cfg['userinfo_endpoint'] || '', + })); showSuccess(t('获取 OIDC 配置成功！'));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
controller/oidc.go(1 hunks)router/api-router.go(1 hunks)web/src/components/settings/SystemSetting.jsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
router/api-router.go (2)
middleware/rate-limit.go (1)
CriticalRateLimit(104-109)controller/oidc.go (1)
GetOIDCDiscovery(230-278)
web/src/components/settings/SystemSetting.jsx (1)
web/src/helpers/api.js (6)
res(224-224)res(225-225)res(301-301)res(302-302)API(29-37)API(29-37)
|
看了下还是需要在请求的地方要加入csrf防护的拦截，这样的话就不能默认开启反代的方式了，需要加一个配置项允许切换，需要手动配置反代的地址到csrf名单那边。 |
OK，我忙完了看看怎么修改合适 |
|
已提交最新的SSRF修复补丁 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
controller/oidc.go (1)
269-292: Add redirect blocking, status code validation, and response size limit for defense-in-depth.The HTTP client follows redirects by default, allowing SSRF bypass via open redirects. Additionally, missing status code validation and unbounded response decoding create reliability and DoS risks.
🔎 Apply these hardening measures:
+ import "io" + // 请求OIDC发现端点 client := http.Client{ Timeout: 5 * time.Second, + CheckRedirect: func(req *http.Request, via []*http.Request) error { + return http.ErrUseLastResponse // Block redirects to prevent SSRF bypass + }, } res, err := client.Get(wellKnownURL) if err != nil { c.JSON(http.StatusInternalServerError, gin.H{ "success": false, "message": "获取 OIDC 配置失败，请检查网络状况和 Well-Known URL 是否正确", }) return } defer res.Body.Close() + if res.StatusCode < 200 || res.StatusCode >= 300 { + c.JSON(http.StatusBadGateway, gin.H{ + "success": false, + "message": fmt.Sprintf("OIDC 服务器返回错误状态码: %d", res.StatusCode), + }) + return + } + var discoveryConfig map[string]interface{} - err = json.NewDecoder(res.Body).Decode(&discoveryConfig) + err = json.NewDecoder(io.LimitReader(res.Body, 1<<20)).Decode(&discoveryConfig) // 1MB limit if err != nil {
🧹 Nitpick comments (1)
web/src/components/settings/SystemSetting.jsx (1)
513-527: Remove debug logging and consider immutable state update.Line 524 has a
console.logthat should be removed. Also, directly mutatinginputs(lines 525-527) can cause React re-render issues, though this pattern is used elsewhere in the file.🔎 Suggested fix:
res = await axios.create().get(inputs['oidc.well_known']).then(response => response.data); } - console.log(res); - inputs['oidc.authorization_endpoint'] = res['authorization_endpoint']; - inputs['oidc.token_endpoint'] = res['token_endpoint']; - inputs['oidc.user_info_endpoint'] = res['userinfo_endpoint']; + setInputs(prev => ({ + ...prev, + 'oidc.authorization_endpoint': res['authorization_endpoint'], + 'oidc.token_endpoint': res['token_endpoint'], + 'oidc.user_info_endpoint': res['userinfo_endpoint'], + })); showSuccess(t('获取 OIDC 配置成功！'));
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
controller/oidc.go(1 hunks)router/api-router.go(1 hunks)web/src/components/settings/SystemSetting.jsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- router/api-router.go
🧰 Additional context used
🧬 Code graph analysis (2)
web/src/components/settings/SystemSetting.jsx (1)
web/src/helpers/api.js (6)
res(224-224)res(225-225)res(301-301)res(302-302)API(29-37)API(29-37)
controller/oidc.go (2)
setting/system_setting/fetch_setting.go (1)
GetFetchSetting(32-34)common/ssrf_protection.go (1)
ValidateURLWithFetchSetting(305-327)
🔇 Additional comments (4)
web/src/components/settings/SystemSetting.jsx (4)
65-65: LGTM! Sensible default oftrueforoidc.use_backend_proxyto avoid CORS issues by default.
188-188: LGTM! Correct boolean parsing for the new setting.
582-589: LGTM! Correctly saves the backend proxy setting only when changed.
1375-1390: LGTM! UI implementation follows the existing pattern for checkbox settings. The checkbox correctly integrates with the form and uses the standardhandleCheckboxChangehandler.
| fetchSetting := system_setting.GetFetchSetting() | ||
| // SSRF防护 - 验证URL安全性 | ||
| err := common.ValidateURLWithFetchSetting( | ||
| wellKnownURL, | ||
| true, | ||
| true, | ||
| false, | ||
| false, | ||
| fetchSetting.DomainList, | ||
| fetchSetting.IpList, | ||
| fetchSetting.AllowedPorts, | ||
| true, | ||
| ) | ||
| if err != nil { | ||
| c.JSON(http.StatusForbidden, gin.H{ | ||
| "success": false, | ||
| "message": fmt.Sprintf("URL SSRF安全验证失败: %s", err.Error()), | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
SSRF protection is ineffective due to allowPrivateIp=true.
The second parameter (true on line 253) allows requests to private IP addresses, which defeats the primary purpose of SSRF protection. This permits attackers to target internal services at 127.0.0.1, 192.168.x.x, and cloud metadata endpoints like 169.254.169.254.
🔎 Apply this diff to block private IP access:
fetchSetting := system_setting.GetFetchSetting()
// SSRF防护 - 验证URL安全性
err := common.ValidateURLWithFetchSetting(
wellKnownURL,
true,
- true,
+ fetchSetting.AllowPrivateIp, // Use configured setting, defaults to false for security
false,
false,
fetchSetting.DomainList,
fetchSetting.IpList,
fetchSetting.AllowedPorts,
true,
)📝 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.
| fetchSetting := system_setting.GetFetchSetting() | |
| // SSRF防护 - 验证URL安全性 | |
| err := common.ValidateURLWithFetchSetting( | |
| wellKnownURL, | |
| true, | |
| true, | |
| false, | |
| false, | |
| fetchSetting.DomainList, | |
| fetchSetting.IpList, | |
| fetchSetting.AllowedPorts, | |
| true, | |
| ) | |
| if err != nil { | |
| c.JSON(http.StatusForbidden, gin.H{ | |
| "success": false, | |
| "message": fmt.Sprintf("URL SSRF安全验证失败: %s", err.Error()), | |
| }) | |
| return | |
| } | |
| fetchSetting := system_setting.GetFetchSetting() | |
| // SSRF防护 - 验证URL安全性 | |
| err := common.ValidateURLWithFetchSetting( | |
| wellKnownURL, | |
| true, | |
| fetchSetting.AllowPrivateIp, // Use configured setting, defaults to false for security | |
| false, | |
| false, | |
| fetchSetting.DomainList, | |
| fetchSetting.IpList, | |
| fetchSetting.AllowedPorts, | |
| true, | |
| ) | |
| if err != nil { | |
| c.JSON(http.StatusForbidden, gin.H{ | |
| "success": false, | |
| "message": fmt.Sprintf("URL SSRF安全验证失败: %s", err.Error()), | |
| }) | |
| return | |
| } |
🤖 Prompt for AI Agents
In controller/oidc.go around lines 248 to 267, the SSRF check currently calls
ValidateURLWithFetchSetting with the second boolean set to true
(allowPrivateIp=true), which permits private IPs and defeats SSRF protection;
change that argument to false so private IPs are blocked, keep the other
parameters as-is, and ensure any callers or unit tests that rely on allowing
private IPs are updated accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
controller/oidc.go (1)
284-292: Limit response body size to prevent DoS attacks.The response body is decoded without size limits, allowing malicious or misconfigured OIDC servers to exhaust server memory by sending unbounded response sizes.
🔎 Apply this diff to add 1MB response size limit
+ // 限制响应体大小防止DoS + limitedReader := io.LimitReader(res.Body, 1<<20) // 1MB limit + var discoveryConfig map[string]interface{} - err = json.NewDecoder(res.Body).Decode(&discoveryConfig) + err = json.NewDecoder(limitedReader).Decode(&discoveryConfig) if err != nil { c.JSON(http.StatusInternalServerError, gin.H{ "success": false, "message": "解析 OIDC 配置失败", }) return }Note: You'll need to add
"io"to the imports at the top of the file.Based on past review comments noting this pattern for similar endpoints.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controller/oidc.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
controller/oidc.go (2)
setting/system_setting/fetch_setting.go (1)
GetFetchSetting(32-34)common/ssrf_protection.go (1)
ValidateURLWithFetchSetting(305-327)
🔇 Additional comments (3)
controller/oidc.go (3)
230-238: LGTM!Parameter validation correctly ensures the required
well_known_urlis provided.
240-247: LGTM!URL scheme validation correctly restricts input to HTTP/HTTPS protocols.
248-267: LGTM!SSRF protection is correctly implemented using system fetch settings. The previous concern about hardcoded
allowPrivateIp=truehas been resolved—the code now properly usesfetchSetting.AllowPrivateIpto respect system configuration.
| // 请求OIDC发现端点 | ||
| client := http.Client{ | ||
| Timeout: 5 * time.Second, | ||
| } | ||
|
|
||
| res, err := client.Get(wellKnownURL) | ||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{ | ||
| "success": false, | ||
| "message": "获取 OIDC 配置失败，请检查网络状况和 Well-Known URL 是否正确", | ||
| }) | ||
| return | ||
| } | ||
| defer res.Body.Close() |
There was a problem hiding this comment.
Add CheckRedirect and validate HTTP status code before decoding.
Two critical hardening measures are missing:
-
Missing
CheckRedirect: The HTTP client follows redirects by default, allowing attackers to bypass SSRF validation by redirecting from an allowed URL to internal endpoints (e.g.,169.254.169.254). Past review comments explicitly noted this pattern. -
Missing status code validation: The code attempts to decode the response regardless of HTTP status (4xx/5xx), which leads to confusing error messages and potential JSON parsing failures on error pages.
🔎 Apply this diff to add both protections
// 请求OIDC发现端点
client := http.Client{
Timeout: 5 * time.Second,
+ CheckRedirect: func(req *http.Request, via []*http.Request) error {
+ return http.ErrUseLastResponse
+ },
}
res, err := client.Get(wellKnownURL)
if err != nil {
c.JSON(http.StatusInternalServerError, gin.H{
"success": false,
"message": "获取 OIDC 配置失败，请检查网络状况和 Well-Known URL 是否正确",
})
return
}
defer res.Body.Close()
+
+ if res.StatusCode != http.StatusOK {
+ c.JSON(http.StatusInternalServerError, gin.H{
+ "success": false,
+ "message": fmt.Sprintf("OIDC 服务器返回错误状态码: %d", res.StatusCode),
+ })
+ return
+ }Reference implementations in the codebase:
- CheckRedirect:
service/http_client.go:44-52 - Status code checks:
service/webhook.go:87-88,service/download.go:61-66
🤖 Prompt for AI Agents
In controller/oidc.go around lines 269-282, the HTTP client used to fetch the
OIDC well-known URL lacks a CheckRedirect policy and does not validate the
response status code before decoding; update the client construction to set
CheckRedirect to a safe policy (reject or limit redirects consistent with
service/http_client.go) and keep the Timeout, then after
client.Get(wellKnownURL) check the returned error and also verify res.StatusCode
is in the 2xx range (return a 4xx/5xx appropriate JSON error if not) before
attempting to read or decode res.Body; ensure res.Body is closed in all
early-return paths.
seefs001
added
the
Ready to merge
2 participants
fix #994
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.