Audit follow-ups: id round-tripping, report selection, and three wording/invariant fixes #44

2026-07-31T20:41:17-07:00

branderson commented

2026-07-31 20:41:17 -07:00

Summary generated by LLM agent

Follow-ups to #39, branched off audit-alerting rather than main — this
targets that branch and should land after it.

Closes #40, #41, #42, #43.

#40 — ids could not round-trip through `ack`

The substantive one, and it turned out to have two independent halves.

compare wrote sanitized text to the report, ack derived the baseline
id from that text, and the next compare matched that id against the raw
bytes on disk. Any id containing something sanitize() strips therefore could
never round-trip: the surface stayed unsuppressible, its report unclearable,
and the startup warning fired on it forever — the alert fatigue this feature
exists to end, reached from the other side.

Ids now travel hex-encoded behind a {hex} marker, and the encoded form is
canonical wherever an id is written down: the report and the baseline
file. An id that survives sanitize() unchanged is written verbatim — that is
every ordinary id, so reports and existing baseline files are byte-for-byte
what they were and need no migration. Display-only text (a git config value, a
cross-project slug, the workspace path) still goes through sanitize; only
the id-bearing half of a git config finding is encoded, so a value full of
control bytes cannot drag its key into the opaque form and hide which setting
changed.

The encoded form is also the honest one. sanitize() deletes those bytes,
so a planted .git/hooks/pre<U+202E>commit was displayed as precommit — a
name no file on disk has, sending the operator grepping for nothing. Two tests
asserted that stripped rendering; they now assert the encoded one.

The second half was found while reviewing the first. The snapshot record
ids are derived from is itself path<TAB>sha, read back with cut -f1, so a
filename containing a TAB was truncated upstream of any encoding:
.git/hooks/pre<TAB>commit was reported as .git/hooks/pre — still CRITICAL,
since the truncation still lands under .git/hooks/, but naming a file that
does not exist. Same failure, different route, and it would have survived the
first commit entirely. hash_paths/tcb_snapshot now write the encoded path,
which by construction contains neither TAB nor newline.

That put enc_id on a per-file loop on both the snapshot and compare side,
and a bare git init alone ships ~14 sample hooks — running the exact check
unconditionally took the audit suite from 55s to over two minutes. It has a
subprocess-free fast path now, deliberately conservative: anything that might
need encoding falls through to the exact check.

#41 — `show` and `ack` disagreed on which report they meant

ack prefers the oldest unacknowledged CRITICAL report; show kept its own
sort | tail -1, which always prefers the newer file. An operator following
the startup warning and reaching for show first was handed a different
report than the one being warned about. Both verbs share one
select_report() now, and show names the report it printed (on stderr, so
stdout stays report content alone).

#42 — not reachable as filed; fixed as an invariant

The issue described SANDBOX_GITCONFIG_DIR being unbound in cleanup() when
mktemp -d fails. It is not — a failed VAR="$(...)" binds VAR to the
empty string before set -e fires, so the trap completes. The window between
the trap and the assignment holds only comments, leaving no way to exit inside
it at all.

Fixed anyway as the invariant every other name in cleanup() already keeps,
and pinned with a static test covering the whole class, so the edit that puts
something exitable in that window fails the suite instead of silently
truncating the trap before the audit it deliberately runs last.

#43 — three stores, one warning text

The audit and baseline stores reused the approval store's containment warning
verbatim, so all three reported that "the global approvals and every project's
under it are writable" — right alarm, wrong consequence, printable three times
in a run with no way to tell which store was meant. Each states its own now.
Also collapses the audit-dir default, which was spelled out a second time at
the startup-warning site, back onto the AUDIT_DIR variable that already
holds it.

Testing

Four host-side suites green: audit 84, static 114, project 68,
remote 101.

Every new test was verified fail-first against the unfixed code, and the
first attempt at the #42 test was discarded for passing without the fix —
it stubbed mktemp to fail, which is what revealed that the issue's stated
trigger does not reproduce at all. The #40 marker-forgery test was likewise
rewritten after its first version asserted a vector that does not exist
(a path can never begin with the marker; a hand-written git config section
name can, and that is what it tests now).

test/runtime.bats and test/proxy.bats need Docker and were not run —
unavailable in the sandbox. This branch touches bin/dejima's startup warning
and its exit trap, which is what those suites cover, so they need a host run
before merge, same as #39.

**Summary generated by LLM agent** Follow-ups to #39, branched off `audit-alerting` rather than `main` — this targets that branch and should land after it. Closes #40, #41, #42, #43. ## #40 — ids could not round-trip through `ack` The substantive one, and it turned out to have two independent halves. `compare` wrote **sanitized** text to the report, `ack` derived the baseline id from that text, and the next `compare` matched that id against the **raw** bytes on disk. Any id containing something `sanitize()` strips therefore could never round-trip: the surface stayed unsuppressible, its report unclearable, and the startup warning fired on it forever — the alert fatigue this feature exists to end, reached from the other side. Ids now travel hex-encoded behind a `{hex}` marker, and the encoded form is canonical wherever an id is written down: the report **and** the baseline file. An id that survives `sanitize()` unchanged is written verbatim — that is every ordinary id, so reports and existing baseline files are byte-for-byte what they were and need no migration. Display-only text (a git config value, a cross-project slug, the workspace path) still goes through `sanitize`; only the id-bearing half of a git config finding is encoded, so a value full of control bytes cannot drag its key into the opaque form and hide which setting changed. The encoded form is also the honest one. `sanitize()` **deletes** those bytes, so a planted `.git/hooks/pre<U+202E>commit` was displayed as `precommit` — a name no file on disk has, sending the operator grepping for nothing. Two tests asserted that stripped rendering; they now assert the encoded one. **The second half was found while reviewing the first.** The snapshot record ids are derived from is itself `path<TAB>sha`, read back with `cut -f1`, so a filename containing a TAB was truncated *upstream of any encoding*: `.git/hooks/pre<TAB>commit` was reported as `.git/hooks/pre` — still CRITICAL, since the truncation still lands under `.git/hooks/`, but naming a file that does not exist. Same failure, different route, and it would have survived the first commit entirely. `hash_paths`/`tcb_snapshot` now write the encoded path, which by construction contains neither TAB nor newline. That put `enc_id` on a per-file loop on both the snapshot and compare side, and a bare `git init` alone ships ~14 sample hooks — running the exact check unconditionally took the audit suite from 55s to over two minutes. It has a subprocess-free fast path now, deliberately conservative: anything that might need encoding falls through to the exact check. ## #41 — `show` and `ack` disagreed on which report they meant `ack` prefers the oldest unacknowledged CRITICAL report; `show` kept its own `sort | tail -1`, which always prefers the newer file. An operator following the startup warning and reaching for `show` first was handed a different report than the one being warned about. Both verbs share one `select_report()` now, and `show` names the report it printed (on stderr, so stdout stays report content alone). ## #42 — not reachable as filed; fixed as an invariant The issue described `SANDBOX_GITCONFIG_DIR` being unbound in `cleanup()` when `mktemp -d` fails. **It is not** — a failed `VAR="$(...)"` binds VAR to the empty string before `set -e` fires, so the trap completes. The window between the trap and the assignment holds only comments, leaving no way to exit inside it at all. Fixed anyway as the invariant every other name in `cleanup()` already keeps, and pinned with a static test covering the whole class, so the edit that puts something exitable in that window fails the suite instead of silently truncating the trap before the audit it deliberately runs last. ## #43 — three stores, one warning text The audit and baseline stores reused the approval store's containment warning verbatim, so all three reported that "the global approvals and every project's under it are writable" — right alarm, wrong consequence, printable three times in a run with no way to tell which store was meant. Each states its own now. Also collapses the audit-dir default, which was spelled out a second time at the startup-warning site, back onto the `AUDIT_DIR` variable that already holds it. ## Testing Four host-side suites green: `audit` 84, `static` 114, `project` 68, `remote` 101. Every new test was verified fail-first against the unfixed code, and the first attempt at the #42 test was **discarded for passing without the fix** — it stubbed `mktemp` to fail, which is what revealed that the issue's stated trigger does not reproduce at all. The #40 marker-forgery test was likewise rewritten after its first version asserted a vector that does not exist (a path can never *begin* with the marker; a hand-written git config section name can, and that is what it tests now). `test/runtime.bats` and `test/proxy.bats` need Docker and **were not run** — unavailable in the sandbox. This branch touches `bin/dejima`'s startup warning and its exit trap, which is what those suites cover, so they need a host run before merge, same as #39.

branderson added 5 commits 2026-07-31 20:41:18 -07:00

dejima: keep cleanup()'s one late-bound name from breaking the trap invariant feb60c5a93

SANDBOX_GITCONFIG_DIR is the only name cleanup() removes that is first
assigned below `trap cleanup EXIT` rather than above it, so it alone
lacked the `:-` default its neighbours carry.

Issue #42 described this as reachable when `mktemp -d` fails. It is not:
a failed `VAR="$(...)"` binds VAR to the empty string before set -e
fires, so the trap completes. The window between the trap and the
assignment holds only comments, leaving no way to exit inside it at all.

Fixed anyway as an invariant rather than a live bug, and pinned with a
static test that checks the whole class - every name cleanup()
dereferences is either assigned above the trap or carries a `:-` form
somewhere in the function - so the edit that puts something exitable in
that window fails the suite instead of silently truncating the trap
before the audit it deliberately runs last.

dejima: give each containment warning the consequence it actually has f1f5c9ad3f

The audit report and baseline stores were folded into the approval
store's containment check by reusing its warning text verbatim, so all
three reported that 'the global approvals and every project's under it
are writable' - the right alarm attached to the wrong consequence, and
now printable up to three times in one run with no way to tell which
store was meant.

Each store states its own: the approval store lets a session approve its
own domains and dependencies; the report store lets it delete the
exit-time findings of earlier runs, including unacknowledged CRITICAL
ones naming it; the baseline store lets it write its own
acknowledgements and suppress its findings from every later report.

Also collapses the audit-dir default, which was spelled out a second
time at the startup-warning site, back onto the AUDIT_DIR variable that
already holds it.

Tests now assert the store-specific text, plus that the report store's
warning does not mention the approval store - the reuse this fixes.

audit: make show and ack agree on which report they mean c81ecce3c2

ack was fixed to present the oldest unacknowledged CRITICAL report before
falling back to the newest, so a stale one could not hide behind a newer
clean report. show kept its own `sort | tail -1`, which always prefers
the newer file - so an operator following the startup warning and
reaching for show first was shown a different report than the one being
warned about, and went looking for a finding that was not in it.

Both verbs now share one select_report(), which is where the reasoning
for the ordering now lives. show also names the report it printed, on
stderr so stdout stays report content alone: it is no longer necessarily
the newest, and the warning lists runs by timestamp.

ack's local variable was called `newest` even after it stopped being the
newest, with a comment apologising for it; it is `report` now and the
comment is gone.

audit: encode finding ids instead of sanitizing them, so ack can clear them 6a049c8d8b

compare wrote sanitized text to the report, ack derived the baseline id
from that text, and the next compare matched the id against the raw bytes
on disk. Any id containing a character sanitize() strips - C0, DEL, C1, or
a Unicode bidi override - therefore could not round-trip: the surface
stayed unsuppressible and its report unclearable, so the startup warning
fired on it forever. That is the alert fatigue this feature exists to end,
reached from the other side.

Ids now travel hex-encoded behind a {hex} marker, and the encoded form is
canonical wherever an id is written down - the report AND the baseline
file. An id that survives sanitize() unchanged is written verbatim, which
is every ordinary id, so reports and existing baseline files are
byte-for-byte what they were and need no migration. Display-only text (a
git config value, a cross-project slug, the workspace path) still goes
through sanitize; only the id-bearing half of a git config finding is
encoded, so a value full of control bytes cannot drag its key into the
opaque form and hide which setting changed.

The encoded form is also the honest one. sanitize() DELETES those bytes,
so a planted ".git/hooks/pre<U+202E>commit" was displayed as
"precommit" - a name no file on disk has, sending the operator grepping
for nothing. Two tests asserted that stripped rendering; they now assert
the encoded one.

An id that already begins with the marker is encoded too, so decoding
stays unambiguous. No monitored path can reach that (they are all rooted
at .claude/.vscode/.git/.envrc/.mcp.json), but a git config section name
is free text in a file the session can write and the audit parses that
file directly rather than through git - covered by a test.

docs/security.md documents the marker and how to decode it.

audit: encode paths in the snapshot record too, closing the TAB route b82541af2c

The previous commit fixed ids on the way into the report, but the snapshot
record they are derived from is itself path<TAB>sha, read back with
`cut -f1`. A filename containing a TAB was therefore truncated at it
before any encoding ran: ".git/hooks/pre<TAB>commit" was reported as
".git/hooks/pre" - still CRITICAL, since the truncation still lands under
.git/hooks/, but naming a file that does not exist. Unackable, and nagging
at startup forever: issue #40's exact failure, reached through the record
format instead of through sanitize().

hash_paths and tcb_snapshot now write the encoded path, which by
construction contains neither TAB nor newline, and compare decodes it
before touching the filesystem, classifying severity, or matching a
baseline entry. The report body keeps the encoded form it already arrives
in. Ordinary paths encode to themselves, so the record is byte-identical
for them.

enc_id gains a subprocess-free fast path, because this puts it on a
per-file loop on both the snapshot and the compare side and a bare
`git init` alone ships ~14 sample hooks - unconditionally running the
exact check took the audit suite from 55s to over two minutes. The fast
path decides by pattern matching under `local LC_ALL=C` and is
deliberately conservative: anything that might need encoding falls through
to the exact check, so ordinary text carrying a \302 or \342 lead byte
(an em dash, say) costs a little time and changes nothing.

branderson referenced this pull request

2026-07-31 20:45:30 -07:00

A WARNING-tier git config finding cannot be acknowledged at all #45

branderson added 1 commit 2026-07-31 20:59:00 -07:00

Merge branch 'audit-alerting' into audit-followups

tests / fast (pull_request) Successful in 4m42s

Details

tests / runtime (pull_request) Successful in 4m23s

Details

260198d2bf

branderson merged commit 43b01cfb12 into audit-alerting

2026-07-31 21:08:54 -07:00