Repository navigation
security: audit rounds 2-3 — 55 findings across money safety, identity, and silent failure - #279
Merged
Merged
Conversation
…ripe reads Round 2, batch 1 against docs/findings/. L-04 / R4-DIN-08 — forwardBusinessCollectionPaymentSecurely checked status === 'confirmed' as a READ and then wrote 'forwarding' unconditionally. Two overlapping cron runs both passed the check and both broadcast, sending 100% of the payment twice out of an address holding it once. Overlapping runs are not hypothetical: the retry queue re-enters the same function. The status write is now a conditional UPDATE that acts as the lock, and the loser is told so. CP-024 / IA-017 — uniqueness that existed only for payments. Proposal-to-invoice conversion is a read-then-write, so two concurrent conversions both saw a null invoice_id and both created an invoice, billing the client twice. Escrow and swap creation accepted no idempotency key, so a retried request created a second one. Migration 20260819170000 adds a unique index on proposals.invoice_id and partial idempotency indexes on escrows and swaps, mirroring the existing payments index. Verified beforehand that production holds no duplicate invoice_id values. G-1.2-02 — listAccessibleOwnerMerchantIds widened from "businesses I can see" to "the merchant accounts that own them" with no capability check, so membership of one business at readonly returned that owner's merchant id. The callers query stripe_payouts, stripe_disputes and subscriptions by merchant_id, so a readonly member read payout, dispute and subscription data for every business that owner has. Those tables carry no business_id to scope by, so the available control is who may ask: the capability argument is now required and the three routes pass billing.manage. BL-04 / F-1.1-13 — the comment in secure-forwarding promised the expected amount was kept "as a floor sanity check", and only <= 0 was ever tested. A balance that had dropped below the confirmed amount but stayed above zero forwarded anyway, splitting a pot that no longer matched the payment. Implements the promised check using the same relative epsilon confirmation uses, and refuses when the balance does not exceed the gas reserve. Full suite green (315 files, 4422 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…actions
Round 2, batch 2 against docs/findings/ — the P2 identity and multisig clusters.
L7A-02 / V-02 / REP-F1A-02 are one mechanism. did/register looked a merchant up
by email and bound an arbitrary DID to them with verified:true, on nothing but a
valid issuer key — no proof of possession of the DID, and none of the email.
That let a caller plant a hostile DID on a victim's account, ideally before the
victim claimed their own, since the existing-DID check makes the first
registration win and blocks the legitimate claim. Linking now requires the
merchant to have been provisioned by that platform, verified is no longer
asserted without proof, and an unlinkable DID still registers — just unbound.
F-1.1-02 — resolveSignerRole decided which party you are by comparing the pubkey
you sent against the three stored on the escrow. A public key is public, and
until this branch the escrow GET was unauthenticated too, so anyone could read
all three and act as any party by echoing one back. propose and dispute now
require a secp256k1 signature over a challenge that binds the specific action,
so a captured signature cannot authorise a different payout, a refund instead of
a release, or the same action on another escrow.
F-1.1-03 — GET /api/escrow/multisig was unauthenticated, so possession of an
escrow id exposed both parties' pubkeys, the amount, the lockup address and the
owning business. Now authenticated and ownership-checked, answering 404 rather
than 403 so it is not an existence oracle.
F-1.1-04 — requireMultisigAuth resolved the caller and returned { ok: true },
discarding the identity. A route that cannot see who is calling cannot scope to
them, which is why the body's business_id was persisted unchecked. The context
now comes back and creation authorises it.
R4-ID-RESET — the password-reset limiter keyed only on email, which is the
victim's bucket: an attacker spending it locked that account out of its own
reset flow. Both keys now use dedicated categories; the per-IP one is checked
first and is the tighter constraint. The email category also used
merchant_login, 5 per 5 minutes, while its comment claimed 3 per 15.
Full suite green (316 files, 4433 tests).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…sactions
Round 2, batch 3 against docs/findings/ — the P2 key-handling cluster.
F-L7-01 / F5-L1-02 — saveEncryptedWallet passes --output to gpg, so GPG creates
the file itself at the process umask, typically world-readable. Sibling writes
in the same CLI pass { mode: 0o600 }, but that only applies when Node does the
writing. Every other local user could copy an encrypted wallet and attack the
passphrase offline at leisure. The file is now chmod'd to 0600, and a failure to
do so warns rather than passing silently.
F5-L1-07 / L6B-05 / REC-01 — the passphrase protecting an exported wallet had no
minimum at all, on both the CLI and SDK paths, while the wallet create and
import flows require length plus a strength score. The exported file is the one
artefact that leaves the device, so it had the weakest gate gaurding the
strongest secret.
The rule is length-primary by design. Requiring particular character classes is
a poor proxy for entropy and penalises exactly the passwords people should be
encouraged to use: two existing tests used a 29-character symbol string and a
multi-script unicode passphrase, both genuinely strong, and both would have been
rejected by a naive upper/lower/digit check. Counted in code points so an emoji
counts once rather than twice.
REC-04 — WalletSDK.send() asked the server to prepare a transaction and then
signed whatever came back. Holding the key locally only protects the user if the
thing being signed is inspected; otherwise a hostile server returns an
unsigned_tx paying its own address, echoes the requested recipient in the JSON
beside it, and the SDK signs it. The unsigned transaction is a structured object
rather than an opaque blob, so the recipient can be checked with no chain
decoding: EVM native and ERC-20 calldata, Bitcoin outputs, Solana instruction
fields and account keys. Recipient only, since that is the field that turns a
payment into a theft and compares exactly on all three families; the amount is
verified server-side at broadcast against the prepared row.
Full suite green (317 files, 4448 tests).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // Lengthened when the backup password gained a minimum (L6B-05 / REC-01). | ||
| // The point of this test is that unicode round-trips through the crypto, | ||
| // not that a 9-character password is acceptable. | ||
| const unicodePassword = '密码🔐пароль-очень-длинный'; |
|
|
||
| const PAYEE_EVM = '0x1111111111111111111111111111111111111111'; | ||
| const ATTACKER_EVM = '0x2222222222222222222222222222222222222222'; | ||
| const TOKEN = '0x3333333333333333333333333333333333333333'; |
ThreatCrush Security Scan321 finding(s) HIGH/CRITICAL: 32 | MEDIUM: 37 | LOW: 252
…and 271 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…et claims NEW-20: POST /api/lightning/nodes required a BIP-39 mnemonic, validated it and never used it. The provisioned wallet is custodial LNbits — no signer, nothing to derive — so the field only made every client put the master seed for the whole wallet on the wire. Removed from route, both SDKs, the React component and the CLI; the web page was handing wallet.getMnemonic() straight to the component, and the CLI prompted for the passphrase to decrypt the seed purely to post it. Old clients that still send it are accepted and the value dropped. F-1.1-07: payouts:create is offered when minting a scoped key but enforced nowhere, so a read-only key could send money out of the business wallet. The sibling payments/create has always checked its own scope. NEW-07: WebAuthn login challenges were keyed on the merchant's own id, one slot per key, from a public route — so knowing an email was enough to overwrite a victim's pending challenge and lock them out indefinitely. The key is only a lookup handle, so it is now 32 random bytes per request. NEW-09/V-04: importWallet verified ownership only inside `if (secp256k1)` and the schema needs just one key, so an ed25519-only import skipped the check entirely; createWallet asked for no proof at all, and wallet_addresses is globally unique on (address, chain), so an unproved registration permanently denies an address to its real owner. Both now share one verifyKeyOwnership helper so they cannot drift again, and initial_addresses is capped. Found while wiring that: packages/sdk signed sha256(message) while the server verifies over raw bytes, so its proofs never verified and fromSeed() could not import a wallet at all. Now signs raw bytes. F-1.1-16: public create-order overwrote invoices.paypal_order_id, the only thing capture checked, so an attacker could displace the real payer's order and make the honest capture fail. Orders are now rows in paypal_transactions bound to the invoice; capture accepts any order bound to it. B-03: syncLnbitsPayments used .limit(100) with no .order(), so past 100 wallets the same hundred synced forever and the rest never reached ln_payments — which x402 Lightning settlement now requires as proof. Ordered and walked by keyset. NEW-23: the Signature-Agent directory fetch matched private literals only, so a hostname resolving to a link-local address passed. Now via safeFetch. Suite green at 317 files / 4455 tests, tsc --noEmit clean. BREAKING: /api/web-wallet/create now requires proof_of_ownership. Both in-repo SDKs send it as of this commit; published SDKs older than this cannot create wallets. The browser extension registers via /import and is unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ayees WW-01: the wallet's whitelist and daily spend limit were checked at prepare and never again. Prepare moves no money — broadcast does — and the check ran before the transaction row was inserted, so two prepares racing each other both read the same pre-insert total and both passed. checkTransactionAllowed now also runs at broadcast, against the prepared record the signed bytes were just bound to, with the row being broadcast excluded from the running total so its amount is not charged against the limit twice. WW-03 remains partial by design: the recipient is bound on every chain, but the amount is still unbound on BCH (bitcoinjs-lib does not handle its SIGHASH_FORKID variant) and on Solana (the amount sits in program-specific instruction data). Refusing outright would take those chains offline, so the broadcast proceeds and the gap is now logged at the moment it happens as well as recorded on the row. IA-016: escrow depositor, beneficiary and arbiter addresses were validated by .min(10) and nothing else — no chain-format check and no reserved-address check — while /api/payments/create validates both for the same kind of payout leg. A malformed address means a release broadcasts somewhere unspendable with no recourse; a platform fee wallet makes the leg indistinguishable from a fee payment and corrupts reconciliation on both sides. NEW-14 was already closed by resolveBusinessScope; marked as such rather than re-fixed. Also removes two duplicate `signature` keys introduced in the round-2 multisig test edit. The later key won and it held the original value, so no test changed meaning, but the shadowed literal was misleading. Suite green at 317 files / 4459 tests, tsc --noEmit clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment on lines
+27
to
+31
| import { | ||
| generatePaymentAddress, | ||
| isPlatformFeeWallet, | ||
| type SystemBlockchain, | ||
| } from '../wallets/system-wallet'; |
…ration G-1.2-10: anyone could start a CLI device authorization — it is unauthenticated by necessity, the CLI has no credential yet — and get back a verification_uri_complete that pre-filled the approval form. Sent to a signed-in merchant, one click handed the attacker's terminal a 7-day session JWT for the victim's account, with an attacker-chosen client_name displayed on the page to make it look plausible. Full account takeover, one click. The link is gone: start no longer returns verification_uri_complete, the page no longer reads ?code=, and the login redirect no longer carries a code through the round-trip. The merchant must type the code their own terminal printed — an attacker can still send someone to /cli-auth but cannot make the victim's screen show the attacker's code. client_name is now labelled self-reported. NEW-11: /api/cli-auth/poll was unauthenticated and unlimited while its sibling start was limited, and its answers are distinguishable (invalid / pending / denied / expired), so it was a free status oracle. Limited at 600/10min per IP, against a legitimate CLI's 120 polls per session. R4-ID-OAUTH: both state and code_challenge were optional on /api/oauth/ authorize. The session cookie is sameSite=lax and rides along on a top-level GET navigation, so a crafted link turned a victim's click into a completed authorization they never began. An authorization request must now be bound by one or the other. 278 of 318 live authorization codes already carry PKCE, so this rejects the unprotected minority rather than everybody. R3-ID-02: registration answered "Email already exists", telling any unauthenticated caller whether an address belonged to a merchant — and a confirmed address is exactly the input the email-keyed payout resolution and DID-binding attacks needed. The message is now identical to the route's other rejection paths. The password is also hashed BEFORE the existence check: the check returned early, so a registered address answered in milliseconds while an unregistered one paid for a bcrypt hash, which is an enumeration oracle on its own that no amount of care over the wording would have closed. Suite green at 317 files / 4461 tests, tsc --noEmit clean. BREAKING: OAuth clients sending neither state nor PKCE now get a 400. CLI builds older than this print a login link that no longer pre-fills the code; the code itself is still shown and still works. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
F5-L2-01: GitHub masks the ENV_FILE secret as a single blob, but it has no idea that the LNbits admin key, the droplet SSH key and everything else inside it are secrets in their own right. Any later step that echoed one — a set -x, a curl trace, a failing command printing its arguments — wrote it to the job log in the clear, and job logs outlive the credentials in them. Each parsed value is now registered with the runner's log masker as it is read. ::add-mask:: matches literal strings only, so multi-line values (an SSH private key) are masked line by line. Values under 8 characters are skipped: masking `true` or a port number would redact every incidental occurrence and make the log unreadable without protecting anything. Surrounding quotes are stripped so the masked string is what a consumer actually sees. Verified the parse loop against a representative ENV_FILE: long values masked, quotes stripped, short values skipped, and it does not trip `set -euo pipefail`. F5-L4-02 was already fixed — both HTML paths call escapeHtml. The raw interpolations the finding cites are in the plain-text body of the same email, where HTML-escaping would corrupt the output rather than protect it. Marked ALREADY-FIXED rather than changed. This completes Priority 2. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
F5-L1-06: scripts/sweep-balances.mjs is truncated mid-way through hexToWIF — where it was re-implementing base58 inline despite base58Encode being defined 20 lines above it — so `node --check` has failed since the file was introduced in December 2025. Its own header documents `pnpm sweep-balances`, which was never a script in package.json either. The documented emergency fund-recovery procedure has never worked, and nothing said so. The file now parses and does the half that can be verified: scan payment_addresses, read each balance from the same RPC endpoints (and the same env-var overrides) that monitor-balance.ts uses in production, and report anything above the per-chain dust threshold. An address whose balance could not be read is counted and reported separately rather than folded in as zero — for someone hunting stranded funds, "the node did not answer" and "the address is empty" must not look alike. --execute is deliberately NOT implemented and exits non-zero explaining why. Broadcasting sweeps would be new, untested, multi-chain fund-moving code, and shipping that unexercised risks sending funds somewhere unrecoverable — a worse outcome than the dust it is meant to recover. It points at the reviewed forwarding path instead. This is a partial fix and is recorded as one. hexToWIF is completed rather than deleted, because a WIF export is what an operator needs to sweep by hand once this tool has located the money. Verified against the canonical Bitcoin test vector: 0C28FC…AA1D encodes to 5HueCGU8rMjxEXxiPuD5BDku4MkFqeZyd4dZ1jvhTVqvbTLvyTJ uncompressed, and to a K-prefixed key compressed. Exact match, so the completion is correct rather than merely syntactic. Also wires `pnpm sweep-balances` so the command the file documents exists, and drops the imports the truncated derivation half had left unused. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
BL-02: the monitor marked a pending escrow `expired` on the clock alone, without ever reading its balance. A deposit that landed between two cron runs, or in the last minutes before the deadline, left the escrow holding real money in a status where every exit is closed at once — release wants funded or disputed, refund wants funded, dispute wants funded, and the auto-release sweep selects only funded. Nothing could move that money again, ever. The balance is now read before the write. A deposit that arrived in time is recognised as `funded`, and the existing expired-funded sweep auto-releases or auto-refunds it on the next cycle according to the escrow's own setting. Only a confirmed-empty escrow expires; an unreadable balance leaves it pending and retries, because expiring on a failed read is the same irreversible mistake. F-1.1-01: that sweep ignored escrow_model and flipped expired funded multisig escrows to `refunded`, then handed them to a settlement path that signs with a key the platform holds. A 2-of-3 escrow has no such key: its funds move only through proposeTransaction or disputeMultisigEscrow, both of which require status `funded`. Marking one refunded closed the only two ways its money could move while giving it to a path that cannot sign for it. They are now excluded in the query and again in the loop. CP-025: refundEscrow accepted the depositor's token and refunded on the spot, at any moment the escrow was funded. That is the buyer pulling their money back whenever they like — the escrow protected the buyer from the seller and the seller from nobody, which is the one thing an escrow exists to do. Two refunds remain legitimate and both still work: the beneficiary relinquishing their own claim, at any time; and the depositor once the deadline has passed, so they are not locked in forever. What is gone is the depositor refunding during the window the seller is performing in. The expiry check treats an unreadable date as not-yet-expired. `new Date( undefined)` is an Invalid Date and every comparison against one is false, so the obvious `expires_at > now` form would have granted exactly the refund the gate exists to withhold — the existing test fixture had no expires_at and would have sailed through it. 9 new tests. Suite green at 317 files / 4468 tests, tsc --noEmit clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…xist
All three verified against the live production schema rather than inferred.
B-04: /api/reputation/credentials filtered on `subject_did` and ordered by
`created_at`, and reputation_credentials has neither — the subject column is
`agent_did` and issuance is `issued_at`. PostgREST rejects an unknown column,
so the route returned 500 for every DID ever asked about. The same wrong column
appears in the agent reputation route, where it silently returned a null count
instead, so an agent's page reported zero credentials however many they held.
`subject_did` is real, but on mutual_attestations, which is presumably where it
was copied from.
L7A-04: /api/reputation/check queried `did_identities`, which does not exist.
Every DID came back "not registered" — and this endpoint is documented as the
pre-transaction impersonation check, so it was answering "unverified" to every
honest caller. It now queries merchant_dids, the registry its sibling route has
used all along, and distinguishes a lookup failure (503) from a genuine
non-registration, because reporting `verified: false` on a database error is
what kept the original bug invisible.
It also checked a `revoked` column. Neither that column nor any revocation
table exists, so DID revocation is not modelled anywhere in this system. The
response now says `revocation_checked: false` out loud rather than dropping the
check silently — a caller deciding whether to trust a counterparty must know a
compromised DID cannot currently be turned off.
L5-02: deriveCardanoWallet returned `addr1_${pubkeyHex.slice(0,40)}...` — a
placeholder, trailing ellipsis included, that is not a Cardano address in any
sense. This is the custodial wallet: the address a customer is told to pay.
Production issued five of them between February and July 2026; none was ever
paid, because no wallet accepts a malformed address.
Only generation was a stub — CardanoProvider.sendTransaction builds and submits
real transactions with cardano-serialization-lib, already a dependency. So the
rail was broken at exactly one end. It now derives a mainnet enterprise address
from the same key. Verified against the installed library: valid 58-char
addr1..., round-trips through Address.from_bech32 (which the spend path calls),
and the `to_bytes().slice(1, 29)` key-hash fallback the spend path uses lands
on the matching 28-byte payment key hash.
Restores four test files to the suite. All seven were excluded for a ws
CommonJS/ESM incompatibility that the config's own ws alias already fixes; the
exclusions outlived the problem. This mattered: system-wallet.test.ts covers
custodial address derivation, and while it sat unrun its ADA test asserted the
shape of the *broken* address, so L5-02 had a passing-looking test defending it.
The three still excluded no longer fail on module loading either — they fail on
assertions that drifted while nobody ran them, and the comment now says so.
Suite goes from 317 files / 4468 tests to 321 / 4557. tsc --noEmit clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tches A-05: the Boltz routes logged a failed insert and answered success:true. That row is the only place the platform keeps refundPrivateKey (swap-in) or claimPrivateKey (swap-out) — the sole means of reclaiming or claiming the funds. A failed write therefore left a live swap whose key existed nowhere on our side, and told the user everything was fine so they deposited against it. The swap cannot be un-created at Boltz, and a bare 500 would lose the key just as completely, so the failure response carries the key material with an explicit instruction to save it before depositing. The key is already in the success response to the same authenticated caller, so this exposes nothing new. F-1.3-08: the ChangeNOW route had the same silent success, but no key to lose — the deposit address in the response is all the user needs. Failing outright would break a usable swap, so it now reports `tracked: false` with a warning instead of being indistinguishable from a healthy one. W-08: the refund route's open-dispute guard discarded the query error, so any lookup failure produced a null row and the guard concluded "no dispute" — the opposite of what a failed check means. It now refuses with 503. "Could not check" is not "nothing to find". F-1.3-09 and H-R-05: `.limit(N)` with no `.order()` over sets that never drain. Invoices stay `sent` until paid and the five notifiable escrow statuses are terminal, so past N rows the same N are processed every run and the rest never at all — the invoice rail stalls platform-wide and status emails stop entirely, both with no error anywhere. Ordering alone would not fix it: an ordered query returns the same first page just as reliably. The page has to advance. Adds lib/db/keyset.ts so this family has one implementation rather than four. B-03's inline fix from the earlier batch is refactored onto it; F-1.3-12 is still to convert. Keyset rather than offset because .range() re-scans and rows shifting under a long sweep make it drop or duplicate entries. 6 tests cover the helper, including that a failing page returns the rows already read rather than discarding completed work. Production today: 18 invoices are `sent`, so F-1.3-09 is latent rather than active. It triggers on growth, silently. Suite green at 322 files / 4563 tests, tsc --noEmit clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s idempotency
F-1.3-04: balance-checkers.ts — the oracle secure-forwarding reads — had
`case 'DOGE': console.log('not yet implemented'); return 0`. DOGE payments are
*confirmed* by a different oracle (payments/monitor-balance.ts) which has always
had a real implementation, so the two disagreed by construction. A confirmed
DOGE payment reached forwarding, was told the address held nothing, and was
pushed back to `confirmed`; on every subsequent run, the same. The funds would
sit at the intermediary address permanently while the payment looked healthy.
Now uses the same two sources in the same order as the confirmation-side
checker. Two oracles disagreeing about one address is the defect; a
differently-sourced second implementation would only hide it. Production has
never confirmed a DOGE payment — all four are `expired` — so nothing is
stranded today; this closes the trap before it is walked into.
INV-01: the Stripe webhook dispatched on event.type and never looked at
event.id. Stripe redelivers whenever the endpoint times out or answers non-2xx,
so every handler was re-runnable by a retry the platform does not control, and
charge.refunded and payment_intent.succeeded both write money-shaped records.
Events are now claimed in stripe_webhook_events before dispatch; the primary key
does the work, and a conflicting insert returns 200 rather than re-running. A
handler that throws releases its claim, so a transient failure still gets
Stripe's retry instead of being permanently swallowed — dropping an event is a
worse failure than the double-processing the claim prevents. A claim that fails
for any other reason refuses to process, because we cannot then tell a duplicate
from a first delivery.
Migration 20260819180000 applied to production and verified.
H-R-04: monitorSeries created the escrow first, then wrote periods_completed
and next_charge_at with only `.eq('id', ...)` — no compare-and-swap against the
state it had read. Two overlapping runs both read the same period, both created
an escrow for it, and both wrote the same next period: the subscriber billed
twice, the counter advanced once, nothing downstream showing it. A slow run
overlapping the next tick is all it takes.
The period is now claimed before anything is created for it, conditioned on the
values that were read. A run that loses the race skips rather than duplicates,
and a failed creation gives the claim back so the period is retried.
4 new tests, including that nothing is created for a contested period and that
the claim happens before creation — ordering matters as much as the condition,
since claiming afterwards means the duplicate already exists.
Suite green at 322 files / 4565 tests, tsc --noEmit clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…n tokens H-R-09: walletAddressSchema was z.string().min(26).max(100) — length and nothing else — and was declared separately in wallets/service.ts and wallets/merchant-service.ts, so the two payout tables could drift apart as well as both being wrong. These rows are payout destinations: whatever a merchant saves is where their money is sent, with no confirmation step and no way to recall a transfer. Now one shared validator, format-checking against the chain being saved for, on both the create and update paths in both files. It keeps isValidPayoutAddress's three-state contract: malformed-for-this-chain is rejected, no-validator-for- this-chain is allowed, because blocking a save we have no rule for is worse than the check being unavailable. It immediately caught an invalid address in this repository's own fixtures — 0x742d…bEb1234, 44 hex characters where Ethereum has 40 — which had passed the length check for as long as it had existed. Long enough to look right at a glance, which is the whole problem. G-1.2-04: the realtime SSE endpoint took a 7-day session JWT as ?token=. A URL is the least private place for a bearer credential: it lands in proxy and CDN access logs, browser history, and the Referer of any subsequent navigation, so anyone with log access holds week-long sessions for every merchant who opened a dashboard. The usual excuse is that EventSource cannot set headers — but it does send cookies same-origin, and the session cookie is already httpOnly, so withCredentials authenticates the stream without the token appearing in a URL at all. The query parameter still works for stale bundles and logs a warning when used. NEW-L5-1: business_collection_payments_blockchain_check was written 2025-11-30 and never revisited. It allowed eleven chains; business-collection.ts has since grown to eighteen, and all seven token variants added since (USDT_ETH/POL/SOL, USDC_ETH/POL/SOL/BASE) were rejected at INSERT by a code path that believes the chain is supported. Migration 20260819190000 applied to production and validated; verified beforehand that only ETH and BTC rows exist, so it could be validated immediately rather than left NOT VALID. R3-DIN-03 needed no change: confirmed against the live schema that settle_failed is now in escrows_status_check. 8 new tests for the address validator. Suite green at 323 files / 4573 tests, tsc --noEmit clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| // but it does send cookies on a same-origin request, and the session | ||
| // cookie is httpOnly, so `withCredentials` authenticates the stream | ||
| // without the credential ever appearing in a URL. | ||
| const eventSource = new EventSource(url, { withCredentials: true }); |
F-1.3-12: both escrow settlement windows were `.limit(20)` with no `.order()`. An escrow stays `released` (or `refunded`) until it settles, so one that can never settle — dead RPC, no gas, bad address — holds its slot permanently. Twenty of those and the window is full: no newly-released escrow is ever settled again, and the job reports success every run. The retry gate inside processEscrowSettlement already declined to re-attempt an exhausted escrow, but a skipped escrow still occupied one of the twenty slots, so the gate made the stall cheaper rather than fixing it. Both windows now walk the set via the shared keyset pager, which completes the conversion of the B-03 / F-1.3-09 / F-1.3-12 / H-R-05 family onto one implementation. F3-L5-02: waitForSwap awaited getSwapStatus bare inside its polling loop, so a single 502, 504 or socket timeout threw straight out. The swap is unaffected — it is still in flight at the provider — but the caller has lost all visibility of it, and the natural reading of "the call threw" is that the swap failed. A transient blip during a poll that runs for up to an hour is expected, not exceptional. Isolated failures are now retried on the next tick; five consecutive ones stop the loop with a message saying the swap may still be in progress, rather than spinning silently to the timeout. F-1.1-06 needed no change: settleRefundedEscrows and settleReleasedEscrows both go through processEscrowSettlement now, so they already share its retry cap. Marked ALREADY-FIXED rather than re-fixed. Suite green at 323 files / 4573 tests, tsc --noEmit clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ralyodio
changed the title
F-1.3-10, NEW-F1A-P-01 and R4-DIN-07 are three instances of one class. Four call sites generated INV-NNN; only /api/invoices did it correctly. The other three — convertToInvoice, the recurring scheduler, and the p2p request route — shared two defects: Ordering by created_at and taking the first row. The highest number is not the newest row. Backdate an invoice, import a historical one, or delete the most recent, and the "maximum" is whatever sorts first by time, so the next number collides with one that already exists. No retry on the unique violation. (business_id, invoice_number) is unique, so two concurrent creates both read the same maximum, compute the same number, and the loser fails with 23505. In convertToInvoice that is a proposal that will not convert. In the recurring scheduler it is worse: the cycle throws, the schedule is never advanced, and the subscription silently stops invoicing. Both now live in lib/invoices/numbering.ts, and all four sites use it — including the one that was already right, because keeping four copies is how three of them came to be wrong. The insert stays a callback since the sites select different columns back; the only thing they need to share is the numbering. nextInvoiceNumber reads every numbered invoice for the business rather than ordering. That is a full scan per call, but per-business invoice counts are in the tens to hundreds, and the alternative is a sequence — a schema change that would still need the retry for imports. 8 tests on the helper, including that the highest parsed number wins over the newest row, that a 23505 is retried with a fresh number while a permissions error is not, and that the attempt cap terminates. Suite green at 324 files / 4581 tests, tsc --noEmit clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ge lockfile IA-010: both BitcoinProvider.sendSplitTransaction and EthereumProvider's native send silently scaled the amount down to whatever fit the balance, logged it at console.log, and carried on. The caller asked to send X, X-δ went out, and everything downstream recorded X — the merchant's ledger, the fee split and the webhook all described a payment that did not happen. δ was unbounded: a balance half the requested amount simply sent half. Taking the miner fee (or gas) out of the outputs is the intended behaviour and is kept, because on both chains the fee and the funds are the same asset. What is refused now is a balance that cannot cover the amount *before* fees — that is a real deficit and something is wrong upstream, so it throws instead of quietly sending less than asked. The remaining adjustment logs at warn. L-03: the production image ran `pnpm install --no-frozen-lockfile`, justified by a comment saying the lockfile drifts. That flag lets pnpm resolve versions the lockfile does not record, so two builds of the same commit can ship different dependency trees and a changed — or compromised — transitive dependency reaches production without appearing in any diff. For a payments platform that is the whole argument for having a lockfile. The premise was also stale: `pnpm install --frozen-lockfile` passes against the current tree, verified before making the change. If it drifts again the build now fails loudly, which is the point — a drifted lockfile is something to fix in a commit, not to route around on every deploy. F4-03: the FossBilling config screen never showed the webhook URL, and nothing registers it automatically. A merchant configuring the gateway from that screen alone ends up with a working checkout and no callback: payments are taken and invoices are never marked paid. The failure is silent and reads as CoinPayPortal not paying out, because every field on the page looks correctly filled in. Now a read-only field with the path and what to do with it. Suite green at 324 files / 4581 tests, tsc --noEmit clean. PHP is not installed locally so CI lints the plugin change; delimiters checked by hand. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…te an exit
IA-008: the two balance oracles issue 29 calls between them to third-party
explorers and RPC nodes, and not one set a timeout. Node's fetch has no default
one, so a peer that accepts a connection and then never answers holds the
request open indefinitely — and because the monitor awaits these in sequence,
one unresponsive upstream stalls the entire cycle. Payments stop being confirmed
platform-wide and nothing errors: the cron is simply still running, forever.
All 29 now go through a shared fetchWithTimeout with a 15s deadline. A slow
upstream costs one skipped check on one address rather than the run. The timer
is cleared on the success path too, since an uncleared one keeps the event loop
alive and would stop a short-lived process exiting.
N-03: `indeterminate` had no exit transition. A payout lands there when the
broadcast outcome is unknown — a timeout after the node may already have
accepted the transaction — and retryPayout refuses to touch one, correctly,
because re-sending could pay the recipient twice. Its error message told the
operator to "mark the payout completed with its tx_hash" or "mark it failed and
retry", and no route or function existed to do either. The state described a
procedure nobody could carry out, so those payouts were stuck permanently.
PATCH now accepts {resolution: 'completed'|'failed'}. It stays manual: only
someone who has checked the chain can say which way it went, and a wrong
automatic answer either pays twice or strands a real payment. Marking completed
requires the tx_hash, because that is the evidence the transfer landed —
otherwise a payout could be closed on assertion alone. The update is
conditioned on the status still being indeterminate so two operators cannot
both apply an outcome, and the original error message now names the procedure
that actually exists.
6 new tests. Suite green at 325 files / 4587 tests, tsc --noEmit clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… producer NEW-F1A-P-02: expires_at was checked in exactly one place — acceptProposal — so an expired proposal could still be countered, rejected, withdrawn, re-sent and viewed by token. The deadline meant "you cannot accept this" rather than "this is over", which is not what either party takes it to mean when they set one. A deadline that stops the deal closing but not the negotiation continuing is backwards. 'expired' is also a permitted value of proposals_status_check, and of the ProposalStatus union, that nothing ever wrote. The state was reachable in the schema and unreachable in practice, so no proposal has ever shown as expired in any list or dashboard — they sit as `sent` for ever. One shared assertNotExpired now gates accept, reject and counter, and flips the status on the way past. That gives the state a producer without a new cron: expiry only matters when someone tries to act, which is exactly when this runs. The write is conditioned on the proposal still being negotiable, so a lazy expiry cannot overwrite a concurrent accept or reject — and an already-accepted proposal past its deadline stays accepted, because expiry must not undo a decision. An unparseable deadline is treated as no deadline rather than as an expired one: refusing to act because of a bad timestamp would be a worse failure than letting it through, since the deal is real and only the date field is broken. 6 new tests. Suite green at 326 files / 4593 tests, tsc --noEmit clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ESC-NEW-14: GET /api/escrow/platform-arbiter derives the arbiter key at index 0 of the same system mnemonic that payment and escrow addresses come from. And getMaxFamilyIndex returns -1 for a family with no addresses yet, so acquireFamilyIndex started at `seed + 1` = 0 — meaning the first payment or escrow address ever created on a chain derived the very key the platform signs disputes with. One key, two roles, and customer funds sitting on the address the arbiter controls. Index 0 is now a named reservation the counter never issues, including from a legacy counter already sitting on it. The codebase already reserves BIP44 account 1 for the gas relayer for exactly this reason (see GAS_RELAYER_DERIVATION_PATH) — this is the same idea one level down: a slot the counter cannot enter. The route uses the constant rather than a bare literal, so the two ends cannot drift. Every fresh family now starts at 1. The existing tests asserted the old start-at-0 values, which is to say they asserted the collision; they are updated and two now check the reservation directly. Suite green at 326 files / 4594 tests, tsc --noEmit clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hold SUB-02: /api/subscriptions/checkout was authenticated and otherwise unbounded. Every call derives an HD address, encrypts its private key and writes a business_collection_payments row, so a merchant looping it accumulates rows and key material without limit and burns derivation indexes that are never reclaimed. No attacker required — a retry loop in a client does it. Two bounds, because they catch different things: a per-merchant rate limit stops a burst, and a cap on unpaid checkouts stops a slow accumulation that would never trip a rate limit at all. The cap is deliberately generous at 10, since abandoning a checkout to switch chain or billing period is normal — it exists to stop unbounded growth, not to police indecision. A failed count logs and proceeds rather than blocking the upgrade: refusing a paying merchant because a COUNT query failed would be a worse outcome than the accumulation this guards against. Suite green at 326 files / 4594 tests, tsc --noEmit clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…f it F-1.3-15: a business collection address holds exactly crypto_amount — the quote adds no network fee — and the forwarder asked the provider to send all of it. On UTXO chains BitcoinProvider.sendTransaction refuses outright when amount + fee > balance, which is always true here, so every BTC collection sweep threw and the payment sat in forwarding_failed with the funds stranded at the intermediary address. On EVM the same shortfall was silently absorbed by reducing the amount (IA-010), so it "worked" while under-sending — the two chains failed differently at the same defect. sendSplitTransaction with a single recipient is the sweep primitive: it takes the fee out of the outputs rather than demanding it on top, which is the only thing that can happen when the fee and the funds are the same asset. Used where the provider offers it; sendTransaction stays the path for account-model chains that deduct gas separately. This composes with the IA-010 guard rather than defeating it: that guard refuses a balance below the amount *before* fees, and a sweep of exactly the balance is not that case — it is the fee-absorption path the guard deliberately preserves. Not fixed, and deliberately: the quote still does not add an estimated network fee on top, so the merchant nets amount-minus-fee. That is a pricing decision rather than a defect, and it is recorded in TODO-vulns.md for Anthony rather than changed here. Suite green at 326 files / 4594 tests, tsc --noEmit clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s clean Verified against live production rather than assumed: V-05 / CP-P4 — neutralized, confirmed: monthly_transaction_limit is NULL on both plans, so there is no quota for the card branch to bypass. It re-activates the moment any plan gets a non-null limit. NUEVO-F2-01 / L4-NEW-01 — confirmed remediated: every policy on ln_nodes, ln_offers, ln_payments and swaps is scoped to service_role, or to authenticated with a merchant predicate. No FOR ALL USING(true) reachable by anon. Rotation of gl_creds/gl_rune is still warranted and stays Anthony's call — the code is fixed, but the five-day exposure window happened. V-06 — resolved by the line-by-line cross-check the finding asked for. Every column the code touches on stripe_accounts exists in the live schema. Two apparent misses were a chained merchant_wallets query inside the same Promise.all, not stripe_accounts references. Fixed: ESC-NEW-05 — GET /api/escrow/model-availability advertises multisig_default and only the browser ever acted on it. An API caller omitting escrow_model got a custodial escrow — CoinPay holding the funds — on a deployment advertising the opposite. That is the same silent-custody failure the explicit-request branch was already fixed for, reached by omission instead. Refusing would break every integration that has always omitted the field, so the escrow is still created and the response names the custody model and points at the multisig endpoint. selectEscrowModel is documented as having no callers, so it is not misread as the thing governing custody: it is not, and was not when the audit was written. H-R-03 — the cron webhook POSTed to a merchant-supplied URL with a raw fetch. That is a request-forgery primitive the platform executes on a schedule with no user interaction: a merchant can point webhook_url at anything the cron's network reaches, including metadata endpoints. Now via safeFetch, which resolves the host and re-validates redirects; a refusal is recorded in webhook_logs rather than being invisible. F5-L2-03 — neither .dockerignore excluded .env*, and the root Dockerfile does COPY . ., so any .env in the working tree is baked into an image layer — .env.prod holds live credentials. Railway builds from git where these are gitignored, so the documented path was never affected; a local or CI build from a developer's tree was. Suite green at 326 files / 4594 tests, tsc --noEmit clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
F-1.3-14: the collection address index came from a 32-bit string fold reduced modulo 10^6, and the addresses it derives hold customer money. A million slots means two different collection payments share an index — and therefore an address and a private key — with about even odds by the 1,180th one. When that happens one payment's funds land at another's address: the balance check confirms the wrong payment, and the sweep sends the money to the wrong destination. No attacker is involved; it is the birthday bound. Two changes, because only the second is actually a fix. The hash is now SHA-256 over the full non-hardened BIP32 range, which moves the even-odds point from ~1,180 to ~54,000 — a mitigation, not a guarantee. The call site then checks the derived address against existing collection payments and walks to the next index on a clash, which is what makes a collision impossible rather than merely unlikely. A failed uniqueness check aborts rather than deriving anyway, since deriving anyway is precisely the finding. Collections derive from MASTER_MNEMONIC while payments derive from SYSTEM_MNEMONIC_*, so these are separate key spaces and collections only had to be made safe against themselves. Checked before changing the index source. 5 tests, including one that demonstrates the defect rather than the fix: reducing the same hash back to 10^6 reintroduces collisions on the same input set immediately, which is why the range widening alone was not treated as sufficient. Suite green at 327 files / 4599 tests, tsc --noEmit clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
F-1.3-03: the asset check only ran when the merchant named one in the payment requirements, so a price with no asset stated was satisfied by a proof denominated in an arbitrary token. The payer chooses that token, and 1000 units of something they minted this morning costs nothing — the resource is unlocked for free. Rejecting everything unnamed would break the integrations that quote a price and let the well-known stablecoin for the network be inferred, so the fallback is an allow-list rather than a refusal: native currency, or one of the stablecoins the platform itself settles — the same contracts secure-forwarding knows how to move. An invented token is in neither set. R3-X1 needed no change: the Stripe branch already compares pi.amount_received from Stripe's own record rather than the self-declared payload amount, which was fixed in round 1. The SIGNATURE_BOUND_NETWORKS constant the finding cites no longer exists — dispatch is by network via SCHEMES_BY_NETWORK. One new test, for the invented-token case that motivated the fix. The accepting side is deliberately not given a new test: six existing tests already pay in real USDC with expected.asset unset and still pass, which is exactly that case. A Lightning fixture for it would need a matching ln_payments row and would end up testing the settlement proof rather than the asset rule. Suite green at 327 files / 4600 tests, tsc --noEmit clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ing the platform key E-04: REQUIRED_EVENTS in ensure-platform-stripe-webhook listed 7 of the 10 events the webhook route handles, and --apply reconciles the live endpoint to that list — so running it unsubscribed charge.dispute.updated, charge.dispute.closed and charge.refunded. The handlers stayed in the code and simply stopped being called: disputes recorded on creation and never updated or closed, refunds never recorded at all, and the dashboard showing the stale state as if it were current. Now 10 for 10, with a note that an unlisted handler is a handler --apply turns off. F-1.3-06: when LNbits wallet creation fails with a user-auth error, the address route fell back to the platform's LNBITS_ADMIN_KEY — which controls every wallet on the instance — and wrote it into that user's wallets.ln_wallet_adminkey. Four call sites read that column as "this wallet's key", including POST /api/lightning/payments, which passes it straight to payInvoice. The affected user could have paid Lightning invoices out of the platform's own balance. The fallback exists so a username claim still works when the instance requires user-token auth, and creating the LNURLp pay link is all it is needed for. That use is transient and stays; nothing is persisted, so the wallet keeps no spending authority it did not earn. Verified against production: the fallback has never fired. All 6 wallets holding an admin key hold their own — they have both ln_wallet_adminkey and ln_wallet_inkey, and the fallback path writes only the former (adminkey_only is 0). This fix is preventive rather than remedial. IA-006 needed no change: the "sequential settlement commission leak" is the split-ordering bug where the merchant leg drained the address before the platform-fee leg ran. The split is now computed from the actual balance and both legs go out in a single split transaction, so neither can starve the other. Suite green at 327 files / 4600 tests, tsc --noEmit clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… series path F4-02: processTransaction's only guard against a repeat delivery was `if ($invoice['status'] === 'paid') return;` — a check-then-act with a gap in the middle. Two deliveries of the same event, and CoinPayPortal retries as every webhook sender does, both read a status that is not yet 'paid', both create a transaction and both mark the invoice paid. The merchant's books then show the invoice settled twice. `txid` is the payment id and stable across retries, so it is the natural idempotency key. The lookup is wrapped defensively: if a FOSSBilling build does not expose transaction_get_list, the behaviour degrades to what it was rather than breaking payment capture outright. ESC-NEW-06: the fallback recurring-series path POSTed to our own /api/escrow with `Bearer INTERNAL_API_KEY`. That route authenticates via authenticateRequest, which has no notion of an internal key — it resolves merchant JWTs and business API keys — so every request was rejected 401 and no series escrow was ever created by this path. It went unnoticed because the primary cron creates them by calling createEscrow directly and succeeds, so the fallback only mattered when the primary was down, which is exactly when it was needed. It now calls createEscrow directly, like the primary. Server-side code calling server-side code has no reason to make an HTTP round trip and re-authenticate to itself, and doing so is what created an auth mode that had to exist and did not. createEscrow persists series_id itself, so the follow-up UPDATE is gone as well. Suite green at 327 files / 4600 tests, tsc --noEmit clean. PHP is not installed locally so CI lints the plugin change; delimiters checked by hand. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Priority 5 verification sweep. The tier is the audit's own "no business impact
only because of a stated condition", so the agreed treatment was verification
with evidence rather than pretending a code change happened. Recorded in
TODO-vulns.md; this commit is the one thing the sweep found worth fixing.
IA-002's RLS gap is NOT a live data exposure, and the alarming reading of it is
wrong. Every table in public has RLS enabled, and every {public} INSERT policy
carries a real with_check predicate — none is unconditional, so there is no
anonymous-write hole. Those rows show a null `qual` because INSERT policies keep
their predicate in `with_check`; reading the wrong column is what makes it look
like a hole.
What the sweep did find: reputation_receipts carries SELECT ... USING (true) for
anon and authenticated, which reads as world-readable, and is unreadable today
only because neither role holds a SELECT grant. RLS is evaluated after the grant
check, so a permissive policy on an ungranted table is inert.
That is a trap rather than a control. GRANT SELECT ON ALL TABLES IN SCHEMA
public TO anon is a routine Supabase incantation, and running it once would have
published 14,346 receipts carrying agent_did, buyer_did, escrow_tx and amounts
totalling $1,050,924 — the platform's entire transaction history by counterparty
and value — with no other change and no warning.
The policy now says what the grants already enforce. Nothing reads this table
through PostgREST (the application uses the service role, which bypasses RLS),
so it cannot break a working path. Applied to production and verified.
The sibling tables are deliberately untouched: mutual_attestations,
reputation_credentials and reputation_revocations ARE granted to anon and are
the public verifiable trust graph — the product working as designed.
The dead-code claims were tested rather than trusted. clearSensitiveString has
zero callers, confirmed. initSecrets' two apparent callers are both inside the
module's own doc comment, so it is genuinely dead — though it documents a usage
pattern nothing follows.
Suite green at 327 files / 4600 tests.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous commit message and the P5 sweep note said this table held "amounts totalling $1,050,924 — the platform's entire transaction history by counterparty and value". Both halves were wrong. The figure came from summing the `amount` column without looking at what was in it. Of that $1,050,924, some $1,050,002 is six rows carrying round test values (1000001, 50001) with no platform_did. The real content is 14,333 did:web:ugig.net reputation receipts totalling $921.03. And they are ugig.net's agent trust graph, not CoinPay payment records — those live in `payments` and were never part of this. The migration itself stands and is unchanged in effect: a USING(true) policy held back only by a missing grant is a trap regardless of what the table holds, and scoping it to service_role costs nothing. But the dollar figure made a tidy-up read as an incident, so it is removed from the ledger and the migration comment. The earlier commit message cannot be edited without a force-push, so this commit is the correction of record. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 19, 2026
ralyodio
added a commit
that referenced
this pull request
Aug 19, 2026
…bhooks, installer pinning (#282) Closes the three product gaps left open after #279. ESC-NEW-01: dispute_resolution/dispute_status had no writer, and refund required 'funded' — so raising a dispute removed the refund path entirely. 'disputed' is now refundable and resolveDispute() adds an admin-gated arbiter exit. REC-D-07: webhook retry spent its whole budget in ~3 seconds inside one request. Adds a durable queue with 1m-12h backoff and a dead-letter after 8 attempts; rows are claimed before delivery and payloads re-signed per attempt. W-01: unattended auto-upgrade from a mutable ref is now opt-in. Previously any merge to master executed on every operator host within five minutes. Also deactivated 5 bogus reputation issuers in production (reversible), after verifying only ugig.net and d0rz.com have ever produced a receipt. 334 files / 4690 tests green, all 8 CI checks pass.
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
…bhooks, installer pinning (#282) Closes the three product gaps left open after #279. ESC-NEW-01: dispute_resolution/dispute_status had no writer, and refund required 'funded' — so raising a dispute removed the refund path entirely. 'disputed' is now refundable and resolveDispute() adds an admin-gated arbiter exit. REC-D-07: webhook retry spent its whole budget in ~3 seconds inside one request. Adds a durable queue with 1m-12h backoff and a dead-letter after 8 attempts; rows are claimed before delivery and payloads re-signed per attempt. W-01: unattended auto-upgrade from a mutable ref is now opt-in. Previously any merge to master executed on every operator host within five minutes. Also deactivated 5 bogus reputation issuers in production (reversible), after verifying only ugig.net and d0rz.com have ever produced a receipt. 334 files / 4690 tests green, all 8 CI checks pass.
ralyodio
added a commit
that referenced
this pull request
Aug 19, 2026
…bhooks, installer pinning (#282) Closes the three product gaps left open after #279. ESC-NEW-01: dispute_resolution/dispute_status had no writer, and refund required 'funded' — so raising a dispute removed the refund path entirely. 'disputed' is now refundable and resolveDispute() adds an admin-gated arbiter exit. REC-D-07: webhook retry spent its whole budget in ~3 seconds inside one request. Adds a durable queue with 1m-12h backoff and a dead-letter after 8 attempts; rows are claimed before delivery and payloads re-signed per attempt. W-01: unattended auto-upgrade from a mutable ref is now opt-in. Previously any merge to master executed on every operator host within five minutes. Also deactivated 5 bogus reputation issuers in production (reversible), after verifying only ugig.net and d0rz.com have ever produced a receipt. 334 files / 4690 tests green, all 8 CI checks pass.
ralyodio
added a commit
that referenced
this pull request
Aug 19, 2026
…bhooks, installer pinning (#282) Closes the three product gaps left open after #279. ESC-NEW-01: dispute_resolution/dispute_status had no writer, and refund required 'funded' — so raising a dispute removed the refund path entirely. 'disputed' is now refundable and resolveDispute() adds an admin-gated arbiter exit. REC-D-07: webhook retry spent its whole budget in ~3 seconds inside one request. Adds a durable queue with 1m-12h backoff and a dead-letter after 8 attempts; rows are claimed before delivery and payloads re-signed per attempt. W-01: unattended auto-upgrade from a mutable ref is now opt-in. Previously any merge to master executed on every operator host within five minutes. Also deactivated 5 bogus reputation issuers in production (reversible), after verifying only ugig.net and d0rz.com have ever produced a receipt. 334 files / 4690 tests green, all 8 CI checks pass.
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.
Continues #277 and #278. Master had 64 findings fixed; this branch adds ~55 more, taking the audit from 64 to roughly 160 of 338.
Every schema claim below was checked against the live production database, not inferred from migrations.
Money can no longer be stranded or moved twice
BL-02— the monitor marked a pending escrowexpiredon the clock alone, without reading its balance. A deposit landing between two cron runs left the escrow holding real money in a status where every exit is closed at once: release wantsfundedordisputed, refund wantsfunded, dispute wantsfunded, and the auto-release sweep selects onlyfunded. Nothing could move that money again, ever. The balance is now read before the write, and an unreadable balance leaves it pending rather than expiring on a failed read.F-1.1-01— that same sweep flipped expired funded multisig escrows torefundedand handed them to a settlement path that signs with a key the platform holds. A 2-of-3 escrow has no such key; its funds move only viaproposeTransaction/disputeMultisigEscrow, both of which requirefunded. Marking one refunded closed the only two exits while giving it to a path that cannot sign for it.CP-025—refundEscrowaccepted the depositor's token and refunded on the spot, any time the escrow was funded. That is the buyer pulling their money back whenever they like: the escrow protected the buyer from the seller and the seller from nobody. Both legitimate refunds still work — the beneficiary relinquishing their claim at any time, and the depositor once the deadline passes.F-1.3-04—balance-checkers.ts, the oracle forwarding reads, hadcase 'DOGE': console.log('not yet implemented'); return 0. DOGE payments are confirmed by a different oracle that has always had a real implementation, so the two disagreed by construction: a confirmed DOGE payment reached forwarding, was told the address held nothing, and was pushed back toconfirmedon every run thereafter while the funds sat at the intermediary address. Production has never confirmed a DOGE payment (all four areexpired), so nothing is stranded today.H-R-04,INV-01— a recurring series wroteperiods_completedwith no compare-and-swap against the state it read, so two overlapping runs both billed the same period and the counter advanced once. The Stripe webhook dispatched onevent.typeand never looked atevent.id, so every handler was re-runnable by a retry the platform does not control.Secrets that were never supposed to travel
NEW-20—POST /api/lightning/nodesrequired a valid BIP-39 mnemonic, validated it, and never used it. The provisioned wallet is custodial: there is no signer and nothing to derive. The only effect was to put the master seed for the entire wallet on the wire. The web page calledwallet.getMnemonic()and handed the live seed to a component prop; the CLI prompted for the passphrase to decrypt the stored seed purely so it could post it.G-1.2-04— a 7-day session JWT travelled as?token=on the SSE endpoint, landing in proxy/CDN logs, browser history andReferer.EventSourcecannot set headers, which is why — but it does send cookies same-origin, and the session cookie is already httpOnly.F5-L2-01— GitHub masks theENV_FILEsecret as one blob but has no idea the LNbits admin key and droplet SSH key inside it are secrets too. Each value is now registered with the log masker as it is parsed.Claims nobody checked
V-04/NEW-09—createWalletasked for no proof of anything, andimportWalletverified ownership only insideif (public_key_secp256k1)while the schema requires just one of two keys.wallet_addressesis globally unique on(address, chain), so an unproved registration permanently denies an address to its real owner. Both now share oneverifyKeyOwnership, deliberately, so they cannot drift apart again.G-1.2-10— anyone could start a CLI device authorization and get back a link that pre-filled the approval form. Sent to a signed-in merchant, one click handed the attacker's terminal a 7-day session JWT, with an attacker-chosenclient_nameon the page. The pre-filled link is gone; the code must be typed from your own terminal.R4-ID-OAUTH,NEW-07,R3-ID-02,F-1.1-07,IA-016,H-R-09,W-08,NEW-23— login-CSRF, a WebAuthn challenge slot keyed on the victim's id, an email-enumeration oracle (message and bcrypt timing), a declared-but-unenforcedpayouts:createscope, escrow legs validated by.min(10), payout addresses validated by length alone, a dispute guard that read a discarded error as "no dispute", and SSRF by hostname.Sweeps that silently stopped working
B-03,F-1.3-09,F-1.3-12,H-R-05were all.limit(N)with no.order()over sets that never drain. Past N rows the same N are processed forever and the rest never at all — no error, no log, the job reports success. Ordering alone would not fix it; the page has to advance. Now one sharedlib/db/keyset.tsrather than four copies.Queries against things that do not exist
B-04— filteredreputation_credentialsonsubject_didand ordered bycreated_at; it has neither. Returned 500 for every DID ever asked about.L7A-04— querieddid_identities, which does not exist, so the documented pre-transaction impersonation check answered "unverified" to every honest caller. It also tested arevokedcolumn; neither that column nor any revocation table exists, so the response now saysrevocation_checked: falseout loud rather than dropping the check silently.L5-02—deriveCardanoWalletreturnedaddr1_${pubkeyHex.slice(0,40)}..., ellipsis included. Production issued five of these to customers between February and July. Only generation was a stub; the spend path builds real transactions, so the rail was broken at exactly one end.NEW-L5-1— a chain constraint written in November allowed 11 chains while the code supports 18.Bugs the audit did not list, found while fixing it
signMessagesignssha256(message)while the server verifies over raw bytes — confirmedfalseagainst the repo's own noble build. Every proof it produced was rejected.wsESM problem the config's own alias already fixes. Whilesystem-wallet.test.tssat unrun, its ADA test asserted the shape of the broken address — soL5-02had a passing-looking test defending it. Restored: 317 → 323 files, 4468 → 4573 tests.Migrations
20260819170000(idempotency indexes),20260819180000(Stripe webhook events),20260819190000(collection chain constraint). All three applied to production and verified; the last was checked for violating rows first so it could be validated immediately.Not fixed, and why
WW-03is partial. The recipient is bound on every chain, but the amount is still unbound on BCH (bitcoinjs-lib does not handle its SIGHASH_FORKID variant) and Solana (the amount sits in program-specific instruction data). Refusing outright would take those chains offline, so the broadcast proceeds and the gap is logged and recorded on the row.F5-L1-06is partial.sweep-balances.mjswas truncated and had failednode --checksince December 2025 — the documented emergency fund-recovery procedure has never run. It now parses and the discovery half works.--executedeliberately refuses: broadcasting sweeps would be new, untested, multi-chain fund-moving code, and shipping that unexercised risks sending funds somewhere unrecoverable.hexToWIFis completed and verified against the canonical Bitcoin test vector.Breaking changes
/api/web-wallet/createnow requiresproof_of_ownership. Both in-repo SDKs send it; a published SDK older than this cannot create wallets. The browser extension registers via/importand is unaffected.statenor PKCE get a 400. 278 of 318 live authorization codes already carry PKCE, so this rejects the unprotected minority.🤖 Generated with Claude Code