From e7cfcbb252bbc3d75b343c45aca5205cdbe2aeab Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Sat, 17 Oct 2026 21:57:32 +0200 Subject: [PATCH] test: share the three-install request-log cases between pnpm v11 and v12 (#17063) `pnpr/.fixtures/request-logs/metadata-three-installs.json` lists the three-install request-log cases: settings, arguments, and the metadata requests of each install. The v12 CLI suite and a new v11 e2e test both assert it, so a difference in what the two stacks request fails one of them. The v11 test records requests through a proxy in front of the mock registry. Tarballs bypass that proxy, so the table leaves them out and the v12 test asserts them on its own. The table found one difference: v12's `to_http_date` read only RFC 3339 timestamps and HTTP dates, so a packument whose `modified` is a bare date revalidated with `If-None-Match` alone. pnpm v11 converts the same value with `new Date(...).toUTCString()` and sends both validators. v12 now reads a bare date as midnight UTC too. `httpdate::fmt_http_date` panics outside the years 1970 to 9999, and a registry's `modified` reached it unchecked. `to_http_date` now drops such a value. Part of pnpm/tasks#146 and pnpm/tasks#141. --- .changeset/if-modified-since-bare-date.md | 5 + .changeset/if-modified-since-before-1970.md | 5 + .../cli/tests/suite/metadata_request_log.rs | 173 +++++++++--------- .../src/fetch_full_metadata.rs | 23 ++- .../src/fetch_full_metadata/tests.rs | 11 ++ .../pnpm/test/install/metadataRequestLog.ts | 137 ++++++++++++++ .../request-logs/metadata-three-installs.json | 83 +++++++++ 7 files changed, 346 insertions(+), 91 deletions(-) create mode 100644 .changeset/if-modified-since-bare-date.md create mode 100644 .changeset/if-modified-since-before-1970.md create mode 100644 pnpm11/pnpm/test/install/metadataRequestLog.ts create mode 100644 pnpr/.fixtures/request-logs/metadata-three-installs.json diff --git a/.changeset/if-modified-since-bare-date.md b/.changeset/if-modified-since-bare-date.md new file mode 100644 index 0000000000..a0ffd72b20 --- /dev/null +++ b/.changeset/if-modified-since-bare-date.md @@ -0,0 +1,5 @@ +--- +"pacquet": patch +--- + +When pnpm revalidates cached registry metadata, it now also sends `If-Modified-Since` if the registry's modification time is a date without a time. diff --git a/.changeset/if-modified-since-before-1970.md b/.changeset/if-modified-since-before-1970.md new file mode 100644 index 0000000000..d5cad1386e --- /dev/null +++ b/.changeset/if-modified-since-before-1970.md @@ -0,0 +1,5 @@ +--- +"pacquet": patch +--- + +pnpm no longer crashes when it revalidates cached registry metadata whose modification time is before 1970. diff --git a/pnpm/crates/cli/tests/suite/metadata_request_log.rs b/pnpm/crates/cli/tests/suite/metadata_request_log.rs index 6764612786..0ae73e0196 100644 --- a/pnpm/crates/cli/tests/suite/metadata_request_log.rs +++ b/pnpm/crates/cli/tests/suite/metadata_request_log.rs @@ -1,6 +1,7 @@ //! The registry requests of three installs that resolve the same project -//! against an unchanged registry, sharing one metadata cache. Only the first -//! install may download a body. +//! against an unchanged registry, sharing one metadata cache. The cases and +//! their metadata request logs are shared with pnpm v11, see the table's +//! `description`. use assert_cmd::prelude::*; use pnpm_testing_utils::{ @@ -8,15 +9,54 @@ use pnpm_testing_utils::{ registry::RecordedRequest, }; use pretty_assertions::assert_eq; +use serde_json::Value; use std::{collections::HashMap, fs, io}; -const PKG: &str = "/@pnpm.e2e%2Fpkg-with-1-dep"; -const DEP: &str = "/@pnpm.e2e%2Fdep-of-pkg-with-1-dep"; -const PKG_TARBALL: &str = "/@pnpm.e2e/pkg-with-1-dep/-/pkg-with-1-dep-100.1.0.tgz"; -const DEP_TARBALL: &str = "/@pnpm.e2e/dep-of-pkg-with-1-dep/-/dep-of-pkg-with-1-dep-100.1.0.tgz"; +const CASE_TABLE: &str = + include_str!("../../../../../pnpr/.fixtures/request-logs/metadata-three-installs.json"); -/// Each install's requests, sorted: concurrent fetches finish in any order. -fn three_installs(settings: &str, args: &[&str]) -> [Vec; 3] { +const PKG_TARBALL: &str = "GET /@pnpm.e2e/pkg-with-1-dep/-/pkg-with-1-dep-100.1.0.tgz 200"; +const DEP_TARBALL: &str = + "GET /@pnpm.e2e/dep-of-pkg-with-1-dep/-/dep-of-pkg-with-1-dep-100.1.0.tgz 200"; + +/// Install 1 also downloads both tarballs, which the store keeps for installs +/// 2 and 3. The table leaves tarballs out: in pnpm v11's test they bypass the +/// recording proxy. +#[test] +fn every_case_of_the_shared_table() { + let table: Value = serde_json::from_str(CASE_TABLE).expect("parse the case table"); + for case in table["cases"].as_array().expect("cases") { + let name = case["name"].as_str().expect("case name"); + let settings: Vec = case["settings"] + .as_object() + .expect("case settings") + .iter() + .map(|(key, value)| format!("{key}: {value}")) + .collect(); + let args: Vec<&str> = case["args"] + .as_array() + .expect("case args") + .iter() + .map(|arg| arg.as_str().expect("case arg")) + .collect(); + let expected: Vec> = + serde_json::from_value(case["installs"].clone()).expect("case installs"); + + let (metadata, tarballs) = three_installs(&table["dependencies"], &settings, &args); + assert_eq!(metadata, expected, "{name}"); + assert_eq!(tarballs, [vec![DEP_TARBALL, PKG_TARBALL], vec![], vec![]], "{name}"); + } +} + +type InstallLogs = Vec>; + +/// Each install's metadata requests as the table spells them, and its +/// tarball paths, both sorted: concurrent fetches finish in any order. +fn three_installs( + dependencies: &Value, + settings: &[String], + args: &[&str], +) -> (InstallLogs, InstallLogs) { let CommandTempCwd { mut pacquet, root, @@ -25,40 +65,57 @@ fn three_installs(settings: &str, args: &[&str]) -> [Vec; 3] { .. } = CommandTempCwd::init().add_recording_mocked_registry(); let AddMockedRegistry { mock_instance, .. } = npmrc_info; - let manifest = - serde_json::json!({ "dependencies": { "@pnpm.e2e/pkg-with-1-dep": "^100.0.0" } }); + let manifest = serde_json::json!({ "dependencies": dependencies }); fs::write(workspace.join("package.json"), manifest.to_string()).expect("write package.json"); let workspace_yaml = workspace.join("pnpm-workspace.yaml"); let yaml = fs::read_to_string(&workspace_yaml).expect("read pnpm-workspace.yaml"); - fs::write(&workspace_yaml, format!("{yaml}{settings}")).expect("write pnpm-workspace.yaml"); + fs::write(&workspace_yaml, format!("{yaml}{}\n", settings.join("\n"))) + .expect("write pnpm-workspace.yaml"); pacquet.arg("install").args(args); - let installs = [(); 3].map(|()| { - // Remove what the last install wrote, so this one resolves again. - remove_if_present(fs::remove_dir_all(workspace.join("node_modules"))); - remove_if_present(fs::remove_file(workspace.join("pnpm-lock.yaml"))); - pacquet.assert().success(); - mock_instance.take_requests() - }); + let installs: Vec> = (0..3) + .map(|_| { + // Remove what the last install wrote, so this one resolves again. + remove_if_present(fs::remove_dir_all(workspace.join("node_modules"))); + remove_if_present(fs::remove_file(workspace.join("pnpm-lock.yaml"))); + pacquet.assert().success(); + mock_instance.take_requests() + }) + .collect(); drop(root); - let etags: HashMap<(String, &str), String> = installs + let etags: HashMap<(&str, &str), &str> = installs .iter() .flatten() .filter(|request| request.status == 200) .filter_map(|request| { - let etag = request.etag.clone()?; - Some(((request.path.clone(), document(request)), etag)) + Some(((request.path.as_str(), document(request)), request.etag.as_deref()?)) }) .collect(); - installs.map(|requests| { + let log_of = |requests: &Vec, tarballs: bool| -> Vec { let mut log: Vec = requests .iter() - .map(|request| describe(request, &etags)) + .filter(|request| is_tarball(request) == tarballs) + .map(|request| { + if tarballs { + format!("{} {} {}", request.method, request.path, request.status) + } else { + describe(request, &etags) + } + }) .collect(); log.sort(); log - }) + }; + let metadata = installs + .iter() + .map(|requests| log_of(requests, false)) + .collect(); + let tarballs = installs + .iter() + .map(|requests| log_of(requests, true)) + .collect(); + (metadata, tarballs) } fn remove_if_present(result: io::Result<()>) { @@ -67,6 +124,10 @@ fn remove_if_present(result: io::Result<()>) { } } +fn is_tarball(request: &RecordedRequest) -> bool { + request.path.ends_with(".tgz") +} + fn document(request: &RecordedRequest) -> &'static str { match request.accept.as_deref() { Some(accept) if accept.starts_with("application/vnd.npm.install-v1+json") => "abbreviated", @@ -74,10 +135,9 @@ fn document(request: &RecordedRequest) -> &'static str { } } -/// A metadata request reads `etag` when it sends the `ETag` this registry -/// gave the same representation, `other-etag` for any other value, and -/// `since` when it sends `If-Modified-Since`. -fn describe(request: &RecordedRequest, etags: &HashMap<(String, &str), String>) -> String { +/// A request sending some other `ETag` reads `other-etag`, which no case +/// expects. +fn describe(request: &RecordedRequest, etags: &HashMap<(&str, &str), &str>) -> String { let RecordedRequest { method, path, @@ -86,13 +146,10 @@ fn describe(request: &RecordedRequest, etags: &HashMap<(String, &str), String>) status, .. } = request; - if path.ends_with(".tgz") { - return format!("{method} {path} {status}"); - } let document = document(request); let mut validators = String::new(); if let Some(sent) = if_none_match { - let own = etags.get(&(path.clone(), document)) == Some(sent); + let own = etags.get(&(path.as_str(), document)) == Some(&sent.as_str()); validators.push_str(if own { " etag" } else { " other-etag" }); } if if_modified_since.is_some() { @@ -100,55 +157,3 @@ fn describe(request: &RecordedRequest, etags: &HashMap<(String, &str), String>) } format!("{method} {path} {document}{validators} {status}") } - -fn first_install(document: &str) -> Vec { - let mut log = vec![ - format!("GET {DEP} {document} 200"), - format!("GET {DEP_TARBALL} 200"), - format!("GET {PKG} {document} 200"), - format!("GET {PKG_TARBALL} 200"), - ]; - log.sort(); - log -} - -/// `request` is the document kind and the validators, as [`describe`] -/// spells them. -fn revalidation(request: &str) -> Vec { - vec![format!("GET {DEP} {request} 304"), format!("GET {PKG} {request} 304")] -} - -#[test] -fn abbreviated_metadata_is_revalidated_with_its_etag() { - let [first, second, third] = three_installs("minimumReleaseAge: 0\n", &[]); - assert_eq!(first, first_install("abbreviated")); - assert_eq!(second, revalidation("abbreviated etag")); - assert_eq!(third, revalidation("abbreviated etag")); -} - -#[test] -fn full_metadata_is_revalidated_with_its_etag() { - let [first, second, third] = - three_installs("minimumReleaseAge: 0\ntrustPolicy: no-downgrade\n", &[]); - assert_eq!(first, first_install("full")); - assert_eq!(second, revalidation("full etag since")); - assert_eq!(third, revalidation("full etag since")); -} - -/// A mirror written after the `minimumReleaseAge` cutoff cannot be missing a -/// mature version, so it answers without a request. -#[test] -fn a_mirror_written_after_the_release_age_cutoff_answers_without_a_request() { - let [first, second, third] = three_installs("minimumReleaseAge: 1440\n", &[]); - assert_eq!(first, first_install("abbreviated")); - assert_eq!(second, Vec::::new()); - assert_eq!(third, Vec::::new()); -} - -#[test] -fn prefer_offline_answers_from_the_mirror_without_a_request() { - let [first, second, third] = three_installs("minimumReleaseAge: 0\n", &["--prefer-offline"]); - assert_eq!(first, first_install("abbreviated")); - assert_eq!(second, Vec::::new()); - assert_eq!(third, Vec::::new()); -} diff --git a/pnpm/crates/resolving-npm-resolver/src/fetch_full_metadata.rs b/pnpm/crates/resolving-npm-resolver/src/fetch_full_metadata.rs index 148cc28264..4b57e9adfe 100644 --- a/pnpm/crates/resolving-npm-resolver/src/fetch_full_metadata.rs +++ b/pnpm/crates/resolving-npm-resolver/src/fetch_full_metadata.rs @@ -276,14 +276,23 @@ async fn send_once<'a>( /// Convert a stored `modified` value — the packument's ISO-8601 /// `time.modified` — to the HTTP-date form `If-Modified-Since` requires /// (RFC 9110 §8.8.3), the same conversion the TypeScript CLI applies with -/// `toUTCString()`. A value already in HTTP-date form is kept. `None` when -/// the value parses as neither, so it is dropped instead of sent — a -/// recipient must ignore a malformed `If-Modified-Since` anyway. +/// `toUTCString()`. A date without a time is midnight UTC, as JavaScript's +/// `Date` reads it. A value already in HTTP-date form is kept. `None` when +/// the value parses as none of these, or names a time outside the years +/// 1970 to 9999 that an HTTP date can carry, so it is dropped instead of +/// sent — a recipient must ignore a malformed `If-Modified-Since` anyway. fn to_http_date(value: &str) -> Option { - if let Ok(datetime) = chrono::DateTime::parse_from_rfc3339(value) { - return Some(httpdate::fmt_http_date(datetime.into())); - } - httpdate::parse_http_date(value).ok().map(httpdate::fmt_http_date) + let time: chrono::DateTime = + if let Ok(datetime) = chrono::DateTime::parse_from_rfc3339(value) { + datetime.into() + } else if let Ok(date) = chrono::NaiveDate::parse_from_str(value, "%Y-%m-%d") { + date.and_time(chrono::NaiveTime::MIN).and_utc() + } else { + return httpdate::parse_http_date(value).ok().map(httpdate::fmt_http_date); + }; + // `fmt_http_date` panics outside that range. + let in_range = time.timestamp() >= 0 && chrono::Datelike::year(&time) <= 9999; + in_range.then(|| httpdate::fmt_http_date(time.into())) } /// Fetch the registry metadata document for `pkg_name`. The diff --git a/pnpm/crates/resolving-npm-resolver/src/fetch_full_metadata/tests.rs b/pnpm/crates/resolving-npm-resolver/src/fetch_full_metadata/tests.rs index 966856e3fe..b55beaac52 100644 --- a/pnpm/crates/resolving-npm-resolver/src/fetch_full_metadata/tests.rs +++ b/pnpm/crates/resolving-npm-resolver/src/fetch_full_metadata/tests.rs @@ -520,6 +520,17 @@ fn to_http_date_converts_iso_8601() { ); } +#[test] +fn to_http_date_reads_a_date_without_a_time_as_midnight_utc() { + assert_eq!(super::to_http_date("2022-01-01").as_deref(), Some("Sat, 01 Jan 2022 00:00:00 GMT")); +} + +#[test] +fn to_http_date_drops_a_time_before_1970() { + assert_eq!(super::to_http_date("1969-12-31"), None); + assert_eq!(super::to_http_date("1969-12-31T23:59:59Z"), None); +} + #[test] fn to_http_date_keeps_http_dates() { assert_eq!( diff --git a/pnpm11/pnpm/test/install/metadataRequestLog.ts b/pnpm11/pnpm/test/install/metadataRequestLog.ts new file mode 100644 index 0000000000..2155faa0c2 --- /dev/null +++ b/pnpm11/pnpm/test/install/metadataRequestLog.ts @@ -0,0 +1,137 @@ +import fs from 'node:fs' +import http from 'node:http' +import type { AddressInfo } from 'node:net' +import path from 'node:path' + +import { afterEach, expect, test } from '@jest/globals' +import { prepare } from '@pnpm/prepare' +import { REGISTRY_MOCK_PORT } from '@pnpm/testing.registry-mock' +import { writeYamlFileSync } from 'write-yaml-file' + +import { execPnpm } from '../utils/index.js' + +// The cases and their metadata request logs are shared with pnpm v12, see the +// table's `description`. +const CASE_TABLE = path.join(import.meta.dirname, '../../../../pnpr/.fixtures/request-logs/metadata-three-installs.json') + +interface CaseTable { + dependencies: Record + cases: Array<{ + name: string + settings: Record + args: string[] + installs: string[][] + }> +} + +interface RecordedRequest { + method: string + path: string + accept?: string + ifNoneMatch?: string + ifModifiedSince?: string + status: number + etag?: string +} + +interface RecordingRegistry { + url: string + takeRequests: () => RecordedRequest[] + close: () => Promise +} + +const table: CaseTable = JSON.parse(fs.readFileSync(CASE_TABLE, 'utf8')) + +let registry: RecordingRegistry | undefined + +afterEach(async () => { + await registry?.close() + registry = undefined +}) + +test.each(table.cases)('$name', async ({ settings, args, installs: expected }) => { + registry = await startRecordingRegistry() + prepare({ dependencies: table.dependencies }) + writeYamlFileSync('pnpm-workspace.yaml', { registry: registry.url }) + // The test environment sets `minimumReleaseAge` as an environment + // variable, which outranks pnpm-workspace.yaml. + const env = Object.fromEntries(Object.entries(settings).map(([key, value]) => [ + `pnpm_config_${key.replace(/[A-Z]/g, (letter) => `_${letter.toLowerCase()}`)}`, + String(value), + ])) + const installs: RecordedRequest[][] = [] + for (let install = 0; install < 3; install++) { + // Remove what the last install wrote, so this one resolves again. + fs.rmSync('node_modules', { recursive: true, force: true }) + fs.rmSync('pnpm-lock.yaml', { force: true }) + // eslint-disable-next-line no-await-in-loop -- the installs share one cache and must run in order + await execPnpm(['install', ...args], { env }) + installs.push(registry.takeRequests()) + } + + const etags = new Map(installs.flat() + .filter((request) => request.status === 200 && request.etag != null) + .map((request) => [`${request.path} ${documentOf(request)}`, request.etag])) + expect(installs.map((requests) => requests.map((request) => describe(request, etags)).sort())).toStrictEqual(expected) +}) + +function documentOf (request: RecordedRequest): string { + return request.accept?.startsWith('application/vnd.npm.install-v1+json') ? 'abbreviated' : 'full' +} + +// A request sending some other ETag reads `other-etag`, which no case expects. +function describe (request: RecordedRequest, etags: Map): string { + const document = documentOf(request) + let validators = '' + if (request.ifNoneMatch != null) { + validators += etags.get(`${request.path} ${document}`) === request.ifNoneMatch ? ' etag' : ' other-etag' + } + if (request.ifModifiedSince != null) { + validators += ' since' + } + return `${request.method} ${request.path} ${document}${validators} ${request.status}` +} + +// Forwards to the mock registry. Tarball URLs in the packuments name the mock +// registry itself, so only metadata requests pass through here. +async function startRecordingRegistry (): Promise { + let requests: RecordedRequest[] = [] + const server = http.createServer((req, res) => { + const upstream = http.request({ + host: 'localhost', + port: REGISTRY_MOCK_PORT, + method: req.method, + path: req.url, + headers: req.headers, + }, (upstreamRes) => { + requests.push({ + method: req.method!, + path: req.url!, + accept: req.headers.accept, + ifNoneMatch: req.headers['if-none-match'], + ifModifiedSince: req.headers['if-modified-since'], + status: upstreamRes.statusCode!, + etag: upstreamRes.headers.etag, + }) + res.writeHead(upstreamRes.statusCode!, upstreamRes.headers) + upstreamRes.pipe(res) + }) + req.pipe(upstream) + }) + await new Promise((resolve) => { + server.listen(0, '127.0.0.1', resolve) + }) + return { + url: `http://127.0.0.1:${(server.address() as AddressInfo).port}/`, + takeRequests: () => { + const taken = requests + requests = [] + return taken + }, + close: async () => new Promise((resolve) => { + server.close(() => { + resolve() + }) + }), + } +} diff --git a/pnpr/.fixtures/request-logs/metadata-three-installs.json b/pnpr/.fixtures/request-logs/metadata-three-installs.json new file mode 100644 index 0000000000..d897cb96ee --- /dev/null +++ b/pnpr/.fixtures/request-logs/metadata-three-installs.json @@ -0,0 +1,83 @@ +{ + "description": "Three installs that resolve the same project against an unchanged fixture registry, sharing one metadata cache. Each install's metadata requests, sorted, as `METHOD path document [etag] [since] status`: `etag` when the request sends the ETag the registry gave the same document, `since` when it sends If-Modified-Since. Asserted by pnpm/crates/cli/tests/suite/metadata_request_log.rs and pnpm11/pnpm/test/install/metadataRequestLog.ts.", + "dependencies": { + "@pnpm.e2e/pkg-with-1-dep": "^100.0.0" + }, + "cases": [ + { + "name": "abbreviated metadata is revalidated with its ETag", + "settings": { + "minimumReleaseAge": 0 + }, + "args": [], + "installs": [ + [ + "GET /@pnpm.e2e%2Fdep-of-pkg-with-1-dep abbreviated 200", + "GET /@pnpm.e2e%2Fpkg-with-1-dep abbreviated 200" + ], + [ + "GET /@pnpm.e2e%2Fdep-of-pkg-with-1-dep abbreviated etag since 304", + "GET /@pnpm.e2e%2Fpkg-with-1-dep abbreviated etag since 304" + ], + [ + "GET /@pnpm.e2e%2Fdep-of-pkg-with-1-dep abbreviated etag since 304", + "GET /@pnpm.e2e%2Fpkg-with-1-dep abbreviated etag since 304" + ] + ] + }, + { + "name": "full metadata is revalidated with its ETag", + "settings": { + "minimumReleaseAge": 0, + "trustPolicy": "no-downgrade" + }, + "args": [], + "installs": [ + [ + "GET /@pnpm.e2e%2Fdep-of-pkg-with-1-dep full 200", + "GET /@pnpm.e2e%2Fpkg-with-1-dep full 200" + ], + [ + "GET /@pnpm.e2e%2Fdep-of-pkg-with-1-dep full etag since 304", + "GET /@pnpm.e2e%2Fpkg-with-1-dep full etag since 304" + ], + [ + "GET /@pnpm.e2e%2Fdep-of-pkg-with-1-dep full etag since 304", + "GET /@pnpm.e2e%2Fpkg-with-1-dep full etag since 304" + ] + ] + }, + { + "name": "a mirror written after the release age cutoff answers without a request", + "settings": { + "minimumReleaseAge": 1440 + }, + "args": [], + "installs": [ + [ + "GET /@pnpm.e2e%2Fdep-of-pkg-with-1-dep abbreviated 200", + "GET /@pnpm.e2e%2Fpkg-with-1-dep abbreviated 200" + ], + [], + [] + ] + }, + { + "name": "prefer-offline answers from the mirror without a request", + "settings": { + "minimumReleaseAge": 0 + }, + "args": [ + "--prefer-offline" + ], + "installs": [ + [ + "GET /@pnpm.e2e%2Fdep-of-pkg-with-1-dep abbreviated 200", + "GET /@pnpm.e2e%2Fpkg-with-1-dep abbreviated 200" + ], + [], + [] + ] + } + ] +}