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

nottoseethesun commented 4 years ago

For --allowed-hosts, how to allow also a specific git+ssh://git@gitlab.foo.bar source?

This would also have to work with the option to require https.

lirantal commented 4 years ago

That's a good point. What would be the use-case we'd want to support? a specific one-off complete URL as a source? such as something like git+ssh://lirantal@github.com/lirantal/lockfile-lint.git#1234567890 or would you want to only match the protocol git+ssh?

nottoseethesun commented 4 years ago

I would think that in order to support public repositories such as github.com, we'd want to validate against the whole URI, optionally including the commit hash or branch name, such as you suggest: git+ssh://lirantal@github.com/lirantal/lockfile-lint.git#1234567890.

lirantal commented 4 years ago

so I'm thinking you could whitelist such URLs in this way:

If you only want to lint based on the hostname:

lockfile-lint --type yarn --path yarn.lock --allowed-hosts yarn github.com

notice that with the change I'm thinking about, --allowed-hosts yarn github.com will only match the actual hosts such as: registry.npmjs.org and github.com. To also enforce HTTPS now you'll need to explicitly opt-in to it

If you want to lint based on the hostname and enable https

lockfile-lint --type yarn --path yarn.lock --allowed-protocols "https:" "git+ssh" --allowed-hosts yarn github.com

WDYT?

nottoseethesun commented 4 years ago

That sounds like an improvement. I have a couple suggestions:

Follow the cannonical uri syntax: https://en.wikipedia.org/wiki/URL#Syntax
- So for example, the option would be --allowed-schemes instead of --allowed-protocols.
Allow restriction by other uri parts (as defined in the wikipedia page listed above). So that could mean these options for lockfile-lint, on a per-allowed-host basis:
- --authority,--uriPath,--query, and --fragment

There would need to be some way of grouping these in the commandline interface to correspond to the relevant allowed-host. Here's an example where three hosts are allowed (yarn, github.com, and gitlab.com) but the last two are restricted by authority and the last one is also restricted by uriPath and fragment (which is in git's case, the branch name):

lockfile-lint --type yarn --path yarn.lock --allowed-schemes "https:" "git+ssh" --allowed-hosts yarn github.com --authority foo@ gitlab.com --authority bar@ --uriPath bat/baz --fragment #develop

Notes

I think --query can be implemented last, since that probably won't be used much.

--validate-https wouldn't conflict with any of this, so it could be left in for backwards compatibility.

Specifying an --authority value that only consists of a hostname would be allowed, and would be the same as listing it as an item in --allowed-hosts (see the section on uri syntax on the wikipedia page to see what else can go in the authority part of a uri).

lirantal commented 4 years ago

I have actually aligned with the syntax used by Node.js's URL API: https://nodejs.org/api/url.html#url_url_protocol - though perhaps worth to use the universal language for it instead of the object keys.

Indeed not sure how authority and fragment will only be relevant for specific schemes so I'm not 100% on those. Agree with --validate-https staying as-is.

I actually had to make a change though, since right now we're validating the --allowed-hosts as complete origins, meaning npm validates https://registry.npmjs.org - so the scheme is derived from it however I'm going to change that so that it matches the host only.

lirantal commented 4 years ago

@nottoseethesun could you provide some practical examples of installing from git and how does that look in a package-lock.json and in a yarn.lock files? I think looking at the resolved entry would not suffice

nottoseethesun commented 4 years ago

On the scheme/protocol - I think either would be fine; perhaps per https://tools.ietf.org/html/rfc7320 , scheme is more precise. Very minor topic though.

I agree on authority and fragment perhaps only being useful for git+ssh, but thought to suggest that since, in the future, other schemes might arise, so this would be flexible to adapt. Just a suggestion; this can be made to work many different ways.

As a really simple way to offer the flexibility we're looking for here in terms of letting developers use this tool even on projects that have, for example, dep's on github.com projects, perhaps the commandline option could support something like, --allowed-uri and then the user just supplies the complete uri (inclusive of course to any url). So that way, only one single, dead-simple parameter would need to be added to lockfile-lint.

lirantal commented 4 years ago

but then for --allowed-uri, that would not match all the others being from registry.npmjs.org, yarnpkg, etc, so what should we test exactly with only such parameter provided? Seems like you'd still need to send --allowed-hosts npm --allowed-uri "git+https://...." right?

p.s. appreciate all the feedback ❤️

nottoseethesun commented 4 years ago

Regarding your question about a practical example, here are a couple:

npm's package-lock.json:

    "foo-bar-project": {
      "version": "git+ssh://git@bat.baz:some-git-repo-user-group/foo-bar-project.git#6910d82b0ba139fc4d9a9c92306a21abdc0e508c",
      "from": "git+ssh://git@bat.baz:some-git-repo-user-group/foo-bar-project.git",
      "requires": {
        "@babel/runtime": "^7.5.5",
        "source-map-support": "^0.5.13",
        "ts-log-debug": "^5.1.0"
      }
    },

yarn's yarn.lock:


"foo-bar@git+ssh://git@bat.baz:some-git-repo-user-group/foo-bar.git":
  version "0.1.0"
  resolved "git+ssh://git@bat.baz:some-git-repo-user-group/foo-bar.git#48e839c2fd5fad2b08dc2c0c64671aea5533ad4f"
  dependencies:
    "@babel/runtime" "^7.5.5"
    source-map-support "^0.5.13"
    ts-log-debug "^5.1.0"

lirantal commented 4 years ago

Yep, so looks like for yarn we can go with resolved while with npm its safer to use version when resolved doesn't exist.

nottoseethesun commented 4 years ago

but then for --allowed-uri, that would not match all the others being from registry.npmjs.org, yarnpkg, etc, so what should we test exactly with only such parameter provided? Seems like you'd still need to send --allowed-hosts npm --allowed-uri "git+https://...." right?

p.s. appreciate all the feedback ❤️

So I was just thinking that the --allowed-uri would be something of an island unto itself, in that it would just take precedence, such that (for example) there'd be no need to add the host in --allowed-hosts. Again, just a suggestion.

lirantal commented 4 years ago

Alrighty, let's leave out allowed-uri for now as I think we have a plan around allowed schemes and allowed hosts updates and that should work well for now. We can work out allowed uri in future PR.

ixti commented 4 years ago

I would like to see --alowed-uri-prefix or --allowed-uri-pattern option, as it would allow to for example whitelist packages hosted in private repos, e.g.:

--allowed-uri-prefix git+ssh://github.com/ixti/*.git
--allowed-uri-prefix git+ssh://github.com/ixti/{foo,bar,baz}.git

lirantal commented 4 years ago

@ixti can you jmp over to https://github.com/lirantal/lockfile-lint/issues/39 to discuss? I'm wondering if just changing the current behavior of --allowed-hosts will be enough to cover this.

ixti commented 4 years ago

@lirantal Sure. Added my 2 cents to #39.

lirantal commented 4 years ago

so @bolatovmar suggested this https://github.com/lirantal/lockfile-lint/pull/52/files#diff-803bbd31414b876c6a30b1dcf035cac8R44 which essentially "upgrades' the --allowed-hosts to also be a more specific URL, such as:

--allowed-hosts github.com/SomeOrg/SomeRepo#<hash>

I have some observations on that PR if we want to land that kind of change:

it needs to be extended to support the scheme as well, so that one off URLs like git+ssh://git@gitlab.foo.bar would be supported (notice it's not just the scheme but the git@ user prefix as well. Agree?
if allowed hosts are an entire github repo, such as --allowed-hosts github.com then that will already include all the possible GitHub URLs. As in, there's no way to blacklist a specific one (maybe that's another command argument then?)

a one-off github URL source could be enabled like this:

--allowed-hosts yarn npm git+ssh://lirantal@github.com/lirantal/lockfile-lint.git#1234567890

does this make sense?

semantics-wise supporting the extended URI form for --allowed-hosts is incorrect but there is some elegancy in terms of the simplification here that it is an opt-in upgrade of a sort and doesn't require more command line arguments to be added.
this doesn't address the ability to specify URL globs/patterns as suggested by @ixti in https://github.com/lirantal/lockfile-lint/issues/11#issuecomment-569449047 however it doesn't make it impossible, just not an implementation by default.

cc @ixti @nottoseethesun appreciate your input as you have both been active in suggesting support for this.

ixti commented 4 years ago

I'm fine either way. :D It's just --allowed-hosts git+ssh://lirantal@github.com/lirantal/lockfile-lint.git#1234567890 looks a bit weird (argument name mismatch value). But FWIW it's not that important to me - as long as I can use that one ;))

lirantal commented 4 years ago

Ok. What do you mean by argument name mismatch value ?

ixti commented 4 years ago

@lirantal I mean when I see argument name --allowed-hosts I would expect it to accept a list of hosts, not URL pattern :D But that might be just me, so as I said earlier - I'm fine with any way that will give me an option to whitelist specific private repos :D

lirantal commented 4 years ago

Yep, I was somewhat concerned with that too which is why I hesitated on merging it until now.

lirantal commented 4 years ago

@bolatovumar I have an idea on how we can support a dedicated command line parameter here. Would you be open and available to making the changes in your PR so we can merge it?

bolatovumar commented 4 years ago

@lirantal sorry, was a bit busy recently so didn't see this. I might have some time in a week or so to adjust my PR to support a dedicated command line parameter. Let me know what you are thinking.

lirantal commented 4 years ago

Sure thing. Details on the PR page as I saw you commented there

lirantal commented 4 years ago

Addressed in https://github.com/lirantal/lockfile-lint/pull/52

lirantal / lockfile-lint

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

Notes