Skip to content

fix(npm): use the dev-build fallback only from a source checkout - #1096

Merged
BryanFRD merged 1 commit into
mainfrom
fix/npm-wrapper-devbuild-scope
Sep 20, 2026
Merged

BryanFRD merged 1 commit into
mainfrom
fix/npm-wrapper-devbuild-scope

Conversation

@BryanFRD

@BryanFRD BryanFRD commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

Closes #1097.

The npm wrapper's dev-build fallback resolves outside the package when installed, into a directory any third party can own. Found while checking a scanner alert on npm/bin/ferrflow.js, which was itself a false alarm.

The path

const devBuild = join(__dirname, "..", "..", "target", "release", `ferrflow${ext}`);

From a source checkout __dirname is <repo>/npm/bin, so this is <repo>/target/release/, the Cargo output. That is the intent and it is fine.

Installed from npm, __dirname is node_modules/ferrflow/bin, and the same expression is:

node_modules/target/release/ferrflow

which belongs to whatever package installs as target. target is a real package on npm (0.0.2, harmless today).

Reproduced

$ npm install ferrflow@7.21.5 --omit=optional
$ cp <any binary> node_modules/target/release/ferrflow.exe
$ npx ferrflow --version
ferrflow 7.11.1

The wrapper ran a binary from node_modules/target/. On a supported platform, with the real one simply not installed.

Why it is worth fixing rather than noting

Optional dependency installs fail silently by design, so the fallback is reachable without the user doing anything unusual: a network blip during npm install is enough to leave the platform package absent and the wrapper looking at node_modules/target/.

The sharper point is that this package has no install scripts, which is a property people check for and rely on. This path quietly gives that guarantee away: a hostile package needs no postinstall, only a file at release/ferrflow and a user who eventually types ferrflow.

It is not a high-severity hole. It needs a package named target in the tree, and anyone who can put one there has other options. It is a one-line guard against a case that should never have been reachable.

The fix

Use the dev build only when the parent of npm/ is actually the FerrFlow source tree:

const inSourceCheckout = existsSync(join(repoRoot, "Cargo.toml"));

Cargo.toml is the marker because it cannot appear where the bad case lives: npm unpacks a package's files under node_modules/<name>/, so nothing can create node_modules/Cargo.toml, and the name is not a legal npm package name anyway (uppercase).

Verified both directions

Installed shape, with the planted binary still in place:

$ npx ferrflow --version
Unsupported platform: win32-x64
Install ferrflow from https://github.com/FerrLabs/FerrFlow/releases

Source checkout, Cargo.toml present and target/release/ferrflow.exe built:

$ node npm/bin/ferrflow.js --version
ferrflow 7.11.1

And with Cargo.toml renamed away, the same checkout refuses, which is the node_modules shape.

Tests

None added. There is no Node test runner in this repo and no CI step that would run one, so a test here means introducing a harness for a three-line function. The two runs above are the verification. Happy to add node --test coverage for getBinaryPath as its own change if the wrapper is going to keep growing logic.

Not changed

The two files the scanner flagged are fine. npm/bin/ferrflow.js spawns with an argv array and no shell: true, which is correct for a launcher, and scripts/validate-site-docs.mjs has no child_process at all (its only exec calls are RegExp.exec) and is not even published: the npm tarball is four files.

@ferrfleet ferrfleet Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The guard is correct for the layout it targets and is strictly better than what is on main: __dirname/../.. is the repo root from a checkout and node_modules from an install, and only the first has Cargo.toml.

Nit: the claim that Cargo.toml cannot appear under node_modules does not hold on a case-insensitive filesystem. cargo.toml is a legal npm package name (lowercase, dots allowed, cf. socket.io), npm unpacks it as the directory node_modules/cargo.toml/, and existsSync is true for directories, so on macOS and Windows the marker check passes. The same attacker now needs two planted packages (target and cargo.toml) instead of one, so the bar is raised rather than the case closed.

Requiring a regular file removes it, since npm can only ever create a directory at that name:

import { existsSync, statSync } from "fs";
// ...
const inSourceCheckout =
  statSync(join(repoRoot, "Cargo.toml"), { throwIfNoEntry: false })?.isFile() === true;

throwIfNoEntry needs Node 14.17+, and engines.node here is >=18. Not sent as a suggestion block because it also touches the fs import on line 3.

No test coverage is a fair call at this size; if getBinaryPath grows another branch, a node --test file that stubs the filesystem would be worth it then.

@BryanFRD
BryanFRD merged commit 7fcdde3 into main Sep 20, 2026
31 checks passed
@BryanFRD
BryanFRD deleted the fix/npm-wrapper-devbuild-scope branch September 20, 2026 15:27
ferrflow Bot added a commit that referenced this pull request Sep 20, 2026
## [7.21.7] - 2026-09-20

### Bug Fixes

- fix(npm): use the dev-build fallback only from a source checkout (#1096)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(npm): the dev-build fallback resolves into node_modules/target, which a third party can own

1 participant