feat(backup): restore into live state, and redact what leaves the host - #2764
Conversation
github-actions
Bot
added
the
readiness: checking
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound seams and fail-closed ordering, but the redaction opt-in composes into "no off-host backup at all" on any realistic store, with no in-product remedy. Watch
Suggestions
[DESIGN-REVIEWED] 1655efe |
GPT 5.6 Review — ✅ human override acceptedHuman judgment by @chenmingwei23 overrides the GPT 5.6 finding for This comment is updated in place on each push. The model was not re-run because an authorized human decision supersedes it. False positive or not applicable? A repository writer can comment: |
github-actions
Bot
added
readiness: action required
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsBased on my independent re-derivation from the code: The candidate's core chain holds — I confirmed each leg by opening the code:
I discard the candidate's claim that sqlite3.connect "CREATES an empty database" and prints Classification: this is a hole in a new guard (incomplete restore of a declared component reported as success), triggered by a crafted-but-untrusted bundle. It is not live data loss (the live A crafted bundle that declares FINDING — snapshot.py:143 — [OPUS-REVIEWED] 1655efe Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
20e1ceb to
231ffa9
Compare
github-actions
Bot
added
readiness: checking
Dispositions — GPT 5.6 review of
|
github-actions
Bot
added
readiness: action required
231ffa9 to
181c09d
Compare
github-actions
Bot
added
readiness: checking
Dispositions — GPT review of
|
github-actions
Bot
added
readiness: action required
181c09d to
2665c97
Compare
github-actions
Bot
added
readiness: checking
Dispositions — GPT review of
|
github-actions
Bot
added
readiness: action required
2665c97 to
a9305d4
Compare
github-actions
Bot
added
readiness: checking
Dispositions — GPT review of
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
GPT round on Fixed: rollback followed a symlink planted at the destination
Now routed through This is the third instance in this branch of one shape: a guard present in one branch and Fixed: staging directories were not locked down before their children existedThree Worth naming because it also corrects something in this PR's own reasoning: an earlier draft Also fixed: the lockdown-before-publish gate
Rebutted, with the measurement: refusing on a byte-length changeThe finding is the documented offset-corruption gap: with redaction opted in, a The prescribed remedy -- "refuse rewriting when the encoded byte length changes" -- was A credential and its placeholder differ in length in every case the scanner matches. So the The correct remedy is the one already named as the follow-up: length-PRESERVING substitution. No override is posted on this SHA. Two of the three findings are fixed, so the head has Tests794 pass. The one local failure is an Two test injections needed re-scoping rather than the product changing: they stub Gates clean: subprocess-encoding, black, isort, flake8, mypy (1105 files), docs-lint, |
|
CI round on The unsafe-root refusal was validating against the wrong data home
A tree resolving to itself and still being rejected is the tell: the base it was compared This one only became reachable this round, because the merge path had previously SKIPPED an
|
|
GPT round on Fixed: a contentless FTS5 index rode the bundle unexaminedFTS5 storage tables ( Measured before writing the guard, and the interesting part is that the first attempt at the The first probe searched for the credential verbatim and found nothing, which looked like a The case folding is not a control: an Contentless definitions now REFUSE the upload rather than being scanned. Cleaning FTS5's This is the fifth instance in this branch of one shape -- a rule justified for ONE of a path's The offset finding: same finding, third different remedyRound A: "make substitutions byte-length preserving." Round B: "refuse rewriting when the The finding is real and is documented as a known gap. Each remedy has been answered on its
The gap is bounded and stated in the description: opt-in only, off by default, a file that No override on this SHA: the contentless-FTS fix moved the head, and an override suppresses Tests848 pass, including main's 53 in |
|
GPT round on Fixed: a pinned refusal mid-mutation skipped the rollback
This exposure was created earlier in this review, not inherited: routing recovery through Both handlers now name it -- the phase-two wrapper in The regression test was WRONG TWICE before it was right, and both mistakes are worth naming
Both were caught by mutation-testing the handler and seeing GREEN. The test now asserts Fixed: rollback-name exhaustion escaped the CLI boundary
One of main's tests needed adjusting, and why that is not a weakening
The test's stated point is the WIRING: proving The offset finding: remedy has cycledFour remedies across four rounds -- length-preserving, refuse-on-length-change, refuse-%PDF-, No override on this SHA -- two of three findings are fixed, so the head has moved, and an 849 tests pass, including main's 53. Gates clean: subprocess-encoding, black, isort, flake8, |
|
Fixed on What actually happens
Reproduced on Four files of the operator's own data gone, and the rollback printing "Previous state The recovery function's own docstring already specified the correct behaviour -- "Not saved, Seventh instance of the pattern this branch keeps producingA rule justified for ONE of a path's behaviours, then applied as if the path had only that Fix
Two regression tests, because a fix that merely stops the deletion could do it by breaking the
Both mutation-verified, each killed by a different mutant. Restoring the declared-set One ordering invariant relaxed, and why it is not weaker
851 tests pass. Gates clean: subprocess-encoding, black, isort, flake8, mypy (1105 files), |
|
Fixed on The override precondition was satisfied for the first time -- this was the SOLE blocking item The remedy was new, so it got a new answerFour remedies for this finding have been answered here already: length-preserving replacement, The database half was real and unansweredThe file path refuses a NUL-bearing file precisely because a variable-length edit shifts every The prefix still describes 46 bytes, the payload is 48, the trailing field is shifted -- and The existing comment on that branch is the giveaway, and it is the same pattern this branch The fix is not the prescribed one, and the difference mattersThe prescription was to refuse byte-valued fields. That over-refuses: sqlite stores plain So the test is the FILE path's own test, applied per value: text-shaped bytes (decodable UTF-8, A prior judgement of mine, reversed by measurementI dismissed refuse- Every other offset-dependent container is ALREADY refused by the NUL guard. The uncompressed Narrowed, not closed, and the body says so: a text-shaped format with internal offsets that is TestsFour new, each mutation-verified -- the blob refusal and the PDF refusal are killed by their Two existing tests changed, and one of them had encoded the defect: 856 tests pass. Gates clean: subprocess-encoding, black, isort, flake8, mypy (1105 files), Overrides used in this review: still zero. |
|
Fixed on A saved core-file symlink was skipped by the rollbackRecovery tested Same signature as the ledger finding two rounds ago: silent loss of the operator's own data, A link in the rollback directory is not an exotic state -- Ninth instance, and the code said so out loudThe directory branch carried this: " That is TRUE of trees and FALSE of core files, which a different function saves with the FixA saved link or junction is detected FIRST, ahead of both dereferencing tests, and the link An existing structural invariant needed rewriting, and the first two attempts were wrong
Attempt one pinned the gate to the exact string Attempt two accepted either spelling by looking for a link check within eight lines above. It The form that holds: for every Screenshot Evidence is not a failure
858 tests pass. Gates clean: subprocess-encoding, black, isort, flake8, mypy (1105 files), |
|
Fixed on The mkdir hunk was pure redundancy -- deleted
The sqlite hunk: the finding is right, the prescription is not achievable, and my first fixfor it was wrongThe prescription was "revert both by-name hunks until validation and I/O use the same pinned
What IS real in the finding, and what I fixed: The chain from the source root down to each file is now verified COMPONENT BY COMPONENT through Stated in the code rather than implied: this does NOT close a swap of the FINAL name between the My first attempt at that fix was wrong, and my own comment was the false claimI first pinned the parent via The test was vacuous first, for a reason worth recordingThe obvious test plants the symlink before the pass runs. It proves nothing: The claim is about a swap AFTER enumeration and screening, so the swap is now INJECTED from 860 tests pass. Gates clean: subprocess-encoding, black, isort, flake8, mypy (1105 files), |
|
On Fixed as prescribed: duplicate component names crashed creationReproduced: An uncaught traceback rather than a snapshot. Fixed, but NARROWER than prescribed: a trigger that DELETES rowsReproduced, and it is the worst shape again -- permanent loss reported as success: An This is the TENTH instance of the recurring shape. The fixpoint scan here is justified in its The prescription was to refuse every non-FTS UPDATE trigger. I did not do that, because it
I implemented the blanket form first and the suite said so immediately: it failed both existing So the refusal is scoped to UPDATE triggers whose body DELETEs, with FTS-maintenance triggers Residual, stated in the code rather than implied: this reads the trigger BODY for a DELETE, so Declined: a present
|
|
On Windows: the pinned chain check crashed the whole database pass
The gate is now in, and the degradation is stated rather than implied: where the platform The test simulates Windows, and had to simulate BOTH halvesPatching the capability report alone is not enough: on Linux One detail worth recording because it made the test error rather than fail: pytest's own The remaining red is not this diff
863 tests pass locally. Gates clean: subprocess-encoding, black, isort, flake8, mypy (1105 |
|
First Principles round 1 on Blocker: dead
|
|
GPT 5.6 round 1 on Finding 1 -- forced restore writing outside the data home: NOT REPRODUCIBLE as stated, but it found a real defect next to itBuilt a probe that does exactly the described chain. Getting it honest took three attempts, and the first two are worth naming because each was a way of proving nothing:
With the swap landing in the actual window, nothing was written outside the data home and both planted external files survived. The write is refused at use time, not merely at pre-flight: But the probe surfaced something worse in the same loop, and it is mine. The run reported so a root that failed the check was removed from the set entirely -- neither saved into the rollback copy nor restored -- and the run continued to success. An operator restoring after losing a machine would have believed memory came back when only the databases had.
Fixed: that site now refuses with a message saying the root stopped resolving inside the home AFTER the pre-flight passed, i.e. something moved mid-run. Safe to raise there because it runs before phase one, so no live state has been mutated. Pinned by a regression test that injects the swap mid-run and asserts its own injection fired; mutation-verified by restoring the So: the stated escape is rebutted, and the finding is credited with the defect it led to. Finding 2 -- refuse non-FTS UPDATE triggers: REBUTTED, measuredApplied the prescription to the current tree and ran the suite. It breaks three existing tests, and they are the ones that define what this branch does about value-writing triggers:
The prescription discards the fixpoint. A trigger that copies a pre-update value somewhere the scan has already been is CLEANED by the next pass today; refusing instead means the operator gets no backup at all where they currently get a correctly-scrubbed one. It also refuses the product's own external-content full-text index, whose maintenance is exactly such a trigger -- so the redaction option would refuse the ordinary case. The underlying concern is real and narrower than the prescription: a write-only trigger can touch a row unrelated to the one being redacted, so the outbound copy can differ from the original by more than the intended substitutions. Telling an unrelated write apart from a derived one needs the trigger's EFFECTS, not its text -- the same separate work this refusal already names as its residual for Unrelated to this diff
|
|
GPT round 3 on Merge installs malformed cron JSON -- FIXEDReal, and the reachable case is the one that matters most. That is true of the branch that MERGES and false of its sibling: An absent destination is the fresh-machine case -- the scenario an off-host backup exists for -- so the one path that skipped this check was the likeliest one to need it. Reproduced: a well-formed JSON ARRAY as The gate now asks "will this reach a consumer" rather than "is this a replace": One existing test had to change and it is stronger for it. Windows junction removal breaks rollback -- FIXEDReal, and the codebase already had the answer. Three sites detected with Worth noting for the record: a fifth site in the same function was ALREADY using the helper correctly. The correct form was known and had not been carried to its siblings, which is the same shape as two other findings on this PR (a guard added to the pinned path and not the by-name one; a pre-flight hoisted ahead of the mutation while one site kept the old skip). I have stopped treating that as a coincidence. VerificationLinux: 678 passed, 0 failed across the snapshot, redaction, staging, posture and lockdown suites. subprocess-encoding, black, isort, flake8, mypy (1127 files) and docs-lint all clean. Still open, and not from this round
|
|
GPT rounds 4 and 5. Round 4's merge finding was REAL and is fixed. Round 5 asks for two things I am not doing inside this PR, and I would rather say why in writing than patch a fourth round in the same span. Head Round 4 -- merge escaping through a destination root swapped after the pre-flight: FIXEDThis was right, and my earlier rebuttal of the same theme was right about REPLACE and incomplete about MERGE. Worth stating plainly: in round 2 I reproduced the replace path, found the escape closed, and treated that as a rebuttal of the finding rather than of one of its two paths. Reproduced on merge. The per-file screens cannot catch this, which is the part worth recording: every final component is a fresh regular file, and a by-name open does not examine its ancestors, so Fixed by giving merge the same use-time root re-check replace already had: a root that fails the check now failed AFTER the pre-flight cleared it, so it is refused with a message saying so. Verified: Round 5 finding 1 -- the residual mkdir race: ESCALATING, not patchingThe remaining window is real and narrower: my check runs immediately before Three measured facts about scope:
So the prescribed subtraction -- revert the memory-tree merge branch until writes are anchored to a data-home descriptor -- removes a guard the rest of the function does not have, leaves the identical Anchoring every destination operation in restore to an Round 5 finding 2 -- refuse value-writing UPDATE triggers: REBUTTED, measured twiceThis prescription has now been raised four times and measured twice, both times against the tree as it then stood. Refusing every non-FTS UPDATE trigger breaks three existing tests -- What DID come out of this theme is in the tree: the narrow half is now implemented. A write-only UPDATE trigger whose body contains I am not implementing the broad form. If the maintainer prefers it despite the measured cost, that is their call to make explicitly. StateLinux: 682 passed, 0 failed. subprocess-encoding, black, isort, flake8, mypy (1127 files), docs-lint all clean. |
|
Opus 4.8 round on Case-fragile FTS5 detection: TAKENReproduced exactly as described. The detector was Measured both spellings against the same payload: So a sound database lost its entire off-host backup to a spelling. Fixed by lowering once and testing against that, with the contentless probe lowered alongside it as the finding asked. Two things worth adding to the record. First, this interacted with my own change. The refusal surfaces from the pager's Second, the regression test pins the half a naive case fix would lose. Making detection case-insensitive is easy to get wrong in the direction of no longer recognising A Windows regression I caused, and how
The mechanism is worth stating plainly because it is not a typo. Every other call in that file carries the flag, because those were fixed by an oracle-driven pass over the whole surface. I wrote this test after that pass finished and verified it on Linux only -- the exact "verify BOTH platforms" step I had written into the instructions for that pass. A local green is not a platform green, and I had the tool to check and did not run it. The fix also had to reach the Verified on both platforms this time: Linux 684 passed, and the simulated unpinnable platform leaves only the six tests that carry an Not from this diff
|
|
GPT round on Generated columns bypassing redaction: FIXEDThe stated mechanism is exactly right. Step one would have said the finding is wrong. A STORED and a VIRTUAL generated column each carrying a credential were both refused, so nothing shipped. On that evidence the stated consequence -- the off-host database retains the credential -- does not hold. Step two was asking WHY it refused, because refused-by-design and refused-by-accident are different answers and only one of them closes a finding. The cause was Step three found the shape neither guard covers. Two guards stand between a generated column and an egress leak: the schema scan catches a credential written as a literal in the expression, and a STORED column is recomputed when its source is UPDATEd so redacting the source propagates. Neither covers a credential ASSEMBLED across columns:
Fixed with the prescribed remedy: read The scoping matters as much as the check. It is restricted to hidden flags 2 and 3 -- VIRTUAL and STORED generated -- and deliberately does not touch flag 1, which is what a virtual table's own columns present as. Sweeping flag 1 in would refuse every database with an FTS table, turning a hardening into an outage for exactly the backups this feature exists to take. Three tests pin that: the assembled credential refuses, a generated column holding nothing sensitive does NOT refuse, and an FTS table still redacts normally. Mutation-verified by disabling the guard, which fails on DID NOT RAISE. The write-only trigger findingRaised again, same broad prescription. Already dispositioned twice with measurements and escalated to the maintainer in the round-4/5 comment above; nothing has changed on my side since. The narrow half that IS implementable -- refusing a write-only UPDATE trigger whose body contains StateLinux 687 passed, 0 failed. All gates clean. On the simulated unpinnable platform only the six tests carrying an |
|
Both findings on BLOCKING -- generated BLOB credentials bypass the scan: FIXED
Fixed by scanning One detail worth recording, because the first probe of this would have produced the wrong conclusion. The regression test uses the CAST form and asserts BLOCKING -- generated-column scan can exhaust memory: FIXED
Fixed by iterating the cursor. No page window is needed here, unlike the row pass -- the question is all-or-nothing, so the loop stops at the first match and never holds more than one row. StateLinux 688 passed, 0 failed; all gates clean. The |
|
Both findings on BLOCKING -- stale FTS credentials bypass redaction: FIXEDCorrect, and the code already contained the argument against itself. The comment on the unconditional VACUUM says gating on An external-content FTS index keeps its own tokenized copy and does not auto-sync, so a base table can move on and leave the index holding text no live row contains. The scan then reports Reproduced: base row updated to drop the credential without syncing the index, no refusal, and the egress copy still answered One measurement detail, because the first attempt at it produced a FALSE clean. Checking the file bytes for the credential in its own casing reported nothing present -- FTS5's default tokenizer LOWERCASES terms, so the index copy is not stored in the credential's casing. The database was still fully searchable for the key. The regression test therefore asserts through a Fixed by rebuilding every identified index unconditionally, as prescribed. Safe without a new guard because a CONTENTLESS index -- the one shape with no content table to rebuild from -- is already refused before this point, so every name in the list has a source. Mutation-verified. The manifest's BLOCKING -- replace retains the previous memory index: FIXEDAlso correct, and the justification for the behaviour was mine and was wrong. The redaction pass DROPS Reproduced against The memory-TREE loop in the same function already states the rule -- "a tree the archive does not have is a tree the destination must not keep" -- and its comment records that clearing only when the archive had it once produced "restored memory mixed with stale notes" that still reported success. A derived index is a FILE and never got the same treatment. Fixed as prescribed: in replace mode a derived index the archive omits is moved into the rollback set and removed, so the absence is real and the existing warning fires. Two details follow the discipline established earlier in this PR -- the name enters the The set naming those indexes could not be imported from the redaction module -- it is loaded lazily to stay out of the boot path that StateLinux 693 passed, 0 failed. All gates clean. On the simulated unpinnable platform the same six |
|
Findings on BLOCKING -- UPDATE triggers can corrupt unrelated backup data: FIXEDThis is the sixth time the trigger theme has been raised, and the first time it has arrived with a consequence my earlier measurements do not answer. The previous rounds argued credential SURVIVAL, which the fixpoint handles. This one argues data LOSS: the redaction's own UPDATE fires a trigger that overwrites unrelated rows, and the copy that is uploaded -- and later restored -- carries altered data. Different claim, and it is correct. Reproduced: Fixed, but NOT with the prescribed rule, because I measured the prescription and two variants of it against every shape this suite pins:
The broad rule fails three of four; "any UPDATE in the body" fails the mirror column, which is an ordinary shape the fixpoint provably cleans. Keying on the TARGET -- an Stated residual, narrower than before: an BLOCKING -- SQL views bypass the credential scan: mechanism is WRONG, but there is a real leak beside itThe view is not the mechanism. Four cases, measuring whether the whole key is present in the FILE BYTES after a full pass:
B leaks identically to A, so removing views changes nothing; C does not leak, which identifies the actual cause. SQLite stores a row's cells CONTIGUOUSLY, so So the prescribed fix -- refuse databases containing views -- would refuse ordinary databases and would not close case B. I am not applying it. The leak itself is real and I am NOT claiming otherwise. What it is not is a property of this PR's new code: it is inherent to scanning VALUES, which is how any row-level redaction works. The only remedy I can see is to scan each row's concatenated cells and REFUSE on a match, and refusing cannot be judged from a probe -- it needs measuring against real operator databases first, because a false positive refuses a legitimate backup outright. An AWS-key-shaped match is a 20-character alphanumeric run, which two innocent adjacent values can produce. Escalating it for that reason, in the same position as the merge StateLinux 696 passed, 0 failed. All gates clean. |
|
Finding on The residual was real
The prescription is not the fix"Remove the same-table exclusion" refuses this instead: A mirror column. Ordinary shape, and the fixpoint provably cleans the propagated value -- So the target is the wrong axis to widen. The right one is whether the statement is BOUND to the row that fired it:
"References NEW or OLD" is the bound. It is a spelling rather than an effect, which is what makes it honest for text to decide -- the same standard already applied to Residual, statedA statement can reference NEW and still touch other rows deliberately -- StateLinux 697 passed, 0 failed. All gates clean. |
|
Findings on BLOCKING -- interrupt bypasses replace rollback: FIXEDCorrect, real, and the handler's own comment already contained the argument for it. That comment explains why Reproduced: a Ctrl-C injected after the memory component was replaced left Fixed as prescribed -- catch Two limits worth stating rather than implying. A second interrupt DURING the rollback cannot be defended against from here; the rollback directory is what answers for that. And when the rollback itself fails the original exception is still replaced by BLOCKING -- same-table trigger exemption: unchanged, and already escalatedSeventh raising of this theme, and the first with no change in either the prescription or the consequence from the previous round. "Remove this exemption so own-table UPDATE triggers are refused" is the same instruction I measured one push ago, and "overwrites sibling rows" is the The measurement has not moved: removing the exemption refuses a mirror column, which I am not applying it and I am not re-arguing it an eighth time. It needs a maintainer decision, which is where it already sits -- if the call is that refusing mirror columns is acceptable, say so and I will implement it with the fixpoint tests updated to match. Until then the residual stands as documented in the function's own docstring. StateLinux 698 passed, 0 failed. All gates clean. |
|
Findings on BLOCKING -- manifest-only memory bundles partially erase live memory: FIXEDCorrect, and it lands on a gap in MY reasoning. When I added the derived-index removal I wrote that a missing payload database "is a different question with a different answer (refuse, not delete), and it is answered elsewhere". It is not answered on this path -- and my change added a third thing that gets erased in this scenario, so it widened the damage rather than being merely adjacent to it. Two measurements decided how seriously to treat it. What replace leaves behind, reproduced: memory trees CLEARED (unconditionally, by the rule that a tree the archive lacks must not be kept), derived index REMOVED (my change), and Whether the product can produce such a bundle -- this is what moved it from defensive to live. It can: a snapshot of a home with no memory payload declares Fixed as prescribed: a DECLARED component the bundle carries no non-derived payload for is refused. The explicit-selection branch a few lines below already refused exactly this situation, with the rationale that replace "would move the live files of that component out to the rollback dir and have nothing to put back" -- the same question had two different answers and only one branch was guarded. A derived index does not count as payload, since a bundle carrying only The payload test derives what counts from One note on verification, because it changes what the evidence is worth. My first probe drove BLOCKING -- NEW/OLD does not prove a row-bounded update: unchanged, already escalatedEighth raising. The case it names -- which Not applying it, not re-arguing it a ninth time. It needs the maintainer call it is already waiting on -- if refusing mirror columns is acceptable, say so and I will implement it and update the fixpoint tests in the same change. StateLinux 700 passed, 0 failed. All gates clean. |
|
Correct, and it is the half I missed one push ago. Fixed on BLOCKING -- explicit selections trust hollow components: FIXEDThe previous fix guarded the manifest-derived branch and left the explicit-selection branch testing What makes this worth stating plainly: my own disposition for the previous round said "the same question had two different answers and only one branch was guarded", and then I fixed one branch with a payload test while leaving the other on the weaker membership test. Same defect, one layer down, authored by the fix for it. Reproduced, and the reproduction is more useful than the description. Driving it end to end showed Both branches now ask the same question: a component is absent if the bundle does not declare it OR carries no non-derived payload for it. StateLinux 701 passed, 0 failed. All gates clean. The trigger exemption remains where it was left -- eight raisings, awaiting the maintainer call, not re-argued here. |
|
Both findings on BLOCKING -- NUL-separated credentials bypass outbound redaction: FIXEDCorrect, and precisely diagnosed. UTF-16LE with no BOM decodes as valid UTF-8 -- NUL is a legal codepoint -- so The check that would have caught it already existed, one branch too deep. Inside Treating NUL-bearing decoded files as opaque, as prescribed, is wrong. Measured: it fails Shipped instead: when NULs are present, the UTF-16LE and UTF-16BE INTERPRETATIONS are scanned, and only a credential actually found there refuses. A hit cannot be rewritten -- the real encoding is unknown and a replacement of a different length shifts every byte after it -- so BLOCKING -- rejected archive links trigger live-state deletion: FIXED, at a different layerThe consequence is real: extraction PRINTS a warning and drops a rejected entry, then continues, so a bundle can reach the restore missing part of its payload while the manifest still declares it. In replace mode the memory trees are cleared unconditionally and nothing refills the dropped one. The prescribed fix -- clear Extraction is the only layer that knows the difference, so the refusal went there. And it is scoped: a rejection is recorded only when the entry lies inside a tree replace CLEARS ( One test needed updating and it is worth being explicit that no guarantee was weakened. StateLinux 704 passed, 0 failed. All gates clean. |
Shrunk to the two things the AWS Control app does not do: restoring a snapshot into live state (replace/merge, two-phase backup-before-remove, rollback ledger, saved-link reinstatement) and redacting the outbound copy before it is pushed. The destination, its hardening, the consent grant, the transport, session backups and the schedule now belong to that app, so snapshot_remote.py, backup_cli.py, the --to-s3 and --aws-profile flags and the s3:// fetch path are deleted here rather than shipped twice. prepare_redacted_copy is the destination-free seam the app calls before authorizing an upload, so a redaction that cannot complete stops the push instead of falling through to sending unredacted.
|
Findings on BLOCKING -- encoded credentials still bypass the scan: FIXEDBoth halves are right, and the UTF-32 half is the sharper one because it explains why the previous fix was not enough on its own: read as UTF-16LE, a UTF-32LE credential is STILL NUL-separated ( The column path had the identical hole. It decodes latin-1, which is lossless byte-to-codepoint and therefore PRESERVES the NUL spacing, so its zero-hit result was not evidence of a clean value either. Both now go through one shared detector over UTF-16LE/BE and UTF-32LE/BE, rather than two copies that can drift -- that drift is what produced this finding. A hit can only REFUSE, never rewrite: the real encoding is a guess, and a replacement of a different length shifts every byte after it. The file path returns the file to Measured on four shapes, with the discrimination halves pinned as tests:
The last two matter as much as the first two: refusing them would trade a credential leak for an outage on ordinary backups. Mutation-verified by dropping UTF-32 from the encoding list, which fails on DID NOT RAISE. BLOCKING -- forced restore retains an ancestor-symlink race: unchanged, already escalatedThis is the The race is real. Closing it properly means anchoring every destination write to a descriptor opened once, which is an architectural change to the by-name restore path, repo-wide rather than specific to this PR, and it belongs in its own change with its own tests. That is the maintainer decision it is already parked on. Not applying a revert that measurement says is a regression. StateLinux 707 passed, 0 failed. All gates clean. |
|
Findings on BLOCKING -- expression indexes bypass redaction: the index is a BYSTANDERAn expression index does materialise, so this was worth taking seriously -- materialisation is exactly what made the generated-column finding real while the view finding was not. Measured on three shapes, asking whether the whole key is in the FILE BYTES after a full pass:
A and B are identical, which settles it: the expression index adds NOTHING over the leak that is already open without it. SQLite stores a row's cells contiguously, so C is the other half of the answer, and it shows the ordinary path is sound: when the credential IS a value the scanner can see, the row rewrite plus SQLite's own transactional index maintenance plus the VACUUM removes it from the index pages too. So there is no stale-expression-index case to fix -- unlike external-content FTS, which is documented as not auto-syncing and which is why that one was a real defect. The prescribed remedy would therefore refuse ordinary databases that carry an expression index while leaving case B -- the same credential, same bytes, no index -- completely untouched. Not applying it. What IS real here remains what I escalated: a credential split across adjacent columns is invisible to any value-level scan, and the only remedy is to scan each row's concatenated cells and REFUSE on a match. That cannot be judged from a probe, because an AWS-key-shaped match is a 20-character alphanumeric run that two innocent adjacent values can produce, and a false positive refuses a legitimate backup outright. It needs measuring against real operator databases, which is a decision rather than a patch. BLOCKING -- NEW/OLD does not prove a row-bound update: ninth raisingSame prescription, same case ( Waiting on the maintainer call, unchanged. If refusing mirror columns is acceptable, say so and I will implement it and update the fixpoint tests in the same change. StateLinux 707 passed, 0 failed. All gates clean. Nothing was changed on this head, so the previous verification still stands. |
|
/ai-review override gpt 1655efe: Both measured; neither remedy applies. Expression index: a control with NO index leaks identically (SQLite stores row cells contiguously), so the index is a bystander; refusing them rejects ordinary DBs and leaves the same bytes exposed. The split-credential leak itself is escalated. Trigger: 9th raising; removing the exemption refuses UPDATE t SET mirror=NEW.body WHERE id=NEW.id, which test_a_plain_value_writing_trigger_is_still_cleaned_not_refused pins as required. |
Human judgment recorded@chenmingwei23 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
3 participants
1. What is the problem?
A Kiro Crew operator's memory, lessons, and session history live on one machine. Losing
that machine loses them. Two pieces are needed to make a backup actually useful, and
neither existed:
Restoring is not the same as downloading. Having an archive is not having your agent
back. Live state has to be replaced or merged with the gateway stopped, databases have to
be staged rather than copied over an open handle, saved symlinks have to be reinstated as
links, and a failure partway through must leave the machine as it was rather than half
restored. Nothing did that.
A backup that leaves the host carries every secret it holds. A snapshot is a faithful
copy, which is what makes it a good backup and a bad thing to put in object storage. The
AWS Control app now creates a private drive bucket and pushes to it, but a private bucket
still holds whatever was put into it, and a bundle restored elsewhere carries the original's
tokens verbatim. Nothing rewrote the bytes on their way out.
Scope note: this PR used to include the destination, its hardening, the consent grant, the
transport, session backups and the schedule. PR #5517 (AWS Control) shipped all of that, so
those parts were deleted here rather than merged twice.
2. Why this issue matters to the user
Without restore, a backup is a file you cannot use on the day you need it -- and the day you
need it is the day the machine is gone, which is the worst time to discover the archive only
half applies.
Without outbound redaction, turning on off-host backup silently ships your credentials. The
push path exists now, so this is live exposure rather than a hypothetical: whatever is in
the memory store leaves the machine on a nightly schedule.
3. How our fix solves it
Restore into live state.
kirocrew restoretakes a local archive and applies it with--mode replaceor--mode merge. Replace is two-phase: every declared tree is backed upinto a rollback directory BEFORE any is removed, so a failure on the third tree cannot leave
the first two gone. The rollback ledger records per file inside the copy loop rather than
from the declared set -- an earlier version recorded the intent, and a failure then printed
"Previous state restored." with an empty failure list while four config files were actually
deleted. Databases are only replaced when an existing regular file is there to replace, a
saved symlink is reinstated as a link before any branch that would dereference it, and
PinnedPathRefusalis caught in both rollback handlers because it subclassesException,not
OSError. Ans3://argument is refused rather than treated as a filename, pointingthe operator at the app that owns fetching.
Redact what leaves.
snapshot_redact.pyrewrites the outbound copy through the repo'scredential and exfiltration-URL scanners. The design constraint is that it must never
degrade into shipping unredacted: a pass that cannot complete raises, and the caller treats
that as a refusal to send. Content that cannot be proven safe is refused rather than
scrubbed-and-hoped: bytes that are not text-shaped, a database that fails its integrity
check, a text container that declares its own extents (
startxref,content-length:--one rule covering PDF, WARC, HTTP archives, MIME multipart and mbox rather than a per-format
list), and a database whose triggers keep reintroducing values the scan just removed.
Databases are rewritten value-by-value through SQL and then rebuilt, so no old value
survives in page slack. Row reads are paged, text is capped at 64 MiB before being read, and
the local archive is never touched -- it lives on the machine that already holds these
secrets, and rewriting it would damage the only copy that restores complete.
Wired into the push, in the order that makes it a control.
prepare_redacted_copyis adestination-free seam: it takes a finished tarball and returns a redacted copy, or
Nonewhen the operator has not opted in. The app's
run_snapshot_backupcalls it BEFOREauthorizing the upload and before handing bytes to the transport, so a redaction that raises
stops the push. Reversing those two steps would leave the guarantee intact on paper while
sending the secrets anyway, which is why the app module is registered as its own redaction
sink in the security posture rather than left implicit.
The switch is beyond the agent's reach. Redaction is opt-in via
<data home>/backup/redaction.json, and that directory is classified sensitive for readingas well as writing. Flipping it off is the attack; reading it tells an attacker whether the
store is currently being scrubbed. The DIRECTORY is classified, not just the leaf, because a
writable container is the same hole one level up -- replace it with a symlink and the
protected leaf resolves somewhere unprotected.
4. What tests we did
673 tests across the snapshot, redaction, staging and posture suites; all hard gates green
(subprocess-encoding, black, isort, flake8, mypy over 1127 files).
Findings are mutation-verified rather than assumed. Two worth naming because both were
mine:
_upload_bundlewas deleted.To prove that rewrite was not vacuous,
prepare_redacted_copywas mutated to hand backthe unredacted original: 16 of 21 tests turned red. They detect unredacted egress.
occurrences of the function name -- which counted its own
defline, so a threshold offour passed with three real callers. It now asserts the call is present inside each reader
by name, so a new reader that skips the bound has to be added here to stay green.
Two defects were found by verification rather than by the tests passing:
backup/redaction.json, which lives in the same directory. Six tests caught it. The entryis restored with a rationale describing its live occupant instead of the deleted one.
test_a_database_that_never_settles_is_refuseddid not fail, it HUNG, which is why itread as a suite timeout. The paged row reader advanced on
handle > lastand its commentasserted an UPDATE never changes the handle -- but the trigger under test INSERTS, so new
rows kept appearing above the cursor and a single pass never terminated. The fixpoint cap
counts passes, so it never got to fire. Each pass is now bounded by the maximum handle at
its start; rows that appear mid-pass belong to the next one, which reports them, so a
trigger that keeps reintroducing values is refused by the pass cap instead of hanging.
5. Any other suggestions on the work
No scope question after all -- I withdrew one. An earlier version of this description
offered to split
security.pyout, calling it +88/-68 of new shell/parameter scannerlogic. That was a misreading: those regexes exist on the merge base and the diff only
reformatted them. First Principles caught it. The file is now reverted to base with only
the one substantive change re-applied on top -- +15/-0, the
"backup"sensitive-path entrythat fences the redaction switch, which belongs in this PR. The
.github/black-baseline.txtline went with the reformat and that file is out of the diff entirely.
Still unverified end to end. The redaction hook and the restore path have not been
exercised against a real push. The transport is the app's now, so what remains to verify
live is this PR's two halves rather than bucket provisioning.
A gap the posture registry does not model. The app's backup module is an egress boundary
that DELEGATES redaction to a registered sink. The registry has a slot for "runs a scanner"
and an allowlist named for non-egress modules, but none for "egress boundary that delegates",
so the row states the delegation in prose. Worth a shape the registry can check.