diff --git a/src/server/auth/errors.ts b/src/server/auth/errors.ts index 48949d7c..cdca4123 100644 --- a/src/server/auth/errors.ts +++ b/src/server/auth/errors.ts @@ -169,4 +169,14 @@ export class TooManyRequestsError extends OAuthError { constructor(message: string, errorUri?: string) { super("too_many_requests", message, errorUri); } +} + +/** + * Invalid client metadata error - The client metadata is invalid. + * (Custom error for dynamic client registration - RFC 7591) + */ +export class InvalidClientMetadataError extends OAuthError { + constructor(message: string, errorUri?: string) { + super("invalid_client_metadata", message, errorUri); + } } \ No newline at end of file diff --git a/src/server/auth/handlers/authorize.test.ts b/src/server/auth/handlers/authorize.test.ts index 232bed3e..8bb454b3 100644 --- a/src/server/auth/handlers/authorize.test.ts +++ b/src/server/auth/handlers/authorize.test.ts @@ -113,7 +113,6 @@ describe('Authorization Handler', () => { .query({ client_id: 'nonexistent-client' }); expect(response.status).toBe(400); - expect(response.text).toContain('invalid client_id'); }); }); @@ -144,7 +143,6 @@ describe('Authorization Handler', () => { }); expect(response.status).toBe(400); - expect(response.text).toContain('missing redirect_uri'); }); it('validates redirect_uri against client registered URIs', async () => { @@ -159,7 +157,6 @@ describe('Authorization Handler', () => { }); expect(response.status).toBe(400); - expect(response.text).toContain('unregistered redirect_uri'); }); it('accepts valid redirect_uri that client registered with', async () => { diff --git a/src/server/auth/handlers/authorize.ts b/src/server/auth/handlers/authorize.ts index ddf362d1..b9f86ec4 100644 --- a/src/server/auth/handlers/authorize.ts +++ b/src/server/auth/handlers/authorize.ts @@ -4,6 +4,14 @@ import express from "express"; import { OAuthServerProvider } from "../provider.js"; import { rateLimit, Options as RateLimitOptions } from "express-rate-limit"; import { allowedMethods } from "../middleware/allowedMethods.js"; +import { + InvalidRequestError, + InvalidClientError, + InvalidScopeError, + ServerError, + TooManyRequestsError, + OAuthError +} from "../errors.js"; export type AuthorizationHandlerOptions = { provider: OAuthServerProvider; @@ -42,81 +50,116 @@ export function authorizationHandler({ provider, rateLimit: rateLimitConfig }: A max: 100, // 100 requests per windowMs standardHeaders: true, legacyHeaders: false, - message: { - error: 'too_many_requests', - error_description: 'You have exceeded the rate limit for authorization requests' - }, + message: new TooManyRequestsError('You have exceeded the rate limit for authorization requests').toResponseObject(), ...rateLimitConfig })); } - // Define the handler router.all("/", async (req, res) => { res.setHeader('Cache-Control', 'no-store'); - let client_id, redirect_uri; + // In the authorization flow, errors are split into two categories: + // 1. Pre-redirect errors (direct response with 400) + // 2. Post-redirect errors (redirect with error parameters) + + // Phase 1: Validate client_id and redirect_uri. Any errors here must be direct responses. + let client_id, redirect_uri, client; try { - const data = req.method === 'POST' ? req.body : req.query; - ({ client_id, redirect_uri } = ClientAuthorizationParamsSchema.parse(data)); - } catch (error) { - res.status(400).end(`Bad Request: ${error}`); - return; - } - - const client = await provider.clientsStore.getClient(client_id); - if (!client) { - res.status(400).end("Bad Request: invalid client_id"); - return; - } - - if (redirect_uri !== undefined) { - if (!client.redirect_uris.includes(redirect_uri)) { - res.status(400).end("Bad Request: unregistered redirect_uri"); - return; + const result = ClientAuthorizationParamsSchema.safeParse(req.method === 'POST' ? req.body : req.query); + if (!result.success) { + throw new InvalidRequestError(result.error.message); } - } else if (client.redirect_uris.length === 1) { - redirect_uri = client.redirect_uris[0]; - } else { - res.status(400).end("Bad Request: missing redirect_uri"); - return; - } - let params; - try { - const authData = req.method === 'POST' ? req.body : req.query; - params = RequestAuthorizationParamsSchema.parse(authData); + client_id = result.data.client_id; + redirect_uri = result.data.redirect_uri; + + client = await provider.clientsStore.getClient(client_id); + if (!client) { + throw new InvalidClientError("Invalid client_id"); + } + + if (redirect_uri !== undefined) { + if (!client.redirect_uris.includes(redirect_uri)) { + throw new InvalidRequestError("Unregistered redirect_uri"); + } + } else if (client.redirect_uris.length === 1) { + redirect_uri = client.redirect_uris[0]; + } else { + throw new InvalidRequestError("redirect_uri must be specified when client has multiple registered URIs"); + } } catch (error) { - const errorUrl = new URL(redirect_uri); - errorUrl.searchParams.set("error", "invalid_request"); - errorUrl.searchParams.set("error_description", String(error)); - res.redirect(302, errorUrl.href); + // Pre-redirect errors - return direct response + if (error instanceof OAuthError) { + const status = error instanceof ServerError ? 500 : 400; + res.status(status).end(error.message); + } else { + console.error("Unexpected error looking up client:", error); + res.status(500).end("Internal Server Error"); + } + return; } - let requestedScopes: string[] = []; - if (params.scope !== undefined && client.scope !== undefined) { - requestedScopes = params.scope.split(" "); - const allowedScopes = new Set(client.scope.split(" ")); + // Phase 2: Validate other parameters. Any errors here should go into redirect responses. + let state; + try { + // Parse and validate authorization parameters + const parseResult = RequestAuthorizationParamsSchema.safeParse(req.method === 'POST' ? req.body : req.query); + if (!parseResult.success) { + throw new InvalidRequestError(parseResult.error.message); + } - // If any requested scope is not in the client's registered scopes, error out - for (const scope of requestedScopes) { - if (!allowedScopes.has(scope)) { - const errorUrl = new URL(redirect_uri); - errorUrl.searchParams.set("error", "invalid_scope"); - errorUrl.searchParams.set("error_description", `Client was not registered with scope ${scope}`); - res.redirect(302, errorUrl.href); - return; + const { scope, code_challenge } = parseResult.data; + state = parseResult.data.state; + + // Validate scopes + let requestedScopes: string[] = []; + if (scope !== undefined && client.scope !== undefined) { + requestedScopes = scope.split(" "); + const allowedScopes = new Set(client.scope.split(" ")); + + // Check each requested scope against allowed scopes + for (const scope of requestedScopes) { + if (!allowedScopes.has(scope)) { + throw new InvalidScopeError(`Client was not registered with scope ${scope}`); + } } } - } - await provider.authorize(client, { - state: params.state, - scopes: requestedScopes, - redirectUri: redirect_uri, - codeChallenge: params.code_challenge, - }, res); + // All validation passed, proceed with authorization + await provider.authorize(client, { + state, + scopes: requestedScopes, + redirectUri: redirect_uri, + codeChallenge: code_challenge, + }, res); + } catch (error) { + // Post-redirect errors - redirect with error parameters + if (error instanceof OAuthError) { + res.redirect(302, createErrorRedirect(redirect_uri, error, state)); + } else { + console.error("Unexpected error during authorization:", error); + const serverError = new ServerError("Internal Server Error"); + res.redirect(302, createErrorRedirect(redirect_uri, serverError, state)); + } + } }); return router; +} + +/** + * Helper function to create redirect URL with error parameters + */ +function createErrorRedirect(redirectUri: string, error: OAuthError, state?: string): string { + const errorUrl = new URL(redirectUri); + errorUrl.searchParams.set("error", error.errorCode); + errorUrl.searchParams.set("error_description", error.message); + if (error.errorUri) { + errorUrl.searchParams.set("error_uri", error.errorUri); + } + if (state) { + errorUrl.searchParams.set("state", state); + } + return errorUrl.href; } \ No newline at end of file diff --git a/src/server/auth/handlers/register.ts b/src/server/auth/handlers/register.ts index 308ccd35..675e8733 100644 --- a/src/server/auth/handlers/register.ts +++ b/src/server/auth/handlers/register.ts @@ -5,6 +5,12 @@ import cors from 'cors'; import { OAuthRegisteredClientsStore } from "../clients.js"; import { rateLimit, Options as RateLimitOptions } from "express-rate-limit"; import { allowedMethods } from "../middleware/allowedMethods.js"; +import { + InvalidClientMetadataError, + ServerError, + TooManyRequestsError, + OAuthError +} from "../errors.js"; export type ClientRegistrationHandlerOptions = { /** @@ -54,10 +60,7 @@ export function clientRegistrationHandler({ max: 20, // 20 requests per hour - stricter as registration is sensitive standardHeaders: true, legacyHeaders: false, - message: { - error: 'too_many_requests', - error_description: 'You have exceeded the rate limit for client registration requests' - }, + message: new TooManyRequestsError('You have exceeded the rate limit for client registration requests').toResponseObject(), ...rateLimitConfig })); } @@ -65,33 +68,41 @@ export function clientRegistrationHandler({ router.post("/", async (req, res) => { res.setHeader('Cache-Control', 'no-store'); - let clientMetadata; try { - clientMetadata = OAuthClientMetadataSchema.parse(req.body); + const parseResult = OAuthClientMetadataSchema.safeParse(req.body); + if (!parseResult.success) { + throw new InvalidClientMetadataError(parseResult.error.message); + } + + const clientMetadata = parseResult.data; + + // Generate client credentials + const clientId = crypto.randomUUID(); + const clientSecret = clientMetadata.token_endpoint_auth_method !== 'none' + ? crypto.randomBytes(32).toString('hex') + : undefined; + const clientIdIssuedAt = Math.floor(Date.now() / 1000); + + let clientInfo: OAuthClientInformationFull = { + ...clientMetadata, + client_id: clientId, + client_secret: clientSecret, + client_id_issued_at: clientIdIssuedAt, + client_secret_expires_at: clientSecretExpirySeconds > 0 ? clientIdIssuedAt + clientSecretExpirySeconds : 0 + }; + + clientInfo = await clientsStore.registerClient!(clientInfo); + res.status(201).json(clientInfo); } catch (error) { - res.status(400).json({ - error: "invalid_client_metadata", - error_description: String(error), - }); - return; + if (error instanceof OAuthError) { + const status = error instanceof ServerError ? 500 : 400; + res.status(status).json(error.toResponseObject()); + } else { + console.error("Unexpected error registering client:", error); + const serverError = new ServerError("Internal Server Error"); + res.status(500).json(serverError.toResponseObject()); + } } - - const clientId = crypto.randomUUID(); - const clientSecret = clientMetadata.token_endpoint_auth_method !== 'none' - ? crypto.randomBytes(32).toString('hex') - : undefined; - const clientIdIssuedAt = Math.floor(Date.now() / 1000); - - let clientInfo: OAuthClientInformationFull = { - ...clientMetadata, - client_id: clientId, - client_secret: clientSecret, - client_id_issued_at: clientIdIssuedAt, - client_secret_expires_at: clientSecretExpirySeconds > 0 ? clientIdIssuedAt + clientSecretExpirySeconds : 0 - }; - - clientInfo = await clientsStore.registerClient!(clientInfo); - res.status(201).json(clientInfo); }); return router; diff --git a/src/server/auth/handlers/revoke.ts b/src/server/auth/handlers/revoke.ts index f98e91c1..95e8b4b3 100644 --- a/src/server/auth/handlers/revoke.ts +++ b/src/server/auth/handlers/revoke.ts @@ -5,6 +5,12 @@ import { authenticateClient } from "../middleware/clientAuth.js"; import { OAuthTokenRevocationRequestSchema } from "../../../shared/auth.js"; import { rateLimit, Options as RateLimitOptions } from "express-rate-limit"; import { allowedMethods } from "../middleware/allowedMethods.js"; +import { + InvalidRequestError, + ServerError, + TooManyRequestsError, + OAuthError +} from "../errors.js"; export type RevocationHandlerOptions = { provider: OAuthServerProvider; @@ -36,10 +42,7 @@ export function revocationHandler({ provider, rateLimit: rateLimitConfig }: Revo max: 50, // 50 requests per windowMs standardHeaders: true, legacyHeaders: false, - message: { - error: 'too_many_requests', - error_description: 'You have exceeded the rate limit for token revocation requests' - }, + message: new TooManyRequestsError('You have exceeded the rate limit for token revocation requests').toResponseObject(), ...rateLimitConfig })); } @@ -50,27 +53,31 @@ export function revocationHandler({ provider, rateLimit: rateLimitConfig }: Revo router.post("/", async (req, res) => { res.setHeader('Cache-Control', 'no-store'); - let revocationRequest; try { - revocationRequest = OAuthTokenRevocationRequestSchema.parse(req.body); + const parseResult = OAuthTokenRevocationRequestSchema.safeParse(req.body); + if (!parseResult.success) { + throw new InvalidRequestError(parseResult.error.message); + } + + const client = req.client; + if (!client) { + // This should never happen + console.error("Missing client information after authentication"); + throw new ServerError("Internal Server Error"); + } + + await provider.revokeToken!(client, parseResult.data); + res.status(200).json({}); } catch (error) { - res.status(400).json({ - error: "invalid_request", - error_description: String(error), - }); - return; + if (error instanceof OAuthError) { + const status = error instanceof ServerError ? 500 : 400; + res.status(status).json(error.toResponseObject()); + } else { + console.error("Unexpected error revoking token:", error); + const serverError = new ServerError("Internal Server Error"); + res.status(500).json(serverError.toResponseObject()); + } } - - const client = req.client; - if (!client) { - console.error("Missing client information after authentication"); - res.status(500).end("Internal Server Error"); - return; - } - - await provider.revokeToken!(client, revocationRequest); - // Return empty response on success (per OAuth 2.0 spec) - res.status(200).json({}); }); return router; diff --git a/src/server/auth/handlers/token.test.ts b/src/server/auth/handlers/token.test.ts index f30fc83d..3071ea46 100644 --- a/src/server/auth/handlers/token.test.ts +++ b/src/server/auth/handlers/token.test.ts @@ -5,10 +5,13 @@ import { OAuthClientInformationFull, OAuthTokenRevocationRequest, OAuthTokens } import express, { Response } from 'express'; import supertest from 'supertest'; import * as pkceChallenge from 'pkce-challenge'; +import { InvalidGrantError } from '../errors.js'; // Mock pkce-challenge jest.mock('pkce-challenge', () => ({ - verifyChallenge: jest.fn() + verifyChallenge: jest.fn().mockImplementation(async (verifier, challenge) => { + return verifier === 'valid_verifier' && challenge === 'mock_challenge'; + }) })); describe('Token Handler', () => { @@ -46,9 +49,9 @@ describe('Token Handler', () => { if (authorizationCode === 'valid_code') { return 'mock_challenge'; } else if (authorizationCode === 'expired_code') { - throw new Error('invalid_grant'); + throw new InvalidGrantError('The authorization code has expired'); } - throw new Error('invalid_grant'); + throw new InvalidGrantError('The authorization code is invalid'); }, async exchangeAuthorizationCode(client: OAuthClientInformationFull, authorizationCode: string): Promise { @@ -60,7 +63,7 @@ describe('Token Handler', () => { refresh_token: 'mock_refresh_token' }; } - throw new Error('invalid_grant'); + throw new InvalidGrantError('The authorization code is invalid or has expired'); }, async exchangeRefreshToken(client: OAuthClientInformationFull, refreshToken: string, scopes?: string[]): Promise { @@ -78,7 +81,7 @@ describe('Token Handler', () => { return response; } - throw new Error('invalid_grant'); + throw new InvalidGrantError('The refresh token is invalid or has expired'); }, async revokeToken(_client: OAuthClientInformationFull, _request: OAuthTokenRevocationRequest): Promise { @@ -88,7 +91,7 @@ describe('Token Handler', () => { // Mock PKCE verification (pkceChallenge.verifyChallenge as jest.Mock).mockImplementation( - (verifier: string, challenge: string) => { + async (verifier: string, challenge: string) => { return verifier === 'valid_verifier' && challenge === 'mock_challenge'; } ); @@ -212,7 +215,7 @@ describe('Token Handler', () => { it('verifies code_verifier against challenge', async () => { // Setup invalid verifier - (pkceChallenge.verifyChallenge as jest.Mock).mockReturnValueOnce(false); + (pkceChallenge.verifyChallenge as jest.Mock).mockResolvedValueOnce(false); const response = await supertest(app) .post('/token') @@ -226,7 +229,7 @@ describe('Token Handler', () => { }); expect(response.status).toBe(400); - expect(response.body.error).toBe('invalid_request'); + expect(response.body.error).toBe('invalid_grant'); expect(response.body.error_description).toContain('code_verifier'); }); @@ -242,7 +245,8 @@ describe('Token Handler', () => { code_verifier: 'valid_verifier' }); - expect(response.status).toBe(500); // Implementation currently doesn't handle exceptions properly + expect(response.status).toBe(400); + expect(response.body.error).toBe('invalid_grant'); }); it('returns tokens for valid code exchange', async () => { @@ -292,7 +296,8 @@ describe('Token Handler', () => { refresh_token: 'invalid_refresh_token' }); - expect(response.status).toBe(500); // Implementation currently doesn't handle exceptions properly + expect(response.status).toBe(400); + expect(response.body.error).toBe('invalid_grant'); }); it('returns new tokens for valid refresh token', async () => { diff --git a/src/server/auth/handlers/token.ts b/src/server/auth/handlers/token.ts index 3e7aa853..79312068 100644 --- a/src/server/auth/handlers/token.ts +++ b/src/server/auth/handlers/token.ts @@ -6,6 +6,14 @@ import { verifyChallenge } from "pkce-challenge"; import { authenticateClient } from "../middleware/clientAuth.js"; import { rateLimit, Options as RateLimitOptions } from "express-rate-limit"; import { allowedMethods } from "../middleware/allowedMethods.js"; +import { + InvalidRequestError, + InvalidGrantError, + UnsupportedGrantTypeError, + ServerError, + TooManyRequestsError, + OAuthError +} from "../errors.js"; export type TokenHandlerOptions = { provider: OAuthServerProvider; @@ -47,10 +55,7 @@ export function tokenHandler({ provider, rateLimit: rateLimitConfig }: TokenHand max: 50, // 50 requests per windowMs standardHeaders: true, legacyHeaders: false, - message: { - error: 'too_many_requests', - error_description: 'You have exceeded the rate limit for token requests' - }, + message: new TooManyRequestsError('You have exceeded the rate limit for token requests').toResponseObject(), ...rateLimitConfig })); } @@ -61,79 +66,72 @@ export function tokenHandler({ provider, rateLimit: rateLimitConfig }: TokenHand router.post("/", async (req, res) => { res.setHeader('Cache-Control', 'no-store'); - let grant_type; try { - ({ grant_type } = TokenRequestSchema.parse(req.body)); + const parseResult = TokenRequestSchema.safeParse(req.body); + if (!parseResult.success) { + throw new InvalidRequestError(parseResult.error.message); + } + + const { grant_type } = parseResult.data; + + const client = req.client; + if (!client) { + // This should never happen + console.error("Missing client information after authentication"); + throw new ServerError("Internal Server Error"); + } + + switch (grant_type) { + case "authorization_code": { + const parseResult = AuthorizationCodeGrantSchema.safeParse(req.body); + if (!parseResult.success) { + throw new InvalidRequestError(parseResult.error.message); + } + + const { code, code_verifier } = parseResult.data; + + // Verify PKCE challenge + const codeChallenge = await provider.challengeForAuthorizationCode(client, code); + if (!(await verifyChallenge(code_verifier, codeChallenge))) { + throw new InvalidGrantError("code_verifier does not match the challenge"); + } + + const tokens = await provider.exchangeAuthorizationCode(client, code); + res.status(200).json(tokens); + break; + } + + case "refresh_token": { + const parseResult = RefreshTokenGrantSchema.safeParse(req.body); + if (!parseResult.success) { + throw new InvalidRequestError(parseResult.error.message); + } + + const { refresh_token, scope } = parseResult.data; + + const scopes = scope?.split(" "); + const tokens = await provider.exchangeRefreshToken(client, refresh_token, scopes); + res.status(200).json(tokens); + break; + } + + // Not supported right now + //case "client_credentials": + + default: + throw new UnsupportedGrantTypeError( + "The grant type is not supported by this authorization server." + ); + } } catch (error) { - res.status(400).json({ - error: "invalid_request", - error_description: String(error), - }); - return; - } - - const client = req.client; - if (!client) { - console.error("Missing client information after authentication"); - res.status(500).end("Internal Server Error"); - return; - } - - switch (grant_type) { - case "authorization_code": { - let grant; - try { - grant = AuthorizationCodeGrantSchema.parse(req.body); - } catch (error) { - res.status(400).json({ - error: "invalid_request", - error_description: String(error), - }); - return; - } - - const codeChallenge = await provider.challengeForAuthorizationCode(client, grant.code); - if (!(await verifyChallenge(grant.code_verifier, codeChallenge))) { - res.status(400).json({ - error: "invalid_request", - error_description: "code_verifier does not match the challenge", - }); - - return; - } - - const tokens = await provider.exchangeAuthorizationCode(client, grant.code); - res.status(200).json(tokens); - break; + if (error instanceof OAuthError) { + const status = error instanceof ServerError ? 500 : 400; + res.status(status).json(error.toResponseObject()); + } else { + console.error("Unexpected error exchanging token:", error); + const serverError = new ServerError("Internal Server Error"); + res.status(500).json(serverError.toResponseObject()); } - - case "refresh_token": { - let grant; - try { - grant = RefreshTokenGrantSchema.parse(req.body); - } catch (error) { - res.status(400).json({ - error: "invalid_request", - error_description: String(error), - }); - return; - } - - const scopes = grant.scope ? grant.scope.split(" ") : undefined; - const tokens = await provider.exchangeRefreshToken(client, grant.refresh_token, scopes); - res.status(200).json(tokens); - break; - } - - // Not supported right now - //case "client_credentials": - - default: - res.status(400).json({ - error: "unsupported_grant_type", - error_description: "The grant type is not supported by this authorization server.", - }); - return; } }); diff --git a/src/server/auth/middleware/clientAuth.ts b/src/server/auth/middleware/clientAuth.ts index 8cdd0e94..78aa595b 100644 --- a/src/server/auth/middleware/clientAuth.ts +++ b/src/server/auth/middleware/clientAuth.ts @@ -2,7 +2,7 @@ import { z } from "zod"; import { RequestHandler } from "express"; import { OAuthRegisteredClientsStore } from "../clients.js"; import { OAuthClientInformationFull } from "../../../shared/auth.js"; -import { InvalidRequestError, InvalidClientError } from "../errors.js"; +import { InvalidRequestError, InvalidClientError, ServerError, OAuthError } from "../errors.js"; export type ClientAuthenticationMiddlewareOptions = { /** @@ -28,15 +28,12 @@ declare module "express-serve-static-core" { export function authenticateClient({ clientsStore }: ClientAuthenticationMiddlewareOptions): RequestHandler { return async (req, res, next) => { try { - let client_id, client_secret; - try { - const result = ClientAuthenticatedRequestSchema.parse(req.body); - client_id = result.client_id; - client_secret = result.client_secret; - } catch (error) { - throw new InvalidRequestError(String(error)); + const result = ClientAuthenticatedRequestSchema.safeParse(req.body); + if (!result.success) { + throw new InvalidRequestError(String(result.error)); } + const { client_id, client_secret } = result.data; const client = await clientsStore.getClient(client_id); if (!client) { throw new InvalidClientError("Invalid client_id"); @@ -49,11 +46,13 @@ export function authenticateClient({ clientsStore }: ClientAuthenticationMiddlew req.client = client; next(); } catch (error) { - if (error instanceof InvalidRequestError || error instanceof InvalidClientError) { - res.status(400).json(error.toResponseObject()); + if (error instanceof OAuthError) { + const status = error instanceof ServerError ? 500 : 400; + res.status(status).json(error.toResponseObject()); } else { console.error("Unexpected error authenticating client:", error); - res.status(500).end("Internal Server Error"); + const serverError = new ServerError("Internal Server Error"); + res.status(500).json(serverError.toResponseObject()); } } }