🛡️ Sentinel: Secure OAuth code transmission and fix origin verification#13
🛡️ Sentinel: Secure OAuth code transmission and fix origin verification#13AGI-Corporation wants to merge 2 commits intomainfrom
Conversation
…ification - Replace insecure '*' wildcard in postMessage target origin with resolved platform origin in /api/redirect. - Upgrade frontend origin validation from startsWith to strict equality in oauth2-utils. - Optimize origin check by moving calculation outside the listener. - Correct typo in redirect success message. - Add security learning to Sentinel journal. 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. |
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request addresses a security vulnerability in OAuth code transmission via postMessage. The changes implement server-side dynamic resolution of the frontend origin and strengthen client-side origin verification from loose pattern matching to strict equality checks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: c531dbe6ad
ℹ️ 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 platformId = await platformUtils.getPlatformIdForRequest(request) | ||
| const frontendUrl = await domainHelper.getPublicUrl({ platformId }) | ||
| const targetOrigin = new URL(frontendUrl).origin |
There was a problem hiding this comment.
targetOrigin is computed from platformUtils.getPlatformIdForRequest(request) and domainHelper.getPublicUrl(...), but the OAuth redirect URL is issued via federatedAuthnService.getThirdPartyRedirectUrl using domainHelper.getInternalUrl (which prefers INTERNAL_URL). When INTERNAL_URL is set, /redirect arrives on a shared host without tenant context, so getPlatformIdForRequest can return null and getPublicUrl falls back to the global frontend origin; for custom-domain tenants this origin differs from the popup opener, so window.opener.postMessage is discarded and the OAuth code never reaches the UI.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-ui/src/lib/oauth2-utils.ts (1)
74-88:⚠️ Potential issue | 🔴 CriticalOrigin mismatch breaks CLOUD_OAUTH2 flows silently.
The
/redirectendpoint inapp.ts(lines 240-262) computestargetOriginfromdomainHelper.getPublicUrl({ platformId }), which resolves to either a custom domain or the configuredFRONTEND_URL. It then sends apostMessagecontaining the OAuth code to this computed origin.However, for
CLOUD_OAUTH2connections, the frontend hardcodesredirectUrlto'https://secrets.activepieces.com/redirect'(oauth2-connection-settings.tsx, line 98). This causesgetCode()to listen forpostMessagefrom'https://secrets.activepieces.com'.When the OAuth provider redirects to the server's
/redirectendpoint, the server posts the code to the actual frontend domain, notsecrets.activepieces.com. The browser rejects thispostMessagebecause the origin does not match the listener's expectation, causing the OAuth flow to hang indefinitely without error feedback.This affects all OAuth flow types, as they all call
openPopup()which invokesgetCode()regardless of connection type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-ui/src/lib/oauth2-utils.ts` around lines 74 - 88, getCode currently only accepts postMessage events whose origin equals expectedOrigin, which breaks CLOUD_OAUTH2 when the server posts to a different frontend domain; update getCode to still validate event.data but not rely solely on event.origin: inside the message handler (function handler in getCode) keep the existing expectedOrigin check but also accept messages where event.data.code exists and either event.data.redirectUrl === redirectUrl or event.data.source === 'oauth-redirect' (or another agreed-upon marker), then resolve(decodeURIComponent(event.data.code)), close currentPopup, and remove the listener; this preserves security by validating message payload (code + redirectUrl/source) while fixing cross-origin redirects for openPopup flows.
🧹 Nitpick comments (2)
packages/server/api/src/app/app.ts (1)
258-261: Consider escaping interpolated values in the HTML template.While
targetOrigincomes from trusted sources (custom domain orFRONTEND_URLenv var), interpolating it directly into a<script>tag without escaping is a defense-in-depth concern. If a malicious value were ever stored as a custom domain, it could potentially break out of the string context.Similarly,
params.codeis URL-encoded which helps, but the single quotes around the template string could still be vulnerable to certain payloads.🛡️ Optional: Use JSON.stringify for safer interpolation
return reply .type('text/html') .send( - `<script>if(window.opener){window.opener.postMessage({ 'code': '${encodeURIComponent( - params.code, - )}' }, '${targetOrigin}')}</script> <html>Redirect successfully, this window should close now</html>`, + `<script>if(window.opener){window.opener.postMessage({ 'code': ${JSON.stringify(encodeURIComponent( + params.code, + ))} }, ${JSON.stringify(targetOrigin)})}</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 258 - 261, The HTML response embeds unescaped values into a script string (the send(...) that writes window.opener.postMessage with targetOrigin and encodeURIComponent(params.code)); to harden this, wrap the interpolated values using a safe serializer like JSON.stringify before inserting them into the inline script (e.g., serialize targetOrigin and the code value passed to postMessage) so the string context cannot be broken out of; update the send(...) call to use the JSON-stringified versions of targetOrigin and params.code (or an equivalent escape function) and ensure you still URL-encode params.code where needed.packages/react-ui/src/lib/oauth2-utils.ts (1)
78-81: Minor: RedundantredirectUrltruthiness check.The check
redirectUrl &&on line 79 is unnecessary sinceredirectUrlwas already used to constructexpectedOriginon line 76. IfredirectUrlwere falsy,new URL(redirectUrl)would have thrown an error before reaching this point.♻️ Suggested simplification
window.addEventListener('message', function handler(event) { if ( - redirectUrl && event.origin === expectedOrigin && event.data['code'] ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-ui/src/lib/oauth2-utils.ts` around lines 78 - 81, Remove the redundant truthiness check for redirectUrl in the conditional that inspects the postMessage event: when evaluating if (redirectUrl && event.origin === expectedOrigin && event.data['code']) drop the leading "redirectUrl &&" since expectedOrigin is already derived from new URL(redirectUrl) earlier; update the conditional to only check event.origin === expectedOrigin and event.data['code'] (preserving the same variable names event, expectedOrigin, and event.data['code']) so behavior remains identical but the check is simplified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/react-ui/src/lib/oauth2-utils.ts`:
- Around line 74-88: getCode currently only accepts postMessage events whose
origin equals expectedOrigin, which breaks CLOUD_OAUTH2 when the server posts to
a different frontend domain; update getCode to still validate event.data but not
rely solely on event.origin: inside the message handler (function handler in
getCode) keep the existing expectedOrigin check but also accept messages where
event.data.code exists and either event.data.redirectUrl === redirectUrl or
event.data.source === 'oauth-redirect' (or another agreed-upon marker), then
resolve(decodeURIComponent(event.data.code)), close currentPopup, and remove the
listener; this preserves security by validating message payload (code +
redirectUrl/source) while fixing cross-origin redirects for openPopup flows.
---
Nitpick comments:
In `@packages/react-ui/src/lib/oauth2-utils.ts`:
- Around line 78-81: Remove the redundant truthiness check for redirectUrl in
the conditional that inspects the postMessage event: when evaluating if
(redirectUrl && event.origin === expectedOrigin && event.data['code']) drop the
leading "redirectUrl &&" since expectedOrigin is already derived from new
URL(redirectUrl) earlier; update the conditional to only check event.origin ===
expectedOrigin and event.data['code'] (preserving the same variable names event,
expectedOrigin, and event.data['code']) so behavior remains identical but the
check is simplified.
In `@packages/server/api/src/app/app.ts`:
- Around line 258-261: The HTML response embeds unescaped values into a script
string (the send(...) that writes window.opener.postMessage with targetOrigin
and encodeURIComponent(params.code)); to harden this, wrap the interpolated
values using a safe serializer like JSON.stringify before inserting them into
the inline script (e.g., serialize targetOrigin and the code value passed to
postMessage) so the string context cannot be broken out of; update the send(...)
call to use the JSON-stringified versions of targetOrigin and params.code (or an
equivalent escape function) and ensure you still URL-encode params.code where
needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da8e12cb-cf36-4f5c-b885-6068bb40e641
📒 Files selected for processing (3)
.jules/sentinel.mdpackages/react-ui/src/lib/oauth2-utils.tspackages/server/api/src/app/app.ts
…ification - Replace insecure '*' wildcard in postMessage target origin with resolved platform origin in /api/redirect. - Upgrade frontend origin validation from startsWith to strict equality in oauth2-utils. - Optimize origin check by moving calculation outside the listener. - Correct typo in redirect success message. - Add security learning to Sentinel journal. Co-authored-by: AGI-Corporation <186229839+AGI-Corporation@users.noreply.github.com>
1 participant
🛡️ Sentinel Security Fix
🚨 Severity: HIGH
💡 Vulnerability: Insecure OAuth code transmission via
postMessage.🎯 Impact: An attacker could intercept sensitive OAuth codes if they opened the redirect URL in a popup, potentially leading to account takeover. The frontend also used an insecure
startsWithcheck for origin verification, which is susceptible to bypass.🔧 Fix:
targetOriginforpostMessage.===) for origin verification.✅ Verification: Targeted linting with ESLint passed for modified files. Verification of origin resolution logic via existing
platformUtilsanddomainHelper.PR created automatically by Jules for task 1346839949802919774 started by @AGI-Corporation
Summary by CodeRabbit
Documentation
Bug Fixes