From 1b96a778845cf753570edea2ffab3a730c7a8ba4 Mon Sep 17 00:00:00 2001 From: Martin Pluskal Date: Wed, 12 Aug 2026 09:27:34 +0200 Subject: [PATCH] fix(policy): reserve the rule names the guard decides under MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Policy::validate never looked at Rule::name at all — not for reserved words, not for uniqueness, not even for emptiness. So an operator could name a rule "default", "unavailable" or "min_bug_age_days", or end one with ":unreadable-metadata", and that rule's decisions became indistinguishable in the audit stream from the guard's own synthetic ones. Two rules could also share a name. A log consumer counting default-decided calls by rule == "default" silently over-counts, and an incident reviewer cannot tell a named rule's grant from the default's. guard.rule is the only field saying what decided a per-bug assessment, and I3 keeps that fact from the client, so the audit stream is the only place it can hold. A name that identifies two things does not hold it. Reject all of them at startup, plus blank names — the one check that is not about collision, since a name carrying no content identifies nothing either. This is boot-breaking by design: a policy that started before now fails, naming the offending rule, rather than running while writing records that cannot answer what decided a call. A boot failure is recoverable in one edit; an ambiguous audit trail is not recoverable at all. The collision checks are exact and untrimmed, because the collision is byte equality on the string that lands in the record and nothing normalizes it on the way there. "Default", " default" and "unreadable-metadata" never collided, so rejecting them would break policies that never had the problem. The reservation is closed, not just complete: no accepted name may end in the suffix and names are unique, so no generated name can equal a bare one either. Single-source the four names as constants and reference them at all five emit sites, so renaming a decision renames what is reserved. Reserving a list that classify no longer emits would reintroduce this issue in silence — the guard_wiremock tests now feed the name assess actually emitted back through validation rather than asserting a literal. Namespacing the synthetics in the record instead was rejected: that is a schema change and belongs with the v2 work in #34. Closes #84 --- README.md | 12 +- crates/bugwarden-core/src/guard.rs | 6 +- crates/bugwarden-core/src/policy.rs | 270 +++++++++++++++++- crates/bugwarden-core/tests/guard_wiremock.rs | 13 +- crates/bugwarden/src/audit.rs | 7 +- docs/DESIGN.md | 100 +++++-- examples/policy.toml | 9 + 7 files changed, 380 insertions(+), 37 deletions(-) diff --git a/README.md b/README.md index 9ea1d29..98068f2 100644 --- a/README.md +++ b/README.md @@ -380,6 +380,14 @@ is a `restrict` rule without capabilities, an `allow`/`deny` rule *with* capabilities, or `default_action = "restrict"`. On Unix, bugwarden logs a warning at startup if the policy file is group- or other-writable. +**Upgrading with an existing policy:** rule names are now checked at startup +(see `name` below), so a policy file that started an older bugwarden can stop +this one from starting at all — if it names a rule `default`, +`min_bug_age_days` or `unavailable`, ends a name with `:unreadable-metadata`, +leaves a name blank, or uses one name twice. The error names the offending +rule; rename it and the file loads unchanged. Nothing else about the file's +meaning changed. + A complete, commented example ships in [`examples/policy.toml`](examples/policy.toml). @@ -410,7 +418,7 @@ applies. Put your most specific (usually most restrictive) rules first. | Key | Type | Default | Description | |-----|------|---------|-------------| -| `name` | string | *required* | Rule identifier. It reaches the audit stream, where it names the rule that decided a call, and nothing a client can see | +| `name` | string | *required* | Rule identifier. It reaches the audit stream, where it names the rule that decided a call, and nothing a client can see. Must be non-blank and unique within the file, and may not be one of the names the guard decides under itself — `default`, `min_bug_age_days`, `unavailable`, or anything ending in `:unreadable-metadata` — otherwise an audit record could not say whether your rule or the guard decided. A reserved name, a blank name and a repeated name are each a startup error naming the offending rule. The *collision* checks compare exactly, so `Default`, ` default` and `unreadable-metadata` (no colon) are ordinary names; only the blank check ignores surrounding whitespace | | `description` | string | `""` | Free-form operator documentation | | `match` | table | `{}` (matches every bug) | Match criteria, see below | | `action` | `"allow"` \| `"deny"` \| `"restrict"` | *required* | `allow` grants all capabilities, `deny` grants none, `restrict` grants exactly `capabilities` | @@ -617,7 +625,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 — 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 | +| `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. A policy naming one of its own rules `default` — or any of the others — is a startup error, so this field says what decided *and* 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, 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 | diff --git a/crates/bugwarden-core/src/guard.rs b/crates/bugwarden-core/src/guard.rs index d47cd05..f1fa34e 100644 --- a/crates/bugwarden-core/src/guard.rs +++ b/crates/bugwarden-core/src/guard.rs @@ -22,7 +22,9 @@ use chrono::Utc; use serde_json::{Map, Value}; use crate::client::{BugzillaClient, CLASSIFY_FIELDS}; -use crate::policy::{Access, BugMeta, Capability, IdentitySource, Operation, Policy}; +use crate::policy::{ + Access, BugMeta, Capability, IdentitySource, Operation, Policy, RULE_UNAVAILABLE, +}; /// Fields kept by the redacted summary-only projection of a bug /// ([`Guard::summary_view`]). Everything else — assignee, CC, groups, @@ -217,7 +219,7 @@ impl Guard { // or fetch failure — all fail closed identically (I4). None => ( Access::Denied { - rule: "unavailable".into(), + rule: RULE_UNAVAILABLE.into(), }, Value::Null, ), diff --git a/crates/bugwarden-core/src/policy.rs b/crates/bugwarden-core/src/policy.rs index 4c7a317..8d10ff4 100644 --- a/crates/bugwarden-core/src/policy.rs +++ b/crates/bugwarden-core/src/policy.rs @@ -476,6 +476,11 @@ pub enum Operation { #[serde(deny_unknown_fields)] pub struct Rule { /// Operator-facing identifier (logged server-side, never sent to clients). + /// + /// Must be non-blank, unique across the policy, and not one of the names + /// the guard decides under itself — all three enforced by + /// [`Policy::from_toml_str`] validation, because an audit record names + /// the deciding rule and nothing else says what decided a call. pub name: String, /// Free-form operator documentation. #[serde(default)] @@ -654,6 +659,33 @@ impl Default for Policy { } } +/// Name [`Policy::classify`] decides under when no rule matched and +/// `default_action` settled it — grant and denial alike. +pub(crate) const RULE_DEFAULT: &str = "default"; + +/// Name [`Policy::classify`] denies under when the global age quarantine +/// fired, before any rule ran. +pub(crate) const RULE_MIN_BUG_AGE_DAYS: &str = "min_bug_age_days"; + +/// Name `Guard::assess` denies under when the classification fetch never +/// reached the bug. +pub(crate) const RULE_UNAVAILABLE: &str = "unavailable"; + +/// Suffix [`Policy::classify`] appends to a GRANTING rule's name when that +/// rule's verdict hinged on metadata nobody could read (I4), so a rule +/// literally named `foo:unreadable-metadata` would collide with what a rule +/// named `foo` denies under there. +pub(crate) const UNREADABLE_METADATA_SUFFIX: &str = ":unreadable-metadata"; + +/// Every rule name the guard decides under on its own behalf, which no +/// operator rule may take. +/// +/// This is the single source for both the emit sites above and +/// [`Policy::validate`]'s rejection, so the reservation cannot drift from +/// what the engine actually writes into a record: renaming a decision +/// renames what is reserved with it. +const RESERVED_RULE_NAMES: [&str; 3] = [RULE_DEFAULT, RULE_MIN_BUG_AGE_DAYS, RULE_UNAVAILABLE]; + impl Policy { /// Strict parse + validation of a policy document. /// @@ -661,6 +693,13 @@ impl Policy { /// typo like `product = [...]` for `products` fails loudly instead of /// silently matching nothing. Validation then enforces: /// + /// - every rule has a non-blank name, no two rules share one, and none + /// takes a name the guard decides under itself (`"default"`, + /// `"min_bug_age_days"`, `"unavailable"`, or anything ending in + /// `":unreadable-metadata"`) — an audit record names the deciding rule + /// and nothing else does, so the name has to identify exactly one + /// thing. The comparison is exact: `"Default"` and `" default"` reach + /// the record as themselves and collide with nothing; /// - `restrict` rules carry at least one capability; /// - `allow`/`deny` rules carry no capabilities (a capability list on /// them would be dead configuration masking operator intent); @@ -740,7 +779,61 @@ impl Policy { } _ => {} } - for rule in &self.rules { + // A rule name has to identify exactly one thing: it is the only + // field in an audit record saying what decided an assessment, and I3 + // keeps that fact out of the client's reach entirely, so the stream + // is where it has to hold. Beside the operator's rules the guard + // decides under synthetic names of its own; a rule spelled the same, + // or a second rule sharing a name, makes the record ambiguous. + // The COLLISION comparisons are exact — byte equality is what a log + // consumer greps, so "Default" and " default" collide with nothing + // and stay legal. The blank check is the one that trims, because it + // asks a different question: not "does this name collide" but "does + // it name anything at all". Blankness is judged bytewise too, so + // "embargo" and "embargo\u{200B}" are two accepted names a log + // viewer renders identically; nothing here normalizes a name, by + // design (the policy file is the trust root). + let mut seen_names: BTreeSet<&str> = BTreeSet::new(); + for (idx, rule) in self.rules.iter().enumerate() { + if rule.name.trim().is_empty() { + // Positional, because the name is exactly what cannot + // identify the rule here. + anyhow::bail!( + "rule #{} (name = \"{}\"): name must not be blank; the name \ + is what an audit record reports as the rule that decided \ + a call", + idx + 1, + rule.name + ); + } + if RESERVED_RULE_NAMES.contains(&rule.name.as_str()) { + anyhow::bail!( + "rule \"{}\": the name is reserved for the guard's own \ + decisions (\"{}\"), which an audit record could then no \ + longer tell apart from this rule's — rename the rule", + rule.name, + RESERVED_RULE_NAMES.join("\", \"") + ); + } + if rule.name.ends_with(UNREADABLE_METADATA_SUFFIX) { + anyhow::bail!( + "rule \"{}\": names ending in \"{}\" are reserved for the \ + guard's own decisions — it denies under \"{}\" when a \ + granting rule's verdict hinged on metadata it could not \ + read — rename the rule", + rule.name, + UNREADABLE_METADATA_SUFFIX, + UNREADABLE_METADATA_SUFFIX + ); + } + if !seen_names.insert(rule.name.as_str()) { + anyhow::bail!( + "rule \"{}\": two rules carry this name; an audit record \ + names the rule that decided a call and could not say \ + which of them it was — give each rule a distinct name", + rule.name + ); + } match rule.action { Action::Restrict if rule.capabilities.is_empty() => { anyhow::bail!( @@ -846,7 +939,7 @@ impl Policy { }; if too_young { return Access::Denied { - rule: "min_bug_age_days".to_string(), + rule: RULE_MIN_BUG_AGE_DAYS.to_string(), }; } } @@ -887,18 +980,18 @@ impl Policy { rule: rule.name.clone(), }, Action::Allow | Action::Restrict => Access::Denied { - rule: format!("{}:unreadable-metadata", rule.name), + rule: format!("{}{UNREADABLE_METADATA_SUFFIX}", rule.name), }, }; } } } match self.default_action { - Action::Allow => self.grant(Capability::ALL.iter().copied(), "default"), + Action::Allow => self.grant(Capability::ALL.iter().copied(), RULE_DEFAULT), // `Restrict` as default is rejected by validation; if it appears // in a hand-constructed Policy, fail closed and deny. Action::Deny | Action::Restrict => Access::Denied { - rule: "default".to_string(), + rule: RULE_DEFAULT.to_string(), }, } } @@ -1073,10 +1166,10 @@ impl BugMeta { pub enum Access { /// 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). + /// `":unreadable-metadata"` / `"unavailable"`, every one of which + /// [`Policy::from_toml_str`] 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, @@ -1872,6 +1965,165 @@ products = ["SUSE*"] assert!(msg.contains("default_action"), "unexpected error: {msg}"); } + // ---------- rule names ---------- + + #[test] + fn reject_rule_named_after_a_guard_decision() { + // Each spelling the guard itself decides under: a rule taking one + // would be indistinguishable from the guard's own record. + for name in ["default", "min_bug_age_days", "unavailable"] { + let s = format!("[[rule]]\nname = \"{name}\"\naction = \"deny\"\n"); + let err = Policy::from_toml_str(&s).unwrap_err(); + let msg = format!("{err:#}"); + assert!( + msg.contains(&format!("rule \"{name}\"")), + "unexpected error: {msg}" + ); + assert!(msg.contains("reserved"), "unexpected error: {msg}"); + } + } + + #[test] + fn reject_rule_name_ending_in_the_unreadable_metadata_suffix() { + // classify() denies under `format!("{name}:unreadable-metadata")`, + // so a rule spelled that way collides with the synthetic name + // generated for a rule named "x". + let s = "[[rule]]\nname = \"x:unreadable-metadata\"\naction = \"deny\"\n"; + let err = Policy::from_toml_str(s).unwrap_err(); + let msg = format!("{err:#}"); + assert!( + msg.contains("rule \"x:unreadable-metadata\"") && msg.contains("reserved"), + "unexpected error: {msg}" + ); + } + + #[test] + fn reject_duplicate_rule_names() { + // NOT adjacent: the rule is "no two rules share a name", not "no two + // CONSECUTIVE rules do", so the repeat is separated by a third rule. + let s = concat!( + "[[rule]]\nname = \"embargo\"\naction = \"deny\"\n", + "[[rule]]\nname = \"reporters\"\naction = \"deny\"\n", + "[[rule]]\nname = \"embargo\"\naction = \"allow\"\n", + ); + let err = Policy::from_toml_str(s).unwrap_err(); + let msg = format!("{err:#}"); + assert!( + msg.contains("rule \"embargo\"") && msg.contains("two rules"), + "unexpected error: {msg}" + ); + } + + #[test] + fn reject_blank_rule_name() { + // "" (and whitespace) in a record names nothing at all, the one + // thing the field exists to say. The error is positional because the + // name cannot point at the rule here — put the blank one second so a + // wrong index shows up. + for name in ["", " "] { + let s = format!( + "[[rule]]\nname = \"first\"\naction = \"deny\"\n\ + [[rule]]\nname = \"{name}\"\naction = \"deny\"\n" + ); + let err = Policy::from_toml_str(&s).unwrap_err(); + let msg = format!("{err:#}"); + assert!(msg.contains("blank"), "unexpected error: {msg}"); + assert!(msg.contains("rule #2"), "error must locate it: {msg}"); + } + } + + #[test] + fn rule_names_merely_resembling_a_reserved_one_are_accepted() { + // The collision is byte equality in the record, so the check is + // exact: anything that reaches the audit stream as a different + // string is a different name and stays legal. Over-rejecting here + // would break policies that never had the problem. + for name in [ + "default-allow", + "my-default", + "Default", + " default", + "default ", + "MIN_BUG_AGE_DAYS", + "Unavailable", + "unreadable-metadata", + "x:unreadable-metadata-2", + "x-unreadable-metadata", + ] { + let s = format!("[[rule]]\nname = \"{name}\"\naction = \"deny\"\n"); + let p = Policy::from_toml_str(&s) + .unwrap_or_else(|e| panic!("name {name:?} must stay valid: {e:#}")); + assert_eq!(p.rules[0].name, name); + } + } + + #[test] + fn distinct_rule_names_validate() { + let s = concat!( + "[[rule]]\nname = \"embargo\"\naction = \"deny\"\n", + "[[rule]]\nname = \"reporters\"\naction = \"restrict\"\n", + "capabilities = [\"summary\"]\n", + "[[rule]]\nname = \"Embargo\"\naction = \"allow\"\n", + ); + let p = Policy::from_toml_str(s).unwrap(); + assert_eq!(p.rules.len(), 3); + } + + #[test] + fn every_name_classify_can_emit_is_reserved() { + // Pins the reservation to what the engine actually emits: drive + // each synthetic path and check validation refuses a rule spelled + // the same. ("unavailable" is Guard::assess's, tied to the same + // constant and checked against validation in `guard_wiremock.rs`.) + let deny_default = Policy::from_toml_str("default_action = \"deny\"").unwrap(); + let aged = Policy::from_toml_str("[global]\nmin_bug_age_days = 30\n").unwrap(); + let granting = Policy::from_toml_str(concat!( + "default_action = \"deny\"\n", + "[[rule]]\nname = \"x\"\naction = \"allow\"\n", + "[rule.match]\ngroups = [\"secret\"]\n", + )) + .unwrap(); + let emitted = [ + deny_default.classify( + &BugMeta::default(), + t("2024-05-01T00:00:00Z"), + Operation::Access, + ), + aged.classify( + &BugMeta::default(), + t("2024-05-01T00:00:00Z"), + Operation::Access, + ), + granting.classify( + &BugMeta::default(), + t("2024-05-01T00:00:00Z"), + Operation::Access, + ), + ]; + let names: Vec = emitted + .into_iter() + .map(|access| match access { + Access::Denied { rule } => rule, + Access::Granted { rule, .. } => rule, + }) + .collect(); + // Each fixture must still drive a DIFFERENT synthetic path: if one + // ever fell through to another (a changed BugMeta default would send + // the suffix fixture to "default", also reserved), the loop below + // would keep passing while covering one path twice. + assert_eq!( + names, + ["default", "min_bug_age_days", "x:unreadable-metadata"] + ); + for name in names { + let s = format!("[[rule]]\nname = \"{name}\"\naction = \"deny\"\n"); + assert!( + Policy::from_toml_str(&s).is_err(), + "classify emits {name:?} but validation accepts a rule named that" + ); + } + } + // ---------- operations (rule scoping) ---------- #[test] diff --git a/crates/bugwarden-core/tests/guard_wiremock.rs b/crates/bugwarden-core/tests/guard_wiremock.rs index 89de3e3..ed9d1c1 100644 --- a/crates/bugwarden-core/tests/guard_wiremock.rs +++ b/crates/bugwarden-core/tests/guard_wiremock.rs @@ -150,7 +150,18 @@ async fn assess_costs_one_request_per_distinct_id_whatever_the_answer() { assert!(out[&1].0.allows(Capability::Read)); for id in [2u64, 3] { match &out[&id].0 { - Access::Denied { rule } => assert_eq!(rule, "unavailable"), + Access::Denied { rule } => { + assert_eq!(rule, "unavailable"); + // Whatever name the guard denies under here, validation must + // refuse an operator rule spelled the same — the reservation + // has to track the name actually emitted, not a copy of it + // that a rename could leave behind (#84). + let policy = format!("[[rule]]\nname = \"{rule}\"\naction = \"deny\"\n"); + assert!( + Policy::from_toml_str(&policy).is_err(), + "an operator rule may not be named {rule:?}" + ); + } other => panic!("bug {id} must be denied, got {other:?}"), } assert!(out[&id].1.is_null()); diff --git a/crates/bugwarden/src/audit.rs b/crates/bugwarden/src/audit.rs index 50ec4b7..a4dbb71 100644 --- a/crates/bugwarden/src/audit.rs +++ b/crates/bugwarden/src/audit.rs @@ -491,9 +491,10 @@ pub struct GuardInfo { /// 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. + /// namespace with the operator's names, and for a policy loaded from + /// TOML validation reserves all four against it — such a policy + /// defining a rule literally called `default` is a startup error — so + /// this field names what decided and proves which kind of thing it was. /// /// Absent only where no single rule decided the call: /// diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 774af6a..9ebcd43 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -279,17 +279,22 @@ impl Default for Policy; // allow-all, no rules, defaults impl Policy { pub fn from_toml_str(s: &str) -> anyhow::Result; // strict parse + validate pub fn load(path: &std::path::Path) -> anyhow::Result; // read + from_toml_str; on unix warn (tracing::warn) if file is group/other-writable - // validate: Restrict rules need >=1 capability; Allow/Deny rules must have - // empty capabilities; default_action must not be Restrict; a written - // `operations = []` is an error (a rule applying to no operation is dead - // configuration); a Restrict rule scoped to ONLY `create` must grant - // exactly the `create` capability (without it the rule could grant - // nothing reachable, and any other capability it named would be dead — - // the create gate consults only `create`); a Restrict rule scoped away - // from `create` must not grant `create` (nothing outside the create - // gate consults it, so a typoed scope would otherwise silently lose - // both the intended filing grant and the reads the capability list - // withholds). Unknown operation names are rejected by serde. + // validate: every rule name is non-blank, unique, and not one the guard + // decides under itself ("default", "min_bug_age_days", "unavailable", + // or any name ending in ":unreadable-metadata") — the collision + // comparisons are exact, so "Default" and " default" stay legal, while + // the blank check trims; Restrict rules need >=1 capability; Allow/Deny + // rules must have empty capabilities; default_action must not be + // Restrict; a written `operations = []` is an error (a rule applying to + // no operation is dead configuration); a Restrict rule scoped to ONLY + // `create` must grant exactly the `create` capability (without it the + // rule could grant nothing reachable, and any other capability it named + // would be dead — the create gate consults only `create`); a Restrict + // rule scoped away from `create` must not grant `create` (nothing + // outside the create gate consults it, so a typoed scope would + // otherwise silently lose both the intended filing grant and the reads + // the capability list withholds). Unknown operation names are rejected + // by serde. pub fn classify(&self, bug: &BugMeta, now: chrono::DateTime, op: Operation) -> Access; // Whether any rule consulted for Operation::Access carries a // created_by_me criterion — the laziness gate for whoami (see @@ -1090,14 +1095,15 @@ Decisions, all deliberate: `":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`, the attachment withhold + classification fetch never reached the bug). For a policy loaded from + TOML, validation reserves those spellings against an operator choosing + the same rule name (issue #84, below), so `rule` names what decided AND + 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`, 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 @@ -1161,6 +1167,51 @@ Decisions, all deliberate: 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. +- **Rule names the operator may not take (issue #84).** `Policy::validate` + rejects a rule named `"default"`, `"min_bug_age_days"` or + `"unavailable"`, a rule whose name ends in `":unreadable-metadata"`, a + blank name, and two rules sharing one name. Those four spellings are + exactly what the guard decides under on its own behalf — the first two + and the suffix form from `Policy::classify`, `"unavailable"` from + `Guard::assess` — and an audit record must identify what decided it. + Without the reservation a log consumer counting default-decided calls by + `rule == "default"` over-counted silently, a duplicate name said nothing + about which of two rules decided, and a blank name identified nothing at + all; I3 keeps that fact from the client, so the stream is the only place + it can hold. This is the same startup-error class `validate` already + applies to dead configuration (`operations = []`, a capability list on an + allow/deny rule, a create-scoped restrict rule whose capabilities + disagree with its scope). BOOT-BREAKING and accepted: a policy that + started before now fails at startup with an error naming the rule — the + correct direction, because the server refusing to run beats it writing + ambiguous audit records. The comparison is EXACT, byte equality on the + string that reaches the record: `"Default"`, `" default"` and + `"unreadable-metadata"` (no colon) collide with nothing and stay legal, + and over-rejecting them would break policies that never had the problem. + Blank is the one check that is not about collision — `trim().is_empty()`, + the same reading `global.identity_login` gets in the same function — + because a name that identifies nothing is the one thing the field exists + to prevent; its error is POSITIONAL (`rule #3 (name = "")`) since the + name is exactly what cannot identify the rule there. Blankness is still + judged bytewise, so `"embargo"` and `"embargo\u{200B}"` are two accepted + names that any log viewer renders identically — the guarantee is + byte-level, not human-reader-level, and nothing normalizes a name + anywhere between `Rule::name` and the JSON record. That is deliberate: + normalizing would break the exactness the collision checks rest on, and + the policy file is the trust root, so a name only its author can + distinguish is operator self-harm across no privilege boundary. The + reservation is SINGLE-SOURCED rather than restated: `RULE_DEFAULT`, + `RULE_MIN_BUG_AGE_DAYS`, `RULE_UNAVAILABLE` and + `UNREADABLE_METADATA_SUFFIX` are consumed both by the emit sites + (`Policy::classify`, and `Guard::assess` across the module boundary) and + by `RESERVED_RULE_NAMES`, so renaming a decision renames what is + reserved with it and the reservation cannot drift from what the engine + writes into a record. The set is also CLOSED: no accepted name may end + in the suffix and names are unique, so no generated + `":unreadable-metadata"` can ever equal a bare reserved name. The + alternative, namespacing the synthetics in the record (a `rule_kind` + field, or a prefix), was rejected here: it is a record-schema change and + belongs to #34. ## rmcp 3.1 usage notes @@ -1425,7 +1476,16 @@ wired, `server.rs` and `main.rs` are the reference. skipped into an allowing default — and validation rejects `operations = []`, unknown operation names, a restrict rule scoped to only `create` whose capabilities are not exactly `create`, and a - restrict rule scoped away from `create` that grants `create`; the + restrict rule scoped away from `create` that grants `create`; rule names + — validation rejects a rule named `default`, `min_bug_age_days` or + `unavailable`, a name ending in `:unreadable-metadata`, a blank + (empty or whitespace-only) name, and two rules sharing a name even when + they are not adjacent, while near misses stay LEGAL (`default-allow`, + `my-default`, `Default`, ` default`, `unreadable-metadata` without the + colon), and every name `classify` actually emits is checked to be one + validation rejects — with `Guard::assess`'s `"unavailable"` checked the + same way in `guard_wiremock.rs`, so the reservation cannot drift from + what the engine writes into a record; the shipped examples/policy.toml is pinned end to end against its own header: it parses, accepts filing into the desktop products, refuses an embargo-marked title everywhere, refuses filing elsewhere (omitted and diff --git a/examples/policy.toml b/examples/policy.toml index fb35aa9..d64df39 100644 --- a/examples/policy.toml +++ b/examples/policy.toml @@ -35,6 +35,15 @@ # is never visible to the MCP client. Parsing is strict: unknown keys anywhere # are a startup error, so typos cannot silently disable a guard. # +# Rule names: each `name` below reaches the audit stream as the rule that +# decided a call, so it has to identify exactly one thing. A blank name, two +# rules sharing a name, and the names the guard decides under itself +# ("default", "min_bug_age_days", "unavailable", or anything ending in +# ":unreadable-metadata") are all startup errors. The COLLISION checks are +# exact — "Default" and "unreadable-metadata" are ordinary names; only the +# blank check ignores surrounding whitespace. An upgrade can therefore stop +# a policy file that started an older bugwarden; the error names the rule. +# # Rule evaluation order: # 1. global.min_bug_age_days (a too-young bug is invisible, no rule runs; # this gate is global — it is never operation-scoped, and it refuses