-
Notifications
You must be signed in to change notification settings - Fork 259
fix(web): prevent XSS via OAuth redirect URI scheme injection #1136
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,21 +1,33 @@ | ||
| 'use client'; | ||
|
|
||
| import { useEffect } from 'react'; | ||
| import { useEffect, useState } from 'react'; | ||
| import { UNPERMITTED_SCHEMES } from '@/ee/features/oauth/constants'; | ||
|
|
||
| // Handles the final redirect after OAuth authorization. For http/https redirect URIs, | ||
| // a server-side redirect is sufficient. For custom protocol URIs (e.g. cursor://, claude://), | ||
| // browsers won't follow HTTP redirects, so we use window.location.href instead. | ||
| export default function OAuthCompletePage() { | ||
| const [error, setError] = useState<string | null>(null); | ||
|
|
||
| useEffect(() => { | ||
| const url = new URLSearchParams(window.location.search).get('url'); | ||
| if (url) { | ||
| window.location.href = decodeURIComponent(url); | ||
| 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)) { | ||
| setError('Redirect URL is not permitted. You may close this window.'); | ||
| return; | ||
| } | ||
| window.location.href = decoded; | ||
Check failureCode scanning / CodeQL Client-side cross-site scripting High
Cross-site scripting vulnerability due to
user-provided value Error loading related location Loading Check warningCode scanning / CodeQL Client-side URL redirect Medium
Untrusted URL redirection depends on a
user-provided value Error loading related location Loading |
||
|
msukkari marked this conversation as resolved.
Dismissed
|
||
| }, []); | ||
|
|
||
| return ( | ||
| <div className="min-h-screen flex items-center justify-center bg-background"> | ||
| <p className="text-sm text-muted-foreground">Redirecting, you may close this window...</p> | ||
| <p className="text-sm text-muted-foreground"> | ||
| {error ?? 'Redirecting, you may close this window...'} | ||
| </p> | ||
| </div> | ||
| ); | ||
| } | ||
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| import { expect, test, describe } from 'vitest'; | ||
| import { UNPERMITTED_SCHEMES, isPermittedRedirectUrl } from './constants'; | ||
|
|
||
| describe('UNPERMITTED_SCHEMES', () => { | ||
| // Dangerous schemes that must be blocked | ||
| test.each([ | ||
| 'javascript:', | ||
| 'JavaScript:', | ||
| 'JAVASCRIPT:', | ||
| 'data:', | ||
| 'Data:', | ||
| 'DATA:', | ||
| 'vbscript:', | ||
| 'VBScript:', | ||
| 'VBSCRIPT:', | ||
| ])('blocks %s', (scheme) => { | ||
| expect(UNPERMITTED_SCHEMES.test(scheme)).toBe(true); | ||
| }); | ||
|
|
||
| // Full URL strings (used in /oauth/complete page) | ||
| test.each([ | ||
| 'javascript:alert(1)', | ||
| 'data:text/html,<script>alert(1)</script>', | ||
| 'vbscript:MsgBox("xss")', | ||
| ])('blocks full URL string: %s', (url) => { | ||
| expect(UNPERMITTED_SCHEMES.test(url)).toBe(true); | ||
| }); | ||
|
|
||
| // Permitted schemes that must not be blocked | ||
| test.each([ | ||
| 'http:', | ||
| 'https:', | ||
| 'vscode:', | ||
| 'cursor:', | ||
| 'claude:', | ||
| 'vscode://callback', | ||
| 'cursor://callback?code=abc', | ||
| 'http://localhost:8080/callback', | ||
| 'https://example.com/callback', | ||
| ])('permits %s', (url) => { | ||
| expect(UNPERMITTED_SCHEMES.test(url)).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe('isPermittedRedirectUrl', () => { | ||
| // --- Permitted URLs --- | ||
|
|
||
| test('permits https URLs', () => { | ||
| expect(isPermittedRedirectUrl('https://example.com/callback')).toBe(true); | ||
| }); | ||
|
|
||
| test('permits http URLs', () => { | ||
| expect(isPermittedRedirectUrl('http://localhost:8080/callback')).toBe(true); | ||
| }); | ||
|
|
||
| test('permits /oauth/complete relative paths', () => { | ||
| expect(isPermittedRedirectUrl('/oauth/complete?url=vscode%3A%2F%2Fcallback')).toBe(true); | ||
| }); | ||
|
|
||
| test('permits /oauth/complete with encoded custom scheme', () => { | ||
| expect(isPermittedRedirectUrl('/oauth/complete?url=cursor%3A%2F%2Fcallback%3Fcode%3Dabc')).toBe(true); | ||
| }); | ||
|
|
||
| test('permits https URL with query params and fragment', () => { | ||
| expect(isPermittedRedirectUrl('https://example.com/callback?code=abc&state=xyz#fragment')).toBe(true); | ||
| }); | ||
|
|
||
| // --- Blocked URLs --- | ||
|
|
||
| test('blocks javascript: URLs', () => { | ||
| expect(isPermittedRedirectUrl('javascript:alert(1)')).toBe(false); | ||
| }); | ||
|
|
||
| test('blocks data: URLs', () => { | ||
| expect(isPermittedRedirectUrl('data:text/html,<script>alert(1)</script>')).toBe(false); | ||
| }); | ||
|
|
||
| test('blocks vbscript: URLs', () => { | ||
| expect(isPermittedRedirectUrl('vbscript:MsgBox("xss")')).toBe(false); | ||
| }); | ||
|
|
||
| test('blocks javascript: with mixed case', () => { | ||
| expect(isPermittedRedirectUrl('JavaScript:alert(1)')).toBe(false); | ||
| }); | ||
|
|
||
| test('blocks custom scheme URLs not wrapped in /oauth/complete', () => { | ||
| expect(isPermittedRedirectUrl('vscode://callback?code=abc')).toBe(false); | ||
| }); | ||
|
|
||
| test('blocks malformed URLs', () => { | ||
| expect(isPermittedRedirectUrl('not a url at all')).toBe(false); | ||
| }); | ||
|
|
||
| test('blocks empty string', () => { | ||
| expect(isPermittedRedirectUrl('')).toBe(false); | ||
| }); | ||
|
|
||
| test('blocks relative paths that do not start with /oauth/complete', () => { | ||
| expect(isPermittedRedirectUrl('/some/other/path')).toBe(false); | ||
| }); | ||
| }); |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,22 @@ | ||
|
|
||
| export const OAUTH_NOT_SUPPORTED_ERROR_MESSAGE = 'OAuth is not supported on this instance. Please authenticate using a Sourcebot API key instead. See https://docs.sourcebot.dev/docs/features/mcp-server for more information.'; | ||
| export const OAUTH_NOT_SUPPORTED_ERROR_MESSAGE = 'OAuth is not supported on this instance. Please authenticate using a Sourcebot API key instead. See https://docs.sourcebot.dev/docs/features/mcp-server for more information.'; | ||
|
|
||
| export const UNPERMITTED_SCHEMES = /^(javascript|data|vbscript):/i; | ||
|
msukkari marked this conversation as resolved.
|
||
|
|
||
| /** | ||
| * Returns true if the URL is permitted for use as a redirect target. | ||
| * Allows relative paths starting with /oauth/complete and http(s) URLs. | ||
| * Returns false for dangerous schemes like javascript:, data:, vbscript:. | ||
| */ | ||
| export function isPermittedRedirectUrl(url: string): boolean { | ||
| if (url.startsWith('/oauth/complete')) { | ||
| return true; | ||
| } | ||
|
|
||
| try { | ||
| const parsed = new URL(url); | ||
| return parsed.protocol === 'http:' || parsed.protocol === 'https:'; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } | ||
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.