Skip to content

fix(path-validation): normalize Windows paths case-insensitively (#1689) - #1882

Open
anisayakmitra-in wants to merge 3 commits into
Nano-Collective:mainfrom
anisayakmitra-in:fix/windows-path-case-sensitivity
Open

anisayakmitra-in wants to merge 3 commits into
Nano-Collective:mainfrom
anisayakmitra-in:fix/windows-path-case-sensitivity

Conversation

@anisayakmitra-in

Copy link
Copy Markdown
Contributor

Summary

Fixes #1689.

Windows filesystem paths are case-insensitive, but Nanocoder compared them strictly as case-sensitive text (=== and startsWith). On Windows, passing an absolute path or working in a directory whose drive letter or folder name differed only in case (e.g. c:\work\proj vs C:\Work\Proj) resulted in:

  1. isDirectoryTrusted returning false for previously trusted directories and re-prompting.
  2. isValidFilePath and resolveFilePath rejecting valid absolute paths with Invalid file path: ....
  3. trustRecheck failing to recognize recorded entries.
  4. Duplicate entries in trustedDirectories when saved with varying casing.

Proposed Changes

  1. source/utils/path-validation.ts:
    • Added normalizeForCompare(p, platform = process.platform) helper that resolves paths and lowercases them on win32.
    • Updated isPathInside(target, root, platform = process.platform) to use normalizeForCompare so containment checks on Windows respect case-insensitivity without affecting POSIX platforms.
  2. source/config/preferences.ts:
    • Used normalizeForCompare in isDirectoryTrusted, trustRecheck, grantDirectoryTrust, and isProjectPreferencesPath.
    • Preserved stored paths with user casing, while comparing in normalized form.
  3. source/daemon/full-commit-trust.ts:
    • Consolidated path normalization to import normalizeForCompare from source/utils/path-validation.ts.
  4. Unit Tests & Changeset:
    • Added tests in source/utils/path-validation.spec.ts verifying cross-platform normalizeForCompare and Windows path containment case-insensitivity for isPathInside, isValidFilePath, and resolveFilePath.
    • Added tests in source/config/preferences.spec.ts verifying case-insensitive matching in isDirectoryTrusted, trustRecheck, and deduplication in grantDirectoryTrust.
    • Added patch changeset .changeset/windows-path-case-sensitivity.md.

Verification

  • All new tests pass via AVA (ava source/config/preferences.spec.ts source/utils/path-validation.spec.ts --match "*case*").
  • TypeScript typecheck (pnpm run test:types, test:types:voice, test:types:vscode) passes with 0 errors.
  • Biome check (npx biome check) passes with 0 errors.

@github-actions

Copy link
Copy Markdown
Contributor

nc-review: comments — 2 important, 1 nit

@anisayakmitra-in — a few things worth a look, none blocking.

The PR adds a normalizeForCompare(p, platform) helper in source/utils/path-validation.ts and threads it through isPathInside, isDirectoryTrusted, trustRecheck, grantDirectoryTrust and isProjectPreferencesPath, and consolidates the pre-existing Windows lowercasing in source/daemon/full-commit-trust.ts onto the same helper. The core logic is correct: lowercasing applies only on win32, POSIX behaviour is unchanged, the sibling-prefix protection still holds, the trust fingerprint is still computed from the un-lowercased resolved path so it stays stable across spellings, and stored entries keep the user's original casing — so the four symptoms in #1689 are addressed, including the plugins/host.ts isProjectTrusted case, which is fixed transitively via isDirectoryTrusted. It is not ready to merge as-is, though: the Windows-only regression tests this bug fix depends on never execute in CI, and one assertion in the new spec is a tautology that leaves the POSIX case-sensitivity guarantee unverified.

🟠 important · tests · source/utils/path-validation.spec.ts:450

The if (process.platform === 'win32') block here, and the matching block the PR adds in source/config/preferences.spec.ts, never run in CI. The shared pr-checks job is Linux-only, and the one windows-latest job in .github/workflows/pr-checks.yml runs only source/custom-tools/template.spec.ts, source/custom-tools/handler.spec.ts and source/lsp/windows-spawn.spec.ts. So isValidFilePath/resolveFilePath against a differently-cased absolute path, and all three of isDirectoryTrusted / trustRecheck / grantDirectoryTrust case-insensitivity, are dark — a regression in the fix would ship green.

This is the failure mode the project rubric names: a test that names a failure path but never triggers it. Fix: add source/utils/path-validation.spec.ts and source/config/preferences.spec.ts to the existing custom-tools-windows job in .github/workflows/pr-checks.yml (rename the job if it no longer only covers custom tools), so the Windows-gated blocks actually execute.

🟠 important · tests · source/utils/path-validation.spec.ts:431

The first assertion of this test is a tautology — it compares the same call to itself and cannot fail:

t.is(
    normalizeForCompare('/Proj/App', 'linux'),
    normalizeForCompare('/Proj/App', 'linux'),
);

The test name claims "preserves case on posix" but nothing checks it, and nothing else in the suite would catch a regression either: if normalizeForCompare lowercased on POSIX, the existing isPathInside POSIX assertions (/proj/src/app.ts inside /proj, /proj-evil outside /proj) would still pass, because lowercasing both sides of those comparisons does not change the outcome. That regression would widen the containment check on a case-sensitive filesystem — isPathInside('/Proj/src', '/proj') would wrongly return true, which is a real path-traversal boundary used by isValidFilePath, resolveFilePath and isRealPathInside.

Fix: assert the property, e.g. t.not(normalizeForCompare('/Proj/App', 'linux'), normalizeForCompare('/proj/app', 'linux')).

⚪ nit · design · source/utils/path-validation.ts

isPathInside(target, root, platform) and normalizeForCompare(p, platform) take a platform override that exists purely so the spec can simulate win32 on a Linux runner. No production caller passes it, which also makes the sep special-case and the || normalizedRoot.endsWith(path.sep) clause dead in production.

That is a reasonable seam, but it puts the security semantics of a containment check under caller control: isPathInside(a, b, 'win32') on Linux would return case-insensitive answers on a case-sensitive filesystem. A short doc comment on both functions noting that the platform argument is a test seam and that production must leave it at the default would remove the footgun without changing the code.


🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional

Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.

@github-actions github-actions Bot added the agent:comments nc-review left non-blocking findings label Oct 24, 2026
@github-actions github-actions Bot added the area:ci GitHub Actions and CI label Oct 24, 2026
@anisayakmitra-in

Copy link
Copy Markdown
Contributor Author

Thanks @nc-review! Addressed in commit 2318183:

  1. Fixed test assertion: Updated the POSIX assertion in normalizeForCompare to t.not(normalizeForCompare('/Proj/App', 'linux'), normalizeForCompare('/proj/app', 'linux')).
  2. Documented test seam: Added JSDoc comments to normalizeForCompare and isPathInside clarifying that platform is a test seam and production callers leave it at the default (process.platform).
  3. CI coverage on Windows: Added a step to custom-tools-windows in .github/workflows/pr-checks.yml running source/utils/path-validation.spec.ts and source/config/preferences.spec.ts (--match="*case on Windows*") on the windows-latest runner.

platform: NodeJS.Platform = process.platform,
): string {
const resolved = path.resolve(p); // nosemgrep
return platform === 'win32' ? resolved.toLowerCase() : resolved;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

L213: 🟡 risk: Lowercasing every Win32 path collapses distinct entries in case-sensitive NTFS directories (Windows supports per-directory case sensitivity: https://learn.microsoft.com/en-us/windows/wsl/case-sensitivity), so a sibling can pass this trusted-root check. Does Nanocoder explicitly exclude that filesystem mode? If so, please document it; otherwise add a guard/regression so the trust boundary preserves actual filesystem semantics. Codex-assisted review; checked exact head 2318183.

@anisayakmitra-in

Copy link
Copy Markdown
Contributor Author

Thanks @ducanhnguyen223! Addressed in commit 83617a6:

Nanocoder's
ormalizeForCompare and isPathInside are purely lexical containment checks (they operate on string paths and do not inspect on-disk filesystem attributes or volume flags like FILE_FLAG_OPEN_REPARSE_POINT / NTFS case sensitivity attributes). As with symlink boundaries, disk-authoritative validation is delegated to isRealPathInside.

We have updated the docstrings in source/utils/path-validation.ts and the changeset to explicitly document that lexical normalization assumes standard Win32 case-insensitivity and excludes NTFS per-directory case-sensitivity modes, pointing callers to isRealPathInside whenever filesystem-level case semantics must be enforced.

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

Labels

agent:comments nc-review left non-blocking findings area:ci GitHub Actions and CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Windows path checks are case-sensitive: a trusted folder is untrusted under another spelling, and file tools reject valid paths

2 participants