fix(security): second-audit findings + restore cold CI

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.
This commit is contained in:
thePR0M3TH3AN committed 2026-08-10 09:18:17 -04:00
1 parent f697668f8f
commit 7cc5756c15
7 files changed
+123 -7

No files matched your search

+15 -4
View File
@@ -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(",")}}`;
}
+9
View File
@@ -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 {
+18 -1
View File
@@ -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();
}
+12
View File
@@ -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);
}
+4 -1
View File
@@ -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() {
+4 -1
View File
@@ -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() {
+61
View File
@@ -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);
});
});