fix(pnpr): clarify publish, unpublish, and Worker errors (#17854)

Validate unpublish versions with the existing publish semver predicate before
reading or mutating storage. Reject build metadata in publish version keys at
the document validation boundary, preserving manifest normalization for
archives whose versions carry build metadata.

Catch startup, forwarding, and container-access failures in the documented
Cloudflare Worker and return a generic availability response with a retry
delay. Exercise the actual deployment snippet with mocked container failures.

Closes https://github.com/zkochan/pnpm-tasks/issues/19.

.changeset/pnpr-clear-validation-errors.md

@@ -0,0 +1,7 @@
+---
+"@pnpm/pnpr": patch
+---
+
+Publishing a version key with build metadata now returns an explicit error. `DELETE` requests for invalid versions return `400`.
+
+[Related task](https://github.com/zkochan/pnpm-tasks/issues/19).

pnpr/crates/pnpr/src/server/package_mutation/unpublish.rs

@@ -150,6 +150,12 @@ pub(crate) async fn delete_tarball(
         Ok(parsed) => parsed,
         Err(err) => return err.into_response(),
     };
+    if !super::super::publishing::is_valid_semver(&version) {
+        return RegistryError::BadRequest {
+            reason: format!("version {version:?} is not valid semver"),
+        }
+        .into_response();
+    }
     let target = match resolve_and_authorize_unpublish(state, identity, registry, &name).await {
         Ok(target) => target,
         Err(response) => return response,

pnpr/crates/pnpr/src/server/publishing.rs

@@ -291,6 +291,11 @@ pub(super) fn check_body_versions(incoming: &Value) -> Result<(), RegistryError>
                 reason: format!("version key {version_key:?} is not valid semver"),
             });
         }
+        if version_key.contains('+') {
+            return Err(RegistryError::BadRequest {
+                reason: format!("cannot publish version {version_key:?} with build metadata"),
+            });
+        }
         let manifest_version = manifest.get("version").and_then(Value::as_str);
         let Some(manifest_version) = manifest_version else {
             return Err(RegistryError::BadRequest {
@@ -311,7 +316,7 @@ pub(super) fn check_body_versions(incoming: &Value) -> Result<(), RegistryError>
 /// Whether `raw` is a version npm clients can install: strict `SemVer` 2.0,
 /// as the `semver` crate parses it for Cargo publishes, within the length and
 /// integer limits of the npm parser pnpr reads stored versions with.
-fn is_valid_semver(raw: &str) -> bool {
+pub(super) fn is_valid_semver(raw: &str) -> bool {
     semver::Version::parse(raw).is_ok() && node_semver::Version::parse(raw).is_ok()
 }
 

pnpr/crates/pnpr/src/server/routing/npm_handlers.rs

@@ -407,7 +407,9 @@ async fn unpublish_version_tarball(
     raw_name: &str,
     version: &str,
 ) -> Response {
-    if super::super::revision_tarballs::parse_revision_version(version).is_some() {
+    if super::super::publishing::is_valid_semver(version)
+        && super::super::revision_tarballs::parse_revision_version(version).is_some()
+    {
         return not_found();
     }
     let canonical = match CanonicalPackageName::parse(raw_name, Ecosystem::Npm) {

pnpr/crates/pnpr/tests/auth_publish/manifest_verification.rs

@@ -301,6 +301,7 @@ async fn a_version_with_build_metadata_cannot_be_published() {
         let response = publish(&app, &token, "widgets", &doc).await;
         assert_eq!(response.status(), StatusCode::BAD_REQUEST, "archive {archive_version}");
         let reason = String::from_utf8(body_bytes(response.into_body()).await).unwrap();
-        assert!(reason.contains("version"), "{reason}");
+        assert!(reason.contains("cannot publish version"), "{reason}");
+        assert!(reason.contains("with build metadata"), "{reason}");
     }
 }

pnpr/crates/pnpr/tests/auth_publish/publish_validation.rs

@@ -1053,3 +1053,60 @@ async fn publish_supports_scoped_packages() {
         .unwrap();
     assert_eq!(response.status(), StatusCode::OK);
 }
+
+#[tokio::test]
+async fn unpublish_rejects_invalid_versions() {
+    let tmp = TempDir::new().unwrap();
+    let app = router(static_config(tmp.path().to_path_buf()));
+    let (app, token) = add_user_and_get_token(app, "publisher", "secret123").await;
+    for name in ["widgets", "@example/widgets"] {
+        let archive = npm_tgz(name, "1.0.0", b"module.exports = 1");
+        let mut request = put_json(&format!("/npm/{name}"), publish_doc(name, "1.0.0", &archive));
+        request
+            .headers_mut()
+            .insert("Authorization", format!("Bearer {token}").parse().unwrap());
+        let response = app
+            .clone()
+            .oneshot(request)
+            .await
+            .unwrap();
+        assert_eq!(response.status(), StatusCode::CREATED);
+    }
+    for path in [
+        "/npm/widgets@not-a-version",
+        "/npm/widgets@not-a-version+r1",
+        "/npm/@example/widgets@not-a-version+r1",
+        "/npm/@example/widgets@not-a-version",
+        "/npm/widgets/-/widgets-not-a-version.tgz",
+        "/npm/@example/widgets/-/widgets-not-a-version.tgz",
+        "/npm/widgets@01.0.0/-rev/1",
+        "/npm/@example/widgets@01.0.0/-rev/1",
+    ] {
+        let response = app
+            .clone()
+            .oneshot(
+                Request::delete(path)
+                    .header("Authorization", format!("Bearer {token}"))
+                    .body(Body::empty())
+                    .unwrap(),
+            )
+            .await
+            .unwrap();
+        assert_eq!(response.status(), StatusCode::BAD_REQUEST, "{path}");
+        let reason = String::from_utf8(body_bytes(response.into_body()).await).unwrap();
+        assert!(reason.contains("not valid semver"), "{reason}");
+    }
+    for path in ["/npm/widgets@1.0.0+r1", "/npm/@example/widgets@1.0.0+r1"] {
+        let response = app
+            .clone()
+            .oneshot(
+                Request::delete(path)
+                    .header("Authorization", format!("Bearer {token}"))
+                    .body(Body::empty())
+                    .unwrap(),
+            )
+            .await
+            .unwrap();
+        assert_eq!(response.status(), StatusCode::NOT_FOUND, "{path}");
+    }
+}

pnpr/docs/cloudflare.md

@@ -176,22 +176,29 @@ export class PnprContainer extends Container<Env> {
   // Wait longer than the library's default for a cold start, so the first
   // request after a sleep gets an answer instead of a 500.
   override async fetch(request: Request): Promise<Response> {
-    if (!this.ctx.container?.running) {
-      try {
+    try {
+      if (!this.ctx.container?.running) {
         await this.startAndWaitForPorts(7677, {
           instanceGetTimeoutMS: 120_000,
           portReadyTimeoutMS: 180_000,
           abort: request.signal,
         });
-      } catch (error) {
-        const message = error instanceof Error ? error.message : String(error);
-        return new Response(`pnpr is starting, retry shortly (${message})\n`, { status: 503 });
       }
+      return await super.fetch(request);
+    } catch (error) {
+      return unavailable(error);
     }
-    return super.fetch(request);
   }
 }
 
+function unavailable(error: unknown): Response {
+  console.error("Registry request failed", error);
+  return new Response("Registry temporarily unavailable, retry shortly\n", {
+    status: 503,
+    headers: { "Retry-After": "5" },
+  });
+}
+
 export default {
   async fetch(request: Request, env: Env): Promise<Response> {
     try {
@@ -204,15 +211,19 @@ export default {
     headers.set("accept-encoding", "zstd, gzip");
     headers.delete("x-forwarded-for");
     headers.set("x-real-ip", request.headers.get("cf-connecting-ip") ?? "");
-    const response = await getContainer(env.PNPR, "pnpr").fetch(
-      new Request(request, { headers }),
-    );
-    return new Response(response.body, {
-      status: response.status,
-      statusText: response.statusText,
-      headers: response.headers,
-      encodeBody: "manual",
-    });
+    try {
+      const response = await getContainer(env.PNPR, "pnpr").fetch(
+        new Request(request, { headers }),
+      );
+      return new Response(response.body, {
+        status: response.status,
+        statusText: response.statusText,
+        headers: response.headers,
+        encodeBody: "manual",
+      });
+    } catch (error) {
+      return unavailable(error);
+    }
   },
 };
 ```

pnpr/docs/endpoints.md

@@ -478,7 +478,9 @@ name and target rules. A package-document update cannot remove `latest` while
 published versions remain.
 
 Every key under `versions` and its manifest's `version` must be valid SemVer.
-pnpr rejects an invalid version with `400`.
+pnpr rejects an invalid version with `400`, including versions in DELETE requests.
+Publish version keys with build metadata, such as `1.0.0+build.5`, return `400`
+with a message explaining that build metadata cannot be published.
 
 ### Packument revisions
 

pnpr/examples/cloudflare/worker.test.mjs

@@ -1,16 +1,55 @@
 import assert from 'node:assert/strict'
-import { Buffer } from 'node:buffer'
 import { readFile } from 'node:fs/promises'
 import { stripTypeScriptTypes } from 'node:module'
 import { test } from 'node:test'
 
 const guide = await readFile(new globalThis.URL('../../docs/cloudflare.md', import.meta.url), 'utf8')
-const source = guide.match(/```ts title="src\/index.ts"\n([\s\S]*?)\n```/)[1]
-const stubbed = source.replace(
-  'import { Container, getContainer } from "@cloudflare/containers";',
-  'class Container {}; function getContainer(env) { return env }',
-)
-const { default: worker } = await import(`data:text/javascript;base64,${Buffer.from(stripTypeScriptTypes(stubbed)).toString('base64')}`)
+const source = guide.split('```ts title="src/index.ts"\n')[1].split('\n```')[0]
+const javascript = stripTypeScriptTypes(source.replace(/^import .*;\n/m, ''))
+  .replace('export class ', 'class ')
+  .replace('export default ', 'const worker = ')
+
+function fixture ({ startupError = false, forwardingError = false, workerError = false } = {}) {
+  const errors = []
+  const logger = { error: (...args) => errors.push(args) }
+  class Container {
+    constructor () { this.ctx = { container: { running: !startupError } } }
+    async startAndWaitForPorts () { throw new Error('private startup details') }
+    async fetch () {
+      if (forwardingError) throw new Error('private forwarding details')
+      return new globalThis.Response('registry answer', { headers: { 'x-registry': 'yes' } })
+    }
+  }
+  const getContainer = (namespace) => {
+    if (workerError) throw new Error('private Worker details')
+    return namespace ?? new classes.PnprContainer({}, {})
+  }
+  const classes = new Function('Container', 'getContainer', 'console', `${javascript}\nreturn { PnprContainer, worker };`)(Container, getContainer, logger)
+  return { ...classes, errors }
+}
+
+for (const failure of ['startupError', 'forwardingError', 'workerError']) {
+  test(`the deployment Worker hides ${failure} and supplies a retry delay`, async () => {
+    const { worker, errors } = fixture({ [failure]: true })
+    const response = await worker.fetch(new globalThis.Request('https://registry.example/npm/widgets'), {})
+    assert.equal(response.status, 503)
+    assert.equal(response.headers.get('retry-after'), '5')
+    assert.equal(await response.text(), 'Registry temporarily unavailable, retry shortly\n')
+    assert.equal(errors.length, 1)
+    assert.equal(errors[0][0], 'Registry request failed')
+    assert.match(errors[0][1].stack, /private .* details/)
+  })
+}
+
+test('the deployment Worker preserves a successful registry response', async () => {
+  const { worker } = fixture()
+  const response = await worker.fetch(new globalThis.Request('https://registry.example/npm/widgets'), {})
+  assert.equal(response.status, 200)
+  assert.equal(response.headers.get('x-registry'), 'yes')
+  assert.equal(await response.text(), 'registry answer')
+})
+
+const { worker } = fixture()
 
 for (const method of ['GET', 'HEAD', 'PUT', 'DELETE', 'POST']) {
   for (const path of ['/npm/%E0%A4%A', '/npm/%', '/npm/%GG', '/npm/widget@%FF', '/npm/~example/%C0%AF', '/other/%ED%A0%80']) {
@@ -41,9 +80,14 @@ for (const path of ['/npm/widget', '/npm/@example%2Fwidget', '/npm/%E2%9C%93', '
   })
 }
 
-test('container errors propagate', async () => {
+test('container URI errors return an availability response', async () => {
+  const { worker, errors } = fixture()
   const error = new URIError('origin failure')
-  await assert.rejects(worker.fetch(new globalThis.Request('https://registry.example/npm/widget'), {
+  const response = await worker.fetch(new globalThis.Request('https://registry.example/npm/widget'), {
     PNPR: { fetch () { throw error } },
-  }), error)
+  })
+  assert.equal(response.status, 503)
+  assert.equal(response.headers.get('retry-after'), '5')
+  assert.equal(await response.text(), 'Registry temporarily unavailable, retry shortly\n')
+  assert.equal(errors[0][1], error)
 })