🛡️ Sentinel: [HIGH] Harden OAuth postMessage security#11
🛡️ Sentinel: [HIGH] Harden OAuth postMessage security#11AGI-Corporation wants to merge 4 commits intomainfrom
Conversation
- Restrict target origin for postMessage in OAuth redirect handlers on both server and client. - Replace insecure `startsWith` origin validation with strict equality check of the derived origin. - Add safety checks for URL parsing to prevent potential crashes. 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughHardened OAuth postMessage handling: replaced wildcard targets with explicit origin values, tightened origin validation using parsed origins, added guards for undefined redirect URLs, and ensured popups are closed and listeners removed after successful code retrieval. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Popup as OAuth Popup
participant Server as Auth Server
participant App as Parent App
User->>Popup: initiate auth
Popup->>Server: auth request / redirect
Server-->>Popup: redirect with code to redirectUrl
Popup->>App: window.opener.postMessage({code}, targetOrigin=<parsed origin>)
App->>App: onmessage handler receives event
App->>App: if event.origin === expectedOrigin (new URL(redirectUrl).origin)
App-->>Popup: (optional) close popup
App->>App: remove message listener
App->>Server: exchange code for token
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)
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: 1333df9ffb
ℹ️ 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".
| return reply.send('The code is missing in url') | ||
| } | ||
| else { | ||
| const origin = await domainHelper.getPublicUrl({}) |
There was a problem hiding this comment.
Using domainHelper.getPublicUrl({}) here hard-codes the default frontend URL as the postMessage target, which breaks OAuth callbacks opened from tenant/custom domains. In Enterprise, THIRD_PARTY_AUTH_PROVIDER_REDIRECT_URL is generated per platformId (via getThirdPartyRedirectUrl(platformId)), so the popup can run on a custom domain while this script posts only to the global frontend origin; browsers drop that message and the login flow never resolves.
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-95:⚠️ Potential issue | 🟠 MajorPromise never resolves if
redirectUrlis undefined or URL parsing fails.If
redirectUrlis undefined (lines 77-79) or URL parsing throws (lines 83-85), the handler returns early but the Promise created on line 75 never resolves or rejects. This leaves callers hanging indefinitely.Looking at actual call sites (
openWithLoginUrlandopenOAuth2Popup),redirectUrlis always typed as a requiredstring, so undefined should never occur in practice. However, the defensive guards should either:
- Reject the Promise to signal failure, or
- Keep the signature as
stringonly and handle the try/catch by rejecting🛠️ Proposed fix: Reject Promise on failure
-function getCode(redirectUrl: string | undefined): Promise<string> { - return new Promise<string>((resolve) => { - window.addEventListener('message', function handler(event) { - if (!redirectUrl) { - return; - } - let origin; - try { - origin = new URL(redirectUrl).origin; - } catch (e) { - return; - } +function getCode(redirectUrl: string): Promise<string> { + return new Promise<string>((resolve, reject) => { + let origin: string; + try { + origin = new URL(redirectUrl).origin; + } catch (e) { + reject(new Error('Invalid redirect URL')); + return; + } + window.addEventListener('message', function handler(event) { if ( origin === event.origin && event.data['code'] ) { resolve(decodeURIComponent(event.data.code)); currentPopup?.close(); window.removeEventListener('message', handler); } }); }); }This also moves URL parsing outside the event handler since it only needs to be computed once.
🤖 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 - 95, The getCode function currently can leave its Promise unresolved when redirectUrl is undefined or URL parsing fails; update getCode to require a string redirectUrl (or validate and immediately reject if undefined) and move new URL(redirectUrl) parsing outside the message handler so parsing happens once; if parsing throws or redirectUrl is falsy, immediately reject the Promise with a clear error and ensure any created popup (currentPopup) is closed and the message handler removed on both success and failure paths; reference getCode, currentPopup, openWithLoginUrl and openOAuth2Popup to check callsites/types so the signature and error handling remain consistent.
🤖 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-95: The getCode function currently can leave its Promise
unresolved when redirectUrl is undefined or URL parsing fails; update getCode to
require a string redirectUrl (or validate and immediately reject if undefined)
and move new URL(redirectUrl) parsing outside the message handler so parsing
happens once; if parsing throws or redirectUrl is falsy, immediately reject the
Promise with a clear error and ensure any created popup (currentPopup) is closed
and the message handler removed on both success and failure paths; reference
getCode, currentPopup, openWithLoginUrl and openOAuth2Popup to check
callsites/types so the signature and error handling remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 00df8a50-7491-44a0-9809-da7f785617ee
📒 Files selected for processing (3)
packages/react-ui/src/app/routes/redirect.tsxpackages/react-ui/src/lib/oauth2-utils.tspackages/server/api/src/app/app.ts
- Restrict target origin for postMessage in OAuth redirect handlers on both server and client. - Replace insecure `startsWith` origin validation with strict equality check of the derived origin. - Add safety checks for URL parsing to prevent potential crashes. Co-authored-by: AGI-Corporation <186229839+AGI-Corporation@users.noreply.github.com>
…y journal - Restrict target origin for postMessage in OAuth redirect handlers. - Replace insecure `startsWith` origin validation with strict equality. - Add safety checks for URL parsing. - Add sentinel security journal entry. Co-authored-by: AGI-Corporation <186229839+AGI-Corporation@users.noreply.github.com>
…y journal - Restrict target origin for postMessage in OAuth redirect handlers. - Replace insecure `startsWith` origin validation with strict equality. - Add safety checks for URL parsing. - Add sentinel security journal entry. Co-authored-by: AGI-Corporation <186229839+AGI-Corporation@users.noreply.github.com>
1 participant
🚨 Severity: HIGH
💡 Vulnerability: Insecure use of
postMessagewith wildcard origin ('*') and weak origin validation usingstartsWith.🎯 Impact: Sensitive OAuth codes could be intercepted by malicious origins if they are able to open the redirect window or if the origin validation is bypassed.
🔧 Fix:
postMessagetarget origins to trusted frontend URLs usingdomainHelper.getPublicUrlon the server andwindow.location.originon the client.origininstead ofstartsWith.✅ Verification: Targeted linting of modified files and manual code review. Logic ensures defense-in-depth for sensitive data transmission between windows.
PR created automatically by Jules for task 11497284544736004468 started by @AGI-Corporation
Summary by CodeRabbit
Bug Fixes
Documentation