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