diff --git a/README.md b/README.md index bf899ed..d45c914 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,45 @@ 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. 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. 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 A lockfile is what turns "the manifest allows `^19.0.0`" into "you are actually @@ -70,6 +110,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 +133,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-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/item.rs b/crates/dependable-core/src/item.rs index 4f3bbd5..7caab5b 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 450c96c..c9db268 100644 --- a/crates/dependable-core/src/lib.rs +++ b/crates/dependable-core/src/lib.rs @@ -23,10 +23,12 @@ 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_name_variants, + swift_package_resolved_items, }; pub use manifest::{ AlternateRegistryDecl, LockfileKind, ManifestKind, ParsedManifest, UNREADABLE_MANIFESTS, @@ -36,10 +38,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/lockfiles/mod.rs b/crates/dependable-core/src/lockfiles/mod.rs index b18d73d..d8f7426 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,10 @@ 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_name_variants, + swift_package_resolved_items, +}; /// Parse lockfile `content` with the parser for the file that was found. /// @@ -44,6 +50,34 @@ 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. +/// +/// `None` **also** when the kind is a dependency source whose file did not read. +/// Both answers mean the same thing to a caller β€” "no dependency list came from +/// this file" β€” and both leave it to fall through to [`parse_lockfile_kind`], +/// which reports the failure. Handing back a prefix of a truncated +/// `Package.resolved` would instead present a short list as a complete one. +#[must_use] +pub fn lockfile_items(kind: LockfileKind, content: &str) -> Option> { + match kind { + 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 new file mode 100644 index 0000000..542fa80 --- /dev/null +++ b/crates/dependable-core/src/lockfiles/swift_package_resolved.rs @@ -0,0 +1,791 @@ +//! 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. +//! +//! 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_document; + +/// 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, 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. +/// +/// # 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) -> Option> { + Some(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 +/// 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 items { + 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. +/// +/// # Case +/// The **host** is lowercased unconditionally: hostnames are case-insensitive by +/// definition, and every OSV `SwiftURL` key spells one in lowercase, so +/// `GitHub.com/vapor/vapor` would otherwise match nothing. +/// +/// The **path is left exactly as written**, because OSV's keys are case-sensitive +/// and mixed-case ones are real β€” `github.com/weichsel/ZIPFoundation`, +/// `github.com/marmelroy/Zip`, `github.com/migueldeicaza/SwiftTerm`. Lowercasing +/// the path would break precisely those. +/// +/// That leaves one case nothing local can repair: a `Package.resolved` that +/// records a *lowercase* spelling of a repository whose OSV key is mixed-case +/// (`…/zipfoundation` for `…/ZIPFoundation`). Git clones either spelling happily +/// and SwiftPM lowercases `identity`, so both spellings circulate; recovering the +/// canonical one needs the forge, not this string. The other direction *is* +/// repaired β€” see [`swift_package_name_variants`], which the OSV scan queries +/// alongside the name β€” so only "written lower, keyed mixed" is missed. +#[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(); + } + + let name = name.trim_end_matches('/'); + let name = name.strip_suffix(".git").unwrap_or(name); + normalize_host(name.trim_end_matches('/'), scheme.is_some()) +} + +/// Lowercase `name`'s host and join it to the path below it with a `/`, leaving +/// that path exactly as written. +/// +/// 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 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(), + } +} + +/// Split `name` into its host and the path beneath it, dropping the separator (and +/// a port, where there is one). +/// +/// # 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 *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 `]`. +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 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 { + // 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) + } +} + +/// 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. +/// +/// 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_ascii_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. +/// +/// `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 scanned.values { + 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), + _ => {} + } + } + Some(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, + // `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, + }) +} + +/// 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 +} +"#; + + /// 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() + .find(|item| item.name == name) + .unwrap_or_else(|| panic!("no pin {name}")) + } + + #[test] + fn reads_every_pin_as_a_dependency() { + let items = 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(&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. + #[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!(items(&v3), 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 = 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 = 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 = 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}"); + } + } + + /// 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", + ), + // 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 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 an_scp_shorthand_colon_is_a_path_separator_whatever_follows_it() { + let cases = [ + // 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"), + // 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}"); + } + } + + /// 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] + 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"), + // 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]"), + ]; + 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 + /// 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(); + 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!(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 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-core/src/manifest.rs b/crates/dependable-core/src/manifest.rs index c8c438d..144a892 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, @@ -373,6 +385,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 { @@ -386,9 +403,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 { @@ -400,6 +433,7 @@ impl LockfileKind { LockfileKind::ComposerLock, LockfileKind::PubspecLock, LockfileKind::MixLock, + LockfileKind::PackageResolved, ] .into_iter() .find(|kind| kind.file_name() == name) @@ -444,6 +478,7 @@ mod tests { ManifestKind::GradleVersionCatalog, ), ("services/api/pom.xml", ManifestKind::PomXml), + ("app/Package.swift", ManifestKind::PackageSwift), ]; for (path, expected) in cases { assert_eq!( @@ -479,6 +514,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()); } @@ -507,6 +543,7 @@ mod tests { ManifestKind::Csproj, ManifestKind::GradleVersionCatalog, ManifestKind::PomXml, + ManifestKind::PackageSwift, ] { assert!(kind.workspace_roots().is_none(), "{kind:?}"); assert!( @@ -559,6 +596,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] @@ -571,6 +631,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); } @@ -585,6 +649,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/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"] } }"#; 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 abf158a..3c83673 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 diff --git a/crates/dependable-fetch/src/check.rs b/crates/dependable-fetch/src/check.rs index 7bda56f..b36babe 100644 --- a/crates/dependable-fetch/src/check.rs +++ b/crates/dependable-fetch/src/check.rs @@ -13,8 +13,9 @@ 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, swift_package_name_variants, + to_semver_constraint, }; use futures::stream::{self, StreamExt}; use semver::Version as SemverVersion; @@ -41,6 +42,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 { @@ -158,6 +163,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 @@ -386,7 +395,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(()); @@ -404,7 +417,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)), } } @@ -567,15 +591,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)) } @@ -588,11 +623,22 @@ impl Checker { workspace: WorkspaceContext, ) -> 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() && self.registryless.contains(&ecosystem) => None, + None => return Err(CheckError::UnsupportedEcosystem(ecosystem)), + }; let mut parsed = parse(kind, manifest)?; @@ -619,45 +665,68 @@ impl Checker { WorkspaceContext::Unsearched => {} } - // 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. - if let Some((lock_kind, lock)) = lockfile - && let Ok(data) = parse_lockfile_kind(lock_kind, lock) - { - apply_lockfile(&mut parsed.items, &data); + // 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 + // 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); } + if let Some(warning) = currency_is_unknowable( + ecosystem, + &parsed.items, + self.osv.is_some(), + fetcher.is_some(), + ) { + 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 @@ -669,7 +738,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}")); @@ -914,6 +987,97 @@ 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 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. +/// +/// `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(); + 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 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" + ) + } 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 { + 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, + // A coordinate this manifest could not state is the same shape of + // ignorance reached through the name instead of the version: nothing was + // asked, so nothing is known. `Local` would again say the wrong thing β€” + // that there is no registry behind the entry, rather than that this file + // never said which package it is. + PackageSource::Unidentified => 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( @@ -923,23 +1087,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, - // A coordinate this manifest could not state is the same shape of - // ignorance reached through the name instead of the version: nothing was - // asked, so nothing is known. `Local` would again say the wrong thing β€” - // that there is no registry behind the entry, rather than that this file - // never said which package it is. - PackageSource::Unidentified => DependencyStatus::Undetermined, - _ => DependencyStatus::Local, - }; - return CheckResult::new(item.clone(), status); + return unfetchable(item); } match fetched.get(&item.name) { Some(Ok(versions)) => { @@ -1146,20 +1294,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 @@ -1173,7 +1341,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); } @@ -1187,7 +1355,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; } } @@ -1205,6 +1382,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, @@ -1227,6 +1405,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, @@ -1281,6 +1460,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. @@ -1338,7 +1535,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 @@ -1431,6 +1634,7 @@ impl CheckerBuilder { registries, rust_registries, jsr: self.jsr, + registryless: self.registryless.into_iter().collect(), osv, advisory_details: self.advisory_details, licenses: self.licenses, @@ -1466,6 +1670,401 @@ 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) + .registryless(Ecosystem::Swift) + .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); + } + + /// `--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`. + /// 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" + ); + } + } + + /// 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" + ); + } + + /// 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 + ); + } + + /// 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] + 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" + ); + } + + /// 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(); @@ -1473,7 +2072,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"); @@ -1483,14 +2082,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] @@ -1498,7 +2097,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] @@ -1509,7 +2108,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`. diff --git a/crates/dependable-fetch/src/discover.rs b/crates/dependable-fetch/src/discover.rs index 1a7aa1f..a84b977 100644 --- a/crates/dependable-fetch/src/discover.rs +++ b/crates/dependable-fetch/src/discover.rs @@ -114,6 +114,7 @@ fn walk( found.notices.push(LockfileNotice { path, reason: unreadable.reason.to_owned(), + dependency_list_unread: false, }); } } @@ -210,23 +211,46 @@ fn is_superseded(dir: &Path, scan_root: Option<&Path>, unreadable: &UnreadableMa } } +/// 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)); @@ -249,15 +273,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) @@ -434,6 +465,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 { @@ -461,26 +502,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 @@ -640,6 +710,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-report/src/html/model.rs b/crates/dependable-report/src/html/model.rs index e840fa3..dfcbf04 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,12 @@ 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" 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, } @@ -372,6 +382,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 +502,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..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 %} @@ -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/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/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-report/src/sarif.rs b/crates/dependable-report/src/sarif.rs index caf3eb3..6676f33 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,11 +267,12 @@ 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(), + 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()), @@ -303,11 +321,58 @@ 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(), + status: Some(result.status.token()), + dependency_list_unread: None, + 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, + }, + } +} + +/// 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, + // 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, @@ -499,7 +564,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 +610,29 @@ 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 +680,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,13 +815,32 @@ 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, + /// 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")] @@ -848,11 +955,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-report/src/summary.rs b/crates/dependable-report/src/summary.rs index acd86b0..957f6f6 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: @@ -205,6 +214,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) @@ -303,6 +315,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/cli.rs b/crates/dependable/src/cli.rs index c9e3082..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. @@ -138,7 +141,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/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/output/github.rs b/crates/dependable/src/output/github.rs index b2e5e26..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 { @@ -815,6 +983,7 @@ mod tests { ecosystem: Ecosystem::Rust, results, workspace_root: None, + dependencies_unread: false, } } 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 e556afb..a93fef2 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. @@ -60,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 { @@ -89,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 } @@ -185,6 +204,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..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(()) @@ -84,6 +90,7 @@ mod tests { ecosystem: Ecosystem::Rust, results: Vec::new(), workspace_root: None, + dependencies_unread: false, } } 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/src/runner.rs b/crates/dependable/src/runner.rs index 736e92c..72e8d4c 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()); } @@ -251,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 { @@ -262,6 +269,7 @@ impl Engine { ecosystem: check.ecosystem, results: check.results, workspace_root: check.workspace_root, + dependencies_unread, })) } Err(CheckError::UnsupportedEcosystem(eco)) => { @@ -285,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 @@ -400,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 } @@ -602,7 +624,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) { @@ -643,9 +665,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); @@ -686,12 +707,38 @@ 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, - items: &mut [Item], + annotations: bool, + 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)); + } + if !annotations { + return None; + } let (path, resolved) = dependable_fetch::find_lockfile(manifest, kind)?; apply_lockfile(items, &resolved); Some(relative_to(root, &path)) @@ -859,11 +906,26 @@ 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() + .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; @@ -878,8 +940,44 @@ pub async fn run_fix(args: FixArgs) -> anyhow::Result { total += 1; } } - if total == 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 + // 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. + // + // 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{}.", @@ -1084,11 +1182,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() @@ -1366,7 +1470,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/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}"); +} 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. diff --git a/crates/dependable/tests/fixture_swift.rs b/crates/dependable/tests/fixture_swift.rs new file mode 100644 index 0000000..e4947cf --- /dev/null +++ b/crates/dependable/tests/fixture_swift.rs @@ -0,0 +1,988 @@ +//! 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", + "github.com/apple/swift-atomics", + "sample-helpers", + "github.com/acme/swift-experimental", + ], + "every pin, in the order the file records them" + ); +} + +/// `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] +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}"); +} + +/// `--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 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. +#[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] +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}" + ); +} + +/// `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")); +} + +/// 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 +/// 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}"); +} + +/// 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 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 +/// 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}" + ); + } +} + +/// 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_dir = scratch("swift_sarif_unread"); + std::fs::copy( + fixture("sample-swift/Package.swift"), + unread_dir.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_dir); + 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!( + 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:?}" + ); + } +} + +/// 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" + ); + // Β§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. + 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")); + 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}" + ); +} 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..ff3794f --- /dev/null +++ b/crates/dependable/tests/fixtures/sample-swift/Package.resolved @@ -0,0 +1,58 @@ +{ + "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" : "swift-atomics", + "kind" : "remoteSourceControl", + "location" : "https://github.com/apple/swift-atomics.git", + "state" : { + "revision" : "cd142fd2f64be2100422d658e7411e39489da985", + "version" : "1.2.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..bbdee8e --- /dev/null +++ b/crates/dependable/tests/fixtures/sample-swift/Package.swift @@ -0,0 +1,34 @@ +// 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. +// +// 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] = [ + .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..edd36a4 --- /dev/null +++ b/crates/dependable/tests/fixtures/sample-swift/legacy/Package.resolved @@ -0,0 +1,57 @@ +{ + "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" : "swift-atomics", + "kind" : "remoteSourceControl", + "location" : "https://github.com/apple/swift-atomics.git", + "state" : { + "revision" : "cd142fd2f64be2100422d658e7411e39489da985", + "version" : "1.2.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..bbdee8e --- /dev/null +++ b/crates/dependable/tests/fixtures/sample-swift/legacy/Package.swift @@ -0,0 +1,34 @@ +// 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. +// +// 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] = [ + .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")] +)