fix(#337): Sanitize OAuth callback error parameter to prevent open redirect
- Validate error against allowlist of OAuth error codes - Unknown errors map to generic message - Encode all URL parameters Refs #337 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -71,6 +71,66 @@ describe("CallbackPage", (): void => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("should sanitize unknown error codes to prevent open redirect", async (): Promise<void> => {
|
||||||
|
// Malicious error parameter that could be used for XSS or redirect attacks
|
||||||
|
mockSearchParams.set("error", "<script>alert('xss')</script>");
|
||||||
|
|
||||||
|
render(<CallbackPage />);
|
||||||
|
|
||||||
|
await waitFor(() => {
|
||||||
|
// Should replace unknown error with generic authentication_error
|
||||||
|
expect(mockPush).toHaveBeenCalledWith("/login?error=authentication_error");
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should sanitize URL-like error codes to prevent open redirect", async (): Promise<void> => {
|
||||||
|
// Attacker tries to inject a URL-like value
|
||||||
|
mockSearchParams.set("error", "https://evil.com/phishing");
|
||||||
|
|
||||||
|
render(<CallbackPage />);
|
||||||
|
|
||||||
|
await waitFor(() => {
|
||||||
|
expect(mockPush).toHaveBeenCalledWith("/login?error=authentication_error");
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should allow valid OAuth 2.0 error codes", async (): Promise<void> => {
|
||||||
|
const validErrors = [
|
||||||
|
"access_denied",
|
||||||
|
"invalid_request",
|
||||||
|
"unauthorized_client",
|
||||||
|
"server_error",
|
||||||
|
"login_required",
|
||||||
|
"consent_required",
|
||||||
|
];
|
||||||
|
|
||||||
|
for (const errorCode of validErrors) {
|
||||||
|
mockPush.mockClear();
|
||||||
|
mockSearchParams.clear();
|
||||||
|
mockSearchParams.set("error", errorCode);
|
||||||
|
|
||||||
|
const { unmount } = render(<CallbackPage />);
|
||||||
|
|
||||||
|
await waitFor(() => {
|
||||||
|
expect(mockPush).toHaveBeenCalledWith(`/login?error=${errorCode}`);
|
||||||
|
});
|
||||||
|
|
||||||
|
unmount();
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should encode special characters in error parameter", async (): Promise<void> => {
|
||||||
|
// Even valid errors should be encoded in the URL
|
||||||
|
mockSearchParams.set("error", "session_failed");
|
||||||
|
|
||||||
|
render(<CallbackPage />);
|
||||||
|
|
||||||
|
await waitFor(() => {
|
||||||
|
// session_failed doesn't need encoding but the function should still call encodeURIComponent
|
||||||
|
expect(mockPush).toHaveBeenCalledWith("/login?error=session_failed");
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
it("should handle refresh session errors gracefully", async (): Promise<void> => {
|
it("should handle refresh session errors gracefully", async (): Promise<void> => {
|
||||||
const mockRefreshSession = vi.fn().mockRejectedValue(new Error("Session error"));
|
const mockRefreshSession = vi.fn().mockRejectedValue(new Error("Session error"));
|
||||||
vi.mocked(useAuth).mockReturnValue({
|
vi.mocked(useAuth).mockReturnValue({
|
||||||
|
|||||||
@@ -5,6 +5,44 @@ import { Suspense, useEffect } from "react";
|
|||||||
import { useRouter, useSearchParams } from "next/navigation";
|
import { useRouter, useSearchParams } from "next/navigation";
|
||||||
import { useAuth } from "@/lib/auth/auth-context";
|
import { useAuth } from "@/lib/auth/auth-context";
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Allowlist of valid OAuth 2.0 and OpenID Connect error codes.
|
||||||
|
* RFC 6749 Section 4.1.2.1 and OpenID Connect Core Section 3.1.2.6
|
||||||
|
*/
|
||||||
|
const VALID_OAUTH_ERRORS = new Set([
|
||||||
|
// OAuth 2.0 RFC 6749
|
||||||
|
"access_denied",
|
||||||
|
"invalid_request",
|
||||||
|
"unauthorized_client",
|
||||||
|
"unsupported_response_type",
|
||||||
|
"invalid_scope",
|
||||||
|
"server_error",
|
||||||
|
"temporarily_unavailable",
|
||||||
|
// OpenID Connect Core
|
||||||
|
"interaction_required",
|
||||||
|
"login_required",
|
||||||
|
"account_selection_required",
|
||||||
|
"consent_required",
|
||||||
|
"invalid_request_uri",
|
||||||
|
"invalid_request_object",
|
||||||
|
"request_not_supported",
|
||||||
|
"request_uri_not_supported",
|
||||||
|
"registration_not_supported",
|
||||||
|
// Internal error codes
|
||||||
|
"session_failed",
|
||||||
|
]);
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Sanitizes an OAuth error parameter to prevent open redirect attacks.
|
||||||
|
* Returns the error if it's in the allowlist, otherwise returns a generic error.
|
||||||
|
*/
|
||||||
|
function sanitizeOAuthError(error: string | null): string | null {
|
||||||
|
if (!error) {
|
||||||
|
return null;
|
||||||
|
}
|
||||||
|
return VALID_OAUTH_ERRORS.has(error) ? error : "authentication_error";
|
||||||
|
}
|
||||||
|
|
||||||
function CallbackContent(): ReactElement {
|
function CallbackContent(): ReactElement {
|
||||||
const router = useRouter();
|
const router = useRouter();
|
||||||
const searchParams = useSearchParams();
|
const searchParams = useSearchParams();
|
||||||
@@ -13,10 +51,11 @@ function CallbackContent(): ReactElement {
|
|||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
async function handleCallback(): Promise<void> {
|
async function handleCallback(): Promise<void> {
|
||||||
// Check for OAuth errors
|
// Check for OAuth errors
|
||||||
const error = searchParams.get("error");
|
const rawError = searchParams.get("error");
|
||||||
|
const error = sanitizeOAuthError(rawError);
|
||||||
if (error) {
|
if (error) {
|
||||||
console.error("OAuth error:", error, searchParams.get("error_description"));
|
console.error("OAuth error:", rawError, searchParams.get("error_description"));
|
||||||
router.push(`/login?error=${error}`);
|
router.push(`/login?error=${encodeURIComponent(error)}`);
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user