[BUG] npm install deletes an existing symlinked node_modules directory

[GrumpyLittleTed]

Is there an existing issue for this?

• I have searched the existing issues

Current Behavior

If a package contains a node_modules that is symlinked then an npm install will issue the message
npm WARN reify Removing non-directory ....
and delete the node_modules directory.

Expected Behavior

A symlinked node_modules directory should be used as is and not recreated - many people symlink node_modules to a local filesystem so that it does not get synced to cloud storage (like Dropbox).

I suspect that the npm code is just checking if any existing file object named node_modules is a directory and does not also check to see if the non-directory file object is actually a symlinked directory.

Steps To Reproduce

cd /tmp
mkdir symlinked
mkdir z
cd z
npm init -y
ln -s /tmp/symlinked node_modules .
npm install lodash

Environment

• OS: Ubuntu 20.04.1
• Node: 16.7
• npm: 7.21.0

[vicary]

This behavior is introduced between npm 7.15 and 7.21.

The new npm install behavior is to always replace a symlinked node_modules with an actual directory, which breaks the usage of node_modules.nosync and a few existing npm modules that favors this behavior, e.g. nosync-icloud

If this breaking change is intended, it should be introduced with a major version upgrade, shouldn’t it?

---

Redirected to here from a long trip,

1. https://npm.community/ is archived and directs you to GitHub community
2. https://github.community/ doesn't have a hashtag/category for NPM
3. When through GitHub's support email and got redirected to https://github.com/npm/feedback/discussions
4. From npm should not forcibly replaces symlinked node_modules feedback#571 I got here.

Commenting in the hope for Google to index this and aid future lost souls to land here.

[dejanceltra]

This still works in 7.20.0, so it had to be introduced in later versions.

This also breaks when developing using Docker, where code is mounted from the host machine, but node_modules is symlinked to a different location, to improve performance.

[vicary]

Confirming a new lower bound of 7.20.6, the lower bound in my previous comment comes from a local cached version of the binary which is incorrect.

[dejanceltra]

Seems like this was added with 7.21.0 with upgrade of npm/arborist: npm/arborist@f2b0cee

Related: https://github.com/npm/arborist/issues/328

[vicary]

From the commit message it reads,

This is performed at every level, so that the symlink cannot be hidden
within a workspace, or created via some other creative means.

Sounds like an intentional incompatibility with yarn existing tools in the ecosystem.

If --force is set, then the safety measure is disabled.

Although skippable with --force, it piggybacks a lot of useful checks under the same flag.

In the spirit of semantic versioning it should be at least a major upgrade so the community can be prepared, shouldn't it?

[dejanceltra]

It works with --force, but this flags also does other things, which might not be desirable:

The -f or --force argument will force npm to fetch remote resources even if a local copy exists on disk.

[isaacs]

This is by design, and prevents a class of RCE security exploits. GHSA-gmw6-94gg-2rc2

[MartinMystikJonas]

Could there option to keep node_modules as symlink? It basically broke our CI becuse we used node_modules symlinked to shared folder to optimize build - now build that took 2-3 minutes takes about an hour and CI fails on timeout. Reason is it again and again needs to install/copy node_modules in each job.

[blaumeise20]

Please add an option to disable this! I used node_modules.nosync with iCloud, and the folder has a size of 300 MB. This uses all of my iCloud storage.

Using --force doesn't work either, so I can't really use my project until this is fixed.

[htorohn]

Has somebody find a solution or a workaround about this issue? I'm having the same issue and had to downgrade my node version on the server, but it is not the ideal solution.

[vicary]

@htorohn This is not about node, this is an npm design flaw. You may either downgrade, or switch away, from npm.

[mk-pmb]

The issue title seems confusing to me, someone please fix: npm seems to not delete the "symlinked node_modules directory" but instead (and way less harmful) it deletes the "symlink to the node_modules directory".

[mk-pmb]

Asking here since the arborist repo with the original security bulletin GHSA-gmw6-94gg-2rc2 is read-only:

If the node_modules folder of the root project or any of its dependencies is somehow replaced with a symbolic link, it could allow Arborist to write package dependencies to any arbitrary location on the file system.

This is meant to defend against malicious scripts installed from/by npm, right? So if npm cannot modify/replace the symbolic link, e.g. because it resides on a read-only mountpoint, shouldn't npm trust the symlink's apparent invulnerability?

Instead, for me, npm@8.5.5 on node@v16.14.2 on Ubuntu focal fails with EACCES.
I simplified my setup to this easy to reproduce example which yields

Error: EACCES: permission denied, unlink '/tmp/cage/lib/node_modules'

[mk-pmb]

Even easier workaround, if you can afford to sacrific one extra layer of directory depth on your shared storage as praise for the npm devs to help npm devs reduce distractions from scenarios where their protective tendency is justified:

1. Name that extra layer node_modules.
2. Move all your actual module folders into it.
3. Adjust your permanent symlinks.
4. Make a dummy directory somewhere.
5. In the dummy directory, create a symlink called lib that points to your original shared storage path.
6. npm install --global --prefix=/your/dummy/directory …

Thus, npm sees that $PREFIX/lib/node_modules is a non-symlink directory and will not try (and fail) to unlink it.

Edit: A bind mount might be even easier.

[vicary]

@mk-pmb Please note that the arborist commit is exactly meant to prevent this kind of usage, all workarounds may be patched just to fulfill this opinion. This is a private company's decision, and, while questionable, it doesn't look like it's up for debate. A concise choice is to move away from NPM.

[mk-pmb]

In my current automated install strategy, it has huge benefits to use a node package manager that I can install from a debian repo as trustworthy as nodesource. (I consider them the same trust level as the nodejs website.) So unless they patch away even the bind mount, … actually, even at that point I'd probably try and just monkey-patch npm.

[blaumeise20]

@vicary It doesn't really do what the commit says. It says we can use --force to disable that, but it doesn't work. See my issue #4358.

[vicary]

@blaumeise20 That's unfortunate, they really should fix it.

[ljharb]

Seems like the fix is to avoid a publicly known security attack by not trying to do this unwise idea in the first place.

[mk-pmb]

@ljharb That part was explained in the comment above the close. I understand how it might be dangerous if npm, and thus potential scripts, can modify or replace the symlink during install. The remaining mystery is why we need to defend against symlinks that npm cannot modify, due to lack of permission or lack of filesystem capability.

[ljharb]

@mk-pmb npm's inability to modify them might not be permanent, and such a pattern in one system might end up in another system where those guarantees don't exist.

[mk-pmb]

I see a common pattern: Security defenses that are usually useful, except in edge cases where external safeguards prevent the attack. There should thus be an option to give npm a list of vulnerability IDs (CVEs etc.) that it does not need to defend because the admin has decided it does not apply to their edge case.

[ljharb]

I totally agree! see npm/rfcs#422.

That, however, doesn't mean that npm itself should make decisions that are unsafe in nonzero scenarios.

[xiromoreira]

@ljharb deleting user files without being asked for is by far an "unsafe in nonzero scenarios" decision, no program should do it. The sane option is to refuse working and/or display a visible warning explaining why.

[ljharb]

Sure, eagerly failing loudly would be better also. The outcome is still the same - you wouldn’t be able to do the thing you want, you just would be informed faster.

[mk-pmb]

you just would be informed faster.

Which can be crucial. In my case, removal of the symlink would have caused backup cron jobs to fail if I had not noticed early enough.

Worst case scenario would probably be somehow breaking the path to someone's SSH authorization mechanism. I can't imagine what such a setup would look like, but from my tech support experience, I'm sure there are people who will have invented a fragile enough setup.

[xiromoreira]

Not sure the outcome is still the same. If you delete the symlink but continue the job:

• I may think nothing happened, specially since the warning is not critical and is on the top of the output (the action may cause other output with more critical appearance).
• (Since it is also done with global node_modules) You may break my node installation, you are deleting all reference to global node_modules, potentially removing files from PATH and duplicating files.

For me the reasonable decision is: the symlink should not be removed, the command should not continue and should exit with non-zero status. It is doing the exact opposite.

[vicary]

Being a few package maintainer myself, this very decision creates wondrous moments with npm link.

[rohan-v8rma]

@GrumpyLittleTed

A symlinked node_modules directory should be used as is and not recreated - many people symlink node_modules to a local filesystem so that it does not get synced to cloud storage (like Dropbox).

Check dropboxignore out if you just want node_modules not to get synced to dropbox, works like a charm.

[jan-hudec]

This is by design, and prevents a class of RCE security exploits. GHSA-gmw6-94gg-2rc2

I disagree this actually fixes anything, because the preinstall script can do the same damage in myriad of other ways besides replacing node_modules with a symlink. Because it is ultimately running with the same permissions as npm.

To prevent a preinstall script from doing damage, it would have to be sandboxed, and then that sandbox can prevent it from, among many other things, replacing node_modules (and touching the subdirectories of any other packages than itself).

[mk-pmb]

Good point. In before "just disable npm scripts".