diff --git a/src/parsers/ivy.rs b/src/parsers/ivy.rs index db62e99db..5161659d2 100644 --- a/src/parsers/ivy.rs +++ b/src/parsers/ivy.rs @@ -196,11 +196,7 @@ fn interpret_ivy_xml(content: &str, path: &Path) -> PackageData { package_data.dependencies = dependencies; if let (Some(namespace), Some(name)) = (&package_data.namespace, &package_data.name) { - package_data.purl = Some(truncate_field(build_ivy_purl( - namespace, - name, - package_data.version.as_deref(), - ))); + package_data.purl = build_ivy_purl(namespace, name, package_data.version.as_deref()); } package_data @@ -401,13 +397,15 @@ fn parse_ivy_dependency(element: &BytesStart) -> Option { let rev = attr_value(element, b"rev"); let conf = attr_value(element, b"conf"); + // An ivy dependency without an organisation has no namespace to place, so it + // falls back to a name-only PURL — still through the encoder. let purl = match &org { Some(org) => build_ivy_purl(org, &name, None), - None => format!("pkg:ivy/{}", name), + None => crate::parsers::utils::simple_purl("ivy", &name, None), }; Some(Dependency { - purl: Some(truncate_field(purl)), + purl, extracted_requirement: rev.map(truncate_field), scope: conf.map(truncate_field), is_runtime: None, @@ -419,11 +417,12 @@ fn parse_ivy_dependency(element: &BytesStart) -> Option { }) } -fn build_ivy_purl(organisation: &str, module: &str, revision: Option<&str>) -> String { - let mut purl = format!("pkg:ivy/{}/{}", organisation, module); - if let Some(revision) = revision.filter(|value| !value.trim().is_empty()) { - purl.push('@'); - purl.push_str(revision); - } - purl +/// Builds an ivy PURL through the encoder. +/// +/// `organisation` and `module` come straight from XML attributes, so they can +/// hold anything. Hand-formatting them produced strings that silently lost data +/// on re-parse: a space in either component truncated the PURL, taking the +/// revision with it. +fn build_ivy_purl(organisation: &str, module: &str, revision: Option<&str>) -> Option { + crate::parsers::utils::namespaced_purl("ivy", organisation, module, revision) } diff --git a/src/parsers/ivy_test.rs b/src/parsers/ivy_test.rs index 53d0dd7c8..edcd3188f 100644 --- a/src/parsers/ivy_test.rs +++ b/src/parsers/ivy_test.rs @@ -253,3 +253,55 @@ fn test_xml_entities_are_decoded() { assert_eq!(pkg.namespace.as_deref(), Some("org.example&co")); assert_eq!(pkg.name.as_deref(), Some("a&b")); } + +#[test] +fn test_ivy_purls_encode_components_and_keep_the_revision() { + use std::io::Write; + use std::str::FromStr; + + // `organisation`, `module` and `rev` are raw XML attributes. Hand-formatting + // them lost data on re-parse: a space truncated the PURL and took the + // revision with it, so the module silently lost its version. + let dir = std::env::temp_dir().join(format!( + "provenant-ivy-{}", + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("system time should be after epoch") + .as_nanos() + )); + std::fs::create_dir_all(&dir).expect("create temp dir"); + let path = dir.join("ivy.xml"); + let mut file = std::fs::File::create(&path).expect("create ivy.xml"); + write!( + file, + r#" + + + + +"# + ) + .expect("write ivy.xml"); + + let package_data = IvyXmlParser::extract_first_package(&path); + + let purl = package_data.purl.as_deref().expect("package purl"); + assert_eq!(purl, "pkg:ivy/org%20with%20space/mod%3Fx@1.0"); + let parsed = packageurl::PackageUrl::from_str(purl).expect("purl should parse"); + assert_eq!(parsed.namespace(), Some("org with space")); + assert_eq!(parsed.name(), "mod?x"); + // The revision is the part that used to disappear. + assert_eq!(parsed.version(), Some("1.0")); + assert_eq!(parsed.to_string(), purl); + + let dependency = package_data + .dependencies + .first() + .expect("one dependency expected"); + assert_eq!( + dependency.purl.as_deref(), + Some("pkg:ivy/dep%20org/dep%3Fname") + ); + + std::fs::remove_dir_all(&dir).ok(); +} diff --git a/src/parsers/maven/manifest.rs b/src/parsers/maven/manifest.rs index 878a93940..971b2f2d1 100644 --- a/src/parsers/maven/manifest.rs +++ b/src/parsers/maven/manifest.rs @@ -141,7 +141,7 @@ pub(crate) fn interpret_manifest_mf(content: &str, path: &Path) -> PackageData { package_data.version = bundle_version.clone(); if let (Some(name), Some(version)) = (&package_data.name, &package_data.version) { - package_data.purl = Some(format!("pkg:osgi/{}@{}", name, version)); + package_data.purl = crate::parsers::utils::simple_purl("osgi", name, Some(version)); } } @@ -291,7 +291,7 @@ pub(super) fn parse_osgi_package_list(package_list: &str, scope: &str) -> Vec Vec = package_data + .dependencies + .iter() + .filter_map(|dependency| dependency.purl.as_deref()) + .collect(); + assert!(purls.contains(&"pkg:osgi/com.foo%20bar"), "got {purls:?}"); + // `#` would otherwise become a subpath rather than part of the name. + assert!( + purls.contains(&"pkg:osgi/req%20bundle%23z"), + "got {purls:?}" + ); + for purl in &purls { + let parsed = packageurl::PackageUrl::from_str(purl).expect("purl should parse"); + assert_eq!(parsed.subpath(), None); + assert_eq!(parsed.to_string(), *purl); + } +} diff --git a/src/parsers/opam.rs b/src/parsers/opam.rs index 8cab1d853..bcc57c9f1 100644 --- a/src/parsers/opam.rs +++ b/src/parsers/opam.rs @@ -658,7 +658,7 @@ fn extract_parties(authors: &[String], maintainers: &[String]) -> Vec { fn extract_dependencies(deps: &[(String, String)]) -> Vec { deps.iter() .map(|(name, version_constraint)| Dependency { - purl: crate::parsers::utils::simple_purl("opam", name, None).map(truncate_field), + purl: crate::parsers::utils::simple_purl("opam", name, None), extracted_requirement: Some(truncate_field(version_constraint.clone())), scope: Some("dependency".to_string()), is_runtime: Some(true), diff --git a/src/parsers/utils.rs b/src/parsers/utils.rs index f025dfa41..baebb4aa7 100644 --- a/src/parsers/utils.rs +++ b/src/parsers/utils.rs @@ -307,18 +307,25 @@ pub fn url_authority_host(authority: &str) -> &str { /// when the components cannot form a PURL, which is the honest outcome: the /// declared text is still reported in the fields that carry it. /// +/// Components are bounded before construction, never after: truncating an +/// assembled PURL can cut a percent escape in half or drop a trailing component, +/// turning a valid PURL into one that no longer parses. Callers must not apply +/// `truncate_field` to the result. +/// /// Only for types the crate does not rewrite. It lowercases names for /// `bitbucket`, `deb`, `github`, `hex`, `npm` and `pypi`, so those need a /// deliberate decision about case rather than this helper. pub fn simple_purl(package_type: &str, name: &str, version: Option<&str>) -> Option { - let name = name.trim(); + let name = truncate_field(name.trim().to_string()); if name.is_empty() { return None; } - let mut package_url = PackageUrl::new(package_type.to_string(), name.to_string()).ok()?; + let mut package_url = PackageUrl::new(package_type.to_string(), name).ok()?; if let Some(version) = version.map(str::trim).filter(|value| !value.is_empty()) { - package_url.with_version(version.to_string()).ok()?; + package_url + .with_version(truncate_field(version.to_string())) + .ok()?; } Some(package_url.to_string()) } @@ -336,15 +343,18 @@ pub fn namespaced_purl( name: &str, version: Option<&str>, ) -> Option { - let (namespace, name) = (namespace.trim(), name.trim()); + let namespace = truncate_field(namespace.trim().to_string()); + let name = truncate_field(name.trim().to_string()); if namespace.is_empty() || name.is_empty() { return None; } - let mut package_url = PackageUrl::new(package_type.to_string(), name.to_string()).ok()?; - package_url.with_namespace(namespace.to_string()).ok()?; + let mut package_url = PackageUrl::new(package_type.to_string(), name).ok()?; + package_url.with_namespace(namespace).ok()?; if let Some(version) = version.map(str::trim).filter(|value| !value.is_empty()) { - package_url.with_version(version.to_string()).ok()?; + package_url + .with_version(truncate_field(version.to_string())) + .ok()?; } Some(package_url.to_string()) }