fix(signer): harden the boundary check's globalThis blind spot, and stop overstating it
The review verified six obfuscations that bypass boundary.test.ts's ambient-
global and forbidden-import checks: globalThis['fetch'](...), an alias
(const f = fetch; f(...)), an aliased bracket reference, the comma-operator
trick ((0, globalThis.fetch)(...)), and two dynamic import()s with a
concatenated specifier (import('node:' + 'dns'), import('child_' +
'process')). README.md called the check "the package's central security
guarantee" and boundary.test.ts's header claimed the signer "CANNOT reach
the network -- not that it chooses not to." Neither line was true of what a
text scan can actually promise.
Hardened cheaply, within reach of a text scan:
- FORBIDDEN_IMPORTS gains node:dns, node:dns/promises, node:http2,
node:worker_threads, node:vm, each with a one-line rationale.
- AMBIENT_NETWORK_GLOBALS gains globalThisQualifiedAccess(name), matching a
global reached via globalThis in either dot or bracket ("computed member
access") notation, plus navigator['sendBeacon'] bracket notation.
Verified empirically against the real hardened check (temporary probe
files under src/, one per evasion, deleted immediately after -- see the
task report for the full per-file breakdown): globalThis['fetch'](...),
the aliased-bracket-reference variant, and the comma-operator
globalThis.fetch trick are now all caught. The plain-identifier alias and
both concatenated dynamic imports still pass -- those need real data-flow
tracking or evaluating an arbitrary runtime expression, neither of which a
text scan can do, and no attempt was made to chase them.
Reworded both the README section and boundary.test.ts's header comment to
say what the check actually verifies: today's shipped src/ has no
accidental or careless network capability. Deliberate obfuscation is named
as a code-review responsibility, not something this file promises to catch.
Final whole-branch review, Fix 4 of 4.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user