From aada71bf303377b9b70828aa9e6028495443c9be Mon Sep 17 00:00:00 2001 From: Olivier Chafik Date: Fri, 5 Jun 2026 20:22:04 +0100 Subject: [PATCH] lazy-auth: address review nits - Build the well-known rewrite regex from LAZY_AUTH_SLUG so it can't drift from the mount path if the slug changes. - Clarify in the docstring that an explicit PUBLIC_URL must include the /lazy-auth mount path (the package advertises it verbatim). - Tests: clear/restore ambient PUBLIC_URL around the integration server (was a latent flake); add unit coverage for the PUBLIC_URL derivation (trailing-slash strip, no-trailing-slash, explicit-wins, unset cases); add a public tool-call success case and an invalid-token 401 case. --- .../lazy-auth.integration.test.ts | 56 +++++++++++++++++++ src/modules/example-apps/lazy-auth.ts | 29 ++++++---- 2 files changed, 73 insertions(+), 12 deletions(-) diff --git a/src/modules/example-apps/lazy-auth.integration.test.ts b/src/modules/example-apps/lazy-auth.integration.test.ts index eb0ae1c..879f18e 100644 --- a/src/modules/example-apps/lazy-auth.integration.test.ts +++ b/src/modules/example-apps/lazy-auth.integration.test.ts @@ -51,8 +51,15 @@ function callTool(base: string, name: string, accessToken?: string): Promise { let server: http.Server; let base: string; + let savedPublicUrl: string | undefined; beforeAll(async () => { + // These tests rely on the package's per-request Host resolution (no baseUri + // passed below), so an ambient PUBLIC_URL would pin advertised URLs and + // break the assertions. Clear it for the duration and restore afterwards. + savedPublicUrl = process.env.PUBLIC_URL; + delete process.env.PUBLIC_URL; + const app = express(); // Mirror src/index.ts: the example mounts before the host's middleware // and routes. @@ -75,6 +82,8 @@ describe('Lazy Auth example mount', () => { afterAll(async () => { await new Promise((resolve) => server.close(resolve)); + if (savedPublicUrl === undefined) delete process.env.PUBLIC_URL; + else process.env.PUBLIC_URL = savedPublicUrl; }); it('serves public MCP requests without auth', async () => { @@ -95,12 +104,24 @@ describe('Lazy Auth example mount', () => { expect(res.body).toContain('Lazy Auth'); }); + it('serves a public tool call without auth', async () => { + const res = await callTool(base, 'show_auth_button'); + expect(res.status).toBe(200); + expect(res.headers['www-authenticate']).toBeUndefined(); + }); + it('answers 401 with resource_metadata under the mount path for protected tools', async () => { const res = await callTool(base, 'get_secret'); expect(res.status).toBe(401); expect(res.headers['www-authenticate']).toContain(`resource_metadata="${base}/lazy-auth/auth/prm"`); }); + it('answers 401 with invalid_token for a bad bearer token', async () => { + const res = await callTool(base, 'get_secret', 'not-a-real-token'); + expect(res.status).toBe(401); + expect(res.headers['www-authenticate']).toContain('invalid_token'); + }); + it('advertises mount-prefixed URLs in PRM and AS metadata', async () => { const prm = JSON.parse((await request(`${base}/lazy-auth/auth/prm`)).body); expect(prm.resource).toBe(`${base}/lazy-auth/mcp`); @@ -172,3 +193,38 @@ describe('Lazy Auth example mount', () => { expect(secretRes.body).toContain('the-answer-is-42'); }); }); + +describe('mountLazyAuthExample PUBLIC_URL derivation', () => { + let savedPublicUrl: string | undefined; + + beforeEach(() => { + savedPublicUrl = process.env.PUBLIC_URL; + delete process.env.PUBLIC_URL; + }); + + afterEach(() => { + if (savedPublicUrl === undefined) delete process.env.PUBLIC_URL; + else process.env.PUBLIC_URL = savedPublicUrl; + }); + + it('derives PUBLIC_URL from baseUri, stripping trailing slashes and appending the mount path', () => { + mountLazyAuthExample(express(), 'https://example.test/'); + expect(process.env.PUBLIC_URL).toBe('https://example.test/lazy-auth'); + }); + + it('handles a baseUri with no trailing slash', () => { + mountLazyAuthExample(express(), 'https://example.test'); + expect(process.env.PUBLIC_URL).toBe('https://example.test/lazy-auth'); + }); + + it('does not override an explicitly set PUBLIC_URL', () => { + process.env.PUBLIC_URL = 'https://tunnel.example/lazy-auth'; + mountLazyAuthExample(express(), 'https://example.test/'); + expect(process.env.PUBLIC_URL).toBe('https://tunnel.example/lazy-auth'); + }); + + it('leaves PUBLIC_URL unset when no baseUri is provided', () => { + mountLazyAuthExample(express()); + expect(process.env.PUBLIC_URL).toBeUndefined(); + }); +}); diff --git a/src/modules/example-apps/lazy-auth.ts b/src/modules/example-apps/lazy-auth.ts index b5010ca..e5aa393 100644 --- a/src/modules/example-apps/lazy-auth.ts +++ b/src/modules/example-apps/lazy-auth.ts @@ -17,30 +17,35 @@ * them from PUBLIC_URL, falling back to the request Host header for loopback * hosts only. We derive PUBLIC_URL from this server's own public base URI - * already the source of truth for the host's OAuth issuer - so deployments - * need no separate env var. An explicit PUBLIC_URL still wins (e.g. a tunnel); - * omitting baseUri (e.g. tests on an ephemeral port) keeps the package's - * per-request Host resolution. + * need no separate env var. An explicit PUBLIC_URL still wins (e.g. a tunnel) - + * note it must include the /lazy-auth mount path, since the package advertises + * it verbatim. Omitting baseUri (e.g. tests on an ephemeral port) keeps the + * package's per-request Host resolution. */ import { Express, Request, Response, NextFunction } from 'express'; import { createApp as createLazyAuthApp } from '@modelcontextprotocol/server-lazy-auth'; export const LAZY_AUTH_SLUG = 'lazy-auth'; +// RFC 8414/9728 place well-known discovery documents at the origin root, with +// the resource path inserted after the well-known prefix +// (e.g. /.well-known/oauth-authorization-server/lazy-auth). Built from +// LAZY_AUTH_SLUG so it can't drift from the mount path. +const WELL_KNOWN_INSERTION = new RegExp( + `^/\\.well-known/(oauth-authorization-server|oauth-protected-resource)/${LAZY_AUTH_SLUG}(/.*)?$` +); + export function mountLazyAuthExample(app: Express, baseUri?: string): void { if (baseUri && !process.env.PUBLIC_URL) { process.env.PUBLIC_URL = `${baseUri.replace(/\/+$/, '')}/${LAZY_AUTH_SLUG}`; } - // RFC 8414/9728 place well-known discovery documents at the origin root, - // with the resource path inserted after the well-known prefix - // (e.g. /.well-known/oauth-authorization-server/lazy-auth), and MCP SDK - // clients only try that insertion form. Rewrite those root paths into the - // mount - rather than dispatching to the sub-app directly - so req.baseUrl - // (and therefore every URL the example advertises) stays consistent. + // MCP SDK clients only try the path-insertion form above for discovery. + // Rewrite those root paths into the mount - rather than dispatching to the + // sub-app directly - so req.baseUrl (and therefore every URL the example + // advertises) stays consistent. app.use((req: Request, _res: Response, next: NextFunction) => { - const wellKnown = req.url.match( - /^\/\.well-known\/(oauth-authorization-server|oauth-protected-resource)\/lazy-auth(\/.*)?$/ - ); + const wellKnown = req.url.match(WELL_KNOWN_INSERTION); if (wellKnown) { req.url = `/${LAZY_AUTH_SLUG}/.well-known/${wellKnown[1]}${wellKnown[2] ?? ''}`; }