Skip to content

fix(command-code): preserve case in project-context confinement - #673

Closed
luvs01 wants to merge 2 commits into
devfrom
codex/fix-windows-case-folding-vulnerability
Closed

luvs01 wants to merge 2 commits into
devfrom
codex/fix-windows-case-folding-vulnerability

Conversation

@luvs01

@luvs01 luvs01 commented Sep 29, 2026 •

Copy link
Copy Markdown
Owner

Motivation

  • Prevent a Windows-specific path-containment bypass where case-folding and path.relative semantics could admit files from a case-distinct sibling directory into the Command Code projectContext envelope.
  • Ensure that post-open validation and canonical containment checks use exact, case-preserving identity so outside files cannot be read and sent by the loader.

Description

  • Replace the case-folding + relative-based containment logic with an exact, case-preserving canonical-prefix check implemented in isContainedCanonicalPath and remove the normalizePathIdentity folding. (src/adapters/command-code-project-context.ts).
  • Strengthen the post-open validation to require exact canonical-path equality rather than case-insensitive equality when confirming the opened file remains confined. (src/adapters/command-code-project-context.ts).
  • Add a focused regression test that asserts Windows-style, case-colliding sibling directories are not considered contained. (tests/providers/command-code-project-context.test.ts).
  • Update adapter contract documentation to state the exact, case-preserving canonical containment requirement. (structure/providers-and-adapters.md).

Testing

  • Ran bun run typecheck and it completed successfully.
  • Ran bun run structure:check and it completed successfully.
  • Ran bun run privacy:scan and it completed successfully.
  • Ran git diff --check and it reported no issues.
  • Attempted bun test tests/providers/command-code-project-context.test.ts, but the local test runner failed to start due to the installed Bun runtime lacking the node:zlib zstdDecompressSync export required by the test environment; this is an environment limitation and the test file was added to cover the regression.

Codex Task


Devin Review

Summary by CodeRabbit

  • Bug Fixes
    • Project file access and skill loading now use exact, case-preserving canonical path checks. Files whose paths differ in capitalization, or that fall outside the project directory, are not treated as contained. A skill is also rejected if its canonical path changes capitalization after it is opened.

@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: luvs01/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: e2e528c5-04a9-4929-8cdb-ccdd202477d8

📥 Commits

Reviewing files that changed from the base of the PR and between 017e399 and 517a1a4.

📒 Files selected for processing (1)
  • tests/providers/command-code-project-context.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

Path containment now uses exact, case-preserving canonical path comparisons. Opened-file confinement requires exact resolved-path equality. Documentation and tests describe these checks, including Windows case differences.

Changes

Canonical path checks

Layer / File(s) Summary
Containment and confinement behavior
src/adapters/command-code-project-context.ts, structure/providers-and-adapters.md, tests/providers/command-code-project-context.test.ts
isContainedCanonicalPath checks exact equality or a separator-terminated prefix. openedFileIsConfined requires exact resolved-path equality. Tests cover filesystem roots and case-different Windows paths. Documentation describes the canonical path checks.

Priority: ⬆️ High

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: lidge-jun

Merge Risk: ⚪ Minimal · up to 517a1

The change tightens skill-file confinement without an identified functional regression. No actionable merge-blocking risk remains; normal validation, including the Windows regression tests, should complete before release.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 517a1

The change tightens file confinement without expanding read authority. No introduced or worsened security concern was established. Native Windows race behavior and execution of the regression tests remain unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • observed — The affected disclosure surface is locally readable skill-file content selected for a project-context scan and placed in its returned XML. The scoped evidence establishes that return boundary, but not subsequent provider transmission.

Trust Boundaries and Controls

  • observed — Filesystem paths cross into trusted project context only after canonical confinement and opened-file identity validation. The PR tightens case identity while retaining regular-file, device/inode, and before-and-after-read checks. Separator termination prevents sibling-prefix collisions.

Resilience and Maintainability Implications

  • observed — Windows confinement remains a best-effort race defense because it lacks the POSIX no-follow opening controls used here. This limitation predates the PR; exact-case validation strengthens the existing checks but does not establish atomic Windows confinement.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: preserving case during Command Code project-context confinement to prevent Windows path-containment bypasses.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Sep 29, 2026
devin-ai-integration[bot]

This comment was marked as resolved.

@github-actions

Copy link
Copy Markdown

@coderabbitai review

@luvs01

luvs01 commented Sep 29, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai rate limit

@coderabbitai

coderabbitai Bot commented Sep 29, 2026

Copy link
Copy Markdown

Your plan includes PR reviews subject to rate limits. More reviews will be available in 21 minutes.

@luvs01

luvs01 commented Sep 29, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

…r level

The strict post-open equality check had only unit coverage through isContainedCanonicalPath. Drive it through the loader: after the file's canonicalization, make the post-open realpath return the same file with a differently cased name; containment still passes while resolved !== path rejects, so the skill is refused.
@luvs01

luvs01 commented Sep 30, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@luvs01

luvs01 commented Oct 5, 2026

Copy link
Copy Markdown
Owner Author

Retrospective closure record (2026-10-05): this explanation is being added after the original close, not recorded as an earlier comment.

The implementation at 517a1a4ba59b1a68ff55610695f95c66badf1c83 is preserved in landed commit faf946e8bf27ebe3f673f6e22a1f191772d21495, which is an ancestor of the checked dev revision 829a18ba94941aa7997d34cbd88f97390844d79b. The production file src/adapters/command-code-project-context.ts has the same blob c79eff473c149b9a1047c28ae94e075a5763766c in the fork head and the landed change; the latter also strengthened the Windows drive/UNC and post-open regression coverage.

The fix has not been abandoned. This duplicate proposal remains closed because the implementation has landed; reopening or repushing this historical head would duplicate completed work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aardvark bug Something isn't working codex

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant