From eba03e0d5c320194775909ce42fd8e53462bbf63 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Tue, 16 Jun 2026 07:29:26 +0200 Subject: [PATCH] fix: detect reverted catalog entries on install (#12438) * fix: detect reverted catalog entries on install After an update bumped a catalog entry in pnpm-workspace.yaml, the workspace state cache stored the pre-update catalog versions, so reverting the entry back to its original version was reported as "Already up to date" instead of reinstalling the previous version. Fold the catalogs written during the install into the catalogs recorded in the workspace state so a later install detects the reverted entry as outdated. Closes https://github.com/pnpm/pnpm/issues/12418 * fix: harden catalog merge against prototype pollution and entry loss Address review feedback on the catalog-merge helper: - mergeCatalogs now builds null-prototype records and copies entries with Object.defineProperty, so a catalog or dependency name like __proto__ (which can flow in from parsed pnpm-workspace.yaml) becomes an ordinary own property instead of corrupting the result's prototype. - The recursive per-project install path now accumulates updatedCatalogs with mergeCatalogs instead of a shallow Object.assign, so two projects updating different entries of the same catalog no longer clobber each other. --- .changeset/twelve-catalogs-revert.md | 7 +++ catalogs/config/src/index.ts | 1 + catalogs/config/src/mergeCatalogs.ts | 40 ++++++++++++++++ catalogs/config/test/mergeCatalogs.test.ts | 53 ++++++++++++++++++++++ installing/commands/package.json | 1 + installing/commands/src/installDeps.ts | 30 +++++++++--- installing/commands/src/recursive.ts | 29 ++++++++---- installing/commands/tsconfig.json | 3 ++ pnpm-lock.yaml | 3 ++ pnpm/test/catalogUpToDate.ts | 48 ++++++++++++++++++++ 10 files changed, 201 insertions(+), 14 deletions(-) create mode 100644 .changeset/twelve-catalogs-revert.md create mode 100644 catalogs/config/src/mergeCatalogs.ts create mode 100644 catalogs/config/test/mergeCatalogs.test.ts create mode 100644 pnpm/test/catalogUpToDate.ts diff --git a/.changeset/twelve-catalogs-revert.md b/.changeset/twelve-catalogs-revert.md new file mode 100644 index 0000000000..9bb7258526 --- /dev/null +++ b/.changeset/twelve-catalogs-revert.md @@ -0,0 +1,7 @@ +--- +"@pnpm/catalogs.config": patch +"@pnpm/installing.commands": patch +"pnpm": patch +--- + +Fix `pnpm install` reporting "Already up to date" after a catalog entry in `pnpm-workspace.yaml` was reverted to a previous version. After an update modified a catalog, the workspace state cache stored the pre-update catalog versions, so reverting the entry back to its original version was not detected as an outdated state [#12418](https://github.com/pnpm/pnpm/issues/12418). diff --git a/catalogs/config/src/index.ts b/catalogs/config/src/index.ts index 2f2143896e..edc1d800fa 100644 --- a/catalogs/config/src/index.ts +++ b/catalogs/config/src/index.ts @@ -1 +1,2 @@ export { getCatalogsFromWorkspaceManifest } from './getCatalogsFromWorkspaceManifest.js' +export { mergeCatalogs } from './mergeCatalogs.js' diff --git a/catalogs/config/src/mergeCatalogs.ts b/catalogs/config/src/mergeCatalogs.ts new file mode 100644 index 0000000000..ac26c2b2a9 --- /dev/null +++ b/catalogs/config/src/mergeCatalogs.ts @@ -0,0 +1,40 @@ +import type { Catalog, Catalogs } from '@pnpm/catalogs.types' + +/** + * Deep-merges catalog definitions, later arguments taking precedence over + * earlier ones at the individual entry level. Use it to fold the catalog + * changes produced by an install (`updatedCatalogs`) back into the catalogs + * read at startup, so the result reflects what was actually written to + * `pnpm-workspace.yaml`. + * + * Catalog and dependency names originate from `pnpm-workspace.yaml`, so the + * result is built from null-prototype records and entries are copied with + * `Object.defineProperty`. A name like `__proto__` then becomes an ordinary + * own property instead of mutating a prototype. + */ +export function mergeCatalogs (...catalogsList: Array): Catalogs { + const result = Object.create(null) as Record + for (const catalogs of catalogsList) { + if (catalogs == null) continue + for (const catalogName of Object.keys(catalogs)) { + const catalog = catalogs[catalogName] + if (catalog == null) continue + const target: Record = result[catalogName] ?? Object.create(null) + for (const dependencyName of Object.keys(catalog)) { + Object.defineProperty(target, dependencyName, { + value: catalog[dependencyName], + writable: true, + enumerable: true, + configurable: true, + }) + } + Object.defineProperty(result, catalogName, { + value: target, + writable: true, + enumerable: true, + configurable: true, + }) + } + } + return result +} diff --git a/catalogs/config/test/mergeCatalogs.test.ts b/catalogs/config/test/mergeCatalogs.test.ts new file mode 100644 index 0000000000..6456672662 --- /dev/null +++ b/catalogs/config/test/mergeCatalogs.test.ts @@ -0,0 +1,53 @@ +import { expect, test } from '@jest/globals' +import { mergeCatalogs } from '@pnpm/catalogs.config' + +test('returns an empty catalog when nothing is passed', () => { + expect(mergeCatalogs()).toEqual({}) + expect(mergeCatalogs(undefined, undefined)).toEqual({}) +}) + +test('later entries override earlier ones at the dependency level', () => { + expect(mergeCatalogs( + { + default: { foo: '1.0.0', bar: '1.0.0' }, + named: { baz: '1.0.0' }, + }, + { + default: { foo: '2.0.0' }, + } + )).toEqual({ + default: { foo: '2.0.0', bar: '1.0.0' }, + named: { baz: '1.0.0' }, + }) +}) + +test('adds catalog entries that did not exist in the base', () => { + expect(mergeCatalogs( + { default: { foo: '1.0.0' } }, + { default: { bar: '1.0.0' }, named: { baz: '1.0.0' } } + )).toEqual({ + default: { foo: '1.0.0', bar: '1.0.0' }, + named: { baz: '1.0.0' }, + }) +}) + +test('skips nullish catalog arguments and nullish named catalogs', () => { + expect(mergeCatalogs( + undefined, + { default: { foo: '1.0.0' }, named: undefined } + )).toEqual({ + default: { foo: '1.0.0' }, + }) +}) + +test('treats dangerous catalog and dependency names as ordinary own properties', () => { + // Catalogs parsed from YAML/JSON can carry `__proto__` as an own property, + // unlike an object literal where `{ __proto__: ... }` sets the prototype. + const malicious = JSON.parse('{"__proto__":{"polluted":"yes"},"default":{"__proto__":"1.0.0","constructor":"2.0.0"}}') + const merged = mergeCatalogs(malicious) + expect(({} as Record).polluted).toBeUndefined() + expect(Object.prototype.hasOwnProperty.call(merged, '__proto__')).toBe(true) + expect(Object.prototype.hasOwnProperty.call(merged.default, '__proto__')).toBe(true) + expect((merged.default as Record).__proto__).toBe('1.0.0') + expect((merged.default as Record).constructor).toBe('2.0.0') +}) diff --git a/installing/commands/package.json b/installing/commands/package.json index b5248094d3..4f106264f3 100644 --- a/installing/commands/package.json +++ b/installing/commands/package.json @@ -35,6 +35,7 @@ "@inquirer/prompts": "catalog:", "@pnpm/building.after-install": "workspace:*", "@pnpm/building.policy": "workspace:*", + "@pnpm/catalogs.config": "workspace:*", "@pnpm/catalogs.types": "workspace:*", "@pnpm/cli.command": "workspace:*", "@pnpm/cli.common-cli-options-help": "workspace:*", diff --git a/installing/commands/src/installDeps.ts b/installing/commands/src/installDeps.ts index b203f1e414..15df3a5a05 100644 --- a/installing/commands/src/installDeps.ts +++ b/installing/commands/src/installDeps.ts @@ -1,6 +1,8 @@ import path from 'node:path' import { buildProjects } from '@pnpm/building.after-install' +import { mergeCatalogs } from '@pnpm/catalogs.config' +import type { Catalogs } from '@pnpm/catalogs.types' import type { CommandHandler } from '@pnpm/cli.command' import { readProjectManifestOnly, @@ -426,7 +428,7 @@ export async function installDeps ( if (!opts.lockfileOnly) { await updateWorkspaceState({ allProjects, - settings: opts, + settings: withUpdatedCatalogs(opts, updatedCatalogs), workspaceDir: opts.workspaceDir ?? opts.lockfileDir ?? opts.dir, pnpmfiles: opts.pnpmfile, filteredInstall: allProjects.length !== Object.keys(opts.selectedProjectsGraph ?? {}).length, @@ -485,7 +487,7 @@ export async function installDeps ( selectedProjectsGraph, workspaceDir: opts.workspaceDir, // Otherwise TypeScript doesn't understand that is not undefined runPacquet, - }, 'install') + }, 'install', updatedCatalogs) if (opts.ignoreScripts) return @@ -508,7 +510,7 @@ export async function installDeps ( if (!opts.lockfileOnly) { await updateWorkspaceState({ allProjects, - settings: opts, + settings: withUpdatedCatalogs(opts, updatedCatalogs), workspaceDir: opts.workspaceDir ?? opts.lockfileDir ?? opts.dir, pnpmfiles: opts.pnpmfile, filteredInstall: allProjects.length !== Object.keys(opts.selectedProjectsGraph ?? {}).length, @@ -528,20 +530,36 @@ async function recursiveInstallThenUpdateWorkspaceState ( allProjects: Project[], params: string[], opts: RecursiveOptions & WorkspaceStateSettings, - cmdFullName: CommandFullName + cmdFullName: CommandFullName, + updatedCatalogs?: Catalogs ): Promise { const recursiveResult = await recursive(allProjects, params, opts, cmdFullName) if (!opts.lockfileOnly) { await updateWorkspaceState({ allProjects, - settings: opts, + settings: withUpdatedCatalogs(opts, updatedCatalogs, recursiveResult.updatedCatalogs), workspaceDir: opts.workspaceDir, pnpmfiles: opts.pnpmfile, filteredInstall: allProjects.length !== Object.keys(opts.selectedProjectsGraph ?? {}).length, configDependencies: opts.configDependencies, }) } - return recursiveResult + return recursiveResult.passed +} + +/** + * Folds the catalog entries written to `pnpm-workspace.yaml` during this + * install into the catalogs read at startup. The workspace state cache records + * these so a later install detects when a catalog entry was reverted; without + * this, the cache would keep the stale pre-install catalogs and report + * "Already up to date" even though the manifest changed. + */ +function withUpdatedCatalogs ( + settings: T, + ...updatedCatalogs: Array +): T { + if (updatedCatalogs.every((catalogs) => catalogs == null)) return settings + return { ...settings, catalogs: mergeCatalogs(settings.catalogs, ...updatedCatalogs) } } function severityStringToNumber (severity: VulnerabilitySeverity): number { diff --git a/installing/commands/src/recursive.ts b/installing/commands/src/recursive.ts index 93af6cfad1..a5eef2fc16 100755 --- a/installing/commands/src/recursive.ts +++ b/installing/commands/src/recursive.ts @@ -1,6 +1,7 @@ import { promises as fs } from 'node:fs' import path from 'node:path' +import { mergeCatalogs } from '@pnpm/catalogs.config' import type { Catalogs } from '@pnpm/catalogs.types' import type { CommandHandler } from '@pnpm/cli.command' import { @@ -144,21 +145,31 @@ export type RecursiveOptions = CreateStoreControllerOptions & Pick { +): Promise { if (allProjects.length === 0) { // It might make sense to throw an exception in this case - return false + return { passed: false } } const pkgs = Object.values(opts.selectedProjectsGraph).map((wsPkg) => wsPkg.package) if (pkgs.length === 0) { - return false + return { passed: false } } const manifestsByPath = getManifestsByPath(allProjects) @@ -225,7 +236,7 @@ export async function recursive ( const calculatedRepositoryRoot = await fs.realpath(calculateRepositoryRoot(opts.workspaceDir, importers.map(x => x.rootDir))) const isFromWorkspace = isSubdir.bind(null, calculatedRepositoryRoot) importers = await pFilter(importers, async ({ rootDirRealPath }) => isFromWorkspace(rootDirRealPath)) - if (importers.length === 0) return true + if (importers.length === 0) return { passed: true } let mutation: 'install' | 'installSome' | 'uninstallSome' switch (cmdFullName) { case 'remove': @@ -341,7 +352,7 @@ export async function recursive ( await Promise.all(promises) } await handleIgnoredBuilds(opts, ignoredBuilds) - return true + return { passed: true, updatedCatalogs } } const pkgPaths = (Object.keys(opts.selectedProjectsGraph) as ProjectRootDir[]).sort() @@ -459,8 +470,10 @@ export async function recursive ( if (opts.save !== false) { await writeProjectManifest(newManifest) if (newCatalogsAddition) { - updatedCatalogs ??= {} - Object.assign(updatedCatalogs, newCatalogsAddition) + // Per-project additions are partial maps keyed by catalog name then + // dependency. Merge at the dependency level so two projects updating + // different entries of the same catalog don't clobber each other. + updatedCatalogs = mergeCatalogs(updatedCatalogs, newCatalogsAddition) } } if (ignoredBuilds?.size) { @@ -527,7 +540,7 @@ export async function recursive ( 'None of the specified packages were found in the dependencies of any of the projects.') } - return true + return { passed: true, updatedCatalogs } } function calculateRepositoryRoot ( diff --git a/installing/commands/tsconfig.json b/installing/commands/tsconfig.json index 8d30b0d2d1..68fde6a9b4 100644 --- a/installing/commands/tsconfig.json +++ b/installing/commands/tsconfig.json @@ -27,6 +27,9 @@ { "path": "../../building/policy" }, + { + "path": "../../catalogs/config" + }, { "path": "../../catalogs/types" }, diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 1e71d6546d..130ae03484 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -5246,6 +5246,9 @@ importers: '@pnpm/building.policy': specifier: workspace:* version: link:../../building/policy + '@pnpm/catalogs.config': + specifier: workspace:* + version: link:../../catalogs/config '@pnpm/catalogs.types': specifier: workspace:* version: link:../../catalogs/types diff --git a/pnpm/test/catalogUpToDate.ts b/pnpm/test/catalogUpToDate.ts new file mode 100644 index 0000000000..14d4b99033 --- /dev/null +++ b/pnpm/test/catalogUpToDate.ts @@ -0,0 +1,48 @@ +import { expect, test } from '@jest/globals' +import { prepare } from '@pnpm/prepare' +import { addDistTag } from '@pnpm/testing.registry-mock' +import type { ProjectManifest } from '@pnpm/types' +import { writeYamlFileSync } from 'write-yaml-file' + +import { execPnpm } from './utils/index.js' + +test('reverting a catalog entry after updating it is detected as an outdated state (#12418)', async () => { + await addDistTag({ package: '@pnpm.e2e/foo', version: '100.1.0', distTag: 'latest' }) + + const manifest: ProjectManifest = { + name: 'test-catalog-up-to-date', + version: '0.0.0', + private: true, + dependencies: { + '@pnpm.e2e/foo': 'catalog:', + }, + } + const project = prepare(manifest) + + writeYamlFileSync('pnpm-workspace.yaml', { + packages: ['.'], + catalog: { + '@pnpm.e2e/foo': '100.0.0', + }, + }) + + await execPnpm(['install']) + expect(project.readCurrentLockfile().importers['.'].dependencies?.['@pnpm.e2e/foo'].version).toBe('100.0.0') + + await execPnpm(['update', '@pnpm.e2e/foo', '--latest']) + expect(project.readCurrentLockfile().importers['.'].dependencies?.['@pnpm.e2e/foo'].version).toBe('100.1.0') + + // Restore the catalog entry to the version it had before the update. A + // subsequent install must notice that the workspace state is no longer up to + // date and reinstall the previous version instead of reporting + // "Already up to date". + writeYamlFileSync('pnpm-workspace.yaml', { + packages: ['.'], + catalog: { + '@pnpm.e2e/foo': '100.0.0', + }, + }) + + await execPnpm(['install']) + expect(project.readCurrentLockfile().importers['.'].dependencies?.['@pnpm.e2e/foo'].version).toBe('100.0.0') +})