(changes): list every untracked file, and give it a real diff and real counts - #298
jbr-sekoia wants to merge 5 commits into
Conversation
git status ran with git's default --untracked-files=normal, so a brand-new directory arrived as a single row whose path was a directory: unopenable, and hiding every file under it. -uall lists them one by one. An untracked row also had no diff and no counts — git diff --numstat never reports an untracked file. Clicking one now runs git diff --no-index against /dev/null, which yields the new-file diff and, from that same output, the added-line count the status pass cannot know. The count lands on the row and in the header total at no extra process cost; computing it up front would mean one git invocation per untracked file on every refresh, and one ssh round-trip each on a remote session. --no-index offers neither of the guarantees the pathspec path relies on: it exits 1 whenever the two inputs differ (success here, not failure), and it has no repository-boundary check, so its operand gets a guard that also rejects absolute paths and a leading dash. The real-git test file now pins LC_ALL=C — its assertions match git's English diagnostics, which a localized machine does not produce.
…its shape A syntactic guard cannot enforce containment. A symlink inside the repository pointing at a directory outside it produces an operand with no "..", not absolute, no leading dash — and git diff --no-index, which has no repository-boundary check of its own, then reads whatever is under it. The operand is now resolved before git sees it, and what git receives is the guard's own value. Locally the cwd and the operand's parent directory are resolved with realpath and the parent must lie inside the resolved root; the parent and not the leaf, so a symlink git itself lists stays openable and still diffs as its target string rather than the target's content. An lstat then requires a regular file or a symlink, which also keeps a FIFO — where --no-index blocks until the timeout — away from git. Remotely there is no filesystem to resolve against, so git's own listing is the oracle: ls-files --others must return the path before any diff is sent. -uall now has a floor and a ceiling. Its output is a hard error when it overruns the transport's stdout cap, around 25-35k untracked files, which would blank a panel that used to work; a failing -uall status is retried once with git's default untracked mode and comes back flagged, so tracked changes still render. The renderer caps the list at 500 rows with a note for the remainder: every row is a DOM node plus a listener, rebuilt on every busy-to-idle edge. Two more defects fixed. A count computed against one status result could be written onto a later one, replacing git's authoritative numstat with a stale "+N -0" when a refresh landed while the diff was in flight; the write now requires the status result it was computed from. And the ".." guard was a substring test, so a file legitimately named "has..dots.txt" was listed and then refused on click — it now tests whole path segments. The real-git test file held a literal NUL byte where the escape belonged, which made git store it as a binary blob: no diff on review, no text search, no blame.
Resolving the operand's parent is not enough. Git lstats a symlink whose target is a file — that one diffs as its target string and leaks nothing — but it follows a symlink whose target is a directory, and git diff --no-index then pairs the two operands by basename: /dev/null against <dirlink>/null. A symlink in the working tree pointing at any directory holding a file called null reads that file, and git status -uall lists such a symlink as a row of its own, so it takes a click and no crafted path. The claim in the code, the docs and a test name was true only for a symlink to a file, and now says so. Two layers close it. The leaf gets a stat() as well as an lstat(), and a symlink resolving to anything but a regular file is refused; a dangling symlink still opens, since there is nothing to follow. And the diff itself must name the file that was asked for — every --no-index diff opens with "diff --git a/<p> b/<p>", including a binary one with no +++ line and an empty new file with no hunk, while the paired case says a/dirlink/null. That check needs no filesystem, which is what makes it work for a remote session, where ls-files legitimately lists the symlink itself. The call runs under core.quotepath=false so the comparison is against two spellings, verbatim and C-quoted, rather than against every escaping git might choose. The -uall retry is gated on the failure it was written for. It fired on any non-zero exit and always reported a collapsed listing, so a repository that could not be read told the user it had too many untracked files, and the real error was discarded. It now retries only on a stdout-cap overrun, ours or execFile's, and returns every other failure untouched.
…path rules The containment check is path arithmetic, and on Windows that arithmetic has two outcomes the POSIX-only reading missed. path.relative returns the empty string for two spellings of one directory, and the guard read that as "outside": a drive-less root like /repo against the \repo that path.resolve derives from it, a trailing separator, a different case. Every untracked file sitting directly in the repository root was refused before git was ever invoked — which is what the fifteen red tests on the windows-2022 legs were saying, all of them failing with the injected exec never called. An empty relative path now means the same directory, which is what it means. The operand handed to git was also spelled with backslashes there, while git writes forward slashes in the diff header the second containment layer compares against. The guard converts on the way out, so the requested path, the operand and the header agree on one spelling on every platform. The fixtures were wrong in the same way: "/repo" is drive-relative on Windows, so a fake realpath returning it verbatim described a directory the operand never resolved into, and the tests exercised nothing. They build their roots with path.resolve now. path.win32 and path.posix exist everywhere, so the guard takes an injectable path implementation and both flavours are asserted from whichever machine runs the suite — a legitimate path resolving, traversal and an out-of-tree symlink refused, a sibling sharing the root's prefix refused, and the leaf type rules — rather than only the one the runner happens to be on.
The test matrix is ubuntu-latest and windows-2022; macos-14 appears only in build.yml, which never invokes the suite. The paragraph exists to separate executed evidence from inference, so an unsupported leg in it is the one kind of claim it must not make.
devsuitup
left a comment
devsuitup
left a comment
There was a problem hiding this comment.
Reviewed b14a585 against main 9004ec5: full read of git-changes-runner.js, git-changes.js, the IPC surface and the changes tab, attacks replayed on real scratch repos with real NTFS symlinks (fs.symlinkSync — note for anyone re-running these: Git-Bash ln -s silently byte-copies when unprivileged on Windows), three of the listed mutations applied to a scratch copy, and the windows-2022 job log read rather than the badge.
Holds, confirmed: --no-index exit rule (mutation → 12 red); .. segment test (mutation → 3 red, has..dots.txt flips); symlink-to-file yields the link's own target string, never the target's bytes; symlink-to-directory refused by the leaf stat; the /dev/null ↔ <dirlink>/null pairing ran unskipped on windows-2022 (git 2.55, ok 548/549 in the log — git 2.24 here does not even exhibit it); header check anchored to the first line; porcelain lists tracked before untracked so the 500 cap drops untracked first; every remote argument goes through shQuote with -- on both invocations; both overrun signatures match the real strings; renderer uses textContent throughout; comments at the one-line ceiling; no attribution trailers. 145/145 locally, CI green.
One inaccuracy in the body, not in the code: "removing either containment layer turns a different test red" is false for the local parent check. With isInsideRoot removed from resolveLocalNoIndexOperand (line 90 → if (!root || !parent) return null;), the unmodified suites stay green — 79/79 and 12/12 real-git, including the sibling <repo>-evil and the cross-drive D:\secrets cases. The trailing isSafeNoIndexPath(operand) on the computed relative operand subsumes every escape it is meant to catch (path.relative from a realpath'd root yields a leading .. segment or, cross-drive, the untouched absolute path). No exploit results — it is defense in depth, and worth keeping — but the "must stay red: either containment layer removed (1 each)" bullet overstates what the suite proves. Please reword that paragraph (or add a test that isolates it, if one can be constructed; I could not).
Approving: the code is sound and the security tests are real; the correction is to the PR text.
2 participants
Why
The Changes panel listed untracked files, but two gaps made "what did I actually touch?" an unreliable question to ask it.
A brand-new directory collapsed to one row.
status()rangit status --porcelain=v2 --branch -zwith no-uflag, so git applied its default--untracked-files=normal, which reports a wholly untracked directory as a single entry and never descends into it. On a scratch repo with one tracked file modified andnewdir/a.txt+newdir/sub/b.txtcreated:A session that created a whole new directory — a new module, a scaffolded package, a test fixture tree — showed one row whose
pathwas a directory. Clicking it produced nothing, and the header's file count was wrong.An untracked file had no counts and no diff.
mergeChanges()setsadded: null, deleted: nullfor an untracked entry, becausegit diff --numstatgenuinely never reports untracked files. The row rendered no counts, the header's+A −Bexcluded everything new, and clicking short-circuited to "Untracked file — nothing to diff yet." A new 400-line file is the largest change a session can make, and it was the one change the panel refused to show.What
-uall, so each untracked file is its own record and no row'spathis ever a directory.A real diff and real counts for an untracked file, from
git diff --no-index -- /dev/null <path>. Counts are derived from the diff already fetched on row click — no extra git invocation, and no per-file invocation instatus(), which runs on every busy→idle edge.--no-indexis a different command with different rulesThree properties of
--no-indexdo not match the pathspec path this panel was built on, and each one cost a defect before it was closed:It exits 1 when the inputs differ. The runner treats any non-zero code as failure via
firstError(), so the untracked call needs its own success rule: 0 and 1 are both success, and exit 1 with empty stdout plus non-empty stderr is a genuine error (--no-indexalso uses exit 1 forerror: Could not access 'x').Its operands are filesystem paths, not pathspecs.
--literal-pathspecsdoes not reach them, and git performs no repository-containment check on them — measured: raw git printed the contents of a file outside the repository. On the pathspec path git refuses that itself, so nothing in this panel had ever needed its own containment.Git follows a symlink to a directory, then pairs the operands by basename.
/dev/nullagainst a symlinked directory becomes/dev/null↔<dirlink>/null. Sincegit status -ualllists a symlink as its own untracked row, this was reachable by an ordinary click, not only by a crafted IPC argument.Containment is therefore closed by two independent layers, each separately load-bearing (removing either turns a different test red; removing both turns the leak test red):
fs.realpathSync.native, the parent must be inside the resolved root bypath.relativerather than a prefix test (so a sibling<repo>-evildoes not pass), the leaf islstated andstated so a symlink resolving to anything but a regular file is refused, and the operand handed to git is the guard's own value. A dangling symlink is allowed: git has nothing to follow and renders it as its target string, and refusing it would recreate the "listed but unopenable" shape.diff --git a/<p> b/<p>header must name exactly the path requested.+++was rejected for this because it is absent from a binary diff and from an empty new file, while thediff --gitline is present in every case, including the leak (a/dirlink/null b/dirlink/null); a missing header fails closed. The comparison is exact string equality against a constructed header, run under-c core.quotepath=false, against two candidate spellings (verbatim and git's C-quoted form).The second layer is what covers the remote transport, where there is no local filesystem to interrogate, and it is also what catches the race on the deliberately-allowed dangling symlink: with the guard seeing a dangling link and the real filesystem holding a directory, raw git emitted the secret and the runner refused it.
The remote path additionally requires
git ls-files --others --exclude-standard -z -- <path>to return exactly that path before any diff is sent. One extra sequential ssh round-trip per untracked click;status()makes none.Scale
-uallwalks every untracked file. At 20k untracked files the time is fine (8 ms → 70 ms) but the output reaches 570 KB, and the remote transport's 2 MiB stdout cap is a hard error — above roughly 25–35k untracked files at realistic path lengths the whole panel went dark, including tracked changes that had worked before. That is a regression, not a limit, so a failing-uallstatus is retried once with git's default untracked mode and the result is flaggeduntrackedCollapsed; tracked rows still render with a note. The retry fires only on the two real overrun signatures (the remote transport's ownstdout exceeded <n> bytesandexecFile'sstdout maxBuffer length exceeded); any other failure returns its own error untouched. Node does not localise those messages, verified underfr_FR.UTF-8.The renderer builds one DOM node and one listener per file on every busy→idle refresh, so rows are capped at 500 with a
+N more files not shownnote. Header totals still count every file. Porcelain emits tracked records strictly before untracked (measured, including with untracked sorting alphabetically first), so the cap drops untracked first and can never hide a tracked change.Also fixed
isSafeGitPathmatched..as a substring, so a legitimately named file likehas..dots.txtwas listed but refused to open with "invalid path". It is now a path-segment test on both separators; real traversal (..,../x,a/..,a/../../b,..\windows\x) is still rejected, and the file both lists and opens.A FIFO in the working tree hung the diff for the full timeout; the same two mechanisms close it (10007 ms → 0 ms, rejected before git runs).
test/git-changes-runner-real-git.test.jsasserted/outside repository/against a localised git message and failed on a French host. It now pinsLC_ALL=C/LANGUAGE=C; it is the only test file in the suite that shells out to real git.Not in scope
--ignored): the parser drops!records and keeps dropping them.Verification
git diff --no-indexwas measured on real git rather than assumed, including the leak and every guard case. The security tests run against a realgit initrepo with no injectedexec.Mutations that must stay red: the
--no-indexsuccess rule reverted tocode !== 0(7),-ualldropped (3), either containment layer removed (1 each), both removed (3, including the directory-symlink leak test), the..segment test reverted to a substring (2), the-uallfallback removed (1), the stale-count identity check removed (1), the row cap removed (1), the header check reduced to verbatim-only (2).npx eslint .→ 0 errors, 317 warnings.npm test→ 1586 tests, 1583 pass, 1 fail, 2 skipped, plus 120/119 in the isolated stage. The single failure istest/ipc-path-validator.test.js"allows files under ~/.claude/", pre-existing onmainand environmental: this machine's~/.claude/CLAUDE.mdis a symlink out of~/.claudeand the validator resolves on disk. This branch touches neither that test noripc-path-validator.js./dev/nullon Windows is executed evidence, not reasoning. Thewindows-2022legs runreal git: an untracked file yields a new-file diff and its added-line count, which drives both rawgit diff --no-index -- /dev/null <path>throughspawnSyncand the runner itself. Git for Windows accepts the literal/dev/null. The containment tests also ran there by name rather than skipping — including the directory-symlink leak case — so symlink creation works on that runner and not.skipfallback fired.That leg also caught two real defects in the guard, neither of them a test-fixture problem:
isInsideRoottreatedpath.relativereturning the empty string as outside. An empty result means the same directory, and two spellings of one directory arise on Windows for ordinary reasons — a drive-less root, a trailing separator, a case difference. Any untracked file sitting directly in the repository root would have been refused on a Windows checkout.newdir\a.txt), while git writesnewdir/a.txtin thediff --githeader the second containment layer compares against. The two would have disagreed for every file in a subdirectory.resolveLocalNoIndexOperandnow takes an injectablepathOps, and the suite runs its containment cases overpath.win32andpath.posixon whichever platform executes it: a legitimate path resolves and comes back forward-slashed, traversal is refused, an out-of-tree symlink on a different drive is refused, a sibling sharing the root's string prefix is refused, and the leaf-type rules hold.Still unverified: the remote transport against a real ssh host, which is untested by design — the rule against ssh-ing to a real host from tests stands, and everything downstream of the transport is covered with injected fakes.