Adding nvmrc over ssh submodule is wrong

[lleirborras]

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

[ljharb]

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.

[jeroenvanasten]

The submodule is automatically cloned through git clone command. We saw this issue too just by doing git clone https://github.com/nvm-sh/nvm.git.

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

[lleirborras]

concerned with

Using standar ansible git , trying to perform a git clone, it tries to fetch the submodule and in our case firewall issues arise and it fails.
At the same time, a module used only for testing should not be mandatory by default on a normal clone

- name: Download nvm
  git:
    repo: https://github.com/nvm-sh/nvm.git
    dest: "{{ XXX.home }}/.nvm"
  diff: no
  changed_when: false

[ljharb]

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?

[lleirborras]

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 curl | bash is a potential security hole , there's documentation about it as early as 2016

https://www.idontplaydarts.com/2016/04/detecting-curl-pipe-bash-server-side/
(apologies for the link with broken certificate)

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.

[jeroenvanasten]

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

[ljharb]

is a potential security hole

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.