Repository navigation
Conversation
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
ljharb
left a comment
ljharb
left a comment
There was a problem hiding this comment.
this is a massive PR diff, and i have no idea why nvm_download tests are deleted.
Any changes that are not strictly relevant to setting Proxy-Authorization headers do not belong in this PR - if you want to add dockerfile linting, or update AGENTS.md, or whatever, please do each thing in a separate PR.
I just needed some way to ensure these changes to support the proxy-authorization headers would not affect something else so I made updates to get the entire test suite working for me through a container. I can move all that out in separate PRs. |
|
one of your added tests is failing |
…ication When downloads are routed through a proxy that requires its own credentials (for example via http_proxy or https_proxy), set NVM_PROXY_AUTH_HEADER to pass a Proxy-Authorization header to the proxy. It is independent of NVM_AUTH_HEADER, which passes an Authorization header through to the origin. The value is sanitized with nvm_sanitize_auth_header before being appended to the curl or wget invocation in nvm_download. Add a test in the fast suite that installs a node release, runs the dependency-free mock proxy (test/mocks/mock-proxy.js) with that installation's node binary, and uses a local httpbin container as the origin to verify the four combinations of Authorization and Proxy-Authorization headers: no headers fails, an Authorization header alone fails, a Proxy-Authorization header alone fails, and both headers together succeed. On Alpine, node -musl binaries are served by the unofficial-builds mirror (the default nodejs.org mirror serves no arm64-musl artifacts), and a -musl binary needs a libstdc++ as new as the Alpine it was built on, so older Alpine releases can only run older node releases: the test selects that mirror and tries candidate releases newest first (lts/*, then v20.20.1, v18.20.4, v14.21.3 - the same-era pairings locked by the 'musl-binary' CI job), keeping the first whose binary runs. The httpbin-dependent checks are skipped, rather than failing, if the image cannot be pulled or run, and the test itself skips the same way if no candidate release has a runnable binary (for example arm64 on Alpine 3.15, which no arm64-musl release targets). Fixes: nvm-sh#3003 Assisted-by: omp:llama.cpp:ggml-org/Qwen3.8-27B-GGUF
| if [ -n "${NVM_PROXY_AUTH_HEADER:-}" ]; then | ||
| set -- "$@" --header "Proxy-Authorization: ${sanitized_proxy_header}" | ||
| fi |
There was a problem hiding this comment.
--header is never sent on the CONNECT request, so this gets a 407 for every https:// download. Worse, when the proxy is authenticated some other way, the header goes through the tunnel to the origin server. curl has --proxy-header for exactly this (since 7.37.0, 2014). It's sent to the proxy only, both on CONNECT and on plain HTTP requests.
wget has no way to send an arbitrary header to the proxy (only --proxy-user/--proxy-password, which are Basic), so under wget this should fail clearly rather than fall back to --header:
| if [ -n "${NVM_PROXY_AUTH_HEADER:-}" ]; then | |
| set -- "$@" --header "Proxy-Authorization: ${sanitized_proxy_header}" | |
| fi | |
| if [ -n "${NVM_PROXY_AUTH_HEADER:-}" ]; then | |
| if [ "${NVM_DOWNLOADER}" != 'curl' ]; then | |
| nvm_err "\$NVM_PROXY_AUTH_HEADER requires curl; with wget, put Basic proxy credentials in the proxy URL instead (eg, https_proxy=http://user:pass@proxy.example.com:3128)" | |
| return 1 | |
| fi | |
| set -- "$@" --proxy-header "Proxy-Authorization: ${sanitized_proxy_header}" | |
| fi |
(I checked this in sh/dash/bash/zsh against the mock proxy: an HTTPS download through it succeeds, the header appears only on the CONNECT and not on the request to the origin, and under wget it errors without running wget.)
|
|
||
| ```sh | ||
| export http_proxy="http://proxy.example.com:3128" | ||
| export https_proxy="http://proxy.example.com:3128" | ||
| export NVM_PROXY_AUTH_HEADER="Basic dXNlcjpwYXNzd29yZA==" | ||
| nvm install node |
There was a problem hiding this comment.
This example is Basic auth, which already works with no nvm change: export https_proxy="http://user:password@proxy.example.com:3128" (and the same for http_proxy). I think this section should lead with that, and present $NVM_PROXY_AUTH_HEADER as being for other schemes (eg, Bearer), noting that it requires curl.
| # no Authorization and no Proxy-Authorization: expected fail | ||
| http_proxy="${proxy_url}" nvm_download "http://127.0.0.1/get" > /dev/null && die 'nvm_download with no Authorization and no Proxy-Authorization header should fail' | ||
| # Authorization but no Proxy-Authorization: expected fail | ||
| NVM_AUTH_HEADER="Bearer test-token" http_proxy="${proxy_url}" nvm_download "http://127.0.0.1/bearer" > /dev/null && die 'nvm_download with Authorization but no Proxy-Authorization header should fail' | ||
| # no Authorization but Proxy-Authorization: expected fail | ||
| NVM_PROXY_AUTH_HEADER="Bearer proxy-token" http_proxy="${proxy_url}" nvm_download "http://127.0.0.1/bearer" > /dev/null && die 'nvm_download with Proxy-Authorization but no Authorization header should fail' | ||
| # Authorization and Proxy-Authorization: expected success | ||
| NVM_AUTH_HEADER="Bearer test-token" NVM_PROXY_AUTH_HEADER="Bearer proxy-token" http_proxy="${proxy_url}" nvm_download "http://127.0.0.1/bearer" > /dev/null || die 'nvm_download with Authorization and Proxy-Authorization headers should succeed' |
There was a problem hiding this comment.
All of these use a plain http:// origin, so the proxy only ever sees absolute-form requests, and mock-proxy.js's CONNECT handler never runs. That's the path that's broken (and that nvm actually uses for nodejs.org). Please add an HTTPS case that goes through CONNECT, and ideally assert that Proxy-Authorization reaches the proxy but not the origin, eg by having httpbin echo the headers it received.
| for NODE_CANDIDATE in 'lts/*' v20.20.1 v18.20.4 v14.21.3; do | ||
| nvm install -b --no-progress --skip-default-packages "${NODE_CANDIDATE}" || continue | ||
| CURRENT_VERSION="$(nvm current)" | ||
| if [ -n "${CURRENT_VERSION}" ] && [ "${CURRENT_VERSION}" != 'none' ]; then | ||
| NODE_VERSION="${CURRENT_VERSION}" | ||
| break | ||
| fi | ||
| nvm uninstall "${NODE_CANDIDATE}" >/dev/null 2>&1 || true | ||
| done | ||
| if [ -z "${NODE_VERSION}" ]; then | ||
| echo 'skip: no node release with a runnable binary on this system (tried: lts/*, v20.20.1, v18.20.4, v14.21.3)' | ||
| exit 0 |
There was a problem hiding this comment.
This installs a full Node release from the network inside a fast test, only to run the mock proxy, and never uninstalls it, so it stays in $NVM_DIR/versions. If Node can't be installed, the test exits 0, so it "passes" without testing anything. Can the mock run some lighter way, or the test move out of fast and clean up after itself?
Alternatively, a much cheaper test wouldn't need a proxy at all: put an executable curl stub first on PATH that records its arguments (like the MOCK_BIN in test/fast/Unit tests/nvm_echo), then assert that nvm_download passes --proxy-header 'Proxy-Authorization: …' and never --header 'Proxy-Authorization: …'. (A shell function named curl won't work: nvm_download runs command curl, which skips functions; see test/fast/Unit tests/nvm_download and curl helpers bypass shadowed curl.) That only proves nvm passes the right flag, not that curl sends it to the proxy, but the mock proxy doesn't prove that either until the test has an HTTPS case.
| # `nvm install` and `nvm ls-remote` create aliases; keep the checkout clean | ||
| nvm_alias_dir="$(nvm_alias_path)" | ||
| nvm_alias_bak='' | ||
| if [ -d "${nvm_alias_dir}" ]; then | ||
| nvm_alias_bak="${nvm_alias_dir}.test-bak" | ||
| mv "${nvm_alias_dir}" "${nvm_alias_bak}" | ||
| fi |
There was a problem hiding this comment.
This moves the real $NVM_DIR/alias aside under set -ex, but cleanup only runs from die or at the very end; there's no trap. If any command fails outside an || die, the script exits with the aliases still sitting in alias.test-bak. A trap cleanup EXIT (or avoiding the move entirely) would make this safe.
4 participants
This PR adds changes to support setting "Proxy-Authorization" HTTP headers. This will allow for nvm with proxies that require authentication. The "Proxy-Authorization" headers are set through the
NVM_PROXY_AUTH_HEADERenvironment variable which will behave similar to theNVM_AUTH_HEADERenvironment variable for setting "Authorization" headers.Other changes included in this PR are updates to the dev container to make it easier to develop on the dev container and run the full test suite. I did this to verify I did not break anything else with the new "Proxy-Authorization" header support.
This fixes #3003.