🛡️ Sentinel: Fix insecure postMessage target origins in OAuth2 flow#12
🛡️ Sentinel: Fix insecure postMessage target origins in OAuth2 flow#12AGI-Corporation wants to merge 2 commits intomainfrom
Conversation
- Restricted postMessage target origin in redirect.tsx and server-side /redirect endpoint to prevent leaking authorization codes. - Tightened origin verification in oauth2-utils.ts to use strict equality, preventing potential origin bypasses. - Use domainHelper to dynamically resolve the trusted frontend origin on the server. Co-authored-by: AGI-Corporation <186229839+AGI-Corporation@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughTightened OAuth2 postMessage usage and origin checks across client and server: wildcard targets replaced with explicit origins and event.origin comparisons changed from startsWith to strict equality to restrict message recipients to the exact expected origin. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd71aab47a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const frontendUrl = await domainHelper.getPublicUrl({ platformId: null }) | ||
| const frontendOrigin = new URL(frontendUrl).origin |
There was a problem hiding this comment.
Using domainHelper.getPublicUrl({ platformId: null }) hard-codes the postMessage target to the global FRONTEND_URL, which breaks tenant custom-domain flows. In enterprise/cloud, the redirect URL is generated per platform (see enterprise-flags.hooks.ts), so /redirect can run on tenant.example.com while this script targets the default origin; when targetOrigin does not match the opener’s origin, browsers drop the message and OAuth login/connection popups never deliver the code for those tenants.
Useful? React with 👍 / 👎.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server/api/src/app/app.ts`:
- Around line 260-261: Fix the typo in the HTML response string that currently
reads "Redirect succuesfully, this window should close now" to "Redirect
successfully, this window should close now" where that string is built (near the
template using frontendOrigin and the injected script) so the user-facing
message returned by the response in app.ts is spelled correctly.
- Around line 253-260: Replace the hardcoded null platformId passed to
domainHelper.getPublicUrl by extracting platformId from the request hostname
using getPlatformIdForHostname(request.headers.host) and pass that value
(falling back to default if it returns falsy) into getPublicUrl; update the
postMessage HTML string to fix the "succuesfully" typo to "successfully" and
keep using encodeURIComponent(params.code) and frontendOrigin as before so the
OAuth code is posted to the correct tenant origin.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a4b6f8b-4761-468a-8bee-8f02a7532eb6
📒 Files selected for processing (4)
.jules/sentinel.mdpackages/react-ui/src/app/routes/redirect.tsxpackages/react-ui/src/lib/oauth2-utils.tspackages/server/api/src/app/app.ts
| const frontendUrl = await domainHelper.getPublicUrl({ platformId: null }) | ||
| const frontendOrigin = new URL(frontendUrl).origin | ||
| return reply | ||
| .type('text/html') | ||
| .send( | ||
| `<script>if(window.opener){window.opener.postMessage({ 'code': '${encodeURIComponent( | ||
| params.code, | ||
| )}' },'*')}</script> <html>Redirect succuesfully, this window should close now</html>`, | ||
| )}' }, '${frontendOrigin}')}</script> <html>Redirect succuesfully, this window should close now</html>`, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how domainHelper.getPublicUrl handles platformId and multi-tenant scenarios
# Find the implementation of getPublicUrl
ast-grep --pattern 'getPublicUrl($$$) {
$$$
}'
# Check how other redirect endpoints handle platformId
rg -n "getPublicUrl|getInternalUrl" --type ts -C 3 | head -80
# See if there are any existing multi-platform redirect handling patterns
rg -n "platformId.*redirect|redirect.*platformId" --type ts -C 2Repository: AGI-Corporation/Route.X
Length of output: 8543
🏁 Script executed:
# Get the full context of the endpoint where platformId: null is used
sed -n '240,270p' packages/server/api/src/app/app.ts
# Search for the endpoint definition and see if there's any state parameter handling
rg -n "platformId.*null|params\.code|oauth.*redirect" packages/server/api/src/app/app.ts -B 5 -A 2Repository: AGI-Corporation/Route.X
Length of output: 2018
🏁 Script executed:
# Check if OAuth implementations use state parameter to track platform context
rg -n "state.*platform|platform.*state|oauth.*state" packages/server/api/src/app/ee/authentication --type ts -i | head -40
# Search for OAuth redirect implementations and see if any track platform
rg -n "getThirdPartyRedirectUrl|oauthRedirectUrl|redirect_uri" packages/server/api/src/app/ee/authentication --type ts -C 3 | head -100
# Check if there's any platform tracking mechanism in OAuth flows
rg -n "window.opener.postMessage|state.*=|platformId" packages/server/api/src/app --type ts -B 2 -A 2Repository: AGI-Corporation/Route.X
Length of output: 50380
🏁 Script executed:
# Search for how OAuth flows pass context through the OAuth flow
rg -n "state|oauth.*code|code.*oauth" packages/server/api/src/app/ee/authentication --type ts -C 2 | grep -E "(state|oauth)" | head -50
# Check the frontend to see how it handles the postMessage
fd "redirect" packages/server -t ts | head -5
# Search for comments or patterns around platformId: null
rg -n "platformId.*null" packages/server/api/src/app/app.ts -B 5 -A 5
# Check if there's a state parameter or similar mechanism in OAuth flow
rg -n "searchParams|query|state" packages/server/api/src/app/ee/authentication/federated-authn --type ts | head -30Repository: AGI-Corporation/Route.X
Length of output: 1927
🏁 Script executed:
# Check Google OAuth implementation for state parameter
sed -n '1,100p' packages/server/api/src/app/ee/authentication/federated-authn/google-authn-provider.ts
# Check if state is used anywhere in federated auth
rg -n "state" packages/server/api/src/app/ee/authentication/federated-authn/ --type ts -C 2
# Look at the entire app.ts redirect endpoint context
sed -n '240,270p' packages/server/api/src/app/app.tsRepository: AGI-Corporation/Route.X
Length of output: 4295
🏁 Script executed:
# Get the full implementation of getThirdPartyRedirectUrl
sed -n '50,60p' packages/server/api/src/app/ee/authentication/federated-authn/federated-authn-service.ts
# Check if there are other redirect endpoints that handle platformId
rg -n "app.get.*redirect|app.post.*redirect" packages/server/api/src/app -C 2 | head -50Repository: AGI-Corporation/Route.X
Length of output: 296
🏁 Script executed:
# Check if there's a way to extract platformId from request hostname
rg -n "request.hostname|request.host|req.headers.host" packages/server/api/src/app -C 2 | head -40
# Check if customDomainService can lookup platformId by hostname
sed -n '1,50p' packages/server/api/src/app/ee/custom-domains/custom-domain.service.tsRepository: AGI-Corporation/Route.X
Length of output: 2286
Extract platformId from request hostname instead of hardcoding null.
The endpoint receives the OAuth redirect at a platform-specific URL (built by getThirdPartyRedirectUrl(platformId)), but then discards that platform context by using platformId: null. This causes the postMessage to target the default platform's origin instead of the correct tenant's origin in multi-platform deployments with custom domains.
Use getPlatformIdForHostname() (available in platform.utils) to extract the platformId from request.headers.host, matching the pattern already used elsewhere in the codebase. If platformId cannot be determined from the hostname, fall back to the default.
Minor: Fix typo "succuesfully" → "successfully".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/server/api/src/app/app.ts` around lines 253 - 260, Replace the
hardcoded null platformId passed to domainHelper.getPublicUrl by extracting
platformId from the request hostname using
getPlatformIdForHostname(request.headers.host) and pass that value (falling back
to default if it returns falsy) into getPublicUrl; update the postMessage HTML
string to fix the "succuesfully" typo to "successfully" and keep using
encodeURIComponent(params.code) and frontendOrigin as before so the OAuth code
is posted to the correct tenant origin.
| )}' }, '${frontendOrigin}')}</script> <html>Redirect succuesfully, this window should close now</html>`, | ||
| ) |
There was a problem hiding this comment.
Typo: "succuesfully" → "successfully".
Minor typo in the HTML response message.
📝 Proposed fix
)}' }, '${frontendOrigin}')}</script> <html>Redirect succuesfully, this window should close now</html>`,
+ )}' }, '${frontendOrigin}')}</script> <html>Redirect successfully, this window should close now</html>`,📝 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.
| )}' }, '${frontendOrigin}')}</script> <html>Redirect succuesfully, this window should close now</html>`, | |
| ) | |
| )}' }, '${frontendOrigin}')}</script> <html>Redirect successfully, this window should close now</html>`, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/server/api/src/app/app.ts` around lines 260 - 261, Fix the typo in
the HTML response string that currently reads "Redirect succuesfully, this
window should close now" to "Redirect successfully, this window should close
now" where that string is built (near the template using frontendOrigin and the
injected script) so the user-facing message returned by the response in app.ts
is spelled correctly.
- Restricted postMessage target origin in redirect.tsx and server-side /redirect endpoint to prevent leaking authorization codes. - Tightened origin verification in oauth2-utils.ts to use strict equality, preventing potential origin bypasses. - Use domainHelper to dynamically resolve the trusted frontend origin on the server and ensure only the origin component is used. Co-authored-by: AGI-Corporation <186229839+AGI-Corporation@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.jules/sentinel.md (1)
4-4: Document the strict origin validation requirement.The prevention guidance focuses on deriving the correct origin when sending messages but doesn't explicitly mention the receiving side fix. According to the PR description, a key part of this security fix was changing the message listener validation from
startsWithto strict equality (===) when checkingevent.origin. This is critical for preventing subdomain-based bypasses.Consider adding guidance like: "On the receiving side, validate
event.originusing strict equality (===) against the expected origin, not prefix matching, to prevent subdomain or similar-origin attacks."📝 Suggested enhancement
-**Prevention:** Use `window.location.origin` on the client and `domainHelper.getPublicUrl` on the server to resolve the trusted frontend origin. Always extract only the origin (scheme + host + port) using `new URL(url).origin` to ensure strict matching by the browser. +**Prevention:** Use `window.location.origin` on the client and `domainHelper.getPublicUrl` on the server to resolve the trusted frontend origin. Always extract only the origin (scheme + host + port) using `new URL(url).origin` to ensure strict matching by the browser. When receiving messages, validate `event.origin` using strict equality (`===`) against the expected origin, not prefix matching like `startsWith`, to prevent subdomain or similar-origin attacks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.jules/sentinel.md at line 4, Update the prevention guidance to explicitly require strict equality when validating origins on the receiving side: document that message listeners should check event.origin using strict equality (event.origin === expectedOrigin) rather than prefix matching (startsWith), and reference the same origin-resolving helpers (window.location.origin on the client, domainHelper.getPublicUrl on the server) and the requirement to extract origins with new URL(url).origin so implementers know to compare the exact origin in their message handler (e.g., the 'message' event listener or any handleMessageEvent/onMessage function).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.jules/sentinel.md:
- Line 1: Update the documentation header line "## 2025-05-15 - [Secure
postMessage origins in OAuth2 flow]" to the correct, current documentation/fix
date (e.g., 2026-03-08 or the actual fix date) so the timeline matches the PR;
locate that header in .jules/sentinel.md and replace the old date string with
the accurate date.
---
Nitpick comments:
In @.jules/sentinel.md:
- Line 4: Update the prevention guidance to explicitly require strict equality
when validating origins on the receiving side: document that message listeners
should check event.origin using strict equality (event.origin ===
expectedOrigin) rather than prefix matching (startsWith), and reference the same
origin-resolving helpers (window.location.origin on the client,
domainHelper.getPublicUrl on the server) and the requirement to extract origins
with new URL(url).origin so implementers know to compare the exact origin in
their message handler (e.g., the 'message' event listener or any
handleMessageEvent/onMessage function).
| @@ -0,0 +1,4 @@ | |||
| ## 2025-05-15 - [Secure postMessage origins in OAuth2 flow] | |||
There was a problem hiding this comment.
Date inconsistency in the documentation header.
The date "2025-05-15" appears to predate the PR creation timestamp (2026-03-08). This could cause confusion when reviewing the security timeline. Consider using the actual date when the vulnerability was fixed or documented.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.jules/sentinel.md at line 1, Update the documentation header line "##
2025-05-15 - [Secure postMessage origins in OAuth2 flow]" to the correct,
current documentation/fix date (e.g., 2026-03-08 or the actual fix date) so the
timeline matches the PR; locate that header in .jules/sentinel.md and replace
the old date string with the accurate date.
1 participant
This PR addresses a security vulnerability where sensitive OAuth2 authorization codes were being broadcasted using
window.postMessagewith a wildcard (*) target origin.Changes:
*with the actual frontend origin in both the React frontend (redirect.tsx) and the Fastify backend (app.ts). The backend now usesdomainHelper.getPublicUrlto resolve the correct frontend origin dynamically.oauth2-utils.tsto use strict equality (===) for origin verification instead of a prefix match (startsWith), preventing potential bypasses from malicious subdomains or similar-looking origins.Impact:
This fix prevents malicious third-party websites from intercepting OAuth2 authorization codes if they manage to open or be opened by the Activepieces window during the authentication flow.
Verification:
domainHelperis the standard way to resolve the frontend URL in this monorepo.PR created automatically by Jules for task 3982164033771812107 started by @AGI-Corporation
Summary by CodeRabbit
Bug Fixes
Documentation