feat(swift): Package.resolved reader + SwiftURL OSV scanning (no registry client) - #97
Open
justin13888 wants to merge 24 commits into
Open
feat(swift): Package.resolved reader + SwiftURL OSV scanning (no registry client)#97justin13888 wants to merge 24 commits into
justin13888 wants to merge 24 commits into
Conversation
SwiftPM's Package.resolved is plain JSON carrying the full flattened pin set: identity, location URL, revision, and version for every resolved package. Read v2 (Swift 5.6+) and v3 (Xcode 15+), whose pin shapes are identical, plus v1's differently-spelled one, because reporting a Swift 5.5 project as depending on nothing is the silent wrong answer. The pin set is the only record of a Swift project's dependencies - Package.swift is executable Swift and cannot be read as text honestly - so this lockfile is the source of the dependency list rather than an annotation on one. LockfileKind::is_dependency_source and lockfile_items are that path; apply_lockfile's insert-nothing contract is untouched for the five formats that already have it. Names are normalized to the URL OSV keys SwiftURL advisories by: scheme and .git suffix removed. Either left on matches nothing and reports a vulnerable package as clean.
Ecosystem::Swift, whose osv_name is SwiftURL and whose package name is a repository URL rather than anything a registry issued. Ecosystem::has_registry() is new and is false for Swift alone. It states a fact about the ecosystem, which the absence of a registered fetcher cannot: without it, "the user turned this ecosystem off" and "there is nothing to turn on" are the same observation, and they want opposite behaviour. default_registry is empty for Swift and the two must agree. package_url reassembles the repository URL; version_url falls back to it, because a git tag's spelling is not derivable from the version and a link to the wrong one 404s. No registry link is fabricated. ManifestKind::PackageSwift's parser reads no dependencies on purpose: Package.swift is a Swift program whose dependencies are assembled in loops, behind conditionals, and from variables, so a text-level reader returns a confidently wrong list rather than a short one. The list comes from Package.resolved instead.
check_inner returned UnsupportedEcosystem whenever no RegistryFetcher was registered, which the CLI prints as "skipping ...: <Eco> is not enabled or not yet supported". A registry-less ecosystem was therefore dropped before the OSV scan ran - not degraded, absent. It now branches on Ecosystem::has_registry(). An ecosystem that has no registry proceeds with no fetch at all and every checkable dependency reported Undetermined, then scans OSV as usual: that scan needs a package and a version, not a registry, and the lockfile supplied both. The discriminator is the point. has_registry() is true for the other nine ecosystems, so a config-disabled one still takes the old UnsupportedEcosystem path and the CLI still says it is not enabled. Config-disabled and structurally-registry-less must not collapse into one branch, and there are tests on both sides of that line. A lockfile that IS the dependency list may now supply items; apply_lockfile still only annotates, for the five formats that always relied on it. License collection is skipped where there is no metadata endpoint to ask, rather than warning about it once per manifest.
For every ecosystem with a registry, registering the fetcher IS the on switch: a Checker without one skips those manifests. An ecosystem with nothing to register would have had no off switch at all, so `[swift] enabled = false` would have been a config key that did nothing. CheckerBuilder::registryless is that switch. Off by default, exactly as every non-Rust ecosystem is, and declining it stays possible - the answers a registry-less ecosystem gives are shaped differently from every other one's, reporting vulnerable but never outdated.
The hazard is precise: a Swift run that turns up no advisories looks exactly like a clean, up-to-date one, and a reader who is not told otherwise will read it that way. DependencyStatus::Undetermined says so per row, in a table nobody is obliged to read column by column; this says it once per manifest, in the same place and as loudly as the unreadable-lockfile notices - for every Swift manifest, including one that pins nothing. With --no-vuln the notice says nothing was established at all, rather than naming a scan that did not run. CLI: [swift] enabled, which reaches the checker through CheckerBuilder::registryless so it actually switches something off. list takes its items from Package.resolved for the one lockfile that is a dependency source; find_lockfile's path is untouched for the other five. Policy accepts swift/swiftpm/spm as an ecosystem word. Fixtures: a Package.swift that assembles its dependencies in a loop and behind a #if - the reason it is never read - with a v3 Package.resolved beside it and a v2 one under legacy/, asserted to yield identical pins. No span round-trip assertion, because there is no span: the tests assert Swift items have no position and are not rewritable, which is why fix.rs needs no change. The live OSV test is #[ignore]d and was run once by hand: SwiftURL matched GHSA-r6r4-5pr8-gjcp for vapor 4.83.0.
`fix` ended every run that rewrote nothing with "Everything is already up to date." A Swift project rewrites nothing by construction - its versions live in Package.resolved, not in any file this tool parses - so that line was a flat claim of currency on the one ecosystem that can never establish it, printed directly beneath a warning saying the opposite. A run that rewrote nothing and left dependencies undetermined now says how many could not be checked instead. "We did not look" must never be printed as "we looked and found nothing".
Two claims went stale as the branch landed. The comment above the lockfile block said apply_lockfile never inserts, full stop, which is now only true of the branch it introduces. And ProgressEvent promised one Started -> Finished cycle per call: an ecosystem with no registry fetches nothing and emits neither, which is worth stating so a UI knows it will never be left with a bar running. README now also says that Package.resolved records the flattened resolution, so a Swift project lists its transitive pins beside its direct ones - more than every other ecosystem shows, not less.
`head -c 700 Package.resolved` was enough to panic the process: an object or array that ends before its closing delimiter still advanced the cursor past it, and the next `self.bytes[self.i..]` in `skip_trivia` indexed off the end. A closing delimiter where a value belongs had the matching problem in the other direction — `skip_scalar` consumed nothing, so the enclosing loop handed it the same byte forever. Both are reachable from any half-written JSON file, in any of the six ecosystems that read one. The cursor now reads through `rest()`, which yields the empty slice every caller already meant, and a scalar that makes no progress is consumed. Adds `scan_document`, which reports whether the scan reached the end of the document. `scan_strings` keeps its exact contract — a prefix of the strings, for the readers that annotate a list some manifest already produced — but a reader of a file that *is* a dependency list cannot tell a short answer from a complete one, and now has something to ask.
Three claims this reader was making that the file does not support. **A malformed file read as a short list.** Every other lockfile here annotates items a manifest already produced, so a pin it misses costs a locked version. This one *is* the list, so a pin it misses is a dependency that is never scanned for advisories — handed back, with no warning, as the complete set. A truncated file now reports as unread: `swift_package_resolved_items` returns `None` and `parse_swift_package_resolved` returns a `ParseError`, which is what puts the existing "could not be parsed" notice in front of the reader. **The repository path was matched case-sensitively against a key that is not.** OSV keys `SwiftURL` byte for byte and real keys are mixed-case — `github.com/weichsel/ZIPFoundation`, `github.com/marmelroy/Zip`, `github.com/migueldeicaza/SwiftTerm` — so `GitHub.com/vapor/vapor` matched nothing and reported a vulnerable package clean. The host is now lowercased, which is safe because hostnames are case-insensitive by definition and every OSV key spells one lowercase. The path is deliberately left exactly as written: lowercasing it would break precisely the mixed-case keys that exist. `swift_package_name_variants` offers the all-lowercase spelling as a second key to ask about. One case stays unreachable and is documented in the module and the README: a lowercase spelling in the file of a repository keyed mixed-case cannot be canonicalized without the forge. **Every pin was reported as a direct dependency.** `Package.resolved` records the flattened resolution and marks no pin apart, so `"direct": true` in `list --format json` was a claim the file never made — a project depending only on `swift-nio-ssl` gets pins for `swift-nio`, `swift-collections` and `swift-atomics` too. Pins are now `DependencyKind::Indirect`, the kind that declines to claim directness. The fixture gains a genuinely transitive pin, so the case is actually exercised.
…un that did not fetch Two corrections to what a check states about itself. A Swift pin whose repository path is not lowercase is now queried under the written spelling *and* the all-lowercase one. OSV matches its `SwiftURL` keys byte for byte while a git forge treats the path case-insensitively, so the same repository circulates under either, and a miss is silent — a vulnerable package simply reports clean. The second query costs one batch entry, only for a name that is not already lowercase, and can add only a true match, since OSV answers about the package it was asked about or not at all. Every other ecosystem still asks exactly one question per result; IDs are appended and deduplicated rather than assigned, which for a single query is the same list in the same order. `currency_is_unknowable` now also requires that no fetcher ran. It branched on `Ecosystem::has_registry` alone, so a library consumer registering a fetcher for `Ecosystem::Swift` — the plausible SE-0292 future — got real `UpToDate` rows *and* a warning saying nothing there could be checked. And where the count is zero it no longer says "the 0 dependencies here". That phrasing turns "we could not look" into a claim about the project, which is the exact inversion this notice exists to prevent; the lockfile notice names the cause instead.
`locate_lockfile` and `find_lockfile` walk up to the `.git` boundary, which is right while a lockfile *annotates* items a manifest produced: a workspace member's `Cargo.lock` at the root pins the very crates the member declared, and a pin it does not declare simply goes unused. It is wrong now that one lockfile *is* the item list. A nested SwiftPM package with no `Package.resolved` of its own adopted its ancestor's and reported the ancestor's dependencies as its own — with scanning on, attributing the root's advisories to a package that does not have the dependency, a false positive on the one verdict Swift can give. In a SwiftPM monorepo that is every package not yet resolved. A dependency source now counts only in the manifest's own directory; the five annotating formats keep the ancestor walk unchanged, and a test holds each of them to it. The state that walk was papering over also needed saying out loud. Apple advises library packages not to commit `Package.resolved`, so a Swift project with none is the common case, and nothing distinguished it from one that resolved to nothing: `check` reported "0 dependencies" and exited 0 even under `--fail-on any`. A missing dependency-source lockfile is now a `LockfileNotice` of its own, carrying the same loudness as the unreadable-lockfile notices and naming the cause, and `LockfileNotice::dependency_list_unread` carries into the exit code — `--fail-on any` asks whether everything here is checked and current, and a list nobody read cannot answer yes. A missing *annotating* lockfile is still no notice at all: it costs a locked version, not a dependency list.
Three things the Swift section did not say, each now visible in the tool's own output: no pin is reported as a direct dependency, because `Package.resolved` records the flattened resolution and marks none apart; a `Package.swift` with no readable `Package.resolved` beside it reports as unknown rather than as zero dependencies, and fails `--fail-on any`; and a `Package.resolved` counts only in its own directory, so a nested package never adopts the root's pins. Also records the one limitation that remains: OSV keys `SwiftURL` advisories case-sensitively while a git forge does not, so a lowercase spelling in the file of a repository whose advisory is keyed mixed-case matches nothing. The host is lowercased and the all-lowercase path is queried alongside the written one, which covers every direction but that.
…t/85-swift-package-resolved
OSV keys a Swift package by `github.com/apple/swift-nio`, with no port. A pin written `ssh://git@github.com:22/apple/swift-nio.git` would otherwise ask about `github.com:22/apple/swift-nio` — a key OSV has never heard of — so the same repository at the same version comes back clean through one URL and vulnerable through another. A port addresses the transport, not the package. Only an all-digit suffix counts, which is the test the SCP-shorthand branch of `swift_package_name` already applies: `github.com:vapor` stays a path. An IPv6 literal is unharmed, since `[::1]` ends in `]` rather than a digit, while `[::1]:22` loses only the port.
`--no-lock-file` is documented as "ignore sibling lockfiles (do not report locked versions)": it suppresses the `locked_at` column, an annotation on a list the manifest already produced. A `Package.resolved` is not that — it *is* the list, because a `Package.swift` is a program this tool declines to read. Honouring the flag there did not withhold a version column, it reported a Swift project as depending on nothing at all, which is the inversion this ecosystem's support exists to prevent. `apply_nearest_lockfile` now takes the flag itself and applies it to the annotating half only, so a dependency-source lockfile is read regardless and the flag keeps exactly the meaning its help text claims.
An empty `results` array is what a clean project and an unread one share. Every status count tallies rows that *were* read, so a manifest nothing was read from contributes zero to all of them and its document is otherwise byte-identical to a genuinely clean project's — the report asserts "no findings" about a project it learned nothing about. Only a SwiftPM project can reach this state today: a `Package.swift` is a program the checker declines to read, so with no readable `Package.resolved` beside it there is no dependency list at all. `ManifestResults` carries the fact, `Summary` counts it as `manifests_unread`, `check --format json` exposes that count, and SARIF gains `DEP003` — emitted per *manifest* rather than per dependency, precisely because there are no dependencies to emit one against, and pushed before that manifest's rows so a consumer reading the array in order meets the caveat first. Both JSON changes are additive: a consumer pinned to the documented shape is unaffected, and one that wants the distinction gates on `manifests_unread > 0`.
Exercises the fixture through the CLI rather than the parser: the pins become dependencies, an unreadable list is reported rather than rendered as clean, and `--no-lock-file` still lists the project's dependencies.
…t/85-swift-package-resolved
`Checker::read_lockfile` returned `None` on `!read_lockfiles` before it ever looked at what kind of lockfile it had found, so `--no-lock-file` (and `[global] lock_file = false`) emptied a Swift project's dependency list rather than dropping a version column: `check` reported "0 dependencies", the OSV scan ran over an empty item list, and a project with a known-vulnerable pin exited 0. `list` was taught the distinction in c0f8c86; `check` never reached that code. The lockfile is now located first and the switch applied only to one that annotates, matching `apply_nearest_lockfile`. `is_dependency_source()` is true only for `Package.resolved`, so no annotating lockfile changes behaviour. Also corrects `CheckArgs::no_lock_file`, whose help still read "Ignore `Cargo.lock`" — c0f8c86 updated `ListArgs::no_lock_file` beside it and missed this one.
`git@github.com:owner/repo` writes a colon where a URL writes a slash, and the reader decided which it was by looking at the segment after it. Both directions of that guess were wrong. An owner beginning with a digit kept the colon — `git@github.com:1024jp/GzipSwift.git` yielded `github.com:1024jp/GzipSwift`, where OSV holds `github.com/1024jp/GzipSwift`, so a real and widely used package always reported clean (`0xOpenBytes/*` and `4np/*` are others). An owner that is *all* digits lost its segment instead: `git@github.com:42/pkg.git` yielded `github.com/pkg`, a well-formed key naming a different repository, which nothing downstream can recognise as garbage and which can collide with a real advisory key. The two shapes are textually identical, so the segment can never decide between them. The form of the location can: a port is URL syntax and only ever follows a scheme, while SCP shorthand has none. `split_authority` now takes that as its input and never inspects the digits. An IPv6 literal is bracketed, so the scan for either separator starts after the `]`. `swift_package_name_variants` also folds ASCII rather than Unicode now, matching the host: OSV's keys are ASCII, and a Unicode fold can change a string's byte length and hand OSV a key nobody wrote.
Three of the rule's string literals were wrapped across source lines without `\` continuations, so the emitted JSON carried runs of 14-18 literal spaces — text a GitHub Code Scanning alert renders verbatim. DEP003 also wrote `properties.status = "unread"`. That key otherwise always holds a `DependencyStatus::token()`, so a consumer switching on it exhaustively met a word no status can produce. DEP003 names no dependency and has no dependency status: `status` is now absent for it, and the fact it was carrying gets its own key, `dependencyListUnread`. Every other finding's `properties` is byte-identical to before.
`report` built its `ManifestResults` without `with_dependencies_unread`, on both the HTML path and the policy path, so the fact reached the document only as a run note — and `runner.rs` gates every note on `!args.quiet`. `dependable report --quiet -o report.html` over a Swift project with no `Package.resolved` therefore produced an artifact indistinguishable from a resolved, clean one, right down to §3 printing "This manifest declares no dependencies", which nothing had established. Both paths now set the flag, `Summary` counts the manifests it covers, and the templates state it: a coverage caveat in §1 and, in §3, a sentence in place of the "declares no dependencies" claim. A caveat about what a report does not cover is not chatter, so it survives `--quiet` by being part of the model rather than a note beside it. A resolved project with no pins is unchanged.
`split_authority` decided port-vs-path from whether a scheme was present, and on the scheme branch it dropped whatever followed the colon without looking at it. `ssh://git@github.com:vapor/vapor.git` — a scheme written in front of git's SCP shorthand — therefore normalized to `github.com/vapor`: a well-formed OSV `SwiftURL` key naming a *different* repository, so the scan answered about a package nobody asked about and the real one reported clean. `https://host:notaport/x/y` lost `notaport` the same way. The scheme still decides whether a port is possible at all; the digits now decide whether this one is. A non-numeric segment keeps its colon, which yields a key that matches nothing — a miss a reader can see, rather than a wrong answer nobody can. The no-scheme branch is untouched: there a colon is always SCP's path separator, whatever follows it, which is what keeps `git@github.com:42/pkg.git` and `1024jp/GzipSwift` intact. Reachability is narrow — a non-numeric port is invalid URL syntax and SwiftPM will not write one into a working `Package.resolved` — so this needs a hand-edited or generated file, the same class as the empty-version case already guarded here. IPv6 literals are unaffected: the scan for a separator still starts after the closing `]`, and the zone-id form is now pinned by a test too.
§3's per-manifest heading read `Package.swift — Swift (0 dependencies)` directly above the paragraph explaining that this is *not* a project with no dependencies. A count is a claim about the project, so the heading asserted precisely what the prose beneath it, the §1 coverage caveat and DEP003 all exist to disclaim — and a heading is what a reader skimming §3 actually reads. `check`'s per-manifest heading said the same thing in the same words. Both now read `(dependency list unread)` when there is nothing to count *and* the file that would have said so went unread. A project that really is resolved and really declares nothing still counts zero and says so, in both outputs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #85.
Swift has no canonical registry, so this ships without a
RegistryFetcherat all. The result reports vulnerable but can never report outdated, and--fixcannot apply. That limitation is stated loudly — per dependency and per manifest — rather than left for a reader to infer from a clean run.What lands
Package.resolvedreader (v2, v3, and v1's spelling) asLockfileKind::PackageResolved. It is the source of the dependency list, not an annotation on one:Package.swiftis executable Swift — the fixture assembles its dependencies in a loop and behind a#if, which is exactly why a text-level reader returns a wrong list rather than a short one — so it is never read.LockfileKind::is_dependency_source+lockfile_itemsare that path;apply_lockfile's insert-nothing contract is unchanged for the five formats that already rely on it.Ecosystem::Swift,osv_name() == "SwiftURL", package name = the repository URL with its scheme and.gitsuffix stripped. Both matter:github.com/vapor/vapormatches,github.com/vapor/vapor.gitreturns{}, and a wrong key fails silently by reporting a vulnerable package as clean. Verified against the live API. No registry link is ever fabricated —package_urlreassembles the repository,version_urlfalls back to it (a git tag's spelling is not derivable from the version),default_registryis empty.ManifestKind::PackageSwift, whose parser returns an empty item list on purpose.check_innerused to returnUnsupportedEcosystemwhenever no fetcher was registered, which the CLI prints asskipping … is not enabled or not yet supported— Swift's manifest would have been dropped before the scan ran, and the whole feature would have been silently absent. It now branches onEcosystem::has_registry().DependencyStatus::Undetermined— the variant feat(jvm): pom.xml parser — literal versions and same-file properties only #95 added, reused verbatim, not duplicated — with a manifest-level warning saying why, on every Swift manifest including one that pins nothing.The discriminator
has_registry()istruefor all nine other ecosystems, so a config-disabled ecosystem still takes the oldUnsupportedEcosystempath and still printsskipping … is not enabled or not yet supported. Config-disabled and structurally-registry-less must not collapse into one branch;check.rshas tests on both sides, over three disabled ecosystems and Swift.Two things surfaced while building that:
[swift] enabled = falsewould have been a dead config key. For every other ecosystem, registering the fetcher is the on switch; Swift has nothing to register.CheckerBuilder::registrylessis that switch — off by default, exactly as every non-Rust ecosystem is — and there is a CLI test that a disabled Swift project is skipped and says nothing further.fixtold a Swift project it was already up to date. It ended every run that rewrote nothing withEverything is already up to date., and a Swift project rewrites nothing by construction — a flat claim of currency printed directly beneath a warning saying the opposite. A run that rewrote nothing and left dependencies undetermined now says how many could not be checked.Not included, deliberately
No manifest parser, no registry fetcher, no
graph_parser—Package.resolvedrecords pins, not edges, sotreereportsGraphSource::Unsupported, which is the shippedpubspec.lockbehaviour reused as-is. Nofix.rschange: Swift items carry no span, sois_rewritable()is alreadyfalse— asserted by test rather than assumed, in place of the span round-trip every other fixture test does.Tests
sample-swift/carries a v3Package.resolvedand a v2 one underlegacy/, asserted to yield identical pins. Plus CLI tests forcheck,list,fix, and the disabled-ecosystem skip. The live OSV test is#[ignore]d somise run teststays hermetic; it was run once by hand andSwiftURLmatchedGHSA-r6r4-5pr8-gjcpforvapor4.83.0.mise run fmt:check,mise run lint,mise run test— all clean (41 test binaries, 0 failures).Stack
Four deep, merge in order:
refactor/83-…feat/82-…feat/84-pom-xml-parser← this PR's baseRepairs from review
An independent review confirmed the
has_registry()fork changed nothing for the other nine ecosystems (680 stdout/stderr pairs, zero diffs) and raised six defects. All six are repaired here, each with a regression test proven to fail before the repair.Package.resolved(High). The upward lockfile walk is safe while a lockfile annotates a list a manifest produced; it is not safe now that one is the list. A nested package with noPackage.resolvedreported the root's dependencies as its own, and with scanning on attributed the root's advisories to a package that has neither. A dependency source is now accepted only from the manifest's own directory; the five annotating formats keep the ancestor walk, with a test that holds each of Cargo, npm, Bun, Composer and mix to it.SwiftURLkey was matched without normalizing case (High). The host is now lowercased unconditionally — hostnames are case-insensitive and every OSV key spells one lowercase, which also fixesGitHub.com/…. The path is left exactly as written, because OSV matches byte for byte and real keys are mixed-case (github.com/weichsel/ZIPFoundation,github.com/marmelroy/Zip,github.com/migueldeicaza/SwiftTerm); a blanketto_lowercase()would break precisely those, and there is a test that fails if anyone tries it. On top of that, a name whose path is not already lowercase is queried under both spellings and the results unioned — one extra batch entry, only for such names, and it can add only a true match. One residual limitation, stated in the module docs and the README: aPackage.resolvedrecording a lowercase spelling of a repository whose advisory is keyed mixed-case still matches nothing, and that package reports clean. The canonical casing is a fact only the forge holds; nothing in the file recovers it.Package.resolvedclaimed zero dependencies as fact (High). Apple advises library packages not to commit one, so this is the common state. It now raises aLockfileNoticeof its own through the existing lockfile-notice channel — same loudness as theUnreadableLockfilenotices — naming the cause and how to fix it, the "the 0 dependencies here" phrasing is gone, and--fail-on anyexits non-zero, because a list nobody read cannot be reported clean.list --format jsonmarked every Swift pin"direct": true(Medium).Package.resolvedis the flattened resolution and marks no pin apart, so pins are nowDependencyKind::Indirect, the kind that declines the claim. The fixture gains a genuinely transitive pin (swift-atomics, pulled in byswift-nioand declared nowhere) so the case is exercised.Package.resolvedpanicked (Medium).json_scanwalked the cursor past the end of the buffer and indexed off it; a closing delimiter where a value belongs looped forever. Both are fixed in the shared scanner, since both are crashes. The policy is in the Swift reader: a malformedPackage.resolvedreports as unread, not as a partial success, because a partial scan of this file is a silently short dependency list presented as the whole one.currency_is_unknowableignored whether a fetcher ran (Low). It now also requiresfetcher.is_none(), so a consumer registering an SE-0292 fetcher forEcosystem::Swiftno longer gets realUpToDaterows beside a warning that nothing could be checked.One cross-ecosystem output change
The
run_fixchange in this PR — replacing the unconditionalEverything is already up to date.— also changes the closing line for the JVM.sample-mavennow ends withNothing to rewrite. 2 dependencies could not be checked for a newer version…instead of the old flat claim. It is correct and an improvement, and it is kept, but it is not a Swift-only change: a JVM user's CI log will differ.The other nine ecosystems are otherwise byte-identical. Re-verified after these repairs: 17 non-Swift fixtures × 7 commands (
list,list --format json,check --no-vuln,check --no-vuln --format json,check --fail-on any,tree,fix --dry-run), plus purpose-built ancestor-lockfile repositories for all five annotating formats with the lockfile a directory above the manifest — 134 stdout/stderr/exit-code triples, zero diffs.Unresolved review notes
Four findings from the same review are not repaired here, each for a reason.
dependable-tuidrives the checker through its own path and never callscheck_path, so the manifest-level warning that every other frontend prints cannot reach it; it also shows a Swift project with zero children whilechecklists its pins. This is the same family as the already-filed TUI defect fix(tui): a dependency whose version was never read renders as up to date #96, which the maintainer chose to handle as a follow-up, and repairing it means touchingtui/andtree.rs— out of scope for a repair pass on this PR.PackageSource::Inheritedis overloaded (Medium). It now has four producers meaning four different things (a Cargo workspace inheritance, a POM deferring to its parent, a pnpm catalog, and a Swift pin whose version is written in a file no manifest parser read). The reviewer suggests a distinctLockedsource. That is a core data-model change every PR in this four-deep stack depends on; changing it in the last PR of the stack would force the three below it to be revisited.DependencyStatus::Undeterminedproduces no SARIF finding, so a Swift scan uploads an empty run. That is the deliberate feat(jvm): pom.xml parser — literal versions and same-file properties only #95 decision about whatUndeterminedmeans in SARIF, not a Swift defect; changing it changes SARIF output for the JVM too.kind: "registry"pin produces a key that can never match (Low). Such a pin has nolocation, so the reader falls back toidentity(mona.linkedlist), which is a registry identifier and not a URL — it is queried againstSwiftURLand matches nothing, silently. No public SE-0292 registry has meaningful adoption yet, so no suchPackage.resolvedexists in the wild today; the honest fix is to skip a registry-kind pin rather than query a key that cannot match, and it belongs with whatever ships registry support.Follow-up commits
Four further commits land on this branch.
fix(core): strip a port from a Swift package host— OSV keys a Swift package asgithub.com/apple/swift-nio, with no port. A pin writtenssh://git@github.com:22/apple/swift-nio.gitasked aboutgithub.com:22/apple/swift-nio, a key OSV has never heard of, so the same repository at the same version came back clean through one URL and vulnerable through another. Only an all-digit suffix is stripped — the same test the SCP-shorthand branch already applies, sogithub.com:vaporstays a path — and an IPv6 literal is unharmed because[::1]ends in]rather than a digit.fix(cli): let --no-lock-file suppress annotations, not the list itself— the flag is documented as "do not report locked versions", an annotation on a list the manifest already produced. APackage.resolvedis the list. Honouring the flag there did not withhold a column, it reported a Swift project as depending on nothing at all.apply_nearest_lockfilenow takes the flag and applies it to the annotating half only.feat(report): say when a project's dependency list went unread— closes the residual reported in this PR's own review notes for the machine-readable surfaces. Every status count tallies rows that were read, so a manifest nothing was read from contributes zero to all of them and its document was otherwise byte-identical to a genuinely clean project's.ManifestResultsnow carries the fact,Summarycounts it asmanifests_unread,check --format jsonexposes that count, and SARIF gainsDEP003— emitted per manifest rather than per dependency, precisely because there are no dependencies to emit one against, and pushed before that manifest's rows so a consumer reading the array in order meets the caveat first. Both JSON changes are additive: a consumer pinned to the documented shape is unaffected; one that wants the distinction gates onmanifests_unread > 0.test(swift): cover a Package.resolved project end to end— exercises the fixture through the CLI rather than the parser.origin/feat/84-pom-xml-parseris also merged in, so this branch now contains the whole stack beneath it plusmaster.Validation
cargo test --workspace— 41 test binaries, 0 failures.cargo clippy --workspace --all-targets -- -D warnings— clean.cargo fmt --all --check— clean.convco check origin/master..HEAD— no errors in 35 commits.Still deferred
The
PackageSource::Inheritedoverload noted above is now filed as #98 and is being taken as its own change stacked on this branch, which is what its "wants doing deliberately rather than inside a feature PR" note asks for. The TUI limitation remains #96.Review repairs
An independent review of this PR ran the built binary and confirmed five defects. All five are fixed in four commits on this branch, each with a test that fails before it and passes after. The six design questions the review left open are answered at the bottom.
1. HIGH —
check --no-lock-fileemptied a Swift project's dependency listChecker::read_lockfilereturnedNoneon!read_lockfilesbefore it looked at what kind of lockfile it had found, so--no-lock-file(and[global] lock_file = false) did tocheckexactly whatc0f8c86had just stopped it doing tolist.Reproduced on
0ab4e6a:The OSV scan runs over the resulting empty item list, so a Swift project with a known-vulnerable pin reported clean and exited 0 — a silent security false negative, and the exact inversion
c0f8c86's own message claims to repair.dependencies_unreadstayedfalse(the file is there and parses), so neithermanifests_unreadnor SARIFDEP003caught it either, and the warning that was printed — "no dependency with a version to check was found here at all" — was false; there were four.Fix (
fix(fetch): let --no-lock-file suppress annotations for check too): the lockfile is located first and the switch applied only to one that annotates, matching whatapply_nearest_lockfilealready does forlist.is_dependency_source()is true only forLockfileKind::PackageResolved, which appears inManifestKind::lockfiles()only forPackageSwift, so no annotating lockfile changes behaviour.CheckArgs::no_lock_file, whose help still read "IgnoreCargo.lock", is corrected too —c0f8c86updatedListArgs::no_lock_filebeside it and missed this one.Tests:
check::tests::lockfiles_off_still_reads_the_lockfile_that_is_the_dependency_listand…_still_suppresses_an_annotating_lockfile(both halves of the switch, at theCheckerlevel);no_lock_file_does_not_empty_what_check_scansinfixture_swift.rscovers the CLI, including the--fail-on anyexit code and the--format jsoncounts. The first fails on the old code withleft: [],right: ["github.com/apple/swift-nio", "helpers"].After: the same command lists all six pins, warns that four are undetermined rather than that none exist, and
--fail-on anyexits 1.2 & 3. MEDIUM / LOW — the SCP-shorthand colon was read as a port when the segment looked numeric
Two faces of one heuristic.
git@github.com:1024jp/GzipSwift.gitproducedgithub.com:1024jp/GzipSwift(OSV holdsgithub.com/1024jp/GzipSwift), so a real and widely used package always reported clean —0xOpenBytes/*and4np/*are others.git@github.com:42/pkg.gitproducedgithub.com/pkg: a well-formed key naming a different repository, which nothing downstream can recognise as garbage and which can collide with a real advisory key. Reproduced throughlist --format jsonover aPackage.resolvedholding both.Fix (
fix(core): read an SCP-shorthand colon as a path separator, never a port): the two shapes are textually identical, so no test applied to the colon's neighbours can separate them — but the form of the location can. A port is URL syntax and only ever follows a scheme; SCP shorthand has no scheme by definition.split_authoritytakes that as its input and never inspects the digits. An IPv6 literal is bracketed, so the scan for either separator starts after the].One existing expectation changes with it:
github.com:22/apple/swift-nio.git, written with no scheme, is nowgithub.com/22/apple/swift-nio. That is the same string shape asgithub.com:42/pkgand under the new rule must read the same way; it moves into the new test with the rest.Tests:
an_scp_shorthand_colon_is_a_path_separator_whatever_follows_itcovers1024jp/GzipSwift,42/pkg,vapor/vapor,ssh://…:22/apple/swift-nioandhttps://…:443/apple/swift-nio;an_ipv6_literal_keeps_its_colonscovers[::1]:22and[2001:db8::1]. Both fail against the old implementation.After: the four SCP pins read
github.com/1024jp/GzipSwift,github.com/42/pkg,github.com/vapor/vapor, and both schemed forms readgithub.com/apple/swift-nio.4. LOW — DEP003's SARIF strings carried runs of literal spaces
Three string literals wrapped across source lines without
\continuations, so the emitted JSON read"…no dependency in it was checked."— text a GitHub Code Scanning alert renders verbatim. Fixed with continuations, as the codebase does elsewhere.Test:
sarif_reports_an_unread_dependency_list_as_a_findingnow asserts that the message,fullDescription, andhelpcontain no double space.5. LOW — the HTML report rendered an unread Swift project as clean
reportbuilt itsManifestResultswithoutwith_dependencies_unreadon both the HTML path and the policy path, so the fact reached the document only as a run note — and every note is gated on!args.quiet.dependable report --quiet -o report.htmlover a Swift project with noPackage.resolvedproduced an artifact indistinguishable from a resolved one, right down to §3 printing "This manifest declares no dependencies", which nothing had established. Reproduced by diffing the quiet and loud artifacts: the caveat was the only difference.Fix (
fix(report): carry an unread dependency list into the report itself): both paths set the flag,Summarycounts it asmanifests_unread, and the templates state it — a coverage caveat in §1, and in §3 a sentence in place of the "declares no dependencies" claim.Test:
a_quiet_html_report_still_says_the_dependency_list_went_unreadrenders both an unread and a genuinely-empty project under--quietand asserts the caveat appears for one and not the other;an_unread_manifest_is_counted_apart_from_an_empty_onepins the counter itself. The CLI test fails on the old code at the first assertion.Design decisions
Recorded here because the review raised them and they were settled rather than deferred.
list --no-lock-filekeeps reporting"lockfile": "Package.resolved". No change. It did read the file, and the flag's contract is about locked-version annotations, not about concealing what was read.currency_is_unknowable's "no dependency with a version to check was found here at all" keeps its wording. With finding 1 fixed it is no longer reachable for a suppressed Swift list. The remaining paths to it are honest: it fires only when the item list genuinely holds nothing with a version, which after this change means the pins really are all path/branch entries — a true statement about a real, non-empty list.properties.status. That key otherwise always holds aDependencyStatus::token(), so a consumer switching on it exhaustively met"unread", which is not a status. DEP003 names no dependency and has no dependency status:statusis now absent for it and the fact moves to its own key,dependencyListUnread. DEP003 is new and unreleased, so the change costs nothing; every other finding'spropertiesis byte-identical to before.manifests_unreadstays a summary-only count. No change. SARIFDEP003already names the manifest, which is the surface a consumer uses to locate it; the counter is additive and sufficient.--quiet. It is a correctness caveat about what the report does not cover, not chatter, so it belongs in the model rather than in a note beside it — which is what finding 5's fix does.swift_package_name_variantsused Unicodeto_lowercase()whilelowercase_hostusedto_ascii_lowercase(). Hostnames and OSV keys are ASCII, and a Unicode fold can change a string's byte length, silently producing a key for a repository nobody wrote. Both are ASCII now.Validation
cargo test --workspace— 41 test binaries, 948 passed, 0 failed, 21 ignored (the#[ignore]d live-network set).cargo clippy --workspace --all-targets -- -D warnings— clean.cargo fmt --all --check— clean.Second repair round
Two Lows from the re-review, both silent-wrong-answer defects rather than crashes.
6. LOW —
split_authoritydropped a non-numeric segment after a colon when a scheme was presentThe earlier repair decided port-vs-path from whether a URL scheme was present, and on the scheme branch it discarded whatever followed the colon without checking it was a port:
github.com/vaporis a well-formed OSVSwiftURLkey naming a different repository — the class this PR's own commit message calls "the worse of the two, because nothing about it looks wrong". The scan answers about a package nobody asked about, and the real one reports clean.Reachability is narrow and stated honestly: a non-numeric port is invalid URL syntax and SwiftPM will not write one into a working
Package.resolved, so this needs a hand-edited or generator-produced file — the same class as the empty-version case this stack already guards.Fix (
fix(core): a port is digits, so a non-numeric segment is not one): the scheme still decides whether a port is possible; the digits now decide whether this one is, viaport.bytes().all(|b| b.is_ascii_digit())on thehas_schemebranch only. A non-numeric segment keeps its colon, yieldinggithub.com:vapor/vapor— a key that matches nothing, which is a miss a reader can see rather than a wrong answer nobody can. The no-scheme branch is untouched: there a colon is always SCP shorthand's path separator whatever follows it, which is what keepsgit@github.com:42/pkg.gitand1024jp/GzipSwiftintact.Test:
a_non_numeric_segment_after_a_scheme_is_not_a_portcoversssh://git@github.com:vapor/vapor.gitandhttps://host:notaport/x/y, and re-confirmsssh://git@github.com:22/…andhttps://github.com:443/…still strip. It fails on the old code withleft: "github.com/vapor".an_ipv6_literal_keeps_its_colonsgains the zone-id casessh://git@[fe80::1%25eth0]:22/…, so the IPv6 handling this branch introduced is pinned against regression too.7. LOW — §3's heading counted "0 dependencies" for a list nobody read
The HTML report's per-manifest
<summary>readPackage.swift — Swift (0 dependencies)directly above the paragraph explaining that this is not a project with no dependencies. A count is a claim about the project, so the heading asserted exactly what the prose beneath it, the §1 coverage caveat andDEP003all exist to disclaim — and a heading is the half a reader skimming §3 actually reads.check's per-manifest heading said the same thing in the same words.Fix (
fix(report): head an unread dependency list as unread, not as zero): both headings read(dependency list unread)when there is nothing to count and the file that would have said so went unread. A project that really is resolved and really declares nothing still counts zero and says so, in both outputs.Test:
a_quiet_html_report_still_says_the_dependency_list_went_unreadgains assertions on the heading itself for both the unread and the genuinely-empty project;the_check_heading_says_the_list_went_unread_rather_than_counting_zerodoes the same for the table output. Both fail on the old code.list's own header still prints(0 dependencies)for an unread manifest:ProjectReportdoes not carry the flag, and plumbing it there is a JSON-schema change rather than a wording one.listdoes emit the explanatory warning on stderr.Gates
948 before; the two new tests take it to 950. No test was deleted or weakened.