Skip to content

Commit fab9b08

Browse files
committed
Route nested pnpm settings through the decoder
Review follow-ups for #1918: a double-quoted value with an escaped quote before " #" is no longer cut at the quote, YAML escapes (surrogate pairs, \x, \U, \b) decode as pnpm reads them and a malformed one is not guessed, a block scalar header is not read as the value, and the trustLockfile, registries and yarn nested readers use the same decoder instead of trimming quote characters. Assisted-by: Claude Code:claude-opus-5-5
1 parent e913c96 commit fab9b08

4 files changed

Lines changed: 132 additions & 58 deletions

File tree

‎crates/socket-patch-core/src/formats/pnpm/version.rs‎

Lines changed: 9 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -98,18 +98,15 @@ fn version_bounds(spec: &str) -> Option<(Release, Release)> {
9898
})
9999
}
100100

101-
/// The `packageManager` value of a `.modules.yaml`.
102-
fn modules_yaml_package_manager(text: &str) -> Option<String> {
103-
super::modules_yaml::field(text, "packageManager")
104-
}
105-
106101
/// Every pnpm version pin the project's files carry, as `(where, spec)`.
107102
fn pnpm_pins(
108103
package_json: Option<&str>,
109104
modules_yaml: Option<&str>,
110105
) -> Vec<(&'static str, String)> {
111106
let mut pins = Vec::new();
112-
if let Some(spec) = modules_yaml.and_then(modules_yaml_package_manager) {
107+
if let Some(spec) =
108+
modules_yaml.and_then(|text| super::modules_yaml::field(text, "packageManager"))
109+
{
113110
if let Some(version) = pinned_version(&spec, "pnpm") {
114111
pins.push(("node_modules/.modules.yaml", version.to_string()));
115112
}
@@ -201,20 +198,13 @@ mod tests {
201198
/// #1918: a YAML `.modules.yaml` is read like every other pnpm settings
202199
/// file: the last top-level key wins, comments are not part of the value.
203200
#[test]
204-
fn modules_yaml_package_manager_reads_the_last_key() {
201+
fn modules_yaml_pin_reads_the_last_key() {
202+
let pins = |text: &str| pnpm_pins(None, Some(text));
205203
let text = "packageManager: pnpm@9.0.0\npackageManager: pnpm@11.0.0 # newer\n";
206-
assert_eq!(
207-
modules_yaml_package_manager(text).as_deref(),
208-
Some("pnpm@11.0.0")
209-
);
210-
assert_eq!(
211-
modules_yaml_package_manager("packageManager: 'pnpm@8.6.0'\n").as_deref(),
212-
Some("pnpm@8.6.0")
213-
);
214-
assert_eq!(
215-
modules_yaml_package_manager("{\"packageManager\":\"pnpm@10.1.0\"}").as_deref(),
216-
Some("pnpm@10.1.0")
217-
);
204+
let want = vec![("node_modules/.modules.yaml", "11.0.0".to_string())];
205+
assert_eq!(pins(text), want);
206+
assert_eq!(pins("packageManager: 'pnpm@11.0.0'\n"), want);
207+
assert_eq!(pins("{\"packageManager\":\"pnpm@11.0.0\"}"), want);
218208
}
219209

220210
#[test]

‎crates/socket-patch-core/src/formats/pnpm/workspace.rs‎

Lines changed: 66 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -58,15 +58,20 @@ pub(crate) fn top_level_key(line: &str) -> Option<(String, &str)> {
5858

5959
/// The value of the last top-level `key` in a YAML settings file
6060
/// (pnpm-workspace.yaml, .yarnrc.yml, a YAML `.modules.yaml`), decoded by
61-
/// [`yaml_scalar`]. An empty or block value is `Some("")`; callers that
62-
/// read empty as unset use [`yaml_top_level_value`]. [`top_level_key`]
61+
/// [`yaml_scalar`]. An empty value or a nested block is `Some("")`
62+
/// (callers that read empty as unset use [`yaml_top_level_value`]); a
63+
/// block scalar (`key: |`) is `None`, unread. [`top_level_key`]
6364
/// skips the first line's BOM. Every pnpm settings reader goes through
6465
/// this one function so a quoted value reads the same everywhere (#1918).
6566
pub(crate) fn top_level_scalar(text: &str, key: &str) -> Option<String> {
6667
text.lines()
6768
.filter_map(top_level_key)
6869
.rfind(|(k, _)| k == key)
69-
.map(|(_, value)| yaml_scalar(value))
70+
.and_then(|(_, value)| {
71+
// A block scalar (`|`, `>-`) continues on the next lines; its
72+
// value is not on this one, so it is not read here.
73+
(!value.starts_with(['|', '>'])).then(|| yaml_scalar(value))
74+
})
7075
}
7176

7277
/// [`top_level_scalar`], with an empty value read as unset.
@@ -332,8 +337,13 @@ fn is_marker(line: &str, marker: &str) -> bool {
332337
pub(crate) fn strip_comment(text: &str) -> &str {
333338
let bytes = text.as_bytes();
334339
let mut quote = None;
340+
let mut escaped = false;
335341
for (i, &b) in bytes.iter().enumerate() {
336342
match quote {
343+
// Inside double quotes a backslash escapes the next byte, so
344+
// `\"` does not close the scalar (#1918).
345+
Some(b'"') if escaped => escaped = false,
346+
Some(b'"') if b == b'\\' => escaped = true,
337347
Some(q) if b == q => quote = None,
338348
Some(_) => {}
339349
None if b == b'\'' || b == b'"' => quote = Some(b),
@@ -347,35 +357,60 @@ pub(crate) fn strip_comment(text: &str) -> &str {
347357
}
348358

349359
/// A double-quoted scalar at the start of `s`: its value and byte length.
360+
/// Escapes decode as YAML 1.2 (and js-yaml) does, `\uD83D\uDE00`
361+
/// surrogate pairs included; a malformed escape is `None`, as the file
362+
/// does not parse.
350363
fn double_quoted(s: &str) -> Option<(String, usize)> {
364+
/// `n` hex digits as a code point (`None` when short or not hex).
365+
fn hex(chars: &mut impl Iterator<Item = (usize, char)>, n: usize) -> Option<u32> {
366+
let digits: String = (0..n).map_while(|_| chars.next().map(|(_, h)| h)).collect();
367+
(digits.len() == n && digits.chars().all(|c| c.is_ascii_hexdigit()))
368+
.then(|| u32::from_str_radix(&digits, 16).ok())
369+
.flatten()
370+
}
351371
let mut out = String::new();
352372
let mut chars = s.char_indices().skip(1);
353373
while let Some((i, c)) = chars.next() {
354374
match c {
355375
'"' => return Some((out, i + 1)),
356376
'\\' => {
357377
let (_, esc) = chars.next()?;
358-
match esc {
359-
'n' => out.push('\n'),
360-
't' => out.push('\t'),
361-
'r' => out.push('\r'),
378+
let decoded = match esc {
379+
'0' => '\0',
380+
'a' => '\u{7}',
381+
'b' => '\u{8}',
382+
't' | '\t' => '\t',
383+
'n' => '\n',
384+
'v' => '\u{b}',
385+
'f' => '\u{c}',
386+
'r' => '\r',
387+
'e' => '\u{1b}',
388+
'N' => '\u{85}',
389+
'_' => '\u{a0}',
390+
'L' => '\u{2028}',
391+
'P' => '\u{2029}',
392+
'"' | '\\' | '/' | ' ' => esc,
393+
'x' => char::from_u32(hex(&mut chars, 2)?)?,
394+
'U' => char::from_u32(hex(&mut chars, 8)?)?,
362395
'u' => {
363-
let hex: String = (0..4)
364-
.filter_map(|_| chars.next().map(|(_, h)| h))
365-
.collect();
366-
let code = u32::from_str_radix(&hex, 16)
367-
.ok()
368-
.filter(|_| hex.len() == 4);
369-
out.push(code.and_then(char::from_u32)?);
370-
}
371-
'"' | '\\' | '/' | ' ' => out.push(esc),
372-
// Any other escape decodes to text no pnpm key uses;
373-
// keep it visibly distinct rather than guessing.
374-
other => {
375-
out.push('\\');
376-
out.push(other);
396+
let unit = hex(&mut chars, 4)?;
397+
if (0xD800..0xDC00).contains(&unit) {
398+
// A high surrogate pairs with a following `\uDCxx`.
399+
if chars.next()?.1 != '\\' || chars.next()?.1 != 'u' {
400+
return None;
401+
}
402+
let low = hex(&mut chars, 4)?;
403+
if !(0xDC00..0xE000).contains(&low) {
404+
return None;
405+
}
406+
char::from_u32(0x10000 + ((unit - 0xD800) << 10) + (low - 0xDC00))?
407+
} else {
408+
char::from_u32(unit)?
409+
}
377410
}
378-
}
411+
_ => return None,
412+
};
413+
out.push(decoded);
379414
}
380415
c => out.push(c),
381416
}
@@ -424,6 +459,15 @@ mod tests {
424459
("k: 'a' b\n", Some("'a' b")),
425460
("\u{feff}k: x\n", Some("x")),
426461
("k: a\r\nk: 'b'\r\n", Some("b")),
462+
// Review on #1973: escapes the old JSON decoder read, an escaped
463+
// quote before ` #`, a malformed escape, and a block scalar.
464+
("k: \"/p/a\\\" #b/.pnpm\"\n", Some("/p/a\" #b/.pnpm")),
465+
("k: \"nm-\\ud83d\\ude00\"\n", Some("nm-\u{1f600}")),
466+
("k: \"a\\bb\\x41\\U0001F600\"\n", Some("a\u{8}bA\u{1f600}")),
467+
("k: \"\\u+041\"\n", Some("\"\\u+041\"")),
468+
("k: \"\\ud83d\"\n", Some("\"\\ud83d\"")),
469+
("k: >-\n long value\n", None),
470+
("k: |\n text\n", None),
427471
(" k: nested\n", None),
428472
("other: x\n", None),
429473
] {

‎crates/socket-patch-core/src/hosted/guidance.rs‎

Lines changed: 30 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -377,7 +377,7 @@ pub const TRUST_SCAFFOLD_LINES: [&str; 3] = [
377377
/// workspace surgery: untouched lines stay byte-identical, so a revert can
378378
/// remove exactly what was added.
379379
pub fn plan_workspace_trust(existing: Option<&str>) -> TrustPlan {
380-
use crate::formats::pnpm::workspace::{block_insert_point, top_level_key};
380+
use crate::formats::pnpm::workspace::{block_insert_point, top_level_key, yaml_scalar};
381381
let Some(text) = existing else {
382382
return TrustPlan::Create("packages:\n - '.'\ntrustLockfile: true\n".to_string());
383383
};
@@ -401,17 +401,18 @@ pub fn plan_workspace_trust(existing: Option<&str>) -> TrustPlan {
401401
// Top-level key only (every spelling pnpm reads: quoted, `key :`, a
402402
// trailing comment): an indented `trustLockfile:` under some other
403403
// mapping is not the setting pnpm reads.
404-
for line in &lines {
405-
if let Some((key, value)) = top_level_key(line) {
406-
if key != "trustLockfile" {
407-
continue;
408-
}
409-
let value = value.trim_matches(|c| c == '\'' || c == '"');
410-
if value == "true" {
411-
return TrustPlan::AlreadyTrue;
412-
}
413-
return TrustPlan::UserSet(value.to_string());
404+
// The last assignment is the one pnpm reads, decoded by the shared pnpm
405+
// settings reader (#1918).
406+
let existing = lines
407+
.iter()
408+
.filter_map(|line| top_level_key(line))
409+
.rfind(|(key, _)| key == "trustLockfile");
410+
if let Some((_, value)) = existing {
411+
let value = yaml_scalar(value);
412+
if value == "true" {
413+
return TrustPlan::AlreadyTrue;
414414
}
415+
return TrustPlan::UserSet(value);
415416
}
416417
// After the document's last non-empty line (no blank separator; before a
417418
// `...` end marker): a revert removes exactly one line and the file's
@@ -662,6 +663,24 @@ mod tests {
662663
assert!(!merged.contains("packageManager"), "{merged}");
663664
}
664665

666+
/// #1918: the last `trustLockfile:` is the one pnpm reads, decoded by
667+
/// the shared pnpm settings reader (an unpaired quote is not `true`).
668+
#[test]
669+
fn workspace_trust_plan_reads_the_last_key_as_yaml() {
670+
assert!(matches!(
671+
plan_workspace_trust(Some("trustLockfile: false\ntrustLockfile: true\n")),
672+
TrustPlan::AlreadyTrue
673+
));
674+
match plan_workspace_trust(Some("trustLockfile: \"true'\n")) {
675+
TrustPlan::UserSet(value) => assert_eq!(value, "\"true'"),
676+
_ => panic!("expected UserSet"),
677+
}
678+
assert!(matches!(
679+
plan_workspace_trust(Some("trustLockfile: 'true'\n")),
680+
TrustPlan::AlreadyTrue
681+
));
682+
}
683+
665684
/// #904 (hosted): a BOM-prefixed `trustLockfile:` first line is the
666685
/// user's explicit setting — kept, never shadowed by an appended
667686
/// duplicate key — and a BOM file that needs the key keeps its BOM.

‎crates/socket-patch-core/src/patch/redirect/upstream/npm.rs‎

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -559,7 +559,7 @@ fn yaml_path_in(lines: &[String], path: &[&str]) -> Result<Option<String>, Strin
559559
return Ok(None);
560560
}
561561
if rest.is_empty() {
562-
let value = value.trim_matches(['"', '\'']).to_string();
562+
let value = crate::formats::pnpm::workspace::yaml_scalar(&value);
563563
return Ok((!value.is_empty()).then_some(value));
564564
}
565565
if !value.is_empty() {
@@ -1314,7 +1314,7 @@ fn yaml_block_value(text: &str, section: &str, key: &str) -> Option<String> {
13141314
})
13151315
.filter_map(top_level_key)
13161316
.rfind(|(k, _)| k == key)
1317-
.map(|(_, value)| value.trim_matches(['"', '\'']).to_string())
1317+
.map(|(_, value)| crate::formats::pnpm::workspace::yaml_scalar(value))
13181318
.filter(|value| !value.is_empty())
13191319
}
13201320

@@ -1549,10 +1549,7 @@ fn pnpm_lock_tarball_evidence(
15491549
/// (`.modules.yaml`'s `packageManager`, package.json's corepack pin,
15501550
/// `+sha…` suffix allowed).
15511551
fn pnpm_spec_version(spec: &str) -> Option<PnpmVersion> {
1552-
let version = spec
1553-
.trim()
1554-
.trim_matches(['"', '\''])
1555-
.strip_prefix("pnpm@")?;
1552+
let version = spec.trim().strip_prefix("pnpm@")?;
15561553
pnpm_version(version)
15571554
}
15581555

@@ -4459,4 +4456,28 @@ mod tests {
44594456
"common/config/subspaces/x/"
44604457
);
44614458
}
4459+
4460+
/// #1918: nested pnpm/yarn settings values decode YAML quoting through
4461+
/// the same decoder as the top-level reader.
4462+
#[test]
4463+
fn nested_settings_values_decode_yaml_quoting() {
4464+
assert_eq!(
4465+
super::yaml_block_value(
4466+
"registries:\n default: 'https://r/it''s/'\n",
4467+
"registries",
4468+
"default"
4469+
)
4470+
.as_deref(),
4471+
Some("https://r/it's/")
4472+
);
4473+
assert_eq!(
4474+
super::yaml_path_value(
4475+
"npmScopes:\n s:\n npmRegistryServer: \"https://r/\\u0041\"\n",
4476+
&["npmScopes", "s", "npmRegistryServer"]
4477+
)
4478+
.unwrap()
4479+
.as_deref(),
4480+
Some("https://r/A")
4481+
);
4482+
}
44624483
}

0 commit comments

Comments
 (0)