Compare recorded paths as locations, and let the walk reach an end - #327
Merged
Conversation
Two defects, one causal chain, both measured on a Windows runner before any of
this was written.
The containment check compared a stored string against a resolved one. The
installer records the root through node, and the shell git runs hooks with
answers `pwd -P` in its own path world, so the two never matched -- not for an
attacker's path, and not for the installer's own bundle either. A check that
refuses everything is not containment; it is a dead branch, and it is what let
control reach the resolution step below it.
Both sides now go through `cd ... && pwd -P`, so whatever form each was written
in, they are compared as the locations they name. The separator is normalised
first: neither `dirname` nor `${var%/*}` finds a parent in a backslash-separated
path -- both answer `.`, which resolves to the repository instead of the install,
and would have quietly compared the wrong directory even once the forms agreed.
The walk that follows stopped when stripping a component produced an empty
string. `${dir%/*}` returns its input unchanged once no `/` remains, so a
drive-letter root settles on `C:` and stays there. A probe hook on the runner
reported `$PWD` as `C:/Users/...` and the walk as non-terminating; a real commit
was killed at a 90s bound. It now stops when stripping stops making progress,
which ends at a drive root, at a filesystem root, and on a relative path -- all
three hung before, so the shape was never Windows-specific, only Windows made it
common.
`doctor`'s mirror was green in exactly that state, and for its own reason: it
checked containment against the root of whichever CLI was running rather than the
root the stub trusts, which is the one `hooks install` recorded. Two different
roots, one of them never consulted. It reads the recorded root now, falling back
to the package root for a repository recorded before that key existed, and it
reports a symlinked target -- which the stub has always refused and the mirror
never mentioned.
The helper keeps its name. T-1125's invariant asserts `isInsidePackage` appears in
this file as its proxy for "#71's containment survived the removal of the
compiled arm", and that test is not this ticket's to edit. The signature changed;
the name is what another ticket is entitled to rely on.
Recovery is asserted rather than assumed. The stub is written into
`.git/hooks/commit-msg` at install time, so a corrected release does not reach a
repository that already has one: `hooks install` has to be re-run there. That it
can be re-run at all is the part worth testing, because a wedged repository
cannot commit -- and `hooks install` is not a commit.
Record-Id: r-winpath1127
Limit: the stub is written to .git/hooks at install time, so a repository installed before this fix keeps the old text and must re-run `commitlore hooks install`; installing a corrected release is not enough
Limit: `${dir%/*}` returns its input unchanged when no separator remains, so a loop that tests for emptiness never terminates at a drive root
Limit: neither dirname nor ${var%/*} finds a parent in a backslash-separated path; both answer `.`
Ruled-out: Normalising only the recorded root at write time | it would fix new installs and leave every existing repository comparing a stored win32 string against pwd -P output
Ruled-out: Loosening the containment match so the legitimate bundle runs | that trades #71's property for a working hook, which is exchanging one defect for another and is what this ticket's forbidden scope names
Ruled-out: Renaming the containment helper to match its new signature | T-1125's invariant asserts the old identifier as its proxy for the property surviving, and that test belongs to another ticket
Ruled-out: Fixing the stub and leaving doctor's mirror alone | the mirror reports no problem in precisely the state where the hook is dead, so the check that should warn would keep saying healthy
Certainty: firm
Blast: local
Undo: easy
Verified: the walk terminates from a drive-letter root, a bare drive letter, a posix root and a relative path, executing the shipped stub text rather than a paraphrase -- all four hung before this change; the recorded bundle runs when the root names the same directory another way, and a bundle outside that root is still refused; doctor reports a bundle outside the recorded root and a symlinked bundle; an outdated stub is reported and repaired by re-running hooks install, after which a commit succeeds; typecheck clean, dist rebuilt and committed, full suite 76 files, 1880 passed, 1 skipped
Unverified: everything on Windows. The defect is only reachable there, and #320 is the gate -- its baseline and both containment attacks have never executed, because the baseline failed first
CommitLore — record lintTrailers: clean — 1 commit in Active constraints for the paths this PR touchesLimits (59)
Ruled out (139)
Warnings (48)
Trailer violations fail this check. Active constraints are informational — they are what the repository already decided, not a verdict on this PR. |
MongLong0214
marked this pull request as ready for review
July 31, 2026 15:09
This was referenced Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #321 (T-1127). P0. Blocks #283.
Two defects, one causal chain
Both were measured on a
windows-latestrunner before any of this was written; the evidence is in #321.The containment check compared a stored string against a resolved one. The installer records the root through node; the shell git runs hooks with answers
pwd -Pin its own path world. The two never matched — not for an attacker's path, and not for the installer's own bundle either. A check that refuses everything is not containment, it is a dead branch, and it is what let control reach the resolution step beneath it.Both sides now go through
cd … && pwd -P, so whatever form each was written in, they are compared as the locations they name. The separator is normalised first: neitherdirnamenor${var%/*}finds a parent in a backslash-separated path — both answer., which resolves to the repository instead of the install, and would have compared the wrong directory even once the forms agreed.The walk had no end.
${dir%/*}returns its input unchanged once no/remains, so a drive-letter root settles onC:and stays there. A probe hook reported$PWDinside a hook asC:/Users/...and the walk asNON-TERMINATING, stuck at [C:]; a real commit was killed at a 90s bound. It now stops when stripping stops making progress.That shape was never Windows-specific — a relative path hung too. Windows only made it common.
doctorwas green in exactly that statereadRecordedHookTargetchecked containment against the root of whichever CLI was running, not the root the stub trusts — the onehooks installrecorded. Two different roots, one never consulted. It reads the recorded root now (falling back to the package root for a repository recorded before that key existed) and reports a symlinked target, which the stub has always refused and the mirror never mentioned.The helper keeps its name
T-1125's invariant in
test/no-compiled-executable.test.tsassertsisInsidePackageappears in this file, as its proxy for "#71's containment survived the removal of the compiled arm." Renaming it to match the new signature broke that test — and that test is not this ticket's to edit. The signature changed; the name is what another ticket is entitled to rely on.Recovery is asserted, not assumed
The stub is written into
.git/hooks/commit-msgat install time, so a corrected release does not reach a repository that already has one.commitlore hooks installmust be re-run there.That it can be re-run is the part worth testing: a wedged repository cannot commit, and
hooks installis not a commit.doctorreports such a repository asoutdated. This belongs in the release note.Acceptance, against the ticket's six conditions
test/hooks.test.ts— the recorded bundle runs when the root names the same directory another waydoctorand the hook agreetest/hook-target.test.ts— outside the recorded root, and a symlinked targethooks installrepairs it, a commit then succeedsVerification
dist/rebuilt and committed