fix(desktop): Harden local file tree paths - #43618
Conversation
Normalize Electron local path handling across file tree, preview, media, and git-root flows. Reject malformed and Windows device paths, recheck sensitive files after realpath resolution, and preserve external symlink traversal with stable renderer errors.
|
Code Review — Clean ✅ Reviewed the full diff (13 files, +880/-138). The security hardening is well-structured:
One minor observation: |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR improves the desktop project tree and IPC filesystem helpers by adding explicit UI placeholders for folder read errors and centralizing/hardening path resolution for Electron IPC (with expanded automated tests).
Changes:
- Show an error placeholder child row in the project tree when
readDirfails for a folder. - Normalize Windows-style paths in renderer-side caching / gitignore filtering and add renderer IPC tests.
- Refactor Electron IPC handlers for
readDirandgitRootinto dedicated modules backed by hardened path helpers and new platform tests.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/desktop/src/app/right-sidebar/files/use-project-tree.ts | Adds placeholder node metadata and inserts an error placeholder child on folder read errors. |
| apps/desktop/src/app/right-sidebar/files/use-project-tree.test.ts | Updates project tree test to assert the new error placeholder child behavior. |
| apps/desktop/src/app/right-sidebar/files/tree.tsx | Renders warning vs loading icons based on the new placeholder field. |
| apps/desktop/src/app/right-sidebar/files/ipc.ts | Normalizes incoming paths to POSIX-style for consistent caching/filtering (Windows support). |
| apps/desktop/src/app/right-sidebar/files/ipc.test.ts | Adds coverage for Windows-style path handling and gitignore filtering in renderer IPC helper. |
| apps/desktop/package.json | Adds new Electron platform tests for fs readDir and git root. |
| apps/desktop/electron/main.cjs | Routes fs IPC handlers through new modules; uses hardening helpers for file URL/path resolution in preview/external URL flows. |
| apps/desktop/electron/hardening.test.cjs | Expands tests for new hardening helpers and additional path/sensitivity cases. |
| apps/desktop/electron/hardening.cjs | Introduces reusable IPC path validation/resolution helpers with structured error codes. |
| apps/desktop/electron/git-root.test.cjs | Adds tests for gitRootForIpc. |
| apps/desktop/electron/git-root.cjs | Extracts git root discovery into a dedicated module using hardened path resolution. |
| apps/desktop/electron/fs-read-dir.test.cjs | Adds comprehensive tests for readDirForIpc behavior (hidden entries, sorting, symlinks, invalid input). |
| apps/desktop/electron/fs-read-dir.cjs | Extracts readDir IPC implementation with hidden-entry filtering and symlink/unknown dirent handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…e-tree-path-hardening
|
This PR is ready for maintainer review. The branch is mergeable, review threads are resolved, and local validation results are listed in the PR body. Could you please approve the pending workflow run when convenient? |
…e-tree-path-hardening # Conflicts: # apps/desktop/package.json
austinpickett
left a comment
There was a problem hiding this comment.
Reviewed the full diff with focus on the security boundary, traced all IPC call sites, and ran the test suites — approving. This is a careful, well-scoped hardening.
The vulnerabilities fixed
- Symlink sensitive-file bypass (the headline) — previously
sensitiveFileBlockReasonchecked only the requested path, so a safe-named symlink (e.g.notes.txt→~/.ssh/id_rsa) slipped through. The fix now checks sensitive-file rules against both the resolved requested path and itsrealpath: cheap reject onresolvedPathbefore stat, then a secondrejectSensitiveFilePath(realPath)after confirming it's a regular file. Correct fix. - Windows device-namespace paths —
rejectUnsafePathSyntaxblocks\\?\,\\.\, andglobalroot/device/forms, preventing raw-device access (\\.\PhysicalDrive0). - file: URL hardening — now parses via
new URL()+ explicitprotocol === 'file:'check beforefileURLToPath, and re-runs syntax rejection on the resolved path. - File-tree robustness — reparse-point classification, stable ordering, bounded concurrent symlink classification, and unreadable folders kept visible with an error placeholder instead of looking empty.
Verified
- All IPC call sites route through
resolveReadableFileForIpc/resolveDirectoryForIpc(Media stream, Image file, Preview target, Preview file, file reads) with noblockSensitive: falseanywhere inmain.cjs— so sensitive blocking (incl. the realpath check) is active on every entry point.blockSensitive !== falsekeeps it on by default. - Tests: 24/24 pass (1 skipped), including the decisive case
resolveReadableFileForIpc blocks symlinks whose realpath is sensitive, plus device-path, NUL, blank, directory-symlink, and file-URL cases.
Design note — agreed
The PR deliberately does not add workspace containment, and that's the right call: desktop attachments/previews legitimately target files outside the active project, and file-tree symlinks/junctions remain expandable. The hardening targets the actual threat (sensitive-file exfiltration via symlink + malformed/device paths) without breaking the feature's purpose. openExternal(file://) applying syntax/device validation only (delegating to the OS handler) is consistent.
MERGEABLE; BLOCKED is branch-protection (needs a review), not red CI. Solid work.
* fix(desktop): Harden local file tree paths Normalize Electron local path handling across file tree, preview, media, and git-root flows. Reject malformed and Windows device paths, recheck sensitive files after realpath resolution, and preserve external symlink traversal with stable renderer errors. * fix(desktop): Address file tree review feedback
* fix(desktop): Harden local file tree paths Normalize Electron local path handling across file tree, preview, media, and git-root flows. Reject malformed and Windows device paths, recheck sensitive files after realpath resolution, and preserve external symlink traversal with stable renderer errors. * fix(desktop): Address file tree review feedback
* fix(desktop): Harden local file tree paths Normalize Electron local path handling across file tree, preview, media, and git-root flows. Reject malformed and Windows device paths, recheck sensitive files after realpath resolution, and preserve external symlink traversal with stable renderer errors. * fix(desktop): Address file tree review feedback
* fix(desktop): Harden local file tree paths Normalize Electron local path handling across file tree, preview, media, and git-root flows. Reject malformed and Windows device paths, recheck sensitive files after realpath resolution, and preserve external symlink traversal with stable renderer errors. * fix(desktop): Address file tree review feedback
* fix(desktop): Harden local file tree paths Normalize Electron local path handling across file tree, preview, media, and git-root flows. Reject malformed and Windows device paths, recheck sensitive files after realpath resolution, and preserve external symlink traversal with stable renderer errors. * fix(desktop): Address file tree review feedback
* fix(desktop): Harden local file tree paths Normalize Electron local path handling across file tree, preview, media, and git-root flows. Reject malformed and Windows device paths, recheck sensitive files after realpath resolution, and preserve external symlink traversal with stable renderer errors. * fix(desktop): Address file tree review feedback
* fix(desktop): Harden local file tree paths Normalize Electron local path handling across file tree, preview, media, and git-root flows. Reject malformed and Windows device paths, recheck sensitive files after realpath resolution, and preserve external symlink traversal with stable renderer errors. * fix(desktop): Address file tree review feedback
* fix(desktop): Harden local file tree paths Normalize Electron local path handling across file tree, preview, media, and git-root flows. Reject malformed and Windows device paths, recheck sensitive files after realpath resolution, and preserve external symlink traversal with stable renderer errors. * fix(desktop): Address file tree review feedback
Harden Hermes Desktop's local file tree and local-file IPC paths without
adding workspace containment.
The file tree previously depended on
Dirent.isDirectory()and direct pathresolution. Windows reparse points could be classified as neither files nor
directories, unreadable child directories looked like empty folders, and
sensitive-file checks could be bypassed through a safe-named symlink.
This change extracts directory and git-root handling into testable Electron
helpers, adds stable directory ordering and bounded concurrent per-entry
symlink classification, and keeps unreadable folders visible with an error
placeholder. Local path resolution is shared across file reads, previews,
media, git-root discovery, and
file:URL handling. It rejects blank, NUL,malformed file URL, and Windows device namespace paths, then checks
sensitive-file rules against both the requested path and its realpath.
Workspace containment is intentionally not included. Desktop attachments and
previews may target files outside the active project, and file-tree symlinks
or junctions remain expandable.
openExternal(file://)intentionally appliessyntax and device-path validation only because it delegates opening to the OS
instead of reading content into the renderer or model. Content-reading paths
continue to use the stricter readable-file checks. The remaining
validation-to-read TOCTOU window is left for a separate hardening change.
Updated after merging latest
origin/mainthrough3edd09a46into thisbranch with merge commits. The latest
apps/desktop/package.jsonconflict wasresolved by preserving main's new
electron/update-remote.test.cjsplatformtest while keeping this PR's
electron/fs-read-dir.test.cjsandelectron/git-root.test.cjsentries. Current PR state is Ready for review andmergeable at head
bde879cc4.Validation on Windows after the final merge:
git diff --check: passed.git merge-tree --write-tree HEAD origin/main: passed.node --test electron/hardening.test.cjs electron/fs-read-dir.test.cjs electron/git-root.test.cjs: 23 passed, 2 skipped.npm run test:ui --workspace apps/desktop -- src/app/right-sidebar/files/use-project-tree.test.ts src/app/right-sidebar/files/ipc.test.ts: 13 passed.npm run typecheck --workspace apps/desktop: passed.npm run test:desktop:platforms --workspace apps/desktop: 142 passed, 2 skipped, 1 failed. The only failure remains the known unrelatedelectron/windows-child-process.test.cjsassertion looking for the formattedexecFileSync('reg')call site.Replaces #43593.