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

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

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<String> {
     (!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<AccessSpec>,
@@ -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<String, PackageAccess>,
 ) -> Result<PackagePolicies, RegistryError> {
@@ -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::<Result<Vec<_>, _>>()?;
     Ok(PackagePolicies::new(rules))

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]

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<Self, RegistryError> {
         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,
+        }
     }
 }
 

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

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(());

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<u8> {
     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,

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
 ```