🛡️ Sentinel: Secure OAuth2 redirect postMessage and origin verification#21
Conversation
…ification - Replace wildcard `'*'` with specific target origin in `/redirect` endpoint (backend) and `RedirectPage` (frontend) to prevent authorization code interception. - Implement strict equality for `event.origin` verification in `oauth2-utils.ts` to prevent origin bypass. - Optimize origin calculation in `oauth2-utils.ts` by pre-calculating it outside the event listener. - Use `platformUtils` and `domainHelper` to resolve platform-specific frontend origins 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. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughUpdated OAuth2 redirect flow to use specific target origins instead of wildcard ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
…tion - Replace wildcard '*' with specific target origin in /redirect endpoint (backend) and RedirectPage (frontend) to prevent authorization code interception. - Implement strict equality for event.origin verification in oauth2-utils.ts to prevent origin bypass. - Optimize origin calculation in oauth2-utils.ts by pre-calculating it outside the event listener. - Use platformUtils and domainHelper to resolve platform-specific frontend origins on the server. Co-authored-by: AGI-Corporation <186229839+AGI-Corporation@users.noreply.github.com>
![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: 090989581d
ℹ️ 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.
This computes targetOrigin from the /redirect request context, which breaks OAuth popups when the callback URL is served from INTERNAL_URL but the opener is on a tenant/custom frontend domain. In that configuration, federatedAuthnService.getThirdPartyRedirectUrl returns INTERNAL_URL/redirect and domainHelper.getInternalUrl ignores platformId, so getPlatformIdForRequest on this route cannot recover the tenant and getPublicUrl({ platformId: null }) falls back to the global FRONTEND_URL; the subsequent postMessage is sent to the wrong origin and the code is never delivered to the opener. This regression only appears in deployments with differing internal/public domains, but it causes OAuth login/connection flows to hang for affected 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: 1
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-94:⚠️ Potential issue | 🟡 MinorPromise hangs indefinitely if
redirectUrlis malformed.If
new URL(redirectUrl)throws (malformed URL),expectedOriginstaysnulland the condition at lines 85-86 never passes. The returned Promise will never resolve, potentially leaving the UI in a perpetual loading state.Consider rejecting the Promise early when
expectedOrigincannot be parsed:Suggested fix
function getCode(redirectUrl: string): Promise<string> { let expectedOrigin: string | null = null; try { expectedOrigin = new URL(redirectUrl).origin; } catch (e) { - // ignore + return Promise.reject(new Error(`Invalid redirectUrl: ${redirectUrl}`)); } return new Promise<string>((resolve) => {🤖 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 - 94, getCode currently leaves expectedOrigin null when redirectUrl is malformed, causing the returned Promise to hang; update getCode to detect a failed URL parse (expectedOrigin === null) and immediately return a rejected Promise (or throw) with a clear error message before attaching the window 'message' listener; keep references to getCode and currentPopup in your change and ensure you don't add the listener if parsing fails so there are no dangling handlers.
🤖 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 254-262: When platformUtils.getPlatformIdForRequest(request)
returns null, log a warning (including request.hostname and
request.headers.origin) and validate that domainHelper.getPublicUrl({ platformId
})'s origin (targetOrigin) matches the actual request origin; if they differ,
use the request origin as a safer fallback for targetOrigin or return an error
reply indicating mismatched origins. Update the handler around
platformId/targetOrigin (the block computing frontendUrl/targetOrigin and
sending reply) to emit the warning when platformId is null and to choose
request.headers.origin as the postMessage target if it produces a different
origin than new URL(frontendUrl).origin.
---
Outside diff comments:
In `@packages/react-ui/src/lib/oauth2-utils.ts`:
- Around line 74-94: getCode currently leaves expectedOrigin null when
redirectUrl is malformed, causing the returned Promise to hang; update getCode
to detect a failed URL parse (expectedOrigin === null) and immediately return a
rejected Promise (or throw) with a clear error message before attaching the
window 'message' listener; keep references to getCode and currentPopup in your
change and ensure you don't add the listener if parsing fails so there are no
dangling handlers.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 696f53a3-cf2a-4bf8-8378-25a94fdee514
📒 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 platformId = await platformUtils.getPlatformIdForRequest(request) | ||
| const frontendUrl = await domainHelper.getPublicUrl({ platformId }) | ||
| const targetOrigin = 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>`, | ||
| )}' }, '${targetOrigin}')}</script> <html>Redirect succuesfully, this window should close now</html>`, |
There was a problem hiding this comment.
Silent failure when platform cannot be determined for custom domain requests.
When getPlatformIdForRequest returns null (e.g., CLOUD edition with unknown hostname per platform.utils.ts:19), domainHelper.getPublicUrl falls back to the default FRONTEND_URL. If the actual opener is on a custom domain, the computed targetOrigin won't match, and postMessage will silently fail to deliver—the OAuth flow hangs without error feedback.
Consider logging a warning when platformId is null to aid debugging, or validating that the computed origin aligns with the request's origin.
Suggested improvement
const platformId = await platformUtils.getPlatformIdForRequest(request)
+if (platformId === null) {
+ request.log.warn({ host: request.headers.host }, 'Could not determine platform for OAuth redirect, falling back to default frontend URL')
+}
const frontendUrl = await domainHelper.getPublicUrl({ platformId })
const targetOrigin = new URL(frontendUrl).origin📝 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.
| const platformId = await platformUtils.getPlatformIdForRequest(request) | |
| const frontendUrl = await domainHelper.getPublicUrl({ platformId }) | |
| const targetOrigin = 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>`, | |
| )}' }, '${targetOrigin}')}</script> <html>Redirect succuesfully, this window should close now</html>`, | |
| const platformId = await platformUtils.getPlatformIdForRequest(request) | |
| if (platformId === null) { | |
| request.log.warn({ host: request.headers.host }, 'Could not determine platform for OAuth redirect, falling back to default frontend URL') | |
| } | |
| const frontendUrl = await domainHelper.getPublicUrl({ platformId }) | |
| const targetOrigin = new URL(frontendUrl).origin | |
| return reply | |
| .type('text/html') | |
| .send( | |
| `<script>if(window.opener){window.opener.postMessage({ 'code': '${encodeURIComponent( | |
| params.code, | |
| )}' }, '${targetOrigin}')}</script> <html>Redirect succuesfully, 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 254 - 262, When
platformUtils.getPlatformIdForRequest(request) returns null, log a warning
(including request.hostname and request.headers.origin) and validate that
domainHelper.getPublicUrl({ platformId })'s origin (targetOrigin) matches the
actual request origin; if they differ, use the request origin as a safer
fallback for targetOrigin or return an error reply indicating mismatched
origins. Update the handler around platformId/targetOrigin (the block computing
frontendUrl/targetOrigin and sending reply) to emit the warning when platformId
is null and to choose request.headers.origin as the postMessage target if it
produces a different origin than new URL(frontendUrl).origin.
…tion - Replace wildcard '*' with specific target origin in /redirect endpoint (backend) and RedirectPage (frontend) to prevent authorization code interception. - Implement strict equality for event.origin verification in oauth2-utils.ts to prevent origin bypass. - Optimize origin calculation in oauth2-utils.ts by pre-calculating it outside the event listener. - Use platformUtils and domainHelper to resolve platform-specific frontend origins on the server. fixes #1 Co-authored-by: AGI-Corporation <186229839+AGI-Corporation@users.noreply.github.com>
- Replace wildcard '*' with specific target origin in /redirect endpoint (backend) and RedirectPage (frontend) to prevent authorization code interception. - Implement strict equality for event.origin verification in oauth2-utils.ts to prevent origin bypass. - Optimize origin calculation in oauth2-utils.ts by pre-calculating it outside the event listener. - Use platformUtils and domainHelper to resolve platform-specific frontend origins on the server. fixes #1 Co-authored-by: AGI-Corporation <186229839+AGI-Corporation@users.noreply.github.com>
1 participant
This PR enhances the security of the OAuth2 redirect flow by securing
window.postMessagecalls used to transmit authorization codes from the redirect popup back to the main application.Previously, both the server-side redirect endpoint and the frontend redirect page used a wildcard (
'*') as thetargetOriginforpostMessage. This could allow a malicious origin that successfully positions itself as the window's opener to intercept sensitive OAuth2 codes. Additionally, the frontend verification usedstartsWith, which is less secure than a strict equality check.Changes:
packages/server/api/src/app/app.ts): Resolved the platform-specific frontend origin usingplatformUtils.getPlatformIdForRequestanddomainHelper.getPublicUrl, and used it as thetargetOriginforpostMessage.packages/react-ui/src/lib/oauth2-utils.ts): ReplacedstartsWithwith a strict equality check (event.origin === expectedOrigin) and optimized the origin parsing by moving it outside the message listener.packages/react-ui/src/app/routes/redirect.tsx): Updated thepostMessagecall to usewindow.location.originas thetargetOrigininstead of a wildcard.These changes follow security best practices for Cross-Document Messaging as defined by OWASP.
PR created automatically by Jules for task 398388136488173243 started by @AGI-Corporation
Summary by CodeRabbit