fix(security): P1 money-oracle hardening — fail-closed BCI, amount/account match guard, no 172.* admin #1

Open

kavi wants to merge 1 commit from fix/security-audit-p1 into main

pull from: fix/security-audit-p1

merge into: kavi:main

kavi:main

kavi:feat/global66-webhook

Conversation 0 Commits 1 Files changed 5 +210 -35

kavi commented 2026-07-12 07:33:20 +00:00

Owner

Copy link

Security P1 fixes — kua-cashier (the payment-confirmation "money truth")

Fixes the P1s (plus low-risk P2/P3 wins) from the read-only audit
(sec-audit-kua-cashier.md). Every finding was verified against source first.
kua-cashier fires real payment.confirmed callbacks that release downstream goods,
so each of these was a direct free-goods / underpayment vector.

Reviewed (self-reviewed diff), NOT deployed. No service run, no prod touched.

Fixes → findings

Finding	Where	Fix
P1-1 BCI webhook fails OPEN when secret empty (the one public endpoint)	server.js /bci/notify	Fail closed: 503 when BCI_WEBHOOK_SECRET unset; verify secret from a header first (x-webhook-token/apikey/x-apikey), ?token= kept only as BCI URL-callback fallback and redacted from logs; timingSafeEqual compare.
P1-2 reference-code match ignores amount AND account (pay 1 peso → fully paid; cross-account confirm)	matcher.js tier 1	Require inbound.amount >= exact_amount AND matching account_id on the reference path; otherwise fall through.
P1-3 any 172.* / loopback source IP auto-granted tag:admin, no creds	server.js auth hook	Removed private-range/loopback auto-admin; every non-public route requires a valid bearer or a Tailscale-verified identity. Set trustProxy: false so req.ip can't be spoofed via forwarded headers.
P2-1 auth hook falls through (no 403) when NODE_ENV != production	server.js auth hook	Now default-deny in every environment; removed the !DEV gate.
P2-2 BCI secret carried in URL query string → leaks to logs	server.js logger	Secret preferred in a header; query fallback redacted from all request logs via a pino req serializer.
P2-3 no audit of who mutates money	server.js + migrations/003_admin_audit.sql	New append-only admin_audit table records actor (bearer / Tailscale identity), action, target for intent create/cancel, account upsert, inbound inject, manual match/expire.
P3-4 non-timing-safe secret compares	util.js timingSafeStrEq	Admin bearer + BCI secret now constant-time.

Preserved (verified intact)

• Race-safe single-use match transaction (dispatcher.js) — untouched.
• Timing-safe, fail-closed Global66 key check (global66.js) — untouched; adopted the same pattern for BCI.

Tests / checks

• node --check passes on all changed files.
• New test/matcher.test.mjs (4 tests, DI'd query): valid code + full amount + same account → match; 1-peso underpay → NO match; different account → NO match; overpay → match. All pass.
• Existing test/global66.test.mjs (6 tests) still passes (GLOBAL66_API_KEY=testkey).
• Local Node v20; container targets Node 22.

Deploy notes (for the human merger)

• migrations/003_admin_audit.sql is additive (CREATE TABLE IF NOT EXISTS), applied on next start by the existing migrate().
• /bci/notify now fails closed: confirm BCI_WEBHOOK_SECRET is set in kua-vault kua-cashier/prod before rollout, or BCI callbacks will 503.
• BCI header verification is best-effort primary; the redacted ?token= fallback keeps the live BCI URL-callback flow working, so no BCI re-subscription is required.

🤖 Generated with Claude Code

## Security P1 fixes — kua-cashier (the payment-confirmation "money truth") Fixes the P1s (plus low-risk P2/P3 wins) from the read-only audit (`sec-audit-kua-cashier.md`). Every finding was **verified against source first**. kua-cashier fires real `payment.confirmed` callbacks that release downstream goods, so each of these was a direct free-goods / underpayment vector. **Reviewed (self-reviewed diff), NOT deployed.** No service run, no prod touched. ### Fixes → findings | Finding | Where | Fix | |---|---|---| | **P1-1** BCI webhook fails OPEN when secret empty (the one public endpoint) | `server.js` `/bci/notify` | Fail **closed**: `503` when `BCI_WEBHOOK_SECRET` unset; verify secret from a **header** first (`x-webhook-token`/`apikey`/`x-apikey`), `?token=` kept only as BCI URL-callback fallback and **redacted from logs**; `timingSafeEqual` compare. | | **P1-2** reference-code match ignores amount AND account (pay 1 peso → fully paid; cross-account confirm) | `matcher.js` tier 1 | Require `inbound.amount >= exact_amount` **AND** matching `account_id` on the reference path; otherwise fall through. | | **P1-3** any `172.*` / loopback source IP auto-granted `tag:admin`, no creds | `server.js` auth hook | Removed private-range/loopback auto-admin; every non-public route requires a valid **bearer** or a **Tailscale-verified** identity. Set `trustProxy: false` so `req.ip` can't be spoofed via forwarded headers. | | **P2-1** auth hook falls through (no 403) when `NODE_ENV != production` | `server.js` auth hook | Now **default-deny in every environment**; removed the `!DEV` gate. | | **P2-2** BCI secret carried in URL query string → leaks to logs | `server.js` logger | Secret preferred in a header; query fallback **redacted** from all request logs via a pino `req` serializer. | | **P2-3** no audit of *who* mutates money | `server.js` + `migrations/003_admin_audit.sql` | New append-only `admin_audit` table records actor (bearer / Tailscale identity), action, target for intent create/cancel, account upsert, inbound inject, manual match/expire. | | **P3-4** non-timing-safe secret compares | `util.js` `timingSafeStrEq` | Admin bearer + BCI secret now constant-time. | ### Preserved (verified intact) - Race-safe single-use match transaction (`dispatcher.js`) — untouched. - Timing-safe, fail-closed Global66 key check (`global66.js`) — untouched; adopted the same pattern for BCI. ### Tests / checks - `node --check` passes on all changed files. - New `test/matcher.test.mjs` (4 tests, DI'd query): valid code + full amount + same account → match; **1-peso underpay → NO match**; **different account → NO match**; overpay → match. All pass. - Existing `test/global66.test.mjs` (6 tests) still passes (`GLOBAL66_API_KEY=testkey`). - Local Node v20; container targets Node 22. ### Deploy notes (for the human merger) - `migrations/003_admin_audit.sql` is additive (`CREATE TABLE IF NOT EXISTS`), applied on next start by the existing `migrate()`. - `/bci/notify` now **fails closed**: confirm `BCI_WEBHOOK_SECRET` is set in kua-vault `kua-cashier/prod` before rollout, or BCI callbacks will `503`. - BCI header verification is best-effort primary; the redacted `?token=` fallback keeps the live BCI URL-callback flow working, so no BCI re-subscription is required. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

kavi added 1 commit 2026-07-12 07:33:20 +00:00

fix(security): P1 money-oracle hardening — fail-closed BCI, amount/account match guard, no 172.* admin ... 58ef6dbb2c

The confirmation oracle fired real payment.confirmed callbacks on several
fail-open / under-validated paths. Verified each against source, fixed minimally.

- P1-1 (server.js /bci/notify): the token check was SKIPPED when the secret was
  empty → the one public endpoint failed OPEN. Now fails CLOSED (503 when
  BCI_WEBHOOK_SECRET unset), verifies the secret from a request header first
  (query ?token= kept only as BCI URL-callback fallback and redacted from logs),
  timing-safe compare.
- P1-2 (matcher.js reference tier): a valid 6-char code alone marked an intent
  fully paid regardless of amount or account. Now requires
  inbound.amount >= exact_amount AND matching account_id before confirming.
  Added regression tests (underpay / cross-account / overpay).
- P1-3 (server.js auth hook): any 172.* (and loopback) source IP was auto-granted
  tag:admin with no credentials — lateral-movement compromise from any co-tenant
  container. Removed private-range/loopback auto-admin; every non-public route now
  requires a valid bearer or a Tailscale-verified identity. Set trustProxy=false
  so req.ip cannot be spoofed via forwarded headers.
- P2-1: auth hook is now default-deny in every environment (removed the
  NODE_ENV!=production bypass that let the whole check fall through).
- P2-2: BCI secret preferred in a header; query-string fallback redacted from all
  request logs via a pino req serializer.
- P2-3: new append-only admin_audit table records WHO mutated money
  (intent create/cancel, account upsert, inbound inject, manual match/expire),
  attributed to the bearer or Tailscale identity.
- P3-4: timing-safe admin bearer + BCI secret comparisons (timingSafeStrEq).

Preserves the race-safe single-use match transaction (dispatcher.js) and the
timing-safe fail-closed Global66 check (global66.js). Reviewed, NOT deployed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

This pull request can be merged automatically.

This branch is out-of-date with the base branch

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin fix/security-audit-p1:fix/security-audit-p1

git switch fix/security-audit-p1

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main

git merge --no-ff fix/security-audit-p1

git switch fix/security-audit-p1

git rebase main

git switch main

git merge --ff-only fix/security-audit-p1

git switch fix/security-audit-p1

git rebase main

git switch main

git merge --no-ff fix/security-audit-p1

git switch main

git merge --squash fix/security-audit-p1

git switch main

git merge --ff-only fix/security-audit-p1

git switch main

git merge fix/security-audit-p1

git push origin main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

kavi/kua-cashier!1

No description provided.