diff --git a/packages/spacecat-shared-http-utils/src/auth/auth-wrapper.js b/packages/spacecat-shared-http-utils/src/auth/auth-wrapper.js index 40f5cdc92..b99c5f1c8 100644 --- a/packages/spacecat-shared-http-utils/src/auth/auth-wrapper.js +++ b/packages/spacecat-shared-http-utils/src/auth/auth-wrapper.js @@ -16,20 +16,86 @@ import { isObject } from '@adobe/spacecat-shared-utils'; import AuthenticationManager from './authentication-manager.js'; import { checkScopes } from './check-scopes.js'; +/** + * Route-based entries that bypass authentication. + * + * SECURITY CONTRACT (VULN-39365): an entry here means this library performs NO authentication + * for that route, so the consuming service MUST authenticate it by other means. For + * `POST /slack/events` that means verifying the Slack request signature (`X-Slack-Signature` + + * `X-Slack-Request-Timestamp`) before the payload reaches any handler. spacecat-api-service does + * this in `slackSignatureWrapper`, which is mounted OUTSIDE this wrapper so it runs first. + * + * IMPORTANT: this list is NOT the whole unauthenticated surface. Two further bypasses are + * unconditional clauses in the wrapper below and are not represented here or overridable via + * `anonymousEndpoints`: + * - every `OPTIONS` request (CORS preflight), and + * - any route matching the `POST /hooks/site-detection/` prefix. + * + * Historically this list also contained `GET /slack/events`. That was removed because Slack only + * ever POSTs events and interactive payloads, and a GET carries no body to sign — so a GET could + * never be signature-verified and existed purely as an unauthenticated entry point. + * + * Do NOT add entries here. A service that needs an unauthenticated route should pass its own + * `anonymousEndpoints` (see below) rather than widening the default for every consumer. + */ const ANONYMOUS_ENDPOINTS = [ - 'GET /slack/events', 'POST /slack/events', ]; +// Entries are compared verbatim against `${METHOD.toUpperCase()} ${suffix}`, so they must be an +// upper-case method, a single space, then an absolute path. +const ANONYMOUS_ROUTE_SHAPE = /^[A-Z]+ \/\S*$/; + +/** + * Wraps a function with authentication. + * + * @param {UniversalFunction} fn - the function to wrap. + * @param {object} [opts] - options. + * @param {Array} [opts.authHandlers] - the authentication handler classes to try, in order. + * @param {string[]} [opts.anonymousEndpoints] - overrides the default set of **route-based** + * anonymous entries, as `'METHOD /path'` strings (exact match, method upper-case). + * + * Pass `[]` to remove the route-based entries. Note this does NOT authenticate everything: + * `OPTIONS` requests and `POST /hooks/site-detection/*` bypass authentication + * unconditionally and are not affected by this option. A service that does not verify Slack + * request signatures SHOULD pass `[]`, otherwise it inherits an unauthenticated + * `POST /slack/events` it may not be defending. + * + * Supplying a value that is not an array of strings THROWS at wrapper-construction time + * rather than falling back to the default. This is security-sensitive configuration: a typo + * by a service trying to *disable* the bypass must not silently re-enable it. Entries are + * copied, so mutating the caller's array afterwards cannot widen the bypass. + * @returns {UniversalFunction} the wrapped function. + * @throws {Error} when `opts.anonymousEndpoints` is present but not an array of strings. + */ export function authWrapper(fn, opts = {}) { let authenticationManager; + let anonymousEndpoints = ANONYMOUS_ENDPOINTS; + + if (opts.anonymousEndpoints !== undefined) { + if (!Array.isArray(opts.anonymousEndpoints) + || opts.anonymousEndpoints.some((route) => typeof route !== 'string')) { + throw new Error('authWrapper: anonymousEndpoints must be an array of "METHOD /path" strings'); + } + // Shape-check each entry too. Matching is an exact string compare against + // `${METHOD.toUpperCase()} ${suffix}`, so an entry like 'post /slack/events' or a missing + // leading slash would validate, never match, and silently fail closed -- a debugging trap + // for a service that believes it has allowed a route. Same rationale as the throw above: + // security-sensitive config must not misconfigure quietly. + const malformed = opts.anonymousEndpoints.filter((route) => !ANONYMOUS_ROUTE_SHAPE.test(route)); + if (malformed.length > 0) { + throw new Error(`authWrapper: anonymousEndpoints entries must look like "METHOD /path" with an upper-case method; got: ${malformed.join(', ')}`); + } + // Defensive copy: the caller must not be able to widen the bypass after construction. + anonymousEndpoints = [...opts.anonymousEndpoints]; + } return async (request, context) => { const { log, pathInfo: { method, suffix } } = context; const route = `${method.toUpperCase()} ${suffix}`; - if (ANONYMOUS_ENDPOINTS.includes(route) + if (anonymousEndpoints.includes(route) || route.startsWith('POST /hooks/site-detection/') || method.toUpperCase() === 'OPTIONS') { return fn(request, context); diff --git a/packages/spacecat-shared-http-utils/src/auth/readme.md b/packages/spacecat-shared-http-utils/src/auth/readme.md index cb662198b..999a1830d 100644 --- a/packages/spacecat-shared-http-utils/src/auth/readme.md +++ b/packages/spacecat-shared-http-utils/src/auth/readme.md @@ -50,6 +50,29 @@ export const main = wrap(run) .with(auth, { authHandlers: [LegacyApiKeyHandler, AdobeImsHandler] }); ``` +#### Anonymous routes (`anonymousEndpoints`) + +Some routes bypass authentication. By default that is `POST /slack/events`, which the +consuming service is expected to authenticate by other means — spacecat-api-service verifies +the Slack request signature in a wrapper mounted outside this one (VULN-39365). + +A service that does **not** verify Slack request signatures should opt out explicitly, so it +does not inherit an unauthenticated route it is not defending: + +```javascript +export const main = wrap(run) + .with(auth, { authHandlers: [...], anonymousEndpoints: [] }); +``` + +Two caveats: + +- `anonymousEndpoints` governs only the **route-based** list (exact `'METHOD /path'` matches). + `OPTIONS` requests and `POST /hooks/site-detection/*` bypass authentication unconditionally + and are **not** affected by this option — `[]` does not mean "authenticate everything". +- Passing anything other than an array of strings **throws at construction** rather than + falling back to the default, so a typo while locking things down cannot silently re-open the + bypass. + ### Implementing an Authentication Handler To implement a new authentication handler, extend the `AbstractHandler` class and implement the `checkAuth` method. diff --git a/packages/spacecat-shared-http-utils/test/auth/auth-wrapper.test.js b/packages/spacecat-shared-http-utils/test/auth/auth-wrapper.test.js index 26bc3fdec..3a00b2dd8 100644 --- a/packages/spacecat-shared-http-utils/test/auth/auth-wrapper.test.js +++ b/packages/spacecat-shared-http-utils/test/auth/auth-wrapper.test.js @@ -76,14 +76,125 @@ describe('auth wrapper', () => { }); it('passes anonymous route', async () => { + // Slack only ever POSTs; GET /slack/events is no longer anonymous (VULN-39365). context.pathInfo.suffix = '/slack/events'; - const resp = await action(new Request('https://space.cat/slack/events'), context); + const resp = await action(new Request('https://space.cat/slack/events', { method: 'POST' }), context); expect(resp).to.equal(42); expect(context.attributes.authInfo).to.be.undefined; }); + it('does NOT treat GET /slack/events as anonymous (VULN-39365)', async () => { + // A GET carries no body to sign, so it could never be Slack-signature-verified. It must + // fall through to the authentication manager, which rejects it. + context.pathInfo.suffix = '/slack/events'; + + const resp = await action(new Request('https://space.cat/slack/events'), context); + + expect(resp.status).to.equal(401); + expect(context.attributes.authInfo).to.be.undefined; + }); + + it('honours an anonymousEndpoints override that disables the bypass', async () => { + const locked = wrap(() => 42) + .with(authWrapper, { authHandlers: [DummyHandler], anonymousEndpoints: [] }) + .with(enrichPathInfo); + context.pathInfo.suffix = '/slack/events'; + + const resp = await locked(new Request('https://space.cat/slack/events', { method: 'POST' }), context); + + expect(resp.status).to.equal(401); + }); + + // The option governs ONLY the route-based list. These pin the two unconditional bypasses it + // does not reach, so the documented contract cannot silently drift from the behaviour. + it('anonymousEndpoints: [] does NOT authenticate OPTIONS requests', async () => { + const locked = wrap(() => 42) + .with(authWrapper, { authHandlers: [DummyHandler], anonymousEndpoints: [] }) + .with(enrichPathInfo); + context.pathInfo.suffix = '/sites'; + + const resp = await locked(new Request('https://space.cat/sites', { method: 'OPTIONS' }), context); + + expect(resp).to.equal(42); + }); + + it('anonymousEndpoints: [] does NOT authenticate POST /hooks/site-detection/*', async () => { + const locked = wrap(() => 42) + .with(authWrapper, { authHandlers: [DummyHandler], anonymousEndpoints: [] }) + .with(enrichPathInfo); + context.pathInfo.suffix = '/hooks/site-detection/cdn/some-secret'; + + const resp = await locked( + new Request('https://space.cat/hooks/site-detection/cdn/some-secret', { method: 'POST' }), + context, + ); + + expect(resp).to.equal(42); + }); + + it('a non-empty override drops the default POST /slack/events', async () => { + const custom = wrap(() => 42) + .with(authWrapper, { authHandlers: [DummyHandler], anonymousEndpoints: ['POST /custom/hook'] }) + .with(enrichPathInfo); + context.pathInfo.suffix = '/slack/events'; + + const resp = await custom(new Request('https://space.cat/slack/events', { method: 'POST' }), context); + + expect(resp.status).to.equal(401); + }); + + it('copies anonymousEndpoints so later mutation cannot widen the bypass', async () => { + const caller = []; + const locked = wrap(() => 42) + .with(authWrapper, { authHandlers: [DummyHandler], anonymousEndpoints: caller }) + .with(enrichPathInfo); + caller.push('POST /slack/events'); + context.pathInfo.suffix = '/slack/events'; + + const resp = await locked(new Request('https://space.cat/slack/events', { method: 'POST' }), context); + + expect(resp.status).to.equal(401); + }); + + it('honours an anonymousEndpoints override that names a different route', async () => { + const custom = wrap(() => 42) + .with(authWrapper, { authHandlers: [DummyHandler], anonymousEndpoints: ['POST /custom/hook'] }) + .with(enrichPathInfo); + context.pathInfo.suffix = '/custom/hook'; + + const resp = await custom(new Request('https://space.cat/custom/hook', { method: 'POST' }), context); + + expect(resp).to.equal(42); + }); + + it('throws on a non-array anonymousEndpoints instead of silently using the default', () => { + // Security-sensitive config: a typo by a service trying to DISABLE the bypass must not + // silently re-enable an unauthenticated POST /slack/events. + expect(() => wrap(() => 42) + .with(authWrapper, { authHandlers: [DummyHandler], anonymousEndpoints: 'POST /slack/events' }) + .with(enrichPathInfo)).to.throw('anonymousEndpoints must be an array'); + }); + + it('throws on an anonymousEndpoints array containing a non-string', () => { + expect(() => wrap(() => 42) + .with(authWrapper, { authHandlers: [DummyHandler], anonymousEndpoints: ['POST /slack/events', 42] }) + .with(enrichPathInfo)).to.throw('anonymousEndpoints must be an array'); + }); + + it('throws on a lower-case method, which would validate but never match', () => { + expect(() => wrap(() => 42) + .with(authWrapper, { authHandlers: [DummyHandler], anonymousEndpoints: ['post /slack/events'] }) + .with(enrichPathInfo)).to.throw('upper-case method'); + }); + + it('throws on an entry with no leading slash on the path', () => { + expect(() => wrap(() => 42) + .with(authWrapper, { authHandlers: [DummyHandler], anonymousEndpoints: ['POST slack/events'] }) + .with(enrichPathInfo)).to.throw('upper-case method'); + }); + it('passes options method', async () => { context.pathInfo.method = 'OPTIONS'; context.pathInfo.suffix = '/sites';