diff --git a/crates/bugwarden-core/src/guard.rs b/crates/bugwarden-core/src/guard.rs index 62522db..d47cd05 100644 --- a/crates/bugwarden-core/src/guard.rs +++ b/crates/bugwarden-core/src/guard.rs @@ -236,10 +236,18 @@ impl Guard { /// Rows requested from Bugzilla per scan step. const SEARCH_SCAN_CHUNK: u32 = 200; - /// Ceiling on upstream rows examined for one search, i.e. at most - /// `SEARCH_SCAN_MAX / SEARCH_SCAN_CHUNK` sequential requests. + /// Ceiling on upstream rows examined for one search. const SEARCH_SCAN_MAX: u32 = 2_000; + /// Ceiling on sequential upstream requests for one search. + /// + /// Used to imply this as `SEARCH_SCAN_MAX / SEARCH_SCAN_CHUNK`, but that + /// assumed every request returns a full chunk. Bugzilla may cap a page + /// below the requested `limit` (an admin-configured `max_search_results`, + /// for instance), so rows-per-request is the server's choice, not ours, + /// and the request count needs its own explicit bound. + const SEARCH_SCAN_REQUESTS: u32 = 10; + /// Run a search whose `limit`/`offset` address the bugs the client may /// actually SEE, not the rows Bugzilla happens to return. /// @@ -266,10 +274,17 @@ impl Guard { /// The bugs returned are the objects that were classified, so no second /// fetch can serve something the verdict never saw. /// - /// Bounds: at most [`Guard::MAX_SEARCH_WINDOW`] addressable, and at most - /// `SEARCH_SCAN_MAX` upstream rows examined. Hitting either truncates the - /// page, which is indistinguishable from running out of results — the - /// safe direction, since it hides more rather than less. + /// Bounds: at most [`Guard::MAX_SEARCH_WINDOW`] addressable, at most + /// `SEARCH_SCAN_MAX` upstream rows examined, and at most + /// `SEARCH_SCAN_REQUESTS` sequential requests issued. Whichever binds + /// first truncates the page, which is indistinguishable from running out + /// of results — the safe direction, since it hides more rather than + /// less. The scan does NOT stop on a short page: Bugzilla may cap a page + /// below the requested chunk size (an admin-configured + /// `max_search_results`, for instance), and a short page caused by that + /// cap looks identical to one caused by the result set ending. Only an + /// empty page, or one of the two scan bounds above (`SEARCH_SCAN_MAX`, + /// `SEARCH_SCAN_REQUESTS`), ends the scan. /// /// Residual, and deliberately accepted: filling a window of VISIBLE bugs /// takes more upstream rows when bugs are hidden, so the request count @@ -323,8 +338,12 @@ impl Guard { let mut seen: BTreeSet = BTreeSet::new(); let mut scanned: u32 = 0; let mut dropped: Vec = Vec::new(); + let mut requests: u32 = 0; - while (visible.len() as u32) < target && scanned < Self::SEARCH_SCAN_MAX { + while (visible.len() as u32) < target + && scanned < Self::SEARCH_SCAN_MAX + && requests < Self::SEARCH_SCAN_REQUESTS + { let chunk = Self::SEARCH_SCAN_CHUNK.min(Self::SEARCH_SCAN_MAX - scanned); let envelope = bz .quicksearch(key, query, status, include_fields, chunk, scanned) @@ -351,8 +370,9 @@ impl Guard { dropped.extend(chunk_dropped); visible.extend(kept); scanned += returned; + requests += 1; - if returned < chunk { + if returned == 0 { break; // upstream has no more rows } } diff --git a/crates/bugwarden-core/tests/guard_wiremock.rs b/crates/bugwarden-core/tests/guard_wiremock.rs index 7f21c5c..89de3e3 100644 --- a/crates/bugwarden-core/tests/guard_wiremock.rs +++ b/crates/bugwarden-core/tests/guard_wiremock.rs @@ -556,6 +556,13 @@ const EMBARGO_POLICY: &str = concat!( /// Serve `total` bugs, every id in `hidden` carrying an embargo group, and /// answer any offset/limit the way Bugzilla would. async fn corpus(server: &MockServer, total: u64, hidden: &[u64]) { + corpus_capped(server, total, hidden, usize::MAX).await; +} + +/// Like [`corpus`], but clamps every response to at most `cap` rows the way +/// an administrator's `max_search_results` does, regardless of the `limit` +/// requested. +async fn corpus_capped(server: &MockServer, total: u64, hidden: &[u64], cap: usize) { let hidden: std::collections::BTreeSet = hidden.iter().copied().collect(); let all: Vec = (1..=total) .map(|id| { @@ -573,7 +580,7 @@ async fn corpus(server: &MockServer, total: u64, hidden: &[u64]) { let q: std::collections::HashMap<_, _> = req.url.query_pairs().collect(); let get = |k: &str| q.get(k).and_then(|v| v.parse::().ok()); let offset = get("offset").unwrap_or(0); - let limit = get("limit").unwrap_or(all.len()); + let limit = get("limit").unwrap_or(all.len()).min(cap); let page: Vec<_> = all.iter().skip(offset).take(limit).cloned().collect(); ResponseTemplate::new(200).set_body_json(json!({ "bugs": page })) }) @@ -766,6 +773,46 @@ async fn quicksearch_window_scan_is_bounded() { ); } +#[tokio::test] +async fn quicksearch_window_survives_a_capped_page() { + // A server capping pages at 100 rows used to look identical to one + // running out of results after the first request: `returned < chunk` + // fired either way. A short page must not end the scan while the + // request/row bounds still have room. + let server = MockServer::start().await; + corpus_capped(&server, 300, &[], 100).await; + let g = guard(EMBARGO_POLICY); + + let got = g + .quicksearch_window(&client(&server), KEY, &search("q", 5, 150), None) + .await + .expect("search succeeds") + .bugs; + assert_eq!(ids_of(&got), (151..=155).collect::>()); +} + +#[tokio::test] +async fn quicksearch_window_capped_page_still_bounds_requests() { + // The pathological case: a 1-row page cap. The request bound must hold + // even though the row bound (2000) is nowhere near reached, and the + // result must look like ordinary truncation, not an error. + let server = MockServer::start().await; + corpus_capped(&server, 5_000, &[], 1).await; + let g = guard(EMBARGO_POLICY); + + let got = g + .quicksearch_window(&client(&server), KEY, &search("q", 50, 0), None) + .await + .expect("search succeeds") + .bugs; + assert_eq!( + requests_to(&server).await, + 10, + "the request bound must hold when the row bound cannot" + ); + assert_eq!(ids_of(&got), (1..=10).collect::>()); +} + #[tokio::test] async fn quicksearch_window_deep_offset_is_empty_whether_or_not_bugs_are_hidden() { // Past the addressable window the answer is an empty page — and it must @@ -945,15 +992,24 @@ async fn quicksearch_window_accounting_skips_idless_rows_and_repeats() { let server = MockServer::start().await; Mock::given(method("GET")) .and(path("/rest/bug")) - .respond_with(ResponseTemplate::new(200).set_body_json(json!({ - "bugs": [ - bug(1, &[], OLD), - bug(2, &["embargo-security"], OLD), - json!({ "summary": "no id here", "groups": [] }), - bug(1, &[], OLD), // repeated inside the chunk - bug(3, &[], OLD), - ] - }))) + .respond_with(|req: &Request| { + // A short page is no longer end-of-results, so only the first + // request may return rows — the second must be empty or the + // scan keeps going and `scanned` stops meaning "one chunk". + let q: std::collections::HashMap<_, _> = req.url.query_pairs().collect(); + if q.get("offset").is_some_and(|v| v != "0") { + return ResponseTemplate::new(200).set_body_json(json!({ "bugs": [] })); + } + ResponseTemplate::new(200).set_body_json(json!({ + "bugs": [ + bug(1, &[], OLD), + bug(2, &["embargo-security"], OLD), + json!({ "summary": "no id here", "groups": [] }), + bug(1, &[], OLD), // repeated inside the chunk + bug(3, &[], OLD), + ] + })) + }) .mount(&server) .await; diff --git a/crates/bugwarden/tests/audit_wiremock.rs b/crates/bugwarden/tests/audit_wiremock.rs index 585f4ea..5e270dc 100644 --- a/crates/bugwarden/tests/audit_wiremock.rs +++ b/crates/bugwarden/tests/audit_wiremock.rs @@ -190,10 +190,19 @@ async fn mount_fixture(mock: &MockServer) { .mount(mock) .await; // Catch-all for /rest/bug: classification and body fetches for id=7 - // and the quicksearch scan all serve bug 7. + // and the quicksearch scan all serve bug 7. The quicksearch scan must + // see an empty page past offset 0, or a short page no longer being + // end-of-results makes it replay this single row up to the request + // bound. Mock::given(method("GET")) .and(path("/rest/bug")) - .respond_with(ResponseTemplate::new(200).set_body_json(json!({ "bugs": [world_bug(7)] }))) + .respond_with(|req: &wiremock::Request| { + let q: std::collections::HashMap<_, _> = req.url.query_pairs().collect(); + if q.get("offset").is_some_and(|v| v != "0") { + return ResponseTemplate::new(200).set_body_json(json!({ "bugs": [] })); + } + ResponseTemplate::new(200).set_body_json(json!({ "bugs": [world_bug(7)] })) + }) .mount(mock) .await; Mock::given(method("GET")) diff --git a/crates/bugwarden/tests/tools_wiremock.rs b/crates/bugwarden/tests/tools_wiremock.rs index 4d1834f..d47343e 100644 --- a/crates/bugwarden/tests/tools_wiremock.rs +++ b/crates/bugwarden/tests/tools_wiremock.rs @@ -359,10 +359,17 @@ async fn create_scoped_rule_files_bugs_without_hiding_reads_issue_26() { Mock::given(method("GET")) .and(path("/rest/bug")) .and(query_param("quicksearch", "ALL product:Enterprise")) + .and(query_param("offset", "0")) .respond_with(ResponseTemplate::new(200).set_body_json(json!({ "bugs": [bug] }))) .expect(1) .mount(&mock) .await; + Mock::given(method("GET")) + .and(path("/rest/bug")) + .and(query_param("quicksearch", "ALL product:Enterprise")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ "bugs": [] }))) + .mount(&mock) + .await; Mock::given(method("POST")) .and(path("/rest/bug")) .respond_with(ResponseTemplate::new(200).set_body_json(json!({ "id": 4242 }))) @@ -514,7 +521,16 @@ async fn mount_search(mock: &MockServer, rows: Vec) { .await; Mock::given(method("GET")) .and(path("/rest/bug")) - .respond_with(ResponseTemplate::new(200).set_body_json(json!({ "bugs": rows }))) + .respond_with(move |req: &wiremock::Request| { + // A short page is no longer end-of-results, so past offset 0 the + // scan must see an empty page or it replays `rows` up to the + // request bound. + let q: std::collections::HashMap<_, _> = req.url.query_pairs().collect(); + if q.get("offset").is_some_and(|v| v != "0") { + return ResponseTemplate::new(200).set_body_json(json!({ "bugs": [] })); + } + ResponseTemplate::new(200).set_body_json(json!({ "bugs": rows.clone() })) + }) .mount(mock) .await; } diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 8858e17..0e159e0 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -868,7 +868,7 @@ constraints the model must know. | bug_info | bug_ids: Vec | per-id: Read => full, else Summary => redacted, else restricted entry | envelope `{"bugs":[..], "restricted":[{"id":N,"note":denial(N)}]}`; full fetch only for Read-granted ids. Every fetched body is RE-CLASSIFIED before it is served (assemble_bug_info): the verdict came from the classification fetch and the body from a later request, so a bug embargoed in between must not be served on the stale verdict (TOCTOU; same reason download_attachment re-checks). Costs no request — the body is a superset of CLASSIFY_FIELDS — and a body that now earns only summary is served as the summary view, one that earns nothing becomes the uniform restricted entry. A failed body fetch is logged server-side ONLY and leaves those ids restricted: Bugzilla's message names the bug and says whether it exists, so forwarding it would undo I2 | | 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 | +| 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). A short page is NOT read as end-of-results — Bugzilla is free to cap a page below the requested chunk size (an admin-configured `max_search_results`, for instance), and that cap looks identical to a short page at the genuine end of a result set; only an empty page ends the scan early. Bounds, independent of each other: MAX_SEARCH_WINDOW=1000 addressable, 2000 rows scanned, and 10 sequential requests; hitting any of the three 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, 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) | | @@ -1406,7 +1406,10 @@ wired, `server.rs` and `main.rs` are the reference. upstream order; an unstable one can drop a bug from every page, which hides more rather than less), a hidden bug never shortens a page, a deep offset is an empty page whether or not bugs were hidden, the scan bound is counted in - requests, the scan target does not track `limit`, id-less rows are dropped, + requests, the scan target does not track `limit`, a page capped below the + requested chunk size (Bugzilla's own `max_search_results`, for instance) is + not read as end-of-results and the scan keeps going to fill the window, a + 1-row page cap still bounds the scan at 10 requests, id-less rows are dropped, rows repeated across chunks are served once, exhaustion and scan truncation look alike, zero limit touches nothing (and accounts for nothing), the returned objects are the classified ones, and the scan accounting (issue #29) — `scanned` counts