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