diff --git a/crates/bugwarden/tests/audit_wiremock.rs b/crates/bugwarden/tests/audit_wiremock.rs index e1952f3..8f61325 100644 --- a/crates/bugwarden/tests/audit_wiremock.rs +++ b/crates/bugwarden/tests/audit_wiremock.rs @@ -900,6 +900,140 @@ async fn bug_comments_with_nothing_filtered_stays_served_and_counts_zero() { ); } +/// The counterpart to [`mount_hidden_link`]: bug 7 names bug 808 twice — +/// its `depends_on` links it, and its one comment is a duplicate marker +/// naming it — and 808 is a plain bug [`HIDE_SECRET_POLICY`] lets through. +/// That comment is public, so the I5 gate drops nothing either. +/// +/// The point is that each id-set call site HAS a candidate to weigh: a +/// clean record over this fixture means "the guard withheld nothing", not +/// "there was nothing to withhold". A guard that never classified 808 — +/// or classified it and said no — fails closed and scrubs the link, which +/// shows up here as a suppression rather than as a pass. +/// +/// Every row is load-bearing, which is the property a fixture built +/// against vacuity had better have: deleting 808's classification mock, +/// making 808 a hidden `Secret*` bug, dropping the `depends_on` link, +/// emptying the comment list, making the comment private, and replacing +/// the marker with plain text each turn one of the two tests below red. +async fn mount_disclosable_link(mock: &MockServer) { + let mut linked = world_bug(7); + linked["depends_on"] = json!([808]); + Mock::given(method("GET")) + .and(path("/rest/bug")) + .and(query_param("id", "7")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ "bugs": [linked] }))) + .mount(mock) + .await; + Mock::given(method("GET")) + .and(path("/rest/bug")) + .and(query_param("id", "808")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ "bugs": [world_bug(808)] }))) + .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": "*** Bug 808 has been marked as a duplicate of this bug ***" }, + ] } } + }))) + .mount(mock) + .await; +} + +#[tokio::test] +async fn summarize_bug_with_nothing_filtered_stays_served_and_counts_zero() { + // Issue #88, the sibling of the bug_comments case above: summarize_bug + // feeds BOTH tallies on every call — the id-less counter + // unconditionally, the id set under `if !hidden.is_empty()`. Drop that + // guard and `note_suppressed` fires with an empty iterator and merges + // `served_filtered` into every summarize_bug record, over an empty + // `suppressed_ids`; the one field an operator filters on to find the + // calls the guard acted upon would then say the same thing about all + // of them. The other tally leans on the `n == 0` early return inside + // `note_suppressed_count`, which the issue #87 tests pin for + // bug_comments and list_attachments but not at this third call site. + let mock = MockServer::start().await; + mount_disclosable_link(&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(); + assert!( + envelope.contains("Bug 808 has been marked as a duplicate"), + "the marker names a bug the policy allows, so the scrub keeps it \ + and the recorded zero means nothing was withheld: {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::Served, + "every comment is public and the only bug they name is disclosable, \ + 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" + ); +} + +#[tokio::test] +async fn bug_info_with_every_link_disclosable_stays_served_and_redacts_nothing() { + // The same defect class at bug_info's two notes, neither of which + // anything was pinning: the I14-scrubbed link ids under + // `if !hidden_links.is_empty()`, and the summary-view marker under + // `if redacted`. Both notes fire unconditionally once their guard is + // gone, and either one merges `served_filtered` into every bug_info + // record — the redaction note additionally naming a view the client + // was never put into. + let mock = MockServer::start().await; + mount_disclosable_link(&mock).await; + let audited = audited_client_for(HIDE_SECRET_POLICY, &mock, "test-key").await; + + let result = call(&audited.client, "bug_info", json!({ "bug_ids": [7] })).await; + assert_ne!(result.is_error, Some(true), "bug 7 is served"); + let envelope = serde_json::to_string(&result).unwrap(); + assert!( + envelope.contains("808"), + "the link names a bug the policy allows, so I14 keeps it and the \ + empty suppression means nothing was withheld: {envelope}" + ); + + let events = read_events(&audited.audit_path); + let tc = last_tool_call(&events); + assert_eq!(tc.request.tool, "bug_info"); + let guard = tc.guard.as_ref().expect("guard info recorded"); + assert_eq!( + guard.verdict, + Verdict::Served, + "bug 7 is served whole and its only link was disclosable: {guard:?}" + ); + assert_eq!(guard.suppressed_count, 0); + assert!(guard.suppressed_ids.is_empty()); + assert!( + guard.redacted_fields.is_empty(), + "and the bug was served whole, not as a summary view: {:?}", + guard.redacted_fields + ); + 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 { diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 9ebcd43..a0d85e5 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -1755,7 +1755,29 @@ wired, `server.rs` and `main.rs` are the reference. 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. + `note_suppressed_count`, cannot stop — are all detectable; and the + empty-set guards at the note call sites themselves (issue #88), where + the counter's zero guard reaches nothing: `note_suppressed` merges + `served_filtered` whatever it is handed, so a call site that drops its + `if !hidden.is_empty()` claims a filtered serve over an EMPTY + `suppressed_ids` on every call, and `note_redacted` does the same over + a `redacted_fields` naming a view the client was never put into. A + `summarize_bug` call whose comments are all public and name one + DISCLOSABLE bug records verdict `served`, `suppressed_count == 0`, no + id and the rule that decided it — pinning that tool's id-set guard and + giving the counter's third call site the clean-call record #87 left it + without; a `bug_info` call over a bug served whole whose only link the + guard weighed and allowed records the same clean serve with an empty + `redacted_fields`, pinning both of that tool's notes on the side no + assertion had reached — its link suppression had only ever been read + on a call that DID suppress, its redaction note on no record at all. + The one shared fixture hands each id-set site a candidate to weigh + instead of an empty set, and earns bug 7 a FULL grant rather than a + summary view, so a clean record means "the guard withheld nothing" and + not "there was nothing to withhold". The remaining note sites — + `bug_history`'s and `bug_comments`' id sets, `bugs_quicksearch`'s two + id sets and its redaction note — already fail on a clean-call record + above, but over fixtures with nothing to weigh. - 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