morphit/docs/AUDIT-cp138-FINDINGS.md

# cp138 deep-deep audit — findings ledger

**Campaign:** Pre-launch deep-deep, 94 tasks across 11 phases.
**Trigger:** Ken's directive: "do a full deep deep on absolutely everything, every file and script, .md/.ts/all svelte-related" + "put on your black hat. FULL security and code audits."
**Started:** 2026-05-25 (cp138 from cp137 baseline).
**Closed:** 2026-05-25.
**Outcome:** 11 findings shipped, 1 standing follow-up, 0 outstanding HIGH/CRITICAL, smoke suite green at 169/0.

---

## Phase A — Hostile chain-op review (all 17 handlers reviewed)

### cp138-A-1 SHIPPED (MED) — ADR-0004 amendment overstated frontend price-provider wiring

**Where:** `docs/adr/0004-price-feeds.md`, 2026-05-09 amendment + status header date line.

**Claim:** Amendment said "frontend defaults to fallback prices unless the user explicitly opts into the indexer-relayed feed via Settings" and "a direct-CoinGecko provider also exists and ships [...]; a user who wants freshest prices and is OK with the IP leak can flip to it."

**Reality (caught by orphan-file scan):**
- `apps/web/src/lib/prices/providers/composite.ts` — exported, **not imported anywhere**
- `apps/web/src/lib/prices/providers/coingecko.ts` — exported, **not imported anywhere**
- `apps/web/src/lib/prices/index.ts:93` `setProvider()` — exported, **never called**
- Settings page has no price-provider toggle

The frontend uses `fallbackProvider` only. Indexer-side composite source IS real and live at `/v1/price/...`, but the frontend doesn't currently consume it.

**Fix:** Corrected ADR-0004 amendment to accurately describe state (RFC code parked for a future `ApiRelayProvider` + Settings opt-in); updated status header date line to reflect partial Phase-3 completion. Added cp138-R-1 (future "ship ApiRelayProvider + Settings opt-in") to REVISIT-LIST.

**Severity rationale:** MED not HIGH because no user-facing security issue (fallback is static + transparent) and no user-facing privacy regression (the privacy-positive default of "frontend doesn't call CoinGecko directly" is trivially upheld when there's no provider switch). The drift is purely doc↔code accuracy.

### cp138-A-2 SHIPPED (MED) — feedbackResponse.ts parseInt-on-BIGSERIAL

**Where:** `apps/indexer/src/indexer/handlers/feedbackResponse.ts:95`.

**Issue:** Was passing `parseInt(row.id, 10)` as a SQL parameter. `feedback.id` is BIGSERIAL (range 2^63 ≈ 9.2e18) but `Number.MAX_SAFE_INTEGER` is 2^53 ≈ 9e15. For realistic message volumes parseInt is safe, but the pattern is wrong because pg accepts BIGINT params as strings; the correct fix preserves full 2^63 range with no cost.

**Fix:** Pass `row.id` directly (the BIGINT-as-string pg returned). Same problem exists at 10 other call sites (all `parseInt(row.id, 10)` for display purposes); tracked as cp138-R-1 since the display path goes to JSON which has no native bigint type anyway.

### cp138-A-3 SHIPPED (LOW) — stale comment claimed chat_messages.id is SERIAL

**Where:** `apps/indexer/src/api/chatStream.ts:104`.

**Issue:** Comment said "(safe — chat_messages.id is SERIAL, max ~2^31)". Actual schema declares it BIGSERIAL.

**Fix:** Comment corrected with note that parseInt is safe at practical Morphit scale + cross-reference to cp138-R-1.

### cp138-A-4 SHIPPED (LOW) — operatorPaymentMethod forbidden-char + NFC drift

**Where:** `apps/indexer/src/indexer/handlers/operatorPaymentMethod.ts`.

**Issue:** Used a narrower forbidden-char set than `order.ts/feedback.ts/profile.ts` (just bidi + ZW + paragraph separators, missing C0/C1 control chars and DEL). Also lacked NFC normalization.

**Risk surface:** Small — operator-only signer = single trusted account. Operator could accidentally smuggle control chars into their own payment-method display. The inconsistency was the real issue.

**Fix:** Aligned policy. STRIP_CODEPOINTS_RE now covers `\u0000-\u001F\u007F-\u009F\u202A-\u202E\u2066-\u2069\u200B-\u200D\u2028\u2029\uFEFF` (matches `FORBIDDEN_TEXT_CHARS` in order.ts). `sanitize()` now NFC-normalizes first.

### cp138-A-5 SHIPPED (LOW) — operatorBlock.sanitizeReason lacked NFC

**Where:** `apps/indexer/src/indexer/handlers/operatorBlock.ts`.

**Issue:** `sanitizeReason()` stripped C0/C1 control chars and bidi marks but didn't NFC-normalize first. NFD-decomposed input could carry visually-equivalent codepoints that don't match the strip set.

**Fix:** Added `raw.normalize('NFC')` before the for-of strip loop; loop now iterates `normalized` rather than `raw`.

### Phase A clean-handler summary

All 17 chain-op handlers reviewed deeply: dispatcher (integer-typed savepoint keys), order (NFC + forbidden chars + length caps + duplicate detection), chat (block-check before order-permlink bypass; base64 regex anchored + ReDoS-safe at 10k chars <1ms), feedback (three-prong order verification: exists + subject + fee_status='verified'), feedbackResponse (A-2 fix), operatorRegister (comprehensive SSRF defense: RFC1918 + link-local + IMDS + IPv6 + pseudo-TLD), chatRead (strict ISO-8601 + monotonic-advance via atomic UPDATE WHERE + 60s future skew tolerance + 2020-floor), chatIdentity (RFC 7748 small-order X25519 point rejection including bit-255-masking variants), release (triple-AND trust anchor + XMR view-key privacy invariant), featureBid (11 rejection paths + fee verification + anti-sniping 6h floor), orderCancel (atomic UPDATE WHERE constrained to signer's (account, permlink) + state filter prevents replay), orderReplace (mirrors order.ts validation), profile (NFC + FORBIDDEN_DISPLAY_NAME_CHARS + codepoint count + impersonation check), strangerFee (memo-binding + sibling-op transfer + escalating fees 5/2x/cap 640), feeAttest (2-distinct + 1-non-poster sybil defense), operatorBlock (A-5 fix), operatorPaymentMethod (A-4 fix), block (self-block rejection + action whitelist + "no prior block" rejection).

---

## Phase B — HTTP / API surface (no findings)

- **Relay security headers** (`apps/relay/src/middleware/security.ts`): nosniff + no-referrer + frame-deny + Permissions-Policy + locked-down CSP (`default-src 'none'; frame-ancestors 'none'; base-uri 'none'`). Body cap enforces Content-Length + explicit Transfer-Encoding:chunked rejection (HTTP 411) for body-bearing methods.
- **Indexer security headers** (`apps/indexer/src/api/middleware/security.ts`): matches relay posture per ADR-0006 + `cross-origin-resource-policy: cross-origin` (public-read API) + Cache-Control defaulting.
- **CORS** (`apps/relay/src/middleware/cors.ts`): exact-match origin allowlist, no wildcards.
- **Origin enforcement** (`apps/relay/src/middleware/origin_enforcement.ts`): server-side allowlist for fund-spending endpoints (defends curl bypass of browser CORS).
- **Error message leakage**: sampled 4 catch sites; all return `{status:'internal'}` to client + log err server-side. No PII / stack / host info leaks.

---

## Phase C — Crypto specifics

### cp138-C-1 SHIPPED (MED, was M4 from 2026-04-28 audit) — KDF floor was 6000× too generous

**Where:**
- `apps/web/src/lib/crypto/keystore.ts:629-630` (passphrase wrap)
- `apps/web/src/lib/crypto/yubikey/wrap.ts:53-54` (YubiKey HMAC wrap)

**Issue:** `MIN_KDF_OPSLIMIT = 1, MIN_KDF_MEMLIMIT = 1 MB` was way below libsodium's `INTERACTIVE` (ops=2, mem=64 MiB) which the encrypt path actually uses. **Latent downgrade-attack surface:** a tampered envelope could claim weak Argon2 params (ops=1, mem=1 MB) and bypass ~6000× of the password-strength wall.

**Prior history:** Identified as finding M4 by the 2026-04-28 batch-I audit and left open for a month. Closed in cp138.

**Fix:** Raised both floors to match libsodium INTERACTIVE exactly (`MIN_KDF_OPSLIMIT = 2`, `MIN_KDF_MEMLIMIT = 64 * 1024 * 1024`). Floor matches encrypt-time default; no envelope written by Morphit can be downgraded. 184 crypto+auth tests pass post-fix.

### Phase C clean-area summary

- **AEAD nonce uniqueness**: every nonce comes from `sodium.randombytes_buf(NONCEBYTES)`. Zero fixed/derived nonces. ✓
- **BIP-39**: uses `@scure/bip39` (audited Paul Miller library). ✓
- **secp256k1 signing**: uses `dblurt`'s `PrivateKey.sign` which internally generates canonical RFC6979 signatures (deterministic k). No nonce reuse. ✓
- **Random source audit**: 2 `Math.random()` uses in production (modal ID, RPC endpoint shuffle). Both non-security. ✓
- **Forward secrecy posture**: `docs/CHAT-CRYPTO.md §1-5` explicitly documents the design decision (stateless ECIES — multi-device default; chat-priv compromise implies posting-priv compromise so per-message rotation defends a small slice while leaving everything else exploitable; fast key rotation is the right defense). Not a gap.

---

## Phase D — Database

### cp138-D-1 SHIPPED (LOW) — non-deterministic triggered_at

**Where:** `apps/indexer/src/indexer/loyalty.ts` (both INSERT sites at line 159 and 242).

**Issue:** `account_loyalty_milestones.triggered_at` had `DEFAULT NOW()` in the schema and was never explicitly written. `NOW()` is the indexer's wall clock at insert moment, NOT the block's timestamp. For a deterministic chain-replay system, this would diverge across replays. Column currently unread, but a future reader would hit the determinism gap.

**Fix:** Both INSERT sites now pass `blockTime` explicitly. 7 loyalty smoke scenarios + 10 loyalty vitest tests still pass.

### cp138-D-2 SHIPPED (MED) — push subscription amplification

**Where:** `apps/relay/src/policy/pushSubscriptions.ts` `upsert()`.

**Issue:** No per-account cap. A user could register thousands of `(account, endpoint)` pairs and create push-notification amplification surface. The push-sender fan-out loop (`pushSender.ts:190`) awaits one POST per device per inbound message — every chat message they receive would thrash outbound HTTPS calls against arbitrary push services. Concrete attack: hostile user signs up many endpoints, then asks a popular trader for a long conversation; every reply ties up the relay's outbound HTTPS pool.

**Fix:** Added `MAX_SUBSCRIPTIONS_PER_ACCOUNT = 20`. Wrapped upsert in `this.db.withTx`. Added eviction step before INSERT: if the incoming endpoint is NEW (not an ON CONFLICT path) and existing count is at the cap, DELETE the oldest by `created_at ASC` first. The 3-step transaction (count → evict → upsert) is atomic — no race between parallel upserts can both see the same under-cap snapshot. 244 relay tests pass.

### Phase D clean-area summary

- **Race conditions**: 0 SELECT-then-INSERT patterns without `ON CONFLICT` — all upserts properly atomic.
- **SQL injection**: 18 template-literal interpolations, all SAVEPOINT names with integer-typed keys — safe.
- **LIKE-pattern injection**: 2 ILIKE queries, both use `escapeLike()` with explicit `ESCAPE '\\'`.
- **FK integrity**: all FKs have explicit `ON DELETE CASCADE` or `RESTRICT`.
- **Migration linearization**: 27 migrations, no gaps.
- **Dead fields**: only candidate was `triggered_at` (D-1, fixed).
- **statement_timeout**: pool has `connectionTimeoutMillis: 5000` but no `statement_timeout`. Defensible — operator can set via Postgres `postgresql.conf` or connection-string `?statement_timeout=30000`. Documented as REVISIT-LIST follow-up rather than changing pool defaults (which risks breaking long-running queue drains).

---

### cp138-D-3 IDENTIFIED (LOW practical, MED-on-paper) — matrix-bot-sdk transitive vulnerabilities

**Where:** `package-lock.json` — transitive deps of `matrix-bot-sdk@0.7.1` (used by `apps/matrix-bot`).

**`npm audit` summary (audit-level=moderate):**
- 2 critical (form-data unsafe-random-boundary GHSA-fjxv-7rqg-78g4; request SSRF GHSA-p8p7-x288-28g6)
- 14 moderate (qs DoS, tough-cookie prototype pollution, uuid bounds, esbuild dev-server, vite path traversal in optimized deps)
- 6 low (deduplicated)

**Practical exposure assessment:**

- `form-data` boundary uses unsafe random — exploitable only against an attacker-controlled multipart upload, which matrix-bot doesn't accept (it only POSTs JSON to a configured Matrix homeserver). Near-zero practical risk.
- `request` SSRF — exploitable only if user-controlled URLs are passed to it. matrix-bot doesn't accept inbound URLs to fetch; it sends outbound to operator-env-configured homeserver. Near-zero practical risk.
- `qs`/`tough-cookie`/`uuid` — same trust-boundary reasoning. matrix-bot doesn't parse user-supplied query strings or accept user cookies.
- `esbuild`/`vite` — dev-only. Production builds use compiled output served by nginx; the dev server is never exposed to the public.

**Why this is "LOW practical":** matrix-bot is **opt-in** (per the design-decision comment in `apps/matrix-bot/src/main.ts`: "if an operator doesn't use Matrix, the systemd unit can be safely enabled (or not) and the bot will exit cleanly without consuming resources"). An operator who doesn't set `MORPHIT_MATRIX_BOT_ALERT_MXID` never starts the bot, so the SDK isn't even loaded in their running process. For operators who DO opt in, the SDK is talking outbound to their own configured homeserver — there's no inbound user URL surface.

**Why this is "MED-on-paper":** The CVEs are real, the SDK ships with deprecated/abandoned `request@2.88.2`, and an automated CVE scanner will report criticals. For an operator running production, that's a quarterly-review item.

**Upstream constraint:** Upgrading to `matrix-bot-sdk@0.8.0` (latest) doesn't fix it — 0.8.0 still depends on `request@^2.88.2` and `request-promise@^4.2.6`. The fix would require either (a) switching to `matrix-js-sdk` (the official SDK with a bigger surface) or (b) adding `npm overrides` to force-resolve transitives, which requires testing matrix-bot's actual code paths still function with the overridden versions.

**Fix:** Documented in REVISIT-LIST as **cp138-R-2 standing follow-up**. No code change shipped in cp138 because (a) the practical risk is near-zero given matrix-bot's input surface, (b) the proper fix involves either an SDK swap or `npm overrides` testing that exceeds pre-launch scope, and (c) operators get clear opt-in semantics today.

**OPERATIONS.md update committed:** Added a new sub-section documenting the matrix-bot opt-in security posture and the known-issue list with practical-exposure annotations, so operators making the matrix-bot enablement decision have full information.

---

## Phase E — Frontend XSS surface (no findings)

All 16 `{@html}` sites verified trusted:

1. **8 i18n `@html` sites** — Pinned by `i18n-html-injection-smoke.ts` which walks every callsite, collects keys, scans all 10 locales for dangerous patterns. Allowed tags: `strong/em/b/i/br/a/code/span`.
2. **3 QR-code sites** (LoginQrInitiator, QrPanel, 2fa) — `qrcode` npm library `toString({type:'svg'})` outputs well-formed SVG.
3. **ProtectedTextarea overlay** — User input HTML-escaped via local `escapeHtml()`; `m.kind` from closed-set type `PrivateKeyMatchKind = 'wif' | 'hex_64' | 'mnemonic'`.
4. **Head.svelte onion-location** — `computeOnionLocation` validates `.onion` suffix; quotes escaped via `.replace(/"/g, '&quot;')`.
5. **Head.svelte JSON-LD** — `JSON.stringify(node).replace(/</g, '\\u003c')` prevents `</script>` injection.
6. **IdentityLabel + account profile avatar SVG** — Sanitized via `$lib/avatar/index.ts` (39 tests covering script-strip, on*-attr strip, javascript:-URL strip, foreignObject strip, etc.).

Other checks clean: 0 external `<script>/<link>` tags (no SRI surface needed), 0 `postMessage` sites, 0 direct external fetches (all RPC goes through endpoint rotator).

---

## Phase F — Static code quality

- **TODO/FIXME/XXX/HACK**: 0 instances in entire codebase. ✓
- **Math.random in security paths**: 0. ✓
- **Loose equality**: 7 intentional `== null` idiom + 3 inside prose. ✓
- **Secrets in error/log channels**: 0. ✓
- **Unguarded JSON.parse**: 3 candidates, all inside functions whose callers properly try/catch. ✓
- **Silent catch blocks**: 20+ found; sampled 3 — all legitimate "validate or fall through" patterns. ✓
- **setTimeout/setInterval cleanup**: 10 candidates with `setTimeout` only; all short fire-and-forget. The one "setInterval without clearInterval" hit was actually a comment, not code.
- **Dead-code orphan exports**: noisy scan returned 297 candidates, all type-export false positives. Cp138-A-1 was the one real dead-export case.

---

## Phase G — Regex accuracy

Substantively complete:

- **ReDoS scan**: 8 candidate `(X+)*` patterns, all the same permlink validator `/^[a-z0-9]+(?:-[a-z0-9]+)*$/`. Tested at 10k adversarial chars: 0.1ms. Safe (V8 linear-time on non-overlapping match alphabets).
- **22 unanchored production `.test()` regexes** all verified intentional (scheme prefixes, error-message matching, single-char alnum checks).
- **Base64 ciphertext regex** tested at 10k chars: <1ms. Safe.

---

## Phase H — Smokes

### cp138-H-1 SHIPPED (LOW) — persona-walkthrough ALERT_COPY sentinel undercount

**Where:** `apps/web/scripts/persona-walkthrough-smoke.ts` `P121-CP10-4` sentinel.

**Issue:** Sentinel listed only 14 of 17 host-resource ALERT_COPY events. `mount_critical`, `mount_warn`, `mount_info` had been added to `classifier.ts` but never added to the sentinel. The sentinel description "(14 entries)" was also stale.

**Fix:** Updated sentinel name to enumerate all 17 entries + added the 3 missing mustHave strings. Smoke remains 169/0.

### Phase H clean-area summary

- 17 hardcoded counts verified: "16 assets" ✓, "10 locales" ✓, "6 backlog locales" ✓ (it/pl/ru/fa/zh-CN/zh-HK), "≥20 [lang]/ entries" passes with 31 actual.
- Part-N citation staleness: oldest are Part 19 audit-doc citations describing WHEN bugs were discovered. Smokes themselves are still defending live invariants.
- 12 `describe.skipIf(!INTEGRATION_ENABLED)` sites — gated on Postgres harness availability, not silent debt.
- 2 genuine skips both documented (cross-tab jsdom env; module-removed audit trail).
- Property-based / mutation testing: not adopted. Future work, not a regression.

---

## Phase I — Documentation accuracy

### cp138-I-1 SHIPPED (LOW) — no repo-root SECURITY.md

**Where:** Repo root.

**Issue:** Forgejo's convention (like GitHub) is to look for `SECURITY.md` at root, `.forgejo/`, or `docs/`. The full security policy lived only at `docs/SECURITY.md`. While Forgejo does fall through to `docs/`, the root convention is friendlier to researchers using auto-discovery tools.

**Fix:** Added 27-line repo-root `SECURITY.md` pointing to `docs/SECURITY.md` with the Matrix DM + Forgejo paths + 72h ack + 7d triage commitments.

### Phase I clean-area summary

- README.md asset enumeration (16 assets) ✓
- RUN-A-MORPHIT-NODE.md "Trade-only assets" heading lists 13 + BTC/XMR/BLURT = 16 ✓
- OPERATIONS.md grep for drift indicators: all Part-N citations historical, not stale
- Broken doc-relative links: 0 (one false positive on `(url)` inside a markdown example syntax)
- METADATA-LEAK-CATALOG.md claim "no IP addresses logged" verified by `access_log.ts` source
- PRIVACY.md: doesn't exist as separate file — METADATA-LEAK-CATALOG.md + SECURITY.md cover the privacy posture (documented design choice)
- FAQ entries: 256 total, asset coverage uneven but expected (BTC/XMR/BLURT most-mentioned)

---

## Phase J — Wiring drift

### cp138-J-1 SHIPPED (LOW) — XRP address placeholder unwired

**Where:** `apps/web/src/lib/components/AddressShareModal.svelte`.

**Issue:** Placeholder ternary chain covered 14 of 16 assets (BTC, XMR, USDT, USDC, DAI, BCH, LTC, DASH, DOGE, ZEC, ARRR, DCR, SOL, ETH) but XRP was missing. When `method === 'xrp'`, the chain fell through to BLURT's placeholder. The `chat.address.address_placeholder_xrp` locale key existed in all 10 locales but was unreferenced. Address VALIDATION for XRP already worked (line 292).

**Fix:** Inserted the XRP branch between ETH and the BLURT fallback.

### Phase J clean-area summary

- **Locale parity**: 9 non-English locales achieve perfect parity with `en.json` (3,095 keys × 10 locales = 30,950 pairs). 0 orphans, 0 missing. ✓
- **Code-referenced keys missing from en.json**: 0 real cases (1 false positive in a docstring example). ✓

---

## Phase K — Failover / external-dep fallback (no findings)

- **All-endpoints-failed**: endpoint rotator throws cleanly; UI catches and shows degraded state. ✓
- **Unactionable errors**: 1 hit (`chat.message.failed_label: 'Failed'`) — actually correct as a per-message status label, not an error popup. ✓

---

## Standing follow-ups (logged in REVISIT-LIST)

- **cp138-R-1 (post-launch scaling) — bigint id propagation**. 11 sites in `apps/indexer/src/{api,indexer}/...` use `parseInt(row.id, 10)` to convert BIGSERIAL ids back to JS numbers. Safe up to 2^53 (~9 quadrillion rows) which is far beyond Morphit's realistic scale, but the correct pattern is to keep ids as strings end-to-end (DB → wire → client) since JSON has no native bigint. Long-horizon scaling item, not a pre-launch blocker.

- **Ship `ApiRelayProvider` + Settings opt-in for live prices** to deliver the user-facing price-staleness UX that ADR-0004 originally promised. Frontend module exists, indexer endpoint exists; what's missing is the apirelay provider implementation + a Settings toggle. Was overstated as shipped in 2026-05-09; corrected in cp138.

- **Add `statement_timeout` guidance** to OPERATIONS.md so operators know to set Postgres-side `statement_timeout` for production deployments (current pool defaults don't ship one to avoid breaking long-running queue drains). — **SHIPPED 2026-05-25 (post-cp138)** at OPERATIONS.md §37.8 sub-item `e.`, with sentinel coverage in `scripts/operations-hardening-smoke.ts` and the §37.8 one-liner in RUN-A-MORPHIT-NODE.md §11 updated to match.

---

## Out-of-scope answer to "what would a pro firm do?"

See `docs/AUDIT-OUTSIDE-SCOPE.md` for the answer to Ken's "would a pro firm find anything I haven't?" question — leverage/urgency table for 15 test categories, top 5 (DAST against running instance, cryptographic specialist review, active fuzzing of parsers, threat modeling workshop, supply-chain audit + SBOM), with budget estimates (tier-1 firm $40-120k vs $5-15k contractor rotation vs $5k HackerOne starter).

---

**End of cp138 audit findings.**