diff --git a/packages/signer/README.md b/packages/signer/README.md index 47c7f52..7dd30d5 100644 --- a/packages/signer/README.md +++ b/packages/signer/README.md @@ -12,13 +12,21 @@ provider. - **no Provider and no Lucid instance** — signing uses CML directly - **submission lives on the builder**, so the signer needs no egress either -`test/boundary.test.ts` enforces this over both the import graph and the manifest — it walks -every file under `src/`, checks `package.json` for network-capable dependencies, and greps for -ambient network globals (`fetch`, `WebSocket`, ...). It is the package's central security -guarantee. +`test/boundary.test.ts` checks this mechanically — it walks every file under `src/` for a +network-capable import or an ambient network global (`fetch`, `WebSocket`, ...), and checks +`package.json` for a network-capable dependency. That check is a text scan, not a sandbox, and +it is scoped accordingly: it reliably catches *accidental* regressions and *casual* misuse — an +added dependency, a forgotten forbidden import, an unthinking `fetch(...)` call, including one +routed around the bare identifier via `globalThis['fetch']` or `globalThis.fetch`. It does not +catch *deliberate* obfuscation. Verified directly against the hardened check: an ambient global +reached through a plain alias (`const f = fetch; f(...)`) or a forbidden module specifier +assembled at runtime from concatenated strings (`import('node:' + 'dns')`) both still pass. +Closing gaps like those is a code-review responsibility, not something a fixed set of text +patterns can guarantee — what this check actually verifies is that today's shipped `src/` has no +*accidental or careless* network capability, not a proof that none could ever be smuggled in. `test/integration/roundtrip.test.ts` imports `@lucid-evolution/lucid` and the shared package's -`/provider` — that does **not** weaken the guarantee above. That file lives under `test/`, +`/provider` — that does **not** weaken the check above. That file lives under `test/`, which `boundary.test.ts` deliberately scans only as far as `src/`, and which the build never bundles (`tsup`'s only entry point is `src/index.ts`). Those imports exist so the test can build a real transaction the way the hosted *builder* does (no key) before handing it to the diff --git a/packages/signer/test/boundary.test.ts b/packages/signer/test/boundary.test.ts index 91f7c03..39d545d 100644 --- a/packages/signer/test/boundary.test.ts +++ b/packages/signer/test/boundary.test.ts @@ -3,9 +3,23 @@ import assert from 'node:assert/strict'; import { readFileSync, readdirSync } from 'node:fs'; import { join } from 'node:path'; -// The signer's security claim is that it CANNOT reach the network — not that it -// chooses not to. These assertions are that claim, mechanically enforced. -// Weakening them defeats the purpose of the package. +// This is a text scan over src/'s import graph, package.json's declared +// dependencies, and a fixed list of ambient-global patterns — not a sandbox, +// and not a proof that no sequence of JavaScript could ever reach the +// network from this package. What it reliably catches: an accidentally +// added network-capable dependency, a forgotten forbidden import, an +// unthinking `fetch(...)` call — the shape of mistake code review can miss +// but a mechanical check cannot. What it does not catch: deliberate +// obfuscation. A reference to an ambient global reached through an alias +// (`const f = fetch; f(...)`) or a module specifier assembled at runtime +// from concatenated strings (`import('node:' + 'dns')`) defeats a text scan +// by construction — closing every such route is not a fixed-pattern problem, +// so it is not this file's job. Catching deliberate evasion is a code-review +// responsibility; this file's job is to make the accidental and the casual +// loud and immediate. See the README for the specific evasions verified +// against this file. Within that narrower, accurate scope, weakening these +// assertions — narrowing FORBIDDEN_IMPORTS, loosening a pattern, deleting a +// check — still defeats the purpose they do serve. const PKG_DIR = new URL('..', import.meta.url).pathname.replace(/^\/([A-Za-z]:)/, '$1'); const SRC_DIR = join(PKG_DIR, 'src'); @@ -16,13 +30,28 @@ const FORBIDDEN_IMPORTS = [ 'cross-fetch', 'node:http', 'node:https', + // Same network-transport class as http/https above, different protocol + // version. + 'node:http2', 'node:net', 'node:dgram', 'node:tls', + // A DNS lookup is network I/O in its own right, not merely a step before + // some other request — and a well-known covert exfiltration channel (DNS + // tunneling) even with no HTTP client available at all. + 'node:dns', + 'node:dns/promises', // Reaching the network by shelling out (execSync('curl ...')) bypasses every // other assertion here, so the process-spawning modules are forbidden too. 'node:child_process', 'child_process', + // Same reasoning as child_process just above: both are general + // code-execution primitives that can run a separate script (a worker's + // entry file, or a string handed to vm.Script/vm.runInNewContext) outside + // this file's own source text — precisely what an import-graph scan + // cannot see into. + 'node:worker_threads', + 'node:vm', // Opens a debugger port. 'node:inspector', ]; @@ -40,12 +69,42 @@ const ALLOWED_SHARED_SUBPATHS = [`${SHARED_PKG}/tx`, `${SHARED_PKG}/types`]; // scan above is structurally blind to their use. These patterns are matched with // word boundaries so they don't false-positive on legitimate identifiers like a // variable named `prefetchCount`. +// +// Every pattern below this comment requires the global's BARE identifier +// (`fetch`, `WebSocket`, ...) to appear, and most require it to be called or +// constructed immediately. Both are one-line evasions: `globalThis['fetch']` +// never spells the bare word `fetch` followed by `(`, and neither does +// `(0, globalThis.fetch)(...)` — the comma-operator trick (used to strip +// `this` binding so a borrowed method doesn't throw on an unexpected +// receiver) puts a `)` between `fetch` and the call parens that actually +// invoke it. Both still name `globalThis` and the target property, though — +// just via a different route to the same runtime object — so that route is +// checked for directly below: dot notation and bracket notation (the latter +// is literally "computed member access", the formal name for what +// `obj[expr]` is), covering both quoting styles quotedForms() above exists +// to handle. This still only recognises the literal text `globalThis` — a +// further level of indirection (aliasing globalThis to another name first, +// or building the property name from concatenated substrings the way +// FORBIDDEN_IMPORTS's own dynamic-`import()` gap allows, see the README) is +// the kind of arbitrary obfuscation this file does not attempt to defeat. +function globalThisQualifiedAccess(name: string): RegExp { + return new RegExp(`\\bglobalThis\\s*(?:\\.\\s*${name}\\b|\\[\\s*['"\`]${name}['"\`]\\s*\\])`); +} + const AMBIENT_NETWORK_GLOBALS: { label: string; pattern: RegExp }[] = [ { label: 'fetch(...)', pattern: /\bfetch\s*\(/ }, { label: 'new WebSocket', pattern: /\bnew\s+WebSocket\b/ }, { label: 'new EventSource', pattern: /\bnew\s+EventSource\b/ }, { label: 'new XMLHttpRequest', pattern: /\bnew\s+XMLHttpRequest\b/ }, { label: 'navigator.sendBeacon', pattern: /\bnavigator\s*\.\s*sendBeacon\b/ }, + { label: `navigator['sendBeacon']`, pattern: /\bnavigator\s*\[\s*['"`]sendBeacon['"`]\s*\]/ }, + { label: `globalThis.fetch / globalThis['fetch']`, pattern: globalThisQualifiedAccess('fetch') }, + { label: `globalThis.WebSocket / globalThis['WebSocket']`, pattern: globalThisQualifiedAccess('WebSocket') }, + { label: `globalThis.EventSource / globalThis['EventSource']`, pattern: globalThisQualifiedAccess('EventSource') }, + { + label: `globalThis.XMLHttpRequest / globalThis['XMLHttpRequest']`, + pattern: globalThisQualifiedAccess('XMLHttpRequest'), + }, ]; // Any module format that Node can execute, not just .ts — a stray .js or .mjs