Repository navigation
fix(security): remediate the audit's 29 medium and 15 low findings (GHSA-q642) - #260
Merged
Merged
Conversation
…m 20260815000000
Every claim was checked against production by assuming the anon role first.
That check found two things worth stating plainly.
CORRECTION. The column-level REVOKEs shipped in 20260815000000 for P-013 and
P-014 were NO-OPS. Supabase grants table-level SELECT to anon/authenticated,
and a table-level grant already permits every column, so revoking a column
subset subtracts nothing. Verified after the fact:
has_column_privilege('authenticated','escrows','release_token','SELECT') was
still true. Neither table has legitimate anon/authenticated access, so the
grant is now dropped outright rather than carved up.
P-029 was the only LIVE finding. reputation_receipts is readable by anon with
USING (true), and production holds 13,711 rows carrying amount (up to 999,999)
and escrow_tx — enumerable by anyone with the publishable key. Public
verifiability is the point of a receipt, so the row stays readable and the
financial columns do not: the table grant is replaced by an explicit column
grant. Consumers must now name columns; a bare SELECT * as anon fails.
The rest are LATENT, not live: this app authenticates with its own JWT and
talks to Postgres as service_role, so auth.uid() is never set and every policy
keyed on it denies. Confirmed empirically — as anon, businesses/payments/
invoices/stripe_webhook_secrets all return 0 rows, and cleanup_seen_signatures()
executes but deletes nothing because RLS blocks the underlying delete. They are
fixed anyway, since each is one policy edit away from becoming real:
V-018 businesses.api_key, P-016 stripe webhook secret — grant removed
P-002 did_reputation_events public SELECT — policy and grant removed
P-026 invoices INSERT constrained only user_id, leaving business_id free
P-027 payments INSERT accepted an arbitrary status (confirmed/forwarded)
N-021 'cancelled' allowed by the CHECK but never written or handled
P-028 calendar_events.user_id had no foreign key
P-025 escrow_series.merchant_id references businesses — documented, not
renamed, because live code reads it
P-023 21 functions were EXECUTE-able by anon; now service_role only
Verified after applying: every privilege assertion passes, anon still reads the
allowed receipt columns, and the live reputation and landing endpoints answer.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…l flows P-021 PayPal capture accepted 'COMPLETED' as proof of payment without ever reading the captured amount or currency — both of which the client already returns. A capture for a token amount, or in a different currency, marked a full-value invoice paid. Now verified against the invoice, and the paid write is conditional so two captures cannot both record a settlement. V-008 An invoice flipped to 'paid' when its linked payment was in forwarding_failed — money confirmed at the intermediary address but never delivered. The merchant saw a settled invoice and no reason to look. That status is no longer treated as settling; the retry queue and rescanLateDeposits re-drive the forward, so it resolves rather than sticking. V-010 Invoice numbers were allocated with an unlocked read-then-write MAX+1, so concurrent creates silently produced duplicates. The application cannot make that atomic, so the guarantee is a UNIQUE (business_id, invoice_number) constraint with a retry around the insert. The lookup also ordered by created_at, which returns the most recently created number rather than the highest — wrong as soon as an invoice is deleted or backdated. P-022 Accepting a proposal is what binds the payee address and fee, and the write was unconditional; it is now a compare-and-swap on the observed status. N-015 A failed escrow fee forward was caught, logged and forgotten: the escrow was marked settled with fee_tx_hash NULL and nothing recorded that the platform was still owed. It now writes a fee_forward_failed escrow_events row. N-016 The escrow fee is bounded at creation, so an out-of-range rate cannot store a fee that exceeds the escrow and make the beneficiary leg negative. N-013 The payment window had only an upper bound, so expires_in_minutes could be set below a second — every payment against such a quote lands late and strands. Floor added. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… client IP V-028 getClientIp read the LEFTMOST X-Forwarded-For entry. XFF is appended to by each proxy, so a request arriving with "X-Forwarded-For: 1.2.3.4" leaves the edge as "1.2.3.4, <real ip>" — the leftmost value is the one the client chose. Every per-IP rate limit was therefore bypassable by rotating a header. Parsing now counts in from the RIGHT (TRUSTED_PROXY_HOPS, default 1), and the vendor headers CF-Connecting-IP / X-Real-IP are only trusted when explicitly enabled, since they are equally forgeable if the origin is reachable directly. Seven tests cover it, including that prepending a forged address cannot move the rate-limit bucket. V-026 Web-wallet signature replay protection used the synchronous checker, which consults only this process's in-memory Map and pushes to Supabase fire-and-forget. Across more than one instance that is not replay protection: a signature spent on instance A is unknown to instance B. Now awaits the shared store. V-027 jwt.refreshToken() decoded WITHOUT verifying, then re-signed the payload. A token this side never issued would have been accepted and minted into a valid one. It is unused today, which is why the audit rated it low, but it is a loaded gun in a library. Now verifies the signature while still allowing an expired token, which is the actual intent. V-021 The Web Bot Auth signature lifetime cap only applied when both created and expires were present, so omitting expires skipped it — the most permissive case had no check at all. Both are now required. V-020 The CLI device-authorization start endpoint is unauthenticated by design and had no rate limit, so one caller could mint unlimited pending device codes and raise the collision odds against a code a real user is being shown. V-009 p2p/request had no upper bound on amount_usd and no rate limit, on an endpoint that provisions merchant accounts and client records. P-014 (completion) The issuer API-key lookup still queried the raw api_key column, so the hash column restored by 20260816020000 was unused. Lookups now match the hash and lazily upgrade any row still found by its raw key, so the column drains without forcing a key rotation. V-004 x402 raw_proof stored the payer's full signed authorization verbatim in a column nothing ever reads. Secret-bearing fields are now hashed, keeping the proof shape for forensics without retaining anything reusable. V-014 rescanLateDeposits treated "funds still at the address" as proof no forward succeeded. For a payment already in forwarding with a recorded transaction hash that is false — the send may simply not be mined yet — so a known broadcast now gets a six-hour grace before it can be re-driven. V-002 was already fixed by enforcePriceBinding (commit 5717556); no change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
P-018 The LNbits deploy workflow read its SSH private key and dotenv credentials as "secrets.X || vars.X". GitHub Actions VARIABLES are not secrets: plaintext, readable by anyone with repo access, and not masked in job logs. Had either been set as a variable, the SSH key would have been printed in the clear. The fallbacks are gone; a missing secret now fails the job. N-020 That same workflow ran on every push to master with SSH access to a production droplet, so any merged commit reached production infrastructure with nobody in the loop. It is workflow_dispatch only and pinned to the "production" environment, so a required-reviewer rule applies. NOTE: that rule must be enabled in Settings -> Environments -> production; the environment key here is what makes it apply. Its actions are SHA-pinned too — it was the one workflow missed in #257, and it is the highest-value target in the repo. P-017 gen-mnemonic.mjs fetched the BIP-39 wordlist from GitHub at master on every run, unverified. Two problems, and the second is the serious one: nothing checked what came back, and it was a DIFFERENT source from the one used to DERIVE addresses (@scure/bip39). Any divergence would emit a phrase deriving to different keys than the platform expects, making funds sent there unrecoverable. Now uses the local lockfile-pinned wordlist with a canonical SHA-256 assertion. Verified: generated phrases pass the same validateMnemonic the wallet code uses. P-024 send-announcement.ts had six real merchant names and email addresses hardcoded, plus two more in a skip list — production PII committed to the repository. Recipients are read from the database at run time, and the skip list is domain patterns rather than named individuals. The file carries a note that this does not remove them from git history; treat those addresses as disclosed. P-003 The installer piped curl straight into tar, so a truncated transfer extracted whatever arrived. It now downloads to a file, verifies it is a well-formed non-empty gzip, logs the archive digest, supports pinning via COINPAY_SHA256, and constrains COINPAY_REF, which was interpolated into the download URL unchecked. P-019 In the SDK webhook verifier, parseInt('abc', 10) is NaN and every comparison against NaN is false — so the timestamp freshness check silently PASSED for any non-numeric timestamp. A security check that no-ops on malformed input is worse than none, because it reads as one. The signature header parser also split on every '=', truncating any value containing one. N-019 Every allowBuilds entry was literally "set this to true or false", which pnpm treats as unset, so none of those packages ran their install scripts and nobody had made the decision. Each is now resolved explicitly, with native builds enabled and telemetry-only postinstalls declined. N-012 The optional-dependency loader built a "new Function" evaluator taking a free-form module specifier. Every caller passes a constant, but the shape is worth not having in a payments codebase; it is now an allowlist. N-023 The wallet SDK logged every address the wallet holds on an error path triggered by an ordinary user mistake, correlating a user's whole account set into any shared console. Logs the count instead. N-014 Payment creation had no idempotency, so a client retrying after a timeout got a second payment — another HD address, another unit of quota, another charge. It now honours an Idempotency-Key, backed by a partial unique index so concurrent retries cannot both insert. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Broadcast checked that tx_id belonged to the wallet, that it was still pending and that it had not expired — but never looked at the signed_tx bytes. A caller could prepare one transfer and then broadcast a COMPLETELY DIFFERENT signed transaction while quoting the legitimate tx_id. The user signs with their own key, so this is not a theft primitive against them. It matters because everything the platform records and enforces hangs off the prepared row: the wallet's history, per-transaction limits, fee accounting and notifications would all describe a transaction that never happened, while the one that did goes unrecorded. EVM transactions are self-describing, so the recipient is decoded and compared (handling ERC-20 transfers, where the recipient is in the calldata rather than the tx `to`). A decoded mismatch is refused outright. UTXO and Solana need chain-specific parsing that is not available here, so rather than pretend those are checked, the row records binding_verified plus the reason — downstream accounting can then tell what it may rely on. Also annotates the unreachable 'detected' payment status (N-022). It was removed from the payments.status CHECK constraint without the application being updated, so the database can no longer produce it, yet roughly two dozen UI branches still render a "Payment Detected!" stage no payment can reach. The canonical type definitions now say so. The branches themselves are left in place: removing them is a mechanical sweep across payer-facing pages and does not belong in a security change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ThreatCrush Security Scan327 finding(s) HIGH/CRITICAL: 35 | MEDIUM: 38 | LOW: 254
…and 277 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
ralyodio
added a commit
that referenced
this pull request
Aug 16, 2026
#260 changed this workflow to workflow_dispatch-only under a "production" environment, to satisfy audit finding N-020 (SSH deploy without an approval gate). That removed continuous deployment, which is behaviour the maintainer relies on. Reverted at their request. Continuous deployment here is a deliberate trade-off and it is the maintainer's to make, not the audit's. The workflow comment records that, and records how to put the gate back in two lines if that ever changes. What is NOT reverted, because it was a separate finding and was not asked for: P-018 — ENV_FILE and DIRECT_SSH_KEY still read from `secrets` only. They used to fall back to `vars.*`, and GitHub Actions variables are plaintext, readable by anyone with repo access, and NOT masked in job logs; an SSH private key supplied that way would be printed in the clear. Action pinning — actions/checkout stays pinned to its commit SHA. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
ralyodio
added a commit
that referenced
this pull request
Aug 16, 2026
…-role fallback, mnemonic temp files, replay fail-open) (#262) * revert(ci): restore auto-deploy for the LNbits droplet #260 changed this workflow to workflow_dispatch-only under a "production" environment, to satisfy audit finding N-020 (SSH deploy without an approval gate). That removed continuous deployment, which is behaviour the maintainer relies on. Reverted at their request. Continuous deployment here is a deliberate trade-off and it is the maintainer's to make, not the audit's. The workflow comment records that, and records how to put the gate back in two lines if that ever changes. What is NOT reverted, because it was a separate finding and was not asked for: P-018 — ENV_FILE and DIRECT_SSH_KEY still read from `secrets` only. They used to fall back to `vars.*`, and GitHub Actions variables are plaintext, readable by anyone with repo access, and NOT masked in job logs; an SSH private key supplied that way would be printed in the clear. Action pinning — actions/checkout stays pinned to its commit SHA. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(security): remediate the three newly-reported advisories GHSA-j73w-r4j5-3wx9 (critical) — hardcoded service-role key fallback. The literal 'service-role-key' string the report cites is already gone from the two files it names, but the class of bug is alive in 16 others, which fall back to the ANON key or an empty string. Both are worse than they look: - Falling back to the anon key does not fail, it silently DOWNGRADES. The route keeps serving while RLS starts applying to a code path written on the assumption that it does not. Reads come back empty, writes are refused, and the symptom appears far from the cause. - Falling back to a literal is a hardcoded credential in the sense that matters: the deployment believes it is authenticated and is not. All 16 now go through createServiceClient(), which refuses to build a client without a real URL and service-role key. Zero fallbacks remain. GHSA-cqrc-mhqx-48mr (high) — wallet mnemonic in predictable temp files. The CLI wrote the mnemonic and the password to /tmp/coinpay-wallet-<Date.now()> and /tmp/coinpay-pass-<Date.now()> before shelling out to gpg. The names are a guess away, mode 0600 stops other OS users but not anything running as the same user, and overwrite-then-unlink does not reliably erase on journalling or copy-on-write filesystems. The mnemonic derives every key on every chain. Both files are gone. gpg now takes the passphrase on fd 3 and the plaintext on stdin, so neither touches disk and the passphrase never appears in the process arguments where ps would show it. Verified end to end: round-trip recovers the mnemonic, the file on disk is real ciphertext, a wrong password is rejected, and a filesystem watcher polling every 2ms during the operation observed ZERO temp files. GHSA-c9jw-9f79-j657 (high) — optional auth nonce enables replay. The report recommends making the nonce mandatory. That would 401 every installed browser extension: packages/extension sends a 3-part header with no nonce. So the extension now sends one (in both the signed message and the header — an unsigned nonce would let an attacker mint fresh replay-store keys for a captured signature), while the server keeps accepting both formats. More importantly, the nonce was not the actual hole. Replay protection is the shared signature store, and it FAILED OPEN: on any Supabase error the check returned null and fell through to a per-process in-memory Map. On a multi-instance deployment a signature spent on instance A is unknown to instance B, so protection evaporated exactly when the store was unhealthy — which is when an attacker is most likely probing. It now rejects when the store is configured but unreachable, keeping the in-memory path only for deployments with no shared store at all. Four tests cover the state machine. Tests: 4049 passing (up from 4045). The global test setup now provides Supabase env vars, since routes legitimately refuse to run without them. 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
…HSA-q642) (#260) * fix(db): medium/low audit findings, and correct the no-op REVOKEs from 20260815000000 Every claim was checked against production by assuming the anon role first. That check found two things worth stating plainly. CORRECTION. The column-level REVOKEs shipped in 20260815000000 for P-013 and P-014 were NO-OPS. Supabase grants table-level SELECT to anon/authenticated, and a table-level grant already permits every column, so revoking a column subset subtracts nothing. Verified after the fact: has_column_privilege('authenticated','escrows','release_token','SELECT') was still true. Neither table has legitimate anon/authenticated access, so the grant is now dropped outright rather than carved up. P-029 was the only LIVE finding. reputation_receipts is readable by anon with USING (true), and production holds 13,711 rows carrying amount (up to 999,999) and escrow_tx — enumerable by anyone with the publishable key. Public verifiability is the point of a receipt, so the row stays readable and the financial columns do not: the table grant is replaced by an explicit column grant. Consumers must now name columns; a bare SELECT * as anon fails. The rest are LATENT, not live: this app authenticates with its own JWT and talks to Postgres as service_role, so auth.uid() is never set and every policy keyed on it denies. Confirmed empirically — as anon, businesses/payments/ invoices/stripe_webhook_secrets all return 0 rows, and cleanup_seen_signatures() executes but deletes nothing because RLS blocks the underlying delete. They are fixed anyway, since each is one policy edit away from becoming real: V-018 businesses.api_key, P-016 stripe webhook secret — grant removed P-002 did_reputation_events public SELECT — policy and grant removed P-026 invoices INSERT constrained only user_id, leaving business_id free P-027 payments INSERT accepted an arbitrary status (confirmed/forwarded) N-021 'cancelled' allowed by the CHECK but never written or handled P-028 calendar_events.user_id had no foreign key P-025 escrow_series.merchant_id references businesses — documented, not renamed, because live code reads it P-023 21 functions were EXECUTE-able by anon; now service_role only Verified after applying: every privilege assertion passes, anon still reads the allowed receipt columns, and the live reputation and landing endpoints answer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(payments): audit medium/low findings in invoice, escrow and PayPal flows P-021 PayPal capture accepted 'COMPLETED' as proof of payment without ever reading the captured amount or currency — both of which the client already returns. A capture for a token amount, or in a different currency, marked a full-value invoice paid. Now verified against the invoice, and the paid write is conditional so two captures cannot both record a settlement. V-008 An invoice flipped to 'paid' when its linked payment was in forwarding_failed — money confirmed at the intermediary address but never delivered. The merchant saw a settled invoice and no reason to look. That status is no longer treated as settling; the retry queue and rescanLateDeposits re-drive the forward, so it resolves rather than sticking. V-010 Invoice numbers were allocated with an unlocked read-then-write MAX+1, so concurrent creates silently produced duplicates. The application cannot make that atomic, so the guarantee is a UNIQUE (business_id, invoice_number) constraint with a retry around the insert. The lookup also ordered by created_at, which returns the most recently created number rather than the highest — wrong as soon as an invoice is deleted or backdated. P-022 Accepting a proposal is what binds the payee address and fee, and the write was unconditional; it is now a compare-and-swap on the observed status. N-015 A failed escrow fee forward was caught, logged and forgotten: the escrow was marked settled with fee_tx_hash NULL and nothing recorded that the platform was still owed. It now writes a fee_forward_failed escrow_events row. N-016 The escrow fee is bounded at creation, so an out-of-range rate cannot store a fee that exceeds the escrow and make the beneficiary leg negative. N-013 The payment window had only an upper bound, so expires_in_minutes could be set below a second — every payment against such a quote lands late and strands. Floor added. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(auth): audit medium/low findings in tokens, replay protection and client IP V-028 getClientIp read the LEFTMOST X-Forwarded-For entry. XFF is appended to by each proxy, so a request arriving with "X-Forwarded-For: 1.2.3.4" leaves the edge as "1.2.3.4, <real ip>" — the leftmost value is the one the client chose. Every per-IP rate limit was therefore bypassable by rotating a header. Parsing now counts in from the RIGHT (TRUSTED_PROXY_HOPS, default 1), and the vendor headers CF-Connecting-IP / X-Real-IP are only trusted when explicitly enabled, since they are equally forgeable if the origin is reachable directly. Seven tests cover it, including that prepending a forged address cannot move the rate-limit bucket. V-026 Web-wallet signature replay protection used the synchronous checker, which consults only this process's in-memory Map and pushes to Supabase fire-and-forget. Across more than one instance that is not replay protection: a signature spent on instance A is unknown to instance B. Now awaits the shared store. V-027 jwt.refreshToken() decoded WITHOUT verifying, then re-signed the payload. A token this side never issued would have been accepted and minted into a valid one. It is unused today, which is why the audit rated it low, but it is a loaded gun in a library. Now verifies the signature while still allowing an expired token, which is the actual intent. V-021 The Web Bot Auth signature lifetime cap only applied when both created and expires were present, so omitting expires skipped it — the most permissive case had no check at all. Both are now required. V-020 The CLI device-authorization start endpoint is unauthenticated by design and had no rate limit, so one caller could mint unlimited pending device codes and raise the collision odds against a code a real user is being shown. V-009 p2p/request had no upper bound on amount_usd and no rate limit, on an endpoint that provisions merchant accounts and client records. P-014 (completion) The issuer API-key lookup still queried the raw api_key column, so the hash column restored by 20260816020000 was unused. Lookups now match the hash and lazily upgrade any row still found by its raw key, so the column drains without forcing a key rotation. V-004 x402 raw_proof stored the payer's full signed authorization verbatim in a column nothing ever reads. Secret-bearing fields are now hashed, keeping the proof shape for forensics without retaining anything reusable. V-014 rescanLateDeposits treated "funds still at the address" as proof no forward succeeded. For a payment already in forwarding with a recorded transaction hash that is false — the send may simply not be mined yet — so a known broadcast now gets a six-hour grace before it can be re-driven. V-002 was already fixed by enforcePriceBinding (commit 0f0f300); no change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(infra): audit medium/low findings in CI, installer, scripts and SDK P-018 The LNbits deploy workflow read its SSH private key and dotenv credentials as "secrets.X || vars.X". GitHub Actions VARIABLES are not secrets: plaintext, readable by anyone with repo access, and not masked in job logs. Had either been set as a variable, the SSH key would have been printed in the clear. The fallbacks are gone; a missing secret now fails the job. N-020 That same workflow ran on every push to master with SSH access to a production droplet, so any merged commit reached production infrastructure with nobody in the loop. It is workflow_dispatch only and pinned to the "production" environment, so a required-reviewer rule applies. NOTE: that rule must be enabled in Settings -> Environments -> production; the environment key here is what makes it apply. Its actions are SHA-pinned too — it was the one workflow missed in #257, and it is the highest-value target in the repo. P-017 gen-mnemonic.mjs fetched the BIP-39 wordlist from GitHub at master on every run, unverified. Two problems, and the second is the serious one: nothing checked what came back, and it was a DIFFERENT source from the one used to DERIVE addresses (@scure/bip39). Any divergence would emit a phrase deriving to different keys than the platform expects, making funds sent there unrecoverable. Now uses the local lockfile-pinned wordlist with a canonical SHA-256 assertion. Verified: generated phrases pass the same validateMnemonic the wallet code uses. P-024 send-announcement.ts had six real merchant names and email addresses hardcoded, plus two more in a skip list — production PII committed to the repository. Recipients are read from the database at run time, and the skip list is domain patterns rather than named individuals. The file carries a note that this does not remove them from git history; treat those addresses as disclosed. P-003 The installer piped curl straight into tar, so a truncated transfer extracted whatever arrived. It now downloads to a file, verifies it is a well-formed non-empty gzip, logs the archive digest, supports pinning via COINPAY_SHA256, and constrains COINPAY_REF, which was interpolated into the download URL unchecked. P-019 In the SDK webhook verifier, parseInt('abc', 10) is NaN and every comparison against NaN is false — so the timestamp freshness check silently PASSED for any non-numeric timestamp. A security check that no-ops on malformed input is worse than none, because it reads as one. The signature header parser also split on every '=', truncating any value containing one. N-019 Every allowBuilds entry was literally "set this to true or false", which pnpm treats as unset, so none of those packages ran their install scripts and nobody had made the decision. Each is now resolved explicitly, with native builds enabled and telemetry-only postinstalls declined. N-012 The optional-dependency loader built a "new Function" evaluator taking a free-form module specifier. Every caller passes a constant, but the shape is worth not having in a payments codebase; it is now an allowlist. N-023 The wallet SDK logged every address the wallet holds on an error path triggered by an ordinary user mistake, correlating a user's whole account set into any shared console. Logs the count instead. N-014 Payment creation had no idempotency, so a client retrying after a timeout got a second payment — another HD address, another unit of quota, another charge. It now honours an Idempotency-Key, backed by a partial unique index so concurrent retries cannot both insert. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(web-wallet): bind broadcast to the prepared transaction (V-024) Broadcast checked that tx_id belonged to the wallet, that it was still pending and that it had not expired — but never looked at the signed_tx bytes. A caller could prepare one transfer and then broadcast a COMPLETELY DIFFERENT signed transaction while quoting the legitimate tx_id. The user signs with their own key, so this is not a theft primitive against them. It matters because everything the platform records and enforces hangs off the prepared row: the wallet's history, per-transaction limits, fee accounting and notifications would all describe a transaction that never happened, while the one that did goes unrecorded. EVM transactions are self-describing, so the recipient is decoded and compared (handling ERC-20 transfers, where the recipient is in the calldata rather than the tx `to`). A decoded mismatch is refused outright. UTXO and Solana need chain-specific parsing that is not available here, so rather than pretend those are checked, the row records binding_verified plus the reason — downstream accounting can then tell what it may rely on. Also annotates the unreachable 'detected' payment status (N-022). It was removed from the payments.status CHECK constraint without the application being updated, so the database can no longer produce it, yet roughly two dozen UI branches still render a "Payment Detected!" stage no payment can reach. The canonical type definitions now say so. The branches themselves are left in place: removing them is a mechanical sweep across payer-facing pages and does not belong in a security change. 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
#260 changed this workflow to workflow_dispatch-only under a "production" environment, to satisfy audit finding N-020 (SSH deploy without an approval gate). That removed continuous deployment, which is behaviour the maintainer relies on. Reverted at their request. Continuous deployment here is a deliberate trade-off and it is the maintainer's to make, not the audit's. The workflow comment records that, and records how to put the gate back in two lines if that ever changes. What is NOT reverted, because it was a separate finding and was not asked for: P-018 — ENV_FILE and DIRECT_SSH_KEY still read from `secrets` only. They used to fall back to `vars.*`, and GitHub Actions variables are plaintext, readable by anyone with repo access, and NOT masked in job logs; an SSH private key supplied that way would be printed in the clear. Action pinning — actions/checkout stays pinned to its commit SHA. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
ralyodio
added a commit
that referenced
this pull request
Aug 19, 2026
…-role fallback, mnemonic temp files, replay fail-open) (#262) * revert(ci): restore auto-deploy for the LNbits droplet #260 changed this workflow to workflow_dispatch-only under a "production" environment, to satisfy audit finding N-020 (SSH deploy without an approval gate). That removed continuous deployment, which is behaviour the maintainer relies on. Reverted at their request. Continuous deployment here is a deliberate trade-off and it is the maintainer's to make, not the audit's. The workflow comment records that, and records how to put the gate back in two lines if that ever changes. What is NOT reverted, because it was a separate finding and was not asked for: P-018 — ENV_FILE and DIRECT_SSH_KEY still read from `secrets` only. They used to fall back to `vars.*`, and GitHub Actions variables are plaintext, readable by anyone with repo access, and NOT masked in job logs; an SSH private key supplied that way would be printed in the clear. Action pinning — actions/checkout stays pinned to its commit SHA. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(security): remediate the three newly-reported advisories GHSA-j73w-r4j5-3wx9 (critical) — hardcoded service-role key fallback. The literal 'service-role-key' string the report cites is already gone from the two files it names, but the class of bug is alive in 16 others, which fall back to the ANON key or an empty string. Both are worse than they look: - Falling back to the anon key does not fail, it silently DOWNGRADES. The route keeps serving while RLS starts applying to a code path written on the assumption that it does not. Reads come back empty, writes are refused, and the symptom appears far from the cause. - Falling back to a literal is a hardcoded credential in the sense that matters: the deployment believes it is authenticated and is not. All 16 now go through createServiceClient(), which refuses to build a client without a real URL and service-role key. Zero fallbacks remain. GHSA-cqrc-mhqx-48mr (high) — wallet mnemonic in predictable temp files. The CLI wrote the mnemonic and the password to /tmp/coinpay-wallet-<Date.now()> and /tmp/coinpay-pass-<Date.now()> before shelling out to gpg. The names are a guess away, mode 0600 stops other OS users but not anything running as the same user, and overwrite-then-unlink does not reliably erase on journalling or copy-on-write filesystems. The mnemonic derives every key on every chain. Both files are gone. gpg now takes the passphrase on fd 3 and the plaintext on stdin, so neither touches disk and the passphrase never appears in the process arguments where ps would show it. Verified end to end: round-trip recovers the mnemonic, the file on disk is real ciphertext, a wrong password is rejected, and a filesystem watcher polling every 2ms during the operation observed ZERO temp files. GHSA-c9jw-9f79-j657 (high) — optional auth nonce enables replay. The report recommends making the nonce mandatory. That would 401 every installed browser extension: packages/extension sends a 3-part header with no nonce. So the extension now sends one (in both the signed message and the header — an unsigned nonce would let an attacker mint fresh replay-store keys for a captured signature), while the server keeps accepting both formats. More importantly, the nonce was not the actual hole. Replay protection is the shared signature store, and it FAILED OPEN: on any Supabase error the check returned null and fell through to a per-process in-memory Map. On a multi-instance deployment a signature spent on instance A is unknown to instance B, so protection evaporated exactly when the store was unhealthy — which is when an attacker is most likely probing. It now rejects when the store is configured but unreachable, keeping the in-memory path only for deployments with no shared store at all. Four tests cover the state machine. Tests: 4049 passing (up from 4045). The global test setup now provides Supabase env vars, since routes legitimately refuse to run without them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Closes out GHSA-q642-mvvc-5w7x — the umbrella "86 confirmed" remediation list. Its 3 critical + 39 high were fixed in #257; this covers the 29 medium + 15 low that were enumerated in that document but never filed as individual advisories.
Verification
pnpm type-checkcleanpnpm buildsucceedsvitest run— 4045 passing, 3 skipped, 0 failingEvery database claim below was checked against production before writing the fix, by assuming the
anonrole and reading. That mattered more than expected — see the next section.The two things worth reading
1. A correction to my own earlier work. The column-level
REVOKEs shipped in #257 for P-013/P-014 were no-ops. Supabase grants table-levelSELECT, and a table-level grant already permits every column — revoking a column subset subtracts nothing. Verified after the fact:has_column_privilege('authenticated','escrows','release_token','SELECT')still returnedtrue. Those controls did not exist. They do now, by dropping the table grant outright rather than trying to carve columns out of it.2. Only one of these findings was actually live.
reputation_receiptsis readable byanonwithUSING (true), and production holds 13,711 rows carryingamount(up to 999,999) andescrow_tx— enumerable by anyone with the publishable key. Public verifiability is the point of a receipt, so the row stays readable and the financial columns do not.Everything else is latent: this app authenticates with its own JWT and talks to Postgres as
service_role, soauth.uid()is never set and every policy keyed on it denies. Confirmed empirically — asanon,businesses/payments/invoices/stripe_webhook_secretsall return 0 rows, andcleanup_seen_signatures()executes but deletes nothing because RLS blocks the write. Fixed anyway, since each is one policy edit from becoming real.Highlights
Client IP (V-028) —
getClientIpread the leftmostX-Forwarded-Forentry. XFF is appended to by each proxy, so a request arriving withX-Forwarded-For: 1.2.3.4leaves the edge as1.2.3.4, <real ip>: the leftmost value is the one the client chose. Every per-IP rate limit was bypassable by rotating a header. Now counts in from the right; 7 tests cover it.Replay across instances (V-026) — signature replay protection used the in-memory checker with a fire-and-forget DB sync. Across more than one instance that is not replay protection: a signature spent on instance A is unknown to instance B.
Token minting (V-027) —
jwt.refreshToken()decoded without verifying, then re-signed the payload. A token this side never issued would have been minted into a valid one. Unused today, which is why it's rated low, but it's a loaded gun in a library.PII in git (P-024) —
send-announcement.tshad six real merchant names and emails hardcoded. Now read from the database. They remain in git history and should be treated as disclosed.Wallet generation (P-017) —
gen-mnemonic.mjsfetched the BIP-39 wordlist from GitHub atmaster, unverified, and from a different source than the derivation code uses. Any divergence would emit a phrase deriving to different keys than the platform expects, making funds sent there unrecoverable. Now uses the pinned local wordlist with a canonical checksum; generated phrases verified against the samevalidateMnemonicthe wallet code uses.Deploy secrets (P-018) — the LNbits workflow read its SSH key as
secrets.X || vars.X. Actions variables are plaintext and not masked in logs. Also nowworkflow_dispatchonly under aproductionenvironment (the reviewer rule needs enabling in Settings → Environments).Others: PayPal capture verifying amount and currency (P-021); invoices no longer marked paid while the forward failed (V-008); invoice-number uniqueness with retry (V-010); proposal accept CAS (P-022); escrow fee-forward failures recorded rather than swallowed (N-015); payment idempotency keys (N-014); SDK timestamp
NaNfail-open (P-019);allowBuildsplaceholders resolved (N-019); installer integrity (P-003); broadcast bound to the prepared transaction (V-024).Deliberately not done
detectedis unreachable (removed from the CHECK constraint) but ~26 UI branches still render it. Annotated at the canonical types; removing the branches is a mechanical sweep across payer-facing pages that doesn't belong in a security change.enforcePriceBinding(commit 5717556) before this work.Migrations
Two, both already applied to production and verified:
20260816020000_audit_medium_low_database.sqland the invoice-uniqueness/fee-event follow-up. Idempotent.🤖 Generated with Claude Code