lirantal / lockfile-lint

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

Feature: Integrity hash type validator #135

Closed yoavain closed 1 year ago

yoavain commented 1 year ago

Description

Added a new validator to check that the integrity field uses sha512 hash.
Requires a flag --validate-integrity (alias: -i) so should not be a breaking change.

Types of changes

Related Issue

Motivation and Context

How Has This Been Tested?

Wrote unit tests.
Also, tested on a real package-lock.json file that contains sha1

Screenshots (if appropriate):

Checklist:

lirantal commented 1 year ago

This is awesome Yoav. I'm traveling to Open Source Summit this week so a bit low on availability but will review shortly!

codecov-commenter commented 1 year ago

Codecov Report

Base: 97.75% // Head: 95.90% // Decreases project coverage by -1.84% :warning:

Coverage data is based on head (9a14afa) compared to base (4667c3d). Patch coverage: 77.41% of modified lines in pull request are covered.

:exclamation: Current head 9a14afa differs from pull request most recent head 3357a87. Consider uploading reports for the commit 3357a87 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #135 +/- ## ========================================== - Coverage 97.75% 95.90% -1.85% ========================================== Files 12 13 +1 Lines 312 342 +30 Branches 67 73 +6 ========================================== + Hits 305 328 +23 - Misses 7 14 +7 ``` | [Impacted Files](https://codecov.io/gh/lirantal/lockfile-lint/pull/135?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Liran+Tal) | Coverage Δ | | |---|---|---| | [packages/lockfile-lint/src/config.js](https://codecov.io/gh/lirantal/lockfile-lint/pull/135/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Liran+Tal#diff-cGFja2FnZXMvbG9ja2ZpbGUtbGludC9zcmMvY29uZmlnLmpz) | `100.00% <ø> (ø)` | | | [packages/lockfile-lint/src/main.js](https://codecov.io/gh/lirantal/lockfile-lint/pull/135/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Liran+Tal#diff-cGFja2FnZXMvbG9ja2ZpbGUtbGludC9zcmMvbWFpbi5qcw==) | `100.00% <ø> (ø)` | | | [packages/lockfile-lint/src/validators/index.js](https://codecov.io/gh/lirantal/lockfile-lint/pull/135/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Liran+Tal#diff-cGFja2FnZXMvbG9ja2ZpbGUtbGludC9zcmMvdmFsaWRhdG9ycy9pbmRleC5qcw==) | `73.07% <12.50%> (-11.37%)` | :arrow_down: | | [packages/lockfile-lint-api/index.js](https://codecov.io/gh/lirantal/lockfile-lint/pull/135/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Liran+Tal#diff-cGFja2FnZXMvbG9ja2ZpbGUtbGludC1hcGkvaW5kZXguanM=) | `100.00% <100.00%> (ø)` | | | [...kfile-lint-api/src/validators/ValidateIntegrity.js](https://codecov.io/gh/lirantal/lockfile-lint/pull/135/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Liran+Tal#diff-cGFja2FnZXMvbG9ja2ZpbGUtbGludC1hcGkvc3JjL3ZhbGlkYXRvcnMvVmFsaWRhdGVJbnRlZ3JpdHkuanM=) | `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

Besides the comments, note the coverage miss.

lirantal commented 1 year ago

@yoavain looks good. one last check before we land this, with regards to this comment:

To be more precise we're not validating the integrity itself here but rather it's type, so wdyt about updating the flag and naming convention to be explicit about the type validate-integrity-sha256 ?

Do you think it's better to leave this as validate-integrity rather than call out that we're strictly checking the type conforms to a good type?

yoavain commented 1 year ago

I think that as you said, we're not actually testing the validity, only the type. How about --validate-integrity-type? This sounds more generic and will allow changing the "recommended" types in the future.

lirantal commented 1 year ago

I'm pretty open about this. As in, we could also keep it as is with --validate-integrity, and in the future add an actual integrity check on the data itself to validate that the signature hasn't been spoofed.

lirantal commented 1 year ago

Yep, let's do that and land this. In the meanwhile, open for others to chime in on this thread or a new issue and suggest as need.

julienw commented 1 year ago

I would love to see the Motivation and Context paragraph with more information :-)

lirantal commented 1 year ago

@julienw kind of based on Yoav's prior work with this package: https://github.com/yoavain/fix-lockfile-integrity