2 Commits

Author	SHA1	Message	Date
Anso	f03c9dc7b6	fix(webhooks): address Codex review of PR #1177 (#1181) ... * fix(webhooks): close HMAC timing oracle on trigger reject paths The trigger handler in PR #1177 returned a uniform 404 for every unauthenticated rejection, but only the wrong-signature path computed an HMAC over the request body. The other reject paths (unknown id, disabled webhook, non-paid tier, missing X-Webhook-Signature header, missing rawBody) short-circuited before any HMAC work. Repeated near-rate-limit probes with a large attacker-controlled body could distinguish a valid-and-enabled paid webhook id from the other reject cases through response latency. WebhookService.validateSignature is now constant-time over every input shape: it always runs crypto.createHmac and crypto.timingSafeEqual against fixed-length 32-byte buffers regardless of whether the signature is missing, has the wrong prefix, is malformed hex, or is the wrong length. The trigger handler calls it unconditionally before any reject branch fires, using a stable per-process decoy secret (WebhookService.getDecoySecret) when the webhook does not exist and an empty buffer when the request has no body. Response timing now depends only on the size of the request body, which the attacker already controls and which reveals nothing webhook-specific. Six new tests pin the behaviour: validateSignature is observed firing on the unknown-id and missing-signature paths through a spy assertion, and four direct-call tests confirm validateSignature returns false without throwing for empty, wrong-prefix, malformed-hex, and wrong-length signatures. * fix(safe-log): redact Basic auth and lowercase Windows drive letters The redactSensitiveText helper now covers two cases the prior chain missed: * Authorization: Basic <base64> previously left the base64 payload intact. The existing key/value regex caught only the literal word Basic before stopping at the space. A new Basic\s+[A-Za-z0-9+/=]+ replacement runs before the key/value regex so the credential is scrubbed first. * Windows homedir paths like c:\Users\<user>\... with a lowercase drive letter previously slipped through because the regex required [A-Z]. Changed to [A-Za-z] so both letter cases are covered. Two new tests pin both fixes. * docs(webhooks): document 429, fix shared schema, comply with D27/D31 * Trigger endpoint declares the 429 response that webhookTriggerLimiter can return (500 requests per minute per source IP); both docs/openapi.yaml and the response table in docs/features/webhooks.mdx carry the new row, and a new troubleshooting accordion explains the shared-NAT scenario. * Shared Webhook schema in docs/openapi.yaml extends the action enum to include git-pull and documents the node_id property. The GET list endpoint returns these fields; the prior schema would have failed validation for any git-pull row. * docs/features/webhooks.mdx:7 rewritten from a customer-side role enumeration ("non-admins on a paid tier can view the list but cannot manage it") to a single requirement statement ("Webhooks require a Skipper or Admiral license. Managing webhooks is admin-only.") per CLAUDE.md D27/D31; the prior phrasing was customer-side fence-spec. * Two em dashes in webhook description strings I had touched in the prior OpenAPI sync commit replaced with semicolons per D18.	2026-05-23 16:03:48 -04:00
Anso	21ec5e7e0a	fix(webhooks): harden trigger response surface (#1177) ... * fix(webhooks): harden trigger response surface Bundles six audit findings on the incoming-webhooks trigger path. All changes preserve the documented happy path: a CI caller signing the exact request body with the webhook secret still receives 202 Accepted. * Uniform 404 on every unauthenticated rejection (missing webhook, disabled webhook, non-paid tier, missing signature header, missing raw body, signature mismatch). The four-way response surface previously let an unauthenticated probe enumerate webhook ids and fingerprint the instance's licence tier; callers now see one shape for any failed auth. * Fail closed when express.json()'s verify callback did not capture the raw request body. Previously the handler fell back to JSON.stringify(req.body), which compares the HMAC against a re-serialised payload that is not byte-equal to what the client signed. * Pass the already-loaded webhook through to WebhookService.execute() instead of re-fetching by id. Closes the delete-during-execution race where an admin deletion between the trigger handler's load and the async dispatch silently dropped the execution row. The webhook_executions table has ON DELETE CASCADE, so recordExecution now wraps the insert in try/catch and logs a warning when the FK constraint trips because the parent webhook was deleted mid-flight. * Redact bearer tokens, JWTs, URL credentials, and homedir paths from error strings before persisting to webhook_executions.error. The execution history is readable by any paid user via GET /webhooks/:id/ history; redactSensitiveText gains three home-directory patterns (/home/<user>, /Users/<user>, <drive>:\Users\<user>) and now runs on every error stored from this path. * Cap webhook name at 100 characters on both POST and PUT, rejecting non-string and oversized values with 400 before they reach the DB. * Validate the body's action override against a typed allowlist (isWebhookAction type guard) on the trigger endpoint, returning 400 before queueing execution. An unknown override no longer reaches recordExecution as a stored failure row. Tests updated to pass db.getWebhook(id)! instead of the raw id to the new execute() signature. Docs at docs/features/webhooks.mdx updated to reflect the new uniform 404 response, the new 400-on-invalid-action behaviour, and a rewritten troubleshooting accordion that walks operators through every cause of the uniform 404. * test(webhooks): cover trigger handler auth, race, and redaction paths Adds 21 vitest cases for the public webhook trigger handler and the WebhookService.execute / recordExecution pipeline, plus 3 cases for the new homedir patterns in redactSensitiveText. webhooks-trigger.test.ts covers, per audit finding: * M1 + H3 uniform 404: id unknown, webhook disabled, non-paid tier, missing signature header, missing rawBody, sha1= prefix, malformed hex signature, sig mismatch. Each asserts identical 404 body so a future regression that re-introduces 401 / 403 / PAID_REQUIRED breaks one of the 8 tests. * Happy path: 202 with configured action, valid action override, unknown action override returns 400 after auth succeeds (L2), non-string action override returns 400. * L1 name cap: POST and PUT both reject names over 100 chars and non-string names; 100-char boundary still accepted; PUT allows partial updates that omit name. * M5 race: deleting the parent webhook before recordExecution runs no longer crashes the async dispatch; the FK cascade is swallowed with a console.warn, and a happy-path test pins the recordExecution row. * M6 redaction: stubs ComposeService.runCommand to throw errors containing a bearer token and a homedir path, then asserts the persisted webhook_executions.error has both scrubbed. safe-log.test.ts gains three unit tests pinning the new homedir patterns in redactSensitiveText (Linux, macOS, Windows). The existing credentials test is untouched. Tests use prototype spies on FileSystemService and ComposeService (both hand out a fresh instance per nodeId), so per-test mocks do not leak. beforeEach restores all mocks and reapplies the LicenseService 'paid' spy. Closes audit finding H2 (zero trigger-path test coverage). * docs(webhooks): sync openapi spec with new trigger response surface Brings docs/openapi.yaml in line with the behaviour changes from the trigger hardening commit. Mintlify auto-generates the per-endpoint reference pages from this spec, so the spec drift would surface as wrong response codes in the public API reference. POST /api/webhooks and PUT /api/webhooks/🆔 * name: maxLength 100 (matches MAX_WEBHOOK_NAME_LENGTH on the route). * action enum: add git-pull (pre-existing omission; the route has always accepted it). * node_id: documented as an integer property (pre-existing omission). POST /api/webhooks/:id/trigger: * requestBody required: true (body is now mandatory; the H3 fail-closed branch rejects a missing rawBody). * action override: enum restricted to the allowlist. * 401 and 403 responses removed. * 404 response: description rewritten to reflect uniform-404 behaviour; the body is { error: "Webhook not found or signature invalid" } for every unauthenticated reject. * 400 response added for an authenticated request whose action override is not in the allowlist.	2026-05-23 15:42:49 -04:00