From 7c0fcf2d9dae639b8d0cca00ffcd3e86ecb2067d Mon Sep 17 00:00:00 2001 From: Martin Pluskal Date: Tue, 11 Aug 2026 22:43:33 +0200 Subject: [PATCH] docs: record what guard.rule names and when it is absent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GuardInfo.rule was documented as "absent when the policy default decided". Policy::classify writes the literal string "default" in both default arms, so the parenthetical inverted the encoding: a consumer would have read "default" as a rule of that name matching, and absence as the default deciding. The README corrected in #66 was left as the only place stating the truth. Fix the documentation, not the behaviour. The literal "default" is strictly more informative than absence, which could not also express "no single rule decided the call"; re-encoding it would be a record schema change and belongs with the v2 work in #34. Record that decision in DESIGN.md's audit-stream decisions list, where a reader auditing the schema will find it, rather than only in a field comment. Document what the field really carries: the four synthetic names the guard emits beside the operator's own, that nothing reserves those spellings so a policy may define a rule called "default", and the six cases where the field genuinely is absent — the issue named three and missed the create gate, an unassessable id, and the attachment paths. A tool removed from the router (I13) is a separate case again: it records no guard object at all. Pin it with two audit-record tests: a default-decided call records "default" on the deny and allow sides alike, and a clean search records no rule even though a named rule granted every row it served. The clean path is load-bearing — with drops, note_scan's strict upgrade clears the rule regardless, so a dropping search cannot observe the property. Closes #67 --- README.md | 2 +- crates/bugwarden-core/src/policy.rs | 18 ++++-- crates/bugwarden/src/audit.rs | 63 ++++++++++++++++--- crates/bugwarden/tests/audit_wiremock.rs | 77 ++++++++++++++++++++++++ docs/DESIGN.md | 32 +++++++++- 5 files changed, 177 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index ffdea95..acb0a2e 100644 --- a/README.md +++ b/README.md @@ -617,7 +617,7 @@ consult the guard — a `guard` object: | `guard` field | Meaning | |---------------|---------| | `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, `min_bug_age_days` for the age quarantine, `:unreadable-metadata` for a rule that could not be decided, `unavailable` for a bug the classification fetch could not reach. Absent where no single rule decided — a search, a refusal, the pre-dispatch gate | +| `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_ids` | The withheld bug ids, subject to the `suppressed_ids` switch | diff --git a/crates/bugwarden-core/src/policy.rs b/crates/bugwarden-core/src/policy.rs index 08c0b36..4c7a317 100644 --- a/crates/bugwarden-core/src/policy.rs +++ b/crates/bugwarden-core/src/policy.rs @@ -825,7 +825,10 @@ impl Policy { /// whose `operations` cover `op`. The scope check precedes matcher /// evaluation, so a rule scoped away from `op` can neither match nor /// fail closed on unreadable metadata for this classification. - /// 3. `default_action`. + /// 3. `default_action`, which decides under the synthetic rule name + /// `"default"` — allow and deny alike. A default decision is NAMED, + /// never rule-less: downstream records carry `"default"`, not an + /// absent rule. /// /// Every grant — from a rule or from the default — has write capabilities /// stripped when `global.read_only` is set. @@ -1068,9 +1071,12 @@ impl BugMeta { /// The outcome of classifying one bug against the policy. #[derive(Debug, Clone)] pub enum Access { - /// No access at all. `rule` names the deciding rule (or the synthetic - /// `"min_bug_age_days"` / `"default"` / `"unavailable"`) for server-side - /// logging only — it is never sent to the MCP client (I1/I2). + /// No access at all. `rule` names the deciding rule (or one of the + /// synthetic `"min_bug_age_days"` / `"default"` / + /// `":unreadable-metadata"` / `"unavailable"`, none of which + /// validation reserves against an operator choosing the same name) for + /// server-side logging only — it is never sent to the MCP client + /// (I1/I2). Denied { /// Server-side-only name of the deciding rule. rule: String, @@ -1079,7 +1085,9 @@ pub enum Access { Granted { /// The granted capability set (writes already stripped if read-only). caps: BTreeSet, - /// Server-side-only name of the deciding rule. + /// Server-side-only name of the deciding rule, or `"default"` — the + /// one synthetic name that appears on this side too, when an + /// allowing `default_action` is what granted. rule: String, }, } diff --git a/crates/bugwarden/src/audit.rs b/crates/bugwarden/src/audit.rs index 2e51a4f..fac4150 100644 --- a/crates/bugwarden/src/audit.rs +++ b/crates/bugwarden/src/audit.rs @@ -474,8 +474,48 @@ pub struct RequestInfo { pub struct GuardInfo { /// The overall verdict for the call. pub verdict: Verdict, - /// Name of the policy rule that decided the verdict, when one did - /// (absent when the policy default decided). + /// Name of the rule that decided the verdict, when a single rule did. + /// + /// A call the policy DEFAULT decided records the literal `"default"` — + /// NOT an absent field. Beside the operator's own rule names the guard + /// emits four synthetic ones: `"default"` (no rule matched), + /// `"min_bug_age_days"` (the age quarantine denied before any rule + /// ran), `":unreadable-metadata"` (a granting rule whose verdict + /// hinged on metadata nobody could read, I4), and `"unavailable"` (the + /// classification fetch never reached the bug). They share one + /// namespace with the operator's names and nothing reserves them, so a + /// policy may define a rule literally called `default`: this field + /// names what decided, it does not prove which kind of thing it was. + /// + /// Absent only where no single rule decided the call: + /// + /// - a refusal answered from the request alone — bad arguments, too + /// many ids; + /// - the pre-dispatch audit gate, where the guard never ran at all; + /// - a search, whose verdict is the window's and not one bug's; + /// - the create gate, refusing or granting: it judges the request as a + /// whole and names no rule either way; + /// - an id the guard produced no matching [`Access`] for — none at + /// all, or one whose variant the re-checked outcome contradicts; + /// - withheld attachment content after a granted assessment, and the + /// constant-cost padding assessment against bug id 0, neither of + /// which any rule decided. + /// + /// A tool the router never carried is a different case again, not an + /// absent rule: read-only mode, `global.disabled_tools` and discovery + /// being off REMOVE the route at construction (I13), so the call + /// reaches no guard, touches no cell, and its record carries no + /// `guard` object at all. + /// + /// [`AuditCell`]'s worst-wins merge carries this through: a rule-less + /// note that outranks a rule-decided one clears the rule. + /// + /// Recording `"default"` rather than absence is deliberate — absence + /// could not also express "no single rule decided". Schema v1 encodes + /// it this way; the decision is recorded in DESIGN.md under "Audit + /// stream", and #34 tracks the schema change. + /// + /// [`Access`]: bugwarden_core::policy::Access #[serde(skip_serializing_if = "Option::is_none")] pub rule: Option, /// Digest of the policy document in force, so a record can be tied to @@ -1127,7 +1167,7 @@ impl AuditCell { /// Worst-wins verdict merge. The rule follows the deciding verdict: a /// strict upgrade replaces the stored rule with the new one (present - /// or not — a default-decided verdict has none), an equally severe + /// or not — a rule-less verdict clears it), an equally severe /// verdict may only fill an empty slot (first rule recorded wins), /// and a less severe verdict changes nothing. fn merge(&self, verdict: Verdict, rule: Option<&str>) { @@ -1150,8 +1190,13 @@ impl AuditCell { } } - /// Note a verdict with no deciding rule (the policy default, or a - /// refusal that never consulted a rule). + /// Note a verdict no single rule decided: a refusal that never + /// consulted one, a search or create gate judging the request as a + /// whole, an id the guard produced no matching decision for, or a + /// withheld attachment — [`GuardInfo::rule`] carries the full list. + /// NOT for a policy-default decision: the default decides under the + /// rule name `"default"` and goes through + /// [`AuditCell::note_verdict_rule`] like any other. pub fn note_verdict(&self, verdict: Verdict) { self.merge(verdict, None); } @@ -2182,10 +2227,12 @@ mod tests { } #[test] - fn cell_upgrade_by_a_default_decision_clears_the_rule() { + fn cell_upgrade_by_a_rule_less_verdict_clears_the_rule() { // The rule names the DECIDING rule of the worst verdict: when a - // default-decided denial outranks a rule-decided serve, no rule - // may stay attached. + // denial no single rule decided outranks a rule-decided serve, no + // rule may stay attached. A policy-DEFAULT denial is not this + // case — it decides under the name "default" and arrives through + // note_verdict_rule. let cell = AuditCell::default(); cell.note_verdict_rule(Verdict::Served, "allow-most"); cell.note_verdict(Verdict::Denied); diff --git a/crates/bugwarden/tests/audit_wiremock.rs b/crates/bugwarden/tests/audit_wiremock.rs index 5e270dc..c60f2f9 100644 --- a/crates/bugwarden/tests/audit_wiremock.rs +++ b/crates/bugwarden/tests/audit_wiremock.rs @@ -446,6 +446,49 @@ async fn refusal_paths_write_exactly_one_record_each() { check(read_events(&audited.audit_path), Verdict::Refused, None); } +#[tokio::test] +async fn a_default_decided_call_records_the_literal_default_rule() { + // Issue #67: the policy default is a NAMED decision. Both arms of it + // write the literal "default" into the record — an absent rule means + // no single rule decided, which is a different statement. Asserted on + // the audit record rather than on Access, because the record is what + // a consumer reads. + let mock = MockServer::start().await; + mount_fixture(&mock).await; + + // Denying default, no rules at all: bug 7 is denied by the default. + let audited = audited_client_for("default_action = \"deny\"\n", &mock, "test-key").await; + let denied = call(&audited.client, "bug_history", json!({ "id": 7 })).await; + assert_eq!(denied.is_error, Some(true)); + let events = read_events(&audited.audit_path); + let guard = last_tool_call(&events) + .guard + .as_ref() + .expect("the guard was consulted"); + assert_eq!(guard.verdict, Verdict::Denied); + assert_eq!( + guard.rule.as_deref(), + Some("default"), + "a default-decided denial is named, not rule-less" + ); + + // Allowing default, no rules at all: the same name on the serve side. + let audited = audited_client_for("default_action = \"allow\"\n", &mock, "test-key").await; + let served = call(&audited.client, "bug_history", json!({ "id": 7 })).await; + assert_ne!(served.is_error, Some(true), "the history is served"); + let events = read_events(&audited.audit_path); + let guard = last_tool_call(&events) + .guard + .as_ref() + .expect("the guard was consulted"); + assert_eq!(guard.verdict, Verdict::Served); + assert_eq!( + guard.rule.as_deref(), + Some("default"), + "a default-decided serve is named too" + ); +} + #[tokio::test] async fn unknown_and_stripped_tools_error_identically_and_still_record() { let mock = MockServer::start().await; @@ -807,6 +850,40 @@ async fn quicksearch_clean_scan_records_zero_drops_and_stays_served() { assert_eq!(guard.suppressed_count, 0); } +#[tokio::test] +async fn quicksearch_records_no_rule_even_when_a_named_rule_judged_every_row() { + // The other half of issue #67: a search verdict belongs to the WINDOW, + // not to any one bug, so the record names no rule — here even though + // `allow-openSUSE` is what granted every row it served. Asserted on a + // CLEAN scan deliberately: once a scan drops rows it merges + // served_filtered rule-lessly, which would clear any rule the search + // had noted and make this assertion pass for the wrong reason. + let policy = concat!( + "default_action = \"deny\"\n", + "[[rule]]\nname = \"allow-openSUSE\"\naction = \"allow\"\n", + "[rule.match]\nproducts = [\"openSUSE\"]\n", + ); + let mock = MockServer::start().await; + mount_search_corpus(&mock, vec![world_bug(1), world_bug(2), world_bug(3)]).await; + let audited = audited_client_for(policy, &mock, "test-key").await; + + let result = call(&audited.client, "bugs_quicksearch", json!({ "query": "q" })).await; + assert_ne!(result.is_error, Some(true), "the search is served"); + + let events = read_events(&audited.audit_path); + let guard = last_tool_call(&events) + .guard + .as_ref() + .expect("guard info recorded"); + assert_eq!(guard.verdict, Verdict::Served, "a clean scan stays served"); + assert_eq!(guard.scan.expect("a search records its scan").dropped, 0); + assert_eq!( + guard.rule.as_deref(), + None, + "no single rule decided the window" + ); +} + #[tokio::test] async fn canary_content_never_reaches_the_audit_file() { const KEY_CANARY: &str = "canary-api-key-5c9e02"; diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 59f1aea..e17ac3b 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -1079,6 +1079,30 @@ Decisions, all deliberate: withheld rows IS a filtered serve; a clean scan leaves the verdict untouched. Client-invisible by construction (I3): the response is built from the window's bugs alone, byte-identical with or without drops. +- **The `guard.rule` encoding (issue #67).** `Access` carries + `rule: String`, never an `Option`, so every assessment names what + decided it — the policy default included, under the literal `"default"`. + That is deliberate, and strictly more informative than absence: it + separates "the default decided this call" from "no single rule decided + it", which one absent field cannot express. Beside the operator's own + names the guard emits three further synthetic ones — + `"min_bug_age_days"` (the age quarantine, before any rule runs), + `":unreadable-metadata"` (a GRANTING rule whose verdict hinged on + metadata nobody could read, I4; an undecidable deny rule keeps its plain + name, having denied for its own reason), and `"unavailable"` (the + classification fetch never reached the bug). Nothing reserves those + spellings against an operator choosing the same rule name, so `rule` + names what decided without proving which kind of thing it was. Absence + therefore carries exactly one meaning — no single rule decided the + 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"`. ## rmcp 3.1 usage notes @@ -1578,7 +1602,13 @@ wired, `server.rs` and `main.rs` are the reference. are elided; a clean search records `scan.dropped == 0` under verdict `served`; and a pre-#29 record line without `guard.scan` deserializes with the scan absent (plus the cell-level note_scan - merge tests and the updated schema golden in audit.rs). + merge tests and the updated schema golden in audit.rs); and the + `guard.rule` encoding (issue #67) — a call decided by the policy + default records the literal `"default"`, on the deny side and the + 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". - 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