fix: respect SSRF_Allowlist in Custom OAuth token exchange#41333
fix: respect SSRF_Allowlist in Custom OAuth token exchange#41333prathamesh04 wants to merge 3 commits into
Conversation
…ring When switching AutoTranslate providers, the old callback was not being removed because callbacks.add() with an existing ID silently returns a removal function instead of replacing. This caused translations to continue using the old provider until server restart. Fixes RocketChat#41197
The Custom OAuth provider bypassed SSRF validation entirely (ignoreSsrfValidation: true) in getAccessToken, getIdentity, and getEmailFromPath. This prevented admins from using private-IP IdPs (e.g. Keycloak at 192.168.x.x) even when explicitly allowlisting them in Administration > Settings > General > SSRF Protection. All other server-side fetch calls in the codebase pass the SSRF_Allowlist setting as the allowList option. Apply the same pattern to Custom OAuth so SSRF protection is enforced while the allowlist is respected. Signed-off-by: Prathamesh Hukkeri <prathamesh04@users.noreply.github.com>
|
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 |
|
|
cc @tassoevan This fixes the SSRF bypass in Custom OAuth — all other fetch calls in the codebase already pass the allowlist. Would appreciate a review when you get a chance. |
|
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 with no reviewable changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
WalkthroughCustom OAuth token, identity, and email requests now apply SSRF validation with the configured allowlist. Auto-translate deregisters its message callback before checking whether translation is enabled. ChangesCustom OAuth SSRF protection
Auto-translate callback cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 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 187-188: Remove the newly added implementation comments near the
OAuth identity endpoint handling and the corresponding code at the second
referenced location, while leaving the SSRF validation behavior and its options
unchanged.
🪄 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: e1b1e521-6069-4c2b-89f5-479daf0da087
📒 Files selected for processing (2)
apps/meteor/app/autotranslate/server/autotranslate.tsapps/meteor/app/custom-oauth/server/custom_oauth_server.js
📜 Review details
⏰ Context from checks skipped due to timeout. (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/autotranslate/server/autotranslate.tsapps/meteor/app/custom-oauth/server/custom_oauth_server.js
🧠 Learnings (3)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/autotranslate/server/autotranslate.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/autotranslate/server/autotranslate.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/app/autotranslate/server/autotranslate.ts
🔇 Additional comments (3)
apps/meteor/app/autotranslate/server/autotranslate.ts (1)
105-106: LGTM!apps/meteor/app/custom-oauth/server/custom_oauth_server.js (2)
147-153: SSRF validation changes are correct and consistent across all three fetch calls.All three Custom OAuth endpoint requests now properly enable SSRF validation (
ignoreSsrfValidation: false) and pass the configured allowlist. The downstreamgetFetchAgentWithValidationinpackages/server-fetch/src/index.tsconfirms that whenignoreSsrfValidationisfalse, it callscheckForSsrfWithIp(url, allowList)and throws if the result is not allowed. This correctly addresses the issue objective of allowing private-IP identity providers when explicitly allowlisted.Also applies to: 187-195, 301-308
149-149: 🔒 Security & PrivacyNo action needed:
apps/meteor/server/settings/general.tsregistersSSRF_Allowlistas a string setting with''as the default, so unset values are not an unexpected object/null case.> Likely an incorrect or invalid review comment.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
Removed the code comments per the coding guidelines (see inline review feedback). This PR is ready for review — @RocketChat/backend could you take a look? |
|
Hi @julio-rocketchat, could you please review this when you get a chance? Thanks! |
1 participant
Proposed changes
The Custom OAuth provider's
getAccessToken,getIdentity, andgetEmailFromPathmethods bypassed SSRF validation entirely (ignoreSsrfValidation: true). This meant theSSRF_Allowlistadmin setting was completely ignored during OAuth token exchanges, identity lookups, and email fetches.This makes it impossible to use Custom OAuth with an identity provider on a private IP (e.g.
192.168.x.x,10.x.x.x) in self-hosted/homelab deployments, even when the administrator has explicitly allowlisted the host in Administration > Settings > General > SSRF Protection > SSRF Allowlist.Fix: Change all three
fetch()calls fromignoreSsrfValidation: truetoignoreSsrfValidation: false, allowList: settings.get('SSRF_Allowlist')— the same pattern used by every other server-side fetch call in the codebase (avatar providers, OEmbed, Apps Engine HTTP bridge, marketplace, etc.).This preserves SSRF protection (private IPs are blocked by default) while allowing admins to explicitly permit their internal IdPs via the allowlist.
Types of changes
Checklist
Further comments
Issue: Closes #40586
Affected methods:
getAccessToken— token exchange POST to the OAuth providergetIdentity— GET user profile from the OAuth providergetEmailFromPath— GET user emails from the OAuth providerBefore: Admin adds
idp.localto SSRF Allowlist → Custom OAuth login still fails witherror-ssrf-validation-failedAfter: SSRF validation runs and respects the allowlist → private-IP IdPs work when explicitly permitted
Summary by CodeRabbit
Bug Fixes
Security Enhancements