fix(#4120): replace shellcheck npm dep with dependency-free downloader (#4121)

* fix(#4120): replace shellcheck npm dep with dependency-free downloader

The `shellcheck` devDependency (added in #4109) pulled in decompress@4.2.1
for archive extraction, which carries an unpatched CRITICAL zip-slip
vulnerability (GHSA-mp2f-45pm-3cg9, CVSS 9.1) plus two moderate findings.
decompress's latest published version IS the vulnerable one -- no patched
release exists upstream, so npm audit fix cannot resolve this by upgrading.

Removes the shellcheck package entirely and replaces its role with
scripts/lib/shellcheck-fetch.cjs: a small downloader using only Node's
built-in https/zlib plus a hand-written tar-entry reader, fetching a pinned
koalaman/shellcheck release directly from GitHub releases. The reader never
uses an archive-supplied name as a filesystem path (the exact defect class
decompress had) -- it only returns the matched entry's bytes; the caller
writes those bytes to a path it constructs itself. Bounds the download with
a 30s-per-hop timeout, consistent with the ShellCheck subprocess's own
timeout. Covers linux/darwin on x86_64/aarch64, matching this repo's actual
CI (lint-tests runs only on ubuntu-latest) and local dev needs; Windows
fails with a clear, honest error rather than silently misbehaving.

Adds tests/lint-workflow-shellcheck-fetch.test.cjs covering the tar-parser
(unit cases plus a fast-check property test per CLAUDE.md's parser-testing
requirement), a security behavioral pin confirming traversal-style entry
names are treated as opaque strings never filesystem paths, and boundary
coverage for the redirect-following logic's MAX_REDIRECTS limit
(limit-1/limit/limit+1, via an injectable transport, no real network I/O).

npm audit: 0 vulnerabilities (was 1 critical + 5 moderate). The lint script
reproduces the identical result against the current tree:
"212 pre-existing finding(s) from baseline, 0 new" -- no behavior
regression, no baseline changes needed.

* fix(#4120): register shellcheck-fetch.cjs with the installer

scripts/lib/shellcheck-fetch.cjs shipped without being added to
GSD_SCRIPTS_LIB_FILES in bin/install.js, which would have left it orphaned
on uninstall and broken the golden install-tree fixtures for every runtime.
Adds the entry and regenerates the 19 affected fixtures via
npm run gen:install-tree.

* docs(#4120): add changeset for the decompress CVE fix

---------

Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
2026-08-31 22:31:23 -04:00
committed by GitHub
parent 8c9265d4e5
commit cad70f4f3e
26 changed files with 646 additions and 1417 deletions

View File

@@ -0,0 +1,5 @@
---
type: Security
pr: 4121
---
**Removed a critical unpatched supply-chain vulnerability from the `lint:ci` toolchain** — the `shellcheck` devDependency pulled in `decompress@4.2.1`, which carries an unpatched critical zip-slip flaw (GHSA-mp2f-45pm-3cg9); replaced with a small dependency-free downloader that fetches a pinned ShellCheck release directly and extracts it without the vulnerable extraction library. (#4120)

View File

@@ -411,7 +411,7 @@ const GSD_CHANGESET_FILES = [
'github-release-notes.cjs', 'lint.cjs', 'new.cjs',
'README.md', // documentation only — not user-authored
];
const GSD_SCRIPTS_LIB_FILES = ['cli-exit.cjs', 'allowlist-ratchet.cjs', 'drift-scan.cjs', 'alias-drift-families.cjs', 'exit-code-registry.cjs', 'ndjson-reporter.cjs', 'ci-job-timing.cjs'];
const GSD_SCRIPTS_LIB_FILES = ['cli-exit.cjs', 'allowlist-ratchet.cjs', 'drift-scan.cjs', 'alias-drift-families.cjs', 'exit-code-registry.cjs', 'ndjson-reporter.cjs', 'ci-job-timing.cjs', 'shellcheck-fetch.cjs'];
/**
* Resolve a runtime's shared-hooks directory name from its descriptor.

1351
package-lock.json generated

File diff suppressed because it is too large Load Diff

View File

@@ -78,7 +78,6 @@
"js-yaml": "^4.3.1",
"mutation-testing-metrics": "^3.7.3",
"re2js": "^2.8.6",
"shellcheck": "^4.1.0",
"typescript": "^6.0.3",
"typescript-eslint": "^8.60.0"
},

View File

@@ -0,0 +1,247 @@
'use strict';
/**
* shellcheck-fetch.cjs
*
* Dependency-free replacement for the `shellcheck` npm package (removed in
* #4120 — its extraction step pulled in `decompress@4.2.1`, which carries an
* unpatched CRITICAL zip-slip vulnerability, GHSA-mp2f-45pm-3cg9, CVSS 9.1,
* plus two moderate findings, with no patched version available upstream).
*
* This module fetches a PINNED koalaman/shellcheck release directly from
* GitHub releases (never "latest" — see SHELLCHECK_VERSION below), extracts
* the single `shellcheck` binary from the release's `.tar.gz` asset using
* only Node's built-in `https`/`zlib` modules plus a small hand-written tar
* reader (no third-party archive library), and caches the extracted binary
* for reuse across runs.
*
* Zip-slip defense: unlike `decompress`, which wrote extracted files using
* PATHS TAKEN FROM THE ARCHIVE (the exact defect class in GHSA-mp2f-45pm-
* 3cg9 — a malicious archive entry named e.g. `../../etc/passwd` gets
* written there verbatim), this reader NEVER uses an archive-supplied name
* as a filesystem path. `extractFileFromTar` only ever returns the matched
* entry's raw byte content; the caller (`resolveShellcheckBin`) writes those
* bytes to a path it constructs itself (`<cacheDir>/shellcheck`), and the
* archive's own name field is used only for a string-equality/suffix CHECK
* (`name === targetName || name.endsWith('/' + targetName)`), never
* interpolated into a path passed to `fs.writeFileSync`/`fs.mkdirSync`/etc.
*/
const fs = require('node:fs');
const path = require('node:path');
const zlib = require('node:zlib');
const https = require('node:https');
const { ExitError } = require('./cli-exit.cjs');
// Pinned explicitly — verified against `koalaman/shellcheck`'s GitHub
// releases API as the current latest tag at the time this was written
// (2026-08-31). Never resolved dynamically ("latest") — a moving target
// would make this lint's exact ShellCheck version, and therefore its exact
// finding set against scripts/lint-workflow-shellcheck-baseline.json,
// non-reproducible across runs/machines/CI.
const SHELLCHECK_VERSION = 'v0.11.0';
const ROOT = path.join(__dirname, '..', '..');
const CACHE_DIR = path.join(ROOT, 'node_modules', '.cache', 'shellcheck', SHELLCHECK_VERSION);
const CACHED_BIN_PATH = path.join(CACHE_DIR, 'shellcheck');
// process.arch -> the arch token ShellCheck's release asset names use.
// Only the two architectures that actually matter for this repo (per
// .github/workflows/test.yml: the lint-tests job that runs this script only
// runs on ubuntu-latest, which is x86_64; and Apple Silicon dev machines are
// aarch64) are supported — anything else fails with a clear error rather
// than guessing.
const ARCH_MAP = { x64: 'x86_64', arm64: 'aarch64' };
const MAX_REDIRECTS = 5;
// Bounds each individual HTTP hop (the initial request AND every redirect
// hop get their own fresh 30s budget, rather than one shared budget across
// the whole redirect chain) — a stalled connection on any single hop is
// caught in a bounded time, mirroring lint-workflow-shellcheck.cjs's own
// SHELLCHECK_TIMEOUT_MS bound on the ShellCheck subprocess. A bare `timeout`
// option on the request does NOT abort it by itself — Node only emits a
// 'timeout' event, which must be handled by destroying the request (see the
// `req.on('timeout', ...)` below).
const DOWNLOAD_TIMEOUT_MS = 30_000;
/**
* Issue one real HTTPS request. Exists as its own function purely so tests
* can inject a fake in its place (see `httpsGetFollowingRedirects`'s
* `requestFn` parameter) — production callers never pass an override, so
* the real download path always uses this exact implementation.
*/
function defaultRequestFn(url, options, callback) {
return https.get(url, options, callback);
}
/**
* GET `url` following HTTP redirects manually — `https.get` does NOT follow
* redirects automatically, and GitHub release asset URLs redirect through
* `objects.githubusercontent.com`. Resolves with the full response body as a
* Buffer once a 200 response is received.
*
* `requestFn` defaults to a real `https.get`-based transport
* (`defaultRequestFn`) and is only ever overridden in tests, so calling this
* with zero/one arg from `resolveShellcheckBin` is unchanged behavior.
*/
function httpsGetFollowingRedirects(url, redirectsLeft = MAX_REDIRECTS, requestFn = defaultRequestFn) {
return new Promise((resolve, reject) => {
const req = requestFn(
url,
{ headers: { 'User-Agent': 'gsd-core-shellcheck-fetch' }, timeout: DOWNLOAD_TIMEOUT_MS },
(res) => {
const status = res.statusCode || 0;
if (status >= 300 && status < 400 && res.headers.location) {
res.resume(); // drain so the socket can be reused/closed
if (redirectsLeft <= 0) {
reject(new Error(`too many redirects fetching ${url}`));
return;
}
const next = new URL(res.headers.location, url).toString();
httpsGetFollowingRedirects(next, redirectsLeft - 1, requestFn).then(resolve, reject);
return;
}
if (status !== 200) {
res.resume();
reject(new Error(`unexpected HTTP ${status} fetching ${url}`));
return;
}
const chunks = [];
res.on('data', (chunk) => chunks.push(chunk));
res.on('end', () => resolve(Buffer.concat(chunks)));
res.on('error', reject);
},
);
req.on('error', reject);
// `timeout` in the options above only ARMS a timer — Node emits a
// 'timeout' event on the request but does not abort it. Without this
// handler the request (and this Promise) would hang indefinitely past
// the configured bound on a stalled connection.
req.on('timeout', () => {
req.destroy(new Error(`timed out after ${DOWNLOAD_TIMEOUT_MS}ms fetching ${url}`));
});
});
}
/**
* Find the entry named (or path-ending-in) `targetName` inside a raw
* (already gunzipped) POSIX tar byte stream and return its content as a
* Buffer, or `null` if not found.
*
* Tar format: a sequence of 512-byte headers — name at offset 0/length 100,
* size at offset 124/length 12 (octal ASCII), typeflag at offset 156 — each
* followed by that many content bytes, padded up to the next 512-byte
* boundary, terminated by an all-zero 512-byte block. This deliberately
* implements only enough to locate ONE known entry name (no general
* multi-file extraction, no symlink handling, no GNU long-name `@LongLink`
* entries — ShellCheck's own release tarballs never need them) — see this
* module's header comment for why the entry's NAME is never used as a
* filesystem path.
*/
function extractFileFromTar(buffer, targetName) {
let offset = 0;
while (offset + 512 <= buffer.length) {
const header = buffer.subarray(offset, offset + 512);
if (header.every((b) => b === 0)) break; // end-of-archive marker
const name = header.subarray(0, 100).toString('utf8').replace(/\0.*$/, '');
const sizeRaw = header.subarray(124, 136).toString('utf8').replace(/\0.*$/, '').trim();
const size = sizeRaw === '' ? 0 : parseInt(sizeRaw, 8);
const typeflag = String.fromCharCode(header[156]);
const dataStart = offset + 512;
const isRegularFile = typeflag === '0' || typeflag === '\0';
if (isRegularFile && (name === targetName || name.endsWith(`/${targetName}`))) {
return buffer.subarray(dataStart, dataStart + size);
}
const contentBlocks = Math.ceil(size / 512);
offset = dataStart + contentBlocks * 512;
}
return null;
}
/**
* Resolve the local path to a working, executable `shellcheck` binary,
* downloading and caching the pinned release on first use. Subsequent calls
* (same version) reuse the cached binary with no network activity — mirrors
* the `fs.accessSync(bin, F_OK | X_OK)` cache-check pattern already used by
* this script's own `runShellcheck`.
*/
async function resolveShellcheckBin() {
try {
fs.accessSync(CACHED_BIN_PATH, fs.constants.F_OK | fs.constants.X_OK);
return CACHED_BIN_PATH;
} catch {
// not cached yet — fall through to download
}
if (process.platform === 'win32') {
throw new ExitError(
1,
'lint-workflow-shellcheck: automatic ShellCheck download is not supported on Windows yet ' +
'(this lint only ever runs in the ubuntu-latest lint-tests CI job — see .github/workflows/test.yml — ' +
'so this is an honest platform gap, not expected to be hit in CI).',
);
}
const platform = process.platform === 'darwin' || process.platform === 'linux' ? process.platform : null;
if (!platform) {
throw new ExitError(
1,
`lint-workflow-shellcheck: unsupported platform '${process.platform}' for ShellCheck auto-download ` +
`(supported: linux, darwin).`,
);
}
const arch = ARCH_MAP[process.arch];
if (!arch) {
throw new ExitError(
1,
`lint-workflow-shellcheck: unsupported architecture '${process.arch}' for ShellCheck auto-download ` +
`(supported: x86_64 [node arch 'x64'], aarch64 [node arch 'arm64']).`,
);
}
const assetName = `shellcheck-${SHELLCHECK_VERSION}.${platform}.${arch}.tar.gz`;
const url = `https://github.com/koalaman/shellcheck/releases/download/${SHELLCHECK_VERSION}/${assetName}`;
let gz;
try {
gz = await httpsGetFollowingRedirects(url);
} catch (e) {
throw new ExitError(1, `lint-workflow-shellcheck: failed to download ShellCheck (${url}): ${e.message}`);
}
let tarBuf;
try {
tarBuf = zlib.gunzipSync(gz);
} catch (e) {
throw new ExitError(1, `lint-workflow-shellcheck: failed to gunzip downloaded ShellCheck archive: ${e.message}`);
}
const entry = extractFileFromTar(tarBuf, 'shellcheck');
if (!entry) {
throw new ExitError(
1,
`lint-workflow-shellcheck: could not find a 'shellcheck' entry inside downloaded archive ${assetName}`,
);
}
fs.mkdirSync(CACHE_DIR, { recursive: true });
// Write to a per-process temp path and rename into place — avoids any
// other concurrent invocation observing (and trying to execute) a
// partially-written binary at the real cache path.
const tmpPath = path.join(CACHE_DIR, `.shellcheck.tmp-${process.pid}`);
fs.writeFileSync(tmpPath, entry);
fs.chmodSync(tmpPath, 0o755);
fs.renameSync(tmpPath, CACHED_BIN_PATH);
return CACHED_BIN_PATH;
}
module.exports = {
SHELLCHECK_VERSION,
CACHED_BIN_PATH,
MAX_REDIRECTS,
DOWNLOAD_TIMEOUT_MS,
extractFileFromTar,
httpsGetFollowingRedirects,
resolveShellcheckBin,
};

View File

@@ -13,14 +13,15 @@
* bash) from landing undetected a second time (it already landed 4 times in
* this repo's workflow templates before #4109's fix).
*
* ShellCheck source: the `shellcheck` npm package (gunar/shellcheck), a thin
* wrapper that downloads the official koalaman/shellcheck binary on first
* use and caches it under node_modules/shellcheck/bin/. Chosen over the
* alternatives surveyed (node-shellcheck: ~5 weekly downloads, last
* published 2022; shellcheck-binaries: ~280 weekly downloads, last
* published 2022) because it has ~80k weekly downloads and is the
* only actively-maintained wrapper — it downloads the CURRENT upstream
* ShellCheck release rather than vendoring a stale binary snapshot.
* ShellCheck source: scripts/lib/shellcheck-fetch.cjs, a small dependency-
* free downloader that fetches a PINNED koalaman/shellcheck release directly
* from GitHub releases and caches the extracted binary under
* node_modules/.cache/shellcheck/<version>/. This replaces the `shellcheck`
* npm package (gunar/shellcheck) originally used here (#4109) — removed in
* #4120 because its extraction dependency, `decompress@4.2.1`, carries an
* unpatched CRITICAL zip-slip vulnerability (GHSA-mp2f-45pm-3cg9, CVSS 9.1)
* with no patched version available upstream. See shellcheck-fetch.cjs's own
* header comment for the extraction implementation and its zip-slip defense.
*
* Extraction: reuses scanFencedBlocks from markdown-sectionizer.cts (the
* canonical fence-scanning engine — see tests/review-plan-coverage-manifest
@@ -112,23 +113,17 @@ const os = require('node:os');
const path = require('node:path');
const childProcess = require('node:child_process');
const { ExitError, runMain } = require('./lib/cli-exit.cjs');
const { resolveShellcheckBin } = require('./lib/shellcheck-fetch.cjs');
// Hard bound on the ShellCheck binary's run time, matching this repo's
// npm-subprocess timeout convention (5-30s git, 60s npm — same "external
// process that could hang" hazard class). See runShellcheck's comment for
// why this cannot be applied via the `shellcheck` npm package's own API.
// process that could hang" hazard class). Applied directly to runShellcheck's
// own spawnSync call below.
const SHELLCHECK_TIMEOUT_MS = 60_000;
const ROOT = path.join(__dirname, '..');
const WORKFLOWS_DIR = path.join(ROOT, 'gsd-core', 'workflows');
const SECTIONIZER_PATH = path.join(ROOT, 'gsd-core', 'bin', 'lib', 'markdown-sectionizer.cjs');
const SHELLCHECK_BIN_MODULE = path.join(ROOT, 'node_modules', 'shellcheck', 'build', 'index.js');
// The top-level SHELLCHECK_BIN_MODULE barrel (build/index.js) does NOT
// re-export `configs/index.js` (verified: `export *`-ing helpers/logger/
// utils/shellcheck.js only — no configs), so `config` (which carries the
// resolved binary path used by runShellcheck's own spawnSync call, see
// below) has to be imported from its own submodule directly.
const SHELLCHECK_CONFIG_MODULE = path.join(ROOT, 'node_modules', 'shellcheck', 'build', 'configs', 'index.js');
const BASELINE_PATH = path.join(__dirname, 'lint-workflow-shellcheck-baseline.json');
// Codes excluded for structural reasons documented in the module header above.
@@ -443,58 +438,25 @@ function loadSectionizer() {
}
}
/** Load the `shellcheck` npm package's programmatic API — its own `shellcheck()`
* function transparently downloads the real binary to node_modules/shellcheck/
* bin/shellcheck (caching it there) on first use if it is not already present. */
async function loadShellcheckModule() {
try {
const mod = await import(SHELLCHECK_BIN_MODULE);
// See SHELLCHECK_CONFIG_MODULE's comment above — `config` is not part of
// the top-level barrel's exports, so it is imported separately and
// attached here for runShellcheck's direct spawnSync call to consume.
const { config } = await import(SHELLCHECK_CONFIG_MODULE);
return { ...mod, config };
} catch (e) {
throw new ExitError(
1,
`lint-workflow-shellcheck: cannot load the 'shellcheck' npm package at ` +
`${path.relative(ROOT, SHELLCHECK_BIN_MODULE)} — run 'npm install' first (${e.message})`,
);
}
}
/**
* Run ShellCheck (json1 output) over every staged temp file in one invocation,
* bounded by SHELLCHECK_TIMEOUT_MS.
*
* The `shellcheck` npm package's own `shellcheck()` function does NOT accept a
* `timeout` — its `ShellCheckArgs` type is `{bin, args, stdio, token}` only
* (verified against node_modules/shellcheck/build/shellcheck.d.ts and .js),
* and internally it hardcodes `child_process.spawnSync(opts.bin, opts.args, {
* stdio: opts.stdio })` with no pass-through for extra spawnSync options.
* Wrapping the call in `Promise.race` against a timer would not help either:
* spawnSync is synchronous and blocks the event loop for its whole duration,
* so a timer callback racing it can never fire before it returns (or hangs).
* Instead, this reimplements the same binary-resolve-and-download step the
* wrapper performs (via the package's own exported `config`/`download`), then
* invokes `child_process.spawnSync` directly with a native `timeout` so a
* hung ShellCheck binary is killed (Node sets `result.error.code ===
* 'ETIMEDOUT'` and `result.signal` on expiry) rather than hanging this lint —
* and, transitively, CI — indefinitely.
* `bin` is resolved by the caller via scripts/lib/shellcheck-fetch.cjs's
* `resolveShellcheckBin()` (downloading and caching the pinned release on
* first use, per that module's own header comment). This invokes
* `child_process.spawnSync` directly with a native `timeout` so a hung
* ShellCheck binary is killed (Node sets `result.error.code === 'ETIMEDOUT'`
* and `result.signal` on expiry) rather than hanging this lint — and,
* transitively, CI — indefinitely.
*/
async function runShellcheck(mod, filePaths) {
function runShellcheck(bin, filePaths) {
const args = [
'--shell=bash',
'--format=json1',
`--exclude=${EXCLUDED_CODES.join(',')}`,
...filePaths,
];
const bin = mod.config.bin;
try {
fs.accessSync(bin, fs.constants.F_OK | fs.constants.X_OK);
} catch {
await mod.download({ destination: bin, token: process.env.GITHUB_TOKEN });
}
const result = childProcess.spawnSync(bin, args, { stdio: 'pipe', timeout: SHELLCHECK_TIMEOUT_MS });
if (result.error) {
const timedOut = result.error.code === 'ETIMEDOUT';
@@ -560,7 +522,7 @@ async function main() {
}
const structuralFailed = structuralFindings.length > 0;
const shellcheckModule = await loadShellcheckModule();
const shellcheckBin = await resolveShellcheckBin();
const stageDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-workflow-shellcheck-'));
try {
@@ -573,7 +535,7 @@ async function main() {
byPath.set(scriptPath, block);
});
const findings = await runShellcheck(shellcheckModule, stagedPaths);
const findings = runShellcheck(shellcheckBin, stagedPaths);
if (findings.length === 0) {
process.stdout.write(

View File

@@ -439,6 +439,7 @@
"scripts/lib/drift-scan.cjs",
"scripts/lib/exit-code-registry.cjs",
"scripts/lib/ndjson-reporter.cjs",
"scripts/lib/shellcheck-fetch.cjs",
"skills/gsd-add-tests/SKILL.md",
"skills/gsd-ai-integration-phase/SKILL.md",
"skills/gsd-audit-fix/SKILL.md",

View File

@@ -509,6 +509,7 @@
"scripts/lib/drift-scan.cjs",
"scripts/lib/exit-code-registry.cjs",
"scripts/lib/ndjson-reporter.cjs",
"scripts/lib/shellcheck-fetch.cjs",
"skills/gsd-ns-context/SKILL.md",
"skills/gsd-ns-context/skills/docs-update/SKILL.md",
"skills/gsd-ns-context/skills/extract-learnings/SKILL.md",

View File

@@ -508,5 +508,6 @@
"scripts/lib/cli-exit.cjs",
"scripts/lib/drift-scan.cjs",
"scripts/lib/exit-code-registry.cjs",
"scripts/lib/ndjson-reporter.cjs"
"scripts/lib/ndjson-reporter.cjs",
"scripts/lib/shellcheck-fetch.cjs"
]

View File

@@ -438,6 +438,7 @@
"scripts/lib/drift-scan.cjs",
"scripts/lib/exit-code-registry.cjs",
"scripts/lib/ndjson-reporter.cjs",
"scripts/lib/shellcheck-fetch.cjs",
"skills/gsd-add-tests/SKILL.md",
"skills/gsd-ai-integration-phase/SKILL.md",
"skills/gsd-audit-fix/SKILL.md",

View File

@@ -401,6 +401,7 @@
"scripts/lib/drift-scan.cjs",
"scripts/lib/exit-code-registry.cjs",
"scripts/lib/ndjson-reporter.cjs",
"scripts/lib/shellcheck-fetch.cjs",
"skills/gsd-ns-context/SKILL.md",
"skills/gsd-ns-context/skills/docs-update/SKILL.md",
"skills/gsd-ns-context/skills/extract-learnings/SKILL.md",

View File

@@ -509,6 +509,7 @@
"scripts/lib/drift-scan.cjs",
"scripts/lib/exit-code-registry.cjs",
"scripts/lib/ndjson-reporter.cjs",
"scripts/lib/shellcheck-fetch.cjs",
"skills/gsd-add-tests/SKILL.md",
"skills/gsd-ai-integration-phase/SKILL.md",
"skills/gsd-audit-fix/SKILL.md",

View File

@@ -439,5 +439,6 @@
"scripts/lib/cli-exit.cjs",
"scripts/lib/drift-scan.cjs",
"scripts/lib/exit-code-registry.cjs",
"scripts/lib/ndjson-reporter.cjs"
"scripts/lib/ndjson-reporter.cjs",
"scripts/lib/shellcheck-fetch.cjs"
]

View File

@@ -401,6 +401,7 @@
"scripts/lib/drift-scan.cjs",
"scripts/lib/exit-code-registry.cjs",
"scripts/lib/ndjson-reporter.cjs",
"scripts/lib/shellcheck-fetch.cjs",
"skills/gsd-add-tests/SKILL.md",
"skills/gsd-ai-integration-phase/SKILL.md",
"skills/gsd-audit-fix/SKILL.md",

View File

@@ -412,6 +412,7 @@
"scripts/lib/drift-scan.cjs",
"scripts/lib/exit-code-registry.cjs",
"scripts/lib/ndjson-reporter.cjs",
"scripts/lib/shellcheck-fetch.cjs",
"skills/gsd-add-tests/SKILL.md",
"skills/gsd-ai-integration-phase/SKILL.md",
"skills/gsd-audit-fix/SKILL.md",

View File

@@ -438,6 +438,7 @@
"scripts/lib/drift-scan.cjs",
"scripts/lib/exit-code-registry.cjs",
"scripts/lib/ndjson-reporter.cjs",
"scripts/lib/shellcheck-fetch.cjs",
"skills/gsd/DESCRIPTION.md",
"skills/gsd/gsd-ns-context/SKILL.md",
"skills/gsd/gsd-ns-context/skills/docs-update/SKILL.md",

View File

@@ -512,6 +512,7 @@
"scripts/lib/drift-scan.cjs",
"scripts/lib/exit-code-registry.cjs",
"scripts/lib/ndjson-reporter.cjs",
"scripts/lib/shellcheck-fetch.cjs",
"skills/gsd-add-tests/SKILL.md",
"skills/gsd-ai-integration-phase/SKILL.md",
"skills/gsd-audit-fix/SKILL.md",

View File

@@ -439,6 +439,7 @@
"scripts/lib/drift-scan.cjs",
"scripts/lib/exit-code-registry.cjs",
"scripts/lib/ndjson-reporter.cjs",
"scripts/lib/shellcheck-fetch.cjs",
"skills/gsd-add-tests/SKILL.md",
"skills/gsd-ai-integration-phase/SKILL.md",
"skills/gsd-audit-fix/SKILL.md",

View File

@@ -436,6 +436,7 @@
"scripts/lib/drift-scan.cjs",
"scripts/lib/exit-code-registry.cjs",
"scripts/lib/ndjson-reporter.cjs",
"scripts/lib/shellcheck-fetch.cjs",
"skills/gsd-add-tests/SKILL.md",
"skills/gsd-ai-integration-phase/SKILL.md",
"skills/gsd-audit-fix/SKILL.md",

View File

@@ -512,6 +512,7 @@
"scripts/lib/drift-scan.cjs",
"scripts/lib/exit-code-registry.cjs",
"scripts/lib/ndjson-reporter.cjs",
"scripts/lib/shellcheck-fetch.cjs",
"skills/gsd-add-tests/SKILL.md",
"skills/gsd-ai-integration-phase/SKILL.md",
"skills/gsd-audit-fix/SKILL.md",

View File

@@ -404,5 +404,6 @@
"scripts/lib/cli-exit.cjs",
"scripts/lib/drift-scan.cjs",
"scripts/lib/exit-code-registry.cjs",
"scripts/lib/ndjson-reporter.cjs"
"scripts/lib/ndjson-reporter.cjs",
"scripts/lib/shellcheck-fetch.cjs"
]

View File

@@ -438,6 +438,7 @@
"scripts/lib/drift-scan.cjs",
"scripts/lib/exit-code-registry.cjs",
"scripts/lib/ndjson-reporter.cjs",
"scripts/lib/shellcheck-fetch.cjs",
"skills/gsd-ns-context/SKILL.md",
"skills/gsd-ns-context/skills/docs-update/SKILL.md",
"skills/gsd-ns-context/skills/extract-learnings/SKILL.md",

View File

@@ -399,6 +399,7 @@
"scripts/lib/drift-scan.cjs",
"scripts/lib/exit-code-registry.cjs",
"scripts/lib/ndjson-reporter.cjs",
"scripts/lib/shellcheck-fetch.cjs",
"skills/gsd-ns-context/SKILL.md",
"skills/gsd-ns-context/skills/docs-update/SKILL.md",
"skills/gsd-ns-context/skills/extract-learnings/SKILL.md",

View File

@@ -401,5 +401,6 @@
"scripts/lib/cli-exit.cjs",
"scripts/lib/drift-scan.cjs",
"scripts/lib/exit-code-registry.cjs",
"scripts/lib/ndjson-reporter.cjs"
"scripts/lib/ndjson-reporter.cjs",
"scripts/lib/shellcheck-fetch.cjs"
]

View File

@@ -470,6 +470,7 @@
"scripts/lib/drift-scan.cjs",
"scripts/lib/exit-code-registry.cjs",
"scripts/lib/ndjson-reporter.cjs",
"scripts/lib/shellcheck-fetch.cjs",
"skills/gsd-ns-context/SKILL.md",
"skills/gsd-ns-context/skills/docs-update/SKILL.md",
"skills/gsd-ns-context/skills/extract-learnings/SKILL.md",

View File

@@ -0,0 +1,348 @@
'use strict';
/**
* lint-workflow-shellcheck-fetch.test.cjs — unit + property coverage for the
* hand-rolled tar reader exported by scripts/lib/shellcheck-fetch.cjs
* (#4120: `decompress` npm package removal, GHSA-mp2f-45pm-3cg9 zip-slip).
*
* Per this repo's CLAUDE.md: "Parsers, budget limits, and bijective
* contracts must include at least one fast-check (`fc`) property test."
* `extractFileFromTar` is a hand-written tar-format parser with a
* security-relevant property (an archive-supplied entry name must never
* influence anything beyond a string-equality/suffix lookup — see
* scripts/lib/shellcheck-fetch.cjs's header comment on the zip-slip defense
* this module replaces), so the coverage below is a binding gate, not
* optional polish.
*
* Test matrix: .gsd/bug/fix-4120-shellcheck-decompress-cve/50-test-matrix.md
*/
const { test, describe } = require('node:test');
const assert = require('node:assert/strict');
const path = require('node:path');
const fs = require('node:fs');
const { EventEmitter } = require('node:events');
const fc = require('./helpers/fast-check-setup.cjs');
const {
extractFileFromTar,
httpsGetFollowingRedirects,
MAX_REDIRECTS,
DOWNLOAD_TIMEOUT_MS,
} = require(path.join(__dirname, '..', 'scripts', 'lib', 'shellcheck-fetch.cjs'));
// --- test-only fixture builders --------------------------------------------
//
// Construct synthetic POSIX tar buffers matching exactly what
// extractFileFromTar's own header comment (scripts/lib/shellcheck-fetch.cjs)
// documents: name at offset 0/length 100, size as octal ASCII at offset
// 124/length 12, typeflag at offset 156, content padded to a 512-byte
// boundary, terminated by an all-zero 512-byte block. These builders are
// test-only fixture plumbing — deliberately NOT exported from the
// production module.
/** Build a single 512-byte header + padded-content tar entry. */
function buildTarEntry(name, content) {
const contentBuf = Buffer.isBuffer(content) ? content : Buffer.from(content, 'utf8');
const header = Buffer.alloc(512); // zero-filled: null-pads name/size fields for free
header.write(name, 0, 100, 'utf8');
const sizeOctal = `${contentBuf.length.toString(8).padStart(11, '0')}\0`; // 12 bytes total
header.write(sizeOctal, 124, 12, 'utf8');
header[156] = '0'.charCodeAt(0); // typeflag '0' == regular file
const paddedLen = Math.ceil(contentBuf.length / 512) * 512;
const contentBlock = Buffer.alloc(paddedLen);
contentBuf.copy(contentBlock, 0);
return Buffer.concat([header, contentBlock]);
}
/** Build a full tar byte stream from `[name, content]` pairs, terminated by
* the mandatory all-zero 512-byte end-of-archive block. */
function buildTar(entries) {
const parts = entries.map(([name, content]) => buildTarEntry(name, content));
parts.push(Buffer.alloc(512)); // terminating zero block
return Buffer.concat(parts);
}
describe('extractFileFromTar', () => {
test('finds and returns the exact bytes for an entry matching the target name exactly', () => {
const tar = buildTar([['shellcheck', 'binary-bytes-here']]);
const result = extractFileFromTar(tar, 'shellcheck');
assert.equal(result.toString('utf8'), 'binary-bytes-here');
});
test('finds a path-prefixed entry (name.endsWith("/" + targetName)) per the documented match rule', () => {
const tar = buildTar([['somedir/shellcheck', 'nested-binary']]);
const result = extractFileFromTar(tar, 'shellcheck');
assert.equal(result.toString('utf8'), 'nested-binary');
});
test('returns null when no entry matches', () => {
const tar = buildTar([
['README.md', 'docs'],
['LICENSE', 'license text'],
]);
assert.equal(extractFileFromTar(tar, 'shellcheck'), null);
});
test('skips over non-matching entries of varying sizes (exact 512-multiples and padding-requiring sizes) to reach a later match', () => {
const tar = buildTar([
['a-file', Buffer.alloc(512, 0x41)], // exact block multiple, no padding needed
['b-file', Buffer.alloc(1024, 0x42)], // exact 2-block multiple
['c-file', Buffer.alloc(1, 0x43)], // 1 byte, needs 511 bytes of padding
['d-file', Buffer.alloc(513, 0x44)], // 513 bytes, needs 511 bytes of padding
['shellcheck', 'the-real-binary'],
]);
const result = extractFileFromTar(tar, 'shellcheck');
assert.equal(result.toString('utf8'), 'the-real-binary');
});
test('stops at the terminating all-zero block rather than reading garbage (or a later match) past it', () => {
// Deliberately bypass buildTar's automatic single terminator: place a
// real terminating zero block in the MIDDLE of the buffer, with a
// matching entry planted after it. If the reader kept scanning past the
// terminator, this would (incorrectly) find and return the post-
// terminator entry instead of null.
const tar = Buffer.concat([
buildTarEntry('unrelated', 'x'),
Buffer.alloc(512), // terminating zero block, NOT at end of buffer
buildTarEntry('shellcheck', 'should-never-be-reached'),
]);
assert.equal(extractFileFromTar(tar, 'shellcheck'), null);
});
// --- security property: the archive-supplied name is data, never a path ---
//
// The actual zip-slip defense is architectural: resolveShellcheckBin (in
// scripts/lib/shellcheck-fetch.cjs) never passes the matched entry's name
// to fs.writeFileSync/mkdirSync/etc — it only ever writes to a path it
// constructs itself. This test does NOT prove that architectural fact by
// itself; it pins the narrower, directly-testable behavioral contract of
// extractFileFromTar in isolation: a traversal-style or otherwise
// malicious name is matched by plain string equality/suffix like any other
// name, and the function never touches the filesystem while doing so.
test('security: traversal-style names are matched by plain string equality/suffix and the function never touches the filesystem', () => {
const fsMethods = ['writeFileSync', 'mkdirSync', 'renameSync', 'chmodSync', 'accessSync', 'readFileSync', 'unlinkSync'];
const originals = {};
for (const m of fsMethods) {
originals[m] = fs[m];
fs[m] = () => {
throw new Error(`extractFileFromTar unexpectedly invoked fs.${m} — the archive name must never reach the filesystem`);
};
}
try {
// A traversal-style name that does NOT satisfy the match rule (does
// not equal, and does not end with "/shellcheck") — no match, no fs
// activity, no throw.
const noMatchTar = buildTar([['../../../etc/passwd', 'pwned-content']]);
assert.equal(extractFileFromTar(noMatchTar, 'shellcheck'), null);
// A traversal-prefixed name that DOES satisfy the documented suffix
// rule (ends with "/shellcheck") matches exactly like any other
// path-prefixed name — the traversal segments are inert string data,
// never resolved or touched as a path by this function.
const matchTar = buildTar([['../../shellcheck', 'traversal-prefixed-but-matches']]);
const result = extractFileFromTar(matchTar, 'shellcheck');
assert.equal(result.toString('utf8'), 'traversal-prefixed-but-matches');
} finally {
for (const m of fsMethods) fs[m] = originals[m];
}
});
// --- fast-check property test (CLAUDE.md-mandated for parsers) ---
//
// Property: for a randomly generated set of tar entries — some named
// exactly the target name, most not, with content lengths spanning the
// 512-byte block boundary at several points (0, 1, 511, 512, 513, 1023,
// 1024 bytes) — extractFileFromTar returns exactly the bytes of the FIRST
// entry whose name equals the target name, or null if none does. This is
// the round-trip check that would catch an off-by-one in the
// padding/block-alignment arithmetic (contentBlocks = ceil(size / 512))
// across a wide range of sizes and entry-count/ordering combinations,
// which the hand-picked boundary cases above only sample.
test('property: finds the first entry whose name matches the target and returns its exact original bytes, else null', () => {
const targetName = 'shellcheck';
// Never collides with targetName and never contains "/", so it can
// never accidentally satisfy either match branch.
const otherNameArb = fc.stringMatching(/^[a-z][a-z0-9_-]{0,10}$/).map((s) => `not-${s}`);
const nameArb = fc.oneof({ arbitrary: otherNameArb, weight: 3 }, { arbitrary: fc.constant(targetName), weight: 1 });
const sizeArb = fc.constantFrom(0, 1, 511, 512, 513, 1023, 1024);
const entryArb = fc
.tuple(nameArb, sizeArb)
.chain(([name, size]) =>
fc.uint8Array({ minLength: size, maxLength: size }).map((bytes) => ({ name, content: Buffer.from(bytes) })),
);
fc.assert(
fc.property(fc.array(entryArb, { maxLength: 8 }), (entries) => {
const tar = buildTar(entries.map((e) => [e.name, e.content]));
const result = extractFileFromTar(tar, targetName);
const firstMatchIdx = entries.findIndex((e) => e.name === targetName);
if (firstMatchIdx === -1) {
assert.equal(result, null);
} else {
assert.notEqual(result, null);
assert.equal(Buffer.compare(result, entries[firstMatchIdx].content), 0);
}
}),
);
});
});
describe('httpsGetFollowingRedirects', () => {
// --- test-only fake transport ---------------------------------------
//
// httpsGetFollowingRedirects's third parameter (`requestFn`, defaulting
// to a real https.get-based transport) exists purely so these tests can
// script a deterministic, offline sequence of redirect/200/error
// responses instead of hitting the real network. Each call below
// consumes exactly one queued `step` and invokes the callback with a
// fake response (a plain EventEmitter carrying statusCode/headers, plus
// a no-op resume() and, for step.body, async data/end emission).
/** Build a fake response EventEmitter matching what real `http.IncomingMessage` exposes here: statusCode, headers, resume(), and (for 200s) data/end events. */
function makeFakeResponse(step) {
const res = new EventEmitter();
res.statusCode = step.status;
res.headers = step.headers || {};
res.resume = () => {};
if (step.body !== undefined) {
// Emit asynchronously so the production code's `res.on(...)` calls
// (registered synchronously right after this response is handed to
// the callback) are attached before anything fires — matching real
// stream timing.
process.nextTick(() => {
res.emit('data', Buffer.isBuffer(step.body) ? step.body : Buffer.from(step.body));
res.emit('end');
});
}
return res;
}
/**
* Build a scriptable `requestFn` that hands back the next queued `step`
* (`{status, headers?, body?}`) on each call, in order — one call per
* HTTP hop (initial request + each redirect). Throws if called more
* times than steps were provided, so a test's step count doubles as an
* assertion on exactly how many hops the production code performs.
* `reqs` (if provided) collects every fake request object created, for
* tests that need to drive request-level events (e.g. 'timeout').
*/
function makeFakeRequestFn(steps, reqs) {
let i = 0;
return function fakeRequestFn(url, options, callback) {
const req = new EventEmitter();
req.destroy = (err) => {
if (err) req.emit('error', err);
};
if (reqs) reqs.push(req);
const step = steps[i++];
assert.ok(step, `fakeRequestFn called more times (call #${i}) than steps provided (${steps.length})`);
process.nextTick(() => callback(makeFakeResponse(step)));
return req;
};
}
test(`limit-1 (${MAX_REDIRECTS - 1} redirects, MAX_REDIRECTS=${MAX_REDIRECTS}): succeeds and returns the final 200 body`, async () => {
const steps = [
...Array.from({ length: MAX_REDIRECTS - 1 }, (_, n) => ({
status: 302,
headers: { location: `https://example.invalid/hop-${n + 1}` },
})),
{ status: 200, body: 'final-bytes' },
];
const result = await httpsGetFollowingRedirects(
'https://example.invalid/start',
MAX_REDIRECTS,
makeFakeRequestFn(steps),
);
assert.equal(result.toString('utf8'), 'final-bytes');
});
test(`limit (exactly ${MAX_REDIRECTS} redirects, MAX_REDIRECTS=${MAX_REDIRECTS}): succeeds — a chain exactly at the limit is not off-by-one rejected`, async () => {
const steps = [
...Array.from({ length: MAX_REDIRECTS }, (_, n) => ({
status: 302,
headers: { location: `https://example.invalid/hop-${n + 1}` },
})),
{ status: 200, body: 'final-bytes-at-limit' },
];
const result = await httpsGetFollowingRedirects(
'https://example.invalid/start',
MAX_REDIRECTS,
makeFakeRequestFn(steps),
);
assert.equal(result.toString('utf8'), 'final-bytes-at-limit');
});
test(`limit+1 (${MAX_REDIRECTS + 1} redirects, MAX_REDIRECTS=${MAX_REDIRECTS}): rejects with a clear "too many redirects" error`, async () => {
// No trailing 200 step: the (MAX_REDIRECTS + 1)th redirect is expected
// to be rejected before a further hop would ever be attempted.
const steps = Array.from({ length: MAX_REDIRECTS + 1 }, (_, n) => ({
status: 302,
headers: { location: `https://example.invalid/hop-${n + 1}` },
}));
await assert.rejects(
httpsGetFollowingRedirects('https://example.invalid/start', MAX_REDIRECTS, makeFakeRequestFn(steps)),
/too many redirects fetching/,
);
});
test('a non-3xx, non-200 status (404) rejects with a clear error naming the status', async () => {
const steps = [{ status: 404 }];
await assert.rejects(
httpsGetFollowingRedirects('https://example.invalid/asset', MAX_REDIRECTS, makeFakeRequestFn(steps)),
/unexpected HTTP 404 fetching https:\/\/example\.invalid\/asset/,
);
});
test('a non-3xx, non-200 status (500) rejects with a clear error naming the status', async () => {
const steps = [{ status: 500 }];
await assert.rejects(
httpsGetFollowingRedirects('https://example.invalid/asset', MAX_REDIRECTS, makeFakeRequestFn(steps)),
/unexpected HTTP 500 fetching https:\/\/example\.invalid\/asset/,
);
});
test('a 3xx response with a MISSING location header does not redirect-loop — it falls through to the status!==200 rejection', async () => {
// Per the current code: the redirect branch is only entered when
// `status >= 300 && status < 400 && res.headers.location` — no
// location means that condition is false, so this falls straight
// through to the `status !== 200` check below it and rejects there,
// rather than looping or crashing on a missing `location`.
const steps = [{ status: 302, headers: {} }];
await assert.rejects(
httpsGetFollowingRedirects('https://example.invalid/asset', MAX_REDIRECTS, makeFakeRequestFn(steps)),
/unexpected HTTP 302 fetching https:\/\/example\.invalid\/asset/,
);
});
// --- Gap 1 coverage: the request-level timeout handler -----------------
//
// Confirms the fix for the unbounded-download hazard: a request that
// never receives a response is rejected once its 'timeout' event fires,
// rather than hanging forever. The fake transport never calls back on
// its own — this test drives the timeout itself, so it runs instantly
// rather than waiting out the real DOWNLOAD_TIMEOUT_MS.
test('a request that times out (no response ever received) is destroyed and rejects with a diagnostic message', async () => {
const reqs = [];
const hangingRequestFn = (_url, _options, _callback) => {
const req = new EventEmitter();
req.destroy = (err) => {
if (err) req.emit('error', err);
};
reqs.push(req);
// Deliberately never invokes _callback — simulates a stalled
// connection that never produces a response.
return req;
};
const promise = httpsGetFollowingRedirects('https://example.invalid/stalls', MAX_REDIRECTS, hangingRequestFn);
assert.equal(reqs.length, 1);
reqs[0].emit('timeout'); // simulate the timer configured via the `timeout` option firing
await assert.rejects(
promise,
new RegExp(`timed out after ${DOWNLOAD_TIMEOUT_MS}ms fetching https://example\\.invalid/stalls`),
);
});
});