From 7cc5756c1512be65c5874d330b7c51c1372d8438 Mon Sep 17 00:00:00 2001
From: thePR0M3TH3AN <53631862+PR0M3TH3AN@users.noreply.github.com>
Date: Mon, 10 Aug 2026 09:18:17 -0400
Subject: [PATCH] fix(security): second-audit findings + restore cold CI
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

A second adversarial pass over areas the first audit did not reach: codecs,
transport internals, and the verify/outbox/private-mute paths added since.

Reliability/CI (most urgent)
- The previous commit's Node-safety base-class fallback lost the HTMLElement
  type, so defineElement() and this-typing broke. It only appeared with dist
  cleared, so it passed locally with stale declarations and would fail CI from
  a clean checkout. Annotated the fallback; cold lint/test/build now pass.

Security
- Relay URLs from a followed contact's kind:10002 flow straight to
  new WebSocket(url). normalizeRelayUrl only checked the ws(s) prefix, so a URL
  containing CR/LF or spaces passed through to the socket constructor — a
  CRLF-injection shape. Now validated with the URL constructor and rejected for
  any control character or whitespace, with a second guard at the socket
  boundary regardless of caller.
- canonicalStringify recursed without cycle detection: a self-referential value
  was a stack-overflow DoS rather than a wrong answer. Now guarded with a
  WeakSet; a repeat serializes as a sentinel.
- The viewer's private mute payload was JSON.parsed with no size bound. Capped
  at 128 KiB, checked before spending a decrypt on it.

What held up under this pass: address-coordinate parsing, the outbox per-author
relay cap (3), verifier handling of NaN/Infinity/string timestamps, and mute
tag robustness against non-string entries.

861 tests; audit clean; cold gates green.
---
 packages/core/src/fingerprint.js         | 19 ++++++--
 packages/nostr/src/mutes.js              |  9 ++++
 packages/nostr/src/relayList.js          | 19 +++++++-
 packages/runtime/src/relayTransport.js   | 12 +++++
 packages/widget/src/base.js              |  5 +-
 packages/widget/src/provider.js          |  5 +-
 tests/security/audit-regressions.test.js | 61 ++++++++++++++++++++++++
 7 files changed, 123 insertions(+), 7 deletions(-)

diff --git a/packages/core/src/fingerprint.js b/packages/core/src/fingerprint.js
index a91d6ba..fc16c48 100644
--- a/packages/core/src/fingerprint.js
+++ b/packages/core/src/fingerprint.js
@@ -11,15 +11,26 @@
  * Canonical JSON: object keys sorted at every depth, so two structurally equal
  * values always serialize to the same string.
  * @param {unknown} value
+ * @param {WeakSet<object>} [seen] - Internal, for cycle detection across recursion
  * @returns {string}
  */
-export function canonicalStringify(value) {
+export function canonicalStringify(value, seen) {
   if (value === null || typeof value !== "object") {
     return JSON.stringify(value) ?? "null";
   }
 
+  // Cycle guard: a caller could hand us a snapshot fragment that references
+  // itself, and unguarded recursion is a stack-overflow DoS rather than a
+  // wrong answer. A repeated object serializes as a sentinel — fingerprints
+  // only need to be stable and collision-resistant, not reversible.
+  const visited = seen ?? new WeakSet();
+  if (visited.has(value)) {
+    return '"[cycle]"';
+  }
+  visited.add(value);
+
   if (Array.isArray(value)) {
-    return `[${value.map((entry) => canonicalStringify(entry)).join(",")}]`;
+    return `[${value.map((entry) => canonicalStringify(entry, visited)).join(",")}]`;
   }
 
   if (value instanceof Set) {
@@ -30,7 +41,7 @@ export function canonicalStringify(value) {
     const entries = Array.from(value.entries())
       .map(([key, entry]) => [String(key), entry])
       .sort((a, b) => (a[0] < b[0] ? -1 : a[0] > b[0] ? 1 : 0));
-    return `{${entries.map(([key, entry]) => `${JSON.stringify(key)}:${canonicalStringify(entry)}`).join(",")}}`;
+    return `{${entries.map(([key, entry]) => `${JSON.stringify(key)}:${canonicalStringify(entry, visited)}`).join(",")}}`;
   }
 
   const keys = Object.keys(/** @type {Record<string, unknown>} */ (value)).sort();
@@ -40,7 +51,7 @@ export function canonicalStringify(value) {
     if (entry === undefined) {
       continue;
     }
-    parts.push(`${JSON.stringify(key)}:${canonicalStringify(entry)}`);
+    parts.push(`${JSON.stringify(key)}:${canonicalStringify(entry, visited)}`);
   }
   return `{${parts.join(",")}}`;
 }
diff --git a/packages/nostr/src/mutes.js b/packages/nostr/src/mutes.js
index d07db24..b1bc9ed 100644
--- a/packages/nostr/src/mutes.js
+++ b/packages/nostr/src/mutes.js
@@ -20,6 +20,9 @@ import { getTags } from "./replaceable.js";
 /** NIP-51 mute list kind. */
 export const MUTE_LIST_KIND = 10000;
 
+/** Largest encrypted mute payload decoded, in bytes of ciphertext. */
+export const MAX_MUTE_CONTENT_BYTES = 128 * 1024;
+
 /**
  * Resolve the category attached to a mute tag.
  *
@@ -95,6 +98,12 @@ export async function decodePrivateMuteEntries(event, { viewerPubkey, decrypt })
   if (!content || typeof decrypt !== "function") {
     return [];
   }
+  // Bound the ciphertext before spending a decrypt and a parse on it. A mute
+  // list of any realistic size is far under this; anything larger is a resource
+  // trap, not a real list.
+  if (content.length > MAX_MUTE_CONTENT_BYTES) {
+    return [];
+  }
 
   let tags;
   try {
diff --git a/packages/nostr/src/relayList.js b/packages/nostr/src/relayList.js
index b168253..23cc799 100644
--- a/packages/nostr/src/relayList.js
+++ b/packages/nostr/src/relayList.js
@@ -39,9 +39,26 @@ export function normalizeRelayUrl(value) {
     return "";
   }
   const trimmed = value.trim();
-  if (!/^wss?:\/\//i.test(trimmed)) {
+
+  // A relay URL comes from a followed contact's kind:10002 and flows straight
+  // to `new WebSocket(url)`. Control characters or internal whitespace have no
+  // legitimate place in a URL and are exactly what a CRLF-injection attempt
+  // looks like, so reject anything that is not a clean ws(s) URL. The URL
+  // constructor does the structural validation the old prefix regex skipped.
+  if (!/^wss?:\/\//i.test(trimmed) || /[\s\u0000-\u001f]/.test(trimmed)) {
     return "";
   }
+
+  let parsed;
+  try {
+    parsed = new URL(trimmed);
+  } catch {
+    return "";
+  }
+  if ((parsed.protocol !== "wss:" && parsed.protocol !== "ws:") || !parsed.hostname) {
+    return "";
+  }
+
   return trimmed.replace(/\/+$/, "").toLowerCase();
 }
 
diff --git a/packages/runtime/src/relayTransport.js b/packages/runtime/src/relayTransport.js
index c8694e3..80806c7 100644
--- a/packages/runtime/src/relayTransport.js
+++ b/packages/runtime/src/relayTransport.js
@@ -105,6 +105,15 @@ export function createRelayTransport(urls, options = {}) {
     const existing = connections.get(url) ?? { socket: null, ready: false, queue: [], attempts: 0, timer: null };
     connections.set(url, existing);
 
+    // Defence in depth: even though every internal caller pre-validates relay
+    // URLs, this is the last point before the socket constructor. A URL with
+    // control characters or whitespace is malformed by definition and never
+    // reaches the network.
+    if (typeof url !== "string" || !/^wss?:\/\//i.test(url) || /[\s\u0000-\u001f]/.test(url)) {
+      diagnostic("relay url rejected", { url });
+      return;
+    }
+
     let socket;
     try {
       socket = new SocketImpl(url);
@@ -234,6 +243,9 @@ export function createRelayTransport(urls, options = {}) {
       if (!url) {
         continue;
       }
+      if (typeof url !== "string" || !/^wss?:\/\//i.test(url) || /[\s\u0000-\u001f]/.test(url)) {
+        continue;
+      }
       if (!connections.has(url)) {
         connect(url);
       }
diff --git a/packages/widget/src/base.js b/packages/widget/src/base.js
index f9b6f45..485f18a 100644
--- a/packages/widget/src/base.js
+++ b/packages/widget/src/base.js
@@ -172,8 +172,11 @@ export function targetFromAttributes(element) {
  * Falling back to a plain class keeps the module importable for a
  * server-rendering host; registration is still a no-op there.
  */
+/** @type {typeof globalThis.HTMLElement} */
 const ElementBase =
-  typeof globalThis.HTMLElement === "function" ? globalThis.HTMLElement : /** @type {any} */ (class {});
+  typeof globalThis.HTMLElement === "function"
+    ? globalThis.HTMLElement
+    : /** @type {any} */ (class {});
 
 export class GovernanceElement extends ElementBase {
   constructor() {
diff --git a/packages/widget/src/provider.js b/packages/widget/src/provider.js
index 8e518c8..0550a07 100644
--- a/packages/widget/src/provider.js
+++ b/packages/widget/src/provider.js
@@ -34,8 +34,11 @@ export function requestContext(element) {
   return detail.context;
 }
 
+/** @type {typeof globalThis.HTMLElement} */
 const ProviderBase =
-  typeof globalThis.HTMLElement === "function" ? globalThis.HTMLElement : /** @type {any} */ (class {});
+  typeof globalThis.HTMLElement === "function"
+    ? globalThis.HTMLElement
+    : /** @type {any} */ (class {});
 
 export class BitGateProvider extends ProviderBase {
   static get observedAttributes() {
diff --git a/tests/security/audit-regressions.test.js b/tests/security/audit-regressions.test.js
index 60f3d52..629a694 100644
--- a/tests/security/audit-regressions.test.js
+++ b/tests/security/audit-regressions.test.js
@@ -349,3 +349,64 @@ describe("finding 7: growth is bounded", () => {
     expect(runtime.mutes.lists.size).toBeLessThanOrEqual(6);
   });
 });
+
+describe("second audit: relay url injection", () => {
+  it("rejects relay urls with control characters or whitespace", async () => {
+    const { normalizeRelayUrl } = await import("@bitgate/nostr");
+    for (const url of ["wss://a\r\nb", "ws:// spaces", "wss://\thost", "wss://", "javascript:alert(1)"]) {
+      expect(normalizeRelayUrl(url), url).toBe("");
+    }
+    expect(normalizeRelayUrl("wss://ok.example/")).toBe("wss://ok.example");
+  });
+
+  it("drops a malicious contact's crlf relay from an outbox grouping", async () => {
+    const { groupAuthorsByWriteRelay } = await import("@bitgate/nostr");
+    const pubkey = "d4".repeat(32);
+    const lists = new Map([
+      [pubkey, { pubkey, updatedAt: NOW, read: [], write: ["wss://evil\r\nInjected: x", "wss://good.example"] }],
+    ]);
+    const grouped = groupAuthorsByWriteRelay([pubkey], lists);
+    expect([...grouped.keys()]).toEqual(["wss://good.example"]);
+  });
+});
+
+describe("second audit: fingerprint cycle safety", () => {
+  it("does not overflow on a self-referential value", async () => {
+    const { canonicalStringify, fingerprint } = await import("@bitgate/core");
+    const cyclic = { a: 1 };
+    cyclic.self = cyclic;
+    expect(() => canonicalStringify(cyclic)).not.toThrow();
+    expect(() => fingerprint(cyclic)).not.toThrow();
+  });
+
+  it("still distinguishes genuinely different structures", async () => {
+    const { fingerprint } = await import("@bitgate/core");
+    expect(fingerprint({ a: 1 })).not.toBe(fingerprint({ a: 2 }));
+  });
+});
+
+describe("second audit: private mute payload is bounded", () => {
+  it("refuses an oversized encrypted payload before parsing", async () => {
+    const { decodePrivateMuteEntries, MUTE_LIST_KIND } = await import("@bitgate/nostr");
+    const viewer = "d4".repeat(32);
+    const event = {
+      id: "a".repeat(64),
+      pubkey: viewer,
+      kind: MUTE_LIST_KIND,
+      created_at: 1,
+      tags: [],
+      content: "x".repeat(200 * 1024),
+    };
+    let decryptCalled = false;
+    const entries = await decodePrivateMuteEntries(event, {
+      viewerPubkey: viewer,
+      decrypt: async () => {
+        decryptCalled = true;
+        return "[]";
+      },
+    });
+    expect(entries).toEqual([]);
+    // The bound is checked before spending a decrypt on it.
+    expect(decryptCalled).toBe(false);
+  });
+});