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" + ], + [], + [] + ] + } + ] +}