Repository navigation
fix(registry): keep project npmrc env refs literal - #856
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds an untrusted .npmrc parser that preserves ChangesUntrusted .npmrc Parsing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Greptile SummaryThis PR hardens registry auth handling by stopping
Confidence Score: 3/5The env-expansion security fix is correctly implemented, but the loader change silently reverses a documented pnpm invariant — user-declared auth files now lose to the project The core security goal (no env expansion in project-controlled sources) is sound and well-tested. However, the two-call pattern in crates/aube-registry/src/config/load.rs — specifically the ordering of UserNpmrcAuthFile entries relative to Project entries and whether the precedence reversal is intentional and safe for existing setups. Important Files Changed
Reviews (3): Last reviewed commit: "fix(registry): preserve user auth file p..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /// Parse a repository-controlled .npmrc file without `${VAR}` expansion. | ||
| pub(super) fn parse_npmrc_untrusted(path: &Path) -> Result<Vec<(String, String)>, std::io::Error> { | ||
| parse_npmrc_inner(path, false) | ||
| } |
There was a problem hiding this comment.
The widely-used pattern of placing ${NPM_TOKEN} (or ${NEXUS_URL}:_auth=${NEXUS_TOKEN}) in a project .npmrc to receive CI-injected credentials is now broken without any diagnostic. Before this change, aube explicitly regression-fixed this (#519) because "pnpm/npm both substitute ${VAR} in keys as well as values." With parse_npmrc_untrusted the literal string ${NPM_TOKEN} is stored as the auth token; every registry request will receive that literal string and get a 401, with no indication that env expansion was suppressed. A warning log when a value matching \$\{[^}]+\} is stored from an untrusted source would help users diagnose this failure.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/aube-registry/src/config/tests.rs (1)
149-156: 💤 Low valueConsider consolidating duplicate RAII guard.
The new
ScopedEnvVarshelper (lines 7-15) provides the same functionality as this localEnvVarsstruct. Consider replacing this withScopedEnvVarsfor consistency.♻️ Proposed refactor
- struct EnvVars(&'static [&'static str]); - impl Drop for EnvVars { - fn drop(&mut self) { - for name in self.0 { - unsafe { std::env::remove_var(name) }; - } - } - } - let _vars = EnvVars(&["AUBE_TEST_NEXUS_HOST_CFG", "AUBE_TEST_NEXUS_TOKEN_CFG"]); + let _vars = ScopedEnvVars(&["AUBE_TEST_NEXUS_HOST_CFG", "AUBE_TEST_NEXUS_TOKEN_CFG"]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aube-registry/src/config/tests.rs` around lines 149 - 156, The local RAII guard EnvVars (struct EnvVars and its Drop impl) duplicates ScopedEnvVars; remove EnvVars and replace its usage in the tests with ScopedEnvVars so the same cleanup behavior is reused. Find the EnvVars definition and any places creating EnvVars instances, delete the struct and Drop impl, and construct a ScopedEnvVars instead (import or fully qualify the module where ScopedEnvVars lives) so environment variables are removed on drop exactly as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/aube-registry/src/config/tests.rs`:
- Around line 149-156: The local RAII guard EnvVars (struct EnvVars and its Drop
impl) duplicates ScopedEnvVars; remove EnvVars and replace its usage in the
tests with ScopedEnvVars so the same cleanup behavior is reused. Find the
EnvVars definition and any places creating EnvVars instances, delete the struct
and Drop impl, and construct a ScopedEnvVars instead (import or fully qualify
the module where ScopedEnvVars lives) so environment variables are removed on
drop exactly as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b4cd8093-b140-456f-89dc-1d77c21fab84
📒 Files selected for processing (4)
crates/aube-registry/src/config.rscrates/aube-registry/src/config/load.rscrates/aube-registry/src/config/npmrc.rscrates/aube-registry/src/config/tests.rs
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4da94a3. Configure here.
Summary
Stop expanding
${VAR}references from repository-controlled.npmrcsources before registry/auth config is applied. User/global.npmrcand env-derived config keep trusted substitution, while project.npmrcand project-reachablenpmrc-auth-fileentries now preserve env references literally.This prevents cloned repositories from turning the caller's environment into registry destinations or credentials.
Validation
This PR was generated by Codex.
Note
High Risk
Changes trusted vs untrusted registry auth parsing and credential application—security-critical—and may break projects that relied on env-expanded auth in committed
.npmrc.Overview
Hardens registry auth so repository-controlled
.npmrccannot turn the caller's environment into registry URLs or credentials.Project
.npmrcand project-reachablenpmrc-auth-filecontent are parsed withparse_npmrc_untrusted, which leaves${VAR}literals in keys and values. Duringapply_tagged, auth entries from project-controlled sources that still contain env references are dropped andWARN_AUBE_UNTRUSTED_AUTH_ENVis logged. User/global sources (~/.npmrc, pnpmauth.ini, user-declared auth files) keep full${…}expansion.NpmrcSource::NpmrcAuthFileis split intoUserNpmrcAuthFilevsProjectNpmrcAuthFileaccording to which layer setnpmrc-auth-file, with matching trust for subprocess settings and load order. Tests and the error-code catalog document the new warning.Reviewed by Cursor Bugbot for commit 12a2616. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit