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