🛡️ Sentinel: Secure OAuth2 postMessage target origin and origin validation#14
Conversation
…ation - Restricted backend and frontend OAuth redirect `postMessage` target origins to the trusted application origin instead of using the wildcard `'*'`. - Hardened frontend origin validation for `postMessage` events by replacing insecure `startsWith` matching with strict equality checks using resolved origins. - Resolved missing `platformUtils` import in `packages/server/api/src/app/app.ts`. - Cleaned up environment noise by removing the auto-generated `pnpm-lock.yaml`. Security Impact: Prevents interception of sensitive authorization codes by malicious sites and mitigates domain-shadowing attacks. 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. |
📝 WalkthroughWalkthroughThe changes implement stricter origin validation for postMessage communications across the authentication flow. Wildcard origin targeting ("*") is replaced with specific origin matching derived from the frontend URL, enhancing security by preventing unintended message delivery. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: d75d84fbe4
ℹ️ 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".
| code: code, | ||
| }, | ||
| '*', | ||
| window.location.origin, |
There was a problem hiding this comment.
Setting postMessage targetOrigin to window.location.origin means the popup can only message an opener on the same origin, but the cloud OAuth2 flow uses https://secrets.activepieces.com/redirect as the redirect URL (packages/react-ui/src/app/connections/oauth2-connection-settings.tsx lines 96-99) while the opener is the main app domain, so the browser drops the message and the OAuth popup never returns a code. Use the expected opener origin (or a configured trusted origin) instead of the popup's own origin.
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-89:⚠️ Potential issue | 🟠 MajorStrict origin validation is correct, but
new URL(redirectUrl)can throw before the guard.The security fix using strict equality (
===) instead ofstartsWithis correct and prevents domain-shadowing attacks. However,new URL(redirectUrl)on line 77 executes before theredirectUrl &&check on line 79. IfredirectUrlis empty, undefined, or malformed (see callers inoauth2-connection-settings.tsxandthird-party-logins.tsxwhere it comes from flags), this throws aTypeErrorand the promise never resolves, causing the OAuth flow to hang silently.🛡️ Proposed fix: validate and parse URL before adding listener or inside guard
function getCode(redirectUrl: string): Promise<string> { + let expectedOrigin: string; + try { + expectedOrigin = new URL(redirectUrl).origin; + } catch { + return Promise.reject(new Error('Invalid redirectUrl provided')); + } return new Promise<string>((resolve) => { window.addEventListener('message', function handler(event) { - const expectedOrigin = new URL(redirectUrl).origin; if ( - redirectUrl && event.origin === expectedOrigin && event.data['code'] ) { resolve(decodeURIComponent(event.data.code)); currentPopup?.close(); window.removeEventListener('message', handler); } }); }); }🤖 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 - 89, In getCode, new URL(redirectUrl) is called before checking redirectUrl and can throw, leaving the promise unresolved; validate and parse redirectUrl before adding the message listener: first check redirectUrl is truthy, attempt to construct new URL(redirectUrl) in a try/catch to obtain expectedOrigin (or reject the Promise immediately on failure), only then add window.addEventListener; alternatively parse expectedOrigin inside the handler with a try/catch and if parsing fails reject/cleanup (close currentPopup and remove the handler) so the Promise never hangs; refer to getCode, redirectUrl, currentPopup, and the message handler to locate places to change.
🤖 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-89: In getCode, new URL(redirectUrl) is called before checking
redirectUrl and can throw, leaving the promise unresolved; validate and parse
redirectUrl before adding the message listener: first check redirectUrl is
truthy, attempt to construct new URL(redirectUrl) in a try/catch to obtain
expectedOrigin (or reject the Promise immediately on failure), only then add
window.addEventListener; alternatively parse expectedOrigin inside the handler
with a try/catch and if parsing fails reject/cleanup (close currentPopup and
remove the handler) so the Promise never hangs; refer to getCode, redirectUrl,
currentPopup, and the message handler to locate places to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0f3aea06-c163-4842-9041-dd737d35ddb9
📒 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
…ation - Restricted backend and frontend OAuth redirect `postMessage` target origins to the trusted application origin instead of using the wildcard `'*'`. - Hardened frontend origin validation for `postMessage` events by replacing insecure `startsWith` matching with strict equality checks using resolved origins. - Resolved missing `platformUtils` import in `packages/server/api/src/app/app.ts`. - Cleaned up environment noise by removing the auto-generated `pnpm-lock.yaml`. Security Impact: Prevents interception of sensitive authorization codes by malicious sites and mitigates domain-shadowing attacks. Co-authored-by: AGI-Corporation <186229839+AGI-Corporation@users.noreply.github.com>
…ation - Restricted backend and frontend OAuth redirect `postMessage` target origins to the trusted application origin instead of using the wildcard `'*'`. - Hardened frontend origin validation for `postMessage` events by replacing insecure `startsWith` matching with strict equality checks using resolved origins. - Resolved missing `platformUtils` import in `packages/server/api/src/app/app.ts`. - Cleaned up environment noise by removing the auto-generated `pnpm-lock.yaml`. Security Impact: Prevents interception of sensitive authorization codes by malicious sites and mitigates domain-shadowing attacks. Co-authored-by: AGI-Corporation <186229839+AGI-Corporation@users.noreply.github.com>
…ation - Restricted backend and frontend OAuth redirect `postMessage` target origins to the trusted application origin instead of using the wildcard `'*'`. - Hardened frontend origin validation for `postMessage` events by replacing insecure `startsWith` matching with strict equality checks using resolved origins. - Resolved missing `platformUtils` import in `packages/server/api/src/app/app.ts`. - Cleaned up environment noise by removing the auto-generated `pnpm-lock.yaml`. Security Impact: Prevents interception of sensitive authorization codes by malicious sites and mitigates domain-shadowing attacks. Co-authored-by: AGI-Corporation <186229839+AGI-Corporation@users.noreply.github.com>
1 participant
🛡️ Sentinel Security Fix: OAuth2 postMessage Hardening
🚨 Severity: HIGH
💡 Vulnerability: Insecure
postMessagetarget origins and origin validation./redirectroute and frontend redirect page were using'*'as thetargetOriginforpostMessage, which could allow a malicious site to intercept the OAuth2code..startsWith(event.origin)to validate incoming messages, which is vulnerable to domain-shadowing attacks (e.g.,trusted.com.malicious.compasses a check fortrusted.com).🎯 Impact: An attacker could potentially steal OAuth2 authorization codes, leading to account takeover or unauthorized access to third-party services.
🔧 Fix:
packages/server/api/src/app/app.tsto resolve the trusted frontend origin usingplatformUtils.getPlatformIdForRequestanddomainHelper.getPublicUrl, then used it as thetargetOriginforpostMessage.packages/react-ui/src/app/routes/redirect.tsxto usewindow.location.originas thetargetOrigin.packages/react-ui/src/lib/oauth2-utils.tsto use strict equality (event.origin === expectedOrigin) for origin validation.✅ Verification:
read_fileto ensure correct logic and imports.npx eslintto confirm no syntax or basic type errors were introduced.pnpm-lock.yamlfile was removed to keep the PR focused on security changes.PR created automatically by Jules for task 14274471365459152318 started by @AGI-Corporation
Summary by CodeRabbit