diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index bcbf0fa68..f5ce6af00 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -112,7 +112,7 @@ Beyond the globals above, each subcommand defines a small set of local arguments Each matching package instance is spliced, including scoped, quoted and nested-peer keys, one `redirect_pnpm_resolution` edit per changed instance (`rollback` / `remove` restore each from the npm registry — see "Hosted unwind coverage"). LF/CRLF and unrelated lock bytes are preserved. Unsupported matching instances refuse that dependency across the lockfile set; an already-hosted URL elsewhere cannot confirm a partial rewrite. -For a **9.0 root lock**, the CLI ensures `pnpm-workspace.yaml` carries `trustLockfile: true` (created with a root-only `packages:` scaffold, or appended while preserving user bytes). pnpm >=11 requires this to accept hosted URLs; it disables registry re-verification for the whole lock, while sha512 tarball integrity remains enforced. The write (edit kind `redirect_pnpm_workspace_trust`) respects `--dry-run`, skips legacy locks and Rush repos, preserves explicit user settings, and is disabled by `--no-trust-lockfile-config`. The `redirect_pnpm_trust_lockfile` warning explains manual configuration when required and clean reinstall guidance for all pnpm versions. Existing installs and warm stores can retain upstream files; use a clean install tree and empty store, then verify installed files with `socket-patch vex`. Neither a successful install nor a local VEX export guarantees hosted SBOM recognition or changes dashboard alert actions/counts. +For a **9.0 root lock**, the CLI ensures `pnpm-workspace.yaml` carries `trustLockfile: true` (created with a root-only `packages:` scaffold, or appended while preserving user bytes). pnpm >=11 requires this to accept hosted URLs; it disables registry re-verification for the whole lock, while sha512 tarball integrity remains enforced. The write (edit kind `redirect_pnpm_workspace_trust`) respects `--dry-run`, skips legacy locks and Rush repos, preserves explicit user settings (an existing top-level key in any YAML spelling: quoted, `trustLockfile :`, with a trailing comment), and is disabled by `--no-trust-lockfile-config`. The key goes inside the document (before a `...` end marker); a file a line append would corrupt (a flow-style root, an indented root, several documents) is left untouched and the warning gives the manual recoveries. The vendored `overrides:` mirror in `pnpm-workspace.yaml` reads keys the same way and refuses those shapes before writing. The `redirect_pnpm_trust_lockfile` warning explains manual configuration when required and clean reinstall guidance for all pnpm versions. Existing installs and warm stores can retain upstream files; use a clean install tree and empty store, then verify installed files with `socket-patch vex`. Neither a successful install nor a local VEX export guarantees hosted SBOM recognition or changes dashboard alert actions/counts. **npm hosted-mode `allow-remote` contract**: npm >=12 defaults `allow-remote=none` and refuses (EALLOWREMOTE) every lockfile entry whose `resolved` tarball is not served by the configured registry — exactly what a hosted redirect writes into `package-lock.json` / `npm-shrinkwrap.json`. Whenever a run leaves a ROOT npm lock carrying a granted hosted artifact URL (spliced this run, or already redirected by an earlier one — a missed config heals on re-run), the CLI ensures `allow-remote=all` in the project-root `.npmrc`: the file is created holding exactly `allow-remote=all\n` when absent, otherwise one `allow-remote=all` line is spliced in after the last non-empty top-level line (before any ini `[section]` header), in the file's own line ending, with the BOM, CRLF and trailing-newline shape preserved. The write lands in `redirect.rewrittenFiles` (edit kind `redirect_npmrc_allow_remote`), respects `--dry-run` (nothing written; the warning says what would be — including for a vendored → hosted takeover the dry run only previews), and is disabled by `--no-npm-allow-remote-config` / `SOCKET_NO_NPM_ALLOW_REMOTE_CONFIG`. The `.npmrc` grammar is npm's own `ini` parser's (cross-checked against it): lines split on any run of `\r` / `\n` (a bare `\r` ends a line), only the exact key `allow-remote` counts after ini unquoting (npm ignores `allow_remote` / `ALLOW-REMOTE` in a `.npmrc`; such a line is left alone and the real key appended), comment lines are ignored, a `[section]` header is recognized only as npm does — on the UNTRIMMED line (an indented or BOM-prefixed `[sec]` is a plain top-level key) — and ends the top-level scope, quotes and inline comments are stripped, the LAST top-level assignment wins, and the value is case-sensitive. An explicit other value (`allow-remote=none` / `root` / anything but `all`) is RESPECTED and never rewritten — the pnpm `trustLockfile: false` precedent — in the project `.npmrc` AND in every other npm config layer npm would consult: an `npm_config_allow_remote` environment variable (any spelling npm normalizes; it beats every `.npmrc`, so a project write could not take effect), and — when the project file sets nothing — the user (`npm_config_userconfig` / `~/.npmrc`), global (`npm_config_globalconfig` / `/etc/npmrc`, prefix from `npm_config_prefix`, the user/builtin config, `PREFIX` or the `node` binary's install root) and builtin (npm's own `npmrc` beside the `node` binary: `/lib/node_modules/npm/npmrc`, `\node_modules\npm\npmrc` on Windows) config files — path values `${VAR}`-expanded and `~`-expanded like npm, env names case-insensitive on Windows, where a committed project line would silently override a machine / org policy. A symlinked, non-regular or unreadable `.npmrc`, or one with bare-`\r` line endings (npm splits on them, the line splice does not), is left untouched. Every variant emits the `redirect_npm_allow_remote` warning (written / would write / already set / explicit value respected — naming the project file, the env var, or the user/global/builtin config path — / opted out / unreadable or unsupported), always with the tradeoff: `allow-remote=all` lets npm install ANY url-resolved dependency, not just Socket's patched ones, while the per-entry sha512 integrity pins stay enforced; the remedy for the non-writing variants is `allow-remote=all` in `.npmrc` or `npm ci --allow-remote=all`. npm <=11 is unaffected (11 defaults to `all`, <=10 has no such setting). **Unwind (v5.0: no ledger)**: once `rollback`, `remove` or a hosted → vendored takeover has restored the last hosted entry of the root `package-lock.json` / `npm-shrinkwrap.json` to its upstream registry entry (see "Hosted unwind coverage"), a project `.npmrc` holding exactly `allow-remote=all\n` (the file hosted mode creates) is deleted; any other `.npmrc` that still has a top-level `allow-remote=all` line is left untouched and the `npm_allow_remote_left` warning says the line may be removed if nothing else needs it (v5 keeps no record of whether hosted mode added it, so it is never removed behind the user's back). The rewrite's stage file is created with the `.npmrc`'s own permission bits (a 0600 token-bearing file is never staged world-readable). **Vendored mode is unaffected**: its `file:.socket/vendor/…` resolutions are npm `file` specs, which npm gates by `allow-file` (default `all`), never `allow-remote` — verified by the real npm 12 vendored matrix. diff --git a/crates/socket-patch-cli/src/commands/scan/hosted.rs b/crates/socket-patch-cli/src/commands/scan/hosted.rs index d6031e784..adb6c2f1b 100644 --- a/crates/socket-patch-cli/src/commands/scan/hosted.rs +++ b/crates/socket-patch-cli/src/commands/scan/hosted.rs @@ -2432,6 +2432,71 @@ mod tests { } } + /// #402: every key spelling pnpm reads as `trustLockfile` is the + /// setting — an explicit value is respected, never duplicated. + #[test] + fn plan_workspace_trust_reads_quoted_and_spaced_keys() { + for spelled in [ + "packages:\n - '.'\n\"trustLockfile\": false\n", + "packages:\n - '.'\ntrustLockfile : false\n", + "packages:\n - '.'\n'trustLockfile': false # opt out\n", + ] { + match plan_workspace_trust(Some(spelled)) { + TrustPlan::UserSet(value) => assert_eq!(value, "false", "{spelled:?}"), + _ => panic!("an explicit false must be respected for {spelled:?}"), + } + } + for spelled in [ + "'trustLockfile': true\npackages:\n - '.'\n", + "\"trustLockfile\" : \"true\"\n", + "trustLockfile: true # set by hand\n", + ] { + assert!( + matches!(plan_workspace_trust(Some(spelled)), TrustPlan::AlreadyTrue), + "already-true must be a no-op for {spelled:?}" + ); + } + } + + /// #400: the key goes inside the document — before a `...` marker — + /// and shapes a line append would corrupt are never appended to. + #[test] + fn plan_workspace_trust_respects_the_document_shape() { + match plan_workspace_trust(Some("packages:\n - '.'\n...\n")) { + TrustPlan::Append(text) => { + assert_eq!(text, "packages:\n - '.'\ntrustLockfile: true\n...\n") + } + _ => panic!("a `...`-terminated block mapping must plan an Append"), + } + // The refusal reason reaches the warning with both manual recoveries. + let TrustPlan::Unsupported(why) = plan_workspace_trust(Some("{packages: [.]}\n")) else { + panic!("a flow-style document must be refused"); + }; + let detail = socket_patch_core::hosted::guidance::pnpm_trust_workspace_unsupported_detail( + "the hosted patch server (patch.test)", + &why, + ); + assert!(detail.contains("flow-style"), "{detail}"); + assert!(detail.contains("left untouched"), "{detail}"); + assert!(detail.contains("--trust-lockfile"), "{detail}"); + assert!(detail.contains("trustLockfile: true"), "{detail}"); + assert!(detail.contains("pnpm clean --lockfile"), "{detail}"); + for text in [ + "{packages: [.]}\n", + "--- {packages: [.]}\n", + "packages:\n - '.'\n---\ncatalog: {}\n", + "packages:\n - '.'\n...\n---\ncatalog: {}\n", + ] { + assert!( + !matches!( + plan_workspace_trust(Some(text)), + TrustPlan::Append(_) | TrustPlan::Create(_) + ), + "{text:?} must not be appended to" + ); + } + } + /// The warning variants: the configured text says trust is in place and /// installs need no flags; the dry-run text says WOULD; both carry the /// whole-lock tradeoff disclosure and the don't-rebuild caution; the diff --git a/crates/socket-patch-cli/tests/in_process_redirect_pnpm.rs b/crates/socket-patch-cli/tests/in_process_redirect_pnpm.rs index 592d72c14..3c7338d28 100644 --- a/crates/socket-patch-cli/tests/in_process_redirect_pnpm.rs +++ b/crates/socket-patch-cli/tests/in_process_redirect_pnpm.rs @@ -364,6 +364,47 @@ async fn hosted_merges_trust_key_into_existing_workspace_yaml_byte_exactly() { ); } +/// #400 / #402: a workspace file the trust edit must not append to — an +/// explicit opt-out spelled with a quoted key, a `trustLockfile : false` +/// key, or a flow-style document — is left byte-identical (no duplicate key, +/// no block line after a flow mapping), while the lock is still redirected. +/// A `...`-terminated file gains the key inside the document. +#[tokio::test] +#[serial] +async fn hosted_trust_edit_reads_the_workspace_yaml_shape() { + let server = MockServer::start().await; + mock_discovery(&server).await; + mock_reference(&server).await; + + for (user_ws, want) in [ + ("packages:\n - '.'\n\"trustLockfile\": false\n", None), + ("packages:\n - '.'\ntrustLockfile : false\n", None), + ("{packages: [.]}\n", None), + ( + "packages:\n - '.'\n...\n", + Some("packages:\n - '.'\ntrustLockfile: true\n...\n"), + ), + ] { + let tmp = tempfile::tempdir().unwrap(); + write_pnpm_project(tmp.path()); + std::fs::write(tmp.path().join("pnpm-workspace.yaml"), user_ws).unwrap(); + + let code = run(hosted_args(tmp.path(), server.uri())).await; + assert_eq!(code, 0, "scan --mode hosted should succeed for {user_ws:?}"); + assert!( + std::fs::read_to_string(tmp.path().join("pnpm-lock.yaml")) + .unwrap() + .contains(HOSTED_URL), + "the lock is still redirected for {user_ws:?}" + ); + assert_eq!( + std::fs::read_to_string(tmp.path().join("pnpm-workspace.yaml")).unwrap(), + want.unwrap_or(user_ws), + "workspace file for {user_ws:?}" + ); + } +} + /// `--dry-run` previews: NOTHING lands on disk — no lock rewrite, no /// pnpm-workspace.yaml, no ledger — while the envelope still reports both /// files as would-be-rewritten (`dryRun: true`). diff --git a/crates/socket-patch-core/src/formats/pnpm/mod.rs b/crates/socket-patch-core/src/formats/pnpm/mod.rs index 1d495d69a..c7d47c1f2 100644 --- a/crates/socket-patch-core/src/formats/pnpm/mod.rs +++ b/crates/socket-patch-core/src/formats/pnpm/mod.rs @@ -27,6 +27,7 @@ pub(crate) mod grammar; pub(crate) mod hosted; pub(crate) mod lines; +pub(crate) mod workspace; pub(crate) use grammar::{entry_field, is_pnpm_lock_text, Entry, Resolution}; pub(crate) use hosted::plan_hosted; diff --git a/crates/socket-patch-core/src/formats/pnpm/workspace.rs b/crates/socket-patch-core/src/formats/pnpm/workspace.rs new file mode 100644 index 000000000..f30c28fc4 --- /dev/null +++ b/crates/socket-patch-core/src/formats/pnpm/workspace.rs @@ -0,0 +1,283 @@ +//! The two questions every pnpm-workspace.yaml line splice must ask first, +//! answered the way pnpm's YAML parser reads the file (#400, #402): +//! +//! * [`top_level_key`] — is this line a top-level key, and which one? A key +//! may be plain, single- or double-quoted, and may carry spaces before the +//! colon (`"trustLockfile": false`, `overrides :`); a literal line prefix +//! misses all of those and the splice then appends a duplicate key. +//! * [`block_insert_point`] — where a new top-level key can go. Only a +//! single block-mapping document can be extended by appending lines; a +//! flow-style root (`{packages: [.]}`), an indented root, or a second +//! document would be corrupted, so they are refused. A `...` end marker +//! is honoured by inserting before it. +//! +//! Pure text in, answers out; the editors own the reads and writes. + +/// The parsed key (quotes removed) and its inline value (comment and +/// surrounding blanks stripped; `""` for a block-valued key) when `line` is +/// a top-level mapping key. Indented lines, comments, sequence items and +/// document markers are not keys. +pub(crate) fn top_level_key(line: &str) -> Option<(String, &str)> { + let line = line.strip_suffix('\r').unwrap_or(line); + let first = *line.as_bytes().first()?; + if matches!(first, b' ' | b'\t' | b'#' | b'-' | b'{' | b'[' | b'%') || is_marker(line, "...") { + return None; + } + let (key, after) = match first { + b'"' => { + let (key, len) = double_quoted(line)?; + (key, &line[len..]) + } + b'\'' => { + let (key, len) = single_quoted(line)?; + (key, &line[len..]) + } + _ => { + let bytes = line.as_bytes(); + let colon = (0..bytes.len()).find(|&i| { + bytes[i] == b':' && bytes.get(i + 1).is_none_or(|&b| b == b' ' || b == b'\t') + })?; + let key = line[..colon].trim_end(); + if key.is_empty() || key.contains(" #") { + return None; + } + (key.to_string(), &line[colon..]) + } + }; + let rest = after.trim_start_matches([' ', '\t']).strip_prefix(':')?; + if !(rest.is_empty() || rest.starts_with([' ', '\t'])) { + return None; + } + Some((key, strip_comment(rest).trim())) +} + +/// The line index a new top-level key is inserted at: after the document's +/// last non-blank line, before a `...` end marker. `Err` names why the +/// document is not a single block mapping a line append can extend. +pub(crate) fn block_insert_point(lines: &[String]) -> Result { + const MULTI: &str = "holds more than one YAML document"; + let mut started = false; // a `---` start marker seen + let mut content = false; // the root mapping's first key seen + let mut end_marker = None; + let mut last = None; // the last non-blank line of the document + for (i, raw) in lines.iter().enumerate() { + let line = raw.strip_suffix('\r').unwrap_or(raw); + let comment = line.trim_start().starts_with('#'); + if end_marker.is_some() { + if !(line.trim().is_empty() || comment) { + return Err(MULTI.to_string()); + } + continue; + } + if is_marker(line, "---") { + if content || started { + return Err(MULTI.to_string()); + } + if !strip_comment(&line[3..]).trim().is_empty() { + return Err("is not a block mapping (its root is on the `---` line)".to_string()); + } + started = true; + } else if is_marker(line, "...") { + end_marker = Some(i); + continue; + } else if line.trim().is_empty() { + continue; + } else if !content && !comment && !line.starts_with('%') { + if line.starts_with([' ', '\t']) { + return Err("is not a block mapping at column 0".to_string()); + } + if line.starts_with(['{', '[']) { + return Err("is a flow-style YAML document".to_string()); + } + if top_level_key(line).is_none() { + return Err("is not a block mapping".to_string()); + } + content = true; + } + last = Some(i); + } + Ok(match (last, end_marker) { + (Some(i), _) => i + 1, + (None, Some(end)) => end, + (None, None) => lines.len(), + }) +} + +/// The top-level `name:` section holding a block mapping: `(header, end)`, +/// `end` being the next column-0 line that is not a comment (exclusive) — a +/// `#` comment at column 0 does not close a YAML block mapping. Unlike +/// `lines::section_bounds` it accepts every spelling of the key. `None` when +/// absent or inline-valued. +pub(crate) fn block_section_bounds(lines: &[String], name: &str) -> Option<(usize, usize)> { + let start = lines.iter().position(|l| { + top_level_key(l).is_some_and(|(key, value)| key == name && value.is_empty()) + })?; + let end = lines + .iter() + .enumerate() + .skip(start + 1) + .find(|(_, l)| { + let l = l.strip_suffix('\r').unwrap_or(l); + !l.is_empty() && !l.starts_with([' ', '\t', '#']) + }) + .map(|(i, _)| i) + .unwrap_or(lines.len()); + Some((start, end)) +} + +/// Whether `line` is the document marker `marker` (`---` / `...`), alone or +/// followed by a blank. +fn is_marker(line: &str, marker: &str) -> bool { + line.strip_prefix(marker) + .is_some_and(|rest| rest.is_empty() || rest.starts_with([' ', '\t'])) +} + +/// `text` up to a ` #` comment that sits outside quotes. +fn strip_comment(text: &str) -> &str { + let bytes = text.as_bytes(); + let mut quote = None; + for (i, &b) in bytes.iter().enumerate() { + match quote { + Some(q) if b == q => quote = None, + Some(_) => {} + None if b == b'\'' || b == b'"' => quote = Some(b), + None if b == b'#' && (i == 0 || matches!(bytes[i - 1], b' ' | b'\t')) => { + return &text[..i]; + } + None => {} + } + } + text +} + +/// A double-quoted scalar at the start of `s`: its value and byte length. +fn double_quoted(s: &str) -> Option<(String, usize)> { + let mut out = String::new(); + let mut chars = s.char_indices().skip(1); + while let Some((i, c)) = chars.next() { + match c { + '"' => return Some((out, i + 1)), + '\\' => { + let (_, esc) = chars.next()?; + match esc { + 'n' => out.push('\n'), + 't' => out.push('\t'), + '"' | '\\' | '/' | ' ' => out.push(esc), + // Any other escape decodes to text no pnpm key uses; + // keep it visibly distinct rather than guessing. + other => { + out.push('\\'); + out.push(other); + } + } + } + c => out.push(c), + } + } + None +} + +/// A single-quoted scalar at the start of `s` (`''` is a literal quote). +fn single_quoted(s: &str) -> Option<(String, usize)> { + let bytes = s.as_bytes(); + let mut out = String::new(); + let mut i = 1; + while i < bytes.len() { + if bytes[i] == b'\'' { + if bytes.get(i + 1) == Some(&b'\'') { + out.push('\''); + i += 2; + continue; + } + return Some((out, i + 1)); + } + let c = s[i..].chars().next()?; + out.push(c); + i += c.len_utf8(); + } + None +} + +#[cfg(test)] +mod tests { + use super::*; + + fn lines(text: &str) -> Vec { + text.split('\n').map(str::to_string).collect() + } + + #[test] + fn top_level_key_reads_every_spelling() { + for (line, key, value) in [ + ("trustLockfile: true", "trustLockfile", "true"), + ("\"trustLockfile\": false", "trustLockfile", "false"), + ( + "'trustLockfile': false # opt out", + "trustLockfile", + "false", + ), + ("trustLockfile : false", "trustLockfile", "false"), + ("\"trust\\\"q\" : 1", "trust\"q", "1"), + ("'it''s': x", "it's", "x"), + ("overrides:", "overrides", ""), + ("overrides: # pins", "overrides", ""), + ("overrides:\r", "overrides", ""), + ("url: 'a # b' # c", "url", "'a # b'"), + ("a:b: c", "a:b", "c"), + ] { + assert_eq!( + top_level_key(line), + Some((key.to_string(), value)), + "{line:?}" + ); + } + for line in [ + " trustLockfile: true", + "# trustLockfile: true", + "- a: b", + "---", + "...", + "{a: b}", + "plain scalar", + "\"unterminated: x", + "\"key\"x: y", + "key:value", + ] { + assert_eq!(top_level_key(line), None, "{line:?}"); + } + } + + #[test] + fn block_insert_point_follows_the_document() { + assert_eq!(block_insert_point(&lines("a: 1\nb:\n - c\n")), Ok(3)); + assert_eq!(block_insert_point(&lines("a: 1\n\n# tail\n\n")), Ok(3)); + assert_eq!(block_insert_point(&lines("a: 1\n...\n")), Ok(1)); + assert_eq!(block_insert_point(&lines("a: 1\n\n...\n# done\n")), Ok(1)); + assert_eq!(block_insert_point(&lines("%YAML 1.2\n---\na: 1\n")), Ok(3)); + assert_eq!(block_insert_point(&lines("# only a comment\n")), Ok(1)); + assert_eq!(block_insert_point(&lines("")), Ok(1)); + for text in [ + "{a: 1}\n", + "[a]\n", + "--- {a: 1}\n", + " a: 1\n", + "- a\n", + "a: 1\n---\nb: 2\n", + "a: 1\n...\nb: 2\n", + "---\n---\n", + ] { + assert!(block_insert_point(&lines(text)).is_err(), "{text:?}"); + } + } + + #[test] + fn block_section_bounds_matches_any_key_spelling() { + let l = lines("packages:\n - '.'\n\"overrides\":\n a: 1\nnext: x\n"); + assert_eq!(block_section_bounds(&l, "overrides"), Some((2, 4))); + // A column-0 comment (or a stray `\r` blank) stays inside the section. + let l = lines("\"overrides\":\n a: 1\n# note\n\r\n b: 2\nnext: x\n"); + assert_eq!(block_section_bounds(&l, "overrides"), Some((0, 5))); + let inline = lines("overrides : {a: 1}\n"); + assert_eq!(block_section_bounds(&inline, "overrides"), None); + } +} diff --git a/crates/socket-patch-core/src/hosted/engine.rs b/crates/socket-patch-core/src/hosted/engine.rs index 2d9173b9a..01a1a59b5 100644 --- a/crates/socket-patch-core/src/hosted/engine.rs +++ b/crates/socket-patch-core/src/hosted/engine.rs @@ -46,9 +46,9 @@ use super::guidance::{ npm_allow_remote_user_set_detail, npm_lock_url_needles, plan_workspace_trust, pnpm_heal_root, pnpm_lock_may_need_store_flag, pnpm_lock_version_major, pnpm_trust_configured_detail, pnpm_trust_legacy_detail, pnpm_trust_manual_guidance, pnpm_trust_policy_preamble, - pnpm_trust_workspace_unreadable_detail, read_npmrc_for_allow_remote, read_workspace_for_trust, - url_host, TrustPlan, NPM_LOCKS, PNPM_TRUST_TRADEOFF_AND_CAUTION, PNPM_WORKSPACE_REL, - REDIRECT_PNPM_WORKSPACE_TRUST_EDIT_KIND, + pnpm_trust_workspace_unreadable_detail, pnpm_trust_workspace_unsupported_detail, + read_npmrc_for_allow_remote, read_workspace_for_trust, url_host, TrustPlan, NPM_LOCKS, + PNPM_TRUST_TRADEOFF_AND_CAUTION, PNPM_WORKSPACE_REL, REDIRECT_PNPM_WORKSPACE_TRUST_EDIT_KIND, }; use super::vlt::bun_lockb_present; @@ -1057,6 +1057,9 @@ fn pnpm_trust( artifacts. {PNPM_TRUST_TRADEOFF_AND_CAUTION}", pnpm_trust_policy_preamble(&server), ), + TrustPlan::Unsupported(why) => { + pnpm_trust_workspace_unsupported_detail(&server, &why) + } }, } }; diff --git a/crates/socket-patch-core/src/hosted/guidance.rs b/crates/socket-patch-core/src/hosted/guidance.rs index 8473ff64d..51ec85483 100644 --- a/crates/socket-patch-core/src/hosted/guidance.rs +++ b/crates/socket-patch-core/src/hosted/guidance.rs @@ -91,6 +91,24 @@ pub fn pnpm_trust_legacy_detail(server: &str) -> String { ) } +/// The unspliceable-workspace fallback: pnpm-workspace.yaml is valid YAML +/// that a line append would corrupt (a flow-style root, several +/// documents), so the auto-config stands down and the warning names the +/// reason and both manual recoveries. +pub fn pnpm_trust_workspace_unsupported_detail(server: &str, why: &str) -> String { + format!( + "{}. {PNPM_WORKSPACE_REL} {why}, which the trust edit cannot extend \ + without corrupting it; it was left untouched. Install with \ + `pnpm install --trust-lockfile`, or add `trustLockfile: true` to it \ + yourself so every install accepts the patched artifacts. Do NOT \ + follow pnpm's advice to rebuild the lockfile (`pnpm clean \ + --lockfile`): that silently discards the hosted patches and \ + reinstalls the vulnerable upstream artifact. pnpm <=10 installs \ + work unchanged", + pnpm_trust_policy_preamble(server), + ) +} + /// The unreadable-workspace fallback: pnpm-workspace.yaml EXISTS but could /// not be read (permissions, invalid UTF-8, I/O error). Planning a Create /// here would OVERWRITE the user's file with the root-only scaffold — @@ -218,6 +236,10 @@ pub enum TrustPlan { /// call is respected — flipping an explicit security setting behind the /// user's back is worse than a failing install with a clear warning. UserSet(String), + /// The file is valid YAML a line splice cannot extend (a flow-style + /// root, an indented root, several documents): nothing is written and + /// the reason is surfaced, since appending would corrupt it. + Unsupported(String), } /// Decide how to ensure `trustLockfile: true` in pnpm-workspace.yaml. @@ -225,28 +247,35 @@ pub enum TrustPlan { /// workspace surgery: untouched lines stay byte-identical, so a revert can /// remove exactly what was added. pub fn plan_workspace_trust(existing: Option<&str>) -> TrustPlan { + use crate::formats::pnpm::workspace::{block_insert_point, top_level_key}; let Some(text) = existing else { return TrustPlan::Create("packages:\n - '.'\ntrustLockfile: true\n".to_string()); }; - // Top-level key only: an indented `trustLockfile:` under some other + let mut lines: Vec = text.split('\n').map(str::to_string).collect(); + // Where the key would go — refused when the document is not a single + // block mapping, so the scan for an existing key below is meaningful. + let anchor = match block_insert_point(&lines) { + Ok(anchor) => anchor, + Err(why) => return TrustPlan::Unsupported(why), + }; + // Top-level key only (every spelling pnpm reads: quoted, `key :`, a + // trailing comment): an indented `trustLockfile:` under some other // mapping is not the setting pnpm reads. - for line in text.split('\n') { - if let Some(rest) = line.strip_prefix("trustLockfile:") { - let value = rest.trim().trim_matches(|c| c == '\'' || c == '"'); + for line in &lines { + if let Some((key, value)) = top_level_key(line) { + if key != "trustLockfile" { + continue; + } + let value = value.trim_matches(|c| c == '\'' || c == '"'); if value == "true" { return TrustPlan::AlreadyTrue; } return TrustPlan::UserSet(value.to_string()); } } - let mut lines: Vec = text.split('\n').map(str::to_string).collect(); - // After the last non-empty line (no blank separator): a revert removes - // exactly one line and the file's trailing bytes stay put. - let anchor = lines - .iter() - .rposition(|l| !l.trim().is_empty()) - .map(|i| i + 1) - .unwrap_or(lines.len()); + // After the document's last non-empty line (no blank separator; before a + // `...` end marker): a revert removes exactly one line and the file's + // trailing bytes stay put. lines.insert(anchor, "trustLockfile: true".to_string()); TrustPlan::Append(lines.join("\n")) } diff --git a/crates/socket-patch-core/src/patch/redirect/upstream/npm.rs b/crates/socket-patch-core/src/patch/redirect/upstream/npm.rs index 379726b25..1c61efeb8 100644 --- a/crates/socket-patch-core/src/patch/redirect/upstream/npm.rs +++ b/crates/socket-patch-core/src/patch/redirect/upstream/npm.rs @@ -738,7 +738,11 @@ pub(crate) async fn cleanup_side_config( if let Ok(Some(ws)) = view.read(WORKSPACE).await { if ws == "packages:\n - '.'\ntrustLockfile: true\n" { view.remove(WORKSPACE); - } else if ws.lines().any(|l| l.trim_end() == "trustLockfile: true") { + } else if ws.lines().any(|l| { + crate::formats::pnpm::workspace::top_level_key(l).is_some_and(|(key, value)| { + key == "trustLockfile" && value.trim_matches(['\'', '"']) == "true" + }) + }) { result.warnings.push(( "pnpm_trust_lockfile_left", format!( diff --git a/crates/socket-patch-core/src/vendor/pnpm_lock.rs b/crates/socket-patch-core/src/vendor/pnpm_lock.rs index 6fd5c4cc1..e77a4efdb 100644 --- a/crates/socket-patch-core/src/vendor/pnpm_lock.rs +++ b/crates/socket-patch-core/src/vendor/pnpm_lock.rs @@ -76,6 +76,7 @@ use crate::formats::pnpm::lines::{ indent_of, next_block, parse_key_line, section_bounds, split_lines, unquote_value, yaml_key, yaml_key_like, YamlBlock, }; +use crate::formats::pnpm::workspace; use crate::formats::pnpm::{check_v9_lock_version as check_lock_version, vendored_npm_uuids}; const PACKAGE_JSON: &str = "package.json"; @@ -1586,14 +1587,16 @@ struct WorkspaceEdit { created_overrides: bool, } -/// Locate the top-level `overrides:` block and the indent its entries use +/// Locate the top-level `overrides:` block (any key spelling pnpm reads: +/// quoted, `overrides :`, a trailing comment) and the indent its entries use /// (pnpm's canonical is 2 spaces; a hand-authored file may differ). `None` /// when there is no block-style `overrides:` section. fn ws_overrides_section(lines: &[String]) -> Option<(usize, usize, usize)> { - let (start, end) = section_bounds(lines, "overrides")?; + let (start, end) = workspace::block_section_bounds(lines, "overrides")?; + // Comment lines (any indent) say nothing about the entries' indent. let indent = lines[start + 1..end] .iter() - .find(|l| !l.trim().is_empty()) + .find(|l| !l.trim().is_empty() && !l.trim_start().starts_with('#')) .map(|l| indent_of(l)) .filter(|&n| n >= 1) .unwrap_or(2); @@ -1615,10 +1618,18 @@ fn check_workspace_override( return Ok(()); }; let lines = split_lines(text); - if lines - .iter() - .any(|l| l.starts_with("overrides:") && l.trim_end() != "overrides:") - { + // A document the line surgery cannot extend (flow-style root, several + // documents) would be corrupted by any splice: refuse before writing. + if let Err(why) = workspace::block_insert_point(&lines) { + return Err(format!( + "{PNPM_WORKSPACE} {why}, which the override surgery cannot edit without \ + corrupting it — rewrite it as a single block mapping and re-run" + )); + } + if lines.iter().any(|l| { + workspace::top_level_key(l) + .is_some_and(|(key, value)| key == "overrides" && !value.is_empty()) + }) { return Err(format!( "{PNPM_WORKSPACE} has an inline `overrides:` mapping the pair surgery cannot \ edit — rewrite it as a block mapping (`overrides:` then indented entries) \ @@ -1716,14 +1727,12 @@ fn apply_workspace_override( }); } - // File exists without an `overrides:` section: append one after the last - // non-empty line (no blank separator, so revert removes exactly two - // lines and the file's trailing bytes stay put). - let anchor = lines - .iter() - .rposition(|l| !l.trim().is_empty()) - .map(|i| i + 1) - .unwrap_or(lines.len()); + // File exists without an `overrides:` section: append one after the + // document's last non-empty line, before a `...` end marker (no blank + // separator, so revert removes exactly two lines and the file's trailing + // bytes stay put). + let anchor = workspace::block_insert_point(&lines) + .map_err(|why| format!("{PNPM_WORKSPACE} {why}; the overrides section cannot be added"))?; lines.splice( anchor..anchor, [ @@ -8512,4 +8521,165 @@ snapshots: "1.3.0" )); } + + /// pnpm-workspace.yaml spellings and document shapes the overrides surgery + /// must read the way pnpm's YAML parser does (#400, #402). + mod workspace_yaml_shape_tests { + use super::*; + + const SPEC: &str = "file:.socket/vendor/npm/u1/left-pad-1.3.0.tgz"; + + fn apply(text: &str) -> Result { + check_workspace_override(Some(text), "left-pad", "1.3.0", "left-pad@1.3.0")?; + let mut wiring = Vec::new(); + let edit = apply_workspace_override(Some(text), "left-pad@1.3.0", SPEC, &mut wiring)?; + Ok(edit.new_text.expect("an edit")) + } + + /// #402: a quoted or space-before-colon `overrides` key is the section + /// pnpm reads; the entry goes inside it, never into a duplicate key. + #[test] + fn quoted_or_spaced_overrides_key_is_edited_in_place() { + for header in [ + "\"overrides\":", + "'overrides':", + "overrides :", + "overrides: # pins", + ] { + let text = format!("packages:\n - '.'\n{header}\n is-number: 7.0.0\n"); + assert_eq!( + apply(&text).unwrap(), + format!( + "packages:\n - '.'\n{header}\n is-number: 7.0.0\n left-pad@1.3.0: {SPEC}\n" + ), + "{header}" + ); + } + } + + /// A column-0 comment inside the section does not hide later + /// entries from the check, the edit or the revert. + #[test] + fn column_zero_comment_inside_the_section_keeps_later_entries() { + let text = "packages:\n - '.'\noverrides:\n is-number: 7.0.0\n# pinned\n left-pad@1.3.0: 1.3.1\n"; + let err = check_workspace_override(Some(text), "left-pad", "1.3.0", "left-pad@1.3.0") + .unwrap_err(); + assert!(err.contains("already carries an override"), "{err}"); + + let text = format!( + "overrides:\n is-number: 7.0.0\n# ours below\n left-pad@1.3.0: {SPEC}\nnext: x\n" + ); + // Our entry below the comment is found: already in sync, no edit. + let mut wiring = Vec::new(); + let edit = + apply_workspace_override(Some(&text), "left-pad@1.3.0", SPEC, &mut wiring).unwrap(); + assert!(edit.new_text.is_none(), "{:?}", edit.new_text); + let rec = ws_record("left-pad@1.3.0", SPEC, WiringAction::Added, None); + let mut lines = split_lines(&text); + let (mut dirty, mut warnings) = (false, Vec::new()); + revert_ws_record(&mut lines, &rec, "u1", &mut dirty, &mut warnings); + assert!(dirty && warnings.is_empty(), "{warnings:?}"); + assert_eq!( + lines.join("\n"), + "overrides:\n is-number: 7.0.0\n# ours below\nnext: x\n" + ); + } + + /// A leading comment does not decide the entries' indent: a + /// 4-space section is still read, edited and reverted as such. + #[test] + fn leading_comment_does_not_set_the_entry_indent() { + for comment in ["# pins", " # pins"] { + let text = format!("overrides:\n{comment}\n left-pad@1.3.0: 1.3.1\nnext: x\n"); + let err = + check_workspace_override(Some(&text), "left-pad", "1.3.0", "left-pad@1.3.0") + .unwrap_err(); + assert!( + err.contains("already carries an override"), + "{comment:?}: {err}" + ); + let text = format!("overrides:\n{comment}\n is-number: 7.0.0\nnext: x\n"); + assert_eq!( + apply(&text).unwrap(), + format!( + "overrides:\n{comment}\n is-number: 7.0.0\n left-pad@1.3.0: {SPEC}\nnext: x\n" + ), + "{comment:?}" + ); + } + } + + /// #402: the pre-flight conflict check examines a quoted section too. + #[test] + fn quoted_overrides_section_conflict_is_refused() { + let text = "packages:\n - '.'\n\"overrides\":\n left-pad@1.3.0: 1.3.1\n"; + let err = check_workspace_override(Some(text), "left-pad", "1.3.0", "left-pad@1.3.0") + .unwrap_err(); + assert!(err.contains("already carries an override"), "{err}"); + } + + /// #402: a quoted inline mapping is refused like an unquoted one. + #[test] + fn quoted_inline_overrides_mapping_is_refused() { + for line in [ + "\"overrides\": {is-number: 7.0.0}", + "overrides : {is-number: 7.0.0}", + ] { + let text = format!("packages:\n - '.'\n{line}\n"); + let err = + check_workspace_override(Some(&text), "left-pad", "1.3.0", "left-pad@1.3.0") + .unwrap_err(); + assert!(err.contains("inline `overrides:`"), "{line}: {err}"); + } + } + + /// #402: revert finds our key inside a quoted section. + #[test] + fn revert_removes_our_key_from_a_quoted_section() { + let rec = ws_record("left-pad@1.3.0", SPEC, WiringAction::Added, None); + let mut lines = split_lines(&format!( + "packages:\n - '.'\n\"overrides\":\n is-number: 7.0.0\n left-pad@1.3.0: {SPEC}\n" + )); + let (mut dirty, mut warnings) = (false, Vec::new()); + revert_ws_record(&mut lines, &rec, "u1", &mut dirty, &mut warnings); + assert!(dirty && warnings.is_empty(), "{warnings:?}"); + assert_eq!( + lines.join("\n"), + "packages:\n - '.'\n\"overrides\":\n is-number: 7.0.0\n" + ); + } + + /// #400: a `...` document-end marker keeps the new section inside the + /// document (inserted before the marker). + #[test] + fn document_end_marker_keeps_the_section_in_the_document() { + assert_eq!( + apply("packages:\n - '.'\n...\n").unwrap(), + format!("packages:\n - '.'\noverrides:\n left-pad@1.3.0: {SPEC}\n...\n") + ); + // A leading `---` document start is an ordinary single document. + assert_eq!( + apply("---\npackages:\n - '.'\n").unwrap(), + format!("---\npackages:\n - '.'\noverrides:\n left-pad@1.3.0: {SPEC}\n") + ); + } + + /// #400: shapes a line splice cannot extend are refused before any write. + #[test] + fn unspliceable_document_shapes_are_refused() { + for text in [ + "{packages: [.]}\n", + "{\n packages: [.]\n}\n", + "--- {packages: [.]}\n", + "packages:\n - '.'\n...\n---\ncatalog: {}\n", + "packages:\n - '.'\n---\ncatalog: {}\n", + " packages:\n - '.'\n", + ] { + let err = + check_workspace_override(Some(text), "left-pad", "1.3.0", "left-pad@1.3.0") + .unwrap_err(); + assert!(err.contains(PNPM_WORKSPACE), "{text:?}: {err}"); + } + } + } }