fix: four findings from the multi-agent review (proto-safe writes, load-format null, import attrs)#18
Open
exo-nikita wants to merge 4 commits into
Open
fix: four findings from the multi-agent review (proto-safe writes, load-format null, import attrs)#18exo-nikita wants to merge 4 commits into
exo-nikita wants to merge 4 commits into
Conversation
1. Prototype-safe serialization: introduce `fromEntries` in util.js that returns a
null-prototype object. `Object.fromEntries` itself is prototype-safe, but later
`result[key] = val` on its output triggers the `__proto__` setter for malicious
keys. Swap all `Object.fromEntries` calls to the wrapper, and normalise
`module.files` to null-prototype on lockfile load so the subsequent
`module.files[rel] = integrity` assignment is safe regardless of the loaded JSON.
2. Document the bundle-authoritative cases (lock=ignore/none with bundle=load) in
config.js. The existing comment in state.js explained the runtime tautology;
this adds the policy rationale at the validation site.
3. Loosen the bundle=load load-hook assertion `format === context.format`: Node's
resolve chain leaves `context.format` as `null` (not `undefined`) when the
default resolver was used without populating it, which happens in node_modules
scope for non-nm parents resolving CJS deps. Only cross-check when the chain
actually provided a format. Added a CJS-in-nm-scope regression test that
reproduces the original failure ('commonjs' !== null) and pins the fix.
4. Include `importAttributes` in the addImport / getImport lookup key (NUL-separated
alongside the specifier), so two imports differing only in `with { type: 'json' }`
don't collapse to one map entry. wasm attributes remain rejected upstream.
There was a problem hiding this comment.
Pull request overview
This PR hardens Stasis lock/bundle serialization and fixes loader behavior around bundle loading and import attribute-aware resolution.
Changes:
- Adds null-prototype object serialization helpers and normalizes loaded lockfile module file maps.
- Relaxes bundle load format cross-checking when Node provides
null, and forwards import attributes through import lookup. - Adds a CJS node_modules bundle-load regression test and documents bundle-as-authority policy.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tests/cli.test.js |
Adds regression coverage for CJS node_modules deps under bundle=load. |
src/util.js |
Introduces null-prototype fromEntries and applies it to file map serialization. |
src/state.js |
Normalizes lockfile file maps, updates serialization, and keys imports by attributes. |
src/loader.js |
Adjusts format assertion behavior and threads import attributes through resolve/load paths. |
src/config.js |
Documents the bundle=load trust-root policy. |
Comments suppressed due to low confidence (1)
src/state.js:256
- The new import-attribute keying behavior has no regression test: existing tests cover
addImport/getImportand conditions, but none exercise attributes or the case this PR describes (same parent/specifier resolving differently with and withoutwith { type: 'json' }). Please add coverage so future changes cannot collapse those entries again.
addImport(parentURL, specifier, url, { conditions = '*', format, importAttributes } = {}) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1. #importKey: treat empty {} importAttributes the same as undefined. Node passes
`{}` (not undefined) when no attributes are used, and the previous code returned
`${specifier}\0` for empty vs `${specifier}` for undefined -- splitting equivalent
addImport/getImport calls. Now both produce the bare specifier.
2. loader.js load(): include 'json' alongside 'module' and 'commonjs' in the
bundle=load allowed-formats assertion. The PR records format='json' for
`with { type: 'json' }` imports, and a downstream `bundle=load` was tripping
the assertion. Added a CLI regression that bundles a JSON import and reloads
from the bundle with the on-disk JSON removed.
3. config.js comment fix: the lock=add/replace prohibition is about rewriting the
*lockfile* from bundle-loaded sources (which would defeat the lockfile's role
as the verifier), not about rewriting the bundle.
4. Regression tests:
- state.test.js: addImport with vs without {type:'json'} stores distinct entries
and resolves to different URLs; empty {} attributes match undefined attributes.
- cli.test.js: forged `__proto__` key in a lockfile must not poison
Object.prototype (runs a probe that exits 2 if `({}).polluted` is truthy).
Comment on lines
+126
to
+130
| // Recording with {} must collide with a prior recording with undefined attributes; | ||
| // noupsert lets the call pass only because the URL matches. | ||
| state.addImport(parentURL, './foo.js', childURL, { importAttributes: {} }) | ||
| const got = state.getImport(parentURL, './foo.js', { importAttributes: {} }) | ||
| t.assert.equal(got.url, childURL) |
Comment on lines
+580
to
+583
| // Inject a hostile entry: `__proto__` mapped to a "polluted":1 object. A vulnerable | ||
| // loader would later do `module.files['__proto__'] = integrity` and set the prototype, | ||
| // making `({}).polluted === 1` in the loader process. | ||
| lock.sources['.'].files.__proto__ = { polluted: 1 } |
Comment on lines
+586
to
+592
| // Run with a probe that crashes hard if Object.prototype.polluted is set. The lockfile | ||
| // is invalid (extra entry), but the failure mode must not be prototype pollution. | ||
| writeFileSync(join(tmp, 'src/probe.js'), | ||
| "if (({}).polluted !== undefined) { console.error('POLLUTED'); process.exit(2) }\n" | ||
| + "import { greet } from './hello.js'\nconsole.log(greet('world'))\n") | ||
|
|
||
| const r = run(['run', '--lock=frozen', '--full', 'src/probe.js'], { cwd: tmp }) |
Three of the comments were valid (test bugs); one was a structural concern that's
acknowledged but deferred (schema change), and the config.js comment overstated
what the lockfile actually verifies.
- state.test.js: rewrite the `{}` vs `undefined` equivalence test. The old version
used `{}` on both add and get sides, so a broken impl could insert and retrieve
under a separate key and the test would still pass. The new version reads back
the prior no-attributes record with `{}`, re-adds with `{}` to verify noupsert
collision, and inspects the specifier map directly to assert no second key
(`./foo.js\0` etc.) appeared.
- Replace the broken CLI `__proto__` test with a dedicated state-load fixture and
test. The old CLI test (a) assigned `obj.__proto__ = X` in JS, which triggers
the prototype setter rather than creating an own property -- so JSON.stringify
omitted it and the lockfile on disk had no forged key; and (b) added a probe
via a new entry `src/probe.js` that wasn't in the fixture lockfile, so
--lock=frozen would have rejected it before the probe could run. The new
state-load-proto fixture has a hand-written lockfile with `"__proto__"` as a
literal JSON property, and state-load-proto.test.js verifies that loading it
leaves Object.prototype untouched, that `module.files` is null-prototype, and
that the forged value lives as an own data property (not on the prototype).
- src/config.js: rewrite the bundle-load policy comment. The previous version
read as if `frozen` lockfiles verified the whole bundle, but the lockfile only
hashes source bytes -- the bundle's `formats` and `imports` metadata is
loaded without integrity coverage. Document that, and mark it TODO for a
follow-up that includes format/imports in the lockfile-attested data.
- src/loader.js: add a matching TODO at the load-hook format assertion noting
that `format` is taken from bundle metadata that the lockfile doesn't cover,
with the suggested fix (derive from extension + module.type recorded in the
lockfile).
Comment on lines
+36
to
45
| // bundle=load needs a trust root for source bytes: frozen pins each file's sha512 in | ||
| // the lockfile and we cross-check it on load; otherwise the bundle is itself | ||
| // authoritative -- lock=none with no lockfile on disk, or lock=ignore with a lockfile | ||
| // deliberately bypassed. Note: the lockfile only attests to source bytes; the bundle's | ||
| // formats/imports metadata is NOT covered by it today, so a tampered bundle can still | ||
| // flip e.g. commonjs <-> module for a hash-valid file (TODO: include format/imports in | ||
| // lockfile-verified data). add/replace are forbidden because the lockfile would be | ||
| // rewritten from bundle-loaded sources, attesting only to what the bundle already said. | ||
| if (this.#bundle === 'load' && this.#lock !== 'frozen' && this.#lock !== 'none' && this.#lock !== 'ignore') { | ||
| throw new RangeError('bundle=load requires lock=(frozen|none|ignore)') |
Comment on lines
+253
to
+254
| const parts = entries.sort(([a], [b]) => (a < b ? -1 : 1)) | ||
| return `${specifier}\0${parts.map(([k, v]) => `${k}=${v}`).join(',')}` |
| @@ -0,0 +1 @@ | |||
| { "name": "stasis-load-proto-fixture", "version": "0.0.0", "private": true } | |||
Comment on lines
+17
to
+22
| test('Object.prototype is not poisoned by a forged __proto__ key in lockfile.files', (t) => { | ||
| // A vulnerable loader would have assigned `module.files['__proto__'] = 'sha512-evil'` | ||
| // on a plain `{}`, making `({}).polluted` (or any inherited lookup) observable. We | ||
| // assert nothing leaked onto Object.prototype. | ||
| t.assert.equal(({}).polluted, undefined) | ||
| t.assert.equal(Object.prototype.__proto__, null) |
Comment on lines
122
to
+124
| for (const [dir, info] of Object.entries(json.sources)) { | ||
| assert.ok(!dir.includes('node_modules')) | ||
| this.modules.set(dir, info) | ||
| this.modules.set(dir, normalize(info)) |
Five new comments; four are valid and addressed, one is a re-raise of the
deferred format/imports trust gap.
1. (valid) state.js #importKey encoding is ambiguous when attribute values
contain `=` or `,` -- e.g. {type: 'a,b'} and {type: 'a', b: ''} would both
serialize to "type=a,b" / "type=a,b=" and could collide. Switch to
JSON.stringify of the sorted entries, which is unambiguous by construction.
Added a regression test in state.test.js that pins distinct delimiter-laden
attribute sets to distinct lookup results.
2. (valid) state-load-proto.test.js's first test was misleading -- JSON.parse
of a literal `"__proto__"` key creates an own data property, so the load
itself never invokes the prototype setter regardless of the fix. The
Object.prototype assertion would have passed even without the null-prototype
normalisation. Rewrote the test file to:
- narrow the load-time assertions to the null-prototype invariant on
`module.files`,
- cover BOTH the modules branch AND the full-scope sources branch (the PR
normalises in two separate places; a regression in the sources branch
would not have been caught by the prior fixture),
- exercise the actual unsafe write path by `delete`ing the forged entry and
then bracket-assigning a fresh value to `module.files['__proto__']`. On a
null-prototype object this creates an own data property; on a plain `{}`
the inherited __proto__ setter would either silently drop a string value
or rebind the prototype on an object value.
3. (valid) The fixture was missing the empty pnpm-workspace.yaml sentinel that
every other package-based fixture has, so State's root walk depended on the
repo root never gaining stasis files. Added the sentinel.
4. (valid) Strengthen config.js comment to be explicit about what frozen+load
does NOT verify: the bundle's `imports` map is also outside the lockfile's
attestation, so a tampered bundle can redirect a recorded import to a
different hash-valid file in the bundle. The format flip and the import
redirection are now both called out, with a single TODO covering both.
5. (deferred) The validator still allows lock=frozen + bundle=load despite the
metadata-trust gap. Documented as TODO in src/loader.js and src/config.js;
the structural fix (pin format and imports in lockfile-verified data) is
out of scope for this PR.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #16. Six Opus 4.7 reviewers (high-level, security, correctness, module-resolution, per-file, test-coverage) found a number of issues; this PR addresses the four you picked.
1. Prototype-safe serialization
Object.fromEntriesis itself prototype-safe (it usesCreateDataPropertyOrThrow), but the resulting object is a plain{}whose__proto__isObject.prototype— so a laterresult[key] = valtriggers the__proto__setter whenkey === '__proto__'. We do exactly that onmodule.filesafter loading it from the lockfile.fromEntrieswrapper inutil.jsthat returns a null-prototype object.Object.fromEntriescall (instate.jsandutil.js) to the wrapper.module.filesto null-prototype on lockfile load (both themodulesand the full-scopesourcesbranch) somodule.files[rel] = integrityis safe regardless of forged keys in the JSON.2. Document the bundle-as-authority cases
bundle=loadalready permitslock=ignoreandlock=none, andstate.getFilealready skips the hash check when no lockfile is in play. Added a comment at the validation site inconfig.jsexplaining the policy:bundle=loadneeds a trust root — either the lockfile (frozen) verifies the bundle, or the bundle is treated as self-attesting.add/replaceremain forbidden because consuming the bundle while rewriting it would be incoherent.3. The
format === context.formatassertion was a real bugThe module-resolution reviewer flagged it; I added a regression test before touching the assertion. The test (
run --lock=frozen --bundle=load roundtrips a CJS node_modules dep under node_modules scope) reproduces:The path: in
node_modulesscope with a non-nm parent (e.g.src/entry.js) importing a CJS dep, our resolve hook delegates tonextResolveso Node's default resolver runs. Node resolves the URL but leavescontext.formatasnull(notundefined) when handing off to the load hook. Our recorded format (commonjs) then tripsassert.equal(format, context.format).Fix: only cross-check when
context.format != null; otherwise trust our recorded format.4.
importAttributesin the import lookup keyTwo imports differing only by
with { type: 'json' }previously collapsed to one entry instate.imports, becauseaddImportkeyed by(conditions, parent, specifier)only. ThreadedimportAttributesthroughaddImport/getImport(and the loader'sresolvehook), and built the lookup key asspecifier\0type=…when attributes are present. wasm attributes remain rejected upstream inloader.jsper scope.Tests
pnpm testandpnpm lintboth green.https://claude.ai/code/session_012kvoSopfMtM2ke4EpCRkpF
Generated by Claude Code