fix(decompress): a zip symlink must not point outside the destination

[ofri-peretz]

The bug

unzip() creates symbolic links straight from archive data:

const linkTarget = Buffer.concat(chunks).toString("utf8").trim();
...
await fs.promises.symlink(linkTarget, entryPathAbs);

The link's own name is safe — yauzl-promise refuses an entry called ../outside before this module ever sees it. I checked that rather than assuming it, and it throws Error: Relative path: ../outside.txt.

The link's target is checked by nobody. An archive can drop a link inside cacheDir that points anywhere on disk, and anything later written through that link lands outside cacheDir. That is the symlink variant of zip slip, CWE-22.

It matters here because decompress() runs on binaries this package downloads — main.js calls it for the NW.js, ffmpeg and node archives — so the extracted content is only as trustworthy as the mirror it came from and the cache directory it sat in.

Scope, honestly

I want to be straight about how much of this was actually exposed, because the first read looked worse than it is. Running the new tests against unmodified main:

case	unfixed	fixed
entry name ../outside.txt	rejected — by yauzl, not by this code	rejected
absolute entry name	rejected — by yauzl	rejected
symlink whose target escapes	link is created ❌	rejected ✅
ordinary nested entry	extracted ✅	extracted ✅

So one real hole, not four. The entry-name cases are yauzl's guarantee, not this package's.

The fix

function resolveWithin(root, entryName) {
  const rootAbs = path.resolve(root);
  const target = path.resolve(rootAbs, entryName);
  if (target !== rootAbs && !target.startsWith(rootAbs + path.sep)) {
    throw new Error(`Refusing to extract ${JSON.stringify(entryName)}: it resolves outside the destination directory.`);
  }
  return target;
}

Applied in three places — file entries, symlink entry names, and the resolved symlink target. The third is the fix; the first two are defence in depth, so a future change of zip backend cannot quietly remove the guarantee we currently inherit.

Resolving before checking is what makes it total: it also catches absolute names and .. buried mid-path. The trailing path.sep matters too — without it /tmp/cache-evil would pass as being inside /tmp/cache.

tar.extract is left alone: node-tar v7 already strips .. and absolute paths unless preservePaths is set, and it is not set here.

Tests

tests/specs/decompress.zip-slip.test.js — four cases. It builds real STORE-method zips inline (including a small CRC-32) rather than adding a dev dependency just to write four archives, which seemed the wrong trade for a test file.

• npm test — the four new tests pass; src/decompress.js coverage is 96.40% lines / 89.47% branches.
• npm run lint — clean.
• The pre-existing decompress.test.js fails in my checkout with ENOENT ... cache/nwjs-v0.107.0-osx-arm64.zip — it expects an archive a prior download step puts there. Unrelated to this change, but I would rather say so than let it look like I ignored a red suite.

Note

I found this while running a set of security lint rules across a sample of npm packages, so I have no prior involvement with this project — happy to adjust the shape of the fix or the tests to whatever suits the codebase.

[ayushmanchhabra]

Thanks, will review in detail shortly

[ayushmanchhabra]

@ofri-peretz could you run npm audit fix, npm run format and npm run lint? Otherwise good from my end.

Maintainer making changes

[ofri-peretz]

Done — all three are green on 9759a59:

• npm audit — 0 vulnerabilities
• npm run format — all matched files use Prettier code style
• npm run lint — 0 errors (exits 0)

Credit where it's due: your 74bdfbe and the dep bumps did most of that. I rebased onto the current tip and re-ran everything against the updated toolchain (eslint 10.9.1, jsdoc 64.2.1) rather than my older checkout.

npm test is 12 pass / 1 fail, the failure being the pre-existing cache/nwjs-v0.107.0-osx-arm64.zip ENOENT I mentioned in the description — it expects an archive a prior download step puts there, unrelated to this change.

---

One addition you did not ask for, so please push back freely.

That same commit also adds two eslint plugins — eslint-plugin-node-security and eslint-plugin-secure-coding. I maintain both (published from github.com/ofri-peretz/eslint), so I want that stated plainly rather than buried in a lockfile diff. The security fix in this PR does not depend on them, and I will drop the commit without argument if you would rather not take the dependency.

What it does, if it is of interest:

Rules are enabled individually, not via either plugin's recommended set. This package fetches a URL, writes it to disk and unpacks it — most of the 48 recommended rules cover surface that does not exist here (SQL, GraphQL, LDAP, auth, session). 17 apply; the rest would just be noise.

It found one real thing: the SIGINT handler in request.js did existsSync then unlinkSync, which is a check-then-act race (CWE-367) — the file can vanish between the two calls. Now it unlinks unconditionally and swallows ENOENT.

Two findings I suppressed rather than "fixed", with the reasoning inline at each site:

• no-zip-slip reports both extraction call sites in decompress(). Both are safe — every entry unzip() writes goes through resolveWithin(), and node-tar v7 strips .. and absolute paths unless preservePaths is set, which it is not. The rule reports the call site rather than the write site, so it cannot see the guard from there. That is a limitation in my rule, not something wrong with this code; I disabled per-line so a future change still gets checked, and I will fix the rule upstream.

• no-unlimited-resource-allocation is left at warn rather than silenced. Decompression bounds are a genuine open question here — a zip bomb from a compromised mirror is unbounded — and turning it off would hide that.

Also noticed src/util.js from 74bdfbe was unreferenced; looks like it was already removed in 99538c0.

[ayushmanchhabra]

Thanks, I'll keep the plugins for now and see what it turns up, interesting concept of using Eslint plugins for static analysis