Skip to content

Reserve dangerous webhook headers, drop webhookUrl leak, harden wallet-challenge and webhook-test coverage - #639

Merged
therealjhay merged 5 commits into
Betta-Pay:mainfrom
richardtoms100:fix/issues-607-608-611-624-v2
Sep 1, 2026
Merged

Reserve dangerous webhook headers, drop webhookUrl leak, harden wallet-challenge and webhook-test coverage#639
therealjhay merged 5 commits into
Betta-Pay:mainfrom
richardtoms100:fix/issues-607-608-611-624-v2

Conversation

@richardtoms100

@richardtoms100 richardtoms100 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Verified each issue against the actual current code (not just the issue text) before changing anything — two of the four premises turned out to be partially stale, documented below.

#607 — Webhook header CRLF injection

WebhookHeadersSchema (added under #569) already does most of this: header names are validated against an HTTP-token character class (already excludes CRLF/colons/spaces), and header values are already rejected outright if they contain \r/\n — the exact X-Ok: "a\r\nInjected: b" attack this issue names. It's already wired into both settlement-engine (extractWebhookHeaders, which re-validates the stored JSON blob) and the indexer's subscription endpoints.

The real gap: RESERVED_WEBHOOK_HEADER_NAMES only reserved content-type/x-bettapay-signature — not Host/Content-Length (this issue's other explicit acceptance criterion). Added those plus Transfer-Encoding/Connection (same request-smuggling-adjacent risk).

#608 — webhookUrl leaked in settlement payload

Real, still-open bug: buildSettlementWebhookData included webhookUrl: s.webhookUrl in the payload sent to merchants — an internal delivery target (Vercel preview / ngrok / internal hostnames) that has no business being echoed back. Removed it; internal routing never read it from this payload anyway (call sites use Settlement.webhookUrl directly). Updated the docs example and added a regression test.

#611 — Wallet challenge concurrent-consume coverage

The issue's premise (a separate wallet-auth-challenge.ts with an untested Lua script, next to a tested getdel in wallet-challenge-store.ts) is stale: #605 already consolidated both into the one WalletChallengeStore (Lua GET+DEL, since it needs to work on Redis < 6.2 too) that the real /api/auth/challenge//api/auth/verify routes actually call. Its test file already had a concurrent-consume test, just at 2 parallel calls — strengthened to 10, matching this issue's acceptance criteria exactly.

Also deleted wallet-auth-challenge.test.ts: it built its own throwaway Fastify app with hand-rolled, non-atomic get-then-del route handlers that never touch WalletChallengeStore or the real gateway routes at all. A passing "wallet auth challenge" suite that doesn't exercise any production code is worse than no suite — it's the same class of blind spot this issue was raised about.

#624 — Cross-merchant webhook-test poisoning

The merchant-facing route (api-gateway, POST /api/webhooks/:id/test) already correctly checks existing.merchantId !== payload.merchantId and returns 403 — verified by reading it, not assumed. So the exact end-to-end attack isn't reachable through the public API today. The indexer's own internal /api/webhooks/:id/test (the file this issue names) genuinely had no such check though — it sits behind serviceAuth (trusted-caller check) but never looks at which merchant the caller is acting for, and WebhookSubscription.merchantId was never referenced anywhere in that file. That's a real defense-in-depth gap: it relies on exactly one caller (api-gateway) always pre-checking ownership.

Forwarded api-gateway's already-verified merchantId to the indexer as ?merchantId= (mirroring the existing getPaymentEvents forwarding convention) and added the ownership check at the indexer layer too, returning 403 without mutating lastTestedAt/lastTestStatus on a mismatch.

Unrelated pre-existing bugs fixed only to unblock verification

This workspace has several pre-existing breaks unrelated to any of the four issues above, confirmed via git stash against a clean checkout before touching anything:

  • @bettapay/shared-validation doesn't exist as a package anywhere (real name: @bettapay/validation) — was blocking services/indexer/src/index.ts, services/settlement-engine/src/{settlement-amounts,webhook-payload}.ts, and scripts/encrypt-existing-secrets.ts from loading at all.
  • webhook-payload.ts imported a bare logger from @bettapay/validation, which has never exported one (this codebase logs via fastify.log) — swapped the one corrupt-feeSnapshot log line to console.error.
  • shared/validation/schemas.ts's MerchantSettings .refine() referenced data.feeSchedules, a field the schema never declared — added as an untyped optional passthrough (same runtime behavior as before, now type-checks).

Two further pre-existing breaks I found but did not fix (out of scope, not blocking this PR's own tests): settlement-amounts.ts calls an undefined BN (imports BigNumber but never aliases it), and settlement-engine/src/index.ts has a standalone try/catch syntax error around line 1026. Both are unrelated to #607/#608/#611/#624.

Test plan

  • node --loader ts-node/esm --test webhookSchema.test.ts (shared/validation) — 48/48 passing (6 new, Webhook custom headers are stored as JSON without CRLF header-injection validation #607)
  • node --loader ts-node/esm src/webhook-test.test.ts (indexer) — 21/23 passing; the 5 new/changed assertions for Webhook test endpoint stores lastTestedAt without verifying the URL was owned by the requesting merchant #624 all pass; the 2 failures are a pre-existing .end() already called race in the first, unmodified test in the file — that suite could not run at all before the @bettapay/validation fix, so this was previously unknown rather than a regression
  • webhook-payload.test.ts (settlement-engine): could not execute end-to-end — its test fixture transitively hits the unrelated pre-existing BN bug above. Verified via tsc --noEmit (identical pre-existing error set before/after, no new errors from this file) plus manual review
  • wallet-challenge-expiry.test.ts (api-gateway): the WalletChallengeStore-level tests (not the route-level ones, which need index.ts to load and hit the unrelated pre-existing syntax error above) were verified logically against the already-passing 2-parallel-call version being extended to 10

@richardtoms100
fix: reserve Host/Content-Length/Transfer-Encoding/Connection in webh…
28cb0a8 
…ook headers

Closes Betta-Pay#607

WebhookHeadersSchema (added under Betta-Pay#569) already did the hard part of
this issue: header names are validated against an HTTP-token
character class (which already excludes CRLF, colons, and spaces —
so name-based injection was already blocked), and header *values*
are already rejected outright if they contain \r or \n — exactly the
"X-Ok: a\r\nInjected: b" attack this issue's acceptance criteria
names. It's already wired into both settlement-engine (via
extractWebhookHeaders, which re-validates the stored JSON blob rather
than trusting the DB) and the indexer's subscription endpoints.

The one real gap: RESERVED_WEBHOOK_HEADER_NAMES only reserved
content-type and x-bettapay-signature (headers this system itself
controls) — Host and Content-Length (this issue's other explicit
acceptance criterion) weren't reserved at all, and neither were
Transfer-Encoding/Connection, which carry the same request-smuggling-
adjacent risk of a client-supplied header confusing whatever HTTP
client actually performs the delivery. Added all four.

Added tests for the exact acceptance-criteria cases (CRLF-in-value,
Host, Content-Length, Transfer-Encoding, Connection) plus a
round-trip test confirming valid headers pass through unchanged.

`node --loader ts-node/esm --test webhookSchema.test.ts` — 48/48
passing (6 new).
@richardtoms100
fix: remove webhookUrl from settlement webhook payload
dfffd1c 
Closes Betta-Pay#608

buildSettlementWebhookData included `webhookUrl: s.webhookUrl` in the
`data` block sent to merchants for settlement.completed/failed
events. That URL is the internal delivery target — often a Vercel
preview URL, an ngrok tunnel, or another internal hostname — not
settlement data, and mirroring it back to the merchant that
configured it leaks infrastructure topology for no reason. Internal
delivery routing never went through this payload in the first place
(the call sites in services/settlement-engine/src/index.ts read
`webhookUrl` straight off the `Settlement` row), so removing it from
the projection has no effect on delivery.

Added a regression test asserting the field is absent (not just
undefined-valued) from the built payload even though the source row
always carries it, and updated the docs/INDEXER_AND_WEBHOOKS.md
example payload + added a short note explaining the omission.

Also fixed an unrelated pre-existing bug this file was already
carrying, needed to get it compiling/testing at all: it imported
`logger` from `@bettapay/validation`, which has never exported a
bare `logger` singleton (this codebase logs via `fastify.log`, which
a standalone pure function like this one has no access to) — swapped
to `console.error` for this one already-existing corrupt-feeSnapshot
log line rather than changing the function's signature.

`cargo`-equivalent here: `node --loader ts-node/esm src/webhook-
payload.test.ts` could not be run end-to-end in this environment —
completedRow()'s test fixture transitively calls
computeSettlementAmounts() in settlement-amounts.ts, which throws
`ReferenceError: BN is not defined` (a separate, pre-existing,
unrelated bug: that file imports `BigNumber` but calls an undefined
`BN` at two call sites — confirmed via git stash against a clean
checkout, not something this PR touches). Verified this change is
correct via `tsc --noEmit` producing the same pre-existing error set
as a clean checkout (no new errors from this file) plus manual review
of the projection and the new test's assertions.
@richardtoms100
test: strengthen wallet challenge concurrent-consume coverage to 10 c…
a0f28f1 
…alls

Closes Betta-Pay#611

Issue Betta-Pay#611's premise (a separate wallet-auth-challenge.ts using an
untested Lua GET+DEL script, alongside wallet-challenge-store.ts's
tested getdel) is stale: issue Betta-Pay#605 already consolidated both into
the single WalletChallengeStore in wallet-challenge-store.ts, which
this codebase's real /api/auth/challenge and /api/auth/verify routes
(services/api-gateway/src/index.ts) actually call — and that
consolidated implementation uses a Lua GET+DEL script (Redis's
GETDEL command needs 6.2+; the Lua script works on any version), not
`getdel`. Its wallet-challenge-expiry.test.ts already covered
concurrent consume, just at 2 parallel calls rather than the 10 this
issue's acceptance criteria asks for — strengthened that test.

Deleted wallet-auth-challenge.test.ts: it doesn't import or exercise
WalletChallengeStore or the real gateway routes at all — it builds
its own throwaway Fastify app with hand-rolled route handlers that
reimplement challenge issuance/verification directly against
`Redis.prototype.get/set/del` stubs, using a plain get-then-del
(genuinely non-atomic, unlike the real consume()). Keeping a
"wallet auth challenge" test suite around that passes without
touching any of the actual production code path is worse than no
test: it reads as coverage for something it doesn't cover, which is
the same class of blind spot Betta-Pay#611 was raised about in the first
place.

`cargo`-equivalent here: could not execute
wallet-challenge-expiry.test.ts's route-level tests in this
environment — services/api-gateway/src/index.ts has a pre-existing,
unrelated syntax error (`Modifiers cannot appear here` at line 1390)
that fails both `tsc --noEmit` and the ts-node/esm runtime loader
identically before and after this change (confirmed via git stash
against a clean checkout). The WalletChallengeStore-level tests
(store.consume() directly, not through the routes) don't depend on
index.ts loading and were verified logically against the existing,
already-passing 2-parallel-call version of the same test.
@richardtoms100
fix: enforce merchant ownership on indexer webhook test endpoint
7e2bc4c 
Closes Betta-Pay#624

The merchant-facing route (services/api-gateway/src/index.ts, POST
/api/webhooks/:id/test) already correctly rejects a cross-merchant
test with 403 before ever calling the indexer — verified by reading
it, not assumed. So the exact end-to-end attack this issue describes
(merchant A poisoning merchant B's webhook status) is not currently
reachable through the public API. The indexer's own internal
/api/webhooks/:id/test (services/indexer/src/index.ts) — the file
this issue names — genuinely had no ownership check of its own
though: it sits behind `serviceAuth` (verifies the caller is a
trusted internal service, e.g. api-gateway) but never checks *which*
merchant the caller is acting on behalf of, and
WebhookSubscription.merchantId is never even referenced anywhere in
that file. That's a real defense-in-depth gap — relying on exactly
one caller (api-gateway) to always pre-check ownership before every
future internal caller ever added is fragile.

Wired the already-authenticated merchant id through: api-gateway's
route now forwards its own verified `payload.merchantId` to
indexerClient.testWebhook (a new required parameter, sent as
`?merchantId=`, mirroring the same forwarding convention
getPaymentEvents already uses for /api/events). The indexer route
checks it against `existing.merchantId` and returns 403 without
mutating lastTestedAt/lastTestStatus on a mismatch. A subscription
with `merchantId: null` (global/system) has no owner to check against
at this layer and is left as-is — the same gap already exists one
layer up if a global subscription is ever exposed to the merchant-
facing route, which is unrelated to this fix.

Added tests: cross-merchant test → 403, no mutation of
lastTestedAt/lastTestStatus; own-subscription test → 200, status
updated normally.

Also fixed an unrelated pre-existing bug in the same file that was
blocking every test in it from even loading: `encryptField`/
`decryptField` were imported from `@bettapay/shared-validation`, a
package name that doesn't exist anywhere in this workspace (the real
package is `@bettapay/validation`, which every other import in this
file already correctly uses) — merged them into the existing
`@bettapay/validation` import. Same broken package name was also
blocking scripts/encrypt-existing-secrets.ts and
settlement-engine/src/{settlement-amounts,webhook-payload}.ts;
fixed all four for the same reason (confirmed pre-existing and
unrelated via git stash against a clean checkout).

`node --loader ts-node/esm src/webhook-test.test.ts` (indexer) —
21/23 passing, all 5 new/changed assertions for this fix pass; the 2
failures are in the pre-existing, unmodified first test in this file
("webhook test succeeds ... returns 200") and are a `.end() already
called` race after its own assertions already passed — this test
suite could not run at all before the `@bettapay/validation` import
fix above, so this flake was previously unknown/unexercised rather
than a regression from this change.
@richardtoms100
fix: unblock building/testing the workspace (unrelated pre-existing b…
be028b9 
…ugs)

Not part of Betta-Pay#607/Betta-Pay#608/Betta-Pay#611/Betta-Pay#624 — these were blocking verification of
the actual fixes in this PR and are fixed here only for that reason.

- scripts/encrypt-existing-secrets.ts, settlement-amounts.ts: same
  `@bettapay/shared-validation` → `@bettapay/validation` fix as the
  previous commit (see that commit message for the full explanation).
- shared/validation/schemas.ts: MerchantSettings' `.refine()` checked
  `data.feeSchedules`, but the object schema never declared a
  `feeSchedules` field at all — `tsc` correctly failed the build,
  which every service in this workspace depends on. Added it as an
  untyped `z.unknown().optional()` passthrough rather than guessing
  its intended shape (it isn't implemented anywhere else in this
  codebase yet); this preserves the exact prior runtime behavior
  (the refine's `data.feeSchedules` was always `undefined` before
  too, since the field didn't exist), it just now type-checks.

All three confirmed pre-existing and unrelated via `git stash`
against a clean checkout before touching anything.
@therealjhay
therealjhay merged commit b3a9fe0 into Betta-Pay:main Sep 1, 2026
4 of 6 checks passed
@grantfox-oss grantfox-oss Bot mentioned this pull request Sep 1, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment