fix(hash): SHA-256 without shasum, and no fingerprint it could not compute (#861) - #862
Closed
fujibee wants to merge 8 commits into
Closed
fix(hash): SHA-256 without shasum, and no fingerprint it could not compute (#861)#862fujibee wants to merge 8 commits into
fujibee wants to merge 8 commits into
Conversation
…mpute (#861) connect --e2ee registered the team with the server and then died on the way to the sync engine. The cause is one absent command: shasum is a Perl script that ships on macOS and most Linux and is NOT in Git for Windows' Git Bash. This repository already knew. scripts/lib/hash.sh exists for exactly this reason and its comment describes today's output -- "NOT in Git for Windows Git Bash, where it fails with shasum: command not found, leaving the hash empty". It was written for SHA-1 only, and the four SHA-256 sites never got the same treatment. Derived rather than taken on report: git grep shasum -- scripts/ gives six invocations across five files. Four are SHA-256 in production -- key.sh's two fingerprint helpers, key.sh's previous-snapshot digest, and remote.sh's age-v1 checkpoint -- and all four now go through the new helper. The fifth is hash.sh's own SHA-1 arm, unchanged. The sixth is release/update-cask.sh, which downloads a .dmg and pushes a Homebrew tap; it cannot run on the platform this is about, so it is deliberately left alone. THE LAST RESORT IS DIFFERENT ON PURPOSE. agmsg_sha1 falls through to cksum because its callers name a socket after the digest and need only stability on one machine. These callers need a real SHA-256: one value is the fingerprint two people read to each other over a separate channel, the other decides whether an epoch snapshot is the one it claims to be. A non-cryptographic stand-in there does not weaken the check, it makes it say something untrue. So agmsg_sha256 ends by FAILING, and the comment says why, because the obvious next edit is to make the two consistent. A FINGERPRINT THAT COULD NOT BE COMPUTED IS WORSE THAN NONE. The five print sites read `echo "Recipient fingerprint: $(_key_fingerprint ...)"`. A command substitution that fails inside a simple command's arguments leaves that command's own status alone, so echo succeeded and printed the label with nothing after it -- and the whole point of the string is that two people compare it. Both would have seen the same blank and agreed. Each site now takes the value into a variable of its own first, where a bare assignment's status IS the substitution's and set -e stops. Not `local x="$(...)"`, which puts the status back on the declaration and undoes it. Preflighted at connect, not only in doctor. Every SHA-256 in the e2ee path happens at or after key generation, which is after registration, so the check is asked before either. Reported by doctor as optional, next to age and for the same reason: a team on cipher "none" never computes one. The preflight probes by RUNNING the digest rather than asking command -v. A tool that is installed and broken answers yes to a presence check and no to the question, which is the direction that hurts -- the preflight passes and the digest fails later, which is the shape of this bug again. Left alone deliberately: remote.sh's fail-closed structure. An empty digest made the checkpoint invalid, setup refused, and connect reported "binding recorded, sync engine not started" without falling back to plaintext. That behaviour was correct; it is the digest that was wrong. UNLIKE #850, THIS IS VERIFIED HERE. The behaviour is chosen by command -v, so a PATH holding only the tools we pick reproduces each platform's choice on any platform -- no Windows machine required. Each arm is asserted against a literal digest rather than against the other arms, since three arms can agree on a wrong answer. The suite includes its own negative control (that a restricted PATH really does hide what it claims to) and covers the case that motivated the run-it probe: tools present and failing.
…forbid The mutation that matters most for this change is "add a cksum arm, to make agmsg_sha256 consistent with agmsg_sha1" -- the exact edit the helper's comment warns the next reader against. Applied, the suite stayed green on the one test named for it. Not because the refusal survived. The sandbox PATH held only `awk`, so the mutant's `cksum` was not found either, the pipeline failed anyway, and the assertion passed for a reason that had nothing to do with the decision under test. It was measuring the box, not the behaviour. cksum is present on every machine this runs on, so that is the shape the test has to run in. With it in the box, the same mutation now turns four cases red, including the one whose name is the claim. Same correction applied to the other sandboxes built for "no SHA-256 tool": the message case, the usable probe, and the require preflight.
CI's enforceable-assertions check counted 642 against a baseline of 638. The four are mine, and they are the shape that check exists for: a non-last `[[ ]]` cannot fail a test on bash 3.2, so it reads as an assertion and behaves as a comment. Three were in the "says which tools were looked for" case, where only the third of three could ever have fired -- the two that name shasum and sha256sum were decoration. The others were the first half of the doctor case and the message half of the connect preflight case, each followed by another assertion. All four are now plain `grep -qF`, which fails the test under bats' errexit. The count is back at the baseline and the suites are green; the mutation rows that rest on these assertions are being re-run, since rewriting an assertion changes the instrument and the earlier matrix was measured with the old one.
… which line agmsg_sha256 sits in the middle of the fingerprint pipeline and cut and sed are content with the empty input a failed digest leaves them. Without pipefail the pipeline exits 0 with an empty string and the label prints with nothing after it - the exact output the callers were changed to prevent. Measured: replacing set -euo pipefail with set -eu reddens both no-blank -fingerprint cases and nothing else. The comment names that control so the next person shortening line 2 has somewhere to look.
…swer was unchecked Two findings from static review, both one level below where the same shape was already noted. The arms were shasum|awk, so the function's status was awk's, and awk is content with the empty input a failed digest hands it. 'this FAILS' held only because key.sh and remote.sh set pipefail on line 2 - a caller without it got an empty success and carried it into a fingerprint. Each tool now runs as its own substitution with its status checked. And nothing looked at the answer. A tool that exits 0 while printing a warning is not a tool that failed, so 64 lowercase hex is now required, and the usable probe compares against the known digest of its input rather than asking whether anything came back - sixty-four characters that are the wrong sixty-four are exactly what a fingerprint cannot survive. Five cases, and one of them was rewritten after it failed to discriminate: the first no-pipefail case used an empty PATH, where the helper returns 1 outright and pipefail was never involved. A present- but-broken tool is the only path that isolates it. The case pinning the status check separately from the shape check exists because reverting the arms alone changed no result until it was written.
… entry points Review: the correctness check lived in agmsg_sha256_usable, whose only production callers are remote.sh's doctor and connect preflight. key.sh is its own CLI - generate, show, import and rotate all reach a digest without going near connect - so a tool exiting 0 with a well-formed but wrong digest was accepted by every one of them, and the suite pinned that as expected behaviour. agmsg_sha256 now asks the selected tool a question whose answer is known, once per process, before it will return anything. Keyed on the helper rather than on the callers because the alternative is a list of entry points kept complete by hand, which is what was missed. A key.sh case drives it directly with a lying shim, because a helper -level case cannot fail if a caller stops calling the helper. Also from review: require and doctor said 'none was found' and 'tool on PATH' for a tool that is present and broken. Presence and usability are different questions and these lines answer the second.
…ything Review, twice over on the same three lines. The self-test's flag was read from the environment, so _AGMSG_SHA256_VERIFIED=1 in an inherited environment meant 'already checked' and skipped it - an undocumented env override of a fail-closed contract, on the hole closed one commit ago. And it saved nothing. Every production call is printf | agmsg_sha256 or x="$(agmsg_sha256 …)", both subshells, so the flag never reached the parent and the self-test ran again on the next digest anyway. Removed rather than made private: a memo whose saving is imaginary is only a surface. The cost is now stated where the code is - one extra digest of a 5-byte input per digest taken, at most three per command. A case pins that no inherited variable can skip it, trying several names, because the property is about the environment and not about one spelling.
Three claims left over from the memo, all found in review, none needing code. The block heading still said ONCE PER PROCESS with RUN BEFORE EVERY DIGEST six lines under it - one comment stating two contracts, and a reader has no way to know which one the code has. agmsg_sha256_usable said it costs one subprocess. It calls agmsg_sha256, which self-tests and then digests, so it runs the tool twice. And the worst path was undercounted: key rotate with an accepted rotation to verify takes four digests, not three - the accepted recipient's fingerprint before the three already listed.
fujibee
added a commit
that referenced
this pull request
Aug 18, 2026
#862 asserted on "All checks passed"; #865's doctor narrowed the summary to "All prerequisite checks passed." so it cannot be read as cancelling a stale lock and the removal command printed above it. The two landed on different lines, so the merge was clean and the assertion was left describing a sentence the code no longer emits.
fujibee
added a commit
that referenced
this pull request
Aug 18, 2026
…ingerprint is real, and doctor names a wedged lock (#868) * ci(windows): take the package feed off the Windows legs entirely (#824) #827 made a chocolatey outage legible -- retry, cache, and a presence check that is the only step allowed to be red about it. This removes the cause. The issue's own Directions ranked the retry last, "the cheapest change and the least durable", and that was right: on a cache miss the leg still needed community.chocolatey.org. What changed since is the weight. `bats` is a required check on `main` with enforce_admins true, so a red Windows leg now blocks every landing in the repository, and nobody can wave it through. sqlite3 now comes from one pinned sqlite.org URL, verified by hash: https://sqlite.org/2026/sqlite-tools-win-x64-3530400.zip sha256 f46ee2475de4cbe287e6e5f7d43c838796b14e7379cd216bdbb28d391429f9fc 6,557,792 bytes, contains sqlite3.exe All four measured by fetching it, not read off a page: the URL answered 200, the digest is of those bytes, and the archive was listed to confirm the binary is in it. What is gone is the FEED, not the third party -- sqlite.org is still somebody else's host. What it is not is a package index: no resolver, no two-phase lookup, and nothing that answers 503 for one concurrent job while serving another, which is the behaviour that made the original failure unreadable. The hash is load-bearing. Without it this would trade a feed that answers errors for a host that could answer anything, and the tests would run against whatever arrived. Kept from #827, deliberately: the presence check. Whatever supplies sqlite3, the step that decides the leg's colour must be the one that says the dependency is missing, not the one that runs the tests. Also corrects a comment #827 left behind, which still said a bump means "one trip to chocolatey, which is the only time the community feed is asked at all". There is no feed to ask. Not measured: whether the GitHub Windows image already ships sqlite3. It is absent from the Windows 2022 and 2025 image manifests, which is the published list rather than a `where sqlite3` on a live runner. * fix(remote): render embedded curl-config paths for the platform (#850) `_remote_http_post_json` writes a curl `-K` config so the request body — which holds the token — never reaches curl's argv. Two of the values it writes are paths: `dump-header` and `data = "@..."`. On Windows/Git Bash those paths are unopenable. MSYS translates POSIX paths to Windows form for a native binary's ARGV, and not for the contents of a file that binary reads. So `/tmp/...` written into the config stays `/tmp/...`, and native curl has no such path. curl fails, and the caller reports the "000" it reports for every failure, with no indication that a path was the problem. `cygpath -m`, not `-w`: -m yields a Windows drive path with FORWARD slashes, and curl's config parser treats a backslash as an escape — so the -w form is re-mangled by curl itself. Measured on the machine, not reasoned about. Gated on `command -v cygpath`, not on an OS name. Where cygpath does not exist the helper returns its argument unchanged, so macOS and Linux take exactly the path they took before — verified directly rather than inferred from a green suite: the helper is a pass-through for /tmp, $TMPDIR and a /private/tmp path on this machine. * fix(remote): dump headers to a file where a real fifo cannot exist (#850) The headers go through a fifo so a broken or hostile server cannot make us buffer an unbounded response: bounded-copy.py enforces the ceiling while the transfer is still running. That mechanism needs a real named pipe. On Windows/Git Bash there is no real named pipe to have. MSYS emulates mkfifo with a .lnk file that only MSYS-aware programs understand, and curl there is a NATIVE binary — it cannot open what mkfifo made. curl fails, and the caller sees the "000" it reports for every failure alike. Where cygpath exists, dump straight to the destination file and skip both the fifo and the copier. Gated on `command -v cygpath` rather than an OS name: what decides is whether a real fifo can be made, and that is what the probe asks. WHAT THIS GIVES UP, on that platform only: streaming enforcement of the header size ceiling. curl's own `max-filesize` still bounds the body; the header dump is what becomes unbounded. Written here rather than left implicit, because it is a real difference between the platforms. Also fixes a deletion the branch form makes dangerous. The cleanup ran `rm -f "$cfg" "$header_fifo"` unconditionally — and on the cygpath path `header_fifo` IS `header_file`, so it deleted the headers this function was asked to produce, before the caller read them. The fifo only exists when a copier was started, so that is what the removal is keyed on now. * fix(hash): SHA-256 without shasum, and no fingerprint it could not compute (#861) connect --e2ee registered the team with the server and then died on the way to the sync engine. The cause is one absent command: shasum is a Perl script that ships on macOS and most Linux and is NOT in Git for Windows' Git Bash. This repository already knew. scripts/lib/hash.sh exists for exactly this reason and its comment describes today's output -- "NOT in Git for Windows Git Bash, where it fails with shasum: command not found, leaving the hash empty". It was written for SHA-1 only, and the four SHA-256 sites never got the same treatment. Derived rather than taken on report: git grep shasum -- scripts/ gives six invocations across five files. Four are SHA-256 in production -- key.sh's two fingerprint helpers, key.sh's previous-snapshot digest, and remote.sh's age-v1 checkpoint -- and all four now go through the new helper. The fifth is hash.sh's own SHA-1 arm, unchanged. The sixth is release/update-cask.sh, which downloads a .dmg and pushes a Homebrew tap; it cannot run on the platform this is about, so it is deliberately left alone. THE LAST RESORT IS DIFFERENT ON PURPOSE. agmsg_sha1 falls through to cksum because its callers name a socket after the digest and need only stability on one machine. These callers need a real SHA-256: one value is the fingerprint two people read to each other over a separate channel, the other decides whether an epoch snapshot is the one it claims to be. A non-cryptographic stand-in there does not weaken the check, it makes it say something untrue. So agmsg_sha256 ends by FAILING, and the comment says why, because the obvious next edit is to make the two consistent. A FINGERPRINT THAT COULD NOT BE COMPUTED IS WORSE THAN NONE. The five print sites read `echo "Recipient fingerprint: $(_key_fingerprint ...)"`. A command substitution that fails inside a simple command's arguments leaves that command's own status alone, so echo succeeded and printed the label with nothing after it -- and the whole point of the string is that two people compare it. Both would have seen the same blank and agreed. Each site now takes the value into a variable of its own first, where a bare assignment's status IS the substitution's and set -e stops. Not `local x="$(...)"`, which puts the status back on the declaration and undoes it. Preflighted at connect, not only in doctor. Every SHA-256 in the e2ee path happens at or after key generation, which is after registration, so the check is asked before either. Reported by doctor as optional, next to age and for the same reason: a team on cipher "none" never computes one. The preflight probes by RUNNING the digest rather than asking command -v. A tool that is installed and broken answers yes to a presence check and no to the question, which is the direction that hurts -- the preflight passes and the digest fails later, which is the shape of this bug again. Left alone deliberately: remote.sh's fail-closed structure. An empty digest made the checkpoint invalid, setup refused, and connect reported "binding recorded, sync engine not started" without falling back to plaintext. That behaviour was correct; it is the digest that was wrong. UNLIKE #850, THIS IS VERIFIED HERE. The behaviour is chosen by command -v, so a PATH holding only the tools we pick reproduces each platform's choice on any platform -- no Windows machine required. Each arm is asserted against a literal digest rather than against the other arms, since three arms can agree on a wrong answer. The suite includes its own negative control (that a restricted PATH really does hide what it claims to) and covers the case that motivated the run-it probe: tools present and failing. * test(hash): the no-tool sandbox was hiding the fallback it exists to forbid The mutation that matters most for this change is "add a cksum arm, to make agmsg_sha256 consistent with agmsg_sha1" -- the exact edit the helper's comment warns the next reader against. Applied, the suite stayed green on the one test named for it. Not because the refusal survived. The sandbox PATH held only `awk`, so the mutant's `cksum` was not found either, the pipeline failed anyway, and the assertion passed for a reason that had nothing to do with the decision under test. It was measuring the box, not the behaviour. cksum is present on every machine this runs on, so that is the shape the test has to run in. With it in the box, the same mutation now turns four cases red, including the one whose name is the claim. Same correction applied to the other sandboxes built for "no SHA-256 tool": the message case, the usable probe, and the require preflight. * test: make the new assertions able to fail (#670 ratchet) CI's enforceable-assertions check counted 642 against a baseline of 638. The four are mine, and they are the shape that check exists for: a non-last `[[ ]]` cannot fail a test on bash 3.2, so it reads as an assertion and behaves as a comment. Three were in the "says which tools were looked for" case, where only the third of three could ever have fired -- the two that name shasum and sha256sum were decoration. The others were the first half of the doctor case and the message half of the connect preflight case, each followed by another assertion. All four are now plain `grep -qF`, which fails the test under bats' errexit. The count is back at the baseline and the suites are green; the mutation rows that rest on these assertions are being re-run, since rewriting an assertion changes the instrument and the earlier matrix was measured with the old one. * docs(key): the no-blank-fingerprint refusal rides on pipefail, so say which line agmsg_sha256 sits in the middle of the fingerprint pipeline and cut and sed are content with the empty input a failed digest leaves them. Without pipefail the pipeline exits 0 with an empty string and the label prints with nothing after it - the exact output the callers were changed to prevent. Measured: replacing set -euo pipefail with set -eu reddens both no-blank -fingerprint cases and nothing else. The comment names that control so the next person shortening line 2 has somewhere to look. * fix(hash): the helper's refusal was the caller's pipefail, and its answer was unchecked Two findings from static review, both one level below where the same shape was already noted. The arms were shasum|awk, so the function's status was awk's, and awk is content with the empty input a failed digest hands it. 'this FAILS' held only because key.sh and remote.sh set pipefail on line 2 - a caller without it got an empty success and carried it into a fingerprint. Each tool now runs as its own substitution with its status checked. And nothing looked at the answer. A tool that exits 0 while printing a warning is not a tool that failed, so 64 lowercase hex is now required, and the usable probe compares against the known digest of its input rather than asking whether anything came back - sixty-four characters that are the wrong sixty-four are exactly what a fingerprint cannot survive. Five cases, and one of them was rewritten after it failed to discriminate: the first no-pipefail case used an empty PATH, where the helper returns 1 outright and pipefail was never involved. A present- but-broken tool is the only path that isolates it. The case pinning the status check separately from the shape check exists because reverting the arms alone changed no result until it was written. * fix(hash): put the known-answer check in the helper, not in a list of entry points Review: the correctness check lived in agmsg_sha256_usable, whose only production callers are remote.sh's doctor and connect preflight. key.sh is its own CLI - generate, show, import and rotate all reach a digest without going near connect - so a tool exiting 0 with a well-formed but wrong digest was accepted by every one of them, and the suite pinned that as expected behaviour. agmsg_sha256 now asks the selected tool a question whose answer is known, once per process, before it will return anything. Keyed on the helper rather than on the callers because the alternative is a list of entry points kept complete by hand, which is what was missed. A key.sh case drives it directly with a lying shim, because a helper -level case cannot fail if a caller stops calling the helper. Also from review: require and doctor said 'none was found' and 'tool on PATH' for a tool that is present and broken. Presence and usability are different questions and these lines answer the second. * fix(hash): a memo that could be preseeded, and that never memoised anything Review, twice over on the same three lines. The self-test's flag was read from the environment, so _AGMSG_SHA256_VERIFIED=1 in an inherited environment meant 'already checked' and skipped it - an undocumented env override of a fail-closed contract, on the hole closed one commit ago. And it saved nothing. Every production call is printf | agmsg_sha256 or x="$(agmsg_sha256 …)", both subshells, so the flag never reached the parent and the self-test ran again on the next digest anyway. Removed rather than made private: a memo whose saving is imaginary is only a surface. The cost is now stated where the code is - one extra digest of a 5-byte input per digest taken, at most three per command. A case pins that no inherited variable can skip it, trying several names, because the property is about the environment and not about one spelling. * docs(hash): the comment kept the contract the code stopped having Three claims left over from the memo, all found in review, none needing code. The block heading still said ONCE PER PROCESS with RUN BEFORE EVERY DIGEST six lines under it - one comment stating two contracts, and a reader has no way to know which one the code has. agmsg_sha256_usable said it costs one subprocess. It calls agmsg_sha256, which self-tests and then digests, so it runs the tool twice. And the worst path was undercounted: key rotate with an accepted rotation to verify takes four digests, not three - the accepted recipient's fingerprint before the three already listed. * test(remote): bind the curl-config path rendering, both fields separately (#850) The stop condition was that reverting the production change leaves CI green. It does now: reverting the renderer turns two tests red, and reverting either single field turns exactly its own test red. The test drives the real _remote_http_post_json, sourced from remote.sh, with two stubs on PATH. What makes the stub worth anything is that it does not just record the config -- it OPENS WHAT THE CONFIG NAMES: cygpath -m yields forward slashes, -w yields backslashes. It must really produce both, or the test cannot tell the two apart, and telling them apart is the whole point of the fix naming -m. curl resolves each embedded path the way a native Windows binary resolves a Windows path back to the same file, and EXITS NON-ZERO when it cannot. A stub that accepts any string as a path would pass on a rendering no curl could open -- which is the defect itself. Four cases: no cygpath both fields byte-for-byte, so macOS and Linux are demonstrably unchanged rather than assumed unchanged cygpath, data rendered mixed cygpath, dump-header rendered mixed either way no backslash anywhere in the config The two field cases were one test until the matrix showed why they cannot be. Reverting the header field and reverting the data field both reddened the same combined assertion, which says 'something is untranslated' and points at neither. They are two effects of one line and regress apart: translate the body and not the header and curl opens the body, fails on the header, and the caller reports 000 -- which reads as a header problem. A fifth case is the control on the stub: replace _remote_curl_path after sourcing so it emits a path nothing can open, and the helper must return 000 -- the code the user actually saw. Without it, every assertion above could be passing on a string that no curl would accept. M0 no mutation 0 red M1 renderer back to passthrough 2 -- DATA and DUMP-HEADER M2 cygpath -m becomes -w 3 -- both fields and the slash assertion M3 header field left untranslated 1 -- DUMP-HEADER M4 data field left untranslated 1 -- DATA Production is untouched. Windows: still unverified here -- the cygpath branch is driven by a stub, and a stub cannot tell you what MSYS does. * test(remote): make the stubs disagree the way the two platforms do (#850) Review found two holes, both in my instrument rather than in the fix. P1-a THE NEGATIVE ARM WAS NOT NEGATIVE. 'without cygpath' prepended a directory holding only a curl symlink and left the host PATH behind it, so production's 'command -v cygpath' would still have found a real one. It passed here because macOS has no cygpath -- the test inherited an absence instead of creating it, and on the one machine this fix is for it would have run the wrong arm. Now an allowlist sandbox, the shape test_helper already uses for path_without_python3. A subtraction cannot express this: on Git Bash cygpath lives in the same directory as mktemp. The first test asserts the sandbox produces BOTH answers, so the capability every other arm rests on is measured rather than assumed. P1-b THE STUB ACCEPTED THE BROKEN PATH. Its resolve() took any absolute POSIX path whatever the platform, so it could not fail on the untranslated /tmp/... that broke the user -- while the file header claimed it opened what curl would open and the commit message claimed it failed where curl would fail. M1 went red on string assertions alone. The claim and the stub now say the same thing. The stub is told which consumer it plays. native-windows opens only Windows paths; posix opens only POSIX ones. Neither accepts both, which is the actual shape of the defect: the same bytes are openable or not depending on who reads them. Both directions have a case, so 'refuses a POSIX path' cannot be read as 'refuses whatever the test needs'. The outcome assertion moved out of the two field tests, and this was forced by the matrix rather than chosen. With the stub now failing on either untranslated field, asserting 200 inside both field tests made both go red for either mutation -- destroying the per-field attribution that splitting them was for. The 200 has its own test, whose job is to fail for any of them. M0 no mutation 0 red M1 renderer back to passthrough 3 -- DATA, DUMP-HEADER, completes M2 cygpath -m becomes -w 4 -- those three and the slash assertion M3 header field untranslated 2 -- DUMP-HEADER and completes M4 data field untranslated 2 -- DATA and completes 7 ok / 0 not ok. Production untouched; the diff is this one test file. Windows: still unverified by me. The cygpath arm is driven by stubs, and a stub cannot tell you what MSYS does. What this closes is 'revert production and CI stays green', not 'it works on Windows'. * test(remote): bind the header sink, and narrow the comment about the probe (#850) Two changes. The tests are the stop condition; the one production line is the ARTIFACT finding from the same review, and it is a comment. TESTS. tests/test_remote_header_sink.bats, 6 ok / 0 not ok, driving the real _remote_http_post_json under an allowlist sandbox PATH. Absences are the whole subject here -- no fifo, no copier -- and an absence is only observable if the instrument can record a presence. mkfifo and python3 are replaced by wrappers that log and then exec the real thing, so nothing about the run changes, and the first test proves the wrappers record at all. A log that was never wired up looks exactly like a call that never happened. no fifo is made marker present no bounded copier is started marker present the caller's header file survives marker present fifo AND copier still used marker absent -- the platforms that are supposed to be unchanged, asserted the fifo is removed, the header file is not marker absent The third is the trap this fix had to avoid: on the marker path header_fifo IS header_file, so a cleanup that removes the fifo deletes the headers the caller asked for, after a request that succeeded. M0 no mutation 0 red M1 marker ignored, always fifo 2 -- no fifo, no copier M2 cleanup removes header_fifo 1 -- header file survives M3 marker branch builds a fifo 1 -- no fifo M4 marker branch starts a copier 1 -- no copier M3 and M4 are why the two absences are two tests. They are separate machinery and are left behind separately: a fifo nobody drains, or a reader blocked on a file that never fills. One test asserting both reddens for either and names neither. COMMENT. The gate said "what decides is whether a real fifo can be made, and that is what the probe asks". It does not ask that. It runs a command -v on cygpath, which is a capability MARKER for an environment where MSYS fifos and a native curl coexist -- nothing here tests whether a real fifo can be made. As written it would have told the next reader that a machine passing the probe had been checked for the property that matters. Behaviour is unchanged: the diff outside tests/ is comment lines. This branch does not carry #851, so paths stay POSIX here and the curl stub is a POSIX consumer. On a real Windows machine this fix alone is not enough -- both are needed, and they compose only on the combined branch. Windows remains unverified by me. * feat(doctor): name the registry locks, and which directory to remove (#865) A lock left behind by a killed process is never broken by anything: nothing expires, nothing sweeps, and acquire waits out its budget and reports 'timed out acquiring registry lock', which describes contention. The operator goes looking for the process holding it; there is none, and no message anywhere names the directory to remove. A machine in that state cannot get itself out. doctor now lists each team's lock with what it records, whether that process is running, and the removal as a line to paste. Three answers, not two. 'held' and 'the holder is gone' are what an operator acts on; 'no holder recorded' is neither - a lock this cannot ask about, written either by a library version that recorded nothing or by a process killed between creating the lock and writing its record. Calling that gone would be a guess, and the guess that costs is the one that says a live lock is dead. It removes nothing, and it does not touch the exit code. A team being locked is not a failed prerequisite - reporting it as one would fail doctor every time somebody is joining - and sweeping automatically is a separate decision with a worse failure mode. Reported for the named team when doctor is given one, matching what its header already says. * fix(doctor): an unusable pid was never asked about, so it is not stale (#865 review) Three findings, all from static review. _agmsg_pid_alive_local returns false for a value it never put to the process table - non-numeric, leading zero, past the POSIX ceiling - and folding that into 'not running' turned 'pid not-a-pid' into a stale verdict with a rm -r beside it. The number is validated with the same ceiling the helper uses before it is asked about, and an unusable one joins 'no holder recorded' under cannot-tell. The sweep used *, which does not match a leading dot, and team names may begin with one - the validator rejects empty, . , .. , a leading -, / , \ and control characters, and nothing else. A named team is now looked at directly, and the sweep adds the two dot patterns. All checks passed. printed under a stale lock and its removal command reads as withdrawing them. Narrowed to All prerequisite checks passed.; the exit code deliberately does not move, so the wording is what carries it. Five existing assertions updated with it. Reporting one lock is its own function now. Both callers share it, so neither has to join paths into a string and read them back - which would mean an unquoted heredoc, running command substitution on a team name, or unquoted word splitting, globbing one. The first attempt at the sweep did build such a string and fed a while-read loop that had no redirection at all, so it read stdin and reported nothing. * fix(doctor): validate the named team before it becomes a path (#865 review) cmd_doctor takes an optional team and, until the direct lookup landed, only ever put it in a header sentence - so nothing had ever validated it. Building TEAMS_DIR/$team/.config.lock from it turned 'doctor ../outside' into a read of a directory outside the store, reported with its records and a rm -r to paste. The sweep it replaced never reached one only because no team was named that. Validated with agmsg_validate_team_name, the same one every other team-taking path uses, and a rejection ends the command rather than falling through to a prerequisites verdict for a question nobody answered. Three cases: a traversal argument reads nothing (with a sentinel .config .lock outside the store to prove the refusal is about reach), a slash is refused, and an ordinary and a dot-leading name still resolve - a refusal that took the legitimate names with it would be the cheapest way to pass the first two. * test(doctor): follow the summary line the lock report narrowed #862 asserted on "All checks passed"; #865's doctor narrowed the summary to "All prerequisite checks passed." so it cannot be read as cancelling a stale lock and the removal command printed above it. The two landed on different lines, so the merge was clean and the assertion was left describing a sentence the code no longer emits. * test(remote): assert the status line, not the stream a killed copier writes into (#850) * test(remote): name the issue the loosened assertion is recording (#869)
Owner
Author
|
Shipped in v1.2.1. This landed through Verified present in
Closing. The behaviour is live. |
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
1 participant
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.
Fixes #861.
connect --e2eeon Git for Windows registered the team with the server and then died on the way to the sync engine. One absent command explains it:shasumis a Perl script that ships on macOS and most Linux and is not in Git for Windows' Git Bash.Change classification
Behaviour changes for existing users. A path that could not be walked on Windows now can be. Nothing changes on macOS or Linux:
shasumis still the first arm and returns the same digests it always did.The repository already knew
scripts/lib/hash.shexists for exactly this reason, and its comment describes the output seen this week — "NOT in Git for Windows Git Bash, where it fails withshasum: command not found, leaving the hash empty". It was written for SHA-1 only. The SHA-256 sites never got the same treatment.What was changed, derived rather than taken on report
git grep shasum -- scripts/gives six invocations across five files. Four are SHA-256 in production and all four now go through a newagmsg_sha256:key.sh_key_fingerprintkey.sh_key_fingerprint_sha256key.shrotateremote.shThe fifth is
hash.sh's own SHA-1 arm, unchanged. The sixth isscripts/release/update-cask.sh, which downloads a.dmgand pushes a Homebrew tap — it cannot run on the platform this is about, so it is deliberately left alone rather than converted for symmetry.The last resort is different from the SHA-1 helper, on purpose
agmsg_sha1falls through tocksumbecause its callers name a socket after the digest and need only that the same input give the same name on the same machine. These callers need the opposite property: one value is a fingerprint two people read to each other to confirm they hold the same key, the other decides whether an epoch snapshot is the one it claims to be. A non-cryptographic stand-in there does not weaken the check — it makes it assert something untrue.So
agmsg_sha256ends by failing, and says so in a comment, because the obvious next edit is to make the two helpers consistent.A fingerprint that could not be computed is worse than none
The five print sites read
echo "Recipient fingerprint: $(_key_fingerprint ...)". A command substitution that fails inside a simple command's arguments leaves that command's own status alone — soechosucceeded and printed the label with nothing after it. The entire purpose of that string is that two people compare it, and both would have seen the same blank and agreed.Making the helper fail is not enough to fix this; the call site has to be able to notice. Each one now takes the value into a variable of its own first, where a bare assignment's status is the substitution's and
set -estops. Notlocal x="$(...)", which puts the status back on the declaration and undoes it.Preflighted before registration, not only in
doctorEvery SHA-256 in the e2ee path happens at or after key generation, which is after the team is registered — which is why the symptom read as a server problem rather than a missing tool. The check is now asked before both.
doctoralso reports it, as optional and next toage, for the same reason: a team on ciphernonenever computes one.The probe runs the digest rather than asking
command -v. A tool that is installed and broken answers yes to a presence check and no to the question, and that is the direction that hurts — the preflight passes and the digest fails later, which is the shape of this bug again.Left alone deliberately
remote.sh's fail-closed structure. An empty digest made the checkpoint invalid, setup refused, and connect reported "binding recorded, sync engine not started" without ever falling back to plaintext. That behaviour was correct. It was the digest that was wrong, and with a digest it can compute, this path now simply completes.Unlike the sibling Windows fix, this one is verified here
The behaviour is selected by
command -v, not by an OS name, so a PATH holding only the tools we choose reproduces each platform's choice on any platform. What that buys is bounded, and the boundary is worth stating: the arms, the refusal and the shape of each answer are host-independent and are measured here. Whether Git for Windows'sha256sumis on a given operator's PATH is not, and no run here says anything about it.Each arm is asserted against a literal digest rather than against the other arms — three arms that agree can agree on a wrong answer. The suite carries its own negative control (that a restricted PATH really does hide what it claims to hide) and covers the case that motivated the run-it probe: tools present and failing.
Mutation results
Green means nothing on its own, so each part of the change was reverted in turn to see whether anything noticed. Every row turned red, and each mutation hits its own case:
sha256sumarmopensslarmcksuminstead of failingshasumwithout-a 256echo(generate)echo(show)doctoralways claims a toolset -o pipefailfromkey.shtool | awkusableback to "something came back"key generatewith a lying tool, and the helper's own caseThree rows were re-measured after the instrument changed. Four assertions were rewritten to
grep -qFfor the enforceable-assertions check, which means the earlier table was taken with a different instrument than the one now in the tree. The three rows those assertions could touch were run again on542c2ff:cksuminstead of failing (4 cases red — the refusal, the message, the usable probe, the install guidance), removing the connect preflight (its case, and only its case), anddoctoralways claiming a tool (its case, and only its case). The other six rows stand as taken.The last row is the mechanism, and it was not obvious.
_key_fingerprintisprintf | agmsg_sha256 | cut | sed, andcutandsedare perfectly content with the empty input a failed digest leaves them — so the pipeline's own status issed's, which is 0. Taking the value into a variable of its own is necessary but not sufficient; what makes the assignment fail ispipefail, on line 2, three hundred lines away from the code it protects. Replacingset -euo pipefailwithset -eureddens exactly the two cases that exist to forbid a blank fingerprint.key.shnow says so where the pipeline is.One mutation run was thrown away. The first attempt at the
cksumrow dropped a closing quote,bash -nfailed, and nine of eleven cases went red — a red from a script that does not parse, which says nothing about any assertion. The harness now runsbash -nover all three scripts before it will report a result.The
cksumrow is worth naming: the first time it ran, it changed nothing. The sandbox PATH held onlyawk, so the mutant'scksumwas not found either and the assertion passed for a reason unrelated to the decision under test — it was measuring the box, not the behaviour.cksumis present on every machine this runs on, so the sandbox now holds it too, and that row is the one it should be.The helper's refusal was the caller's, and its answer was unchecked
Static review found the shape this PR describes in
key.shsitting one level below it, in the helper itself.shasum -a 256 | awk '{print $1}'makes the function's statusawk's, andawkis content with the empty input a failed digest hands it — so "no tool, or a broken one, and this FAILS" held only because both callers happen toset -o pipefailon line 2. Each tool now runs as its own command substitution with its status checked, and the refusal travels with the function.Nothing looked at the answer either. A tool that exits 0 while printing a warning has not failed as far as
$?is concerned, and this value is the one two people read to each other. The helper now requires 64 lowercase hex, andagmsg_sha256_usablecompares against the known digest of its own probe input rather than asking whether anything came back — sixty-four characters that are the wrong sixty-four is the case a fingerprint cannot survive.The two checks overlap and are not redundant: a tool that exits non-zero while printing a well-formed digest is caught only by the status, one that exits zero while printing anything else only by the shape. There is a mutation row for each.
One of the new cases was rewritten because it could not fail. The first "refuses without pipefail" case used an empty PATH — where the helper takes its
elsebranch and returns 1 outright, so pipefail was never in the picture. Reverting the arms changed no result, which is how it was caught. A present-but-broken tool is the only path that isolates the status.And the correctness check was wired to the wrong thing. For one head it lived in
agmsg_sha256_usable, whose only production callers areremote.sh'sdoctorand theconnectpreflight — sokey.sh, which is its own CLI and reaches a digest throughgenerate,show,importandrotatewithout going nearconnect, accepted a tool that exits 0 with a well-formed but wrong digest. The suite pinned that as expected behaviour. Raised in review.agmsg_sha256now asks the selected tool a question whose answer is known, before every digest, and returns nothing until it has. Keyed on the helper and not on the callers, because the alternative is a list of entry points kept complete by hand — which is precisely what was missed. Akey.shcase drives it with a lying shim, because a helper-level case cannot fail if a caller stops calling the helper. What it claims is bounded: the tool answered correctly the last time it was asked, which is immediately before the digest being taken.The self-test was memoised, and the memo was worse than useless. Its flag was read from the environment, so
_AGMSG_SHA256_VERIFIED=1in an inherited environment read as "already checked" and skipped the check — an undocumented override of a fail-closed contract, sitting on the hole closed one commit earlier. It also saved nothing: every production call isprintf | agmsg_sha256orx="$(agmsg_sha256 …)", both subshells, so the flag never reached the parent and the next digest self-tested anyway. Both halves raised in review.It is removed rather than made private — a memo whose saving is imaginary is only a surface. The cost is stated instead: one extra digest of a five-byte input per digest taken. The worst path is
key rotatewith an accepted rotation to verify — the accepted recipient's fingerprint, the new recipient's journal fingerprint, the previous snapshot, and the short fingerprint printed at the end: four digests, so eight runs of the tool, each already alongside file and lock work. (Three, and one run each, was what this section said until review counted the accepted-rotation branch.) The case that pins it tries several variable names, because the property is about the environment rather than about one spelling.requireanddoctorsaid "none was found" and "tool on PATH" for a tool that is present and broken. Presence and usability are different questions and both lines answer the second one; reworded, with the cases following.The fallback is for an ABSENT tool, not a broken one. Presence picks the arm and a chosen arm that fails ends it; the working tool behind it is not tried. That was undocumented, is now stated in the helper, and has a case.
What is not measured
No part of this ran on Windows. What is claimed is that the fallback chain and the refusal behave as described when the tools are absent or broken, which is measured directly. Whether Git for Windows'
sha256sumis on the operator's PATH at the moment they runconnectis a property of their install, not of this change.Suites
tests/test_hash.bats(new, 18 cases), and akey.shcase driving the same question through the CLI,tests/test_key.bats,tests/test_remote.bats.