From 85d9e7708a8b5714dc6c404f66981dea9de499bf Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Mon, 22 Jun 2026 16:29:12 +0200 Subject: [PATCH] fix(pnpr): enforce unpublish authorization (#12579) Enforce pnpr's unpublish package policy independently from publish authorization. pnpr already parsed an `unpublish` field from Verdaccio-style package rules, but destructive registry writes still checked only the publish permission. That let a caller who was allowed to publish but excluded from unpublish delete packages, delete tarballs, or replace a packument. The policy model now carries an effective unpublish access list. Missing, null, or empty unpublish values deny destructive writes. The bundled config and README examples set unpublish explicitly where authenticated users should keep destructive-write access. Package deletion and tarball deletion now check Action::Unpublish. Full packument replacement checks both publish and unpublish because it can rewrite package contents while also serving the partial-unpublish flow. Regression coverage exercises missing and explicit deny behavior, publish-authorized package deletion denial, publish-authorized tarball deletion denial, and packument replacement requiring both permissions. --- pnpr/crates/pnpr/config.yaml | 10 ++ pnpr/crates/pnpr/src/config.rs | 31 ++--- pnpr/crates/pnpr/src/config/tests.rs | 57 +++++++++ pnpr/crates/pnpr/src/policy.rs | 44 +++++-- pnpr/crates/pnpr/src/policy/tests.rs | 4 + pnpr/crates/pnpr/src/server.rs | 13 +- pnpr/crates/pnpr/tests/auth_publish.rs | 164 ++++++++++++++++++++++++- pnpr/npm/pnpr/README.md | 3 + 8 files changed, 290 insertions(+), 36 deletions(-) diff --git a/pnpr/crates/pnpr/config.yaml b/pnpr/crates/pnpr/config.yaml index b9b73a5eef..2f9fb117c9 100644 --- a/pnpr/crates/pnpr/config.yaml +++ b/pnpr/crates/pnpr/config.yaml @@ -76,43 +76,53 @@ packages: '@private/*': access: $authenticated publish: $authenticated + unpublish: $authenticated '@pnpm.e2e/needs-auth': access: $authenticated publish: $authenticated + unpublish: $authenticated '@*/*': access: $all publish: $authenticated + unpublish: $authenticated proxy: npmjs 'plugin-example': access: $all publish: $authenticated + unpublish: $authenticated 'pkg-with-1-dep': access: $all publish: $authenticated + unpublish: $authenticated 'dep-of-pkg-with-1-dep': access: $all publish: $authenticated + unpublish: $authenticated 'foobar': access: $all publish: $authenticated + unpublish: $authenticated 'alpha': access: $all publish: $authenticated + unpublish: $authenticated 'deprecated': access: $all publish: $authenticated + unpublish: $authenticated '**': access: $all publish: $authenticated + unpublish: $authenticated proxy: npmjs # log settings (verdaccio 6+ shape: a single `log:` object, not a list) diff --git a/pnpr/crates/pnpr/src/config.rs b/pnpr/crates/pnpr/src/config.rs index 64bf31639b..cd899460a3 100644 --- a/pnpr/crates/pnpr/src/config.rs +++ b/pnpr/crates/pnpr/src/config.rs @@ -82,9 +82,9 @@ pub struct Config { /// re-fetched from the resolved uplink. Ignored when no uplink /// matches. pub packument_ttl: Duration, - /// Per-package access and publish rules. [`Config::from_yaml`] + /// Per-package access, publish, and unpublish rules. [`Config::from_yaml`] /// compiles these from the YAML `packages:` block (each entry's - /// `access` / `publish` tokens); the programmatic + /// `access` / `publish` / `unpublish` tokens); the programmatic /// [`Config::proxy`] / [`Config::static_serve`] constructors use /// [`PackagePolicies::registry_mock_defaults`] instead, enforcing /// the `@private/*` and `@pnpm.e2e/needs-auth` rules @@ -757,12 +757,11 @@ fn non_empty_token(token: &str) -> Option { (!token.trim().is_empty()).then(|| token.to_string()) } -/// Per-package routing and access rules. `access` / `publish` are -/// verdaccio permission lists (built-in groups like `$all` / -/// `$authenticated` / `$anonymous`, plus usernames / group names), -/// compiled into the [`PackagePolicies`] that gate reads and writes. -/// `unpublish` is parsed but currently folded into `publish` at -/// enforcement time. `proxy` selects the [`UplinkConfig`] by name. +/// Per-package routing and access rules. `access` / `publish` / +/// `unpublish` are verdaccio permission lists (built-in groups like +/// `$all` / `$authenticated` / `$anonymous`, plus usernames / group +/// names), compiled into the [`PackagePolicies`] that gate reads and +/// writes. `proxy` selects the [`UplinkConfig`] by name. #[derive(Debug, Default, Clone, Deserialize)] pub struct PackageAccess { pub access: Option, @@ -1426,11 +1425,11 @@ fn build_log_config(entry: Option<&LogEntryFile>) -> LogConfig { /// Compile the YAML `packages:` rules into the runtime /// [`PackagePolicies`], in declared order (first match wins). A /// missing `access` defaults to `$all`, a missing `publish` to -/// `$authenticated` — the same safe fallback [`PackagePolicies`] -/// applies to packages no rule matches. `unpublish` is parsed for -/// config compatibility but not yet enforced separately (it folds -/// into `publish`). Errors only on an invalid glob pattern — any -/// token string is a valid group/username, as in verdaccio. +/// `$authenticated`, and a missing, empty, or null `unpublish` denies +/// destructive writes. The same safe fallback [`PackagePolicies`] +/// applies to packages no rule matches. Errors only on an invalid glob +/// pattern — any token string is a valid group/username, as in +/// verdaccio. fn build_policies( packages: &IndexMap, ) -> Result { @@ -1445,7 +1444,11 @@ fn build_policies( .publish .as_ref() .map_or_else(|| AccessList::parse("$authenticated"), AccessSpec::to_access_list); - PackagePolicy::new(pattern, access_list, publish_list) + let unpublish_list = access + .unpublish + .as_ref() + .map_or_else(AccessList::default, AccessSpec::to_access_list); + PackagePolicy::new(pattern, access_list, publish_list, unpublish_list) }) .collect::, _>>()?; Ok(PackagePolicies::new(rules)) diff --git a/pnpr/crates/pnpr/src/config/tests.rs b/pnpr/crates/pnpr/src/config/tests.rs index 75e173949e..07d4ca5d65 100644 --- a/pnpr/crates/pnpr/src/config/tests.rs +++ b/pnpr/crates/pnpr/src/config/tests.rs @@ -1397,6 +1397,7 @@ packages: '@secret/*': access: $authenticated publish: $authenticated + unpublish: admin '**': access: $all publish: $authenticated @@ -1408,6 +1409,8 @@ packages: let public = config.policies.for_package("lodash"); assert!(public.access.allows(&Identity::Anonymous)); assert!(!public.publish.allows(&Identity::Anonymous)); + assert!(!secret.unpublish.allows(&user("alice"))); + assert!(secret.unpublish.allows(&user("admin"))); } #[test] @@ -1441,6 +1444,60 @@ packages: assert!(effective.access.allows(&Identity::Anonymous)); assert!(!effective.publish.allows(&Identity::Anonymous)); assert!(effective.publish.allows(&user("alice"))); + assert!(!effective.unpublish.allows(&Identity::Anonymous)); + assert!(!effective.unpublish.allows(&user("alice"))); +} + +#[test] +fn policy_missing_unpublish_denies_destructive_writes() { + let yaml = "\ +storage: ./s +uplinks: {} +packages: + '@team/*': + publish: alice +"; + let config = Config::from_yaml_str(yaml, Path::new("/x"), listen(), None).unwrap(); + let team = config.policies.for_package("@team/x"); + assert!(team.publish.allows(&user("alice"))); + assert!(!team.publish.allows(&user("bob"))); + assert!(!team.unpublish.allows(&user("alice"))); + assert!(!team.unpublish.allows(&user("bob"))); +} + +#[test] +fn policy_empty_unpublish_denies_destructive_writes() { + let as_null = "\ +storage: ./s +uplinks: {} +packages: + '@team/*': + publish: $authenticated + unpublish: +"; + let as_empty_string = "\ +storage: ./s +uplinks: {} +packages: + '@team/*': + publish: $authenticated + unpublish: '' +"; + let as_empty_sequence = "\ +storage: ./s +uplinks: {} +packages: + '@team/*': + publish: $authenticated + unpublish: [] +"; + for yaml in [as_null, as_empty_string, as_empty_sequence] { + let config = Config::from_yaml_str(yaml, Path::new("/x"), listen(), None).unwrap(); + let team = config.policies.for_package("@team/x"); + assert!(team.publish.allows(&user("alice")), "{yaml}"); + assert!(!team.unpublish.allows(&Identity::Anonymous), "{yaml}"); + assert!(!team.unpublish.allows(&user("alice")), "{yaml}"); + } } #[test] diff --git a/pnpr/crates/pnpr/src/policy.rs b/pnpr/crates/pnpr/src/policy.rs index c9cb4b5c12..2695b40de9 100644 --- a/pnpr/crates/pnpr/src/policy.rs +++ b/pnpr/crates/pnpr/src/policy.rs @@ -110,12 +110,13 @@ impl Identity { /// compiled from the verdaccio-style key (e.g. `@private/*`, /// `@pnpm.e2e/needs-auth`, `**`). `access` controls who can read the /// packument and tarballs; `publish` controls who can publish or -/// change dist-tags. +/// change dist-tags; `unpublish` controls destructive writes. #[derive(Debug, Clone)] pub struct PackagePolicy { pattern: Glob<'static>, pub access: AccessList, pub publish: AccessList, + pub unpublish: AccessList, } impl PackagePolicy { @@ -123,6 +124,7 @@ impl PackagePolicy { pattern: &str, access: AccessList, publish: AccessList, + unpublish: AccessList, ) -> Result { let glob = Glob::new(pattern) .map_err(|err| RegistryError::InvalidPolicyPattern { @@ -130,7 +132,7 @@ impl PackagePolicy { reason: err.to_string(), })? .into_owned(); - Ok(Self { pattern: glob, access, publish }) + Ok(Self { pattern: glob, access, publish, unpublish }) } fn matches(&self, package: &str) -> bool { @@ -149,6 +151,7 @@ pub struct PackagePolicies { /// borrow. default_access: AccessList, default_publish: AccessList, + default_unpublish: AccessList, } impl Default for PackagePolicies { @@ -163,6 +166,7 @@ impl Default for PackagePolicies { pub struct Effective<'a> { pub access: &'a AccessList, pub publish: &'a AccessList, + pub unpublish: &'a AccessList, } impl PackagePolicies { @@ -172,6 +176,7 @@ impl PackagePolicies { rules, default_access: AccessList::parse("$all"), default_publish: AccessList::parse("$authenticated"), + default_unpublish: AccessList::default(), } } @@ -179,21 +184,26 @@ impl PackagePolicies { /// box `Config` already enforces the same access rules verdaccio /// did. The relevant patterns from `registry-mock`'s `config.yaml`: /// - /// * `@private/*` — authenticated access + publish - /// * `@pnpm.e2e/needs-auth` — authenticated access + publish - /// * everything else — $all access, $authenticated publish + /// * `@private/*` — authenticated access + publish + unpublish + /// * `@pnpm.e2e/needs-auth` — authenticated access + publish + unpublish + /// * everything else — $all access, $authenticated publish + unpublish #[must_use] pub fn registry_mock_defaults() -> Self { let rules = [ - ("@private/*", "$authenticated", "$authenticated"), - ("@pnpm.e2e/needs-auth", "$authenticated", "$authenticated"), - ("**", "$all", "$authenticated"), + ("@private/*", "$authenticated", "$authenticated", "$authenticated"), + ("@pnpm.e2e/needs-auth", "$authenticated", "$authenticated", "$authenticated"), + ("**", "$all", "$authenticated", "$authenticated"), ]; let rules = rules .into_iter() - .map(|(pattern, access, publish)| { - PackagePolicy::new(pattern, AccessList::parse(access), AccessList::parse(publish)) - .expect("registry-mock defaults compile") + .map(|(pattern, access, publish, unpublish)| { + PackagePolicy::new( + pattern, + AccessList::parse(access), + AccessList::parse(publish), + AccessList::parse(unpublish), + ) + .expect("registry-mock defaults compile") }) .collect(); Self::new(rules) @@ -203,10 +213,18 @@ impl PackagePolicies { pub fn for_package(&self, package: &str) -> Effective<'_> { for rule in &self.rules { if rule.matches(package) { - return Effective { access: &rule.access, publish: &rule.publish }; + return Effective { + access: &rule.access, + publish: &rule.publish, + unpublish: &rule.unpublish, + }; } } - Effective { access: &self.default_access, publish: &self.default_publish } + Effective { + access: &self.default_access, + publish: &self.default_publish, + unpublish: &self.default_unpublish, + } } } diff --git a/pnpr/crates/pnpr/src/policy/tests.rs b/pnpr/crates/pnpr/src/policy/tests.rs index b39eb6d506..76c64a7e97 100644 --- a/pnpr/crates/pnpr/src/policy/tests.rs +++ b/pnpr/crates/pnpr/src/policy/tests.rs @@ -79,6 +79,8 @@ fn defaults_match_registry_mock_config() { assert!(public.access.allows(&Identity::Anonymous)); assert!(!public.publish.allows(&Identity::Anonymous)); assert!(public.publish.allows(&user("alice"))); + assert!(!public.unpublish.allows(&Identity::Anonymous)); + assert!(public.unpublish.allows(&user("alice"))); } #[test] @@ -96,4 +98,6 @@ fn falls_back_to_safe_defaults_when_no_rules_match() { assert!(effective.access.allows(&Identity::Anonymous)); assert!(!effective.publish.allows(&Identity::Anonymous)); assert!(effective.publish.allows(&user("alice"))); + assert!(!effective.unpublish.allows(&Identity::Anonymous)); + assert!(!effective.unpublish.allows(&user("alice"))); } diff --git a/pnpr/crates/pnpr/src/server.rs b/pnpr/crates/pnpr/src/server.rs index 434fd85d27..2037cbdbdc 100644 --- a/pnpr/crates/pnpr/src/server.rs +++ b/pnpr/crates/pnpr/src/server.rs @@ -1747,8 +1747,10 @@ async fn update_packument( Ok(n) => n, Err(err) => return error_response(&err), }; - if let Err(err) = authorize(state, identity, name.as_str(), Action::Publish) { - return error_response(&err); + for action in [Action::Publish, Action::Unpublish] { + if let Err(err) = authorize(state, identity, name.as_str(), action) { + return error_response(&err); + } } let mut packument: Value = match serde_json::from_slice(body) { Ok(v) => v, @@ -1786,7 +1788,7 @@ async fn delete_package(state: &AppState, identity: &Identity, raw_name: &str) - Ok(n) => n, Err(err) => return error_response(&err), }; - if let Err(err) = authorize(state, identity, name.as_str(), Action::Publish) { + if let Err(err) = authorize(state, identity, name.as_str(), Action::Unpublish) { return error_response(&err); } // Serialize against same-package publishers so a delete can't race a @@ -1823,7 +1825,7 @@ async fn delete_tarball( Ok(c) => c, Err(err) => return error_response(&err), }; - if let Err(err) = authorize(state, identity, name.as_str(), Action::Publish) { + if let Err(err) = authorize(state, identity, name.as_str(), Action::Unpublish) { return error_response(&err); } // Serialize against same-package publishers so a delete can't race a @@ -2010,6 +2012,7 @@ where enum Action { Access, Publish, + Unpublish, } impl Action { @@ -2017,6 +2020,7 @@ impl Action { match self { Action::Access => "access", Action::Publish => "publish", + Action::Unpublish => "unpublish", } } } @@ -2154,6 +2158,7 @@ fn authorize( let list = match action { Action::Access => effective.access, Action::Publish => effective.publish, + Action::Unpublish => effective.unpublish, }; if list.allows(identity) { return Ok(()); diff --git a/pnpr/crates/pnpr/tests/auth_publish.rs b/pnpr/crates/pnpr/tests/auth_publish.rs index 05040f9b21..43ba266ad3 100644 --- a/pnpr/crates/pnpr/tests/auth_publish.rs +++ b/pnpr/crates/pnpr/tests/auth_publish.rs @@ -27,6 +27,19 @@ fn static_config(storage: PathBuf) -> Config { config } +fn static_config_with_packages(dir: &TempDir, packages_block: &str) -> (Config, PathBuf) { + let listen = SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::LOCALHOST, 4873)); + let storage = dir.path().join("storage"); + std::fs::create_dir_all(&storage).unwrap(); + let yaml = + format!("storage: {}\nuplinks: {{}}\npackages:\n{packages_block}\n", storage.display()); + let config_path = dir.path().join("config.yaml"); + std::fs::write(&config_path, yaml).unwrap(); + let config = + Config::from_yaml(&config_path, listen, Some("http://example.test".to_string())).unwrap(); + (config, storage) +} + async fn body_bytes(body: Body) -> Vec { to_bytes(body, usize::MAX).await.expect("read body").to_vec() } @@ -1155,12 +1168,153 @@ async fn unpublish_scoped_tarball_via_six_segment_route() { } #[tokio::test] -async fn unpublish_requires_publish_auth() { +async fn missing_unpublish_policy_denies_destructive_writes() { let tmp = TempDir::new().unwrap(); - let app = router(static_config(tmp.path().to_path_buf())); - let request = Request::delete("/some-pkg/-rev/anything").body(Body::empty()).unwrap(); - let response = app.oneshot(request).await.unwrap(); - assert_eq!(response.status(), StatusCode::UNAUTHORIZED); + let (config, storage) = static_config_with_packages( + &tmp, + " 'missing-unpublish': + access: $all + publish: alice", + ); + let app = router(config); + let (app, alice) = add_user_and_get_token(app, "alice", "secret").await; + + let body = sample_publish_body("missing-unpublish", "1.0.0", b"contents"); + let request = Request::put("/missing-unpublish") + .header("content-type", "application/json") + .header("Authorization", format!("Bearer {alice}")) + .body(Body::from(serde_json::to_vec(&body).unwrap())) + .unwrap(); + assert_eq!(app.clone().oneshot(request).await.unwrap().status(), StatusCode::CREATED); + + let request = Request::delete("/missing-unpublish/-rev/anything") + .header("Authorization", format!("Bearer {alice}")) + .body(Body::empty()) + .unwrap(); + assert_eq!(app.clone().oneshot(request).await.unwrap().status(), StatusCode::FORBIDDEN); + assert!(storage.join("missing-unpublish/package.json").exists()); +} + +#[tokio::test] +async fn unpublish_policy_denies_publish_authorized_package_delete() { + let tmp = TempDir::new().unwrap(); + let (config, storage) = static_config_with_packages( + &tmp, + " 'unpub-policy': + access: $all + publish: $authenticated + unpublish: admin", + ); + let app = router(config); + let (app, alice) = add_user_and_get_token(app, "alice", "secret").await; + let (app, admin) = add_user_and_get_token(app, "admin", "secret").await; + + let body = sample_publish_body("unpub-policy", "1.0.0", b"contents"); + let request = Request::put("/unpub-policy") + .header("content-type", "application/json") + .header("Authorization", format!("Bearer {alice}")) + .body(Body::from(serde_json::to_vec(&body).unwrap())) + .unwrap(); + assert_eq!(app.clone().oneshot(request).await.unwrap().status(), StatusCode::CREATED); + assert!(storage.join("unpub-policy/package.json").exists()); + + let request = Request::delete("/unpub-policy/-rev/anything") + .header("Authorization", format!("Bearer {alice}")) + .body(Body::empty()) + .unwrap(); + assert_eq!(app.clone().oneshot(request).await.unwrap().status(), StatusCode::FORBIDDEN); + assert!(storage.join("unpub-policy/package.json").exists()); + + let request = Request::delete("/unpub-policy/-rev/anything") + .header("Authorization", format!("Bearer {admin}")) + .body(Body::empty()) + .unwrap(); + assert_eq!(app.clone().oneshot(request).await.unwrap().status(), StatusCode::CREATED); + assert!(!storage.join("unpub-policy").exists()); +} + +#[tokio::test] +async fn unpublish_policy_denies_publish_authorized_tarball_delete() { + let tmp = TempDir::new().unwrap(); + let (config, storage) = static_config_with_packages( + &tmp, + " 'tarball-policy': + access: $all + publish: $authenticated + unpublish: admin", + ); + let app = router(config); + let (app, alice) = add_user_and_get_token(app, "alice", "secret").await; + let (app, admin) = add_user_and_get_token(app, "admin", "secret").await; + + let body = sample_publish_body("tarball-policy", "1.0.0", b"contents"); + let request = Request::put("/tarball-policy") + .header("content-type", "application/json") + .header("Authorization", format!("Bearer {alice}")) + .body(Body::from(serde_json::to_vec(&body).unwrap())) + .unwrap(); + assert_eq!(app.clone().oneshot(request).await.unwrap().status(), StatusCode::CREATED); + assert!(storage.join("tarball-policy/tarball-policy-1.0.0.tgz").exists()); + + let request = Request::delete("/tarball-policy/-/tarball-policy-1.0.0.tgz/-rev/anything") + .header("Authorization", format!("Bearer {alice}")) + .body(Body::empty()) + .unwrap(); + assert_eq!(app.clone().oneshot(request).await.unwrap().status(), StatusCode::FORBIDDEN); + assert!(storage.join("tarball-policy/tarball-policy-1.0.0.tgz").exists()); + + let request = Request::delete("/tarball-policy/-/tarball-policy-1.0.0.tgz/-rev/anything") + .header("Authorization", format!("Bearer {admin}")) + .body(Body::empty()) + .unwrap(); + assert_eq!(app.clone().oneshot(request).await.unwrap().status(), StatusCode::CREATED); + assert!(!storage.join("tarball-policy/tarball-policy-1.0.0.tgz").exists()); +} + +#[tokio::test] +async fn packument_replacement_requires_publish_and_unpublish_policy() { + let tmp = TempDir::new().unwrap(); + let (config, storage) = static_config_with_packages( + &tmp, + " 'replace-policy': + access: $all + publish: $authenticated + unpublish: admin", + ); + let app = router(config); + let (app, alice) = add_user_and_get_token(app, "alice", "secret").await; + let (app, admin) = add_user_and_get_token(app, "admin", "secret").await; + + let body = sample_publish_body("replace-policy", "1.0.0", b"contents"); + let request = Request::put("/replace-policy") + .header("content-type", "application/json") + .header("Authorization", format!("Bearer {alice}")) + .body(Body::from(serde_json::to_vec(&body).unwrap())) + .unwrap(); + assert_eq!(app.clone().oneshot(request).await.unwrap().status(), StatusCode::CREATED); + + let packument_path = storage.join("replace-policy/package.json"); + let mut replacement: Value = + serde_json::from_slice(&std::fs::read(&packument_path).unwrap()).unwrap(); + replacement["owner"] = json!("admin"); + + let request = Request::put("/replace-policy/-rev/anything") + .header("content-type", "application/json") + .header("Authorization", format!("Bearer {alice}")) + .body(Body::from(serde_json::to_vec(&replacement).unwrap())) + .unwrap(); + assert_eq!(app.clone().oneshot(request).await.unwrap().status(), StatusCode::FORBIDDEN); + let on_disk: Value = serde_json::from_slice(&std::fs::read(&packument_path).unwrap()).unwrap(); + assert!(on_disk.get("owner").is_none()); + + let request = Request::put("/replace-policy/-rev/anything") + .header("content-type", "application/json") + .header("Authorization", format!("Bearer {admin}")) + .body(Body::from(serde_json::to_vec(&replacement).unwrap())) + .unwrap(); + assert_eq!(app.clone().oneshot(request).await.unwrap().status(), StatusCode::CREATED); + let on_disk: Value = serde_json::from_slice(&std::fs::read(&packument_path).unwrap()).unwrap(); + assert_eq!(on_disk["owner"], "admin"); } /// Two different versions of the same package, published concurrently, diff --git a/pnpr/npm/pnpr/README.md b/pnpr/npm/pnpr/README.md index 8b48fb2e38..cdaa2a990a 100644 --- a/pnpr/npm/pnpr/README.md +++ b/pnpr/npm/pnpr/README.md @@ -65,11 +65,13 @@ packages: '@*/*': access: $all publish: $authenticated + unpublish: $authenticated proxy: npmjs '**': access: $all publish: $authenticated + unpublish: $authenticated proxy: npmjs ``` @@ -156,6 +158,7 @@ packages: '**': access: $all publish: $authenticated + unpublish: $authenticated proxy: npmjs ```