feat(validators): add URL validator #52

bolatovumar · 2020-01-26T23:13:58Z

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

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Related Issue

#11

Checklist:

I have updated the documentation (if required).
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.
I added a picture of a cute animal cause it's fun

codecov-io · 2020-01-26T23:19:55Z

Codecov Report

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

@@            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 · 2020-02-01T10:14:48Z

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

lirantal · 2020-02-19T18:43:57Z

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

lirantal · 2020-02-19T19:07:17Z

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

We'll support a new --allowed-urls that will specify full URLs that would have to match.
For example, this should work:

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

Acceptance tests:

Also just specifying the --allowed-urls should work regardless of --allowed-hosts.
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

First change is in lockfile-lint-api package, by introducing a new validator for URLs. Copying from src/validators/ValidateHost.js should be a good start.
The second change is in lockfile-lint package to add the command line arg (src/cli.js), update the existing hosts validator (the one in src/validators/index.js) to actually also check the url option as well, and manage the errors thrown to also really throw if one of --allowed-hosts or --allowed-urls throws

bolatovumar · 2020-02-22T17:20:53Z

@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 · 2020-02-22T19:09:49Z

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 · 2020-03-02T01:00:32Z

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 · 2020-03-02T11:18:31Z

@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 · 2020-03-03T05:49:10Z

@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 · 2020-03-04T21:28:58Z

@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 · 2020-03-05T03:57:50Z

@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 · 2020-03-05T08:43:18Z

@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

Sorry for the hassle, almost there! :-)

packages/lockfile-lint-api/__tests__/utils.test.js

packages/lockfile-lint-api/src/validators/ValidateHost.js

packages/lockfile-lint-api/__tests__/validators.url.test.js

packages/lockfile-lint-api/src/validators/ValidateUrl.js

address lirantal#39, lirantal#11

lirantal · 2020-03-16T10:44:57Z

packages/lockfile-lint-api/__tests__/validators.url.test.js

+
+    expect(() => {
+      validator.validate(['https://registry.npms.org/@babel/code-frame'])
+    }).not.toThrow()


in a future PR we need to remember to replace all these no.toThrow() with the expected returned objects since we changed throwing to returning an object with an error

@lirantal, yeah, will do but will have to be a separate PR.

lirantal · 2020-03-16T14:48:42Z

packages/lockfile-lint/src/validators/index.js

+  if (validatorOptions && validatorOptions.allowedUrls) {
+    const urlValidator = new ValidateUrl({packages: lockfile.object})
+
+    validationResult.errors = validationResult.errors.filter(


what happens if validationResult.errors is already populated from the host validator in line 49? (https://github.com/lirantal/lockfile-lint/pull/52/files#diff-b5633cfe76c3d56193e776b9dc02140dR49) we'll overwrite it here. It might be ok to overwrite some of the results because the url validation will allow them, but not others.

I think would be good to iterate specifically on each package there in the errors objects and then based on the results from urlValidator.validateSingle() into that instead of assigning to it and overwriting whatever is there. Since validateSingle() only returns true or false then you can't just pass the same to the validationResult.errors because that expects to have a specific structure, right? so I think that validateSingle() needs to return the same error object instead of a boolean. Either that, or you'll need to construct it properly here.

@lirantal line 55 will filter out all errors that the did not pass the HostValidator check but passed the UrlValidator check. So, let's say you have ['npm', 'yarn'] specified for allowed hosts and some package fails the check because this packages resolves to a URL on Github. Now, we will have this error in the validationResult.errors array. However, if we explicitly specify this exact Github URL as allowed in our allowedUrls option then it will pass the UrlValidator check and will be filtered out. Is this not what we want?

Yeah I think you're right. This check happens only for errors, and only for those that then have been sent through the allowed-urls so a failing check should keep the error object, and a passing one should remove it.

lirantal · 2020-03-16T14:52:15Z

packages/lockfile-lint/src/validators/index.js

@@ -59,3 +81,35 @@ function ValidateHttpsManager ({path, type, validatorValues, validatorOptions})

  return validator.validate()
 }
+
+function ValidateUrlManager ({path, type, validatorValues, validatorOptions}) {


Hmm, so this is needed, but only in the case that specific urls were specific -u and no hostnames, otherwise we don't need to trigger this one. In the case that we do need to trigger this only on specific URLs then the whole work with ValidateHost below is then needless.

@bolatovumar ok so last comment before we merge - where do we stand on this one? does my comment make sense.

I understand that you've put it here because the validators are being run regardless to one another so maybe we can update the code for runValidators() to not run the url validator manager, if the host validator manager is run, and if so, then skip it. I guess it changes things a bit so I'm happy to take a look at refactoring this after landing this PR.

@lirantal yeah, your comment makes sense. I will look into it.

Ok. I'll get this merged later tonight or tomorrow and we can pick up the remaining work after this one gets in.

@lirantal ok, I added an additional check to skip running the ValidateUrlManager if both the --allowed-urls and --allowed-host are used.

lirantal · 2020-03-16T14:52:48Z

@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 · 2020-03-19T19:15:11Z

@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 · 2020-03-23T21:22:14Z

🎉🎉🎉

lirantal self-requested a review February 1, 2020 10:03

lirantal assigned bolatovumar Feb 1, 2020

bolatovumar closed this Mar 1, 2020

bolatovumar force-pushed the extend-hosts branch from a0a6355 to 3e894df Compare March 1, 2020 22:55

bolatovumar reopened this Mar 2, 2020

bolatovumar changed the title ~~feat(validators): extend host validator to allow specific paths~~ feat(validators): add URL validator Mar 2, 2020

bolatovumar force-pushed the extend-hosts branch from 0ed044e to bbc1041 Compare March 2, 2020 00:57

bolatovumar force-pushed the extend-hosts branch from 430a6ad to 5d94a11 Compare March 7, 2020 05:32

lirantal added the enhancement New feature or request label Mar 7, 2020

lirantal requested changes Mar 7, 2020

View reviewed changes

packages/lockfile-lint-api/__tests__/utils.test.js Outdated Show resolved Hide resolved

packages/lockfile-lint-api/src/validators/ValidateHost.js Outdated Show resolved Hide resolved

lirantal requested changes Mar 7, 2020

View reviewed changes

packages/lockfile-lint-api/__tests__/validators.url.test.js Outdated Show resolved Hide resolved

packages/lockfile-lint-api/__tests__/validators.url.test.js Outdated Show resolved Hide resolved

packages/lockfile-lint-api/src/validators/ValidateUrl.js Outdated Show resolved Hide resolved

bolatovumar force-pushed the extend-hosts branch from 5d94a11 to e60b855 Compare March 14, 2020 05:26

bolatovumar added 2 commits March 14, 2020 20:27

feat: add URL validator

2e3a9a6

address lirantal#39, lirantal#11

feat: allow allowed-urls and allowed-hosts to work in conjunction

428d7ff

bolatovumar force-pushed the extend-hosts branch from e60b855 to 428d7ff Compare March 15, 2020 04:02

feat(allowed-urls): add tests for validateSingle method

f7cb87a

bolatovumar force-pushed the extend-hosts branch from 50dc5aa to f7cb87a Compare March 15, 2020 19:39

lirantal reviewed Mar 16, 2020

View reviewed changes

lirantal requested changes Mar 16, 2020

View reviewed changes

feat(allowed-urls): skip extra work when possible

04b5e7f

lirantal approved these changes Mar 23, 2020

View reviewed changes

lirantal merged commit e81ffe9 into lirantal:master Mar 23, 2020

lirantal mentioned this pull request Mar 23, 2020

README updates for URL validator #82

Closed

bolatovumar deleted the extend-hosts branch March 24, 2020 01:20

This was referenced Jun 21, 2020

Feat: whitelist sources based on specific properties #39

Closed

How to add exception to allow e.g. git+ssh://git@gitlab.foo.bar #11

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(validators): add URL validator #52

feat(validators): add URL validator #52

bolatovumar commented Jan 26, 2020

codecov-io commented Jan 26, 2020 •

edited

Loading

lirantal commented Feb 1, 2020

lirantal commented Feb 19, 2020

lirantal commented Feb 19, 2020

bolatovumar commented Feb 22, 2020

lirantal commented Feb 22, 2020

bolatovumar commented Mar 2, 2020 •

edited

Loading

lirantal commented Mar 2, 2020 •

edited

Loading

bolatovumar commented Mar 3, 2020

lirantal commented Mar 4, 2020

bolatovumar commented Mar 5, 2020

lirantal commented Mar 5, 2020

lirantal left a comment

lirantal Mar 16, 2020

bolatovumar Mar 17, 2020

lirantal Mar 19, 2020

lirantal Mar 16, 2020

bolatovumar Mar 17, 2020

lirantal Mar 19, 2020

lirantal Mar 16, 2020

lirantal Mar 19, 2020

bolatovumar Mar 20, 2020

lirantal Mar 20, 2020

bolatovumar Mar 20, 2020

lirantal commented Mar 16, 2020

lirantal commented Mar 19, 2020

lirantal commented Mar 23, 2020

feat(validators): add URL validator #52

feat(validators): add URL validator #52

Conversation

bolatovumar commented Jan 26, 2020

Description

Types of changes

Related Issue

Checklist:

codecov-io commented Jan 26, 2020 • edited Loading

Codecov Report

lirantal commented Feb 1, 2020

lirantal commented Feb 19, 2020

lirantal commented Feb 19, 2020

bolatovumar commented Feb 22, 2020

lirantal commented Feb 22, 2020

bolatovumar commented Mar 2, 2020 • edited Loading

lirantal commented Mar 2, 2020 • edited Loading

bolatovumar commented Mar 3, 2020

lirantal commented Mar 4, 2020

bolatovumar commented Mar 5, 2020

lirantal commented Mar 5, 2020

lirantal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lirantal commented Mar 16, 2020

lirantal commented Mar 19, 2020

lirantal commented Mar 23, 2020

codecov-io commented Jan 26, 2020 •

edited

Loading

bolatovumar commented Mar 2, 2020 •

edited

Loading

lirantal commented Mar 2, 2020 •

edited

Loading