Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
eppisapiafsl opened this issue Jul 24, 2023 · 6 comments
Labels

Comments

@eppisapiafsl
Copy link

eppisapiafsl commented Jul 24, 2023

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";
  1. Log the modified files danger.git.modified_files
  2. 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
Screenshot 2023-07-24 at 1 57 32 PM

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
Copy link
Member

orta commented Jul 24, 2023

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
Copy link
Member

fbartho commented Jul 25, 2023

@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
Copy link
Member

fbartho commented Jul 25, 2023

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
Copy link
Author

@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
Copy link
Member

fbartho commented Jul 25, 2023

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
Copy link
Member

orta commented Jul 25, 2023

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

@fbartho fbartho changed the title [BUG] Danger taking comments as modified files [BUG] Double-hyphens can be interpreted as diff-commands leading danger to try to load comments as modified file paths Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants