Skip to content

fix(git): keep single-file operations out of .git and symlinked directories - #683

Open
cursor[bot] wants to merge 1 commit into
mainfrom
cm/critical-bug-investigation-fad6
Open

cursor[bot] wants to merge 1 commit into
mainfrom
cm/critical-bug-investigation-fad6

Conversation

@cursor

@cursor cursor Bot commented Sep 7, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Builds on #679, which moved single-file git operations onto repo_relative_path and closed the absolute-path and .. escapes. Three gaps remained:

 repo_relative_path(dir, path)
   canonicalize dir + workdir, strip workdir, normalize `..` lexically
+  reject an empty result (the work tree root)
+  reject any `.git` component, case-insensitively (macOS volumes ignore case)
+  ensure_parent_inside_workdir: the deepest existing ancestor of the target
+    must canonicalize inside the work tree; the leaf itself is not resolved

 restore_file (tracked path)
-  checkout.force().update_index(true).path(&rel)
+  checkout.force().update_index(true).disable_pathspec_match(true).path(&rel)
Input Before After
linked-out/victim.txt, with linked-out a symlink to a directory outside the repo restore_file deletes the outside file and restore_hunk rewrites it rejected
.git/HEAD or .GIT/HEAD restore_file deletes repository metadata rejected
a tracked file named *.nix discarding it also restores every other .nix file restores only that file
result -> /nix/store/... (leaf symlink) works still works; the link is removed or committed, never its target

Every caller of repo_relative_path (commit_files, commit_hunk, restore_hunk, restore_file) gets the checks. It sits on top of #679, now merged. It replaces the first version of this PR, which targeted develop; its symlinked-directory check is kept.

#no-linear

Test Plan

  • Five new git::exec tests: metadata and root rejection, restore_file refusing .git/HEAD, the symlinked-directory escape through restore_file, restore_hunk and commit_file, removing an untracked leaf symlink without touching its target, and literal glob matching. Four fail without the fix; the leaf-symlink test guards against over-rejection and passes either way. All 47 git::exec tests pass with the fix.
  • Local cargo clippy -p nixmac --all-targets -- -D warnings flags nothing in the changed code (its only finding is an existing let_and_return in evolve/nixmac_ignore.rs, untouched here).

Docs

  • No docs update needed

Merge Danger

Door: two-way.

Blast Radius: single-file git operations (commit or discard a file or hunk). Paths under .git, the work tree root, and paths that pass through a symlinked directory resolving outside the repository now return an error. Symlinked directories that stay inside the repository and leaf symlinks are unaffected.

build will fail at notarization until the Apple Developer agreement is renewed, as on every PR right now.

🤖 Generated with Claude Code

@greptile-apps

greptile-apps Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 3/5

[Medium impact] The changes affect file and hunk commit and discard actions in the app.

The PR should not merge until directory symlink aliases into .git are rejected.

Findings

  1. P1 Security Symlinks bypass the metadata check ▶

Summary

Adds checks against repository-root paths, git metadata names, and directory symlinks that leave the repository. File restores now match wildcard characters literally.

  • Single-file Git actions stay inside the working tree.
  • Restoring a tracked file now leaves other files alone when its name contains *.

Reviews (2) · Last reviewed commit: "fix(git): keep single-file operations ou..." · Reviewed by Greptile

Comment thread apps/native/src-tauri/src/git/exec.rs Resolved
cooper (czxtm) added a commit that referenced this pull request Nov 9, 2026
…tories

repo_relative_path only normalized paths lexically, so two escapes remained
after the absolute and `..` checks:

- A path through a symlinked directory (`linked/victim.txt`, with `linked`
  pointing outside the repository) passed every check, and restore_file,
  restore_hunk and commit_file then read, wrote or deleted outside the work
  tree. The deepest existing ancestor of the target must now resolve inside
  the work tree. The final component is not resolved, so tracked symlinks
  such as `result` keep working.
- `.git/HEAD` (or `.GIT/HEAD` on a case-insensitive volume) was a valid
  repository-relative path, so discarding it deleted repository metadata.
  Any `.git` component is now rejected, as is a path naming the work tree
  root.

restore_file also matched its path as a pathspec, so discarding a tracked
file named `*.nix` restored every `.nix` file. The checkout now matches the
path literally.

The symlinked-directory check follows the first version of #683.
@czxtm
cooper (czxtm) force-pushed the cm/critical-bug-investigation-fad6 branch from c96908a to 2abc42a Compare November 9, 2026 14:37
@czxtm cooper (czxtm) changed the title Fix single-file git path confinement fix(git): keep single-file operations out of .git and symlinked directories Nov 9, 2026
@czxtm
cooper (czxtm) changed the base branch from develop to 09-04-scott-git-repo-rel-path November 9, 2026 14:37
@czxtm
cooper (czxtm) marked this pull request as ready for review November 9, 2026 14:37
Comment on lines +101 to +106
anyhow::ensure!(
resolved.starts_with(workdir),
"path `{}` resolves outside repository `{}`",
target.display(),
workdir.display()
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 security Symlinks bypass the metadata check

With linked -> .git, restore_file(dir, "linked/HEAD") passes the new checks: the supplied path contains no .git component, and the resolved parent passes starts_with(workdir). The untracked-file branch then deletes .git/HEAD, breaking the repository. Reject resolved parents inside git metadata as well as literal .git components.

How this was verified: discard_single_file forwards the filename to restore_file, whose parent check accepts the alias before remove_file deletes the metadata file.

@darkmatter

darkmatter Bot commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

🎨 Storybook preview

Open Storybook preview

Updated for 1bf79fe


⚠️ Detected UI changes (5)

These stories' HTML snapshots changed. I've added screenshots + links to the changed stories below. Review them carefully then accept the changes to regenerate baselines and include them in this PR:

Flows/Evolve › Playground

Flows/Evolve › Playground

Flows/Evolve › 1. Begin (idle)

Flows/Evolve › 1. Begin (idle)

Flows/Evolve › 2. Evolving (progress)

Flows/Evolve › 2. Evolving (progress)

Flows/Evolve › Evolving With Error Event

Flows/Evolve › Evolving With Error Event

Flows/Evolve › 3. Review (changes generated)

Flows/Evolve › 3. Review (changes generated)


Accept UI changes

  • Click here to accept these changes

Alternatively, you can run bun run test:update-snapshots locally to re-generate the baselines and then push the changes to this PR.

What does this do?

The screenshots above show UI changes detected by the Storybook
snapshot tests run on this PR. Each image is the rendered output of
a Storybook story from the code in this PR branch; the snapshot
test compared it against the committed baseline in
__snapshots__/ and flagged the difference.

Checking the box tells the darkmatter[bot] to regenerate the
baselines from this PR's current code and commit them directly to
this branch. The new baselines become the source of truth for
future runs — only accept after confirming the visual changes are
intentional.

Comparison baseline: the committed __snapshots__/ files on this
PR branch (carried forward from develop). Accept updates them in
place on this branch.

@github-actions

github-actions Bot commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

📋 PR Overview

Lines changed 198 (+197 / -1)
Files 0 added, 1 modified, 0 deleted
Draft / WIP no
Has Test Plan yes
Linear issue no
No Test Plan Needed no
New UI components no
New Storybook stories no
New Rust modules no
New TS source files no
New tests no
package.json touched no
Cargo.toml touched no
Infra / CI touched no

🔬 Coverage

Report Lines Statements Functions Branches
apps/native/coverage/coverage-summary.json 35.0% 34.4% 29.8% 29.6%

Generated by 🚫 dangerJS against 1bf79fe

Base automatically changed from 09-04-scott-git-repo-rel-path to main November 9, 2026 15:38
…tories

repo_relative_path only normalized paths lexically, so two escapes remained
after the absolute and `..` checks:

- A path through a symlinked directory (`linked/victim.txt`, with `linked`
  pointing outside the repository) passed every check, and restore_file,
  restore_hunk and commit_file then read, wrote or deleted outside the work
  tree. The deepest existing ancestor of the target must now resolve inside
  the work tree. The final component is not resolved, so tracked symlinks
  such as `result` keep working.
- `.git/HEAD` (or `.GIT/HEAD` on a case-insensitive volume) was a valid
  repository-relative path, so discarding it deleted repository metadata.
  Any `.git` component is now rejected, as is a path naming the work tree
  root.

restore_file also matched its path as a pathspec, so discarding a tracked
file named `*.nix` restored every `.nix` file. The checkout now matches the
path literally.

The symlinked-directory check follows the first version of #683.
@czxtm
cooper (czxtm) force-pushed the cm/critical-bug-investigation-fad6 branch from 2abc42a to 1bf79fe Compare November 9, 2026 15:42
@sonarqubecloud

sonarqubecloud Bot commented Nov 9, 2026

Copy link
Copy Markdown

@prelint

prelint Bot commented Nov 9, 2026

Copy link
Copy Markdown

Ship it Single-file git actions stay inside the repository and out of git metadata

Product decisions in this change

Agree 1. A user can no longer commit or discard a file when its path runs through a symlinked folder that points outside the repository. The action returns an error.

Before this change, discarding linked-out/victim.txt could delete a file the user never put in the configuration. The user would not expect a "discard change" button to touch files outside their config. Users who symlink folders inside the repository still work, so the rule only blocks the dangerous case. The cost is one narrow case. Some users may keep a linked folder of dotfiles outside the repository. They now get an error and cannot commit those files through the app. That is the right default for a destructive action. The error message names the path and the repository, so the user can understand it.

Agree 2. The app treats anything under `.git` as off limits for commit and discard actions. The match ignores letter case.

Git metadata is not part of the user's configuration. Discarding .git/HEAD would damage the user's history, and the user would have no way to see that coming. The case-insensitive match fits the macOS default of case-insensitive volumes, which is the main platform for this app. Files such as .gitignore, .github/... and .gitmodules stay allowed, and the tests cover that. The rule is cheap to loosen later, because it only changes which errors users see.

Agree 3. An empty path or the repository root is no longer a valid target for a single-file action.

A single-file action that points at the whole repository is almost certainly a mistake. An error is the safe outcome. No user flow should depend on this behavior.

Agree 4. Discarding a tracked file restores only that exact file. Special characters in the name, such as `*`, are no longer treated as a pattern.

A user who discards one file expects exactly one file to change. Before this change, a file named *.nix would also reset every other .nix file and lose unsaved work. The new behavior matches the button label and removes a surprising data loss path. Such file names are rare, but the loss is silent and hard to undo.

Agree 5. A symlink at the end of a path is handled as the link itself. Discarding or committing it never changes what it points to. Examples are `result` links to the Nix store.

Build output links like result are common in Nix repositories. Users see them as files in the repository and expect "discard" to remove the link, not the build output. The PR keeps this behavior and adds a test so the new safety checks do not block it. This is the right split. Folder links can leak outside the repository, but a final link is a normal repository entry.

Open questions

  • Do any users keep a linked folder outside the repository, such as a shared dotfiles folder, and expect to commit files through it?

  • Does the user interface show the new error text clearly when a discard or commit is refused, or does the user only see a generic failure?

Recommendation

Ship it
Each rule blocks a case where a discard or commit action could destroy data the user did not mean to touch. The only cost is a clear error in rare setups with linked folders outside the repository.

@prelint prelint Bot added the ship it label Nov 9, 2026

This branch was successfully deployed

1 active deployment
storybook-preview — 1bf79fe6 Deployed Nov 9, 2026 by github-actions[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant