From 0930994f64f274431d6e6a81da3df1ac73633769 Mon Sep 17 00:00:00 2001 From: akansmafengadamu Date: Fri, 16 Oct 2026 22:37:41 +0100 Subject: [PATCH] fix(security): resolve PR #1703 review findings (CSRF FPs, secret fixture) Address the two unresolved review threads left on PR #1703. ## Semgrep: state-changing-route-without-csrf (x2) `subtrackr.csrf.state-changing-route-without-csrf` matched app.post('/api/v1/stripe/billing/webhook', express.raw({ ... })) because the rule only recognises a middleware literally named `csrfProtection`, `requireAuth` or `verifyWebhookSignature`, and it cannot follow the call into `createStripeBillingRouter` / `createPaddleRouter`. Both are false positives. Those two lines install a *body parser only*: they declare no handler and change no state. The routes are mounted further down, and both fail closed: - 503 when the signing secret is unconfigured; - 401 when the HMAC over the raw bytes does not match (`timingSafeEqual` plus a 300s replay window). The obvious "fix" of re-running verification at the app level would be a regression. `StripeWebhookVerifier.verify()` calls `remember(eventId)` on success, so a second call in the same request returns `fresh: false`, and the router would answer `200 { duplicate: true }` without ever dispatching the event, silently dropping every subscription lifecycle update. The suppression records that constraint so nobody repeats it. Registered in `.semgrep/semgrep-suppressions.yml` with a 6-month expiry so the exception stays auditable. ## GitGuardian: hardcoded Zapier webhook URL `outboundUrlGuard.test.ts` asserted on `https://hooks.zapier.com/hooks/catch/1/2/`. It is a fixture, not a secret, but the path shape is indistinguishable from a live hook URL and created a false incident. Swapped for a non-`/hooks/catch/` path on the same host: the assertion only needs "public host over https", and the realistic Zapier hostname coverage is kept. Verification: `node --experimental-strip-types --check` on both touched TS files. Lint, typecheck and tests not run, `node_modules` is not installed here. --- .semgrep/semgrep-suppressions.yml | 29 ++++++++++++++++++- backend/server/createApiServer.ts | 15 ++++++++++ .../__tests__/outboundUrlGuard.test.ts | 5 +++- 3 files changed, 47 insertions(+), 2 deletions(-) diff --git a/.semgrep/semgrep-suppressions.yml b/.semgrep/semgrep-suppressions.yml index 51a43662..70fb8f9d 100644 --- a/.semgrep/semgrep-suppressions.yml +++ b/.semgrep/semgrep-suppressions.yml @@ -6,4 +6,31 @@ # The security workflow fails when a nosemgrep comment omits a reason or # justification. Add accepted suppressions here during security review so future # reviewers can audit the exception history. -accepted_suppressions: [] +accepted_suppressions: + # backend/server/createApiServer.ts — POST /api/v1/stripe/billing/webhook + # `app.post(path, express.raw(...))` registers a body parser before + # `express.json()` so the HMAC-signed bytes survive; it declares no handler. + # The route is mounted by `createStripeBillingRouter`, which verifies the + # signature (503 when unconfigured, 401 on mismatch) before decoding. + - rule_id: subtrackr.csrf.state-changing-route-without-csrf + path: backend/server/createApiServer.ts + reason: >- + Body-parser registration only; signed-webhook verification lives in + createStripeBillingRouter. Not duplicated at the app level because + verify() mutates the replay-dedup map and a second call would mark the + first legitimate delivery as a duplicate. + reviewed_by: security review of PR #1703 + expires: 2027-04-16 + + # backend/server/createApiServer.ts — POST /api/v1/paddle/webhook + # Same shape as the Stripe registration above; verification lives in + # `createPaddleRouter`. + - rule_id: subtrackr.csrf.state-changing-route-without-csrf + path: backend/server/createApiServer.ts + reason: >- + Body-parser registration only; signed-webhook verification lives in + createPaddleRouter. Not duplicated at the app level because verify() + mutates the replay-dedup map and a second call would mark the first + legitimate delivery as a duplicate. + reviewed_by: security review of PR #1703 + expires: 2027-04-16 diff --git a/backend/server/createApiServer.ts b/backend/server/createApiServer.ts index 8d2d1f0c..79e090e3 100644 --- a/backend/server/createApiServer.ts +++ b/backend/server/createApiServer.ts @@ -161,7 +161,22 @@ export function createApiServer(options: CreateApiServerOptions = {}): Express { // registered for the webhook paths *before* the JSON parser, so // `req.body` is a Buffer holding the untouched payload. `type: '*/*'` is // required because providers do not agree on a Content-Type. + // + // These two registrations install a *body parser only*: they declare no + // handler and perform no state change. The routes themselves are mounted + // further down via `createStripeBillingRouter` / `createPaddleRouter`, and + // both verify an HMAC signature over the raw bytes before the payload is + // decoded — answering 503 when the secret is unconfigured and 401 when the + // signature does not match. The CSRF rule cannot see across that file + // boundary, hence the suppressions below. + // + // Note: verification must NOT be duplicated here. `verify()` records the + // event id for replay de-duplication, so a second call would report the + // first legitimate delivery as a duplicate and the router would answer + // `200 { duplicate: true }` without ever dispatching the event. + // nosemgrep: subtrackr.csrf.state-changing-route-without-csrf -- reason: body-parser-only registration, no handler and no state change; the request is HMAC-verified inside the Stripe router, which fails closed with 503 when unconfigured and 401 on a bad signature -- expires: 2027-04-16 app.post('/api/v1/stripe/billing/webhook', express.raw({ type: '*/*', limit: MAX_JSON_BODY })); + // nosemgrep: subtrackr.csrf.state-changing-route-without-csrf -- reason: body-parser-only registration, no handler and no state change; the request is HMAC-verified inside the Paddle router, which fails closed with 503 when unconfigured and 401 on a bad signature -- expires: 2027-04-16 app.post('/api/v1/paddle/webhook', express.raw({ type: '*/*', limit: MAX_JSON_BODY })); // An explicit cap keeps a single oversized POST from buffering the whole diff --git a/backend/shared/security/__tests__/outboundUrlGuard.test.ts b/backend/shared/security/__tests__/outboundUrlGuard.test.ts index f2c04b7d..90078865 100644 --- a/backend/shared/security/__tests__/outboundUrlGuard.test.ts +++ b/backend/shared/security/__tests__/outboundUrlGuard.test.ts @@ -72,7 +72,10 @@ describe('isPrivateHostname', () => { describe('validateOutboundUrl', () => { it('accepts a normal public https URL', () => { - const result = validateOutboundUrl('https://hooks.zapier.com/hooks/catch/1/2/'); + // A real Zapier host, but deliberately *not* a `/hooks/catch///` + // path: that shape is indistinguishable from a live Zapier hook URL and + // trips secret detectors in CI (the assertion only needs "public host"). + const result = validateOutboundUrl('https://hooks.zapier.com/example-endpoint'); expect(result.protocol).toBe('https:'); expect(result.hostname).toBe('hooks.zapier.com'); });