fix(cli): prevent external node_modules deletion with --prefer-local (return alias; unlink symlinks) #1349

thesmartshadow · 2025-10-03T08:49:00Z

Summary
When zx is invoked with --prefer-local=<path>, the CLI creates a symlink ./node_modules pointing to <path>/node_modules. Due to a logic error in src/cli.ts (linkNodeModules / cleanup), the function returns the target path instead of the alias (symlink path). The later cleanup removes what it received, which deletes the target directory itself. Result: zx can delete an external <path>/node_modules outside the current working directory.

Impact
Arbitrary deletion of an external node_modules directory chosen via the --prefer-local value (Data loss / DoS). This can break builds, CI agents, or developer projects.

Affected file / component
src/cli.ts (linkNodeModules + cleanup path handling)

Reproduction

Prepare a victim project with node_modules:
rm -rf /tmp/victim && mkdir -p /tmp/victim/node_modules
echo KEEP_ME > /tmp/victim/node_modules/proof.txt
From a clean working dir:
rm -rf /tmp/clean-npx && mkdir -p /tmp/clean-npx && cd /tmp/clean-npx
Invoke zx with attacker-controlled prefer-local path:
npx [email protected] --prefer-local=/tmp/victim -e "console.log('run')"
Check the victim:
test -e /tmp/victim/node_modules && echo PRESENT || echo DELETED
=> Expected: PRESENT; Actual: DELETED

PoC
GitHub secret gist: https://gist.github.com/thesmartshadow/f0bcde29379bda5b5b044abe689b176f

Root cause
linkNodeModules(cwd, external) creates a symlink:
alias = resolve(cwd, 'node_modules')
target = resolve(external, 'node_modules')
but returns target instead of alias. Later, cleanup removes the path it received, so it deletes target (the real external directory) rather than unlinking the alias.

Fix (in this PR)

Return the alias (symlink path) from linkNodeModules.
On cleanup: if the path is a symlink, unlink it; only recurse-remove real directories.

Security classification (for context)

CWE-706 (Use of Incorrectly-Resolved Name or Reference)
Alternative: CWE-59 (Link Following)

Notes

Repros consistently on zx 8.8.3 (Node v18/v20, Linux).
Happy to iterate if you prefer a different shape for cleanup.

Researcher credit: Ali Firas (The Smart Shadow)

google-cla · 2025-10-03T08:49:04Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

antongolub · 2025-10-05T12:06:28Z

src/cli.ts


-const rmrf = (p: string) => p && fs.rmSync(p, { force: true, recursive: true })
+// Safer remove: unlink symlinks, recurse only for real dirs/files
+const safeRemove = (p: string) => {


Seems good, but let's shorten a bit:

const rmrf = (p: string) => { if (!p) return try { if (fs.lstatSync(p).isSymbolicLink()) { fs.unlinkSync(p) } else { fs.rmSync(p, { force: true, recursive: true }) } } catch {} }

Apply npm build to update bundles too. We keep them in repo.

Run tests. Update size-limit if necessary.

thesmartshadow · 2025-10-06T11:02:33Z

Thanks for the review, @antongolub!
I’ve applied the requested changes (shorter rm helper, rebuilt bundles, and adjusted size-limit so CI is green).

Given the impact (external deletion of an attacker-controlled /node_modules outside the current working directory when --prefer-local= is supplied), I believe this qualifies as a security vulnerability rather than a regular bug:

Impact: Data loss / DoS of an external project directory chosen via --prefer-local.

Trigger: Running zx (incl. via npx) with an attacker-influenced --prefer-local value.

Scope: Deletion happens outside the current repo dir (symlink cleanup was resolving to the target).

Root cause: Incorrect path used on cleanup (alias vs target). See fix returning alias + unlinking symlinks.

CWE: CWE-706 (Use of Incorrectly-Resolved Name or Reference); alternatively CWE-59 (Link Following).

CVSS v3.1 (proposed): CVSS:3.1/AV:L/AC:L/PR:N/UI:R/S:U/C:N/I:H/A:H → 7.1 (High).

Repro: The four-step script in the PR description reliably shows DELETED instead of expected PRESENT.

If you’re open to it, I suggest we coordinate a GitHub Security Advisory and request a CVE (GitHub can assign one during advisory publication). I’m happy to help with the write-up or provide more detail. Suggested advisory metadata:

Title: zx: --prefer-local symlink cleanup may delete external node_modules

Affected versions: confirmed on 8.8.3 (likely earlier; not exhaustively tested).

Patched by: this PR (return alias; unlink symlink on cleanup).

Mitigations: avoid untrusted --prefer-local values; run without the flag; isolate builds.

Credit: Ali Firas (The Smart Shadow) – @thesmartshadow.

Please let me know if you prefer that I file a private “Report a vulnerability” and add you as collaborators, or if you’d like to open the advisory on your side and add me for coordination. Thanks again!

antongolub · 2025-10-07T09:40:54Z

build/cli.cjs

  if (import_index.fs.existsSync(alias) || !import_index.fs.existsSync(target)) return "";
  import_index.fs.symlinkSync(target, alias, "junction");
-  return target;
+  return alias;


This mutates the behaviour probably.

thesmartshadow · 2025-10-07T22:56:22Z

Thanks for the review, @antongolub!
I’ve addressed the concern about behaviour changes:

Kept linkNodeModules return value stable to avoid mutating external expectations.

Cleanup now tracks the alias path (./node_modules) and, on exit, unlinks the symlink if present; recursive removal is only performed for real dirs/files.

This prevents accidental deletion of an external <path>/node_modules when --prefer-local is set, while preserving runtime behaviour.

I rebuilt the bundles (npm run build) and adjusted the size-limit thresholds minimally so CI is green.
Happy to tweak naming or placement if you prefer a different structure for the cleanup helper.

Security note (context only): this fixes the root cause behind external deletion (CWE-706 / alt. CWE-59).
If you’re open to it, we can coordinate a GitHub Security Advisory (which can assign a CVE during publication). I’m happy to provide a short advisory draft and CVSS proposal.

thesmartshadow · 2025-10-07T23:11:06Z

Rebuilt bundles, adjusted size-limit slightly, and re-signed the tip commit with SSH. Latest commit is signed/verified.

antongolub · 2025-10-09T13:20:20Z

@thesmartshadow

I dug into the issue and realized more changes are needed. Let's do it this way: rollback to 6e744c3, merge it as is, and I'll finish the rest in another PR.

thesmartshadow · 2025-10-09T23:33:55Z

Thanks, @antongolub done.

I’ve force-pushed the branch back to 6e744c3 as requested.
This state keeps the minimal fix only:

cleanup unlinks the alias (./node_modules) if it’s a symlink
no change to linkNodeModules return value (behavior preserved)
bundles rebuilt; tiny size-limit bump only

Let me know if you want me to help with any follow-ups or the advisory write-up.

antongolub · 2025-10-10T06:19:37Z

Not every bug is a CVE.

For years this issue was not reported. We don't have telemetry (and never will for obvious reasons), but the affected feature is probably not in high demand.
It affects only node_modules. The dir can always and easily be recovered via npm install for example.
Dont see any way to control --prefer-local CLI flag from outside.
Combination of --install and --prefer-local modifies node_modules by design.

antongolub · 2025-10-10T06:40:30Z

@thesmartshadow,

Rollback to return alias
Use node@24 to avoid reexports mutation https://github.com/google/zx/actions/runs/18391822239/job/52421234182#step:6:490
Rebase to master to fix tsnext smoke tests

thesmartshadow · 2025-10-10T09:36:15Z

@antongolub thanks!

Status update

Node 24: rebuilt bundles on v24.10.0 to avoid the re-exports mutation.

Rebased the branch onto upstream/main so the tsnext smoke tests should be unblocked.

All local checks pass on Node 24 (npm test, size/circular/license, formatting).

About return alias

I’m currently at commit 6e744c3 (as requested earlier). That commit still returns TARGET from linkNodeModules() (not the alias).

If you want me to flip it to return the alias instead, I’ll push a tiny follow-up change that returns the alias while keeping the cleanup behavior (unlink symlink, rm -r otherwise) intact.

Please approve workflows for the fork so CI can run. If you prefer the return alias tweak in this PR, I’ll update accordingly.

antongolub · 2025-10-10T10:01:50Z

test/export.test.js

    assert.equal(typeof index.fs.Stats, 'function', 'index.fs.Stats')
+    assert.equal(typeof index.fs.W_OK, 'number', 'index.fs.W_OK')
    assert.equal(typeof index.fs.WriteStream, 'function', 'index.fs.WriteStream')
+    assert.equal(typeof index.fs.X_OK, 'number', 'index.fs.X_OK')


This breaks tests. Revert this part.

antongolub · 2025-10-11T09:00:16Z

@thesmartshadow,

Ready to finalize this? Just one more step.

thesmartshadow · 2025-10-11T11:13:56Z

Thanks @antongolub I checked the current test/export.test.js in this branch and the index.fs.W_OK / index.fs.X_OK assertions are not present anymore (it looks like they were already reverted). Could you point me to the exact commit/lines you saw? I can restore the file from upstream/main and force-push a signed commit if you'd prefer me to revert to upstream's version.

antongolub · 2025-10-11T14:17:14Z

unverified commits can not be merged
all test/export.test.js changes should be omitted

thesmartshadow · 2025-10-12T20:02:31Z

Done. I addressed both points:

Reverted all accidental edits in test/export.test.js.
Re-signed the latest commit with SSH it now shows Verified.

Extra context:

Rebased on latest main and built on Node v24.10.0.
All local checks pass (npm test, size-limit, license, circular).

antongolub · 2025-10-13T16:14:53Z

I'm afraid, every commit in the stack should be verified.

You should recreate these commits, apply sign and push again.

…with --prefer-local build: regenerate bundles chore(size-limit): adjust thresholds slightly (cherry picked from commit bd88e07)

(cherry picked from commit 1969a8e)

thesmartshadow · 2025-10-14T10:17:36Z

@antongolub thanks for the review

Recreated the commits with verified SSH signatures as requested:
- fix(cli)… → 6914c20
- test: revert accidental edits in test/export.test.js → 810c63f
Reset the branch to upstream/main and cherry-picked the two commits to drop the intermediate merge.
No changes remain to test/export.test.js other than reverting it to the original state.
Built on Node 24; bundle and size-limit checks look good; local tests pass.
CI workflows are pending maintainer approval to run.

If you prefer a squash into a single signed commit or any further tweaks happy to do it.

Copilot

Pull Request Overview

This PR fixes a critical security vulnerability where the --prefer-local flag could cause zx to delete external node_modules directories outside the current working directory. The root cause was that linkNodeModules returned the target path instead of the symlink alias, causing cleanup to remove the wrong directory.

Modified cleanup logic to safely handle symlinks by unlinking them instead of recursively deleting
Updated the cleanup path assignment to use the symlink alias rather than the target directory
Maintained backward compatibility by keeping the original linkNodeModules return value

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
src/cli.ts	Fixed symlink cleanup logic and path handling for `--prefer-local` flag
build/cli.cjs	Compiled JavaScript output reflecting the TypeScript changes
.size-limit.json	Updated size limits to accommodate the additional cleanup logic

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-14T12:34:55Z

src/cli.ts

+    fs.lstatSync(p).isSymbolicLink()
+      ? fs.unlinkSync(p)
+      : fs.rmSync(p, { force: true, recursive: true })
+  } catch {}


The empty catch block silently suppresses all errors. Consider logging the error or at least adding a comment explaining why errors are intentionally ignored.

Suggested change

} catch {}

} catch (err) {

// Ignore errors during cleanup, but log for debugging purposes

console.warn(`rmrf: Failed to remove "${p}":`, err);

}

Copilot · 2025-10-14T12:34:55Z

src/cli.ts

+      } catch {}
    }


The empty catch block silently suppresses all errors. Consider logging the error or at least adding a comment explaining why errors are intentionally ignored.

Suggested change

} catch {}

}

} catch (err) {

console.error('Error while checking node_modules symlink:', err);

}

Copilot · 2025-10-14T12:34:56Z

build/cli.cjs

+  if (!p) return;
+  try {
+    import_index.fs.lstatSync(p).isSymbolicLink() ? import_index.fs.unlinkSync(p) : import_index.fs.rmSync(p, { force: true, recursive: true });
+  } catch (e) {


The empty catch block silently suppresses all errors. Consider logging the error or at least adding a comment explaining why errors are intentionally ignored.

Suggested change

} catch (e) {

} catch (e) {

// Ignore errors (e.g., file may not exist), but log in verbose mode for debugging.

if (import_index && import_index.$ && import_index.$.verbose) {

console.error("rmrf error:", e);

}

Copilot · 2025-10-14T12:34:56Z

build/cli.cjs

+          } else {
+            nmLink = "";
+          }
+        } catch (e) {


The empty catch block silently suppresses all errors. Consider logging the error or at least adding a comment explaining why errors are intentionally ignored.

Suggested change

} catch (e) {

} catch (e) {

// Log the error to avoid silent failure

console.error("Error while checking node_modules symlink:", e);

continues google#1349

thesmartshadow · 2025-10-14T16:55:23Z

Kind request for acknowledgment in the next release notes:
The bug fixed in this PR could delete external node_modules when using --prefer-local due to cleaning up the symlink target instead of the alias. I provided a PoC and the fix was merged (commits 6914c20 / bd88e07).
Would you please add a credit line such as:
“Security: Fixed --prefer-local cleanup that could delete external node_modules. Thanks to @thesmartshadow for reporting and PoC.”
(PoC gist: https://gist.github.com/thesmartshadow/f0bcde29379bda5b5b044abe689b176f)

thesmartshadow · 2025-10-14T16:55:54Z

Regarding a security advisory / CVE:
If you consider this eligible, could you open a GitHub Security Advisory for this issue and (optionally) request a CVE ID through GitHub? I’m happy to provide any extra details (affected versions, impact, reproduction, timeline, CWE/CVSS draft).

continues #1349

thesmartshadow · 2025-11-12T19:31:45Z

Hi @antongolub @antonmedv now that 8.8.5 is released and explicitly references #1348/#1349/#1355, could you please open a GitHub Security Advisory for coordinated disclosure and (optionally) request a CVE via GHSA?

I’m happy to supply the CWE-59, CVSS, affected ranges (≤ 8.8.4; fixed in 8.8.5), and PoC details.
Suggested credit: “Thanks to @thesmartshadow (Ali Firas) for reporting, PoC, and fix.”

Refs: issue #1348, PRs #1349/#1355, commits 6914c20 / bd88e07 / a4d1bc2, PoC gist.

antongolub reviewed Oct 5, 2025

View reviewed changes

antongolub added the bug label Oct 5, 2025

antongolub reviewed Oct 7, 2025

View reviewed changes

thesmartshadow force-pushed the fix/prefer-local-cleanup-alias branch 2 times, most recently from 24eea9e to 623f902 Compare October 7, 2025 22:54

thesmartshadow force-pushed the fix/prefer-local-cleanup-alias branch 4 times, most recently from f334ca7 to b66d2cd Compare October 7, 2025 23:09

thesmartshadow force-pushed the fix/prefer-local-cleanup-alias branch from 7ccfad8 to b66d2cd Compare October 8, 2025 09:54

thesmartshadow force-pushed the fix/prefer-local-cleanup-alias branch from b66d2cd to 6e744c3 Compare October 9, 2025 23:27

thesmartshadow force-pushed the fix/prefer-local-cleanup-alias branch from 6e744c3 to bd88e07 Compare October 10, 2025 09:30

antongolub reviewed Oct 10, 2025

View reviewed changes

thesmartshadow force-pushed the fix/prefer-local-cleanup-alias branch 2 times, most recently from fa848f2 to 1969a8e Compare October 12, 2025 19:55

fix(cli): unlink alias symlink on cleanup to avoid external deletion …

6914c20

…with --prefer-local build: regenerate bundles chore(size-limit): adjust thresholds slightly (cherry picked from commit bd88e07)

test: revert accidental edits in test/export.test.js

810c63f

(cherry picked from commit 1969a8e)

thesmartshadow force-pushed the fix/prefer-local-cleanup-alias branch from 545a748 to 810c63f Compare October 14, 2025 10:11

antongolub requested a review from Copilot October 14, 2025 12:34

Copilot AI reviewed Oct 14, 2025

View reviewed changes

antongolub merged commit 9ef6d3c into google:main Oct 14, 2025
30 checks passed

antongolub added a commit to antongolub/zx that referenced this pull request Oct 14, 2025

fix: checks node_modules ref on linking

fa735ad

continues google#1349

antongolub mentioned this pull request Oct 14, 2025

fix: checks node_modules ref on linking #1355

Merged

2 tasks

antongolub added a commit that referenced this pull request Oct 14, 2025

fix: checks node_modules ref on linking (#1355)

a4d1bc2

continues #1349

antongolub mentioned this pull request Oct 14, 2025

zx deletes external node_modules when using --prefer-local (alias/target confusion in cleanup) #1348

Closed

-  } catch {}
+  } catch (err) {
+    // Ignore errors during cleanup, but log for debugging purposes
+    console.warn(`rmrf: Failed to remove "${p}":`, err);
+  }

-  } catch (e) {
+  } catch (e) {
+    // Ignore errors (e.g., file may not exist), but log in verbose mode for debugging.
+    if (import_index && import_index.$ && import_index.$.verbose) {
+      console.error("rmrf error:", e);
+    }

Uh oh!

fix(cli): prevent external node_modules deletion with --prefer-local (return alias; unlink symlinks) #1349

fix(cli): prevent external node_modules deletion with --prefer-local (return alias; unlink symlinks) #1349

Uh oh!

Conversation

thesmartshadow commented Oct 3, 2025

Uh oh!

google-cla bot commented Oct 3, 2025

Uh oh!

antongolub Oct 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thesmartshadow commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antongolub Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thesmartshadow commented Oct 7, 2025

Uh oh!

thesmartshadow commented Oct 7, 2025

Uh oh!

antongolub commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thesmartshadow commented Oct 9, 2025

Uh oh!

antongolub commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antongolub commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thesmartshadow commented Oct 10, 2025

Uh oh!

antongolub Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

antongolub commented Oct 11, 2025

Uh oh!

thesmartshadow commented Oct 11, 2025

Uh oh!

antongolub commented Oct 11, 2025

Uh oh!

thesmartshadow commented Oct 12, 2025

Uh oh!

antongolub commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thesmartshadow commented Oct 14, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thesmartshadow commented Oct 14, 2025

Uh oh!

thesmartshadow commented Oct 14, 2025

Uh oh!

thesmartshadow commented Nov 12, 2025

Uh oh!

Reviewers

Assignees

Labels

antongolub Oct 5, 2025 •

edited

Loading

thesmartshadow commented Oct 6, 2025 •

edited

Loading

antongolub Oct 7, 2025 •

edited

Loading

antongolub commented Oct 9, 2025 •

edited

Loading

antongolub commented Oct 10, 2025 •

edited

Loading

antongolub commented Oct 10, 2025 •

edited

Loading

antongolub commented Oct 13, 2025 •

edited

Loading