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

for npm security alerts, devDependencies should be ignored by default or configurable #4146

Closed
zkrising opened this issue Aug 17, 2021 · 28 comments
Labels
F: dependabot-alerts F: security-updates 🔐 Issues specific to security updates L: javascript:npm npm packages via npm T: feature-request Requests for new features

Comments

@zkrising
Copy link

zkrising commented Aug 17, 2021

#3475 and others discuss the ability to do this, but I personally think this should be the default behaviour, since devDependencies aren't going to be public facing, and it causes a ridiculous amount of PR spam even on repositories where I quite literally have 0 dependencies!

The alternative is making a .github/dependabot.yml file in all of my repositories, which seems like it'd get really annoying. If this isn't acceptable as the default behaviour - could we get a way to globally configure dependabot for all of our repositories?

If this already exists, please let me know!

@zkrising zkrising added the T: feature-request Requests for new features label Aug 17, 2021
@asciimike asciimike changed the title [NPM] devDependencies should be ignored by default, or should be externally configurable [NPM] devDependencies should be ignored by default, or Dependabot should be globally configurable Aug 24, 2021
@asciimike
Copy link
Contributor

I think there are several things at play:

  • the current PR (I can only see one?) mentioned is a security update, not a version update. Security updates on dev deps are potentially equally important, e.g. the eslint-scope vuln, which stole credentials, happened on dev dependencies. I don't think it would be prudent for us to no longer alert on those types of issues.
  • allow allows this behavior already
  • I agree on a "global" config, though I think there is an open question on what this looks like (e.g. do you have to specify all manifest directories, or do we have to implement Config file: Support Wildcards in directory #2178; where does it live, etc.)

@zkrising
Copy link
Author

I think there are several things at play:

* the current PR (I can only see one?) mentioned is a security update, not a version update. Security updates on dev deps are potentially equally important, e.g. the eslint-scope vuln, which stole credentials, happened on dev dependencies. I don't think it would be prudent for us to no longer alert on those types of issues.

I completely agree that it's preferable to catch these things rather than potentially ignore a security vulnerability. As it currently stands, a lot of repositories (especially those with lots of dev dependencies, like react apps) get a ridiculous amount of dependabot spam.

I definitely agree that this is unwise for a default, but personally I would like to globally configure dependabot to not tell me about devDependency issues.

It's not really dependabots fault, I don't think - it's more to do with how npm is rather generous with its security disclosures. As it stands though, this results in an annoying amount of spam on repositories that I would really love to opt out of (but still get real-dependency security vuln notifications).

@asciimike
Copy link
Contributor

asciimike commented Aug 25, 2021

Totally get the noise issue, and I agree it's partially our fault and partially the fault of how ecossytems operate and are used. I know npm is considering this as well, e.g.:

I agree that we want some form of global configuration, though we don't have any concrete ideas on how to make this work (e.g. are settings additive or do they get overridden by more specific configs, or does this depend on the setting). If you have suggestions on how you'd like to see this work, I'd appreciate any thoughts.

@zkrising
Copy link
Author

zkrising commented Aug 26, 2021

This is actually a bit more complicated than I thought to wire up a global config file. This post got way longer than I anticipated, whoops.

In short, I think things like these should be as predictable as possible and avoid things like setting-specific mergings. My proposal is for a simple separate dependabot-defaults file (that goes, somewhere, i guess?) and if a local configuration exists for that ecosystem, it should replace the defaults.

Copying existing format for a defaults file

I don't think you could reuse the dependabot format in whole, and then deepmerge it with any repository-overrides. If I were to globally reconfigure my NPM defaults, the merge would either result in it being appended to updates or with some specific hacking it could override elements in update where updates.package-ecosystem === "npm".
This sounds fairly messy, and with things like these I think the best approach is to be cleanly predictable.

package-ecosystem as a key?

Maybe a separate UI could work, using package-ecosystem as unique keys. If a user configures some global defaults for npm, say:

# dependabot-defaults.yml -- goes... somewhere?
defaults:
    npm:
        directory: "/"
        schedule:
            interval: "daily"
        allow:
          - dependency-name: "lodash"

Then that should apply to all repositories where dependabot thinks NPM is relevant. That said...

Merging with repository specific overrides

If a repository has a dependabot.yml file that configures it, the two need to merge predictably. I think it might actually be for the best that no merging takes place.

That is, if dependabot.yml contains configuration for package-ecosystem: "npm", the defaults configured for npm should be completely ignored, and that config should take priority.

I'm not sure how in-line that is with the rest of githubs configurations, merging could take place, but I'm skeptical of how it would have to merge arrays when you might want to override an array entirely, or try and exclude a prop defined in -defaults.

# dependabot.yml
version: 2
updates:
  - package-ecosystem: "npm"
    schedule:
      interval: "weekly"

viz. If this was present in a repository, the only configuration dependabot would see for NPM is exactly that. The defaults explicitly would not apply if a local configuration exists.

Advantages:

  • No breaking other repositories with global configuration
  • No questionable merging of arrays/clashing allow and disallow fields
  • Config is only ever in one place

Disadvantages:

  • Probably not as convienient.

@asciimike
Copy link
Contributor

asciimike commented Aug 26, 2021

My proposal is for a simple separate dependabot-defaults file (that goes, somewhere, i guess?) and if a local configuration exists for that ecosystem, it should replace the defaults.

The likely place is in a .github repo, as this is where a few other "org-wide configs" live. We'd probably just call the file dependabot.yml and assume if it lives in that repo it applies to all repos in the org (potentially including the current one, e.g. if there are actions workflow files that need to be kept up to date).

I think it might actually be for the best that no merging takes place.

While I think I agree conceptually, there are going to be cases where we will need some form of inheritance, for example, many large organizations want to define their private registries once and have them available to all repos.

Maybe a separate UI could work, using package-ecosystem as unique keys.

I think that the combo of package-ecosystem and directory (basically, the manifest file) is really what we care about. I think for a global config, we'd want to provide wildcards for both of these, so that an organization could basically say "we want the following settings to apply to all manifests."

I think the right model might be "we will inherit any settings that aren't explicitly overridden." As an example:

# github.com/org/.github/dependabot.yml
version: 2
# set up an org-wide npm registry
registries:
  org-wide-npm-github:
    type: npm-registry
    url: https://npm.pkg.github.com
    token: ${{secrets.ORG_GITHUB_PERSONAL_TOKEN}}
# by default, check every manifest, regardless of ecosystem once per week, but ignore all patch updates
updates:
  - package-ecosystem: "*"
    directory: "/**"
    schedule:
      interval: "weekly"
    ignore:
      - update-types: ["version-update:semver-patch"]
# github.com/org/repo/.github/dependabot.yml
version: 2
updates:
  - package-ecosystem: "npm"
    directory: "/"
    schedule:
      interval: "daily"
    # use the inherited registry
    # this is potentially pretty difficult to discover
    # orgs will have to make sure the .github repo is visible
    registries:
      - org-wide-npm-github
    # potentially leave a key blank to override an inherited key
    # in this case, we want to get patch updates
    # I think this is the most confusing part
    ignore:

Note that the registry could be overridden in the repo (for just the repo), which is also potentially bad, but I don't have a good way to prevent this (maybe we show a check run failure?).

In your case, this would allow you to:

# github.com/org/.github/dependabot.yml
version: 2
updates:
  - package-ecosystem: "npm"
    directory: "/**"
    schedule:
      interval: "weekly"
    allow:
      - dependency-type: "production"

@zkrising
Copy link
Author

While I think I agree conceptually, there are going to be cases where we will need some form of inheritance, for example, many large organizations want to define their private registries once and have them available to all repos.

You might well be right. The registries: key is clear that this inherits from a global configuration, while allowing people to override things.

To be honest, I don't use dependabot enough (or at an enterprise level) and I think you will probably have a better idea of what design will mesh better with github. Regardless, that solution looks very comfortable!

@asciimike
Copy link
Contributor

To be honest, I don't use dependabot enough (or at an enterprise level) and I think you will probably have a better idea of what design will mesh better with github. Regardless, that solution looks very comfortable!

I'm here to make sure that the solutions work for you, so your feedback is critical :)

I'll keep this issue open, and it will likely get implemented along with org-wide registries, since the functionality is similar.

@asciimike asciimike reopened this Aug 30, 2021
@zkrising
Copy link
Author

I appreciate it! Thanks for responding.

@clintandrewhall
Copy link

clintandrewhall commented Oct 22, 2021

I believe this issue is relevant to an issue in create-react-app, specifically this comment and my comment.

@zkrising
Copy link
Author

More or less, yeah. npm audit is just at odds with build tools as Dan says.

nicks added a commit to tilt-dev/tilt that referenced this issue Dec 17, 2021
nicks added a commit to tilt-dev/tilt that referenced this issue Dec 17, 2021
…endency vulnerabilities

I have no idea why this isn't the default, here are some threads on the issues:
dependabot/dependabot-core#4146
dependabot/dependabot-core#2521
facebook/create-react-app#11174
nicks added a commit to tilt-dev/tilt that referenced this issue Dec 17, 2021
…endency vulnerabilities (#5304)

I have no idea why this isn't the default, here are some threads on the issues:
dependabot/dependabot-core#4146
dependabot/dependabot-core#2521
facebook/create-react-app#11174
@synap5e
Copy link

synap5e commented Feb 16, 2022

Is there any update on the status of this?

I was looking to enable dependabot alerts org-wide, but am concerned that majority of alerts will be about ReDOS in dev deps etc.

@jirikrepl
Copy link

I think this should be implemented

@MeltedYeti
Copy link

MeltedYeti commented Apr 13, 2022

Would love to see this implemented dependabot spam is a pain.

@Farenheith
Copy link

Farenheith commented Apr 14, 2022

I endorse this too. It'd be very nice to just ignore dev dependencies, especially because we don't really care if some test packages bring hundreds of other packages together, for example, if our final package will have zero dependencies.

@kienstra
Copy link

kienstra commented May 5, 2022

It'd be great if .github/dependabot.yml could prevent npm devDependencies dependabot alerts.

It looks like the .gihub/dependabot.yml syntax the documentation suggests doesn't work:

version: 2
updates:
  - package-ecosystem: "npm"
    directory: "/**"
    schedule:
      interval: "daily"
    allow:
      - dependency-type: "production"

npm devDependencies still appear in the dependabot alerts, even after disabling dependabot and enabling it again.

@mikermcneil
Copy link

This is keeping us from turning on dependabot as well.

@Nantris
Copy link

Nantris commented Jul 15, 2022

The clutter from devDependencies dramatically limits the usefulness of Dependabot for quickly responding to consequential security issues.

@jeffwidman jeffwidman added the L: javascript:npm npm packages via npm label Aug 23, 2022
@mikermcneil
Copy link

Security updates on dev deps are potentially equally important, e.g. the eslint-scope vuln, which stole credentials, happened on dev dependencies.

To be clear, there was no way to exploit this vulnerability. It was exploited by default, because the actual dev dependency itself was malicious. Like, a malicious version of it was published by the package maintainer:
https://security.snyk.io/vuln/SNYK-JS-ESLINTSCOPE-11120

image

Malicious packages are a categorically different kind of risk that is very relevant.

It's helpful to monitor for malicious packages, even in dev deps.

But malicious packages are very rare, and my 2¢: I would much rather have dependabot be usable by having it ignore dev deps, rather than not being able to use it because it creates too much noise from vulns which turn out not to be vulns 👦 🐺 .

@kienstra
Copy link

kienstra commented Oct 4, 2022

Would it be possible to fix this? It looks like it's still not working.

You've done a lot of the work already by marking dependencies with a scope.

This would make Dependabot much more usable with JavaScript. It's very hard to catch a real vulnerability, with all the false positives.

Otherwise, would you be open to a PR for this?

@Nantris
Copy link

Nantris commented Oct 4, 2022

@kienstra do you use npm or Yarn? It sounds like scopes only works with npm, but I've not tested either.

@kienstra
Copy link

kienstra commented Oct 4, 2022

Hi @slapbox,
Thanks, I use npm. It sounds like you're facing this issue also 😄

@kienstra
Copy link

kienstra commented Oct 20, 2022

Hi @jakecoffman,
You're probably really busy, so thanks for any time you might be able to give.

Would it be possible to fix this?

If dependabot could ignore devDependencies via .github/dependabot.yml, it'd be much more useful for npm. Now, we're basically ignoring alerts because almost all are false.

Sorry for the random ping, and thanks for any attention you can give this.

@tduncanjc
Copy link

If you're facing this issue when trying to get vuln metrics/KPI, you can use the graphql api to add the scope filter to get the correct aggregate of vulns.

something like this:

{
	repository(name: "dependabot-core", owner: "dependabot") {
	vulnerabilityAlerts(states: OPEN, dependencyScopes: RUNTIME) {
			totalCount
		}
	}
}

Curious if this is the same behavior/attitude towards Rust, PHP, etc. as they have the same concepts within those package managers. If so, we have consistency, if not - this is breaking assumptions for Dependabot behavior in conjunction with other languages in an enterprise setting.

@jeffwidman jeffwidman added F: security-updates 🔐 Issues specific to security updates F: dependabot-alerts labels Feb 2, 2023
@jeffwidman
Copy link
Member

👋 Sorry for the slow update here from the core team.

Since this issue was opened, the scope of this repo / issue tracker / team that monitors it has changed... we no longer handle alerts, we now only handle the code that creates PR's to bump versions--whether from cron or from security alerts--but not creating the security alerts themselves.

Versus as best I can tell this issue is about limiting/filter the alerts themselves, not the resulting PRs.

If so, then given that the team behind alerts doesn't monitor this repo (nor should they) we should probably close this and redirect to a discussion over in https://github.com/community/community/discussions/categories/code-security as that's the best way to reach the team that handles alerts.

This is clearly a popular issue, so I'll leave open for a little bit in case I misunderstand something.

@jportner
Copy link

jportner commented Feb 2, 2023

I've been following this issue for some time, so I'll chime in. I want to be able to configure Dependabot to completely ignore devDependencies -- both for PRs and alerts.

As of now, I have completely disabled Dependabot PRs for security alerts because of the "PR spam".

@jeffwidman
Copy link
Member

jeffwidman commented Feb 2, 2023

I want to be able to configure Dependabot to completely ignore devDependencies -- both for PRs and alerts.

For security PR's (not alerts), did you try setting allow: production as mentioned upthread? It should prevent PR's from being opened for dev dependencies.

@Nantris
Copy link

Nantris commented Feb 2, 2023

@jeffwidman if it doesn't affect alerts then it isn't really useful to us.

An alternative happy medium for us would be to receive ONE weekly alert per repo that contains the alerts for all devDependencies - but I suspect that's not easy to implement.

@jeffwidman
Copy link
Member

jeffwidman commented Feb 2, 2023

I get it, that kind of noise is annoying.

But as I mentioned above, feature requests related to configuring alerts are out of scope for this repo's issue tracker, as this repo and the associated internal service at GitHub only handle creating PR's triggered by cron/alerts, not generating the alerts themselves.

The team managing alerts doesn't monitor this repo (nor should they), so feature requests related to alerts need to go to https://github.com/community/community/discussions/categories/code-security.

If you do create a discussion there, feel free to drop a breadcrumb link here.

@jeffwidman jeffwidman changed the title [NPM] devDependencies should be ignored by default, or Dependabot should be globally configurable for npm security alerts, devDependencies should be ignored by default or configurable Feb 5, 2023
@jeffwidman jeffwidman closed this as not planned Won't fix, can't repro, duplicate, stale Feb 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F: dependabot-alerts F: security-updates 🔐 Issues specific to security updates L: javascript:npm npm packages via npm T: feature-request Requests for new features
Projects
None yet
Development

No branches or pull requests