diff --git a/.env.example b/.env.example index 1ef7acbb..5f436c6c 100644 --- a/.env.example +++ b/.env.example @@ -82,6 +82,11 @@ ALLOW_OPEN_REGISTRATION=false # oauth2 — OAuth 2.0 Authorization Code (PKCE) + Client Credentials # both — OAuth 2.0 + Legacy (accepts either) MCP_AUTH_MODE=oauth2 +# PKCE (S256) is required on /authorize. Platforms that cannot send it, such as +# Microsoft Copilot Studio connectors, can be let through by client id. Only +# confidential clients (registered with client_secret_basic/post and a secret) +# are exempted; a listed public client still has to use PKCE. +# OAUTH_PKCE_EXEMPT_CLIENT_IDS= # Legacy auth tokens (used when MCP_AUTH_MODE=legacy or both) # MCP_BEARER_TOKEN=your-secret-token diff --git a/docker-compose.cloud.yml b/docker-compose.cloud.yml index 9a22758d..77678591 100644 --- a/docker-compose.cloud.yml +++ b/docker-compose.cloud.yml @@ -109,6 +109,11 @@ x-backend: &backend - SERVER_URL=https://${DOMAIN} - FRONTEND_URL=https://${DOMAIN} - MCP_AUTH_MODE=${MCP_AUTH_MODE:-oauth2} + # OAuth clients allowed to authorize without PKCE (comma-separated ids). + # Only for confidential clients registered with a secret, such as the + # Microsoft Copilot Studio connector, whose platform cannot send PKCE. + # Unset = PKCE required for every client. + - OAUTH_PKCE_EXEMPT_CLIENT_IDS=${OAUTH_PKCE_EXEMPT_CLIENT_IDS:-} # MCP Streamable HTTP response framing. `true` = single-shot # `application/json` responses; `false` = SSE-framed `text/event-stream`. # Microsoft Copilot Studio's MCP client cannot deserialize SSE-framed diff --git a/packages/backend/src/auth/authorize-pkce.middleware.spec.ts b/packages/backend/src/auth/authorize-pkce.middleware.spec.ts index 9c704bb1..d0c6764e 100644 --- a/packages/backend/src/auth/authorize-pkce.middleware.spec.ts +++ b/packages/backend/src/auth/authorize-pkce.middleware.spec.ts @@ -25,10 +25,10 @@ describe('AuthorizePkceMiddleware', () => { const bodyOf = () => res.json.mock.calls[0][0]; - it('allows a conforming S256 authorization request', () => { + it('allows a conforming S256 authorization request', async () => { const challenge = s256(randomBytes(32).toString('base64url')); - middleware.use( + await middleware.use( req({ client_id: 'c1', code_challenge: challenge, code_challenge_method: 'S256' }), res, next, @@ -38,8 +38,8 @@ describe('AuthorizePkceMiddleware', () => { expect(res.status).not.toHaveBeenCalled(); }); - it('rejects a request with no code_challenge', () => { - middleware.use(req({ client_id: 'c1' }), res, next); + it('rejects a request with no code_challenge', async () => { + await middleware.use(req({ client_id: 'c1' }), res, next); expect(next).not.toHaveBeenCalled(); expect(res.status).toHaveBeenCalledWith(400); @@ -47,11 +47,11 @@ describe('AuthorizePkceMiddleware', () => { expect(bodyOf().error_description).toMatch(/code_challenge is required/); }); - it("rejects code_challenge_method 'plain'", () => { + it("rejects code_challenge_method 'plain'", async () => { // `plain` puts the verifier in the clear on the authorization request, so // it defends against nothing an attacker who can read the request cannot // already do. Upstream defaults to exactly this when the method is absent. - middleware.use( + await middleware.use( req({ code_challenge: s256('v'), code_challenge_method: 'plain' }), res, next, @@ -61,15 +61,15 @@ describe('AuthorizePkceMiddleware', () => { expect(bodyOf().error_description).toMatch(/S256/); }); - it('rejects a challenge with no method (upstream would assume plain)', () => { - middleware.use(req({ code_challenge: s256('v') }), res, next); + it('rejects a challenge with no method (upstream would assume plain)', async () => { + await middleware.use(req({ code_challenge: s256('v') }), res, next); expect(next).not.toHaveBeenCalled(); expect(res.status).toHaveBeenCalledWith(400); }); - it('rejects a malformed challenge', () => { - middleware.use( + it('rejects a malformed challenge', async () => { + await middleware.use( req({ code_challenge: 'too-short', code_challenge_method: 'S256' }), res, next, @@ -79,10 +79,10 @@ describe('AuthorizePkceMiddleware', () => { expect(bodyOf().error_description).toMatch(/base64url/); }); - it('never redirects to a client-supplied redirect_uri', () => { + it('never redirects to a client-supplied redirect_uri', async () => { // Redirecting on error would mean trusting an unvalidated client URI, // which is how an authorization endpoint becomes an open redirector. - middleware.use( + await middleware.use( req({ client_id: 'c1', redirect_uri: 'https://evil.tld/cb' }), res, next, @@ -93,8 +93,106 @@ describe('AuthorizePkceMiddleware', () => { expect(JSON.stringify(bodyOf())).not.toContain('evil.tld'); }); - it('leaves non-GET requests alone', () => { - middleware.use(req({}, 'POST'), res, next); + it('leaves non-GET requests alone', async () => { + await middleware.use(req({}, 'POST'), res, next); expect(next).toHaveBeenCalled(); }); + + describe('OAUTH_PKCE_EXEMPT_CLIENT_IDS', () => { + const original = process.env.OAUTH_PKCE_EXEMPT_CLIENT_IDS; + let findUnique: jest.Mock; + + const withClient = (client: Record | null) => { + findUnique = jest.fn().mockResolvedValue(client); + middleware = new AuthorizePkceMiddleware({ + oAuthClient: { findUnique }, + } as any); + }; + + const confidential = { + tokenEndpointAuthMethod: 'client_secret_post', + clientSecret: 's3cret', + }; + + beforeEach(() => { + process.env.OAUTH_PKCE_EXEMPT_CLIENT_IDS = 'copilot-studio, other'; + }); + afterAll(() => { + if (original === undefined) delete process.env.OAUTH_PKCE_EXEMPT_CLIENT_IDS; + else process.env.OAUTH_PKCE_EXEMPT_CLIENT_IDS = original; + }); + + it('lets a listed confidential client authorize without PKCE', async () => { + withClient(confidential); + await middleware.use(req({ client_id: 'copilot-studio' }), res, next); + + expect(next).toHaveBeenCalled(); + expect(res.status).not.toHaveBeenCalled(); + expect(findUnique).toHaveBeenCalledWith( + expect.objectContaining({ where: { clientId: 'copilot-studio' } }), + ); + }); + + it('accepts client_secret_basic too', async () => { + withClient({ ...confidential, tokenEndpointAuthMethod: 'client_secret_basic' }); + await middleware.use(req({ client_id: 'other' }), res, next); + expect(next).toHaveBeenCalled(); + }); + + it('keeps PKCE required for a listed public client', async () => { + // A public client has no secret, so nothing would stand in for PKCE. + withClient({ tokenEndpointAuthMethod: 'none', clientSecret: null }); + await middleware.use(req({ client_id: 'copilot-studio' }), res, next); + + expect(next).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(400); + }); + + it('keeps PKCE required for a listed client with no stored secret', async () => { + withClient({ tokenEndpointAuthMethod: 'client_secret_post', clientSecret: null }); + await middleware.use(req({ client_id: 'copilot-studio' }), res, next); + expect(res.status).toHaveBeenCalledWith(400); + }); + + it('keeps PKCE required for a listed id that is not registered', async () => { + withClient(null); + await middleware.use(req({ client_id: 'copilot-studio' }), res, next); + expect(res.status).toHaveBeenCalledWith(400); + }); + + it('never looks up clients that are not listed', async () => { + withClient(confidential); + await middleware.use(req({ client_id: 'some-dcr-client' }), res, next); + + expect(findUnique).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(400); + }); + + it('still validates a challenge a listed client chooses to send', async () => { + withClient(confidential); + await middleware.use( + req({ client_id: 'copilot-studio', code_challenge: 'x', code_challenge_method: 'plain' }), + res, + next, + ); + expect(res.status).toHaveBeenCalledWith(400); + }); + + it('does not exempt a request that names a method without a challenge', async () => { + withClient(confidential); + await middleware.use( + req({ client_id: 'copilot-studio', code_challenge_method: 'plain' }), + res, + next, + ); + expect(res.status).toHaveBeenCalledWith(400); + }); + + it('fails closed when the lookup throws', async () => { + findUnique = jest.fn().mockRejectedValue(new Error('db down')); + middleware = new AuthorizePkceMiddleware({ oAuthClient: { findUnique } } as any); + await middleware.use(req({ client_id: 'copilot-studio' }), res, next); + expect(res.status).toHaveBeenCalledWith(400); + }); + }); }); diff --git a/packages/backend/src/auth/authorize-pkce.middleware.ts b/packages/backend/src/auth/authorize-pkce.middleware.ts index eeae910a..138cd68b 100644 --- a/packages/backend/src/auth/authorize-pkce.middleware.ts +++ b/packages/backend/src/auth/authorize-pkce.middleware.ts @@ -1,5 +1,9 @@ -import { Injectable, Logger, NestMiddleware } from '@nestjs/common'; +import { Injectable, Logger, NestMiddleware, Optional } from '@nestjs/common'; import type { Request, Response, NextFunction } from 'express'; +import { PrismaService } from '../common/prisma.service'; + +/** Token-endpoint auth methods that prove possession of a client secret. */ +const SECRET_AUTH_METHODS = new Set(['client_secret_basic', 'client_secret_post']); /** * Enforces PKCE with S256 on GET /authorize. @@ -24,12 +28,25 @@ import type { Request, Response, NextFunction } from 'express'; * `redirect_uri`. Redirecting would require trusting a client-supplied URI * before it has been validated against a registration, which is exactly how an * authorization endpoint becomes an open redirector. + * + * ONE EXCEPTION. Some platforms cannot send PKCE at all: Power Platform + * connectors (Microsoft Copilot Studio) run a plain OAuth 2.0 authorization + * code flow with a client secret. An operator can list such clients in + * `OAUTH_PKCE_EXEMPT_CLIENT_IDS`; a listed client may omit the challenge only + * while its registration is confidential (client_secret_basic/post with a + * stored secret). The token endpoint then refuses the code without that + * secret, which is what PKCE would otherwise protect, and the code is still + * bound to the client's registered redirect URI. A request that does carry a + * challenge is validated as usual, and a client registered through open DCR is + * never exempt unless the operator names it. */ @Injectable() export class AuthorizePkceMiddleware implements NestMiddleware { private readonly logger = new Logger(AuthorizePkceMiddleware.name); - use(req: Request, res: Response, next: NextFunction): void { + constructor(@Optional() private readonly prisma?: PrismaService) {} + + async use(req: Request, res: Response, next: NextFunction): Promise { // Only the authorization request itself carries PKCE parameters. if (req.method !== 'GET') { return next(); @@ -40,6 +57,12 @@ export class AuthorizePkceMiddleware implements NestMiddleware { const method = query.code_challenge_method; if (typeof challenge !== 'string' || challenge.trim() === '') { + if ( + method === undefined && + (await this.isExemptConfidentialClient(query.client_id)) + ) { + return next(); + } this.logger.warn( `Rejecting /authorize without PKCE (client_id=${String(query.client_id ?? '')})`, ); @@ -72,6 +95,43 @@ export class AuthorizePkceMiddleware implements NestMiddleware { next(); } + /** + * True only for a client the operator listed in OAUTH_PKCE_EXEMPT_CLIENT_IDS + * whose registration requires a client secret at the token endpoint. The + * database is consulted only for listed ids, so ordinary requests never pay + * for a lookup. + */ + private async isExemptConfidentialClient(clientId: unknown): Promise { + if (typeof clientId !== 'string' || !clientId) return false; + const listed = (process.env.OAUTH_PKCE_EXEMPT_CLIENT_IDS ?? '') + .split(',') + .map((id) => id.trim()) + .filter(Boolean); + if (!listed.includes(clientId) || !this.prisma) return false; + + try { + const client = await this.prisma.oAuthClient.findUnique({ + where: { clientId }, + select: { tokenEndpointAuthMethod: true, clientSecret: true }, + }); + if ( + client && + SECRET_AUTH_METHODS.has(client.tokenEndpointAuthMethod) && + !!client.clientSecret + ) { + return true; + } + this.logger.warn( + `Client ${clientId} is listed in OAUTH_PKCE_EXEMPT_CLIENT_IDS but is not a confidential client; PKCE stays required`, + ); + } catch (err) { + this.logger.warn( + `PKCE exemption lookup failed for ${clientId}: ${(err as Error).message}`, + ); + } + return false; + } + private reject(res: Response, description: string): void { res .status(400)