Repository navigation
fix(config): confine config-driven filesystem and env access - #586
Conversation
|
Standing release PR: #582 · v0.41.1 queued (4 packages) · open 2h · ✅ ready to merge Release Preview — v0.41.0 → v0.41.1
These changes will be added to the release PR (#582) when merged: Changelogmonorepo 0.41.0 → 0.41.1
After merge — predicted release
Updated automatically by ReleaseKit |
1a6f1b6 to
4f026ad
Compare
Prevent a malicious or mistaken config from driving reads/writes outside
the repository.
- version.cargo.paths / version.pub.paths manifest writes are confined to
the workspace root, in both the async PackageProcessor path and the
sync/single updateCargoFiles path; an entry escaping the root via `..`
or an absolute path is rejected with a VersionError (UNSAFE_CONFIG_PATH).
- npm.copyFiles source and destination are confined to the repo root; a
`../` or absolute entry is rejected with a FILE_COPY_ERROR.
- {file:} substitution is confined to the config file's directory (the old
`~` home-dir expansion is dropped) so a config can no longer read files
such as ~/.ssh/id_rsa or /etc/passwd; an escaping reference throws a
ConfigError, while an in-bounds-but-unreadable path still resolves to
empty. {env:} is unchanged — it is the trusted-operator secret-injection
mechanism — and the trust boundary is documented.
The shared boundary check lives in @releasekit/core (isPathWithinRoot).
Closes #559
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BT6fVBZrmExXZj7vQ29881
4f026ad to
0d17ccc
Compare
|
Re: Greptile "Two unreachable conditions in boundary predicate" ( Partially applied:
"Always true on POSIX" is correct, but the predicate is cross-platform. |
Summary
From the 2026-07-19 security review (#559): config-supplied paths and the
{file:}/{env:}substitution primitive were unconfined, letting a config drive writes outside the repo and reads of arbitrary files. This grouped fix confines all three surfaces to the repository root.version.cargo.paths/version.pub.pathsare resolved and confined to the workspace root before writing. A../…or absolute entry that escapes the tree is rejected with a clearVersionError(UNSAFE_CONFIG_PATH) instead of rewriting a foreignCargo.toml/pubspec.yaml. Applied to both the asyncPackageProcessorand the sync/singleupdateCargoFileswrite paths.npm.copyFiles— both the copy source and destination must resolve inside the repo root, else the prepare stage throws aFILE_COPY_ERROR.{file:}substitution — confined to the config file's directory; the previous~home-directory expansion (the{file:~/.ssh/id_rsa}exfiltration vector) is removed. An out-of-bounds reference throws aConfigError; an in-bounds but unreadable path still resolves to an empty string.{env:}substitution — intentionally left unconstrained: it is the trusted-operator injection point for secrets (npm/LLM tokens), so allowlisting it would break every consumer. The trust boundary — config values can read files/env and may surface in bot-authored output — is documented on the substitution helper and in the config README.The boundary predicate
isPathWithinRootis added to@releasekit/coreand shared acrossconfig,version, andpublish.Test plan
@releasekit/corepaths.spec.ts;@releasekit/versionconfinedPath.spec.ts+versionStrategies.spec.ts(threaded root);@releasekit/configsubstitute.spec.ts(confinement + escape cases);@releasekit/publishprepare.spec.ts(escape rejected).main(which now includes Publish hardening: leading-dash guard on gh calls; 0600 mode on temp .npmrc #561); the two@releasekit/coreindex exports coexist. Re-verified the CI way:biome check .exit 0,pnpm schema:checkup to date, forcedturbo run typecheck test→ 24/24.Closes #559
🤖 Generated with Claude Code
https://claude.ai/code/session_01BT6fVBZrmExXZj7vQ29881
Greptile Summary
This PR closes the config-driven path-traversal attack surface identified in security review #559 by introducing a shared
isPathWithinRootboundary predicate in@releasekit/coreand applying it consistently across the three unconfined surfaces.{file:}substitution — confined to the config file's own directory; the~home-directory expansion vector is removed and out-of-bounds references now throw aConfigErrorrather than silently reading arbitrary files.version.cargo.paths/version.pub.pathsmanifest writes — both thePackageProcessor(async strategy) and theupdateCargoFileshelper (sync/single strategies) now resolve config-supplied paths throughresolveConfinedManifestPath, throwingUNSAFE_CONFIG_PATHfor any path that escapes the workspace root.npm.copyFiles— both the read source and the write destination are checked againstcwdbefore the copy executes; an escaping entry throwsFILE_COPY_ERROR.Confidence Score: 5/5
Safe to merge — all three unconfined surfaces are now gated by
isPathWithinRootbefore any filesystem read or write, with no regressions to the happy path.The boundary predicate is correct for POSIX and Windows cross-drive paths, the
resolveConfinedManifestPathhelper is applied consistently across both the asyncPackageProcessorand the sync/singleupdateCargoFilespaths, and the{file:}confinement throws before anyreadFileSynccall. The~-expansion exfiltration vector is fully removed. Tests cover equal-root, child, sibling-prefix,..-escape, absolute-outside, and the..fooname edge case. No off-by-one or partial-coverage gaps were found.No files require special attention.
Important Files Changed
isPathWithinRoot; handles equal-path,..-escape, and Windows cross-drive cases correctly. The!path.isAbsolute(relative)guard is intentionally retained for Windows even though it is always false on POSIX.~-expansion, addsisPathWithinRootguard before everyfs.readFileSync;osimport retained forloadAuth/saveAuth. TherootDirdefault ofprocess.cwd()is a safe fallback for the public API surface.path.dirname(configPath)as the confinement root intosubstituteInObject, confining{file:}reads to the config file's directory.resolveConfinedManifestPath; correctly resolves config-supplied paths and rejects escaping entries with a typedVersionError(UNSAFE_CONFIG_PATH).repoRootparameter toprocessPackages; routes bothcargoPathsanddartPathsentries throughresolveConfinedManifestPath.repoRoot(derived frompackages.root ?? process.cwd()) through all three strategy paths (sync, single, async); theprocess.cwd()fallback is conservative and documented.src(resolved viapath.resolve) anddest(joined viapath.join); thesrccheck catches absolute-path injection even whenpath.joinembeds the absolute component as a relative segment.UNSAFE_CONFIG_PATHerror code with a clear user-facing message and actionable suggestions.isPathWithinRootfrom the newpaths.tsmodule...fooedge case (name starts with..) correctly passes.~-expansion test (removed feature), adds confinement and escape tests. Calls withoutrootDirare all env-only scenarios so theprocess.cwd()default is never exercised with a{file:}reference.resolveConfinedManifestPath; covers in-bounds, sibling-within-root,..-escape, absolute-escape, and error-code assertions.repoRootargument ('/test/workspace') inprocessPackagescalls.../../escape.txtentry incopyFilescausesrunPrepareStageto throw with a message matching/outside the repository root/.rootDirparameter onsubstituteInObject.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Config-supplied value] --> B{Which surface?} B --> C["{file:PATH} substitution\n(substitute.ts)"] B --> D["cargo.paths / pub.paths\n(confinedPath.ts + packageProcessor.ts)"] B --> E["npm.copyFiles\n(prepare.ts)"] B --> F["{env:NAME} substitution"] C --> C1["path.resolve(rootDir, filePath)"] C1 --> G{isPathWithinRoot?} G -- No --> H["throw ConfigError\n(no read performed)"] G -- Yes --> I["fs.readFileSync\n(or '' on ENOENT)"] D --> D1["resolveConfinedManifestPath(repoRoot, pkgDir, path, file)"] D1 --> D2["path.resolve(pkgDir, configuredPath, manifest)"] D2 --> G2{isPathWithinRoot?} G2 -- No --> J["throw VersionError\nUNSAFE_CONFIG_PATH"] G2 -- Yes --> K["Write manifest"] E --> E1["src = path.resolve(cwd, file)\ndest = path.join(pkgDir, file)"] E1 --> G3{"isPathWithinRoot(cwd, src)\n&& isPathWithinRoot(cwd, dest)?"} G3 -- No --> L["throw PublishError\nFILE_COPY_ERROR"] G3 -- Yes --> M["fs.copyFileSync(src, dest)"] F --> N["process.env lookup\n(unconstrained — operator trust boundary)"] style H fill:#f88,color:#000 style J fill:#f88,color:#000 style L fill:#f88,color:#000%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A[Config-supplied value] --> B{Which surface?} B --> C["{file:PATH} substitution\n(substitute.ts)"] B --> D["cargo.paths / pub.paths\n(confinedPath.ts + packageProcessor.ts)"] B --> E["npm.copyFiles\n(prepare.ts)"] B --> F["{env:NAME} substitution"] C --> C1["path.resolve(rootDir, filePath)"] C1 --> G{isPathWithinRoot?} G -- No --> H["throw ConfigError\n(no read performed)"] G -- Yes --> I["fs.readFileSync\n(or '' on ENOENT)"] D --> D1["resolveConfinedManifestPath(repoRoot, pkgDir, path, file)"] D1 --> D2["path.resolve(pkgDir, configuredPath, manifest)"] D2 --> G2{isPathWithinRoot?} G2 -- No --> J["throw VersionError\nUNSAFE_CONFIG_PATH"] G2 -- Yes --> K["Write manifest"] E --> E1["src = path.resolve(cwd, file)\ndest = path.join(pkgDir, file)"] E1 --> G3{"isPathWithinRoot(cwd, src)\n&& isPathWithinRoot(cwd, dest)?"} G3 -- No --> L["throw PublishError\nFILE_COPY_ERROR"] G3 -- Yes --> M["fs.copyFileSync(src, dest)"] F --> N["process.env lookup\n(unconstrained — operator trust boundary)"] style H fill:#f88,color:#000 style J fill:#f88,color:#000 style L fill:#f88,color:#000Comments Outside Diff (1)
packages/core/src/paths.ts, line 231-232 (link)The early-return
if (resolvedTarget === resolvedRoot) return trueon line 228 ensures that whenresolvedTargetequalsresolvedRoot, we return early. After that branch,path.relative(resolvedRoot, resolvedTarget)will never return''(which only happens when the two paths are identical), makingrelative !== ''permanentlytrueand therefore redundant. Similarly,!path.isAbsolute(relative)is alwaystrueon POSIX becausepath.relativenever produces an absolute path. Neither condition is harmful, but both add noise to the security-critical boundary predicate and could mislead a future reader into thinking they guard meaningful cases.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (3): Last reviewed commit: "fix(config): confine config-driven files..." | Re-trigger Greptile