-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding nvmrc over ssh submodule is wrong #3373
Comments
The submodule is only needed for running tests - are you suggesting the install script will fail because it's present? I'm not concerned with people doing development on nvm needing a normal unobstructed network connection, that should be a baseline, and that commit is unreleased, so nobody should be hitting it with the install script yet - but I definitely wouldn't want to cause issues once it is released. |
The submodule is automatically cloned through git clone command. We saw this issue too just by doing Yes, we also use Ansible to clone through https. This should not happen and is a major issue, since it being an optional submodule for testing purposes being on the master branch |
Using standar ansible git , trying to perform a
|
thanks for the heads up (but nobody should be using the master branch; only tagged releases) So, the solution for your problem is to fix that step - because an end user of nvm never should be doing a normal clone, only a clone via the install script. Can you use the install script instead? |
And that's a bad idea, the whole trend of https://www.idontplaydarts.com/2016/04/detecting-curl-pipe-bash-server-side/ For now we will stick to the latest tag without that but I still think this submodule used only for testing proposes should not be handled like that. |
We first pull the repo, then check for latest tag and checkout to that. However the initial pull just fails due to this. Piping to bash from a curl is definitely a security risk setting aside the way WE install, this will cause issues to clone the repo through https |
no, it's not, actually :-) worrying about that is security theater. The curl is from github, and github isn't doing any detection that would cause this problem. nvm recently had a security audit in partnership with OSTIF, and this simply was not an identified security risk as considered by a veteran security firm. I'll leave this issue open so I can remember to test the standard install script before the next release, so that end users won't fetch the submodule by default. |
https://stackoverflow.com/a/38254893/632724 seems to summarize what all my research has indicated, which is that cloning does not include submodules by default. You'd have to be running So, I don't think there's anything that needs to be done here. |
Just to help others, there is a recursive option in |
One of the latest commit on the master branch
29dce5e
introduces nvmrc as a submodule, and for that it uses the ssh URL. This will (is) causing firewall issues on more sensitive systems where https traffic is allowed but ssh is blocked by default.
29dce5e#diff-fe7afb5c9c916e521401d3fcfb4277d5071798c3baf83baf11d6071742823584R3
The correct way of dealing with this would be either allowing the submodule to be an optional feature, or clone it over https
Thanks
The text was updated successfully, but these errors were encountered: