v0.4.5: restructure redact(), add property tests, verify every Linux path on real hardware - #3
Merged
Merged
Conversation
…nments Found by attacking my own v0.4.4 change (round 3, my pass). The rule "a key preceded by '/' is a filename component, not an assignment target, so show it" was too broad: it also exempted genuine assignments whose key happened to follow a slash. Six shapes printed the secret in cleartext to the terminal, daily.log and --json, all regressions introduced in v0.4.4 and LIVE in the published tag: //registry.npmjs.org/_authToken=<secret> (a real .npmrc spelling) //npm.pkg.github.com/_password=<secret> https://host/api_key=<secret> /etc/foo/password=<secret> source /opt/x/secret=<secret> PATH=/usr/bin:/x/token=<secret> The exemption now requires a WHITESPACE separator, which is the shape it was written for (`NOPASSWD: /usr/bin/passwd backdoor2026` — a command basename, whose following token is an argument, not a value). An '=' or ':' after a '/'-preceded key is still an assignment and redacts unconditionally as before. The suite passed WITH the leak: it had no case where a '/' sits immediately before a credential key. Added 11 (each on bare/+/- markers), plus mutations Y1 (widen back to any separator) and Y2 (drop the exemption) — both caught. Suite 228 -> 239; 44/44 mutations cumulatively. Co-Authored-By: Claude <noreply@anthropic.com>
The root cause of nearly every redact() bug in this project's history was one
design choice: the matcher's value group was `(\S.*)$` — rest-of-line. From that
followed the "show exempts every later secret on the line" leaks, the "mask
swallows the rest of the attack" hides, and the recursive tail rescan added to
patch the first half, which became an unprivileged RecursionError kill switch.
The value is now a SINGLE token (or a quoted run, atomic), stopping at whitespace
or a shell separator. `re.sub` then continues scanning after each match, so every
assignment on a line is decided INDEPENDENTLY — no recursion, no depth cap, no
tail semantics. The decision table is written out in four numbered rules, each
with the leak or hidden-attack that motivates it. Auth-scheme values (the only
multi-token secrets that occur in practice, `Authorization: Token <t>`) moved to
the shape pass, which is what lets the assignment scan stay single-token.
Verified property: redact("a; b") == redact("a") + "; " + redact("b") over 2700
random compositions. That is precisely what the old design made impossible.
The property tests (tests/test_redact_properties.py, stdlib-only with fixed seeds
— no Hypothesis, so CI is unchanged and failures are replayable) immediately found
three real bugs the 239 example tests missed:
- redact("") raised IndexError: `line[:1] in "+-"` is True for the empty string.
- `-AuthorizedKeysFile .ssh/authorized_keys` was REDACTED while the `+` form was
shown: '-' is in the key char class, so the marker joined the key and turned the
SOFT Authorized* directive into a HARD `[_-]auth` match. Classification now
happens on a marker-free copy of the key; the output keeps the original.
- `CREDENTIAL /t_wvSrE+2I.23-VqI33q` printed in cleartext: the whitespace branch
still used the loose "starts with / ~ $" test. Now keyword/short/single-class
only, plus a strict full-string $VAR reference (which is deliberately NOT
honoured for assignments — `SSHPASS=$ecretPassw0rd` shipped as a leak once).
Properties: no-leak, no-hide, idempotence, totality (never raises on arbitrary
unicode/control input), bounded cost, marker preservation, pre-filter soundness,
no unbounded regex run, no rescan loop, and compositionality. 239 -> 415 tests;
6 structural mutations of the new design all caught; the 31-case adversarial
battery (every historical finding, both directions) passes with 0 failures.
Co-Authored-By: Claude <noreply@anthropic.com>
…-time stall Third adversarial round, against the code the second round produced. Every finding reproduced by execution and pinned to a revision as control. The restructure had already fixed the reviewer's worst case (a `--api-key=` inside a sudoers grant); these are what survived it. LEAKS - A token passed as argv to a credential-named script printed in cleartext: `*/5 * * * * /opt/bin/refresh_token <secret>`, `/etc/foo/api_key <secret>`. The path-component exemption is REMOVED. There is no structural difference between that and `/tmp/token_stealer.sh <host>`, so the tie is broken toward not leaking: only the immediately following token is masked, and the script path plus anything after it survive. The case the exemption was added for (`NOPASSWD: /usr/bin/passwd backdoor2026`) is unaffected — the sudoers rule consumes the command path as its value, so the account name is never scanned. - `_SUDO_TAG_RE` was case-insensitive, so a lowercase token posed as "a further tag" and exempted the rest of the line. Sudoers tags are uppercase by spec. - `_SUDO_CMD_RE`'s lookahead accepted `ALL=` and `ALL:`, so `NOPASSWD: ALL=<secret>` was treated as a command spec and exempted. DETECTION - The per-line 4096 cap in malicious_hits re-opened the hole blob_flags exists to close: a payload after 300KB of padding ON ONE LINE was never flagged, so past BLOB_MAX the finding lost RED and its "why" entirely. The scan now CHUNKS with a 512-byte overlap (> the longest bounded run) instead of truncating. - `SSH_ASKPASS=/evil` was masked: a single-segment absolute path is a real hijack shape. Allowed when its entropy is low; `SECRET_FILE=/hunter2Xyz9` still masks. PERFORMANCE - `curl[^\n|]*\|` and `wget[^\n|]*\|` were still unbounded — `[^\n|]*` is unbounded in exactly the way `.*` is, it merely excludes two characters — and so quadratic in LINE LENGTH: 8MB of 4096-column `curl ` lines cost 20.2s at SNAPSHOT time, before anything is saved, from one appended line. Each pattern now carries a required literal checked with str.find before the regex runs: 20183ms -> 13ms at 8MB, with detection parity verified on all 8 real payloads. Two of my own tests let PERF-1 through and are replaced: the linearity test used a single 600KB line, which the per-line cap truncated to nothing (it now uses multi-line 4096-column payloads), and the "no unbounded runs" test only checked for `.*`. In their place: every pattern must have a required literal, each literal must appear in its own pattern source, removing the literal from a payload must kill the match, and the chunk overlap must exceed the longest bounded run. Suite 415 -> 426; mutations R1-R7 (revert each fix) all caught, 57/57 cumulative. Co-Authored-By: Claude <noreply@anthropic.com>
…tches, 3 half-fixes Second half of round 3, against the guard machinery round 2 introduced. Every finding reproduced by execution with v0.4.3 and v0.4.4-r1 as controls. The worst is mine twice over: `recover_baselines`, added to stop a blind day from becoming its own baseline, recovered EPHEMERAL categories across a PRIVILEGE mismatch. A 3-day-old root-taken `listening` set produced 27 fabricated ORANGE findings and fired the notification — re-opening the precise phantom flood the capability guard exists to prevent, and bypassing the euid guard including the unstamped case it deliberately fails closed on. Recovery is now restricted to durable inventory (cls == "software", never PRIV_SENSITIVE_CATS), requires a matching and STAMPED privilege level, and may not reach forward of the requested baseline (with `--since 8d` it recovered a snapshot NEWER than the baseline, hiding a change inside the window while claiming it had compared). Also fixed: - a non-container `blob_flags` VALUE was an uncaught TypeError: no snapshot saved, same poisoned baseline re-read, dead every day. Fourth instance of that class in this release; the outer dict was guarded, the value was not. - the flag escalation was suppressible by poisoning the baseline: flags are pattern DESCRIPTIONS and `curl|sh` shares one with `wget|sh`, so a single benign decoy comment marked it present forever. Flags are now COUNTS; any increase escalates. - `curl[^\n|]*` / `wget[^\n|]*` were still unbounded, so >4KB of curl arguments pushed the `|` past every scan window and defeated the chunked scan. Bounded. - `run_checked` covered only HALF of _mac_brew (the cask list) and NONE of _linux_packages, so the phantom flood stayed fully reachable for casks and for every package on Linux. - CAT_TOOLS stamped `brew` on Linux, where that category is dpkg/rpm/pacman — so the guard was a no-op there: no LOST VISIBILITY when the package manager broke, and recovery could never fire for the most important inventory category. - a malicious LaunchAgent still reported `signature: Apple-signed` (that is the INTERPRETER's signature); now it names the interpreter and says the signature is not meaningful. The argv is scanned in full via malicious_hits — padding argv[0] past the old 4KB cap had evaded the scan entirely. - dead `skip` reassignment removed; a recovered category no longer emits both "comparison skipped" and "compared against", so the all-clear stops claiming something could not be compared when it was successfully recovered. A NameError I introduced while fixing the above (a note reading `recovered` before assignment) survived a green 454-test run because NOTHING drove cmd_diff with a skipped category. There are now CLI-level tests that do. Four more of my tests were theater, caught by the matrix: the unchecked-run scan matched only `run([`, the package-tool test asserted a tautology on macOS, and the blob_flags cases never put a non-int under a MATCHING description key. All four now discriminate. Suite 426 -> 454; mutations G1-G10 all caught, 67/67 cumulative. Co-Authored-By: Claude <noreply@anthropic.com>
The three deferred items, all pre-existing since v0.4.3 and all in the same crash-permanence class this release has now fixed five times: - `safe_load` checked that `created`/`epoch` were PRESENT, not their types. An int `created` reaches datetime.fromisoformat in render() and a non-str blob value reaches .splitlines() in build_findings — both raise out of an unisolated path, so no snapshot is saved and the same bad file is re-read tomorrow. Types are now validated and blob values filtered. - A snapshot claiming a FUTURE epoch is refused: it is a planted or clock-broken file, never a baseline. - Snapshots are ordered by FILENAME, and the state dir is user-writable, so a planted `99999999T999999-9999999999.json` sorted last and simply became the baseline — the attacker choosing what "unchanged" means. Names must now match the format we write, and the future-epoch check covers the all-nines case that is syntactically legal. Both defenses are needed; the test asserts the OUTCOME (which baseline resolve_baseline picks) rather than one layer. Also fixed two test fixtures that used unrealistic snapshot names (`-1.json`), which the name validation correctly rejected. Suite 454 -> 461; mutations H1-H4 caught, 71/71 cumulative. Co-Authored-By: Claude <noreply@anthropic.com>
Found on the real Linux box, not by CI. The fixture hardcoded root=False, but the suite runs as root on a VPS, so the recovery privilege gate added earlier in this release correctly refused the synthetic baseline and the test failed. The gate is right; the fixture was not portable. It now defaults to since.IS_ROOT. Co-Authored-By: Claude <noreply@anthropic.com>
Found on the real Linux box; neither CI nor my container check could see it. In the JOB_PATH rewrite I wrote `[ -d "$1" ] || return`, which returns status 1 when a PATH entry does not exist as a directory — /snap/bin on a box without snapd. That is a simple command in a for-loop body, so `set -euo pipefail` aborted install.sh immediately after the first snapshot: no prompt, no units, no daily job, exit 1, and no error message. Every `since` install on such a machine silently ended up with no monitoring. My earlier "verification" of JOB_PATH ran the loop as a standalone `bash -c` snippet where the trailing exit status was discarded, which is exactly why it looked fine. _add_dir now returns 0 on every path. Co-Authored-By: Claude <noreply@anthropic.com>
…oring A shell bug, but it aborted the installer under set -e right after the first snapshot on any box with a missing PATH entry — no prompt, no units, no daily job, no error message. That is a total loss of monitoring, so it gets a regression test: _add_dir is extracted from install.sh and run under `set -euo pipefail` against a missing dir, a present dir, a duplicate and a relative path, asserting exit 0; plus `bash -n` and a check for the exact `|| return` shape that caused it. Verified by mutation: reverting install.sh to `[ -d "$1" ] || return` fails it. Co-Authored-By: Claude <noreply@anthropic.com>
…ied on a real box Bumps to 0.4.5 with the full CHANGELOG entry for rounds 2 and 3 of the self-review (24 findings), the redact() restructure and its property tests, and the real-Linux verification that discharges the three items outstanding since v0.4.0/v0.4.4. Suite 228 -> 463 (287 example-based + 176 property); 72/72 mutations caught. Co-Authored-By: Claude <noreply@anthropic.com>
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.
Follows #2. 24 findings from two more adversarial rounds against v0.4.4, plus a restructure of the function that produced most of them, plus its first property tests. Every finding was reproduced by execution before being fixed and is pinned by a mutation-tested regression test: 72/72 mutations caught. Suite 228 → 463.
The restructure is the headline
redact()'s matcher took the value as(\S.*)$— rest of line — and nearly every leak and hidden attack in this project's history followed from that one choice:RecursionErrorfrom ~800 credential keys in one line → the digest died before saving a snapshot, so it recurred every day forever)The value is now a single token (or an atomic quoted run).
re.subcontinues after each match, so every assignment is decided independently — no recursion, no depth cap, no tail semantics. The property that proves it:Property tests found what 239 example tests missed
tests/test_redact_properties.py— stdlib-only with fixed seeds, so no new dependency and failures replay from the printed seed. Properties: no-leak, no-hide, idempotence, totality, bounded cost, marker-independence, pre-filter soundness, compositionality.They immediately found three real bugs, including
redact("")raisingIndexError—line[:1] in "+-"is true for the empty string. A crash on the most trivial input imaginable, in the most-audited function in the tool, after five review rounds.Fixed
Leaks:
//registry.npmjs.org/_authToken=<secret>andhttps://host/api_key=<secret>(a/-preceded key exempted real assignments) · a token passed as argv to a credential-named script ·;-chained secrets after a shown path ·/- and$-leading values under*_FILE/*_PATHkeys ·sshpass -p,mysql -u root -p,https://<token>@github.com,MYSQL_PWD=· two sudoers exemption bypasses.Hidden attacks:
SSH_AUTH_SOCK/*_ASKPASS/PGPASSFILEhijacks ·PasswordAuthentication=yes· sudoers command specs including the account being reset · a payload pastBLOB_MAXlost its RED escalation, and the escalation was suppressible by planting one benign decoy comment (flags are now counts) · a malicious LaunchAgent reportedsignature: Apple-signed— that is the interpreter's signature, and paddingargv[0]evaded the scan.Availability: two kill switches ·
curl[^\n|]*\|still unbounded and quadratic in line length (20.2 s → 13 ms on 8 MB, at snapshot time) · a planted snapshot filename became the baseline ·safe_loadvalidated presence but not types.The capability guard's own bugs:
recover_baselines, added in 0.4.4, recovered ephemeral categories across a privilege mismatch — 27 fabricated ORANGE findings that fired the notification, re-opening the exact flood the guard exists to prevent.run_checkedcovered half of_mac_brewand none of_linux_packages;CAT_TOOLSstampedbrewon Linux where the category is dpkg/rpm/pacman, making the guard a no-op there.install.shsilently disabled all monitoring:[ -d "$1" ] || returnreturns 1 for aPATHentry that doesn't exist (/snap/binwithout snapd), andset -euo pipefailaborted the installer right after the first snapshot — no prompt, no units, no daily job, no error message.Verified on real hardware, not just CI
CI's ubuntu runners run the suite; they cannot run
install.sh, systemd timers ordpkg. On a real Ubuntu 24.04 box (systemd 255):--usertimerenabled/active, next fire 09:00), and runs —Result=success— with its pinnedEnvironment=PATH=_systemctl_execstartsystemctl showoutput; a genuine drop-inExecStartoverride changes the fingerprint (then restored)(system)tags, service ExecStart hashesLOST VISIBILITYdpkg-querybreaks, with the right label--guard correct on the real boxTwo of the bugs above (
install.sh, and a non-portable test fixture that assumed a non-root runner) were only findable there. The box was left clean — units removed, timer gone, test clones and state deleted.Upgrade note
Schema 5. Snapshots predating v0.4.4 carry no tool-identity stamp, so
brew/npm/pipcomparisons are skipped once, with a note, until a new baseline exists.🤖 Generated with Claude Code