fix(bundle,lockfile): require every flat file key to be canonical

stasis-core/src/artifact-util.js

@@ -261,6 +261,26 @@ export function mergeExecutableSets(a, b, bModules, scope) {
   return out
 }
 
+// An empty, '.' or '..' path segment.
+const NON_CANONICAL_SEGMENT = /(?:^|\/)\.{0,2}(?:\/|$)/u
+
+// Maps each file's flat key to its bucket; rejects non-canonical keys, reports duplicates to onDuplicate.
+export function flatFileKeys(modules, what, onDuplicate) {
+  const owners = new Map()
+  for (const [dir, { files }] of modules) {
+    if (typeof dir !== 'string') assert(false, `${what}: bucket dir ${String(dir)} is not a string`)
+    for (const rel of Object.keys(files)) {
+      const key = moduleFileKey(dir, rel)
+      // Messages built only on failure: this loop visits every file.
+      if (key !== '.' && NON_CANONICAL_SEGMENT.test(key)) assert(false, `${what}: non-canonical file key ${JSON.stringify(key)}`)
+      const owner = owners.get(key)
+      if (owner !== undefined) onDuplicate(key, owner, dir)
+      owners.set(key, dir)
+    }
+  }
+  return owners
+}
+
 // Result `files` objects are null-prototype, so a `__proto__` file name is a plain own key.
 export function mergeModuleMaps(a, b, label) {
   const out = new Map()
@@ -302,21 +322,10 @@ export function mergeModuleMaps(a, b, label) {
   // releases (a versionless workspace package used to fall through to a parent bucket and now owns
   // its own), and per-dir absorption cannot see that: without this check the merge would WRITE an
   // artifact that then fails its own next parse on the duplicate-file-key guard.
-  const owners = new Map()
-  for (const [dir, { files }] of out) {
-    for (const rel of Object.keys(files)) {
-      const key = moduleFileKey(dir, rel)
-      const owner = owners.get(key)
-      if (owner !== undefined) {
-        // Message built only on failure: this loop visits every merged file.
-        assert(false,
-          `${label}: file '${key}' is bucketed under both '${owner}' and '${dir}' -- module bucketing ` +
-          `changed between the artifacts (a workspace package without a version now owns its own ` +
-          `bucket); regenerate the artifact (bundle=replace / lock=replace)`)
-      }
-      owners.set(key, dir)
-    }
-  }
+  flatFileKeys(out, label, (key, owner, dir) => assert(false,
+    `${label}: file '${key}' is bucketed under both '${owner}' and '${dir}' -- module bucketing ` +
+    `changed between the artifacts (a workspace package without a version now owns its own ` +
+    `bucket); regenerate the artifact (bundle=replace / lock=replace)`))
   return out
 }
 

stasis-core/src/bundle.js

@@ -4,6 +4,7 @@ import {
   fileMapToObject,
   fileSetToObject,
   fromEntries,
+  flatFileKeys,
   hasNodeModulesSegment,
   isPlainObject,
   mergeFormatMaps,
@@ -29,6 +30,10 @@ const normalize = ({ name, version, ecosystem, files }) => {
   return { name, version: version ?? undefined, ...(ecosystem === undefined ? {} : { ecosystem }), files: fromEntries(Object.entries(files)) }
 }
 
+const duplicateKey = (key) => assert(false, `duplicate file key '${key}' across bundle buckets -- module bucketing ` +
+  `changed between writes (a workspace package without a version now owns its own ` +
+  `bucket); regenerate the artifact (bundle=replace)`)
+
 const inferModuleDir = (path) =>
   splitNodeModulesPath(path) ?? { dir: '.', rel: path, name: null }
 
@@ -152,10 +157,6 @@ export class Bundle {
         assert(json.entries === undefined)
         assert(json.sources === undefined)
       }
-      for (const [, { files }] of modules) {
-        // posixPathEscapes (not a '..' prefix): also catches mid-path escapes and absolute paths.
-        for (const rel of Object.keys(files)) assert(!posixPathEscapes(rel))
-      }
     } else {
       assert(json.sources)
       for (const [path, content] of Object.entries(json.sources)) {
@@ -168,19 +169,7 @@ export class Bundle {
     }
 
     // Flat keys must be unique across buckets: two different bucket splits can flatten to one path, and the `sources` getter would serve either payload.
-    const flatKeys = new Set()
-    for (const [dir, { files }] of modules) {
-      for (const rel of Object.keys(files)) {
-        const key = moduleFileKey(dir, rel)
-        if (flatKeys.has(key)) {
-          // Message built only on failure: this loop visits every bundled file.
-          assert(false, `duplicate file key '${key}' across bundle buckets -- module bucketing ` +
-            `changed between writes (a workspace package without a version now owns its own ` +
-            `bucket); regenerate the artifact (bundle=replace)`)
-        }
-        flatKeys.add(key)
-      }
-    }
+    const flatKeys = flatFileKeys(modules, 'bundle', duplicateKey)
 
     // Reject paths escaping the root here (incl. mid-path `a/../../x`): getImport resolves against the root at load.
     const imports = objectToMaps(json.imports)
@@ -237,6 +226,8 @@ export class Bundle {
   }
 
   serialize() {
+    // Never write an artifact that parse would reject.
+    flatFileKeys(this.modules, 'bundle', duplicateKey)
     const entries = fileSetToObject(this.entries)
     const { modules, sources } = this.#groupedFromModules()
     const formats = fileMapToObject(this.formats)

stasis-core/src/lockfile.js

@@ -1,7 +1,11 @@
-import { KNOWN_FORMATS, assert, serializeExecutable, fileMapToObject, fileSetToObject, fromEntries, hasNodeModulesSegment, isPlainObject, mergeExecutableSets, mergeFormatMaps, mergeImportMaps, mergeModuleMaps, moduleFileKey, moduleFileKeys, parseExecutable, posixPathEscapes, sortPaths } from './artifact-util.js'
+import { KNOWN_FORMATS, assert, serializeExecutable, fileMapToObject, fileSetToObject, fromEntries, flatFileKeys, hasNodeModulesSegment, isPlainObject, mergeExecutableSets, mergeFormatMaps, mergeImportMaps, mergeModuleMaps, parseExecutable, posixPathEscapes, sortPaths } from './artifact-util.js'
 
 const VERSION = 0
 
+const duplicateKey = (key) => assert(false, `duplicate file key '${key}' across lockfile buckets -- module bucketing ` +
+  `changed between writes (a workspace package without a version now owns its own ` +
+  `bucket); regenerate the lockfile (lock=replace)`)
+
 const normalize = ({ name, version, ecosystem, files }) => {
   assert(ecosystem === undefined || typeof ecosystem === 'string')
   // An absent version has one spelling: null (hand-edited or legacy JSON) folds into undefined so
@@ -63,22 +67,11 @@ export class Lockfile {
 
     // Flat keys must be unique across buckets (mirrors Bundle.parse): two bucket splits can flatten
     // to one path, and hashes/attestation lookups key on the flat path.
-    const flatKeys = new Set()
     for (const [dir, { files }] of modules) {
       assert(!posixPathEscapes(dir))
       assert(files)
-      for (const name of Object.keys(files)) {
-        assert(!posixPathEscapes(name))
-        const key = moduleFileKey(dir, name)
-        if (flatKeys.has(key)) {
-          // Message built only on failure: this loop visits every attested file.
-          assert(false, `duplicate file key '${key}' across lockfile buckets -- module bucketing ` +
-            `changed between writes (a workspace package without a version now owns its own ` +
-            `bucket); regenerate the lockfile (lock=replace)`)
-        }
-        flatKeys.add(key)
-      }
     }
+    const flatKeys = flatFileKeys(modules, 'lockfile', duplicateKey)
 
     assert(isPlainObject(json.imports))
     const imports = new Map()
@@ -123,15 +116,17 @@ export class Lockfile {
       formats.set(key, format)
     }
 
-    // Every executable must be a file this lockfile attests; the key index is built only when the (usually absent) key is present.
+    // Every executable must be a file this lockfile attests.
     const executable = json.executable === undefined
       ? new Set()
-      : parseExecutable(json.executable, { what: 'lockfile', files: moduleFileKeys(modules), formats, scope: json.config.scope })
+      : parseExecutable(json.executable, { what: 'lockfile', files: flatKeys, formats, scope: json.config.scope })
 
     return new Lockfile({ config: json.config, entries, modules, imports, formats, executable })
   }
 
   serialize() {
+    // Never write an artifact that parse would reject.
+    flatFileKeys(this.modules, 'lockfile', duplicateKey)
     const entries = fileSetToObject(this.entries)
     const modules = []
     const sources = []

tests/public-exports.test.js

@@ -328,6 +328,54 @@ test('Bundle.parse rejects a v0 flat source path that escapes the project root',
   t.assert.throws(() => Bundle.parse(v0('/etc/passwd')))
 })
 
+test('Bundle.parse and Lockfile.parse reject a non-canonical file key', (t) => {
+  // An empty, '.' or '..' segment spells a file more than one way, or makes its key absolute.
+  const sources = (spec, value) => Object.fromEntries(Object.entries(spec).map(([dir, names]) =>
+    [dir, { name: 'x', version: '1.0.0', files: Object.fromEntries(names.map((name) => [name, value])) }]))
+  const bundle = (spec) => JSON.stringify({
+    version: 1, config: { scope: 'full' }, entries: [], sources: sources(spec, 'x'), formats: {}, imports: {},
+  })
+  const lockfile = (spec) => JSON.stringify({
+    version: 0, config: { scope: 'full' }, entries: [], sources: sources(spec, 'sha512-x'), modules: {}, formats: {}, imports: {},
+  })
+  const rejected = [
+    { '': ['etc/passwd'] },
+    { '': [''] },
+    ...['src/', 'src/.', './src', 'src//lib', 'a/../src'].map((dir) => ({ [dir]: ['x.js'] })),
+    ...['./x.js', 'a/../x.js', 'x//y.js', '/x.js', 'x/'].map((name) => ({ '.': [name] })),
+    { '.': ['src/x.js'], src: ['./x.js'] },
+  ]
+  for (const spec of rejected) {
+    t.assert.throws(() => Bundle.parse(bundle(spec)), /non-canonical file key/, JSON.stringify(spec))
+    t.assert.throws(() => Lockfile.parse(lockfile(spec)), /non-canonical file key/, JSON.stringify(spec))
+  }
+  const v0 = (paths) => JSON.stringify({
+    version: 0, config: { scope: 'full' }, formats: {}, imports: {}, sources: Object.fromEntries(paths.map((path) => [path, 'x'])),
+  })
+  t.assert.throws(() => Bundle.parse(v0(['src/x.js', './src/x.js'])), /non-canonical file key/)
+  // Dot-names are ordinary names.
+  const names = ['x.js', '..foo', '...', '.pnpm/x', 'node_modules/.bin/x']
+  const accepted = { '.': names, src: ['a/b.js'] }
+  t.assert.deepStrictEqual([...Bundle.parse(bundle(accepted)).sources.keys()], [...names, 'src/a/b.js'])
+  t.assert.deepStrictEqual(Object.keys(Lockfile.parse(lockfile(accepted)).modules.get('.').files), names)
+})
+
+test('Bundle and Lockfile refuse to merge or serialize what their parse rejects', (t) => {
+  const absolute = () => new Map([['', { name: 'x', version: '1.0.0', files: { 'etc/passwd': 'x' } }]])
+  const duplicate = () => new Map([
+    ['.', { name: 'a', version: '1.0.0', files: { 'src/x.js': 'x' } }],
+    ['src', { name: 's', version: '1.0.0', files: { 'x.js': 'y' } }],
+  ])
+  const full = { config: { scope: 'full' } }
+  t.assert.throws(() => new Bundle({ modules: absolute() }).serialize(), /non-canonical file key "\/etc\/passwd"/)
+  t.assert.throws(() => new Bundle({ modules: duplicate() }).serialize(), /duplicate file key 'src\/x\.js'/)
+  t.assert.throws(() => new Bundle().merge(new Bundle({ modules: absolute() })), /bundle merge: non-canonical file key/)
+  t.assert.throws(() => new Lockfile({ ...full, modules: absolute() }).serialize(), /non-canonical file key/)
+  t.assert.throws(() => new Lockfile({ ...full, modules: duplicate() }).serialize(), /duplicate file key 'src\/x\.js'/)
+  t.assert.throws(() => new Lockfile(full).merge(new Lockfile({ ...full, modules: absolute() })), /non-canonical file key/)
+  t.assert.throws(() => new Bundle({ modules: new Map([[1, { name: 'x', files: { a: 'b' } }]]) }).serialize(), /bucket dir 1 is not a string/)
+})
+
 test('Bundle.parse keeps accepting a directory listing keyed at a module root (rel === \'\')', (t) => {
   // `stasis run --fs` keys a readdir of a package root at rel '' inside its own
   // bucket (moduleFileKey collapses that to the dir itself, see fs.test.js).