From 7dc07513b18ec3c662e67b9e9d2be666d5d4a3f4 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 00:16:24 -0400 Subject: [PATCH 01/24] feat(core): read Package.resolved v2 and v3 SwiftPM's Package.resolved is plain JSON carrying the full flattened pin set: identity, location URL, revision, and version for every resolved package. Read v2 (Swift 5.6+) and v3 (Xcode 15+), whose pin shapes are identical, plus v1's differently-spelled one, because reporting a Swift 5.5 project as depending on nothing is the silent wrong answer. The pin set is the only record of a Swift project's dependencies - Package.swift is executable Swift and cannot be read as text honestly - so this lockfile is the source of the dependency list rather than an annotation on one. LockfileKind::is_dependency_source and lockfile_items are that path; apply_lockfile's insert-nothing contract is untouched for the five formats that already have it. Names are normalized to the URL OSV keys SwiftURL advisories by: scheme and .git suffix removed. Either left on matches nothing and reports a vulnerable package as clean. --- crates/dependable-core/src/lib.rs | 3 +- crates/dependable-core/src/lockfiles/mod.rs | 27 ++ .../src/lockfiles/swift_package_resolved.rs | 410 ++++++++++++++++++ crates/dependable-core/src/manifest.rs | 26 ++ 4 files changed, 465 insertions(+), 1 deletion(-) create mode 100644 crates/dependable-core/src/lockfiles/swift_package_resolved.rs diff --git a/crates/dependable-core/src/lib.rs b/crates/dependable-core/src/lib.rs index 450c96c..ba3141e 100644 --- a/crates/dependable-core/src/lib.rs +++ b/crates/dependable-core/src/lib.rs @@ -23,10 +23,11 @@ pub use graph::{ }; pub use item::{DependencyKind, Item, PackageSource}; pub use lockfiles::{ - LockedPackage, LockfileData, ResolvedLockfile, apply_lockfile, parse_bun_lock, + LockedPackage, LockfileData, ResolvedLockfile, apply_lockfile, lockfile_items, parse_bun_lock, parse_bun_lock_graph, parse_cargo_lock, parse_cargo_lock_graph, parse_composer_lock, parse_composer_lock_graph, parse_dart_pubspec_lock, parse_lockfile, parse_lockfile_kind, parse_mix_lock, parse_mix_lock_graph, parse_package_lock, parse_package_lock_graph, + parse_swift_package_resolved, swift_package_name, swift_package_resolved_items, }; pub use manifest::{ AlternateRegistryDecl, LockfileKind, ManifestKind, ParsedManifest, UNREADABLE_MANIFESTS, diff --git a/crates/dependable-core/src/lockfiles/mod.rs b/crates/dependable-core/src/lockfiles/mod.rs index b18d73d..c35e056 100644 --- a/crates/dependable-core/src/lockfiles/mod.rs +++ b/crates/dependable-core/src/lockfiles/mod.rs @@ -1,6 +1,7 @@ //! Lockfile parsers and per-kind dispatch. use crate::error::ParseError; +use crate::item::Item; use crate::manifest::{LockfileKind, ManifestKind}; pub mod bun_lock; @@ -14,6 +15,7 @@ pub mod mix_lock; pub mod mix_lock_graph; pub mod package_lock_graph; pub mod package_lock_json; +pub mod swift_package_resolved; pub use bun_lock::parse_bun_lock; pub use bun_lock_graph::parse_bun_lock_graph; @@ -26,6 +28,9 @@ pub use mix_lock::parse_mix_lock; pub use mix_lock_graph::parse_mix_lock_graph; pub use package_lock_graph::parse_package_lock_graph; pub use package_lock_json::parse_package_lock; +pub use swift_package_resolved::{ + parse_swift_package_resolved, swift_package_name, swift_package_resolved_items, +}; /// Parse lockfile `content` with the parser for the file that was found. /// @@ -44,6 +49,28 @@ pub fn parse_lockfile_kind(kind: LockfileKind, content: &str) -> Result parse_composer_lock(content), LockfileKind::PubspecLock => parse_dart_pubspec_lock(content), LockfileKind::MixLock => parse_mix_lock(content), + LockfileKind::PackageResolved => parse_swift_package_resolved(content), + } +} + +/// The dependency list a lockfile *is*, for the formats that are the only record +/// of one. +/// +/// [`apply_lockfile`] annotates items a manifest already produced and never +/// inserts, which is the right contract wherever the manifest is readable data. +/// Swift's is not — `Package.swift` is executable Swift — so `Package.resolved` +/// is the only honest source of the dependency list, and a caller has to be able +/// to take items *from* a lockfile rather than only apply one *to* them. +/// +/// `None` for every other kind, whose contract is unchanged: ask +/// [`parse_lockfile_kind`] for their versions and apply them. +/// [`LockfileKind::is_dependency_source`] answers the same question without +/// parsing. +#[must_use] +pub fn lockfile_items(kind: LockfileKind, content: &str) -> Option> { + match kind { + LockfileKind::PackageResolved => Some(swift_package_resolved_items(content)), + _ => None, } } diff --git a/crates/dependable-core/src/lockfiles/swift_package_resolved.rs b/crates/dependable-core/src/lockfiles/swift_package_resolved.rs new file mode 100644 index 0000000..7131548 --- /dev/null +++ b/crates/dependable-core/src/lockfiles/swift_package_resolved.rs @@ -0,0 +1,410 @@ +//! Reader for SwiftPM's `Package.resolved`. +//! +//! Unlike every other lockfile here, this one is the **source** of the dependency +//! list rather than an annotation on one. `Package.swift` is executable Swift — +//! dependencies are routinely assembled in loops, behind conditionals, and from +//! variables — so reading it as text produces wrong answers rather than +//! incomplete ones, and it is deliberately not read at all. +//! `Package.resolved` is plain JSON carrying the full flattened pin set: +//! identity, location URL, revision, and version for every resolved package. +//! +//! Formats: v2 (Swift 5.6+) and v3 (Xcode 15+) both spell pins as a top-level +//! `pins` array of `{identity, kind, location, state}`; v3 only adds an +//! `originHash` field beside it. The v1 shape (`object.pins[]`, with +//! `repositoryURL` in place of `location`) costs one extra key to accept and is +//! read too, because the alternative — reporting a Swift 5.5 project as having no +//! dependencies at all — is the silent wrong answer this whole ecosystem is +//! shaped to avoid. + +use std::collections::{BTreeMap, HashMap}; + +use crate::error::ParseError; +use crate::item::{DependencyKind, Item, PackageSource}; +use crate::lockfiles::LockfileData; +use crate::parsers::json_scan::scan_strings; + +/// URL schemes a Swift package location may carry, longest-prefix first so +/// `git+https://` is never mistaken for `https://` with a `git+` host. +const SCHEMES: &[&str] = &[ + "git+https://", + "git+ssh://", + "https://", + "http://", + "ssh://", + "git://", +]; + +/// One pin exactly as `Package.resolved` records it, before interpretation. +#[derive(Debug, Default)] +struct Pin { + /// SwiftPM's package identity (the repository's last path segment, lowercased). + identity: Option, + /// `remoteSourceControl`, `localSourceControl`, `fileSystem`, or `registry`. + kind: Option, + /// The package's location: a git URL, or a path for a local package. + location: Option, + /// The resolved semantic version, when the pin resolved to a tag. + version: Option, + /// The branch, when the pin follows one instead of a version. + branch: Option, + /// The resolved git revision. Always present for a source-control pin. + revision: Option, +} + +/// The dependencies `Package.resolved` pins, in the order it records them. +/// +/// This is the whole flattened resolution — SwiftPM records transitive pins +/// beside direct ones and does not distinguish them, so neither does this. +/// +/// Never fails: malformed JSON yields whatever pins were scanned before the +/// error, which is the same degradation every other reader here offers. +#[must_use] +pub fn swift_package_resolved_items(content: &str) -> Vec { + pins(content).iter().filter_map(pin_item).collect() +} + +/// Parse `Package.resolved` into a name → resolved-version map. +/// +/// Keyed by the same name [`swift_package_resolved_items`] gives each pin, so the +/// two agree about what a package is called. +/// +/// # Errors +/// Never fails today; the signature matches every other lockfile reader so the +/// dispatch in [`crate::lockfiles::parse_lockfile_kind`] stays uniform. +pub fn parse_swift_package_resolved(content: &str) -> Result { + let mut versions: HashMap> = HashMap::new(); + for item in swift_package_resolved_items(content) { + if let Some(version) = item.locked_version { + versions.entry(item.name).or_default().push(version); + } + } + Ok(LockfileData { versions }) +} + +/// The OSV `SwiftURL` name for a package location. +/// +/// SwiftPM identifies a package by its git URL; OSV keys its 60-odd Swift +/// advisories by the same URL with the scheme and the `.git` suffix removed +/// (`github.com/vapor/vapor`). Getting either wrong does not fail loudly — it +/// silently matches nothing — so both are stripped here rather than at the query. +#[must_use] +pub fn swift_package_name(location: &str) -> String { + let trimmed = location.trim(); + let scheme = SCHEMES.iter().find(|s| trimmed.starts_with(**s)).copied(); + let mut name = scheme.map_or(trimmed, |s| &trimmed[s.len()..]).to_string(); + + // A `user@` prefix addresses the host; it does not name the package. + if let Some(at) = name.find('@') + && !name[..at].contains('/') + { + name = name[at + 1..].to_string(); + } + + // git's SCP shorthand (`github.com:owner/repo`) writes a colon where a URL + // writes a slash. A port number is digits and is never this. + if scheme.is_none() + && let Some(colon) = name.find(':') + && !name[colon + 1..].starts_with(|c: char| c.is_ascii_digit()) + { + name.replace_range(colon..=colon, "/"); + } + + let name = name.trim_end_matches('/'); + let name = name.strip_suffix(".git").unwrap_or(name); + name.trim_end_matches('/').to_string() +} + +/// Collect every pin in the document, keyed by its array index so the fields of +/// one pin — which the scanner reports one at a time — reassemble in order. +fn pins(content: &str) -> Vec { + let mut by_index: BTreeMap = BTreeMap::new(); + for entry in scan_strings(content) { + let Some((index, field)) = pin_field(&entry.path) else { + continue; + }; + let pin = by_index.entry(index).or_default(); + match field.as_slice() { + // `package` is v1's spelling of `identity`. + ["identity"] | ["package"] => pin.identity = Some(entry.value), + ["kind"] => pin.kind = Some(entry.value), + // `repositoryURL` is v1's spelling of `location`. + ["location"] | ["repositoryURL"] => pin.location = Some(entry.value), + ["state", "version"] => pin.version = Some(entry.value), + ["state", "branch"] => pin.branch = Some(entry.value), + ["state", "revision"] => pin.revision = Some(entry.value), + _ => {} + } + } + by_index.into_values().collect() +} + +/// Split a scanned path into the pin index and the field path within that pin, +/// or `None` when the path is not inside a pin list. +/// +/// Only `pins` at the document root (v2/v3) or directly under `object` (v1) is a +/// pin list; a `pins` key nested anywhere else belongs to something we are not +/// reading. +fn pin_field(path: &[String]) -> Option<(usize, Vec<&str>)> { + let at = path.iter().position(|segment| segment == "pins")?; + let rooted = at == 0 || (at == 1 && path[0] == "object"); + if !rooted { + return None; + } + let index: usize = path.get(at + 1)?.parse().ok()?; + let field: Vec<&str> = path[at + 2..].iter().map(String::as_str).collect(); + (!field.is_empty()).then_some((index, field)) +} + +/// Interpret one pin as a dependency, or `None` when it names nothing. +fn pin_item(pin: &Pin) -> Option { + let local = matches!( + pin.kind.as_deref(), + Some("fileSystem" | "localSourceControl") + ) || pin.location.as_deref().is_some_and(is_local_path); + + // A local package's location is a path, which is not a name; its identity is. + let name = if local { + pin.identity.clone() + } else { + pin.location + .as_deref() + .map(swift_package_name) + .filter(|name| !name.is_empty()) + .or_else(|| pin.identity.clone()) + }?; + + // What the pin resolved to, in descending order of usefulness to a reader. + let state = pin + .version + .clone() + .or_else(|| pin.branch.clone()) + .or_else(|| pin.revision.clone()) + .unwrap_or_default(); + + // `Inherited`, not `Registry`: the version was written somewhere other than + // this entry — in `Package.resolved`, never in the manifest — so there is no + // span in `Package.swift` to report or to rewrite, which is exactly what + // `Item::has_position` reads the source to decide. A branch pin has no + // version at all and is the git dependency it looks like. + let (source, constraint, locked) = if local { + (PackageSource::Local, state, None) + } else if let Some(version) = pin.version.clone() { + (PackageSource::Inherited, version.clone(), Some(version)) + } else { + (PackageSource::Git, state, None) + }; + + Some(Item { + name, + version_constraint: constraint, + source, + version_line: 0, + version_col_start: 0, + version_col_end: 0, + registry: None, + locked_version: locked, + kind: DependencyKind::Normal, + }) +} + +/// Whether a location addresses the filesystem rather than a remote repository. +fn is_local_path(location: &str) -> bool { + let trimmed = location.trim(); + trimmed.starts_with("file://") + || trimmed.starts_with('/') + || trimmed.starts_with('.') + || trimmed.starts_with('~') +} + +#[cfg(test)] +mod tests { + use super::*; + + const V2: &str = r#"{ + "pins" : [ + { + "identity" : "swift-nio", + "kind" : "remoteSourceControl", + "location" : "https://github.com/apple/swift-nio.git", + "state" : { + "revision" : "635b2589494c97e48c62514bc8b37ced762e0a62", + "version" : "2.65.0" + } + }, + { + "identity" : "vapor", + "kind" : "remoteSourceControl", + "location" : "https://github.com/vapor/vapor", + "state" : { + "revision" : "0f1b6d1e1d6c86b2a2c5b0a1f8a1c8d5e1f0a9b8", + "version" : "4.92.1" + } + } + ], + "version" : 2 +} +"#; + + fn find<'a>(items: &'a [Item], name: &str) -> &'a Item { + items + .iter() + .find(|item| item.name == name) + .unwrap_or_else(|| panic!("no pin {name}")) + } + + #[test] + fn reads_every_pin_as_a_dependency() { + let items = swift_package_resolved_items(V2); + let names: Vec<&str> = items.iter().map(|i| i.name.as_str()).collect(); + assert_eq!( + names, + ["github.com/apple/swift-nio", "github.com/vapor/vapor"] + ); + + let nio = find(&items, "github.com/apple/swift-nio"); + assert_eq!(nio.locked_version.as_deref(), Some("2.65.0")); + assert_eq!(nio.version_constraint, "2.65.0"); + assert_eq!(nio.source, PackageSource::Inherited); + } + + /// The pin set is the only record of what the project depends on, so a pin has + /// to be worth checking — and it can never be worth *rewriting*, because the + /// version it states is not written in any manifest this tool parsed. + #[test] + fn a_pin_is_checkable_but_has_nowhere_to_be_rewritten() { + let nio = find( + &swift_package_resolved_items(V2), + "github.com/apple/swift-nio", + ) + .clone(); + assert!(nio.is_checkable()); + assert!(!nio.has_position()); + assert!(!nio.is_rewritable()); + } + + /// v3 adds `originHash` and nothing else that matters, so it must read + /// identically. The fixtures under `crates/dependable/tests/fixtures` assert + /// the same thing over two real files. + #[test] + fn v3_reads_the_same_pins_as_v2() { + let v3 = V2.replace("\"version\" : 2", "\"version\" : 3").replace( + "\"pins\" : [", + "\"originHash\" : \"abc123\",\n \"pins\" : [", + ); + assert_eq!( + swift_package_resolved_items(&v3), + swift_package_resolved_items(V2) + ); + } + + /// v1 spells the same facts differently. Reading it wrong would report a + /// Swift 5.5 project as depending on nothing at all. + #[test] + fn v1_pins_are_read_from_their_own_spelling() { + let v1 = r#"{ + "object": { + "pins": [ + { + "package": "SwiftNIO", + "repositoryURL": "https://github.com/apple/swift-nio.git", + "state": { "branch": null, "revision": "635b25", "version": "2.65.0" } + } + ] + }, + "version": 1 +}"#; + let items = swift_package_resolved_items(v1); + assert_eq!(items.len(), 1); + assert_eq!(items[0].name, "github.com/apple/swift-nio"); + assert_eq!(items[0].locked_version.as_deref(), Some("2.65.0")); + } + + /// A branch pin resolves to a revision, not a version: there is nothing to ask + /// OSV about and nothing to compare, and calling it a git dependency is what + /// every other ecosystem already says about the same situation. + #[test] + fn a_branch_pin_is_a_git_dependency() { + let lock = r#"{ + "pins": [ + { + "identity": "experimental", + "kind": "remoteSourceControl", + "location": "https://github.com/acme/experimental.git", + "state": { "branch": "main", "revision": "deadbeef" } + } + ], + "version": 2 +}"#; + let items = swift_package_resolved_items(lock); + assert_eq!(items[0].source, PackageSource::Git); + assert_eq!(items[0].version_constraint, "main"); + assert_eq!(items[0].locked_version, None); + assert!(!items[0].is_checkable()); + } + + #[test] + fn a_local_package_is_named_by_its_identity_and_never_fetched() { + let lock = r#"{ + "pins": [ + { "identity": "helpers", "kind": "fileSystem", "location": "/Users/me/helpers", "state": {} } + ], + "version": 2 +}"#; + let items = swift_package_resolved_items(lock); + assert_eq!(items[0].name, "helpers"); + assert_eq!(items[0].source, PackageSource::Local); + assert!(!items[0].is_checkable()); + } + + /// OSV keys `SwiftURL` by the repository URL with no scheme and no `.git`; + /// either left on matches nothing and reports a vulnerable package as clean. + #[test] + fn a_package_name_is_the_url_osv_keys_advisories_by() { + let cases = [ + ( + "https://github.com/vapor/vapor.git", + "github.com/vapor/vapor", + ), + ("https://github.com/vapor/vapor", "github.com/vapor/vapor"), + ( + "https://github.com/vapor/vapor.git/", + "github.com/vapor/vapor", + ), + ("http://example.com/a/b.git", "example.com/a/b"), + ("git://github.com/vapor/vapor.git", "github.com/vapor/vapor"), + ( + "ssh://git@github.com/vapor/vapor.git", + "github.com/vapor/vapor", + ), + ("git@github.com:vapor/vapor.git", "github.com/vapor/vapor"), + ( + "git+https://github.com/vapor/vapor.git", + "github.com/vapor/vapor", + ), + ]; + for (location, expected) in cases { + assert_eq!(swift_package_name(location), expected, "{location}"); + } + } + + #[test] + fn locked_versions_agree_with_the_items() { + let data = parse_swift_package_resolved(V2).unwrap(); + assert_eq!(data.versions["github.com/apple/swift-nio"], ["2.65.0"]); + assert_eq!(data.versions["github.com/vapor/vapor"], ["4.92.1"]); + assert_eq!(data.versions.len(), 2); + } + + /// A `pins` key that is not the pin list must not be read as one. + #[test] + fn an_unrelated_pins_key_is_not_a_pin_list() { + let lock = r#"{ "meta": { "pins": [ { "location": "https://x/y.git" } ] } }"#; + assert!(swift_package_resolved_items(lock).is_empty()); + } + + #[test] + fn malformed_json_yields_no_pins_rather_than_an_error() { + assert!(swift_package_resolved_items("not json at all {{{").is_empty()); + assert!(parse_swift_package_resolved("").is_ok()); + } +} diff --git a/crates/dependable-core/src/manifest.rs b/crates/dependable-core/src/manifest.rs index 6ef3d9f..eb61e74 100644 --- a/crates/dependable-core/src/manifest.rs +++ b/crates/dependable-core/src/manifest.rs @@ -353,6 +353,11 @@ pub enum LockfileKind { PubspecLock, /// Mix's `mix.lock`. MixLock, + /// SwiftPM's `Package.resolved`. + /// + /// The only kind here that is a *source* of dependencies rather than an + /// annotation on them — see [`LockfileKind::is_dependency_source`]. + PackageResolved, } impl LockfileKind { @@ -366,9 +371,25 @@ impl LockfileKind { LockfileKind::ComposerLock => "composer.lock", LockfileKind::PubspecLock => "pubspec.lock", LockfileKind::MixLock => "mix.lock", + LockfileKind::PackageResolved => "Package.resolved", } } + /// Whether this lockfile *is* the dependency list rather than an annotation on + /// one the manifest beside it already produced. + /// + /// False for every format whose manifest is readable data: there the lockfile + /// only supplies resolved versions, and + /// [`apply_lockfile`](crate::lockfiles::apply_lockfile) annotates existing items + /// and never inserts. Swift is the exception — `Package.swift` is executable + /// Swift and is deliberately not read — so `Package.resolved` is the only honest + /// record of what the project depends on, and a caller has to take items *from* + /// it. [`lockfile_items`](crate::lockfiles::lockfile_items) is that path. + #[must_use] + pub fn is_dependency_source(self) -> bool { + matches!(self, LockfileKind::PackageResolved) + } + /// Recognise a lockfile by its file name. #[must_use] pub fn detect(path: &Path) -> Option { @@ -380,6 +401,7 @@ impl LockfileKind { LockfileKind::ComposerLock, LockfileKind::PubspecLock, LockfileKind::MixLock, + LockfileKind::PackageResolved, ] .into_iter() .find(|kind| kind.file_name() == name) @@ -551,6 +573,10 @@ mod tests { LockfileKind::detect(Path::new("package-lock.json")), Some(LockfileKind::PackageLockJson) ); + assert_eq!( + LockfileKind::detect(Path::new("app/Package.resolved")), + Some(LockfileKind::PackageResolved) + ); assert_eq!(LockfileKind::detect(Path::new("Cargo.toml")), None); assert_eq!(LockfileKind::detect(Path::new("")), None); } From 8351c5855ffe2abf8d63fd3e51800c8f72c30394 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 00:18:29 -0400 Subject: [PATCH 02/24] feat(core): add the Swift ecosystem Ecosystem::Swift, whose osv_name is SwiftURL and whose package name is a repository URL rather than anything a registry issued. Ecosystem::has_registry() is new and is false for Swift alone. It states a fact about the ecosystem, which the absence of a registered fetcher cannot: without it, "the user turned this ecosystem off" and "there is nothing to turn on" are the same observation, and they want opposite behaviour. default_registry is empty for Swift and the two must agree. package_url reassembles the repository URL; version_url falls back to it, because a git tag's spelling is not derivable from the version and a link to the wrong one 404s. No registry link is fabricated. ManifestKind::PackageSwift's parser reads no dependencies on purpose: Package.swift is a Swift program whose dependencies are assembled in loops, behind conditionals, and from variables, so a text-level reader returns a confidently wrong list rather than a short one. The list comes from Package.resolved instead. --- crates/dependable-core/src/ecosystem.rs | 97 ++++++++++++++++++- crates/dependable-core/src/lib.rs | 8 +- crates/dependable-core/src/manifest.rs | 39 ++++++++ crates/dependable-core/src/parsers/mod.rs | 3 + .../src/parsers/package_swift.rs | 84 ++++++++++++++++ crates/dependable-core/src/parsers/project.rs | 4 + 6 files changed, 227 insertions(+), 8 deletions(-) create mode 100644 crates/dependable-core/src/parsers/package_swift.rs diff --git a/crates/dependable-core/src/ecosystem.rs b/crates/dependable-core/src/ecosystem.rs index ebbeb82..f7e05f0 100644 --- a/crates/dependable-core/src/ecosystem.rs +++ b/crates/dependable-core/src/ecosystem.rs @@ -4,8 +4,10 @@ use serde::{Deserialize, Serialize}; /// A package ecosystem. /// -/// Every variant is wired end-to-end: a parser, a registry fetcher, and an OSV -/// mapping. Which languages that adds up to is a wider question than this enum — +/// Most variants are wired end-to-end: a parser, a registry fetcher, and an OSV +/// mapping. [`Ecosystem::has_registry`] names the exception — an ecosystem that +/// publishes no registry has an OSV mapping and nothing to compare a version +/// against. Which languages that adds up to is a wider question than this enum — /// `deno.json` and `pnpm-workspace.yaml` are both [`Ecosystem::Npm`] — so the /// **Supported languages** table in `README.md` is authoritative for status, and /// `docs/ECOSYSTEM-CANDIDATES.md` records what a new variant has to clear. @@ -21,6 +23,15 @@ pub enum Ecosystem { CSharp, Elixir, Jvm, + /// Swift packages, identified by their git URL. + /// + /// The one ecosystem here with no registry: SwiftPM discovers versions by + /// enumerating a repository's git tags, and while SE-0292 defines a registry + /// API, no dominant public instance operates one. [`Ecosystem::has_registry`] + /// is `false`, and a check reports currency as + /// [`Undetermined`](crate::result::DependencyStatus::Undetermined) rather than + /// guessing. + Swift, } impl Ecosystem { @@ -37,9 +48,32 @@ impl Ecosystem { Ecosystem::CSharp => "NuGet", Ecosystem::Elixir => "Hex", Ecosystem::Jvm => "Maven", + // OSV keys its Swift advisories by repository URL, not by a package + // name any registry issued — which is why the name we send is the URL + // with its scheme stripped (`dependable_core::swift_package_name`). + Ecosystem::Swift => "SwiftURL", } } + /// Whether this ecosystem publishes a registry a version can be compared + /// against. + /// + /// `false` for exactly one ecosystem, [`Swift`](Self::Swift), and it is a fact + /// about the ecosystem rather than about this tool's configuration — which is + /// the whole reason it is a method here and not the absence of a fetcher. A + /// caller with no fetcher registered for an ecosystem cannot otherwise tell "the + /// user turned this off" from "there is nothing to turn on", and the two want + /// opposite behaviour: the first should skip the manifest, the second should + /// carry on and scan it for vulnerabilities. + /// + /// A `false` here means [`default_registry`](Self::default_registry) is empty and + /// nothing will ever be fetched, so currency is unknowable rather than merely + /// unread. + #[must_use] + pub fn has_registry(self) -> bool { + !matches!(self, Ecosystem::Swift) + } + /// A human-readable name for display. #[must_use] pub fn display_name(self) -> &'static str { @@ -53,10 +87,17 @@ impl Ecosystem { Ecosystem::CSharp => "C#", Ecosystem::Elixir => "Elixir", Ecosystem::Jvm => "JVM", + Ecosystem::Swift => "Swift", } } - /// The default registry base URL for the ecosystem. + /// The default registry base URL for the ecosystem, or `""` for an ecosystem + /// that has none. + /// + /// Empty is the honest answer for Swift and the only one: inventing a URL here + /// would hand a fetcher somewhere to send requests that cannot be answered. + /// [`has_registry`](Self::has_registry) is the predicate to branch on; this is + /// the value to configure a fetcher with once it says `true`. #[must_use] pub fn default_registry(self) -> &'static str { match self { @@ -69,6 +110,7 @@ impl Ecosystem { Ecosystem::CSharp => "https://api.nuget.org", Ecosystem::Elixir => "https://hex.pm", Ecosystem::Jvm => "https://repo1.maven.org/maven2", + Ecosystem::Swift => "", } } @@ -103,6 +145,11 @@ impl Ecosystem { "https://central.sonatype.com/artifact/{}", name.replace(':', "/") ), + // A Swift package name *is* its repository URL with the scheme taken + // off, so the page is that URL put back together. There is no registry + // page to link to instead, and inventing one would send the reader to a + // site that has never heard of this package. + Ecosystem::Swift => format!("https://{name}"), } } @@ -130,6 +177,10 @@ impl Ecosystem { ), // Packagist renders every version on the package page itself. Ecosystem::Php => self.package_url(name), + // A Swift version is a git tag, and the tag's spelling is not derivable + // from the version: `2.65.0` and `v2.65.0` are both common, and a link + // to the wrong one 404s. The repository is what we can name truthfully. + Ecosystem::Swift => self.package_url(name), } } @@ -160,7 +211,7 @@ mod tests { /// Every variant, so a new ecosystem cannot be added without being given /// its pages. - const ALL: [Ecosystem; 9] = [ + const ALL: [Ecosystem; 10] = [ Ecosystem::Rust, Ecosystem::Go, Ecosystem::Npm, @@ -170,8 +221,46 @@ mod tests { Ecosystem::CSharp, Ecosystem::Elixir, Ecosystem::Jvm, + Ecosystem::Swift, ]; + /// Exactly one ecosystem has no registry, and the rest must not drift into + /// claiming they have none — a `false` here routes a manifest past the + /// registry entirely. + #[test] + fn swift_is_the_only_ecosystem_without_a_registry() { + for ecosystem in ALL { + let expected = ecosystem != Ecosystem::Swift; + assert_eq!(ecosystem.has_registry(), expected, "{ecosystem:?}"); + assert_eq!( + !ecosystem.default_registry().is_empty(), + expected, + "{ecosystem:?}: a registry URL and `has_registry` must agree" + ); + } + } + + /// The OSV ecosystem strings are what a query is keyed on; a wrong one matches + /// nothing and reports a vulnerable package as clean. + #[test] + fn swift_advisories_are_keyed_by_repository_url() { + assert_eq!(Ecosystem::Swift.osv_name(), "SwiftURL"); + assert_eq!( + Ecosystem::Swift.package_url("github.com/vapor/vapor"), + "https://github.com/vapor/vapor" + ); + // No per-version page: a git tag's spelling is not derivable from the + // version, so the repository is all that can be named truthfully. + assert_eq!( + Ecosystem::Swift.version_url("github.com/vapor/vapor", "4.92.1"), + Ecosystem::Swift.package_url("github.com/vapor/vapor") + ); + assert_eq!( + Ecosystem::Swift.docs_url("github.com/vapor/vapor", "4.92.1"), + None + ); + } + #[test] fn every_ecosystem_can_name_a_page_for_a_package() { for ecosystem in ALL { diff --git a/crates/dependable-core/src/lib.rs b/crates/dependable-core/src/lib.rs index ba3141e..58be4bb 100644 --- a/crates/dependable-core/src/lib.rs +++ b/crates/dependable-core/src/lib.rs @@ -37,10 +37,10 @@ pub use npmrc::{NpmrcConfig, parse_npmrc}; pub use parsers::{ AutoTargets, CargoPackageManifest, CargoTarget, CargoTargetKind, CargoTomlParser, CfgDependencyTable, ComposerJsonParser, CsprojParser, DenoJsonParser, DependencySection, - GoModParser, GradleCatalogParser, MixExsParser, PackageField, PackageJsonParser, Parser, - PnpmWorkspaceParser, PomXmlParser, ProjectMeta, ProjectRole, PubspecYamlParser, - PyprojectTomlParser, RequirementsTxtParser, WorkspaceDecl, parse, parse_cargo_config, - parse_package_manifest, parse_package_name, parse_project, parse_workspace, + GoModParser, GradleCatalogParser, MixExsParser, PackageField, PackageJsonParser, + PackageSwiftParser, Parser, PnpmWorkspaceParser, PomXmlParser, ProjectMeta, ProjectRole, + PubspecYamlParser, PyprojectTomlParser, RequirementsTxtParser, WorkspaceDecl, parse, + parse_cargo_config, parse_package_manifest, parse_package_name, parse_project, parse_workspace, resolve_workspace_inheritance, }; pub use result::{CheckResult, DependencyStatus}; diff --git a/crates/dependable-core/src/manifest.rs b/crates/dependable-core/src/manifest.rs index eb61e74..86f0e10 100644 --- a/crates/dependable-core/src/manifest.rs +++ b/crates/dependable-core/src/manifest.rs @@ -61,6 +61,13 @@ pub enum ManifestKind { Csproj, GradleVersionCatalog, PomXml, + /// SwiftPM's `Package.swift`. + /// + /// Its parser reads no dependencies at all, deliberately — the file is a Swift + /// program, not data. The dependency list comes from its `Package.resolved`, + /// which [`LockfileKind::is_dependency_source`] marks as a source of items + /// rather than an annotation on them. + PackageSwift, } impl ManifestKind { @@ -79,6 +86,7 @@ impl ManifestKind { ManifestKind::MixExs => Ecosystem::Elixir, ManifestKind::Csproj => Ecosystem::CSharp, ManifestKind::GradleVersionCatalog | ManifestKind::PomXml => Ecosystem::Jvm, + ManifestKind::PackageSwift => Ecosystem::Swift, } } @@ -99,6 +107,9 @@ impl ManifestKind { ManifestKind::ComposerJson => &[LockfileKind::ComposerLock], ManifestKind::PubspecYaml => &[LockfileKind::PubspecLock], ManifestKind::MixExs => &[LockfileKind::MixLock], + // The only entry here that is not merely a source of resolved versions: + // without it a Swift project has no dependency list at all. + ManifestKind::PackageSwift => &[LockfileKind::PackageResolved], _ => &[], } } @@ -193,6 +204,7 @@ impl ManifestKind { "mix.exs" => ManifestKind::MixExs, "Directory.Packages.props" => ManifestKind::Csproj, "pom.xml" => ManifestKind::PomXml, + "Package.swift" => ManifestKind::PackageSwift, // Gradle reads every `*.versions.toml` under `gradle/` as a catalog; // `libs` is only the conventional name of the default one. _ if name.ends_with(".versions.toml") => ManifestKind::GradleVersionCatalog, @@ -446,6 +458,7 @@ mod tests { ManifestKind::GradleVersionCatalog, ), ("services/api/pom.xml", ManifestKind::PomXml), + ("app/Package.swift", ManifestKind::PackageSwift), ]; for (path, expected) in cases { assert_eq!( @@ -481,6 +494,7 @@ mod tests { assert_eq!(names(ManifestKind::ComposerJson), ["composer.lock"]); assert_eq!(names(ManifestKind::PubspecYaml), ["pubspec.lock"]); assert_eq!(names(ManifestKind::MixExs), ["mix.lock"]); + assert_eq!(names(ManifestKind::PackageSwift), ["Package.resolved"]); assert!(names(ManifestKind::GoMod).is_empty()); assert!(!ManifestKind::GoMod.has_lockfile_support()); } @@ -509,6 +523,7 @@ mod tests { ManifestKind::Csproj, ManifestKind::GradleVersionCatalog, ManifestKind::PomXml, + ManifestKind::PackageSwift, ] { assert!(kind.workspace_roots().is_none(), "{kind:?}"); assert!( @@ -561,6 +576,29 @@ mod tests { // A `pom.xml` is data and reads fine; what it cannot resolve is reported // entry by entry, so there is nothing here to declare unreadable. assert!(ManifestKind::PomXml.unreadable_manifests().is_empty()); + // A `Package.swift` is a program too, and is not listed here on purpose: an + // unreadable manifest is one whose dependencies went unread, and Swift's are + // read in full from `Package.resolved` beside it. What a Swift run cannot + // establish is currency, which is a different statement and is made per check. + assert!(ManifestKind::PackageSwift.unreadable_manifests().is_empty()); + } + + /// Exactly one lockfile supplies the dependency list; the rest annotate one the + /// manifest already produced, and a drift here would silently insert transitive + /// packages into five other ecosystems' results. + #[test] + fn only_package_resolved_is_a_source_of_dependencies() { + assert!(LockfileKind::PackageResolved.is_dependency_source()); + for kind in [ + LockfileKind::CargoLock, + LockfileKind::PackageLockJson, + LockfileKind::BunLock, + LockfileKind::ComposerLock, + LockfileKind::PubspecLock, + LockfileKind::MixLock, + ] { + assert!(!kind.is_dependency_source(), "{kind:?}"); + } } #[test] @@ -591,6 +629,7 @@ mod tests { ManifestKind::ComposerJson, ManifestKind::PubspecYaml, ManifestKind::MixExs, + ManifestKind::PackageSwift, ] { for lockfile in kind.lockfiles() { assert_eq!( diff --git a/crates/dependable-core/src/parsers/mod.rs b/crates/dependable-core/src/parsers/mod.rs index 41fd4e4..15fd3a0 100644 --- a/crates/dependable-core/src/parsers/mod.rs +++ b/crates/dependable-core/src/parsers/mod.rs @@ -19,6 +19,7 @@ pub mod gradle_catalog; pub mod json_scan; pub mod mix_exs; pub mod package_json; +pub mod package_swift; pub mod pnpm_workspace; pub mod pom_xml; pub mod position; @@ -42,6 +43,7 @@ pub use go_mod::GoModParser; pub use gradle_catalog::GradleCatalogParser; pub use mix_exs::MixExsParser; pub use package_json::PackageJsonParser; +pub use package_swift::PackageSwiftParser; pub use pnpm_workspace::PnpmWorkspaceParser; pub use pom_xml::PomXmlParser; pub use project::{ProjectMeta, ProjectRole, parse_project}; @@ -71,5 +73,6 @@ pub fn parse(kind: ManifestKind, content: &str) -> Result MixExsParser.parse(content), ManifestKind::GradleVersionCatalog => GradleCatalogParser.parse(content), ManifestKind::PomXml => PomXmlParser.parse(content), + ManifestKind::PackageSwift => PackageSwiftParser.parse(content), } } diff --git a/crates/dependable-core/src/parsers/package_swift.rs b/crates/dependable-core/src/parsers/package_swift.rs new file mode 100644 index 0000000..0865901 --- /dev/null +++ b/crates/dependable-core/src/parsers/package_swift.rs @@ -0,0 +1,84 @@ +//! Reader for `Package.swift` — which reads nothing, on purpose. +//! +//! `Package.swift` is not a manifest format. It is a Swift program whose output +//! happens to be a package description: dependencies are routinely assembled in +//! loops, appended behind `#if` conditionals, and built from variables and +//! functions defined elsewhere in the file. Extracting them with a regex does not +//! produce an *incomplete* list, it produces a *wrong* one — the entries it +//! happens to match, presented as the whole set — and `mix.exs`'s literal +//! `deps` list, which this crate does read, is not the same shape of file. +//! +//! So the parser declines. The dependency list comes from `Package.resolved` +//! instead ([`crate::lockfiles::swift_package_resolved_items`]), which is plain +//! JSON and records the full flattened pin set. That is the reason +//! [`crate::manifest::LockfileKind::is_dependency_source`] exists: a lockfile +//! that is the only honest record of what a project depends on has to be able to +//! *supply* items, not merely annotate them. + +use crate::error::ParseError; +use crate::manifest::ManifestKind; +use crate::manifest::ParsedManifest; +use crate::parsers::Parser; + +/// Reads a `Package.swift` and returns no dependencies, deliberately. +pub struct PackageSwiftParser; + +impl Parser for PackageSwiftParser { + /// Always succeeds with an empty item list. + /// + /// Not an error: the file is a legitimate, correctly-formed Swift manifest, and + /// failing here would be reported as "this file is broken" rather than "this + /// file is a program". The dependencies arrive from `Package.resolved`, and the + /// check that runs afterwards says what could not be established about them. + fn parse(&self, _content: &str) -> Result { + Ok(ParsedManifest { + kind: ManifestKind::PackageSwift, + items: Vec::new(), + alternate_registries: Vec::new(), + notices: Vec::new(), + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The motivating case: a loop and a conditional. Any text-level reader + /// produces a confidently wrong answer here, which is worse than none. + const MANIFEST: &str = r#"// swift-tools-version:5.9 +import PackageDescription + +var deps: [Package.Dependency] = [ + .package(url: "https://github.com/apple/swift-nio.git", from: "2.65.0"), +] +for extra in extraPackages { + deps.append(.package(url: extra.url, from: extra.version)) +} +#if canImport(Darwin) +deps.append(.package(url: "https://github.com/apple/swift-log.git", from: "1.5.0")) +#endif + +let package = Package(name: "demo", dependencies: deps) +"#; + + #[test] + fn reads_no_dependencies_from_an_executable_manifest() { + let parsed = PackageSwiftParser.parse(MANIFEST).expect("never fails"); + assert!( + parsed.items.is_empty(), + "Package.swift must not be read as text" + ); + assert_eq!(parsed.kind, ManifestKind::PackageSwift); + // No notice here: the manifest-level statement a Swift run owes its reader + // is about currency, and it is emitted per check rather than per parse, so + // it reaches a caller that never had a `Package.swift` in hand. + assert!(parsed.notices.is_empty()); + } + + #[test] + fn even_nonsense_parses_rather_than_failing() { + assert!(PackageSwiftParser.parse("").is_ok()); + assert!(PackageSwiftParser.parse("{{{ not swift").is_ok()); + } +} diff --git a/crates/dependable-core/src/parsers/project.rs b/crates/dependable-core/src/parsers/project.rs index 14d6ef0..5e447b9 100644 --- a/crates/dependable-core/src/parsers/project.rs +++ b/crates/dependable-core/src/parsers/project.rs @@ -75,6 +75,10 @@ pub fn parse_project(kind: ManifestKind, content: &str) -> ProjectMeta { ManifestKind::PubspecYaml => pubspec(content), ManifestKind::MixExs => mix(content), ManifestKind::PomXml => pom(content), + // A `Package.swift` names its package in a Swift expression, and reading + // that expression is exactly what this ecosystem declines to do. Reporting + // no name is what every other manifest whose identity we cannot see reports. + ManifestKind::PackageSwift => unnamed(), // A `pnpm-workspace.yaml` exists to hold catalogs; `Directory.Packages.props` // exists to hold central versions. A Gradle version catalog is the same shape // again — the project it serves is described by a build script. None names a From 859c95ca54fafc7ca4fc06a090b8727427c39631 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 00:21:54 -0400 Subject: [PATCH 03/24] feat(fetch): scan an ecosystem that has no registry check_inner returned UnsupportedEcosystem whenever no RegistryFetcher was registered, which the CLI prints as "skipping ...: is not enabled or not yet supported". A registry-less ecosystem was therefore dropped before the OSV scan ran - not degraded, absent. It now branches on Ecosystem::has_registry(). An ecosystem that has no registry proceeds with no fetch at all and every checkable dependency reported Undetermined, then scans OSV as usual: that scan needs a package and a version, not a registry, and the lockfile supplied both. The discriminator is the point. has_registry() is true for the other nine ecosystems, so a config-disabled one still takes the old UnsupportedEcosystem path and the CLI still says it is not enabled. Config-disabled and structurally-registry-less must not collapse into one branch, and there are tests on both sides of that line. A lockfile that IS the dependency list may now supply items; apply_lockfile still only annotates, for the five formats that always relied on it. License collection is skipped where there is no metadata endpoint to ask, rather than warning about it once per manifest. --- crates/dependable-fetch/src/check.rs | 279 ++++++++++++++++++++++----- 1 file changed, 235 insertions(+), 44 deletions(-) diff --git a/crates/dependable-fetch/src/check.rs b/crates/dependable-fetch/src/check.rs index c39d42b..b78b50f 100644 --- a/crates/dependable-fetch/src/check.rs +++ b/crates/dependable-fetch/src/check.rs @@ -13,8 +13,8 @@ use std::sync::atomic::{AtomicUsize, Ordering}; use dependable_core::{ CheckResult, DependencyStatus, Ecosystem, Evaluation, Item, LockfileKind, ManifestKind, - PackageSource, UnstableFilter, apply_lockfile, check_version, parse, parse_lockfile_kind, - resolve_workspace_inheritance, to_semver_constraint, + PackageSource, UnstableFilter, apply_lockfile, check_version, lockfile_items, parse, + parse_lockfile_kind, resolve_workspace_inheritance, to_semver_constraint, }; use futures::stream::{self, StreamExt}; use semver::Version as SemverVersion; @@ -551,11 +551,22 @@ impl Checker { workspace: Option<(PathBuf, Arc>)>, ) -> Result { let ecosystem = kind.ecosystem(); - let fetcher = self - .registries - .get(&ecosystem) - .ok_or(CheckError::UnsupportedEcosystem(ecosystem))? - .clone(); + // An ecosystem that publishes **no registry at all** is not an unsupported + // one: there is nothing to register, and its dependencies are still worth + // scanning for vulnerabilities. Returning `UnsupportedEcosystem` here would + // drop the manifest before the OSV scan ran, which is the whole feature + // silently absent. + // + // Every other ecosystem keeps the contract it has always had, and that is + // the point of asking [`Ecosystem::has_registry`] rather than merely + // observing that no fetcher is registered: a *config-disabled* ecosystem + // has a registry and is switched off, so it must still be skipped with + // "is not enabled or not yet supported" rather than half-checked. + let fetcher = match self.registries.get(&ecosystem) { + Some(fetcher) => Some(fetcher.clone()), + None if !ecosystem.has_registry() => None, + None => return Err(CheckError::UnsupportedEcosystem(ecosystem)), + }; let mut parsed = parse(kind, manifest)?; @@ -577,40 +588,54 @@ impl Checker { // lockfile is ignored — the dependency is simply checked without a locked // version. `apply_lockfile` only annotates existing items, never inserts, // so transitive deps are never introduced. - if let Some((lock_kind, lock)) = lockfile - && let Ok(data) = parse_lockfile_kind(lock_kind, lock) - { - apply_lockfile(&mut parsed.items, &data); + if let Some((lock_kind, lock)) = lockfile { + if let Some(pins) = lockfile_items(lock_kind, lock) { + // The one lockfile that *is* the dependency list. Its manifest is a + // program this crate declines to read, so without this a Swift + // project reports zero dependencies with a `Package.resolved` full + // of them sitting beside it. Appending rather than replacing keeps + // the rule that a lockfile never removes what a manifest declared. + parsed.items.extend(pins); + } else if let Ok(data) = parse_lockfile_kind(lock_kind, lock) { + apply_lockfile(&mut parsed.items, &data); + } } if let Some(warning) = deferred_versions(&parsed.items, kind) { warnings.push(warning); } - // Build the fetch task list, routing each checkable item to a fetcher: - // JSR-sourced items (Deno `jsr:` deps) to the JSR fetcher, items naming a - // resolved alternate Rust registry to that registry, and everything else - // to the ecosystem fetcher — each with a distinct cache key. Deduplicated - // by (cache_key, name). - let mut seen: HashSet<(String, String)> = HashSet::new(); - let mut tasks: Vec = Vec::new(); - for item in parsed.items.iter().filter(|i| i.is_checkable()) { - let (task_fetcher, cache_key) = self.route_item(item, &fetcher, ecosystem); - if seen.insert((cache_key.clone(), item.name.clone())) { - tasks.push(FetchTask { - name: item.name.clone(), - fetcher: task_fetcher, - cache_key, - }); + let mut results: Vec = if let Some(fetcher) = &fetcher { + // Build the fetch task list, routing each checkable item to a fetcher: + // JSR-sourced items (Deno `jsr:` deps) to the JSR fetcher, items naming a + // resolved alternate Rust registry to that registry, and everything else + // to the ecosystem fetcher — each with a distinct cache key. Deduplicated + // by (cache_key, name). + let mut seen: HashSet<(String, String)> = HashSet::new(); + let mut tasks: Vec = Vec::new(); + for item in parsed.items.iter().filter(|i| i.is_checkable()) { + let (task_fetcher, cache_key) = self.route_item(item, fetcher, ecosystem); + if seen.insert((cache_key.clone(), item.name.clone())) { + tasks.push(FetchTask { + name: item.name.clone(), + fetcher: task_fetcher, + cache_key, + }); + } } - } - let fetched = self.fetch_all(tasks).await; - let mut results: Vec = parsed - .items - .iter() - .map(|item| evaluate_item(item, &fetched, ecosystem, self.unstable)) - .collect(); + let fetched = self.fetch_all(tasks).await; + parsed + .items + .iter() + .map(|item| evaluate_item(item, &fetched, ecosystem, self.unstable)) + .collect() + } else { + // Nothing to ask, so nothing is claimed. The OSV scan below still runs: + // it needs a package and a version, not a registry, and the lockfile + // supplied both. + parsed.items.iter().map(without_a_registry).collect() + }; if let Some(osv) = &self.osv && let Err(e) = scan_vulnerabilities(osv, ecosystem, &mut results).await @@ -622,7 +647,11 @@ impl Checker { // exactly like the vulnerability scan above: it degrades to a warning // rather than failing the check, because the version data is still // correct and useful without a license column. + // A registry-less ecosystem publishes no metadata endpoint either, so this + // would fail every time and say so in a warning about a feature the user + // never asked this ecosystem for. if self.licenses + && fetcher.is_some() && let Err(e) = self.attach_licenses(ecosystem, &mut results).await { warnings.push(format!("license collection skipped: {e}")); @@ -835,6 +864,36 @@ fn deferred_versions(items: &[Item], kind: ManifestKind) -> Option { )) } +/// The verdict for an item nothing was ever going to fetch. +fn unfetchable(item: &Item) -> CheckResult { + let status = match item.source { + PackageSource::Git => DependencyStatus::Git, + // An entry that defers its version elsewhere and found nothing there is + // a real package on a real registry whose version this run never read. + // `Local` would say the opposite — that there is no registry for it — + // which of `spring-boot-starter-web` is simply false, and is the wrong + // token for a CI consumer to read. + PackageSource::Inherited => DependencyStatus::Undetermined, + _ => DependencyStatus::Local, + }; + CheckResult::new(item.clone(), status) +} + +/// The verdict for one item in an ecosystem that publishes no registry. +/// +/// A path or git dependency reports exactly what it always did — nothing was +/// going to be fetched for it either way. Everything else is +/// [`DependencyStatus::Undetermined`]: currency here is not merely unread but +/// *unknowable*, and both `UpToDate` and `Error` would be claims this run has no +/// basis for. `Local` would be worse still, since these are real published +/// packages that simply have no registry behind them. +fn without_a_registry(item: &Item) -> CheckResult { + if !item.is_checkable() { + return unfetchable(item); + } + CheckResult::new(item.clone(), DependencyStatus::Undetermined) +} + /// Evaluate one parsed item against the fetched version lists, applying the /// configured pre-release filter before classification. fn evaluate_item( @@ -844,17 +903,7 @@ fn evaluate_item( unstable: UnstableFilter, ) -> CheckResult { if !item.is_checkable() { - let status = match item.source { - PackageSource::Git => DependencyStatus::Git, - // An entry that defers its version elsewhere and found nothing there is - // a real package on a real registry whose version this run never read. - // `Local` would say the opposite — that there is no registry for it — - // which of `spring-boot-starter-web` is simply false, and is the wrong - // token for a CI consumer to read. - PackageSource::Inherited => DependencyStatus::Undetermined, - _ => DependencyStatus::Local, - }; - return CheckResult::new(item.clone(), status); + return unfetchable(item); } match fetched.get(&item.name) { Some(Ok(versions)) => { @@ -1308,6 +1357,148 @@ mod tests { item("[dependencies]\ntime = \"0.2.7\"\n") } + /// A `Package.resolved` v2 pin set, the only record of a Swift project's + /// dependencies. + const PACKAGE_RESOLVED: &str = r#"{ + "pins" : [ + { + "identity" : "swift-nio", + "kind" : "remoteSourceControl", + "location" : "https://github.com/apple/swift-nio.git", + "state" : { "revision" : "635b25", "version" : "2.65.0" } + }, + { + "identity" : "helpers", + "kind" : "fileSystem", + "location" : "/Users/me/helpers", + "state" : { } + } + ], + "version" : 2 +}"#; + + /// A checker wired the way the CLI wires one when an ecosystem is switched off + /// in config: no fetcher for it, and no network reachable if one were tried. + fn offline_checker() -> Checker { + Checker::builder() + .rust_registry("http://127.0.0.1:1".to_string(), None) + .vulnerabilities(false) + .disk_cache(false) + .build() + .expect("a checker builds without a network") + } + + /// The feature this whole ecosystem rests on. Before `has_registry`, a manifest + /// whose ecosystem had no registered fetcher was dropped with + /// `UnsupportedEcosystem` *before* the OSV scan — so a registry-less ecosystem + /// was not degraded, it was absent. + #[tokio::test] + async fn an_ecosystem_with_no_registry_is_checked_rather_than_skipped() { + let check = offline_checker() + .check_manifest(ManifestKind::PackageSwift, "", Some(PACKAGE_RESOLVED)) + .await + .expect("a registry-less ecosystem is not an unsupported one"); + + assert_eq!(check.ecosystem, Ecosystem::Swift); + let names: Vec<&str> = check.results.iter().map(|r| r.item.name.as_str()).collect(); + assert_eq!( + names, + ["github.com/apple/swift-nio", "helpers"], + "the pin set is the dependency list; `Package.swift` supplied none" + ); + + let nio = &check.results[0]; + assert_eq!( + nio.status, + DependencyStatus::Undetermined, + "no registry exists to compare against, so no currency claim is made" + ); + assert_eq!(nio.item.locked_version.as_deref(), Some("2.65.0")); + assert_eq!( + nio.latest_available, None, + "nothing was fetched, so nothing is offered as newer" + ); + // A local package reports what it always did — nothing was going to be + // fetched for it in any ecosystem. + assert_eq!(check.results[1].status, DependencyStatus::Local); + } + + /// The discriminator. `has_registry()` is true for every ecosystem but Swift, + /// so an ecosystem the user switched off in config keeps the old path and the + /// CLI keeps printing `skipping … is not enabled or not yet supported`. + /// Collapsing the two would silently half-check every disabled ecosystem. + #[tokio::test] + async fn a_config_disabled_ecosystem_is_still_reported_unsupported() { + for (kind, manifest, ecosystem) in [ + ( + ManifestKind::PubspecYaml, + "dependencies:\n http: ^1.1.0\n", + Ecosystem::Dart, + ), + ( + ManifestKind::GoMod, + "require github.com/a/b v1.0.0\n", + Ecosystem::Go, + ), + ( + ManifestKind::MixExs, + "defp deps do\n [{:jason, \"~> 1.4\"}]\nend\n", + Ecosystem::Elixir, + ), + ] { + let outcome = offline_checker().check_manifest(kind, manifest, None).await; + assert!( + matches!(outcome, Err(CheckError::UnsupportedEcosystem(eco)) if eco == ecosystem), + "{ecosystem:?} has a registry and was switched off, so it must be skipped, not checked" + ); + } + } + + /// Two ways to have no fetcher, two different answers. This is the pair a + /// reviewer should attack first. + #[tokio::test] + async fn having_no_registry_and_being_switched_off_are_not_the_same_state() { + let checker = offline_checker(); + assert!( + checker + .check_manifest(ManifestKind::PackageSwift, "", Some(PACKAGE_RESOLVED)) + .await + .is_ok() + ); + assert!( + checker + .check_manifest( + ManifestKind::PubspecYaml, + "dependencies:\n http: ^1.1.0\n", + None + ) + .await + .is_err() + ); + } + + /// `apply_lockfile` never inserts, and five ecosystems depend on that. Only the + /// one lockfile that *is* the dependency list may supply items. + #[tokio::test] + async fn a_lockfile_that_is_not_a_dependency_source_still_only_annotates() { + let lock = "[[package]]\nname = \"serde\"\nversion = \"1.0.0\"\n\n\ + [[package]]\nname = \"transitive\"\nversion = \"9.9.9\"\n"; + let check = offline_checker() + .check_manifest( + ManifestKind::CargoToml, + "[dependencies]\nserde = \"1\"\n", + Some(lock), + ) + .await + .expect("rust is registered"); + let names: Vec<&str> = check.results.iter().map(|r| r.item.name.as_str()).collect(); + assert_eq!( + names, + ["serde"], + "a transitive lock entry is not a dependency" + ); + } + #[test] fn a_locked_version_outranks_the_best_compatible_one() { let mut declared = registry_item(); From e844737a776777a833ce4fcc0b6ee45e91bad7e0 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 00:24:17 -0400 Subject: [PATCH 04/24] feat(fetch): let a caller decline an ecosystem that has no registry For every ecosystem with a registry, registering the fetcher IS the on switch: a Checker without one skips those manifests. An ecosystem with nothing to register would have had no off switch at all, so `[swift] enabled = false` would have been a config key that did nothing. CheckerBuilder::registryless is that switch. Off by default, exactly as every non-Rust ecosystem is, and declining it stays possible - the answers a registry-less ecosystem gives are shaped differently from every other one's, reporting vulnerable but never outdated. --- crates/dependable-fetch/src/check.rs | 52 +++++++++++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-) diff --git a/crates/dependable-fetch/src/check.rs b/crates/dependable-fetch/src/check.rs index b78b50f..8a61383 100644 --- a/crates/dependable-fetch/src/check.rs +++ b/crates/dependable-fetch/src/check.rs @@ -158,6 +158,10 @@ pub struct Checker { /// Fetcher for [`PackageSource::Jsr`] items (a sub-registry of the npm /// ecosystem), used for Deno `jsr:` dependencies. jsr: Option>, + /// Ecosystems that publish no registry and that the caller has nonetheless + /// asked to check. `registries` is the on switch for every ecosystem that has + /// a fetcher; this is the on switch for the ones that cannot have one. + registryless: HashSet, osv: Option>, /// Whether `check_*` runs the advisory-enrichment post-pass. Off by default: /// enrichment costs one extra OSV request per vulnerable package version, so @@ -564,7 +568,7 @@ impl Checker { // "is not enabled or not yet supported" rather than half-checked. let fetcher = match self.registries.get(&ecosystem) { Some(fetcher) => Some(fetcher.clone()), - None if !ecosystem.has_registry() => None, + None if !ecosystem.has_registry() && self.registryless.contains(&ecosystem) => None, None => return Err(CheckError::UnsupportedEcosystem(ecosystem)), }; @@ -1096,6 +1100,7 @@ pub struct CheckerBuilder { rust_alt_registries: Vec<(String, String, Option)>, extra_registries: Vec<(Ecosystem, Arc)>, jsr: Option>, + registryless: Vec, vulnerabilities: bool, include_ghsa: bool, advisory_details: bool, @@ -1118,6 +1123,7 @@ impl Default for CheckerBuilder { rust_alt_registries: Vec::new(), extra_registries: Vec::new(), jsr: None, + registryless: Vec::new(), vulnerabilities: true, include_ghsa: false, advisory_details: false, @@ -1172,6 +1178,24 @@ impl CheckerBuilder { self } + /// Check an ecosystem that publishes no registry, and so has no fetcher to + /// register. + /// + /// For every other ecosystem [`CheckerBuilder::registry`] *is* the switch: a + /// `Checker` with no fetcher for one skips its manifests with + /// [`CheckError::UnsupportedEcosystem`]. An ecosystem with nothing to register + /// would otherwise have no off switch at all, and declining it has to stay + /// possible — the answers it gives are shaped differently from every other + /// ecosystem's, reporting *vulnerable* but never *outdated*. + /// + /// Off by default, exactly as every non-Rust ecosystem is. Passing an ecosystem + /// for which [`Ecosystem::has_registry`] is `true` does nothing: that ecosystem + /// is enabled by registering its fetcher. + pub fn registryless(mut self, ecosystem: Ecosystem) -> Self { + self.registryless.push(ecosystem); + self + } + /// Register the JSR fetcher used for Deno `jsr:` dependencies. JSR is a /// sub-registry of the npm ecosystem: items with [`PackageSource::Jsr`] route /// here instead of to the npm fetcher. @@ -1322,6 +1346,7 @@ impl CheckerBuilder { registries, rust_registries, jsr: self.jsr, + registryless: self.registryless.into_iter().collect(), osv, advisory_details: self.advisory_details, licenses: self.licenses, @@ -1382,6 +1407,7 @@ mod tests { fn offline_checker() -> Checker { Checker::builder() .rust_registry("http://127.0.0.1:1".to_string(), None) + .registryless(Ecosystem::Swift) .vulnerabilities(false) .disk_cache(false) .build() @@ -1454,6 +1480,30 @@ mod tests { } } + /// Having no registry is not the same as being asked for. A caller that never + /// opted in gets the same skip every other unregistered ecosystem gets, which + /// is what gives `[swift] enabled = false` something to do. + #[tokio::test] + async fn a_registryless_ecosystem_not_asked_for_is_skipped() { + let checker = Checker::builder() + .rust_registry("http://127.0.0.1:1".to_string(), None) + .vulnerabilities(false) + .disk_cache(false) + .build() + .expect("a checker builds without a network"); + + let outcome = checker + .check_manifest(ManifestKind::PackageSwift, "", Some(PACKAGE_RESOLVED)) + .await; + assert!( + matches!( + outcome, + Err(CheckError::UnsupportedEcosystem(Ecosystem::Swift)) + ), + "an ecosystem nobody asked for is off, registry or no registry" + ); + } + /// Two ways to have no fetcher, two different answers. This is the pair a /// reviewer should attack first. #[tokio::test] From 30912ab86afec49dcb4ceb4b29b218192b2c43f6 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 00:31:53 -0400 Subject: [PATCH 05/24] feat(swift): report that currency is unknown for every Swift dependency The hazard is precise: a Swift run that turns up no advisories looks exactly like a clean, up-to-date one, and a reader who is not told otherwise will read it that way. DependencyStatus::Undetermined says so per row, in a table nobody is obliged to read column by column; this says it once per manifest, in the same place and as loudly as the unreadable-lockfile notices - for every Swift manifest, including one that pins nothing. With --no-vuln the notice says nothing was established at all, rather than naming a scan that did not run. CLI: [swift] enabled, which reaches the checker through CheckerBuilder::registryless so it actually switches something off. list takes its items from Package.resolved for the one lockfile that is a dependency source; find_lockfile's path is untouched for the other five. Policy accepts swift/swiftpm/spm as an ecosystem word. Fixtures: a Package.swift that assembles its dependencies in a loop and behind a #if - the reason it is never read - with a v3 Package.resolved beside it and a v2 one under legacy/, asserted to yield identical pins. No span round-trip assertion, because there is no span: the tests assert Swift items have no position and are not rewritable, which is why fix.rs needs no change. The live OSV test is #[ignore]d and was run once by hand: SwiftURL matched GHSA-r6r4-5pr8-gjcp for vapor 4.83.0. --- README.md | 25 +- crates/dependable-fetch/src/check.rs | 94 ++++++ crates/dependable-report/src/policy.rs | 3 + crates/dependable/src/config.rs | 24 ++ crates/dependable/src/runner.rs | 24 +- crates/dependable/tests/fixture_swift.rs | 274 ++++++++++++++++++ .../fixtures/sample-swift/Package.resolved | 49 ++++ .../tests/fixtures/sample-swift/Package.swift | 29 ++ .../sample-swift/legacy/Package.resolved | 48 +++ .../sample-swift/legacy/Package.swift | 29 ++ 10 files changed, 591 insertions(+), 8 deletions(-) create mode 100644 crates/dependable/tests/fixture_swift.rs create mode 100644 crates/dependable/tests/fixtures/sample-swift/Package.resolved create mode 100644 crates/dependable/tests/fixtures/sample-swift/Package.swift create mode 100644 crates/dependable/tests/fixtures/sample-swift/legacy/Package.resolved create mode 100644 crates/dependable/tests/fixtures/sample-swift/legacy/Package.swift diff --git a/README.md b/README.md index bf899ed..22716c7 100644 --- a/README.md +++ b/README.md @@ -39,6 +39,7 @@ Or download a prebuilt binary for your platform from the | C# / .NET | `*.csproj`, `Directory.Packages.props` | NuGet | — | 🧪 Experimental | | Elixir | `mix.exs` | Hex | `mix.lock` | 🧪 Experimental | | Kotlin / Java | `gradle/libs.versions.toml`, `pom.xml` | Maven Central | — | 🧪 Experimental | +| Swift | `Package.swift` (never read) | — none exists | `Package.resolved` | 🧪 Experimental | Kotlin / Java coverage is the **declarative** half of a JVM build. For Gradle that is the version catalog: a build script (`build.gradle`, `build.gradle.kts`) is a program, @@ -54,6 +55,22 @@ is a resolution engine rather than a parser. Those dependencies are still listed no version and nothing claimed about them, so a POM that inherits some of its versions is never presented as depending on only the rest. +**Swift is vulnerability-only, and says so.** SwiftPM identifies a package by its git +URL and discovers versions by enumerating git tags. SE-0292 defines a registry API, but +no dominant public instance operates one — so there is nothing to ask "is a newer +version available?", and `dependable` never pretends otherwise. A Swift dependency is +reported `undetermined`: scanned against OSV's `SwiftURL` advisories, and **never** +compared for currency. Every Swift manifest carries a warning saying so, because a +Swift run that turns up no advisories otherwise reads exactly like a clean, up-to-date +one. `--fix` cannot apply to a Swift project. + +`Package.swift` is executable Swift, and unlike `mix.exs` it cannot be read as text +honestly — dependencies are routinely assembled in loops, behind conditionals, and from +variables, so a text-level reader returns a *wrong* list rather than a short one. It is +never read. `Package.resolved` is plain JSON carrying the full flattened pin set, and it +is where every Swift dependency comes from: the one lockfile here that is the dependency +list rather than an annotation on one. + ### Lockfiles A lockfile is what turns "the manifest allows `^19.0.0`" into "you are actually @@ -70,6 +87,7 @@ dependencies. | `composer.lock` | ✅ | ✅ | | `mix.lock` | ✅ | ✅ | | `pubspec.lock` | ✅ | ✕ — records versions but not which package required which | +| `Package.resolved` | ✅ | ✕ — records pins but not which package required which | Not read: `yarn.lock`, `pnpm-lock.yaml`, `deno.lock`, `go.sum`, `uv.lock`, `poetry.lock`, `Pipfile.lock`, `packages.lock.json`. @@ -92,17 +110,14 @@ V2 reporting features and other deferred work are tracked as GitHub issues; see ### Not yet supported -Three languages come up often enough to answer here. Each is absent for a different -reason, and one of them is closer than it looks: +Two languages come up often enough to answer here. Each is absent for a different +reason: - **Gradle build scripts and `pom.xml`** — the JVM's declarative half ships (see the table above); the rest does not. A `build.gradle.kts` is a program, and its ground truth needs `./gradlew dependencies` — a JVM daemon executing your build. `pom.xml` is data and is readable in principle, but its versions are frequently `${properties}` inherited through a parent chain, which is a resolution step of its own. -- **Swift** — SwiftPM has no canonical registry: packages are git URLs and versions are - git tags, and `Package.swift` is executable Swift. `Package.resolved` is readable, so - locked versions and vulnerability scanning are feasible, but "outdated" is not. - **C / C++** — there is no canonical registry (vcpkg is a git repository of ports whose versions are pinned by one baseline commit), vcpkg's `version-string` scheme is unordered by design, and OSV publishes no advisory data for vcpkg or ConanCenter. This diff --git a/crates/dependable-fetch/src/check.rs b/crates/dependable-fetch/src/check.rs index 8a61383..8c53507 100644 --- a/crates/dependable-fetch/src/check.rs +++ b/crates/dependable-fetch/src/check.rs @@ -608,6 +608,10 @@ impl Checker { if let Some(warning) = deferred_versions(&parsed.items, kind) { warnings.push(warning); } + if let Some(warning) = currency_is_unknowable(ecosystem, &parsed.items, self.osv.is_some()) + { + warnings.push(warning); + } let mut results: Vec = if let Some(fetcher) = &fetcher { // Build the fetch task list, routing each checkable item to a fetcher: @@ -868,6 +872,42 @@ fn deferred_versions(items: &[Item], kind: ManifestKind) -> Option { )) } +/// Say, once per manifest, that this ecosystem publishes no registry — so nothing +/// here was, or could be, compared against a newer version. +/// +/// Emitted for **every** manifest of such an ecosystem, including one that declares +/// nothing, and deliberately conditioned on nothing else. The hazard is precise: a +/// Swift run that turns up no advisories looks exactly like a clean, up-to-date one, +/// and a reader who is not told otherwise will read it that way. +/// [`DependencyStatus::Undetermined`] says so per row, in a table nobody is obliged +/// to read column by column; this says it in the same place, and as loudly as, the +/// unreadable-lockfile notices. +fn currency_is_unknowable(ecosystem: Ecosystem, items: &[Item], scanned: bool) -> Option { + if ecosystem.has_registry() { + return None; + } + let name = ecosystem.display_name(); + let count = items.iter().filter(|item| item.is_checkable()).count(); + let plural = if count == 1 { "y" } else { "ies" }; + // With scanning off there is no verdict left at all, and saying "scanned for + // vulnerabilities only" would name a check that did not run. + let outcome = if scanned { + format!( + "{count} dependenc{plural} scanned for known vulnerabilities only. A run that \ + reports none is not a run that found them up to date" + ) + } else { + format!( + "with vulnerability scanning off, nothing was established about any of the \ + {count} dependenc{plural} here at all" + ) + }; + Some(format!( + "{name} publishes no package registry, so nothing here can be checked for a newer \ + version: {outcome}, and `--fix` cannot apply to a {name} project." + )) +} + /// The verdict for an item nothing was ever going to fetch. fn unfetchable(item: &Item) -> CheckResult { let status = match item.source { @@ -1504,6 +1544,60 @@ mod tests { ); } + /// The issue's hard requirement: a Swift run that finds no advisories looks + /// exactly like a clean, current one, so the manifest has to say otherwise + /// every time — not only when there is something to report. + #[tokio::test] + async fn every_registryless_manifest_states_that_currency_is_unknown() { + let with_pins = offline_checker() + .check_manifest(ManifestKind::PackageSwift, "", Some(PACKAGE_RESOLVED)) + .await + .expect("checked"); + let warning = with_pins + .warnings + .iter() + .find(|w| w.contains("no package registry")) + .expect("a manifest-level statement, not just a status word"); + assert!(warning.contains("1 dependency"), "{warning}"); + assert!( + warning.contains("vulnerability scanning off"), + "this checker has scanning off, so it must not claim a scan ran: {warning}" + ); + assert!(warning.contains("`--fix` cannot apply"), "{warning}"); + + // And with nothing pinned at all, where there is no table row to carry it. + let empty = offline_checker() + .check_manifest(ManifestKind::PackageSwift, "", None) + .await + .expect("checked"); + assert!( + empty + .warnings + .iter() + .any(|w| w.contains("no package registry")), + "{:?}", + empty.warnings + ); + + // Never for an ecosystem that does have a registry. + let rust = offline_checker() + .check_manifest( + ManifestKind::CargoToml, + "[dependencies]\nserde = \"1\"\n", + None, + ) + .await + .expect("checked"); + assert!( + !rust + .warnings + .iter() + .any(|w| w.contains("no package registry")), + "{:?}", + rust.warnings + ); + } + /// Two ways to have no fetcher, two different answers. This is the pair a /// reviewer should attack first. #[tokio::test] diff --git a/crates/dependable-report/src/policy.rs b/crates/dependable-report/src/policy.rs index bf489f7..09be79c 100644 --- a/crates/dependable-report/src/policy.rs +++ b/crates/dependable-report/src/policy.rs @@ -906,6 +906,7 @@ fn parse_ecosystem(raw: &str) -> Option { "csharp" | "c#" | "dotnet" | "nuget" => Some(Ecosystem::CSharp), "elixir" | "hex" | "mix" => Some(Ecosystem::Elixir), "jvm" | "maven" | "kotlin" | "java" => Some(Ecosystem::Jvm), + "swift" | "swiftpm" | "spm" => Some(Ecosystem::Swift), _ => None, } } @@ -1129,6 +1130,8 @@ reason = "CVE-2023-xxxx fix" ("mix", Ecosystem::Elixir), ("kotlin", Ecosystem::Jvm), ("Maven", Ecosystem::Jvm), + ("swift", Ecosystem::Swift), + ("SwiftPM", Ecosystem::Swift), ]; for (word, expected) in cases { let parsed = policy(&format!( diff --git a/crates/dependable/src/config.rs b/crates/dependable/src/config.rs index fff8158..9f7d921 100644 --- a/crates/dependable/src/config.rs +++ b/crates/dependable/src/config.rs @@ -39,6 +39,8 @@ pub struct Config { #[serde(default)] pub jvm: JvmConfig, #[serde(default)] + pub swift: SwiftConfig, + #[serde(default)] pub vulnerability: VulnConfig, /// CI gating rules. Empty by default, so policy gates nothing until a /// `[policy]` block is written. @@ -70,6 +72,7 @@ impl Config { Ecosystem::CSharp => self.csharp.enabled, Ecosystem::Elixir => self.elixir.enabled, Ecosystem::Jvm => self.jvm.enabled, + Ecosystem::Swift => self.swift.enabled, // `Ecosystem` is `#[non_exhaustive]`: a variant added there but not // configured here is on, which is what every ecosystem defaults to. _ => true, @@ -248,6 +251,27 @@ impl Default for JvmConfig { } } +/// Swift, the one ecosystem with no `registry` key. +/// +/// SwiftPM identifies a package by its git URL and discovers versions by +/// enumerating git tags; SE-0292 defines a registry API, but no dominant public +/// instance operates one. There is nothing to point at, so nothing is offered — +/// a URL here would only give a fetcher somewhere to send requests that cannot be +/// answered. `enabled` still means what it does everywhere else: a Swift project +/// reports what its `Package.resolved` pins and what OSV knows about them, or it +/// is skipped. +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(default)] +pub struct SwiftConfig { + pub enabled: bool, +} + +impl Default for SwiftConfig { + fn default() -> Self { + Self { enabled: true } + } +} + #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(default)] pub struct VulnConfig { diff --git a/crates/dependable/src/runner.rs b/crates/dependable/src/runner.rs index fce5e22..febdaff 100644 --- a/crates/dependable/src/runner.rs +++ b/crates/dependable/src/runner.rs @@ -11,8 +11,9 @@ use std::sync::{Arc, Mutex}; use anyhow::Context; use dependable_fetch::core::{ - AlternateRegistryDecl, NpmrcConfig, PackageField, ProjectMeta, apply_lockfile, parse, - parse_cargo_config, parse_npmrc, parse_project, parse_workspace, resolve_workspace_inheritance, + AlternateRegistryDecl, NpmrcConfig, PackageField, ProjectMeta, apply_lockfile, lockfile_items, + parse, parse_cargo_config, parse_npmrc, parse_project, parse_workspace, + resolve_workspace_inheritance, }; use dependable_fetch::{ CheckError, Checker, DependencyStatus, Ecosystem, GoProxyFetcher, GraphSource, HexFetcher, @@ -240,6 +241,12 @@ impl Engine { )), ); } + // Swift has no fetcher to register — it has no registry — so enabling it is + // an assertion rather than a registration. Same switch, same config key, + // and without it `[swift] enabled = false` would do nothing at all. + if cfg.swift.enabled { + builder = builder.registryless(Ecosystem::Swift); + } if show_progress { builder = builder.on_progress(progress_sink()); } @@ -686,8 +693,19 @@ fn apply_nearest_lockfile( manifest: &Path, kind: ManifestKind, root: &Path, - items: &mut [Item], + items: &mut Vec, ) -> Option { + // One lockfile *is* the dependency list rather than an annotation on one: a + // `Package.swift` is a program this tool declines to read, so its + // `Package.resolved` is where the dependencies come from. Without this branch + // `list` reports a Swift project as depending on nothing. + if let Some((path, lock_kind)) = dependable_fetch::locate_lockfile(manifest, kind) + && lock_kind.is_dependency_source() + { + let content = std::fs::read_to_string(&path).ok()?; + items.extend(lockfile_items(lock_kind, &content)?); + return Some(relative_to(root, &path)); + } let (path, resolved) = dependable_fetch::find_lockfile(manifest, kind)?; apply_lockfile(items, &resolved); Some(relative_to(root, &path)) diff --git a/crates/dependable/tests/fixture_swift.rs b/crates/dependable/tests/fixture_swift.rs new file mode 100644 index 0000000..7922dd2 --- /dev/null +++ b/crates/dependable/tests/fixture_swift.rs @@ -0,0 +1,274 @@ +//! Offline read of the Swift fixture, plus the statements a Swift run owes its +//! reader. +//! +//! Swift is the one ecosystem here with no registry, so its results are shaped +//! differently from every other ecosystem's: `Package.resolved` supplies the +//! dependencies, OSV supplies the only verdict there is, and *currency is never +//! claimed*. These tests hold that shape in place — the failure mode this feature +//! exists to prevent is a clean-looking Swift run that quietly means nothing. + +use std::path::{Path, PathBuf}; +use std::process::Command; + +use dependable_fetch::ManifestKind; +use dependable_fetch::core::{Item, PackageSource, lockfile_items, parse, swift_package_name}; +use dependable_fetch::{Ecosystem, LockfileKind}; + +fn fixture(rel: &str) -> PathBuf { + Path::new(env!("CARGO_MANIFEST_DIR")) + .join("tests/fixtures") + .join(rel) +} + +/// The pins recorded by the `Package.resolved` at `rel`. +fn pins(rel: &str) -> Vec { + let content = std::fs::read_to_string(fixture(rel)).expect("read the fixture"); + lockfile_items(LockfileKind::PackageResolved, &content) + .expect("Package.resolved supplies items") +} + +fn find<'a>(items: &'a [Item], name: &str) -> &'a Item { + items + .iter() + .find(|item| item.name == name) + .unwrap_or_else(|| panic!("no pin {name} in {:?}", names(items))) +} + +fn names(items: &[Item]) -> Vec<&str> { + items.iter().map(|item| item.name.as_str()).collect() +} + +/// A `Package.swift` is a Swift program. The fixture deliberately assembles its +/// dependencies in a loop and behind a `#if`, which is what makes any text-level +/// reading of it wrong rather than merely partial. +#[test] +fn package_swift_declares_nothing_because_it_is_a_program() { + let manifest = std::fs::read_to_string(fixture("sample-swift/Package.swift")).unwrap(); + assert!(manifest.contains("for (name, version) in extraPackages")); + assert!(manifest.contains("#if canImport(Darwin)")); + + let parsed = parse(ManifestKind::PackageSwift, &manifest).expect("never fails"); + assert!(parsed.items.is_empty(), "Package.swift must not be read"); + assert_eq!(ManifestKind::PackageSwift.ecosystem(), Ecosystem::Swift); + assert!( + !Ecosystem::Swift.has_registry(), + "the whole shape of this ecosystem follows from this" + ); +} + +/// v3 (`originHash`, `"version": 3`) and v2 record the same pins in the same +/// shape, so a project resolved by either Xcode 15 or Swift 5.6 must read alike. +#[test] +fn v2_and_v3_package_resolved_yield_the_same_pins() { + let v3 = pins("sample-swift/Package.resolved"); + let v2 = pins("sample-swift/legacy/Package.resolved"); + assert_eq!(v2, v3, "the format version must not change the answer"); + + assert_eq!( + names(&v3), + [ + "github.com/apple/swift-crypto", + "github.com/apple/swift-log", + "github.com/apple/swift-nio", + "sample-helpers", + "github.com/acme/swift-experimental", + ], + "every pin, in the order the file records them" + ); +} + +/// The names are what OSV keys `SwiftURL` advisories by. Scheme and `.git` left on +/// match nothing, and matching nothing is indistinguishable from being clean. +#[test] +fn a_pin_is_named_by_the_url_osv_keys_advisories_by() { + let items = pins("sample-swift/Package.resolved"); + let nio = find(&items, "github.com/apple/swift-nio"); + assert_eq!(nio.locked_version.as_deref(), Some("2.65.0")); + assert_eq!( + swift_package_name("https://github.com/apple/swift-nio.git"), + nio.name + ); + assert_eq!(Ecosystem::Swift.osv_name(), "SwiftURL"); + assert_eq!( + Ecosystem::Swift.package_url(&nio.name), + "https://github.com/apple/swift-nio", + "the repository, never an invented registry page" + ); +} + +/// Every other fixture test slices the manifest at a dependency's recorded span +/// and asserts it round-trips. There is nothing to slice here, and that is the +/// assertion: a Swift version is written in `Package.resolved`, not in any file +/// this tool parsed, so nothing may point at it and `--fix` can never rewrite it. +#[test] +fn a_swift_dependency_has_no_position_and_is_never_rewritable() { + for item in pins("sample-swift/Package.resolved") { + assert!( + !item.has_position(), + "{}: no span in Package.swift means nothing may point at one", + item.name + ); + assert!( + !item.is_rewritable(), + "{}: `--fix` must never edit a Swift project", + item.name + ); + assert_eq!(item.version_line, 0); + assert_eq!(item.version_col_start, item.version_col_end); + } +} + +/// A branch pin has no version to compare or to query, and a local package has no +/// registry in any ecosystem. Neither is `Undetermined`: both are states every +/// other ecosystem already reports the same way. +#[test] +fn a_branch_pin_and_a_local_package_report_what_they_always_did() { + let items = pins("sample-swift/Package.resolved"); + + let experimental = find(&items, "github.com/acme/swift-experimental"); + assert_eq!(experimental.source, PackageSource::Git); + assert_eq!(experimental.version_constraint, "main"); + assert!(!experimental.is_checkable()); + + let helpers = find(&items, "sample-helpers"); + assert_eq!(helpers.source, PackageSource::Local); + assert!(!helpers.is_checkable()); +} + +fn run(args: &[&str]) -> std::process::Output { + Command::new(env!("CARGO_BIN_EXE_dependable")) + .args(args) + .output() + .expect("run dependable") +} + +/// The requirement the issue calls the hard part: a Swift `check` that turns up no +/// advisories must not read as "all current". Hermetic — `--no-vuln` is the only +/// network this command would have used, since there is no registry to fetch from. +#[test] +fn check_says_out_loud_that_currency_was_never_established() { + let manifest = fixture("sample-swift/Package.swift"); + let output = run(&[ + "check", + "--manifest", + manifest.to_str().unwrap(), + "--no-vuln", + ]); + let stderr = String::from_utf8_lossy(&output.stderr); + let stdout = String::from_utf8_lossy(&output.stdout); + + assert!( + output.status.success(), + "expected exit 0; stderr: {stderr}\nstdout: {stdout}" + ); + assert!( + stderr.contains("Swift publishes no package registry"), + "the limitation must be stated, not inferred; stderr: {stderr}" + ); + assert!( + stderr.contains("vulnerability scanning off"), + "`--no-vuln` means no verdict at all, and the notice must not claim one ran; \ + stderr: {stderr}" + ); + assert!(stderr.contains("`--fix` cannot apply"), "stderr: {stderr}"); + assert!( + !stdout.contains("up to date") || stdout.contains("undetermined"), + "no dependency may be reported current; stdout: {stdout}" + ); +} + +/// `list` reads no registry at all, so it is the command that shows a Swift +/// project is *found* — and it only can because `Package.resolved` supplies the +/// items its manifest declines to. +#[test] +fn list_surfaces_the_pins_a_package_swift_never_declared() { + let manifest = fixture("sample-swift/Package.swift"); + let output = run(&["list", "--manifest", manifest.to_str().unwrap()]); + let stdout = String::from_utf8_lossy(&output.stdout); + + assert!(output.status.success()); + assert!( + stdout.contains("github.com/apple/swift-nio"), + "stdout: {stdout}" + ); + assert!(stdout.contains("2.65.0"), "stdout: {stdout}"); +} + +/// Switching Swift off has to switch it off. Without the checker-level opt-in this +/// key would parse, validate, and do nothing. +#[test] +fn a_disabled_swift_ecosystem_is_skipped() { + let dir = PathBuf::from(env!("CARGO_TARGET_TMPDIR")).join("swift_disabled"); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let config = dir.join("dependable.toml"); + std::fs::write(&config, "[swift]\nenabled = false\n").unwrap(); + + let manifest = fixture("sample-swift/Package.swift"); + let output = run(&[ + "check", + "--manifest", + manifest.to_str().unwrap(), + "--config", + config.to_str().unwrap(), + "--no-vuln", + ]); + let stderr = String::from_utf8_lossy(&output.stderr); + + assert!(output.status.success(), "stderr: {stderr}"); + assert!( + stderr.contains("skipping") && stderr.contains("Swift"), + "a disabled ecosystem is skipped, exactly as every other one is; stderr: {stderr}" + ); + assert!( + !stderr.contains("Swift publishes no package registry"), + "nothing to say about an ecosystem that was not checked; stderr: {stderr}" + ); +} + +/// Live: the one verdict a Swift run can actually give. `SwiftURL` advisories are +/// keyed by the repository URL with no scheme and no `.git`, and getting that +/// wrong fails silently — it reports a vulnerable package as clean — so only a +/// real query proves the mapping. Ignored by default; `mise run test:live`. +#[test] +#[ignore = "queries api.osv.dev"] +fn live_osv_reports_a_known_vulnerable_swift_package() { + let dir = PathBuf::from(env!("CARGO_TARGET_TMPDIR")).join("swift_live_osv"); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(dir.join("Package.swift"), "// swift-tools-version:5.9\n").unwrap(); + // Vapor 4.83.0 is affected by GHSA-r6r4-5pr8-gjcp (integer overflow in URI). + std::fs::write( + dir.join("Package.resolved"), + r#"{ + "pins" : [ + { + "identity" : "vapor", + "kind" : "remoteSourceControl", + "location" : "https://github.com/vapor/vapor.git", + "state" : { "revision" : "0f1b6d", "version" : "4.83.0" } + } + ], + "version" : 2 +}"#, + ) + .unwrap(); + + let manifest = dir.join("Package.swift"); + let output = run(&[ + "check", + "--manifest", + manifest.to_str().unwrap(), + "--include-ghsa", + "--format", + "json", + ]); + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + + assert!( + stdout.contains("\"VULN\""), + "an advisory keyed by repository URL must match; stdout: {stdout}\nstderr: {stderr}" + ); + assert!(stdout.contains("GHSA-r6r4-5pr8-gjcp"), "stdout: {stdout}"); +} diff --git a/crates/dependable/tests/fixtures/sample-swift/Package.resolved b/crates/dependable/tests/fixtures/sample-swift/Package.resolved new file mode 100644 index 0000000..195cc4a --- /dev/null +++ b/crates/dependable/tests/fixtures/sample-swift/Package.resolved @@ -0,0 +1,49 @@ +{ + "originHash" : "8f2c0e3d5b7a41f9c6d2e8b0a4f7c1d3e9b5a2c8f0d6e4b1a7c3f9d5e2b8a604", + "pins" : [ + { + "identity" : "swift-crypto", + "kind" : "remoteSourceControl", + "location" : "https://github.com/apple/swift-crypto.git", + "state" : { + "revision" : "8fa345d1f1b0a1cd80eb2d7d1e8f3c9a5b7d2e40", + "version" : "3.4.0" + } + }, + { + "identity" : "swift-log", + "kind" : "remoteSourceControl", + "location" : "https://github.com/apple/swift-log.git", + "state" : { + "revision" : "9cb486020ebf03bfa5b5df985387a14a98744537", + "version" : "1.5.4" + } + }, + { + "identity" : "swift-nio", + "kind" : "remoteSourceControl", + "location" : "https://github.com/apple/swift-nio.git", + "state" : { + "revision" : "635b2589494c97e48c62514bc8b37ced762e0a62", + "version" : "2.65.0" + } + }, + { + "identity" : "sample-helpers", + "kind" : "fileSystem", + "location" : "../sample-helpers", + "state" : { + } + }, + { + "identity" : "swift-experimental", + "kind" : "remoteSourceControl", + "location" : "https://github.com/acme/swift-experimental.git", + "state" : { + "branch" : "main", + "revision" : "0f1b6d1e1d6c86b2a2c5b0a1f8a1c8d5e1f0a9b8" + } + } + ], + "version" : 3 +} diff --git a/crates/dependable/tests/fixtures/sample-swift/Package.swift b/crates/dependable/tests/fixtures/sample-swift/Package.swift new file mode 100644 index 0000000..04bf82b --- /dev/null +++ b/crates/dependable/tests/fixtures/sample-swift/Package.swift @@ -0,0 +1,29 @@ +// swift-tools-version:5.10 +import PackageDescription + +// Everything below is why this file is never read as text. The dependency list is +// assembled at build time: one entry comes from a literal, one from a loop over a +// value defined elsewhere, and one only exists on Apple platforms. A regex over +// this file does not return a short list, it returns a wrong one. +let extraPackages = ["swift-log": "1.5.0"] + +var dependencies: [Package.Dependency] = [ + .package(url: "https://github.com/apple/swift-nio.git", from: "2.65.0"), +] + +for (name, version) in extraPackages { + dependencies.append( + .package(url: "https://github.com/apple/\(name).git", from: Version(stringLiteral: version)) + ) +} + +#if canImport(Darwin) +dependencies.append(.package(url: "https://github.com/apple/swift-crypto.git", from: "3.0.0")) +#endif + +let package = Package( + name: "SampleApp", + products: [.library(name: "SampleApp", targets: ["SampleApp"])], + dependencies: dependencies, + targets: [.target(name: "SampleApp")] +) diff --git a/crates/dependable/tests/fixtures/sample-swift/legacy/Package.resolved b/crates/dependable/tests/fixtures/sample-swift/legacy/Package.resolved new file mode 100644 index 0000000..29e9f19 --- /dev/null +++ b/crates/dependable/tests/fixtures/sample-swift/legacy/Package.resolved @@ -0,0 +1,48 @@ +{ + "pins" : [ + { + "identity" : "swift-crypto", + "kind" : "remoteSourceControl", + "location" : "https://github.com/apple/swift-crypto.git", + "state" : { + "revision" : "8fa345d1f1b0a1cd80eb2d7d1e8f3c9a5b7d2e40", + "version" : "3.4.0" + } + }, + { + "identity" : "swift-log", + "kind" : "remoteSourceControl", + "location" : "https://github.com/apple/swift-log.git", + "state" : { + "revision" : "9cb486020ebf03bfa5b5df985387a14a98744537", + "version" : "1.5.4" + } + }, + { + "identity" : "swift-nio", + "kind" : "remoteSourceControl", + "location" : "https://github.com/apple/swift-nio.git", + "state" : { + "revision" : "635b2589494c97e48c62514bc8b37ced762e0a62", + "version" : "2.65.0" + } + }, + { + "identity" : "sample-helpers", + "kind" : "fileSystem", + "location" : "../sample-helpers", + "state" : { + } + }, + { + "identity" : "swift-experimental", + "kind" : "remoteSourceControl", + "location" : "https://github.com/acme/swift-experimental.git", + "state" : { + "branch" : "main", + "revision" : "0f1b6d1e1d6c86b2a2c5b0a1f8a1c8d5e1f0a9b8" + } + } + ], + "version" : 2 +} diff --git a/crates/dependable/tests/fixtures/sample-swift/legacy/Package.swift b/crates/dependable/tests/fixtures/sample-swift/legacy/Package.swift new file mode 100644 index 0000000..04bf82b --- /dev/null +++ b/crates/dependable/tests/fixtures/sample-swift/legacy/Package.swift @@ -0,0 +1,29 @@ +// swift-tools-version:5.10 +import PackageDescription + +// Everything below is why this file is never read as text. The dependency list is +// assembled at build time: one entry comes from a literal, one from a loop over a +// value defined elsewhere, and one only exists on Apple platforms. A regex over +// this file does not return a short list, it returns a wrong one. +let extraPackages = ["swift-log": "1.5.0"] + +var dependencies: [Package.Dependency] = [ + .package(url: "https://github.com/apple/swift-nio.git", from: "2.65.0"), +] + +for (name, version) in extraPackages { + dependencies.append( + .package(url: "https://github.com/apple/\(name).git", from: Version(stringLiteral: version)) + ) +} + +#if canImport(Darwin) +dependencies.append(.package(url: "https://github.com/apple/swift-crypto.git", from: "3.0.0")) +#endif + +let package = Package( + name: "SampleApp", + products: [.library(name: "SampleApp", targets: ["SampleApp"])], + dependencies: dependencies, + targets: [.target(name: "SampleApp")] +) From 4dbf399ee85cd61e38e43aae65a4aa6e077a426d Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 00:34:26 -0400 Subject: [PATCH 06/24] fix(cli): never tell a Swift project it is already up to date `fix` ended every run that rewrote nothing with "Everything is already up to date." A Swift project rewrites nothing by construction - its versions live in Package.resolved, not in any file this tool parses - so that line was a flat claim of currency on the one ecosystem that can never establish it, printed directly beneath a warning saying the opposite. A run that rewrote nothing and left dependencies undetermined now says how many could not be checked instead. "We did not look" must never be printed as "we looked and found nothing". --- crates/dependable/src/runner.rs | 20 +++++++++++++++++- crates/dependable/tests/fixture_swift.rs | 26 ++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/crates/dependable/src/runner.rs b/crates/dependable/src/runner.rs index febdaff..af823c9 100644 --- a/crates/dependable/src/runner.rs +++ b/crates/dependable/src/runner.rs @@ -873,11 +873,17 @@ pub async fn run_fix(args: FixArgs) -> anyhow::Result { let engine = Engine::new(&settings, &cfg, true)?; let mut total = 0; + let mut unchecked = 0; for manifest in &manifests { let Some(report) = engine.check_manifest(manifest).await? else { continue; }; report_inherited_skips(manifest, &report); + unchecked += report + .results + .iter() + .filter(|result| result.status == DependencyStatus::Undetermined) + .count(); let records = fix::apply_fixes(manifest, &report.results, args.all, args.dry_run)?; if records.is_empty() { continue; @@ -892,8 +898,20 @@ pub async fn run_fix(args: FixArgs) -> anyhow::Result { total += 1; } } - if total == 0 { + if total == 0 && unchecked == 0 { println!("Everything is already up to date."); + } else if total == 0 { + // "Up to date" is a claim about versions that were compared against a + // registry. Where none could be — an ecosystem that publishes no registry + // at all, or an entry whose version this manifest never states — nothing + // was established, and printing the clean line anyway turns "we did not + // look" into "we looked and found nothing", which is the one thing a fix + // run must never say. + println!( + "Nothing to rewrite. {unchecked} dependenc{} could not be checked for a newer \ + version; see the warnings above.", + if unchecked == 1 { "y" } else { "ies" } + ); } else if !args.dry_run { println!( "\nUpdated {total} dependenc{}.", diff --git a/crates/dependable/tests/fixture_swift.rs b/crates/dependable/tests/fixture_swift.rs index 7922dd2..3e71b78 100644 --- a/crates/dependable/tests/fixture_swift.rs +++ b/crates/dependable/tests/fixture_swift.rs @@ -226,6 +226,32 @@ fn a_disabled_swift_ecosystem_is_skipped() { ); } +/// `fix` used to end every run that rewrote nothing with "Everything is already up +/// to date." For a Swift project it rewrites nothing *by construction*, so that +/// line would be a flat claim of currency on the one ecosystem that can never +/// establish it. +#[test] +fn fix_never_claims_a_swift_project_is_up_to_date() { + let manifest = fixture("sample-swift/Package.swift"); + let output = run(&["fix", "--manifest", manifest.to_str().unwrap(), "--dry-run"]); + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + + assert!(output.status.success(), "stderr: {stderr}"); + assert!( + !stdout.contains("Everything is already up to date"), + "\"we did not look\" must never be printed as \"we looked and found nothing\"; \ + stdout: {stdout}" + ); + assert!( + stdout.contains("could not be checked for a newer version"), + "stdout: {stdout}" + ); + // And the manifest is left exactly as it was. + let before = std::fs::read_to_string(&manifest).unwrap(); + assert!(before.contains("for (name, version) in extraPackages")); +} + /// Live: the one verdict a Swift run can actually give. `SwiftURL` advisories are /// keyed by the repository URL with no scheme and no `.git`, and getting that /// wrong fails silently — it reports a vulnerable package as clean — so only a From cdb5c5765389ca013fb4c1bcac137dea052e46d0 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 00:35:18 -0400 Subject: [PATCH 07/24] docs(fetch): say what a registry-less check does not emit Two claims went stale as the branch landed. The comment above the lockfile block said apply_lockfile never inserts, full stop, which is now only true of the branch it introduces. And ProgressEvent promised one Started -> Finished cycle per call: an ecosystem with no registry fetches nothing and emits neither, which is worth stating so a UI knows it will never be left with a bar running. README now also says that Package.resolved records the flattened resolution, so a Swift project lists its transitive pins beside its direct ones - more than every other ecosystem shows, not less. --- README.md | 5 ++++- crates/dependable-fetch/src/check.rs | 15 ++++++++++----- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 22716c7..d5542ef 100644 --- a/README.md +++ b/README.md @@ -69,7 +69,10 @@ honestly — dependencies are routinely assembled in loops, behind conditionals, variables, so a text-level reader returns a *wrong* list rather than a short one. It is never read. `Package.resolved` is plain JSON carrying the full flattened pin set, and it is where every Swift dependency comes from: the one lockfile here that is the dependency -list rather than an annotation on one. +list rather than an annotation on one. SwiftPM records the *flattened* resolution there +and does not mark which pins are direct, so a Swift project lists its transitive +dependencies alongside its direct ones — which is more than every other ecosystem shows, +not less. ### Lockfiles diff --git a/crates/dependable-fetch/src/check.rs b/crates/dependable-fetch/src/check.rs index 8c53507..d9b90e5 100644 --- a/crates/dependable-fetch/src/check.rs +++ b/crates/dependable-fetch/src/check.rs @@ -41,6 +41,10 @@ type ProgressSink = Arc; /// Each [`Checker::check_manifest`]/[`Checker::check_path`] call emits one /// `Started` → `Advanced`* → `Finished` cycle, letting a UI manage a per-manifest /// progress bar. `#[non_exhaustive]` so new phases can be added later. +/// +/// A manifest whose ecosystem publishes no registry fetches nothing and emits no +/// cycle at all — never a `Started` without its `Finished`, so a bar is never left +/// running. #[non_exhaustive] #[derive(Debug, Clone)] pub enum ProgressEvent { @@ -587,11 +591,12 @@ impl Checker { warnings.extend(undeclared_inheritance(&parsed.items, root)); } - // Apply the lockfile to annotate locked versions, dispatching on the file - // that was found rather than on the manifest beside it. An unparseable - // lockfile is ignored — the dependency is simply checked without a locked - // version. `apply_lockfile` only annotates existing items, never inserts, - // so transitive deps are never introduced. + // Apply the lockfile, dispatching on the file that was found rather than on + // the manifest beside it. An unparseable lockfile is ignored — the + // dependency is simply checked without a locked version. `apply_lockfile` + // only annotates existing items, never inserts, so transitive deps are never + // introduced; the one lockfile that *is* the dependency list takes the other + // branch, and its ecosystem has no manifest-declared items to add to. if let Some((lock_kind, lock)) = lockfile { if let Some(pins) = lockfile_items(lock_kind, lock) { // The one lockfile that *is* the dependency list. Its manifest is a From 9acf7c1111982a2819a51a23c76ff4ef74f92366 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 01:18:09 -0400 Subject: [PATCH 08/24] fix(core): stop a truncated JSON document crashing the scanner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `head -c 700 Package.resolved` was enough to panic the process: an object or array that ends before its closing delimiter still advanced the cursor past it, and the next `self.bytes[self.i..]` in `skip_trivia` indexed off the end. A closing delimiter where a value belongs had the matching problem in the other direction — `skip_scalar` consumed nothing, so the enclosing loop handed it the same byte forever. Both are reachable from any half-written JSON file, in any of the six ecosystems that read one. The cursor now reads through `rest()`, which yields the empty slice every caller already meant, and a scalar that makes no progress is consumed. Adds `scan_document`, which reports whether the scan reached the end of the document. `scan_strings` keeps its exact contract — a prefix of the strings, for the readers that annotate a list some manifest already produced — but a reader of a file that *is* a dependency list cannot tell a short answer from a complete one, and now has something to ask. --- .../dependable-core/src/parsers/json_scan.rs | 155 +++++++++++++++++- 1 file changed, 149 insertions(+), 6 deletions(-) diff --git a/crates/dependable-core/src/parsers/json_scan.rs b/crates/dependable-core/src/parsers/json_scan.rs index 2eae9a5..807c446 100644 --- a/crates/dependable-core/src/parsers/json_scan.rs +++ b/crates/dependable-core/src/parsers/json_scan.rs @@ -20,19 +20,57 @@ pub struct JsonStringValue { pub content_end: usize, } +/// A whole-document scan: the string values found, and whether the document was +/// structurally sound. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ScannedJson { + /// Every string value found, in document order. + pub values: Vec, + /// Whether the document parsed to its end with nothing unexpected. `false` + /// means [`values`](Self::values) is a *prefix* of the document's strings, + /// which is a different thing from the document's strings. + pub well_formed: bool, +} + /// Scan JSON or JSONC `src`, returning every string value with its path, in /// document order. Malformed input yields whatever was scanned up to the error. +/// +/// A caller that cannot tell a short list from a complete one — because the file +/// *is* the list rather than an annotation on one — wants [`scan_document`] +/// instead, which says whether the scan reached the end. #[must_use] pub fn scan_strings(src: &str) -> Vec { + scan_document(src).values +} + +/// Scan JSON or JSONC `src`, reporting both the string values and whether the +/// document was well-formed. +/// +/// Well-formedness is judged structurally: every object and array closed, every +/// string terminated, every key followed by a `:`, every bare scalar a real JSON +/// literal, and nothing left over after the top-level value. It is deliberately +/// not a validator — duplicate keys, JSONC comments, and lone surrogates all +/// pass — it answers only "did the scan see the whole document". +#[must_use] +pub fn scan_document(src: &str) -> ScannedJson { let mut scanner = Scanner { bytes: src.as_bytes(), src, i: 0, out: Vec::new(), + well_formed: true, }; scanner.skip_trivia(); scanner.parse_value(&[]); - scanner.out + scanner.skip_trivia(); + // Anything after the top-level value belongs to no value at all. + if scanner.i < scanner.bytes.len() { + scanner.well_formed = false; + } + ScannedJson { + values: scanner.out, + well_formed: scanner.well_formed, + } } struct Scanner<'a> { @@ -40,23 +78,36 @@ struct Scanner<'a> { src: &'a str, i: usize, out: Vec, + /// Cleared the moment the document departs from JSON's grammar. Never + /// consulted by the scan itself, which always keeps going. + well_formed: bool, } impl Scanner<'_> { + /// The bytes from the cursor on, empty once the cursor has run off the end. + /// + /// The cursor is advanced past a delimiter that turned out not to be there — + /// a truncated document ends mid-object — so it can sit *beyond* the last + /// byte, and `self.bytes[self.i..]` panics there rather than yielding the + /// empty slice every caller here means. + fn rest(&self) -> &[u8] { + self.bytes.get(self.i..).unwrap_or_default() + } + /// Skip whitespace and `//` line / `/* */` block comments. fn skip_trivia(&mut self) { loop { while self.i < self.bytes.len() && self.bytes[self.i].is_ascii_whitespace() { self.i += 1; } - if self.bytes[self.i..].starts_with(b"//") { + if self.rest().starts_with(b"//") { self.i += 2; while self.i < self.bytes.len() && self.bytes[self.i] != b'\n' { self.i += 1; } - } else if self.bytes[self.i..].starts_with(b"/*") { + } else if self.rest().starts_with(b"/*") { self.i += 2; - while self.i < self.bytes.len() && !self.bytes[self.i..].starts_with(b"*/") { + while self.i < self.bytes.len() && !self.rest().starts_with(b"*/") { self.i += 1; } self.i = (self.i + 2).min(self.bytes.len()); @@ -80,8 +131,12 @@ impl Scanner<'_> { content_start: start, content_end: end, }); + } else { + self.well_formed = false; } } + // Nothing at all where a value belongs: the document ended early. + None => self.well_formed = false, _ => self.skip_scalar(), } } @@ -91,7 +146,13 @@ impl Scanner<'_> { loop { self.skip_trivia(); match self.bytes.get(self.i) { - Some(b'}') | None => { + Some(b'}') => { + self.i += 1; + return; + } + // End of input before the closing brace: the object is truncated. + None => { + self.well_formed = false; self.i += 1; return; } @@ -102,15 +163,18 @@ impl Scanner<'_> { Some(b'"') => {} _ => { // Unexpected; bail to avoid looping forever. + self.well_formed = false; self.i += 1; continue; } } let Some((key, ..)) = self.parse_string() else { + self.well_formed = false; return; }; self.skip_trivia(); if self.bytes.get(self.i) != Some(&b':') { + self.well_formed = false; continue; } self.i += 1; // consume ':' @@ -127,7 +191,13 @@ impl Scanner<'_> { loop { self.skip_trivia(); match self.bytes.get(self.i) { - Some(b']') | None => { + Some(b']') => { + self.i += 1; + return; + } + // End of input before the closing bracket: the array is truncated. + None => { + self.well_formed = false; self.i += 1; return; } @@ -177,6 +247,7 @@ impl Scanner<'_> { /// Skip a non-string scalar (`number`, `true`, `false`, `null`). fn skip_scalar(&mut self) { + let start = self.i; while self.i < self.bytes.len() { match self.bytes[self.i] { b',' | b'}' | b']' => break, @@ -184,9 +255,29 @@ impl Scanner<'_> { _ => self.i += 1, } } + if self.i == start { + // A closing delimiter where a value belongs. Consuming it is what + // keeps the walk finite: the enclosing loop would otherwise hand the + // same byte back to this function forever. + self.well_formed = false; + self.i += 1; + return; + } + if !is_json_literal(&self.bytes[start..self.i]) { + self.well_formed = false; + } } } +/// Whether `token` is one of JSON's bare literals or a number. +/// +/// Only [`ScannedJson::well_formed`] reads this; the scan itself skips the token +/// either way. +fn is_json_literal(token: &[u8]) -> bool { + matches!(token, b"true" | b"false" | b"null") + || std::str::from_utf8(token).is_ok_and(|text| text.parse::().is_ok()) +} + /// Unescape the common JSON string escapes (enough for package names, versions, /// and URLs). fn unescape(raw: &str) -> String { @@ -260,6 +351,58 @@ mod tests { assert!(paths(&values).contains(&(vec!["imports", "lodash"], "npm:lodash@^4"))); } + /// A truncated document used to walk the cursor off the end of the buffer and + /// panic on the next slice — a crash on `dependable list`, from nothing worse + /// than a half-written file. Every prefix of a real document must scan. + #[test] + fn every_prefix_of_a_document_scans_without_panicking() { + let src = r#"{ + "pins": [ + { "identity": "swift-nio", "location": "https://github.com/apple/swift-nio.git", + "state": { "version": "2.65.0" } } + ], + "version": 2 + }"#; + for cut in 0..=src.len() { + let _ = scan_document(&src[..cut]); + } + } + + /// A closing delimiter where a value belongs used to hand the same byte back to + /// the enclosing loop forever. Terminating matters more than what it returns. + #[test] + fn a_delimiter_where_a_value_belongs_terminates() { + for src in ["[ } ]", "{ \"a\": }", "{ \"a\": ] }", "[[[", "{{{"] { + let scanned = scan_document(src); + assert!(!scanned.well_formed, "{src}"); + } + } + + /// The signal a reader of a file that *is* a dependency list depends on: a + /// document that did not scan to its end must not pass as one that did. + #[test] + fn well_formedness_separates_a_whole_document_from_a_prefix() { + let src = r#"{ "a": [1, 2, {"b": "c"}], "d": null, "e": true }"#; + assert!(scan_document(src).well_formed); + assert!(scan_document(&src[..src.len() - 1]).well_formed.eq(&false)); + + // JSONC still counts as well-formed: comments are this scanner's business. + assert!(scan_document("{ /* hi */ \"a\": 1 } // done").well_formed); + + // Trailing content after the top-level value belongs to no value at all. + assert!(!scan_document("not json at all {{{").well_formed); + assert!(!scan_document("{} garbage").well_formed); + assert!(!scan_document("").well_formed); + + // An unterminated string, and a key with no value. + assert!(!scan_document(r#"{ "a": "unterminated "#).well_formed); + assert!(!scan_document(r#"{ "a" 1 }"#).well_formed); + + // A bare token that is no JSON literal. + assert!(!scan_document(r#"{ "a": nope }"#).well_formed); + assert!(scan_document(r#"{ "a": -1.5e3 }"#).well_formed); + } + #[test] fn handles_arrays_with_indices() { let src = r#"{ "project": { "dependencies": ["flask>=2.0", "requests"] } }"#; From cc77be024c22ecb94dd97e88a79d93f5e8c7c5a9 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 01:18:32 -0400 Subject: [PATCH 09/24] fix(swift): stop Package.resolved answering questions it cannot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three claims this reader was making that the file does not support. **A malformed file read as a short list.** Every other lockfile here annotates items a manifest already produced, so a pin it misses costs a locked version. This one *is* the list, so a pin it misses is a dependency that is never scanned for advisories — handed back, with no warning, as the complete set. A truncated file now reports as unread: `swift_package_resolved_items` returns `None` and `parse_swift_package_resolved` returns a `ParseError`, which is what puts the existing "could not be parsed" notice in front of the reader. **The repository path was matched case-sensitively against a key that is not.** OSV keys `SwiftURL` byte for byte and real keys are mixed-case — `github.com/weichsel/ZIPFoundation`, `github.com/marmelroy/Zip`, `github.com/migueldeicaza/SwiftTerm` — so `GitHub.com/vapor/vapor` matched nothing and reported a vulnerable package clean. The host is now lowercased, which is safe because hostnames are case-insensitive by definition and every OSV key spells one lowercase. The path is deliberately left exactly as written: lowercasing it would break precisely the mixed-case keys that exist. `swift_package_name_variants` offers the all-lowercase spelling as a second key to ask about. One case stays unreachable and is documented in the module and the README: a lowercase spelling in the file of a repository keyed mixed-case cannot be canonicalized without the forge. **Every pin was reported as a direct dependency.** `Package.resolved` records the flattened resolution and marks no pin apart, so `"direct": true` in `list --format json` was a claim the file never made — a project depending only on `swift-nio-ssl` gets pins for `swift-nio`, `swift-collections` and `swift-atomics` too. Pins are now `DependencyKind::Indirect`, the kind that declines to claim directness. The fixture gains a genuinely transitive pin, so the case is actually exercised. --- crates/dependable-core/src/item.rs | 7 +- crates/dependable-core/src/lib.rs | 3 +- crates/dependable-core/src/lockfiles/mod.rs | 11 +- .../src/lockfiles/swift_package_resolved.rs | 251 +++++++++++++++--- crates/dependable/tests/fixture_swift.rs | 101 +++++++ .../fixtures/sample-swift/Package.resolved | 9 + .../tests/fixtures/sample-swift/Package.swift | 5 + .../sample-swift/legacy/Package.resolved | 9 + .../sample-swift/legacy/Package.swift | 5 + 9 files changed, 365 insertions(+), 36 deletions(-) diff --git a/crates/dependable-core/src/item.rs b/crates/dependable-core/src/item.rs index 5bcf9ef..aef39c4 100644 --- a/crates/dependable-core/src/item.rs +++ b/crates/dependable-core/src/item.rs @@ -117,8 +117,11 @@ pub enum DependencyKind { /// Cargo's `[workspace.dependencies]`, pnpm catalogs, NuGet `PackageVersion`. /// Members opt in by name, so the declaration alone means nothing is depended on. Workspace, - /// A transitive dependency the manifest records explicitly (`go.mod`'s - /// `// indirect`). Not a direct dependency of the module. + /// A dependency that is not known to be a direct one: either recorded as + /// transitive (`go.mod`'s `// indirect`), or drawn from a flattened + /// resolution that marks direct and transitive pins alike (SwiftPM's + /// `Package.resolved`). Either way, calling it direct would be a claim the + /// file does not support. Indirect, } diff --git a/crates/dependable-core/src/lib.rs b/crates/dependable-core/src/lib.rs index 58be4bb..c9db268 100644 --- a/crates/dependable-core/src/lib.rs +++ b/crates/dependable-core/src/lib.rs @@ -27,7 +27,8 @@ pub use lockfiles::{ parse_bun_lock_graph, parse_cargo_lock, parse_cargo_lock_graph, parse_composer_lock, parse_composer_lock_graph, parse_dart_pubspec_lock, parse_lockfile, parse_lockfile_kind, parse_mix_lock, parse_mix_lock_graph, parse_package_lock, parse_package_lock_graph, - parse_swift_package_resolved, swift_package_name, swift_package_resolved_items, + parse_swift_package_resolved, swift_package_name, swift_package_name_variants, + swift_package_resolved_items, }; pub use manifest::{ AlternateRegistryDecl, LockfileKind, ManifestKind, ParsedManifest, UNREADABLE_MANIFESTS, diff --git a/crates/dependable-core/src/lockfiles/mod.rs b/crates/dependable-core/src/lockfiles/mod.rs index c35e056..d8f7426 100644 --- a/crates/dependable-core/src/lockfiles/mod.rs +++ b/crates/dependable-core/src/lockfiles/mod.rs @@ -29,7 +29,8 @@ pub use mix_lock_graph::parse_mix_lock_graph; pub use package_lock_graph::parse_package_lock_graph; pub use package_lock_json::parse_package_lock; pub use swift_package_resolved::{ - parse_swift_package_resolved, swift_package_name, swift_package_resolved_items, + parse_swift_package_resolved, swift_package_name, swift_package_name_variants, + swift_package_resolved_items, }; /// Parse lockfile `content` with the parser for the file that was found. @@ -66,10 +67,16 @@ pub fn parse_lockfile_kind(kind: LockfileKind, content: &str) -> Result Option> { match kind { - LockfileKind::PackageResolved => Some(swift_package_resolved_items(content)), + LockfileKind::PackageResolved => swift_package_resolved_items(content), _ => None, } } diff --git a/crates/dependable-core/src/lockfiles/swift_package_resolved.rs b/crates/dependable-core/src/lockfiles/swift_package_resolved.rs index 7131548..9fb35ea 100644 --- a/crates/dependable-core/src/lockfiles/swift_package_resolved.rs +++ b/crates/dependable-core/src/lockfiles/swift_package_resolved.rs @@ -15,13 +15,28 @@ //! read too, because the alternative — reporting a Swift 5.5 project as having no //! dependencies at all — is the silent wrong answer this whole ecosystem is //! shaped to avoid. +//! +//! Two consequences of the file being the list rather than an annotation on one: +//! a malformed file is reported as **unread** rather than degraded to the pins +//! scanned before the error, and every pin is [`DependencyKind::Indirect`], +//! because the resolution is flattened and marks no pin as direct. +//! +//! # Known limitation: repository path case +//! OSV keys `SwiftURL` advisories case-sensitively and real keys are mixed-case +//! (`github.com/weichsel/ZIPFoundation`, `github.com/marmelroy/Zip`). A +//! `Package.resolved` recording a lowercase spelling of such a repository — +//! which git clones happily, and which SwiftPM's own `identity` field uses — +//! produces a key OSV does not match, and the package is reported clean. +//! [`swift_package_name`] lowercases the host and queries a lowercase path +//! variant alongside the written one, which covers every direction but this: the +//! canonical casing is a fact only the forge holds. use std::collections::{BTreeMap, HashMap}; use crate::error::ParseError; use crate::item::{DependencyKind, Item, PackageSource}; use crate::lockfiles::LockfileData; -use crate::parsers::json_scan::scan_strings; +use crate::parsers::json_scan::scan_document; /// URL schemes a Swift package location may carry, longest-prefix first so /// `git+https://` is never mistaken for `https://` with a `git+` host. @@ -51,16 +66,24 @@ struct Pin { revision: Option, } -/// The dependencies `Package.resolved` pins, in the order it records them. +/// The dependencies `Package.resolved` pins, in the order it records them, or +/// `None` when the file did not read. /// /// This is the whole flattened resolution — SwiftPM records transitive pins /// beside direct ones and does not distinguish them, so neither does this. /// -/// Never fails: malformed JSON yields whatever pins were scanned before the -/// error, which is the same degradation every other reader here offers. +/// # Malformed input is unread, not partial +/// Every other reader here degrades to "whatever was scanned before the error", +/// because it annotates a list some manifest already produced: a pin it misses +/// costs a locked version, not a dependency. This file *is* the list — a +/// `Package.swift` is a program this crate declines to read — so a partial scan +/// would hand back a silently **short** dependency list presented as the whole +/// one, and a package dropped off the end is a package never scanned for +/// advisories. `None` says "this file told us nothing", which callers already +/// know how to report; a short list is the silent wrong answer. #[must_use] -pub fn swift_package_resolved_items(content: &str) -> Vec { - pins(content).iter().filter_map(pin_item).collect() +pub fn swift_package_resolved_items(content: &str) -> Option> { + Some(pins(content)?.iter().filter_map(pin_item).collect()) } /// Parse `Package.resolved` into a name → resolved-version map. @@ -69,11 +92,21 @@ pub fn swift_package_resolved_items(content: &str) -> Vec { /// two agree about what a package is called. /// /// # Errors -/// Never fails today; the signature matches every other lockfile reader so the -/// dispatch in [`crate::lockfiles::parse_lockfile_kind`] stays uniform. +/// Returns [`ParseError::Structural`] when the JSON does not read to its end, for +/// the reason [`swift_package_resolved_items`] documents: a partial pin set is a +/// short dependency list, not a partial annotation, and reporting the file as +/// unreadable is what puts a notice in front of the user. pub fn parse_swift_package_resolved(content: &str) -> Result { + let items = swift_package_resolved_items(content).ok_or_else(|| { + ParseError::Structural( + "Package.resolved is not well-formed JSON, so the pins it records could not be \ + read; this file is the whole dependency list of a Swift project, so a partial \ + read of it is not a shorter answer but a wrong one" + .to_owned(), + ) + })?; let mut versions: HashMap> = HashMap::new(); - for item in swift_package_resolved_items(content) { + for item in items { if let Some(version) = item.locked_version { versions.entry(item.name).or_default().push(version); } @@ -87,6 +120,24 @@ pub fn parse_swift_package_resolved(content: &str) -> Result String { let trimmed = location.trim(); @@ -111,14 +162,51 @@ pub fn swift_package_name(location: &str) -> String { let name = name.trim_end_matches('/'); let name = name.strip_suffix(".git").unwrap_or(name); - name.trim_end_matches('/').to_string() + lowercase_host(name.trim_end_matches('/')) +} + +/// Lowercase the host component of an OSV `SwiftURL` name, leaving the path alone. +fn lowercase_host(name: &str) -> String { + match name.split_once('/') { + Some((host, path)) => format!("{}/{path}", host.to_ascii_lowercase()), + None => name.to_ascii_lowercase(), + } +} + +/// Extra OSV `SwiftURL` keys worth asking about for a package named `name`. +/// +/// OSV matches its Swift keys byte for byte while git forges treat a repository +/// path case-insensitively, so the same repository reaches us under whichever +/// spelling somebody pasted into `Package.swift`. Where a pin's path is not +/// already lowercase, the all-lowercase spelling is a second real key for the +/// same repository — `github.com/vapor/vapor` is keyed that way — and asking for +/// it too costs one batch entry and can only ever add a true match, since OSV +/// answers for the package it was asked about or not at all. +/// +/// Empty when the name is already lowercase, which is the overwhelming majority. +#[must_use] +pub fn swift_package_name_variants(name: &str) -> Vec { + let lowered = name.to_lowercase(); + if lowered == name { + Vec::new() + } else { + vec![lowered] + } } /// Collect every pin in the document, keyed by its array index so the fields of /// one pin — which the scanner reports one at a time — reassemble in order. -fn pins(content: &str) -> Vec { +/// +/// `None` when the document is not well-formed JSON: see +/// [`swift_package_resolved_items`] for why a prefix of the pins is refused here +/// rather than returned. +fn pins(content: &str) -> Option> { + let scanned = scan_document(content); + if !scanned.well_formed { + return None; + } let mut by_index: BTreeMap = BTreeMap::new(); - for entry in scan_strings(content) { + for entry in scanned.values { let Some((index, field)) = pin_field(&entry.path) else { continue; }; @@ -135,7 +223,7 @@ fn pins(content: &str) -> Vec { _ => {} } } - by_index.into_values().collect() + Some(by_index.into_values().collect()) } /// Split a scanned path into the pin index and the field path within that pin, @@ -203,7 +291,13 @@ fn pin_item(pin: &Pin) -> Option { version_col_end: 0, registry: None, locked_version: locked, - kind: DependencyKind::Normal, + // `Package.resolved` is the *flattened* resolution: a project depending + // only on `swift-nio-ssl` gets pins for `swift-nio`, `swift-collections`, + // and `swift-atomics` too, and the file marks none of them apart. So + // nothing here can be called a direct dependency without inventing the + // claim — and `direct: true` in `list --format json` is exactly that claim, + // read by machines. `Indirect` is the kind that declines to make it. + kind: DependencyKind::Indirect, }) } @@ -245,6 +339,11 @@ mod tests { } "#; + /// The pins of a file that is expected to read. + fn items(content: &str) -> Vec { + swift_package_resolved_items(content).expect("well-formed Package.resolved") + } + fn find<'a>(items: &'a [Item], name: &str) -> &'a Item { items .iter() @@ -254,7 +353,7 @@ mod tests { #[test] fn reads_every_pin_as_a_dependency() { - let items = swift_package_resolved_items(V2); + let items = items(V2); let names: Vec<&str> = items.iter().map(|i| i.name.as_str()).collect(); assert_eq!( names, @@ -272,16 +371,25 @@ mod tests { /// version it states is not written in any manifest this tool parsed. #[test] fn a_pin_is_checkable_but_has_nowhere_to_be_rewritten() { - let nio = find( - &swift_package_resolved_items(V2), - "github.com/apple/swift-nio", - ) - .clone(); + let nio = find(&items(V2), "github.com/apple/swift-nio").clone(); assert!(nio.is_checkable()); assert!(!nio.has_position()); assert!(!nio.is_rewritable()); } + /// `Package.resolved` is the flattened resolution: it lists a package the + /// project depends on and a package that package depends on identically. So no + /// pin may be reported as a direct dependency — `list --format json` publishes + /// exactly that field, and a machine reading `"direct": true` off a transitive + /// pin is being told something the file never said. + #[test] + fn a_pin_is_never_claimed_to_be_a_direct_dependency() { + for item in items(V2) { + assert_eq!(item.kind, DependencyKind::Indirect, "{}", item.name); + assert!(!item.kind.is_direct(), "{}", item.name); + } + } + /// v3 adds `originHash` and nothing else that matters, so it must read /// identically. The fixtures under `crates/dependable/tests/fixtures` assert /// the same thing over two real files. @@ -291,10 +399,7 @@ mod tests { "\"pins\" : [", "\"originHash\" : \"abc123\",\n \"pins\" : [", ); - assert_eq!( - swift_package_resolved_items(&v3), - swift_package_resolved_items(V2) - ); + assert_eq!(items(&v3), items(V2)); } /// v1 spells the same facts differently. Reading it wrong would report a @@ -313,7 +418,7 @@ mod tests { }, "version": 1 }"#; - let items = swift_package_resolved_items(v1); + let items = items(v1); assert_eq!(items.len(), 1); assert_eq!(items[0].name, "github.com/apple/swift-nio"); assert_eq!(items[0].locked_version.as_deref(), Some("2.65.0")); @@ -335,7 +440,7 @@ mod tests { ], "version": 2 }"#; - let items = swift_package_resolved_items(lock); + let items = items(lock); assert_eq!(items[0].source, PackageSource::Git); assert_eq!(items[0].version_constraint, "main"); assert_eq!(items[0].locked_version, None); @@ -350,7 +455,7 @@ mod tests { ], "version": 2 }"#; - let items = swift_package_resolved_items(lock); + let items = items(lock); assert_eq!(items[0].name, "helpers"); assert_eq!(items[0].source, PackageSource::Local); assert!(!items[0].is_checkable()); @@ -387,6 +492,65 @@ mod tests { } } + /// OSV's `SwiftURL` keys are matched byte for byte, and the real ones are + /// mixed-case: `github.com/weichsel/ZIPFoundation`, `github.com/marmelroy/Zip`, + /// `github.com/migueldeicaza/SwiftTerm`. Lowercasing the path would turn every + /// one of those into a key OSV has never heard of — reporting a vulnerable + /// package as clean, silently, which is the failure this ecosystem exists to + /// avoid. + #[test] + fn a_mixed_case_repository_path_is_preserved_exactly() { + let cases = [ + ( + "https://github.com/weichsel/ZIPFoundation.git", + "github.com/weichsel/ZIPFoundation", + ), + ( + "https://github.com/marmelroy/Zip", + "github.com/marmelroy/Zip", + ), + ( + "git@github.com:migueldeicaza/SwiftTerm.git", + "github.com/migueldeicaza/SwiftTerm", + ), + ]; + for (location, expected) in cases { + assert_eq!(swift_package_name(location), expected, "{location}"); + } + } + + /// A hostname is case-insensitive by definition and every OSV `SwiftURL` key + /// spells one in lowercase, so `GitHub.com/...` must not be carried through as + /// written — and normalizing it must not touch the path beside it. + #[test] + fn the_host_is_lowercased_and_the_path_is_not() { + assert_eq!( + swift_package_name("https://GitHub.com/vapor/vapor.git"), + "github.com/vapor/vapor" + ); + assert_eq!( + swift_package_name("https://GitHub.COM/weichsel/ZIPFoundation.git"), + "github.com/weichsel/ZIPFoundation" + ); + assert_eq!( + swift_package_name("git@GitHub.com:marmelroy/Zip.git"), + "github.com/marmelroy/Zip" + ); + } + + /// A forge treats the repository path case-insensitively while OSV does not, so + /// the all-lowercase spelling of a mixed-case name is a second real key for the + /// same repository and is worth asking about too. A name that is already + /// lowercase has no second spelling and must not cost a second query. + #[test] + fn a_mixed_case_name_offers_its_lowercase_spelling_as_a_second_key() { + assert_eq!( + swift_package_name_variants("github.com/Vapor/Vapor"), + ["github.com/vapor/vapor"] + ); + assert!(swift_package_name_variants("github.com/vapor/vapor").is_empty()); + } + #[test] fn locked_versions_agree_with_the_items() { let data = parse_swift_package_resolved(V2).unwrap(); @@ -399,12 +563,37 @@ mod tests { #[test] fn an_unrelated_pins_key_is_not_a_pin_list() { let lock = r#"{ "meta": { "pins": [ { "location": "https://x/y.git" } ] } }"#; - assert!(swift_package_resolved_items(lock).is_empty()); + assert!(items(lock).is_empty()); } + /// Malformed input is reported as unread rather than degraded to the pins that + /// happened to scan first. Every other lockfile here annotates a list a manifest + /// already produced, so a pin it misses costs a locked version; this file *is* + /// the list, so a pin it misses is a dependency that is never scanned for + /// advisories — presented, with no warning, as the complete set. #[test] - fn malformed_json_yields_no_pins_rather_than_an_error() { - assert!(swift_package_resolved_items("not json at all {{{").is_empty()); - assert!(parse_swift_package_resolved("").is_ok()); + fn a_malformed_file_reads_as_unread_not_as_a_short_list() { + assert!(swift_package_resolved_items("not json at all {{{").is_none()); + assert!(swift_package_resolved_items("").is_none()); + assert!(parse_swift_package_resolved("not json at all {{{").is_err()); + } + + /// A file truncated mid-pin is the realistic malformed case (an interrupted + /// write, a bad merge, a partial checkout) and the one that scans *most* of the + /// pins before failing — which is exactly what makes a partial answer dangerous + /// rather than obviously broken. It also used to panic outright. + #[test] + fn a_truncated_file_is_unread_rather_than_partially_read() { + // Up to but not including the closing brace — a prefix that happens to end + // there is the whole document, trailing newline aside. + for cut in 1..V2.trim_end().len() { + let truncated = &V2[..cut]; + assert!( + swift_package_resolved_items(truncated).is_none(), + "a prefix of {cut} bytes must not read as a dependency list" + ); + } + // The whole file still reads, so the check above is not vacuous. + assert_eq!(items(V2).len(), 2); } } diff --git a/crates/dependable/tests/fixture_swift.rs b/crates/dependable/tests/fixture_swift.rs index 3e71b78..0e144b8 100644 --- a/crates/dependable/tests/fixture_swift.rs +++ b/crates/dependable/tests/fixture_swift.rs @@ -70,6 +70,7 @@ fn v2_and_v3_package_resolved_yield_the_same_pins() { "github.com/apple/swift-crypto", "github.com/apple/swift-log", "github.com/apple/swift-nio", + "github.com/apple/swift-atomics", "sample-helpers", "github.com/acme/swift-experimental", ], @@ -77,6 +78,52 @@ fn v2_and_v3_package_resolved_yield_the_same_pins() { ); } +/// `Package.resolved` records the flattened resolution. The fixture's +/// `Package.swift` declares swift-nio and never mentions swift-atomics, yet the +/// pin list holds both and marks neither apart — so nothing read from it may be +/// called a direct dependency. `list --format json` publishes that as a boolean a +/// machine reads, and `"direct": true` on a transitive pin is a claim the file +/// never made. +#[test] +fn no_pin_is_published_as_a_direct_dependency() { + let manifest = std::fs::read_to_string(fixture("sample-swift/Package.swift")).unwrap(); + let code: String = manifest + .lines() + .filter(|line| !line.trim_start().starts_with("//")) + .collect::>() + .join("\n"); + assert!( + !code.contains("swift-atomics"), + "the fixture's transitive pin must not be declared by its manifest" + ); + + for item in pins("sample-swift/Package.resolved") { + assert!( + !item.kind.is_direct(), + "{}: a flattened resolution cannot say which pins are direct", + item.name + ); + } + + let output = run(&[ + "list", + "--manifest", + fixture("sample-swift/Package.swift").to_str().unwrap(), + "--format", + "json", + ]); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!(output.status.success(), "stdout: {stdout}"); + assert!( + stdout.contains("\"name\": \"github.com/apple/swift-atomics\""), + "stdout: {stdout}" + ); + assert!( + !stdout.contains("\"direct\": true"), + "no Swift pin may be published as direct; stdout: {stdout}" + ); +} + /// The names are what OSV keys `SwiftURL` advisories by. Scheme and `.git` left on /// match nothing, and matching nothing is indistinguishable from being clean. #[test] @@ -298,3 +345,57 @@ fn live_osv_reports_a_known_vulnerable_swift_package() { ); assert!(stdout.contains("GHSA-r6r4-5pr8-gjcp"), "stdout: {stdout}"); } + +/// A scratch directory with a `.git`, so the lockfile walk sees a repository +/// boundary exactly where a real checkout would put one. +fn scratch(name: &str) -> PathBuf { + let dir = PathBuf::from(env!("CARGO_TARGET_TMPDIR")).join(name); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(dir.join(".git")).expect("create the scratch repository"); + dir +} + +/// A half-written `Package.resolved` used to crash the process outright, and the +/// degradation waiting behind that crash was worse: a *prefix* of the pins, +/// presented as the whole dependency list, with the packages past the cut never +/// scanned and nothing said about it. +#[test] +fn a_truncated_package_resolved_is_reported_unread_rather_than_read_short() { + let dir = scratch("swift_truncated"); + std::fs::copy( + fixture("sample-swift/Package.swift"), + dir.join("Package.swift"), + ) + .unwrap(); + let whole = std::fs::read_to_string(fixture("sample-swift/Package.resolved")).unwrap(); + // Past the first pin and into the second, so a partial scan would return a + // plausible, and wrong, list. + let cut = whole.find("swift-log").expect("the second pin") + 20; + std::fs::write(dir.join("Package.resolved"), &whole[..cut]).unwrap(); + + for command in [ + vec!["list", dir.to_str().unwrap()], + vec![ + "check", + "--manifest", + dir.join("Package.swift").to_str().unwrap(), + "--no-vuln", + ], + ] { + let output = run(&command); + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + !stderr.contains("panicked"), + "{command:?} must not crash; stderr: {stderr}" + ); + assert!( + !stdout.contains("swift-crypto"), + "{command:?}: a prefix of the pins is not a dependency list; stdout: {stdout}" + ); + assert!( + stderr.contains("could not be parsed"), + "{command:?}: and the file must be reported unread; stderr: {stderr}" + ); + } +} diff --git a/crates/dependable/tests/fixtures/sample-swift/Package.resolved b/crates/dependable/tests/fixtures/sample-swift/Package.resolved index 195cc4a..ff3794f 100644 --- a/crates/dependable/tests/fixtures/sample-swift/Package.resolved +++ b/crates/dependable/tests/fixtures/sample-swift/Package.resolved @@ -28,6 +28,15 @@ "version" : "2.65.0" } }, + { + "identity" : "swift-atomics", + "kind" : "remoteSourceControl", + "location" : "https://github.com/apple/swift-atomics.git", + "state" : { + "revision" : "cd142fd2f64be2100422d658e7411e39489da985", + "version" : "1.2.0" + } + }, { "identity" : "sample-helpers", "kind" : "fileSystem", diff --git a/crates/dependable/tests/fixtures/sample-swift/Package.swift b/crates/dependable/tests/fixtures/sample-swift/Package.swift index 04bf82b..bbdee8e 100644 --- a/crates/dependable/tests/fixtures/sample-swift/Package.swift +++ b/crates/dependable/tests/fixtures/sample-swift/Package.swift @@ -5,6 +5,11 @@ import PackageDescription // assembled at build time: one entry comes from a literal, one from a loop over a // value defined elsewhere, and one only exists on Apple platforms. A regex over // this file does not return a short list, it returns a wrong one. +// +// Note also what is *absent*: nothing here declares swift-atomics. The +// Package.resolved beside this file pins one anyway, because it records the +// flattened resolution — swift-nio's own dependency, indistinguishable there from +// the three declared below. That is why no pin may be reported as direct. let extraPackages = ["swift-log": "1.5.0"] var dependencies: [Package.Dependency] = [ diff --git a/crates/dependable/tests/fixtures/sample-swift/legacy/Package.resolved b/crates/dependable/tests/fixtures/sample-swift/legacy/Package.resolved index 29e9f19..edd36a4 100644 --- a/crates/dependable/tests/fixtures/sample-swift/legacy/Package.resolved +++ b/crates/dependable/tests/fixtures/sample-swift/legacy/Package.resolved @@ -27,6 +27,15 @@ "version" : "2.65.0" } }, + { + "identity" : "swift-atomics", + "kind" : "remoteSourceControl", + "location" : "https://github.com/apple/swift-atomics.git", + "state" : { + "revision" : "cd142fd2f64be2100422d658e7411e39489da985", + "version" : "1.2.0" + } + }, { "identity" : "sample-helpers", "kind" : "fileSystem", diff --git a/crates/dependable/tests/fixtures/sample-swift/legacy/Package.swift b/crates/dependable/tests/fixtures/sample-swift/legacy/Package.swift index 04bf82b..bbdee8e 100644 --- a/crates/dependable/tests/fixtures/sample-swift/legacy/Package.swift +++ b/crates/dependable/tests/fixtures/sample-swift/legacy/Package.swift @@ -5,6 +5,11 @@ import PackageDescription // assembled at build time: one entry comes from a literal, one from a loop over a // value defined elsewhere, and one only exists on Apple platforms. A regex over // this file does not return a short list, it returns a wrong one. +// +// Note also what is *absent*: nothing here declares swift-atomics. The +// Package.resolved beside this file pins one anyway, because it records the +// flattened resolution — swift-nio's own dependency, indistinguishable there from +// the three declared below. That is why no pin may be reported as direct. let extraPackages = ["swift-log": "1.5.0"] var dependencies: [Package.Dependency] = [ From e35946cf7a267347bd9ced8cf73ac70a0330cb8c Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 01:19:14 -0400 Subject: [PATCH 10/24] fix(fetch): ask OSV every spelling of a name, and warn only about a run that did not fetch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two corrections to what a check states about itself. A Swift pin whose repository path is not lowercase is now queried under the written spelling *and* the all-lowercase one. OSV matches its `SwiftURL` keys byte for byte while a git forge treats the path case-insensitively, so the same repository circulates under either, and a miss is silent — a vulnerable package simply reports clean. The second query costs one batch entry, only for a name that is not already lowercase, and can add only a true match, since OSV answers about the package it was asked about or not at all. Every other ecosystem still asks exactly one question per result; IDs are appended and deduplicated rather than assigned, which for a single query is the same list in the same order. `currency_is_unknowable` now also requires that no fetcher ran. It branched on `Ecosystem::has_registry` alone, so a library consumer registering a fetcher for `Ecosystem::Swift` — the plausible SE-0292 future — got real `UpToDate` rows *and* a warning saying nothing there could be checked. And where the count is zero it no longer says "the 0 dependencies here". That phrasing turns "we could not look" into a claim about the project, which is the exact inversion this notice exists to prevent; the lockfile notice names the cause instead. --- crates/dependable-fetch/src/check.rs | 213 +++++++++++++++++++++++---- 1 file changed, 188 insertions(+), 25 deletions(-) diff --git a/crates/dependable-fetch/src/check.rs b/crates/dependable-fetch/src/check.rs index d9b90e5..abfb4cd 100644 --- a/crates/dependable-fetch/src/check.rs +++ b/crates/dependable-fetch/src/check.rs @@ -14,7 +14,8 @@ use std::sync::atomic::{AtomicUsize, Ordering}; use dependable_core::{ CheckResult, DependencyStatus, Ecosystem, Evaluation, Item, LockfileKind, ManifestKind, PackageSource, UnstableFilter, apply_lockfile, check_version, lockfile_items, parse, - parse_lockfile_kind, resolve_workspace_inheritance, to_semver_constraint, + parse_lockfile_kind, resolve_workspace_inheritance, swift_package_name_variants, + to_semver_constraint, }; use futures::stream::{self, StreamExt}; use semver::Version as SemverVersion; @@ -362,7 +363,11 @@ impl Checker { .iter() .enumerate() .filter(|(_, result)| !result.current_vulnerabilities.is_empty()) - .filter_map(|(i, result)| osv_query_for(result, ecosystem).map(|query| (i, query))) + .flat_map(|(i, result)| { + osv_queries_for(result, ecosystem) + .into_iter() + .map(move |query| (i, query)) + }) .collect(); if pending.is_empty() { return Ok(()); @@ -380,7 +385,18 @@ impl Checker { let mut failure: Option = None; for (index, outcome) in fetched { match outcome { - Ok(advisories) => check.results[index].advisories = advisories, + // Appended and deduplicated by ID rather than assigned: a result + // asked about under two spellings of its name is enriched from + // both, and re-enriching a check that already holds its + // advisories stays a no-op. + Ok(advisories) => { + let slot = &mut check.results[index].advisories; + for advisory in advisories { + if !slot.iter().any(|held| held.id == advisory.id) { + slot.push(advisory); + } + } + } Err(e) => failure = failure.or(Some(e)), } } @@ -613,8 +629,12 @@ impl Checker { if let Some(warning) = deferred_versions(&parsed.items, kind) { warnings.push(warning); } - if let Some(warning) = currency_is_unknowable(ecosystem, &parsed.items, self.osv.is_some()) - { + if let Some(warning) = currency_is_unknowable( + ecosystem, + &parsed.items, + self.osv.is_some(), + fetcher.is_some(), + ) { warnings.push(warning); } @@ -880,15 +900,32 @@ fn deferred_versions(items: &[Item], kind: ManifestKind) -> Option { /// Say, once per manifest, that this ecosystem publishes no registry — so nothing /// here was, or could be, compared against a newer version. /// -/// Emitted for **every** manifest of such an ecosystem, including one that declares -/// nothing, and deliberately conditioned on nothing else. The hazard is precise: a +/// Emitted for **every** manifest of such an ecosystem that was in fact checked +/// without a fetcher, including one that declares nothing. The hazard is precise: a /// Swift run that turns up no advisories looks exactly like a clean, up-to-date one, /// and a reader who is not told otherwise will read it that way. /// [`DependencyStatus::Undetermined`] says so per row, in a table nobody is obliged /// to read column by column; this says it in the same place, and as loudly as, the /// unreadable-lockfile notices. -fn currency_is_unknowable(ecosystem: Ecosystem, items: &[Item], scanned: bool) -> Option { - if ecosystem.has_registry() { +/// +/// `has_fetcher` is the second half of the condition rather than a detail: a library +/// consumer may register a fetcher for an ecosystem this build ships no registry for +/// — an SE-0292 Swift registry is the obvious candidate — and that run produces real +/// `UpToDate` rows. Telling its reader "nothing here can be checked" would then be +/// the same kind of false statement in the other direction. +/// +/// The count is of items that *exist*, and says nothing about whether the list they +/// came from was ever read. Where it is zero the wording says only that nothing was +/// found to check: "the 0 dependencies here" would turn "we could not look" into a +/// claim about the project, which is the inversion this whole notice exists to +/// prevent. The lockfile notice names the cause. +fn currency_is_unknowable( + ecosystem: Ecosystem, + items: &[Item], + scanned: bool, + has_fetcher: bool, +) -> Option { + if ecosystem.has_registry() || has_fetcher { return None; } let name = ecosystem.display_name(); @@ -896,7 +933,9 @@ fn currency_is_unknowable(ecosystem: Ecosystem, items: &[Item], scanned: bool) - let plural = if count == 1 { "y" } else { "ies" }; // With scanning off there is no verdict left at all, and saying "scanned for // vulnerabilities only" would name a check that did not run. - let outcome = if scanned { + let outcome = if count == 0 { + "no dependency with a version to check was found here at all".to_owned() + } else if scanned { format!( "{count} dependenc{plural} scanned for known vulnerabilities only. A run that \ reports none is not a run that found them up to date" @@ -1086,20 +1125,40 @@ fn same_flavour_only( /// actually install. Shared by the batch scan and the advisory-enrichment pass so /// the two produce identical cache keys, and so the advisories describe the exact /// version that was flagged. -fn osv_query_for(result: &CheckResult, ecosystem: Ecosystem) -> Option { +fn osv_queries_for(result: &CheckResult, ecosystem: Ecosystem) -> Vec { if !result.item.is_checkable() || matches!(result.status, DependencyStatus::Error(_)) { - return None; + return Vec::new(); } - let version = result + let Some(version) = result .item .locked_version .clone() - .or_else(|| result.latest_compatible.clone())?; - Some(OsvQuery { + .or_else(|| result.latest_compatible.clone()) + else { + return Vec::new(); + }; + let query = |name: String| OsvQuery { ecosystem: ecosystem.osv_name().to_string(), - name: result.item.name.clone(), - version, - }) + name, + version: version.clone(), + }; + let name = result.item.name.clone(); + // Swift is the one ecosystem whose OSV key is a repository URL rather than a + // registry name. OSV matches those byte for byte while a git forge treats the + // path case-insensitively, so the same repository arrives under whichever + // spelling someone pasted into `Package.swift`, and a mismatch is silent — a + // vulnerable package simply reports clean. Asking the all-lowercase spelling + // too costs one batch entry, only for a name that is not already lowercase, + // and can add only a true match: OSV answers about the package it was asked + // about or not at all. Every other ecosystem asks exactly one question, as + // before. + let extra = match ecosystem { + Ecosystem::Swift => swift_package_name_variants(&name), + _ => Vec::new(), + }; + std::iter::once(query(name)) + .chain(extra.into_iter().map(query)) + .collect() } /// Query OSV for the current version of each checkable dependency and flip its @@ -1113,7 +1172,7 @@ async fn scan_vulnerabilities( let mut queries = Vec::new(); let mut index_for = Vec::new(); for (i, result) in results.iter().enumerate() { - if let Some(query) = osv_query_for(result, ecosystem) { + for query in osv_queries_for(result, ecosystem) { queries.push(query); index_for.push(i); } @@ -1127,7 +1186,16 @@ async fn scan_vulnerabilities( if let Some(ids) = osv_results.get(query_idx) && !ids.is_empty() { - results[result_idx].current_vulnerabilities = ids.clone(); + // Appended rather than assigned, because one result may have been + // asked about under more than one spelling of its name. Every + // ecosystem but Swift sends exactly one query per result, so this + // still ends up as that query's ID list, in its order. + let found = &mut results[result_idx].current_vulnerabilities; + for id in ids { + if !found.contains(id) { + found.push(id.clone()); + } + } results[result_idx].status = DependencyStatus::Vulnerable; } } @@ -1603,6 +1671,62 @@ mod tests { ); } + /// A fetcher registered for an ecosystem this build ships no registry for. + /// SE-0292 gives SwiftPM a package registry, so a library consumer wiring one + /// up is the plausible case, not a contrived one. + struct StubFetcher; + + impl RegistryFetcher for StubFetcher { + fn fetch_versions<'a>( + &'a self, + _name: &'a str, + ) -> futures::future::BoxFuture<'a, Result> + { + use futures::FutureExt as _; + futures::future::ready(Ok(crate::registries::FetchedVersions::new(vec![ + "2.65.0".to_string(), + ]))) + .boxed() + } + } + + /// The warning is about this run, not about the ecosystem in the abstract. A + /// caller that registered a fetcher for Swift gets real `UpToDate` rows, and + /// telling its reader "nothing here can be checked for a newer version" would + /// be the same false statement in the other direction — a claim about what the + /// run did, contradicted by the table beside it. + #[tokio::test] + async fn a_registryless_ecosystem_with_a_fetcher_is_not_told_it_cannot_be_checked() { + let checker = Checker::builder() + .rust_registry("http://127.0.0.1:1".to_string(), None) + .registry(Ecosystem::Swift, Arc::new(StubFetcher)) + .vulnerabilities(false) + .disk_cache(false) + .build() + .expect("a checker builds without a network"); + + let check = checker + .check_manifest(ManifestKind::PackageSwift, "", Some(PACKAGE_RESOLVED)) + .await + .expect("checked"); + assert!( + !check + .warnings + .iter() + .any(|w| w.contains("no package registry")), + "a run that fetched must not say it could not: {:?}", + check.warnings + ); + assert!( + check + .results + .iter() + .any(|r| matches!(r.status, DependencyStatus::UpToDate)), + "and it really did fetch: {:?}", + check.results + ); + } + /// Two ways to have no fetcher, two different answers. This is the pair a /// reviewer should attack first. #[tokio::test] @@ -1648,6 +1772,45 @@ mod tests { ); } + /// The one query every ecosystem but Swift ever asks. + fn only_query(result: &CheckResult, ecosystem: Ecosystem) -> OsvQuery { + let mut queries = osv_queries_for(result, ecosystem); + assert_eq!(queries.len(), 1, "one query per result"); + queries.remove(0) + } + + /// A Swift pin whose repository path is not lowercase is asked about under both + /// spellings: OSV matches its `SwiftURL` keys byte for byte while a git forge + /// does not, so the same repository circulates under either, and a miss here is + /// silent — a vulnerable package reported clean. Every other ecosystem, and a + /// name that is already lowercase, still asks exactly one question. + #[test] + fn a_mixed_case_swift_pin_is_asked_about_under_both_spellings() { + let mut pin = registry_item(); + pin.name = "github.com/weichsel/ZIPFoundation".to_string(); + pin.locked_version = Some("0.9.16".to_string()); + let result = CheckResult::new(pin, DependencyStatus::Undetermined); + + let names: Vec = osv_queries_for(&result, Ecosystem::Swift) + .into_iter() + .map(|query| query.name) + .collect(); + assert_eq!( + names, + [ + "github.com/weichsel/ZIPFoundation", + "github.com/weichsel/zipfoundation" + ], + "the written spelling first, then the lowercase one" + ); + + let mut lower = registry_item(); + lower.name = "github.com/vapor/vapor".to_string(); + lower.locked_version = Some("4.83.0".to_string()); + let lower = CheckResult::new(lower, DependencyStatus::Undetermined); + assert_eq!(osv_queries_for(&lower, Ecosystem::Swift).len(), 1); + } + #[test] fn a_locked_version_outranks_the_best_compatible_one() { let mut declared = registry_item(); @@ -1655,7 +1818,7 @@ mod tests { let mut result = CheckResult::new(declared, DependencyStatus::UpToDate); result.latest_compatible = Some("0.2.9".to_string()); - let query = osv_query_for(&result, Ecosystem::Rust).expect("a query"); + let query = only_query(&result, Ecosystem::Rust); assert_eq!(query.ecosystem, "crates.io"); assert_eq!(query.name, "time"); assert_eq!(query.version, "0.2.7"); @@ -1665,14 +1828,14 @@ mod tests { fn an_unlocked_dependency_is_queried_at_its_best_compatible_version() { let mut result = CheckResult::new(registry_item(), DependencyStatus::UpToDate); result.latest_compatible = Some("0.2.9".to_string()); - let query = osv_query_for(&result, Ecosystem::Rust).expect("a query"); + let query = only_query(&result, Ecosystem::Rust); assert_eq!(query.version, "0.2.9"); } #[test] fn nothing_is_queried_without_a_version() { let result = CheckResult::new(registry_item(), DependencyStatus::UpToDate); - assert!(osv_query_for(&result, Ecosystem::Rust).is_none()); + assert!(osv_queries_for(&result, Ecosystem::Rust).is_empty()); } #[test] @@ -1680,7 +1843,7 @@ mod tests { let declared = item("[dependencies]\nlocal = { path = \"../local\" }\n"); assert!(!declared.is_checkable()); let result = CheckResult::new(declared, DependencyStatus::Local); - assert!(osv_query_for(&result, Ecosystem::Rust).is_none()); + assert!(osv_queries_for(&result, Ecosystem::Rust).is_empty()); } #[test] @@ -1691,7 +1854,7 @@ mod tests { declared, DependencyStatus::Error("registry unreachable".to_string()), ); - assert!(osv_query_for(&result, Ecosystem::Rust).is_none()); + assert!(osv_queries_for(&result, Ecosystem::Rust).is_empty()); } /// The single item declared by `manifest`, parsed as `kind`. From d889a8a451f9232f30e1a76b6a9123c6b645474e Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 01:19:47 -0400 Subject: [PATCH 11/24] fix(fetch): keep one project from adopting another's dependency list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `locate_lockfile` and `find_lockfile` walk up to the `.git` boundary, which is right while a lockfile *annotates* items a manifest produced: a workspace member's `Cargo.lock` at the root pins the very crates the member declared, and a pin it does not declare simply goes unused. It is wrong now that one lockfile *is* the item list. A nested SwiftPM package with no `Package.resolved` of its own adopted its ancestor's and reported the ancestor's dependencies as its own — with scanning on, attributing the root's advisories to a package that does not have the dependency, a false positive on the one verdict Swift can give. In a SwiftPM monorepo that is every package not yet resolved. A dependency source now counts only in the manifest's own directory; the five annotating formats keep the ancestor walk unchanged, and a test holds each of them to it. The state that walk was papering over also needed saying out loud. Apple advises library packages not to commit `Package.resolved`, so a Swift project with none is the common case, and nothing distinguished it from one that resolved to nothing: `check` reported "0 dependencies" and exited 0 even under `--fail-on any`. A missing dependency-source lockfile is now a `LockfileNotice` of its own, carrying the same loudness as the unreadable-lockfile notices and naming the cause, and `LockfileNotice::dependency_list_unread` carries into the exit code — `--fail-on any` asks whether everything here is checked and current, and a list nobody read cannot answer yes. A missing *annotating* lockfile is still no notice at all: it costs a locked version, not a dependency list. --- crates/dependable-fetch/src/discover.rs | 206 ++++++++++++++++++++++- crates/dependable/src/output/github.rs | 1 + crates/dependable/src/output/mod.rs | 10 ++ crates/dependable/src/output/sarif.rs | 1 + crates/dependable/src/runner.rs | 27 ++- crates/dependable/tests/fixture_swift.rs | 105 ++++++++++++ 6 files changed, 337 insertions(+), 13 deletions(-) diff --git a/crates/dependable-fetch/src/discover.rs b/crates/dependable-fetch/src/discover.rs index 1b77ede..225e1ad 100644 --- a/crates/dependable-fetch/src/discover.rs +++ b/crates/dependable-fetch/src/discover.rs @@ -110,6 +110,7 @@ fn walk( found.notices.push(LockfileNotice { path, reason: unreadable.reason.to_owned(), + dependency_list_unread: false, }); } } @@ -168,23 +169,46 @@ fn is_superseded(dir: &Path, unreadable: &UnreadableManifest) -> bool { } } +/// Whether `lockfile` may be adopted from `dir`, given the manifest sits in `own_dir`. +/// +/// The upward walk is safe while a lockfile *annotates* items a manifest already +/// produced: a workspace member's `Cargo.lock` at the root pins the very crates the +/// member declared, and a pin for a crate it does not declare simply goes unused. +/// +/// It is not safe when the lockfile **is** the item list. A nested SwiftPM package +/// with no `Package.resolved` of its own would adopt its ancestor's, and report the +/// ancestor's dependencies as its own — attributing advisories to a package that +/// does not have the dependency, which is a false positive on the one verdict Swift +/// can give. In a monorepo that is every not-yet-resolved package. So a dependency +/// source counts only in the manifest's own directory. +fn adoptable_from(lockfile: LockfileKind, dir: &Path, own_dir: &Path) -> bool { + !lockfile.is_dependency_source() || dir == own_dir +} + /// Locate the lockfile governing `manifest` without reading it. /// /// Same upward walk as [`find_lockfile`] — the manifest's own directory first, then /// each ancestor, stopping at a `.git` boundary — but it answers only "where is it", /// which is what callers that need a different parse of the same file want. +/// +/// A lockfile that is a dependency source rather than an annotation is accepted only +/// from the manifest's own directory; see [`adoptable_from`]. #[must_use] pub fn locate_lockfile(manifest: &Path, kind: ManifestKind) -> Option<(PathBuf, LockfileKind)> { let candidates = kind.lockfiles(); if candidates.is_empty() { return None; } - let mut dir = manifest.parent()?; + let own_dir = manifest.parent()?; + let mut dir = own_dir; loop { // A directory is searched for every candidate before moving up, so a // lockfile beside the manifest always beats one further away whichever // package manager wrote it. for lockfile in candidates { + if !adoptable_from(*lockfile, dir, own_dir) { + continue; + } let candidate = dir.join(lockfile.file_name()); if candidate.is_file() { return Some((candidate, *lockfile)); @@ -207,15 +231,22 @@ pub fn locate_lockfile(manifest: &Path, kind: ManifestKind) -> Option<(PathBuf, /// /// Returns the path and the parsed data, or `None` for manifest kinds that have no /// lockfile ([`ManifestKind::lockfiles`]) and when none is found. +/// +/// A lockfile that is a dependency source rather than an annotation is accepted only +/// from the manifest's own directory; see [`adoptable_from`]. #[must_use] pub fn find_lockfile(manifest: &Path, kind: ManifestKind) -> Option<(PathBuf, LockfileData)> { let candidates = kind.lockfiles(); if candidates.is_empty() { return None; } - let mut dir = manifest.parent()?; + let own_dir = manifest.parent()?; + let mut dir = own_dir; loop { for lockfile in candidates { + if !adoptable_from(*lockfile, dir, own_dir) { + continue; + } let candidate = dir.join(lockfile.file_name()); if let Ok(content) = std::fs::read_to_string(&candidate) && let Ok(parsed) = parse_lockfile_kind(*lockfile, &content) @@ -392,6 +423,16 @@ pub struct LockfileNotice { pub path: PathBuf, /// What is wrong with it, phrased for the person who has to fix it. pub reason: String, + /// Whether this file *is* the project's dependency list rather than an + /// annotation on one, so that failing to read it leaves the dependency list + /// itself unknown — not merely unannotated. + /// + /// Only SwiftPM's `Package.resolved` can set this today + /// ([`LockfileKind::is_dependency_source`]). A caller that gates an exit code + /// on "is everything here checked and current" has to treat it as a failure: + /// with the list unread there is nothing to check, and reporting nothing + /// wrong would assert exactly what was never established. + pub dependency_list_unread: bool, } impl std::fmt::Display for LockfileNotice { @@ -419,26 +460,55 @@ pub fn lockfile_notices(manifest: &Path, kind: ManifestKind) -> Vec format!("could not be read: {error}"), + let (reason, unread) = match std::fs::read_to_string(&path) { + Err(error) => (format!("could not be read: {error}"), source), Ok(content) => match parse_lockfile_kind(*lockfile, &content) { - Err(error) => format!("could not be parsed: {error}"), + Err(error) => (format!("could not be parsed: {error}"), source), + // The file read: it simply pins nothing with a version. The + // dependency list is as complete as it was ever going to be, so + // this is not the unread case even for a dependency source. Ok(data) if data.versions.is_empty() => { - "was read but records no versions".to_owned() + ("was read but records no versions".to_owned(), false) } Ok(_) => continue, }, }; - notices.push(LockfileNotice { path, reason }); + notices.push(LockfileNotice { + path, + reason, + dependency_list_unread: unread, + }); } notices @@ -598,6 +668,128 @@ mod tests { assert_eq!(path, nested.join("package-lock.json")); } + /// Restricting the walk to the manifest's own directory is right for the one + /// lockfile that *is* the dependency list, and wrong for every other. All five + /// annotating formats keep the ancestor walk they have always had: each one + /// here sits at a workspace root with the manifest a directory below, which is + /// how a monorepo is actually laid out. + #[test] + fn an_annotating_lockfile_is_still_adopted_from_an_ancestor() { + let cases = [ + ( + ManifestKind::CargoToml, + "Cargo.toml", + LockfileKind::CargoLock, + "[package]\nname = \"member\"\n", + "[[package]]\nname = \"serde\"\nversion = \"1.0.1\"\n", + ), + ( + ManifestKind::PackageJson, + "package.json", + LockfileKind::PackageLockJson, + "{}", + r#"{"packages":{"node_modules/left-pad":{"version":"1.3.0"}}}"#, + ), + ( + ManifestKind::PackageJson, + "package.json", + LockfileKind::BunLock, + "{}", + r#"{"packages":{"left-pad":["left-pad@1.3.0","",{},""]}}"#, + ), + ( + ManifestKind::ComposerJson, + "composer.json", + LockfileKind::ComposerLock, + "{}", + r#"{"packages":[{"name":"monolog/monolog","version":"2.9.1"}]}"#, + ), + ( + ManifestKind::MixExs, + "mix.exs", + LockfileKind::MixLock, + "defmodule M do\nend\n", + "%{\n \"jason\": {:hex, :jason, \"1.4.1\", \"abc\", [:mix], [], \"hexpm\", \"def\"},\n}\n", + ), + ]; + + for (manifest_kind, manifest_name, lock_kind, manifest_body, lock_body) in cases { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + std::fs::create_dir_all(root.join(".git")).expect("mkdir .git"); + write(&root.join(lock_kind.file_name()), lock_body); + let manifest = root.join("packages/app").join(manifest_name); + write(&manifest, manifest_body); + + let located = locate_lockfile(&manifest, manifest_kind); + assert_eq!( + located, + Some((root.join(lock_kind.file_name()), lock_kind)), + "{lock_kind:?} must still be found in an ancestor" + ); + + let (path, data) = find_lockfile(&manifest, manifest_kind) + .unwrap_or_else(|| panic!("{lock_kind:?} must still be read from an ancestor")); + assert_eq!(path, root.join(lock_kind.file_name())); + assert!(!data.versions.is_empty(), "{lock_kind:?}: {data:?}"); + } + } + + /// The one lockfile that *is* the dependency list is accepted only beside its + /// own manifest. A nested SwiftPM package that adopted the root's would report + /// the root's dependencies as its own — and, with scanning on, the root's + /// advisories against a package that does not have the dependency. + #[test] + fn a_dependency_source_lockfile_is_never_adopted_from_an_ancestor() { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + std::fs::create_dir_all(root.join(".git")).expect("mkdir .git"); + let resolved = r#"{"pins":[{"identity":"vapor","kind":"remoteSourceControl", + "location":"https://github.com/vapor/vapor.git", + "state":{"revision":"abc","version":"4.83.0"}}],"version":2}"#; + write(&root.join("Package.resolved"), resolved); + write(&root.join("Package.swift"), "// swift-tools-version:5.9\n"); + + let nested = root.join("Examples/Demo/Package.swift"); + write(&nested, "// swift-tools-version:5.9\n"); + + assert_eq!( + locate_lockfile(&nested, ManifestKind::PackageSwift), + None, + "the ancestor's pins are not this package's dependencies" + ); + assert!(find_lockfile(&nested, ManifestKind::PackageSwift).is_none()); + + // Beside its own manifest it is read exactly as before. + assert_eq!( + locate_lockfile(&root.join("Package.swift"), ManifestKind::PackageSwift), + Some((root.join("Package.resolved"), LockfileKind::PackageResolved)) + ); + } + + /// The nested package's own `Package.resolved` is missing, not merely + /// unannotated, and that has to reach the reader — nothing else distinguishes + /// it from a package that genuinely depends on nothing. + #[test] + fn a_missing_dependency_source_is_a_notice_of_its_own() { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + write(&root.join("Package.swift"), "// swift-tools-version:5.9\n"); + + let notices = lockfile_notices(&root.join("Package.swift"), ManifestKind::PackageSwift); + assert_eq!(notices.len(), 1, "{notices:?}"); + assert_eq!(notices[0].path, root.join("Package.resolved")); + assert!(notices[0].dependency_list_unread); + + // A missing *annotating* lockfile is not a notice: the manifest still says + // what the project depends on. + write(&root.join("Cargo.toml"), "[package]\nname = \"x\"\n"); + assert!( + lockfile_notices(&root.join("Cargo.toml"), ManifestKind::CargoToml).is_empty(), + "a missing Cargo.lock costs a locked version, not a dependency list" + ); + } + #[test] fn a_binary_bun_lockfile_is_reported_rather_than_ignored() { // The state this exists for: a lockfile is right there, and the project diff --git a/crates/dependable/src/output/github.rs b/crates/dependable/src/output/github.rs index b2e5e26..df18537 100644 --- a/crates/dependable/src/output/github.rs +++ b/crates/dependable/src/output/github.rs @@ -815,6 +815,7 @@ mod tests { ecosystem: Ecosystem::Rust, results, workspace_root: None, + dependencies_unread: false, } } diff --git a/crates/dependable/src/output/mod.rs b/crates/dependable/src/output/mod.rs index e556afb..f65644b 100644 --- a/crates/dependable/src/output/mod.rs +++ b/crates/dependable/src/output/mod.rs @@ -25,6 +25,15 @@ pub struct ManifestReport { /// The manifest whose `[workspace.dependencies]` supplied any inherited constraint. /// `None` outside a workspace. pub workspace_root: Option, + /// Whether the file that *is* this project's dependency list went unread, so + /// [`Self::results`] being empty says nothing about the project. + /// + /// Only a SwiftPM project can set this: a `Package.swift` is a program this + /// tool declines to read, so with no readable `Package.resolved` beside it + /// there is no dependency list at all. `--fail-on any` reads it, because + /// "nothing was found wrong" and "nothing was looked at" must not share an + /// exit code. + pub dependencies_unread: bool, } /// Aggregate status counts across one or more reports. @@ -185,6 +194,7 @@ mod tests { }) .collect(), workspace_root: None, + dependencies_unread: false, } } diff --git a/crates/dependable/src/output/sarif.rs b/crates/dependable/src/output/sarif.rs index 112c81b..718948a 100644 --- a/crates/dependable/src/output/sarif.rs +++ b/crates/dependable/src/output/sarif.rs @@ -84,6 +84,7 @@ mod tests { ecosystem: Ecosystem::Rust, results: Vec::new(), workspace_root: None, + dependencies_unread: false, } } diff --git a/crates/dependable/src/runner.rs b/crates/dependable/src/runner.rs index af823c9..c0e2a6c 100644 --- a/crates/dependable/src/runner.rs +++ b/crates/dependable/src/runner.rs @@ -258,7 +258,7 @@ impl Engine { /// has no registered checker or no parser yet — so a polyglot repo with a /// not-yet-supported manifest does not abort the whole run. async fn check_manifest(&self, path: &Path) -> anyhow::Result> { - report_lockfile_notices(path); + let dependencies_unread = report_lockfile_notices(path); match self.checker.check_path(path).await { Ok(check) => { for warning in &check.warnings { @@ -269,6 +269,7 @@ impl Engine { ecosystem: check.ecosystem, results: check.results, workspace_root: check.workspace_root, + dependencies_unread, })) } Err(CheckError::UnsupportedEcosystem(eco)) => { @@ -292,18 +293,26 @@ impl Engine { } } -/// Warn about lockfiles that are present beside `manifest` but cannot be used. +/// Warn about lockfiles that are present beside `manifest` but cannot be used — +/// or, for the one format that *is* the dependency list, absent altogether. /// /// Without this a `bun.lockb` is silently skipped and every dependency is /// reported unlocked, with nothing to tell the user that a lockfile they can /// migrate is the reason. -fn report_lockfile_notices(manifest: &Path) { +/// +/// Returns whether any notice means the project's dependency list itself went +/// unread, which the caller has to carry into the exit code: a run that knows +/// nothing about a project must not report it clean. +fn report_lockfile_notices(manifest: &Path) -> bool { let Some(kind) = ManifestKind::detect(manifest) else { - return; + return false; }; + let mut unread = false; for notice in dependable_fetch::lockfile_notices(manifest, kind) { eprintln!("warning: {notice}"); + unread |= notice.dependency_list_unread; } + unread } /// A progress sink that drives a per-manifest indicatif bar. Each manifest's @@ -605,7 +614,7 @@ pub async fn run_list(args: ListArgs) -> anyhow::Result { let Some(kind) = ManifestKind::detect(manifest) else { continue; }; - report_lockfile_notices(manifest); + let _ = report_lockfile_notices(manifest); let content = std::fs::read_to_string(manifest) .with_context(|| format!("reading {}", manifest.display()))?; let mut parsed = match parse(kind, &content) { @@ -1398,7 +1407,13 @@ fn exit_code(reports: &[ManifestReport], fail_on: FailOn) -> ExitCode { DependencyStatus::UpToDate | DependencyStatus::Local | DependencyStatus::Git ), }); - if triggered { + // A manifest whose dependency list was never read has no results to inspect, + // so the loop above sees an empty list and finds nothing wrong with it. That + // is the inversion in its purest form: zero rows read as a clean project. Only + // `--fail-on any` asks the question this answers — `vulnerable` and `outdated` + // ask about findings, and there are none to have. + let unread = fail_on == FailOn::Any && reports.iter().any(|r| r.dependencies_unread); + if triggered || unread { ExitCode::from(1) } else { ExitCode::SUCCESS diff --git a/crates/dependable/tests/fixture_swift.rs b/crates/dependable/tests/fixture_swift.rs index 0e144b8..60080a0 100644 --- a/crates/dependable/tests/fixture_swift.rs +++ b/crates/dependable/tests/fixture_swift.rs @@ -355,6 +355,111 @@ fn scratch(name: &str) -> PathBuf { dir } +/// A nested SwiftPM package must never adopt an ancestor's `Package.resolved`. +/// +/// The upward walk is right for the five lockfiles that *annotate* a list some +/// manifest declared — an unused pin costs nothing. It is wrong for the one that +/// **is** the list: the nested package here declares neither of the root's +/// dependencies, and adopting them reports the root's packages as its own. With +/// scanning on that attributes the root's advisories to a project that does not +/// have the dependency — a false positive on the one verdict Swift can give — and +/// in a SwiftPM monorepo it happens to every package not yet resolved. +#[test] +fn a_nested_package_does_not_adopt_an_ancestors_package_resolved() { + let root = scratch("swift_monorepo"); + let nested = root.join("Examples/Demo"); + std::fs::create_dir_all(&nested).unwrap(); + let manifest = std::fs::read_to_string(fixture("sample-swift/Package.swift")).unwrap(); + std::fs::write(root.join("Package.swift"), &manifest).unwrap(); + std::fs::copy( + fixture("sample-swift/Package.resolved"), + root.join("Package.resolved"), + ) + .unwrap(); + std::fs::write(nested.join("Package.swift"), &manifest).unwrap(); + + let output = run(&[ + "check", + "--manifest", + nested.join("Package.swift").to_str().unwrap(), + "--no-vuln", + ]); + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + !stdout.contains("github.com/apple/swift-nio"), + "the nested package declares none of the root's dependencies; stdout: {stdout}" + ); + assert!( + stderr.contains("Package.resolved") && stderr.contains("is not here"), + "and it must say the list is unknown rather than empty; stderr: {stderr}" + ); + + // The root itself is unaffected: its own Package.resolved sits beside it. + let output = run(&[ + "check", + "--manifest", + root.join("Package.swift").to_str().unwrap(), + "--no-vuln", + ]); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("github.com/apple/swift-nio"), + "stdout: {stdout}" + ); +} + +/// Apple advises library packages *not* to commit `Package.resolved`, so a Swift +/// project with none is the common state rather than an edge. Nothing about it may +/// read as "this project has no dependencies": `Package.swift` is a program this +/// tool declines to read, so the list was never seen, and `--fail-on any` — which +/// asks whether everything here is checked and current — must not answer yes. +#[test] +fn a_swift_project_with_no_package_resolved_says_so_and_fails_a_strict_gate() { + let dir = scratch("swift_no_resolved"); + std::fs::copy( + fixture("sample-swift/Package.swift"), + dir.join("Package.swift"), + ) + .unwrap(); + let manifest = dir.join("Package.swift"); + + let output = run(&[ + "check", + "--manifest", + manifest.to_str().unwrap(), + "--no-vuln", + ]); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("is not here") && stderr.contains("`swift package resolve`"), + "the cause must be named; stderr: {stderr}" + ); + assert!( + !stderr.contains("0 dependencies here"), + "\"we could not look\" must not be phrased as a count of what is here; \ + stderr: {stderr}" + ); + assert!( + output.status.success(), + "no gate was asked for; stderr: {stderr}" + ); + + let strict = run(&[ + "check", + "--manifest", + manifest.to_str().unwrap(), + "--no-vuln", + "--fail-on", + "any", + ]); + assert!( + !strict.status.success(), + "exit 0 would assert that a list nobody read is clean; stdout: {}", + String::from_utf8_lossy(&strict.stdout) + ); +} + /// A half-written `Package.resolved` used to crash the process outright, and the /// degradation waiting behind that crash was worse: a *prefix* of the pins, /// presented as the whole dependency list, with the packages past the cut never From 44d44949293da3b903c0d544356db6c4c18d3d83 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 01:20:21 -0400 Subject: [PATCH 12/24] docs(readme): say what a Swift check leaves unestablished Three things the Swift section did not say, each now visible in the tool's own output: no pin is reported as a direct dependency, because `Package.resolved` records the flattened resolution and marks none apart; a `Package.swift` with no readable `Package.resolved` beside it reports as unknown rather than as zero dependencies, and fails `--fail-on any`; and a `Package.resolved` counts only in its own directory, so a nested package never adopts the root's pins. Also records the one limitation that remains: OSV keys `SwiftURL` advisories case-sensitively while a git forge does not, so a lowercase spelling in the file of a repository whose advisory is keyed mixed-case matches nothing. The host is lowercased and the all-lowercase path is queried alongside the written one, which covers every direction but that. --- README.md | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index d5542ef..d45c914 100644 --- a/README.md +++ b/README.md @@ -72,7 +72,27 @@ is where every Swift dependency comes from: the one lockfile here that is the de list rather than an annotation on one. SwiftPM records the *flattened* resolution there and does not mark which pins are direct, so a Swift project lists its transitive dependencies alongside its direct ones — which is more than every other ecosystem shows, -not less. +not less. Because the file cannot say which is which, no pin is reported as a direct +dependency: `list` marks every one `(indirect)`, and `--format json` gives it +`"direct": false`. + +Being the dependency list rather than an annotation on one has two more consequences. +A `Package.resolved` counts only in the manifest's own directory — a nested package in +a monorepo never adopts the root's pins, which would report the root's dependencies, +and the root's advisories, against a package that has neither. And a `Package.swift` +with no readable `Package.resolved` beside it — a missing one (Apple advises library +packages not to commit theirs) or a malformed one — is reported as *unknown*, never as +zero dependencies: a warning names the cause, and `--fail-on any` exits non-zero, +because nothing was established about that project at all. + +One limitation remains, and it is worth stating. OSV matches its `SwiftURL` keys +case-sensitively while a git forge does not, and real keys are mixed-case +(`github.com/weichsel/ZIPFoundation`). `dependable` lowercases the host, keeps the +repository path exactly as `Package.resolved` wrote it, and additionally queries the +all-lowercase spelling. That covers every direction but one: a `Package.resolved` +recording a lowercase spelling of a repository whose advisory is keyed under mixed case +matches nothing, and the package reports clean. Recovering the canonical casing needs +the forge, not the file. ### Lockfiles From 53586856b093f4b8e3703fbdb09c196e88aacbba Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 16:44:11 -0400 Subject: [PATCH 13/24] fix(core): strip a port from a Swift package host MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OSV keys a Swift package by `github.com/apple/swift-nio`, with no port. A pin written `ssh://git@github.com:22/apple/swift-nio.git` would otherwise ask about `github.com:22/apple/swift-nio` — a key OSV has never heard of — so the same repository at the same version comes back clean through one URL and vulnerable through another. A port addresses the transport, not the package. Only an all-digit suffix counts, which is the test the SCP-shorthand branch of `swift_package_name` already applies: `github.com:vapor` stays a path. An IPv6 literal is unharmed, since `[::1]` ends in `]` rather than a digit, while `[::1]:22` loses only the port. --- .../src/lockfiles/swift_package_resolved.rs | 86 ++++++++++++++++++- 1 file changed, 83 insertions(+), 3 deletions(-) diff --git a/crates/dependable-core/src/lockfiles/swift_package_resolved.rs b/crates/dependable-core/src/lockfiles/swift_package_resolved.rs index 9fb35ea..ff179aa 100644 --- a/crates/dependable-core/src/lockfiles/swift_package_resolved.rs +++ b/crates/dependable-core/src/lockfiles/swift_package_resolved.rs @@ -165,11 +165,35 @@ pub fn swift_package_name(location: &str) -> String { lowercase_host(name.trim_end_matches('/')) } -/// Lowercase the host component of an OSV `SwiftURL` name, leaving the path alone. +/// Normalize the host component of an OSV `SwiftURL` name, leaving the path alone. +/// +/// Lowercased, for the reason [`swift_package_name`] gives, and stripped of any +/// port: `ssh://git@github.com:22/apple/swift-nio.git` and +/// `https://github.com/apple/swift-nio.git` address the same repository, but only +/// the second spells the key OSV holds. A port is transport, not identity, and +/// leaving it on is the same silent false negative a mis-cased host is — the query +/// matches nothing and the package is reported clean. fn lowercase_host(name: &str) -> String { match name.split_once('/') { - Some((host, path)) => format!("{}/{path}", host.to_ascii_lowercase()), - None => name.to_ascii_lowercase(), + Some((host, path)) => format!("{}/{path}", strip_port(host).to_ascii_lowercase()), + None => strip_port(name).to_ascii_lowercase(), + } +} + +/// `host` without a trailing `:`. +/// +/// Only an all-digit suffix is a port, which is the same test the SCP-shorthand +/// branch of [`swift_package_name`] already applies: `github.com:vapor` is a path +/// and keeps its colon here too. An IPv6 literal is unharmed — `[::1]` ends in `]`, +/// not a digit — while `[::1]:22` loses only the port. +fn strip_port(host: &str) -> &str { + match host.rsplit_once(':') { + Some((rest, port)) + if !rest.is_empty() && !port.is_empty() && port.bytes().all(|b| b.is_ascii_digit()) => + { + rest + } + _ => host, } } @@ -492,6 +516,62 @@ mod tests { } } + /// A port addresses the transport, not the package. OSV keys + /// `github.com/apple/swift-nio`, so a pin written `ssh://git@github.com:22/…` + /// would otherwise ask about `github.com:22/apple/swift-nio` — a key OSV has + /// never heard of — and the same repository at the same version would come back + /// clean through one URL and vulnerable through another. + #[test] + fn a_port_is_stripped_from_the_host() { + let cases = [ + ( + "ssh://git@github.com:22/apple/swift-nio.git", + "github.com/apple/swift-nio", + ), + ( + "https://github.com:443/apple/swift-nio.git", + "github.com/apple/swift-nio", + ), + ( + "git://GitHub.com:9418/apple/swift-nio", + "github.com/apple/swift-nio", + ), + // No scheme: `:22` is read as a port by the SCP-shorthand branch, so + // the path starts after it. + ( + "github.com:22/apple/swift-nio.git", + "github.com/apple/swift-nio", + ), + // The port is transport only; a mixed-case path still survives it. + ( + "ssh://git@github.com:22/weichsel/ZIPFoundation.git", + "github.com/weichsel/ZIPFoundation", + ), + ]; + for (location, expected) in cases { + assert_eq!(swift_package_name(location), expected, "{location}"); + } + } + + /// The strip is an all-digit suffix and nothing else, so a colon that is part of + /// a name — SCP shorthand, an IPv6 literal — keeps it. + #[test] + fn a_colon_that_is_not_a_port_survives() { + let cases = [ + // SCP shorthand: the colon becomes the path separator, not a port. + ("git@github.com:vapor/vapor.git", "github.com/vapor/vapor"), + // An IPv6 literal ends in `]`, never a digit. + ("https://[::1]/apple/swift-nio.git", "[::1]/apple/swift-nio"), + ( + "ssh://git@[::1]:22/apple/swift-nio", + "[::1]/apple/swift-nio", + ), + ]; + for (location, expected) in cases { + assert_eq!(swift_package_name(location), expected, "{location}"); + } + } + /// OSV's `SwiftURL` keys are matched byte for byte, and the real ones are /// mixed-case: `github.com/weichsel/ZIPFoundation`, `github.com/marmelroy/Zip`, /// `github.com/migueldeicaza/SwiftTerm`. Lowercasing the path would turn every From c0f8c8620c0659e213e8546889ee26afb1fbabe6 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 16:44:16 -0400 Subject: [PATCH 14/24] fix(cli): let --no-lock-file suppress annotations, not the list itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--no-lock-file` is documented as "ignore sibling lockfiles (do not report locked versions)": it suppresses the `locked_at` column, an annotation on a list the manifest already produced. A `Package.resolved` is not that — it *is* the list, because a `Package.swift` is a program this tool declines to read. Honouring the flag there did not withhold a version column, it reported a Swift project as depending on nothing at all, which is the inversion this ecosystem's support exists to prevent. `apply_nearest_lockfile` now takes the flag itself and applies it to the annotating half only, so a dependency-source lockfile is read regardless and the flag keeps exactly the meaning its help text claims. --- crates/dependable/src/cli.rs | 4 +++- crates/dependable/src/runner.rs | 20 +++++++++++++++++--- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/crates/dependable/src/cli.rs b/crates/dependable/src/cli.rs index c9e3082..6c9972e 100644 --- a/crates/dependable/src/cli.rs +++ b/crates/dependable/src/cli.rs @@ -138,7 +138,9 @@ pub struct ListArgs { /// How many directories deep to search. #[arg(long, default_value_t = 3)] pub depth: usize, - /// Ignore sibling lockfiles (do not report locked versions). + /// Ignore sibling lockfiles (do not report locked versions). A lockfile that + /// *is* the dependency list rather than an annotation on one — SwiftPM's + /// `Package.resolved` — is still read, or the project would list nothing. #[arg(long)] pub no_lock_file: bool, /// Show each crate's available feature flags (Rust only; fetches the diff --git a/crates/dependable/src/runner.rs b/crates/dependable/src/runner.rs index a530537..385b029 100644 --- a/crates/dependable/src/runner.rs +++ b/crates/dependable/src/runner.rs @@ -659,9 +659,8 @@ pub async fn run_list(args: ListArgs) -> anyhow::Result { resolve_workspace_inheritance(&mut parsed.items, &declarations) }) .unwrap_or_default(); - let lockfile = (!args.no_lock_file) - .then(|| apply_nearest_lockfile(manifest, kind, &root, &mut parsed.items)) - .flatten(); + let lockfile = + apply_nearest_lockfile(manifest, kind, &root, !args.no_lock_file, &mut parsed.items); let meta = parse_project(kind, &content); let (version, version_inherited) = resolve_version(manifest, kind, &meta); @@ -702,10 +701,22 @@ pub async fn run_list(args: ListArgs) -> anyhow::Result { /// found by walking up. The walk stops at the repository root (the first ancestor /// holding a `.git`) so a stray lockfile outside the project is never read, and the /// path that was used is reported rather than assumed. +/// +/// # `annotations` +/// `--no-lock-file` clears this, and it governs **only** the annotating half. The flag +/// is documented as "ignore sibling lockfiles (do not report locked versions)": it +/// suppresses the `locked_at` column, which is an annotation on a list the manifest +/// already produced. A `Package.resolved` is not that — it *is* the list, because a +/// `Package.swift` is a program this tool declines to read. Honouring the flag there +/// would not withhold a version column, it would report a Swift project as depending +/// on nothing at all, which is the inversion this ecosystem's support exists to +/// prevent. So a dependency-source lockfile is read regardless, and the flag keeps +/// exactly the meaning its help text claims. fn apply_nearest_lockfile( manifest: &Path, kind: ManifestKind, root: &Path, + annotations: bool, items: &mut Vec, ) -> Option { // One lockfile *is* the dependency list rather than an annotation on one: a @@ -719,6 +730,9 @@ fn apply_nearest_lockfile( items.extend(lockfile_items(lock_kind, &content)?); return Some(relative_to(root, &path)); } + if !annotations { + return None; + } let (path, resolved) = dependable_fetch::find_lockfile(manifest, kind)?; apply_lockfile(items, &resolved); Some(relative_to(root, &path)) From 06bcd6d70df159eacb0f1f9ede78d312d4783d70 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 16:44:26 -0400 Subject: [PATCH 15/24] feat(report): say when a project's dependency list went unread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An empty `results` array is what a clean project and an unread one share. Every status count tallies rows that *were* read, so a manifest nothing was read from contributes zero to all of them and its document is otherwise byte-identical to a genuinely clean project's — the report asserts "no findings" about a project it learned nothing about. Only a SwiftPM project can reach this state today: a `Package.swift` is a program the checker declines to read, so with no readable `Package.resolved` beside it there is no dependency list at all. `ManifestResults` carries the fact, `Summary` counts it as `manifests_unread`, `check --format json` exposes that count, and SARIF gains `DEP003` — emitted per *manifest* rather than per dependency, precisely because there are no dependencies to emit one against, and pushed before that manifest's rows so a consumer reading the array in order meets the caveat first. Both JSON changes are additive: a consumer pinned to the documented shape is unaffected, and one that wants the distinction gates on `manifests_unread > 0`. --- crates/dependable-report/src/model.rs | 25 +++++- crates/dependable-report/src/sarif.rs | 108 +++++++++++++++++++++++--- crates/dependable/src/output/json.rs | 11 +++ crates/dependable/src/output/mod.rs | 10 +++ crates/dependable/src/output/sarif.rs | 16 ++-- crates/dependable/tests/cli_sarif.rs | 3 +- 6 files changed, 155 insertions(+), 18 deletions(-) diff --git a/crates/dependable-report/src/model.rs b/crates/dependable-report/src/model.rs index d41bdb3..8d8b008 100644 --- a/crates/dependable-report/src/model.rs +++ b/crates/dependable-report/src/model.rs @@ -76,18 +76,41 @@ pub struct ManifestResults { pub ecosystem: Ecosystem, /// One result per declared dependency. pub results: Vec, + /// Whether the file that *is* this project's dependency list went unread, so + /// [`Self::results`] being empty says nothing about the project. + /// + /// Only a SwiftPM project can set this: a `Package.swift` is a program the + /// checker declines to read, so with no readable `Package.resolved` beside it + /// there is no dependency list at all. A renderer that omits this reports a + /// project nothing was read from exactly as it reports a clean one. + /// + /// `false` from [`ManifestResults::new`]; set it with + /// [`ManifestResults::with_dependencies_unread`]. + pub dependencies_unread: bool, } impl ManifestResults { - /// The results for one manifest. + /// The results for one manifest, with its dependency list assumed read. #[must_use] pub fn new(path: PathBuf, ecosystem: Ecosystem, results: Vec) -> Self { Self { path, ecosystem, results, + dependencies_unread: false, } } + + /// Record whether this manifest's dependency list went unread. + /// + /// See [`ManifestResults::dependencies_unread`]. A builder rather than a + /// constructor argument so callers that cannot produce the flag keep the + /// two-line construction they have. + #[must_use] + pub fn with_dependencies_unread(mut self, unread: bool) -> Self { + self.dependencies_unread = unread; + self + } } #[cfg(test)] diff --git a/crates/dependable-report/src/sarif.rs b/crates/dependable-report/src/sarif.rs index caf3eb3..ff20de2 100644 --- a/crates/dependable-report/src/sarif.rs +++ b/crates/dependable-report/src/sarif.rs @@ -6,7 +6,7 @@ //! //! # Public surface //! -//! [`render`], [`DEP001`] and [`DEP002`]. Everything else in this module is +//! [`render`], [`DEP001`], [`DEP002`] and [`DEP003`]. Everything else in this module is //! private on purpose: the SARIF document shape is an output artifact, not an //! API, and publishing the structs would freeze the JSON layout as semver //! surface. @@ -17,10 +17,13 @@ //! | --- | --- | --- | //! | [`DEP001`] | a newer version of the dependency is available | `warning` | //! | [`DEP002`] | the version in use is affected by a known advisory | `error` | +//! | [`DEP003`] | the project's dependency list could not be read | `warning` | //! //! One result is emitted **per advisory ID**, so each CVE becomes its own alert //! carrying its own `properties.cvssScore`, and one result per dependency for -//! [`DEP001`]. `DEP003` onwards are unused and free for later rules. +//! [`DEP001`]. [`DEP003`] is per *manifest*, not per dependency — it is emitted +//! precisely because there are no dependencies to emit one against. `DEP004` +//! onwards are unused and free for later rules. //! //! # Purity and determinism //! @@ -75,6 +78,14 @@ pub const DEP001: &str = "DEP001"; /// The rule ID for a dependency affected by a known advisory. pub const DEP002: &str = "DEP002"; +/// The rule ID for a manifest whose dependency list could not be read. +/// +/// Unlike [`DEP001`] and [`DEP002`] this describes the *manifest*, not a +/// dependency in it: an empty `results` array is what a clean project and an +/// unread one otherwise share, so the finding that distinguishes them cannot +/// itself be a per-dependency one. +pub const DEP003: &str = "DEP003"; + /// The SARIF schema this renderer targets. const SCHEMA: &str = "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json"; @@ -149,6 +160,12 @@ fn findings(report: &Report) -> Vec { for manifest in &report.manifests { let uri = uri_for(&report.root, &manifest.path); let ecosystem = manifest.ecosystem; + // First, and unconditional on there being results — it is emitted precisely + // because there are none, and a consumer scanning the array in order meets + // the caveat before the rows it qualifies. + if manifest.dependencies_unread { + findings.push(unread_finding(ecosystem.osv_name(), &uri)); + } for result in &manifest.results { let line = start_line(&result.item); match result.status { @@ -250,9 +267,9 @@ fn vulnerable_finding( start_line, fingerprint: fingerprint(DEP002, ecosystem, &result.item.name, id, uri), properties: ResultProperties { - package: result.item.name.clone(), + package: Some(result.item.name.clone()), ecosystem, - current_version: current, + current_version: Some(current), latest_version: latest_version(result), status: result.status.token(), advisory_id: Some(id.to_string()), @@ -303,9 +320,9 @@ fn outdated_finding( uri, ), properties: ResultProperties { - package: result.item.name.clone(), + package: Some(result.item.name.clone()), ecosystem, - current_version: current, + current_version: Some(current), latest_version: latest, status: result.status.token(), advisory_id: None, @@ -321,6 +338,49 @@ fn outdated_finding( } } +/// A [`DEP003`] finding for one manifest whose dependency list went unread. +/// +/// Level `warning` rather than `error`: nothing is known to be wrong, which is +/// exactly the point — the run established nothing about this project, and a +/// consumer must not read the absence of findings as their absence in fact. +/// +/// No `region`: the missing information is a *file that is not there*, so there is +/// no line in the manifest to point at. The `artifactLocation` still names the +/// manifest, which SARIF permits — see [`start_line`]. +fn unread_finding(ecosystem: &'static str, uri: &str) -> Finding { + Finding { + rule_id: DEP003, + rule_index: 2, + level: Level::Warning, + message: format!( + "The dependency list for `{uri}` could not be read, so no dependency in it was checked. An empty result set for this manifest means nothing was looked at, not that nothing is wrong." + ), + uri: uri.to_string(), + start_line: None, + // No package to key on, so the manifest is the identity: one alert per + // manifest, reopened only if the same manifest goes unread again. + fingerprint: format!("{DEP003}:{ecosystem}:{uri}"), + properties: ResultProperties { + package: None, + ecosystem, + current_version: None, + latest_version: None, + // Not a `DependencyStatus` token — no dependency has this status, + // because no dependency was read. The manifest does. + status: "unread", + advisory_id: None, + cvss_score: None, + severity: None, + severity_label: None, + cvss_vector: None, + fixed_versions: Vec::new(), + aliases: Vec::new(), + cwe_ids: Vec::new(), + advisory_url: None, + }, + } +} + /// The severity band the level follows: a computed CVSS score wins over the /// publisher's band, which wins over nothing. /// @@ -499,7 +559,7 @@ fn build(findings: &[Finding]) -> SarifLog { /// The full rule catalogue, always emitted whether or not a rule fired — a /// consumer reading `tool.driver.rules` learns what this tool can report. -fn rules() -> [ReportingDescriptor; 2] { +fn rules() -> [ReportingDescriptor; 3] { [ ReportingDescriptor { id: DEP001, @@ -545,6 +605,25 @@ fn rules() -> [ReportingDescriptor; 2] { security_severity: Some(DEP002_SECURITY_SEVERITY), }, }, + ReportingDescriptor { + id: DEP003, + name: "UnreadDependencyList", + short_description: Text::new("The project's dependency list could not be read."), + full_description: Text::new( + "The file that is this project's dependency list — a SwiftPM `Package.resolved` — is missing or unreadable, and its manifest declares no dependencies of its own. No dependency was checked, so the absence of other findings for this manifest says nothing about the project.", + ), + help: Text::new( + "Resolve the project (`swift package resolve`) and commit the resulting `Package.resolved`, or repair the existing one.", + ), + help_uri: INFORMATION_URI, + default_configuration: RuleConfig { + level: Level::Warning, + }, + properties: RuleProperties { + tags: &["dependencies", "coverage"], + security_severity: None, + }, + }, ] } @@ -592,7 +671,7 @@ struct ToolComponent { version: &'static str, semantic_version: &'static str, information_uri: &'static str, - rules: [ReportingDescriptor; 2], + rules: [ReportingDescriptor; 3], } /// One rule in the catalogue. @@ -727,10 +806,15 @@ enum Level { #[derive(Serialize, Clone)] #[serde(rename_all = "camelCase")] struct ResultProperties { - package: String, + /// Absent only for [`DEP003`], which is about the manifest rather than any one + /// dependency in it. Every other finding names its package. + #[serde(skip_serializing_if = "Option::is_none")] + package: Option, /// The OSV ecosystem name (`crates.io`, `npm`, …), the machine-readable form. ecosystem: &'static str, - current_version: String, + /// Absent for [`DEP003`], for the same reason `package` is. + #[serde(skip_serializing_if = "Option::is_none")] + current_version: Option, #[serde(skip_serializing_if = "Option::is_none")] latest_version: Option, status: &'static str, @@ -848,11 +932,13 @@ mod tests { assert_eq!(driver["informationUri"], INFORMATION_URI); let rules = driver["rules"].as_array().expect("rules"); - assert_eq!(rules.len(), 2); + assert_eq!(rules.len(), 3); assert_eq!(rules[0]["id"], DEP001); assert_eq!(rules[1]["id"], DEP002); + assert_eq!(rules[2]["id"], DEP003); assert_eq!(rules[0]["defaultConfiguration"]["level"], "warning"); assert_eq!(rules[1]["defaultConfiguration"]["level"], "error"); + assert_eq!(rules[2]["defaultConfiguration"]["level"], "warning"); assert_eq!(rules[1]["properties"]["security-severity"], "7.0"); // Present and empty: an *absent* `results` would mean the run failed. diff --git a/crates/dependable/src/output/json.rs b/crates/dependable/src/output/json.rs index 47f3696..7a4d60f 100644 --- a/crates/dependable/src/output/json.rs +++ b/crates/dependable/src/output/json.rs @@ -31,6 +31,16 @@ struct SummaryDto { /// its ``, an unresolved workspace inheritance. Additive, and /// deliberately not folded into `error`: nothing failed, nothing was asked. undetermined: usize, + /// How many manifests had their dependency list go unread — a `Package.swift` + /// with no readable `Package.resolved` beside it. + /// + /// The key that separates "read nothing" from "genuinely empty". Every other + /// count here tallies rows that were read, so a manifest nothing was read from + /// contributes zero to all of them and its document is otherwise identical to a + /// clean one's. Additive, like `manifests` and `unique_packages`: a consumer + /// pinned to the documented shape is unaffected, and one that wants the + /// distinction gates on `manifests_unread > 0`. + manifests_unread: usize, } #[derive(Serialize)] @@ -99,6 +109,7 @@ pub fn render(reports: &[ManifestReport]) -> anyhow::Result<()> { vulnerable: summary.vulnerable, error: summary.error, undetermined: summary.undetermined, + manifests_unread: summary.manifests_unread, }, results, }; diff --git a/crates/dependable/src/output/mod.rs b/crates/dependable/src/output/mod.rs index f65644b..a93fef2 100644 --- a/crates/dependable/src/output/mod.rs +++ b/crates/dependable/src/output/mod.rs @@ -69,6 +69,15 @@ pub struct Summary { /// [`git`](Self::git), which are deliberately skipped and therefore clean, /// because these were not skipped on purpose — nothing was learned about them. pub undetermined: usize, + /// How many of the [`manifests`](Self::manifests) had their dependency list go + /// unread — [`ManifestReport::dependencies_unread`]. + /// + /// The counter that stops a machine-readable report saying "clean" about a + /// project nothing was read from. Every status count above is a tally of rows + /// that *were* read, so all of them are zero for such a manifest and the + /// document is byte-identical to a genuinely empty one. Non-zero here is the + /// only thing in the summary that separates the two. + pub manifests_unread: usize, } impl Summary { @@ -98,6 +107,7 @@ impl Summary { } } s.manifests = reports.len(); + s.manifests_unread = reports.iter().filter(|r| r.dependencies_unread).count(); s.unique_packages = unique.len(); s } diff --git a/crates/dependable/src/output/sarif.rs b/crates/dependable/src/output/sarif.rs index 718948a..de0fc5d 100644 --- a/crates/dependable/src/output/sarif.rs +++ b/crates/dependable/src/output/sarif.rs @@ -19,11 +19,17 @@ use super::ManifestReport; pub fn render(reports: &[ManifestReport]) -> anyhow::Result<()> { let mut report = Report::new(scan_root(reports)); for manifest in reports { - report.push(ManifestResults::new( - manifest.path.clone(), - manifest.ecosystem, - manifest.results.clone(), - )); + report.push( + ManifestResults::new( + manifest.path.clone(), + manifest.ecosystem, + manifest.results.clone(), + ) + // Without this the one manifest whose empty result set means "nothing was + // read" is indistinguishable from every manifest whose empty result set + // means "nothing is wrong". + .with_dependencies_unread(manifest.dependencies_unread), + ); } println!("{}", dependable_report::sarif::render(&report)?); Ok(()) diff --git a/crates/dependable/tests/cli_sarif.rs b/crates/dependable/tests/cli_sarif.rs index 2d36658..664b6e7 100644 --- a/crates/dependable/tests/cli_sarif.rs +++ b/crates/dependable/tests/cli_sarif.rs @@ -65,9 +65,10 @@ fn check_format_sarif_emits_a_valid_log() { let driver = &runs[0]["tool"]["driver"]; assert_eq!(driver["name"], "dependable"); let rules = driver["rules"].as_array().expect("rules"); - assert_eq!(rules.len(), 2); + assert_eq!(rules.len(), 3); assert_eq!(rules[0]["id"], "DEP001"); assert_eq!(rules[1]["id"], "DEP002"); + assert_eq!(rules[2]["id"], "DEP003"); // `results` must be present even with nothing to report: an absent `results` // means the run produced none because it failed. From 24a264f593a759f8a254cd342287d9b732768125 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 16:44:26 -0400 Subject: [PATCH 16/24] test(swift): cover a Package.resolved project end to end Exercises the fixture through the CLI rather than the parser: the pins become dependencies, an unreadable list is reported rather than rendered as clean, and `--no-lock-file` still lists the project's dependencies. --- crates/dependable/tests/fixture_swift.rs | 190 +++++++++++++++++++++++ 1 file changed, 190 insertions(+) diff --git a/crates/dependable/tests/fixture_swift.rs b/crates/dependable/tests/fixture_swift.rs index 60080a0..c1f0298 100644 --- a/crates/dependable/tests/fixture_swift.rs +++ b/crates/dependable/tests/fixture_swift.rs @@ -241,6 +241,68 @@ fn list_surfaces_the_pins_a_package_swift_never_declared() { assert!(stdout.contains("2.65.0"), "stdout: {stdout}"); } +/// `--no-lock-file` is documented as "do not report locked versions" — it suppresses +/// an *annotation*. A `Package.resolved` is not an annotation: it is the only +/// dependency list a Swift project has. Honouring the flag there turned +/// `list --no-lock-file` into a silent assertion that the project depends on nothing, +/// with no warning anywhere, which is the same inversion issue #85 exists to prevent. +#[test] +fn no_lock_file_does_not_empty_a_swift_dependency_list() { + let manifest = fixture("sample-swift/Package.swift"); + let output = run(&[ + "list", + "--manifest", + manifest.to_str().unwrap(), + "--no-lock-file", + ]); + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + + assert!(output.status.success(), "stderr: {stderr}"); + assert!( + stdout.contains("github.com/apple/swift-nio"), + "the pins are the dependency list, not an annotation on one; stdout: {stdout}" + ); + assert!( + !stdout.contains("(0 dependencies)"), + "a Swift project must never be listed as depending on nothing; stdout: {stdout}" + ); +} + +/// The other half of the same flag: for a lockfile that only *annotates* a list the +/// manifest already produced, `--no-lock-file` must still suppress it. Fixing Swift +/// by ignoring the flag everywhere would have taken this with it. +#[test] +fn no_lock_file_still_suppresses_an_annotating_lockfile() { + let manifest = fixture("sample-rust/Cargo.toml"); + let with = run(&[ + "list", + "--manifest", + manifest.to_str().unwrap(), + "--format", + "json", + ]); + let without = run(&[ + "list", + "--manifest", + manifest.to_str().unwrap(), + "--format", + "json", + "--no-lock-file", + ]); + let with = String::from_utf8_lossy(&with.stdout).into_owned(); + let without = String::from_utf8_lossy(&without.stdout).into_owned(); + + assert!( + with.contains("Cargo.lock") && with.contains("\"locked\": \"1.0.100\""), + "the fixture must have a lockfile to suppress; stdout: {with}" + ); + assert!( + !without.contains("Cargo.lock") && !without.contains("\"locked\": \"1.0.100\""), + "`--no-lock-file` must still ignore an annotating lockfile; stdout: {without}" + ); +} + /// Switching Swift off has to switch it off. Without the checker-level opt-in this /// key would parse, validate, and do nothing. #[test] @@ -504,3 +566,131 @@ fn a_truncated_package_resolved_is_reported_unread_rather_than_read_short() { ); } } + +/// The machine-readable twin of the exit-code inversion, and the reason it matters: +/// a CI job that parses `--format json` never sees an exit code per manifest. +/// +/// Two Swift projects — one with no `Package.resolved` at all, one with a genuinely +/// empty pin set — must not produce the same document. Every status count is a tally +/// of rows that *were* read, so both are zero either way; `manifests_unread` is the +/// only field that separates "we looked and there is nothing" from "we never looked". +#[test] +fn json_distinguishes_an_unread_dependency_list_from_an_empty_one() { + let unread = scratch("swift_json_unread"); + std::fs::copy( + fixture("sample-swift/Package.swift"), + unread.join("Package.swift"), + ) + .unwrap(); + + let empty = scratch("swift_json_empty"); + std::fs::copy( + fixture("sample-swift/Package.swift"), + empty.join("Package.swift"), + ) + .unwrap(); + std::fs::write(empty.join("Package.resolved"), r#"{"pins":[],"version":2}"#).unwrap(); + + let document = |dir: &Path| { + let output = run(&[ + "check", + "--manifest", + dir.join("Package.swift").to_str().unwrap(), + "--no-vuln", + "--format", + "json", + ]); + serde_json::from_slice::(&output.stdout).expect("a JSON document") + }; + + let unread = document(&unread); + let empty = document(&empty); + + assert_ne!( + unread, empty, + "a project nothing was read from must not serialize as a clean one" + ); + assert_eq!(unread["summary"]["manifests_unread"], 1); + assert_eq!(empty["summary"]["manifests_unread"], 0); + // The pinned shape is unchanged: every documented key keeps its name and value, + // so a consumer that does not know about the new one is unaffected. + for key in [ + "total", + "vulnerable", + "error", + "undetermined", + "up_to_date", + "outdated", + ] { + assert_eq!(unread["summary"][key], 0, "{key}"); + assert_eq!(empty["summary"][key], 0, "{key}"); + } + assert_eq!(unread["results"].as_array().unwrap().len(), 0); +} + +/// The same distinction in SARIF, which has no summary object to carry a counter: +/// the unread manifest gets a `DEP003` result naming it, and the empty one gets the +/// empty `results` array it has earned. +#[test] +fn sarif_reports_an_unread_dependency_list_as_a_finding() { + let unread = scratch("swift_sarif_unread"); + std::fs::copy( + fixture("sample-swift/Package.swift"), + unread.join("Package.swift"), + ) + .unwrap(); + + let empty = scratch("swift_sarif_empty"); + std::fs::copy( + fixture("sample-swift/Package.swift"), + empty.join("Package.swift"), + ) + .unwrap(); + std::fs::write(empty.join("Package.resolved"), r#"{"pins":[],"version":2}"#).unwrap(); + + let results = |dir: &Path| { + let output = run(&[ + "check", + "--manifest", + dir.join("Package.swift").to_str().unwrap(), + "--no-vuln", + "--format", + "sarif", + ]); + let log: serde_json::Value = + serde_json::from_slice(&output.stdout).expect("a SARIF document"); + log["runs"][0]["results"].as_array().cloned().unwrap() + }; + + let unread = results(&unread); + let empty = results(&empty); + + assert_eq!( + empty.len(), + 0, + "a resolved project with no pins genuinely has no findings" + ); + assert_eq!( + unread.len(), + 1, + "an unread dependency list is a finding of its own: {unread:?}" + ); + assert_eq!(unread[0]["ruleId"], "DEP003"); + assert_eq!(unread[0]["level"], "warning"); + assert!( + unread[0]["locations"][0]["physicalLocation"]["artifactLocation"]["uri"] + .as_str() + .expect("a uri") + .ends_with("Package.swift"), + "the finding must name the manifest it is about: {unread:?}" + ); + // No package was read, so none is named — and `region` is absent because the + // missing information is a file that is not there, not a line in this one. + assert!(unread[0]["properties"].get("package").is_none()); + assert_eq!(unread[0]["properties"]["status"], "unread"); + assert!( + unread[0]["locations"][0]["physicalLocation"] + .get("region") + .is_none() + ); +} From 21b9132d66975c68d72c3299bbf87da0238abda9 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 17:11:18 -0400 Subject: [PATCH 17/24] fix(fetch): let --no-lock-file suppress annotations for check too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Checker::read_lockfile` returned `None` on `!read_lockfiles` before it ever looked at what kind of lockfile it had found, so `--no-lock-file` (and `[global] lock_file = false`) emptied a Swift project's dependency list rather than dropping a version column: `check` reported "0 dependencies", the OSV scan ran over an empty item list, and a project with a known-vulnerable pin exited 0. `list` was taught the distinction in c0f8c86; `check` never reached that code. The lockfile is now located first and the switch applied only to one that annotates, matching `apply_nearest_lockfile`. `is_dependency_source()` is true only for `Package.resolved`, so no annotating lockfile changes behaviour. Also corrects `CheckArgs::no_lock_file`, whose help still read "Ignore `Cargo.lock`" — c0f8c86 updated `ListArgs::no_lock_file` beside it and missed this one. --- crates/dependable-fetch/src/check.rs | 102 ++++++++++++++++++++++- crates/dependable/src/cli.rs | 5 +- crates/dependable/tests/fixture_swift.rs | 64 ++++++++++++++ 3 files changed, 167 insertions(+), 4 deletions(-) diff --git a/crates/dependable-fetch/src/check.rs b/crates/dependable-fetch/src/check.rs index 0535cb1..a1b03d8 100644 --- a/crates/dependable-fetch/src/check.rs +++ b/crates/dependable-fetch/src/check.rs @@ -554,15 +554,26 @@ impl Checker { /// directory first, then each ancestor, stopping at a repository boundary — /// so a workspace member picks up the lockfile at the workspace root rather /// than reporting no locked versions. + /// + /// # `read_lockfiles` governs annotations only + /// [`CheckerBuilder::read_lockfiles`] is the `--no-lock-file` switch, and that + /// flag suppresses *locked-version annotations* on a list the manifest already + /// produced. A `Package.resolved` is not that: it **is** the list, because a + /// `Package.swift` is a program this crate declines to read + /// ([`LockfileKind::is_dependency_source`]). Honouring the switch there does + /// not withhold a column, it reports a Swift project as depending on nothing — + /// and since the OSV scan then runs over an empty item list, a project with a + /// vulnerable pin comes back clean. So the file is located first and the switch + /// is applied only to a lockfile that annotates. async fn read_lockfile( &self, path: &Path, kind: ManifestKind, ) -> Option<(LockfileKind, String)> { - if !self.read_lockfiles { + let (lock_path, lock_kind) = crate::discover::locate_lockfile(path, kind)?; + if !self.read_lockfiles && !lock_kind.is_dependency_source() { return None; } - let (lock_path, lock_kind) = crate::discover::locate_lockfile(path, kind)?; let content = tokio::fs::read_to_string(&lock_path).await.ok()?; Some((lock_kind, content)) } @@ -1473,7 +1484,13 @@ impl CheckerBuilder { self } - /// Whether [`Checker::check_path`] reads the sibling lockfile (default: true). + /// Whether [`Checker::check_path`] reads an **annotating** sibling lockfile + /// (default: true). + /// + /// A lockfile that *is* the dependency list rather than an annotation on one — + /// SwiftPM's `Package.resolved`, see [`LockfileKind::is_dependency_source`] — + /// is read regardless: switching it off would report the project as depending + /// on nothing rather than as having no locked versions. pub fn read_lockfiles(mut self, enabled: bool) -> Self { self.read_lockfiles = enabled; self @@ -1669,6 +1686,85 @@ mod tests { assert_eq!(check.results[1].status, DependencyStatus::Local); } + /// `--no-lock-file` suppresses locked-version *annotations*. A + /// `Package.resolved` is not one — it is the whole dependency list a Swift + /// project has — so honouring the switch there did not withhold a column, it + /// handed the OSV scan an empty item list and reported a project with a + /// vulnerable pin as clean. + #[tokio::test] + async fn lockfiles_off_still_reads_the_lockfile_that_is_the_dependency_list() { + let dir = tempfile::tempdir().expect("tempdir"); + std::fs::write(dir.path().join("Package.swift"), "// a program\n").unwrap(); + std::fs::write(dir.path().join("Package.resolved"), PACKAGE_RESOLVED).unwrap(); + + let checker = Checker::builder() + .rust_registry("http://127.0.0.1:1".to_string(), None) + .registryless(Ecosystem::Swift) + .vulnerabilities(false) + .disk_cache(false) + .read_lockfiles(false) + .build() + .expect("a checker builds without a network"); + + let check = checker + .check_path(dir.path().join("Package.swift")) + .await + .expect("checked"); + let names: Vec<&str> = check.results.iter().map(|r| r.item.name.as_str()).collect(); + assert_eq!( + names, + ["github.com/apple/swift-nio", "helpers"], + "with the list suppressed there is nothing to scan and nothing to report" + ); + } + + /// The other half of the same switch: a lockfile that only *annotates* a list + /// the manifest already produced must still be ignored. Reading a dependency + /// source regardless must not become reading everything regardless. + #[tokio::test] + async fn lockfiles_off_still_suppresses_an_annotating_lockfile() { + let dir = tempfile::tempdir().expect("tempdir"); + std::fs::write( + dir.path().join("Cargo.toml"), + "[package]\nname = \"sample\"\n\n[dependencies]\ntime = \"0.2.7\"\n", + ) + .unwrap(); + std::fs::write( + dir.path().join("Cargo.lock"), + "[[package]]\nname = \"time\"\nversion = \"0.2.7\"\n", + ) + .unwrap(); + + async fn locked(manifest: &std::path::Path, read_lockfiles: bool) -> Option { + let checker = Checker::builder() + .rust_registry("http://127.0.0.1:1".to_string(), None) + .vulnerabilities(false) + .disk_cache(false) + .read_lockfiles(read_lockfiles) + .build() + .expect("a checker builds without a network"); + checker + .check_path(manifest) + .await + .expect("checked") + .results + .first() + .and_then(|r| r.item.locked_version.clone()) + } + + let manifest = dir.path().join("Cargo.toml"); + assert_eq!( + locked(&manifest, true).await.as_deref(), + Some("0.2.7"), + "the fixture must have an annotation to suppress" + ); + assert_eq!( + locked(&manifest, false).await, + None, + "`--no-lock-file` must still drop an annotating lockfile" + ); + } + /// The discriminator. `has_registry()` is true for every ecosystem but Swift, /// so an ecosystem the user switched off in config keeps the old path and the /// CLI keeps printing `skipping … is not enabled or not yet supported`. diff --git a/crates/dependable/src/cli.rs b/crates/dependable/src/cli.rs index 6c9972e..768b551 100644 --- a/crates/dependable/src/cli.rs +++ b/crates/dependable/src/cli.rs @@ -75,7 +75,10 @@ pub struct CheckArgs { /// `include-if-current`. Overrides `[global] unstable`. #[arg(long, value_enum)] pub unstable: Option, - /// Ignore `Cargo.lock`. + /// Ignore sibling lockfiles (do not report locked versions). A lockfile that + /// *is* the dependency list rather than an annotation on one — SwiftPM's + /// `Package.resolved` — is still read, or the project would be checked as + /// though it had no dependencies at all. #[arg(long)] pub no_lock_file: bool, /// Skip vulnerability scanning. diff --git a/crates/dependable/tests/fixture_swift.rs b/crates/dependable/tests/fixture_swift.rs index c1f0298..ee2ffe4 100644 --- a/crates/dependable/tests/fixture_swift.rs +++ b/crates/dependable/tests/fixture_swift.rs @@ -269,6 +269,70 @@ fn no_lock_file_does_not_empty_a_swift_dependency_list() { ); } +/// The same flag on the command that decides the exit code. `list` was taught that a +/// `Package.resolved` is the dependency list rather than an annotation on one; `check` +/// was not, so `--no-lock-file` handed the OSV scan an empty item list. A Swift project +/// with a known-vulnerable pin then reported clean and exited 0 — a silent security +/// false negative, and the exact inversion the flag's own help text disclaims. +#[test] +fn no_lock_file_does_not_empty_what_check_scans() { + let manifest = fixture("sample-swift/Package.swift"); + let output = run(&[ + "check", + "--manifest", + manifest.to_str().unwrap(), + "--no-lock-file", + "--no-vuln", + ]); + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + + assert!( + stdout.contains("github.com/apple/swift-nio"), + "the pins are what `check` has to scan; stdout: {stdout}" + ); + assert!( + !stdout.contains("(0 dependencies)") && !stdout.contains("nothing to check"), + "an empty scan of a resolved project is a false clean bill; stdout: {stdout}" + ); + assert!( + !stderr.contains("no dependency with a version to check was found here at all"), + "there are four; stderr: {stderr}" + ); + + // The exit code is what a CI job acts on, and it was the part that silently + // inverted: nothing scanned means nothing found means success. + let gated = run(&[ + "check", + "--manifest", + manifest.to_str().unwrap(), + "--no-lock-file", + "--no-vuln", + "--fail-on", + "any", + ]); + assert_eq!( + gated.status.code(), + Some(1), + "`--fail-on any` over four undetermined pins must fail exactly as it does without the flag" + ); + + // …and `--format json` must publish the same list, not a summary of nothing. + let json = run(&[ + "check", + "--manifest", + manifest.to_str().unwrap(), + "--no-lock-file", + "--no-vuln", + "--format", + "json", + ]); + let doc: serde_json::Value = + serde_json::from_slice(&json.stdout).expect("`check --format json` emits JSON"); + assert_eq!(doc["summary"]["total"], 6, "{doc}"); + assert_eq!(doc["summary"]["undetermined"], 4, "{doc}"); +} + /// The other half of the same flag: for a lockfile that only *annotates* a list the /// manifest already produced, `--no-lock-file` must still suppress it. Fixing Swift /// by ignoring the flag everywhere would have taken this with it. From 1193ba307a7c17786d52b22728230e236244be5f Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 17:13:56 -0400 Subject: [PATCH 18/24] fix(core): read an SCP-shorthand colon as a path separator, never a port MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `git@github.com:owner/repo` writes a colon where a URL writes a slash, and the reader decided which it was by looking at the segment after it. Both directions of that guess were wrong. An owner beginning with a digit kept the colon — `git@github.com:1024jp/GzipSwift.git` yielded `github.com:1024jp/GzipSwift`, where OSV holds `github.com/1024jp/GzipSwift`, so a real and widely used package always reported clean (`0xOpenBytes/*` and `4np/*` are others). An owner that is *all* digits lost its segment instead: `git@github.com:42/pkg.git` yielded `github.com/pkg`, a well-formed key naming a different repository, which nothing downstream can recognise as garbage and which can collide with a real advisory key. The two shapes are textually identical, so the segment can never decide between them. The form of the location can: a port is URL syntax and only ever follows a scheme, while SCP shorthand has none. `split_authority` now takes that as its input and never inspects the digits. An IPv6 literal is bracketed, so the scan for either separator starts after the `]`. `swift_package_name_variants` also folds ASCII rather than Unicode now, matching the host: OSV's keys are ASCII, and a Unicode fold can change a string's byte length and hand OSV a key nobody wrote. --- .../src/lockfiles/swift_package_resolved.rs | 143 +++++++++++++----- 1 file changed, 101 insertions(+), 42 deletions(-) diff --git a/crates/dependable-core/src/lockfiles/swift_package_resolved.rs b/crates/dependable-core/src/lockfiles/swift_package_resolved.rs index ff179aa..6712d0e 100644 --- a/crates/dependable-core/src/lockfiles/swift_package_resolved.rs +++ b/crates/dependable-core/src/lockfiles/swift_package_resolved.rs @@ -151,49 +151,72 @@ pub fn swift_package_name(location: &str) -> String { name = name[at + 1..].to_string(); } - // git's SCP shorthand (`github.com:owner/repo`) writes a colon where a URL - // writes a slash. A port number is digits and is never this. - if scheme.is_none() - && let Some(colon) = name.find(':') - && !name[colon + 1..].starts_with(|c: char| c.is_ascii_digit()) - { - name.replace_range(colon..=colon, "/"); - } - let name = name.trim_end_matches('/'); let name = name.strip_suffix(".git").unwrap_or(name); - lowercase_host(name.trim_end_matches('/')) + normalize_host(name.trim_end_matches('/'), scheme.is_some()) } -/// Normalize the host component of an OSV `SwiftURL` name, leaving the path alone. +/// Lowercase `name`'s host and join it to the path below it with a `/`, leaving +/// that path exactly as written. /// -/// Lowercased, for the reason [`swift_package_name`] gives, and stripped of any -/// port: `ssh://git@github.com:22/apple/swift-nio.git` and +/// The host is lowercased for the reason [`swift_package_name`] gives, and any port +/// is dropped: `ssh://git@github.com:22/apple/swift-nio.git` and /// `https://github.com/apple/swift-nio.git` address the same repository, but only /// the second spells the key OSV holds. A port is transport, not identity, and /// leaving it on is the same silent false negative a mis-cased host is — the query /// matches nothing and the package is reported clean. -fn lowercase_host(name: &str) -> String { - match name.split_once('/') { - Some((host, path)) => format!("{}/{path}", strip_port(host).to_ascii_lowercase()), - None => strip_port(name).to_ascii_lowercase(), +fn normalize_host(name: &str, has_scheme: bool) -> String { + match split_authority(name, has_scheme) { + (host, Some(path)) => format!("{}/{path}", host.to_ascii_lowercase()), + (host, None) => host.to_ascii_lowercase(), } } -/// `host` without a trailing `:`. +/// Split `name` into its host and the path beneath it, dropping the separator (and +/// a port, where there is one). /// -/// Only an all-digit suffix is a port, which is the same test the SCP-shorthand -/// branch of [`swift_package_name`] already applies: `github.com:vapor` is a path -/// and keeps its colon here too. An IPv6 literal is unharmed — `[::1]` ends in `]`, -/// not a digit — while `[::1]:22` loses only the port. -fn strip_port(host: &str) -> &str { - match host.rsplit_once(':') { - Some((rest, port)) - if !rest.is_empty() && !port.is_empty() && port.bytes().all(|b| b.is_ascii_digit()) => - { - rest - } - _ => host, +/// # A colon is a port or a path separator, and only the form of the location says which +/// `github.com:22/apple/swift-nio` and `github.com:42/pkg` are the same string +/// shape and mean opposite things, so no test applied to the colon's *neighbours* +/// can tell them apart. Guessing from whether the segment is numeric got both +/// wrong: an owner beginning with a digit (`1024jp/GzipSwift`, `0xOpenBytes`, +/// `4np`) kept a colon that OSV never matches, and an all-digit owner (`42/pkg`) +/// silently lost its segment, producing a well-formed key naming a *different* +/// package — the worse of the two, because nothing about it looks wrong. +/// +/// What actually distinguishes them is the form: a port is URL syntax and only ever +/// follows a scheme, while git's SCP shorthand (`git@github.com:owner/repo`) has no +/// scheme by definition and writes a colon exactly where a URL writes a slash. So +/// `has_scheme` decides it, and the digits are never consulted. +/// +/// An IPv6 literal is bracketed and full of colons that are neither, so the scan for +/// a separator begins after the closing `]`. +fn split_authority(name: &str, has_scheme: bool) -> (&str, Option<&str>) { + let after_host = if name.starts_with('[') { + name.find(']').map_or(0, |close| close + 1) + } else { + 0 + }; + let find = |needle: char| name[after_host..].find(needle).map(|i| i + after_host); + let slash = find('/'); + let colon = find(':'); + let split_at = |i: usize| (&name[..i], Some(&name[i + 1..])); + + if has_scheme { + // URL syntax: the authority runs to the first `/`, and a `:` inside it is a + // port. + let (authority, path) = slash.map_or((name, None), split_at); + let host = colon + .filter(|i| *i < authority.len()) + .map_or(authority, |i| &authority[..i]); + (host, path) + } else { + // No scheme, so no port: the first colon — if it comes before any slash — is + // SCP shorthand's path separator. + colon + .filter(|colon| slash.is_none_or(|slash| *colon < slash)) + .or(slash) + .map_or((name, None), split_at) } } @@ -208,9 +231,13 @@ fn strip_port(host: &str) -> &str { /// answers for the package it was asked about or not at all. /// /// Empty when the name is already lowercase, which is the overwhelming majority. +/// +/// The fold is **ASCII**, matching the host's: OSV's `SwiftURL` keys and the +/// hostnames in them are ASCII, and Unicode lowercasing can change a string's byte +/// length, which would hand OSV a key for a repository nobody wrote. #[must_use] pub fn swift_package_name_variants(name: &str) -> Vec { - let lowered = name.to_lowercase(); + let lowered = name.to_ascii_lowercase(); if lowered == name { Vec::new() } else { @@ -536,12 +563,6 @@ mod tests { "git://GitHub.com:9418/apple/swift-nio", "github.com/apple/swift-nio", ), - // No scheme: `:22` is read as a port by the SCP-shorthand branch, so - // the path starts after it. - ( - "github.com:22/apple/swift-nio.git", - "github.com/apple/swift-nio", - ), // The port is transport only; a mixed-case path still survives it. ( "ssh://git@github.com:22/weichsel/ZIPFoundation.git", @@ -553,19 +574,57 @@ mod tests { } } - /// The strip is an all-digit suffix and nothing else, so a colon that is part of - /// a name — SCP shorthand, an IPv6 literal — keeps it. + /// The colon in git's SCP shorthand is a path separator, whatever the segment + /// after it happens to look like. Reading it as a port when the segment was + /// numeric produced two silent false negatives at once: `1024jp/GzipSwift` — a + /// real, widely used package, as are `0xOpenBytes/*` and `4np/*` — kept a colon + /// that OSV can never match, and `42/pkg` lost its owner entirely, yielding a + /// well-formed key for a *different* repository, which no reader can spot as + /// garbage and which could collide with a real advisory key. #[test] - fn a_colon_that_is_not_a_port_survives() { + fn an_scp_shorthand_colon_is_a_path_separator_whatever_follows_it() { let cases = [ - // SCP shorthand: the colon becomes the path separator, not a port. + // The owner begins with a digit. Nothing distinguishes this from a port + // but the absence of a scheme. + ( + "git@github.com:1024jp/GzipSwift.git", + "github.com/1024jp/GzipSwift", + ), + // The owner is *all* digits — the case the old heuristic deleted. + ("git@github.com:42/pkg.git", "github.com/42/pkg"), ("git@github.com:vapor/vapor.git", "github.com/vapor/vapor"), - // An IPv6 literal ends in `]`, never a digit. + // A scheme is present, so here the same shape really is a port. + ( + "ssh://git@github.com:22/apple/swift-nio.git", + "github.com/apple/swift-nio", + ), + ( + "https://github.com:443/apple/swift-nio.git", + "github.com/apple/swift-nio", + ), + ]; + for (location, expected) in cases { + assert_eq!(swift_package_name(location), expected, "{location}"); + } + } + + /// An IPv6 literal is bracketed and full of colons that separate nothing, so the + /// search for a port or a path separator starts after the `]`. + #[test] + fn an_ipv6_literal_keeps_its_colons() { + let cases = [ ("https://[::1]/apple/swift-nio.git", "[::1]/apple/swift-nio"), + // A scheme, so `:22` is a port. ( "ssh://git@[::1]:22/apple/swift-nio", "[::1]/apple/swift-nio", ), + // No scheme, so by the same rule as every other SCP location the colon + // separates the host from the path — degenerate, but consistent, and it + // does not mangle the address. + ("[::1]:22", "[::1]/22"), + // No separator at all: the whole literal is the host. + ("[2001:db8::1]", "[2001:db8::1]"), ]; for (location, expected) in cases { assert_eq!(swift_package_name(location), expected, "{location}"); From f0f0209c12e522cc75d7293ddb93f7da3f9eb5f7 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 17:15:12 -0400 Subject: [PATCH 19/24] fix(report): unwrap DEP003's strings and stop overloading its status key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three of the rule's string literals were wrapped across source lines without `\` continuations, so the emitted JSON carried runs of 14-18 literal spaces — text a GitHub Code Scanning alert renders verbatim. DEP003 also wrote `properties.status = "unread"`. That key otherwise always holds a `DependencyStatus::token()`, so a consumer switching on it exhaustively met a word no status can produce. DEP003 names no dependency and has no dependency status: `status` is now absent for it, and the fact it was carrying gets its own key, `dependencyListUnread`. Every other finding's `properties` is byte-identical to before. --- crates/dependable-report/src/sarif.rs | 41 ++++++++++++++++----- crates/dependable/tests/fixture_swift.rs | 45 +++++++++++++++++++++--- 2 files changed, 73 insertions(+), 13 deletions(-) diff --git a/crates/dependable-report/src/sarif.rs b/crates/dependable-report/src/sarif.rs index ff20de2..6676f33 100644 --- a/crates/dependable-report/src/sarif.rs +++ b/crates/dependable-report/src/sarif.rs @@ -271,7 +271,8 @@ fn vulnerable_finding( ecosystem, current_version: Some(current), latest_version: latest_version(result), - status: result.status.token(), + status: Some(result.status.token()), + dependency_list_unread: None, advisory_id: Some(id.to_string()), cvss_score: advisory.and_then(|a| a.severity.score), severity: advisory.and_then(|a| a.severity.band).map(|b| b.token()), @@ -324,7 +325,8 @@ fn outdated_finding( ecosystem, current_version: Some(current), latest_version: latest, - status: result.status.token(), + status: Some(result.status.token()), + dependency_list_unread: None, advisory_id: None, cvss_score: None, severity: None, @@ -353,7 +355,9 @@ fn unread_finding(ecosystem: &'static str, uri: &str) -> Finding { rule_index: 2, level: Level::Warning, message: format!( - "The dependency list for `{uri}` could not be read, so no dependency in it was checked. An empty result set for this manifest means nothing was looked at, not that nothing is wrong." + "The dependency list for `{uri}` could not be read, so no dependency in it was \ + checked. An empty result set for this manifest means nothing was looked at, \ + not that nothing is wrong." ), uri: uri.to_string(), start_line: None, @@ -365,9 +369,10 @@ fn unread_finding(ecosystem: &'static str, uri: &str) -> Finding { ecosystem, current_version: None, latest_version: None, - // Not a `DependencyStatus` token — no dependency has this status, - // because no dependency was read. The manifest does. - status: "unread", + // No dependency has a status here, because no dependency was read. + // The fact belongs to the manifest, and to its own key. + status: None, + dependency_list_unread: Some(true), advisory_id: None, cvss_score: None, severity: None, @@ -610,10 +615,14 @@ fn rules() -> [ReportingDescriptor; 3] { name: "UnreadDependencyList", short_description: Text::new("The project's dependency list could not be read."), full_description: Text::new( - "The file that is this project's dependency list — a SwiftPM `Package.resolved` — is missing or unreadable, and its manifest declares no dependencies of its own. No dependency was checked, so the absence of other findings for this manifest says nothing about the project.", + "The file that is this project's dependency list — a SwiftPM \ + `Package.resolved` — is missing or unreadable, and its manifest declares \ + no dependencies of its own. No dependency was checked, so the absence of \ + other findings for this manifest says nothing about the project.", ), help: Text::new( - "Resolve the project (`swift package resolve`) and commit the resulting `Package.resolved`, or repair the existing one.", + "Resolve the project (`swift package resolve`) and commit the resulting \ + `Package.resolved`, or repair the existing one.", ), help_uri: INFORMATION_URI, default_configuration: RuleConfig { @@ -817,7 +826,21 @@ struct ResultProperties { current_version: Option, #[serde(skip_serializing_if = "Option::is_none")] latest_version: Option, - status: &'static str, + /// A [`DependencyStatus::token`] — and nothing else, ever. + /// + /// Absent for [`DEP003`], which is about the manifest rather than a dependency + /// in it. Overloading this key with a word no `DependencyStatus` produces would + /// break the one thing a consumer can safely do with it: switch on it + /// exhaustively. [`ResultProperties::dependency_list_unread`] carries that fact + /// instead. + #[serde(skip_serializing_if = "Option::is_none")] + status: Option<&'static str>, + /// [`DEP003`] only: the file that *is* this project's dependency list went + /// unread, so an empty result set for the manifest means nothing was looked at. + /// Its own key rather than a value of [`ResultProperties::status`], because it + /// is a fact about the manifest and not a status any dependency can hold. + #[serde(skip_serializing_if = "Option::is_none")] + dependency_list_unread: Option, #[serde(skip_serializing_if = "Option::is_none")] advisory_id: Option, #[serde(skip_serializing_if = "Option::is_none")] diff --git a/crates/dependable/tests/fixture_swift.rs b/crates/dependable/tests/fixture_swift.rs index ee2ffe4..55ee09a 100644 --- a/crates/dependable/tests/fixture_swift.rs +++ b/crates/dependable/tests/fixture_swift.rs @@ -697,10 +697,10 @@ fn json_distinguishes_an_unread_dependency_list_from_an_empty_one() { /// empty `results` array it has earned. #[test] fn sarif_reports_an_unread_dependency_list_as_a_finding() { - let unread = scratch("swift_sarif_unread"); + let unread_dir = scratch("swift_sarif_unread"); std::fs::copy( fixture("sample-swift/Package.swift"), - unread.join("Package.swift"), + unread_dir.join("Package.swift"), ) .unwrap(); @@ -726,7 +726,7 @@ fn sarif_reports_an_unread_dependency_list_as_a_finding() { log["runs"][0]["results"].as_array().cloned().unwrap() }; - let unread = results(&unread); + let unread = results(&unread_dir); let empty = results(&empty); assert_eq!( @@ -751,10 +751,47 @@ fn sarif_reports_an_unread_dependency_list_as_a_finding() { // No package was read, so none is named — and `region` is absent because the // missing information is a file that is not there, not a line in this one. assert!(unread[0]["properties"].get("package").is_none()); - assert_eq!(unread[0]["properties"]["status"], "unread"); assert!( unread[0]["locations"][0]["physicalLocation"] .get("region") .is_none() ); + + // `properties.status` otherwise always holds a `DependencyStatus` token, so a + // consumer switching on it exhaustively is doing the one safe thing with the + // key. DEP003 must not put a word there that no status can produce; the fact + // is about the manifest and gets its own key. + assert!( + unread[0]["properties"].get("status").is_none(), + "DEP003 names no dependency, so it claims no dependency status: {unread:?}" + ); + assert_eq!(unread[0]["properties"]["dependencyListUnread"], true); + + // The strings a Code Scanning alert renders verbatim. Wrapped string literals + // without `\` continuations carried runs of 14-18 literal spaces into them. + let text = |value: &serde_json::Value| value.as_str().expect("a string").to_owned(); + let rule = { + let output = run(&[ + "check", + "--manifest", + unread_dir.join("Package.swift").to_str().unwrap(), + "--no-vuln", + "--format", + "sarif", + ]); + let log: serde_json::Value = + serde_json::from_slice(&output.stdout).expect("a SARIF document"); + log["runs"][0]["tool"]["driver"]["rules"][2].clone() + }; + assert_eq!(rule["id"], "DEP003"); + for rendered in [ + text(&unread[0]["message"]["text"]), + text(&rule["fullDescription"]["text"]), + text(&rule["help"]["text"]), + ] { + assert!( + !rendered.contains(" "), + "a run of spaces renders verbatim in the alert: {rendered:?}" + ); + } } From e060ef4ca1e77669f69b56288962cb8e596b382b Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 17:17:57 -0400 Subject: [PATCH 20/24] fix(report): carry an unread dependency list into the report itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `report` built its `ManifestResults` without `with_dependencies_unread`, on both the HTML path and the policy path, so the fact reached the document only as a run note — and `runner.rs` gates every note on `!args.quiet`. `dependable report --quiet -o report.html` over a Swift project with no `Package.resolved` therefore produced an artifact indistinguishable from a resolved, clean one, right down to §3 printing "This manifest declares no dependencies", which nothing had established. Both paths now set the flag, `Summary` counts the manifests it covers, and the templates state it: a coverage caveat in §1 and, in §3, a sentence in place of the "declares no dependencies" claim. A caveat about what a report does not cover is not chatter, so it survives `--quiet` by being part of the model rather than a note beside it. A resolved project with no pins is unchanged. --- crates/dependable-report/src/html/model.rs | 11 +++ .../src/html/templates/dependencies.html | 4 + .../src/html/templates/summary.html | 10 +++ crates/dependable-report/src/summary.rs | 42 ++++++++++ crates/dependable/src/runner.rs | 32 +++++--- crates/dependable/tests/fixture_swift.rs | 79 +++++++++++++++++++ 6 files changed, 168 insertions(+), 10 deletions(-) diff --git a/crates/dependable-report/src/html/model.rs b/crates/dependable-report/src/html/model.rs index e840fa3..d70fa1a 100644 --- a/crates/dependable-report/src/html/model.rs +++ b/crates/dependable-report/src/html/model.rs @@ -181,6 +181,10 @@ pub(crate) struct View { #[derive(Debug, Serialize)] pub(crate) struct SummaryView { pub manifests: usize, + /// How many manifests contributed no rows because their dependency list could + /// not be read. The template states it whenever it is nonzero, because a + /// reader who is not told cannot tell this report from a clean one. + pub manifests_unread: usize, pub total: usize, pub checkable: usize, pub up_to_date: usize, @@ -258,6 +262,11 @@ pub(crate) struct ManifestView { pub path: String, pub ecosystem: String, pub total: usize, + /// Whether the file that *is* this project's dependency list went unread, so + /// zero rows means nothing was read rather than nothing was declared. Without + /// it the section prints "This manifest declares no dependencies", which is a + /// claim the run never established. + pub dependencies_unread: bool, pub rows: Vec, } @@ -372,6 +381,7 @@ impl View { fn summary_view(summary: &Summary) -> SummaryView { SummaryView { manifests: summary.manifests, + manifests_unread: summary.manifests_unread, total: summary.total, checkable: summary.checkable, up_to_date: summary.up_to_date, @@ -491,6 +501,7 @@ fn manifest_view(manifest: &ManifestResults) -> ManifestView { path: manifest.path.display().to_string(), ecosystem: manifest.ecosystem.display_name().to_owned(), total: manifest.results.len(), + dependencies_unread: manifest.dependencies_unread, rows: manifest.results.iter().map(dep_row).collect(), } } diff --git a/crates/dependable-report/src/html/templates/dependencies.html b/crates/dependable-report/src/html/templates/dependencies.html index b452732..4ade957 100644 --- a/crates/dependable-report/src/html/templates/dependencies.html +++ b/crates/dependable-report/src/html/templates/dependencies.html @@ -38,6 +38,10 @@

3. Dependency status

{%- endfor %} +{%- elif manifest.dependencies_unread %} +

The file that is this project's dependency list could not be read, so no dependency +of it was checked. This is not a project with no dependencies: it is a project nothing here was +established about.

{%- else %}

This manifest declares no dependencies.

{%- endif %} diff --git a/crates/dependable-report/src/html/templates/summary.html b/crates/dependable-report/src/html/templates/summary.html index 69c6970..77c1a36 100644 --- a/crates/dependable-report/src/html/templates/summary.html +++ b/crates/dependable-report/src/html/templates/summary.html @@ -1,6 +1,16 @@ {% import "macros.html" as m %}

1. Executive summary

+{%- if summary.manifests_unread %} +
+

Coverage caveat

+
    +
  • {{ summary.manifests_unread }} of the {{ summary.manifests }} manifest(s) here had no readable +dependency list, so they contributed nothing to the counts below. Their sections in §3 say which. +Nothing in this report speaks for them.
    • +
+
+{%- endif %} {%- if notes %}

Notes from this run

diff --git a/crates/dependable-report/src/summary.rs b/crates/dependable-report/src/summary.rs index 18d1df6..8f9b5dc 100644 --- a/crates/dependable-report/src/summary.rs +++ b/crates/dependable-report/src/summary.rs @@ -21,6 +21,15 @@ use crate::model::Report; pub struct Summary { /// How many manifests the report covers. pub manifests: usize, + /// How many of [`Self::manifests`] had the file that *is* their dependency list + /// go unread — see [`crate::model::ManifestResults::dependencies_unread`]. + /// + /// Nonzero means the counts below are drawn from fewer projects than + /// [`Self::manifests`] names, and that the ones missing contributed no rows + /// because none could be read — not because they had none. A renderer that + /// ignores this presents a project nothing was established about exactly as it + /// presents a clean one. + pub manifests_unread: usize, /// Every declared dependency across every manifest. pub total: usize, /// Dependencies whose currency this run actually established: @@ -197,6 +206,9 @@ impl Report { let mut by_ecosystem: Vec = Vec::new(); for manifest in &self.manifests { + if manifest.dependencies_unread { + summary.manifests_unread += 1; + } let slot = by_ecosystem .iter() .position(|e| e.ecosystem == manifest.ecosystem) @@ -295,6 +307,36 @@ mod tests { report } + /// A manifest that contributed no rows because none could be read is not the + /// same as one that contributed none because it declares none, and the counts + /// alone cannot tell them apart — both are zero. Only this counter can, and a + /// renderer that has it can say so however quiet the run was. + #[test] + fn an_unread_manifest_is_counted_apart_from_an_empty_one() { + let unread = report(vec![ + ManifestResults::new( + PathBuf::from("a/Package.swift"), + Ecosystem::Swift, + Vec::new(), + ) + .with_dependencies_unread(true), + ]); + let empty = report(vec![ManifestResults::new( + PathBuf::from("b/Package.swift"), + Ecosystem::Swift, + Vec::new(), + )]); + + assert_eq!(unread.summary().manifests_unread, 1); + assert_eq!(empty.summary().manifests_unread, 0); + assert_eq!( + unread.summary().total, + empty.summary().total, + "the dependency counts are identical, which is exactly why the caveat has to be \ + carried separately" + ); + } + #[test] fn counts_every_status_and_only_counts_checkable_once() { let report = report(vec![ManifestResults::new( diff --git a/crates/dependable/src/runner.rs b/crates/dependable/src/runner.rs index 385b029..79420a9 100644 --- a/crates/dependable/src/runner.rs +++ b/crates/dependable/src/runner.rs @@ -416,11 +416,17 @@ pub async fn run_check(args: CheckArgs) -> anyhow::Result { fn build_report(root: PathBuf, reports: &[ManifestReport]) -> dependable_report::Report { let mut report = dependable_report::Report::new(root); for manifest in reports { - report.push(dependable_report::ManifestResults::new( - manifest.path.clone(), - manifest.ecosystem, - manifest.results.clone(), - )); + report.push( + dependable_report::ManifestResults::new( + manifest.path.clone(), + manifest.ecosystem, + manifest.results.clone(), + ) + // A policy rule counts rows. A manifest whose dependency list went + // unread contributes none, and a rule that passes over no rows has + // established nothing — so the model has to carry the difference. + .with_dependencies_unread(manifest.dependencies_unread), + ); } report } @@ -1143,11 +1149,17 @@ pub async fn run_report(args: crate::cli::ReportArgs) -> anyhow::Result report.push(dependable_report::ManifestResults::new( - relative_to(&root, &checked.path), - checked.ecosystem, - checked.results, - )), + Some(checked) => report.push( + dependable_report::ManifestResults::new( + relative_to(&root, &checked.path), + checked.ecosystem, + checked.results, + ) + // Structural, not a note: `--quiet` suppresses the notes below, and + // a caveat about what the report does not cover is not chatter. A + // report that omits it is indistinguishable from a clean one. + .with_dependencies_unread(checked.dependencies_unread), + ), None => notes.push(format!( "Skipped {}: its ecosystem is not enabled or not yet supported.", relative_to(&root, manifest).display() diff --git a/crates/dependable/tests/fixture_swift.rs b/crates/dependable/tests/fixture_swift.rs index 55ee09a..f19d1e7 100644 --- a/crates/dependable/tests/fixture_swift.rs +++ b/crates/dependable/tests/fixture_swift.rs @@ -795,3 +795,82 @@ fn sarif_reports_an_unread_dependency_list_as_a_finding() { ); } } + +/// An HTML report is frequently the only artifact a reviewer ever sees, so the fact +/// that a project's dependency list went unread has to be *in the document*. +/// +/// It used to reach the page only as a run note, and `report --quiet` drops notes — +/// so the quiet artifact for a Swift project with no `Package.resolved` was +/// byte-for-byte the shape of a resolved, clean one, right down to §3 asserting +/// "This manifest declares no dependencies", which nothing had established. +#[test] +fn a_quiet_html_report_still_says_the_dependency_list_went_unread() { + let unread_dir = scratch("swift_html_unread"); + std::fs::copy( + fixture("sample-swift/Package.swift"), + unread_dir.join("Package.swift"), + ) + .unwrap(); + + let empty_dir = scratch("swift_html_empty"); + std::fs::copy( + fixture("sample-swift/Package.swift"), + empty_dir.join("Package.swift"), + ) + .unwrap(); + std::fs::write( + empty_dir.join("Package.resolved"), + r#"{"pins":[],"version":2}"#, + ) + .unwrap(); + + let render = |dir: &Path| { + let out = dir.join("report.html"); + let output = run(&[ + "report", + "--manifest", + dir.join("Package.swift").to_str().unwrap(), + "--no-vuln", + "--quiet", + "--output", + out.to_str().unwrap(), + ]); + assert!( + output.status.success(), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + // The templates wrap their prose, so the rendered document carries newlines + // inside sentences. Collapse them, or an assertion about a phrase would be + // an assertion about where a template happens to break its lines. + std::fs::read_to_string(&out) + .expect("the report was written") + .split_whitespace() + .collect::>() + .join(" ") + }; + + let unread = render(&unread_dir); + let empty = render(&empty_dir); + + assert!( + unread.contains("no readable dependency list"), + "the summary must carry the caveat structurally, not as a suppressible note" + ); + assert!( + unread.contains("could not be read"), + "and the manifest's own section must say which project it is about" + ); + assert!( + !unread.contains("This manifest declares no dependencies"), + "nothing established that; the file that would have said so was never read" + ); + + // The other half: a project that really is resolved and really has no pins is + // still reported exactly as before, with no caveat it has not earned. + assert!( + !empty.contains("no readable dependency list") && !empty.contains("could not be read"), + "a resolved project with no pins has nothing to caveat" + ); + assert!(empty.contains("This manifest declares no dependencies")); +} From 58579a481719492b54ea9ed5ba7b3be0cdc363e6 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 18:13:36 -0400 Subject: [PATCH 21/24] fix(core): a port is digits, so a non-numeric segment is not one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `split_authority` decided port-vs-path from whether a scheme was present, and on the scheme branch it dropped whatever followed the colon without looking at it. `ssh://git@github.com:vapor/vapor.git` — a scheme written in front of git's SCP shorthand — therefore normalized to `github.com/vapor`: a well-formed OSV `SwiftURL` key naming a *different* repository, so the scan answered about a package nobody asked about and the real one reported clean. `https://host:notaport/x/y` lost `notaport` the same way. The scheme still decides whether a port is possible at all; the digits now decide whether this one is. A non-numeric segment keeps its colon, which yields a key that matches nothing — a miss a reader can see, rather than a wrong answer nobody can. The no-scheme branch is untouched: there a colon is always SCP's path separator, whatever follows it, which is what keeps `git@github.com:42/pkg.git` and `1024jp/GzipSwift` intact. Reachability is narrow — a non-numeric port is invalid URL syntax and SwiftPM will not write one into a working `Package.resolved` — so this needs a hand-edited or generated file, the same class as the empty-version case already guarded here. IPv6 literals are unaffected: the scan for a separator still starts after the closing `]`, and the zone-id form is now pinned by a test too. --- .../src/lockfiles/swift_package_resolved.rs | 59 ++++++++++++++++++- 1 file changed, 56 insertions(+), 3 deletions(-) diff --git a/crates/dependable-core/src/lockfiles/swift_package_resolved.rs b/crates/dependable-core/src/lockfiles/swift_package_resolved.rs index 6712d0e..542fa80 100644 --- a/crates/dependable-core/src/lockfiles/swift_package_resolved.rs +++ b/crates/dependable-core/src/lockfiles/swift_package_resolved.rs @@ -187,7 +187,19 @@ fn normalize_host(name: &str, has_scheme: bool) -> String { /// What actually distinguishes them is the form: a port is URL syntax and only ever /// follows a scheme, while git's SCP shorthand (`git@github.com:owner/repo`) has no /// scheme by definition and writes a colon exactly where a URL writes a slash. So -/// `has_scheme` decides it, and the digits are never consulted. +/// `has_scheme` decides *whether a port is even possible*, and it is the only thing +/// consulted where there is no scheme. +/// +/// Where there is one, the digits get a second, narrower job: a port is digits, so a +/// non-numeric segment after the colon is not one and the colon stays where it was +/// written. Without that guard `ssh://git@github.com:vapor/vapor.git` — a scheme in +/// front of SCP shorthand, which only a hand-edited or generated file contains — +/// loses `vapor` and yields `github.com/vapor`: a well-formed key naming a +/// *different* repository, the same silent, unspottable false negative the old +/// numeric heuristic produced for `42/pkg`. Keeping the colon instead yields a key +/// that matches nothing, which is a miss anyone can see rather than a wrong answer +/// nobody can. The guard is only ever reached behind a scheme, so no SCP location +/// is judged by its digits. /// /// An IPv6 literal is bracketed and full of colons that are neither, so the scan for /// a separator begins after the closing `]`. @@ -203,11 +215,12 @@ fn split_authority(name: &str, has_scheme: bool) -> (&str, Option<&str>) { let split_at = |i: usize| (&name[..i], Some(&name[i + 1..])); if has_scheme { - // URL syntax: the authority runs to the first `/`, and a `:` inside it is a - // port. + // URL syntax: the authority runs to the first `/`, and a `:` inside it may be + // a port — it is one only where what follows is digits. let (authority, path) = slash.map_or((name, None), split_at); let host = colon .filter(|i| *i < authority.len()) + .filter(|i| authority[i + 1..].bytes().all(|b| b.is_ascii_digit())) .map_or(authority, |i| &authority[..i]); (host, path) } else { @@ -608,6 +621,40 @@ mod tests { } } + /// A scheme says a colon in the authority *may* be a port; it does not say the + /// segment after it is one. `ssh://git@github.com:vapor/vapor.git` is a scheme + /// written in front of SCP shorthand — invalid URL syntax that only a hand-edited + /// or generated `Package.resolved` contains — and dropping `vapor` as if it were + /// a port yields `github.com/vapor`: a well-formed OSV key naming a *different* + /// repository, so the scan answers about a package nobody asked about and the + /// real one reports clean. A port is digits; anything else is left where it was + /// written, which yields a key that matches nothing — a visible miss instead of a + /// silent wrong answer. + #[test] + fn a_non_numeric_segment_after_a_scheme_is_not_a_port() { + let cases = [ + // A scheme in front of SCP shorthand: `vapor` is not a port, so the colon + // stays and the owner is never dropped. + ( + "ssh://git@github.com:vapor/vapor.git", + "github.com:vapor/vapor", + ), + ("https://host:notaport/x/y", "host:notaport/x/y"), + // Still ports, and still stripped. + ( + "ssh://git@github.com:22/apple/swift-nio.git", + "github.com/apple/swift-nio", + ), + ( + "https://github.com:443/apple/swift-nio.git", + "github.com/apple/swift-nio", + ), + ]; + for (location, expected) in cases { + assert_eq!(swift_package_name(location), expected, "{location}"); + } + } + /// An IPv6 literal is bracketed and full of colons that separate nothing, so the /// search for a port or a path separator starts after the `]`. #[test] @@ -623,6 +670,12 @@ mod tests { // separates the host from the path — degenerate, but consistent, and it // does not mangle the address. ("[::1]:22", "[::1]/22"), + // A zone id puts `%` and letters inside the brackets; the port after the + // `]` is still digits and still goes. + ( + "ssh://git@[fe80::1%25eth0]:22/apple/swift-nio", + "[fe80::1%25eth0]/apple/swift-nio", + ), // No separator at all: the whole literal is the host. ("[2001:db8::1]", "[2001:db8::1]"), ]; From 5616ebc1dd9e8e88e2381bec8646d38faecfa153 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 18:13:36 -0400 Subject: [PATCH 22/24] fix(report): head an unread dependency list as unread, not as zero MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit §3's per-manifest heading read `Package.swift — Swift (0 dependencies)` directly above the paragraph explaining that this is *not* a project with no dependencies. A count is a claim about the project, so the heading asserted precisely what the prose beneath it, the §1 coverage caveat and DEP003 all exist to disclaim — and a heading is what a reader skimming §3 actually reads. `check`'s per-manifest heading said the same thing in the same words. Both now read `(dependency list unread)` when there is nothing to count *and* the file that would have said so went unread. A project that really is resolved and really declares nothing still counts zero and says so, in both outputs. --- crates/dependable-report/src/html/model.rs | 5 +- .../src/html/templates/dependencies.html | 2 +- crates/dependable/src/output/table.rs | 13 +++- crates/dependable/tests/fixture_swift.rs | 68 +++++++++++++++++++ 4 files changed, 82 insertions(+), 6 deletions(-) diff --git a/crates/dependable-report/src/html/model.rs b/crates/dependable-report/src/html/model.rs index d70fa1a..dfcbf04 100644 --- a/crates/dependable-report/src/html/model.rs +++ b/crates/dependable-report/src/html/model.rs @@ -264,8 +264,9 @@ pub(crate) struct ManifestView { pub total: usize, /// Whether the file that *is* this project's dependency list went unread, so /// zero rows means nothing was read rather than nothing was declared. Without - /// it the section prints "This manifest declares no dependencies", which is a - /// claim the run never established. + /// it the section prints "This manifest declares no dependencies" and heads + /// itself "(0 dependencies)", both of which are claims the run never + /// established — and the heading is the half a reader skimming §3 sees. pub dependencies_unread: bool, pub rows: Vec, } diff --git a/crates/dependable-report/src/html/templates/dependencies.html b/crates/dependable-report/src/html/templates/dependencies.html index 4ade957..9b8519a 100644 --- a/crates/dependable-report/src/html/templates/dependencies.html +++ b/crates/dependable-report/src/html/templates/dependencies.html @@ -4,7 +4,7 @@

3. Dependency status

{%- if manifests %} {%- for manifest in manifests %}
-{{ manifest.path }} — {{ manifest.ecosystem }} ({{ manifest.total }} dependencies) +{{ manifest.path }} — {{ manifest.ecosystem }} ({% if manifest.dependencies_unread and manifest.total == 0 %}dependency list unread{% else %}{{ manifest.total }} dependencies{% endif %}) {%- if manifest.rows %} diff --git a/crates/dependable/src/output/table.rs b/crates/dependable/src/output/table.rs index 23d8da3..67c41df 100644 --- a/crates/dependable/src/output/table.rs +++ b/crates/dependable/src/output/table.rs @@ -38,15 +38,22 @@ pub fn render(reports: &[ManifestReport], quiet: bool) -> anyhow::Result<()> { fn render_one(report: &ManifestReport) { let count = report.results.len(); + // "0 dependencies" is a count, and a count is a claim about the project. Where + // the file that *is* the dependency list went unread there was nothing to + // count, so the heading says that instead: a reader skimming headings must not + // come away believing a project depends on nothing when nothing was read. + let scope = if count == 0 && report.dependencies_unread { + "dependency list unread".to_owned() + } else { + format!("{count} dependenc{}", if count == 1 { "y" } else { "ies" }) + }; println!( - "{} — {} ({} dependenc{})", + "{} — {} ({scope})", report .path .display() .if_supports_color(Stream::Stdout, OwoColorize::bold), report.ecosystem.display_name(), - count, - if count == 1 { "y" } else { "ies" } ); println!(); diff --git a/crates/dependable/tests/fixture_swift.rs b/crates/dependable/tests/fixture_swift.rs index f19d1e7..d72119e 100644 --- a/crates/dependable/tests/fixture_swift.rs +++ b/crates/dependable/tests/fixture_swift.rs @@ -865,6 +865,17 @@ fn a_quiet_html_report_still_says_the_dependency_list_went_unread() { !unread.contains("This manifest declares no dependencies"), "nothing established that; the file that would have said so was never read" ); + // §3's headings are what a reader skims, and a count is a claim. "(0 + // dependencies)" sat directly above the paragraph disclaiming it, so the + // heading asserted exactly what the prose below denied. + assert!( + !unread.contains("Swift (0 dependencies)"), + "the heading counted a list nobody read: {unread}" + ); + assert!( + unread.contains("Swift (dependency list unread)"), + "the heading has to say the list went unread: {unread}" + ); // The other half: a project that really is resolved and really has no pins is // still reported exactly as before, with no caveat it has not earned. @@ -873,4 +884,61 @@ fn a_quiet_html_report_still_says_the_dependency_list_went_unread() { "a resolved project with no pins has nothing to caveat" ); assert!(empty.contains("This manifest declares no dependencies")); + assert!( + empty.contains("Swift (0 dependencies)"), + "a resolved project with no pins really does declare none: {empty}" + ); +} + +/// The same claim in the same words, in the output most runs actually look at. +/// `check`'s per-manifest heading counted the results it had, so a Swift project +/// with no readable `Package.resolved` was headed "(0 dependencies)" — a statement +/// about the project, made by a run that read nothing about it. +#[test] +fn the_check_heading_says_the_list_went_unread_rather_than_counting_zero() { + let unread_dir = scratch("swift_check_heading_unread"); + std::fs::copy( + fixture("sample-swift/Package.swift"), + unread_dir.join("Package.swift"), + ) + .unwrap(); + + let empty_dir = scratch("swift_check_heading_empty"); + std::fs::copy( + fixture("sample-swift/Package.swift"), + empty_dir.join("Package.swift"), + ) + .unwrap(); + std::fs::write( + empty_dir.join("Package.resolved"), + r#"{"pins":[],"version":2}"#, + ) + .unwrap(); + + let heading = |dir: &Path| { + let output = run(&[ + "check", + "--manifest", + dir.join("Package.swift").to_str().unwrap(), + "--no-vuln", + ]); + String::from_utf8_lossy(&output.stdout).into_owned() + }; + + let unread = heading(&unread_dir); + assert!( + !unread.contains("(0 dependencies)"), + "nothing was counted because nothing was read: {unread}" + ); + assert!( + unread.contains("Swift (dependency list unread)"), + "the heading has to say so: {unread}" + ); + + // And a project that really is resolved and really has no pins still counts. + let empty = heading(&empty_dir); + assert!( + empty.contains("Swift (0 dependencies)"), + "a resolved project with no pins declares none, and says so: {empty}" + ); } From 2c68e69494996be0e61638c3d44469d574c82f56 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Sun, 6 Sep 2026 12:18:30 -0400 Subject: [PATCH 23/24] fix(cli): carry the unread and undetermined caveats into the GitHub summary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The job summary and the pull-request annotations were the last surface still reporting a Swift project as a clean, fully checked one. `--annotations auto` is on by default inside Actions, so this is the surface CI actually renders. `totals()` formatted its line from `vulnerable`, `outdated`, `error` and `up_to_date` and never read `summary.undetermined` or `summary.manifests_unread`, both of which `Summary::of` already computes. Every checkable Swift pin is `Undetermined`, a status `level_of` does not annotate, so no table was built, so the `tables.is_empty()` branch printed "No outdated or vulnerable dependencies found." A project with four pins rendered as 4 dependencies checked — 0 vulnerable, 0 outdated, 0 errors, 0 up to date. No outdated or vulnerable dependencies found. and one with no committed `Package.resolved` — the common state, since Apple advises library packages not to commit one — rendered the same all-clear with a total of zero. The totals line now carries `undetermined`, a coverage caveat names the manifests whose dependency list went unread, and the all-clear is withheld unless the run established it. A manifest that produced no rows at all also gets one `::notice` of its own, keyed per manifest for the same reason SARIF keys `DEP003` that way: there is no dependency to hang the finding on, and no annotation is indistinguishable from a clean project. Those notices lead the notice level and share its ten-per-step cap rather than taking a second one, which would let twenty be emitted of which ten never render. Wording is taken from the surfaces that already draw this line — the table heading, the HTML report, `manifests_unread` in JSON, `DEP003` in SARIF — rather than invented again here. --- crates/dependable/src/output/github.rs | 186 +++++++++++++++++++++++-- crates/dependable/tests/cli_github.rs | 161 +++++++++++++++++++++ 2 files changed, 338 insertions(+), 9 deletions(-) diff --git a/crates/dependable/src/output/github.rs b/crates/dependable/src/output/github.rs index df18537..fe22188 100644 --- a/crates/dependable/src/output/github.rs +++ b/crates/dependable/src/output/github.rs @@ -102,6 +102,13 @@ impl Level { /// declared constraint is noise on a pull request. It still appears in the table /// and in the job summary's totals. /// +/// `Undetermined` is excluded too, but for a different reason and with a +/// different remedy. It is not a per-dependency finding a reviewer can act on — +/// in a registryless ecosystem *every* row carries it — so annotating each one +/// would spend the whole ten-per-step budget saying the same thing. The fact is +/// instead carried by the job summary, in [`totals`] and in [`caveats`], and by +/// [`unread_command`] where the dependency list itself went unread. +/// /// Levels are independent of `--fail-on`. Deriving the level from whether a /// finding trips the gate would make a vulnerability a warning under /// `--fail-on outdated`, and would silence everything under `--fail-on none`. @@ -428,6 +435,42 @@ fn elision(level: Level, omitted: usize) -> String { ) } +/// The `title=` carried by the annotation for a manifest whose dependency list +/// went unread. Distinct from [`Level::title`], because the subject is a file +/// rather than a dependency. +const UNREAD_TITLE: &str = "dependable: dependency list could not be read"; + +/// The `::notice` for one manifest whose dependency list went unread. +/// +/// Per **manifest**, not per dependency — it exists precisely because there is no +/// dependency to emit one against, which is the same reason SARIF keys `DEP003` +/// that way. Without it such a manifest reaches a pull request as no annotation +/// at all, and no annotation is exactly what a clean project looks like. +/// +/// No `line=`: the missing information is a file that is *not there*, so there is +/// no line in the manifest to point at. The `file=` still names the manifest. +fn unread_command(report: &ManifestReport, workspace: Option<&Path>, cwd: Option<&Path>) -> String { + let file = relative_file(&report.path, workspace, cwd); + let manifest = file + .clone() + .unwrap_or_else(|| report.path.display().to_string()); + let mut properties = Vec::new(); + if let Some(file) = &file { + properties.push(format!("file={}", escape_property(file))); + } + properties.push(format!("title={}", escape_property(UNREAD_TITLE))); + format!( + "::{} {}::{}", + Level::Notice.token(), + properties.join(","), + escape_data(&format!( + "The dependency list for {manifest} could not be read, so no dependency in it \ + was checked. An empty result set for this manifest means nothing was looked at, \ + not that nothing is wrong." + )) + ) +} + /// Every line to write to stderr: the workflow commands, plus a plain elision /// note for each level that had to be capped. /// @@ -445,12 +488,41 @@ pub fn annotations( .into_iter() .enumerate() { + // The unread-list notices lead the notice level and share its cap rather + // than getting one of their own: GitHub's ten-per-step limit is per + // level, so a second independent cap would let twenty notices be emitted + // of which ten silently never render. Leading, because they are the only + // thing this channel has to say about a manifest that produced no rows at + // all — everything behind them is still named in the job summary, which + // is not capped at ten. + let leading: Vec = if level == Level::Notice { + reports + .iter() + .filter(|report| report.dependencies_unread) + .map(|report| unread_command(report, workspace, cwd)) + .collect() + } else { + Vec::new() + }; let findings = &grouped[slot]; - for finding in findings.iter().take(MAX_ANNOTATIONS_PER_LEVEL) { + let total = leading.len() + findings.len(); + let mut emitted = 0; + for line in leading { + if emitted == MAX_ANNOTATIONS_PER_LEVEL { + break; + } + lines.push(line); + emitted += 1; + } + for finding in findings { + if emitted == MAX_ANNOTATIONS_PER_LEVEL { + break; + } lines.push(command(finding, level)); + emitted += 1; } - if findings.len() > MAX_ANNOTATIONS_PER_LEVEL { - lines.push(elision(level, findings.len() - MAX_ANNOTATIONS_PER_LEVEL)); + if total > emitted { + lines.push(elision(level, total - emitted)); } } lines @@ -515,18 +587,98 @@ fn counted(count: usize, singular: &str, plural: &str) -> String { } /// The totals line, from the same [`Summary`] the table renderer uses. -fn totals(reports: &[ManifestReport]) -> String { - let summary = Summary::of(reports); +/// +/// `undetermined` is carried here for the same reason [`crate::output::table`] +/// carries it: a dependency whose currency was never established is neither up +/// to date nor outdated, so leaving it out of the line makes every one of them +/// vanish into a row of zeros. A Swift project — every checkable pin of which is +/// undetermined, because the ecosystem publishes no registry — would otherwise +/// render as a fully checked project with nothing wrong. +fn totals(summary: &Summary) -> String { format!( - "{} checked — {} vulnerable, {} outdated, {}, {} up to date.", + "{} checked — {} vulnerable, {} outdated, {}, {} undetermined, {} up to date.", counted(summary.total, "dependency", "dependencies"), summary.vulnerable, summary.outdated + summary.update_available, counted(summary.error, "error", "errors"), + summary.undetermined, summary.up_to_date + summary.patch_available ) } +/// The manifests whose dependency list went unread, named the way an annotation +/// names them: repository-relative where that is possible, absolute otherwise. +fn unread_manifests( + reports: &[ManifestReport], + workspace: Option<&Path>, + cwd: Option<&Path>, +) -> Vec { + reports + .iter() + .filter(|report| report.dependencies_unread) + .map(|report| { + relative_file(&report.path, workspace, cwd) + .unwrap_or_else(|| report.path.display().to_string()) + }) + .collect() +} + +/// The coverage caveat: what this run did **not** establish, said in the one +/// place a line of zeros would otherwise be read as denying it. +/// +/// Every count in [`totals`] is a tally of rows that were read *and* compared +/// against a registry. An undetermined row was read and never compared; an +/// unread dependency list produced no rows at all. Both therefore land as zeros, +/// and without this block the summary of a project nothing was established about +/// is byte-identical to the summary of a genuinely clean one. +/// +/// The manifests are named for the same reason SARIF's `DEP003` names them: the +/// fact belongs to a file, not to a dependency, because there is no dependency +/// to hang it on. +/// +/// Empty when there is nothing to caveat, so a clean run is unchanged. +fn caveats( + reports: &[ManifestReport], + summary: &Summary, + workspace: Option<&Path>, + cwd: Option<&Path>, +) -> String { + let unread = unread_manifests(reports, workspace, cwd); + if summary.undetermined == 0 && unread.is_empty() { + return String::new(); + } + let mut out = String::from("**Coverage caveat**\n\n"); + if summary.undetermined > 0 { + let _ = writeln!( + out, + "- {} could not be checked for a newer version, so this run establishes \ + nothing about their currency and the totals above do not claim it.", + counted(summary.undetermined, "dependency", "dependencies") + ); + } + if !unread.is_empty() { + let mut list = unread + .iter() + .take(MAX_ROWS_PER_TABLE) + .map(|manifest| code_cell(manifest)) + .collect::>() + .join(", "); + if unread.len() > MAX_ROWS_PER_TABLE { + let _ = write!(list, ", … {} more", unread.len() - MAX_ROWS_PER_TABLE); + } + let _ = writeln!( + out, + "- The dependency list for {} could not be read, so no dependency in {} was \ + checked. An empty result set for such a manifest means nothing was looked at, \ + not that nothing is wrong: {list}.", + counted(unread.len(), "manifest", "manifests"), + if unread.len() == 1 { "it" } else { "them" } + ); + } + out.push('\n'); + out +} + /// The advisory cell: a Markdown link where there is a canonical page, else the /// bare advisory IDs. fn advisory_cell(result: &CheckResult) -> String { @@ -663,13 +815,29 @@ pub fn summary_markdown( cwd: Option<&Path>, ) -> String { let grouped = group(reports, workspace, cwd); - let head = format!("## dependable\n\n{}\n\n", totals(reports)); + let summary = Summary::of(reports); + let head = format!( + "## dependable\n\n{}\n\n{}", + totals(&summary), + caveats(reports, &summary, workspace, cwd) + ); let tables = tables(&grouped); if tables.is_empty() { // An empty summary is indistinguishable from a step that never ran, so - // say so explicitly. - let out = format!("{head}No outdated or vulnerable dependencies found.\n\n"); + // say so explicitly — but only where the run actually established it. + // With an undetermined dependency, or a manifest whose dependency list + // went unread, there is nothing to have found: "no outdated or vulnerable + // dependencies found" would turn "we did not look" into "we looked and + // found nothing", and the caveat above is the whole of what can honestly + // be said. Every other surface already draws this line — the table + // heading, the HTML report, `manifests_unread` in JSON, `DEP003` in + // SARIF — and the job summary is the one a pull request actually shows. + let out = if summary.undetermined == 0 && summary.manifests_unread == 0 { + format!("{head}No outdated or vulnerable dependencies found.\n\n") + } else { + head + }; return if out.len() <= budget { out } else { diff --git a/crates/dependable/tests/cli_github.rs b/crates/dependable/tests/cli_github.rs index 385cf7a..f2014fa 100644 --- a/crates/dependable/tests/cli_github.rs +++ b/crates/dependable/tests/cli_github.rs @@ -236,3 +236,164 @@ fn quiet_empties_stdout_but_keeps_the_annotations() { // `-q` means "only print errors", and the annotations *are* the errors. assert!(stderr_of(&output).contains("::notice ")); } + +/// A `Package.swift` is a Swift program, so it declares nothing readable. The +/// pins are the dependency list, and every versioned one is `Undetermined`: +/// Swift publishes no registry, so no currency can be established for any of them. +const PACKAGE_SWIFT: &str = "// swift-tools-version:5.10\nimport PackageDescription\n\nlet package = Package(name: \"SampleApp\")\n"; + +/// Two `remoteSourceControl` pins carrying versions — two `Undetermined` rows, +/// and not one row of any other status. +const PACKAGE_RESOLVED: &str = r#"{ + "pins" : [ + { + "identity" : "swift-nio", + "kind" : "remoteSourceControl", + "location" : "https://github.com/apple/swift-nio.git", + "state" : { "revision" : "635b2589494c97e48c62514bc8b37ced762e0a62", "version" : "2.65.0" } + }, + { + "identity" : "swift-log", + "kind" : "remoteSourceControl", + "location" : "https://github.com/apple/swift-log.git", + "state" : { "revision" : "9cb486020ebf03bfa5b5df985387a14a98744537", "version" : "1.5.4" } + } + ], + "version" : 2 +}"#; + +/// A Swift project, with a `Package.resolved` beside it only when `resolved` says so. +/// +/// Deliberately *not* [`workdir`]: a `Cargo.toml` in the same directory would +/// contribute rows of its own and the counts below would stop meaning anything. +fn swift_workdir(name: &str, resolved: bool) -> PathBuf { + let dir = PathBuf::from(env!("CARGO_TARGET_TMPDIR")).join(name); + let _ = fs::remove_dir_all(&dir); + fs::create_dir_all(&dir).unwrap(); + fs::write(dir.join("Package.swift"), PACKAGE_SWIFT).unwrap(); + if resolved { + fs::write(dir.join("Package.resolved"), PACKAGE_RESOLVED).unwrap(); + } + fs::write(dir.join(".dependable.toml"), CONFIG).unwrap(); + dir +} + +/// The undetermined case: the pins were read, and not one of them could be +/// checked for a newer version. +/// +/// Every one is `Undetermined`, a status the renderer does not annotate, so no +/// table is built — and an empty table set used to reach the all-clear line. That +/// line is the same false claim of currency the table heading, the HTML report, +/// `manifests_unread` in JSON and `DEP003` in SARIF were all changed to stop +/// making; the job summary is the one a pull request actually renders. +#[test] +fn the_summary_never_calls_an_undetermined_swift_project_clean() { + let dir = swift_workdir("github_swift_undetermined", true); + let summary = dir.join("summary.md"); + let output = check( + &dir, + &["--annotations", "always"], + &[ + ("GITHUB_WORKSPACE", dir.to_str().unwrap()), + ("GITHUB_STEP_SUMMARY", summary.to_str().unwrap()), + ], + ); + assert!(output.status.success(), "{}", stderr_of(&output)); + let markdown = fs::read_to_string(&summary).expect("a summary file"); + + assert!( + !markdown.contains("No outdated or vulnerable dependencies found."), + "nothing was found because nothing could be checked: {markdown}" + ); + assert!(markdown.contains("2 dependencies checked"), "{markdown}"); + assert!(markdown.contains("2 undetermined"), "{markdown}"); + assert!(markdown.contains("**Coverage caveat**"), "{markdown}"); + assert!( + markdown.contains("2 dependencies could not be checked for a newer version"), + "{markdown}" + ); +} + +/// The unread case, which this feature calls the common state: Apple advises a +/// library package not to commit its `Package.resolved`, so there is nothing to +/// read and the run produces no rows at all. +/// +/// Worse than the undetermined case, because every status count is a tally of +/// rows that *were* read: with no rows the totals line is a row of zeros +/// identical to a project with no dependencies. Only the caveat separates them. +#[test] +fn the_summary_never_calls_an_unread_swift_project_clean() { + let dir = swift_workdir("github_swift_unread", false); + let summary = dir.join("summary.md"); + let output = check( + &dir, + &["--annotations", "always"], + &[ + ("GITHUB_WORKSPACE", dir.to_str().unwrap()), + ("GITHUB_STEP_SUMMARY", summary.to_str().unwrap()), + ], + ); + assert!(output.status.success(), "{}", stderr_of(&output)); + let markdown = fs::read_to_string(&summary).expect("a summary file"); + let stderr = stderr_of(&output); + + assert!( + !markdown.contains("No outdated or vulnerable dependencies found."), + "a project nothing was read from must never render as a clean one: {markdown}" + ); + assert!(markdown.contains("**Coverage caveat**"), "{markdown}"); + assert!( + markdown.contains("The dependency list for 1 manifest could not be read"), + "{markdown}" + ); + // Named, the way `DEP003` names it: the fact belongs to a file. + assert!(markdown.contains("`Package.swift`"), "{markdown}"); + + // And the pull request itself hears about it. With no rows there is no + // per-dependency annotation to emit, so without this one the annotation + // channel is silent — which is exactly what a clean project looks like. + assert!( + stderr.contains( + "::notice file=Package.swift,title=dependable%3A dependency list could not be read::" + ), + "{stderr}" + ); + assert!( + stderr.contains("means nothing was looked at, not that nothing is wrong."), + "{stderr}" + ); +} + +/// The guard on the guard: gating the all-clear line must not withhold it from a +/// run that genuinely earned it. +#[test] +fn a_genuinely_clean_run_still_gets_the_all_clear() { + let dir = PathBuf::from(env!("CARGO_TARGET_TMPDIR")).join("github_clean_all_clear"); + let _ = fs::remove_dir_all(&dir); + fs::create_dir_all(&dir).unwrap(); + // No dependencies at all: nothing to check, and nothing left unchecked. + fs::write( + dir.join("Cargo.toml"), + "[package]\nname = \"sample\"\nversion = \"0.1.0\"\n", + ) + .unwrap(); + fs::write(dir.join(".dependable.toml"), CONFIG).unwrap(); + + let summary = dir.join("summary.md"); + let output = check( + &dir, + &["--annotations", "always"], + &[ + ("GITHUB_WORKSPACE", dir.to_str().unwrap()), + ("GITHUB_STEP_SUMMARY", summary.to_str().unwrap()), + ], + ); + assert!(output.status.success(), "{}", stderr_of(&output)); + let markdown = fs::read_to_string(&summary).expect("a summary file"); + + assert!( + markdown.contains("No outdated or vulnerable dependencies found."), + "{markdown}" + ); + assert!(!markdown.contains("Coverage caveat"), "{markdown}"); +} From 27840d33e0fc85548f3bb463cd4a50db1f526b22 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Sun, 6 Sep 2026 12:18:38 -0400 Subject: [PATCH 24/24] fix(cli): stop fix calling an unread project up to date MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `run_fix` counted only `Undetermined` rows, and a manifest whose dependency list went unread produces no rows at all. Both counters therefore stayed at zero and the run fell through to "Everything is already up to date." — the exact claim the honest branch beside it exists to prevent, made about a project nothing was read from. `report.dependencies_unread` was already on the same struct and was never consulted. It is now counted beside the undetermined rows and routed to the same non-clean branch. The two reasons are worded apart because they are different facts: an undetermined dependency was read and could not be checked against a registry, while an unread dependency list was never read, so there is not even a list of dependencies to have failed to check. A run with both says both. The existing test could not catch this: it runs against `sample-swift/`, which has a `Package.resolved` pinning four packages, so the undetermined count is four and the guard fired on that count alone. The new test deletes the file the case is about. Output for the already-covered path is unchanged. --- crates/dependable/src/runner.rs | 45 ++++++++++++++++++++---- crates/dependable/tests/fixture_swift.rs | 44 +++++++++++++++++++++++ 2 files changed, 83 insertions(+), 6 deletions(-) diff --git a/crates/dependable/src/runner.rs b/crates/dependable/src/runner.rs index 79420a9..413235f 100644 --- a/crates/dependable/src/runner.rs +++ b/crates/dependable/src/runner.rs @@ -907,11 +907,20 @@ pub async fn run_fix(args: FixArgs) -> anyhow::Result { let engine = Engine::new(&settings, &cfg, true)?; let mut total = 0; let mut unchecked = 0; + // Counted beside `unchecked`, not folded into it. `unchecked` is a tally of + // rows, and a manifest whose dependency list went unread produces none — so a + // Swift project with no `Package.resolved`, the state Apple advises library + // packages to be in, left both this loop's counters at zero and reached the + // clean closing line below with nothing having been read at all. + let mut unread = 0; for manifest in &manifests { let Some(report) = engine.check_manifest(manifest).await? else { continue; }; report_inherited_skips(manifest, &report); + if report.dependencies_unread { + unread += 1; + } unchecked += report .results .iter() @@ -931,7 +940,7 @@ pub async fn run_fix(args: FixArgs) -> anyhow::Result { total += 1; } } - if total == 0 && unchecked == 0 { + if total == 0 && unchecked == 0 && unread == 0 { println!("Everything is already up to date."); } else if total == 0 { // "Up to date" is a claim about versions that were compared against a @@ -940,11 +949,35 @@ pub async fn run_fix(args: FixArgs) -> anyhow::Result { // was established, and printing the clean line anyway turns "we did not // look" into "we looked and found nothing", which is the one thing a fix // run must never say. - println!( - "Nothing to rewrite. {unchecked} dependenc{} could not be checked for a newer \ - version; see the warnings above.", - if unchecked == 1 { "y" } else { "ies" } - ); + // + // The two reasons are worded apart because they are different facts: an + // undetermined dependency *was* read and could not be checked, while an + // unread dependency list was never read, so there is not even a list of + // dependencies to have failed to check. + let undetermined_phrase = |count: usize| { + format!( + "{count} dependenc{} could not be checked for a newer version", + if count == 1 { "y" } else { "ies" } + ) + }; + let unread_phrase = |count: usize, lead: &str| { + format!( + "{lead} dependency list for {count} manifest{} could not be read, so nothing \ + in {} was checked", + if count == 1 { "" } else { "s" }, + if count == 1 { "it" } else { "them" } + ) + }; + let why = match (unchecked, unread) { + (0, unread) => unread_phrase(unread, "The"), + (unchecked, 0) => undetermined_phrase(unchecked), + (unchecked, unread) => format!( + "{}, and {}", + undetermined_phrase(unchecked), + unread_phrase(unread, "the") + ), + }; + println!("Nothing to rewrite. {why}; see the warnings above."); } else if !args.dry_run { println!( "\nUpdated {total} dependenc{}.", diff --git a/crates/dependable/tests/fixture_swift.rs b/crates/dependable/tests/fixture_swift.rs index d72119e..e4947cf 100644 --- a/crates/dependable/tests/fixture_swift.rs +++ b/crates/dependable/tests/fixture_swift.rs @@ -425,6 +425,50 @@ fn fix_never_claims_a_swift_project_is_up_to_date() { assert!(before.contains("for (name, version) in extraPackages")); } +/// The same requirement in the state Apple actually advises a library package to +/// be in: a `Package.swift` with **no** `Package.resolved` beside it. +/// +/// The test above cannot catch this one. `sample-swift/` has a `Package.resolved` +/// pinning four versioned packages, so `fix` counts four `Undetermined` rows and +/// takes the honest branch on the strength of that count alone. Delete the +/// resolved file and there is no list to read, so there are *no rows* — the +/// undetermined count is zero, and a run that read nothing at all used to fall +/// straight through to "Everything is already up to date." +#[test] +fn fix_never_claims_an_unread_swift_project_is_up_to_date() { + let dir = scratch("swift_fix_unread"); + std::fs::copy( + fixture("sample-swift/Package.swift"), + dir.join("Package.swift"), + ) + .unwrap(); + assert!( + !dir.join("Package.resolved").exists(), + "the whole point of this case is the file that is not there" + ); + + let output = run(&[ + "fix", + "--manifest", + dir.join("Package.swift").to_str().unwrap(), + "--dry-run", + ]); + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + + assert!(output.status.success(), "stderr: {stderr}"); + assert!( + !stdout.contains("Everything is already up to date"), + "nothing was read, so there is nothing to call up to date; stdout: {stdout}" + ); + // Worded apart from the undetermined case on purpose: an undetermined + // dependency was read and could not be checked, this one was never read. + assert!( + stdout.contains("The dependency list for 1 manifest could not be read"), + "stdout: {stdout}" + ); +} + /// Live: the one verdict a Swift run can actually give. `SwiftURL` advisories are /// keyed by the repository URL with no scheme and no `.git`, and getting that /// wrong fails silently — it reports a vulnerable package as clean — so only a