TEMP (do not merge): T-1124's Windows containment steps against T-1127's fix - #326
TEMP (do not merge): T-1124's Windows containment steps against T-1127's fix#326MongLong0214 wants to merge 2 commits into
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
Throwaway verification head. The install-ps1 job belongs to T-1124 and is not T-1127's to ship; this exists only to observe whether the baseline and the two containment attacks execute and pass once the path comparison and the walk are fixed. Not for merge.
CommitLore — record lintTrailers: clean — 2 commits in Active constraints for the paths this PR touchesLimits (69)
Ruled out (181)
Warnings (59)
Trailer violations fail this check. Active constraints are informational — they are what the repository already decided, not a verdict on this PR. |
|
Read and done with. The result is recorded on #320: with T-1127's fix in front of them, T-1127 merged as #327. The evidence for Minimum GREEN is #320 rebased onto |
Throwaway verification head. Not for merge — close after reading.
The
install-ps1containment steps belong to T-1124 (#320) and are not T-1127's to ship. This head is T-1127's fix (#321) with those steps layered on top, purely to observe what has never been observed: whether the baseline passes and whether the two containment attacks execute at all onwindows-latest.Until now the baseline failed first, so attack 1 and attack 2 have never run. What this run answers:
Point 3 is the one that matters most. A fix that made the legitimate path work by loosening the match would pass (1) and (2) and fail (3), and that trade is what T-1124's forbidden scope rules out.
The real gate is #320 rebased onto
devafter #321 merges. This is the early read.