search

lirantal / lockfile-lint

Lint an npm or yarn lockfile to analyze and detect security issues
Apache License 2.0
776 stars 35 forks source link

fix: package names from github #154

Closed tian000 closed 1 year ago

tian000 commented 1 year ago

Description

Some packages come from directly github - these packages fail the validate package name check

Types of changes

Related Issue

Motivation and Context

How Has This Been Tested?

Unit tested

Screenshots (if appropriate):

Checklist:

codecov-commenter commented 1 year ago

Codecov Report

Base: 97.74% // Head: 97.75% // Increases project coverage by +0.01% :tada:

Coverage data is based on head (5a54fa6) compared to base (579bef2). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #154 +/- ## ========================================== + Coverage 97.74% 97.75% +0.01% ========================================== Files 13 13 Lines 354 356 +2 Branches 77 78 +1 ========================================== + Hits 346 348 +2 Misses 8 8 ``` | [Impacted Files](https://codecov.io/gh/lirantal/lockfile-lint/pull/154?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Liran+Tal) | Coverage Δ | | |---|---|---| | [...le-lint-api/src/validators/ValidatePackageNames.js](https://codecov.io/gh/lirantal/lockfile-lint/pull/154?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Liran+Tal#diff-cGFja2FnZXMvbG9ja2ZpbGUtbGludC1hcGkvc3JjL3ZhbGlkYXRvcnMvVmFsaWRhdGVQYWNrYWdlTmFtZXMuanM=) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Liran+Tal). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Liran+Tal)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

lirantal commented 1 year ago

Thanks for bringing this up @tian000

A concern I have is that if we merge this pull request then it essentially allows attackers to bypass it because to satisfy the logic the matches https://github.com/Bundlr-Network/avsc#a730cc8018b79e114b6a3381bbb57760a24c6cef' I can just create my own user/org name instead of Bundlr-Network and spoof the source. Correct?

tian000 commented 1 year ago

Thanks for bringing this up @tian000

A concern I have is that if we merge this pull request then it essentially allows attackers to bypass it because to satisfy the logic the matches https://github.com/Bundlr-Network/avsc#a730cc8018b79e114b6a3381bbb57760a24c6cef' I can just create my own user/org name instead of Bundlr-Network and spoof the source. Correct?

Yes that is correct. However, if you are able to compromise the host you can spoof a package regardless. Do you recommend that I just disable the --validatePackageNames check?

lirantal commented 1 year ago

Not exactly though because the package name is checked to match between the one actually used in the dependency and the one used in the sourced URL. The only occurrence of the --validPackageNames check not being helpful is if you also don't enforce a registry source.

lirantal commented 1 year ago

I'm re-reading this and not confident there's a clear decision to make here. I'll close for now but if you still want to raise this as a significant requirement and can add more context and reference use-cases I'm happy to re-consider.