fix(security): keep paths on a short leash - #1499
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds canonical path validation across artifact, schema, specification, project, archive, and local-state operations. It preserves symlinks during archive fallback, protects lock ownership, and adds regression coverage for traversal and symlink escapes. ChangesPath safety enforcement
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
test/core/archive.test.ts (1)
157-179: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd coverage for an OpenSpec root accessed through a symlink alias.
ArchiveCommandcanonicalizes existing parent directories duringassertPathWithin, so a root symlink like macOS/tmp→/private/tmpwould not triggerarchive_path_outside_root. This test only covers a symlinkedarchiveDirinside the managed tree; add a regression that creates the OpenSpec structure under a symlink alias and archives normally.🤖 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 `@test/core/archive.test.ts` around lines 157 - 179, Add a regression test alongside the existing archive tests that creates the OpenSpec directory structure through a symlinked root alias, prepares a change, and invokes archiveCommand.execute normally. Assert the archive succeeds and the expected change is archived, covering root canonicalization without triggering archive_path_outside_root; skip the test on Windows if symlink support is unavailable.Source: Path instructions
src/commands/workflow/templates.ts (1)
70-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap the containment error for a friendlier CLI message.
assertPathWithinthrows directly inside.map(). This error propagates uncaught to the top-levelcatchintemplatesCommand, which surfaces the raw message ("Path is outside the allowed directory: ...") instead of an actionable, per-artifact message.src/commands/schema.ts'svalidateSchemacatches the same kind of error and reports which artifact and template caused it.Consider catching per-artifact here too, so users see which artifact's template escaped the schema directory.
♻️ Proposed fix for friendlier per-artifact error
const templatesDir = path.join(schemaDir, 'templates'); const templates: TemplateInfo[] = graph.getAllArtifacts().map((artifact) => { const templatePath = path.join(templatesDir, artifact.template); - FileSystemUtils.assertPathWithin(templatesDir, templatePath); + try { + FileSystemUtils.assertPathWithin(templatesDir, templatePath); + } catch { + throw new Error( + `Template '${artifact.template}' for artifact '${artifact.id}' points outside the schema templates directory` + ); + } return { artifactId: artifact.id, templatePath: FileSystemUtils.canonicalizeExistingPath(templatePath), source, }; });🤖 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 `@src/commands/workflow/templates.ts` around lines 70 - 79, Update the artifact mapping in templatesCommand so errors from assertPathWithin or canonicalizeExistingPath are caught per artifact and rethrown with the artifact ID and template name, matching validateSchema’s actionable error style. Preserve successful template generation and allow the top-level command handler to report the contextualized message.src/utils/file-system.ts (1)
107-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect containment and dangling-symlink logic.
The two-phase check (lexical, then canonical) correctly rejects paths that escape
allowedDirectorydirectly, through an intermediate symlink, or through a dangling symlink anywhere in the missing-suffix walk. Manual tracing of parent-escape, dangling-leaf, and dangling-intermediate-segment scenarios confirms the logic holds.One optional readability note:
canonicalizePotentialPathnests two try/catch blocks and three exit conditions in a single loop. Consider extracting the dangling-symlink check into a small named helper (for exampleisDanglingSymlink(path)) to make the loop body easier to scan, since this function guards a security-sensitive path.🤖 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 `@src/utils/file-system.ts` around lines 107 - 173, Optionally improve readability in FileSystem.canonicalizePotentialPath by extracting the nested dangling-symlink detection into a small named helper, such as isDanglingSymlink, and use it during the missing-segment walk while preserving the existing rejection behavior for dangling links and resolution errors.
🤖 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.
Inline comments:
In `@src/core/archive.ts`:
- Around line 317-331: Extend the existing containment-validation loop in the
archive flow to also validate the per-change `changeDir` against `changesDir`
before the `fs.stat` and subsequent processing. Reuse the same
`FileSystemUtils.assertPathWithin` and `ArchiveBlockedError` handling used for
`archiveDir` and the other managed directories, ensuring symlinked change
directories outside `changesDir` are rejected.
In `@src/core/artifact-graph/resolver.ts`:
- Around line 95-104: The resolver’s lexical name validation does not prevent
schema-directory symlink escapes. In resolveSchema() and the related
loadTemplate() path, canonicalize the candidate schema.yaml path and its
corresponding schema root, then verify the canonical file remains within that
root before returning the schema directory; reject candidates that resolve
outside their configured root while preserving existing invalid-name handling.
In `@test/core/artifact-graph/instruction-loader.test.ts`:
- Around line 45-64: Add a Windows-specific test alongside should reject a
template symlink that escapes its schema that passes a template name containing
..\\ to loadTemplate and asserts it throws TemplateLoadError. Do not skip this
case on Windows; retain the existing platform guard only for symlink behavior,
and use the same temporary schema setup and cleanup pattern.
---
Nitpick comments:
In `@src/commands/workflow/templates.ts`:
- Around line 70-79: Update the artifact mapping in templatesCommand so errors
from assertPathWithin or canonicalizeExistingPath are caught per artifact and
rethrown with the artifact ID and template name, matching validateSchema’s
actionable error style. Preserve successful template generation and allow the
top-level command handler to report the contextualized message.
In `@src/utils/file-system.ts`:
- Around line 107-173: Optionally improve readability in
FileSystem.canonicalizePotentialPath by extracting the nested dangling-symlink
detection into a small named helper, such as isDanglingSymlink, and use it
during the missing-segment walk while preserving the existing rejection behavior
for dangling links and resolution errors.
In `@test/core/archive.test.ts`:
- Around line 157-179: Add a regression test alongside the existing archive
tests that creates the OpenSpec directory structure through a symlinked root
alias, prepares a change, and invokes archiveCommand.execute normally. Assert
the archive succeeds and the expected change is archived, covering root
canonicalization without triggering archive_path_outside_root; skip the test on
Windows if symlink support is unavailable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3148a44e-1fdd-45cf-b00a-6254ac425dc9
📒 Files selected for processing (16)
.changeset/tidy-path-leash.mdsrc/commands/schema.tssrc/commands/workflow/instructions.tssrc/commands/workflow/templates.tssrc/core/archive.tssrc/core/artifact-graph/index.tssrc/core/artifact-graph/instruction-loader.tssrc/core/artifact-graph/outputs.tssrc/core/artifact-graph/resolver.tssrc/core/artifact-graph/types.tssrc/utils/file-system.tstest/core/archive.test.tstest/core/artifact-graph/instruction-loader.test.tstest/core/artifact-graph/outputs.test.tstest/core/artifact-graph/resolver.test.tstest/core/artifact-graph/schema.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/core/artifact-graph/resolver.test.ts (1)
141-151: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExercise
resolveSchemafor the escaping-file case.This regression checks only
getSchemaDir(). Add an assertion throughresolveSchema()so the public schema-loading path also proves that the containment guard rejects the symlink.Suggested regression assertion
expect(getSchemaDir('linked-file', tempDir)).toBeNull(); + expect(() => resolveSchema('linked-file', tempDir)).toThrow(/not found/u);🤖 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 `@test/core/artifact-graph/resolver.test.ts` around lines 141 - 151, Extend the escaping-file symlink test to also call the public resolveSchema function with the linked schema identifier and tempDir, asserting that it rejects the escaping schema consistently with getSchemaDir returning null. Keep the existing setup and platform guard unchanged.
🤖 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 `@test/core/artifact-graph/resolver.test.ts`:
- Around line 141-151: Extend the escaping-file symlink test to also call the
public resolveSchema function with the linked schema identifier and tempDir,
asserting that it rejects the escaping schema consistently with getSchemaDir
returning null. Keep the existing setup and platform guard unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e047eacf-2f25-4bef-a461-57e10f1fab88
📒 Files selected for processing (9)
.changeset/tidy-path-leash.mdsrc/commands/schema.tssrc/commands/workflow/templates.tssrc/core/archive.tssrc/core/artifact-graph/resolver.tstest/commands/schema.test.tstest/core/archive.test.tstest/core/artifact-graph/instruction-loader.test.tstest/core/artifact-graph/resolver.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/commands/workflow/templates.ts
- test/core/artifact-graph/instruction-loader.test.ts
- .changeset/tidy-path-leash.md
- src/core/archive.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/spec.ts (1)
112-119: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftClose the check-then-read window at the reported sites. Each site resolves the file path, runs a containment check once, and then reads the file later through a separate call. A symlink swapped during the async gap can bypass the earlier containment guarantee. Revalidate the resolved path immediately before each read and normalize the canonical trust root before reading files under symlinked capability directories.
🤖 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 `@src/commands/spec.ts` around lines 112 - 119, Close the check-then-read window by revalidating each resolved path immediately before its file read and normalizing the canonical trust root first. Apply this to src/commands/spec.ts lines 112-119 and 248-253, src/commands/change.ts lines 82-136, and src/core/validation/validator.ts lines 285-302; preserve existing not-found and validation behavior while ensuring reads under symlinked capability directories remain contained.
🧹 Nitpick comments (1)
test/core/init.test.ts (1)
160-162: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen the test oracle for both symlink cases.
The assertions only prove that the external target was not changed. They do not prove whether
InitCommand.executerejected the unsafe path or completed the intended safe behavior. Assert the expected command result and local filesystem state. If rejection is the contract, assert the rejection. If safe fallback is the contract, assert the fallback artifact and symlink state.Also applies to: 179-182
🤖 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 `@test/core/init.test.ts` around lines 160 - 162, Strengthen both symlink-case tests around InitCommand.execute by asserting the command’s expected outcome, not only that outsideDir remains unchanged. If unsafe paths must be rejected, assert the execute call rejects; otherwise assert the intended local fallback artifact and resulting symlink state, while preserving the external-target safety assertion.
🤖 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.
Inline comments:
In `@src/core/init.ts`:
- Around line 67-80: Move resolveProjectArtifactPath and
assertProjectArtifactPath into src/utils/file-system.ts as the shared
FileSystemUtils methods or exported functions, preserving their existing
containment and absolute-path checks. Remove both local helper pairs from
src/core/init.ts (lines 67-80) and src/core/update.ts (lines 74-87), and update
each file to import and use the shared implementation.
---
Outside diff comments:
In `@src/commands/spec.ts`:
- Around line 112-119: Close the check-then-read window by revalidating each
resolved path immediately before its file read and normalizing the canonical
trust root first. Apply this to src/commands/spec.ts lines 112-119 and 248-253,
src/commands/change.ts lines 82-136, and src/core/validation/validator.ts lines
285-302; preserve existing not-found and validation behavior while ensuring
reads under symlinked capability directories remain contained.
---
Nitpick comments:
In `@test/core/init.test.ts`:
- Around line 160-162: Strengthen both symlink-case tests around
InitCommand.execute by asserting the command’s expected outcome, not only that
outsideDir remains unchanged. If unsafe paths must be rejected, assert the
execute call rejects; otherwise assert the intended local fallback artifact and
resulting symlink state, while preserving the external-target safety assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d3b51add-52eb-487a-88f3-41f460ad6158
📒 Files selected for processing (22)
.changeset/tidy-path-leash.mdsrc/commands/change.tssrc/commands/spec.tssrc/core/archive.tssrc/core/artifact-graph/instruction-loader.tssrc/core/file-state.tssrc/core/init.tssrc/core/project-config.tssrc/core/specs-apply.tssrc/core/update.tssrc/core/validation/validator.tssrc/utils/spec-discovery.tstest/core/archive.test.tstest/core/artifact-graph/instruction-loader.test.tstest/core/commands/change-command.show-validate.test.tstest/core/commands/spec-command.security.test.tstest/core/file-state.test.tstest/core/init.test.tstest/core/project-config.test.tstest/core/specs-apply.security.test.tstest/core/update.test.tstest/utils/spec-discovery.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- test/core/artifact-graph/instruction-loader.test.ts
- .changeset/tidy-path-leash.md
- src/core/archive.ts
- src/core/artifact-graph/instruction-loader.ts
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Reviewed at exact head afc9df2. The path-containment primitive, linked-capability boundary, caller-side rechecks, schema/output confinement, and lock ownership changes are coherent. Isolated build plus 514 focused security, archive, init/update, artifact-graph, config, and discovery tests pass; hosted CI/security is green.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/validation/validator.ts (1)
287-303: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse
options.mainSpecsDiras the confinement root.
path.dirname(mainSpecFile)is derived frommainSpecFileitself. The lexical check then always passes, becausepath.relative(dirname(x), x)isspec.md. The canonical check only rejects aspec.mdthat is itself a symlink out of its own capability directory. Neither check confinesspecIdtooptions.mainSpecsDir, so a capability directory that is a symlink to an outside tree still resolves and is read.Pass
options.mainSpecsDiras the root. The same value flows intofindScenarioLossIssuesasmainSpecRoot(Line 300), so the assertion at Line 458 inherits the same weakness and is fixed by this change.🔒️ Proposed fix
const mainSpecFile = path.join( options.mainSpecsDir, ...specId.split('/'), 'spec.md' ); - FileSystemUtils.assertPathWithin(path.dirname(mainSpecFile), mainSpecFile); + FileSystemUtils.assertPathWithin(options.mainSpecsDir, mainSpecFile); issues.push( ...(await this.findScenarioLossIssues( plan.modified, plan.renamed, mainSpecFile, entryPath, - path.dirname(mainSpecFile) + options.mainSpecsDir )) );🤖 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 `@src/core/validation/validator.ts` around lines 287 - 303, Use options.mainSpecsDir as the confinement root in the assertPathWithin call for mainSpecFile, rather than path.dirname(mainSpecFile). Keep passing options.mainSpecsDir as mainSpecRoot to findScenarioLossIssues so its assertion uses the same root and rejects capability directories that resolve outside the configured main specs directory.
🧹 Nitpick comments (2)
src/core/artifact-graph/outputs.ts (1)
19-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider hiding the recursion state behind a small wrapper.
Four of the seven parameters are internal recursion state (
segmentIndex,visited,canonicalChangeDir,ancestors). A future caller can pass acanonicalChangeDirthat does not matchchangeDirand silently widen the allowed root. Wrap the recursion in a private inner function that closes overcanonicalChangeDir,visited, andancestors, and expose only(changeDir, directorySegments).🤖 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 `@src/core/artifact-graph/outputs.ts` around lines 19 - 37, Refactor assertGlobDirectoryTraversal so its public signature accepts only changeDir and directorySegments, moving the recursive traversal into a private inner function that closes over canonicalChangeDir, visited, and ancestors. Keep segmentIndex as inner recursion state, and ensure canonicalChangeDir is always derived from the supplied changeDir so callers cannot override the traversal root.test/commands/schema.test.ts (1)
320-322: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
it.skipIffor the platform guard.
if (process.platform === 'win32') return;reports the test as passed on Windows.it.skipIf(process.platform === 'win32')reports it as skipped, which matches the pattern used intest/core/commands/spec-command.security.test.ts.🤖 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 `@test/commands/schema.test.ts` around lines 320 - 322, Replace the early platform return in the test “should dereference a confined template link into an independent fork” with the test framework’s it.skipIf guard for Windows, preserving the existing test body while reporting the case as skipped rather than passed.
🤖 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.
Inline comments:
In `@src/commands/schema.ts`:
- Around line 247-256: Update resolveSchemaCopyPath so containment errors from
FileSystemUtils.assertPathWithin and filesystem errors from fs.realpathSync are
not replaced by the generic message. Preserve and propagate the original error
cause/message while retaining the existing successful canonical-path behavior.
---
Outside diff comments:
In `@src/core/validation/validator.ts`:
- Around line 287-303: Use options.mainSpecsDir as the confinement root in the
assertPathWithin call for mainSpecFile, rather than path.dirname(mainSpecFile).
Keep passing options.mainSpecsDir as mainSpecRoot to findScenarioLossIssues so
its assertion uses the same root and rejects capability directories that resolve
outside the configured main specs directory.
---
Nitpick comments:
In `@src/core/artifact-graph/outputs.ts`:
- Around line 19-37: Refactor assertGlobDirectoryTraversal so its public
signature accepts only changeDir and directorySegments, moving the recursive
traversal into a private inner function that closes over canonicalChangeDir,
visited, and ancestors. Keep segmentIndex as inner recursion state, and ensure
canonicalChangeDir is always derived from the supplied changeDir so callers
cannot override the traversal root.
In `@test/commands/schema.test.ts`:
- Around line 320-322: Replace the early platform return in the test “should
dereference a confined template link into an independent fork” with the test
framework’s it.skipIf guard for Windows, preserving the existing test body while
reporting the case as skipped rather than passed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4eeaa073-4ef5-4cd8-abce-0b07f62304b1
📒 Files selected for processing (21)
src/commands/change.tssrc/commands/schema.tssrc/commands/spec.tssrc/core/archive.tssrc/core/artifact-graph/outputs.tssrc/core/file-state.tssrc/core/init.tssrc/core/specs-apply.tssrc/core/update.tssrc/core/validation/validator.tssrc/utils/file-system.tssrc/utils/spec-discovery.tstest/commands/schema.test.tstest/core/archive.test.tstest/core/artifact-graph/outputs.test.tstest/core/commands/spec-command.security.test.tstest/core/file-state.test.tstest/core/init.test.tstest/core/specs-apply.security.test.tstest/core/update.test.tstest/utils/spec-discovery.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/utils/spec-discovery.ts
- src/core/specs-apply.ts
- src/core/file-state.ts
- src/core/archive.ts
- test/core/file-state.test.ts
- src/commands/spec.ts
- src/core/init.ts
- src/core/update.ts
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Re-reviewed at exact head 6e4dfce. The follow-up preserves documented linked monorepo workflows while still confining nested spec, schema, archive, and glob targets, and fork failures now retain the underlying cause. Fresh isolated build plus 353 focused security, archive, schema, init/update, apply, discovery, and lock tests pass; hosted CI/security is green.
3c7a3d4 to
e364336
Compare
2 participants
Status: LGTM 🐕
What was wrong: Files could wander.
How it was fixed: Put every path on a leash without tripping linked monorepos.
Replication / proof: 3,527 tests pass; build, types, lint, package smoke, and audit pass.
Notes / nits: All review comments resolved.