lirantal / lockfile-lint

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

feat: provide the actual bad string in ValidateScheme #20

Closed pratik97 closed 4 years ago

pratik97 commented 5 years ago

Description

In ValidateScheme providing the actual scheme string against which we validate.

Types of changes

Related Issue

Fixes #10

Motivation and Context

How Has This Been Tested?

Added/Updated unit tests.

Screenshots (if appropriate):

Checklist:

codecov-io commented 5 years ago

Codecov Report

Merging #20 into master will increase coverage by 1.11%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #20      +/-   ##
==========================================
+ Coverage    96.1%   97.22%   +1.11%     
==========================================
  Files           9       11       +2     
  Lines         154      180      +26     
  Branches       21       25       +4     
==========================================
+ Hits          148      175      +27     
+ Misses          6        5       -1
Impacted Files Coverage Δ
.../lockfile-lint-api/src/validators/ValidateHttps.js 100% <ø> (ø) :arrow_up:
...lockfile-lint-api/src/validators/ValidateScheme.js 94.73% <ø> (ø) :arrow_up:
...s/lockfile-lint-api/src/validators/ValidateHost.js 100% <ø> (ø) :arrow_up:
packages/lockfile-lint-api/src/common/constants.js 100% <0%> (ø)
...kages/lockfile-lint-api/src/common/ParsingError.js 100% <0%> (ø)
packages/lockfile-lint-api/src/ParseLockfile.js 100% <0%> (+2.63%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4d07dde...c4f0e8a. Read the comment docs.

lirantal commented 5 years ago

@pratik97 a few issues here we should fix.

I think the output would be a bit hard to read, especially when we apply the same to hosts, etc. With this PR it looks like this:

detected invalid scheme(s) https: for package: @babel/code-frame@^7.0.0
error: command failed with exit code 1

the invalid string in the lockfile is "http", which is wrongly showing here as "https" but regardless, it is too blended. How about we format it something like this as an output:

detected invalid scheme(s) for package: @babel/code-frame@^7.0.0
  -> http:
error: command failed with exit code 1

Do you think that's clear for a user?

The other issue is that the current code in this PR logs the allowed scheme, while we want to print out the bad one that is written in the lockfile and doesn't comply with the policy.

lirantal commented 5 years ago

another thought about how to print out the bad values is to maybe use an assertion style, something like this:

detected invalid scheme(s) for package: @babel/code-frame@^7.0.0
  expected: https:
  found: git+https
error: command failed with exit code 1
lirantal commented 5 years ago

@pratik97 great work here.

before we merge, can we:

  1. apply the same error formatting to the other flags too? I don't want to merge this in stages because then the errors will be inconsistent in their print-out.
  2. can you make the last line about the error error: command failed with exit code 1 to have a newline break?
pratik97 commented 5 years ago

@lirantal I will work on it this weekend. Sorry, for the late reply :/

lirantal commented 5 years ago

@pratik97 sure

lirantal commented 4 years ago

@pratik97 thanks for pushing the commits, looks like you addressed all valdiators, or is there something still to be addressed in this PR?

pratik97 commented 4 years ago

@lirantal no, I am done :)

lirantal commented 4 years ago

ok thanks, I shared on twitter a screenshot to get some feedback before we merge: https://twitter.com/liran_tal/status/1190655364758421507?s=20

One of which is to change found to actual which I agree with.

lirantal commented 4 years ago

@pratik97 let's add that change and I'll merge as a new major version. we'll follow it up with other changes as they come by

pratik97 commented 4 years ago

@lirantal done!

lirantal commented 4 years ago

Great stuff @pratik97, thanks for pushing this! ✨💪

lirantal commented 4 years ago

fixes #10