From 53550650da432de8d7cedd539be8a88712265869 Mon Sep 17 00:00:00 2001 From: Martin Pluskal Date: Wed, 12 Aug 2026 08:32:30 +0200 Subject: [PATCH] test(audit): pin the suppressed_count zero guard and list_attachments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two paths in the audit suppression counter had no coverage: deleting note_suppressed_count's `if n == 0 { return; }` early return, or deleting the counter call in list_attachments, each left the whole suite green. That guard is what stops a suppression-free call from merging ServedFiltered, so without it every bug_comments, summarize_bug and list_attachments record would claim the response was filtered whether or not anything was withheld — and verdict is the field an operator filters on to find the calls where the guard actually did something. Add three record-level tests. A clean bug_comments call stays served with a zero count; a list_attachments call whose private metadata the I5 gate dropped records that count over an EMPTY id list, the shape only that tool produces; and a list_attachments call over two PUBLIC attachments stays served at zero. The third serves real rows rather than an empty list on purpose: total == filtered.len() == 2 means the recorded zero says nothing was withheld, not that nothing existed to withhold. The third test covers the call site rather than the guard, which a guard-only test cannot do. Adding any single unconditional note to the list_attachments audit block — note_redacted, note_verdict, or note_suppressed over an empty iterator — makes every call of that tool record served_filtered, and each of those left the full suite green before this commit. Every test here was mutation-proven rather than assumed, since "fails if the guard is removed" is exactly what a green CI run cannot show: zero guard deleted -> both zero tests fail on the verdict counter call deleted -> the private-metadata test fails, served vs served_filtered total, not total-filtered -> both attachment tests fail, 3 vs 2 and served_filtered vs served note_redacted added -> the public-attachment test fails note_verdict added -> same note_suppressed(empty) -> same guard moved to a bug_comments-only call-site condition -> only the list_attachments test fails No production code changes. Closes #87 --- crates/bugwarden/tests/audit_wiremock.rs | 168 +++++++++++++++++++++++ docs/DESIGN.md | 36 ++++- 2 files changed, 197 insertions(+), 7 deletions(-) diff --git a/crates/bugwarden/tests/audit_wiremock.rs b/crates/bugwarden/tests/audit_wiremock.rs index 32098cc..e1952f3 100644 --- a/crates/bugwarden/tests/audit_wiremock.rs +++ b/crates/bugwarden/tests/audit_wiremock.rs @@ -864,6 +864,174 @@ async fn suppressed_count_totals_both_populations_with_the_ids_elided() { ); } +#[tokio::test] +async fn bug_comments_with_nothing_filtered_stays_served_and_counts_zero() { + // Issue #87: the id-less counter is fed on EVERY call of these tools, + // with `0` on the common path where the private filter dropped + // nothing. `note_suppressed_count` returns early on zero for exactly + // that reason — without it this record would claim `served_filtered` + // over an empty suppression, and the one field an operator filters on + // to find the calls the guard acted upon would say the same thing + // about every call. + let mock = MockServer::start().await; + mount_fixture(&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 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::Served, + "the fixture's only comment is public and names no bug, so nothing \ + was withheld: {guard:?}" + ); + assert_eq!(guard.suppressed_count, 0); + assert!(guard.suppressed_ids.is_empty()); + assert_eq!( + guard.rule.as_deref(), + Some("default"), + "and the rule that decided the call survives — a filtered merge \ + outranks a clean serve and would clear it" + ); +} + +/// One row of bug 7's attachment metadata, as Bugzilla serves it with the +/// content excluded. +fn attachment(id: u64, is_private: bool, file_name: &str) -> Value { + json!({ + "id": id, + "bug_id": 7, + "is_private": is_private, + "file_name": file_name, + "summary": "attached", + "content_type": "text/plain", + "size": 3, + }) +} + +/// Serve `rows` as bug 7's attachment metadata — the `list_attachments` +/// counter site. The private rows are what the I5 gate drops, and what it +/// drops carries no bug id of its own: this tool names no ids at all, so +/// the call feeds the id-less tally and nothing else. +async fn mount_attachments(mock: &MockServer, rows: Vec) { + 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; + Mock::given(method("GET")) + .and(path("/rest/bug/7/attachment")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ "bugs": { "7": rows } }))) + .mount(mock) + .await; +} + +#[tokio::test] +async fn list_attachments_counts_the_private_metadata_it_dropped() { + // Issue #87: the third id-less site. Two private attachments the I5 + // gate withheld are counted and never named, so this record carries a + // non-zero count over an EMPTY id list — the shape only this tool + // produces, and the reason nothing else was pinning its counter. + let mock = MockServer::start().await; + mount_attachments( + &mock, + vec![ + attachment(51, false, "log.txt"), + attachment(52, true, "canary-private-3f7b-one.txt"), + attachment(53, true, "canary-private-3f7b-two.txt"), + ], + ) + .await; + let audited = audited_client_for(HIDE_SECRET_POLICY, &mock, "test-key").await; + + let result = call(&audited.client, "list_attachments", json!({ "bug_id": 7 })).await; + assert_ne!(result.is_error, Some(true), "the public metadata is served"); + let envelope = serde_json::to_string(&result).unwrap(); + assert!( + !envelope.contains("canary-private-3f7b"), + "private attachment metadata must not reach the client (I5): {envelope}" + ); + // Nor into the operator's own stream: the record counts the drop, it + // does not describe what was dropped. + let raw = std::fs::read_to_string(&audited.audit_path).expect("audit file readable"); + assert!( + !raw.contains("canary-private-3f7b"), + "the withheld metadata must not reach the audit file either" + ); + + let events = read_events(&audited.audit_path); + let tc = last_tool_call(&events); + assert_eq!(tc.request.tool, "list_attachments"); + let guard = tc.guard.as_ref().expect("guard info recorded"); + assert_eq!(guard.verdict, Verdict::ServedFiltered); + assert_eq!( + guard.suppressed_count, 2, + "both dropped attachments are counted: {guard:?}" + ); + assert!( + guard.suppressed_ids.is_empty(), + "and neither is named — list_attachments names no ids at all: {:?}", + guard.suppressed_ids + ); +} + +#[tokio::test] +async fn list_attachments_with_nothing_private_stays_served_and_counts_zero() { + // Issue #87 at the CALL SITE, which the zero guard alone does not + // cover: an unconditional note added beside the counter — a + // `note_redacted`, a rule-less `note_verdict` — leaves the `n == 0` + // early return intact and still marks every clean listing + // `served_filtered`. + // + // Two PUBLIC attachments rather than an empty list: the filter keeps + // both, so the recorded zero means "nothing was withheld" and not + // "there was nothing to withhold", and a counter fed the list length + // instead of the drop fails here as well. + let mock = MockServer::start().await; + mount_attachments( + &mock, + vec![ + attachment(51, false, "log.txt"), + attachment(52, false, "trace.txt"), + ], + ) + .await; + let audited = audited_client_for(HIDE_SECRET_POLICY, &mock, "test-key").await; + + let result = call(&audited.client, "list_attachments", json!({ "bug_id": 7 })).await; + assert_ne!(result.is_error, Some(true), "both attachments are served"); + let envelope = serde_json::to_string(&result).unwrap(); + for file_name in ["log.txt", "trace.txt"] { + assert!( + envelope.contains(file_name), + "the gate kept {file_name}, so nothing was withheld: {envelope}" + ); + } + + let events = read_events(&audited.audit_path); + let tc = last_tool_call(&events); + assert_eq!(tc.request.tool, "list_attachments"); + let guard = tc.guard.as_ref().expect("guard info recorded"); + assert_eq!( + guard.verdict, + Verdict::Served, + "a listing that withheld nothing is a clean serve: {guard:?}" + ); + assert_eq!(guard.suppressed_count, 0); + assert!(guard.suppressed_ids.is_empty()); + assert!( + guard.redacted_fields.is_empty(), + "and nothing was redacted from it either: {:?}", + guard.redacted_fields + ); +} + /// 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 fb4fbfc..774af6a 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -1097,12 +1097,17 @@ Decisions, all deliberate: call: a refusal answered from the request alone, the pre-dispatch gate (the guard never ran), a search (the verdict is the window's, not one bug's), the create gate on either arm (it judges the request as a - whole), an id with no matching `Access`, and the attachment withhold - together with its constant-cost bug-0 padding assessment. A tool the - router never carried (I13) is not this case at all: it records no - `guard` object. Re-encoding a default decision AS absence would be a - record-schema change and is deferred to #34; schema v1 records - `"default"`. + whole), an id with no matching `Access`, the attachment withhold + together with its constant-cost bug-0 padding assessment, and a SERVE + the cell later upgraded to `served_filtered` through a rule-less note — + a suppression, a redaction, a dropping scan — since that note outranks + the grant and the worst-wins merge clears the rule with it (a + `list_attachments` call that dropped private metadata is the plain + case: the default granted the bug, and the record still names no + rule). A tool the router never carried (I13) is not this case at all: + it records no `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 @@ -1673,7 +1678,24 @@ wired, `server.rs` and `main.rs` are the reference. 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). + number (plus the cell-level sum tests in audit.rs); and the counter's + zero guard (issue #87) — a `bug_comments` call whose private filter + dropped nothing records verdict `served`, `suppressed_count == 0` and + the rule that decided it, so deleting `note_suppressed_count`'s + `n == 0` early return, which would merge `served_filtered` (rule-less, + clearing the rule) on EVERY call of the three tools that feed the + counter, fails on the verdict rather than on a count that stays `0` + either way; and `list_attachments`, the id-less site whose guard fields + no record assertion had reached (the one-record-per-call test calls the + tool but reads only its envelope), records a non-zero count over an + EMPTY `suppressed_ids` when the I5 gate drops private attachment + metadata, and `served` with a zero count over a listing of PUBLIC + attachments the gate kept — a real "nothing was withheld" rather than + an empty list's "nothing to withhold". Between them: deleting that + counter call, counting the whole list rather than the drop, and adding + any unconditional note beside it (a `note_redacted`, a rule-less + `note_verdict`) — which the zero guard, living inside + `note_suppressed_count`, cannot stop — are all detectable. - 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