[BUG] Double-hyphens can be interpreted as diff-commands leading danger to try to load comments as modified file paths

[eppisapiafsl]

Describe the bug A clear and concise description of what the bug is.

To Reproduce Steps to reproduce the behavior:

1. Modify a file with a no-restricted-import rule disabled like this

/* eslint-disable-next-line no-restricted-imports
 -- we need to use useStore to access to Redux Store
 */
 import { useStore } from "react-redux";

2. Log the modified files danger.git.modified_files
3. The comment we need to use useStore to access to Redux Store is listed

Expected behavior we need to use useStore to access to Redux Store shouldn't be in the list of modified files

Screenshots

Your Environment

software	version
danger.js	11.2.1
node	18.15.0
npm	9.5.0
Operating System	MacOs

Additional context

I'm using https://github.com/mysticatea/eslint-plugin-eslint-comments to force comments when disable a lint rule.

The error only happen when the file is modified, If I create a new file it works as expected

danger.git.created_files doesn't take the comment as a new file danger.git.modified_files take the comment as a new file

[orta]

Wow, that's a pretty strange one - I wonder if the npm module parse-diff is struggling with this specific PR shape. We pass the diff from GitHub to that library to generate the list of files:

https://github.com/danger/danger-js/blob/main/source/platforms/git/diffToGitJSONDSL.ts#L1-L25C2

Maybe try use yarn resolves to see if a later version of the dependency fixes it?

[fbartho]

@eppisapiafsl I could be wrong, but I’m pretty sure eslint-disable-next-line doesn’t support explanation comments.

/* eslint-disable-next-line no-restricted-imports */
import { useStore } from "react-redux";

Does this still exhibit your issue?

[fbartho]

I think it treats your comment as an eslint rule or plug-in that it needs to dynamically load or something.

possibly that’s even a security hole in eslint!

[eppisapiafsl]

@orta tried with latest Danger version, same issue

@fbartho Sorry, forgot to mention in the environment section. I'm using https://github.com/mysticatea/eslint-plugin-eslint-comments to force comments when disable a lint rule.

The error only happen when the file is modified, If I create a new file it works as expected

danger.git.created_files doesn't take the comment as a new file danger.git.modified_files take the comment as a new file

[fbartho]

I'm using https://github.com/mysticatea/eslint-plugin-eslint-comments to force comments when disable a lint rule.

@eppisapiafsl That’s interesting! — I wish they could push their changes upstream to eslint. That eslint-plugin hasn’t seen any releases at all since 2019!! — Knowing what we do about the JS ecosystem, I would be shocked if that plugin didn’t have a ton of dependencies that need to be updated, and deprecated eslint APIs to change, and potentially its own security holes.

---

re: the npm-parse-diff created-vs-modified issue, your comment included the double-hyphen -- I think this is often a special sequence when passed to CLI/Shell commands (I know yarn did stuff with it) — I’m wondering if that sequence is being consumed in an improperly escaped call to a sub-shell. — @orta does that sound plausible or helpful? (Feel free to ignore if I’m trying to feed a red herring to a wild goose)

[orta]

It is a special sequence in a diff: https://patch-diff.githubusercontent.com/raw/reactjs/react.dev/pull/6120.diff

So, yep, parse-diff is really where you want to be looking here IMO

diff --git a/src/components/Layout/Footer.tsx b/src/components/Layout/Footer.tsx
index 4d0b066461f..cd0cf25793c 100644
--- a/src/components/Layout/Footer.tsx
+++ b/src/components/Layout/Footer.tsx