fix(609): close two false-arm holes found in cross-family review
Codex review of the first attempt found both, and both were in the git plumbing
that my unit tests never touched -- they only exercised the pure predicate.
1. HIGH: git's `%s` is the first PARAGRAPH, not the first line. It joins
consecutive non-blank lines with spaces, so
`fix: harmless subject`
`This explains [decisions-edit] on line two.`
came back as ONE line containing the token and armed it -- the exact
false-arm this change exists to prevent. The first line is now taken from
`%B` via `subject_of()`.
2. HIGH: the in-band `\x1f`/`\x1e` field separators were injectable. A subject
containing a literal `\x1f` was split at the wrong place and its tail read as
a trailer, arming the token. Framing is now NUL, which git forbids inside a
commit message and which therefore cannot be injected, with exact-arity
parsing (fields must be a multiple of three) that refuses to arm otherwise.
Also from the same review:
- Refuse to arm on a `%(trailers:...)` atom echoed literally by a git older than
2.22, which would otherwise read as a non-empty trailer (exit 0, so `_run`
returns it rather than None).
- Record corrected: 38 subject-tokened commits in ancestry, not "twenty"; and it
no longer claims a blanket fail-safe -- `_token_armed` failing is safe, but the
surrounding `_diff_findings` fails open earlier on an unresolvable merge-base,
skipping every check. That predates this change.
Adds 8 integration tests that drive `_token_armed` against a real throwaway git
repo -- the gap that let both defects pass. Verified non-vacuous by
reconstructing the old implementation in memory: it arms on both inputs, the new
one does not.
Note `--format` uses git's `%x00` escape, not a literal NUL: a NUL in argv raises
ValueError from subprocess, which broke every diff-engine test until fixed.
This commit is contained in:
+17
-7
@@ -3943,14 +3943,24 @@ laundering, archive survivors). `removed` and `demoted` still ran, so the job pr
|
||||
and it bit in the worst possible place: that PR had hand-resolved an append-vs-append conflict inside
|
||||
`docs/decisions.md`, which is precisely the operation the guard exists to police.
|
||||
|
||||
**Scoping to the subject preserves every historical use.** All twenty prior tokened commits append
|
||||
`[decisions-edit]` to the subject (or place it mid-subject, as `docs(434):` does); none put it on its
|
||||
own line, so an "own-line only" rule — the obvious first instinct — would have broken all of them.
|
||||
The `Decisions-Edit:` trailer is added alongside as the forward-looking form because it can carry a
|
||||
*reason*, which the bracketed marker cannot.
|
||||
**Scoping to the subject preserves every historical use.** All 38 prior tokened commits in `main`'s
|
||||
ancestry carry `[decisions-edit]` in the subject — usually appended, sometimes mid-subject as
|
||||
`docs(434):` does; none put it on its own line, so an "own-line only" rule — the obvious first
|
||||
instinct — would have broken all of them. The `Decisions-Edit:` trailer is added alongside as the
|
||||
forward-looking form because it can carry a *reason*, which the bracketed marker cannot.
|
||||
|
||||
**Fail-open posture is unchanged**: if git can't be interrogated the token reads unarmed, so the
|
||||
guard still runs. Failing the other way would silently disable it, which is the bug being fixed.
|
||||
**The subject is computed, not taken from git's `%s`.** That atom is the first *paragraph*: it joins
|
||||
consecutive non-blank lines with spaces, so `fix: harmless` followed by a second line mentioning the
|
||||
token would arm it. The first line is taken from `%B` instead. Fields are framed with **NUL**, which
|
||||
git forbids inside a commit message and is therefore the one separator a message cannot inject — an
|
||||
in-band `\x1f` let a subject containing that byte be split at the wrong place and its tail read as a
|
||||
trailer. Parsing demands exact arity (a multiple of three fields) and refuses to arm on anything
|
||||
else, including a `%(trailers:…)` atom echoed literally by a git too old to expand it.
|
||||
|
||||
**Unarmed is the safe default for this function**: if git can't be interrogated the token reads
|
||||
unarmed, so the rationale comparisons still run. Note the *surrounding* `_diff_findings` fails open
|
||||
separately and earlier — an unresolvable merge-base returns no findings at all, skipping every diff
|
||||
check including this one. That predates this record and is unchanged by it.
|
||||
|
||||
**The general lesson is the one already recorded for CI behaviour**: a check that can silently
|
||||
no-op is worse than no check, because its green reads as coverage. Verify a guard by *measuring that
|
||||
|
||||
@@ -232,7 +232,14 @@ def _archive_records_at(ref: str):
|
||||
|
||||
|
||||
_TRAILER_KEY = "Decisions-Edit"
|
||||
_REC_SEP, _FIELD_SEP = "\x1e", "\x1f"
|
||||
# NUL framing, not \x1f/\x1e. git forbids NUL inside a commit message, so it is the ONLY separator
|
||||
# a crafted (or merely odd) message cannot inject: a subject containing a literal \x1f would
|
||||
# otherwise be split at the wrong place and its tail read as a trailer — falsely ARMING the token,
|
||||
# which is the dangerous direction because arming silently disables the guard.
|
||||
_NUL = "\x00" # what we SPLIT the output on
|
||||
_NUL_FMT = "%x00" # what goes in the --format string; a literal NUL in argv raises ValueError
|
||||
_FIELDS_PER_COMMIT = 3
|
||||
_UNEXPANDED_ATOM = "%(trailers"
|
||||
|
||||
|
||||
def token_armed_in(subject: str, trailer: str) -> bool:
|
||||
@@ -253,25 +260,44 @@ def token_armed_in(subject: str, trailer: str) -> bool:
|
||||
return bool(trailer.strip())
|
||||
|
||||
|
||||
def subject_of(body: str) -> str:
|
||||
"""The commit's true first line.
|
||||
|
||||
NOT git's `%s`: that atom is the first *paragraph*, joining consecutive non-blank lines with
|
||||
spaces, so `fix: harmless\\nprose about [decisions-edit]\\n\\n…` would come back as one line
|
||||
containing the token and arm it. Take the first line ourselves from `%B`.
|
||||
"""
|
||||
return body.lstrip("\n").split("\n", 1)[0].strip()
|
||||
|
||||
|
||||
def _token_armed(mb: str, head: str) -> bool:
|
||||
"""True if ANY commit in mb..head carries the token. Unresolvable git → False (guard runs)."""
|
||||
"""True if ANY commit in mb..head carries the token. Anything unexpected → False (guard runs)."""
|
||||
out = _run(
|
||||
[
|
||||
"git",
|
||||
"log",
|
||||
f"--format=%s{_FIELD_SEP}%(trailers:key={_TRAILER_KEY},valueonly){_REC_SEP}",
|
||||
f"--format=%H{_NUL_FMT}%B{_NUL_FMT}%(trailers:key={_TRAILER_KEY},valueonly){_NUL_FMT}",
|
||||
f"{mb}..{head}",
|
||||
]
|
||||
)
|
||||
if out is None:
|
||||
# Same posture as the previous implementation: on git trouble the token is NOT armed, so
|
||||
# the guard still runs. Failing the other way would silently disable it.
|
||||
# On git trouble the token is NOT armed, so the guard still runs. Failing the other way
|
||||
# would silently disable it — the bug this whole function exists to fix.
|
||||
return False
|
||||
for entry in out.split(_REC_SEP):
|
||||
if not entry.strip():
|
||||
parts = out.split(_NUL)
|
||||
if parts and not parts[-1].strip():
|
||||
parts.pop() # trailing inter-record newline after the final NUL
|
||||
if len(parts) % _FIELDS_PER_COMMIT != 0:
|
||||
# Exact arity, not best-effort. NUL cannot occur in a commit message, so a bad count means
|
||||
# the output was truncated or the format atom wasn't understood — refuse to arm.
|
||||
return False
|
||||
for i in range(0, len(parts), _FIELDS_PER_COMMIT):
|
||||
body, trailer = parts[i + 1], parts[i + 2]
|
||||
if trailer.strip().startswith(_UNEXPANDED_ATOM):
|
||||
# git too old to expand %(trailers:...): it echoes the atom literally with exit 0, which
|
||||
# would read as a non-empty trailer and falsely arm. Treat as unarmed.
|
||||
continue
|
||||
subject, _, trailer = entry.partition(_FIELD_SEP)
|
||||
if token_armed_in(subject.strip(), trailer):
|
||||
if token_armed_in(subject_of(body), trailer):
|
||||
return True
|
||||
return False
|
||||
|
||||
|
||||
@@ -510,3 +510,98 @@ def test_token_NOT_armed_by_body_prose():
|
||||
def test_token_not_armed_by_empty_or_whitespace_trailer():
|
||||
assert not dv.token_armed_in("docs: something", "")
|
||||
assert not dv.token_armed_in("docs: something", " \n ")
|
||||
|
||||
|
||||
# ---- _token_armed INTEGRATION against real git (ersatztv#609 review round 2) ----
|
||||
#
|
||||
# The unit tests above only exercise `token_armed_in`. Both false-arm defects found in review
|
||||
# (git's `%s` folding the first PARAGRAPH, and an injectable in-band separator) lived in the git
|
||||
# plumbing and passed those tests untouched. These drive the real command.
|
||||
|
||||
|
||||
def _repo(tmp_path, messages: list[str]) -> tuple[str, str]:
|
||||
"""Build a throwaway repo; return (base_sha, head_sha)."""
|
||||
import subprocess
|
||||
|
||||
def g(*a, **kw):
|
||||
return subprocess.run(["git", *a], cwd=tmp_path, capture_output=True, text=True, **kw)
|
||||
|
||||
g("init", "-q", ".")
|
||||
g("config", "user.email", "t@e")
|
||||
g("config", "user.name", "t")
|
||||
(tmp_path / "f").write_text("base\n")
|
||||
g("add", "-A")
|
||||
g("commit", "-qm", "base")
|
||||
base = g("rev-parse", "HEAD").stdout.strip()
|
||||
for i, msg in enumerate(messages):
|
||||
(tmp_path / "f").write_text(f"{i}\n")
|
||||
g("add", "-A")
|
||||
g("commit", "-q", "-F", "-", input=msg)
|
||||
return base, g("rev-parse", "HEAD").stdout.strip()
|
||||
|
||||
|
||||
def _armed(tmp_path, message: str) -> bool:
|
||||
import os
|
||||
|
||||
base, head = _repo(tmp_path, [message])
|
||||
cwd = os.getcwd()
|
||||
os.chdir(tmp_path)
|
||||
try:
|
||||
return dv._token_armed(base, head)
|
||||
finally:
|
||||
os.chdir(cwd)
|
||||
|
||||
|
||||
def test_integration_subject_token_arms(tmp_path):
|
||||
assert _armed(tmp_path, "fix(460): write null LastScan [decisions-edit]\n\nbody\n")
|
||||
|
||||
|
||||
def test_integration_trailer_arms(tmp_path):
|
||||
assert _armed(tmp_path, "docs: correct a figure\n\nwhy\n\nDecisions-Edit: the load number was wrong\n")
|
||||
|
||||
|
||||
def test_integration_body_prose_does_not_arm(tmp_path):
|
||||
assert not _armed(tmp_path, "docs: explain it\n\nThe check matches [decisions-edit] as a substring.\n")
|
||||
|
||||
|
||||
def test_integration_second_line_of_first_paragraph_does_not_arm(tmp_path):
|
||||
"""git's %s is the first PARAGRAPH, not the first line — it folds line 2 in with a space.
|
||||
|
||||
Using %s directly, this message arms the token. It must not: line 2 is body prose.
|
||||
"""
|
||||
assert not _armed(tmp_path, "fix: harmless subject\nThis explains [decisions-edit] on line two.\n\nbody\n")
|
||||
|
||||
|
||||
def test_integration_separator_in_subject_cannot_inject(tmp_path):
|
||||
"""An in-band \\x1f separator was injectable; NUL cannot appear in a commit message."""
|
||||
assert not _armed(tmp_path, "docs: harmless\x1fsuffix\n\nbody\n")
|
||||
assert not _armed(tmp_path, "docs: harmless\x1e\x1fsuffix\n\nbody\n")
|
||||
|
||||
|
||||
def test_integration_empty_trailer_does_not_arm(tmp_path):
|
||||
assert not _armed(tmp_path, "docs: something\n\nbody\n\nDecisions-Edit:\n")
|
||||
|
||||
|
||||
def test_integration_only_a_later_commit_carries_it(tmp_path):
|
||||
import os
|
||||
|
||||
base, head = _repo(tmp_path, ["chore: unrelated\n", "fix: real correction [decisions-edit]\n"])
|
||||
cwd = os.getcwd()
|
||||
os.chdir(tmp_path)
|
||||
try:
|
||||
assert dv._token_armed(base, head)
|
||||
finally:
|
||||
os.chdir(cwd)
|
||||
|
||||
|
||||
def test_integration_unresolvable_refs_do_not_arm(tmp_path):
|
||||
"""Guard must still RUN when git can't answer; arming on failure would disable it."""
|
||||
assert not _armed(tmp_path, "docs: x\n") or True # build a repo first
|
||||
import os
|
||||
|
||||
cwd = os.getcwd()
|
||||
os.chdir(tmp_path)
|
||||
try:
|
||||
assert dv._token_armed("nope1", "nope2") is False
|
||||
finally:
|
||||
os.chdir(cwd)
|
||||
|
||||
Reference in New Issue
Block a user