From 42e961f8b7102dc702261c3632b1cd6cfa13fdd2 Mon Sep 17 00:00:00 2001 From: Martin Pluskal Date: Sat, 8 Aug 2026 21:48:13 +0200 Subject: [PATCH] fix(server): derive the http body cap from the attachment ceiling The streamable-HTTP transport capped every POST body at a pinned 4 MiB while global.max_attachment_bytes limits attachments on the decoded size. Base64 expands by 4/3 plus framing, so an operator who raised the attachment cap past ~3 MiB had uploads refused by the transport with a bare 413 before the handler ran: the configured limit was silently overridden, the refusal looked like nothing else bugwarden says, and no audit record was written. Size the transport from the policy instead. http_server_config is now a method on BugWarden reading the same guard the server enforces with -- the config and the guard cannot disagree by construction -- and the cap is derived as ceil(max_attachment_bytes/3)*4 plus 1 MiB of framing headroom, clamped between a 4 MiB floor and a 64 MiB ceiling: * the floor keeps non-attachment traffic and every policy at or below the 2 MiB default exactly where the old pin held them; * the ceiling restores the memory bound for ALL policy values -- 0 ("no policy cap") stays at the floor, and a huge or fat-fingered cap clamps to 64 MiB instead of deriving an effectively unbounded body, which would hand any client that can reach the port an out-of-memory lever. A decoded cap above ~47 MiB is therefore not honored over HTTP, the mirror of the 0 decision, and both are documented. A transport 413 remains unrecordable in the audit stream -- the request never reaches call_tool, the same pre-handler class as an auth refusal (#32) -- and DESIGN.md now says so, with the operator diagnosis path. The derived boundary is also observable pre-auth: probing body sizes recovers the cap, and with it max_attachment_bytes, once the derivation exceeds the floor. Recorded as accepted -- below ~2.25 MiB (the default included) the cap is the constant floor and discloses nothing, what leaks is a memory-tuning number rather than bug data or a rule name, and network reach is the access control until #32 -- with the add_attachment non-disclosure note now pointing at the trade-off. Tests pin the formula at the base64 quantum (a cap of 3 MiB + 1 must derive 4 bytes more than floor division would), both clamp edges, both saturating steps at the u64 extremes, and end to end over HTTP: a 5 MiB body is admitted under a 6 MiB policy, refused past the derived cap, and refused under the default policy where the floor still stands -- with no upstream request in any refused case. Closes #52 --- README.md | 2 +- crates/bugwarden/src/main.rs | 8 +- crates/bugwarden/src/server.rs | 231 +++++++++++++++--- .../tests/http_transport_wiremock.rs | 110 ++++++++- docs/DESIGN.md | 81 +++++- 5 files changed, 383 insertions(+), 49 deletions(-) diff --git a/README.md b/README.md index 72ee963..edb479f 100644 --- a/README.md +++ b/README.md @@ -395,7 +395,7 @@ A complete, commented example ships in | `allow_private_comments` | boolean | `false` | Master switch for **all** private content: comments, attachment metadata, and attachment downloads. Even when `true`, each call must also pass `include_private = true`. On an attachment download a *missing* privacy flag counts as private | | `read_only` | boolean | `false` | Strip write capabilities from every grant and remove write tools from the tool listing. The `--read-only` flag ORs into this | | `disabled_tools` | array of strings | `[]` | Tool names to remove from the tool listing entirely | -| `max_attachment_bytes` | integer | `2097152` (2 MiB) | Largest attachment `download_attachment` may return, and the same ceiling on what `add_attachment` may upload — both measured on the decoded size. `0` removes this cap; over http the transport still refuses a request body above 4 MiB, so an upload stays bounded either way. Downloaded content is embedded base64 in the tool result and lands in the model's context — raise deliberately | +| `max_attachment_bytes` | integer | `2097152` (2 MiB) | Largest attachment `download_attachment` may return, and the same ceiling on what `add_attachment` may upload — both measured on the decoded size. `0` removes this cap. Over http the transport's POST body limit is sized from this value — base64 expansion plus 1 MiB of headroom for the rest of the call, clamped to [4 MiB, 64 MiB] — so a canonical `add_attachment` at this cap fits through it (Bugzilla's own 65535-character comment limit keeps the other arguments well inside the headroom). The clamps mean two values are not honored over http: with `0` the transport keeps its 4 MiB bound, and a cap above ~47 MiB decoded is limited by the 64 MiB ceiling — "no policy cap" and "an enormous policy cap" must not become an unbounded request body. Downloaded content is embedded base64 in the tool result and lands in the model's context — raise deliberately | | `identity_source` | `"whoami"` \| `"declared"` | `"whoami"` | How `created_by_me` resolves the caller's login. `whoami` calls Bugzilla's `GET /rest/whoami` — a fork/BMO extension absent from stock Bugzilla Core v1. `declared` names an operator-configured login instead (see `identity_login`), verified once at startup against the *stock* `GET /rest/valid_login` endpoint and never looked up again per call — the portable path when the deployment has no identity endpoint at all. See the `created_by_me` row below and "Identity resolution" in `docs/DESIGN.md` | | `identity_login` | string | none | Required (and must be non-blank) exactly when `identity_source = "declared"`; a hard startup error if set under `identity_source = "whoami"` (it would otherwise be silently ignored). Names the account that owns *this server's* API key, so it is only meaningful under a server-held key (stdio, or http server-held mode) — a startup error under http per-request key custody, where there is no server-held key for it to describe. Bugzilla compares logins case-sensitively (Perl `eq`); declare it exactly as Bugzilla stores it | | `allow_discovery` | boolean | `false` | Exposes `bugzilla_products` and `bug_fields`, two read-only tools that return this Bugzilla instance's product and bug-field metadata **exactly as Bugzilla returns it to this server's key, never filtered by this guard policy** — filtering the catalog would itself be a way to probe the policy's rules. Leave this off (the default) if product or field names are themselves confidential; `disabled_tools` still works independently once discovery is on. Older bugwarden versions reject a policy using this key at startup (strict parsing fails closed) | diff --git a/crates/bugwarden/src/main.rs b/crates/bugwarden/src/main.rs index 31d5392..9b8077a 100644 --- a/crates/bugwarden/src/main.rs +++ b/crates/bugwarden/src/main.rs @@ -123,10 +123,16 @@ async fn main() -> anyhow::Result<()> { Transport::Http => { let ct = tokio_util::sync::CancellationToken::new(); + // Derived from the server's own guard policy (the POST body cap + // follows `global.max_attachment_bytes`, issue #52), so it is + // built while `server` can still be borrowed. + let config = server + .http_server_config() + .with_cancellation_token(ct.child_token()); let service = StreamableHttpService::new( move || Ok(server.clone()), LocalSessionManager::default().into(), - bugwarden::server::http_server_config().with_cancellation_token(ct.child_token()), + config, ); let router = axum::Router::new().nest_service("/mcp", service); let addr = format!("{}:{}", cfg.host, cfg.port); diff --git a/crates/bugwarden/src/server.rs b/crates/bugwarden/src/server.rs index c29207a..660f7eb 100644 --- a/crates/bugwarden/src/server.rs +++ b/crates/bugwarden/src/server.rs @@ -346,38 +346,64 @@ fn client_of(ctx: &RequestContext) -> audit::ClientInfo { } } -/// The streamable-HTTP transport configuration this build serves with. +/// The POST body cap this build pinned before it was derived from policy, +/// and the floor it never goes below. +const MAX_REQUEST_BODY_FLOOR: usize = 4 * 1024 * 1024; + +/// The largest POST body this build will buffer, whatever the policy says. +/// +/// The transport collects a body before anything inspects it, so the cap is +/// a memory bound first and an attachment allowance second. 64 MiB carries +/// every decoded `max_attachment_bytes` up to ~47 MiB, far above any +/// plausible Bugzilla attachment limit, while keeping the bound finite for +/// the values that are not a considered number at all — an "unlimited" +/// spelled as a huge integer, or a typo with too many digits. +const MAX_REQUEST_BODY_CEILING: usize = 64 * 1024 * 1024; + +/// The transport's POST body cap for a policy whose decoded attachment +/// ceiling is `max_attachment_bytes` (`0` = no policy cap). +/// +/// Sized so that every upload the guard would allow fits through the +/// transport: `add_attachment` takes its payload base64-encoded, which +/// expands the decoded bytes by 4/3 (`ceil(n / 3) * 4`, padding included), +/// and 1 MiB of headroom covers the JSON-RPC envelope plus the call's other +/// arguments — `file_name`, `summary`, `content_type`, `comment`. The +/// expansion assumes canonical unwrapped base64, which is what MCP clients +/// send; an encoder that wraps its output at a line length spends another +/// ~1.4% on separators, eroding that headroom for decoded caps above +/// roughly 30 MiB. /// -/// Lives here rather than in `main` so the integration tests serve the -/// configuration a deployment actually gets. These rmcp 3.1 defaults are set -/// by name rather than inherited, because inheriting them changes how a -/// deployment behaves without anyone choosing it: +/// Clamped at both ends, and the clamps are the point: /// -/// * `allowed_hosts` defaults to loopback only — a DNS-rebinding defence -/// for MCP servers a browser can reach on `localhost`. bugwarden is -/// reached by MCP clients at whatever address the operator bound and -/// named, so that default would refuse every deployment not addressed as -/// `localhost`, containers included. Disabled deliberately: the access -/// control here is the network boundary, and per-caller authentication -/// when it lands (issue #32). -/// * `max_request_body_bytes` is a 4 MiB POST cap with no rmcp 2.2 -/// equivalent. Worth keeping as a memory bound, but it also ceilings -/// `add_attachment` independently of the operator's -/// `global.max_attachment_bytes`, so it is pinned to the SDK's current -/// value: an SDK bump must not move an operator-visible limit. Issue #52 -/// reconciles the two ceilings. +/// * never below `MAX_REQUEST_BODY_FLOOR` — the value this build served +/// before the cap was derived, so non-attachment traffic and the +/// transport's memory bound are unchanged by the derivation, and a policy +/// that lowers `max_attachment_bytes` cannot shrink the body cap under +/// ordinary requests. `0` returns the floor too: it means "the guard +/// imposes no attachment ceiling", not "the transport imposes no memory +/// bound"; +/// * never above `MAX_REQUEST_BODY_CEILING` — over HTTP an unbounded body +/// is an unbounded-memory lever for anyone who can reach the port, and no +/// policy value may hand that out, whether it is `0` or a number so large +/// the derivation would saturate. The honest consequence: a policy cap +/// above ~47 MiB decoded is NOT honored over HTTP, exactly as `0` is not +/// honored as "unlimited" there. Both are deliberate, and recorded in +/// DESIGN.md. /// -/// `allowed_origins` is the browser-facing sibling of `allowed_hosts` and the -/// same reasoning covers it, but it is left inherited: its empty default IS -/// the disabled state, so naming it would assert nothing. Anything that -/// changes the `allowed_hosts` call above should decide this one too rather -/// than leave it behind. The remaining fields, and why each stays inherited, -/// are inventoried in DESIGN.md under "rmcp 3.1 usage notes"; the caller in -/// `main` adds `cancellation_token` so shutdown reaches the live transport. -pub fn http_server_config() -> StreamableHttpServerConfig { - StreamableHttpServerConfig::default() - .disable_allowed_hosts() - .with_max_request_body_bytes(4 * 1024 * 1024) +/// Saturating throughout: `max_attachment_bytes` is operator input and may +/// be `u64::MAX`, where the derivation must clamp rather than panic in +/// debug or wrap in release. +fn max_request_body_bytes(max_attachment_bytes: u64) -> usize { + if max_attachment_bytes == 0 { + return MAX_REQUEST_BODY_FLOOR; + } + let needed = max_attachment_bytes + .div_ceil(3) + .saturating_mul(4) + .saturating_add(1024 * 1024); + usize::try_from(needed) + .unwrap_or(usize::MAX) + .clamp(MAX_REQUEST_BODY_FLOOR, MAX_REQUEST_BODY_CEILING) } /// Whether this request took rmcp's handshake-free lifecycle. @@ -1260,6 +1286,68 @@ impl BugWarden { self } + /// The streamable-HTTP transport configuration this server is served + /// with. + /// + /// A method on the server rather than a free function taking the cap, + /// so the transport is sized from the very policy this instance + /// enforces: the deployment and its integration tests cannot end up + /// deriving the limit from a different value than the guard uses, and + /// there is one place where the policy field is read. Call it before + /// the server moves into the service closure. + /// + /// These rmcp 3.1 defaults are set by name rather than inherited, + /// because inheriting them changes how a deployment behaves without + /// anyone choosing it: + /// + /// * `allowed_hosts` defaults to loopback only — a DNS-rebinding + /// defence for MCP servers a browser can reach on `localhost`. + /// bugwarden is reached by MCP clients at whatever address the + /// operator bound and named, so that default would refuse every + /// deployment not addressed as `localhost`, containers included. + /// Disabled deliberately: the access control here is the network + /// boundary, and per-caller authentication when it lands (issue #32). + /// * `max_request_body_bytes` is a POST cap with no rmcp 2.2 + /// equivalent, worth keeping as a memory bound — but it also ceilings + /// `add_attachment`, so a fixed value silently overrides the + /// operator's `global.max_attachment_bytes`: at the SDK's 4 MiB, + /// base64 expansion alone put every decoded cap above ~3 MiB out of + /// reach, and the upload the operator had permitted was refused by + /// the transport (issue #52). It is therefore derived from this + /// server's policy by `max_request_body_bytes` — `ceil(cap / 3) * 4` + /// for the encoding plus 1 MiB of framing headroom, clamped to + /// `MAX_REQUEST_BODY_FLOOR` (4 MiB, which `0` also returns) and + /// `MAX_REQUEST_BODY_CEILING` (64 MiB, so no policy value can ask + /// this transport to buffer without bound). Derived, not inherited: + /// an SDK bump still must not move an operator-visible limit. + /// + /// A body over that cap is refused by rmcp's tower layer with a bare + /// `413`, which reaches neither `call_tool` nor the guard — so it + /// leaves NO audit record and names neither the tool nor the caller, + /// the same unrecordability as a pre-handler auth refusal (issue #32). + /// An operator diagnosing a 413 therefore has to compare the request + /// body's size with the cap derived here from + /// `global.max_attachment_bytes`, since nothing on the server side will + /// have recorded the attempt. That the boundary is observable to an + /// unauthenticated client, and what it discloses, is recorded in + /// DESIGN.md under "rmcp 3.1 usage notes". + /// + /// `allowed_origins` is the browser-facing sibling of `allowed_hosts` + /// and the same reasoning covers it, but it is left inherited: its + /// empty default IS the disabled state, so naming it would assert + /// nothing. Anything that changes the `allowed_hosts` call below should + /// decide this one too rather than leave it behind. The remaining + /// fields, and why each stays inherited, are inventoried in DESIGN.md + /// under "rmcp 3.1 usage notes"; the caller in `main` adds + /// `cancellation_token` so shutdown reaches the live transport. + pub fn http_server_config(&self) -> StreamableHttpServerConfig { + StreamableHttpServerConfig::default() + .disable_allowed_hosts() + .with_max_request_body_bytes(max_request_body_bytes( + self.guard.policy.global.max_attachment_bytes, + )) + } + /// Turn a silent identity blackout into a loud startup failure. /// /// `Guard::resolve_caller` maps every `whoami` failure to `None`, and @@ -4817,4 +4905,89 @@ mod tests { "the wire identity must match the handshake's {identity:?}: {agent}" ); } + + #[test] + fn no_policy_cap_still_bounds_the_request_body() { + // `0` means the guard imposes no attachment ceiling. It must NOT + // mean an unbounded POST body: the transport buffers what it + // accepts, so an unbounded body is an unbounded-memory lever for + // anyone who can reach the port. + assert_eq!(max_request_body_bytes(0), 4 * 1024 * 1024); + } + + #[test] + fn the_default_attachment_cap_leaves_the_body_cap_at_the_floor() { + // 2 MiB decoded expands to ~2.67 MiB encoded plus 1 MiB framing — + // still under the floor, so the default policy serves exactly the + // 4 MiB this build served before the cap was derived. + let default_cap = Policy::default().global.max_attachment_bytes; + assert_eq!(default_cap, 2 * 1024 * 1024, "the default policy moved"); + assert_eq!(max_request_body_bytes(default_cap), MAX_REQUEST_BODY_FLOOR); + } + + #[test] + fn a_raised_attachment_cap_raises_the_body_cap() { + // The bug in #52: at 3 MiB decoded the base64 payload alone is + // 4 MiB, so the old pin refused an upload the policy permitted. + let cap = 3 * 1024 * 1024; + let derived = max_request_body_bytes(cap); + assert!( + derived > MAX_REQUEST_BODY_FLOOR, + "a cap the floor cannot carry must raise it: {derived}" + ); + assert_eq!(derived, 3 * 1024 * 1024 / 3 * 4 + 1024 * 1024); + } + + #[test] + fn the_encoded_size_rounds_up_to_the_base64_quantum() { + // A cap that is not a multiple of 3: base64 pads the trailing group + // out to four characters, so the encoded size rounds UP. Truncating + // division would size the transport one quantum short of the + // largest upload the policy permits — the #52 bug in miniature, and + // invisible to any test whose cap divides by 3. + let cap = 3 * 1024 * 1024 + 1; + assert_eq!( + max_request_body_bytes(cap), + (1024 * 1024 + 1) * 4 + 1024 * 1024 + ); + } + + #[test] + fn a_cap_past_the_ceiling_clamps_to_it() { + // 47.25 MiB decoded is the largest cap the ceiling can carry: + // ceil(n / 3) * 4 + 1 MiB lands exactly on 64 MiB. Just under it + // the derivation is still doing arithmetic, not clamping; just over + // it the memory bound takes precedence over the policy's wish. + const EXACTLY_THE_CEILING: u64 = 49_545_216; + assert_eq!( + max_request_body_bytes(EXACTLY_THE_CEILING), + MAX_REQUEST_BODY_CEILING + ); + assert_eq!( + max_request_body_bytes(EXACTLY_THE_CEILING - 3), + MAX_REQUEST_BODY_CEILING - 4, + "below the ceiling the cap must still follow the policy" + ); + assert_eq!( + max_request_body_bytes(EXACTLY_THE_CEILING + 3), + MAX_REQUEST_BODY_CEILING + ); + } + + #[test] + fn an_enormous_attachment_cap_clamps_instead_of_overflowing() { + // Operator input, so u64::MAX is reachable — an "unlimited" spelled + // as a huge number, or a typo. Neither may become an unbounded POST + // body: the transport buffers what it accepts before any guard + // runs, so the memory bound has to survive every input. The + // arithmetic saturates on the way (at three quarters of the u64 + // range the framing headroom overflows; at u64::MAX the base64 + // expansion does) rather than panicking in debug or wrapping to a + // tiny cap in release, and the clamp then lands on the ceiling. + assert_eq!( + max_request_body_bytes(u64::MAX / 4 * 3), + MAX_REQUEST_BODY_CEILING + ); + assert_eq!(max_request_body_bytes(u64::MAX), MAX_REQUEST_BODY_CEILING); + } } diff --git a/crates/bugwarden/tests/http_transport_wiremock.rs b/crates/bugwarden/tests/http_transport_wiremock.rs index 37cc83b..89c17df 100644 --- a/crates/bugwarden/tests/http_transport_wiremock.rs +++ b/crates/bugwarden/tests/http_transport_wiremock.rs @@ -21,7 +21,11 @@ //! instead of falling back to `context.meta` (the inverse mutant, //! reading only `context.meta`, is behavior-preserving over every //! serialized transport and is killed by the direct in-process call -//! test in server.rs instead). +//! test in server.rs instead); +//! - the POST body cap going back to a fixed value, which refuses uploads +//! the operator's `global.max_attachment_bytes` permits, or losing its +//! 4 MiB floor, which would let a policy shrink the transport's memory +//! bound (or remove it entirely at `0`). use std::io::Write as _; use std::net::SocketAddr; @@ -84,13 +88,15 @@ async fn serve_http( server = server.with_audit(audit); } + // The deployed configuration, not a default one, and derived the way + // main derives it: the server reads its OWN policy for the POST body + // cap, so this harness cannot agree with a deployment that reads a + // different field or a constant. + let config = server.http_server_config(); let service = StreamableHttpService::new( move || Ok(server.clone()), LocalSessionManager::default().into(), - // The deployed configuration, not a default one: the transport - // knobs bugwarden sets by name are only tested if the harness - // serves what a deployment serves. - bugwarden::server::http_server_config(), + config, ); let router = axum::Router::new().nest_service("/mcp", service); let listener = tokio::net::TcpListener::bind("127.0.0.1:0") @@ -640,3 +646,97 @@ async fn traceparent_over_http_lands_in_the_audit_record() { assert_eq!(trace.trace_id, "0af7651916cd43dd8448eb211c80319c"); assert_eq!(trace.span_id, "b7ad6b7169203331"); } + +/// POST a raw JSON-RPC `initialize` whose serialized body is at least +/// `bytes` long, and answer with the HTTP status the transport gave it. +/// +/// The size rides on `clientInfo.title`, a plain string field: what is +/// under test is the transport's body cap, which is applied while the body +/// is still being collected — before any tool, session or guard exists — +/// so the cheapest well-formed request that the cap can refuse is the +/// handshake itself. A body the cap admits is answered `200`; one it +/// refuses is answered `413` with no JSON-RPC message at all. +async fn initialize_body_of(addr: SocketAddr, bytes: usize) -> reqwest::StatusCode { + let request = |title: String| { + json!({ + "jsonrpc": "2.0", + "id": 1, + "method": "initialize", + "params": { + "protocolVersion": "2025-11-25", + "capabilities": {}, + "clientInfo": { "name": "cap-test", "version": "1", "title": title } + } + }) + }; + let envelope = serde_json::to_vec(&request(String::new())) + .expect("serialize") + .len(); + let body = serde_json::to_vec(&request("A".repeat(bytes.saturating_sub(envelope)))) + .expect("serialize"); + assert!( + body.len() >= bytes, + "the padding must reach the target size" + ); + + reqwest::Client::new() + .post(format!("http://{addr}/mcp")) + .header("Accept", "application/json, text/event-stream") + .header("Content-Type", "application/json") + .body(body) + .send() + .await + .expect("the request must reach the server") + .status() +} + +#[tokio::test] +async fn the_body_cap_follows_the_policy_attachment_ceiling() { + // Issue #52: two ceilings governed one thing. The transport's POST cap + // was pinned at 4 MiB, so base64 expansion (4/3) plus JSON-RPC framing + // put every `max_attachment_bytes` above ~3 MiB out of reach: the + // operator raised the limit, and the transport refused the upload + // anyway — with a bare 413 that reaches no tool and therefore leaves no + // audit record. The cap is now derived from the policy, so what the + // guard permits is what the transport admits. + let mock = MockServer::start().await; + let file = key_file("srv-key\n"); + + // 6 MiB decoded => ceil(6 MiB / 3) * 4 + 1 MiB framing = 9 MiB of body. + let permissive = serve_http( + http_cli(&mock, Some(file.path())), + "[global]\nmax_attachment_bytes = 6291456\n", + &mock, + None, + ) + .await; + let admitted = initialize_body_of(permissive, 5 * 1024 * 1024).await; + assert_eq!( + admitted, 200, + "a body the policy's own attachment cap permits must not be refused \ + by the transport" + ); + assert_eq!( + initialize_body_of(permissive, 10 * 1024 * 1024).await, + 413, + "past the derived cap the memory bound still holds" + ); + + // Nothing was loosened for everyone else: under the default policy the + // 4 MiB floor stands, and the very same 5 MiB body is refused. + let floored = serve_http(http_cli(&mock, Some(file.path())), "", &mock, None).await; + assert_eq!( + initialize_body_of(floored, 5 * 1024 * 1024).await, + 413, + "a policy that permits no such attachment keeps the 4 MiB floor" + ); + + // A refused body reaches neither tool nor guard, so it also reaches no + // upstream: 413 is a transport verdict, invisible to the audit stream. + let upstream = mock.received_requests().await.unwrap_or_default(); + assert!( + upstream.is_empty(), + "no handshake may contact Bugzilla: {} request(s)", + upstream.len() + ); +} diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 7ee6e6a..1cba7ef 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -865,7 +865,7 @@ constraints the model must know. | 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) | -| 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). `comment` travels as a PLAIN string — Bug.add_attachment documents it so; the `{"comment": {"body": ..}}` shape belongs to Bug.update only | +| 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": ""` | | assign_bug | bug_id, assignee (email), comment = "" | assign (write) | payload `{"assigned_to": ..}` | @@ -1158,8 +1158,8 @@ wired, `server.rs` and `main.rs` are the reference. pruned per deployment (I13), so a shared cache must never serve one deployment's list to another — and `CacheScope::default()` is `Public`. - **Every `StreamableHttpServerConfig` field is accounted for below** — set by - name or inherited for a stated reason. `http_server_config()` (server.rs) - names two; main.rs adds a third at the call site. The struct is + name or inherited for a stated reason. `BugWarden::http_server_config()` + (server.rs) names two; main.rs adds a third at the call site. The struct is `#[non_exhaustive]`, so an SDK bump may grow it: a field this table does not list is an unreviewed default, and adding the row is part of the bump, not a follow-up. Counting them in prose is what let two fields go unlisted here @@ -1168,7 +1168,7 @@ wired, `server.rs` and `main.rs` are the reference. | field | rmcp 3.1 default | this build | |---|---|---| | `allowed_hosts` | `localhost`, `127.0.0.1`, `::1` | **set** — `disable_allowed_hosts()` | - | `max_request_body_bytes` | 4 MiB | **set** — pinned to that same 4 MiB | + | `max_request_body_bytes` | 4 MiB | **set** — derived from `global.max_attachment_bytes`, floored at that same 4 MiB (see below) | | `cancellation_token` | fresh token | **set** (main.rs) — a child of the process token | | `allowed_origins` | `[]`, i.e. validation off | inherited, deliberately | | `stateless_protocol_metadata_required` | `false` | inherited; #34 decides it | @@ -1181,13 +1181,58 @@ wired, `server.rs` and `main.rs` are the reference. reach on localhost, and its default would refuse every deployment not addressed as `localhost`, containers included; bugwarden disables it deliberately, since its access control is the network boundary and, when it - lands, per-caller authentication (#32). `max_request_body_bytes` is a 4 MiB - POST cap with no rmcp 2.2 equivalent: kept as a memory bound but pinned to - the current SDK value, because it also ceilings `add_attachment` - independently of `global.max_attachment_bytes` and an SDK bump must not move - an operator-visible limit. Reconciling the two ceilings is #52. The - `cancellation_token` is named so ctrl_c tears the live transport down with - the process instead of leaving it to outlive the shutdown. + lands, per-caller authentication (#32). `max_request_body_bytes` is a POST + cap with no rmcp 2.2 equivalent: kept as a memory bound, but it also + ceilings `add_attachment`, so a fixed value silently overrides the + operator's `global.max_attachment_bytes` — at the SDK's 4 MiB, base64 + expansion alone put every decoded cap above ~3 MiB out of reach (#52). It is + therefore **derived** from the policy, in `max_request_body_bytes` + (server.rs), which `BugWarden::http_server_config` calls with its own + guard's value — one place reads the policy field, so a deployment and its + tests cannot size the transport from different numbers. + `ceil(max_attachment_bytes / 3) * 4` for the base64 expansion, plus 1 MiB + of headroom for the JSON-RPC framing and the call's other arguments, + **clamped to [4 MiB, 64 MiB]** and saturating at every step (the policy + value is operator input up to `u64::MAX`, and this runs in the startup + path). Derived rather than inherited for the original reason: an SDK bump + still must not move an operator-visible limit. Both clamps exist because + the transport buffers a body before anything inspects it, so this is a + memory bound first and an attachment allowance second. The 4 MiB floor is + what this build served before the derivation, so ordinary traffic is + unchanged and a small policy cap cannot shrink it; `0` — "no policy cap" — + returns that floor rather than an unbounded body, since an unbounded body + is an unbounded-memory lever for anyone who can reach the port. The 64 MiB + ceiling says the same thing about a huge finite value, which is the same + operator intent spelled differently (an "unlimited", or a typo): it carries + every decoded cap up to ~47 MiB, far above any plausible Bugzilla + attachment limit, and refuses to let a policy number remove the bound. + Honest consequence, the mirror of the `0` case: a policy cap above ~47 MiB + decoded is not honored over HTTP. A body over the cap is refused by rmcp's + tower layer with a bare `413` that reaches neither `call_tool` nor the + guard, so it is **unrecordable** in the audit stream — the same class as a + pre-handler auth refusal (#32), and accepted on the same terms; an operator + diagnosing a 413 compares the body size against the derived cap, because + nothing server-side recorded the attempt. + + That boundary is also observable to an unauthenticated client, which is + **ACCEPTED**: whenever the derivation exceeds the floor (a decoded cap above + ~2.25 MiB) the 413 threshold is a function of `max_attachment_bytes`, so + binary-searching body sizes recovers it — a value the add_attachment row + above and the download refusal deliberately do not disclose, neither the + size nor the cap. It is accepted for three reasons. Below ~2.25 MiB — + including the 2 MiB default and `0` — the cap is the constant 4 MiB floor + and discloses nothing about the policy at all. What leaks above it is a + memory-tuning number, not bug data: no rule name, no match criterion, no + bug's existence or content, so I1's "the policy file is never readable + through MCP" and I2/I3 are untouched — this is the one policy-derived + number a transport-level limit inherently exposes, in exchange for the + operator's configured limit actually working. And the probing itself needs + network reach, which is the access control until per-caller authentication + lands (#32); a caller who can binary-search POST sizes can already call + tools. If #32 changes that calculus, revisit here and at the add_attachment + row together. The `cancellation_token` is named so ctrl_c tears the live + transport down with the process instead of leaving it to outlive the + shutdown. `allowed_origins` is the browser-facing sibling of `allowed_hosts`, and the #32 argument covers it identically. It is inherited rather than named because @@ -1235,7 +1280,7 @@ wired, `server.rs` and `main.rs` are the reference. update_bug_fields, update_bug_dependencies, add_cc_to_bug, mark_as_duplicate, create_bug, add_attachment. - API key resolution: a match on `key_custody` (resolved once at startup, see Key custody — never re-read per request): `Server(key)` => the server's key, without touching the request at all; `PerRequest` => `ctx.extensions.get::()`, then `parts.headers.get(lowercased_header_name)`. -- HTTP serving: `StreamableHttpService::new(move || Ok(server.clone()), LocalSessionManager::default().into(), http_server_config().with_cancellation_token(ct.child_token()))` — never a bare `StreamableHttpServerConfig::default()`, see the field table above — then `axum::Router::new().nest_service("/mcp", service)`, `tokio::net::TcpListener::bind`, graceful shutdown on ctrl_c cancelling `ct`. +- HTTP serving: `let config = server.http_server_config().with_cancellation_token(ct.child_token());` — built while `server` can still be borrowed, since the body cap comes from its own guard policy — then `StreamableHttpService::new(move || Ok(server.clone()), LocalSessionManager::default().into(), config)`, never a bare `StreamableHttpServerConfig::default()`, see the field table above — then `axum::Router::new().nest_service("/mcp", service)`, `tokio::net::TcpListener::bind`, graceful shutdown on ctrl_c cancelling `ct`. - Request `_meta` (SEP-414, e.g. `traceparent`): over every serialized transport the wire `params._meta` does NOT arrive in the params struct (`CallToolRequestParams.meta` stays `None`) — the SDK's custom @@ -1471,7 +1516,17 @@ wired, `server.rs` and `main.rs` are the reference. http lands its ids in the audit record with the response unchanged — the end-to-end proof that the wire `_meta` is read from where the SDK delivers it (`RequestContext.meta`), not from the params-struct field - that stays empty over serialized transports. + that stays empty over serialized transports; and the derived POST body + cap (#52) — a ~5 MiB body is ADMITTED by a server whose policy sets + `max_attachment_bytes = 6 MiB`, refused (413) once past that policy's + derived cap, and the very same body is refused under the default policy, + where the 4 MiB floor stands. The unit derivation itself is pinned in + server.rs: `0` and the 2 MiB default both give the 4 MiB floor; 3 MiB + gives 5 MiB (expansion plus the 1 MiB headroom); a cap ≡ 1 (mod 3) rounds + the encoded size UP, which a truncating division would not; 47.25 MiB + decoded lands exactly on the 64 MiB ceiling, one quantum below it still + derives and one above clamps; and `u64::MAX` clamps to the ceiling rather + than panicking, wrapping, or saturating into an unbounded body. - Audit tests (crates/bugwarden/tests/audit_wiremock.rs + #[cfg(test)] in server.rs and audit.rs): one record per call for EVERY routed tool, refusal paths and protocol errors included; the refusal map is total