Repository navigation
security: the remaining 49 audit findings (batches 4-28, continues #277) - #278
Merged
Merged
Conversation
From the 2026-08-19 security audit (docs/findings/). Adds TODO-vulns.md as the remediation ledger for all 338 findings, and fixes the three Criticals plus the five High findings that share their code paths. F-1.3-13 — subscription activation read merchant_id and plan_id out of a caller-supplied metadata blob and never compared the amount paid against the plan price, so a $0.01 payment activated the $490/yr plan on any merchant UUID the payer named. Activation now credits the payment row's own merchant_id, validates the plan against SUBSCRIPTION_PRICES, and requires the USD amount to cover it. NEW-04 — payee resolution matched merchants by email with no scoping and then upserted merchant_wallets, overwriting a victim's payout address. The email fallback now refuses non-platform accounts, and persistPayout refuses to write a payout destination for any account the platform did not provision. F-1.3-01 / CP-005 — the Lightning x402 proof was checked against itself: sha256(preimage) == paymentHash, both from the payer, and settle then answered settled:true having verified nothing. Both routes now require a matching incoming, settled ln_payments row for the calling business that covers the price. REC-C-01 — scheme and network are independent attacker-set fields and dispatch fired on either, so a bolt12/ethereum proof ran the Lightning verifier while the response claimed amountAuthenticated. Dispatch is on network alone, with the scheme table shared between verify and settle so they cannot drift. F-1.3-02, F-1.3-03, R3-X1 — price binding now requires expected.payTo and pins expected.asset, and the Stripe verifier compares the amount Stripe reports rather than the payer's self-declared one. Integrator-visible: expected.payTo is now required on v1 verify, matching what v2 already required. 26 new regression tests; full suite green (303 files, 4309 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Second remediation batch against docs/findings/. F5-L4-01 — cleanOrphanedWallets() deleted every wallets row with no transactions on every --execute run. That table is the self-custodial web-wallet store with no merchant_id, so nothing in it can be attributed to a spam signup, and "zero transactions" describes any wallet not yet funded. Now behind --prune-empty-wallets with a 90-day floor, and the header's false safety claim is corrected. W-07 — GET /api/lightning/offers had no auth, optional business_id and an uncapped limit, so one anonymous request dumped every merchant's Lightning offers and revenue. Now authenticated, business-scoped and capped at 100. CP-002 — issuer self-registration set active:true with no identity or domain check and stored the API key in cleartext. Registers inactive now, and persists only the key hash. G-R-07 — /api/oauth/userinfo asserted email_verified:true unconditionally. merchants has no email_verified column in production, which also meant /api/oauth/token's select referencing it errored on every call, so the OAuth ID token has been carrying neither email nor name for any user. All three sites now select real columns only and report false while no verification flow exists. E-03 — the recurring card rail carried a comment claiming application_fee was applied and never set the field, so the platform collected nothing on every subscription. Sets subscription_data.application_fee_percent from the business tier, and spreads caller metadata first so platform keys always win. F-1.1-08 — two invoice paths compared balance >= expected * 0.99 instead of calling the shared isSufficientPayment. Both now use the helper. Two existing tests asserted the vulnerable behaviour and have been inverted: "marks invoice as paid with 1% tolerance", and two oauth tests named "should return email_verified as false" that asserted true. Full suite green (303 files, 4315 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Third remediation batch against docs/findings/ — the report's section 2.5 class, where public documentation states a security property as implemented in direct contradiction with the code. That is the place a partner or auditor looks to decide whether to trust the platform, so a false claim there is worse than the missing control it hides. DOC-01 — layout.tsx titled the whole product "Non-Custodial Crypto Payment Gateway" in the page title, OpenGraph and Twitter card. The browser wallet is non-custodial; the gateway is not, since the platform holds encrypted keys for payment addresses, escrow and collection between receipt and forwarding. F7-01 — docs/SECURITY_KEYS.md checked off audit logging and memory scrubbing, and docs/SECURITY.md described an audit trail in the present tense. Verified: no audit table, no append-only log, no key-access record anywhere in src/, and clearSensitiveString has no callers. Both documents now say so. Building the audit trail (AUD-01) remains open — this corrects the claim, not the gap. GAP-02 — strix_runs/ and three strix_*.log files were tracked in the public repo. .gitignore listed them but was added after they were committed. Untracked; purging them from history remains a decision. F5-L4-03 — scripts/test-spam-detection.ts carried real merchant names, personal Gmail addresses and live corporate domains as fixtures. All now synthetic; each case exercises a shape, not a person. Also fixes a stale expectation in that script that had been failing since the dotted_gmail weight was deliberately reduced from 35 to 20 (confirmed against the original fixture set, so unrelated to the PII swap). 21/21 now. Full suite green (303 files, 4315 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lpers
Fourth remediation batch against docs/findings/. Opens the audit's section 2.7
workstream: eighteen findings that all share one shape — authenticate the
caller, then trust a tenant id the caller supplied.
Patching each route individually leaves the shape in place for the nineteenth,
so this adds two shared helpers and moves routes onto them:
- src/lib/auth/tenant-scope.ts — resolveBusinessScope() always authorizes a
requested business id rather than accepting it, and infers one only when the
caller has exactly one business. The code it replaces was worse than a guess:
`businessId || authResult` passed a merchant id where a business id was
expected, so the lookup matched nothing or matched an unrelated row.
- src/lib/escrow/access.ts — callerOwnsEscrow(), shared by escrow/[id] and
escrow/[id]/events, which are clones of each other.
Closed:
B-01, C-01, NEW-13 — the Stripe api-keys and webhooks routes let a query-string
or body business_id override the authenticated user. POST /stripe/webhooks
creates an endpoint on the named business's Connect account and returns its
signing secret, so this handed over a live feed of another merchant's Stripe
events pointed at a URL of the attacker's choosing.
CP-010, NEW-14, G-1.2-13 — usage/rates (GET/POST/DELETE) and usage/history had
no ownership check, while their siblings credits and deduct in the same
directory did.
CP-014 — reputation/receipts was an unauthenticated select('*'). The endpoint
stays public, since a trust graph is only useful if a counterparty can check it,
but it now returns a narrow projection: no amount, escrow_tx, buyer_did,
platform_did or signatures, capped at 200 rows.
CP-021, NEW-15 — escrow/[id] and its events sibling authenticated the caller and
then fetched by UUID with no ownership check, so any merchant's API key read any
escrow. Both answer 404 rather than 403 so they are not existence oracles.
Full suite green (304 files, 4327 tests).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fifth remediation batch against docs/findings/. Two distinct mistakes, both about a credential being trusted beyond what it was issued for. A capability check that defaulted to read. C-03 / G-1.2-01: payment-methods/manual called verifyBusinessAccess with no capability argument, and the default is business.read, which every team member has including readonly. The write was therefore gated at read level, so a read-only member could rewrite the Venmo, Cash App or Zelle handle customers are told to pay. That is a funds destination and the project's invariant is that moving funds is owner-only, so the write now requires funds.move. The sibling import route wrote the same field at settings.manage; raised to match. API keys acting with account authority. L7A-01 and SUB-01: did/claim, did/delegate and subscriptions/status DELETE checked no scopes, and API_SCOPES has no DID or billing scope to check even if they had. A business key resolves to the owning merchant, so any key — including a read-only wallet:read one — could rebind the merchant's DID, mint DelegatedAuthority credentials carrying wallet:transfer and escrow:settle, or cancel the Professional subscription. A scoped key must not grant authority stronger than itself; all three now require session auth. The existing isMerchantAuth ternaries had identical branches, so the type guard was present and doing nothing. Adds src/lib/p2p/platform-ownership.ts, shared by CP-003 (did/override) and CP-023 (reputation/merchant-wallet). Both acted on any merchant_id in the body on nothing but a valid issuer key, rebinding a merchant's identity or writing their payout wallet so a charge in an unconfigured currency settled to the caller. Both now require the merchant to be platform-provisioned and provisioned by that specific platform — the same guard NEW-04 needed, which is why it is shared rather than copied a third time. Full suite green (304 files, 4327 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… paid payments
Sixth remediation batch against docs/findings/. Each finding here was checked
against the live database rather than the migrations directory, which is known
to drift from production.
L8-02 — payments.payment_address_id does not exist, yet three card routes
inserted it and src/lib/supabase/types.ts still declared it. PostgREST rejects
an insert naming an unknown column, so every card payment failed with a 500
before reaching Stripe. Removed from the routes and from the type; leaving it on
the type is what kept the routes writing it.
L4-NEW-02 — the address-generation failure path wrote status 'failed', which
payments_status_check does not permit, and never checked the result. The row was
left pending forever, on no dashboard's problem list. Now writes 'expired', the
terminal state the schema actually has, records failure_reason in metadata, and
logs if that write fails too.
NEW-L5-2 — blockchainSchema accepts USDT, USDC and USDC_BASE and the code
generates addresses for all three, but payments_blockchain_check permitted none
of them, so payment creation failed outright for those currencies. Widened by
migration 20260819120000, applied to production and verified. Prod holds zero
rows for those three values while every other supported chain has some.
BL-01 — every checker in monitor-balance.ts answered { balance: 0 } on an RPC
error, exactly as it does for an address that has genuinely received nothing,
and processPayment expired the payment on that. A transient provider outage
during the expiry window therefore marked fully-paid payments expired, with the
customer's funds sitting at the address and nothing retrying. BalanceResult now
carries an error field, all 27 failure paths set it, and expiry refuses to fire
on an unreadable balance. The two zeros that are real — an unactivated XRP
account and an unused ADA address — are deliberately left untagged.
Also confirms two findings are already closed: H-R-01 (checkDOGEBalance is fully
implemented, not the stub described) and CP-P5 (isBusinessPaidTier resolves tier
through the merchant subscription and never reads businesses.tier).
Full suite green (304 files, 4329 tests).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Seventh remediation batch against docs/findings/. N-01 — screenCheckout was called from exactly one of the seven code paths that create a real card charge. The other six now screen before the Stripe session exists, which is the last point a payment can be stopped without Stripe ever seeing it, and each forces 3-D Secure on a verify decision: payments/widget/create (a public, secret-free embed and the most exposed of the seven), payments/create's card branch, payments/create-for-merchant, p2p/request's Stripe branch, and lib/payments/invoice-stripe. REC-D-05 — the p2p Stripe branch is one of those six. Its caller is a platform issuer key rather than an authenticated merchant, which made it the least supervised card path in the codebase. FR-01 — screening failed open. Any error anywhere returned decision 'allow', and checkBlocklist returned the same null for a database error as for "no entry matches", so an unreachable blocklist waved through every entry on it — entries usually added after a real chargeback. The reasoning behind failing open was sound, since a broken fraud check must not become a payment outage, but 'allow' was the wrong conclusion from it. Both paths now degrade to 'verify', which keeps payments flowing while forcing 3-D Secure and moving liability for a stolen card back to the issuer. CP-P5 — corrects my own earlier assessment. I marked this already-fixed after checking isBusinessPaidTier, which does resolve tier through the merchant subscription. But payments/create has a separate inline fee calculation that selected businesses.tier, the column that does not exist, so PostgREST rejected the query, the row came back null, and every business was charged the 1% free-tier rate on that rail regardless of plan. The test covering it expressed the pro tier as tier: 'pro' on the businesses row, so it passed against a column that was never read. Full suite green (305 files, 4332 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eighth remediation batch against docs/findings/. proposal-templates.ts had an escapeHtml and used it on every interpolated field. Three siblings did not have it and interpolated merchant-supplied strings raw, so the fix is a shared src/lib/email/escape.ts rather than a fourth copy. NEW-24 — invoice-templates.ts interpolated business name, invoice number, notes, client name and tx hash into the HTML body with no escaping. Subject lines are deliberately left unescaped: they are plain text, and escaping them would show the recipient a literal &. G-1.2-09 — the team invitation email interpolated scopeLabel, which is the business or organization name, into a message delivered to an arbitrary address the same caller supplies. Creating a merchant account is free and unverified, so that was a way to send attacker-authored HTML from the platform's own sending domain, to anyone, with no rate limit. F5-L4-02 — scripts/daily-stats-email.ts put signup name and email into the internal daily report unescaped, and its recipients are the operations team. The helper is duplicated there deliberately: the script runs standalone under tsx and does not resolve the app's path aliases. escapeUrl is separate from escapeHtml because escaping characters does not help with a javascript: or data: href — those survive HTML escaping intact and still execute when clicked, so anything not clearly http(s) or mailto is dropped. Full suite green (306 files, 4342 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ninth remediation batch against docs/findings/. select count(*) from webhook_logs against production returned 0. Not a few gaps — the table has never held a row. The webhook delivery audit trail has never recorded a single attempt, and nothing surfaced that because delivery itself is unaffected by the logging failing. Three separate faults, all of which had to be fixed before one insert could succeed: V-01 — url and payload are NOT NULL with no default, and the application writes webhook_url instead, so every insert failed on a not-null violation. The two column sets are duplicates from different eras of the schema; the code now writes both so either name reads correctly. L8-01 — payment_id had its NOT NULL dropped so escrow events could be logged, but the foreign key to payments(id) was never dropped alongside it. REC-D-02 — the escrow path passed an escrow id as payment_id, with a comment saying it was reusing the column, which violated that foreign key. Migration 20260819130000 relaxes the legacy pair and gives escrow its own escrow_id column with its own FK rather than borrowing one, so the payments FK stays meaningful. A CHECK enforces one subject per row so the column-borrowing cannot quietly return. Applied to production and verified with a probe insert, which succeeded and was then deleted. logWebhookAttempt now logs when it cannot record, instead of returning an error every caller discarded. That silence is why an empty audit table went unnoticed. Full suite green (306 files, 4342 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Tenth remediation batch against docs/findings/. This is the audit's section 2.1
pattern — a strong guard that most of its own consumers bypass.
F9-01 — requireEncryptionKey was correct and protected 4 of 13 real encryption
call sites. The other nine, including the entire custody hot path, hand-rolled
const k = process.env.ENCRYPTION_KEY;
if (!k) return { success: false, error: 'Encryption key not configured' };
which establishes that a key is present and nothing about whether it is usable.
An all-zero or deadbeef-repeated key passed all nine.
Migrated: hd-wallet, system-wallet (two sites), secure-forwarding,
escrow/service, business-collection, business/service, webhooks/secret, and both
Stripe webhook routes.
Two things kept the guard from spreading, and both are fixed. It throws while
those call sites return result objects, so tryRequireEncryptionKey now gives
them a form that fits. And the repository's own fixtures used values the guard
rejects — the one in secure-forwarding.test.ts was literally a KNOWN_WEAK_KEYS
entry and escrow/service.test.ts used a 36-character non-hex string — so
adopting the guard anywhere would have turned the suite red. Both replaced with
a real 32-byte hex key.
keyHashPepper() in scoped-keys.ts is deliberately left reading the raw value and
documented as such: it is a pepper for an HMAC, not an encryption key, and its
value is baked into every api_key_hash already stored, so refusing a weak value
at read time would lock out every integrator at once. The right fix there is a
rotation with re-hashing.
Full suite green (307 files, 4353 tests).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eleventh remediation batch against docs/findings/. F6-01 — COINPAY_REF is the only user-facing mitigation against an auto-upgrade that pulls a mutable branch every five minutes, and it silently did nothing. Two re-invocation paths dropped it: the auto-upgrade poll ran a bare `curl … | sh -s -- update`, and `coinpay update` in the wrapper did the same. Both defaulted COINPAY_REF back to master, so a host pinned to a tag tracked master anyway with no indication. Both now bake in the ref they were installed from, verified by running write_self_upgrade_helper in isolation and reading the generated script. COINPAY_SHA256 is deliberately not carried across: a checksum pins one specific archive, so reusing it for a later version guarantees a mismatch and would break every upgrade. W-01 — partially addressed. An unpinned install now warns on screen that it is following a mutable branch and prints the command to pin a release. The default ref itself is unchanged and needs a release-process decision: the installer compares against packages/sdk/package.json at the ref, so defaulting to a tag whose SDK version trails master would leave the upgrader idle or looping. Also fixes a set -e hazard introduced and caught while writing this: a `[ test ] && VAR=0` line would have aborted the installer for exactly the users who did pin a ref. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twelfth remediation batch against docs/findings/. The rate-limit infrastructure already existed and was well tuned; these endpoints simply never called it. NEW-06 — none of the four WebAuthn routes had any limit. login-options answers differently for a registered and an unregistered email, so unlimited it is a free user-enumeration oracle over the whole merchant base. WW-02 — the web-wallet derive route, whose five sibling mutating routes are all limited. Each call performs HD derivation and writes a row. REC-C-04 — x402 verify and settle. Ledger bloat, and on the Stripe rail each call spends our own Stripe API quota. REC-C-05 — GET /api/swap/quote is anonymous and each quote costs two calls to the ChangeNOW third-party API, so it was a lever for exhausting our own quota with no credential at all. L7A-03 — reputation/attest needed more than a limit. It was completely unauthenticated and took attester_did from the body. submitAttestation does check the attester is a party to the receipt, so this was never unbounded forgery, but knowing a receipt id and the two DIDs on it was enough to attest as either party — and reputation/receipts used to hand out exactly that (CP-014). It now requires authentication and proves the caller controls the DID through the existing merchant_dids link, rather than introducing a new signature scheme. Also fixes something not in the register: checkRateLimit and checkRateLimitAsync returned allowed: true in silence for an unknown category, so a typo in a category name disabled the limit entirely with nothing to notice. It still allows, since a config mistake must not take payments down, but logs. Full suite green (307 files, 4353 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Thirteenth remediation batch against docs/findings/. NEW-01 — createEscrow resolves a wallet address to the owning merchant's email, which is convenient because it lets an escrow notify a counterparty identified only by address, and then returned that email to the caller. Anyone able to create escrows could therefore map any on-chain address to the merchant who owns it, across the whole merchant base — which is precisely the input NEW-04 needed. Auto-resolved emails are now redacted from the response; the row keeps them so notification still works. L5-01 — generateEscrowAddress read next_index and wrote next_index + 1 with no condition on what it had read, and keyed on cryptocurrency while the payment flow keys on the derivation family (ETH/POL/BNB/USDT/USDC share one). The two counters advanced independently over the same key space, so the collision was by construction rather than merely under contention. Now uses acquireFamilyIndex, the compare-and-swap helper the payment flow already used. R3-DIN-03 — escrow-monitor writes status 'settle_failed', which escrows_status_check did not permit, so the UPDATE was rejected and the escrow was left 'released' with nothing signalling the failure. Verified both ways with a probe UPDATE that rolls itself back: rejected before the migration, accepted after. The write now checks its result. Worth recording: the register attributes this to the constraint being NOT VALID. That is wrong. NOT VALID only skips checking rows that already existed when the constraint was added; it does not exempt new INSERTs or UPDATEs. The pre-existing settle_failed rows are what made it look unenforced. ESC-NEW-01 is reclassified as a decision rather than a fix: both dispute columns exist with no writer as reported, but production holds zero escrows in disputed status, so no funds are frozen today. An arbiter flow needs product decisions about who arbitrates and on what evidence. Full suite green (307 files, 4353 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fourteenth remediation batch against docs/findings/. Both findings share a failure mode worth naming: the test asserted the same wrong thing the code did, so a path that could never work in production had full, passing coverage. NEW-L5-01 — CoinPayClient.request takes (endpoint, options). Every call in the SDK's card-payments.js passed three arguments in the shape request(method, path, body), so endpoint received 'POST', the URL became baseUrl + 'POST', and options received the path as a string. All five calls failed against a real client, making release and refund of a card escrow unreachable through the SDK. The tests mocked request and asserted the three-argument shape, so a mock that accepts anything certified a call that could never work. F4-01 — StatusMapper::MAP had mark_paid entries only for payment.completed and payment.overpaid. The backend emits neither; it emits payment.confirmed, payment.forwarded, payment.failed and payment.expired. Every real webhook fell through the ?? 'ignore' default, so automated invoice crediting never fired for any transaction — and ignore is also the correct answer for events we genuinely skip, so nothing looked wrong. All twelve existing PHP tests covered event types the backend never sends. Maps the real events, keeps the legacy keys since a replayed historical delivery may carry them, and logs an unmapped event rather than letting it disappear. Adds a PHP test that walks the full emitted-event list so a new backend event type fails the suite instead of being silently ignored. Full suite green (307 files, 4353 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…sclosure Fifteenth remediation batch against docs/findings/. L-01 — the web-wallet spend controls failed open twice. A whitelist and a daily spend limit are the only two things between a compromised session and the balance, and both were skipped when a query failed: one path returned allowed true with the comment "fail open for now", the other wrapped the limit check in if (!error && todayTxs) so an error simply skipped it. getSettings creates a default row when none exists, so reaching the first path means a real database failure rather than "no settings configured". Both now deny. A test named "should allow if settings fail to load (fail open)" asserted the vulnerability directly and is inverted. NEW-WW34-01 — checkTransactionAllowed took a chain parameter and never used it, so today's total summed raw amounts across every chain: 1 BTC and 1 DOGE counted as 2 against one limit. That blocks trivially cheap sends after one expensive one and lets a small-unit chain run far past the intended cap. The daily total is now scoped to the chain being spent on. WW-L4-01 — /api/wallets/lookup already withholds the email, but answered found true/false unauthenticated and unlimited, which is enough to sweep the merchant base address by address. It stays public, since a sender legitimately checks before paying, with a rate limit that makes bulk enumeration impractical. NEW-19 — /api/partners published the name, description and webhook host of every active business with a webhook configured. Configuring a webhook is a technical step, not consent to appear in a public directory. Migration 20260819150000 adds public_directory_opt_in defaulting to false. Note the visible side effect: the partners page is now empty. 28 businesses were being published and none opted in, because there was nothing to opt into. If consent exists out of band it needs an explicit backfill; I did not assume it on anyone's behalf. Full suite green (307 files, 4355 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…y chain
Sixteenth remediation batch against docs/findings/.
WW-01 — verifySignedTxBinding decoded EVM transactions and compared the
recipient only. A signed transaction paying the right address a different amount
was accepted and recorded as the prepared one, and everything downstream hangs
off that row: the wallet's history, the daily spend limit, fee accounting and
notifications all described a transaction that did not happen. Now compares the
value for native transfers and the second ABI argument for ERC-20 transfers,
using per-chain decimals.
The refusal condition was also reason?.startsWith('recipient '), matching on
message text, so rewording the reason string would have silently stopped it
refusing anything. The decoder now sets an explicit mismatch flag.
WW-03 — BTC, BCH, SOL and USDC_SOL fell through to "no decoder for <chain>" and
broadcast entirely unchecked. Both libraries needed were already dependencies:
BTC decodes with bitcoinjs-lib and sums the outputs paying the prepared address,
requiring at least the prepared amount — "at least" rather than "exactly one
output", because a real spend nearly always has change back to the sender.
SOL and USDC_SOL confirm the prepared recipient appears among the transaction's
account keys, which catches a wholesale substitution of the payee. The amount is
not checked there: it lives in instruction data whose layout depends on the
program, and that is not something to guess at in a broadcast guard, so it is
reported as unverified rather than claimed.
BCH still has no decoder, because bitcoinjs-lib does not support its
SIGHASH_FORKID variant as the codebase notes elsewhere. Left recorded as
unverified with the reason stated in the code.
Full suite green (307 files, 4361 tests).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…in CI Seventeenth remediation batch against docs/findings/. H-R-06 — baseChainFor mapped bare USDC to 'SOL', so a merchant configuring a plain-USDC payout had to supply a Solana address to pass validation. Every other module treats bare USDC as ERC-20 on Ethereum: monitor-balance checks it on the Ethereum RPC, rates/fees prices it off the Ethereum gas path, and address generation puts it on the Ethereum family. The validator and the money disagreed about which chain the payout was on. Validation now follows the money. H-R-08 — the currency list does diverge across modules, and hand-syncing eight copies fixes today and drifts again next month. CRYPTO_NAMES is already the declared operational source of truth, so currency-consistency.test.ts checks the others against it: every symbol must have a declared address family, the validator must accept that family and reject another so the check actually discriminates, and the static-fee fallback table must not carry chains the gateway no longer supports. Adding a symbol without recording a decision now fails the suite. That test immediately found something not in the register: USDC_BASE strips to 'BASE', which had no case in the validator, so it fell through to the null default meaning "no validator, trust the address". USDC on Base is a supported payment chain with a balance checker and a fee path, making it the one EVM variant whose payout addresses were accepted with no format check at all — a typo'd Base address would have been stored and paid to. Full suite green (308 files, 4365 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rifiable Eighteenth remediation batch against docs/findings/. A-03 — encryptProviderSecrets is called when a Boltz swap is created and stripProviderSecrets when history is listed, but decryptProviderSecrets had zero callers. The refund and claim keys went in and never came back out, so when a swap failed the HTLC funds could not be recovered through the product at all. The refund path exists on Boltz's side; the key needed to walk it was locked in our own database. Adds GET /api/swap/boltz/:id/recovery: owner-authorized, rate limited on its own budget, and it logs that key material was released without logging the keys. A separate route rather than extra fields on the status endpoint, because handing out key material is a distinct action that should be asked for explicitly and be visible separately in logs. W-06 — redeemScript and swapTree were declared on both Boltz response types and never read, so the lockup address was taken on faith. A substituted address would have been funded happily, and the refund key generated locally is worthless against it because that key belongs to a different script — the deposit would be unrecoverable, which is exactly what a refund key exists to prevent. Both creation paths now derive the address from the redeem script (P2WSH, P2SH-P2WSH and bare P2SH) and refuse to fund a mismatch. Taproot swaps carry a swapTree whose derivation needs the full tweak; those report "cannot check" and log rather than returning a false pass, keeping the distinction between checked and uncheckable explicit. Full suite green (309 files, 4370 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nineteenth remediation batch against docs/findings/. W-05 — paying a Lightning Address asks the recipient's LNURL server for an invoice and then paid whatever came back, undecoded. The amount actually paid was therefore whatever that server chose to put in the invoice, not the amount the sender entered and was shown, so a hostile or compromised LNURL endpoint could return an invoice for any amount up to the wallet's balance and have it paid silently. The minSendable/maxSendable check that was already present validates the amount we request, which the server is free to ignore. The amount now has to match. bolt11AmountMsat reads it from the invoice's human-readable part, a well-defined grammar in BOLT-11 that needs no bech32 decoding, so this answers one question without adding a full invoice decoder as a dependency. An amountless invoice — a donation invoice, where the payer chooses — and an unparseable one both return null, and the caller treats that as a refusal. "Cannot tell" is not agreement, and an amountless invoice is precisely the shape that would otherwise be paid blind. Full suite green (310 files, 4380 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…igning garbage Twentieth remediation batch against docs/findings/. F3-L3-01 — the extension's batch runner classified "already known" as a transient error and rebuilt the payment from scratch. But "already known" means the node HAS the transaction: the broadcast succeeded. Retrying therefore broadcast a second, real, duplicate payment. It is the most dangerous string in that list, because the response meaning "your money moved" was read as "try again". The error list conflated two different things and is now split. Retried: errors meaning the request never reached the chain — rate limit, timeout, 502/503/504, blockhash not found, block height exceeded. Not retried: errors meaning a transaction like ours already exists — already known, nonce too low, replacement transaction underpriced, missingorspent, txn-mempool-conflict. Every entry in the second group describes chain state that moved because such a transaction exists, which is exactly when a retry duplicates a payment. "already known" is now reported as sent, which is what it means. The existing test asserted that "nonce too low" was retryable. That assertion was the finding, and it is inverted. F3-L5-01 — the SDK's send() called signMessage(unsignedTx, privateKey): a generic message signature over the JSON of the unsigned transaction, posted as signed_tx. That is not a signed transaction on any chain, so the method failed for every integrator who called it, silently, by producing plausible output that only failed later at the node. It now throws with an explanation and points at the primitives that do work. Real serialisation is deliberately not implemented here: the SDK carries only @scure/@noble by design, correct RLP/PSBT/Solana encoding depends on the exact shape of prepareResult.unsigned_tx which the server defines, and none of it can be verified against a live node from this package. Shipping an unverified serialiser would reintroduce the same class of bug with more code behind it. Note: packages/extension has two pre-existing red tests in api.test.ts that this branch does not touch, and that package is not covered by the root suite. Root suite green (310 files, 4380 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twenty-first remediation batch against docs/findings/. C-02 — resolveMerchant returns apiKeyBusinessId, the business an API key belongs to, and its own doc comment says callers can use it to lock writes to that business and reject a mismatched business_id. Half of them did not. The gap is easy to miss because those routes do authorize: they call verifyBusinessAccess or authorizeBusiness, which check the merchant's access. A scoped key resolves to the owning merchant, and that merchant has access to all of their own businesses, so the check passes for every one of them. A key handed to an integrator for a single business could act on the rest, with every ownership check in the codebase agreeing. Adds keyMayActOnBusiness() alongside resolveMerchant and applies it where a scan found a business id accepted without it: wallets/links (list and create), wallets/links/[id] (update and delete, answering 404 rather than 403 so it is not an existence oracle across businesses), wallets/links/[id]/import, payment-methods/config for both handlers, and payment-methods/manual. Listing with a scoped key and no explicit filter now narrows to that key's own business instead of returning everything the merchant owns. merchant/api-key surfaced in the same scan and is a false positive: it authenticates by session only and already filters the requested business from the merchant's own list. Full suite green (311 files, 4384 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…r findings Twenty-second remediation batch against docs/findings/. Four findings were filed as conditional on a production environment variable. Reading those values would answer "is it exploitable today"; changing the code answers "can it ever be", which does not go stale the next time someone renames a variable. NEW-16 — token !== INTERNAL_API_KEY compared two values that are both undefined when the variable is unset, and undefined !== undefined is false. A request with no Authorization header at all passed the check and could start, stop or trigger the payment monitor. isInternalApiKey(), which fails closed on a blank secret and compares in constant time, already existed; this route did not use it. CP-019 — the reputation signing key was process.env.REPUTATION_SIGNING_SECRET || 'cpr-dev-secret', evaluated at module load. That literal is in a public repository, so an unset variable would have signed every credential with a key the whole internet knows, and nothing would look wrong because the signatures verify perfectly. The fallback is gone and the secret resolves lazily so a missing one fails loudly at signing time. verifySignature also compared HMACs with ===, which short-circuits at the first differing byte and leaks how much of a forged signature was correct; it now uses the constant-time helper. Twenty tests depended on the fallback, which is itself the finding — the suite now supplies a value in vitest.setup.ts. G-R-09 — the JWKS endpoint published sha256(signing secret) truncated to 64 bits, unauthenticated. That is an offline oracle: guess a JWT_SECRET, hash it, compare, with no requests and no logs, and a hit means minting tokens. A key id only needs to be stable and unique, so it is now configured via OIDC_KEY_ID. G-1.2-08 and NEW-05 — the RP ID and origin were resolved independently, so setting one variable and not the other left them decoupled: expectedRPID pinned by config while expectedOrigin came from the request. WebAuthn's security rests on those agreeing. Both also fell back to the Host header, which a client chooses. They are now derived together from one source, a configured pair that does not belong together throws, and the header fallback only accepts recognised hosts. Full suite green (312 files, 4391 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twenty-third remediation batch against docs/findings/. This finishes the audit's section 2.7 class. CP-015 — payments/create-for-merchant authenticated an issuer key and then took merchant_id from the body, so any valid issuer could create payments in any merchant's name. Combined with CP-002, where issuer registration was open and auto-activated, that made "anyone with an email address" the real trust boundary on creating charges for other people. Now goes through platformMayManageMerchant, and its key lookup matches the hash first like the other issuer routes. H-R-10 and NEW-F1A-P-03 — invoice and proposal creation wrote client_id straight through. clients rows carry a business_id, so an unvalidated id attaches another business's customer record and the document then renders that client's details. Both now verify the client belongs to the business being written to. G-1.2-12 — naming a payout address is moving funds, and that is owner-only. Invoice creation gated merchant_wallet_address at invoice.write, which a writer holds, and payments/create gated its payee_override on nothing beyond recording who did it. Recording an action makes it answerable afterwards; it does not restrict who may take it. Both now require funds.move for session callers. Using the business's configured payee is unchanged — only overriding it is restricted. REC-C-03 — both x402 routes resolved the key's scopes and ignored them, so a read-only wallet:read key could verify and settle payments, which on the Stripe rail means capturing real PaymentIntents. Both now require payments:create, the closest existing scope, deliberately reused rather than adding an x402-specific scope that would invalidate every key already issued. REC-D-01 is closed by the NEW-04 fix plus platformMayManageMerchant: a platform can no longer reach a merchant it did not provision. Full suite green (312 files, 4393 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twenty-fourth remediation batch against docs/findings/.
CP-001 — every Stripe session is built as
{ ...callerMetadata, business_id, merchant_id, ... }. The spread order protects
the fields listed explicitly and only those. coinpay_payment_id is not one of
them, and the Stripe webhook uses exactly that key to decide which payment row
to mark confirmed. A caller could therefore attach coinpay_payment_id pointing
at another merchant's pending payment, complete their own one-cent checkout, and
have the webhook confirm the victim's payment as paid.
Two independent fixes, because either alone leaves a way back in.
sanitizeStripeMetadata strips platform-reserved keys from caller input at all
five session-creation sites, reserved by prefix (coinpay_*) as well as by name
so a new internal field cannot be forgotten. Stripping beats ordering the spread
correctly because it does not depend on every future call site remembering to
list every reserved field. Dropped keys are logged, since that is either an
integration bug or an attempt.
The webhook now also cross-checks ownership: the payment named in the session
must belong to the business the session says. Both values live in the same
metadata object but are written at different times by different code, and only
one of them was ever caller-controlled.
Full suite green (313 files, 4401 tests).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…forwarded Twenty-fifth remediation batch against docs/findings/. R3-DIN-01 — when funds arrive at an invoice address that no payment record owns, there is no forwarding path and the money is stranded at the intermediary address. The code recognised this and logged "funds need manual recovery", then fell straight through to marking the invoice paid and emailing the merchant "Payment Received". The merchant was therefore told they had been paid while nothing could move the funds to them, and the invoice was cleared off every outstanding list that would have surfaced the problem. A log line in a constantly-running monitor was the only trace. The invoice now stays unpaid, which is the accurate state — the customer paid, the merchant has not been — and the observation is written to invoices.metadata.unforwardable_balance with balance, currency, address, reason and timestamp, so an operator can find it without reading cron logs. Full suite green (313 files, 4401 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twenty-sixth remediation batch against docs/findings/. REC-C-02 — the x402 v1 EVM path verifies an EIP-712 signature and returned pendingConfirmation unset, i.e. false, which tells a merchant's middleware the payment is final and it may serve the resource. Nothing moves tokens on that path: the documented gasless transferFrom collection does not exist there. The signature proves the payer authorised those terms and cannot alter the amount, which is why amountAuthenticated is legitimately true for EVM, and proves nothing about funds having moved. It now reports pendingConfirmation: true, which the route's own documentation already defines as "callers must not serve paid content on this unless they have accepted the risk". The v2/EIP-3009 path does broadcast and sets the flag false deliberately. REP-F14-01 — reputation_receipts.amount is written by the party being scored, and economicScale multiplies weight by log(1 + amount), so declaring large values was the entire work needed to reach the top tier. The anti-gaming penalty is capped at -3 out of 100 and cannot offset it, and the score is consumed by web-bot-auth/verify for real trust decisions. Economic scaling now applies only to a verifiable amount, using a discriminator already on the row: escrow_tx is set when the transaction settled through escrow, so the amount corresponds to funds that actually moved. A self-declared amount with no settlement still counts, since the job may well have happened, but at unit weight so it cannot be inflated by choosing a bigger number. Full suite green (313 files, 4401 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ribed Twenty-seventh remediation batch against docs/findings/. AUD-01 — no audit-logging infrastructure existed anywhere, while docs/SECURITY_KEYS.md carried "[x] Audit logging for key operations" and docs/SECURITY.md described a four-point audit trail in the present tense. Those documents were corrected earlier in this branch; this is the thing they described. Append-only by grant rather than by convention: service_role holds INSERT and SELECT and is granted neither UPDATE nor DELETE, so an event cannot be rewritten by the credential that wrote it. That did not work on the first attempt, which is why there are two migrations. Supabase's default privileges grant ALL on a new public-schema table directly to service_role, and revoking from PUBLIC/anon/authenticated does not remove a grant held by a named role — so "grant insert, select to service_role" added nothing that was not already there and UPDATE/DELETE/TRUNCATE survived. Caught by reading information_schema.role_table_grants after applying rather than trusting the migration to have meant what it said, then verified with a probe running as service_role: insert=t update=f delete=f, rolled back. Other decisions: no foreign keys to the subjects, because an audit record must survive deletion of what it describes; logging never fails the operation it records, since an audit write that can fail a payment turns observability into an availability risk; failures are logged loudly, because webhook_logs sat empty for its entire existence precisely because nothing complained; and detail is redacted on write, recursively and by field-name fragment, so key material cannot be persisted by accident. Wired into the four paths the docs specifically claimed: subscription activation, payout wallet changes, key release, and DID rebinding by a platform. General payment state transitions, API access and failed authentication are not yet covered, and the docs now say so rather than claiming otherwise. Full suite green (314 files, 4409 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…r caches Secrets for this project are managed through the logicsrc team vault (profullstack -> coinpayportal--prod), not Doppler and not /secrets in moshcode. Auditing that vault against the changes on this branch turned one conditional finding into a confirmed one, and cleared the riskiest. CP-019 was LIVE in production: REPUTATION_SIGNING_SECRET was not set, so every reputation credential was signed with 'cpr-dev-secret' — a constant in this public repository — and the signatures verified perfectly, so nothing looked wrong. Now set to a fresh 32-byte random value in the vault, which the fix on this branch requires since the fallback is gone. ENCRYPTION_KEY and LN_KEY_ENCRYPTION_KEY are both 64-hex and not weak, so the F9-01 guard is safe to deploy and custody operations keep working. That was the change on this branch with the most potential to break production. JWT_SECRET is 88 characters with 45 distinct, so G-R-09's offline oracle was not practically exploitable. The fix stands regardless: it should not depend on the secret happening to be long. WEBAUTHN_RP_ID and WEBAUTHN_ORIGIN are deliberately left unset. The rewritten fallback resolves both from one source and accepts only recognised hosts, which serves the apex and www correctly; pinning the origin to one would break the other. Deploy note: the JWKS kid changes from the old sha256(secret)-derived value to coinpay-oidc-hs256. With HS256 the kid is advisory, since a relying party needs the shared secret to verify, but it is client-visible. doppler.env and doppler.json are untracked and added to .gitignore — no existing pattern matched their filenames. They are a PBKDF2-encrypted fallback cache and its config, obsolete now that secrets live in the vault. They remain in git history, which no .gitignore undoes; purging that and rotating what they held is still a decision. Full suite green (314 files, 4409 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts: # TODO-vulns.md # docs/SECURITY.md # docs/SECURITY_KEYS.md # src/app/api/stripe/subscriptions/route.ts # src/app/api/x402/verify/route.test.ts
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ThreatCrush Security Scan320 finding(s) HIGH/CRITICAL: 32 | MEDIUM: 37 | LOW: 251
…and 270 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
ThreatCrush flagged public/install.sh with a high-severity CWE-494, "network output piped into a shell". The line it matched is a warn() message string — printed advice telling the operator how to pin a release — not an executed command. A scanner reading the source cannot tell those apart. It was also, precisely, the line added to mitigate W-01: the advice to pin COINPAY_REF instead of tracking the mutable master branch. Reworded to "set COINPAY_REF=v0.6.13 before running the installer", which keeps the advice and drops the pattern. Suppressing the alert would have been the wrong fix: the rule is correct in general, and this file genuinely does install code that then runs as the operator. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ThreatCrush flagged three high-entropy hex literals in test files. They are fixtures, and the tool said as much — "usually a fixture, still worth confirming it is not a live credential" — but that confirmation is exactly the cost: a key-shaped literal in the repo is flagged on every diff and every reader has to stop and check whether it is live. These keys were the sequential-hex constant 0123456789abcdef..., which is one of the values requireEncryptionKey exists to reject, so the suite was exercising a key production refuses. Replacing it with a hardcoded strong key fixed that and introduced the literal. Generating a fresh 32-byte key per run removes both problems, and proves nothing in these tests depends on one particular value. randomBytes cannot collide with a KNOWN_WEAK_KEYS entry in any realistic universe, so it is a valid "strong key" fixture by construction. Full suite green (314 files, 4409 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ralyodio
added a commit
that referenced
this pull request
Aug 19, 2026
…ity, and silent failure (#279) Continues #277 and #278. Takes the 2026-08-19 audit from 64 to ~195 of 338 findings resolved. Priority 2 complete; Priority 3 complete bar two items needing product decisions; Priority 4 code items done with the remainder verified against live production; Priority 5 closed by an evidence-based verification sweep. Suite: 327 files / 4600 tests green, tsc --noEmit clean. Four migrations applied to production and verified. ThreatCrush reports 2 high alerts, both confirmed false positives in test fixtures: a unicode test password (backup.test.ts) and a 0x3333...3333 placeholder token address (verify-prepared.test.ts). Neither is a live credential. Per-finding detail in TODO-vulns.md.
ralyodio
added a commit
that referenced
this pull request
Aug 19, 2026
…) (#278) * fix(security): close the three Critical audit findings and the x402 rail From the 2026-08-19 security audit (docs/findings/). Adds TODO-vulns.md as the remediation ledger for all 338 findings, and fixes the three Criticals plus the five High findings that share their code paths. F-1.3-13 — subscription activation read merchant_id and plan_id out of a caller-supplied metadata blob and never compared the amount paid against the plan price, so a $0.01 payment activated the $490/yr plan on any merchant UUID the payer named. Activation now credits the payment row's own merchant_id, validates the plan against SUBSCRIPTION_PRICES, and requires the USD amount to cover it. NEW-04 — payee resolution matched merchants by email with no scoping and then upserted merchant_wallets, overwriting a victim's payout address. The email fallback now refuses non-platform accounts, and persistPayout refuses to write a payout destination for any account the platform did not provision. F-1.3-01 / CP-005 — the Lightning x402 proof was checked against itself: sha256(preimage) == paymentHash, both from the payer, and settle then answered settled:true having verified nothing. Both routes now require a matching incoming, settled ln_payments row for the calling business that covers the price. REC-C-01 — scheme and network are independent attacker-set fields and dispatch fired on either, so a bolt12/ethereum proof ran the Lightning verifier while the response claimed amountAuthenticated. Dispatch is on network alone, with the scheme table shared between verify and settle so they cannot drift. F-1.3-02, F-1.3-03, R3-X1 — price binding now requires expected.payTo and pins expected.asset, and the Stripe verifier compares the amount Stripe reports rather than the payer's self-declared one. Integrator-visible: expected.payTo is now required on v1 verify, matching what v2 already required. 26 new regression tests; full suite green (303 files, 4309 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(security): data-loss, exposure and revenue findings from the audit Second remediation batch against docs/findings/. F5-L4-01 — cleanOrphanedWallets() deleted every wallets row with no transactions on every --execute run. That table is the self-custodial web-wallet store with no merchant_id, so nothing in it can be attributed to a spam signup, and "zero transactions" describes any wallet not yet funded. Now behind --prune-empty-wallets with a 90-day floor, and the header's false safety claim is corrected. W-07 — GET /api/lightning/offers had no auth, optional business_id and an uncapped limit, so one anonymous request dumped every merchant's Lightning offers and revenue. Now authenticated, business-scoped and capped at 100. CP-002 — issuer self-registration set active:true with no identity or domain check and stored the API key in cleartext. Registers inactive now, and persists only the key hash. G-R-07 — /api/oauth/userinfo asserted email_verified:true unconditionally. merchants has no email_verified column in production, which also meant /api/oauth/token's select referencing it errored on every call, so the OAuth ID token has been carrying neither email nor name for any user. All three sites now select real columns only and report false while no verification flow exists. E-03 — the recurring card rail carried a comment claiming application_fee was applied and never set the field, so the platform collected nothing on every subscription. Sets subscription_data.application_fee_percent from the business tier, and spreads caller metadata first so platform keys always win. F-1.1-08 — two invoice paths compared balance >= expected * 0.99 instead of calling the shared isSufficientPayment. Both now use the helper. Two existing tests asserted the vulnerable behaviour and have been inverted: "marks invoice as paid with 1% tolerance", and two oauth tests named "should return email_verified as false" that asserted true. Full suite green (303 files, 4315 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(security): correct documentation that claims controls the code lacks Third remediation batch against docs/findings/ — the report's section 2.5 class, where public documentation states a security property as implemented in direct contradiction with the code. That is the place a partner or auditor looks to decide whether to trust the platform, so a false claim there is worse than the missing control it hides. DOC-01 — layout.tsx titled the whole product "Non-Custodial Crypto Payment Gateway" in the page title, OpenGraph and Twitter card. The browser wallet is non-custodial; the gateway is not, since the platform holds encrypted keys for payment addresses, escrow and collection between receipt and forwarding. F7-01 — docs/SECURITY_KEYS.md checked off audit logging and memory scrubbing, and docs/SECURITY.md described an audit trail in the present tense. Verified: no audit table, no append-only log, no key-access record anywhere in src/, and clearSensitiveString has no callers. Both documents now say so. Building the audit trail (AUD-01) remains open — this corrects the claim, not the gap. GAP-02 — strix_runs/ and three strix_*.log files were tracked in the public repo. .gitignore listed them but was added after they were committed. Untracked; purging them from history remains a decision. F5-L4-03 — scripts/test-spam-detection.ts carried real merchant names, personal Gmail addresses and live corporate domains as fixtures. All now synthetic; each case exercises a shape, not a person. Also fixes a stale expectation in that script that had been failing since the dotted_gmail weight was deliberately reduced from 35 to 20 (confirmed against the original fixture set, so unrelated to the PII swap). 21/21 now. Full suite green (303 files, 4315 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(security): close the cross-tenant IDOR class with shared scope helpers Fourth remediation batch against docs/findings/. Opens the audit's section 2.7 workstream: eighteen findings that all share one shape — authenticate the caller, then trust a tenant id the caller supplied. Patching each route individually leaves the shape in place for the nineteenth, so this adds two shared helpers and moves routes onto them: - src/lib/auth/tenant-scope.ts — resolveBusinessScope() always authorizes a requested business id rather than accepting it, and infers one only when the caller has exactly one business. The code it replaces was worse than a guess: `businessId || authResult` passed a merchant id where a business id was expected, so the lookup matched nothing or matched an unrelated row. - src/lib/escrow/access.ts — callerOwnsEscrow(), shared by escrow/[id] and escrow/[id]/events, which are clones of each other. Closed: B-01, C-01, NEW-13 — the Stripe api-keys and webhooks routes let a query-string or body business_id override the authenticated user. POST /stripe/webhooks creates an endpoint on the named business's Connect account and returns its signing secret, so this handed over a live feed of another merchant's Stripe events pointed at a URL of the attacker's choosing. CP-010, NEW-14, G-1.2-13 — usage/rates (GET/POST/DELETE) and usage/history had no ownership check, while their siblings credits and deduct in the same directory did. CP-014 — reputation/receipts was an unauthenticated select('*'). The endpoint stays public, since a trust graph is only useful if a counterparty can check it, but it now returns a narrow projection: no amount, escrow_tx, buyer_did, platform_did or signatures, capped at 200 rows. CP-021, NEW-15 — escrow/[id] and its events sibling authenticated the caller and then fetched by UUID with no ownership check, so any merchant's API key read any escrow. Both answer 404 rather than 403 so they are not existence oracles. Full suite green (304 files, 4327 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(security): stop credentials acting beyond their remit Fifth remediation batch against docs/findings/. Two distinct mistakes, both about a credential being trusted beyond what it was issued for. A capability check that defaulted to read. C-03 / G-1.2-01: payment-methods/manual called verifyBusinessAccess with no capability argument, and the default is business.read, which every team member has including readonly. The write was therefore gated at read level, so a read-only member could rewrite the Venmo, Cash App or Zelle handle customers are told to pay. That is a funds destination and the project's invariant is that moving funds is owner-only, so the write now requires funds.move. The sibling import route wrote the same field at settings.manage; raised to match. API keys acting with account authority. L7A-01 and SUB-01: did/claim, did/delegate and subscriptions/status DELETE checked no scopes, and API_SCOPES has no DID or billing scope to check even if they had. A business key resolves to the owning merchant, so any key — including a read-only wallet:read one — could rebind the merchant's DID, mint DelegatedAuthority credentials carrying wallet:transfer and escrow:settle, or cancel the Professional subscription. A scoped key must not grant authority stronger than itself; all three now require session auth. The existing isMerchantAuth ternaries had identical branches, so the type guard was present and doing nothing. Adds src/lib/p2p/platform-ownership.ts, shared by CP-003 (did/override) and CP-023 (reputation/merchant-wallet). Both acted on any merchant_id in the body on nothing but a valid issuer key, rebinding a merchant's identity or writing their payout wallet so a charge in an unconfigured currency settled to the caller. Both now require the merchant to be platform-provisioned and provisioned by that specific platform — the same guard NEW-04 needed, which is why it is shared rather than copied a third time. Full suite green (304 files, 4327 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(security): reconcile code with the live schema, and stop expiring paid payments Sixth remediation batch against docs/findings/. Each finding here was checked against the live database rather than the migrations directory, which is known to drift from production. L8-02 — payments.payment_address_id does not exist, yet three card routes inserted it and src/lib/supabase/types.ts still declared it. PostgREST rejects an insert naming an unknown column, so every card payment failed with a 500 before reaching Stripe. Removed from the routes and from the type; leaving it on the type is what kept the routes writing it. L4-NEW-02 — the address-generation failure path wrote status 'failed', which payments_status_check does not permit, and never checked the result. The row was left pending forever, on no dashboard's problem list. Now writes 'expired', the terminal state the schema actually has, records failure_reason in metadata, and logs if that write fails too. NEW-L5-2 — blockchainSchema accepts USDT, USDC and USDC_BASE and the code generates addresses for all three, but payments_blockchain_check permitted none of them, so payment creation failed outright for those currencies. Widened by migration 20260819120000, applied to production and verified. Prod holds zero rows for those three values while every other supported chain has some. BL-01 — every checker in monitor-balance.ts answered { balance: 0 } on an RPC error, exactly as it does for an address that has genuinely received nothing, and processPayment expired the payment on that. A transient provider outage during the expiry window therefore marked fully-paid payments expired, with the customer's funds sitting at the address and nothing retrying. BalanceResult now carries an error field, all 27 failure paths set it, and expiry refuses to fire on an unreadable balance. The two zeros that are real — an unactivated XRP account and an unused ADA address — are deliberately left untagged. Also confirms two findings are already closed: H-R-01 (checkDOGEBalance is fully implemented, not the stub described) and CP-P5 (isBusinessPaidTier resolves tier through the merchant subscription and never reads businesses.tier). Full suite green (304 files, 4329 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(security): screen every card sink, and fail safe instead of open Seventh remediation batch against docs/findings/. N-01 — screenCheckout was called from exactly one of the seven code paths that create a real card charge. The other six now screen before the Stripe session exists, which is the last point a payment can be stopped without Stripe ever seeing it, and each forces 3-D Secure on a verify decision: payments/widget/create (a public, secret-free embed and the most exposed of the seven), payments/create's card branch, payments/create-for-merchant, p2p/request's Stripe branch, and lib/payments/invoice-stripe. REC-D-05 — the p2p Stripe branch is one of those six. Its caller is a platform issuer key rather than an authenticated merchant, which made it the least supervised card path in the codebase. FR-01 — screening failed open. Any error anywhere returned decision 'allow', and checkBlocklist returned the same null for a database error as for "no entry matches", so an unreachable blocklist waved through every entry on it — entries usually added after a real chargeback. The reasoning behind failing open was sound, since a broken fraud check must not become a payment outage, but 'allow' was the wrong conclusion from it. Both paths now degrade to 'verify', which keeps payments flowing while forcing 3-D Secure and moving liability for a stolen card back to the issuer. CP-P5 — corrects my own earlier assessment. I marked this already-fixed after checking isBusinessPaidTier, which does resolve tier through the merchant subscription. But payments/create has a separate inline fee calculation that selected businesses.tier, the column that does not exist, so PostgREST rejected the query, the row came back null, and every business was charged the 1% free-tier rate on that rail regardless of plan. The test covering it expressed the pro tier as tier: 'pro' on the businesses row, so it passed against a column that was never read. Full suite green (305 files, 4332 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(security): escape merchant-supplied text in outbound email Eighth remediation batch against docs/findings/. proposal-templates.ts had an escapeHtml and used it on every interpolated field. Three siblings did not have it and interpolated merchant-supplied strings raw, so the fix is a shared src/lib/email/escape.ts rather than a fourth copy. NEW-24 — invoice-templates.ts interpolated business name, invoice number, notes, client name and tx hash into the HTML body with no escaping. Subject lines are deliberately left unescaped: they are plain text, and escaping them would show the recipient a literal &. G-1.2-09 — the team invitation email interpolated scopeLabel, which is the business or organization name, into a message delivered to an arbitrary address the same caller supplies. Creating a merchant account is free and unverified, so that was a way to send attacker-authored HTML from the platform's own sending domain, to anyone, with no rate limit. F5-L4-02 — scripts/daily-stats-email.ts put signup name and email into the internal daily report unescaped, and its recipients are the operations team. The helper is duplicated there deliberately: the script runs standalone under tsx and does not resolve the app's path aliases. escapeUrl is separate from escapeHtml because escaping characters does not help with a javascript: or data: href — those survive HTML escaping intact and still execute when clicked, so anything not clearly http(s) or mailto is dropped. Full suite green (306 files, 4342 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(security): make the webhook audit trail actually recordable Ninth remediation batch against docs/findings/. select count(*) from webhook_logs against production returned 0. Not a few gaps — the table has never held a row. The webhook delivery audit trail has never recorded a single attempt, and nothing surfaced that because delivery itself is unaffected by the logging failing. Three separate faults, all of which had to be fixed before one insert could succeed: V-01 — url and payload are NOT NULL with no default, and the application writes webhook_url instead, so every insert failed on a not-null violation. The two column sets are duplicates from different eras of the schema; the code now writes both so either name reads correctly. L8-01 — payment_id had its NOT NULL dropped so escrow events could be logged, but the foreign key to payments(id) was never dropped alongside it. REC-D-02 — the escrow path passed an escrow id as payment_id, with a comment saying it was reusing the column, which violated that foreign key. Migration 20260819130000 relaxes the legacy pair and gives escrow its own escrow_id column with its own FK rather than borrowing one, so the payments FK stays meaningful. A CHECK enforces one subject per row so the column-borrowing cannot quietly return. Applied to production and verified with a probe insert, which succeeded and was then deleted. logWebhookAttempt now logs when it cannot record, instead of returning an error every caller discarded. That silence is why an empty audit table went unnoticed. Full suite green (306 files, 4342 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(security): route every encryption call site through the key guard Tenth remediation batch against docs/findings/. This is the audit's section 2.1 pattern — a strong guard that most of its own consumers bypass. F9-01 — requireEncryptionKey was correct and protected 4 of 13 real encryption call sites. The other nine, including the entire custody hot path, hand-rolled const k = process.env.ENCRYPTION_KEY; if (!k) return { success: false, error: 'Encryption key not configured' }; which establishes that a key is present and nothing about whether it is usable. An all-zero or deadbeef-repeated key passed all nine. Migrated: hd-wallet, system-wallet (two sites), secure-forwarding, escrow/service, business-collection, business/service, webhooks/secret, and both Stripe webhook routes. Two things kept the guard from spreading, and both are fixed. It throws while those call sites return result objects, so tryRequireEncryptionKey now gives them a form that fits. And the repository's own fixtures used values the guard rejects — the one in secure-forwarding.test.ts was literally a KNOWN_WEAK_KEYS entry and escrow/service.test.ts used a 36-character non-hex string — so adopting the guard anywhere would have turned the suite red. Both replaced with a real 32-byte hex key. keyHashPepper() in scoped-keys.ts is deliberately left reading the raw value and documented as such: it is a pepper for an HMAC, not an encryption key, and its value is baked into every api_key_hash already stored, so refusing a weak value at read time would lock out every integrator at once. The right fix there is a rotation with re-hashing. Full suite green (307 files, 4353 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(security): make COINPAY_REF pinning actually hold across upgrades Eleventh remediation batch against docs/findings/. F6-01 — COINPAY_REF is the only user-facing mitigation against an auto-upgrade that pulls a mutable branch every five minutes, and it silently did nothing. Two re-invocation paths dropped it: the auto-upgrade poll ran a bare `curl … | sh -s -- update`, and `coinpay update` in the wrapper did the same. Both defaulted COINPAY_REF back to master, so a host pinned to a tag tracked master anyway with no indication. Both now bake in the ref they were installed from, verified by running write_self_upgrade_helper in isolation and reading the generated script. COINPAY_SHA256 is deliberately not carried across: a checksum pins one specific archive, so reusing it for a later version guarantees a mismatch and would break every upgrade. W-01 — partially addressed. An unpinned install now warns on screen that it is following a mutable branch and prints the command to pin a release. The default ref itself is unchanged and needs a release-process decision: the installer compares against packages/sdk/package.json at the ref, so defaulting to a tag whose SDK version trails master would leave the upgrader idle or looping. Also fixes a set -e hazard introduced and caught while writing this: a `[ test ] && VAR=0` line would have aborted the installer for exactly the users who did pin a ref. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(security): rate-limit the endpoints that were free to abuse Twelfth remediation batch against docs/findings/. The rate-limit infrastructure already existed and was well tuned; these endpoints simply never called it. NEW-06 — none of the four WebAuthn routes had any limit. login-options answers differently for a registered and an unregistered email, so unlimited it is a free user-enumeration oracle over the whole merchant base. WW-02 — the web-wallet derive route, whose five sibling mutating routes are all limited. Each call performs HD derivation and writes a row. REC-C-04 — x402 verify and settle. Ledger bloat, and on the Stripe rail each call spends our own Stripe API quota. REC-C-05 — GET /api/swap/quote is anonymous and each quote costs two calls to the ChangeNOW third-party API, so it was a lever for exhausting our own quota with no credential at all. L7A-03 — reputation/attest needed more than a limit. It was completely unauthenticated and took attester_did from the body. submitAttestation does check the attester is a party to the receipt, so this was never unbounded forgery, but knowing a receipt id and the two DIDs on it was enough to attest as either party — and reputation/receipts used to hand out exactly that (CP-014). It now requires authentication and proves the caller controls the DID through the existing merchant_dids link, rather than introducing a new signature scheme. Also fixes something not in the register: checkRateLimit and checkRateLimitAsync returned allowed: true in silence for an unknown category, so a typo in a category name disabled the limit entirely with nothing to notice. It still allows, since a config mistake must not take payments down, but logs. Full suite green (307 files, 4353 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(security): close the escrow oracle, index collision and stuck status Thirteenth remediation batch against docs/findings/. NEW-01 — createEscrow resolves a wallet address to the owning merchant's email, which is convenient because it lets an escrow notify a counterparty identified only by address, and then returned that email to the caller. Anyone able to create escrows could therefore map any on-chain address to the merchant who owns it, across the whole merchant base — which is precisely the input NEW-04 needed. Auto-resolved emails are now redacted from the response; the row keeps them so notification still works. L5-01 — generateEscrowAddress read next_index and wrote next_index + 1 with no condition on what it had read, and keyed on cryptocurrency while the payment flow keys on the derivation family (ETH/POL/BNB/USDT/USDC share one). The two counters advanced independently over the same key space, so the collision was by construction rather than merely under contention. Now uses acquireFamilyIndex, the compare-and-swap helper the payment flow already used. R3-DIN-03 — escrow-monitor writes status 'settle_failed', which escrows_status_check did not permit, so the UPDATE was rejected and the escrow was left 'released' with nothing signalling the failure. Verified both ways with a probe UPDATE that rolls itself back: rejected before the migration, accepted after. The write now checks its result. Worth recording: the register attributes this to the constraint being NOT VALID. That is wrong. NOT VALID only skips checking rows that already existed when the constraint was added; it does not exempt new INSERTs or UPDATEs. The pre-existing settle_failed rows are what made it look unenforced. ESC-NEW-01 is reclassified as a decision rather than a fix: both dispute columns exist with no writer as reported, but production holds zero escrows in disputed status, so no funds are frozen today. An arbiter flow needs product decisions about who arbitrates and on what evidence. Full suite green (307 files, 4353 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(integrations): repair two paths that failed 100% of the time Fourteenth remediation batch against docs/findings/. Both findings share a failure mode worth naming: the test asserted the same wrong thing the code did, so a path that could never work in production had full, passing coverage. NEW-L5-01 — CoinPayClient.request takes (endpoint, options). Every call in the SDK's card-payments.js passed three arguments in the shape request(method, path, body), so endpoint received 'POST', the URL became baseUrl + 'POST', and options received the path as a string. All five calls failed against a real client, making release and refund of a card escrow unreachable through the SDK. The tests mocked request and asserted the three-argument shape, so a mock that accepts anything certified a call that could never work. F4-01 — StatusMapper::MAP had mark_paid entries only for payment.completed and payment.overpaid. The backend emits neither; it emits payment.confirmed, payment.forwarded, payment.failed and payment.expired. Every real webhook fell through the ?? 'ignore' default, so automated invoice crediting never fired for any transaction — and ignore is also the correct answer for events we genuinely skip, so nothing looked wrong. All twelve existing PHP tests covered event types the backend never sends. Maps the real events, keeps the legacy keys since a replayed historical delivery may carry them, and logs an unmapped event rather than letting it disappear. Adds a PHP test that walks the full emitted-event list so a new backend event type fails the suite instead of being silently ignored. Full suite green (307 files, 4353 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(security): close wallet spend-limit fail-opens and unconsented disclosure Fifteenth remediation batch against docs/findings/. L-01 — the web-wallet spend controls failed open twice. A whitelist and a daily spend limit are the only two things between a compromised session and the balance, and both were skipped when a query failed: one path returned allowed true with the comment "fail open for now", the other wrapped the limit check in if (!error && todayTxs) so an error simply skipped it. getSettings creates a default row when none exists, so reaching the first path means a real database failure rather than "no settings configured". Both now deny. A test named "should allow if settings fail to load (fail open)" asserted the vulnerability directly and is inverted. NEW-WW34-01 — checkTransactionAllowed took a chain parameter and never used it, so today's total summed raw amounts across every chain: 1 BTC and 1 DOGE counted as 2 against one limit. That blocks trivially cheap sends after one expensive one and lets a small-unit chain run far past the intended cap. The daily total is now scoped to the chain being spent on. WW-L4-01 — /api/wallets/lookup already withholds the email, but answered found true/false unauthenticated and unlimited, which is enough to sweep the merchant base address by address. It stays public, since a sender legitimately checks before paying, with a rate limit that makes bulk enumeration impractical. NEW-19 — /api/partners published the name, description and webhook host of every active business with a webhook configured. Configuring a webhook is a technical step, not consent to appear in a public directory. Migration 20260819150000 adds public_directory_opt_in defaulting to false. Note the visible side effect: the partners page is now empty. 28 businesses were being published and none opted in, because there was nothing to opt into. If consent exists out of band it needs an explicit backfill; I did not assume it on anyone's behalf. Full suite green (307 files, 4355 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(security): bind signed transactions to what was prepared, on every chain Sixteenth remediation batch against docs/findings/. WW-01 — verifySignedTxBinding decoded EVM transactions and compared the recipient only. A signed transaction paying the right address a different amount was accepted and recorded as the prepared one, and everything downstream hangs off that row: the wallet's history, the daily spend limit, fee accounting and notifications all described a transaction that did not happen. Now compares the value for native transfers and the second ABI argument for ERC-20 transfers, using per-chain decimals. The refusal condition was also reason?.startsWith('recipient '), matching on message text, so rewording the reason string would have silently stopped it refusing anything. The decoder now sets an explicit mismatch flag. WW-03 — BTC, BCH, SOL and USDC_SOL fell through to "no decoder for <chain>" and broadcast entirely unchecked. Both libraries needed were already dependencies: BTC decodes with bitcoinjs-lib and sums the outputs paying the prepared address, requiring at least the prepared amount — "at least" rather than "exactly one output", because a real spend nearly always has change back to the sender. SOL and USDC_SOL confirm the prepared recipient appears among the transaction's account keys, which catches a wholesale substitution of the payee. The amount is not checked there: it lives in instruction data whose layout depends on the program, and that is not something to guess at in a broadcast guard, so it is reported as unverified rather than claimed. BCH still has no decoder, because bitcoinjs-lib does not support its SIGHASH_FORKID variant as the codebase notes elsewhere. Left recorded as unverified with the reason stated in the code. Full suite green (307 files, 4361 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(security): make the supported-currency lists agree, and prove it in CI Seventeenth remediation batch against docs/findings/. H-R-06 — baseChainFor mapped bare USDC to 'SOL', so a merchant configuring a plain-USDC payout had to supply a Solana address to pass validation. Every other module treats bare USDC as ERC-20 on Ethereum: monitor-balance checks it on the Ethereum RPC, rates/fees prices it off the Ethereum gas path, and address generation puts it on the Ethereum family. The validator and the money disagreed about which chain the payout was on. Validation now follows the money. H-R-08 — the currency list does diverge across modules, and hand-syncing eight copies fixes today and drifts again next month. CRYPTO_NAMES is already the declared operational source of truth, so currency-consistency.test.ts checks the others against it: every symbol must have a declared address family, the validator must accept that family and reject another so the check actually discriminates, and the static-fee fallback table must not carry chains the gateway no longer supports. Adding a symbol without recording a decision now fails the suite. That test immediately found something not in the register: USDC_BASE strips to 'BASE', which had no case in the validator, so it fell through to the null default meaning "no validator, trust the address". USDC on Base is a supported payment chain with a balance checker and a fee path, making it the one EVM variant whose payout addresses were accepted with no format check at all — a typo'd Base address would have been stored and paid to. Full suite green (308 files, 4365 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(security): make Boltz refunds recoverable and lockup addresses verifiable Eighteenth remediation batch against docs/findings/. A-03 — encryptProviderSecrets is called when a Boltz swap is created and stripProviderSecrets when history is listed, but decryptProviderSecrets had zero callers. The refund and claim keys went in and never came back out, so when a swap failed the HTLC funds could not be recovered through the product at all. The refund path exists on Boltz's side; the key needed to walk it was locked in our own database. Adds GET /api/swap/boltz/:id/recovery: owner-authorized, rate limited on its own budget, and it logs that key material was released without logging the keys. A separate route rather than extra fields on the status endpoint, because handing out key material is a distinct action that should be asked for explicitly and be visible separately in logs. W-06 — redeemScript and swapTree were declared on both Boltz response types and never read, so the lockup address was taken on faith. A substituted address would have been funded happily, and the refund key generated locally is worthless against it because that key belongs to a different script — the deposit would be unrecoverable, which is exactly what a refund key exists to prevent. Both creation paths now derive the address from the redeem script (P2WSH, P2SH-P2WSH and bare P2SH) and refuse to fund a mismatch. Taproot swaps carry a swapTree whose derivation needs the full tweak; those report "cannot check" and log rather than returning a false pass, keeping the distinction between checked and uncheckable explicit. Full suite green (309 files, 4370 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(security): verify the invoice amount when paying a Lightning Address Nineteenth remediation batch against docs/findings/. W-05 — paying a Lightning Address asks the recipient's LNURL server for an invoice and then paid whatever came back, undecoded. The amount actually paid was therefore whatever that server chose to put in the invoice, not the amount the sender entered and was shown, so a hostile or compromised LNURL endpoint could return an invoice for any amount up to the wallet's balance and have it paid silently. The minSendable/maxSendable check that was already present validates the amount we request, which the server is free to ignore. The amount now has to match. bolt11AmountMsat reads it from the invoice's human-readable part, a well-defined grammar in BOLT-11 that needs no bech32 decoding, so this answers one question without adding a full invoice decoder as a dependency. An amountless invoice — a donation invoice, where the payer chooses — and an unparseable one both return null, and the caller treats that as a refusal. "Cannot tell" is not agreement, and an amountless invoice is precisely the shape that would otherwise be paid blind. Full suite green (310 files, 4380 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(security): stop the batch runner paying twice, and stop the SDK signing garbage Twentieth remediation batch against docs/findings/. F3-L3-01 — the extension's batch runner classified "already known" as a transient error and rebuilt the payment from scratch. But "already known" means the node HAS the transaction: the broadcast succeeded. Retrying therefore broadcast a second, real, duplicate payment. It is the most dangerous string in that list, because the response meaning "your money moved" was read as "try again". The error list conflated two different things and is now split. Retried: errors meaning the request never reached the chain — rate limit, timeout, 502/503/504, blockhash not found, block height exceeded. Not retried: errors meaning a transaction like ours already exists — already known, nonce too low, replacement transaction underpriced, missingorspent, txn-mempool-conflict. Every entry in the second group describes chain state that moved because such a transaction exists, which is exactly when a retry duplicates a payment. "already known" is now reported as sent, which is what it means. The existing test asserted that "nonce too low" was retryable. That assertion was the finding, and it is inverted. F3-L5-01 — the SDK's send() called signMessage(unsignedTx, privateKey): a generic message signature over the JSON of the unsigned transaction, posted as signed_tx. That is not a signed transaction on any chain, so the method failed for every integrator who called it, silently, by producing plausible output that only failed later at the node. It now throws with an explanation and points at the primitives that do work. Real serialisation is deliberately not implemented here: the SDK carries only @scure/@noble by design, correct RLP/PSBT/Solana encoding depends on the exact shape of prepareResult.unsigned_tx which the server defines, and none of it can be verified against a live node from this package. Shipping an unverified serialiser would reintroduce the same class of bug with more code behind it. Note: packages/extension has two pre-existing red tests in api.test.ts that this branch does not touch, and that package is not covered by the root suite. Root suite green (310 files, 4380 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(security): bind scoped API keys to the business they were issued for Twenty-first remediation batch against docs/findings/. C-02 — resolveMerchant returns apiKeyBusinessId, the business an API key belongs to, and its own doc comment says callers can use it to lock writes to that business and reject a mismatched business_id. Half of them did not. The gap is easy to miss because those routes do authorize: they call verifyBusinessAccess or authorizeBusiness, which check the merchant's access. A scoped key resolves to the owning merchant, and that merchant has access to all of their own businesses, so the check passes for every one of them. A key handed to an integrator for a single business could act on the rest, with every ownership check in the codebase agreeing. Adds keyMayActOnBusiness() alongside resolveMerchant and applies it where a scan found a business id accepted without it: wallets/links (list and create), wallets/links/[id] (update and delete, answering 404 rather than 403 so it is not an existence oracle across businesses), wallets/links/[id]/import, payment-methods/config for both handlers, and payment-methods/manual. Listing with a scoped key and no explicit filter now narrows to that key's own business instead of returning everything the merchant owns. merchant/api-key surfaced in the same scan and is a false positive: it authenticates by session only and already filters the requested business from the merchant's own list. Full suite green (311 files, 4384 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(security): remove the "depends on a prod env var" caveat from four findings Twenty-second remediation batch against docs/findings/. Four findings were filed as conditional on a production environment variable. Reading those values would answer "is it exploitable today"; changing the code answers "can it ever be", which does not go stale the next time someone renames a variable. NEW-16 — token !== INTERNAL_API_KEY compared two values that are both undefined when the variable is unset, and undefined !== undefined is false. A request with no Authorization header at all passed the check and could start, stop or trigger the payment monitor. isInternalApiKey(), which fails closed on a blank secret and compares in constant time, already existed; this route did not use it. CP-019 — the reputation signing key was process.env.REPUTATION_SIGNING_SECRET || 'cpr-dev-secret', evaluated at module load. That literal is in a public repository, so an unset variable would have signed every credential with a key the whole internet knows, and nothing would look wrong because the signatures verify perfectly. The fallback is gone and the secret resolves lazily so a missing one fails loudly at signing time. verifySignature also compared HMACs with ===, which short-circuits at the first differing byte and leaks how much of a forged signature was correct; it now uses the constant-time helper. Twenty tests depended on the fallback, which is itself the finding — the suite now supplies a value in vitest.setup.ts. G-R-09 — the JWKS endpoint published sha256(signing secret) truncated to 64 bits, unauthenticated. That is an offline oracle: guess a JWT_SECRET, hash it, compare, with no requests and no logs, and a hit means minting tokens. A key id only needs to be stable and unique, so it is now configured via OIDC_KEY_ID. G-1.2-08 and NEW-05 — the RP ID and origin were resolved independently, so setting one variable and not the other left them decoupled: expectedRPID pinned by config while expectedOrigin came from the request. WebAuthn's security rests on those agreeing. Both also fell back to the Host header, which a client chooses. They are now derived together from one source, a configured pair that does not belong together throws, and the header fallback only accepts recognised hosts. Full suite green (312 files, 4391 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(security): close the remaining cross-tenant findings Twenty-third remediation batch against docs/findings/. This finishes the audit's section 2.7 class. CP-015 — payments/create-for-merchant authenticated an issuer key and then took merchant_id from the body, so any valid issuer could create payments in any merchant's name. Combined with CP-002, where issuer registration was open and auto-activated, that made "anyone with an email address" the real trust boundary on creating charges for other people. Now goes through platformMayManageMerchant, and its key lookup matches the hash first like the other issuer routes. H-R-10 and NEW-F1A-P-03 — invoice and proposal creation wrote client_id straight through. clients rows carry a business_id, so an unvalidated id attaches another business's customer record and the document then renders that client's details. Both now verify the client belongs to the business being written to. G-1.2-12 — naming a payout address is moving funds, and that is owner-only. Invoice creation gated merchant_wallet_address at invoice.write, which a writer holds, and payments/create gated its payee_override on nothing beyond recording who did it. Recording an action makes it answerable afterwards; it does not restrict who may take it. Both now require funds.move for session callers. Using the business's configured payee is unchanged — only overriding it is restricted. REC-C-03 — both x402 routes resolved the key's scopes and ignored them, so a read-only wallet:read key could verify and settle payments, which on the Stripe rail means capturing real PaymentIntents. Both now require payments:create, the closest existing scope, deliberately reused rather than adding an x402-specific scope that would invalidate every key already issued. REC-D-01 is closed by the NEW-04 fix plus platformMayManageMerchant: a platform can no longer reach a merchant it did not provision. Full suite green (312 files, 4393 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(security): strip platform-reserved keys from caller Stripe metadata Twenty-fourth remediation batch against docs/findings/. CP-001 — every Stripe session is built as { ...callerMetadata, business_id, merchant_id, ... }. The spread order protects the fields listed explicitly and only those. coinpay_payment_id is not one of them, and the Stripe webhook uses exactly that key to decide which payment row to mark confirmed. A caller could therefore attach coinpay_payment_id pointing at another merchant's pending payment, complete their own one-cent checkout, and have the webhook confirm the victim's payment as paid. Two independent fixes, because either alone leaves a way back in. sanitizeStripeMetadata strips platform-reserved keys from caller input at all five session-creation sites, reserved by prefix (coinpay_*) as well as by name so a new internal field cannot be forgotten. Stripping beats ordering the spread correctly because it does not depend on every future call site remembering to list every reserved field. Dropped keys are logged, since that is either an integration bug or an attempt. The webhook now also cross-checks ownership: the payment named in the session must belong to the business the session says. Both values live in the same metadata object but are written at different times by different code, and only one of them was ever caller-controlled. Full suite green (313 files, 4401 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(security): stop marking an invoice paid when the funds cannot be forwarded Twenty-fifth remediation batch against docs/findings/. R3-DIN-01 — when funds arrive at an invoice address that no payment record owns, there is no forwarding path and the money is stranded at the intermediary address. The code recognised this and logged "funds need manual recovery", then fell straight through to marking the invoice paid and emailing the merchant "Payment Received". The merchant was therefore told they had been paid while nothing could move the funds to them, and the invoice was cleared off every outstanding list that would have surfaced the problem. A log line in a constantly-running monitor was the only trace. The invoice now stays unpaid, which is the accurate state — the customer paid, the merchant has not been — and the observation is written to invoices.metadata.unforwardable_balance with balance, currency, address, reason and timestamp, so an operator can find it without reading cron logs. Full suite green (313 files, 4401 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(security): stop reporting unverified claims as settled facts Twenty-sixth remediation batch against docs/findings/. REC-C-02 — the x402 v1 EVM path verifies an EIP-712 signature and returned pendingConfirmation unset, i.e. false, which tells a merchant's middleware the payment is final and it may serve the resource. Nothing moves tokens on that path: the documented gasless transferFrom collection does not exist there. The signature proves the payer authorised those terms and cannot alter the amount, which is why amountAuthenticated is legitimately true for EVM, and proves nothing about funds having moved. It now reports pendingConfirmation: true, which the route's own documentation already defines as "callers must not serve paid content on this unless they have accepted the risk". The v2/EIP-3009 path does broadcast and sets the flag false deliberately. REP-F14-01 — reputation_receipts.amount is written by the party being scored, and economicScale multiplies weight by log(1 + amount), so declaring large values was the entire work needed to reach the top tier. The anti-gaming penalty is capped at -3 out of 100 and cannot offset it, and the score is consumed by web-bot-auth/verify for real trust decisions. Economic scaling now applies only to a verifiable amount, using a discriminator already on the row: escrow_tx is set when the transaction settled through escrow, so the amount corresponds to funds that actually moved. A self-declared amount with no settlement still counts, since the job may well have happened, but at unit weight so it cannot be inflated by choosing a bigger number. Full suite green (313 files, 4401 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(security): add the append-only audit trail the docs already described Twenty-seventh remediation batch against docs/findings/. AUD-01 — no audit-logging infrastructure existed anywhere, while docs/SECURITY_KEYS.md carried "[x] Audit logging for key operations" and docs/SECURITY.md described a four-point audit trail in the present tense. Those documents were corrected earlier in this branch; this is the thing they described. Append-only by grant rather than by convention: service_role holds INSERT and SELECT and is granted neither UPDATE nor DELETE, so an event cannot be rewritten by the credential that wrote it. That did not work on the first attempt, which is why there are two migrations. Supabase's default privileges grant ALL on a new public-schema table directly to service_role, and revoking from PUBLIC/anon/authenticated does not remove a grant held by a named role — so "grant insert, select to service_role" added nothing that was not already there and UPDATE/DELETE/TRUNCATE survived. Caught by reading information_schema.role_table_grants after applying rather than trusting the migration to have meant what it said, then verified with a probe running as service_role: insert=t update=f delete=f, rolled back. Other decisions: no foreign keys to the subjects, because an audit record must survive deletion of what it describes; logging never fails the operation it records, since an audit write that can fail a payment turns observability into an availability risk; failures are logged loudly, because webhook_logs sat empty for its entire existence precisely because nothing complained; and detail is redacted on write, recursively and by field-name fragment, so key material cannot be persisted by accident. Wired into the four paths the docs specifically claimed: subscription activation, payout wallet changes, key release, and DID rebinding by a platform. General payment state transitions, API access and failed authentication are not yet covered, and the docs now say so rather than claiming otherwise. Full suite green (314 files, 4409 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(security): move secrets onto the logicsrc vault, untrack Doppler caches Secrets for this project are managed through the logicsrc team vault (profullstack -> coinpayportal--prod), not Doppler and not /secrets in moshcode. Auditing that vault against the changes on this branch turned one conditional finding into a confirmed one, and cleared the riskiest. CP-019 was LIVE in production: REPUTATION_SIGNING_SECRET was not set, so every reputation credential was signed with 'cpr-dev-secret' — a constant in this public repository — and the signatures verified perfectly, so nothing looked wrong. Now set to a fresh 32-byte random value in the vault, which the fix on this branch requires since the fallback is gone. ENCRYPTION_KEY and LN_KEY_ENCRYPTION_KEY are both 64-hex and not weak, so the F9-01 guard is safe to deploy and custody operations keep working. That was the change on this branch with the most potential to break production. JWT_SECRET is 88 characters with 45 distinct, so G-R-09's offline oracle was not practically exploitable. The fix stands regardless: it should not depend on the secret happening to be long. WEBAUTHN_RP_ID and WEBAUTHN_ORIGIN are deliberately left unset. The rewritten fallback resolves both from one source and accepts only recognised hosts, which serves the apex and www correctly; pinning the origin to one would break the other. Deploy note: the JWKS kid changes from the old sha256(secret)-derived value to coinpay-oidc-hs256. With HS256 the kid is advisory, since a relying party needs the shared secret to verify, but it is client-visible. doppler.env and doppler.json are untracked and added to .gitignore — no existing pattern matched their filenames. They are a PBKDF2-encrypted fallback cache and its config, obsolete now that secrets live in the vault. They remain in git history, which no .gitignore undoes; purging that and rotating what they held is still a decision. Full suite green (314 files, 4409 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(install): reword pin advice so it is not read as a fetch-pipe-shell ThreatCrush flagged public/install.sh with a high-severity CWE-494, "network output piped into a shell". The line it matched is a warn() message string — printed advice telling the operator how to pin a release — not an executed command. A scanner reading the source cannot tell those apart. It was also, precisely, the line added to mitigate W-01: the advice to pin COINPAY_REF instead of tracking the mutable master branch. Reworded to "set COINPAY_REF=v0.6.13 before running the installer", which keeps the advice and drops the pattern. Suppressing the alert would have been the wrong fix: the rule is correct in general, and this file genuinely does install code that then runs as the operator. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test: generate encryption-key fixtures instead of hardcoding them ThreatCrush flagged three high-entropy hex literals in test files. They are fixtures, and the tool said as much — "usually a fixture, still worth confirming it is not a live credential" — but that confirmation is exactly the cost: a key-shaped literal in the repo is flagged on every diff and every reader has to stop and check whether it is live. These keys were the sequential-hex constant 0123456789abcdef..., which is one of the values requireEncryptionKey exists to reject, so the suite was exercising a key production refuses. Replacing it with a hardcoded strong key fixed that and introduced the literal. Generating a fresh 32-byte key per run removes both problems, and proves nothing in these tests depends on one particular value. randomBytes cannot collide with a KNOWN_WEAK_KEYS entry in any realistic universe, so it is a valid "strong key" fixture by construction. Full suite green (314 files, 4409 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
ralyodio
added a commit
that referenced
this pull request
Aug 19, 2026
…ity, and silent failure (#279) Continues #277 and #278. Takes the 2026-08-19 audit from 64 to ~195 of 338 findings resolved. Priority 2 complete; Priority 3 complete bar two items needing product decisions; Priority 4 code items done with the remainder verified against live production; Priority 5 closed by an evidence-based verification sweep. Suite: 327 files / 4600 tests green, tsc --noEmit clean. Four migrations applied to production and verified. ThreatCrush reports 2 high alerts, both confirmed false positives in test fixtures: a unicode test password (backup.test.ts) and a 0x3333...3333 placeholder token address (verify-prepared.test.ts). Neither is a live credential. Per-finding detail in TODO-vulns.md.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#277 was squash-merged after my first three batches, so master currently has 15 findings fixed. This is everything since — 49 more findings, plus the secrets work.
Master's squash commit (
f4947b6) is byte-identical to this branch's batch-3 state, soorigin/masterhas been merged in and the five conflicts resolved to the branch, which is a strict superset. Verified:git diff origin/master 13444a2over the conflicted files is empty.What's in here that isn't on master
Structural fixes. The audit's argument is that ten patterns recur, so patching instances leaves the shape. Six shared helpers now carry the invariants:
auth/tenant-scope.tsescrow/access.tsCP-021,NEW-15p2p/platform-ownership.tsCP-003,CP-023,CP-015auth/merchant.ts::keyMayActOnBusinessC-02email/escape.tsNEW-24,G-1.2-09,F5-L4-02stripe/metadata.tsCP-001Plus
x402/networks.tsandcurrency-consistency.test.ts, which make divergence fail the suite instead of needing eight lists hand-synced.Verified against production, not just read:
webhook_logshad zero rows. The delivery audit trail had never recorded a single attempt — three separate bugs each blocked the insert.R3-DIN-03proved with a probeUPDATEthat rolls itself back: rejected before the migration, accepted after. This corrected the register:NOT VALIDdoes not stop a CHECK enforcing, it only skips pre-existing rows.payments.payment_address_idandbusinesses.tierdo not exist — so every card payment 500'd, and one rail charged every business the free-tier rate.merchants.email_verifieddoes not exist, which also meant the OAuth ID token carried no email or name for any user. Not in the audit.CP-019was live:REPUTATION_SIGNING_SECRETwas unset in prod, so every reputation credential was signed withcpr-dev-secret— a constant in this public repo. Now set in the logicsrc vault.ENCRYPTION_KEYverified strong, so theF9-01guard is safe to deploy. That was the change with the most potential to break production.Six migrations applied to prod, each verified after. One needed a second migration: the
audit_logappend-only grant silently didn't take, because Supabase's default privileges had already grantedservice_roleeverything and aREVOKEmust name the role. Caught by readingrole_table_grants, then proved with a probe running asservice_role:insert=t update=f delete=f.Tests that asserted the bug. Seven existing tests encoded the vulnerable behaviour and are inverted, each with reasoning inline — including
"should allow if settings fail to load (fail open)","marks invoice as paid with 1% tolerance", and two named"should return email_verified as false"that assertedtrue. Two integrations (F4-01,NEW-L5-01) failed 100% of the time with green tests, because the tests asserted the same wrong thing the code did.Behaviour changes
expected.payTorequired on x402 v1 verify; x402 routes requirepayments:create.email_verifiedis nowfalseeverywhere.kidchanges tocoinpay-oidc-hs256(advisory under HS256, but client-visible).WalletClient.send()throws instead of silently signing the wrong thing.NEW-19's fix defaults consent to false; 28 businesses were listed without ever being asked. Backfill if consent exists.Still needs a decision
18 reputation issuers are active with 17 cleartext keys, including one claiming
Coinpayportal.comas its domain.doppler.env/doppler.jsonare untracked here but remain in git history.ESC-NEW-01has no disputed escrows today, so nothing is frozen, but an arbiter flow is a product call.Tests
Full suite green after the merge: 314 files, 4409 tests,
tsc --noEmitclean.packages/extensionhas two pre-existing red tests inapi.test.tsthat this branch does not touch, and that package is not covered by the root suite.🤖 Generated with Claude Code