From dc25295860b4a5980a75ccb74151b480469ea797 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20S=C3=BAkup?= Date: Sat, 8 Aug 2026 23:33:31 +0200 Subject: [PATCH] feat(server): accept custom fields when filing a bug MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The audit's first Warning noted that create_bug had no way to carry cf_* values, so a product with a mandatory custom entry field could not be filed into at all — update_bug_fields already had the cf_* machinery (I7), but the create side was a gap, not a new mechanism. create_bug now accepts the same custom_fields param and the same I7 cf_* prefix gate as the updater, applied before may_create and before any upstream call. The early error is safe to distinguish from the padded, uniform create refusal because its outcome is a pure function of the client's own key names, not of policy or upstream state. Confirmed no Matcher criterion reads cf_*, so a custom field cannot move a prospective bug between policy rules; a guard test pins that down alongside the existing product/component reclassification tests. --- README.md | 13 ++++---- crates/bugwarden-core/src/guard.rs | 21 +++++++++++++ crates/bugwarden/src/server.rs | 26 +++++++++++++++- crates/bugwarden/tests/tools_wiremock.rs | 38 ++++++++++++++++++++++++ docs/DESIGN.md | 13 +++++--- 5 files changed, 100 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index edb479f..9495b8e 100644 --- a/README.md +++ b/README.md @@ -89,8 +89,9 @@ is `bug_url`, which computes a URL string locally and contacts nothing. sets `allow_private_comments = true` **and** the individual call opts in with `include_private = true`. Either alone is not enough. - **Custom fields cannot smuggle writes.** `update_bug_fields.custom_fields` - accepts only keys starting with `cf_`; anything else (e.g. `groups`, `cc`, - `assigned_to`) is rejected before Bugzilla is contacted. + and `create_bug.custom_fields` accept only keys starting with `cf_`; + anything else (e.g. `groups`, `cc`, `assigned_to`) is rejected before + Bugzilla is contacted. - **The API key never leaks.** The Bugzilla API key is never written to logs, error messages, or tool results; HTTP errors are sanitized so that a key passed as a URL query parameter cannot appear in error text. @@ -104,8 +105,8 @@ is `bug_url`, which computes a URL string locally and contacts nothing. - **Private comments default to off.** The default policy has `allow_private_comments = false`, so a policy file is required to enable them. -- **`update_bug_fields` custom fields are restricted** to `cf_*` keys as - described above. +- **`update_bug_fields` and `create_bug` custom fields are restricted** to + `cf_*` keys as described above. ## Installation @@ -528,7 +529,7 @@ implied. | `assign` | write | changing the assignee | | `cc` | write | modifying the CC list | | `deps` | write | changing blocks/depends_on | -| `create` | write | filing a new bug — judged against the bug *as requested*, so a rule that hides a product by name also refuses filing into it. The request's `groups` claim is never trusted (Bugzilla adds mandatory groups server-side), so **a rule consulting `groups` or `group_restricted` refuses every create request that reaches it** — to accept new bugs under such a policy, grant `create` in a rule scoped with `operations = ["create"]` placed before the group-consulting rules; being create-scoped, the grant leaves reads of existing bugs untouched, and without such a grant the policy refuses all bug filing | +| `create` | write | filing a new bug, including `cf_*` custom fields — judged against the bug *as requested*, so a rule that hides a product by name also refuses filing into it. The request's `groups` claim is never trusted (Bugzilla adds mandatory groups server-side), so **a rule consulting `groups` or `group_restricted` refuses every create request that reaches it** — to accept new bugs under such a policy, grant `create` in a rule scoped with `operations = ["create"]` placed before the group-consulting rules; being create-scoped, the grant leaves reads of existing bugs untouched, and without such a grant the policy refuses all bug filing | | `attach` | write | uploading an attachment to a bug | When the server is read-only (policy or CLI), the eight write capabilities @@ -682,7 +683,7 @@ none of the three needs an API key. Every other tool does, including | `update_bug_dependencies` | Add/remove blocks and depends_on entries | `deps` | | `add_cc_to_bug` | Add an email to the CC list (the tool only adds; removal is not exposed) | `cc` | | `mark_as_duplicate` | Close a bug as DUPLICATE of another, with an auto-generated comment when none is given | `status` on the bug **and** at least `summary` on the duplicate target | -| `create_bug` | File a new bug; the request is policy-checked *as described* before anything is created. A policy refusal and a Bugzilla-side failure return the same refusal text at the same cost, so a failed create never says which of the two refused, or why | `create` on the bug as requested | +| `create_bug` | File a new bug, including `cf_*` custom fields; the request is policy-checked *as described* before anything is created. A policy refusal and a Bugzilla-side failure return the same refusal text at the same cost, so a failed create never says which of the two refused, or why | `create` on the bug as requested | | `add_attachment` | Upload a base64-encoded attachment to a bug, optionally private or flagged as a patch, capped by `max_attachment_bytes` (decoded size) | `attach` on the target bug | | `bug_url` | Compute `{server}/show_bug.cgi?id={id}` locally | none (contacts nothing) | | `bugzilla_server_info` | Bugzilla version, extensions, timezone, time, parameters | none | diff --git a/crates/bugwarden-core/src/guard.rs b/crates/bugwarden-core/src/guard.rs index 3025908..62522db 100644 --- a/crates/bugwarden-core/src/guard.rs +++ b/crates/bugwarden-core/src/guard.rs @@ -1718,6 +1718,27 @@ products = ["NoView*"] assert!(g.may_create(&create_request("openSUSE"))); } + #[test] + fn may_create_verdict_is_unchanged_by_custom_fields() { + // No Matcher criterion reads cf_* (see policy::Matcher), so a + // prospective bug's custom fields cannot move it between rules — + // unlike product/component, which DESIGN.md deliberately withholds + // from create-time reclassification. + let g = Guard { + policy: policy(concat!( + "[[rule]]\nname = \"hide-security\"\naction = \"deny\"\n", + "[rule.match]\nproducts = [\"Security*\"]\n", + )), + }; + let mut allowed = create_request("openSUSE"); + allowed["cf_secret_field"] = json!("hidden"); + assert!(g.may_create(&allowed)); + + let mut denied = create_request("Security Response"); + denied["cf_secret_field"] = json!("hidden"); + assert!(!g.may_create(&denied)); + } + #[test] fn may_create_never_lets_the_claimed_group_list_decide() { // Bugzilla UNIONS the product's mandatory groups into whatever the diff --git a/crates/bugwarden/src/server.rs b/crates/bugwarden/src/server.rs index 660f7eb..8c775a3 100644 --- a/crates/bugwarden/src/server.rs +++ b/crates/bugwarden/src/server.rs @@ -966,6 +966,10 @@ pub struct CreateBugParams { /// Groups to restrict the new bug to. #[serde(default)] pub groups: Vec, + /// Custom fields, e.g. {"cf_fixed_in": "1.2.3"}. Keys must start with + /// 'cf_'. + #[serde(default)] + pub custom_fields: Option, } #[derive(Debug, serde::Deserialize, schemars::JsonSchema)] @@ -1984,7 +1988,7 @@ impl BugWarden { } #[tool( - description = "File a new bug. The bug is checked against server policy AS DESCRIBED before it is created, so a product or component the policy withholds cannot be filed into either. Returns the new bug id on success.", + description = "File a new bug. The bug is checked against server policy AS DESCRIBED before it is created, so a product or component the policy withholds cannot be filed into either. Accepts custom 'cf_*' fields for products with mandatory entry fields. Returns the new bug id on success.", annotations( read_only_hint = false, destructive_hint = false, @@ -2000,6 +2004,7 @@ impl BugWarden { tracing::info!( product = %p.product, component = %p.component, + custom_field_count = p.custom_fields.as_ref().map_or(0, |cf| cf.len()), "tool: create_bug" ); let key = self.api_key(&ctx)?; @@ -2028,6 +2033,25 @@ impl BugWarden { if !p.groups.is_empty() { payload.insert("groups".to_string(), json!(p.groups)); } + if let Some(custom_fields) = &p.custom_fields { + // I7: only cf_* keys may pass through, same gate as + // update_bug_fields. This early error is safe even though the + // create refusal must otherwise stay padded and uniform: its + // outcome is a pure function of the client's own key names, not + // of policy or upstream state, so it discloses nothing about + // either. + for k in custom_fields.keys() { + if !k.starts_with("cf_") { + note_refused(&ctx); + return Ok(err_text(format!( + "Invalid custom field '{k}': custom field names must start with 'cf_'" + ))); + } + } + for (k, v) in custom_fields { + payload.insert(k.clone(), v.clone()); + } + } let payload = Value::Object(payload); // No bug exists yet, so the bug AS REQUESTED is what the policy diff --git a/crates/bugwarden/tests/tools_wiremock.rs b/crates/bugwarden/tests/tools_wiremock.rs index 3940c39..0dc8009 100644 --- a/crates/bugwarden/tests/tools_wiremock.rs +++ b/crates/bugwarden/tests/tools_wiremock.rs @@ -234,6 +234,44 @@ async fn create_bug_success_reaches_bugzilla_untouched() { assert!(text_of(&result).contains("4242")); } +#[tokio::test] +async fn create_bug_custom_field_reaches_the_post_body() { + let mock = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/rest/bug")) + .and(body_partial_json(json!({ "cf_fixed_in": "1.2.3" }))) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ "id": 4243 }))) + .expect(1) + .mount(&mock) + .await; + let client = client_for("", &mock).await; + let mut args = create_args("openSUSE"); + args["custom_fields"] = json!({ "cf_fixed_in": "1.2.3" }); + let result = call(&client, "create_bug", args).await; + assert!(!is_error(&result), "a cf_* key must reach the POST body"); +} + +#[tokio::test] +async fn create_bug_rejects_non_cf_custom_keys_with_no_upstream_request() { + // I7, same gate as update_bug_fields: a non-cf_ key must not smuggle a + // write through the generic create payload, and the refusal must cost + // zero upstream requests — it decides nothing about the policy. + let mock = MockServer::start().await; + let client = client_for("", &mock).await; + let mut args = create_args("openSUSE"); + args["custom_fields"] = json!({ "assigned_to": "someone@example.org" }); + let result = call(&client, "create_bug", args).await; + assert!(is_error(&result)); + assert_eq!( + text_of(&result), + "Invalid custom field 'assigned_to': custom field names must start with 'cf_'" + ); + assert!( + mock.received_requests().await.unwrap().is_empty(), + "the cf_ gate must refuse before any upstream request (I7)" + ); +} + #[tokio::test] async fn create_bug_claimed_groups_never_defeat_a_group_rule() { // The canonical embargo pattern: the policy denies on group names, and diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 1cba7ef..24ea078 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -77,9 +77,14 @@ Dependency direction: `bugwarden -> bugwarden-core`, never the reverse. `allow_private_comments = false` — private data is strictly opt-in. - **I6** Capability implication: `read` implies `summary`. Nothing else is implied. -- **I7** `update_bug_fields.custom_fields`: every key must start with `cf_`; - otherwise the tool errors without calling Bugzilla (prevents smuggling - `groups`/`cc`/`assigned_to` changes through the generic updater). +- **I7** `update_bug_fields.custom_fields` and `create_bug.custom_fields`: + every key must start with `cf_`; otherwise the tool errors without calling + Bugzilla (prevents smuggling `groups`/`cc`/`assigned_to` changes through + the generic updater or the create payload). On `create_bug` the gate runs + before `may_create` and before any upstream request; unlike the padded, + uniform create refusal, this early error is safe to distinguish because + its outcome is a pure function of the client's own key names, not of + policy or upstream state. - **I8** Every tool that takes a bug id performs guard assessment BEFORE any side effect or data return. Exception: `bug_url` (computes a URL string locally, contacts nothing). @@ -864,7 +869,7 @@ constraints the model must know. | bug_history | id, new_since?: DateTime | history | | | bug_comments | id, include_private: bool = false, new_since? | comments | filter_comments applied (I5) | | bugs_quicksearch | query, status: String = "ALL", include_fields: String = "id,product,component,assigned_to,status,resolution,summary,last_change_time", limit: u32 = 50, offset: u32 = 0 | post-filter | fetch include_fields = requested ∪ CLASSIFY_FIELDS; after filter, project kept bugs to requested fields (keep `_redacted` marker); envelope `{"bugs":[..]}` only (I3), except an advisory `note` when the query is nothing but bug ids (comma/whitespace-separated, optional `#` per id) steering exact id sets to bug_info — the note is a pure function of the CLIENT'S REQUEST (the query and status strings), never of results, verdicts, or anything upstream said (no new oracle), and the `bugs` array is byte-identical with or without it (the query is still searched, never rerouted); its wording tracks the request: a non-empty status is prefixed to the query so upstream content-matches the whole expression, while an empty status sends the query bare and Bugzilla routes a bare all-number query to an exact id lookup (bug_id + anyexact) — on that path the note drops the content-matching claim — and a query naming more distinct ids than MAX_ASSESS_IDS steers to batched bug_info calls (the cap is already public in the too_many_ids refusal text) instead of straight into that refusal | **limit/offset address the bugs the client may SEE, not upstream rows** (Guard::quicksearch_window): filtering an already-paginated page left a hole exactly where a hidden bug sat — a short page the next offset contradicted — and since quicksearch matches summary text that hole was a probe for the hidden title, one word at a time. The guard now scans upstream from row 0 in 200-row chunks, classifies each, and fills the window from the survivors; rows are deduped on the server-reported id (relevance order is not stable between calls) and an id-less row is dropped (I4). Bounds: MAX_SEARCH_WINDOW=1000 addressable, 2000 rows scanned (<=10 sequential requests); hitting either truncates, which looks exactly like the end of results. The objects returned are the ones classified. The scan target is quantised to whole chunks so the stopping point does not track the client's `limit`; without that, `limit` could be binary-searched against the clock to recover each block's exact hidden count. Residual, accepted: filling a window of VISIBLE bugs needs more rows when bugs are hidden, so a stopwatch still learns one bit per scanned block ("not entirely visible"). Removing that would mean scanning the worst case on every search, or letting pages go short again. Search failure returns a bare "Search failed"; the upstream text is logged server-side only (it can name a bug and say whether it exists). The scan's accounting — rows examined, verdict-dropped ids — goes to the audit record only (`guard.scan` plus the suppressed-ids machinery, issue #29); the response is byte-identical with or without drops | -| create_bug | product, component, summary, version, description = "", severity?, priority?, op_sys?, platform?, keywords?: Vec, groups?: Vec | create (write), judged on the bug AS REQUESTED (Guard::may_create) | there is no bug id to assess, so the request itself is classified BEFORE any upstream call (I8): the rules that hide a product by name refuse filing into it, a field the request omits fails closed (I4), and a client-claimed `groups` list is never trusted — Bugzilla unions the product's mandatory groups in server-side, so may_create forces groups to unknown, which means a group-consulting rule refuses every create request that REACHES it — creation is possible only where an earlier rule covering the create operation grants it (a rule carrying `operations = ["create"]`, placed ahead of the group-consulting rules, is how an operator permits filing without that grant shadowing reads of existing bugs — issue #26), and a policy with no such grant refuses all creation. **Both refusals are one refusal**: a policy refusal and an upstream failure return the same fixed create_denial text after the same single upstream request — the refused path burns one classify call against bug id 0 (never a valid id, creates nothing; download_attachment's padding precedent) instead of the POST. Two texts, or 0 vs 1 requests, would be a free policy-enumeration oracle: send a guaranteed-invalid `version` plus a probe product and read the policy off which refusal (or which latency) comes back, with nothing created. Residual, accepted: a SUCCESSFUL create still confirms the product is allowed — that is the tool doing its job, and it costs a real, attributable bug; and the padding equalizes request count, not the upstream handler's exact latency (GET classify vs rejected POST). Bugzilla's failure message is logged server-side only (it can say whether a product/component exists) | +| create_bug | product, component, summary, version, description = "", severity?, priority?, op_sys?, platform?, keywords?: Vec, groups?: Vec, custom_fields?: JsonObject | create (write), judged on the bug AS REQUESTED (Guard::may_create) | there is no bug id to assess, so the request itself is classified BEFORE any upstream call (I8): the rules that hide a product by name refuse filing into it, a field the request omits fails closed (I4), and a client-claimed `groups` list is never trusted — Bugzilla unions the product's mandatory groups in server-side, so may_create forces groups to unknown, which means a group-consulting rule refuses every create request that REACHES it — creation is possible only where an earlier rule covering the create operation grants it (a rule carrying `operations = ["create"]`, placed ahead of the group-consulting rules, is how an operator permits filing without that grant shadowing reads of existing bugs — issue #26), and a policy with no such grant refuses all creation. **Both refusals are one refusal**: a policy refusal and an upstream failure return the same fixed create_denial text after the same single upstream request — the refused path burns one classify call against bug id 0 (never a valid id, creates nothing; download_attachment's padding precedent) instead of the POST. Two texts, or 0 vs 1 requests, would be a free policy-enumeration oracle: send a guaranteed-invalid `version` plus a probe product and read the policy off which refusal (or which latency) comes back, with nothing created. Residual, accepted: a SUCCESSFUL create still confirms the product is allowed — that is the tool doing its job, and it costs a real, attributable bug; and the padding equalizes request count, not the upstream handler's exact latency (GET classify vs rejected POST). Bugzilla's failure message is logged server-side only (it can say whether a product/component exists). `custom_fields` keys must start with `cf_` (I7): the gate runs before `may_create` and errors with ZERO upstream requests on a non-`cf_` key — distinguishable from the padded create refusal on purpose, since it decides nothing about policy or Bugzilla. No Matcher criterion reads `cf_*`, so a custom field cannot move a prospective bug between rules the way `product`/`component` do. `custom_fields` is not in `PARAM_ALLOWLIST`, so the audit stream records it as `_len`, same as the updater | | add_attachment | bug_id, data (base64), file_name, summary, content_type, comment = "", is_private = false, is_patch = false | attach (write) on bug_id | guard assessment before the upload (I8), uniform denial (I2); then global.max_attachment_bytes caps the DECODED size of `data` (0 = no cap) — the ceiling the operator set on downloads binds uploads through the same server too, measured after base64 expansion is stripped so encoding overhead cannot shrink it. The refusal names neither the payload's size nor the cap value (max_attachment_bytes is not I1-disclosable, exactly as on the download path). Over http that non-disclosure is partial and knowingly so: the transport's POST body cap is derived from this same value (#52), so its 413 boundary is probeable once the cap exceeds ~2.25 MiB decoded — accepted, with the reasoning, under "rmcp 3.1 usage notes" below. Nothing here changes: this refusal still names neither size nor cap. `comment` travels as a PLAIN string — Bug.add_attachment documents it so; the `{"comment": {"body": ..}}` shape belongs to Bug.update only | | add_comment | bug_id, comment, is_private: bool = false | comment (write) | | | update_bug_status | bug_id, status, resolution?, comment: String = "" | status (write) | CLOSED requires resolution (error otherwise); when reopening (status not CLOSED/VERIFIED and no resolution given) set `"resolution": ""` |