From b62239d042bb7b6eefaf4db795ca1e28afab725b Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Fri, 17 Jul 2026 15:14:43 +0200 Subject: [PATCH] fix(lockfile): hide content from parse errors (#13101) Broken lockfile errors rendered parser-provided source snippets. A repository can commit pnpm-lock.yaml as a symlink, so the snippet could quote content from a readable file outside the repository. Format parse failures from their structured reason and location instead. Pacquet also stops exposing the snippet-bearing parser error through its diagnostic source chain. Closes pnpm/pnpm#13094 --- .changeset/safe-broken-lockfile-errors.md | 7 ++++ pnpm/crates/lockfile/src/env_lockfile.rs | 4 +-- pnpm/crates/lockfile/src/load_lockfile.rs | 31 +++++++++++++--- .../lockfile/src/load_lockfile/tests.rs | 29 +++++++++++++++ pnpm11/lockfile/fs/src/read.ts | 30 +++++++++++++++- pnpm11/lockfile/fs/test/read.test.ts | 35 +++++++++++++++++++ 6 files changed, 128 insertions(+), 8 deletions(-) create mode 100644 .changeset/safe-broken-lockfile-errors.md diff --git a/.changeset/safe-broken-lockfile-errors.md b/.changeset/safe-broken-lockfile-errors.md new file mode 100644 index 0000000000..51f6f48605 --- /dev/null +++ b/.changeset/safe-broken-lockfile-errors.md @@ -0,0 +1,7 @@ +--- +"@pnpm/lockfile.fs": patch +"pacquet": patch +"pnpm": patch +--- + +Prevent broken-lockfile errors from including snippets of the lockfile's contents. diff --git a/pnpm/crates/lockfile/src/env_lockfile.rs b/pnpm/crates/lockfile/src/env_lockfile.rs index e23c7d25d7..33c3ec6c0e 100644 --- a/pnpm/crates/lockfile/src/env_lockfile.rs +++ b/pnpm/crates/lockfile/src/env_lockfile.rs @@ -116,8 +116,8 @@ impl EnvLockfile { let Some(env_doc) = extract_env_document(&content) else { return Ok(None); }; - let mut env: EnvLockfile = - serde_saphyr::from_str(env_doc).map_err(LoadLockfileError::ParseYaml)?; + let mut env: EnvLockfile = serde_saphyr::from_str(env_doc) + .map_err(|source| LoadLockfileError::parse_yaml(&path, &source))?; env.root_importer_mut(); Ok(Some(env)) } diff --git a/pnpm/crates/lockfile/src/load_lockfile.rs b/pnpm/crates/lockfile/src/load_lockfile.rs index 89a2de287d..03e0facf83 100644 --- a/pnpm/crates/lockfile/src/load_lockfile.rs +++ b/pnpm/crates/lockfile/src/load_lockfile.rs @@ -2,10 +2,11 @@ use crate::{Lockfile, extract_main_document}; use derive_more::{Display, Error}; use pacquet_diagnostics::miette::{self, Diagnostic}; use pipe_trait::Pipe; +use serde_saphyr::MessageFormatter; use std::{ env, fs, io::{self, ErrorKind}, - path::Path, + path::{Path, PathBuf}, }; /// Error when reading lockfile the filesystem. @@ -20,9 +21,29 @@ pub enum LoadLockfileError { #[diagnostic(code(ERR_PNPM_LOCKFILE_READ_FILE))] ReadFile(io::Error), - #[display("Failed to parse lockfile content as YAML: {_0}")] - #[diagnostic(code(ERR_PNPM_LOCKFILE_PARSE_YAML))] - ParseYaml(serde_saphyr::Error), + #[display( + "The lockfile at \"{}\" is broken: {}", + path.display(), + reason + )] + #[diagnostic(code(ERR_PNPM_BROKEN_LOCKFILE))] + ParseYaml { path: PathBuf, reason: String }, +} + +impl LoadLockfileError { + pub(super) fn parse_yaml(path: &Path, source: &serde_saphyr::Error) -> Self { + Self::ParseYaml { path: path.to_path_buf(), reason: format_yaml_error(source) } + } +} + +fn format_yaml_error(error: &serde_saphyr::Error) -> String { + let error = error.without_snippet(); + let reason = serde_saphyr::DefaultMessageFormatter.format_message(error); + if let Some(location) = error.location() { + format!("{reason} ({}:{})", location.line(), location.column()) + } else { + reason.into_owned() + } } impl Lockfile { @@ -96,7 +117,7 @@ impl Lockfile { lockfile.reconstruct_missing_directory_resolutions(); Some(lockfile) }) - .map_err(LoadLockfileError::ParseYaml) + .map_err(|source| LoadLockfileError::parse_yaml(file_path, &source)) } } diff --git a/pnpm/crates/lockfile/src/load_lockfile/tests.rs b/pnpm/crates/lockfile/src/load_lockfile/tests.rs index 7048afd24d..ab5a05f819 100644 --- a/pnpm/crates/lockfile/src/load_lockfile/tests.rs +++ b/pnpm/crates/lockfile/src/load_lockfile/tests.rs @@ -2,6 +2,7 @@ use crate::{ DirectoryResolution, ImporterDepVersion, Lockfile, LockfileResolution, PackageKey, PkgName, SnapshotDepRef, }; +use pacquet_diagnostics::miette::Diagnostic; use pretty_assertions::assert_eq; use tempfile::tempdir; use text_block_macros::text_block; @@ -96,6 +97,34 @@ fn env_only_lockfile_loads_as_none() { assert!(result.is_none(), "expected None for env-only lockfile, got: {result:?}"); } +#[test] +fn parse_error_does_not_include_lockfile_content() { + let dir = tempdir().expect("create tempdir"); + let secret = "aws_secret_access_key = marker-secret"; + std::fs::write(dir.path().join(Lockfile::FILE_NAME), format!("[default]\n{secret}\n")) + .expect("write broken lockfile"); + + let error = Lockfile::load_wanted_from_dir(dir.path()).expect_err("lockfile must be broken"); + let message = error.to_string(); + + assert_eq!(error.code().expect("diagnostic code").to_string(), "ERR_PNPM_BROKEN_LOCKFILE"); + assert!( + message.starts_with(&format!( + r#"The lockfile at "{}" is broken: "#, + dir.path().join(Lockfile::FILE_NAME).display() + )), + "unexpected error: {message}", + ); + assert!(message.contains("(1:1)"), "unexpected error: {message}"); + assert!(!message.contains(secret), "error included lockfile content: {message}"); + assert!( + std::error::Error::source(&error).is_none(), + "parse error source could expose lockfile content", + ); + let report = format!("{:?}", pacquet_diagnostics::miette::Report::new(error)); + assert!(!report.contains(secret), "diagnostic included lockfile content: {report}"); +} + /// Heuristic-boundary check: a dropped directory resolution is /// reconstructed for a pruned `file:` peer-variant, but never for a /// `file:` tarball. diff --git a/pnpm11/lockfile/fs/src/read.ts b/pnpm11/lockfile/fs/src/read.ts index b5d08121c4..c1b19b5142 100644 --- a/pnpm11/lockfile/fs/src/read.ts +++ b/pnpm11/lockfile/fs/src/read.ts @@ -141,7 +141,7 @@ async function _read ( hadConflicts = false } catch (err: unknown) { if (!opts.autofixMergeConflicts || !isDiff(lockfileRawContent)) { - throw new PnpmError('BROKEN_LOCKFILE', `The lockfile at "${lockfilePath}" is broken: ${(err as Error).message}`) + throw new PnpmError('BROKEN_LOCKFILE', `The lockfile at "${lockfilePath}" is broken: ${formatLockfileError(err)}`) } hadConflicts = true lockfile = autofixMergeConflicts(lockfileRawContent) @@ -182,6 +182,34 @@ async function _read ( throw new LockfileBreakingChangeError(lockfilePath) } +function formatLockfileError (err: unknown): string { + if (isYamlException(err)) { + const reason = typeof err.reason === 'string' ? err.reason : 'Unable to parse YAML' + const line = err.mark?.line + const column = err.mark?.column + const position = typeof line === 'number' && Number.isFinite(line) && + typeof column === 'number' && Number.isFinite(column) + ? ` (${line + 1}:${column + 1})` + : '' + return `${reason}${position}` + } + return util.types.isNativeError(err) ? err.message : String(err) +} + +function isYamlException (err: unknown): err is YamlExceptionLike { + return typeof err === 'object' && err !== null && + 'name' in err && err.name === 'YAMLException' +} + +interface YamlExceptionLike { + name: 'YAMLException' + reason?: unknown + mark?: { + line?: unknown + column?: unknown + } | null +} + export function createLockfileObject ( importerIds: ProjectId[], opts: { diff --git a/pnpm11/lockfile/fs/test/read.test.ts b/pnpm11/lockfile/fs/test/read.test.ts index ab7a013c26..0165b35342 100644 --- a/pnpm11/lockfile/fs/test/read.test.ts +++ b/pnpm11/lockfile/fs/test/read.test.ts @@ -1,7 +1,9 @@ +import { writeFile } from 'node:fs/promises' import path from 'node:path' import { expect, jest, test } from '@jest/globals' import type { DepPath, ProjectId } from '@pnpm/types' +import yaml from 'js-yaml' import { temporaryDirectory } from 'tempy' jest.unstable_mockModule('@pnpm/network.git-utils', () => ({ getCurrentBranch: jest.fn() })) @@ -48,6 +50,39 @@ test('readWantedLockfile()', async () => { ).rejects.toMatchObject({ code: 'ERR_PNPM_LOCKFILE_BREAKING_CHANGE' }) }) +test('readWantedLockfile() does not include lockfile content in parse errors', async () => { + const projectPath = temporaryDirectory() + const secret = 'aws_secret_access_key = marker-secret' + await writeFile(path.join(projectPath, 'pnpm-lock.yaml'), `[default]\n${secret}\n`) + + const error = await readWantedLockfile(projectPath, { ignoreIncompatible: false }).catch((err: unknown) => err) + + expect(error).toMatchObject({ + code: 'ERR_PNPM_BROKEN_LOCKFILE', + message: expect.stringContaining(`The lockfile at "${path.join(projectPath, 'pnpm-lock.yaml')}" is broken:`), + }) + expect(error).toMatchObject({ message: expect.stringContaining('(2:1)') }) + expect(error).toMatchObject({ message: expect.not.stringContaining(secret) }) +}) + +test('readWantedLockfile() does not use a YAML exception message when its reason is missing', async () => { + const projectPath = temporaryDirectory() + const secret = 'aws_secret_access_key = marker-secret' + await writeFile(path.join(projectPath, 'pnpm-lock.yaml'), 'broken') + jest.spyOn(yaml, 'load').mockImplementationOnce(() => { + throw { + name: 'YAMLException', + message: `Unable to parse YAML\n${secret}`, + mark: { line: 1, column: 2 }, + } + }) + + await expect(readWantedLockfile(projectPath, { ignoreIncompatible: false })).rejects.toMatchObject({ + code: 'ERR_PNPM_BROKEN_LOCKFILE', + message: `The lockfile at "${path.join(projectPath, 'pnpm-lock.yaml')}" is broken: Unable to parse YAML (2:3)`, + }) +}) + test('readWantedLockfile() when lockfileVersion is a string', async () => { { const lockfile = await readWantedLockfile(path.join('fixtures', '4'), {