Repository navigation
[Fix] nvm use: handle regex-special characters in $NVM_DIR - #3939
Conversation
nvm_change_path interpolated `$NVM_DIR` and the new version dir unescaped into grep and sed expressions, so a path containing `&`, `#`, `[`, `*`, etc. corrupted `$PATH` (or made sed fail) when switching versions. Escape them before use.
hridhinvarkeyjoseph
left a comment
hridhinvarkeyjoseph
left a comment
There was a problem hiding this comment.
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.
|
Good catch, added a regression test for that: with On the So with a directory name containing a literal |
|
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 One thing still open from my inline comment: |
This comment was marked as spam.
This comment was marked as spam.
|
Good catch, moved both into the |
ljharb
left a comment
ljharb
left a comment
There was a problem hiding this comment.
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.
|
Thanks for the review. Pushed 4e6d4a3:
Ran the new and related unit tests under sh, bash and dash. |
4e6d4a3 to
19755ea
Compare
19755ea to
906fb60
Compare
New Stuff - select versions by category: `alpha`, `lts`, `non-lts`, `iojs`; exclude alphas by default - `nvm alias`: store `--lts` and `--lts=<name>` - record every node version's LTS status - `nvm ls-remote`: annotate versions with the `node` alias and local aliases (#3920) - add `nvm_has_italics`, to detect terminal italics support via terminfo Bug Fixes - `nvm`: treat a bare `--only` like an empty `--only=` - `nvm_ls_remote`, `nvm_remote_version`: match dots between version components literally (#3982) - `nvm_nvmrc_invalid_msg`: respect `--silent` - `nvm_download`: pass `$NVM_AUTH_HEADER` to curl and GNU Wget in a config file, not as an argument - `nvm_ls_remote_index_tab`, `nvm_remote_version`, `nvm_ls_cached`: pass the version pattern to grep with `-e` - `nvm_die_on_prefix`: never let a project `.npmrc` choose which npmrc file `--delete-prefix` edits - `nvm_download_artifact`: only put an archive in the cache once its checksum has passed - `nvm_normalize_lts`, `nvm_ls_remote_index_tab`: keep untrusted text out of shell arithmetic and LTS aliases - `nvm install`: keep nvm options after the version out of `./configure` (#3981) - `nvm install`: apply `--alias`, `--default` and `--save` after every install (#3981) - `nvm install`: don't leave `NVM_NO_PROGRESS` set in the caller's shell - `nvm_get_default_packages`: ignore CRLF line endings (#3979) - `install.sh`: ignore commented-out profile source lines (#3860) - `nvm_print_implicit_alias`: keep `STABLE` bound, and `STABLE` and `UNSTABLE` independent of the caller (#3975) - `nvm_ls`: treat a version dir with only hidden entries as empty (#3974) - `ls`: list versions whose directory is a symlink (#3973) - `nvm_die_on_prefix`: check a nonexistent prefix on Windows (#3972) - `install.sh`: only write `$HOME` when `$NVM_DIR` is inside it (#3962) - `nvm_is_version_or_alias`: reject uppercase LTS names in every locale - `nvm_process_nvmrc`: don't show the content of a symlinked `.nvmrc` - `nvm_write_nvmrc`: don't write through a symlinked `.nvmrc` - `nvm_ls`: match `NVM_DIR` literally - `nvm use`: do not pile up `PATH` entries in nested sessions (#3959) - `nvm_filter_installed_versions`, `nvm_filter_lts_versions`, `nvm_record_lts_status`, `nvm_get_default_packages`: keep backslashes in `NVM_DIR` paths - `nvm_strip_path`: preserve literal backslashes in `NVM_DIR` (#3958) - `nvm_find_up`, `nvm_find_project_dir`: stop at a Windows drive root instead of looping forever (#3968) - `nvm_ls_cached`: match dots in the version literally, and ignore a trailing dot (#3961) - `install.sh`: `nvm_check_global_modules`: don't run a Windows npm under WSL (#3966) - `nvm`: ignore option variables set by the caller - `nvm reinstall-packages`: don't print `VERSION=''` in zsh - `nvm_alias`, `nvm_ls_remote_index_tab`, `nvm use`: return the exit code of a failed check - `nvm_print_versions`: don't mark `N/A` as installed - `nvm_ls`: no blank line before `system` when nothing is installed - `nvm_print_versions`: widen the version column for long versions - `nvm_ls`: match a prerelease only up to a `.` - `nvm_ls`: list every installed prerelease, in semver order - `nvm uninstall`: only remove aliases that point at the uninstalled version - `nvm_ls_cached`: only match whole version numbers - `nvm debug`: don't let the shell read stdin when asked for its version - `nvm_download`: with GNU Wget, follow only same-origin redirects when sending `$NVM_AUTH_HEADER` - `nvm_download`: only send `$NVM_AUTH_HEADER` to URLs under a mirror it applies to - `install.sh`: don't let a git repository in the current directory affect `git ls-remote` - `nvm_remote_versions`: only consult io.js's version list for majors 1 through 3 - `nvm_remote_versions`: place io.js versions by major version, not at the literal `v4.0.0` - `nvm.sh`: detect `NVM_DIR` without `$0`/`$_`; fall back to the default install location instead of the working directory - `nvm_process_nvmrc_content`: reject control characters - `nvm_download_artifact`, `nvm debug`: mask credentials in mirror URLs when printing them - `nvm_install_binary`, `nvm_install_source`: only use a tarball that `nvm_download_artifact` put in the cache - `nvm_download`, `nvm_get_latest`, `install.sh`: pass `-q` to curl first, so `.curlrc` is ignored again - `nvm_is_version_installed`: fail when `nvm_version_path` fails - `nvm use`, `nvm-exec`: stop option parsing at `--` - `nvm install`: validate an inherited `NVM_MAKE_JOBS` like `-j` - `nvm_is_natural_num`: reject anything but digits - `nvm_remote_version`: match the requested version as a whole version component - `install.sh`: a failed script-method download no longer clobbers the install or reports success - `nvm_ls`: match the version pattern literally - `bash_completion`: list alias and version names without expanding them - `nvm_sanitize_auth_header`: strip newlines, so wrapped `base64` credentials work - `nvm_cd`: bypass a user-defined `cd` function (#3836) - `nvm use`: handle regex-special characters in `$NVM_DIR` (#3939) - `install.sh`: start a new line before appending to a profile that does not end with one (#3952) - Handle filesystem identity when checking path containment (#3921) - `nvm alias`: do not drop output from background jobs on bash 3.2 - `nvm`: do not change or export the caller's `IFS` - `nvm install`: do not remove the caller's `TMPDIR` after a failed download - `nvm exec`/`nvm run`: do not treat filenames with digits as versions Refactors - `install.sh`: check profile lines through one comment-aware helper (#3976) Robustness - `nvm_nvmrc_invalid_msg`: only show an excerpt of the parsed content - `install.sh`, `nvm uninstall`: pass the version to grep with `-e` Performance - `nvm_print_implicit_alias`: classify implicit aliases in one awk pass (#3980) - `nvm_is_installed_exact_version`: also take the fast path for an exact prerelease - `nvm_version_greater`, `nvm_version_greater_than_or_equal_to`: avoid `awk` where possible (#3949) - `nvm_is_valid_version`, `nvm_validate_implicit_alias`: avoid forks (#3949) - `nvm_echo`: use the `printf` shell builtin under zsh (#3949) - `nvm_get_os`: read `/etc/os-release` instead of forking `uname -a` (#3949) - `nvm_npmrc_bad_news_bears`: scan npmrc files without forking `grep` (#3949) - `nvm_die_on_prefix`: detect the OS only when it is needed (#3949) - `nvm_version`: resolve an exact, installed version without `nvm_ls` (#3949) - `nvm_is_version_installed`: check the modern layout's path before the full lookup (#3949) - `nvm_resolve_local_alias`: skip `nvm_version` for an exact, installed version (#3937) - `nvm use`: skip re-resolving an exact, installed version (#3937) Docs - [readme] `cdnvm`: let nvm parse `.nvmrc`, so a symlinked `.nvmrc`'s content is not printed - [readme] Fix a Docker warning from using Docker installation instructions (#3927) - [readme] explain the misleading `npm install` error from `npm run test/fast` (#3944) Misc - [meta] `Makefile`: default `NVM_DIR` to the checkout when it is not set - [meta] `release-notes.sh`: replace control characters in commit subjects Tests - make fast tests pass as root, without `sudo`, and without a terminal - [actions] TOC: use `node/install` instead of `setup-node` - pin `--lts` and `node` installs to end-of-life lines, so a new node release can't break them - `bash_completion`: don't let zsh's `compinit` wait for an answer - [actions] `tests-alpine`: time out jobs well before GitHub's 6-hour limit - `nvm_download`: point the httpbin Authorization check at a configured mirror - `nvm_get_arch_unofficial`: set `NVM_DIR` when sourcing `nvm.sh` in the chroots - `nvm install v1 works`: get the version from `nvm_remote_version`, not the `ls-remote` display - [actions] cache container images, verified against digests looked up live - [actions] pull container images through Google's Docker Hub mirror - [actions] never let an apt install wait on a prompt - [actions] retry apt package installs through Ubuntu mirror outages - add unit coverage for `nvm_version_greater_than_or_equal_to` (#3942) - add unit tests for `nvm_version_greater_than_or_equal_to` (#3941) - `nvm install --reinstall-packages-from`: mock the remote version instead of fetching it - [actions] run the fast suite on macOS and WSL too - make the fast suite pass on macOS - [actions] rename `windows-npm` workflow to `windows-nvm`, since it tests `nvm install`
Problem
nvm_change_path(used bynvm useforPATHandMANPATH) interpolates${NVM_DIR}and the new version directory straight intogrepandsedexpressions. If$NVM_DIRcontains a regex or sed-special character, the resultingPATHis 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) andPATHis emptied[,*,\: the "already contains an nvm path" check silently fails, so the old version's directory stays onPATHnext to the new oneCause
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. Thegrepandsedcalls switch to-Eso 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 existingnvm_change_pathandnvm_strip_pathtests still pass. In the fulltest/fastrun 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.