search

lirantal / lockfile-lint

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

feat(validators): add URL validator #52

Closed bolatovumar closed 4 years ago

bolatovumar commented 4 years ago

address #39

Description

Allows to specify more specific URLs in --allowed-hosts option by specifying something like github.com/SomeOrg/SomeRepo#<hash>. You can be as specific as you want, e.g. github.com, github.com/SomeOrg, github.com/SomeOrg/SomeRepo and github.com/SomeOrg/SomeRepo#<hash> are all valid options.

Types of changes

Related Issue

11

Checklist:

codecov-io commented 4 years ago

Codecov Report

Merging #52 into master will increase coverage by 0.64%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #52      +/-   ##
==========================================
+ Coverage   96.87%   97.52%   +0.64%     
==========================================
  Files          11       12       +1     
  Lines         192      242      +50     
  Branches       31       43      +12     
==========================================
+ Hits          186      236      +50     
  Misses          5        5              
  Partials        1        1
Impacted Files Coverage Δ
packages/lockfile-lint/src/config.js 100.00% <ø> (ø)
packages/lockfile-lint/src/main.js 95.65% <ø> (ø)
packages/lockfile-lint-api/index.js 100.00% <100.00%> (ø)
...s/lockfile-lint-api/src/validators/ValidateHost.js 100.00% <100.00%> (ø)
...es/lockfile-lint-api/src/validators/ValidateUrl.js 100.00% <100.00%> (ø)
packages/lockfile-lint/src/validators/index.js 100.00% <100.00%> (ø)

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 b481e80...f7cb87a. Read the comment docs.

lirantal commented 4 years ago

@bolatovumar thanks for sending over. Let's discuss further in #11

lirantal commented 4 years ago

@bolatovumar are you up for some changes in order to land this in?

lirantal commented 4 years ago

@bolatovumar putting my thoughts around enabling this capability without coupling it into the existing hosts validation logic, see below:

  --allowed-hosts npm.com
  --allowed-urls https://github.com/user/repo/?branch=master&commit=1234

Acceptance tests:

  1. Also just specifying the --allowed-urls should work regardless of --allowed-hosts.
  2. Matching the URL should be best-effort exact match, which is the same as you implemented it in the PR currently - https://github.com/lirantal/lockfile-lint/pull/52/files#diff-803bbd31414b876c6a30b1dcf035cac8R51
bolatovumar commented 4 years ago

@lirantal makes sense. I will probably have some time to look at this in a week or so. Was a bit busy in recent weeks.

lirantal commented 4 years ago

Ok, let's see how it goes in the upcoming week. If you see you're not getting to it anytime soon let me know and I'll pick it up

bolatovumar commented 4 years ago

Ok, I added a separate URL validator which tries to do an exact match for a URL in the list of allowed URLs. However, in its current state it ONLY checks for a list of passed-in URLs and is not usable together with --allowed-hosts flag. I need to get them to work together next.

Not sure how I would go about getting the --allowed-hosts flag to play nicely with --allowed-urls flag. I'm thinking of having the input for --allowed-hosts be also passed into the URL validator which would validate both the hosts and the URLs and report errors only if a certain package fails both checks. This may not be the best approach so if you have an idea on how get this to work in a more elegant manner, please let me know @lirantal.

lirantal commented 4 years ago

@bolatovumar see my last bullet.

The new validator you add to the api package is ok to be standalone and doesn't need to "play nicely together with allowed-hosts" because that's an API and the consumer will implement whatever they want. The change of making them play nicely together is on the CLI package and specifically inside src/validators/index.js. Read that last bullet I mentioned and see if it helps to clear up any vagueness as how to implement it.

bolatovumar commented 4 years ago

@lirantal still not entirely sure what you mean. As it works currently, ValidateHost and ValidateUrl classes are completely independent. ValidateHost validate method receives an array of valid hosts and ValidateUrl validate method receives an array of allowed URLs. Now, if you specify something like:

--allowed-hosts npm.com --allowed-urls https://github.com/user/repo/?branch=master&commit=1234

the ValidateHost validate method will add errors for all packages which have the url of https://github.com/user/repo/?branch=master&commit=1234 since it's not aware of the parameters passed to --allowed-urls. Same thing for --allowed-urls. If will generate errors for all packages which resolve to something other than https://github.com/user/repo/?branch=master&commit=1234.

lirantal commented 4 years ago

@bolatovumar ok, so once more - you should add to the lockfile-lint-api package the individual class that validates a URL. That is indeed a separate class. To the lockfile-lint package, there's a src/validators/index.js file which has a ValidateHostManager function. That is the one which you need to test for both a host and a URL and then do the logic inside to make sure that even if the host passes, the URL may fail it.

Does that help make it clear? :)

bolatovumar commented 4 years ago

@lirantal ValidateHostManager receives values passed to the --allowed-hosts flag like npm. Should it also receive values passed to the --allowed-urls flag then?

If you have something like --allowed-hosts npm --allowed-urls https://github.com/some-repo then if what I'm describing above is correct ValidateHostManager will receive ['npm', 'https://github.com/some-repo'] as validator values instead of just ['npm'] as it does now.

Then ['npm', 'https://github.com/some-repo'] values would be passed along to ValidateHostManager's validate method which would check if all of the packages pass at least one of the validator requirements (i.e., either they are on npm or on https://github.com/some-repo).

Same as above would have to be done for ValidateUrlManager (it accepts values for both allowed hosts and urls, then checks that all packages pass at least one of the validator requirements).

lirantal commented 4 years ago

@lirantal ValidateHostManager receives values passed to the --allowed-hosts flag like npm. Should it also receive values passed to the --allowed-urls flag then?

Exactly. Change it to also receive that flag, and then when it has both it can call the underlying API for both of them and make the proper decision.

I think you got it ;-)

lirantal commented 4 years ago

@bolatovumar thanks for keeping at it. We're getting there and this is really close, just need to polish out the way this validator now works for the CLI

lirantal commented 4 years ago

@bolatovumar sorry about the delay here. Great work and commitment from you. I wanted to say that I appreciate the time and attending to everything! 🙏

lirantal commented 4 years ago

🎉🎉🎉