maven: honour direct-dependency exclusions, resolve per module - #18
Merged
Conversation
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 <exclusions> 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 <exclusion> 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01By6nqX2dcU351oZn6y6Xoq
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
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.
Fixes two defects in the Maven resolver, both surfaced by agent reviews of a Java codebase. They are in one PR because both restructure
expand()and_walk.Closes #17. Closes #16.
Exclusions on a direct dependency were silently dropped
parser.pyread<exclusions>intoRawDependency.exclusionscorrectly, but the coreDependencyhad no field to carry them, so the set was discarded one line later and_walkseeded the frontier with an empty set because nothing was left to seed with. Exclusions declared inside fetched POMs still worked, which is why it went unnoticed.The result was a false positive of the most expensive kind:
com.lowagie:itext 1.3reported with a high, unfixable XXE advisory in a project whosepom.xmlexplicitly excludes it — and credible, because the application really does parse PDFs withcom.lowagieclasses, via openpdf.Exclusions now travel on
Dependency, seed the frontier, match Maven's whole-segment wildcards (*:*,group:*,*:artifact— never a prefix glob), and are picked up fromdependencyManagementat both project and transitive level. Half-declared<exclusion>elements are dropped at parse time so they cannot become accidental wildcards.This is the one code path here that removes packages rather than adding them, inverting the project's usual over-report-when-uncertain posture, so the negative tests asserting a wildcard does not fire are as load-bearing as the positive ones, and each dependency's
exclusionsare emitted in JSON to keep every removal auditable.Multi-module projects resolved as one merged graph
A nine-module reactor was resolved as a single flat graph: one
seenmap let whichever module was walked first decide every shared coordinate's version,knowndropped a module's transitive if any other module declared it directly,_backfillpooled onemanageddict across every POM, and every transitive was stamped withroots[0].source— a file that need not lead to it.Each scan unit now resolves against its own POM on one shared resolver, so the POM cache, connection pool and node budget stay shared.
_maven_unitsregroups the flat dependency list back onto its units bysource.path. A transitive carries theSourceLocation— path and line — and the parent of the declaration that introduced it.Verification
Against real Maven Central, not only mocks, since exclusions remove packages:
displaytag 1.2with the exclusion → 8 packages, no itext; without → 9 including itext. Exactly one package different, so nothing is over-removed.mod-a/pom.xmland mod-b's 1 tomod-b/pom.xml, with per-manifest counts of 9 and 2.Each new regression test was confirmed to fail without its fix, including the scanner-level one, which collapses to a single
sharedattributed to the wrong module under the old merged behaviour.ruff check,ruff format --check,mypy(strict) and 406 tests all pass.Also included
canned_upstreamfixture so a test can reach a registry without weakening conftest's 404-everything-unmocked guarantee.~marker.Deliberately out of scope
A parent POM that exists only on disk is still never read, so an unpublished aggregator parent supplies neither versions nor exclusions to its modules. This is pre-existing, not introduced here —
_backfillnever followed local<parent>; the old pooling only leaked management between siblings, which is the bug being fixed. Recorded in CLAUDE.md's known gaps as the largest remaining gap for real reactors.Generated by Claude Code