From 629b84b0914e26f23fe0a447d52e853c436871e9 Mon Sep 17 00:00:00 2001 From: Martin Pluskal Date: Tue, 11 Aug 2026 23:41:23 +0200 Subject: [PATCH] fix(audit): stop under-reporting suppressed_count as a maximum MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The field took the larger of two tallies — the withheld bug ids and the id-less suppressed content — rather than their total. A summarize_bug call that dropped three private comments while hiding two duplicate marker ids recorded 3 beside a two-element id list, where the truth was 5. It under-reported silently: the record validated and looked complete. That matters because I3 forbids the client learning anything was withheld, so the audit stream is the only place the fact surfaces, and suppressed_count is authoritative by design — suppressed_ids sits behind a config switch, so with ids off the count is all an operator has. A maximum of two disjoint populations is not a count of anything, and cannot satisfy the "authoritative, never infer it from the id list" contract the field already documented. Sum them. The populations are disjoint by construction, not merely by category: the two tools that feed both tallies, bug_comments and summarize_bug, harvest their duplicate-marker ids from the comments that survived the private filter, so a dropped comment contributes its id to nothing even when it carries one. list_attachments, the third id-less site, names no ids at all. Comment at both harvest sites, since that is where the mistake would be made — the I14 rationale beside them is an active invitation to move the harvest pre-filter, which would silently turn this under-report into an over-report. Keep SCHEMA_VERSION at 1. No field is added, removed, renamed or retyped, and a bump would fork the reader for a structurally identical record. #68 asked for the version to move with any semantics change; it deliberately does not, and DESIGN.md records the cost that buys — a v: 1 corpus spanning this change carries both readings, with the deployed build and not the schema version as the boundary. Closes #68 --- README.md | 2 +- crates/bugwarden/src/audit.rs | 61 ++++++-- crates/bugwarden/src/server.rs | 8 + crates/bugwarden/tests/audit_wiremock.rs | 186 +++++++++++++++++++++++ docs/DESIGN.md | 67 +++++++- 5 files changed, 312 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index acb0a2e..9ea1d29 100644 --- a/README.md +++ b/README.md @@ -619,7 +619,7 @@ consult the guard — a `guard` object: | `verdict` | `served`, `served_filtered`, `denied` or `refused`; the worst verdict of the call wins | | `rule` | What decided a per-bug assessment. Alongside the policy's own rule names the guard reports its own: `default` when no rule matched — a default-decided call records that literal, never an absent field — `min_bug_age_days` for the age quarantine, `:unreadable-metadata` for a *granting* rule whose verdict hinged on metadata that could not be read (an undecidable deny rule keeps its plain name, having denied for its own reason), `unavailable` for a bug the classification fetch could not reach. Nothing stops you naming one of your own rules `default`, so this field says what decided, not what kind of thing it was. Absent only where no single rule decided: a refusal, the pre-dispatch gate, a search, either arm of the create gate, an id the guard could not assess, and a withheld attachment. A tool removed from the router (read-only mode, `disabled_tools`, discovery off) records no `guard` object at all | | `policy_hash` | `sha256:` over the raw policy file bytes, so a record says which policy text judged the call. Absent when no policy file is loaded | -| `suppressed_count` | How much the response withheld: bug ids on a search or a multi-bug read, but also private comments and attachment metadata the private-content gate removed. It is the larger of the two tallies, not their sum, and it is the authoritative number — never infer a count from the id list | +| `suppressed_count` | How much the response withheld, in total: bug ids on a search or a multi-bug read, plus the private comments and attachment metadata the private-content gate removed. The two never overlap — a call reads its bug ids off the content that survived filtering — so the field is their sum, and it is the authoritative number: never infer a count from the id list, which names bugs only and can be switched off entirely. Two ids under a count of five means three withheld items had no bug id of their own | | `suppressed_ids` | The withheld bug ids, subject to the `suppressed_ids` switch | | `redacted_fields` | Names of what a response redacted, never values — `summary_view` for a bug served through the redacted summary view | | `scan` | Present on every served search, carrying how many rows the window scanned and how many it dropped, so `dropped: 0` is a statement and not an omission; absent on a search that failed and on tools that scan nothing. The counts are recorded whatever the `suppressed_ids` switch says: counts are not ids | diff --git a/crates/bugwarden/src/audit.rs b/crates/bugwarden/src/audit.rs index fac4150..50ec4b7 100644 --- a/crates/bugwarden/src/audit.rs +++ b/crates/bugwarden/src/audit.rs @@ -75,6 +75,14 @@ use serde::{Deserialize, Serialize}; use sha2::{Digest as _, Sha256}; /// The audit record schema version stamped into every event (`v`). +/// +/// It tracks the STRUCTURE a reader must handle — fields, their names, +/// their types — and not the meaning of what those fields hold. A field +/// whose definition is corrected without changing the wire shape keeps +/// `v`, so a corpus at one version can span both readings and the +/// deployed build, not this number, is the boundary between them (see +/// `guard.suppressed_count` in DESIGN.md, issue #68). Re-encoding the +/// stream so that meaning changes ARE visible is #34's business. pub const SCHEMA_VERSION: u32 = 1; /// How often a persistently failing sink repeats its `tracing` diagnostic. @@ -522,10 +530,26 @@ pub struct GuardInfo { /// the exact policy that produced it. #[serde(skip_serializing_if = "Option::is_none")] pub policy_hash: Option, - /// How many bugs the guard removed from the response. This count is - /// authoritative: [`GuardInfo::suppressed_ids`] may be elided - /// (empty) or a subset by configuration, so consumers must never - /// infer the count from the id list. + /// How much the guard withheld from the response, in total: the + /// withheld bug ids PLUS the suppressed content that carries no bug + /// id of its own — private comments and private attachment metadata + /// the I5 gate removed. Not a count of bugs, then, and not a count of + /// either tally alone. + /// + /// The two populations are disjoint by construction, so the sum can + /// never double-count. The two tools that feed BOTH tallies — + /// `bug_comments` and `summarize_bug` — derive their duplicate-marker + /// ids from the comments that SURVIVED the private filter, so a + /// dropped comment contributes its id to nothing even when it carries + /// one; `list_attachments`, the third id-less site, names no ids at + /// all. + /// + /// This count is authoritative — it is the only tally that always + /// ships. [`GuardInfo::suppressed_ids`] carries bug ids alone, and + /// may be elided entirely by [`AuditConfig::suppressed_ids`], so + /// consumers must never infer the count from the id list: two ids + /// under a count of five says three withheld items had no bug id, not + /// that the record contradicts itself. pub suppressed_count: u64, /// The removed bug ids. Left empty by the recording call sites when /// [`AuditConfig::suppressed_ids`] is `false`. @@ -1219,6 +1243,11 @@ impl AuditCell { /// Note `n` suppressions that carry no bug id (private comments or /// attachment metadata filtered out). Adds across notes; upgrades the /// verdict exactly as [`AuditCell::note_suppressed`] does. + /// + /// This tally and [`AuditCell::note_suppressed`]'s id set must stay + /// disjoint populations: [`GuardInfo::suppressed_count`] SUMS them, + /// so a call site that counted an item here and also named it there + /// would count it twice. pub fn note_suppressed_count(&self, n: u64) { if n == 0 { return; @@ -1265,8 +1294,9 @@ impl AuditCell { /// verdict was ever noted — the tool performed no guard assessment. /// When `suppressed_ids_cfg` is `false` the ids are dropped and only /// the count ships ([`AuditConfig::suppressed_ids`]). The count is - /// the larger of the id set and the id-less counter, so it can only - /// ever under-name, never under-count. + /// the id set PLUS the id-less counter (issue #68): the two are + /// disjoint populations, so their sum is the total withheld and + /// nothing is counted twice. pub fn into_guard_info( &self, policy_hash: Option<&str>, @@ -1274,7 +1304,8 @@ impl AuditCell { ) -> Option { let state = std::mem::take(&mut *self.lock()); let verdict = state.verdict?; - let suppressed_count = (state.suppressed_ids.len() as u64).max(state.suppressed_extra); + let suppressed_count = + (state.suppressed_ids.len() as u64).saturating_add(state.suppressed_extra); Some(GuardInfo { verdict, rule: state.rule, @@ -2264,13 +2295,19 @@ mod tests { } #[test] - fn cell_suppressed_count_is_max_of_ids_and_counter() { + fn cell_suppressed_count_sums_ids_and_counter() { + // Issue #68: the two tallies are disjoint populations, so the + // count is their TOTAL — not the larger of them, which would + // silently under-report whenever both are non-empty. let cell = AuditCell::default(); cell.note_suppressed([7]); cell.note_suppressed_count(2); cell.note_suppressed_count(1); let info = cell.into_guard_info(None, true).unwrap(); - assert_eq!(info.suppressed_count, 3, "id-less counter adds up"); + assert_eq!( + info.suppressed_count, 4, + "one named id plus three id-less suppressions" + ); assert_eq!(info.suppressed_ids, vec![7]); } @@ -2278,9 +2315,13 @@ mod tests { fn cell_suppressed_ids_config_ships_the_count_only() { let cell = AuditCell::default(); cell.note_suppressed([40, 41]); + cell.note_suppressed_count(3); let info = cell.into_guard_info(None, false).unwrap(); assert!(info.suppressed_ids.is_empty(), "ids elided by config"); - assert_eq!(info.suppressed_count, 2, "the count still ships"); + assert_eq!( + info.suppressed_count, 5, + "the elided ids still count, and the id-less ones add to them" + ); } #[test] diff --git a/crates/bugwarden/src/server.rs b/crates/bugwarden/src/server.rs index 9244726..a509c92 100644 --- a/crates/bugwarden/src/server.rs +++ b/crates/bugwarden/src/server.rs @@ -1878,6 +1878,10 @@ impl BugWarden { // Bugzilla writes "*** Bug N has been marked as a duplicate // of this bug ***" itself, so a hidden bug can name itself in // the comments of one the client may read (I2). + // LOAD-BEARING: harvest from the FILTERED list, never the raw + // one. A dropped private comment is already counted id-less + // below; naming its marker id too would put one comment in + // both tallies, and guard.suppressed_count sums them (#68). let named = Guard::duplicate_marker_ids(&filtered); let disclosable = self .guard @@ -3108,6 +3112,10 @@ impl BugWarden { let comments = self.guard.filter_comments(comments, false); // Same scrub as bug_comments: otherwise a client that would be // scrubbed there just asks for a summary instead (I14). + // LOAD-BEARING, as in bug_comments: `comments` is already the + // FILTERED list here. Harvesting marker ids from the pre-filter one + // would count a dropped private comment in both tallies, and + // guard.suppressed_count sums them (#68). let named = Guard::duplicate_marker_ids(&comments); let disclosable = self .guard diff --git a/crates/bugwarden/tests/audit_wiremock.rs b/crates/bugwarden/tests/audit_wiremock.rs index c60f2f9..32098cc 100644 --- a/crates/bugwarden/tests/audit_wiremock.rs +++ b/crates/bugwarden/tests/audit_wiremock.rs @@ -678,6 +678,192 @@ async fn suppressed_ids_off_records_the_count_never_the_ids() { ); } +/// Bug 7's comments mix BOTH suppression populations in one call (issue +/// #68): three private comments the I5 gate drops, which carry no bug id +/// of their own, and two duplicate markers naming the +/// [`HIDE_SECRET_POLICY`]-hidden bugs 666 and 667, which do. +/// +/// One of the three private comments is ITSELF a duplicate marker, naming +/// a third hidden bug 668. That comment is what makes the fixture defend +/// the disjointness the sum rests on rather than merely assume it: the +/// marker ids are harvested from the comments that SURVIVED the private +/// filter, so 668 is counted once as id-less content and never named — +/// total 5. Harvest the ids from the pre-filter list instead and the same +/// dropped comment is counted twice, once in each tally: 6, over a +/// three-id list. Both classify fetches are mounted so that mutation +/// fails on the NUMBER rather than on an unmatched mock. +async fn mount_mixed_suppression(mock: &MockServer) { + Mock::given(method("GET")) + .and(path("/rest/bug")) + .and(query_param("id", "7")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ "bugs": [world_bug(7)] }))) + .mount(mock) + .await; + let secret = |id: u64| { + let mut bug = world_bug(id); + bug["product"] = json!("SecretSauce"); + bug + }; + // The link-disclosure fetch asks for the whole candidate set at once. + // Expected exactly once: without the expectation an id list the guard + // never asks for still 404s into disclosable's fail-closed arm, which + // withholds 666/667 anyway and leaves the fixture decorative. + Mock::given(method("GET")) + .and(path("/rest/bug")) + .and(query_param("id", "666,667")) + .respond_with( + ResponseTemplate::new(200).set_body_json(json!({ "bugs": [secret(666), secret(667)] })), + ) + .expect(1) + .mount(mock) + .await; + // Never requested while the harvest stays post-filter — mounted so a + // pre-filter harvest gets a truthful answer and is caught by the + // count, not by a 404. + Mock::given(method("GET")) + .and(path("/rest/bug")) + .and(query_param("id", "666,667,668")) + .respond_with( + ResponseTemplate::new(200) + .set_body_json(json!({ "bugs": [secret(666), secret(667), secret(668)] })), + ) + .mount(mock) + .await; + Mock::given(method("GET")) + .and(path("/rest/bug/7/comment")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "bugs": { "7": { "comments": [ + { "id": 1, "bug_id": 7, "is_private": false, "text": "public comment" }, + { "id": 2, "bug_id": 7, "is_private": true, "text": "canary-private-8a1c one" }, + { "id": 3, "bug_id": 7, "is_private": true, "text": "canary-private-8a1c two" }, + // Private AND a duplicate marker: id-less content by the + // only reading that keeps the two tallies disjoint. + { "id": 4, "bug_id": 7, "is_private": true, + "text": "canary-private-8a1c *** Bug 668 has been marked as a duplicate of this bug ***" }, + { "id": 5, "bug_id": 7, "is_private": false, + "text": "*** Bug 666 has been marked as a duplicate of this bug ***" }, + { "id": 6, "bug_id": 7, "is_private": false, + "text": "*** Bug 667 has been marked as a duplicate of this bug ***" }, + ] } } + }))) + .mount(mock) + .await; +} + +#[tokio::test] +async fn suppressed_count_totals_both_populations_in_one_call() { + // Issue #68: one call suppresses three id-less private comments AND + // two duplicate-marker bug ids. The count is the TOTAL — five — not + // the larger tally, which would report three beside a two-id list and + // silently lose two withheld items. + let mock = MockServer::start().await; + mount_mixed_suppression(&mock).await; + let audited = audited_client_for(HIDE_SECRET_POLICY, &mock, "test-key").await; + + let result = call(&audited.client, "bug_comments", json!({ "id": 7 })).await; + assert_ne!(result.is_error, Some(true), "bug 7's comments are served"); + let envelope = serde_json::to_string(&result).unwrap(); + for id in ["666", "667"] { + assert!( + !envelope.contains(id), + "the hidden id must be scrubbed from the served comments (I14): {envelope}" + ); + } + assert!( + !envelope.contains("canary-private-8a1c"), + "the private comments must not reach the client (I5): {envelope}" + ); + + let events = read_events(&audited.audit_path); + let tc = last_tool_call(&events); + assert_eq!(tc.request.tool, "bug_comments"); + let guard = tc.guard.as_ref().expect("guard info recorded"); + assert_eq!(guard.verdict, Verdict::ServedFiltered); + assert_eq!( + guard.suppressed_ids, + vec![666, 667], + "the surviving comments' marker ids are named, and 668's — noted \ + only inside a DROPPED private comment — is not: naming it would \ + count that one comment in both tallies" + ); + assert_eq!( + guard.suppressed_count, 5, + "three id-less comments plus two withheld ids: {guard:?}" + ); + assert!( + guard.suppressed_ids.len() < guard.suppressed_count as usize, + "the count is authoritative and outruns the id list it may not be \ + inferred from: {guard:?}" + ); +} + +#[tokio::test] +async fn summarize_bug_records_the_same_total_as_bug_comments() { + // summarize_bug filters and scrubs the same comments as bug_comments + // and notes both tallies from its own call site (server.rs), so this + // diff changes the number it records too — it needs its own record, + // not bug_comments' by analogy. + let mock = MockServer::start().await; + mount_mixed_suppression(&mock).await; + let audited = audited_client_for(HIDE_SECRET_POLICY, &mock, "test-key").await; + + let result = call(&audited.client, "summarize_bug", json!({ "id": 7 })).await; + assert_ne!(result.is_error, Some(true), "the summary prompt is served"); + let envelope = serde_json::to_string(&result).unwrap(); + for id in ["666", "667", "668"] { + assert!( + !envelope.contains(id), + "no hidden id may reach the summarization prompt (I14): {envelope}" + ); + } + assert!( + !envelope.contains("canary-private-8a1c"), + "no private comment may reach the summarization prompt (I5): {envelope}" + ); + + let events = read_events(&audited.audit_path); + let tc = last_tool_call(&events); + assert_eq!(tc.request.tool, "summarize_bug"); + let guard = tc.guard.as_ref().expect("guard info recorded"); + assert_eq!(guard.verdict, Verdict::ServedFiltered); + assert_eq!( + guard.suppressed_ids, + vec![666, 667], + "the same two ids bug_comments names, and 668 no more than it does" + ); + assert_eq!( + guard.suppressed_count, 5, + "three id-less comments plus two withheld ids: {guard:?}" + ); +} + +#[tokio::test] +async fn suppressed_count_totals_both_populations_with_the_ids_elided() { + // The same call under `suppressed_ids = false`, which is the case the + // issue calls out: with no id list at all, the count is the operator's + // ONLY signal that anything was withheld (I3 keeps it from the + // client), so it must still total both populations. + let mock = MockServer::start().await; + mount_mixed_suppression(&mock).await; + let audited = audited_client_with(HIDE_SECRET_POLICY, &mock, "test-key", false).await; + + let result = call(&audited.client, "bug_comments", json!({ "id": 7 })).await; + assert_ne!(result.is_error, Some(true), "bug 7's comments are served"); + + let events = read_events(&audited.audit_path); + let tc = last_tool_call(&events); + let guard = tc.guard.as_ref().expect("guard info recorded"); + assert!( + guard.suppressed_ids.is_empty(), + "ids are elided when the knob is off: {:?}", + guard.suppressed_ids + ); + assert_eq!( + guard.suppressed_count, 5, + "the elided ids still count, and the id-less comments add to them: {guard:?}" + ); +} + /// Serve a quicksearch corpus (issue #29 tests): search requests are paged /// by limit/offset the way Bugzilla pages them, and the I14 link-disclosure /// classify fetch — which addresses bugs by id, not by query — gets an diff --git a/docs/DESIGN.md b/docs/DESIGN.md index e17ac3b..fb4fbfc 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -1103,6 +1103,59 @@ Decisions, all deliberate: `guard` object. Re-encoding a default decision AS absence would be a record-schema change and is deferred to #34; schema v1 records `"default"`. +- **What `guard.suppressed_count` totals (issue #68).** The cell keeps two + tallies: a `BTreeSet` of bug ids the guard withheld (`note_suppressed` — + I14-scrubbed links, scrubbed duplicate markers, verdict-dropped search + rows) and a plain counter of suppressed content that HAS no bug id + (`note_suppressed_count` — private comments in `bug_comments` and + `summarize_bug`, private attachment metadata in `list_attachments`). + `suppressed_count` is their SUM. It used to be their maximum, which is + not a count of anything: a `summarize_bug` call that dropped three + private comments while scrubbing two duplicate-marker ids recorded + `3` beside a two-element id list, under-reporting by two and still + validating. Summing is sound because the populations are disjoint by + construction — both comment sites derive their marker ids from the + comments that SURVIVED the private filter, so a dropped comment can + never also contribute an id, and the attachment site names no ids at + all; within the id set itself a `BTreeSet` dedupes the overlap between + the search scan's dropped ids and the link ids scrubbed beside them. + Disjointness is what makes the sum sound, so it is load-bearing rather + than incidental: both harvest sites in `server.rs` carry a comment + saying so, and the fixture behind the record tests plants a duplicate + marker inside a PRIVATE comment — harvesting pre-filter would count + that one comment in both tallies and over-report, which is strictly + worse than the under-report being fixed. + This matters because I3 forbids telling the CLIENT anything was + withheld, so the audit stream is the only place the fact surfaces, and + `suppressed_ids` is gated behind the `suppressed_ids` config switch + while the count always ships — a maximum cannot be the authoritative + number the field claims to be. + ACCEPTED COST, deliberate: no field is added or removed and the wire + shape is unchanged, so this stays schema v1 — but parseable is not + comparable, and the change of MEANING inside v1 is undetectable from a + record. There is no discriminator: an event carries `v`, `ts`, `seq` + and `session` only, `initialize` records the CLIENT's version and never + bugwarden's build, `policy_hash` is unchanged unless the operator + edited policy, and `suppressed_count >= len(suppressed_ids)` holds + under both readings, so there is no structural tell either. A `v: 1` + corpus spanning the upgrade therefore carries both readings under one + version stamp, with the deployed BUILD as the boundary: + `{suppressed_count: 3, suppressed_ids: [666, 667]}` reads as "3 + withheld" after and "at least 5 withheld" before. The safe reading of a + pre-upgrade record is "at least `max(suppressed_count, + len(suppressed_ids))`, possibly more"; a consumer that asserted + equality between the two, or alerted on the inequality, changes + behaviour on mixed calls — observable only where `suppressed_ids = + true`, since with ids elided the two readings are indistinguishable. A + version bump was considered and rejected: it would fork every reader + over a record whose fields, names and types are identical, for a field + already documented as authoritative — which a maximum never satisfied, + making this a correction to the stated contract rather than a new one. + The rule this relies on is that `v` tracks structure, not meaning; it + is now stated on `SCHEMA_VERSION` itself. Splitting the tallies into + two fields (`suppressed_ids_count` and `suppressed_other_count`) is the + cleaner end state, would have made the change self-announcing, and + stays with the v2 work in #34. ## rmcp 3.1 usage notes @@ -1608,7 +1661,19 @@ wired, `server.rs` and `main.rs` are the reference. allow side alike, while a CLEAN search (no drops, so nothing the scan merge could have cleared) records no rule at all even though a named rule granted every row it served, since absence means "no single rule - decided" and not "the default decided". + decided" and not "the default decided"; and what `suppressed_count` + totals (issue #68) — `bug_comments` AND `summarize_bug`, each on a + call that drops three private comments while scrubbing two + duplicate-marker ids in the SAME request, record + `suppressed_count == 5` over a two-element `suppressed_ids`, so the + count exceeds the id list rather than being the larger tally, and with + `suppressed_ids = false` the same call still records `5` with no id at + all — the operator's only signal. One of those three private comments + is itself a duplicate marker naming a third hidden bug, so the fixture + DEFENDS the disjointness the sum rests on: harvesting marker ids from + the pre-filter comment list counts that one comment in both tallies + and records `6` over three ids, and both tools' tests fail on the + number (plus the cell-level sum tests in audit.rs). - Identity tests (#[cfg(test)] in crates/bugwarden/src/server.rs and crates/bugwarden-core/src/client.rs; crates/bugwarden/tests/ http_transport_wiremock.rs, crates/bugwarden/tests/binary_user_agent.rs