fix(web): prevent XSS via OAuth redirect URL validation#1135
fix(web): prevent XSS via OAuth redirect URL validation#1135
Conversation
WalkthroughValidate OAuth redirect URLs returned by approve/deny flows before assigning to window.location; show an "Invalid redirect URL" or failure toast and clear pending when validation fails or on errors. Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User"
rect rgba(220,240,255,0.5)
participant UI as "ConsentScreen UI"
end
rect rgba(240,255,220,0.5)
participant AuthAPI as "approve/denyAuthorization (API)"
end
rect rgba(255,240,220,0.5)
participant Validator as "validateOAuthRedirectUrl"
end
participant Browser as "window.location"
User->>UI: Click Approve / Deny
UI->>AuthAPI: call approveAuthorization / denyAuthorization
AuthAPI-->>UI: returns { success, redirectUrl?, message? }
UI->>Validator: validateOAuthRedirectUrl(redirectUrl)
alt valid URL
Validator-->>UI: normalized URL
UI->>Browser: set window.location.href = normalized URL
else invalid / parse error
Validator-->>UI: null
UI->>UI: show "Invalid redirect URL" / failure toast
UI->>UI: setPending(null)
end
alt API error
AuthAPI-->>UI: error
UI->>UI: show failure toast
UI->>UI: setPending(null)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: Michael Sukkarieh <msukkari@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/app/oauth/complete/page.tsx`:
- Around line 13-18: The decodeURIComponent call is redundant and can corrupt or
throw for valid callback URLs; in the OAuth redirect flow replace usage of
decodeURIComponent(url) with the raw value from URLSearchParams.get('url') and
pass that directly into validateOAuthRedirectUrl, i.e., remove the decode step
around the string returned by URLSearchParams.get and then, if
validateOAuthRedirectUrl(validated) returns a URL, assign it to
window.location.href; update the logic in the component around the
validateOAuthRedirectUrl call to use the raw url variable instead of decodedUrl.
In `@packages/web/src/lib/utils.ts`:
- Around line 608-618: validateOAuthRedirectUrl currently uses a denylist of
protocols which allows unsafe schemes; replace it with an explicit allowlist:
permit "https:" always, permit "http:" only when hostname is "localhost",
"127.0.0.1" or "::1" (allow optional ports), and permit only well-known
registered custom schemes if your app supports them (otherwise reject other
schemes); update the function validateOAuthRedirectUrl to parse the URL, check
protocol against this allowlist, enforce the http host restriction, and return
null for any other schemes (ensuring blob:, file:, ftp:, intent:, etc. are
rejected).
🪄 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: 9893d124-a944-4f24-ac61-5289cee896c7
📒 Files selected for processing (5)
CHANGELOG.mdpackages/web/src/app/oauth/authorize/components/consentScreen.tsxpackages/web/src/app/oauth/complete/page.tsxpackages/web/src/lib/utils.test.tspackages/web/src/lib/utils.ts
| const url = new URLSearchParams(window.location.search).get('url'); | ||
| if (url) { | ||
| window.location.href = decodeURIComponent(url); | ||
| const decodedUrl = decodeURIComponent(url); | ||
| const validatedUrl = validateOAuthRedirectUrl(decodedUrl); | ||
| if (validatedUrl) { | ||
| window.location.href = validatedUrl; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Demonstrate that URLSearchParams.get already decodes and that a second decode mutates/throws.
node <<'NODE'
const original = 'cursor://callback?state=a%2Fb';
const query = `url=${encodeURIComponent(original)}`;
const valueFromSearchParams = new URLSearchParams(query).get('url');
console.log('after URLSearchParams.get:', valueFromSearchParams);
console.log('after second decode:', decodeURIComponent(valueFromSearchParams));
try {
decodeURIComponent('cursor://callback?state=%E0%A4%A');
} catch (error) {
console.log('decodeURIComponent throws:', error.name);
}
NODERepository: sourcebot-dev/sourcebot
Length of output: 208
🏁 Script executed:
#!/bin/bash
# Verify the file exists and read the relevant lines
if [ -f "packages/web/src/app/oauth/complete/page.tsx" ]; then
echo "=== File exists ==="
wc -l packages/web/src/app/oauth/complete/page.tsx
echo ""
echo "=== Lines 1-30 ==="
head -30 packages/web/src/app/oauth/complete/page.tsx | cat -n
else
echo "File does not exist"
fiRepository: sourcebot-dev/sourcebot
Length of output: 1554
🏁 Script executed:
#!/bin/bash
# Find and read the validateOAuthRedirectUrl function
fd -t f "utils\.(ts|tsx|js|jsx)$" packages/web/src/lib/ -x grep -l "validateOAuthRedirectUrl"Repository: sourcebot-dev/sourcebot
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Read the utils.ts file to find validateOAuthRedirectUrl function
cat -n packages/web/src/lib/utils.tsRepository: sourcebot-dev/sourcebot
Length of output: 23417
Remove the redundant decodeURIComponent() call.
URLSearchParams.get('url') already returns a decoded string. Decoding again corrupts valid callback URLs—for example, state=a%2Fb becomes state=a/b, breaking the intended payload. It can also throw URIError on malformed percent-encoded sequences, leaving the page stuck at "Redirecting…".
🐛 Proposed fix
const url = new URLSearchParams(window.location.search).get('url');
if (url) {
- const decodedUrl = decodeURIComponent(url);
- const validatedUrl = validateOAuthRedirectUrl(decodedUrl);
+ const validatedUrl = validateOAuthRedirectUrl(url);
if (validatedUrl) {
window.location.href = validatedUrl;📝 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 url = new URLSearchParams(window.location.search).get('url'); | |
| if (url) { | |
| window.location.href = decodeURIComponent(url); | |
| const decodedUrl = decodeURIComponent(url); | |
| const validatedUrl = validateOAuthRedirectUrl(decodedUrl); | |
| if (validatedUrl) { | |
| window.location.href = validatedUrl; | |
| const url = new URLSearchParams(window.location.search).get('url'); | |
| if (url) { | |
| const validatedUrl = validateOAuthRedirectUrl(url); | |
| if (validatedUrl) { | |
| window.location.href = validatedUrl; |
🤖 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 - 18, The
decodeURIComponent call is redundant and can corrupt or throw for valid callback
URLs; in the OAuth redirect flow replace usage of decodeURIComponent(url) with
the raw value from URLSearchParams.get('url') and pass that directly into
validateOAuthRedirectUrl, i.e., remove the decode step around the string
returned by URLSearchParams.get and then, if validateOAuthRedirectUrl(validated)
returns a URL, assign it to window.location.href; update the logic in the
component around the validateOAuthRedirectUrl call to use the raw url variable
instead of decodedUrl.
| export const validateOAuthRedirectUrl = (url: string): string | null => { | ||
| try { | ||
| const parsed = new URL(url); | ||
| const protocol = parsed.protocol.toLowerCase(); | ||
|
|
||
| const dangerousProtocols = ['javascript:', 'data:', 'vbscript:']; | ||
| if (dangerousProtocols.includes(protocol)) { | ||
| return null; | ||
| } | ||
|
|
||
| return parsed.toString(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Show schemes currently accepted by the denylist-only validator logic.
node <<'NODE'
const dangerousProtocols = ['javascript:', 'data:', 'vbscript:'];
for (const value of [
'file:///etc/passwd',
'ftp://example.com/callback',
'blob:https://example.com/00000000-0000-0000-0000-000000000000',
'about:blank',
'intent://scan/#Intent;scheme=zxing;end',
]) {
const parsed = new URL(value);
const accepted = !dangerousProtocols.includes(parsed.protocol.toLowerCase());
console.log(`${value} -> protocol=${parsed.protocol}, accepted=${accepted}`);
}
NODERepository: sourcebot-dev/sourcebot
Length of output: 392
🏁 Script executed:
# Find the complete function and its usage
rg -A 15 "validateOAuthRedirectUrl" --type ts --type tsxRepository: sourcebot-dev/sourcebot
Length of output: 94
🏁 Script executed:
# Check for OAuth-related configuration and allowed protocols
rg -i "oauth.*protocol|allowed.*protocol|redirect.*url" --type ts --type tsx -B 2 -A 2Repository: sourcebot-dev/sourcebot
Length of output: 94
🏁 Script executed:
# Look for any additional URL validation happening elsewhere
rg "window.location|location.href|window.open" --type ts --type tsx | head -20Repository: sourcebot-dev/sourcebot
Length of output: 94
🏁 Script executed:
# Find the complete function and its usage - search without file type restriction
rg -A 15 "validateOAuthRedirectUrl"Repository: sourcebot-dev/sourcebot
Length of output: 13026
🏁 Script executed:
# Look at the full utils.ts file to see the complete function
cat -n packages/web/src/lib/utils.ts | sed -n '600,640p'Repository: sourcebot-dev/sourcebot
Length of output: 916
🏁 Script executed:
# Check where this validation function is called
rg "validateOAuthRedirectUrl" -B 2 -A 2Repository: sourcebot-dev/sourcebot
Length of output: 7623
Replace denylist with allowlist for OAuth redirect URL validation.
The implementation does not match its own documentation. Comments specify that http: is allowed only for localhost and custom protocols must be registered, but the code accepts any protocol except three. This allows unintended protocols like file://, ftp://, blob://, and intent:// to pass validation.
The tests are also incomplete—they don't verify rejection of http:// with non-localhost hosts or arbitrary custom protocols. Use an allowlist to enforce the documented constraints:
🛡️ Proposed protocol allowlist
export const validateOAuthRedirectUrl = (url: string): string | null => {
try {
const parsed = new URL(url);
const protocol = parsed.protocol.toLowerCase();
- const dangerousProtocols = ['javascript:', 'data:', 'vbscript:'];
- if (dangerousProtocols.includes(protocol)) {
+ const allowedCustomProtocols = new Set(['cursor:', 'vscode:', 'claude:']);
+ const isLoopbackHttp =
+ protocol === 'http:' &&
+ ['localhost', '127.0.0.1', '[::1]'].includes(parsed.hostname);
+
+ if (
+ protocol !== 'https:' &&
+ !isLoopbackHttp &&
+ !allowedCustomProtocols.has(protocol)
+ ) {
return null;
}
return parsed.toString();
} catch {📝 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.
| export const validateOAuthRedirectUrl = (url: string): string | null => { | |
| try { | |
| const parsed = new URL(url); | |
| const protocol = parsed.protocol.toLowerCase(); | |
| const dangerousProtocols = ['javascript:', 'data:', 'vbscript:']; | |
| if (dangerousProtocols.includes(protocol)) { | |
| return null; | |
| } | |
| return parsed.toString(); | |
| export const validateOAuthRedirectUrl = (url: string): string | null => { | |
| try { | |
| const parsed = new URL(url); | |
| const protocol = parsed.protocol.toLowerCase(); | |
| const allowedCustomProtocols = new Set(['cursor:', 'vscode:', 'claude:']); | |
| const isLoopbackHttp = | |
| protocol === 'http:' && | |
| ['localhost', '127.0.0.1', '[::1]'].includes(parsed.hostname); | |
| if ( | |
| protocol !== 'https:' && | |
| !isLoopbackHttp && | |
| !allowedCustomProtocols.has(protocol) | |
| ) { | |
| return null; | |
| } | |
| return parsed.toString(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/lib/utils.ts` around lines 608 - 618,
validateOAuthRedirectUrl currently uses a denylist of protocols which allows
unsafe schemes; replace it with an explicit allowlist: permit "https:" always,
permit "http:" only when hostname is "localhost", "127.0.0.1" or "::1" (allow
optional ports), and permit only well-known registered custom schemes if your
app supports them (otherwise reject other schemes); update the function
validateOAuthRedirectUrl to parse the URL, check protocol against this
allowlist, enforce the http host restriction, and return null for any other
schemes (ensuring blob:, file:, ftp:, intent:, etc. are rejected).
msukkari
force-pushed
the
cursor/fix-xss-oauth-consent-8415
branch
from
37ddf97 to
6cfe552
Compare
|
@msukkari your pull request is missing a changelog! |
Add validateOAuthRedirectUrl() to block dangerous protocols (javascript:, data:, vbscript:) before assigning to window.location.href in the OAuth consent screen. Fixes CodeQL alert #33 (js/xss-through-exception) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
msukkari
force-pushed
the
cursor/fix-xss-oauth-consent-8415
branch
from
6cfe552 to
c09a4cd
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/web/src/lib/utils.ts (1)
598-611:⚠️ Potential issue | 🟠 MajorDenylist-only validation still permits unsafe schemes (e.g.,
file:,ftp:,blob:,intent:).The function's JSDoc and PR description describe allowing only
http:,https:, and known custom app protocols (cursor:,vscode:,claude:), but the implementation only rejects three protocols and accepts everything else. Schemes likefile://,ftp://,blob:,about:, andintent://pass through, which weakens the XSS/open-redirect hardening this PR is intended to provide. Note also thatresolveCallbackUrlinpackages/web/src/ee/features/oauth/actions.tsalready wraps non-http(s)callbacks in/oauth/complete?url=..., so the values flowing into this validator at the consent screen are expected to behttp:/https:or the/oauth/completepath — an allowlist would match that contract more tightly.Switching to an explicit allowlist would also make the existing unit tests meaningful (they currently don't cover rejection of arbitrary unknown schemes).
🛡️ Proposed allowlist
export const validateOAuthRedirectUrl = (url: string): string | null => { try { const parsed = new URL(url); const protocol = parsed.protocol.toLowerCase(); - const dangerousProtocols = ['javascript:', 'data:', 'vbscript:']; - if (dangerousProtocols.includes(protocol)) { + const allowedCustomProtocols = new Set(['cursor:', 'vscode:', 'claude:']); + const isLoopbackHttp = + protocol === 'http:' && + ['localhost', '127.0.0.1', '[::1]'].includes(parsed.hostname); + + if ( + protocol !== 'https:' && + !isLoopbackHttp && + !allowedCustomProtocols.has(protocol) + ) { return null; } return parsed.toString(); } catch { return null; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/lib/utils.ts` around lines 598 - 611, The current denylist in validateOAuthRedirectUrl still permits unsafe schemes; change it to an explicit allowlist: accept relative callback paths that start with "/oauth/complete" as-is, otherwise parse the input with new URL and only allow protocols 'http:', 'https:', and the known app protocols 'cursor:', 'vscode:', 'claude:'; return null for anything else or on parse failure. Update validateOAuthRedirectUrl (and adjust related tests) so resolveCallbackUrl's contract (http(s) or /oauth/complete) is enforced by allowing only those protocols/paths.
🧹 Nitpick comments (1)
packages/web/src/lib/utils.ts (1)
604-606: Consider documenting or testing control character handling in URL scheme validation.The WHATWG URL parser normalizes embedded control characters (tab, newline, carriage return) in the scheme by stripping them silently. For example,
"java\tscript:alert(1)"parses successfully as protocol"javascript:", which is then correctly caught by the existingdangerousProtocolscheck. While the code is currently protected, adding a test case like"java\tscript:alert(1)"or a brief comment explaining reliance on WHATWG normalization would document this behavior and prevent future regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/lib/utils.ts` around lines 604 - 606, Add a brief comment next to the dangerousProtocols check explaining that WHATWG URL parsing normalizes and strips embedded control characters in schemes (e.g., "java\tscript:alert(1)"), and add unit tests that assert such inputs parse to "javascript:" and are rejected by the existing check; specifically, update the tests exercising the conditional that references dangerousProtocols and the protocol variable (the block containing "if (dangerousProtocols.includes(protocol)) { return null; }") to include cases with tabs, newlines, and carriage returns in the scheme to prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/web/src/lib/utils.ts`:
- Around line 598-611: The current denylist in validateOAuthRedirectUrl still
permits unsafe schemes; change it to an explicit allowlist: accept relative
callback paths that start with "/oauth/complete" as-is, otherwise parse the
input with new URL and only allow protocols 'http:', 'https:', and the known app
protocols 'cursor:', 'vscode:', 'claude:'; return null for anything else or on
parse failure. Update validateOAuthRedirectUrl (and adjust related tests) so
resolveCallbackUrl's contract (http(s) or /oauth/complete) is enforced by
allowing only those protocols/paths.
---
Nitpick comments:
In `@packages/web/src/lib/utils.ts`:
- Around line 604-606: Add a brief comment next to the dangerousProtocols check
explaining that WHATWG URL parsing normalizes and strips embedded control
characters in schemes (e.g., "java\tscript:alert(1)"), and add unit tests that
assert such inputs parse to "javascript:" and are rejected by the existing
check; specifically, update the tests exercising the conditional that references
dangerousProtocols and the protocol variable (the block containing "if
(dangerousProtocols.includes(protocol)) { return null; }") to include cases with
tabs, newlines, and carriage returns in the scheme to prevent regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6a3738c9-6834-48ed-9d63-b0dedf5c90db
📒 Files selected for processing (2)
packages/web/src/app/oauth/authorize/components/consentScreen.tsxpackages/web/src/lib/utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/src/app/oauth/authorize/components/consentScreen.tsx
1 participant
Summary
This PR fixes the CodeQL security alert #33 (js/xss-through-exception) which flagged XSS vulnerabilities in the OAuth consent screen.
Fixes #928
Changes
1. Added URL Validation Utility
Added
validateOAuthRedirectUrl()function topackages/web/src/lib/utils.tsthat:javascript:,data:,vbscript:http:,https:, and custom app protocols (e.g.,cursor://,vscode://,claude://)nullfor invalid URLs or dangerous protocols2. Fixed OAuth Consent Screen (
consentScreen.tsx)window.location.hrefnavigation in bothonApprove()andonDeny()flows${result.message}) to use static error messages, preventing XSS via exception text3. Fixed OAuth Complete Page (
complete/page.tsx)window.location.hrefnavigation4. Added Unit Tests
Added comprehensive tests in
packages/web/src/lib/utils.test.tscovering:Security Impact
javascript:URI injection in OAuth redirect URLsTesting
Linear Issue: SOU-928
Summary by CodeRabbit