Repository navigation
fix(files): refuse to delete, move or overwrite a repository's .git - #17769
brennanb2025 wants to merge 8 commits into
Conversation
The file-explorer mutation RPCs accepted `.git` as an ordinary worktree-relative path and destroyed it. `normalizeRuntimeRelativePath` rejects only `..`, absolute paths and empty segments — `.git` is an ordinary segment — and `resolveAuthorizedPath` only asserts the target is inside an allowed root, which a repository's own `.git` is. No protected-path policy existed anywhere in the runtime or relay. Reproduced on unfixed main before fixing: the real `deleteFileExplorerPath` removes a real `.git` directory while every working-tree file survives — the signature of the 2026-08-30 incident, on demand, with no source changes. The client-version check is not a protection: `assertRuntimeFileMutationExpectation` requires `expectedExecutionHostId`, but passing `'local'` is exactly what an ordinary client request produces, so it gates client version, not what may be deleted. New `src/shared/repository-admin-path.ts` classifies a path as repository admin state by whole path SEGMENT, so `.github`, `.gitignore`, `.gitattributes`, `.gitmodules`, `.gitkeep`, a file named `git` and a directory named `mygit` all stay mutable. It catches `.git` as a directory, `.git` as the `gitdir:` pointer file used by linked worktrees and submodules, anything inside `.git`, both separators, case variants, and the Win32 trailing-dot spelling. It fails closed: a path it cannot classify is treated as admin state. Enforced at the RPC boundary for source AND destination, so a rename or copy cannot substitute a `.git` either. Deliberately NOT in scope: the shared relay primitive (repo creation legitimately deletes a remote `.git` through it) and the direct Electron IPC lane, which the renderer falls back to for local targets and which is a real second hole needing its own ticket. 82 tests. Mutation-tested: neutering the guard turns 27 of them red.
Greptile SummaryThe PR adds shared repository-admin path classification and applies it to local, SSH, and terminal-artifact mutation paths, with extensive coverage for lexical aliases, symlinks, hard links, linked worktrees, and platform-specific spellings. A remote dangling-symlink path and the previously reported copy hard-link path still leave repository metadata reachable. Confidence Score: 1/5The PR is not yet safe to merge because remote dangling symlinks and the outstanding copy-source hard-link path can still bypass repository metadata protection. Remote link-following writes classify only the parent after target realpath fails, allowing a dangling ordinary-looking symlink to create Files Needing Attention: src/main/runtime/repository-admin-path-authorization.ts, src/main/runtime/runtime-file-commands-write-file-explorer-file.ts, src/main/runtime/runtime-file-commands-create-file-explorer-dir-no-clobber.ts
|
| Filename | Overview |
|---|---|
| src/main/runtime/repository-admin-path-authorization.ts | Adds central local and remote mutation authorization, but remote dangling-link fallback and hard-link checking through preserved symlinks leave protected targets reachable. |
| src/main/runtime/runtime-file-commands-write-file-explorer-file.ts | Routes write and create operations through the new guards, while SSH writes remain affected by incomplete remote leaf canonicalization. |
| src/main/runtime/runtime-file-commands-create-file-explorer-dir-no-clobber.ts | Applies guards to copy, rename, delete, upload, and directory creation; copy still exposes the outstanding symlink-to-hard-link path. |
| src/shared/repository-admin-path.ts | Implements conservative whole-segment .git classification with POSIX and Win32 handling. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Request[SSH files.write] --> Guard[assertMutableRemotePath]
Guard --> Realpath[realpath of leaf]
Realpath -->|ENOENT for dangling target| Parent[Canonicalize parent only]
Parent --> Allowed[Ordinary leaf passes .git classifier]
Allowed --> Write[provider.writeFile follows symlink]
Write --> Git[Create or overwrite .git metadata]
Reviews (9): Last reviewed commit: "fix(files): refuse a dangling symlink th..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Why: the executing host is unknown at this boundary, so classify with the strictest flavour. | ||
| function assertMutablePaths(...paths: readonly unknown[]): void { | ||
| for (const path of paths) { | ||
| if (isRepositoryAdminPath(path, 'win32')) { |
There was a problem hiding this comment.
.git guard
When a worktree contains a symlink such as metadata -> .git, assertMutablePaths accepts paths like metadata/config because it checks only lexical segments. Subsequent path resolution follows the symlink and authorizes the resulting .git target because it remains inside the worktree, allowing repository metadata to be overwritten, moved, copied, or deleted. How this was verified: The accepted raw path was traced through resolveAuthorizedPath, whose realpath canonicalization follows symlinks before applying only the allowed-root containment check.
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between d44d854f7074254c84e90bb5201bca80781703f2 and 3e5b7f0. 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughAdds repository-admin path classification with POSIX and Windows path handling. File mutations now reject Merge Risk: 🟠 High · up to The PR blocks many attempts to delete or overwrite repository metadata, but some application-exposed file operations can still modify a repository’s .git through an alternate interface, leaving a high-impact data-integrity vulnerability reachable. The change is not merge-ready until that remaining mutation path is protected or explicitly accepted by the owner. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the problem, implementation, scope, known limits, linked issue, and testing in detail. However, it omits several template sections, including explicit Why, Visual Proof or N/A, AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist responses. Resolution Add the missing template sections. Include an explicit Why section, state Visual Proof as N/A with a reason if no visual change exists, complete AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist items, and retain the existing testing details under Testing.
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The guard matches the raw relative path lexically, but the write/create methods resolve symlinks after it passes — a worktree symlink pointing into .git still lets a caller overwrite files under .git. See the inline comment; address the write-family bypass before merging. The unguarded local Electron IPC lane is noted below as an informational observation.
Reviewed changes
- Shared path predicate —
isRepositoryAdminPathclassifies.git(directory orgitdir:pointer file) and any descendant by whole-segment match, with case-folding and Win32 trailing-dot/space normalization. - RPC guard — every
files.*mutation method (write,writeBase64,writeBase64Chunk,createFile,createDir,createDirNoClobber,commitUpload,rename,copy,delete) now refuses admin-state paths on both source and destination. - Tests — 82 cases exercising the guard through the real
RpcDispatcher/OrcaRuntimeServiceon real repositories, including the linked-worktree pointer file and over-blocking regressions (.github,.gitignore,git,mygit).
ℹ️ The local Electron IPC lane still deletes/renames .git unguarded
This PR guards only the runtime files.* RPC lane. The renderer's local file explorer falls back to the direct Electron IPC handlers — fs:deletePath, fs:rename, and fs:importExternalPaths — for non-environment repos, and none of those apply the new predicate. The description acknowledges this as a follow-up ticket, but the PR title ("refuse to delete, move or overwrite a repository's .git") overstates what is enforced today.
Technical details
# Unguarded local IPC lane
## Affected sites
- src/renderer/src/runtime/runtime-file-mutation-client.ts — local delete/rename/copy falls back to `window.api.fs.*` when the target is not an environment
- src/main/ipc/filesystem.ts (`fs:deletePath`), src/main/ipc/filesystem-mutations.ts (`fs:rename`, `fs:importExternalPaths`) — no admin-path check
## Required outcome
- Confirm the follow-up ticket covers these handlers (or narrow the PR title), so the shipped claim matches the enforced boundary
## Open questions for the human
- Is the local IPC lane tracked with an explicit ticket, and what is its priority relative to this fix?ℹ️ Nitpicks
assertMutablePathshardcodes the'win32'flavour, so on case-sensitive POSIX filesystems a directory literally named.git.or.git(trailing dot/space) is also refused. Safe direction and rare, but this over-blocking beyond the case-fold is not called out in the description notes.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Live bypass in this PR: the guard classifies a string the filesystem does not usePlan review of a follow-up design surfaced a defect in this PR, so recording it here rather than
So: This is the classic guard-bypass: the check inspects one value and the operation executes What it does and does not undermine: the reproduction stands, and the guard genuinely blocks the Fix direction: classify the canonical path — the one the gate resolves — and fail closed when Related and also worth knowing before merge: a blanket "no mutation inside |
… alias .git The RPC-boundary guard classified the caller's relative spelling, but the filesystem operates on what resolveAuthorizedPath canonicalizes. A symlink `foo -> .git` carries no `.git` segment, so every files.* mutation reached repository admin state through it — including creating `.git/hooks/pre-commit`, since the resolver canonicalizes the nearest existing ancestor of a missing path and the callers mkdir its parents. Classify the resolved path at all 13 local call sites, both halves of rename/copy/commitUpload. The relative-path check stays: the SSH branch returns before the local gate and it is the only thing covering that lane.
There was a problem hiding this comment.
Important
The canonical-path guard closes the symlink-alias bypass on the local lane only — an SSH-connected repository is still unprotected. See the inline comment.
Reviewed changes
- Canonicalized-path guard — a new
resolveAuthorizedMutablePathwrapsresolveAuthorizedPathand classifies the resolved path, so a symlinked ancestor (foo -> .git) is refused on the local branch for all ten mutation methods. - Symlink-alias tests — a new
describe.skipIf(win32)block creates a real symlink into.gitand asserts refusal across write/delete/rename/copy/create/commitUpload, plus that deleting the link itself still works. - SSH + resolved-path tests — new blocks verify the lexical RPC-boundary guard refuses literal
.gitpaths before the SSH provider, and document the workspace-under-.gitover-block.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: 594e05e7-6668-4dcf-ad3d-a92a081f7843
📒 Files selected for processing (3)
src/main/runtime/orca-runtime-files-repository-admin-path.test.tssrc/main/runtime/orca-runtime-files.tssrc/main/runtime/repository-admin-path-authorization.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
preserveSymlink keeps the leaf on purpose so rename and delete act on the directory entry, but copyFile reads and writes *through* it. A worktree symlink `hook-link -> .git/hooks/pre-commit` classified as an ordinary path, and the copy then read the real hook — exfiltrating hooks and .git/config into the working tree. Classify the canonicalized leaf for operands of link-following operations, i.e. both ends of copy. rename and delete keep leaf-preserving classification so removing a symlink itself still works. Fail closed when the leaf exists but cannot be canonicalized; a not-yet-created path has no link to follow. The destination half was already blocked by COPYFILE_EXCL, which is the only thing that blocked it; classifying it too removes that dependency.
| function assertMutablePaths(...paths: readonly unknown[]): void { | ||
| for (const path of paths) { | ||
| if (isRepositoryAdminPath(path, 'win32')) { | ||
| throw new Error(REPOSITORY_ADMIN_PATH_DENIED_MESSAGE) | ||
| } | ||
| } |
There was a problem hiding this comment.
When an SSH-backed worktree contains an ordinary-looking path whose ancestor is a symlink to .git, such as foo -> .git, the lexical guard accepts foo/config and the SSH branch forwards the raw path to the relay before canonical-path authorization runs, allowing remote repository metadata to be overwritten, moved, copied, created, or deleted.
Knowledge Base Used:
There was a problem hiding this comment.
Important
This commit is a clean, well-tested tightening of the local copy lane, but the previously flagged SSH-lane gap is still open, so I'm not approving yet. The delta itself introduces no new issues.
Reviewed changes
- Leaf-symlink copy classification —
resolveAuthorizedMutablePathgained afollowsLeafSymlinkoption that canonicalizes the leaf (realpath) so a copy whose source or destination is a symlink into.gitis refused, while rename/delete keep acting on the link entry itself. - Copy enables it —
copyFileExplorerPathnow passesfollowsLeafSymlink: truefor both operands, closing the exfiltration of hooks /.git/configthrough ahook-link -> .git/...source. - Fail-closed canonicalization —
canonicalLeaftreats a missing path as "nothing to follow" but any other resolution failure (e.g. symlink-loopELOOP) as a refusal. - Tests — six new cases covering source and destination aliasing, fail-closed on an uncanonicalizable leaf, and the over-blocking regressions (a plain working-tree symlink still copies; a link into
.gitstill renames/deletes as the entry, not the target).
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
…avour by host Three unaddressed Highs from the #17769 review round. 1. files.writeTerminalArtifact is grant-mediated on an absolute path and never passes the worktree-relative funnel. Grants come only from the terminal-artifact roots (tmpdir, /tmp), so the exploit requires the repository itself to sit under one — a scratch clone or CI checkout, not a normal ~/repos checkout. There the /private spelling of a macOS temp path makes the in-worktree branch miss and a grant for <repo>/.git/config becomes mintable and writable. Refuse admin paths on the write. Bounded: the write opens O_RDONLY first, so it can overwrite an existing hook or config but cannot create .git/hooks/pre-commit from nothing. 2. A hard link has no target to resolve — every name for the inode is equally real and realpath returns the one it was given. `ln .git/hooks/pre-commit hook-hard` then files.write overwrote the hook; copy read through it. Refuse nlink > 1 on operations that act on the object rather than the directory entry, mirroring the existing isTerminalArtifactHardLinked check. This closes the POSIX case and leaves a documented Windows residual — nlink is not dependable on NTFS — and is deliberately not described as closing the vector. Severity differs from the symlink bypasses in a way worth recording: git stores symlinks as tracked objects, so `git checkout` materialises an attacker's symlink on the victim's disk, which is what made those remotely exploitable. Git cannot represent a hard link, so this one needs local worktree write access, and an attacker holding that can write .git/hooks/pre-commit directly without Orca. 3. Move the relative-path check from the RPC handlers into a mutation-only funnel above the local/SSH fork, so the flavour comes from the target worktree instead of a hardcoded win32 that refused a legitimately named `.git.` directory on POSIX. Threading intent through resolveAuthorizedPath cannot work here: every mutation method forks to SSH and returns before that gate. resolveFileExplorerPaths sits above the fork and holds the target worktree, so one move fixes the flavour, keeps SSH covered, and removes the per-handler duplication. Verified by mutation rather than assumed — removing the funnel check turns the four SSH tests red. The funnel is separate from the read funnel so refusing .git never blocks listing or previewing it. Not fixed, recorded as a residual: NTFS 8.3 aliases (GIT~1) still address .git for rename and delete on Windows, because preserveSymlink canonicalizes the parent and keeps the leaf verbatim. The other lanes realpath the whole path and are covered. This is a code-path argument, not a measurement — it cannot be run on macOS — and belongs with STA-6212's hardware work. Split the suite in two behind a shared fixture to stay under max-lines; no tests were dropped (107 -> 119, set difference empty). The worktree mock lives in its own import-free module so the vi.mock factory cannot re-enter the runtime it is mocking.
d44d854 to
f28b3ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: ee03a471-2b54-47b3-a520-cd3646a38fd3
📥 Commits
Reviewing files that changed from the base of the PR and between 9b8d5ca and d44d854f7074254c84e90bb5201bca80781703f2.
📒 Files selected for processing (6)
src/main/runtime/orca-runtime-files-repository-admin-path-aliases.test.tssrc/main/runtime/orca-runtime-files-repository-admin-path.test.tssrc/main/runtime/orca-runtime-files.tssrc/main/runtime/repository-admin-path-authorization.tssrc/main/runtime/repository-admin-path-test-harness.tssrc/main/runtime/repository-admin-path-worktree-mock.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
| assertMutablePath(await canonicalLeaf(resolvedPath)) | ||
| await assertNotHardLinked(resolvedPath) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Sensitive Data Exposure (CWE-59)
Reachability: External · Exploitability: Difficult
Check hard links on the canonical target.
When preserveSymlink and followsLink are enabled, pass the result of canonicalLeaf(resolvedPath) to assertNotHardLinked. Add a regression test for a leaf symlink to a hard-linked repository-admin file.
| const names = await readdir(join(fixture.repoPath, '.git')) | ||
| const entries: Record<string, string> = {} | ||
| for (const name of names.sort()) { | ||
| entries[name] = existsSync(join(fixture.repoPath, '.git', name)) ? 'present' : 'missing' | ||
| } | ||
| entries['HEAD:content'] = await readFile(join(fixture.repoPath, '.git', 'HEAD'), 'utf-8') | ||
| entries['config:content'] = await readFile(join(fixture.repoPath, '.git', 'config'), 'utf-8') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Snapshot the complete .git tree.
readAdminState only lists immediate .git entries. It does not detect changes below refs or worktrees. For example, a regression that deletes .git/worktrees/x/gitdir can leave .git/worktrees present and still pass the refusal tests.
Walk .git recursively and compare every file’s bytes and directory entries.
| if (followsLink) { | ||
| assertMutablePath(await canonicalLeaf(resolvedPath)) | ||
| await assertNotHardLinked(resolvedPath) | ||
| } |
There was a problem hiding this comment.
When a copy source is a leaf symlink to an ordinary-looking file hard-linked to a file under .git, canonicalLeaf classifies the target pathname but assertNotHardLinked uses lstat on the preserved symlink itself. copyFile then follows that symlink and copies the protected Git metadata. How this was verified: The copy path was traced from preserved-leaf authorization through lstat(resolvedPath) to copyFile, which follows the leaf symlink.
| if (followsLink) { | |
| assertMutablePath(await canonicalLeaf(resolvedPath)) | |
| await assertNotHardLinked(resolvedPath) | |
| } | |
| if (followsLink) { | |
| const canonicalPath = await canonicalLeaf(resolvedPath) | |
| assertMutablePath(canonicalPath) | |
| await assertNotHardLinked(canonicalPath) | |
| } |
Knowledge Base Used: Git operations and status
There was a problem hiding this comment.
ℹ️ No critical issues — one minor suggestion inline.
Reviewed changes
- Hard-link refusal —
resolveAuthorizedMutablePathgainedassertNotHardLinked, refusing inodes withnlink > 1for mutations that act on the link target (truncate writes, append, copy source, upload temp). - Host-derived flavour — the lexical guard moved out of the RPC boundary and into
resolveFileExplorerMutationPaths, now classifying with the flavour of the target worktree's path instead of a hardcodedwin32. - Terminal-artifact guard —
writeTerminalArtifactFilenow refuses granted artifact writes into repository admin state viaassertMutableHostPath, before the local/SSH split. - Test reorganisation — suites split into a shared harness plus a new aliases spec covering symlinked ancestors, leaf symlinks, hard links, the SSH lane, and host-flavour behaviour.
ℹ️ Nitpicks
- The
nlink > 1refusal also blocks legitimate hard-linked working-tree files (two ordinary names for one inode, neither inside.git), and is POSIX-only. Both are safe-direction and consistent with the existing terminal-artifact policy, but the PR title scopes to.git, so a one-line note in the description would keep the boundary honest.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
| async function assertNotHardLinked(path: string): Promise<void> { | ||
| let linkCount: number | ||
| try { | ||
| linkCount = (await lstat(path)).nlink |
There was a problem hiding this comment.
lstat reports nlink >= 2 for every directory on POSIX, so any followsLink mutation aimed at a directory — e.g. files.write on an existing directory, which reaches assertNotHardLinked before its own isDirectory check — now fails with the hard-link denial instead of the intended Cannot write to a directory. Skip directories here so the caller's directory error stays accurate.
| linkCount = (await lstat(path)).nlink | |
| const stat = await lstat(path) | |
| if (stat.isDirectory()) { | |
| return | |
| } | |
| linkCount = stat.nlink |
…avour by host Three unaddressed Highs from the #17769 review round. 1. files.writeTerminalArtifact is grant-mediated on an absolute path and never passes the worktree-relative funnel. Grants are confined to the terminal-artifact roots (tmpdir, /tmp), but any repository under one — a scratch clone, a CI checkout — carries its `.git` inside that root, and a tap outside the current worktree makes relativePathInsideRoot return null so the in-worktree branch is skipped. A grant for that repo's .git/config is then mintable and writable. Refuse admin paths on the write. The repository does NOT have to be the open worktree, and no symlink is needed: a second repo under the temp root is enough, which holds on every platform. An earlier reading of this attributed it to the macOS /var -> /private/var spelling; that spelling is incidental, and the test now uses a second repo reached by its plain mkdtemp spelling so the construction is platform-independent. Bounded: the write opens O_RDONLY first, so it can overwrite an existing hook or config but cannot create .git/hooks/pre-commit from nothing. 2. A hard link has no target to resolve — every name for the inode is equally real and realpath returns the one it was given. `ln .git/hooks/pre-commit hook-hard` then files.write overwrote the hook; copy read through it. Refuse nlink > 1 on operations that act on the object rather than the directory entry, mirroring the existing isTerminalArtifactHardLinked check. This closes the POSIX case and leaves a documented Windows residual — nlink is not dependable on NTFS — and is deliberately not described as closing the vector. Severity differs from the symlink bypasses in a way worth recording: git stores symlinks as tracked objects, so `git checkout` materialises an attacker's symlink on the victim's disk, which is what made those remotely exploitable. Git cannot represent a hard link, so this one needs local worktree write access, and an attacker holding that can write .git/hooks/pre-commit directly without Orca. 3. Move the relative-path check from the RPC handlers into a mutation-only funnel above the local/SSH fork, so the flavour comes from the target worktree instead of a hardcoded win32 that refused a legitimately named `.git.` directory on POSIX. Threading intent through resolveAuthorizedPath cannot work here: every mutation method forks to SSH and returns before that gate. resolveFileExplorerPaths sits above the fork and holds the target worktree, so one move fixes the flavour, keeps SSH covered, and removes the per-handler duplication. Verified by mutation rather than assumed — removing the funnel check turns the four SSH tests red. The funnel is separate from the read funnel so refusing .git never blocks listing or previewing it. Not fixed, recorded as a residual: NTFS 8.3 aliases (GIT~1) still address .git for rename and delete on Windows, because preserveSymlink canonicalizes the parent and keeps the leaf verbatim. The other lanes realpath the whole path and are covered. This is a code-path argument, not a measurement — it cannot be run on macOS — and belongs with STA-6212's hardware work. Split the suite in two behind a shared fixture to stay under max-lines; no tests were dropped (107 -> 119, set difference empty). The worktree mock lives in its own import-free module so the vi.mock factory cannot re-enter the runtime it is mocking.
f28b3ec to
3e5b7f0
Compare
There was a problem hiding this comment.
ℹ️ No new issues in this delta.
Reviewed changes
- Terminal-artifact coverage rework —
files.writeTerminalArtifacttests now mint grants against a second scratch repository under the temp root rather than the worktree under test, soassertMutableHostPathis exercised deterministically on every platform instead of relying on macOS's/privatepath quirk.
Approval remains held on the previously flagged SSH-lane symlink-alias gap, which this test-only delta does not touch.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
main's #17605 split RuntimeFileCommands into a mixin chain and gutted orca-runtime-files.ts to a re-export barrel, so every guard this branch added sat in a file that no longer holds the code. Resolved orca-runtime-files.ts to main's barrel verbatim and ported each guard to the method's new home: - the mutation funnel (resolveFileExplorerMutationPath/Paths) into runtime-file-commands-search-local-runtime-files.ts, beside the read funnel it wraps - the ten mutation methods and their 13 resolveAuthorizedMutablePath sites into runtime-file-commands-write-file-explorer-file.ts and runtime-file-commands-create-file-explorer-dir-no-clobber.ts, preserving which operands pass followsLink - assertMutableHostPath into runtime-file-commands-write-terminal-artifact-file.ts Verified by intent, not by compiling: neutering all 3 isRepositoryAdminPath call sites turns 51 of 119 tests red, matching the pre-merge figure, so the guards are still attached to the code the RPC actually reaches. All 13 authorization sites remain downstream of their SSH early-return and the funnel remains above the fork in every method.
…-protected-git-path
There was a problem hiding this comment.
ℹ️ No new issues in this merge delta.
Reviewed changes
- Merged
origin/main'sRuntimeFileCommandsmixin split — the two merge commits brought main's mechanical split oforca-runtime-files.tsintoruntime-file-commands-*mixins into the branch, and re-applied the repository-admin-path guard across the resulting files. - Mutation funnel relocated —
resolveFileExplorerMutationPath(s)is nowprotectedonRuntimeFileCommandsWithSearchLocalRuntimeFiles, still wrappingresolveFileExplorerPaths+assertMutableRuntimeRelativePathbefore the local/SSH fork. - Guard call sites ported intact — all ten mutation methods (write, writeBase64, writeBase64Chunk, createFile, createDir, createDirNoClobber, commitUpload, rename, copy, delete) still call
resolveAuthorizedMutablePathon the local branch, andwriteTerminalArtifactFilestill assertsassertMutableHostPath. Both suites pass: 119 tests.
Approval remains held on the previously flagged SSH-lane symlink-alias gap, which this merge does not touch.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Electron UI validation (this worktree's Orca Dev)Drove a named Playwright session ( Identity ( {"name":"Orca: brennanb2025/sta-6210-protected-git-path","isDev":true,"devLabel":"sta-6210-protected-git-path","devBranch":"brennanb2025/sta-6210-protected-git-path","devWorktreeName":"sta-6210-protected-git-path","devRepoRoot":"/Users/brennanbenson/orca/workspaces/orca/sta-6210-protected-git-path","dockBadgeLabel":null}Throwaway local git repo: What I SAW
Filter Names for The access-denied message never appeared. I never saw:
Ordinary file delete — WORKEDRight-click Rename / create
|
Two Highs from the R2 review, confirmed independently by a Codex round and a Grok validation, and reproduced here against real bytes. SSH ALIASING. The SSH branch returns before any local authorization, so the caller's relative spelling is all the client sees — and a guest-side symlink makes that spelling lie. Reproduced by driving the relay's own exported mutation handlers (the code that runs ON the remote host) against a real temp filesystem with real symlinks, not a mock: writing, deleting, recursively deleting, renaming through `safe -> .git`, and writing/copying through a leaf link `hook-link -> .git/hooks/pre-commit` all reached repository admin state. Classify the target on the execution host with provider.realpath before dispatching, at all 13 SSH operands, mirroring the local lane's per-operand semantics: full realpath where the syscall follows the link, parent-canonical plus preserved leaf for rename and delete so removing a symlink itself still works. fs.realpath has been on the relay since 2026-07-26 (c3a9a5e). Deliberately NOT in the relay: remote-repo-creation.ts:114 still calls fsProvider.deletePath(<target>/.git, true), so a blanket relay or provider guard would break remote create-project rollback. Verified live rather than assumed retired; STA-6079's removal is on a different branch. This is client-side by construction. It closes the lane for this client and does NOT make the host safe against an older or third-party client — the same limitation the local lane has. A host or provider without realpath stays PERMISSIVE, because bricking ordinary remote editing would be worse than the hole; that path is tested both for a rejecting realpath and for a provider that does not implement it at all, since calling a missing method throws synchronously and no .catch() around the call would absorb it. WINDOWS 8.3. rename and delete keep `preserveSymlink`, which canonicalizes the parent and keeps the leaf verbatim, so an 8.3 alias such as GIT~1 is never resolved. Now lstat the leaf: if it is not a symlink, classify its realpath as well; if it is, keep today's entry-preserving behaviour so deleting the link itself still works. Measured only to not regress POSIX. It is NOT evidence that it catches GIT~1 on a short-name-enabled NTFS volume, and its ablation is 0-red by construction: on POSIX the realpath of a non-symlink leaf equals the path already classified, so no test can distinguish it. The symlink-skip that makes it safe IS covered — removing it turns the two "delete the link itself" tests red.
There was a problem hiding this comment.
Important
The new SSH-lane guard still fails open on nested creates through a symlinked ancestor, so a remote host's .git can still be written into. See the inline comment.
Reviewed changes
- SSH mutation targets classified on the execution host —
assertMutableRemotePathcanonicalizes the remote path via the relay'sfs.realpathand refuses repository admin state before any provider mutation. - Remote guard wired into all ten mutation branches — write/append/copy classify the link target (
followsLink: true); create/rename/delete classify entry semantics (followsLink: false), mirroring the local lane. - Local entry-semantics leaf classification —
assertMutableUnlessSymlinkcanonicalizes the leaf forpreserveSymlinkrenames/deletes unless the leaf is itself a symlink. - SSH-lane test coverage — refusals for symlinked-ancestor write/delete/copy/rename and leaf-symlink copy sources, plus permissive fallbacks for hosts without
fs.realpath.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
| const parent = await realpathRemote(path.dirname(remotePath)).catch(() => undefined) | ||
| return parent === undefined ? undefined : path.join(parent, path.basename(remotePath)) |
There was a problem hiding this comment.
canonicalRemotePath returns undefined — which assertMutableRemotePath treats as permissive — whenever the immediate parent cannot be canonicalized, not only when the host lacks fs.realpath. A nested create through a symlinked ancestor slips through: files.createDir('safe/sub/x') with safe -> .git and sub missing classifies nothing (parent realpath throws ENOENT), and the relay's fs.createDir / fs.createFile create the missing parent recursively inside .git. The local lane refuses the same input by walking up to the nearest existing ancestor in resolveAuthorizedMissingPath.
Technical details
# SSH guard fails open on nested creates through a symlinked ancestor
## Affected sites
- src/main/runtime/repository-admin-path-authorization.ts:85-86 — `canonicalRemotePath` returns `undefined` when `realpath(dirname)` fails, so `assertMutableRemotePath` skips classification
- src/relay/fs-path-mutation-requests.ts:42-52 — `createRelayFile` / `createRelayDir` run `mkdir(..., { recursive: true })`, materializing the missing parent
- src/main/runtime/runtime-file-commands-create-file-explorer-dir-no-clobber.ts:36-38, 192-196 and src/main/runtime/runtime-file-commands-write-file-explorer-file.ts:156-159 — `createDir` / `createFile` SSH branches call `assertMutableRemotePath(..., { followsLink: false })`
## Required outcome
- A path whose nearest existing ancestor is a symlink into `.git` cannot be used to create files or directories under `.git` on the SSH lane, even when intermediate segments do not yet exist
## Suggested approach
- When the parent `realpath` fails with `ENOENT`, walk up to the nearest existing ancestor and canonicalize that (mirroring `resolveAuthorizedMissingPath`), then rejoin the missing segments; keep the host-without-`fs.realpath` case permissive but distinguish it from `ENOENT` so genuine canonicalization failures do not silently disable the guard…om nothing `realpath` returns ENOENT for a dangling symlink exactly as it does for a path that is simply absent, and both were treated as "nothing to follow" — so the link's own name was classified, and it carries no `.git` segment. `files.write` then follows the link and CREATES the target. Reproduced against real bytes: writing through such a link created `.git/hooks/post-commit` with attacker content. Every earlier bypass on this branch needed the hook to already exist; this one creates it from nothing, so it is code execution at the next commit with no precondition on the victim's repo. ENOENT now resolves the link by hand — readlink, resolve a relative target against the link's directory, canonicalize whatever of the target's parent exists — and classifies that. A genuinely absent non-link path still returns itself, so ordinary creation is unaffected. `files.createFile` does not carry this: `flag: 'wx'` fails EEXIST on the link. That is the second time an exclusive-create flag has been the only thing between a path and a hook, after COPYFILE_EXCL on the copy destination. Both are deconfliction flags, not security controls — relaxing either reopens a code-execution path. Also makes the hard-link refusal fail closed, matching the symlink check beside it: a link count we cannot read leaves us unable to rule out another name reaching the same bytes, so it refuses rather than allowing. Both branches now have tests that distinguish them — a partial stat is injectable only through a mock, since a real lstat always populates these fields. Remote lane cannot do the dangling case: there is no readlink on the provider contract or the relay, so a dangling remote link still classifies as its own name. Closing it needs a new relay method plus capability negotiation. The remote guard still catches non-dangling ancestor and leaf aliases.
| const resolved = await realpathRemote(remotePath).catch(() => undefined) | ||
| if (resolved !== undefined) { | ||
| return resolved | ||
| } | ||
| } | ||
| // Entry semantics, or a target that does not exist yet: canonicalize the parent and keep the leaf | ||
| // so renaming or deleting a symlink still acts on the link itself. | ||
| const parent = await realpathRemote(path.dirname(remotePath)).catch(() => undefined) | ||
| return parent === undefined ? undefined : path.join(parent, path.basename(remotePath)) |
There was a problem hiding this comment.
When an SSH worktree contains a dangling leaf symlink such as hook -> .git/hooks/post-commit, failed target canonicalization falls back to checking only the worktree parent and the ordinary leaf name. provider.writeFile then follows the symlink and creates or overwrites repository metadata under .git.
How this was verified: The remote files.write path was traced from followsLink: true through the parent-only fallback to provider.writeFile with the original symlink pathname.
Knowledge Base Used:
There was a problem hiding this comment.
ℹ️ No new issues in this delta.
Reviewed changes
- Dangling-symlink writes refused —
canonicalLeafnow resolves a dangling symlink's target by hand (danglingLinkTarget) instead of returning the link's own name, sofiles.writecan no longer follow a link to create state under.git(e.g..git/hooks/post-commit). - Fail-closed stat handling —
assertMutableUnlessSymlinkandassertNotHardLinkednow refuse whenisSymbolicLinkornlinkis absent, instead of treating an unanswerable stat as clean. - Tests — a new unit spec pins the partial-stat refusal behavior, the aliases suite gains the dangling-symlink write and its working-tree counterpart, and the rename-authority mock stat now reports
isSymbolicLink.
Approval remains held on the open SSH nested-create fail-open thread (review 5074453022), which this commit does not touch.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Holding this as a draft — the precedent check does not support what it doesMarking draft rather than closing, because the reproductions are worth keeping and the scoping The problem is the boundary, not the code. This PR refuses So the standard is "don't let automation write to The incident that motivated it was never attributed. What is still worth something regardless of the outcome:
Options, in the order I would rank them:
STA-6210 stays open either way; the capability is real and documented even if this particular fix is |
Recommendation: close this PR unless it is rescopedI checked this change against how established tools handle the same problem, and this one diverges from what they do — so by the standard we're applying to the rest of this workstream, it shouldn't ship as written. What the precedent actually shows. Protection of a repository's internal Why that distinction decides it. This PR takes an ability away from the user — the ability to delete or move a path the OS would let them delete or move — to prevent a mistake they may never make. That is a denial of user capability, unsupported by precedent, in service of a harm nobody has reported hitting through this surface. What is genuinely worth keeping. The underlying observation that started this — that a failed filesystem observation can be read as a dispositive answer — is real, and it is already fixed where it actually caused harm (#17993, #18001), plus the newly filed STA-6379. Those fix the defect class without removing anything from the user. Two options, in order of preference:
Leaving it in draft rather than closing it myself, since these PRs are under review. Flagging it as the one in this batch that should probably not exist. |
|
Closing: the common pattern guards agent writes, not user file operations on their own repo, and the file-mutation code this PR edited was rewritten in #27582. |
Fixes STA-6210.
The file explorer could delete, rename, copy or overwrite a repository's
.git— the folder holdingall of your history. It now refuses, on six distinct paths that each reached
.gitfor a differentreason.
What changes
.gitis refused by name. Delete, rename, copy, write, create-file, create-directory andupload-commit all stop at
.git, at anything inside it, and at the.gitpointer file thatlinked worktrees and submodules use in place of a folder. Matching is by whole path segment, so
.github,.gitignore,.gitattributes,.gitmodules,.gitkeep, a file namedgitand afolder named
mygitall stay editable. Both separators, either case, and the Windows spelling.git.— which Windows resolves back to the real folder — are covered.foopointing at.gitcarries no.gitin its name, sofoo/configused to look ordinary. Orca now resolves the path thefilesystem will actually touch, then classifies that.
.gitis refused too. A link whose own last segment points into.gitwasreadable as a copy source, which hands out
.git/configand hook scripts.exist looks the same as a missing file, so it was classified under its own harmless name — and
writing through it creates the target. Pointed at
.git/hooks/post-commit, that is code thatruns the next time you commit, on any repository, with nothing needing to be there first. Orca
now follows such a link by hand and classifies where it lands.
.githas no target to resolve;every name for it is equally real. Orca refuses to write or copy through a file that has more
than one name on disk.
root and never pass through a worktree, so any repository checked out there carried its
.gitinside the granted area.
before any local path resolution, so the target is now canonicalized on the host before the
request is dispatched. A host too old to support that stays permissive rather than breaking
ordinary remote editing.
closed: a path it cannot classify is treated as
.git.ELI5
.gitis the box holding every version of your work. The file explorer would happily throw thatbox away, because to it
.gitis just another folder name.The fix is to check the name — but a name can lie. Point a shortcut called
fooat the box andfoo/configreads as an ordinary file. Point a shortcut at a slot inside the box that is empty,and writing to the shortcut doesn't just edit something, it puts a new file in the box: name that
slot
hooks/post-commitand Orca has just written a script your next commit will run. Give one filetwo names and neither name reveals the other.
So Orca stops trusting what you typed and asks the filesystem where the path really ends up, then
checks that. If it can't get a straight answer, it refuses.
Known limits
the remote connection today, so a dangling remote link still classifies as its own name.
Closing it needs a new remote method and version negotiation between client and host. Non-dangling
remote links, both the renamed-folder and the last-segment kind, are caught.
make a host safe against an old or third-party client. The local lane has the same shape.
GIT~1) are unresolved on rename and delete. Those two operations keepthe last path segment verbatim so that deleting a symlink still deletes the link, and that path
never resolves the last segment.
files.createFile'sflag: 'wx'andcopy's
COPYFILE_EXCLare the only things stopping two further variants of the dangling-linkcase. They are there to avoid clobbering a file, not as security controls; relaxing either
reopens a code-execution path. Whoever touches that code next should know.
Not fixed here, tracked separately
local targets. STA-6230.
Testing
133 cases across three new suites, and
tsc --noEmitis clean. Every guard was mutation-tested:reverted one at a time, with the specific assertion confirmed failing without it, and the mutation
confirmed to have actually applied before any result was trusted.
Reproductions use real files and real symlinks on a real temp filesystem — no mocked filesystem, so
a passing refusal means the bytes on disk survived. The SSH lane is reproduced by driving the
remote connection's own mutation handlers, which is the code that runs on the remote host. Coverage
is deliberately two-sided: the refusals, and the over-blocking regressions a careless rule would
cause.