diff --git a/lib/http.mts b/lib/http.mts index f654bc2e..0d7414ad 100644 --- a/lib/http.mts +++ b/lib/http.mts @@ -62,6 +62,23 @@ export function assertSafeHttpUrl( return url } +// Assemble a `Bearer` WWW-Authenticate value from the leading auth-params, +// then the RFC 9728 resource_metadata URL and the RFC 6750 scope list. +export function buildBearerChallenge( + parts: string[], + resourceMetadataUrl: string | undefined, + scope: string | undefined, +): string { + const params = [...parts] + if (resourceMetadataUrl) { + params.push(`resource_metadata="${resourceMetadataUrl}"`) + } + if (scope) { + params.push(`scope="${scope}"`) + } + return params.length > 0 ? `Bearer ${params.join(', ')}` : 'Bearer' +} + // Build request headers for the JSON REST endpoints (alerts, organizations, // threat-feed, file-list): `accept: application/json` plus optional user-agent, // bearer token, and caller extra headers. Shared so the four data modules @@ -221,6 +238,32 @@ export function writeJson( res.end(JSON.stringify(body)) } +// RFC 6750 §3.1: when a request carries no usable credentials at all — no +// Authorization header, or a scheme other than Bearer — the challenge SHOULD +// NOT carry an error code. The bare `Bearer resource_metadata=…, scope=…` +// form is the MCP authorization spec's "authentication required" signal: +// clients that only start sign-in for a challenge on the connect handshake +// (initialize / tools/list) key on exactly this. The JSON body still names the +// problem for a human reading the response. +export function writeOAuthChallenge( + res: ServerResponse, + message: string, + resourceMetadataUrl?: string | undefined, + scope?: string | undefined, +): void { + writeJson( + res, + 401, + { + error: 'invalid_request', + error_description: message, + }, + { + 'WWW-Authenticate': buildBearerChallenge([], resourceMetadataUrl, scope), + }, + ) +} + // RFC 6750 §3 OAuth error responses: the JSON body carries // error / error_description and the WWW-Authenticate header signals the // realm, the optional resource_metadata URL for discovery, and the optional @@ -235,15 +278,6 @@ export function writeOAuthError( resourceMetadataUrl?: string | undefined, scope?: string | undefined, ): void { - const parts = [`error="${errorCode}"`, `error_description="${message}"`] - if (resourceMetadataUrl) { - parts.push(`resource_metadata="${resourceMetadataUrl}"`) - } - if (scope) { - parts.push(`scope="${scope}"`) - } - const authenticateValue = `Bearer ${parts.join(', ')}` - writeJson( res, statusCode, @@ -251,6 +285,12 @@ export function writeOAuthError( error: errorCode, error_description: message, }, - { 'WWW-Authenticate': authenticateValue }, + { + 'WWW-Authenticate': buildBearerChallenge( + [`error="${errorCode}"`, `error_description="${message}"`], + resourceMetadataUrl, + scope, + ), + }, ) } diff --git a/lib/oauth.mts b/lib/oauth.mts index 880987b9..c3beabf7 100644 --- a/lib/oauth.mts +++ b/lib/oauth.mts @@ -20,6 +20,7 @@ import { getRequestHeaderValue, parseJsonObject, writeJson, + writeOAuthChallenge, writeOAuthError, } from './http.mts' import { logger } from './logger.mts' @@ -81,12 +82,14 @@ export async function authenticateRequest( const resourceIdentifier = getOAuthResourceIdentifier(baseUrl) const scope = buildOAuthScopeParameter(options) + // No credentials, or credentials in a scheme this resource does not take, + // get the bare RFC 6750 §3.1 challenge. This runs for every MCP method — + // initialize and tools/list included — so a client learns it must sign in + // during the connect handshake, not only at its first tools/call. const authHeader = getRequestHeaderValue(req.headers.authorization).trim() if (!authHeader) { - writeOAuthError( + writeOAuthChallenge( res, - 401, - 'invalid_request', 'Missing Authorization header', resourceMetadataUrl, scope, @@ -97,7 +100,16 @@ export async function authenticateRequest( // `authHeader` is trimmed and non-empty, so splitting on whitespace always // yields a non-empty first element. const { 0: type, 1: token } = authHeader.split(/\s+/u) - if (type!.toLowerCase() !== 'bearer' || !token) { + if (type!.toLowerCase() !== 'bearer') { + writeOAuthChallenge( + res, + "Invalid Authorization header format, expected 'Bearer TOKEN'", + resourceMetadataUrl, + scope, + ) + return { ok: false } + } + if (!token) { writeOAuthError( res, 401, diff --git a/test/repo/integration/http-replicas.test.mts b/test/repo/integration/http-replicas.test.mts index 1cc908d6..ae8046a2 100644 --- a/test/repo/integration/http-replicas.test.mts +++ b/test/repo/integration/http-replicas.test.mts @@ -114,7 +114,10 @@ function mockDepscoreUpstream(token: string): nock.Scope { ) } +const RESOURCE_SCOPES = 'packages:list alerts:list threat-feed:list' + function stubOAuthEnv(): void { + vi.stubEnv('SOCKET_OAUTH_REQUIRED_SCOPES', RESOURCE_SCOPES) vi.stubEnv('SOCKET_OAUTH_ISSUER', ISSUER) vi.stubEnv('SOCKET_OAUTH_INTROSPECTION_CLIENT_ID', 'introspection-client') vi.stubEnv('SOCKET_OAUTH_INTROSPECTION_CLIENT_SECRET', 'introspection-secret') @@ -194,18 +197,42 @@ describe('the OAuth resource-server flow across replicas', () => { const init = await post(replica, initializeBody()) expect(init.status).toBe(401) - const challenge = String(init.headers['www-authenticate']) const metadataUrl = `${replica.endpoint}.well-known/oauth-protected-resource` - expect(challenge).toContain(`resource_metadata="${metadataUrl}"`) + // The bare RFC 6750 §3.1 form on the connect handshake: no error code, + // just where to discover the authorization server and the resource's own + // scopes, so a client never falls back to the issuer's full scope list. + expect(init.headers['www-authenticate']).toBe( + `Bearer resource_metadata="${metadataUrl}", scope="${RESOURCE_SCOPES}"`, + ) const metadata = await httpRequest(metadataUrl) expect(metadata.status).toBe(200) expect(metadata.json()).toMatchObject({ resource: replica.endpoint, authorization_servers: [ISSUER], + scopes_supported: RESOURCE_SCOPES.split(' '), }) }) + test('an anonymous tools/list is challenged the same way', async () => { + stubOAuthEnv() + vi.resetModules() + mockIssuer() + const { setOauthEnabled } = await import('../../../lib/oauth.mts') + setOauthEnabled() + const replica = await startReplica() + + const list = await post(replica, { + jsonrpc: '2.0', + id: 3, + method: 'tools/list', + }) + expect(list.status).toBe(401) + expect(list.headers['www-authenticate']).toBe( + `Bearer resource_metadata="${replica.endpoint}.well-known/oauth-protected-resource", scope="${RESOURCE_SCOPES}"`, + ) + }) + test('an anonymous depscore call is challenged, not scored', async () => { stubOAuthEnv() vi.resetModules() @@ -237,7 +264,7 @@ describe('the OAuth resource-server flow across replicas', () => { active: true, aud: [first.endpoint, second.endpoint], client_id: 'chat-client', - scope: 'packages:list', + scope: RESOURCE_SCOPES, }) const upstream = mockDepscoreUpstream('oauth-access-token') const auth = { authorization: 'Bearer oauth-access-token' } diff --git a/test/repo/unit/http.test.mts b/test/repo/unit/http.test.mts index 68f257e6..94463155 100644 --- a/test/repo/unit/http.test.mts +++ b/test/repo/unit/http.test.mts @@ -12,6 +12,7 @@ import { getRequestHeaderValue, parseJsonObject, writeJson, + writeOAuthChallenge, writeOAuthError, } from '../../../lib/http.mts' @@ -296,3 +297,29 @@ describe('getRequestBaseUrl forwarded-host hardening', () => { } }) }) + +describe('writeOAuthChallenge', () => { + test('emits a bare RFC 6750 §3.1 challenge with no error code', () => { + const { res, calls } = fakeResponse() + writeOAuthChallenge( + res, + 'Missing Authorization header', + 'https://mcp.socket.dev/.well-known/oauth-protected-resource', + 'packages:list alerts:list threat-feed:list', + ) + expect(calls.statusCode).toBe(401) + expect(calls.headers!['WWW-Authenticate']).toBe( + 'Bearer resource_metadata="https://mcp.socket.dev/.well-known/oauth-protected-resource", scope="packages:list alerts:list threat-feed:list"', + ) + expect(JSON.parse(calls.body!)).toEqual({ + error: 'invalid_request', + error_description: 'Missing Authorization header', + }) + }) + + test('omits empty resource_metadata and scope', () => { + const { res, calls } = fakeResponse() + writeOAuthChallenge(res, 'Missing Authorization header') + expect(calls.headers!['WWW-Authenticate']).toBe('Bearer') + }) +}) diff --git a/test/repo/unit/oauth-fixtures.mts b/test/repo/unit/oauth-fixtures.mts index 60de2283..c22d98fd 100644 --- a/test/repo/unit/oauth-fixtures.mts +++ b/test/repo/unit/oauth-fixtures.mts @@ -142,6 +142,28 @@ export function assertOAuthError( ) } +// RFC 6750 §3.1: a request with no usable credentials gets a challenge with +// no error code — only where to discover the authorization server and which +// scopes to ask for. The body still names the problem. +export function assertOAuthChallenge( + captured: CapturedResponse, + metadataUrl: string, + errorDescription: string, +): void { + // oxlint-disable-next-line typescript/no-unsafe-type-assertion -- test double / fixture cast: the mock provides only the members the code under test touches. + const body = JSON.parse(captured.getBody()) as { + error?: string | undefined + error_description?: string | undefined + } + assert.equal(captured.getStatus(), 401) + assert.equal(body.error, 'invalid_request') + assert.equal(body.error_description, errorDescription) + assert.equal( + captured.getHeaders()['WWW-Authenticate'], + `Bearer resource_metadata="${metadataUrl}", scope="packages:list"`, + ) +} + // Build an enabled OAuthConfig pointed at the nock-mocked issuer. Each // call gets a fresh config so the per-config discovery cache is isolated. export function makeConfig(overrides: Partial = {}): OAuthConfig { diff --git a/test/repo/unit/oauth.test.mts b/test/repo/unit/oauth.test.mts index f84956f7..caff8015 100644 --- a/test/repo/unit/oauth.test.mts +++ b/test/repo/unit/oauth.test.mts @@ -11,6 +11,7 @@ import { verifyAccessToken, } from '../../../lib/oauth.mts' import { + assertOAuthChallenge, assertOAuthError, issuerBaseUrl, makeConfig, @@ -240,7 +241,7 @@ test('setOauthEnabled refuses a partial OAuth config', async () => { } }) -test('authenticateRequest rejects a missing Authorization header', async () => { +test('authenticateRequest answers a missing Authorization header with a bare challenge', async () => { const captured = makeMockResponse() const result = await authenticateRequest( makeRequest(), @@ -249,14 +250,14 @@ test('authenticateRequest rejects a missing Authorization header', async () => { makeConfig(), ) expect(result.ok).toBe(false) - assertOAuthError(captured, resourceMetadataUrl, { - status: 401, - error: 'invalid_request', - errorDescription: 'Missing Authorization header', - }) + assertOAuthChallenge( + captured, + resourceMetadataUrl, + 'Missing Authorization header', + ) }) -test('authenticateRequest rejects a malformed Authorization header', async () => { +test('authenticateRequest answers a non-Bearer scheme with a bare challenge', async () => { const captured = makeMockResponse() const result = await authenticateRequest( makeRequest('Basic abc123'), @@ -265,6 +266,22 @@ test('authenticateRequest rejects a malformed Authorization header', async () => makeConfig(), ) expect(result.ok).toBe(false) + assertOAuthChallenge( + captured, + resourceMetadataUrl, + "Invalid Authorization header format, expected 'Bearer TOKEN'", + ) +}) + +test('authenticateRequest rejects a Bearer scheme with no token as invalid_request', async () => { + const captured = makeMockResponse() + const result = await authenticateRequest( + makeRequest('Bearer'), + captured.res, + resourceBaseUrl, + makeConfig(), + ) + expect(result.ok).toBe(false) assertOAuthError(captured, resourceMetadataUrl, { status: 401, error: 'invalid_request', @@ -273,6 +290,77 @@ test('authenticateRequest rejects a malformed Authorization header', async () => }) }) +test('authenticateRequest matches the Bearer scheme case-insensitively', async () => { + // A lowercase scheme with no token is still a malformed Bearer request, not + // an unsupported scheme, so it keeps its error code. + for (const header of ['bearer', 'BEARER ']) { + const captured = makeMockResponse() + const result = await authenticateRequest( + makeRequest(header), + captured.res, + resourceBaseUrl, + makeConfig(), + ) + expect(result.ok).toBe(false) + assertOAuthError(captured, resourceMetadataUrl, { + status: 401, + error: 'invalid_request', + errorDescription: + "Invalid Authorization header format, expected 'Bearer TOKEN'", + }) + } + + // A lowercase scheme with a token reaches introspection. + mockDiscovery() + mockIntrospection() + const captured = makeMockResponse() + const result = await authenticateRequest( + makeRequest('bearer inactive-token'), + captured.res, + resourceBaseUrl, + makeConfig(), + ) + expect(result.ok).toBe(false) + assertOAuthError(captured, resourceMetadataUrl, { + status: 401, + error: 'invalid_token', + errorDescription: 'Invalid or expired token', + }) +}) + +test('the challenge scope is the resource scopes, never offline_access', async () => { + const captured = makeMockResponse() + await authenticateRequest( + makeRequest(), + captured.res, + resourceBaseUrl, + makeConfig({ + requiredScopes: [ + 'packages:list', + 'alerts:list', + 'threat-feed:list', + 'offline_access', + ], + }), + ) + expect(captured.getHeaders()['WWW-Authenticate']).toBe( + `Bearer resource_metadata="${resourceMetadataUrl}", scope="packages:list alerts:list threat-feed:list"`, + ) +}) + +test('the challenge omits scope when the resource enforces none', async () => { + const captured = makeMockResponse() + await authenticateRequest( + makeRequest(), + captured.res, + resourceBaseUrl, + makeConfig({ requiredScopes: [] }), + ) + expect(captured.getHeaders()['WWW-Authenticate']).toBe( + `Bearer resource_metadata="${resourceMetadataUrl}"`, + ) +}) + test('authenticateRequest returns invalid_token for an inactive token', async () => { mockDiscovery() mockIntrospection()