diff --git a/crates/socket-patch-cli/tests/e2e_nuget_dotnet_build.rs b/crates/socket-patch-cli/tests/e2e_nuget_dotnet_build.rs index 98ef5543d..3238fdd3c 100644 --- a/crates/socket-patch-cli/tests/e2e_nuget_dotnet_build.rs +++ b/crates/socket-patch-cli/tests/e2e_nuget_dotnet_build.rs @@ -752,6 +752,19 @@ fn nuget_hosted_dotnet_restore_then_manifestless_vex() { let store_fx = sb.dir("store-fixture"); let registry = restore_fixture(&dn, &sb, &fixture, &store_fx); + // Regression #561/#585: templates often keep inactive sources/mappings. + // A forward rewrite must agree with NuGet and VEX about which XML is live. + let inactive = ""; + std::fs::write( + fixture.join("nuget.config"), + registry.0.replacen( + "", + &format!("\n {inactive}"), + 1, + ), + ) + .unwrap(); + let pristine = std::fs::read(pkg_dir(&store_fx).join(FILE_KEY)).unwrap(); let mut patched = pristine.clone(); patched.extend_from_slice(MARKER); @@ -794,6 +807,10 @@ fn nuget_hosted_dotnet_restore_then_manifestless_vex() { let doc: Value = serde_json::from_slice(&std::fs::read(&embedded).unwrap()).unwrap(); assert_attested(&doc, PURL, HOSTED_UUID, Marker::Redirected, &vulns()); let config = std::fs::read_to_string(fixture.join("nuget.config")).unwrap(); + assert!( + config.contains(inactive), + "inactive XML left byte-exact: {config}" + ); assert!( config.contains(&format!("socket-patch-{HOSTED_UUID}")) && config.contains(&format!("pattern=\"{ID}\"")), diff --git a/crates/socket-patch-core/src/formats/nuget/mod.rs b/crates/socket-patch-core/src/formats/nuget/mod.rs index 294b4ca7e..fdf41875c 100644 --- a/crates/socket-patch-core/src/formats/nuget/mod.rs +++ b/crates/socket-patch-core/src/formats/nuget/mod.rs @@ -11,6 +11,7 @@ //! attribute or a mismatched close tag makes the whole file `None`. use std::collections::BTreeSet; +use std::ops::Range; // ── pure reader ── @@ -28,6 +29,47 @@ pub(crate) struct NugetConfig { pub(crate) mappings: Vec<(String, Vec)>, /// Keys `configuration/disabledPackageSources` turns off. pub(crate) disabled: BTreeSet, + /// Live XML locations for writers. Routing readers and writers share + /// the same treatment of comments, quoted attributes and element scope. + pub(crate) configuration: Option, + pub(crate) package_sources: Option, + pub(crate) source_mapping: Option, + /// Preserve the routing reader's behavior on repeated sections, but do + /// not let a writer guess which occurrence should receive an edit. + pub(crate) repeated_sections: bool, +} + +#[derive(Debug)] +pub(crate) struct ConfigSection { + pub(crate) open: Range, + /// `None` for a self-closing element (unclosed XML fails parsing). + pub(crate) close_start: Option, + /// After the last direct `` child, or the opening tag otherwise. + pub(crate) insert_at: usize, +} + +fn section_mut<'a>( + cfg: &'a mut NugetConfig, + parents: &[&str], + name: &str, +) -> Option<&'a mut Option> { + match (parents, name) { + ([], "configuration") => Some(&mut cfg.configuration), + (["configuration"], "packageSources") => Some(&mut cfg.package_sources), + (["configuration"], "packageSourceMapping") => Some(&mut cfg.source_mapping), + _ => None, + } +} + +fn record_clear(cfg: &mut NugetConfig, parents: &[&str], end: usize) { + let section = match parents { + ["configuration", "packageSources"] => cfg.package_sources.as_mut(), + ["configuration", "packageSourceMapping"] => cfg.source_mapping.as_mut(), + _ => None, + }; + if let Some(section) = section { + section.insert_at = end; + } } /// One open (or self-closing) tag. @@ -68,13 +110,33 @@ pub(crate) fn parse_config(text: &str) -> Option { i = at + rest.find('>')? + 1; } else if let Some(close) = rest.strip_prefix("')?; - if stack.pop()? != close[..end].trim() { + let name = stack.pop()?; + if name != close[..end].trim() { return None; } i = at + 2 + end + 1; + if let Some(Some(section)) = section_mut(&mut cfg, &stack, name) { + section.close_start = Some(at); + } + if name == "clear" { + record_clear(&mut cfg, &stack, i); + } } else { let (tag, consumed) = parse_open_tag(&rest[1..])?; i = at + 1 + consumed; + if let Some(slot) = section_mut(&mut cfg, &stack, tag.name) { + let repeated = slot + .replace(ConfigSection { + open: at..i, + close_start: None, + insert_at: i, + }) + .is_some(); + cfg.repeated_sections |= repeated; + } + if tag.name == "clear" && tag.self_closing { + record_clear(&mut cfg, &stack, i); + } visit(&stack, &tag, &mut cfg, &mut open_mapping); if !tag.self_closing { if stack.len() >= MAX_XML_DEPTH { @@ -170,7 +232,14 @@ fn parse_open_tag(s: &str) -> Option<(Tag<'_>, usize)> { if raw.contains('<') { return None; } - attrs.push((attr, decode_entities(raw))); + // XML first normalizes literal CRLF to one line break, then literal + // attribute whitespace to spaces. Character references preserve their + // referenced whitespace, so normalize before decoding entities. + let normalized = raw + .bytes() + .any(|b| matches!(b, b'\t' | b'\r' | b'\n')) + .then(|| raw.replace("\r\n", "\n").replace(['\t', '\r', '\n'], " ")); + attrs.push((attr, decode_entities(normalized.as_deref().unwrap_or(raw)))); j += 1 + close + 1; } } diff --git a/crates/socket-patch-core/src/patch/redirect/mod.rs b/crates/socket-patch-core/src/patch/redirect/mod.rs index c5b565053..5b5e2d1d1 100644 --- a/crates/socket-patch-core/src/patch/redirect/mod.rs +++ b/crates/socket-patch-core/src/patch/redirect/mod.rs @@ -4755,224 +4755,111 @@ const NUGET_ORG_URL: &str = "https://api.nuget.org/v3/index.json"; /// caller must skip the dep fail-closed — writing the mapping without its /// source (or recording the edit at all) routes the patched id to a source /// that was never defined while the ledger claims the redirect landed. -fn add_nuget_source(config: &str, reg: &str, index_url: &str, pkg_id: &str) -> Option { - // Capture the pre-existing packageSource keys BEFORE the Socket source is - // added — the fallback below fans a `*` mapping out to them. - let mut pre_existing_keys = nuget_package_source_keys(config); - let mut out = config.to_string(); - - // A from-scratch is EXCLUSIVE: once it exists, every - // package must match some source's `*`/pattern or restore fails NU1100. If - // there are NO pre-existing sources to fan `*` out to, the mapping would be - // socket-only and every other package would fail. Seed the implicit default - // nuget.org source so the catch-all has a real target (unless the config - // already has one). Only relevant when we are about to CREATE the mapping. - // The open tag may carry whitespace or attributes (`` - // is valid XML); a literal probe reads it as absent and authors a - // DUPLICATE section. - let creating_mapping = nuget_mapping_open_end(&out).is_none(); - // "Already has one" is decided by the parsed keys ALONE: - // a whole-file "nuget.org" probe is satisfied by text that defines no - // source (a defaultPushSource URL, a entry, a - // comment), and suppressing the seed on it leaves the from-scratch - // mapping socket-only — NU1100 for every other package. +fn add_nuget_source( + config: &str, + parsed: &crate::formats::nuget::NugetConfig, + reg: &str, + index_url: &str, + pkg_id: &str, +) -> Option { + // The same source identities restore and VEX read, before Socket is added. + let mut pre_existing_keys: Vec<&str> = + parsed.sources.iter().map(|(key, _)| key.as_str()).collect(); + let creating_mapping = parsed + .source_mapping + .as_ref() + .is_none_or(|section| section.close_start.is_none()); let seed_nuget_org = creating_mapping && pre_existing_keys.is_empty(); + let reg = nuget_xml_attribute(reg); + let mut source_lines = format!( + " ", + nuget_xml_attribute(index_url) + ); if seed_nuget_org { - out = insert_nuget_source(&out, NUGET_ORG_KEY, NUGET_ORG_URL)?; - pre_existing_keys.push(NUGET_ORG_KEY.to_string()); + // Once a mapping exists, every package needs a matching source. Keep + // the implicit nuget.org fallback when this file defines none. + source_lines.push_str(&format!( + "\n " + )); + pre_existing_keys.push(NUGET_ORG_KEY); } + let out = if let Some(section) = &parsed.package_sources { + insert_nuget_children(config, section, "packageSources", &source_lines) + } else { + insert_nuget_children( + config, + parsed.configuration.as_ref()?, + "configuration", + &format!(" \n{source_lines}\n "), + ) + }; - out = insert_nuget_source(&out, reg, index_url)?; - + // Source insertion shifted the mapping's byte offsets. Re-read through + // the shared tokenizer, never a second grammar over the modified text. + let updated = crate::formats::nuget::parse_config(&out)?; let socket_mapping = format!( - " \n \n " + " \n \n ", + nuget_xml_attribute(pkg_id), ); - if !creating_mapping { - // A mapping already exists (e.g. a prior patched dep, or the project's - // own): append ONLY this source's mapping — every other source is - // already covered. - // After any `` in the section: NuGet drops every mapping - // read before one, leaving the patched id routed nowhere. - let open_end = nuget_mapping_open_end(&out)?; - let at = nuget_after_last_clear(&out, open_end, "packageSourceMapping"); - out = format!("{}\n{socket_mapping}{}", &out[..at], &out[at..]); - } else { - // Creating the mapping from scratch. Once ANY - // exists, NuGet requires EVERY package to match some source's pattern, - // so a mapping that routed only the patched id to the Socket source - // would make every OTHER package fail restore with NU1100. Fan a - // `` out to each pre-existing source (which now - // includes the seeded nuget.org when the config had none) so the rest - // of the restore keeps resolving exactly where it did before. - let fallback_mappings = pre_existing_keys - .iter() - .map(|key| { - format!( - " \n \n " - ) - }) - .collect::>() - .join("\n"); - let inner = if fallback_mappings.is_empty() { - socket_mapping - } else { - format!("{socket_mapping}\n{fallback_mappings}") - }; - let map_block = format!(" \n{inner}\n "); - // The close tag may carry whitespace (`` is valid - // XML); a literal replacen would silently drop the mapping. - let close_re = Regex::new(r"") - .expect("static configuration close-tag regex is valid"); - let m = close_re.find(&out)?; - let at = m.start(); - out = format!("{}{map_block}\n{}", &out[..at], &out[at..]); + let mut inner = socket_mapping; + if creating_mapping { + for key in pre_existing_keys { + inner.push_str(&format!( + "\n \n \n ", + nuget_xml_attribute(key), + )); + } } - Some(out) -} - -/// Insert an `` source under ``, -/// creating the element (right after the `` root open tag, -/// whatever whitespace or attributes it carries) when absent. A self-closing -/// `` (any whitespace before `/>`) is expanded in place -/// into an open/close pair rather than left dangling beside a duplicate -/// element. `None` when no anchor exists at all — the caller must treat the -/// insert as failed rather than proceed on unchanged text. -fn insert_nuget_source(config: &str, key: &str, url: &str) -> Option { - let source_line = format!(" "); - // A self-closing element carries no children, so expand it to an open/close - // pair holding the new source. Matched before the open-tag check because - // the tolerant open-tag regex below also matches the whitespace-carrying - // `` form, and inserting after its `>` would land the - // source OUTSIDE the element. - let self_closing = Regex::new(r"") - .expect("static self-closing packageSources regex is valid"); - // The open tag may carry whitespace (`` is valid XML - // NuGet parses); a literal `` probe reads it as absent - // and the from-scratch branch below authors a DUPLICATE element — the - // vendor/nuget_feed twin already tolerates the spelling. - let open_tag = Regex::new(r"]*)?>") - .expect("static packageSources open-tag regex is valid"); - if let Some(m) = self_closing.find(config) { - let mut out = String::with_capacity(config.len() + source_line.len() + 40); - out.push_str(&config[..m.start()]); - out.push_str(&format!( - "\n{source_line}\n " - )); - out.push_str(&config[m.end()..]); - Some(out) - } else if let Some(m) = open_tag - .find(config) - // An attribute-carrying self-closing form (``, - // schema-invalid but cheap to guard) has no children span: fall - // through to the from-scratch branch rather than insert outside it. - .filter(|m| !m.as_str().ends_with("/>")) - { - // After any ``: NuGet drops every source read before one, - // so the mapping would point at an undefined source (NU1100). - let end = nuget_after_last_clear(config, m.end(), "packageSources"); - Some(format!( - "{}\n{source_line}{}", - &config[..end], - &config[end..] + if let Some(section) = &updated.source_mapping { + Some(insert_nuget_children( + &out, + section, + "packageSourceMapping", + &inner, )) } else { - // The root open tag may carry whitespace or attributes - // (``, ``) — all valid XML a - // literal `` match would silently miss, leaving the - // source undefined while the mapping still lands. - let open_re = Regex::new(r"]*)?>") - .expect("static configuration open-tag regex is valid"); - let end = open_re.find(config)?.end(); + let at = updated.configuration.as_ref()?.close_start?; Some(format!( - "{}\n \n{source_line}\n {}", - &config[..end], - &config[end..] + "{} \n{inner}\n \n{}", + &out[..at], + &out[at..] )) } } -/// The offset just past the `` open tag (any whitespace -/// or attributes), or `None` when the config has no open/close section — a -/// self-closing `` holds no children to append to. -fn nuget_mapping_open_end(config: &str) -> Option { - static OPEN_RE: LazyLock = LazyLock::new(|| { - Regex::new(r"]*)?>") - .expect("static packageSourceMapping open-tag regex is valid") - }); - OPEN_RE - .find(config) - .filter(|m| !m.as_str().ends_with("/>")) - .map(|m| m.end()) -} - -/// The offset just past the last `` between `from` and the `section` -/// element's close tag (any whitespace before `>`), else `from`. Comments are -/// skipped: a commented-out `` clears nothing, and anchoring on it -/// would splice the new entry INSIDE the comment. -fn nuget_after_last_clear(config: &str, from: usize, section: &str) -> usize { - static CLEAR_RE: LazyLock = - LazyLock::new(|| Regex::new(r"").expect("static clear-tag regex is valid")); - static COMMENT_RE: LazyLock = - LazyLock::new(|| Regex::new(r"(?s)").expect("static comment regex is valid")); - // Blank comment bytes in place so offsets still index `config`. - let mut masked = config.as_bytes()[from..].to_vec(); - for m in COMMENT_RE.find_iter(&config[from..]) { - masked[m.range()].fill(b' '); - } - let masked = String::from_utf8(masked).expect("only whole comments are blanked"); - let close_re = - Regex::new(&format!(r"")).expect("section close-tag regex is valid"); - let Some(close) = close_re.find(&masked) else { - return from; - }; - CLEAR_RE - .find_iter(&masked[..close.start()]) - .last() - .map_or(from, |m| from + m.end()) +/// Insert only at live, directly-scoped elements recorded by the XML reader. +/// The model's insertion point follows the last `` child. Expanding a +/// self-closing section keeps its opening attributes and all surrounding bytes. +fn insert_nuget_children( + config: &str, + section: &crate::formats::nuget::ConfigSection, + name: &str, + children: &str, +) -> String { + let mut out = config.to_string(); + if section.close_start.is_some() { + out.insert_str(section.insert_at, &format!("\n{children}")); + } else { + let open = config[section.open.start..section.open.end - 2].trim_end(); + out.replace_range( + section.open.clone(), + &format!("{open}>\n{children}\n "), + ); + } + out } -/// The `key` of every `` under `` (empty when there -/// is no such element). Used to preserve resolution for non-patched packages -/// when a `` is introduced. -// The open tag may carry whitespace (`` is valid XML NuGet -// parses); a literal match reads a real source list as "no sources" — -// duplicate nuget.org seed, missed catch-all fan-out — while the -// vendor/nuget_feed twin already tolerates the spelling. A self-closing -// `` has no close tag, so the regex (correctly) finds no -// children span. -static NUGET_PACKAGE_SOURCES_REGION_RE: LazyLock = LazyLock::new(|| { - Regex::new(r"(?s)]*)?>(.*?)") - .expect("static packageSources region regex is valid") -}); -// Tolerates any attribute order, whitespace around `=`, and single-quoted -// values (all valid XML NuGet accepts): a real source the scan misses would -// read as "no sources", triggering a duplicate nuget.org seed and leaving the -// missed source out of the catch-all fan-out. `[^>]` keeps the match inside -// one element. -static NUGET_ADD_KEY_RE: LazyLock = LazyLock::new(|| { - Regex::new(r#"]*?key\s*=\s*(?:"([^"]+)"|'([^']+)')"#) - .expect("static add-key regex is valid") -}); - -fn nuget_package_source_keys(config: &str) -> Vec { - let scope = NUGET_PACKAGE_SOURCES_REGION_RE - .captures(config) - .map(|c| { - c.get(1) - .expect("region_re always captures group 1") - .as_str() - }) - .unwrap_or(""); - NUGET_ADD_KEY_RE - .captures_iter(scope) - .map(|c| { - c.get(1) - .or_else(|| c.get(2)) - .expect("one quote alternative always captures") - .as_str() - .to_string() - }) - .collect() +/// The reader returns decoded attribute values; encode them when writing so +/// a source named `a&b` still has the same identity in its mapping. +fn nuget_xml_attribute(value: &str) -> String { + value + .replace('&', "&") + .replace('"', """) + .replace('<', "<") + // Literal XML attribute whitespace would be normalized to spaces. + .replace('\t', " ") + .replace('\n', " ") + .replace('\r', " ") } fn rewrite_nuget( @@ -5051,28 +4938,35 @@ fn rewrite_nuget( .clone() .unwrap_or_else(|| dep.name.to_lowercase()); - // Idempotency probe over the parsed `` keys — the - // same reader `add_nuget_source` fans the catch-all out with — so a - // hand-normalized spelling (`key = 'socket-patch-…'`) is recognized - // as already wired instead of being duplicated on a re-run. - if !nuget_package_source_keys(&config) - .iter() - .any(|key| key == ®) - { + let unwritable = || RewriteWarning { + code: "redirect_nuget_config_unwritable".into(), + detail: format!( + "nuget.config has malformed XML or no unambiguous layout \ + to wire {} into; not redirected", + dep.name + ), + }; + // Validate even an apparently existing Socket source before re-pinning + // the lock. A partial parse must never turn into a claimed redirect. + let Some(parsed) = crate::formats::nuget::parse_config(&config).filter(|parsed| { + !parsed.repeated_sections + && parsed + .configuration + .as_ref() + .is_some_and(|root| root.close_start.is_some()) + }) else { + result.warnings.push(unwritable()); + continue; + }; + if !parsed.sources.iter().any(|(key, _)| key == ®) { // A failed insert skips the WHOLE dep (no edit record, no lock // re-pin): a mapping without its source routes the patched id to // a source that was never defined, and a lock pinned at the // patched contentHash over an upstream fetch fails NU1403 — both // while the ledger would claim the redirect landed. - let Some(updated) = add_nuget_source(&config, ®, &ov.index_url, &dep.name) else { - result.warnings.push(RewriteWarning { - code: "redirect_nuget_config_unwritable".into(), - detail: format!( - "nuget.config has no element to wire {} into; \ - not redirected", - dep.name - ), - }); + let Some(updated) = add_nuget_source(&config, &parsed, ®, &ov.index_url, &dep.name) + else { + result.warnings.push(unwritable()); continue; }; config = updated; @@ -7621,6 +7515,155 @@ mod tests { } } + #[test] + fn nuget_shared_model_ignores_commented_sources() { + for sources in [ + r#""#, + r#" + + + "#, + ] { + let config = format!("\n {sources}\n\n"); + let files = BTreeMap::from([("nuget.config".into(), config)]); + let result = rewrite_registry_redirect(&files, &[nuget_override()]); + let out = result.files.get("nuget.config").expect("config rewritten"); + let parsed = crate::formats::nuget::parse_config(out).unwrap(); + assert_eq!( + parsed + .sources + .iter() + .map(|(key, _)| key.as_str()) + .collect::>(), + ["socket-patch-uuid", "nuget.org"], + "only live sources receive mappings: {out}" + ); + assert_eq!( + parsed.mappings, + [ + ("socket-patch-uuid".into(), vec!["Newtonsoft.Json".into()]), + ("nuget.org".into(), vec!["*".into()]), + ], + "every catch-all must name a source NuGet reads: {out}" + ); + } + } + + #[test] + fn nuget_shared_model_never_splices_into_inert_markup() { + let fake = ""; + for inert in [ + format!(""), + format!(""), + format!(""), + ] { + let config = format!( + "\n {inert}\n \n \ + \n \ + \n\n" + ); + let files = BTreeMap::from([("nuget.config".into(), config)]); + let result = rewrite_registry_redirect(&files, &[nuget_override()]); + let out = result.files.get("nuget.config").expect("config rewritten"); + assert!(out.contains(&inert), "inert bytes must be preserved: {out}"); + let parsed = crate::formats::nuget::parse_config(out).unwrap(); + assert!(parsed + .sources + .iter() + .any(|(key, _)| key == "socket-patch-uuid")); + assert!(parsed.mappings.iter().any(|(key, patterns)| { + key == "socket-patch-uuid" && patterns == &["Newtonsoft.Json"] + })); + let rerun = rewrite_registry_redirect(&result.files, &[nuget_override()]); + assert!( + rerun.files.is_empty() && rerun.edits.is_empty(), + "idempotent: {rerun:?}" + ); + } + } + + #[test] + fn nuget_shared_model_refuses_malformed_config_without_repinning_lock() { + for config in [ + "", + " + + +"#; + let files = BTreeMap::from([("nuget.config".into(), config.into())]); + let result = rewrite_registry_redirect(&files, &[nuget_override()]); + let out = result.files.get("nuget.config").expect("config rewritten"); + let parsed = crate::formats::nuget::parse_config(out).unwrap(); + assert!(!parsed.repeated_sections, "reuse the empty mapping: {out}"); + assert_eq!(parsed.sources[1].0, "a&b\"<"); + assert_eq!(parsed.mappings[1], ("a&b\"<".into(), vec!["*".into()])); + assert!(out.contains("\n -->")); + assert!(out.starts_with("\">")); + } + /// Creating a `` from scratch: once ANY mapping /// exists NuGet requires EVERY package to match some source's pattern, so /// the rewriter must fan a `pattern="*"` mapping out to every pre-existing diff --git a/crates/socket-patch-core/src/patch/redirect/upstream/nuget.rs b/crates/socket-patch-core/src/patch/redirect/upstream/nuget.rs index 3531f7d0e..da2403f40 100644 --- a/crates/socket-patch-core/src/patch/redirect/upstream/nuget.rs +++ b/crates/socket-patch-core/src/patch/redirect/upstream/nuget.rs @@ -440,6 +440,7 @@ mod tests { fn hosted_config(config: &str) -> String { super::super::super::add_nuget_source( config, + &parse_config(config).unwrap(), &format!("socket-patch-{UUID}"), &index_url(), "Newtonsoft.Json",