fix(integrity): do not treat a rename-window NotFound as an empty quarantine set (SBS-871) - #774
Conversation
…rantine set (SBS-871)
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 25 minutes Limit details: You’ve used all 1 included review currently available under your plan. You completed 92 included PR reviews in the past 7 days; at that activity level, included reviews refill at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
Comment |
Automated reviewFound 6 issues:
Also noted:
For coding agents: fix BLOCK and FIX IF QUICK findings now; everything else is tracked or informational; never exceed one CodeRev fix round per PR. Advisory. Findings generated by |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| self.policy.quarantined = quarantined; | ||
| // A successful reconcile/rebuild installs a known set, so lift the | ||
| // cold-start fail-closed hide (SBS-871). | ||
| self.policy.fail_closed_catalog = false; |
There was a problem hiding this comment.
requarantine lifts fail-closed on store-error paths · disposition: block · confidence: high · severity: high · quick win
After a FailClosedCatalog router is published, watch_tick rebuilds, ${ROOT} rebuilds, and downstream tools/list_changed all call requarantine_if_needed. fail-closed makes aggregated_tools empty, so maybe_check_integrity runs on an empty list; apply_quarantine then hits the still-unreadable or corrupt store and fail_closed_integrity_catalog calls requarantine with that empty union. This assignment sets fail_closed_catalog=false and rebuild_aggregation exposes every connected tool.
Prompt for AI agents
Clear fail_closed_catalog only at successful-store call sites (reconcile_to, UseSet, requarantine_after_integrity_change Ok). Leave it set on fail_closed_integrity_catalog and the persisted-Err arm. Test: fail-closed router plus fail_closed_integrity_catalog with empty pending and an unreadable store still has catalog_fail_closed() and an empty aggregated_tools(). Verify against the current code first; if no longer valid, skip with a brief reason. Keep the change minimal.
CodeRev · advisory
| if path.exists() { | ||
| return; | ||
| } | ||
| if matches!(load_pins(profile), PinsLoad::Fresh) { |
There was a problem hiding this comment.
ensure_quarantine_store writes {} when pins are Corrupt · disposition: block · confidence: high · severity: high · quick win
ensure_quarantine_store_for_existing_pins returns early only for PinsLoad::Fresh, so a tampered or unreadable pin file takes the heal and atomic_writes "{}" (integrity.rs:973). The next quarantined() or mandatory_quarantined() call then returns Ok(empty), so build_router never sets fail_closed_catalog. mandatory_quarantined_checked later returns Err and the watcher keeps that empty live set, so tools stay callable until something runs check_staged on a non-empty catalog.
Prompt for AI agents
Skip materializing {} unless load_pins is PinsLoad::Loaded. Do not invent an empty quarantine file beside a Corrupt trust root; leave the file missing so missing_quarantine_store stays Err and bootstrap fail-closes. Test: corrupt pins, no quarantine file, build/read path is Err or fail-closed, and no quarantine.json is created. Verify against the current code first; if no longer valid, skip with a brief reason. Keep the change minimal.
CodeRev · advisory
| } | ||
| let (quarantined, fail_closed_catalog) = match bootstrap { | ||
| QuarantineBootstrap::UseSet(set) | QuarantineBootstrap::KeepSet(set) => (set, false), | ||
| QuarantineBootstrap::KeepFailClosed | QuarantineBootstrap::FailClosedCatalog => { |
There was a problem hiding this comment.
Successful empty store read cannot lift fail-closed · disposition: fix-if-quick · confidence: high · severity: medium · quick win
KeepFailClosed and FailClosedCatalog install quarantined=empty with fail_closed_catalog=true. reconcile_to only calls requarantine when guard.quarantined() != want, so a later successful read of {} (want empty) is a no-op and the hide stays up. A process restart recovers because build_router then takes UseSet(empty); a running gateway does not.
Prompt for AI agents
Treat fail_closed_catalog as part of the reconcile comparison, or lift it in reconcile_to whenever the store read is Ok. Test: fail-closed router plus reconcile_to(empty) clears catalog_fail_closed() and re-exposes tools. Verify against the current code first; if no longer valid, skip with a brief reason. Keep the change minimal.
CodeRev · advisory
| fail_closed: true, | ||
| }); | ||
| } | ||
| if router.server_count() == 0 && router.quarantined().is_empty() { |
There was a problem hiding this comment.
Live empty 0-server router is treated as no prior quarantine · disposition: fix-if-quick · confidence: high · severity: medium · quick win
prior_quarantine_from_router returns None when server_count()==0 and quarantined is empty. That is also a live router whose connects failed or whose profile has no servers, not only Router::new(). process_request self-heal then passes None into build_router, so a store Err becomes FailClosedCatalog instead of KeepSet(empty). Combined with the empty-vs-empty reconcile hole, that hide can stick until restart.
Prompt for AI agents
Pass previous_quarantine=None only from the genuine cold-start call in main; treat any already-published router as a prior decision. Test: live 0-server router with a successful empty set, then a store-Err rebuild, stays KeepSet(empty) rather than FailClosedCatalog. Verify against the current code first; if no longer valid, skip with a brief reason. Keep the change minimal.
CodeRev · advisory
…e-rename-retry # Conflicts: # CHANGELOG.md
…BS-871) Review round on #774. The fail-closed hide could be created from a tampered trust root, and could be lifted by paths that never read the store. - ensure_quarantine_store_for_existing_pins only materializes `{}` when the pin store is Loaded. A Corrupt pin store is a destroyed trust root, and inventing an empty quarantine file beside it handed back "nothing is blocked" on exactly the input an attacker controls. The file now stays missing, so the read is Err and the caller fails closed. - Router::requarantine no longer clears fail_closed_catalog. Every caller on a store-error path (integrity write failure, unreadable persisted set) used to re-expose the whole catalog while the store was still unreadable. Lifting moved to the new requarantine_from_store, used only where the set came from a successful read. - reconcile_to compares the fail-closed flag as well as the set, so a successful read of an empty store lifts the hide instead of being a no-op (empty vs empty) that left a running gateway dark until restart. This is now the single lift point. - KeepFailClosed carries the previous set forward, so real blocks are not dropped when the hide later lifts. - prior_quarantine_from_router keys off a new Router::is_built rather than "zero servers and empty set", so a live router whose connects all failed is treated as a prior decision, not a cold start. - A fail-closed build now clears the in-memory and on-disk tool cache. aggregated_tools() is empty under the hide, but every publish path treats an empty build as transient and keeps the last-good cache, so tools/list kept serving the catalog the hide was meant to remove. Tests: corrupt pins must not write an empty store, fail-closed must not lift while the store is unreadable, and a successful read must lift it. All three fail against the previous code.
…e-rename-retry # Conflicts: # CHANGELOG.md
…e-rename-retry # Conflicts: # CHANGELOG.md
…e-rename-retry # Conflicts: # CHANGELOG.md
…e-rename-retry # Conflicts: # CHANGELOG.md
1 participant
Linear: https://linear.app/southboundsoftware/issue/SBS-871/quarantine-store-a-transient-notfound-during-atomic-writes-rename
What a user who hits this now sees
Blocked tools stay blocked across a rewrite tick. A cold-start unreadable store does not expose the catalog. The gateway no longer logs "starting with no quarantine set" when it kept a live set or fail-closed.
The bug
Third arm of a class already closed: SBS-320 (corrupt/unreadable) and SBS-654 (present-but-empty) are Done. A missing
quarantine.jsonduringatomic_write's rename still returned success-empty. The pin and registry stores already retry this transient; quarantine did not.effective_quarantinealready returnsNoneonErr(keeps the live set). That path is untouched. The hole was the cold-start / rebuild path inbuild_router, plus the two NotFound arms that collapsed a vanished file toOk(empty).What changed
NotFoundand other transient IO errors with the same budget asread_pins_at(5 attempts, 40ms). Cite SBS-871.Freshfor that profile (real first run). OtherwiseErr. Same first-run marker shape as the SBS-715 guard.build_routerno longerDefault::default()onErr.build_router(it used to be captured after the build, too late) and passed in.Errwith a prior live set: keep that set.Err: hide the whole catalog (fail_closed_catalog) until a later successful readrequarantines a known set.quarantine.json(baseline written, nothing ever blocked).ensure_quarantine_store_for_existing_pinsmaterializes{}under the store lock before a rebuild read so that shape does not hide the catalog on every boot, and so a later missing-while-pins-exist read is a real rename-window error. First persist after pins exist still works:apply_quarantinestarts empty on the specific "absent, not Fresh" error after retries while holding the lock.Files changed
src-tauri/src/integrity.rs— retry, Fresh vs Loaded/Corrupt, testssrc-tauri/src/bin/toolport-gateway.rs—quarantine_bootstrap, prior-set parameter on everybuild_routercall sitesrc-tauri/src/router.rs—fail_closed_catalogpolicy flagsrc-tauri/tests/quarantine_empty_truncation.rs— comment only: missing is empty when pins are FreshCHANGELOG.md— Unreleased FixedSweep (exclude
targetanddocs/audit)integrity.rsload_quarantineNotFound →Ok(empty)integrity.rsquarantined_sets_checked_atmetadata NotFound →Ok(empty)integrity.rsquarantined_sets_checked_atread NotFound "treat as empty"toolport-gateway.rsbuild_routerErr→Default::default()+ "starting with no quarantine set"effective_quarantineErr→None(keep live set)reconcile_quarantineNone→ noreconcile_toquarantine_listErr→ emptyVecquarantined/ the router. Logged as unavailableall_quarantined/all_quarantined_namesskip unreadable/missing fileslist_quarantinedall_quarantined; same UI-only gapparse_quarantine_rawempty/corrupt (SBS-654 / SBS-320)read_pins_atretryrgfortreat as empty,starting with no quarantine,ErrorKind::NotFound,Default::defaultnear quarantin,load_quarantine,quarantined_sets_checked_at: remaining NotFound hits are unrelated stores (audit, secrets, autostart, rate limits, etc.). Remainingload_quarantinecallers either propagateErr(release,accept_quarantined_pins) or are the apply write path documented above.Fail-without-fix (rule 6)
Reverted only
missing_quarantine_store's Fresh vs Loaded/Corrupt arm to the old "missing = Ok empty" (kept tests and the SBS-715 guard). New tests that must fail without the production change:Restored the production arm; the same tests then passed.
CI results (this worktree, rustc 1.97.1)
Required job Build + test:
npm run format:check— All matched files use Prettier code stylenpm run lint— 0 errors (49 pre-existing warnings insrc/)npm run build— passednpm run test— 37 files, 382 passednpm run test:rust(cargo test --lib --bins --tests, default features) — lib 1090 passed, gateway bin 311 passed, integration tests passed includingquarantine_empty_truncationNew tests that actually ran (default features):
integrity::tests::sbs871_missing_quarantine_with_fresh_pins_is_empty_first_runintegrity::tests::sbs871_missing_quarantine_with_loaded_pins_is_err_not_emptyintegrity::tests::sbs871_quarantine_notfound_after_metadata_is_retried_then_err_when_pins_loadedintegrity::tests::sbs871_quarantine_notfound_after_metadata_recovers_on_retryrouter::tests::sbs871_fail_closed_catalog_hides_every_tool_until_requarantinetests::sbs871_quarantine_bootstrap_keeps_prior_set_on_store_errtests::sbs871_quarantine_bootstrap_fail_closes_catalog_on_cold_start_errtests::sbs871_quarantine_bootstrap_keeps_fail_closed_across_rebuildtests::sbs871_quarantine_bootstrap_uses_the_store_on_oktests::sbs871_prior_quarantine_from_placeholder_router_is_noneExisting SBS-320 / SBS-654 / SBS-715 tests still passed (not weakened):
corrupt_quarantine_store_fails_closed_and_is_not_renamed_asideempty_quarantine_file_must_not_silently_unblocka_missing_quarantine_file_is_still_an_empty_set(pins Fresh; comment updated)quarantine_reads_fail_closed_when_a_legacy_file_was_not_migratedAlso run (not the required job, requested locally):
cargo clippy --no-default-features --lib --bins— finished; new code warning-clean. Pre-existing main warnings remain (e.g.clippy::int_plus_oneinintegrity.rs:2325, unused items in the gateway).build_routeralready hadtoo_many_argumentsat 8 params; allowed on the fn after adding the prior-set argument.cargo test --no-default-features --lib --bins --tests— lib 1011 passed (desktop tests not compiled), gateway 311 passed, same integration tests. Allsbs871_*names ran.Desktop WebKit did compile on this runner (default-features lib tests include
desktop.rs). No tauri bundle.Rule 9 — what this makes more likely
Err(and slower on honest first-run missing, same budget as pins).Okempty only when pins areFresh.build_routernow fail-closes, so a corrupt store on first boot hides tools until readable.ensure_quarantine_store_for_existing_pinswrites{}under the lock when pins already exist and the file is lastingly gone. After retries that is "no file", not a rename window. An attacker who deleted the store still gets an empty durable set on the next rebuild; the live process keeps its set viaeffective_quarantineuntil then. Without this write, an existing install with pins and no quarantine file would hide the catalog forever (check sees an empty aggregated list, apply never creates the file).apply_quarantinestarting empty on the specific absent-not-Fresh error can persist only the new blocks if the file is lastingly gone. It holds the lock and has already retried.Adjacent arms left alone
SBS-320 (corrupt), SBS-654 (present-but-empty), and SBS-715 (unmigrated legacy) are distinct and untouched. Pins Fresh/Corrupt/Loaded plus
read_pins_atretry are unchanged.Gaps
all_quarantined/ desktop UI still skip a missing file rather than surface "unreadable"; enforcement is the router.npm run smoke:headless(required job step after gateway build). Did not run the Windows/macOS matrix.NOT MERGED.
Note
Fix quarantine store read to avoid treating a transient rename-window NotFound as an empty set
NotFoundand IO races, mirroring pin store read behavior.Routergains afail_closed_catalogflag; all tools are blocked with an explicit reason until a successful store read lifts it viarequarantine_from_store.🖇️ Linked Issues
Fixes SBS-871 — transient missing quarantine store no longer installs an empty block set.
Macroscope summarized 550ca3c.