fix(custom-oauth): pass SSRF allowlist to all fetch calls in CustomOAuth#40587
fix(custom-oauth): pass SSRF allowlist to all fetch calls in CustomOAuth#40587Varun789-mx wants to merge 1 commit into
Conversation
|
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughCustom OAuth server HTTP calls for token exchange, identity retrieval, and email lookup now include the SSRF allowlist setting from admin configuration. The ChangesSSRF Allowlist for OAuth Handshake
Sequence Diagram(s)sequenceDiagram
participant CustomOAuthServer
participant Settings
participant IdentityProvider
CustomOAuthServer->>Settings: read SSRF_Allowlist
CustomOAuthServer->>IdentityProvider: POST /oauth/token (allowList: SSRF_Allowlist)
CustomOAuthServer->>IdentityProvider: GET /userinfo (allowList: SSRF_Allowlist)
CustomOAuthServer->>IdentityProvider: GET /email (allowList: SSRF_Allowlist)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Varun789-mx
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/meteor/app/custom-oauth/server/custom_oauth_server.js`:
- Line 150: The SSRF options are contradictory: calls to the server-fetch
options set both ignoreSsrfValidation: true and allowList:
settings.get('SSRF_Allowlist'), so decide and make them consistent across all
usages (the calls that pass these options to `@rocket.chat/server-fetch`): either
remove ignoreSsrfValidation: true and keep allowList to enforce the whitelist,
or remove allowList and keep ignoreSsrfValidation: true if you intend a full
bypass; update every occurrence that currently includes both (references:
ignoreSsrfValidation, allowList, settings.get('SSRF_Allowlist')) and add a short
inline comment stating which of the two approaches you chose and why for future
reviewers.
🪄 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: 256dc29c-263d-4599-ad60-bfd37e68524f
📒 Files selected for processing (1)
apps/meteor/app/custom-oauth/server/custom_oauth_server.js
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/custom-oauth/server/custom_oauth_server.js
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/custom-oauth/server/custom_oauth_server.js">
<violation number="1" location="apps/meteor/app/custom-oauth/server/custom_oauth_server.js:186">
P1: `ignoreSsrfValidation: true` and `allowList` are mutually exclusive in `@rocket.chat/server-fetch`. When `ignoreSsrfValidation` is `true`, the code skips the entire SSRF validation branch including the allowList check — so the `allowList` parameter here is dead code and has no effect.
The existing codebase pattern (e.g., `AppHttpBridge`) treats these as exclusive:
```js
shouldIgnoreSsrf
? { ignoreSsrfValidation: true }
: { ignoreSsrfValidation: false, allowList: settings.get('SSRF_Allowlist') }
To actually enforce the SSRF allowlist (which is the stated goal of this PR), remove ignoreSsrfValidation: true and keep only allowList. If full bypass is intended instead, remove allowList to avoid the misleading appearance of protection.
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
Varun789-mx
force-pushed
the
fix/custom-oauth-ssrf-allowlist
branch
from
d23fa1a to
0f6e0cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/meteor/app/custom-oauth/server/custom_oauth_server.js`:
- Around line 290-293: Update the stale security comment above the fetch call
that uses this.emailPath so it accurately reflects that the SSRF allowlist is
enforced (the fetch includes allowList: settings.get('SSRF_Allowlist')),
matching the wording used by the other two security comments; locate the comment
immediately before the fetch(...) call in custom_oauth_server.js and replace
"It's ok to disable this check here." with a statement indicating the SSRF check
is being enforced (e.g., "SSRF allowlist enforced via allowList option.").
🪄 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: ef16a6e9-eba2-4de0-99f4-a02a4161b189
📒 Files selected for processing (1)
apps/meteor/app/custom-oauth/server/custom_oauth_server.js
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/custom-oauth/server/custom_oauth_server.js
🔇 Additional comments (2)
apps/meteor/app/custom-oauth/server/custom_oauth_server.js (2)
144-150: LGTM!
183-186: LGTM!
Varun789-mx
force-pushed
the
fix/custom-oauth-ssrf-allowlist
branch
from
17d9d7d to
0f97354
Compare
Varun789-mx
force-pushed
the
fix/custom-oauth-ssrf-allowlist
branch
from
f909239 to
86fbcaf
Compare
2 participants
Proposed changes (including videos or screenshots)
This fix ensures the
SSRF_Allowlistadmin setting is respected duringCustom OAuth token exchanges and identity fetches.
Previously, the
getAccessToken,getIdentity, andgetEmailFromPathmethods in
app/custom-oauth/server/custom_oauth_server.jscalledfetch()without passing theallowListoption, causing all CustomOAuth requests to bypass the SSRF allowlist configured by the admin.
This affected self-hosted deployments where the OAuth/OIDC provider
(e.g., Keycloak, WSO2, Authentik) is on a private network — a standard
pattern for homelab, on-premise, and air-gapped environments.
The fix passes
allowList: settings.get('SSRF_Allowlist')to all threefetch calls, consistent with how other internal fetch calls handle this
(e.g.,
app/apps/server/bridges/http.ts).Issue(s)
Closes #40586
Steps to test or reproduce
(e.g., Keycloak at
https://idp.local:9443)Admin > General > SSRF Protection > SSRF Allowlist
error-ssrf-validation-failedFurther comments
The fix also adds
ignoreSsrfValidation: trueto these fetch calls,consistent with the security rationale that these URLs can only be
configured by privileged admins. Reviewers should confirm this
aligns with the intended security posture for OAuth endpoints.
Summary by CodeRabbit