isaacs / rimraf

A `rm -rf` util for nodejs
ISC License
5.66k stars 252 forks source link

Rimraf deletes real files associated with symlinks #299

Closed conartist6 closed 8 months ago

conartist6 commented 8 months ago

I just lost a whole project folder: every file in every repo in a whole organization, because I wanted to programmatically delete the contents of a node_modules folder which contained symlinks to those repos. Instead, all the repos were destroyed. The docs do not say a single word about the program's behavior regarding symlinks.

conartist6 commented 8 months ago

This was reported as https://github.com/isaacs/rimraf/issues/276 assumed to be windows-specific, but this incident occurred on OSX.

conartist6 commented 8 months ago

I was lucky not to lose any work of significance, but needless to say I'm currently scared and angry

conartist6 commented 8 months ago

No I'm just kidding, I really did lose a pretty significant amount of work

isaacs commented 8 months ago

How can I reproduce this?

conartist6 commented 8 months ago

I'll put together a repro. Just to confirm though, this is a bug?

Since the documentation doesn't say one way or another, the semver spec defines any change to the behavior to be breaking since people may be relying on the undocumented behavior as a feature.

isaacs commented 8 months ago

Just to confirm though, this is a bug?

It depends! If you did something like rimraf node_modules/**, then no, you told it to delete all the files in all the subfolders in node_modules, following a single symlink, so a symlink at node_modules/foo to ../foo would by design remove all the files and folders there, then remove the link itself.

However, if you did rimraf node_modules (no /**), then it should just unlink the symlinks, not touching their contents.

conartist6 commented 8 months ago

Here's the script that blew everything up. You can see there's no ** anywhere:

// /dev/scripts/link-modules.js
import { readFile, readdir, stat, symlink, mkdir, rmdir } from 'fs/promises';
import { rimraf } from 'rimraf';

const here = import.meta.url;

try {
  await mkdir(new URL(`../../node_modules/@bablr`, here));
} catch (e) {}

for (const path of await readdir(new URL('../../', here))) {
  const stats = await stat(path);
  if (!path.startsWith('.') && stats.isDirectory()) {
    let pkg;

    try {
      pkg = JSON.parse(await readFile(new URL(`../../${path}/package.json`, here)));
    } catch (e) {}

    if (pkg) {
      // try {
      throw new Error("Only for engineering postmortem purposes")
      // This line deleted everything
      await rimraf(path, new URL(`../../${path}/node_modules/@bablr`, here));
      // } catch (e) {}

      try {
        // these are the links rimraf traversed to delete everything(?)
        await symlink(path, new URL(`../../node_modules/@bablr/${path}`, here));
      } catch (e) {}
    }
  }
}
conartist6 commented 8 months ago

I think I see an error I made actually, I copied the symlink invocation and didn't notice that the first argument was unnecessary, so I'm actually explicitly targeting path for removal as opposed to what I thought I was targeting.

conartist6 commented 8 months ago

At this point then I guess I have no evidence that the titled bug really exists, so I will close the issue. I do think the symlink behavior should be documented though. If I had been able to read the docs and know what the code was supposed to do, I might have found my own error and avoided wasting some of your time.