From 9586571318fba7f96c9ed3df3c340dd416573d98 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 30 Oct 2026 00:20:08 +0000 Subject: [PATCH] Groups and selectors: raw cursors and npm audits name nothing hidden A raw listing set a page's `next` to the last entry it walked, whether a content selector hid it or not, and a group passed the smallest member cursor on unchanged: following `after=` walked through every hidden name of a directory, in the JSON, the `Link` header and the HTML page alike. The same cursor told a caller that a directory of hidden files existed (a 301 for the directory probe, a 200 with `next` for a directory past a page). A page now reads on past hidden entries until it holds `limit` visible ones, walking at most 1,000 entries (`LISTING_SCAN`) for a caller with a selector, and `next` is the last visible one: at the bound the page ends there, and the listing ends when it found none. A directory exists for the caller only when it holds a visible entry. A caller without a selector walks exactly what it did before. `npm audit` through a group (or straight to a proxy) left out only the reserved prefixes on the way, so a package a caller's selector hid still went to the proxy's upstream with the proxy's credentials. A forward now also leaves out every package the caller's package filter hides on any repository on the way to the proxy, the proxy's own included, and refuses (400) a body it cannot filter. Tests cover a group of two hosted members for NuGet, npm, raw and Docker, read by a caller who may read only one member, one with a selector on the group's grant and one with a selector on a member's: downloads and metadata, search and listings with their counts, the browse API, raw cursors and directories (also of a hosted and a proxy repository), and npm audits through nested groups. Docker refuses a name outside a selector of the requested repository's grant by name (403, its registry token grants no `pull` on it), so a hidden image answers as an absent one of the same name pattern does, which the test checks rather than a 404. Fixes #648 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012e6DJZaXD2mBH9tURsdutM --- crates/plugin-api/src/lib.rs | 7 +- crates/server/src/files/listing.rs | 156 ++++++++++++------- crates/server/src/plugins.rs | 89 +++++++---- crates/server/tests/browse.rs | 108 +++++++++++++ crates/server/tests/common/mod.rs | 160 +++++++++++++++++++ crates/server/tests/npm.rs | 220 ++++++++++++++++++++++++++ crates/server/tests/raw.rs | 237 +++++++++++++++++++++++++++++ crates/server/tests/registry.rs | 138 +++++++++++++++++ docs/changes/2.2.0/648.security.md | 13 ++ docs/format-plugins.md | 4 +- docs/npm.md | 16 +- docs/raw.md | 11 +- 12 files changed, 1068 insertions(+), 91 deletions(-) create mode 100644 docs/changes/2.2.0/648.security.md diff --git a/crates/plugin-api/src/lib.rs b/crates/plugin-api/src/lib.rs index 73535a2c..7b6a188d 100644 --- a/crates/plugin-api/src/lib.rs +++ b/crates/plugin-api/src/lib.rs @@ -205,9 +205,10 @@ pub trait FormatPlugin: Send + Sync + 'static { /// load, or an action of the host's. fn render(&self, repo: &Repository, req: &Request, data: &[Loaded]) -> Action; - /// The body of an [`Action::Forward`] through a group without the - /// packages `hidden` names: the reserved prefixes of every group on the - /// way to the proxy keep their packages' names from the upstream. + /// The body of an [`Action::Forward`] without the packages `hidden` + /// names: the reserved prefixes of every group on the way to the proxy, + /// and the caller's content selectors on every repository on the way, + /// keep their packages' names from the upstream. /// `hidden` takes a package id as the body writes it. `None` when the /// body cannot be filtered: the host then refuses the request (400) and /// never sends it on unfiltered. The default filters nothing, so a diff --git a/crates/server/src/files/listing.rs b/crates/server/src/files/listing.rs index b9bd76fd..b43e5d88 100644 --- a/crates/server/src/files/listing.rs +++ b/crates/server/src/files/listing.rs @@ -13,6 +13,14 @@ //! whose first [`SUBDIRECTORY_SCAN`] files is visible is left out). A //! group lists the merged entries of the members the caller may read, the //! highest-priority member's file first; a proxy lists what it has cached. +//! +//! Nothing a selector hides is named, not even by `next`: a page reads on +//! past hidden entries until it holds `limit` visible ones, and `next` is +//! the last of them. That walk is bounded ([`LISTING_SCAN`] entries): a +//! page that reaches the bound ends at its last visible entry, and the +//! listing ends where it found none. A directory exists for the caller +//! when it holds a visible entry, so one of hidden entries alone is not +//! found, whatever its size. use std::collections::BTreeMap; @@ -33,6 +41,14 @@ pub const MAX_LIMIT: usize = 1000; /// a selector-restricted caller may see. pub const SUBDIRECTORY_SCAN: usize = 1000; +/// How many entries of a directory a page walks, at most, to find the +/// `limit` a selector-restricted caller may see (a page of `MAX_LIMIT` +/// walks one more, to know whether another follows). +pub const LISTING_SCAN: usize = 1000; + +/// How many entries a selector-restricted page walks at once, at least. +const WALK_BATCH: usize = 100; + /// One entry of a page. #[derive(Debug, Clone)] pub struct Entry { @@ -64,21 +80,24 @@ fn resume_at(prefix: &str, after: Option<&str>) -> Result { let Some(after) = after.filter(|a| !a.is_empty()) else { return Ok(prefix.to_string()); }; - let (name, directory) = match after.strip_suffix('/') { - Some(name) => (name, true), - None => (after, false), - }; + let name = after.strip_suffix('/').unwrap_or(after); files::validate_segment(name).map_err(|e| FileError::BadRequest(format!("after: {e}")))?; - Ok(if directory { - format!("{prefix}{name}0") - } else { - format!("{prefix}{name}\u{1}") - }) + Ok(past(prefix, after)) +} + +/// The least path past the entry `entry` of directory `prefix`, a name +/// the walk read (see [`resume_at`]). +fn past(prefix: &str, entry: &str) -> String { + match entry.strip_suffix('/') { + Some(name) => format!("{prefix}{name}0"), + None => format!("{prefix}{entry}\u{1}"), + } } /// One repository's entries of a page: what it has after `from`, as far as -/// it read (`more`: it has entries past the last one returned, up to and -/// including the entry named `more`, which it has read). +/// it read (`more`: it may have entries past the last one returned, and +/// has all of them up to and including the entry named `more`, which it +/// returned: a name the caller may see). struct Partial { entries: Vec, more: Option, @@ -86,7 +105,8 @@ struct Partial { impl Files<'_> { /// The entries of `prefix` in hosted or proxy `repo` from `from` on, - /// visible through every one of `filters`. + /// visible through every one of `filters`: `limit` of them, walking + /// past the hidden ones up to [`LISTING_SCAN`] entries. async fn list_local( &self, repo: &Repository, @@ -95,48 +115,82 @@ impl Files<'_> { limit: usize, filters: &[PackageFilter], ) -> Result { - let walked = raw_dao::walk_directory( - &self.state.pool, - repo.id, - prefix, - from, - i64::try_from(limit + 1).unwrap_or(i64::MAX), - ) - .await?; - let more = walked.len() > limit; + // One visible entry past the page says that another page follows. + let wanted = limit + 1; + let unfiltered = filters.iter().all(|f| matches!(f, PackageFilter::AllowAll)); + let bound = if unfiltered { + wanted + } else { + wanted.max(LISTING_SCAN) + }; let mut entries = Vec::new(); - let mut last = None; - for walked in walked.into_iter().take(limit) { - let rest = &walked.path[prefix.len()..]; - let (entry, key) = match (rest.find('/'), &walked.file) { - (None, Some(file)) => { - let visible = filters.iter().all(|f| f.allows(&file.path)); - let entry = Entry { - name: rest.to_string(), - kind: EntryKind::File(Box::new(file.clone())), - }; - (visible.then_some(entry), rest.to_string()) - } - (Some(i), _) => { - let name = &rest[..i]; - let dir = format!("{prefix}{name}/"); - let visible = self.directory_visible(repo, &dir, filters).await?; - let entry = Entry { - name: format!("{name}/"), - kind: EntryKind::Directory(dir), - }; - (visible.then_some(entry), format!("{name}/")) + let mut walked_count = 0; + let mut from = from.to_string(); + let mut ended = false; + while entries.len() < wanted && walked_count < bound { + let batch = if unfiltered { + wanted - entries.len() + } else { + (wanted - entries.len()).max(WALK_BATCH) + } + .min(bound - walked_count); + let walked = raw_dao::walk_directory( + &self.state.pool, + repo.id, + prefix, + &from, + i64::try_from(batch).unwrap_or(i64::MAX), + ) + .await?; + ended = walked.len() < batch; + walked_count += walked.len(); + let mut last = None; + for walked in walked { + if entries.len() == wanted { + break; } - // A file row vanished between the walk and the join. - (None, None) => (None, rest.to_string()), - }; - entries.extend(entry); - last = Some(key); + let rest = &walked.path[prefix.len()..]; + let (entry, key) = match (rest.find('/'), &walked.file) { + (None, Some(file)) => { + let visible = filters.iter().all(|f| f.allows(&file.path)); + let entry = Entry { + name: rest.to_string(), + kind: EntryKind::File(Box::new(file.clone())), + }; + (visible.then_some(entry), rest.to_string()) + } + (Some(i), _) => { + let name = &rest[..i]; + let dir = format!("{prefix}{name}/"); + let visible = self.directory_visible(repo, &dir, filters).await?; + let entry = Entry { + name: format!("{name}/"), + kind: EntryKind::Directory(dir), + }; + (visible.then_some(entry), format!("{name}/")) + } + // A file row vanished between the walk and the join. + (None, None) => (None, rest.to_string()), + }; + entries.extend(entry); + last = Some(key); + } + match last { + Some(last) if !ended => from = past(prefix, &last), + _ => break, + } } - Ok(Partial { - entries, - more: if more { last } else { None }, - }) + // Past the page, at the walk's bound or at the directory's end, the + // page ends at its last visible entry: never at a hidden one. + let more = if entries.len() > limit { + entries.truncate(limit); + entries.last().map(|e| e.name.clone()) + } else if ended { + None + } else { + entries.last().map(|e| e.name.clone()) + }; + Ok(Partial { entries, more }) } /// Whether directory `dir` of `repo` holds a file visible through every diff --git a/crates/server/src/plugins.rs b/crates/server/src/plugins.rs index da15317c..5fe19579 100644 --- a/crates/server/src/plugins.rs +++ b/crates/server/src/plugins.rs @@ -48,7 +48,7 @@ use axum::response::{IntoResponse, Response}; use bytes::Bytes; use forge_core::auth::principal::{AuthMethod, Principal}; use forge_core::auth::token::{check_token, TokenCheck}; -use forge_core::authz::Level; +use forge_core::authz::{Level, PackageFilter}; use forge_core::format::Format; use forge_core::net::upstream::{RequestClass, SpooledBody}; use forge_core::repo::{ @@ -1627,17 +1627,24 @@ impl Ctx<'_> { /// The proxy whose upstream a forward from `repo` goes to: the /// repository itself, or a group's first proxy member (by priority) - /// the caller may read, through nested groups. With it, the reserved - /// prefixes of every group on the way, whose packages the forward - /// leaves out: reading through an outer group keeps an inner group's - /// packages inside as reading it directly does. + /// the caller may read, through nested groups. With it, what the + /// forward leaves out ([`Withheld`]): the reserved prefixes of every + /// group on the way, so that reading through an outer group keeps an + /// inner group's packages inside as reading it directly does, and the + /// packages the caller's selectors hide on any repository on the way. async fn forward_source( &self, repo: &Repository, depth: u32, - ) -> Result)>, RepoError> { + ) -> Result, RepoError> { match repo.kind() { - RepoKind::Proxy => Ok(Some((repo.clone(), Vec::new()))), + RepoKind::Proxy => Ok(Some(( + repo.clone(), + Withheld { + reserved: Vec::new(), + filters: vec![self.state.authz.package_filter(&self.principal, repo).await], + }, + ))), RepoKind::Hosted => Ok(None), RepoKind::Group => { let max = self.state.config.groups.max_nesting_depth.max(1) as u32; @@ -1648,11 +1655,14 @@ impl Ctx<'_> { if member.format != repo.format || !self.may(Level::Read, &member).await { continue; } - if let Some((proxy, mut reserved)) = + if let Some((proxy, mut withheld)) = Box::pin(self.forward_source(&member, depth + 1)).await? { - reserved.extend(repo.reserved_prefixes()); - return Ok(Some((proxy, reserved))); + withheld.reserved.extend(repo.reserved_prefixes()); + withheld + .filters + .push(self.state.authz.package_filter(&self.principal, repo).await); + return Ok(Some((proxy, withheld))); } } Ok(None) @@ -1661,11 +1671,12 @@ impl Ctx<'_> { } /// Send `forward` on to the upstream, with the proxy's credentials (to - /// its origin only), and answer with what the upstream said. Through a - /// group with reserved prefixes on the way, the body goes without their - /// packages ([`FormatPlugin::forward_body`]), or not at all. + /// its origin only), and answer with what the upstream said. With + /// reserved prefixes or the caller's selectors on the way, the body goes + /// without the packages they keep inside ([`FormatPlugin::forward_body`]), + /// or not at all. async fn forward(&self, forward: Forward) -> Response { - let (source, reserved) = match self.forward_source(self.repo, 0).await { + let (source, withheld) = match self.forward_source(self.repo, 0).await { Ok(Some(found)) => found, Ok(None) => { return ( @@ -1691,29 +1702,19 @@ impl Ctx<'_> { }; let url = format!("{}{}", remote.trim_end_matches('/'), forward.path); let content_type = forward.content_type.clone(); - let body = if reserved.is_empty() { + let body = if withheld.is_empty() { forward.body } else { let filtered = self - .call(move |plugin| { - // Prefixes are stored lowercase; a name is matched in - // any case, which keeps more inside, never less. - let hidden = |name: &str| { - let lower = name.to_lowercase(); - reserved - .iter() - .any(|prefix| lower.starts_with(prefix.as_str())) - }; - plugin.forward_body(&forward, &hidden) - }) + .call(move |plugin| plugin.forward_body(&forward, &|name| withheld.hides(name))) .await; match filtered { Ok(Some(body)) => body, Ok(None) => { return ( StatusCode::BAD_REQUEST, - "this request cannot be passed on without the packages this \ - repository keeps from its upstream (its reserved prefixes)", + "this request cannot be passed on without the packages it must \ + leave out: those under reserved prefixes, and those you may not read", ) .into_response() } @@ -1868,6 +1869,38 @@ impl Ctx<'_> { } } +/// What a forward leaves out of its body on the way to a proxy's upstream +/// ([`Ctx::forward_source`]). +struct Withheld { + /// The reserved prefixes of the groups on the way, lowercase. + reserved: Vec, + /// The caller's package filter on each repository on the way, the + /// proxy's included. + filters: Vec, +} + +impl Withheld { + /// Nothing is left out: the body goes on as it is. + fn is_empty(&self) -> bool { + self.reserved.is_empty() + && self + .filters + .iter() + .all(|f| matches!(f, PackageFilter::AllowAll)) + } + + /// Whether the package `name` (as the body writes it) stays inside. + /// Prefixes are stored lowercase, and selectors match a name in any + /// case: a name keeps more inside, never less. + fn hides(&self, name: &str) -> bool { + let lower = name.to_lowercase(); + self.reserved + .iter() + .any(|prefix| lower.starts_with(prefix.as_str())) + || !self.filters.iter().all(|f| f.allows(name)) + } +} + /// The permission `change` needs, and what it is called in a refusal. fn needs(change: &Change) -> (Level, &'static str) { match change { diff --git a/crates/server/tests/browse.rs b/crates/server/tests/browse.rs index d19fbd85..c5acabbe 100644 --- a/crates/server/tests/browse.rs +++ b/crates/server/tests/browse.rs @@ -494,6 +494,114 @@ async fn browse_group_merges_members() { let _ = lower; } +/// A NuGet group shows a caller what it could read from its members +/// directly, no more (#648): with a member it may not read, a selector on +/// the group's grant or one on a member's, a hidden package has no flat +/// container, download or registration (404), and the feed's search and +/// the browse API leave it out, their counts too. +#[tokio::test] +async fn a_group_shows_a_caller_only_the_packages_it_may_read() { + let srv = common::start().await; + let (u, p) = srv.admin_basic(); + let client = srv.client(); + let n = common::unique(); + let (a, b, group) = (format!("va{n}"), format!("vb{n}"), format!("vg{n}")); + // Indexed like `GroupCaller::sees`. + let ids = [ + "Vis.Public.A", + "Vis.Secret.A", + "Vis.Public.B", + "Vis.Secret.B", + ]; + for (i, id) in ids.iter().enumerate() { + publish_one(&srv, if i < 2 { &a } else { &b }, id, "1.0.0", false).await; + } + let resp = client + .post(format!("{}/admin/repositories", srv.base_url)) + .basic_auth(&u, Some(&p)) + .json(&json!({"name": group, "kind": "group"})) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), 201); + for (member, priority) in [(&a, 10), (&b, 5)] { + let resp = client + .post(format!( + "{}/admin/repositories/{group}/members", + srv.base_url + )) + .basic_auth(&u, Some(&p)) + .json(&json!({"member": member, "priority": priority})) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), 201); + } + + for common::GroupCaller { + label, + caller, + sees, + } in srv.group_callers(&group, &a, &b, &["vis.public.*"]).await + { + let get = |path: String| { + caller + .apply(client.get(format!("{}{path}", srv.base_url))) + .send() + }; + let mut visible = Vec::new(); + for (i, id) in ids.iter().enumerate() { + let lower = id.to_lowercase(); + let expected = if sees[i] { 200 } else { 404 }; + for path in [ + format!("/repository/{group}/flat/{lower}/index.json"), + format!("/repository/{group}/flat/{lower}/1.0.0/{lower}.1.0.0.nupkg"), + format!("/repository/{group}/registration/{lower}/index.json"), + format!("/browse/packages/{group}/{id}"), + ] { + let resp = get(path.clone()).await.unwrap(); + assert_eq!(resp.status(), expected, "{label}: {path}"); + } + if sees[i] { + visible.push(id.to_string()); + } + } + visible.sort_unstable(); + let search: Value = get(format!( + "/repository/{group}/query?q=&prerelease=true&take=20" + )) + .await + .unwrap() + .json() + .await + .unwrap(); + let mut found: Vec = search["data"] + .as_array() + .unwrap() + .iter() + .map(|hit| hit["id"].as_str().unwrap().to_string()) + .collect(); + found.sort_unstable(); + assert_eq!(found, visible, "{label}: {search}"); + assert_eq!(search["totalHits"], visible.len(), "{label}: {search}"); + let listed: Value = get(format!("/browse/packages?repo={group}&take=20")) + .await + .unwrap() + .json() + .await + .unwrap(); + let mut found: Vec = listed["items"] + .as_array() + .unwrap() + .iter() + .map(|item| item["original_id"].as_str().unwrap().to_string()) + .collect(); + found.sort_unstable(); + assert_eq!(found, visible, "{label}: {listed}"); + assert_eq!(listed["total"], visible.len(), "{label}: {listed}"); + } +} + /// An offline hosted or group repository serves nothing through browse either, /// as on its feed (#73): its search and package detail answer 503 with /// `Retry-After`, a merged search leaves it out, and `/browse/repos` flags it. diff --git a/crates/server/tests/common/mod.rs b/crates/server/tests/common/mod.rs index 938aa82a..682247c1 100644 --- a/crates/server/tests/common/mod.rs +++ b/crates/server/tests/common/mod.rs @@ -745,6 +745,166 @@ impl TestServer { .unwrap(); body["token"].as_str().expect("plaintext token").to_string() } + + /// An admin API call with the admin's token, expecting a success. + async fn admin_call( + &self, + method: reqwest::Method, + path: &str, + body: serde_json::Value, + ) -> serde_json::Value { + let (u, p) = self.admin_basic(); + let resp = self + .client() + .request(method, format!("{}{path}", self.base_url)) + .basic_auth(&u, Some(&p)) + .json(&body) + .send() + .await + .unwrap(); + let status = resp.status(); + let text = resp.text().await.unwrap(); + assert!(status.is_success(), "{path} {body}: {status} {text}"); + serde_json::from_str(&text).unwrap_or(serde_json::Value::Null) + } + + /// A new account without roles, and an API token of it. + pub async fn caller(&self) -> Caller { + let username = format!("caller{}x{}@example.com", std::process::id(), unique()); + let id = self + .create_user(&username, "caller-password-1", "reader") + .await; + let token = self + .admin_call( + reqwest::Method::POST, + &format!("/admin/users/{id}/tokens"), + serde_json::json!({"name": "caller"}), + ) + .await["token"] + .as_str() + .expect("plaintext token") + .to_string(); + Caller { + id, + username, + token, + } + } + + /// A content selector of `includes`; returns its id. + pub async fn create_selector(&self, includes: &[&str]) -> i64 { + self.admin_call( + reqwest::Method::POST, + "/admin/content-selectors", + serde_json::json!({"name": format!("sel{}x{}", std::process::id(), unique()), "includes": includes}), + ) + .await["id"] + .as_i64() + .expect("selector id") + } + + /// Give account `user_id` a role of its own that holds `grants`: + /// `(repository, level, selector id)`. + pub async fn grant_role(&self, user_id: i64, grants: &[(&str, &str, Option)]) { + let role = self + .admin_call( + reqwest::Method::POST, + "/admin/roles", + serde_json::json!({"name": format!("role{}x{}", std::process::id(), unique())}), + ) + .await["id"] + .as_i64() + .expect("role id"); + for (repository, level, selector) in grants { + self.admin_call( + reqwest::Method::PUT, + &format!("/admin/roles/{role}/grants"), + serde_json::json!({"repository": repository, "level": level, "selector_id": selector}), + ) + .await; + } + self.admin_call( + reqwest::Method::PUT, + &format!("/admin/users/{user_id}/roles"), + serde_json::json!({"role_id": role}), + ) + .await; + } + + /// The callers of a group of members `a` and `b` that see only part of + /// it (#648), each with read on the group: who reads `a` alone of its + /// members; who reads both, the group through a selector of + /// `includes`; and who reads both, `b` through that selector. + pub async fn group_callers( + &self, + group: &str, + a: &str, + b: &str, + includes: &[&str], + ) -> Vec { + let selector = self.create_selector(includes).await; + let cases = [ + ( + "a reader of one member", + vec![(group, "read", None), (a, "read", None)], + [true, true, false, false], + ), + ( + "a selector on the group's grant", + vec![ + (group, "read", Some(selector)), + (a, "read", None), + (b, "read", None), + ], + [true, false, true, false], + ), + ( + "a selector on a member's grant", + vec![ + (group, "read", None), + (a, "read", None), + (b, "read", Some(selector)), + ], + [true, true, true, false], + ), + ]; + let mut callers = Vec::new(); + for (label, grants, sees) in cases { + let caller = self.caller().await; + self.grant_role(caller.id, &grants).await; + callers.push(GroupCaller { + label, + caller, + sees, + }); + } + callers + } +} + +/// A caller of [`TestServer::group_callers`], and what it sees of the +/// group: of `a`, what the selector includes and what it does not; then +/// the same of `b`. +#[derive(Clone, Debug)] +pub struct GroupCaller { + pub label: &'static str, + pub caller: Caller, + pub sees: [bool; 4], +} + +/// An account of a test, with an API token of it. +#[derive(Clone, Debug)] +pub struct Caller { + pub id: i64, + pub username: String, + pub token: String, +} + +impl Caller { + /// Attach the caller's token as Basic credentials. + pub fn apply(&self, rb: reqwest::RequestBuilder) -> reqwest::RequestBuilder { + rb.basic_auth(&self.username, Some(&self.token)) + } } /// Build a minimal valid `.nupkg` (a zip with `[Content_Types].xml` + a diff --git a/crates/server/tests/npm.rs b/crates/server/tests/npm.rs index f71e413b..e17f15a0 100644 --- a/crates/server/tests/npm.rs +++ b/crates/server/tests/npm.rs @@ -1748,6 +1748,226 @@ async fn audits_through_nested_groups_keep_every_reserved_name_inside() { assert_eq!(upstream.audits.lock().unwrap().clone(), vec![bulk]); } +/// A group shows a caller what it could read from its members directly, no +/// more (#648): with a member it may not read, a selector on the group's +/// grant or one on a member's, a hidden package has no packument, tarball +/// or dist-tags (404), and search and the browse API leave it out, their +/// counts too. +#[tokio::test] +async fn groups_show_a_caller_only_the_packages_it_may_read() { + let srv = common::start().await; + let token = srv.admin_token().await; + let n = unique(); + let (a, b, group) = (format!("va{n}"), format!("vb{n}"), format!("vg{n}")); + for name in [&a, &b] { + create( + &srv, + json!({"name": name, "kind": "hosted", "format": "npm"}), + ) + .await; + } + create( + &srv, + json!({"name": group, "kind": "group", "format": "npm"}), + ) + .await; + add_member(&srv, &group, &a, 10).await; + add_member(&srv, &group, &b, 5).await; + // Indexed like `GroupCaller::sees`. + let packages = [ + "vis-public-a", + "vis-secret-a", + "vis-public-b", + "vis-secret-b", + ]; + for (i, name) in packages.iter().enumerate() { + let repo = if i < 2 { &a } else { &b }; + let resp = Npm::new(&srv, repo, Some(token.clone())) + .publish(name, "1.0.0", &["latest"]) + .await; + assert_eq!(resp.status(), 201, "{name}"); + } + + for common::GroupCaller { + label, + caller, + sees, + } in srv.group_callers(&group, &a, &b, &["vis-public-*"]).await + { + let npm = Npm::new(&srv, &group, Some(caller.token.clone())); + let browse = |path: String| { + caller + .apply(srv.client().get(format!("{}{path}", srv.base_url))) + .send() + }; + let mut visible = Vec::new(); + for (i, name) in packages.iter().enumerate() { + let expected = if sees[i] { 200 } else { 404 }; + for abbreviated in [true, false] { + assert_eq!( + npm.packument(name, abbreviated).await.0, + expected, + "{label}: {name}" + ); + } + for path in [ + format!("{name}/-/{name}-1.0.0.tgz"), + format!("-/package/{name}/dist-tags"), + ] { + let resp = npm.req(reqwest::Method::GET, &path).send().await.unwrap(); + assert_eq!(resp.status(), expected, "{label}: {path}"); + } + let resp = browse(format!("/browse/packages/{group}/{name}")) + .await + .unwrap(); + assert_eq!(resp.status(), expected, "{label}: browse {name}"); + if sees[i] { + visible.push(name.to_string()); + } + } + visible.sort_unstable(); + let found: Value = npm + .req(reqwest::Method::GET, "-/v1/search?text=vis&size=20") + .send() + .await + .unwrap() + .json() + .await + .unwrap(); + let mut names: Vec = found["objects"] + .as_array() + .unwrap() + .iter() + .map(|o| o["package"]["name"].as_str().unwrap().to_string()) + .collect(); + names.sort_unstable(); + assert_eq!(names, visible, "{label}: {found}"); + assert_eq!(found["total"], visible.len(), "{label}: {found}"); + let listed: Value = browse(format!("/browse/packages?repo={group}&take=20")) + .await + .unwrap() + .json() + .await + .unwrap(); + let mut names: Vec = listed["items"] + .as_array() + .unwrap() + .iter() + .map(|i| i["original_id"].as_str().unwrap().to_string()) + .collect(); + names.sort_unstable(); + assert_eq!(names, visible, "{label}: {listed}"); + assert_eq!(listed["total"], visible.len(), "{label}: {listed}"); + } +} + +/// An audit tells the upstream only of packages the caller could read +/// through the path to the proxy (#648): a selector on the group's grant, +/// on a nested group's or on the proxy's own leaves the packages it hides +/// out of the body, in a bulk and in a legacy request alike, also when +/// the proxy is audited directly. +#[tokio::test] +async fn audits_leave_out_what_the_callers_selectors_hide() { + let srv = common::start().await; + let upstream = Arc::new(Upstream::default()); + let base = serve_upstream(upstream.clone()).await; + let n = unique(); + let (proxy, inner, outer) = (format!("ap{n}"), format!("ai{n}"), format!("ao{n}")); + create( + &srv, + json!({"name": proxy, "kind": "proxy", "format": "npm", "remote_url": base}), + ) + .await; + create( + &srv, + json!({"name": inner, "kind": "group", "format": "npm"}), + ) + .await; + add_member(&srv, &inner, &proxy, 10).await; + create( + &srv, + json!({"name": outer, "kind": "group", "format": "npm"}), + ) + .await; + add_member(&srv, &outer, &inner, 10).await; + let selector = srv.create_selector(&["left-*"]).await; + + let bulk = json!({"left-pad": ["1.3.0"], "private-lib": ["2.0.0"]}); + let tree = json!({ + "name": "app", + "version": "1.0.0", + "requires": {"left-pad": "^1.3.0", "private-lib": "^2.0.0"}, + "dependencies": { + "left-pad": {"version": "1.3.0"}, + "private-lib": {"version": "2.0.0"} + }, + "install": [], + "remove": [], + "metadata": {"npm_version": "6.14.18"} + }); + // Who reads all three, the selector on one of them; audited through + // the outer group, and directly through the proxy for the last. + for (label, scoped, audited) in [ + ("the outer group's grant", &outer, &outer), + ("the inner group's grant", &inner, &outer), + ("the proxy's grant", &proxy, &outer), + ("the proxy's grant, audited directly", &proxy, &proxy), + ] { + let caller = srv.caller().await; + let grants: Vec<(&str, &str, Option)> = [&outer, &inner, &proxy] + .into_iter() + .map(|repo| { + let selector = (repo == scoped).then_some(selector); + (repo.as_str(), "read", selector) + }) + .collect(); + srv.grant_role(caller.id, &grants).await; + let npm = Npm::new(&srv, audited, Some(caller.token.clone())); + upstream.audits.lock().unwrap().clear(); + for (kind, body) in [("advisories/bulk", &bulk), ("audits/quick", &tree)] { + let resp = npm + .req(reqwest::Method::POST, &format!("-/npm/v1/security/{kind}")) + .json(body) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), 200, "{label}: {kind}"); + } + let sent = upstream.audits.lock().unwrap().clone(); + assert_eq!(sent.len(), 2, "{label}: {sent:?}"); + assert_eq!(sent[0], json!({"left-pad": ["1.3.0"]}), "{label}"); + for body in &sent { + assert!(!body.to_string().contains("private-lib"), "{label}: {body}"); + } + assert_eq!( + sent[1]["dependencies"], + json!({"left-pad": {"version": "1.3.0"}}), + "{label}" + ); + } + + // Without a selector on the way, the body goes on as it is. + let caller = srv.caller().await; + srv.grant_role( + caller.id, + &[ + (&outer, "read", None), + (&inner, "read", None), + (&proxy, "read", None), + ], + ) + .await; + upstream.audits.lock().unwrap().clear(); + let resp = Npm::new(&srv, &outer, Some(caller.token)) + .req(reqwest::Method::POST, "-/npm/v1/security/advisories/bulk") + .json(&bulk) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), 200); + assert_eq!(upstream.audits.lock().unwrap().clone(), vec![bulk]); +} + #[tokio::test] async fn retention_never_deletes_a_tagged_version() { let srv = common::start().await; diff --git a/crates/server/tests/raw.rs b/crates/server/tests/raw.rs index fac5b43f..8f58a6de 100644 --- a/crates/server/tests/raw.rs +++ b/crates/server/tests/raw.rs @@ -876,6 +876,243 @@ async fn selectors_scope_paths() { assert_eq!(names(&mixed), ["visible/"]); } +/// What `caller` gets of `path` in repository `repo`, redirects not +/// followed: its status, the `Link` header and the body. +async fn read_as( + srv: &TestServer, + caller: &common::Caller, + repo: &str, + path: &str, + accept: &str, +) -> (u16, Option, String) { + let client = reqwest::Client::builder() + .redirect(reqwest::redirect::Policy::none()) + .build() + .unwrap(); + let resp = caller + .apply(client.get(format!("{}/repository/{repo}/{path}", srv.base_url))) + .header(header::ACCEPT, accept) + .send() + .await + .unwrap(); + let status = resp.status().as_u16(); + let link = resp + .headers() + .get(header::LINK) + .map(|v| v.to_str().unwrap().to_string()); + (status, link, resp.text().await.unwrap()) +} + +/// Every entry of directory `dir` of `repo` that `caller` is shown, a page +/// of one at a time, with every page's `next`; each page's `Link` header +/// and HTML page link to the next, and none of them, its JSON neither, +/// holds any of `hidden`. +async fn walk_as( + srv: &TestServer, + caller: &common::Caller, + repo: &str, + dir: &str, + hidden: &[&str], +) -> (Vec, Vec) { + let (mut names, mut nexts) = (Vec::new(), Vec::new()); + let mut after = String::new(); + loop { + let query = format!("{dir}?limit=1&after={after}"); + let (status, link, body) = read_as(srv, caller, repo, &query, "application/json").await; + assert_eq!(status, 200, "{repo}/{query}: {body}"); + let page: Value = serde_json::from_str(&body).unwrap(); + let (_, _, html) = read_as(srv, caller, repo, &query, "text/html").await; + for text in [&body, &html, &link.clone().unwrap_or_default()] { + for name in hidden { + assert!(!text.contains(name), "{repo}/{query} names {name}: {text}"); + } + } + names.extend(self::names(&page)); + match page["next"].as_str() { + Some(next) => { + let link = link.expect("a Link to the next page"); + assert!( + link.contains(&format!("after={}", next.replace('/', "%2F"))), + "{link}" + ); + assert!( + html.contains(&format!("after={}", next.replace('/', "%2F"))), + "{html}" + ); + nexts.push(next.to_string()); + after = next.to_string(); + } + None => { + assert_eq!(link, None); + return (names, nexts); + } + } + } +} + +/// A group shows a caller what it could read from the members directly, no +/// more: with a member it may not read, a selector on the group's grant or +/// one on a member's, no file of the others, no checksum, no name in a +/// listing or in a listing's `next`, `Link` or HTML link comes through, +/// and a directory of hidden files alone is not found, however many it +/// holds (#648). A hosted or proxy repository's listings hold the same. +#[tokio::test] +async fn groups_and_selectors_show_only_what_the_caller_may_read() { + let srv = common::start().await; + let n = unique(); + let (a, b, group) = (format!("va{n}"), format!("vb{n}"), format!("vg{n}")); + for name in [&a, &b] { + create( + &srv, + json!({"name": name, "kind": "hosted", "format": "raw"}), + ) + .await; + } + create( + &srv, + json!({"name": group, "kind": "group", "format": "raw"}), + ) + .await; + assert_eq!(add_member(&srv, &group, &a, 10).await, 201); + assert_eq!(add_member(&srv, &group, &b, 5).await, 201); + // Each member holds a file the selector includes, and a directory of + // three it does not, listed ahead of it. + for (repo, m) in [(&a, "a"), (&b, "b")] { + let files = Files::new(&srv, repo).await; + assert_eq!( + files.put(&format!("public/{m}.txt"), m.as_bytes()).await, + 201 + ); + for i in 1..=3 { + assert_eq!( + files + .put(&format!("internal-{m}/{i}.txt"), b"internal") + .await, + 201 + ); + } + } + // Indexed like `GroupCaller::sees`. + let files = [ + "public/a.txt", + "internal-a/1.txt", + "public/b.txt", + "internal-b/1.txt", + ]; + let directories = ["", "internal-a", "", "internal-b"]; + + for common::GroupCaller { + label, + caller, + sees, + } in srv.group_callers(&group, &a, &b, &["public/**"]).await + { + for (i, file) in files.iter().enumerate() { + let expected = if sees[i] { 200 } else { 404 }; + for path in [file.to_string(), format!("{file}.sha256")] { + let (status, _, _) = read_as(&srv, &caller, &group, &path, "*/*").await; + assert_eq!(status, expected, "{label}: {path}"); + } + } + let names = ["a.txt", "internal-a", "b.txt", "internal-b"]; + let hidden: Vec<&str> = (0..4).filter(|i| !sees[*i]).map(|i| names[i]).collect(); + let mut root = Vec::new(); + for (i, dir) in [(1, "internal-a/"), (3, "internal-b/")] { + if sees[i] { + root.push(dir.to_string()); + } + } + root.push("public/".to_string()); + let (listed, nexts) = walk_as(&srv, &caller, &group, "", &hidden).await; + assert_eq!(listed, root, "{label}"); + assert!( + nexts.iter().all(|next| root.contains(next)), + "{label}: {nexts:?}" + ); + let public: Vec = [(0, "a.txt"), (2, "b.txt")] + .into_iter() + .filter(|(i, _)| sees[*i]) + .map(|(_, name)| name.to_string()) + .collect(); + assert_eq!( + walk_as(&srv, &caller, &group, "public/", &hidden).await.0, + public, + "{label}" + ); + for (i, dir) in directories.iter().enumerate() { + if dir.is_empty() { + continue; + } + if sees[i] { + let (status, _, _) = read_as(&srv, &caller, &group, dir, "*/*").await; + assert_eq!(status, 301, "{label}: {dir}"); + continue; + } + for path in [ + dir.to_string(), + format!("{dir}/"), + format!("{dir}/?limit=1"), + format!("{dir}/?limit=1&format=json"), + ] { + let (status, link, body) = read_as(&srv, &caller, &group, &path, "*/*").await; + assert_eq!((status, link), (404, None), "{label}: {path}: {body}"); + } + } + } + + // A hosted repository's selector hides as much. + let reader = srv.caller().await; + let selector = srv.create_selector(&["public/**"]).await; + srv.grant_role(reader.id, &[(&b, "read", Some(selector))]) + .await; + assert_eq!( + walk_as(&srv, &reader, &b, "", &["internal"]).await, + (vec!["public/".to_string()], vec![]) + ); + for path in ["internal-b", "internal-b/", "internal-b/?limit=1"] { + assert_eq!( + read_as(&srv, &reader, &b, path, "*/*").await.0, + 404, + "{path}" + ); + } + + // And a proxy's, over what it cached. + let upstream = Arc::new(Upstream::default()); + *upstream.content.lock().unwrap() = b"cached".to_vec(); + let base = mock_upstreams(upstream).await; + let proxy = format!("vp{n}"); + create( + &srv, + json!({"name": proxy, "kind": "proxy", "format": "raw", + "remote_url": format!("{base}/releases/download/")}), + ) + .await; + let cache = Files::new(&srv, &proxy).await; + for path in [ + "internal/1.bin", + "internal/2.bin", + "internal/3.bin", + "public/x.bin", + ] { + assert_eq!(cache.get(path).await.status(), 200, "{path}"); + } + let reader = srv.caller().await; + srv.grant_role(reader.id, &[(&proxy, "read", Some(selector))]) + .await; + assert_eq!( + walk_as(&srv, &reader, &proxy, "", &["internal"]).await, + (vec!["public/".to_string()], vec![]) + ); + for path in ["internal/", "internal/?limit=1"] { + assert_eq!( + read_as(&srv, &reader, &proxy, path, "*/*").await.0, + 404, + "{path}" + ); + } +} + #[tokio::test] async fn forms_the_web_ui_and_the_admin_api() { let srv = common::start().await; diff --git a/crates/server/tests/registry.rs b/crates/server/tests/registry.rs index 6e3c2d06..2ece120c 100644 --- a/crates/server/tests/registry.rs +++ b/crates/server/tests/registry.rs @@ -3383,6 +3383,144 @@ async fn group_browse_lists_what_its_pulls_serve() { assert_eq!(listed(reader), [("other/app".to_string(), 1)]); } +/// A group shows a caller what it could pull from its members directly, no +/// more (#648): with a member it may not read, a selector on the group's +/// grant or one on a member's, a hidden image's manifests, blobs, tags and +/// referrers answer as an image of a name like it that does not exist +/// (404, or 403 for a name outside a selector of the group's own grant, +/// which grants no `pull` on it), and the browse API's image list (its +/// count too), image page and chart page leave it out. +#[tokio::test] +async fn groups_show_a_caller_only_the_images_it_may_read() { + let srv = common::start().await; + let (a, b, group) = (repo_name("va"), repo_name("vb"), repo_name("vg")); + for repo in [&a, &b] { + create_repo( + &srv, + json!({"name": repo, "kind": "hosted", "format": "docker"}), + ) + .await; + } + create_repo( + &srv, + json!({"name": group, "kind": "group", "format": "docker"}), + ) + .await; + for (member, priority) in [(&a, 10), (&b, 5)] { + let resp = admin_post( + &srv, + &format!("/admin/repositories/{group}/members"), + json!({"member": member, "priority": priority}), + ) + .await; + assert_eq!(resp.status(), 201); + } + // Indexed like `GroupCaller::sees`: a chart each, signed. + let images = ["public/a", "secret/a", "public/b", "secret/b"]; + let oci = admin_oci(&srv).await; + let mut pushed = Vec::new(); + for (i, image) in images.iter().enumerate() { + let name = format!("{}/{image}", if i < 2 { &a } else { &b }); + let (config, archive, manifest) = helm_chart(&image.replace('/', "-"), "1.0.0"); + oci.push_blob(&name, &config).await; + oci.push_blob(&name, &archive).await; + let resp = oci + .put_manifest(&name, "1.0.0", OCI_MANIFEST, &manifest) + .await; + assert_eq!(resp.status(), 201, "{}", resp.text().await.unwrap()); + let digest = sha(&manifest); + let signature = json!({ + "schemaVersion": 2, + "mediaType": OCI_MANIFEST, + "artifactType": "application/vnd.dev.cosign.artifact.sig.v1+json", + "config": {"mediaType": "application/vnd.oci.empty.v1+json", "digest": sha(b"{}"), "size": 2}, + "layers": [], + "subject": {"mediaType": OCI_MANIFEST, "digest": digest, "size": manifest.len()}, + }) + .to_string() + .into_bytes(); + let resp = oci + .put_manifest(&name, &sha(&signature), OCI_MANIFEST, &signature) + .await; + assert_eq!(resp.status(), 201, "{}", resp.text().await.unwrap()); + pushed.push((digest, sha(&archive))); + } + + for common::GroupCaller { + label, + caller, + sees, + } in srv.group_callers(&group, &a, &b, &["public/*"]).await + { + let oci = Oci::new(&srv, Some((caller.username.clone(), caller.token.clone()))); + let browse = |path: String| { + caller + .apply(srv.client().get(format!("{}{path}", srv.base_url))) + .send() + }; + let mut visible = Vec::new(); + for (i, image) in images.iter().enumerate() { + let name = format!("{group}/{image}"); + // An image of the same directory that does not exist. + let absent = format!("{group}/{}/none", image.split('/').next().unwrap()); + let (digest, archive) = &pushed[i]; + for path in [ + "manifests/1.0.0".to_string(), + format!("manifests/{digest}"), + format!("blobs/{archive}"), + "tags/list".to_string(), + format!("referrers/{digest}"), + ] { + let resp = oci.get(&name, &path).await; + let (status, body) = (resp.status(), resp.text().await.unwrap()); + if sees[i] { + assert_eq!(status, 200, "{label}: {name} {path}"); + continue; + } + let resp = oci.get(&absent, &path).await; + let (expected, absent_body) = (resp.status(), resp.text().await.unwrap()); + // Referrers of an unknown subject are an empty index. + assert!( + matches!(expected.as_u16(), 403 | 404) || path.starts_with("referrers/"), + "{label}: {absent} {path}: {expected}" + ); + assert_eq!(status, expected, "{label}: {name} {path}: {body}"); + assert_eq!( + body.replace(image, ""), + absent_body.replace(&absent[group.len() + 1..], ""), + "{label}: {path}" + ); + } + let expected = if sees[i] { 200 } else { 404 }; + for path in [ + format!("/browse/images/{group}/{image}"), + format!("/browse/chart?repo={group}&image={image}&digest={digest}"), + ] { + let resp = browse(path.clone()).await.unwrap(); + assert_eq!(resp.status(), expected, "{label}: {path}"); + } + if sees[i] { + visible.push(image.to_string()); + } + } + visible.sort_unstable(); + let listed: Value = browse(format!("/browse/images?repo={group}")) + .await + .unwrap() + .json() + .await + .unwrap(); + let names: Vec = listed["items"] + .as_array() + .unwrap() + .iter() + .map(|i| i["image"].as_str().unwrap().to_string()) + .collect(); + assert_eq!(names, visible, "{label}: {listed}"); + assert_eq!(listed["total"], visible.len(), "{label}: {listed}"); + } +} + // ── Admin API ─────────────────────────────────────────────────────────────── #[tokio::test] diff --git a/docs/changes/2.2.0/648.security.md b/docs/changes/2.2.0/648.security.md new file mode 100644 index 00000000..7aa522b8 --- /dev/null +++ b/docs/changes/2.2.0/648.security.md @@ -0,0 +1,13 @@ +A raw listing named entries a content selector hid from the caller: a +page's `next` (and its `Link` header and HTML link) was the last entry the +page read, hidden or not, so following `after=` walked through every hidden +name of a directory, in a hosted, proxy or group repository alike, and a +directory of hidden files alone answered `301` or an empty page rather than +`404` once it held more than a page of them. A page now reads on past +hidden entries until it holds `limit` visible ones (up to 1,000 entries), +its `next` is the last of them, and a directory with no visible entry is +not found. `npm audit` through a group, or a proxy, sent the upstream the +packages a selector of the caller's hid, with the proxy's credentials: an +audit now leaves out the packages the caller's selectors hide on any +repository on the way to the proxy, the proxy's own included, as it leaves +out reserved prefixes. diff --git a/docs/format-plugins.md b/docs/format-plugins.md index 2cd82dc5..dfdff418 100644 --- a/docs/format-plugins.md +++ b/docs/format-plugins.md @@ -147,7 +147,9 @@ in this order: plugin renders the outcome (`auth_response`). - `forward`: a POST to the repository's upstream (a proxy's, or a group's first proxy member's), with the proxy's credentials; its answer - is passed back. npm's audits go so. Needs read. + is passed back. npm's audits go so. Needs read. The body goes without + the packages the reserved prefixes on the way and the caller's + selectors keep inside (`forward_body`), or not at all (400). A host error (a refusal, a conflict, a missing package) is rendered by the plugin's `error_response` when it has one: npm clients print diff --git a/docs/npm.md b/docs/npm.md index c9a58bc0..c23ceac1 100644 --- a/docs/npm.md +++ b/docs/npm.md @@ -276,8 +276,10 @@ repository has no upstream to ask (404). Auditing needs `read`. Through a group, the request leaves out the packages under the reserved prefixes of every group on the way to that proxy: the group's own, and a nested group's when the proxy is its member. Your internal package names -stay inside, as they do when you audit the nested group directly. Every -audit request is filtered: +stay inside, as they do when you audit the nested group directly. It also +leaves out the packages your content selectors hide on any repository on +the way, the proxy's included: you learn of advisories only for packages +you could read through it. Every audit request is filtered: - the bulk request of npm 7 and later (`/-/npm/v1/security/advisories/bulk`), which names each package once; @@ -288,11 +290,11 @@ audit request is filtered: reserved prefix. Of each package only its version, integrity and kind of dependency go on. -A request through such a group that cannot be filtered is refused (400), -never passed on: a body that is not a JSON object, and a legacy request -that names its packages by their paths in `node_modules` (`packages`, the -lockfile of npm 7 and later). A proxy, or a group with no reserved prefixes -on the way, passes the request on as it is. +A request that must be filtered and cannot be is refused (400), never +passed on: a body that is not a JSON object, and a legacy request that +names its packages by their paths in `node_modules` (`packages`, the +lockfile of npm 7 and later). With no reserved prefixes and no selector of +yours on the way, a proxy or a group passes the request on as it is. ## Permissions diff --git a/docs/raw.md b/docs/raw.md index 968f1e8f..167e920d 100644 --- a/docs/raw.md +++ b/docs/raw.md @@ -276,7 +276,16 @@ curl -u admin@example.com: -H 'content-type: application/json' \ A file outside the caller's selector is not found, its sidecars neither, and a listing leaves it out; a directory is listed when the caller may see a file -in it (among its first 1,000). +in it (among its first 1,000). The same holds through a group, whichever +grant holds the selector: the group's, a nested group's or a member's. + +A listing never names what the caller may not see, not in `next` either: a +page reads on past the hidden entries until it holds `limit` visible ones, +and its `next` is the last of them. That read stops after 1,000 entries; a +page that reaches it ends at its last visible entry, and the listing ends +there if it found none, so visible entries behind more than 1,000 hidden +ones in a row are not listed. A directory that holds only hidden entries is +not found (`404`), with or without its `/`, however many it holds. Uploads and deletes are recorded in the audit log (`forge::audit`, "package mutation"), and a workload identity's in the trusted-publishing ledger, with