From b90192a020375f4c5b09ea6114bebfed9b2c3dba Mon Sep 17 00:00:00 2001 From: Allen Zhou <46854522+allenzhou101@users.noreply.github.com> Date: Fri, 28 Feb 2025 13:59:23 -0800 Subject: [PATCH] Add skipLocalPkceValidation param and create providers folder --- src/server/auth/handlers/token.test.ts | 2 +- src/server/auth/handlers/token.ts | 6 +++--- src/server/auth/provider.ts | 9 +++++++++ .../{ => providers}/proxyProvider.test.ts | 10 +++++----- .../auth/{ => providers}/proxyProvider.ts | 20 ++++++------------- 5 files changed, 24 insertions(+), 23 deletions(-) rename src/server/auth/{ => providers}/proxyProvider.test.ts (96%) rename src/server/auth/{ => providers}/proxyProvider.ts (89%) diff --git a/src/server/auth/handlers/token.test.ts b/src/server/auth/handlers/token.test.ts index 80997857..bf41b5eb 100644 --- a/src/server/auth/handlers/token.test.ts +++ b/src/server/auth/handlers/token.test.ts @@ -7,7 +7,7 @@ import supertest from 'supertest'; import * as pkceChallenge from 'pkce-challenge'; import { InvalidGrantError, InvalidTokenError } from '../errors.js'; import { AuthInfo } from '../types.js'; -import { ProxyOAuthServerProvider } from '../proxyProvider.js'; +import { ProxyOAuthServerProvider } from '../providers/proxyProvider.js'; // Mock pkce-challenge jest.mock('pkce-challenge', () => ({ diff --git a/src/server/auth/handlers/token.ts b/src/server/auth/handlers/token.ts index acf2c0ab..28412a01 100644 --- a/src/server/auth/handlers/token.ts +++ b/src/server/auth/handlers/token.ts @@ -14,7 +14,6 @@ import { TooManyRequestsError, OAuthError } from "../errors.js"; -import { ProxyOAuthServerProvider } from "../proxyProvider.js"; export type TokenHandlerOptions = { provider: OAuthServerProvider; @@ -91,9 +90,10 @@ export function tokenHandler({ provider, rateLimit: rateLimitConfig }: TokenHand const { code, code_verifier } = parseResult.data; - const skipLocalPkceValidation = provider instanceof ProxyOAuthServerProvider ? provider.skipLocalPkceValidation : false; + const skipLocalPkceValidation = provider.skipLocalPkceValidation; - // Perform local PKCE validation unless explicitly delegated (e.g. by proxy provider) + // Perform local PKCE validation unless explicitly skipped + // (e.g. to validate code_verifier in upstream server) if (!skipLocalPkceValidation) { const codeChallenge = await provider.challengeForAuthorizationCode(client, code); if (!(await verifyChallenge(code_verifier, codeChallenge))) { diff --git a/src/server/auth/provider.ts b/src/server/auth/provider.ts index aacd308b..dc186bca 100644 --- a/src/server/auth/provider.ts +++ b/src/server/auth/provider.ts @@ -54,4 +54,13 @@ export interface OAuthServerProvider { * If the given token is invalid or already revoked, this method should do nothing. */ revokeToken?(client: OAuthClientInformationFull, request: OAuthTokenRevocationRequest): Promise; + + /** + * Whether to skip local PKCE validation. + * + * If true, the server will not perform PKCE validation locally and will pass the code_verifier to the upstream server. + * + * NOTE: This should only be true if the upstream server is performing the actual PKCE validation. + */ + skipLocalPkceValidation?: boolean; } \ No newline at end of file diff --git a/src/server/auth/proxyProvider.test.ts b/src/server/auth/providers/proxyProvider.test.ts similarity index 96% rename from src/server/auth/proxyProvider.test.ts rename to src/server/auth/providers/proxyProvider.test.ts index 5f38d6e5..6e842ea3 100644 --- a/src/server/auth/proxyProvider.test.ts +++ b/src/server/auth/providers/proxyProvider.test.ts @@ -1,10 +1,10 @@ import { Response } from "express"; import { ProxyOAuthServerProvider, ProxyOptions } from "./proxyProvider.js"; -import { AuthInfo } from "./types.js"; -import { OAuthClientInformationFull, OAuthTokens } from "../../shared/auth.js"; -import { ServerError } from "./errors.js"; -import { InvalidTokenError } from "./errors.js"; -import { InsufficientScopeError } from "./errors.js"; +import { AuthInfo } from "../types.js"; +import { OAuthClientInformationFull, OAuthTokens } from "../../../shared/auth.js"; +import { ServerError } from "../errors.js"; +import { InvalidTokenError } from "../errors.js"; +import { InsufficientScopeError } from "../errors.js"; describe("Proxy OAuth Server Provider", () => { // Mock client data diff --git a/src/server/auth/proxyProvider.ts b/src/server/auth/providers/proxyProvider.ts similarity index 89% rename from src/server/auth/proxyProvider.ts rename to src/server/auth/providers/proxyProvider.ts index 3fbff54b..be450305 100644 --- a/src/server/auth/proxyProvider.ts +++ b/src/server/auth/providers/proxyProvider.ts @@ -1,15 +1,15 @@ import { Response } from "express"; -import { OAuthRegisteredClientsStore } from "./clients.js"; +import { OAuthRegisteredClientsStore } from "../clients.js"; import { OAuthClientInformationFull, OAuthClientInformationFullSchema, OAuthTokenRevocationRequest, OAuthTokens, OAuthTokensSchema, -} from "./../../shared/auth.js"; -import { AuthInfo } from "./types.js"; -import { AuthorizationParams, OAuthServerProvider } from "./provider.js"; -import { ServerError } from "./errors.js"; +} from "../../../shared/auth.js"; +import { AuthInfo } from "../types.js"; +import { AuthorizationParams, OAuthServerProvider } from "../provider.js"; +import { ServerError } from "../errors.js"; export type ProxyEndpoints = { authorizationUrl: string; @@ -44,15 +44,7 @@ export class ProxyOAuthServerProvider implements OAuthServerProvider { protected readonly _verifyAccessToken: (token: string) => Promise; protected readonly _getClient: (clientId: string) => Promise; - /** - * Always true for proxy providers since PKCE validation happens at the upstream server. - * Can consider adding to the base OAuthServerProvider interface if it becomes useful elsewhere. - * This ensures that: - * 1. We skip local PKCE validation (which would fail since we don't store challenges) - * 2. The code_verifier is still passed through to the upstream server - * 3. The upstream server performs the actual PKCE validation - */ - readonly skipLocalPkceValidation = true; + skipLocalPkceValidation = true; revokeToken?: ( client: OAuthClientInformationFull,