diff --git a/.semgrepignore b/.semgrepignore index 854bd41f2..63365813c 100644 --- a/.semgrepignore +++ b/.semgrepignore @@ -38,3 +38,16 @@ packages/core/src/api.js # in the file-load helper anyway. No user input flows here. packages/core/test/unit/maestro-hierarchy.test.js packages/core/test/unit/maestro-hierarchy.parity.test.js + +# Regression guard for the packaged-binary crash (PER-9666): it statically +# scans the repo's own source tree for the `require = createRequire` footgun. +# To walk that tree it builds paths with path.join()/path.resolve() inside its +# findRepoRoot() and collectSourceFiles() helpers, which trips semgrep's +# javascript.lang.security.audit.path-traversal.path-join-resolve-traversal +# rule. No user input flows into those joins — every path is derived from +# process.cwd() and fs.readdir() of the repo's own packages/ directory, with a +# fixed allowlist of source extensions and a skip-list of build/dep dirs. It is +# a build-time test, never shipped or exposed to external input. The rule +# cannot follow that the inputs are filesystem-internal, so it false-positives +# on every join. Suppress at the file level with this rationale. +packages/cli-command/test/noRequireBinding.test.js diff --git a/packages/cli-command/src/lockfileDiff.js b/packages/cli-command/src/lockfileDiff.js index 6b6e6ea54..0bbe714b7 100644 --- a/packages/cli-command/src/lockfileDiff.js +++ b/packages/cli-command/src/lockfileDiff.js @@ -6,7 +6,13 @@ import logger from '@percy/logger'; // time — that way importing this module never throws on older Node versions // (or when the optional install was skipped for any other reason). Cached on // first successful load so the require only resolves once per process. -const require = createRequire(import.meta.url); +// Bound to a non-`require` name on purpose: when this ESM file is transpiled to +// CommonJS for the packaged binary, naming it `require` collides with two Babel +// transforms at once — preset-env renames the local `require` to `_require`, and +// transform-import-meta expands `import.meta.url` into a `require('url')` call +// that gets renamed to `_require` too, producing `_require(...)` inside its own +// initializer (TypeError: _require is not a function). See PER smartsnap binary. +const cjsRequire = createRequire(import.meta.url); let _snykModule; /* istanbul ignore next: snyk-backed path — the parser requires Node >=18 while CI runs the suite on Node 14, so these lines can't execute there; they're @@ -14,7 +20,7 @@ let _snykModule; function loadSnyk() { if (_snykModule) return _snykModule; try { - _snykModule = require('snyk-nodejs-lockfile-parser'); + _snykModule = cjsRequire('snyk-nodejs-lockfile-parser'); return _snykModule; } catch (e) { // Surface the underlying require failure so callers can distinguish diff --git a/packages/cli-command/src/smartsnap.js b/packages/cli-command/src/smartsnap.js index a3567106d..bb55258b6 100644 --- a/packages/cli-command/src/smartsnap.js +++ b/packages/cli-command/src/smartsnap.js @@ -8,11 +8,16 @@ import { diffLockfileDeps } from './lockfileDiff.js'; import { renderGraphTraceHtml } from './graphTrace.js'; // stream-json is CommonJS — load via createRequire to avoid named-import interop issues. -const require = createRequire(import.meta.url); -const { parser } = require('stream-json'); -const { pick } = require('stream-json/filters/Pick'); -const { streamArray } = require('stream-json/streamers/StreamArray'); -const { streamValues } = require('stream-json/streamers/StreamValues'); +// Bound to a non-`require` name on purpose: in the CommonJS-transpiled binary, +// naming it `require` collides with Babel's preset-env + transform-import-meta, +// which rewrite `import.meta.url` into a `require('url')` call that then gets +// renamed alongside the local binding to `_require(...)` inside its own +// initializer (TypeError: _require is not a function). +const cjsRequire = createRequire(import.meta.url); +const { parser } = cjsRequire('stream-json'); +const { pick } = cjsRequire('stream-json/filters/Pick'); +const { streamArray } = cjsRequire('stream-json/streamers/StreamArray'); +const { streamValues } = cjsRequire('stream-json/streamers/StreamValues'); // Webpack/Vite stats prefix loader-resolved virtual modules with a NUL byte // (e.g. "\u0000/path/to/file?commonjs-es-import"). Strip it so the path lines diff --git a/packages/cli-command/test/noRequireBinding.test.js b/packages/cli-command/test/noRequireBinding.test.js new file mode 100644 index 000000000..0f398036a --- /dev/null +++ b/packages/cli-command/test/noRequireBinding.test.js @@ -0,0 +1,99 @@ +import fs from 'fs'; +import path from 'path'; + +// Regression guard for the packaged-binary crash fixed in +// fix/cli-command-createRequire-binary-crash. +// +// In an ESM source file, declaring the createRequire result with the binding +// name `require`: +// +// const require = createRequire(import.meta.url); +// +// is fine under Node ESM, but breaks the `pkg`-built executable. When the file +// is transpiled to CommonJS for the binary, two Babel transforms collide: +// - preset-env renames the local `require` binding to `_require`, and +// - transform-import-meta expands `import.meta.url` into a `require('url')` +// call that is *also* renamed to `_require`. +// The result is `_require(...)` evaluated inside its own initializer → +// `TypeError: _require is not a function`, thrown on startup before any command +// runs. The fix is simply to bind to a non-`require` name (e.g. `cjsRequire`). +// +// This is a static source scan — no build step, no new tooling, just fs + a +// regex over every package's published source. It runs inside the existing +// Jasmine node suite. + +// Walk up from the package cwd to the monorepo root (the dir holding lerna.json +// and packages/), so the scan covers the whole repo regardless of which package +// the suite happens to run from. +function findRepoRoot() { + let dir = process.cwd(); + for (;;) { + if (fs.existsSync(path.join(dir, 'lerna.json')) && + fs.existsSync(path.join(dir, 'packages'))) return dir; + let parent = path.dirname(dir); + if (parent === dir) throw new Error('could not locate monorepo root'); + dir = parent; + } +} + +// Collect every source file under packages//src. Build output (dist/build), +// dependencies, tests and coverage are excluded — only authored source matters. +function collectSourceFiles(root) { + const SKIP_DIRS = new Set(['node_modules', 'dist', 'build', 'coverage', 'test', '.nyc_output']); + const SRC_EXT = new Set(['.js', '.cjs', '.mjs']); + const files = []; + + const walk = (dir) => { + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + if (entry.isDirectory()) { + if (!SKIP_DIRS.has(entry.name)) walk(path.join(dir, entry.name)); + } else if (SRC_EXT.has(path.extname(entry.name))) { + files.push(path.join(dir, entry.name)); + } + } + }; + + const packagesDir = path.join(root, 'packages'); + for (const pkg of fs.readdirSync(packagesDir, { withFileTypes: true })) { + if (!pkg.isDirectory()) continue; + const src = path.join(packagesDir, pkg.name, 'src'); + if (fs.existsSync(src)) walk(src); + } + + return files; +} + +// Matches a const/let/var binding literally named `require` assigned from +// createRequire(...). `\s` spans newlines, so a wrapped declaration is caught. +const FORBIDDEN = /\b(?:const|let|var)\s+require\s*=\s*createRequire\b/; + +describe('source: no `require = createRequire` binding', () => { + const root = findRepoRoot(); + const files = collectSourceFiles(root); + + it('scans a non-trivial number of source files', () => { + // Guards against the walk silently finding nothing (wrong cwd, refactor). + expect(files.length).toBeGreaterThan(20); + }); + + it('never binds createRequire to a name `require` (breaks the packaged binary)', () => { + const violations = []; + + for (const file of files) { + const lines = fs.readFileSync(file, 'utf8').split('\n'); + lines.forEach((line, i) => { + if (FORBIDDEN.test(line)) { + violations.push(`${path.relative(root, file)}:${i + 1}: ${line.trim()}`); + } + }); + } + + expect(violations) + .withContext( + 'Bind createRequire to a non-`require` name (e.g. `const cjsRequire = ' + + 'createRequire(import.meta.url)`). Naming it `require` collides with Babel ' + + 'transforms and crashes the pkg binary with "_require is not a function":\n' + + violations.join('\n')) + .toEqual([]); + }); +});