diff --git a/.changeset/22806-uniform-hook-refusal.md b/.changeset/22806-uniform-hook-refusal.md new file mode 100644 index 00000000000..421ca118478 --- /dev/null +++ b/.changeset/22806-uniform-hook-refusal.md @@ -0,0 +1,16 @@ +--- +'@objectstack/trigger-api': minor +'@objectstack/spec': minor +--- + +fix(trigger-api,spec): every inbound hook post that does not verify gets one answer, `401 INVALID_SIGNATURE`, whatever its flow and hook id + +Clause-②: yes + +- **FROM → TO.** On `POST /api/v1/automation/hooks/:flowName/:hookId`, a post that named no armed flow, or the wrong hook id, answered `404 RESOURCE_NOT_FOUND`, while a post naming an armed flow and its hook id with an absent or bad `x-objectstack-signature` answered `401 INVALID_SIGNATURE`. Now all four answer `401 INVALID_SIGNATURE` with the same body, so the answer no longer tells an armed flow name from an unarmed one, whichever hook id a flow is armed with. +- **Who sees a difference.** A correctly configured sender (the flow's URL, its hook id, its secret) sees none: it still gets `202`. A sender posting to a flow that is not armed, or with a hook id that was rotated away, now gets `401` where it got `404`. A sender with a wrong secret or signing code still gets `401`, as before. +- **The cause is in the server log.** The trigger logs one `warn` line per refused post, with the cause and the flow name the post carried as structured fields: `cause` is one of `unknown-flow`, `wrong-hook-id`, `signature-absent`, `signature-malformed`, `signature-mismatch` or `signature-outside-window`. The line never carries the secret, the signature value or the body. +- **Unchanged: the `503` residual.** A post that names an armed flow and its hook id while that hook's secret cannot be read still answers `503 SERVICE_UNAVAILABLE`, signed or not, without verifying. It stays a `503` on purpose: it is the deployment's fault, a sender retries it, and it lasts only while the secret cannot be read. A wrong hook id never reaches the secret read, so it gets the uniform `401` even then. +- **`@objectstack/spec`.** The `ITriggerApiService.handleInboundHook` docblock states the one refusal and names the `503` as its residual. The type is unchanged. + +**To adopt it.** A sender or monitor that read `404` from this route as "wrong URL" can no longer tell that from a bad signature by the answer: read the trigger's `warn` line for the post instead, whose `cause` names which one it was. diff --git a/packages/spec/src/contracts/trigger-api-service.ts b/packages/spec/src/contracts/trigger-api-service.ts index 3da259d7409..0a156ddfaa0 100644 --- a/packages/spec/src/contracts/trigger-api-service.ts +++ b/packages/spec/src/contracts/trigger-api-service.ts @@ -43,21 +43,28 @@ export interface ITriggerApiService { * What it serves, exactly as the plugin's self-hosted mount serves it * today, in this order: * - It looks up the hook armed for `hook.flowName` and compares - * `hook.hookId` with it in constant time. An unknown flow and a wrong - * hook id answer identically, `404 RESOURCE_NOT_FOUND`. The hook is - * matched BEFORE the signature is verified, so an unsigned post that - * names an armed flow and its hook id gets the `503` or `401` below - * instead of that `404`: the hook id, not the flow name, is what keeps - * an armed flow unprobeable. A flow left on the default hook id - * (`default`, when its start node's `config.hookId` names none) can be - * probed that way. Whether every unverified post should get one uniform - * refusal is the hardening card's question (#22769), not this contract's. + * `hook.hookId` with it in constant time. A post that names no armed + * flow, or the wrong hook id, gets the unverified-post refusal below, + * before any secret is read. * - It reads that hook's own secret, at this moment, so a rotation applies * to the next post. A secret that cannot be read answers * `503 SERVICE_UNAVAILABLE`. ⛔ It never verifies against no secret. + * This `503` is the one residual of the refusal below: while an armed + * hook's secret cannot be read, a post that names that flow and its hook + * id gets this `503`, signed or not. It stays a `503` on purpose: it is + * the deployment's fault, a sender retries it, and it lasts only while + * the secret cannot be read. * - It verifies the `x-objectstack-signature` header (`sha256=` and the hex * HMAC-SHA256 of the raw body under that secret). A missing or wrong - * signature answers `401 INVALID_SIGNATURE`. + * signature gets the unverified-post refusal below. + * - **One refusal for every post that does not verify** (#22806): an + * unknown flow, a wrong hook id, an absent signature and a bad one all + * answer `401 INVALID_SIGNATURE`, with one body. So the answer does not + * tell an armed flow name from an unarmed one, whichever hook id a flow + * is armed with. The cause (which of those four) goes to the server log, + * ⛔ never to the sender. FROM → TO: an unknown flow and a wrong hook id + * used to answer `404 RESOURCE_NOT_FOUND`; they now answer the same + * `401 INVALID_SIGNATURE` an absent or bad signature always has. * - Only then does it read the body as JSON. A body that is not valid JSON, * or not a JSON object, answers `400 INVALID_REQUEST`; an empty body is * the empty object. diff --git a/packages/triggers/trigger-api/src/api-trigger.test.ts b/packages/triggers/trigger-api/src/api-trigger.test.ts index ff7cdd049e8..5f7a507ea09 100644 --- a/packages/triggers/trigger-api/src/api-trigger.test.ts +++ b/packages/triggers/trigger-api/src/api-trigger.test.ts @@ -82,12 +82,13 @@ describe('ApiTrigger', () => { expect(runs[0].params).toEqual({ title: 'New lead', amount: 5000 }); }); - it('answers 404 identically for unknown flows and wrong hookIds (no probing oracle)', async () => { + it('answers unknown flows and wrong hookIds identically, with the unverified-post refusal', async () => { arm({ hookId: 'hk1', secret: 's3cret' }); const a = await trigger.handleRequest({ flowName: 'nope', hookId: 'hk1', rawBody: '{}' }); const b = await trigger.handleRequest({ flowName: 'lead_intake', hookId: 'wrong', rawBody: '{}' }); expect(a).toEqual(b); - expect(a.status).toBe(404); + expect(a.status).toBe(401); + expect((a.body as any).error.code).toBe('INVALID_SIGNATURE'); }); it('401s a missing or bad signature', async () => { @@ -115,11 +116,12 @@ describe('ApiTrigger', () => { expect(queue.subscribed).toEqual([]); expect(logger.info).not.toHaveBeenCalledWith(expect.stringContaining('armed:')); - // A post to the flow finds no hook: the same 404 an unknown flow - // gets, and nothing reaches the queue or the flow. + // A post to the flow finds no hook: the same refusal an unknown + // flow gets, and nothing reaches the queue or the flow. const res = await trigger.handleRequest({ flowName: 'lead_intake', hookId: 'default', rawBody: '{"x":1}' }); - expect(res.status).toBe(404); - expect(res.body).toEqual({ success: false, error: { code: 'RESOURCE_NOT_FOUND', message: 'No such hook.' } }); + expect(res).toEqual(await trigger.handleRequest({ flowName: 'never_armed', hookId: 'default', rawBody: '{"x":1}' })); + expect(res.status).toBe(401); + expect((res.body as any).error.code).toBe('INVALID_SIGNATURE'); expect(queue.published).toEqual([]); expect(await queue.deliver()).toBe(0); expect(runs).toEqual([]); @@ -154,8 +156,12 @@ describe('ApiTrigger', () => { it('stop() disarms the hook and unsubscribes the queue', async () => { arm({ hookId: 'hk1', secret: 's3cret' }); trigger.stop('lead_intake'); - const res = await trigger.handleRequest({ flowName: 'lead_intake', hookId: 'hk1', rawBody: '{}' }); - expect(res.status).toBe(404); + const res = await trigger.handleRequest({ + flowName: 'lead_intake', hookId: 'hk1', rawBody: '{}', signatureHeader: sig('s3cret', '{}'), + }); + expect(res.status).toBe(401); + expect((res.body as any).error.code).toBe('INVALID_SIGNATURE'); + expect(queue.published).toEqual([]); expect(trigger.listHooks()).toHaveLength(0); }); }); @@ -213,11 +219,12 @@ describe('ApiTrigger — a secret read at verification time', () => { } }); - it('an unknown hook still answers 404 before any secret is read (no oracle)', async () => { + it('a wrong hook id is refused before any secret is read', async () => { let reads = 0; const { t } = armWith(async () => { reads += 1; return HELD; }); const res = await t.handleRequest({ flowName: 'held_intake', hookId: 'wrong', rawBody: '{}', signatureHeader: sig(HELD, '{}') }); - expect(res.status).toBe(404); + expect(res.status).toBe(401); + expect((res.body as any).error.code).toBe('INVALID_SIGNATURE'); expect(reads).toBe(0); }); }); @@ -357,3 +364,151 @@ describe('ApiTrigger — the timestamped signature form', () => { expect(verifySignature(SECRET, BODY, sigV1(SECRET, BODY, NOW - WINDOW - 1))).toBe(false); }); }); + +/** + * One answer for every post that does not verify, whatever the flow and the + * hook id: an unknown flow, an armed flow under the default hook id, an armed + * flow under a custom hook id posted with the wrong hook id, and each of those + * unsigned or with a signature that does not verify. Every answer is the same + * status and the same body, so the answer does not separate armed flow names + * from unarmed ones. The cause goes to the server log only. + * + * These pins assert the status, the code, and that every answer equals every + * other; the message prose is not pinned. Two CONTROLS beside them: a valid + * post is still accepted under the default and a custom hook id, and the + * `503` for a secret that cannot be read (the declared residual) is unchanged. + */ +describe('ApiTrigger — one answer for every post that does not verify', () => { + const NOW = 1760000000; + const SECRET_DEFAULT = 'pin-uniform-default-61c2'; + const SECRET_CUSTOM = 'pin-uniform-custom-9d04'; + const BODY = JSON.stringify({ event: 'uniform.pin', marker: 'pin-body-marker-4b7e' }); + + function sigV1(secret: string, body: string, t: number): string { + return `t=${t},v1=` + createHmac('sha256', secret).update(`${t}.${body}`, 'utf8').digest('hex'); + } + + let queue: ReturnType; + let log: { info: ReturnType; warn: ReturnType }; + let trigger: ApiTrigger; + + beforeEach(() => { + vi.useFakeTimers({ toFake: ['Date'] }); + vi.setSystemTime(NOW * 1000); + queue = makeFakeQueue(); + log = { info: vi.fn(), warn: vi.fn() }; + trigger = new ApiTrigger(() => queue, log as any); + trigger.start({ flowName: 'armed_on_default', config: { secret: SECRET_DEFAULT } }, async () => {}); + trigger.start({ flowName: 'armed_on_custom', config: { hookId: 'hk_custom', secret: SECRET_CUSTOM } }, async () => {}); + }); + afterEach(() => { + vi.useRealTimers(); + }); + + /** Where a post is sent, and the cause the log must record whatever it is signed with (`null`: the signature's). */ + const ADDRESSES = [ + ['an unknown flow', 'never_armed', 'default', 'unknown-flow'], + ['an unknown flow, under a custom-looking hook id', 'never_armed', 'hk_custom', 'unknown-flow'], + ['an armed flow under the default hook id', 'armed_on_default', 'default', null], + ['an armed flow under a custom hook id, posted with the wrong hook id', 'armed_on_custom', 'default', 'wrong-hook-id'], + ['an armed flow under a custom hook id, posted with its hook id', 'armed_on_custom', 'hk_custom', null], + ] as const; + + /** A signature that does not verify for any of the addresses, and the cause it records. */ + const BAD_SIGNATURES = [ + ['unsigned', undefined, 'signature-absent'], + ['a malformed signature', 'not-a-signature-value', 'signature-malformed'], + ['a body-only signature under another secret', sig('not-the-secret', BODY), 'signature-mismatch'], + ['a timestamped signature under another secret', sigV1('not-the-secret', BODY, NOW), 'signature-mismatch'], + ] as const; + + /** Posts that carry the RIGHT secret for some flow and still must not verify. */ + const SIGNED_BUT_UNVERIFIED = [ + ['the right secret, posted with the wrong hook id', 'armed_on_custom', 'default', sig(SECRET_CUSTOM, BODY), 'wrong-hook-id'], + ['another flow\'s secret, posted to an unknown flow', 'never_armed', 'default', sig(SECRET_DEFAULT, BODY), 'unknown-flow'], + ['another flow\'s secret, posted to an armed flow', 'armed_on_default', 'default', sig(SECRET_CUSTOM, BODY), 'signature-mismatch'], + ['the right secret, with a timestamp outside the window', 'armed_on_default', 'default', sigV1(SECRET_DEFAULT, BODY, NOW - 3600), 'signature-outside-window'], + ] as const; + + type Case = { label: string; flowName: string; hookId: string; header: string | undefined; cause: string }; + const CASES: Case[] = [ + ...ADDRESSES.flatMap(([where, flowName, hookId, addressCause]) => + BAD_SIGNATURES.map(([how, header, signatureCause]) => ({ + label: `${where}, ${how}`, flowName, hookId, header, cause: addressCause ?? signatureCause, + }))), + ...SIGNED_BUT_UNVERIFIED.map(([label, flowName, hookId, header, cause]) => ({ label, flowName, hookId, header, cause })), + ]; + + const post = (c: Case) => + trigger.handleRequest({ flowName: c.flowName, hookId: c.hookId, rawBody: BODY, signatureHeader: c.header }); + + /** The `cause` of every refusal line logged so far. */ + const refusalCauses = () => + log.warn.mock.calls + .map((call) => call[1] as { cause?: unknown } | undefined) + .filter((meta) => meta != null && typeof meta === 'object' && 'cause' in meta) + .map((meta) => meta!.cause); + + it('every unverified post gets the same status and the same envelope, and nothing is enqueued', async () => { + expect(CASES).toHaveLength(ADDRESSES.length * BAD_SIGNATURES.length + SIGNED_BUT_UNVERIFIED.length); + const answers = []; + for (const c of CASES) answers.push({ label: c.label, answer: await post(c) }); + + const [first] = answers; + expect(first!.answer.status).toBe(401); + expect(first!.answer.body.success).toBe(false); + expect((first!.answer.body as any).error.code).toBe('INVALID_SIGNATURE'); + for (const { label, answer } of answers) { + expect(answer, label).toEqual(first!.answer); + } + expect(queue.published).toEqual([]); + }); + + it('the server log records the real cause of each refusal, once, and never the secret, the signature or the body', async () => { + for (const c of CASES) { + log.warn.mockClear(); + await post(c); + expect(refusalCauses(), c.label).toEqual([c.cause]); + const logged = JSON.stringify([...log.warn.mock.calls, ...log.info.mock.calls]); + for (const secret of [SECRET_DEFAULT, SECRET_CUSTOM]) { + expect(logged.includes(secret), `${c.label}: the secret`).toBe(false); + } + if (c.header) expect(logged.includes(c.header), `${c.label}: the signature value`).toBe(false); + expect(logged.includes('pin-body-marker-4b7e'), `${c.label}: the body`).toBe(false); + } + }); + + it('CONTROL: a valid post is still accepted under the default hook id and under a custom one, in both signature forms', async () => { + for (const [flowName, hookId, secret] of [ + ['armed_on_default', 'default', SECRET_DEFAULT], + ['armed_on_custom', 'hk_custom', SECRET_CUSTOM], + ] as const) { + for (const header of [sigV1(secret, BODY, NOW), sig(secret, BODY)]) { + const res = await trigger.handleRequest({ flowName, hookId, rawBody: BODY, signatureHeader: header }); + expect(res.status, `${flowName}/${hookId}`).toBe(202); + expect(res.body.accepted).toBe(true); + } + } + expect(queue.published).toHaveLength(4); + expect(refusalCauses()).toEqual([]); + }); + + it('CONTROL: a secret that cannot be read still answers 503 for its flow and hook id, signed or not; a wrong hook id never reads it', async () => { + let reads = 0; + trigger.start( + { flowName: 'armed_unreadable', config: {}, resolveSecret: async () => { reads += 1; throw new Error('store offline'); } }, + async () => {}, + ); + for (const header of [undefined, sig('not-the-secret', BODY)]) { + const res = await trigger.handleRequest({ flowName: 'armed_unreadable', hookId: 'default', rawBody: BODY, signatureHeader: header }); + expect(res.status).toBe(503); + expect((res.body as any).error.code).toBe('SERVICE_UNAVAILABLE'); + } + expect(reads).toBe(2); + + const wrongHook = await trigger.handleRequest({ flowName: 'armed_unreadable', hookId: 'other', rawBody: BODY }); + expect(wrongHook).toEqual(await post(CASES[0]!)); + expect(reads).toBe(2); + expect(queue.published).toEqual([]); + }); +}); diff --git a/packages/triggers/trigger-api/src/api-trigger.ts b/packages/triggers/trigger-api/src/api-trigger.ts index 85e344ecb3c..a920e032dcd 100644 --- a/packages/triggers/trigger-api/src/api-trigger.ts +++ b/packages/triggers/trigger-api/src/api-trigger.ts @@ -2,6 +2,7 @@ import { timingSafeEqual } from 'node:crypto'; import { HTTP_SIGNATURE_TOLERANCE_SECONDS, verifyHttpSignature } from '@objectstack/core'; +import type { HttpSignatureVerdict } from '@objectstack/core'; import type { AutomationContext } from '@objectstack/spec/contracts'; /** @@ -85,6 +86,39 @@ interface ArmedHook { bodyOnlyDeprecationLogged: boolean; } +/** + * Why an inbound post did not verify. Recorded in the server log only, ⛔ never + * sent: every one of these gets the same answer ({@link unverifiedPostAnswer}), + * so an operator can still tell a misconfigured sender's wrong URL from its + * wrong secret while the sender cannot. The `signature-*` kinds carry + * `verifyHttpSignature`'s own `reason`, the one vocabulary for them. + */ +type UnverifiedPostCause = + | 'unknown-flow' + | 'wrong-hook-id' + | `signature-${Extract['reason']}`; + +const UNVERIFIED_POST_CAUSE_TEXT: Readonly> = { + 'unknown-flow': 'no hook is armed for the flow it names', + 'wrong-hook-id': 'the hook id it names is not the one its flow is armed with', + 'signature-absent': 'it carries no x-objectstack-signature', + 'signature-malformed': 'its x-objectstack-signature has neither accepted form', + 'signature-mismatch': "its x-objectstack-signature is not the hook secret's HMAC of the body it carries", + 'signature-outside-window': + `its timestamped x-objectstack-signature verified, with a timestamp more than ` + + `${HTTP_SIGNATURE_TOLERANCE_SECONDS}s from this server's clock (a replay, or a sender clock that is wrong)`, +}; + +/** + * The ONE answer every inbound post that does not verify gets: an unknown + * flow, a wrong hook id, an absent signature and a bad one alike. One status + * and one body, so the answer does not tell an armed flow name from an unarmed + * one. A fresh object per call, so no caller can alter the next answer. + */ +function unverifiedPostAnswer(): { status: number; body: Record } { + return { status: 401, body: { success: false, error: { code: 'INVALID_SIGNATURE', message: 'Signature verification failed.' } } }; +} + /** The trimmed, non-blank string `value` holds, else `undefined`. */ function usableSecret(value: unknown): string | undefined { return typeof value === 'string' && value.trim() ? value.trim() : undefined; @@ -130,9 +164,12 @@ export function verifySignature(secret: string, rawBody: string, header: string * flow in-band: * * POST /api/v1/automation/hooks/:flowName/:hookId - * → 404 unknown flow / wrong hookId - * → 401 missing/bad HMAC signature (a timestamped one outside its - * window included — see {@link verifySignature}) + * → 401 INVALID_SIGNATURE — every post that does not verify, one answer: + * an unknown flow, a wrong hookId, a missing or bad HMAC signature + * (a timestamped one outside its window included — see + * {@link verifySignature}). The cause goes to the server log only. + * → 503 the armed hook's secret cannot be read (the one residual: it + * answers a post naming that flow and its hookId, signed or not) * → 400 non-JSON body * → 202 { accepted, messageId } — queued; a consumer executes the flow * @@ -244,14 +281,20 @@ export class ApiTrigger implements FlowTrigger { idempotencyKey?: string; }): Promise<{ status: number; body: Record }> { const hook = this.hooks.get(input.flowName); - // Unknown flow and wrong hookId answer identically — no oracle for - // probing which flows exist. - if (!hook || !safeEqual(hook.hookId, input.hookId)) { - return { status: 404, body: { success: false, error: { code: 'RESOURCE_NOT_FOUND', message: 'No such hook.' } } }; - } + // An unknown flow, a wrong hookId, and (below) an absent or bad + // signature all get the one unverified-post answer, so the answer + // (status and body) does not tell an armed flow name from an unarmed + // one; the cause goes to the log only. The claim is about the answer, + // and nothing else. The hook is matched before its secret is read, so + // the secret-unavailable 503 below, the one residual, answers only a + // post that names an armed flow AND its hookId. + if (!hook) return this.refuseUnverified('unknown-flow', input.flowName); + if (!safeEqual(hook.hookId, input.hookId)) return this.refuseUnverified('wrong-hook-id', hook.flowName); // The secret, read now: a rotation applies to the next post. One that // cannot be read is the deployment's fault, not the sender's, and is // never read as "no secret" — the post is refused without verifying. + // A 5xx on purpose: a sender retries it, and the fault lasts only + // while the secret cannot be read. let secret: string | undefined; try { secret = hook.secret.kind === 'literal' ? hook.secret.secret : usableSecret(await hook.secret.resolve()); @@ -266,9 +309,7 @@ export class ApiTrigger implements FlowTrigger { }; } const verdict = verifyHttpSignature(input.rawBody, secret, input.signatureHeader); - if (!verdict.valid) { - return { status: 401, body: { success: false, error: { code: 'INVALID_SIGNATURE', message: 'Signature verification failed.' } } }; - } + if (!verdict.valid) return this.refuseUnverified(`signature-${verdict.reason}`, hook.flowName); if (verdict.scheme === 'body-only' && !hook.bodyOnlyDeprecationLogged) { hook.bodyOnlyDeprecationLogged = true; this.logger.warn( @@ -306,4 +347,19 @@ export class ApiTrigger implements FlowTrigger { return { status: 503, body: { success: false, error: { code: 'ENQUEUE_FAILED', message: 'Could not enqueue the trigger message.' } } }; } } + + /** + * Refuse a post that did not verify: log its real cause, then answer + * {@link unverifiedPostAnswer}, the same for every cause. The line names + * the cause and the flow name the post carried, as structured fields, and + * ⛔ never the secret, the signature value or the body. + */ + private refuseUnverified(cause: UnverifiedPostCause, flowName: string): { status: number; body: Record } { + this.logger.warn( + `[trigger-api] refused an inbound post (${cause}): ${UNVERIFIED_POST_CAUSE_TEXT[cause]}. ` + + `The sender got the one answer every unverified post gets; this line is the only record of the cause.`, + { cause, flowName }, + ); + return unverifiedPostAnswer(); + } }