From d8a361bdcfaa8cc5b39d9675e74b0e1d2e42be2c Mon Sep 17 00:00:00 2001 From: Adam Soltys Date: Sun, 13 Sep 2026 20:44:48 -0700 Subject: security: drop the admin endpoint surface and land pending hardening Removes the admin-gated endpoints and the account flags they set, plus a batch of hardening that had accumulated uncommitted in the tree. These were deployed and verified together, so they land as one change. admin surface: - drop POST /hidepay, POST /unlimit and GET /users, their handlers, and the now-unused admin preValidation export - `unlimited` was only ever written, never read; `hidepay` only appeared in output projections. Both flags go, along with their whitelist, public-fields and type entries. request log: - redactBody now recurses. It only masked top-level keys, so a body that nests credentials one level down wrote them to the log in plaintext, and pin fields were not on the list at all. payments: - sendOnchain/build gate `aid` through requireAccount, so a caller can only spend from an account it owns - sendOnchain signs only a tx we composed, matched against a short-lived per-account build receipt - raise xpay retry_for to 60s; route-finding preferred a drained zero-fee hop and burned the whole budget on it. check()'s reversal window keeps its 2x margin. ecash / invoices / webhooks: - resolve a token's mint through the IP-pinning fetcher instead of plain fetch, closing an unauthenticated SSRF path into the internal network - reject non-positive amounts before the balance write - /sign requires the caller to own the address, rather than signing under any address the hot wallet controls - webhook delivery no longer disables TLS verification; the body carries the merchant's shared secret registration: - refuse a username the v3 registrar holds unless the caller's own nostr key is the holder (lib/names.ts) liquid: - honour the deposit kill switch at invoice generation and at credit elements image: - pin 23.3.4 and verify the release tarball checksum Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0112TGi9GGNWqji1XQ5jZRzc --- lib/app.ts | 37 ++++++++++++++++++++++++++------- lib/auth.ts | 24 ++++++++++++---------- lib/ecash.ts | 13 ++++++++++-- lib/invoices.ts | 2 ++ lib/names.ts | 37 +++++++++++++++++++++++++++++++++ lib/nwc.ts | 25 ++++++++++++++++++++--- lib/payments.ts | 62 +++++++++++++++++++++++++++++++++++++++++++++++++------- lib/register.ts | 3 +++ lib/types.ts | 1 - lib/utils.ts | 1 - lib/webhooks.ts | 8 +++++++- lib/whitelist.ts | 1 - 12 files changed, 180 insertions(+), 34 deletions(-) create mode 100644 lib/names.ts (limited to 'lib') diff --git a/lib/app.ts b/lib/app.ts index d2949e61..65607d24 100644 --- a/lib/app.ts +++ b/lib/app.ts @@ -22,14 +22,37 @@ const app = fastify({ const reqLogger = pino(pino.destination("req")); const resLogger = pino(pino.destination("res")); -// Never log plaintext passwords. Redact password fields from request bodies -// before they reach the request log. -const REDACT_FIELDS = ["password", "confirm", "secret", "otpsecret"]; -const redactBody = (body: any) => { - if (!body || typeof body !== "object") return body; +// Never log plaintext credentials. This MUST recurse: the previous version +// only redacted top-level keys, but POST /register posts {user:{password}} — +// one level down — so every registration wrote its plaintext password to the +// request log, and `pin`/`newpin` were not on the list at all (POST /pin, +// POST /user and POST /take all carry one). Anything nested in an array is +// covered too. Depth is bounded so a pathological body can't spin here. +const REDACT_FIELDS = new Set([ + "password", + "newpassword", + "confirm", + "pin", + "newpin", + "secret", + "otpsecret", + "nsec", + "seed", + "privkey", + "mnemonic", +]); +const redactBody = (body: any, depth = 0): any => { + if (!body || typeof body !== "object" || depth > 6) return body; + if (Array.isArray(body)) return body.map((v) => redactBody(v, depth + 1)); const copy: any = { ...body }; - for (const field of REDACT_FIELDS) - if (field in copy) copy[field] = "[redacted]"; + for (const k of Object.keys(copy)) { + if (REDACT_FIELDS.has(k)) { + if (copy[k] !== undefined && copy[k] !== null && copy[k] !== "") + copy[k] = "[redacted]"; + } else if (copy[k] && typeof copy[k] === "object") { + copy[k] = redactBody(copy[k], depth + 1); + } + } return copy; }; diff --git a/lib/auth.ts b/lib/auth.ts index b299715b..915dcdc8 100644 --- a/lib/auth.ts +++ b/lib/auth.ts @@ -4,17 +4,6 @@ import { fail, getUser } from "$lib/utils"; import fastifyPassport from "@fastify/passport"; import jwt from "passport-jwt"; -export const admin = { - preValidation: fastifyPassport.authenticate( - "jwt", - { session: false }, - (req, res, err, user, info) => { - if (!user.admin) return res.code(401).send("unauthorized"); - req.user = user; - }, - ), -}; - export const optional = { preValidation: fastifyPassport.authenticate( "jwt", @@ -36,6 +25,19 @@ export const requirePin = async ({ body, user }) => { if (!user || (user.pin && user.pin !== body.pin)) fail("Invalid pin"); }; +// A caller-supplied `aid` decides which account's node wallet composes and +// signs a withdrawal and whose balance gets debited, so it has to be an +// account the caller actually owns. `${uid}:accounts` is the ownership list — +// the same one GET /account/:id and payments.list already check — and a user's +// own id is in it. Omitted `aid` means "my own account". +export const requireAccount = async ({ aid, user }) => { + if (!user) fail("unauthorized"); + if (!aid || aid === user.id) return user.id; + if ((await db.lPos(`${user.id}:accounts`, aid)) === null) + fail("unauthorized"); + return aid; +}; + export const jwtStrategy = new jwt.Strategy( { jwtFromRequest: jwt.ExtractJwt.fromExtractors([ diff --git a/lib/ecash.ts b/lib/ecash.ts index c304a8b8..9ce8b4e5 100644 --- a/lib/ecash.ts +++ b/lib/ecash.ts @@ -1,6 +1,7 @@ import config from "$config"; import { g, s } from "$lib/db"; import { lnb } from "$lib/ln"; +import { safeGot } from "$lib/safe-fetch"; import { fail, wait } from "$lib/utils"; import { CashuMint, @@ -46,9 +47,17 @@ const withCashLock = async (fn: () => Promise): Promise => { } }; +// `mint` comes straight out of an attacker-supplied cashu token, and this is +// the first thing we do with it — so fetching it with cashu-ts's plain fetch +// made every /cash and /ecash entry point an unauthenticated SSRF probe into +// the internal network. Use the same IP-pinning fetcher lnurl resolution uses +// (it rejects loopback/private/link-local/CGNAT/metadata targets and closes the +// DNS-rebinding window by pinning the socket to the IP it validated). NUT-06 +// /v1/info is exactly what CashuMint.getInfo() would have called. const ext = async (mint) => { - const issuer = new CashuMint(mint); - const { pubkey: issuerPk } = await issuer.getInfo(); + const { pubkey: issuerPk } = await safeGot( + `${String(mint).replace(/\/+$/, "")}/v1/info`, + ); const { pubkey: ourPk } = await m.getInfo(); return issuerPk !== ourPk; }; diff --git a/lib/invoices.ts b/lib/invoices.ts index 9661bbc7..ba55b77c 100644 --- a/lib/invoices.ts +++ b/lib/invoices.ts @@ -204,6 +204,8 @@ export const generate = async ({ invoice, user }) => { hash = await bc.getNewAddress({ address_type }); text = bip21(hash, invoice); } else if (type === PaymentType.liquid) { + if (await g("liquid:deposits:disabled")) + fail("Liquid deposits are temporarily disabled"); address_type ||= "blech32"; hash = await lq.getNewAddress({ address_type }); text = bip21(hash, invoice); diff --git a/lib/names.ts b/lib/names.ts new file mode 100644 index 00000000..284c595d --- /dev/null +++ b/lib/names.ts @@ -0,0 +1,37 @@ +import { warn } from "$lib/logging"; +import { fail } from "$lib/utils"; + +// The v3 registrar (coinos v3 / halwallet) owns any name it has a record for: +// routes/lnurl.ts serves a claimed name's lightning address from the registrar +// ahead of the local account. So a local account taking a name the registrar +// holds silently hands its incoming payments to whoever holds the v3 name. +// Both places that assign a username — registration and the profile rename — +// must therefore refuse a name the registrar holds, unless the account's own +// nostr key is the holder (a user re-creating their migrated name is fine). +const NAMES_URL = process.env.NAMES_URL || "https://names.coinos.io"; +const DOMAIN = "coinos.io"; + +// Fail CLOSED: if the registrar can't be reached we don't know who owns the +// name, and handing out a squatted name is worse than a retry. +export const assertNameFree = async (username: string, ownPubkey?: string) => { + const name = username.toLowerCase(); + let body: any; + try { + const r = await fetch( + `${NAMES_URL}/name/${encodeURIComponent(name)}?domain=${DOMAIN}`, + { signal: AbortSignal.timeout(3000) }, + ); + if (!r.ok) throw new Error(`registrar said ${r.status}`); + body = await r.json(); + } catch (e: any) { + warn("names registrar unreachable checking", name, e?.message); + fail("Username unavailable right now, please try again"); + } + // `pubkey` is only present for a real v3 record. `taken` without one means + // the registrar is echoing our own account list back at us — not a claim. + const holder = body?.taken && typeof body.pubkey === "string" ? body.pubkey : null; + if (holder && holder !== ownPubkey) { + warn("username held by v3 registrar", name, holder.slice(0, 8)); + fail(`Username ${name} taken`); + } +}; diff --git a/lib/nwc.ts b/lib/nwc.ts index c714ae5a..a15d6c86 100644 --- a/lib/nwc.ts +++ b/lib/nwc.ts @@ -29,6 +29,21 @@ const serverKeys = { const result = (result) => ({ result }); const error = (error) => ({ error }); +// Every rejected NWC payment used to vanish without a trace: the error went +// back to the client and nothing was logged, so a user reporting "zaps over +// 1k fail" left no evidence server-side. One warn line per failure: who, +// which connection, how much, and why. +const payFail = (method, app, user, amount, err) => { + warn( + `nwc ${method} failed`, + user?.username, + String(app?.pubkey || "").slice(0, 12), + amount, + err?.error?.code, + String(err?.error?.message || "").slice(0, 160), + ); + return err; +}; const bc = rpc(config.bitcoin); const methods = [ @@ -638,7 +653,7 @@ const handle = (method, params, ev, app, user) => const { max_fee, pubkey } = app; const { budgetError, remaining } = await checkBudget(app, amount); - if (budgetError) return budgetError; + if (budgetError) return payFail("pay_invoice", app, user, amount, budgetError); if (payee === id) { const invoice = await getInvoice(pr); @@ -698,11 +713,15 @@ const handle = (method, params, ev, app, user) => const done = pays.find((x) => x.status === "complete"); if (done?.preimage) return result({ preimage: done.preimage }); if (pays.length && pays.every((x) => x.status === "failed")) - return error({ code: "PAYMENT_FAILED", message: "Payment failed" }); + return payFail("pay_invoice", app, user, amount, + error({ code: "PAYMENT_FAILED", message: "Payment failed" })); await sleep(2000); } + return payFail("pay_invoice", app, user, amount, + error({ code: "PAYMENT_FAILED", message: "Payment still pending after 60s" })); } catch (e) { - return error({ code: "PAYMENT_FAILED", message: e.message }); + return payFail("pay_invoice", app, user, amount, + error({ code: "PAYMENT_FAILED", message: e.message })); } // Still in flight after the fallback window: the payment may yet settle. diff --git a/lib/payments.ts b/lib/payments.ts index b26b443f..bd706b71 100644 --- a/lib/payments.ts +++ b/lib/payments.ts @@ -1,6 +1,7 @@ import config from "$config"; import { existsSync } from "fs"; import api from "$lib/api"; +import { requireAccount } from "$lib/auth"; import { archive, db, g, ga, gf, s, sa } from "$lib/db"; import { assertWithinSpendLimit } from "$lib/spend-budget"; @@ -325,6 +326,13 @@ export const credit = async ({ }) => { amount = Number.parseInt(amount) || 0; + // Kill switch (see routes/payments.ts confirm): no Liquid credits at all + // while `liquid:deposits:disabled` is set. + if (type === PaymentType.liquid && (await g("liquid:deposits:disabled"))) { + warn("liquid credit blocked (disabled)", hash, amount, ref); + return; + } + let inv = await getInvoice(hash); if (!inv && type === PaymentType.bolt12) { const { invoices } = await ln.listinvoices({ invstring: hash }); @@ -626,7 +634,8 @@ export const decode = async (hex) => { const inflight = {}; export const sendOnchain = async (params) => { let { aid, hex, rate, user, signed } = params; - if (!aid) aid = user.id; + aid = await requireAccount({ aid, user }); + const supplied = !!hex; if (!hex) ({ hex } = await build(params)); const { tx, type } = await decode(hex); @@ -636,6 +645,25 @@ export const sendOnchain = async (params) => { let { txid } = tx; let locked = false; + // Only sign a transaction WE composed for this account. build() leaves a + // short-lived receipt keyed by txid; caller-supplied hex must match one. + // Otherwise the hot wallet signs whatever raw tx is posted to /bitcoin/send + // and the only thing between an arbitrary tx and the chain is the debit math + // below (plus the weSpent credit-side guard in /confirm) — this is the outer + // layer, so an unrecognized tx never reaches signRawTransactionWithWallet + // and never locks our UTXOs on the way to failing. + // + // Receipts survive client-side signing: build() returns an unsigned segwit + // tx and witness data doesn't change the txid, so the self-custody path + // (which posts back hex it finalized itself) matches the same receipt. They + // are not single-use — a replay of the identical tx is already stopped by + // inflight/lockUnspent/testMempoolAccept, and burning the receipt would + // break a legitimate retry after a transient failure. + if (supplied) { + const builder = await db.get(`build:${txid}`); + if (builder !== aid) fail("unrecognized tx"); + } + try { if (inflight[txid]) fail("payment in flight"); inflight[txid] = true; @@ -986,7 +1014,20 @@ export const sendLightning = async ({ // bolt11: pr.replace(/\s/g, "").toLowerCase(), amount_msat: amount_msat ? undefined : amount * 1000, maxfee: fee * 1000, - retry_for: 30, + // 30s wasn't enough to get past a drained zero-fee hop. Route-finding + // prefers free channels, so xpay would spend the whole budget retrying + // one 0-base/0-ppm channel that is permanently empty in our direction + // and time out ("64 attempts, temporary_channel_failure for the same + // scid") without ever trying the paths that work — Strike/Zap payments + // failed this way for every user. xpay is in ln.ts's NO_TIMEOUT set and + // nothing in front of it (fastify, Caddy, the UI fetch) imposes a + // deadline, so the longer budget runs to completion. + // + // COUPLED TO check(): that reversal sweep must keep skipping payments + // for at least twice this window, or it reverses a payment xpay is + // still driving and we refund one that settles (the LEAKED DEBIT + // losses). Change both together. + retry_for: 60, layers: ["prefer-kappa"], }); @@ -1164,7 +1205,7 @@ export const build = async ({ user, }) => { const type = await getAddressType(address); - if (!aid) aid = user.id; + aid = await requireAccount({ aid, user }); const node = aid === user.id ? rpc(config[type]) : rpc({ ...config[type], wallet: aid }); @@ -1241,7 +1282,12 @@ export const build = async ({ } const inputs = []; - const { vin } = await node.decodeRawTransaction(tx.hex); + const { vin, txid: built } = await node.decodeRawTransaction(tx.hex); + + // Receipt for sendOnchain(): this account composed this exact tx. Bound to + // aid so one account can't replay another's build, and short-lived so a hex + // can't be banked for later. + await db.set(`build:${built}`, aid, { EX: 3600 }); for (const { txid, vout } of vin) { const rawTx = await node.getRawTransaction(txid); @@ -1318,13 +1364,15 @@ export const check = async () => { } const p = await getPayment(pr); // Skip payments sendLightning may still be actively driving. xpay retries - // for 30s (retry_for: 30), during which listpays can momentarily show all + // for 60s (retry_for: 60), during which listpays can momentarily show all // attempts "failed" before the winning part lands. The old 10s threshold // let check() reverse/refund such a payment mid-flight; it then completed, // and sendLightning's finalize threw on the deleted record — refunding a // payment that actually settled (the LEAKED DEBIT losses). Wait well past // the retry window so sendLightning has finished finalize()/reverse() and - // removed it from `pending` before check() ever touches it. + // removed it from `pending` before check() ever touches it. This is 2x + // retry_for, the same margin the 60s threshold gave the old 30s window — + // raise it with retry_for if that ever changes again. // No record anywhere (main db or archive) means there's nothing left to // reconcile — the debit was already reversed or the entry is a stale // stray. Drop it: sendLightning now refuses to re-pay an invoice while @@ -1334,7 +1382,7 @@ export const check = async () => { await db.sRem("pending", pr); continue; } - if (Date.now() - p.created < 60000) continue; + if (Date.now() - p.created < 120000) continue; const { pays } = await ln.listpays(pr); const failed = !pays.length || pays.every((p) => p.status === "failed"); diff --git a/lib/register.ts b/lib/register.ts index 16aa5b02..e140947b 100644 --- a/lib/register.ts +++ b/lib/register.ts @@ -3,6 +3,7 @@ import countries from "$lib/countries"; import { db, s } from "$lib/db"; import { l, warn } from "$lib/logging"; import { fail } from "$lib/utils"; +import { assertNameFree } from "$lib/names"; import { bytesToHex, randomBytes } from "@noble/hashes/utils"; import { got } from "got"; import { getPublicKey, nip19 } from "nostr-tools"; @@ -28,6 +29,8 @@ export default async (user, ip) => { const exists = await db.exists(`user:${username}`); if (exists) fail(`Username ${username} taken`); + // A name the v3 registrar holds is taken too (see lib/names.ts). + await assertNameFree(username, pubkey); if (password) { user.password = await Bun.password.hash(password, { diff --git a/lib/types.ts b/lib/types.ts index e878dfb8..802fe995 100644 --- a/lib/types.ts +++ b/lib/types.ts @@ -15,7 +15,6 @@ export interface User { follows: string; haspin: string; hasprinter: string; - hidepay: string; id: string; index: string; keys: string; diff --git a/lib/utils.ts b/lib/utils.ts index f336c144..539258a4 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -172,7 +172,6 @@ export const fields = [ "currency", "display", "id", - "hidepay", "lud16", "memoPrompt", // a v3-migrated account: the names registrar owns this name for receiving diff --git a/lib/webhooks.ts b/lib/webhooks.ts index 2a888991..d724ccdf 100644 --- a/lib/webhooks.ts +++ b/lib/webhooks.ts @@ -12,6 +12,13 @@ export const callWebhook = async (invoice, payment) => { const { amount, confirmed, hash, memo } = payment; l("calling webhook", webhook, amount, hash, address, text); + // TLS verification stays ON. This body carries `secret` — the shared + // value the merchant uses to authenticate the notification — so with + // rejectUnauthorized:false any party able to intercept the connection + // could present its own certificate, harvest the secret, and then forge + // "payment received" callbacks to that merchant. A merchant on a + // self-signed cert now fails here instead, which the catch below logs; + // the payment itself is unaffected. const res = await got.post(webhook, { json: { address, @@ -23,7 +30,6 @@ export const callWebhook = async (invoice, payment) => { text, secret, }, - https: { rejectUnauthorized: false }, }); return res; } diff --git a/lib/whitelist.ts b/lib/whitelist.ts index c1ac5340..d0acb0c4 100644 --- a/lib/whitelist.ts +++ b/lib/whitelist.ts @@ -16,7 +16,6 @@ export default [ "follows", "haspin", "hasprinter", - "hidepay", "id", "index", "keys", -- cgit v1.2.3