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