Skip to content

Commit f83711e

Browse files
arunavo4arunavo4
committed
Potential security fixes
1 parent bde1f7b commit f83711e

File tree

4 files changed

+187
-5
lines changed

4 files changed

+187
-5
lines changed

‎src/components/oauth/ConsentPage.tsx‎

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import { authClient } from '@/lib/auth-client';
1111
import { apiRequest, showErrorToast } from '@/lib/utils';
1212
import { toast, Toaster } from 'sonner';
1313
import { Shield, User, Mail, ChevronRight, AlertTriangle, Loader2 } from 'lucide-react';
14+
import { isValidRedirectUri, parseRedirectUris } from '@/lib/utils/oauth-validation';
1415

1516
interface OAuthApplication {
1617
id: string;
@@ -44,6 +45,7 @@ export default function ConsentPage() {
4445
const params = new URLSearchParams(window.location.search);
4546
const clientId = params.get('client_id');
4647
const scope = params.get('scope');
48+
const redirectUri = params.get('redirect_uri');
4749

4850
if (!clientId) {
4951
setError('Invalid authorization request: missing client ID');
@@ -59,6 +61,16 @@ export default function ConsentPage() {
5961
return;
6062
}
6163

64+
// Validate redirect URI if provided
65+
if (redirectUri) {
66+
const authorizedUris = parseRedirectUris(app.redirectURLs);
67+
68+
if (!isValidRedirectUri(redirectUri, authorizedUris)) {
69+
setError('Invalid authorization request: unauthorized redirect URI');
70+
return;
71+
}
72+
}
73+
6274
setApplication(app);
6375

6476
// Parse requested scopes
@@ -91,8 +103,27 @@ export default function ConsentPage() {
91103
// If denied, redirect back to the application with error
92104
const params = new URLSearchParams(window.location.search);
93105
const redirectUri = params.get('redirect_uri');
94-
if (redirectUri) {
95-
window.location.href = `${redirectUri}?error=access_denied`;
106+
107+
if (redirectUri && application) {
108+
// Validate redirect URI against authorized URIs
109+
const authorizedUris = parseRedirectUris(application.redirectURLs);
110+
111+
if (isValidRedirectUri(redirectUri, authorizedUris)) {
112+
try {
113+
// Parse and reconstruct the URL to ensure it's safe
114+
const url = new URL(redirectUri);
115+
url.searchParams.set('error', 'access_denied');
116+
117+
// Safe to redirect - URI has been validated and sanitized
118+
window.location.href = url.toString();
119+
} catch (e) {
120+
console.error('Failed to parse redirect URI:', e);
121+
setError('Invalid redirect URI');
122+
}
123+
} else {
124+
console.error('Unauthorized redirect URI:', redirectUri);
125+
setError('Invalid redirect URI');
126+
}
96127
}
97128
}
98129
} catch (error) {

‎src/lib/utils/oauth-validation.test.ts‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
import { describe, test, expect } from "bun:test";
2+
import { isValidRedirectUri, parseRedirectUris } from "./oauth-validation";
3+
4+
describe("OAuth Validation", () => {
5+
describe("parseRedirectUris", () => {
6+
test("parses comma-separated URIs", () => {
7+
const result = parseRedirectUris("https://app1.com,https://app2.com, https://app3.com ");
8+
expect(result).toEqual([
9+
"https://app1.com",
10+
"https://app2.com",
11+
"https://app3.com"
12+
]);
13+
});
14+
15+
test("handles empty string", () => {
16+
expect(parseRedirectUris("")).toEqual([]);
17+
});
18+
19+
test("filters out empty values", () => {
20+
const result = parseRedirectUris("https://app1.com,,https://app2.com,");
21+
expect(result).toEqual(["https://app1.com", "https://app2.com"]);
22+
});
23+
});
24+
25+
describe("isValidRedirectUri", () => {
26+
test("validates exact match", () => {
27+
const authorizedUris = ["https://app.example.com/callback"];
28+
29+
expect(isValidRedirectUri("https://app.example.com/callback", authorizedUris)).toBe(true);
30+
expect(isValidRedirectUri("https://app.example.com/other", authorizedUris)).toBe(false);
31+
});
32+
33+
test("validates wildcard paths", () => {
34+
const authorizedUris = ["https://app.example.com/*"];
35+
36+
expect(isValidRedirectUri("https://app.example.com/", authorizedUris)).toBe(true);
37+
expect(isValidRedirectUri("https://app.example.com/callback", authorizedUris)).toBe(true);
38+
expect(isValidRedirectUri("https://app.example.com/deep/path", authorizedUris)).toBe(true);
39+
40+
// Different domain should fail
41+
expect(isValidRedirectUri("https://evil.com/callback", authorizedUris)).toBe(false);
42+
});
43+
44+
test("validates protocol", () => {
45+
const authorizedUris = ["https://app.example.com/callback"];
46+
47+
// HTTP instead of HTTPS should fail
48+
expect(isValidRedirectUri("http://app.example.com/callback", authorizedUris)).toBe(false);
49+
});
50+
51+
test("validates host and port", () => {
52+
const authorizedUris = ["https://app.example.com:3000/callback"];
53+
54+
// Different port should fail
55+
expect(isValidRedirectUri("https://app.example.com/callback", authorizedUris)).toBe(false);
56+
expect(isValidRedirectUri("https://app.example.com:3000/callback", authorizedUris)).toBe(true);
57+
expect(isValidRedirectUri("https://app.example.com:4000/callback", authorizedUris)).toBe(false);
58+
});
59+
60+
test("handles invalid URIs", () => {
61+
const authorizedUris = ["not-a-valid-uri", "https://valid.com"];
62+
63+
// Invalid redirect URI
64+
expect(isValidRedirectUri("not-a-valid-uri", authorizedUris)).toBe(false);
65+
66+
// Valid redirect URI with invalid authorized URI should still work if it matches valid one
67+
expect(isValidRedirectUri("https://valid.com", authorizedUris)).toBe(true);
68+
});
69+
70+
test("handles empty inputs", () => {
71+
expect(isValidRedirectUri("", ["https://app.com"])).toBe(false);
72+
expect(isValidRedirectUri("https://app.com", [])).toBe(false);
73+
});
74+
75+
test("prevents open redirect attacks", () => {
76+
const authorizedUris = ["https://app.example.com/callback"];
77+
78+
// Various attack vectors
79+
expect(isValidRedirectUri("https://app.example.com.evil.com/callback", authorizedUris)).toBe(false);
80+
expect(isValidRedirectUri("https://[email protected]/callback", authorizedUris)).toBe(false);
81+
expect(isValidRedirectUri("//evil.com/callback", authorizedUris)).toBe(false);
82+
expect(isValidRedirectUri("https:evil.com/callback", authorizedUris)).toBe(false);
83+
});
84+
});
85+
});

‎src/lib/utils/oauth-validation.ts‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
/**
2+
* Validates a redirect URI against a list of authorized URIs
3+
* @param redirectUri The redirect URI to validate
4+
* @param authorizedUris List of authorized redirect URIs
5+
* @returns true if the redirect URI is authorized, false otherwise
6+
*/
7+
export function isValidRedirectUri(redirectUri: string, authorizedUris: string[]): boolean {
8+
if (!redirectUri || authorizedUris.length === 0) {
9+
return false;
10+
}
11+
12+
try {
13+
// Parse the redirect URI to ensure it's valid
14+
const redirectUrl = new URL(redirectUri);
15+
16+
return authorizedUris.some(authorizedUri => {
17+
try {
18+
// Handle wildcard paths (e.g., https://example.com/*)
19+
if (authorizedUri.endsWith('/*')) {
20+
const baseUri = authorizedUri.slice(0, -2);
21+
const baseUrl = new URL(baseUri);
22+
23+
// Check protocol, host, and port match
24+
return redirectUrl.protocol === baseUrl.protocol &&
25+
redirectUrl.host === baseUrl.host &&
26+
redirectUrl.pathname.startsWith(baseUrl.pathname);
27+
}
28+
29+
// Handle exact match
30+
const authorizedUrl = new URL(authorizedUri);
31+
32+
// For exact match, everything must match including path and query params
33+
return redirectUrl.href === authorizedUrl.href;
34+
} catch {
35+
// If authorized URI is not a valid URL, treat as invalid
36+
return false;
37+
}
38+
});
39+
} catch {
40+
// If redirect URI is not a valid URL, it's invalid
41+
return false;
42+
}
43+
}
44+
45+
/**
46+
* Parses a comma-separated list of redirect URIs and trims whitespace
47+
* @param redirectUrls Comma-separated list of redirect URIs
48+
* @returns Array of trimmed redirect URIs
49+
*/
50+
export function parseRedirectUris(redirectUrls: string): string[] {
51+
if (!redirectUrls) {
52+
return [];
53+
}
54+
55+
return redirectUrls
56+
.split(',')
57+
.map(uri => uri.trim())
58+
.filter(uri => uri.length > 0);
59+
}

‎src/pages/api/auth/debug.ts‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,13 @@ export const GET: APIRoute = async ({ request }) => {
2727
headers: { "Content-Type": "application/json" },
2828
});
2929
} catch (error) {
30+
// Log full error details server-side for debugging
31+
console.error("Debug endpoint error:", error);
32+
33+
// Only return safe error information to the client
3034
return new Response(JSON.stringify({
3135
success: false,
32-
error: error instanceof Error ? error.message : "Unknown error",
36+
error: error instanceof Error ? error.message : "An unexpected error occurred"
3337
}), {
3438
status: 500,
3539
headers: { "Content-Type": "application/json" },
@@ -60,10 +64,13 @@ export const POST: APIRoute = async ({ request }) => {
6064
headers: { "Content-Type": "application/json" },
6165
});
6266
} catch (error) {
67+
// Log full error details server-side for debugging
68+
console.error("Debug endpoint error:", error);
69+
70+
// Only return safe error information to the client
6371
return new Response(JSON.stringify({
6472
success: false,
65-
error: error instanceof Error ? error.message : "Unknown error",
66-
details: error
73+
error: error instanceof Error ? error.message : "An unexpected error occurred"
6774
}), {
6875
status: 500,
6976
headers: { "Content-Type": "application/json" },

0 commit comments

Comments
 (0)