From 89febd99612f68d4a68fbe1422286cac86f563eb Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 03:40:48 +0000 Subject: [PATCH] maven: honour direct-dependency exclusions, resolve per module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects found while reviewing a Java codebase, both in the Maven resolver. Exclusions declared on a direct dependency were silently dropped. The parser read into RawDependency.exclusions correctly, but the core Dependency had no field to carry them, so the set was discarded one line later and _walk seeded the frontier with an empty set because nothing was left to seed with. The result was a false positive of the worst kind: com.lowagie:itext reported with a high, unfixable XXE advisory in a project whose pom.xml explicitly excludes it. Exclusions now travel on Dependency, are matched with Maven's whole-segment wildcards, and are picked up from dependencyManagement at both project and transitive level. Half-declared elements are dropped at parse time so they cannot become accidental wildcards. Because this is the one code path that removes packages rather than adding them, the negative tests that assert a wildcard does not fire are as load-bearing as the positive ones, and each dependency's exclusions are emitted in JSON so a removal stays auditable. Multi-module projects were merged into a single resolve pass. One `seen` map across a reactor let whichever module was walked first decide every shared coordinate's version, `known` dropped a module's transitive if any other module declared it directly, `_backfill` pooled one managed dict across every POM, and every transitive was stamped with roots[0].source — a file that need not lead to it. Each scan unit now resolves against its own POM on a shared resolver, so the POM cache, connection pool and node budget are still shared, and a transitive carries the SourceLocation and parent of the declaration that introduced it. Verified against Maven Central: displaytag 1.2 with the exclusion yields 8 packages and no itext, without it 9 including itext; a two-module reactor attributes each module's transitives to its own POM. Also adds the first scanner-level test, a legend for the table's bare `~`, and skill guidance stating what may be concluded about Maven provenance — the absence of which is what sent a reviewing agent off to verify findings by hand. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01By6nqX2dcU351oZn6y6Xoq --- CHANGELOG.md | 8 +- CLAUDE.md | 28 +- README.md | 3 +- .../.agents/skills/icebergsca/SKILL.md | 12 + .../icebergsca/references/json-report.md | 16 +- src/icebergsca/core/models.py | 6 + src/icebergsca/core/scanner.py | 48 ++- src/icebergsca/ecosystems/maven/__init__.py | 4 +- src/icebergsca/ecosystems/maven/model.py | 21 ++ src/icebergsca/ecosystems/maven/parser.py | 58 ++- src/icebergsca/ecosystems/maven/resolver.py | 254 +++++++++++--- src/icebergsca/report/json_.py | 4 + src/icebergsca/report/table.py | 7 + tests/conftest.py | 41 ++- tests/fixtures/maven/multimodule/pom.xml | 13 + .../maven/multimodule/service-a/pom.xml | 18 + .../maven/multimodule/service-b/pom.xml | 24 ++ tests/test_maven.py | 332 +++++++++++++++++- tests/test_report.py | 52 +++ website/docs/ecosystems.md | 8 +- website/docs/how-it-works.md | 4 +- website/docs/output.md | 3 + 22 files changed, 870 insertions(+), 94 deletions(-) create mode 100644 tests/fixtures/maven/multimodule/pom.xml create mode 100644 tests/fixtures/maven/multimodule/service-a/pom.xml create mode 100644 tests/fixtures/maven/multimodule/service-b/pom.xml diff --git a/CHANGELOG.md b/CHANGELOG.md index 3faffea..85403ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,5 +63,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Exit codes: `0` completed (regardless of findings), `1` scan failed or partial, `2` usage error. Findings alone never fail a build. - Maven graphs are marked approximate — they are reconstructed, not read, and we never shell - out to `mvn`. + out to `mvn`. Each module of a multi-module build resolves against its own POM, so module + boundaries hold: one module's nearest-wins choice cannot decide another's versions, and each + transitive is attributed to the `` declaration that introduced it. +- Maven `` are honoured on declared dependencies as well as inherited ones, + including entries supplied by `dependencyManagement` and Maven's whole-segment wildcards. + Excluded coordinates are absent from the report, and the `exclusions` field on each + dependency records what was removed. - Licensed under Apache 2.0. diff --git a/CLAUDE.md b/CLAUDE.md index c3fae29..a0ab812 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -123,8 +123,22 @@ as version `(123456,)` and orders as a real version. **Maven is approximate and says so.** Parent inheritance, BOM imports, nearest-wins and exclusions — not profiles, mirrors, relocation or version ranges. Never shell out to `mvn`: running a project's build to discover its dependencies is itself a supply chain risk. -`MavenResolver._backfill` re-reads `pom.xml` to apply BOM-supplied versions to direct dependencies, -which the synchronous parser cannot do because BOMs live on Central. +`MavenResolver._backfill` re-reads `pom.xml` to apply BOM-supplied versions and exclusions to +direct dependencies, which the synchronous parser cannot do because BOMs live on Central. + +**Maven resolves one module at a time.** `_maven_units` in `scanner.py` regroups the flat +dependency list back onto its scan units by `source.path`, and `expand_units` walks each +separately on one shared resolver — shared cache, shared connection pool, shared node budget. +Merging a reactor into one traversal gives whichever module is walked first the deciding vote on +every shared coordinate, and leaves every transitive stamped with a POM that need not lead to it. + +**Exclusions are the one thing here that removes packages.** Everything else in this codebase +fails towards over-reporting; `is_excluded` fails the other way, so a bug in it hides a real +dependency instead of adding noise. Hence: wildcards are whole-segment only (`*:*`, `g:*`, +`*:a` — never a prefix glob), half-declared `` elements are dropped at parse time so +they cannot become accidental wildcards, and `Dependency.exclusions` is emitted in JSON so every +removal stays auditable. The negative tests in `tests/test_maven.py` that assert a wildcard does +*not* fire are load-bearing. **Ranges, SARIF and CycloneDX are hand-written** rather than pulled from packages — a dependency scanner with a large dependency tree of its own is a poor advertisement. Correctness is held by @@ -159,6 +173,14 @@ and versions. This has caught two real bugs (a missing `coverage[toml]` extra tr - No SBOM *ingest*. - npm/yarn v1 lockfiles record no scope, so dev transitives are reported as runtime unless a `package.json` sits alongside. -- Gradle sees only literal declarations — no version catalogues or computed versions. +- Gradle sees only literal declarations — no version catalogues or computed versions, and no + `exclude` handling, so a Gradle graph over-reports where a Maven one would not. +- A parent POM that exists only on disk is never read. `_effective` fetches parents from + Central and `_backfill` reads a module's own POM, so an unpublished aggregator parent supplies + neither versions nor exclusions to its modules; the dependencies affected stay `unresolved`. + This is the largest remaining gap for real reactors. +- `dependencyManagement` exclusions supplied by an imported BOM reach a project only when that + project also has a version to resolve — `_backfill` is gated on an unresolved version so a + fully-versioned project pays no network cost. - OSV's unversioned second pass for `MAL-` advisories on yanked packages is not implemented; malicious advisories affecting the installed version still surface normally. diff --git a/README.md b/README.md index 0220cec..5d12958 100644 --- a/README.md +++ b/README.md @@ -57,7 +57,8 @@ files that introduced it: the report describes what is actually installed, not what would resolve today. Only when no lockfile is present does it fall back to resolving version ranges against the registry, and those findings are labelled `resolved` rather than `pinned`. Java, which has no lockfile, gets -its graph reconstructed from Maven Central and is marked `~` for approximate. +its graph reconstructed from Maven Central and is marked `~` for approximate — one module at a +time, so a reactor's module boundaries survive into the report. **A clean result is never implied unless it was earned.** Most of the rest of the design follows from this: diff --git a/src/icebergsca/.agents/skills/icebergsca/SKILL.md b/src/icebergsca/.agents/skills/icebergsca/SKILL.md index 9775f9e..5674f93 100644 --- a/src/icebergsca/.agents/skills/icebergsca/SKILL.md +++ b/src/icebergsca/.agents/skills/icebergsca/SKILL.md @@ -214,6 +214,18 @@ anything was checked. * **Maven and Gradle graphs are approximate.** Java has no lockfile, so the graph is reconstructed from Maven Central. Affected manifests carry `"approximate": true`. Mention this when reporting Java results. +* **Multi-module Maven provenance is trustworthy — you do not need to re-derive it.** Each + module resolves against its own POM, so a transitive's `source` is the declaration that + actually introduced it, `source.line` points at that `` element, and `parents` + names the package it came through. Two modules may legitimately report different versions of + the same coordinate; that is Maven's per-module nearest-wins, not a bug. What `approximate` + still covers: profiles, mirrors, ``, version ranges, and a parent POM that lives + only on disk and was never published to Central — management supplied that way is missed, and + the dependencies affected show up as `pin: "unresolved"`. +* **`` are applied, so an excluded artifact is absent by design.** Declared and + `dependencyManagement`-supplied exclusions both count, including Maven's whole-segment + wildcards. If a package you expected is missing, check the `exclusions` list on the + dependency that would have pulled it in before concluding the scan missed it. * **Go reads `go.mod`, never `go.sum`.** `go.sum` lists versions merely *considered* during resolution, so scanning it reports vulnerabilities in code that was never built. * **`yarn.lock` v1 records no scope.** Without a sibling `package.json`, dev transitives are diff --git a/src/icebergsca/.agents/skills/icebergsca/references/json-report.md b/src/icebergsca/.agents/skills/icebergsca/references/json-report.md index 4738c2e..60c508a 100644 --- a/src/icebergsca/.agents/skills/icebergsca/references/json-report.md +++ b/src/icebergsca/.agents/skills/icebergsca/references/json-report.md @@ -86,7 +86,9 @@ One entry per ecosystem-and-directory scan unit. * `parsed` is what was **actually read**, which differs from `lockfiles` when a lockfile could not be parsed and the manifest was used instead. Report `parsed`, not `lockfiles`. * `from_lockfile: false` means versions are declared, not installed. -* `approximate: true` means the graph was reconstructed rather than read — currently Maven. +* `approximate: true` means the graph was reconstructed rather than read — currently Maven. It + does *not* mean provenance is guesswork: each module is resolved on its own, so a + transitive's `source` names the declaration that introduced it. * `dependency_count` values sum to `summary.dependencies`. ## `dependencies` @@ -100,7 +102,8 @@ One entry per ecosystem-and-directory scan unit. "pin": "pinned", "constraint": null, "source": { "path": "requirements.txt", "line": 4 }, - "parents": ["pkg:pypi/requests@2.19.1"] + "parents": ["pkg:pypi/requests@2.19.1"], + "exclusions": [] } ``` @@ -112,8 +115,13 @@ One entry per ecosystem-and-directory scan unit. * `constraint` is the raw declared text (`">=2.0,<3.0"`, `"^4.17.21"`) when the version was not pinned outright. * `source.line` is `null` for JSON and XML manifests, where a line number would be guesswork. -* `parents` is populated only for lockfile formats that record edges. Empty means "unknown", - never "nothing depends on it". +* `parents` is populated for lockfile formats that record edges and for reconstructed Maven + transitives. Empty means "unknown", never "nothing depends on it". +* `source` on a reconstructed Maven transitive is the declaration that introduced it — the + module's own POM and the `` line — not merely the first file scanned. +* `exclusions` lists the `group:artifact` keys this declaration removes from its own subtree + (Maven ``, wildcards included). A package named here is absent from the graph + deliberately, which is the one case where something is missing without being an error. ## `findings` diff --git a/src/icebergsca/core/models.py b/src/icebergsca/core/models.py index cf76a4d..e3ed6b0 100644 --- a/src/icebergsca/core/models.py +++ b/src/icebergsca/core/models.py @@ -271,6 +271,12 @@ class Dependency: #: Packages that pull this one in. Only populated for lockfiles that record #: edges; empty is "we don't know", never "nothing depends on it". parents: tuple[PackageRef, ...] = () + #: ``group:artifact`` keys this dependency must not drag in — Maven + #: ```` today, Gradle ``exclude`` and npm ``overrides`` later. + #: Empty means "nothing was excluded", and deliberately not ``None`` for "we + #: could not tell": a parser that cannot read exclusions leaves this empty and + #: the graph over-reports, which is the safe way round. + exclusions: frozenset[str] = frozenset() @property def is_runtime(self) -> bool: diff --git a/src/icebergsca/core/scanner.py b/src/icebergsca/core/scanner.py index 8cda2be..89d4f19 100644 --- a/src/icebergsca/core/scanner.py +++ b/src/icebergsca/core/scanner.py @@ -39,7 +39,7 @@ ) from icebergsca.ecosystems import get as get_ecosystem from icebergsca.ecosystems.base import FileParser -from icebergsca.ecosystems.maven import MavenResolver, MavenResult +from icebergsca.ecosystems.maven import MavenResolver, MavenResult, MavenUnit from icebergsca.osv.client import USER_AGENT, OSVClient, OSVResult from icebergsca.registry import RegistryClient from icebergsca.resolve import Resolver @@ -89,7 +89,7 @@ async def scan(root: Path, options: ScanOptions | None = None) -> ScanReport: discovery = discover(root, options.discovery) kept, results, skipped, warnings = _parse_all(discovery, options.scopes) - outcome = await _resolve_and_check(kept, options, discovery.root) + outcome = await _resolve_and_check(kept, discovery.units, options, discovery.root) return ScanReport( root=discovery.root, @@ -138,7 +138,10 @@ class _VulnOutcome: async def _resolve_and_check( - dependencies: tuple[Dependency, ...], options: ScanOptions, root: Path + dependencies: tuple[Dependency, ...], + units: tuple[ScanUnit, ...], + options: ScanOptions, + root: Path, ) -> _VulnOutcome: """Stages 3 and 4: resolve unpinned constraints, then look everything up in OSV. @@ -173,7 +176,9 @@ async def _resolve_and_check( cache, offline=options.offline, concurrency=options.concurrency, - ).expand(dependencies, root=root) + ).expand_units( + dependencies, _maven_units(dependencies, units), root=root + ) dependencies = tuple( dep for dep in maven.dependencies if dep.scope in options.scopes ) @@ -237,6 +242,41 @@ async def _resolve_and_check( ) +def _maven_units( + dependencies: tuple[Dependency, ...], units: tuple[ScanUnit, ...] +) -> tuple[MavenUnit, ...]: + """Regroup the flat dependency list back onto the scan units it came from. + + Discovery assigns every file to exactly one (ecosystem, directory) unit, and the + parsers stamp each dependency with the file it was declared in, so ``source.path`` + identifies a dependency's unit exactly. Regrouping here rather than threading units + through the parse stage keeps ``_parse_all`` unchanged and — because it happens + after ``_dedupe`` — guarantees the resolver sees the same list the report will. + + A Maven dependency whose path belongs to no unit still gets a unit of its own. + Silently dropping it would remove packages from the report, which is the one thing + this stage must never do. + """ + owner = { + path: PurePath(unit.directory) + for unit in units + if unit.ecosystem is EcosystemId.MAVEN + for path in unit.files + } + + grouped: dict[PurePath, list[Dependency]] = defaultdict(list) + for dep in dependencies: + if dep.ref.ecosystem is not EcosystemId.MAVEN: + continue + path = PurePath(dep.source.path) + grouped[owner.get(path, path.parent)].append(dep) + + return tuple( + MavenUnit(directory=directory, dependencies=tuple(deps)) + for directory, deps in grouped.items() + ) + + def _finalise_manifests( results: tuple[ManifestResult, ...], dependencies: tuple[Dependency, ...], diff --git a/src/icebergsca/ecosystems/maven/__init__.py b/src/icebergsca/ecosystems/maven/__init__.py index ff4952f..00f1d6f 100644 --- a/src/icebergsca/ecosystems/maven/__init__.py +++ b/src/icebergsca/ecosystems/maven/__init__.py @@ -7,7 +7,7 @@ from icebergsca.core.models import EcosystemId from icebergsca.ecosystems.base import EcosystemSpec, FileSpec, unimplemented from icebergsca.ecosystems.maven.parser import parse_manifest -from icebergsca.ecosystems.maven.resolver import MavenResolver, MavenResult +from icebergsca.ecosystems.maven.resolver import MavenResolver, MavenResult, MavenUnit SPEC = EcosystemSpec( id=EcosystemId.MAVEN, @@ -19,4 +19,4 @@ parse_lockfile=unimplemented("Maven lockfile"), ) -__all__ = ["SPEC", "MavenResolver", "MavenResult", "parse_manifest"] +__all__ = ["SPEC", "MavenResolver", "MavenResult", "MavenUnit", "parse_manifest"] diff --git a/src/icebergsca/ecosystems/maven/model.py b/src/icebergsca/ecosystems/maven/model.py index 89a0b94..e5e2c25 100644 --- a/src/icebergsca/ecosystems/maven/model.py +++ b/src/icebergsca/ecosystems/maven/model.py @@ -57,6 +57,27 @@ def key(self) -> str: return f"{self.group}:{self.artifact}" +def is_excluded(key: str, exclusions: frozenset[str]) -> bool: + """True when a ``group:artifact`` key matches any exclusion pattern. + + Maven 3 allows ``*`` in either position, but only as a *whole* segment: ``org.foo*`` + is not a prefix glob in Maven and must not become one here. That restraint matters + more than the feature does. An exclusion is the one thing in this tool that removes + a package from the report rather than adding one, so an over-eager pattern hides a + real dependency instead of merely making noise. + """ + if key in exclusions: + # The overwhelmingly common case, and the only one that costs nothing. + return True + + group, _, artifact = key.partition(":") + return any( + pattern in ("*:*", f"{group}:*", f"*:{artifact}") + for pattern in exclusions + if "*" in pattern + ) + + @dataclass(frozen=True, slots=True) class Pom: """One parsed POM file, with nothing inherited or interpolated yet.""" diff --git a/src/icebergsca/ecosystems/maven/parser.py b/src/icebergsca/ecosystems/maven/parser.py index 58ce944..16d6647 100644 --- a/src/icebergsca/ecosystems/maven/parser.py +++ b/src/icebergsca/ecosystems/maven/parser.py @@ -158,13 +158,21 @@ def _dependencies(container: Element | None) -> tuple[RawDependency, ...]: parsed: list[RawDependency] = [] for element in _children(container, "dependency"): exclusions_element = _child(element, "exclusions") - exclusions = ( - frozenset( - f"{_text(item, 'groupId')}:{_text(item, 'artifactId')}" - for item in _children(exclusions_element, "exclusion") - ) + # A half-declared exclusion is discarded rather than kept as ``g:`` or ``:a``. + # Exclusions remove packages from the report, and a half-key sits one wildcard + # rule away from matching everything — dropping it over-reports, which is the + # direction to fail in. + items = ( + _children(exclusions_element, "exclusion") if exclusions_element is not None - else frozenset() + else [] + ) + exclusions = frozenset( + f"{group}:{artifact}" + for group, artifact in ( + (_text(item, "groupId"), _text(item, "artifactId")) for item in items + ) + if group and artifact ) parsed.append( @@ -241,29 +249,59 @@ def _parse_pom_manifest(path: Path, content: str) -> list[Dependency]: for entry in pom.managed if entry.version } + # Maven merges a managed entry's exclusions with the dependency's own rather than + # letting either replace the other, which is how a parent centralises an exclusion + # for a dependency that declares none of its own. + managed_exclusions = { + entry.key: entry.exclusions for entry in pom.managed if entry.exclusions + } dependencies: list[Dependency] = [] for entry in pom.dependencies: group = interpolate(entry.group, properties) or entry.group artifact = interpolate(entry.artifact, properties) or entry.artifact - version = interpolate(entry.version, properties) or managed_versions.get( - f"{group}:{artifact}" - ) + key = f"{group}:{artifact}" + version = interpolate(entry.version, properties) or managed_versions.get(key) usable = version if version and not has_unresolved_property(version) else None dependencies.append( Dependency( - ref=PackageRef(EcosystemId.MAVEN, f"{group}:{artifact}", usable), + ref=PackageRef(EcosystemId.MAVEN, key, usable), scope=maven_scope(entry.scope or DEFAULT_SCOPE), direct=True, source=SourceLocation(path, _find_line(content, artifact)), pin=Pin.PINNED if usable else Pin.UNRESOLVED, constraint=version, + exclusions=_interpolated_exclusions( + entry.exclusions | managed_exclusions.get(key, frozenset()), + properties, + ), ) ) return dependencies +def _interpolated_exclusions( + exclusions: frozenset[str], properties: dict[str, str] +) -> frozenset[str]: + """Resolve ``${...}`` in exclusion keys, which are coordinates like any other. + + A key left with an unresolved property is kept verbatim: it will simply not match, + which leaves the package in the report rather than removing it on a guess. + """ + if not exclusions: + return frozenset() + + resolved: set[str] = set() + for key in exclusions: + group, _, artifact = key.partition(":") + resolved.add( + f"{interpolate(group, properties) or group}" + f":{interpolate(artifact, properties) or artifact}" + ) + return frozenset(resolved) + + def _parse_gradle(path: Path, content: str) -> list[Dependency]: """Read declared coordinates out of a Gradle build script. diff --git a/src/icebergsca/ecosystems/maven/resolver.py b/src/icebergsca/ecosystems/maven/resolver.py index d55f042..6eea7c1 100644 --- a/src/icebergsca/ecosystems/maven/resolver.py +++ b/src/icebergsca/ecosystems/maven/resolver.py @@ -13,12 +13,20 @@ * parent inheritance, including properties and ``dependencyManagement`` * ``import``-scoped BOMs, expanded recursively -* nearest-wins conflict resolution, breadth-first -* ``test``/``provided`` scopes not propagating, and ```` being honoured +* nearest-wins conflict resolution, breadth-first, **per module** +* ``test``/``provided`` scopes not propagating +* ```` on declared and inherited dependencies alike, including the + whole-segment wildcards (``*:*``, ``group:*``, ``*:artifact``) Maven 3 allows We do not implement profiles, mirrors, ````, version *ranges*, or -classifier-specific graphs. The result is therefore marked ``approximate`` in the -report, and the output says so rather than implying a fidelity it does not have. +classifier-specific graphs, and a parent POM that exists only on disk — never +published to Central — cannot be read, so the management it supplies is missed. The +result is therefore marked ``approximate`` in the report, and the output says so +rather than implying a fidelity it does not have. + +Each module is resolved on its own. Sharing one traversal across a reactor lets +whichever module was walked first decide the others' versions, and leaves every +transitive attributed to a POM that may not lead to it. """ from __future__ import annotations @@ -26,8 +34,9 @@ import asyncio import logging import re +from collections.abc import Sequence from dataclasses import dataclass, field, replace -from pathlib import Path +from pathlib import Path, PurePath import httpx @@ -46,6 +55,7 @@ Coordinate, Pom, RawDependency, + is_excluded, ) from icebergsca.ecosystems.maven.parser import ( has_unresolved_property, @@ -87,6 +97,8 @@ class EffectivePom: managed: dict[str, str] = field(default_factory=dict) #: ``group:artifact`` → scope, so a BOM can pin scope as well as version. managed_scopes: dict[str, str] = field(default_factory=dict) + #: ``group:artifact`` → exclusions, so a parent or BOM can centralise them. + managed_exclusions: dict[str, frozenset[str]] = field(default_factory=dict) dependencies: tuple[RawDependency, ...] = () @@ -98,6 +110,33 @@ class MavenResult: approximate: bool = False +@dataclass(frozen=True, slots=True) +class MavenUnit: + """One module's declared dependencies, resolved on its own. + + Maven resolves each module against its own effective POM; siblings share a parent, + not a graph. Keeping them apart is what stops one module's nearest-wins choice + deciding another's versions. + """ + + directory: PurePath + dependencies: tuple[Dependency, ...] + + +@dataclass(frozen=True, slots=True) +class _Branch: + """One node on the breadth-first frontier.""" + + coordinate: Coordinate + scope: Scope + #: Exclusion patterns accumulated from every edge on the path to this node. + exclusions: frozenset[str] + #: The ```` element that introduced this branch, carried down so that + #: a transitive names the declaration which actually pulled it in rather than + #: whichever file happened to be walked first. + source: SourceLocation + + class MavenResolver: """Expands declared Maven dependencies into a full transitive graph.""" @@ -117,14 +156,60 @@ def __init__( self._semaphore = asyncio.Semaphore(max(1, concurrency)) self._max_depth = max_depth self._max_nodes = max_nodes + #: Nodes walked across every unit this resolver has expanded. The cap is + #: deliberately scan-wide rather than per-module: it exists to bound an + #: unbounded fetch loop against Central, and a nine-module reactor must not + #: get nine times the budget just for being split up. + self._nodes = 0 self._warnings: list[str] = [] # -- public ------------------------------------------------------------ + async def expand_units( + self, + declared: tuple[Dependency, ...], + units: Sequence[MavenUnit], + root: Path | None = None, + ) -> MavenResult: + """Expand several modules, each against its own POM. + + ``declared`` is the whole scan's dependency list and ``units`` groups the Maven + part of it by module. Everything else flows through untouched and in place, so + the only difference a non-Maven project sees is that nothing happened. + + One resolver instance serves every unit, so the POM cache, the connection pool + and the node budget are shared. A fresh resolver per module would refetch + Central for every coordinate the modules have in common — on a reactor, most + of them. + """ + resolved: dict[Dependency, Dependency] = {} + transitive: list[Dependency] = [] + warnings: list[str] = [] + approximate = False + + for unit in units: + result = await self.expand(unit.dependencies, root) + # ``expand`` returns this unit's declarations first, in the order it was + # given them, followed by whatever the walk found beneath them. + count = len(unit.dependencies) + resolved.update( + zip(unit.dependencies, result.dependencies[:count], strict=True) + ) + transitive.extend(result.dependencies[count:]) + warnings.extend(result.warnings) + approximate = approximate or result.approximate + + return MavenResult( + dependencies=tuple(resolved.get(dep, dep) for dep in declared) + + tuple(transitive), + warnings=tuple(dict.fromkeys(warnings)), + approximate=approximate, + ) + async def expand( self, declared: tuple[Dependency, ...], root: Path | None = None ) -> MavenResult: - """Walk the graph beneath a set of declared Maven dependencies. + """Walk the graph beneath one module's declared Maven dependencies. Two things happen here. First any direct dependency that declared no version gets one from the project's own ``dependencyManagement`` — which usually means @@ -139,37 +224,54 @@ async def expand( if not maven_deps: return MavenResult(declared) + # Warnings accumulate on the instance so that one shared resolver can serve + # every unit; slicing from here keeps a module's result to its own problems + # instead of repeating the previous module's. + first_warning = len(self._warnings) + declared = await self._backfill(declared, root) roots = [dep for dep in declared if dep.ref.ecosystem is EcosystemId.MAVEN] - found = await self._walk(roots, roots[0].source) + found = await self._walk(roots) known = {dep.ref.name for dep in declared} transitive = tuple(dep for dep in found if dep.ref.name not in known) return MavenResult( dependencies=declared + transitive, - warnings=tuple(dict.fromkeys(self._warnings)), + warnings=tuple(dict.fromkeys(self._warnings[first_warning:])), approximate=True, ) async def _backfill( self, declared: tuple[Dependency, ...], root: Path | None ) -> tuple[Dependency, ...]: - """Fill in versions the project's own dependencyManagement supplies. + """Fill in what the project's own dependencyManagement supplies. The synchronous parser can only see literal versions in the file it was handed. A ``import`` BOM lives on Maven Central, so resolving it needs the network and has to happen here. + + Only this unit's own POMs are read. Pooling every POM in the tree would let a + BOM imported by one module supply versions to another, which no Maven build + does and which quietly attributes one module's choices to another. """ + if root is None: + return declared + unresolved = { dep.source.path for dep in declared if dep.ref.ecosystem is EcosystemId.MAVEN and dep.ref.version is None } - if not unresolved or root is None: + # Still gated on an unresolved version, so a fully-versioned project pays no + # network cost it did not pay before. Same-file management exclusions are + # already applied by the parser; what this adds is the BOM-supplied ones, + # which is the case that needs Central anyway. + if not unresolved: return declared managed: dict[str, str] = {} + managed_exclusions: dict[str, frozenset[str]] = {} for relative in sorted(unresolved): path = root / relative if path.name.lower() != "pom.xml": @@ -185,99 +287,131 @@ async def _backfill( properties = dict(pom.properties) properties.setdefault("project.version", pom.coordinate.version or "") properties.setdefault("project.groupId", pom.coordinate.group) - await self._apply_management(pom, properties, managed, {}) + await self._apply_management( + pom, properties, managed, {}, managed_exclusions + ) - if not managed: + if not managed and not managed_exclusions: return declared return tuple( - replace( - dep, - ref=PackageRef(EcosystemId.MAVEN, dep.ref.name, managed[dep.ref.name]), - pin=Pin.PINNED, - ) - if dep.ref.ecosystem is EcosystemId.MAVEN - and dep.ref.version is None - and dep.ref.name in managed - else dep - for dep in declared + self._backfilled(dep, managed, managed_exclusions) for dep in declared + ) + + @staticmethod + def _backfilled( + dep: Dependency, + managed: dict[str, str], + managed_exclusions: dict[str, frozenset[str]], + ) -> Dependency: + """Apply management-supplied versions and exclusions to one declaration.""" + if dep.ref.ecosystem is not EcosystemId.MAVEN: + return dep + + inherited = managed_exclusions.get(dep.ref.name, frozenset()) + exclusions = dep.exclusions | inherited + version = managed.get(dep.ref.name) if dep.ref.version is None else None + + if version is None and exclusions == dep.exclusions: + return dep + if version is None: + return replace(dep, exclusions=exclusions) + + return replace( + dep, + ref=PackageRef(EcosystemId.MAVEN, dep.ref.name, version), + pin=Pin.PINNED, + exclusions=exclusions, ) # -- traversal --------------------------------------------------------- - async def _walk( - self, roots: list[Dependency], source: SourceLocation - ) -> list[Dependency]: - """Breadth-first, nearest-wins. + async def _walk(self, roots: list[Dependency]) -> list[Dependency]: + """Breadth-first, nearest-wins, over one module. Maven resolves a version conflict by taking the declaration closest to the root, so a breadth-first walk that ignores any coordinate already seen reproduces that rule exactly — the first time a ``group:artifact`` is reached is by definition its shortest path. + + ``seen`` is local to one call, and one call covers one module. Sharing it + across modules would let whichever module was walked first decide the others' + versions, which is not what Maven does and not what their builds produce. """ seen: dict[str, str | None] = {} results: list[Dependency] = [] - frontier: list[tuple[Coordinate, Scope, frozenset[str]]] = [] + frontier: list[_Branch] = [] for dep in roots: group, _, artifact = dep.ref.name.partition(":") seen[dep.ref.name] = dep.ref.version if dep.ref.version and dep.scope not in (Scope.TEST,): frontier.append( - ( - Coordinate(group, artifact, dep.ref.version), - dep.scope, - frozenset(), + _Branch( + coordinate=Coordinate(group, artifact, dep.ref.version), + scope=dep.scope, + # A declaration's own apply to everything beneath + # it. They are checked against children only, so a dependency + # is never removed by its own exclusion list. + exclusions=dep.exclusions, + source=dep.source, ) ) + self._nodes += len(seen) depth = 0 - while frontier and depth < self._max_depth and len(seen) < self._max_nodes: + while frontier and depth < self._max_depth and self._nodes < self._max_nodes: depth += 1 poms = await asyncio.gather( - *(self._effective(coordinate) for coordinate, _, _ in frontier), + *(self._effective(branch.coordinate) for branch in frontier), return_exceptions=True, ) - next_frontier: list[tuple[Coordinate, Scope, frozenset[str]]] = [] - for (parent_coord, parent_scope, inherited), pom in zip( - frontier, poms, strict=True - ): + next_frontier: list[_Branch] = [] + for branch, pom in zip(frontier, poms, strict=True): if isinstance(pom, BaseException) or pom is None: - logger.debug("could not resolve %s: %s", parent_coord, pom) + logger.debug("could not resolve %s: %s", branch.coordinate, pom) continue - for child, version, scope in self._children(pom, parent_scope): - if child.key in inherited or child.key in seen: + parent = PackageRef( + EcosystemId.MAVEN, branch.coordinate.key, branch.coordinate.version + ) + for child, version, scope in self._children(pom, branch.scope): + if is_excluded(child.key, branch.exclusions) or child.key in seen: continue - if len(seen) >= self._max_nodes: + if self._nodes >= self._max_nodes: break seen[child.key] = version + self._nodes += 1 results.append( Dependency( ref=PackageRef(EcosystemId.MAVEN, child.key, version), scope=scope, direct=False, - source=source, + source=branch.source, pin=Pin.PINNED if version else Pin.UNRESOLVED, + parents=(parent,), ) ) if version: next_frontier.append( - ( - Coordinate(child.group, child.artifact, version), - scope, - inherited | child.exclusions, + _Branch( + coordinate=Coordinate( + child.group, child.artifact, version + ), + scope=scope, + exclusions=branch.exclusions | child.exclusions, + source=branch.source, ) ) frontier = next_frontier - if len(seen) >= self._max_nodes: + if self._nodes >= self._max_nodes: self._warnings.append( - f"Maven graph truncated at {self._max_nodes} packages; " - "the dependency list is incomplete" + f"Maven graph truncated at {self._max_nodes} packages across the " + "scan; the dependency list is incomplete" ) return results @@ -327,7 +461,11 @@ def _children( artifact=artifact, version=version, scope=entry.scope, - exclusions=entry.exclusions, + # Maven merges the two lists rather than letting either replace the + # other, which is how a parent centralises an exclusion for a + # dependency that declares none of its own. + exclusions=entry.exclusions + | pom.managed_exclusions.get(key, frozenset()), ) children.append((resolved, version, scope)) @@ -344,6 +482,7 @@ async def _effective(self, coordinate: Coordinate) -> EffectivePom | None: properties: dict[str, str] = {} managed: dict[str, str] = {} managed_scopes: dict[str, str] = {} + managed_exclusions: dict[str, frozenset[str]] = {} chain: list[Pom] = [pom] current = pom @@ -366,13 +505,16 @@ async def _effective(self, coordinate: Coordinate) -> EffectivePom | None: properties.setdefault("project.artifactId", coordinate.artifact) for entry in reversed(chain): - await self._apply_management(entry, properties, managed, managed_scopes) + await self._apply_management( + entry, properties, managed, managed_scopes, managed_exclusions + ) return EffectivePom( coordinate=coordinate, properties=properties, managed=managed, managed_scopes=managed_scopes, + managed_exclusions=managed_exclusions, dependencies=pom.dependencies, ) @@ -382,6 +524,7 @@ async def _apply_management( properties: dict[str, str], managed: dict[str, str], managed_scopes: dict[str, str], + managed_exclusions: dict[str, frozenset[str]], ) -> None: """Fold one POM's dependencyManagement in, expanding imported BOMs first. @@ -401,12 +544,17 @@ async def _apply_management( ) for key, bom_version in bom.managed.items(): managed.setdefault(key, bom_version) + for key, bom_exclusions in bom.managed_exclusions.items(): + managed_exclusions.setdefault(key, bom_exclusions) continue + key = f"{group}:{artifact}" if version and not has_unresolved_property(version): - managed[f"{group}:{artifact}"] = version + managed[key] = version if entry.scope: - managed_scopes[f"{group}:{artifact}"] = entry.scope + managed_scopes[key] = entry.scope + if entry.exclusions: + managed_exclusions[key] = entry.exclusions async def _fetch_pom(self, coordinate: Coordinate) -> Pom | None: """Fetch and parse one POM, cached for a week. diff --git a/src/icebergsca/report/json_.py b/src/icebergsca/report/json_.py index 9daa2d0..772ebbb 100644 --- a/src/icebergsca/report/json_.py +++ b/src/icebergsca/report/json_.py @@ -97,6 +97,10 @@ def _dependency(dep: Dependency) -> dict[str, Any]: "constraint": dep.constraint, "source": {"path": str(dep.source.path), "line": dep.source.line}, "parents": [ref.purl for ref in dep.parents], + # Emitted so that a removal stays auditable. An exclusion is the one thing + # here that takes a package *out* of the graph, and a reader has no other way + # to tell a package that was excluded from one that was never there. + "exclusions": sorted(dep.exclusions), } diff --git a/src/icebergsca/report/table.py b/src/icebergsca/report/table.py index cabb3c7..e2ea1da 100644 --- a/src/icebergsca/report/table.py +++ b/src/icebergsca/report/table.py @@ -94,6 +94,13 @@ def _render_manifests(console: Console, report: ScanReport) -> None: ) console.print(table) + if any(result.approximate for result in report.manifests): + # The marker was previously printed with nothing anywhere explaining it, which + # left readers to guess whether it meant "incomplete" or "untrusted". + console.print( + "[dim][yellow]~[/yellow] graph reconstructed from the registry rather " + "than read from a lockfile; each module resolved on its own[/dim]" + ) console.print() diff --git a/tests/conftest.py b/tests/conftest.py index 2fcf457..62053d3 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -9,6 +9,7 @@ from __future__ import annotations import json +from collections.abc import Callable from datetime import UTC, datetime from pathlib import Path from typing import Any @@ -16,6 +17,7 @@ import httpx import pytest +from icebergsca.core import scanner from icebergsca.core.models import ( Advisory, Dependency, @@ -67,16 +69,51 @@ def null_client() -> httpx.AsyncClient: class _EmptyOSVTransport(httpx.AsyncBaseTransport): - """Answers any OSV querybatch with "no vulnerabilities" and nothing else.""" + """Answer OSV querybatch with "no vulnerabilities"; serve anything else from a + canned ``"METHOD:url"`` map and 404 the rest. + + The canned map is what lets a scanner-level test reach a registry — Maven Central, + say — without weakening the 404-everything-unmocked guarantee that makes a stray + request fail loudly instead of silently degrading the result. + """ + + def __init__(self, responses: dict[str, Any] | None = None) -> None: + self.responses = responses or {} async def handle_async_request(self, request: httpx.Request) -> httpx.Response: if request.url.path.endswith("/querybatch"): body = json.loads(request.content or b'{"queries": []}') results = [{"vulns": []} for _ in body.get("queries", [])] return httpx.Response(200, json={"results": results}, request=request) + + payload = self.responses.get(f"{request.method}:{request.url}") + if isinstance(payload, httpx.Response): + return payload + if payload is not None: + return httpx.Response(200, content=json.dumps(payload), request=request) return httpx.Response(404, json={"error": "not mocked"}, request=request) +@pytest.fixture +def canned_upstream( + monkeypatch: pytest.MonkeyPatch, +) -> Callable[[dict[str, Any]], None]: + """Re-point ``build_http_client`` at a transport that also serves a registry. + + Autouse fixtures are set up before explicitly requested ones, so this reliably + replaces what ``_no_network`` installed rather than racing it. + """ + + def install(responses: dict[str, Any]) -> None: + monkeypatch.setattr( + scanner, + "build_http_client", + lambda options: httpx.AsyncClient(transport=_EmptyOSVTransport(responses)), + ) + + return install + + @pytest.fixture(autouse=True) def _no_network( monkeypatch: pytest.MonkeyPatch, tmp_path_factory: pytest.TempPathFactory @@ -114,6 +151,7 @@ def make_dependency( path: str = "requirements.txt", line: int | None = 1, constraint: str | None = None, + exclusions: frozenset[str] = frozenset(), ) -> Dependency: return Dependency( ref=PackageRef(ecosystem, name, version), @@ -122,6 +160,7 @@ def make_dependency( source=SourceLocation(Path(path), line), pin=pin, constraint=constraint, + exclusions=exclusions, ) diff --git a/tests/fixtures/maven/multimodule/pom.xml b/tests/fixtures/maven/multimodule/pom.xml new file mode 100644 index 0000000..ea31fe0 --- /dev/null +++ b/tests/fixtures/maven/multimodule/pom.xml @@ -0,0 +1,13 @@ + + + 4.0.0 + com.example + reactor + 1.0.0 + pom + + + service-a + service-b + + diff --git a/tests/fixtures/maven/multimodule/service-a/pom.xml b/tests/fixtures/maven/multimodule/service-a/pom.xml new file mode 100644 index 0000000..a646680 --- /dev/null +++ b/tests/fixtures/maven/multimodule/service-a/pom.xml @@ -0,0 +1,18 @@ + + + 4.0.0 + + com.example + reactor + 1.0.0 + + service-a + + + + com.example + alpha + 1.0 + + + diff --git a/tests/fixtures/maven/multimodule/service-b/pom.xml b/tests/fixtures/maven/multimodule/service-b/pom.xml new file mode 100644 index 0000000..e99f6eb --- /dev/null +++ b/tests/fixtures/maven/multimodule/service-b/pom.xml @@ -0,0 +1,24 @@ + + + 4.0.0 + + com.example + reactor + 1.0.0 + + service-b + + + + com.example + beta + 1.0 + + + com.example + unwanted + + + + + diff --git a/tests/test_maven.py b/tests/test_maven.py index 93fd3e6..895983f 100644 --- a/tests/test_maven.py +++ b/tests/test_maven.py @@ -2,7 +2,7 @@ from __future__ import annotations -from pathlib import Path +from pathlib import Path, PurePath import httpx import pytest @@ -17,7 +17,8 @@ Scope, SourceLocation, ) -from icebergsca.ecosystems.maven import MavenResolver, parse_manifest +from icebergsca.core.scanner import ScanOptions, scan +from icebergsca.ecosystems.maven import MavenResolver, MavenUnit, parse_manifest from icebergsca.ecosystems.maven.model import Coordinate from icebergsca.ecosystems.maven.parser import interpolate, parse_pom from icebergsca.ecosystems.maven.resolver import CENTRAL @@ -193,22 +194,42 @@ def pom_xml( return httpx.Response(200, text=body) -def dep_xml(group: str, artifact: str, version: str = "", scope: str = "") -> str: +def dep_xml( + group: str, + artifact: str, + version: str = "", + scope: str = "", + *, + exclusions: tuple[tuple[str, str], ...] = (), +) -> str: version_tag = f"{version}" if version else "" scope_tag = f"{scope}" if scope else "" + excluded = "".join( + f"{eg}{ea}" + for eg, ea in exclusions + ) + exclusions_tag = f"{excluded}" if exclusions else "" return ( - f"{group}" - f"{artifact}{version_tag}{scope_tag}" + f"{group}{artifact}" + f"{version_tag}{scope_tag}{exclusions_tag}" ) -def direct(name: str, version: str | None, scope: Scope = Scope.RUNTIME) -> Dependency: +def direct( + name: str, + version: str | None, + scope: Scope = Scope.RUNTIME, + *, + exclusions: frozenset[str] = frozenset(), + path: str = "pom.xml", +) -> Dependency: return Dependency( ref=PackageRef(EcosystemId.MAVEN, name, version), scope=scope, direct=True, - source=SourceLocation(Path("pom.xml")), + source=SourceLocation(Path(path), 1), pin=Pin.PINNED if version else Pin.UNRESOLVED, + exclusions=exclusions, ) @@ -276,12 +297,7 @@ async def test_exclusions_are_honoured_down_the_branch() -> None: "g", "a", "1.0", - dependencies=( - "gb" - "2.0" - "gunwanted" - "" - ), + dependencies=dep_xml("g", "b", "2.0", exclusions=(("g", "unwanted"),)), ), f"GET:{pom_url('g', 'b', '2.0')}": pom_xml( "g", "b", "2.0", dependencies=dep_xml("g", "unwanted", "1.0") @@ -447,3 +463,293 @@ async def test_a_bom_may_supply_the_scope_as_well_as_the_version() -> None: ) unmanaged = await resolver(responses).expand((direct("g:a", "1.0"),)) assert "g:b" in {dep.ref.name for dep in unmanaged.dependencies} + + +# --------------------------------------------------------------------------- +# Exclusions declared on a direct dependency +# --------------------------------------------------------------------------- + + +def _one_level(child: str = "unwanted") -> dict[str, object]: + """``g:a 1.0`` pulling in ``g: 1.0``, so an exclusion has something + to bite.""" + return { + f"GET:{pom_url('g', 'a', '1.0')}": pom_xml( + "g", "a", "1.0", dependencies=dep_xml("g", child, "1.0") + ), + f"GET:{pom_url('g', child, '1.0')}": pom_xml("g", child, "1.0"), + } + + +def test_a_pom_manifest_carries_exclusions_onto_the_dependency() -> None: + """The parser reads ; this asserts they survive into the model. + + They used to be dropped one line after being parsed, which is what let an + explicitly excluded artifact be reported as a live, vulnerable dependency. + """ + dependencies = parse_manifest(Path("pom.xml"), read("basic", "pom.xml")) + guava = next(d for d in dependencies if d.ref.name.endswith(":guava")) + assert guava.exclusions == frozenset({"com.google.code.findbugs:jsr305"}) + + +async def test_exclusions_on_a_direct_dependency_are_honoured() -> None: + result = await resolver(_one_level()).expand( + (direct("g:a", "1.0", exclusions=frozenset({"g:unwanted"})),) + ) + assert all(d.ref.name != "g:unwanted" for d in result.dependencies) + + +async def test_a_direct_dependency_is_not_removed_by_its_own_exclusions() -> None: + """Exclusions apply to a declaration's subtree, never to the declaration itself.""" + result = await resolver(_one_level()).expand( + (direct("g:a", "1.0", exclusions=frozenset({"g:a"})),) + ) + assert "g:a" in {d.ref.name for d in result.dependencies} + + +async def test_a_wildcard_group_exclusion_removes_the_whole_group() -> None: + result = await resolver(_one_level()).expand( + (direct("g:a", "1.0", exclusions=frozenset({"g:*"})),) + ) + assert all(d.ref.name != "g:unwanted" for d in result.dependencies) + + +async def test_a_wildcard_artifact_exclusion_removes_it_from_any_group() -> None: + result = await resolver(_one_level()).expand( + (direct("g:a", "1.0", exclusions=frozenset({"*:unwanted"})),) + ) + assert all(d.ref.name != "g:unwanted" for d in result.dependencies) + + +async def test_a_full_wildcard_exclusion_removes_the_subtree() -> None: + result = await resolver(_one_level()).expand( + (direct("g:a", "1.0", exclusions=frozenset({"*:*"})),) + ) + assert [d.ref.name for d in result.dependencies] == ["g:a"] + + +async def test_a_wildcard_does_not_reach_a_different_group() -> None: + """The failure direction that matters: over-matching hides a real dependency.""" + result = await resolver(_one_level()).expand( + (direct("g:a", "1.0", exclusions=frozenset({"other:*"})),) + ) + assert "g:unwanted" in {d.ref.name for d in result.dependencies} + + +async def test_a_wildcard_is_not_a_prefix_glob() -> None: + """Maven's ``*`` is a whole segment: ``g:un*`` matches nothing at all.""" + result = await resolver(_one_level()).expand( + (direct("g:a", "1.0", exclusions=frozenset({"g:un*"})),) + ) + assert "g:unwanted" in {d.ref.name for d in result.dependencies} + + +def test_a_half_declared_exclusion_is_dropped_rather_than_kept() -> None: + """```` with no artifactId must not become the key ``g:``.""" + pom = parse_pom( + """ga1 + gb + g + """ + ) + assert pom.dependencies[0].exclusions == frozenset() + + +async def test_dependency_management_supplies_exclusions_to_a_transitive() -> None: + """A parent centralising an exclusion for a dependency that declares none.""" + responses = { + f"GET:{pom_url('g', 'a', '1.0')}": pom_xml( + "g", + "a", + "1.0", + management=dep_xml("g", "b", "2.0", exclusions=(("g", "unwanted"),)), + dependencies=dep_xml("g", "b"), + ), + f"GET:{pom_url('g', 'b', '2.0')}": pom_xml( + "g", "b", "2.0", dependencies=dep_xml("g", "unwanted", "1.0") + ), + } + result = await resolver(responses).expand((direct("g:a", "1.0"),)) + names = {d.ref.name for d in result.dependencies} + assert "g:b" in names + assert "g:unwanted" not in names + + +def test_declared_and_managed_exclusions_are_merged() -> None: + """Maven unions the two lists; neither replaces the other.""" + dependencies = parse_manifest( + Path("pom.xml"), + """ga1 + + gb2.0 + gfrom-parent + + + + gb + gfrom-child + + """, + ) + assert dependencies[0].exclusions == frozenset({"g:from-parent", "g:from-child"}) + + +# --------------------------------------------------------------------------- +# Per-module resolution +# --------------------------------------------------------------------------- + + +def _two_modules() -> dict[str, object]: + """Two modules that both reach ``g:shared``, by different routes and versions.""" + return { + f"GET:{pom_url('g', 'a', '1.0')}": pom_xml( + "g", "a", "1.0", dependencies=dep_xml("g", "shared", "1.0") + ), + f"GET:{pom_url('g', 'b', '1.0')}": pom_xml( + "g", "b", "1.0", dependencies=dep_xml("g", "shared", "2.0") + ), + f"GET:{pom_url('g', 'shared', '1.0')}": pom_xml("g", "shared", "1.0"), + f"GET:{pom_url('g', 'shared', '2.0')}": pom_xml("g", "shared", "2.0"), + } + + +async def test_each_module_resolves_its_own_version_of_a_shared_coordinate() -> None: + """One ``seen`` map across modules let whichever was walked first decide.""" + units = ( + MavenUnit(PurePath("a"), (direct("g:a", "1.0", path="a/pom.xml"),)), + MavenUnit(PurePath("b"), (direct("g:b", "1.0", path="b/pom.xml"),)), + ) + declared = tuple(dep for unit in units for dep in unit.dependencies) + result = await resolver(_two_modules()).expand_units(declared, units) + + shared = { + (str(d.source.path), d.ref.version) + for d in result.dependencies + if d.ref.name == "g:shared" + } + assert shared == {("a/pom.xml", "1.0"), ("b/pom.xml", "2.0")} + + +async def test_a_transitive_names_the_declaration_that_introduced_it() -> None: + """Provenance used to be whichever root sorted first, for every transitive.""" + units = ( + MavenUnit(PurePath("a"), (direct("g:a", "1.0", path="a/pom.xml"),)), + MavenUnit(PurePath("b"), (direct("g:b", "1.0", path="b/pom.xml"),)), + ) + declared = tuple(dep for unit in units for dep in unit.dependencies) + result = await resolver(_two_modules()).expand_units(declared, units) + + introduced = { + (str(d.source.path), d.parents[0].name, d.source.line) + for d in result.dependencies + if d.ref.name == "g:shared" and not d.direct + } + assert introduced == {("a/pom.xml", "g:a", 1), ("b/pom.xml", "g:b", 1)} + + +async def test_a_directly_declared_package_is_not_dropped_from_another_module() -> None: + """Filtering transitives by bare name across the whole scan lost module B's copy.""" + units = ( + MavenUnit(PurePath("a"), (direct("g:shared", "1.0", path="a/pom.xml"),)), + MavenUnit(PurePath("b"), (direct("g:b", "1.0", path="b/pom.xml"),)), + ) + declared = tuple(dep for unit in units for dep in unit.dependencies) + result = await resolver(_two_modules()).expand_units(declared, units) + + assert any( + d.ref.name == "g:shared" and str(d.source.path) == "b/pom.xml" + for d in result.dependencies + ) + + +async def test_expand_units_leaves_non_maven_dependencies_in_place() -> None: + other = Dependency( + ref=PackageRef(EcosystemId.PYPI, "flask", "3.0.0"), + scope=Scope.RUNTIME, + direct=True, + source=SourceLocation(Path("requirements.txt")), + pin=Pin.PINNED, + ) + unit = MavenUnit(PurePath("."), (direct("g:a", "1.0"),)) + result = await resolver(_one_level()).expand_units( + (other, *unit.dependencies), (unit,) + ) + assert other in result.dependencies + + +async def test_the_node_budget_is_shared_across_modules() -> None: + """The cap bounds a runaway fetch loop, so splitting must not multiply it.""" + units = ( + MavenUnit(PurePath("a"), (direct("g:a", "1.0", path="a/pom.xml"),)), + MavenUnit(PurePath("b"), (direct("g:b", "1.0", path="b/pom.xml"),)), + ) + declared = tuple(dep for unit in units for dep in unit.dependencies) + result = await resolver(_two_modules(), max_nodes=2).expand_units(declared, units) + + assert any("truncated" in warning for warning in result.warnings) + assert sum(1 for _ in result.warnings if "truncated" in _) == 1 + + +# --------------------------------------------------------------------------- +# Multi-module scans, end to end +# --------------------------------------------------------------------------- + + +async def test_a_multi_module_scan_attributes_transitives_to_their_own_module( + canned_upstream: object, +) -> None: + """The whole of both fixes, observed from the outside. + + ``service-a`` and ``service-b`` both reach ``com.example:shared``, by different + routes and at different versions, and ``service-b`` excludes ``unwanted``. A + merged resolve pass gave every transitive one module's POM as its source and let + whichever module was walked first pick the shared version for both. + """ + responses = { + f"GET:{pom_url('com.example', 'alpha', '1.0')}": pom_xml( + "com.example", + "alpha", + "1.0", + dependencies=dep_xml("com.example", "shared", "1.0"), + ), + f"GET:{pom_url('com.example', 'beta', '1.0')}": pom_xml( + "com.example", + "beta", + "1.0", + dependencies=dep_xml("com.example", "shared", "2.0") + + dep_xml("com.example", "unwanted", "1.0"), + ), + f"GET:{pom_url('com.example', 'shared', '1.0')}": pom_xml( + "com.example", "shared", "1.0" + ), + f"GET:{pom_url('com.example', 'shared', '2.0')}": pom_xml( + "com.example", "shared", "2.0" + ), + f"GET:{pom_url('com.example', 'unwanted', '1.0')}": pom_xml( + "com.example", "unwanted", "1.0" + ), + } + canned_upstream(responses) # type: ignore[operator] + + report = await scan( + FIXTURES / "maven" / "multimodule", + ScanOptions(check_vulnerabilities=False, resolve_ranges=False), + ) + + shared = { + (str(dep.source.path), dep.ref.version) + for dep in report.dependencies + if dep.ref.name == "com.example:shared" + } + assert shared == { + ("service-a/pom.xml", "1.0"), + ("service-b/pom.xml", "2.0"), + } + + # Declared on service-b's , so it must not reach the graph at all. + assert all(d.ref.name != "com.example:unwanted" for d in report.dependencies) + + # Every module's count is its own, which only holds once transitives stop being + # attributed to whichever POM happened to sort first. + counts = {str(m.directory): m.dependency_count for m in report.manifests} + assert counts == {".": 0, "service-a": 2, "service-b": 2} diff --git a/tests/test_report.py b/tests/test_report.py index efa3eed..f859f0e 100644 --- a/tests/test_report.py +++ b/tests/test_report.py @@ -300,3 +300,55 @@ def test_every_declared_format_renders(fmt: OutputFormat) -> None: def test_an_unregistered_format_fails_clearly() -> None: with pytest.raises(IcebergSCAError, match="not implemented yet"): render(make_report(), "yaml") # type: ignore[arg-type] + + +def test_the_approximate_marker_is_explained_rather_than_left_bare() -> None: + """A lone ``~`` reads as either "incomplete" or "untrusted"; say which.""" + output = table_of( + { + "manifests": ( + ManifestResult( + ecosystem=EcosystemId.MAVEN, + directory=Path("."), + manifests=(Path("pom.xml"),), + parsed=(Path("pom.xml"),), + approximate=True, + ), + ) + } + ) + assert "reconstructed" in output + + +def test_no_legend_when_nothing_was_reconstructed() -> None: + output = table_of( + { + "manifests": ( + ManifestResult( + ecosystem=EcosystemId.PYPI, + directory=Path("."), + lockfiles=(Path("uv.lock"),), + parsed=(Path("uv.lock"),), + from_lockfile=True, + ), + ) + } + ) + assert "reconstructed" not in output + + +def test_json_records_what_a_dependency_excludes() -> None: + """An exclusion removes a package, so the report has to say it did.""" + report = make_report( + dependencies=( + make_dependency( + "displaytag:displaytag", + "1.2", + ecosystem=EcosystemId.MAVEN, + path="pom.xml", + exclusions=frozenset({"com.lowagie:itext"}), + ), + ) + ) + payload = json.loads(render(report, OutputFormat.JSON)) + assert payload["dependencies"][0]["exclusions"] == ["com.lowagie:itext"] diff --git a/website/docs/ecosystems.md b/website/docs/ecosystems.md index ea0ec80..91351b3 100644 --- a/website/docs/ecosystems.md +++ b/website/docs/ecosystems.md @@ -45,9 +45,15 @@ the file records the selected set, not the shape that produced it. Java has no lockfile. The graph is reconstructed by fetching POMs from Maven Central and applying parent inheritance, BOM imports, nearest-wins and -exclusions. Not modelled: profiles, mirrors, relocation and version ranges. +exclusions. Not modelled: profiles, mirrors, relocation, version ranges, and +parent POMs that exist only on disk rather than on Central. Affected manifests carry `approximate: true`, and the table marks them `~`. +Each module of a multi-module build is resolved against its own POM, the way +Maven does it. Two modules may therefore report different versions of the same +coordinate, and each transitive names the `` declaration that +introduced it rather than whichever POM was read first. + IcebergSCA never shells out to `mvn`. Running a project's own build in order to discover its dependencies is itself a supply chain risk. diff --git a/website/docs/how-it-works.md b/website/docs/how-it-works.md index 462e417..5ed2aa9 100644 --- a/website/docs/how-it-works.md +++ b/website/docs/how-it-works.md @@ -95,7 +95,9 @@ hypothetical fresh install. Java is the systematic exception: it has no lockfile, so the graph is reconstructed from Maven Central and every affected manifest is marked -`approximate`. +`approximate`. Each module is resolved separately, so module boundaries hold: +a transitive is attributed to the declaration that introduced it, and one +module's version choice does not leak into another's. ## Scope diff --git a/website/docs/output.md b/website/docs/output.md index 89da8fc..733ab41 100644 --- a/website/docs/output.md +++ b/website/docs/output.md @@ -55,6 +55,9 @@ Three things are easy to get wrong: - **`manifests[].parsed` is what was actually read**, which differs from `lockfiles` when a lockfile could not be parsed and its manifest was used instead. +- **`dependencies[].exclusions` explains an absence.** A coordinate listed there + was removed from the graph on purpose, which is the one case where something + missing is not something overlooked. The last three arrays — `unchecked_packages`, `skipped` and `warnings` — are the report's account of what it could not do. They are the difference between