The husky lint git hook is vulnerable to PATH changes

[mgol]

The linter git hook added in d94c453 has a significant fault; it creates a pre-commit file where PATH is hard-coded to the one at the end of initial npm install (subsequent npm installs don't update it). This poses a problem for tools like nvm where after removing an older Node version (e.g. when a new patch release comes out you might want to remove the older one to not keep vulnerable & outdated releases on the disk) the previous PATH stops being valid, the hook stops working and packages need to be re-installed.

[markelog]

I wouldn't call it a "significant fault", since when you change your node version, it is always a good idea to reinstall your modules, like if you change from .10 to 5.1 without reinstall you shouldn't expect that everything would work correctly.

[mgol]

Not really. If you change major Node versions all that's usually needed is npm rebuild. And even if you want to be safe & reinstall node_modules that's only necessary if you update to a newer major Node version, not when you upgrade your LTS from 4.2.5 to 4.2.6.

[markelog]

not when you upgrade your LTS from 4.2.5 to 4.2.6.

Why would use nvm to switch back and forth patch versions of node? Why wouldn't you always use 4.2.6. in this case? I'd say it is pretty unconventional.

And even if you want to be safe

I'd say yeah, i would really want to be safe, when i switch major versions.

/cc @typicode

I don't use nvm but n which doesn't have this issue, so i can't relate to this ticket, i'd say if problem really exist than it should be in husky repo anyway.

[mgol]

Why would use nvm to switch back and forth patch versions of node? Why wouldn't you always use 4.2.6. in this case? I'd say it is pretty unconventional.

Of course I'd always use 4.2.6, since the moment it's released... It seems you didn't understand me so let's describe the scenario:

1. The newest Node is 4.2.5, I use it, I do npm install in the jQuery directory.
2. Node 4.2.6 comes out, I install it via nvm & remove 4.2.5.
3. I want to git commit but it says npm: command not found. I do npm install and run git commit again, no luck. I have to remove node_modules & npm install again to make it work.

It seems like a husky issue, indeed. I'll report it to them.

[markelog]

let's describe the scenario

I'd say it is hard to consider this raaare case a "significant fault", nevertheless, if this is frustrating for some of us, that could be a grounds for removal a commit-hook

[mgol]

We'll, it will affect anyone that both:

1. Uses nvm to manage Node.js installs
2. Removes older Nodes after installing patch/minor updates

nvm is very popular so I don't think this qualifies as an edge case. And it's a significant fault to me as it prevents you from committing at all, even locally (unless you bypass hooks) so it's an additional barrier for new contributors.

[timmywil]

What's the proposed solution here? I see a couple possibilities.

1. Remove the git hook.
   • I suspect npm rm husky && npm i husky would also fix the issue, and that seems to be a common suggestion for fixing issues with husky (just from browsing their issues list). Given that, I'm inclined to leave this as-is.
2. Leave the git hook and open an issue on the husky repo.
   • I'm not sure what they can do to fix this. Admittedly, I'm not sure what you mean by "PATH is hardcoded". I assumed all husky does is copy over a hook to .git/hooks that then runs specific npm scripts. I'm not sure how different node versions would affect this.

[mgol]

Admittedly, I'm not sure what you mean by "PATH is hardcoded"

That's how they're doing it:

$ cat .git/hooks/pre-commit
#!/bin/sh
# husky
PATH="/Users/mgol/.nvm/versions/node/v5.6.0/lib/node_modules/npm/bin/node-gyp-bin:/Users/mgol/Documents/projects/public/jquery/jquery-core/node_modules/husky/node_modules/.bin:/Users/mgol/Documents/projects/public/jquery/jquery-core/node_modules/.bin:/Users/mgol/.nvm/versions/node/v5.6.0/bin:/Users/mgol/.nvm/versions/node/v5.6.0/bin:/Users/mgol/Documents/projects/public/v8-tools/depot_tools:/Users/mgol/bin:/Users/mgol/.cabal/bin:/usr/local/heroku/bin:/usr/local/bin:/usr/local/sbin:/Applications/CrossOver.app/Contents/SharedSupport/CrossOver/bin:/usr/bin:/bin:/usr/sbin:/sbin:/opt/X11/bin:/Library/TeX/texbin:./node_modules/.bin:/usr/local/opt/dart/libexec/bin"
cd .
[ -f package.json ] && cat package.json | grep -q '"precommit"\s*:'
[ $? -ne 0 ] && exit 0
npm run precommit
if [ $? -ne 0 ]; then
  echo
  echo "husky - pre-commit hook failed (add --no-verify to bypass)"
  echo
  exit 1
fi

I assume the provided hardcoded PATH is derived from the current shell PATH at the moment of invoking npm install. Note the v5.6.0 inside the string.

[markelog]

That's how they're doing it:

https://github.com/typicode/husky/blob/c06d6e8894b65ff51da39bcf877cedfb277467a0/src/index.js#L36

nvm is very popular so I don't think this qualifies as an edge case

I don't believe i have said that this an edge case, i said when you switching node LTS patch versions is a rare case, not an "edge" though a "rare" one.

We'll, it will affect anyone that both:

I think who this behaviour affects is clear, the other issue, if you should or shouldn't reinstall the node modules or just run npm build command when you changing major node versions.

It seems nvm also favours the reinstall approach then npm rebuild given that flag --reinstall-packages exist in there.

[mgol]

i said when you switching node LTS patch versions is a rare case

So you think people are only upgrading their local Node instances to new major versions? Some do that but I don't think upgrading only a patch or a minor is that rare.

the other issue, if you should or shouldn't reinstall the node modules

husky is the first npm package ever that I encountered which forces me to do that for non-major Node.js upgrades.

It seems nvm also favours the reinstall approach then npm rebuild given that flag --reinstall-packages exist in there.

This only affects global packages so it's not relevant to this case.

[markelog]

So you think people are only upgrading their local Node instances to new major versions?

Again, i didn't said that, i'm trying to be precise, so i wouldn't repeat myself, i.e. see above.

husky is the first npm package ever that I encountered which forces me to do that for non-major Node.js upgrades.

Yeah, one of the most common things maintainers of some package ask you to do before thoroughly reviewing your issue, is "Did you try rm -rf node_modules && npm i"? And that advice solves a lot of cases. I suspect most of them because user switches node versions.

There is a lot of packages that needs to be reinstalled when you change node versions it seems its just easy to solve the issue then to find out why it happened.

This only affects global packages so it's not relevant to this case.

You can install preferGlobal packages as local and vise-versa it doesn't really matter where they are installed. I was point out that nvm would reinstall packages not only rebuild it. Or you saying that global packages need to be reinstalled but local modules only rebuilded?

[timmywil]

I assume the provided hardcoded PATH is derived from the current shell PATH at the moment of invoking npm install. Note the v5.6.0 inside the string.

Yea, that's interesting. I'm surprised it doesn't work without setting PATH.

[markelog]

@mgol it seems we are going to the particulars of things, i suggest to move on and focus on what are we want to do with all that.

Do you want to remove pre-commit because of this?

[typicode]

Hi

@markelog thanks for the heads up :)

At the time, PATH was hard coded mainly for Sublime Text users (the problem may appear with other tools too, I haven't checked).

I was using nvm and Sublime Text with Git package and the problem was that nvm modifies PATH in the terminal but other tools aren't aware of that.

So when committing from Sublime Text, git hook was failing because it couldn't find npm in Sublime's PATH.

Hard-coding PATH makes it possible to run hooks out of the terminal. But yeah, it's a trade-off.

I think a check and warning message in husky code like Can't find npm in PATH, try to reinstall husky if you're using a Node version manager could be an enhancement.

I'll verify if the issue is still present with Sublime, maybe there's no need for setting PATH in git hooks anymore and it'll be removed from husky.

[mgol]

@typicode I know postinstall hooks are discouraged but it seems a perfect case for me... If npm install always replaced the hook this wouldn't surprise me so much.

As for the PATH issue, it's a general issue on OS X that GUI apps have different PATH than terminal ones, even regular, non-dynamic PATH modifications done in ~/.profile or ~/.bashrc are not reflected in GUI apps (I guess it's a security-related measure). I haven't been able to make the extended PATH be reflected in the popular SourceTree Git client (https://www.sourcetreeapp.com/) on OS X 10.11 which means I currently can't commit from SourceTree as commitplease (cc @jzaefferer) doesn't use the hack husky does for its commit hook. Choose your poison!

[jzaefferer]

Regarding commitplease, there's an open issue: jzaefferer/commitplease#47 (still looking for steps to reproduce)

[typicode]

@mgol husky has been updated (v0.11.0) and PATH isn't hard-coded anymore :)

I've also noticed that current Sublime Text build inherits PATH when started using subl CLI, so there's no point in storing it anymore.

This new version supports nvm if it's installed and will use default Node version (in a future release, I guess .nvmrc could be handled as well)

If nvm isn't installed, it will simply check that npm is in PATH and if it's not will skip hook to avoid blocking GUI tools users.

I've tested it on OS X and Ubuntu with Sublime Text. Feedbacks are welcome.

[mgol]

I've also noticed that current Sublime Text build inherits PATH when started using subl CLI, so there's no point in storing it anymore.

What if you start it e.g. from Launchpad? GUI apps on OS X don't inherit the terminal PATH if not started from this very terminal (e.g. currently I can't commit via SourceTree without bypassing commit hooks as it cannot find the node binary used by commitplease).

This new version supports nvm if it's installed and will use default Node version (in a future release, I guess .nvmrc could be handled as well)

How is that done? Will it work in GUI apps with default PATH?

[typicode]

If you start from Launchpad it will work too.

The new hook scripts loads nvm the same way it's loaded from the terminal:

# .git/hooks/pre-commit
export NVM_DIR="/Users/typicode"
[ -s "$NVM_DIR/nvm.sh" ] && . "$NVM_DIR/nvm.sh"

So even though your GUI tool isn't aware of terminal PATH when you start it, PATH will be modified when hook is run and npm will become available at this moment (works with Sublime Text started from Spotlight).

The only thing you need to do is to set a default version nvm alias default stable to make it available to GUI apps not started from the terminal.

[mgol]

@typicode Great, thanks! :) I guess we can treat this ticket as a reminder to update husky in our package.json then.

@jzaefferer What do you think about supporting nvm in the same way in commitplease? I know this is applying a workaround for one app only but considering its popularity for managing Node installations...

[markelog]

I'm glad we worked this out, big thanks to @typicode and to @mgol for bringing this up