From 40cf3cedba3c00a71b3ef8c3db2815413c800400 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Sat, 7 Nov 2026 19:55:33 +0100 Subject: [PATCH] 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 | 7 ++ .../src/server/package_mutation/unpublish.rs | 6 ++ pnpr/crates/pnpr/src/server/publishing.rs | 7 +- .../pnpr/src/server/routing/npm_handlers.rs | 4 +- .../auth_publish/manifest_verification.rs | 3 +- .../tests/auth_publish/publish_validation.rs | 57 +++++++++++++++++ pnpr/docs/cloudflare.md | 41 +++++++----- pnpr/docs/endpoints.md | 4 +- pnpr/examples/cloudflare/worker.test.mjs | 64 ++++++++++++++++--- 9 files changed, 164 insertions(+), 29 deletions(-) create mode 100644 .changeset/pnpr-clear-validation-errors.md diff --git a/.changeset/pnpr-clear-validation-errors.md b/.changeset/pnpr-clear-validation-errors.md new file mode 100644 index 0000000000..0ac9482c77 --- /dev/null +++ b/.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). diff --git a/pnpr/crates/pnpr/src/server/package_mutation/unpublish.rs b/pnpr/crates/pnpr/src/server/package_mutation/unpublish.rs index 68eb754151..280c85a0da 100644 --- a/pnpr/crates/pnpr/src/server/package_mutation/unpublish.rs +++ b/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, diff --git a/pnpr/crates/pnpr/src/server/publishing.rs b/pnpr/crates/pnpr/src/server/publishing.rs index 7c8d363bae..e85e6fbf41 100644 --- a/pnpr/crates/pnpr/src/server/publishing.rs +++ b/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() } diff --git a/pnpr/crates/pnpr/src/server/routing/npm_handlers.rs b/pnpr/crates/pnpr/src/server/routing/npm_handlers.rs index 5508f7ddb0..7c43025ee9 100644 --- a/pnpr/crates/pnpr/src/server/routing/npm_handlers.rs +++ b/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) { diff --git a/pnpr/crates/pnpr/tests/auth_publish/manifest_verification.rs b/pnpr/crates/pnpr/tests/auth_publish/manifest_verification.rs index 8dd11b8af1..555ddaaa0b 100644 --- a/pnpr/crates/pnpr/tests/auth_publish/manifest_verification.rs +++ b/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}"); } } diff --git a/pnpr/crates/pnpr/tests/auth_publish/publish_validation.rs b/pnpr/crates/pnpr/tests/auth_publish/publish_validation.rs index fb96a4c040..843c12fb66 100644 --- a/pnpr/crates/pnpr/tests/auth_publish/publish_validation.rs +++ b/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}"); + } +} diff --git a/pnpr/docs/cloudflare.md b/pnpr/docs/cloudflare.md index 8d87f00b8f..0d5f07c7a8 100644 --- a/pnpr/docs/cloudflare.md +++ b/pnpr/docs/cloudflare.md @@ -176,22 +176,29 @@ export class PnprContainer extends Container { // 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 { - 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 { 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); + } }, }; ``` diff --git a/pnpr/docs/endpoints.md b/pnpr/docs/endpoints.md index dc3b771de0..76394fb893 100644 --- a/pnpr/docs/endpoints.md +++ b/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 diff --git a/pnpr/examples/cloudflare/worker.test.mjs b/pnpr/examples/cloudflare/worker.test.mjs index f144d2d000..c376bb5920 100644 --- a/pnpr/examples/cloudflare/worker.test.mjs +++ b/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) })