fix(pnpr): truncate upstream search at the fetch budget instead of failing with 400 (#14874)

An upstream with search enabled made almost every npm search fail with
400 "refine the query": the guard refused whenever the upstream
reported a total above 2000, and npmjs's loose full-text search reports
five-digit totals for nearly any term (jquery 23k, even narrow
hyphenated names 30k+ because the tokenizer matches each word).

The page and result budgets now bound what pnpr actually downloads.
A source small enough to exhaust keeps the deduplicated total exact,
as before; a larger one is truncated after the budgeted pages and its
unscanned remainder folds into total as an over-approximation. A huge
from offset walks at most the same budget and returns an empty window
instead of a 400.

Related to pnpm/pnpm#11973.

---------

Co-authored-by: Zoltan Kochan <z@kochan.io>
This commit is contained in:
Juan PicadoandZoltan Kochan authored and GitHub committed 2026-09-14 02:19:05 +02:00
1 parent 55434d2463
commit 0b8a1cd07d
5 files changed
+390 -91

No files matched your search

@@ -0,0 +1,5 @@
---
"@pnpm/pnpr": patch
---
`pnpm search` and `npm search` against an upstream with `search: true` no longer fail with a 400 error on broad terms. pnpr now truncates an upstream that holds more results than its fetch budget downloads. The results it could not download keep `total` approximate.
+19 -5
View File
@@ -202,7 +202,9 @@ upstream. pnpr applies registry routing and access rules to returned entries and
uses only the upstream credentials from its configuration, never a browser
caller's authorization header. Discovery refuses redirects and sends configured
upstream headers only over HTTPS or loopback HTTP. Search totals count only
visible, deduplicated results and are exact across every participating source.
visible results: a source small enough to scan completely contributes an exact
deduplicated count, and one too large to scan adds its remaining advertised
results as an estimate, so the total can overstate what paging can reach.
Hosted npm packages and Cargo crates can be browsed without a search term:
```text
@@ -215,10 +217,22 @@ Browse results follow registry routing and access rules, exclude upstreams and
staged publications, and use the same response format and pagination limits as
search. An empty search without `browse=true` still returns no results.
To bound work from a single browser request, pnpr rejects upstream searches
that would scan more than 2,000 upstream results or eight upstream pages.
Offsets that would require a larger upstream scan are also rejected. Refine the
search term when a query reaches that limit.
To bound work from a single browser request, pnpr retains at most 2,000
upstream results per search and spends at most eight budgeted page fetches
across all sources. Separately from those budgets, every search-enabled
upstream whose access and package rules admit the caller gets one request
even after they are spent, sized to whatever result budget remains, down to
a single entry, so a source listed after a large one is never silently
dropped from the results or the total. The exception is a hard ceiling of 32
upstream requests for the whole search: a registry routing more eligible
upstreams than that never queries the ones past it, so they contribute
neither results nor totals. An upstream the caller may not reach is skipped and
never queried at all. A source that advertises more than the budget allows
(the public npm registry does for almost any term) is
truncated rather than rejected: the requested page is served from what was
downloaded, every downloaded result keeps its position, and the rest only
counts toward the estimated total. Pages beyond the downloaded results come
back empty.
## Cargo and Python registries
+86 -60
View File
@@ -20,11 +20,15 @@ pub(super) struct SearchPage<Item> {
pub(super) names: HashSet<String>,
pub(super) from: usize,
pub(super) size: usize,
/// A source's reported total minus what was walked, not deduplicated
/// against `names`. Counted at the tail of the served sequence, behind
/// every downloaded result of every source.
pub(super) unscanned: usize,
}
impl<Item> SearchPage<Item> {
pub(super) fn new(from: usize, size: usize) -> Self {
Self { objects: Vec::new(), names: HashSet::new(), from, size }
Self { objects: Vec::new(), names: HashSet::new(), from, size, unscanned: 0 }
}
pub(super) fn push_name(&mut self, name: &str) -> bool {
@@ -39,7 +43,7 @@ impl<Item> SearchPage<Item> {
}
pub(super) fn total(&self) -> usize {
self.names.len()
self.names.len().saturating_add(self.unscanned)
}
}
@@ -58,9 +62,20 @@ pub(super) const MAX_UPSTREAM_SEARCH_RESULTS: usize = 2_000;
pub(super) const MAX_UPSTREAM_SEARCH_PAGES: usize = 8;
/// The ceiling on upstream requests for one search, however many sources a
/// registry routes to. Continuation pages stop at
/// [`MAX_UPSTREAM_SEARCH_PAGES`]; the fetch each source is guaranteed may
/// carry the search past that, but never past this.
pub(super) const MAX_UPSTREAM_SEARCH_REQUESTS: usize = 32;
/// What one search may download from its upstreams. The budgets bound
/// pnpr's own fetching, never what an upstream advertises: npmjs's loose
/// full-text search reports five-digit totals for almost any term, so a
/// search that refused those would refuse almost every term.
#[derive(Default)]
pub(super) struct UpstreamSearchBudget {
pub(super) pages: usize,
pub(super) requests: usize,
pub(super) results: usize,
}
@@ -78,45 +93,32 @@ impl UpstreamSearchBudget {
MAX_UPSTREAM_SEARCH_RESULTS.saturating_sub(self.results)
}
/// Charge one upstream page against the budget.
pub(super) fn take_page(&mut self) -> Result<(), RegistryError> {
pub(super) fn try_take_page(&mut self) -> bool {
if self.pages == MAX_UPSTREAM_SEARCH_PAGES {
return Err(RegistryError::BadRequest {
reason: format!(
"upstream search is limited to {MAX_UPSTREAM_SEARCH_PAGES} pages; refine the query",
),
});
return false;
}
self.pages += 1;
Ok(())
true
}
/// Charge one page's results against the budget, refusing a source that
/// reports more than the whole search may scan.
pub(super) fn take_results(
&mut self,
reported_total: usize,
object_count: usize,
source_budget: usize,
) -> Result<(), RegistryError> {
if reported_total > source_budget || object_count > self.remaining_results() {
return Err(RegistryError::BadRequest {
reason: format!(
"upstream search is limited to {MAX_UPSTREAM_SEARCH_RESULTS} results; refine the query",
),
});
pub(super) fn try_take_request(&mut self) -> bool {
if self.requests == MAX_UPSTREAM_SEARCH_REQUESTS {
return false;
}
self.results += object_count;
Ok(())
self.requests += 1;
true
}
pub(super) fn add_results(&mut self, object_count: usize) {
self.results = self.results.saturating_add(object_count);
}
}
/// `GET /-/v1/search?text=...&from=...&size=...` — npm search v1 endpoint.
/// Hosted results are counted after routing and access filters, then optional
/// upstream results are appended in registry-source order. Every participating
/// upstream is exhausted so `total` describes the complete visible,
/// deduplicated result set. An upstream only participates when its `search`
/// setting is enabled.
/// upstream results are appended in registry-source order. An upstream only
/// participates when its `search` setting is enabled, and only within the
/// [`UpstreamSearchBudget`].
pub(super) async fn serve_search(
state: &AppState,
identity: &Identity,
@@ -146,13 +148,8 @@ pub(super) async fn serve_search(
.await
}
DiscoverySource::Upstream(source) => {
let search = UpstreamSearch {
registry: &registry,
source: &source,
query_string,
browse,
from: params.from,
};
let search =
UpstreamSearch { registry: &registry, source: &source, query_string, browse };
append_upstream_source(state, identity, search, &mut page, &mut upstream_budget)
.await
}
@@ -216,7 +213,6 @@ pub(super) struct UpstreamSearch<'a> {
pub(super) query_string: &'a str,
/// A browse request lists what pnpr itself holds, so no upstream is asked.
pub(super) browse: bool,
pub(super) from: usize,
}
/// Add one upstream source's matches to the page, if the caller may reach it
@@ -237,13 +233,6 @@ pub(super) async fn append_upstream_source(
let Some(upstream) = state.inner.proxy.upstreams.get(search.source) else {
return Ok(());
};
if search.from > page.total().saturating_add(budget.remaining_results()) {
return Err(RegistryError::BadRequest {
reason: format!(
"search `from` would require scanning more than {MAX_UPSTREAM_SEARCH_RESULTS} upstream results",
),
});
}
let context = UpstreamSearchContext {
state,
identity,
@@ -303,31 +292,68 @@ pub(super) async fn append_upstream_search(
const FETCH_SIZE: usize = 250;
let resolved = RegistrySource::Upstream(context.source.to_string());
let source_result_budget = budget.remaining_results();
// Unconditional against the page budget, though not the request cap: a
// source routed after a large one must still be asked, or it vanishes
// from `objects` and `total` at once.
budget.try_take_page();
let mut from = 0usize;
loop {
budget.take_page()?;
let query = upstream_search_query(context.query_string, from, FETCH_SIZE);
if !budget.try_take_request() {
return Ok(());
}
let size = budget.remaining_results().clamp(1, FETCH_SIZE);
let query = upstream_search_query(context.query_string, from, size);
let response = match context.upstream.fetch_search(&query).await? {
FetchOutcome::Ok(response) => response,
FetchOutcome::NotFound => return Ok(()),
};
let object_count = response.objects.len();
budget.take_results(response.total, object_count, source_result_budget)?;
append_visible_results(&context, &resolved, response.objects, page);
from = from.saturating_add(object_count);
if from >= response.total {
return Ok(());
}
if object_count == 0 {
return Err(RegistryError::UpstreamResponse {
url: format!("{}/-/v1/search", context.source),
reason: format!("reported {} results but returned an empty page", response.total),
});
match consume_upstream_page(&context, &resolved, response, page, budget, &mut from)? {
PageOutcome::Done => return Ok(()),
PageOutcome::More => {}
}
}
}
pub(super) enum PageOutcome {
Done,
More,
}
/// Advances `from` by what the result budget let this page keep.
pub(super) fn consume_upstream_page(
context: &UpstreamSearchContext<'_>,
resolved: &RegistrySource,
response: pnpr_upstream::SearchResponse,
page: &mut SearchPage<Value>,
budget: &mut UpstreamSearchBudget,
from: &mut usize,
) -> Result<PageOutcome, RegistryError> {
let fetched = response.objects.len();
let mut objects = response.objects;
objects.truncate(budget.remaining_results());
let consumed = objects.len();
budget.add_results(consumed);
append_visible_results(context, resolved, objects, page);
*from = from.saturating_add(consumed);
if *from >= response.total {
return Ok(PageOutcome::Done);
}
if fetched == 0 {
return Err(RegistryError::UpstreamResponse {
url: format!("{}/-/v1/search", context.source),
reason: format!("reported {} results but returned an empty page", response.total),
});
}
if budget.remaining_results() == 0 || !budget.try_take_page() {
// Raw, unfiltered by `search_result_is_visible`, yet no leak:
// `upstream_search_admits` withholds a source from any caller its
// access or package rules deny.
page.unscanned = page.unscanned.saturating_add(response.total.saturating_sub(*from));
return Ok(PageOutcome::Done);
}
Ok(PageOutcome::More)
}
/// Take the results of one upstream page that this caller may see.
pub(super) fn append_visible_results(
context: &UpstreamSearchContext<'_>,
@@ -1,7 +1,8 @@
use super::{
AccessList, AuthState, Body, Ecosystem, PackagePattern, PackageRules, Registries, Registry,
Request, ServiceExt, StatusCode, TempDir, Value, access_rule, body_json, config_for, header,
hosted_with_access, json, router, router_with_auth, seed_hosted, sha512_integrity, to_bytes,
hosted_with_access, json, router, router_config, router_with_auth, seed_hosted,
sha512_integrity, to_bytes,
};
#[tokio::test]
@@ -88,6 +89,105 @@ async fn search_paginates_across_hosted_and_upstream_sources() {
visible.assert_async().await;
}
#[tokio::test]
async fn search_truncates_a_huge_upstream_instead_of_refusing() {
let mut upstream = mockito::Server::new_async().await;
let pages = upstream
.mock("GET", "/-/v1/search")
.match_query(mockito::Matcher::UrlEncoded("text".into(), "jquery".into()))
.with_status(200)
.with_header("content-type", "application/json")
.with_body(
json!({
"objects": [
{ "package": { "name": "jquery" } },
{ "package": { "name": "jquery-ui" } },
{ "package": { "name": "jquery-form" } },
],
"total": 23_547,
})
.to_string(),
)
.expect(8)
.create_async()
.await;
let tmp = TempDir::new().unwrap();
let mut config = config_for(&upstream.url(), tmp.path().to_path_buf());
config.routing.upstreams.get_mut("npmjs").unwrap().search = true;
let app = router(config);
let response = app
.oneshot(
Request::get("/-/v1/search?text=jquery&size=20")
.body(Body::empty())
.unwrap(),
)
.await
.unwrap();
assert_eq!(response.status(), StatusCode::OK);
let body = body_json(response.into_body()).await;
let objects = body["objects"].as_array().unwrap();
assert_eq!(objects.len(), 3);
assert_eq!(objects[0]["package"]["name"], json!("jquery"));
// Three deduplicated names plus everything past the 24 results the
// eight budgeted pages walked.
assert_eq!(body["total"], json!(23_526));
pages.assert_async().await;
}
#[tokio::test]
async fn starved_upstream_still_counts_toward_the_search_total() {
let mut corp = mockito::Server::new_async().await;
let corp_objects: Vec<_> = (0..250)
.map(|i| json!({ "package": { "name": format!("@corp/widget-{i}") } }))
.collect();
let corp_pages = corp
.mock("GET", "/-/v1/search")
.match_query(mockito::Matcher::UrlEncoded("text".into(), "widget".into()))
.with_status(200)
.with_header("content-type", "application/json")
.with_body(json!({ "objects": corp_objects, "total": 23_547 }).to_string())
.expect(8)
.create_async()
.await;
let mut npmjs = mockito::Server::new_async().await;
let npmjs_body = json!({ "objects": [{ "package": { "name": "widget-solo" } }], "total": 1 });
let npmjs_page = npmjs
.mock("GET", "/-/v1/search")
.match_query(mockito::Matcher::AllOf(vec![
mockito::Matcher::UrlEncoded("text".into(), "widget".into()),
mockito::Matcher::UrlEncoded("size".into(), "1".into()),
]))
.with_status(200)
.with_header("content-type", "application/json")
.with_body(npmjs_body.to_string())
.expect(1)
.create_async()
.await;
let tmp = TempDir::new().unwrap();
let mut config = router_config(&npmjs.url(), &corp.url(), tmp.path().to_path_buf());
config.routing.upstreams.get_mut("corp").unwrap().search = true;
config.routing.upstreams.get_mut("npmjs").unwrap().search = true;
let app = router(config);
let response = app
.oneshot(
Request::get("/-/v1/search?text=widget&size=20")
.body(Body::empty())
.unwrap(),
)
.await
.unwrap();
assert_eq!(response.status(), StatusCode::OK);
let body = body_json(response.into_body()).await;
// corp: 250 deduplicated names + 21,547 unscanned. npmjs: its result
// budget is gone, so its single match lands in the total (+1) through
// the guaranteed first fetch rather than in the page.
assert_eq!(body["total"], json!(21_798));
corp_pages.assert_async().await;
npmjs_page.assert_async().await;
}
/// Every registry operation routes through the registry graph when addressed as
/// `/~<name>/...` (RFC "registries", implementation point 7): dist-tag
/// read/add/remove, whoami, search, the version manifest, and the whole
@@ -1,8 +1,8 @@
use super::{
AccessList, AuthState, Body, Config, Ipv4Addr, PackagePattern, PackageRules, Registries,
Registry, Request, ServiceExt, SocketAddr, SocketAddrV4, StatusCode, TempDir, Value,
access_rule, body_bytes, body_json, config_for, fs, header, hosted_with_access, json, router,
router_with_auth, seed_hosted, seed_hosted_with_maintainer, to_bytes,
AccessList, AuthState, Body, Config, Ecosystem, Ipv4Addr, PackagePattern, PackageRules,
Registries, Registry, Request, ServiceExt, SocketAddr, SocketAddrV4, StatusCode, TempDir,
Value, access_rule, body_bytes, body_json, config_for, fs, header, hosted_with_access, json,
router, router_with_auth, seed_hosted, seed_hosted_with_maintainer, to_bytes,
};
#[tokio::test]
@@ -196,15 +196,18 @@ async fn upstream_search_exhausts_results_to_return_an_exact_total() {
}
#[tokio::test]
async fn upstream_search_rejects_unbounded_offsets_and_result_sets() {
async fn upstream_search_bounds_the_walk_for_a_huge_offset() {
let mut upstream = mockito::Server::new_async().await;
let oversized = upstream
let objects: Vec<_> = (0..250)
.map(|i| json!({ "package": { "name": format!("remote-{i}") } }))
.collect();
let pages = upstream
.mock("GET", "/-/v1/search")
.match_query("text=remote&from=0&size=250")
.match_query(mockito::Matcher::Any)
.with_status(200)
.with_header("content-type", "application/json")
.with_body(json!({ "objects": [], "total": 2_001 }).to_string())
.expect(1)
.with_body(json!({ "objects": objects, "total": 10_000 }).to_string())
.expect(8)
.create_async()
.await;
let tmp = TempDir::new().unwrap();
@@ -212,8 +215,7 @@ async fn upstream_search_rejects_unbounded_offsets_and_result_sets() {
config.routing.upstreams.get_mut("npmjs").unwrap().search = true;
let app = router(config);
let offset = app
.clone()
let response = app
.oneshot(
Request::get("/-/v1/search?text=remote&from=2001")
.body(Body::empty())
@@ -221,22 +223,17 @@ async fn upstream_search_rejects_unbounded_offsets_and_result_sets() {
)
.await
.unwrap();
assert_eq!(offset.status(), StatusCode::BAD_REQUEST);
let result_set = app
.oneshot(
Request::get("/-/v1/search?text=remote")
.body(Body::empty())
.unwrap(),
)
.await
.unwrap();
assert_eq!(result_set.status(), StatusCode::BAD_REQUEST);
oversized.assert_async().await;
assert_eq!(response.status(), StatusCode::OK);
let body = body_json(response.into_body()).await;
assert_eq!(body["objects"], json!([]));
// 250 deduplicated names from the eight budgeted pages, plus the 8,000
// results past the 2,000 the budget walked.
assert_eq!(body["total"], json!(8_250));
pages.assert_async().await;
}
#[tokio::test]
async fn upstream_search_rejects_more_than_eight_short_pages() {
async fn upstream_search_stops_after_eight_short_pages() {
let mut upstream = mockito::Server::new_async().await;
let short_page = upstream
.mock("GET", "/-/v1/search")
@@ -267,10 +264,135 @@ async fn upstream_search_rejects_more_than_eight_short_pages() {
.await
.unwrap();
assert_eq!(response.status(), StatusCode::BAD_REQUEST);
assert_eq!(response.status(), StatusCode::OK);
let body = body_json(response.into_body()).await;
let objects = body["objects"].as_array().unwrap();
assert_eq!(objects.len(), 1);
// One deduplicated name plus the ninth result the walk never reached.
assert_eq!(body["total"], json!(2));
short_page.assert_async().await;
}
#[tokio::test]
async fn upstream_search_stops_at_the_request_cap_however_many_sources_route() {
let mut upstream = mockito::Server::new_async().await;
let requests = upstream
.mock("GET", "/-/v1/search")
.match_query(mockito::Matcher::Any)
.with_status(200)
.with_header("content-type", "application/json")
.with_body(
json!({ "objects": [{ "package": { "name": "remote-a" } }], "total": 1 }).to_string(),
)
.expect(32)
.create_async()
.await;
let tmp = TempDir::new().unwrap();
let mut config = config_for(&upstream.url(), tmp.path().to_path_buf());
// Forty search-enabled upstreams, each of which would otherwise be
// guaranteed its own fetch.
let template = config.routing.upstreams
.get("npmjs")
.expect("default `npmjs` upstream")
.clone();
let mut graph = vec![];
let mut sources = vec![];
for index in 0..40 {
let name = format!("mirror-{index}");
let mut mirror = template.clone();
mirror.search = true;
config.routing.upstreams.insert(name.clone(), mirror);
// A distinct pattern per mirror, so the router reaches every one of
// them; only the last is a catch-all.
let patterns = if index == 39 {
vec![]
} else {
vec![PackagePattern::parse(&format!("@m{index}/*"), Ecosystem::Npm).unwrap()]
};
graph.push((name.clone(), Registry::Upstream { patterns }));
sources.push(name);
}
graph.push(("main".to_string(), Registry::Router { sources }));
let registries = Registries::new(graph.into_iter().collect(), Some("main".to_string()));
registries.validate().expect("router config is valid");
config.routing.registries = registries;
let app = router(config);
let response = app
.oneshot(
Request::get("/-/v1/search?text=remote")
.body(Body::empty())
.unwrap(),
)
.await
.unwrap();
assert_eq!(response.status(), StatusCode::OK);
requests.assert_async().await;
}
#[tokio::test]
async fn upstream_search_reporting_results_but_returning_none_is_a_gateway_error() {
let mut upstream = mockito::Server::new_async().await;
let lying_page = upstream
.mock("GET", "/-/v1/search")
.match_query(mockito::Matcher::Any)
.with_status(200)
.with_header("content-type", "application/json")
.with_body(json!({ "objects": [], "total": 9 }).to_string())
.expect(1)
.create_async()
.await;
let tmp = TempDir::new().unwrap();
let mut config = config_for(&upstream.url(), tmp.path().to_path_buf());
config.routing.upstreams.get_mut("npmjs").unwrap().search = true;
let app = router(config);
let response = app
.oneshot(
Request::get("/-/v1/search?text=remote")
.body(Body::empty())
.unwrap(),
)
.await
.unwrap();
assert_eq!(response.status(), StatusCode::BAD_GATEWAY);
lying_page.assert_async().await;
}
#[tokio::test]
async fn upstream_without_a_search_endpoint_is_skipped() {
let mut upstream = mockito::Server::new_async().await;
let missing = upstream
.mock("GET", "/-/v1/search")
.match_query(mockito::Matcher::Any)
.with_status(404)
.expect(1)
.create_async()
.await;
let tmp = TempDir::new().unwrap();
seed_hosted(tmp.path(), "ajv");
let mut config = config_for(&upstream.url(), tmp.path().to_path_buf());
config.routing.upstreams.get_mut("npmjs").unwrap().search = true;
let app = router(config);
let response = app
.oneshot(
Request::get("/-/v1/search?text=ajv")
.body(Body::empty())
.unwrap(),
)
.await
.unwrap();
assert_eq!(response.status(), StatusCode::OK);
let body = body_json(response.into_body()).await;
assert_eq!(body["total"], json!(1));
assert_eq!(body["objects"][0]["package"]["name"], json!("ajv"));
missing.assert_async().await;
}
#[tokio::test]
async fn registry_directory_hides_upstream_access_and_package_rule_metadata() {
let tmp = TempDir::new().unwrap();
@@ -358,3 +480,35 @@ async fn registry_directory_describes_oci_only_named_endpoints() {
assert_eq!(response.status(), StatusCode::UNAUTHORIZED, "{path}");
}
}
#[tokio::test]
async fn search_excludes_an_upstream_with_a_denying_package_rule() {
let mut upstream = mockito::Server::new_async().await;
let never_queried = upstream
.mock("GET", "/-/v1/search")
.match_query(mockito::Matcher::Any)
.expect(0)
.create_async()
.await;
let tmp = TempDir::new().unwrap();
seed_hosted(tmp.path(), "ajv");
let mut config = config_for(&upstream.url(), tmp.path().to_path_buf());
let npmjs = config.routing.upstreams.get_mut("npmjs").unwrap();
npmjs.search = true;
npmjs.rules = PackageRules::new(vec![access_rule("@corp/*", "alice")], None);
let app = router(config);
let response = app
.oneshot(
Request::get("/-/v1/search?text=ajv")
.body(Body::empty())
.unwrap(),
)
.await
.unwrap();
assert_eq!(response.status(), StatusCode::OK);
let body = body_json(response.into_body()).await;
assert_eq!(body["total"], json!(1));
assert_eq!(body["objects"][0]["package"]["name"], json!("ajv"));
never_queried.assert_async().await;
}