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