fix(core): stop reading a capped search page as the end of results

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<u64> = BTreeSet::new();
         let mut scanned: u32 = 0;
         let mut dropped: Vec<u64> = 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
             }
         }

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<u64> = hidden.iter().copied().collect();
     let all: Vec<serde_json::Value> = (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::<usize>().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::<Vec<u64>>());
+}
+
+#[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::<Vec<u64>>());
+}
+
 #[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;
 

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"))

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<Value>) {
         .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;
 }