[Fix] nvm use: handle regex-special characters in $NVM_DIR

[Saadanjum0]

Problem

nvm_change_path (used by nvm use for PATH and MANPATH) interpolates ${NVM_DIR} and the new version directory straight into grep and sed expressions. If $NVM_DIR contains a regex or sed-special character, the resulting PATH is wrong:

• & (e.g. /tmp/a&b/.nvm): sed expands it to the matched text, producing /opt/x:/tmp/a/tmp/a&b/.nvm/versions/node/v1.0.0/binb/.nvm/versions/node/v2.0.0/bin:/bin
• #: the sed command fails outright (bad flag in substitute command) and PATH is emptied
• [, *, \: the "already contains an nvm path" check silently fails, so the old version's directory stays on PATH next to the new one

Cause

Neither ${NVM_DIR} nor ${3} is escaped before being used as a pattern / replacement.

Fix

Escape ${NVM_DIR} for ERE (and for the #-delimited sed expression) and escape \, & and # in the replacement. The grep and sed calls switch to -E so a single escaping scheme applies to both.

Testing

Added test/fast/Unit tests/nvm_change_path special characters, which fails before the change and passes after it in sh, bash, dash and zsh. The existing nvm_change_path and nvm_strip_path tests still pass. In the full test/fast run under bash, the same tests fail with and without this change (nvm ls-remote, nvm_get_arch_unofficial, nvm_install_no_progress_bar, nvm_print_implicit_alias success, nvm_process_nvmrc, nvm_stdout_is_terminal), so those failures are unrelated.

[hridhinvarkeyjoseph]

I checked this out and ran the new test with sh — it passes. Separately, I confirmed that on master 4 of the 6 paths really do break (& garbles PATH, # crashes sed).

One more case worth a test: on master, with NVM_DIR=/tmp/a.b/.nvm, an unrelated PATH entry like /tmp/aXb/.nvm/versions/node/v4.5.0/bin gets replaced, because . matches any character. This PR fixes that, but nothing asserts it, so a future change could regress it silently.

Question: why switch the first two branches from basic to extended regex (grep -Eq, sed -E)? Was that needed for the escaping to work, or to share one escape set? I didn't follow that part.

[Saadanjum0]

Good catch, added a regression test for that: with NVM_DIR=/tmp/a.b/.nvm, an unrelated entry like /tmp/aXb/.nvm/versions/node/v4.5.0/bin now has to be left untouched. Checked it against the pre-fix script — it does wrongly match and rewrite that entry there, so the test is a real regression guard, not just decorative.

On the -E/grep -Eq/sed -E switch — it's necessary, not just for sharing one escape set. NVM_DIR_RE escapes + ? ( ) { } | (among others) by prefixing them with a backslash, e.g. a literal + becomes \+. That's only a literal-matching escape under extended regex syntax. Under GNU basic regex (what plain grep/sed use), a backslash-prefixed + ? | ( ) { } is a GNU extension that gives it the opposite meaning — \+ means "one or more", not literal +. I checked this directly:

$ echo aab | grep    'a\+b'   # BRE: \+ means one-or-more (GNU extension)
aab
$ echo aab | grep -E 'a\+b'   # ERE: \+ is a literal plus
(no match)

So with a directory name containing a literal + (or any of ? ( ) { } |), the old basic-regex grep -q/plain sed calls would've silently done the wrong thing with this escaping scheme — -E was required for the escapes to mean what they're supposed to mean, not a stylistic choice.

[hridhinvarkeyjoseph]

Thanks — checked out the latest commit and the special-characters test (including the new regression case) passes in sh, bash and zsh. The BRE vs ERE explanation makes sense; I hadn't realized \+ flips meaning under GNU BRE.

One thing still open from my inline comment: NVM_DIR_SED and NEW_DIR_SED are only used in the final else branch but are computed on every call. Since nvm_change_path runs on every nvm use, including at shell startup (see #3930), could those two move inside the else? That way the common prepend path only pays for NVM_DIR_RE.

[Saadanjum0]

Good catch, moved both into the else branch — only the replace path needs them, and NVM_DIR_RE alone is enough for every grep check. Verified: both test files still pass, and the common prepend path now does 1 fewer forked sed call (down from 3 to 1) since it no longer computes NVM_DIR_SED/NEW_DIR_SED at all.

[ljharb]

Thanks! The escaping itself looks right to me: the bracket expression covers every ERE metacharacter (and \ is literal inside it), switching all the greps to -E lets one escaping scheme serve every check, and the replacement escapes exactly \, &, and #. sed -E is already used elsewhere in nvm.sh, so that's not a new portability requirement.

Before this can land, nvm_sanitize_path has the same bug: it does sed -e "s#${NVM_DIR}#\${NVM_DIR}#g", so a # in $NVM_DIR breaks the substitution and regex characters can mis-match. Since it's the same class of problem, it'd be great to fix it here too.

Inline comments below.

[Saadanjum0]

Thanks for the review. Pushed 4e6d4a3:

• nvm_sanitize_path now replaces $NVM_DIR and $HOME literally through a small awk helper (values go through ENVIRON, so there's no regex or backslash processing and no sed delimiter problem with #). On the old code a # in NVM_DIR made sed fail with "bad flag in substitute command"; a new test covers that.
• In nvm_change_path the escaped NVM_DIR_RE is now computed after the empty-path early return, so the common path skips the extra subshell and sed.
• Tests: added \, $ and | directories, the system-path-first branch, and a MANPATH call (/share/man) to the change_path test, plus a new nvm_sanitize_path special characters test.

Ran the new and related unit tests under sh, bash and dash.