fix(config/reader): resolve relative cafile path against the .npmrc directory#11726
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 (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
🧰 Additional context used📓 Path-based instructions (1)pacquet/**/*.rs📄 CodeRabbit inference engine (pacquet/AGENTS.md)
Files:
🔇 Additional comments (3)
📝 WalkthroughWalkthroughResolve relative ChangesCA File Path Resolution Fix
Sequence Diagram(s)sequenceDiagram
participant CLI as pnpm CLI
participant Reader as loadNpmrcFiles
participant FS as Filesystem
participant Pacquet as NpmrcAuth::from_ini
CLI->>Reader: read .npmrc (with path)
Reader->>Reader: derive npmrcDir and parse cafile value
Reader->>FS: fs.readFileSync(resolvedCafilePath)
CLI->>Pacquet: Config::current passes (npmrc_text, npmrcDir)
Pacquet->>Pacquet: resolve_cafile(value, npmrcDir)
Pacquet->>FS: fs.readFileSync(resolved_cafile) (when applying TLS)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…irectory `cafile=<relative-path>` in `.npmrc` was being read via `fs.readFileSync`, which resolves relative paths against `process.cwd()`. When pnpm is invoked from a different cwd than the project (e.g. `pnpm --dir <project> install` in CI wrappers and monorepo scripts), the CA file silently failed to load: the `try/catch` in the loader dropped the CA list, the install proceeded without the configured CA, and the user only saw TLS errors against a private registry — with no log line tying back to the wrong path. Resolve relative `cafile` values in `readAndFilterNpmrc` against `path.dirname(filePath)` of the .npmrc that declared the key, before `loadCAFile` reads the file. Absolute paths (the dominant CI shape) and CLI `--cafile` are unchanged. Ref: pnpm#11624
e2daf25 to
3b88a15
Compare
The test name and the linked issue already describe the failure mode, so the 4-line preamble on the test and the 5-line in-line comment on the implementation were re-narrating what the tests document. --- Written by an agent (Claude Code, claude-opus-4-7).
…directory Pacquet's `NpmrcAuth::from_ini` used to store the `cafile=` value verbatim and pass it to `std::fs::read_to_string` at apply time. A relative path therefore resolved against the process cwd, so a project `.npmrc` containing `cafile=certs/ca.pem` reached via `pacquet --dir <proj>` from a different cwd silently failed to load the CA — same failure mode as pnpm#11624 on the TypeScript side, which the parent commit fixed by resolving against `path.dirname` of the `.npmrc`. Mirrors the parent commit on the pacquet side: - `NpmrcAuth::from_ini` now takes the directory the `.npmrc` was loaded from. A relative non-empty `cafile=` value is resolved against that directory via `npmrc_dir.join(...)`; empty and absolute values pass through unchanged. - `Config::current` tracks which of `start_dir` / home dir actually provided the `.npmrc` text and passes that path through. - The `load_cafile` doc comment that documented "matches pnpm's surprising cwd-resolution behavior" is gone; that caveat was current only as long as pnpm itself had the bug. - Existing tests updated mechanically to pass `Path::new("")` for the new parameter; four new tests cover the resolution branches (relative resolves, absolute passes through, empty passes through, end-to-end load via `apply_to` with a real tempdir-based fixture). --- Written by an agent (Claude Code, claude-opus-4-7).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11726 +/- ##
==========================================
- Coverage 90.06% 90.06% -0.01%
==========================================
Files 146 146
Lines 16795 16803 +8
==========================================
+ Hits 15127 15134 +7
- Misses 1668 1669 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
… macros
`perfectionist::macro-trailing-comma` is enforced via dylint at CI and
ran clean before the cafile port. Rustfmt reflowed two `assert_eq!`
calls in `parses_strict_ssl_true_and_false` onto multiple lines when
the `Path::new("")` argument made the line too long, but did not add
the trailing comma the dylint rule wants on the last macro argument.
---
Written by an agent (Claude Code, claude-opus-4-7).
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.51693527764,
"stddev": 0.0589456852520619,
"median": 2.50442244204,
"user": 2.7462222599999997,
"system": 3.8450544399999997,
"min": 2.43045235254,
"max": 2.61767438754,
"times": [
2.58255876954,
2.56465115454,
2.45726348054,
2.43045235254,
2.61767438754,
2.54353133654,
2.48073083154,
2.4989338015399998,
2.48364557954,
2.50991108254
]
},
{
"command": "pacquet@main",
"mean": 2.50403080174,
"stddev": 0.08460281975704594,
"median": 2.51189908904,
"user": 2.6943637599999994,
"system": 3.85162824,
"min": 2.39061941954,
"max": 2.69623114254,
"times": [
2.39061941954,
2.41583062754,
2.54422433454,
2.44071096154,
2.52180610654,
2.69623114254,
2.51031847454,
2.51347970354,
2.48397099354,
2.52311625354
]
},
{
"command": "pnpm",
"mean": 5.05457030044,
"stddev": 0.06828588447488329,
"median": 5.06184074554,
"user": 8.494114559999998,
"system": 4.41398324,
"min": 4.92567753654,
"max": 5.14092633654,
"times": [
5.12317257254,
4.97976995954,
5.07647655254,
5.10760079354,
4.92567753654,
5.02870536154,
5.10024207554,
5.04720493854,
5.01592687754,
5.14092633654
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.69900789488,
"stddev": 0.03136649566566289,
"median": 0.69039771468,
"user": 0.38746904,
"system": 1.5928463000000002,
"min": 0.68088532718,
"max": 0.7868289821800001,
"times": [
0.7868289821800001,
0.6905553481800001,
0.6996092191800001,
0.68475510918,
0.68088532718,
0.69024008118,
0.69077926818,
0.68686860118,
0.6959624791800001,
0.6835945331800001
]
},
{
"command": "pacquet@main",
"mean": 0.7073622777799999,
"stddev": 0.02913576233664158,
"median": 0.7023208446800001,
"user": 0.40391554,
"system": 1.5957576,
"min": 0.67404746518,
"max": 0.7705408811800001,
"times": [
0.7365412571800001,
0.67404746518,
0.7214390111800001,
0.7064052231800001,
0.6892322071800001,
0.6769378421800001,
0.7705408811800001,
0.7021935521800001,
0.7024481371800001,
0.69383720118
]
},
{
"command": "pnpm",
"mean": 2.76825257408,
"stddev": 0.0355846354184123,
"median": 2.76626992868,
"user": 3.361547339999999,
"system": 2.3225587999999995,
"min": 2.72956502518,
"max": 2.84407677018,
"times": [
2.7887047581799997,
2.7914647921799998,
2.84407677018,
2.72956502518,
2.7817023331799997,
2.73786962818,
2.73707630418,
2.77819094018,
2.73952627218,
2.7543489171799997
]
}
]
} |
What
cafile=<relative-path>in.npmrcis read viafs.readFileSync, which resolves relative paths againstprocess.cwd(). When pnpm is invoked from a different cwd than the project —pnpm --dir <project> installin CI wrappers, monorepo scripts,npx-style invocations — the CA file silently fails to load: thetry/catchin the loader drops the CA list, the install proceeds without the configured CA, and the user only sees TLS errors against a private registry, with no log line tying back to the wrong path.This PR resolves relative
cafile=values againstpath.dirname(<the .npmrc that declared the key>), mirroring the user-facing intent of "this CA file lives next to the project's.npmrc."Closes #11624.
Why
cafilein the project's.npmrc" is "this CA applies to this project". Resolving against whatever cwd happens to be active when pnpm runs breaks that intent the moment--diris used.try/catchdrops the CA list toundefined, the install continues, and the user only notices when TLS errors surface against a registry the system trust store happens not to cover.How
In
readAndFilterNpmrc(config/reader/src/loadNpmrcFiles.ts), after env-var substitution but before keys are written into the layer dictionary, normalize a relativecafile=topath.resolve(npmrcDir, value)wherenpmrcDir = path.dirname(filePath). The cafile then flows throughloadCAFileas an absolute path, andfs.readFileSyncresolves it correctly regardless of the active cwd.The fix only applies to values that come from a
.npmrc(workspace, user, pnpm auth). CLI--cafileand programmatic defaults are untouched.Compatibility
cafile=paths (the dominant CI shape) — no change.cdinto the project before running pnpm — no change (cwd == npmrcDir, so resolution was identical to the new behavior).pnpm --dir /other installfrom elsewhere with a relativecafile=— they currently see a silent CA-drop, and now see a correctly resolved path. Strictly an improvement.Test plan
getConfig() resolves a relative cafile= from .npmrc against the npmrc directory, not process.cwd()inconfig/reader/test/index.ts— sets up a project with.npmrc+certs/ca.pem, runsgetConfig({ cliOptions: { dir: projectDir } })from a different cwd, assertsconfig.cais loaded.getConfig() should read cafile(absolute path) still passes — same code path is unchanged for absolute paths.config.ca === undefinedconfig.cais the parsed CA bundle.Notes
This contribution was prepared with the assistance of Claude Code.
Summary by CodeRabbit