Uh oh!
There was an error while loading. Please reload this page.
fix(web): prevent XSS via OAuth redirect URI scheme injection - #1136
Merged
Conversation
Block javascript:, data:, and vbscript: URI schemes across the OAuth redirect flow to resolve CodeQL js/xss-through-exception alert. Adds defense-in-depth validation at four layers: client registration, server-side callback resolution, client-side consent screen, and the /oauth/complete handoff page. Fixes SOU-928 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Contributor
WalkthroughA security remediation that blocks dangerous URI schemes (javascript:, data:, vbscript:) across the OAuth lifecycle. Validation is added at the registration, authorization, and completion stages to prevent XSS attacks through OAuth redirect URIs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
🚥 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 docstrings
🧪 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 |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Contributor
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/web/src/app/oauth/authorize/components/consentScreen.tsx (1)
69-72:⚠️ Potential issue | 🟡 MinorShow an error when deny fails.
Line 70 now can receive a
ServiceErrorfrom the new server-side scheme guard, but the deny path silently returns. Mirror the approve path so users are not left with no feedback.💬 Proposed fix
const result = await denyAuthorization({ redirectUri, state }); if (isServiceError(result)) { + toast({+ description: `❌ Failed to deny authorization. ${result.message}`,+ }); setPending(null); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/app/oauth/authorize/components/consentScreen.tsx` around lines 69 - 72, The denyAuthorization call can return a ServiceError but currently the code just clears pending and returns silently; update the deny path (where denyAuthorization, isServiceError, and setPending are used) to mirror the approve flow by setting appropriate UI feedback when a ServiceError occurs (e.g., call the same error handler or setError/toast used in the approve branch) and then clear pending; ensure the user sees the error message instead of a silent return.
♻️ Duplicate comments (1)
packages/web/src/app/oauth/complete/page.tsx (1)
13-23:⚠️ Potential issue | 🔴 CriticalParse and validate the target before assigning
location.href.Line 18 double-decodes because
URLSearchParams.get()already returns a decoded value, and Line 19 checks raw text rather than the parsed protocol. A value such as%0Ajavascript:alert(1)can miss the regex but still parse asjavascript:when navigated.🛡️ Proposed fix
const raw = new URLSearchParams(window.location.search).get('url'); if (!raw) { setError('Missing redirect URL. You may close this window.'); return; } - const decoded = decodeURIComponent(raw);- if (UNPERMITTED_SCHEMES.test(decoded)) {++ let target: URL;+ try {+ target = new URL(raw);+ } catch {+ setError('Invalid redirect URL. You may close this window.');+ return;+ }++ if (UNPERMITTED_SCHEMES.test(target.protocol)) { setError('Redirect URL is not permitted. You may close this window.'); return; } - window.location.href = decoded;++ window.location.href = target.toString();Verification:
#!/bin/bash# Description: Demonstrate that URLSearchParams already decodes and URL parsing normalizes a control-prefixed scheme. node <<'NODE'const raw = new URLSearchParams('url=%0Ajavascript%3Aalert(1)').get('url');console.log({ raw: JSON.stringify(raw), currentRegexMatches: /^(javascript|data|vbscript):/i.test(raw), parsedProtocol: new URL(raw).protocol,});NODE🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/app/oauth/complete/page.tsx` around lines 13 - 23, The code double-decodes and validates the raw string instead of the parsed URL; replace the decode-and-regex check with proper parsing and validation: retrieve the value from URLSearchParams (do not call decodeURIComponent), attempt to construct a new URL(target, window.location.href) inside a try/catch, extract and test the URL.protocol against UNPERMITTED_SCHEMES (or invert the regex check), call setError on parse failures or disallowed protocols, and only then assign window.location.href = parsed.href; reference the existing URLSearchParams.get('url'), decodeURIComponent usage, UNPERMITTED_SCHEMES, and window.location.href locations when making the changes.
🧹 Nitpick comments (1)
packages/web/src/ee/features/oauth/constants.test.ts (1)
20-27: Add regression coverage for browser-trimmed dangerous schemes.The current blocked-scheme tests only cover schemes at position 0. Please add cases with leading spaces/control characters so the OAuth XSS bypass cannot regress.
🧪 Proposed test additions
test.each([ 'javascript:alert(1)', 'data:text/html,<script>alert(1)</script>', 'vbscript:MsgBox("xss")', + ' javascript:alert(1)',+ '\njavascript:alert(1)',+ '\tdata:text/html,<script>alert(1)</script>', ])('blocks full URL string: %s', (url) => { expect(UNPERMITTED_SCHEMES.test(url)).toBe(true); });test('blocks javascript: with mixed case', () => { expect(isPermittedRedirectUrl('JavaScript:alert(1)')).toBe(false); }); ++ test('blocks dangerous schemes with leading whitespace/control characters', () => {+ expect(isPermittedRedirectUrl('\njavascript:alert(1)')).toBe(false);+ expect(isPermittedRedirectUrl(' data:text/html,<script>alert(1)</script>')).toBe(false);+ });Also applies to: 70-84
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/ee/features/oauth/constants.test.ts` around lines 20 - 27, Add regression tests for browser-trimmed dangerous schemes by extending the existing test.each that asserts UNPERMITTED_SCHEMES.test(url) to include variants of the same dangerous schemes prefixed with leading whitespace and control characters (e.g., spaces, tabs, CR/LF, %0A/%0D) so schemes like 'javascript:alert(1)' and 'data:...' still match after trimming; update both the full-URL tests around UNPERMITTED_SCHEMES and the similar cases in the other block that covers positions 70-84 to include these leading-character variants to prevent an OAuth XSS bypass regression.
🤖 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/web/src/ee/features/oauth/constants.ts`:
- Line 4: UNPERMITTED_SCHEMES currently only matches dangerous schemes at the
string start, so inputs with leading C0 controls or whitespace (e.g.
"%0ajavascript:") bypass it; update the UNPERMITTED_SCHEMES regex to allow and
skip any leading C0 control characters and whitespace before matching the scheme
(i.e. permit a leading character class for U+0000–U+001F and whitespace before
the (javascript|data|vbscript): group) so checks in /oauth/complete/page.tsx
catch normalized inputs.
---
Outside diff comments:
In `@packages/web/src/app/oauth/authorize/components/consentScreen.tsx`:
- Around line 69-72: The denyAuthorization call can return a ServiceError but
currently the code just clears pending and returns silently; update the deny
path (where denyAuthorization, isServiceError, and setPending are used) to
mirror the approve flow by setting appropriate UI feedback when a ServiceError
occurs (e.g., call the same error handler or setError/toast used in the approve
branch) and then clear pending; ensure the user sees the error message instead
of a silent return.
---
Duplicate comments:
In `@packages/web/src/app/oauth/complete/page.tsx`:
- Around line 13-23: The code double-decodes and validates the raw string
instead of the parsed URL; replace the decode-and-regex check with proper
parsing and validation: retrieve the value from URLSearchParams (do not call
decodeURIComponent), attempt to construct a new URL(target,
window.location.href) inside a try/catch, extract and test the URL.protocol
against UNPERMITTED_SCHEMES (or invert the regex check), call setError on parse
failures or disallowed protocols, and only then assign window.location.href =
parsed.href; reference the existing URLSearchParams.get('url'),
decodeURIComponent usage, UNPERMITTED_SCHEMES, and window.location.href
locations when making the changes.
---
Nitpick comments:
In `@packages/web/src/ee/features/oauth/constants.test.ts`:
- Around line 20-27: Add regression tests for browser-trimmed dangerous schemes
by extending the existing test.each that asserts UNPERMITTED_SCHEMES.test(url)
to include variants of the same dangerous schemes prefixed with leading
whitespace and control characters (e.g., spaces, tabs, CR/LF, %0A/%0D) so
schemes like 'javascript:alert(1)' and 'data:...' still match after trimming;
update both the full-URL tests around UNPERMITTED_SCHEMES and the similar cases
in the other block that covers positions 70-84 to include these
leading-character variants to prevent an OAuth XSS bypass regression.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d06977f7-b637-490f-93b5-71576f55e444
📒 Files selected for processing (7)
CHANGELOG.mdpackages/web/src/app/api/(server)/ee/oauth/register/route.tspackages/web/src/app/oauth/authorize/components/consentScreen.tsxpackages/web/src/app/oauth/complete/page.tsxpackages/web/src/ee/features/oauth/actions.tspackages/web/src/ee/features/oauth/constants.test.tspackages/web/src/ee/features/oauth/constants.ts
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes SOU-928
Summary
javascript:,data:, andvbscript:URI schemes across the full OAuth redirect flow to resolve CodeQLjs/xss-through-exceptionalert/oauth/completehandoff pageUNPERMITTED_SCHEMESconstant andisPermittedRedirectUrlhelper inconstants.tsfor consistent validation across server and clientUNPERMITTED_SCHEMESandisPermittedRedirectUrlcovering all supported MCP OAuth flows (http, https, vscode://, cursor://, claude://)Test plan
javascript:redirect URIs are rejected at client registration/oauth/completepage shows error for blocked schemesyarn workspace @sourcebot/web test— all 295 tests passyarn workspace @sourcebot/web build— production build succeeds🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
javascript:,data:,vbscript:) across registration, authorization, and redirect stages.Tests