Repository navigation
Stop vendored Maven liveness attesting unpatched builds - #2006
Mikola Lysenko (mikolalysenko) wants to merge 4 commits into
Conversation
Three cases where vendored JVM (Maven reactor) liveness proved an entry "wired" and let vex attest while the build may still use the unpatched jar. Hosted mode and VEX discovery already refuse all three. J2, executable classifier copies: when the project also declares an executable classifier copy of the patched GA (tests, a native build), the reactor planner only warned `classifier_declared` and moved on, so `attestable_checked` passed. The classifier-less pin doesn't retarget the copy and the vendored tree serves only the main jar, so Maven fetches the public release and its code runs. A used (non-managed) executable classifier declaration whose version isn't the suffixed one now marks the reactor uncertain, so the entry isn't attestable. `sources` and `javadoc` still only warn. The executable-classifier rule now lives in `formats::maven::is_executable_classifier`, shared with the hosted rewriter and discovery. J8, unscoped "wired" check: `maven_reactor::wired_checked` was a substring match for the suffixed version over every reactor pom's comment-masked text. A suffixed version left only in a build plugin's dependencies, a profile, an unused property, another artifact's or a classifier copy's declaration kept the entry live. It now requires a `<dependencies>`/`<dependencyManagement>` declaration outside `<profiles>` (Doc::declarations already excludes `<build>`/`<reporting>`) naming the GA's main jar at the suffixed version, literally or through a property the checkout defines. That covers `vex`/`attestable_checked`, `vendor --check` (`check_entry`) and the plan's `refuse_unwired`. The old substring check is kept as `references_checked` and used only by the `scan --prune` GC (`entry_references`): the maven.config tail also serves plugin resolution, so a tree a `<build>` plugin or a profile still names must not be reverted out from under the build. J15, fail-open on read errors: `entry_wired` was `entry_wired_checked(..).unwrap_or(true)`, so an unreadable or unparseable pom made `Discovery::vendor_entry_live` (vendor's kept-patch rule, scan's takeover classifier and vex's ledger liveness) report a JVM entry as live. It now fails closed (`false`). The GC doesn't use it: it asks `entry_references`, which still answers "keep" on a read error, so nothing is deleted on an unknown answer. Tests (each fails on origin/main): - maven_reactor: an_executable_classifier_copy_is_not_attestable - maven_reactor: a_suffixed_version_outside_the_declarations_is_not_wired - apply: an_unreadable_pom_is_not_live_but_is_kept (also pins that entry_references / vendor_entry_in_use still keep the tree) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review findings on the profile / user-property attestation fix:
- A profile <dependencyManagement> entry was flagged even when the live
pin is a direct literal <version>, which beats management; likewise a
profile-managed classifier entry no declaration takes. Profile
management now withholds the pin only when management governs it (no
live direct version). The old test case managed(Some("1.7.36")) beside
a direct pin asserted the wrong outcome and now expects an attested
pin; the governing case (managed live pin) expects the opposite.
- A version-less profile classifier copy was always flagged. One shared
classifier check now covers live and profile copies and resolves a
version-less copy from the classifier-keyed management (live, with a
profile's entry overriding it; none in this pom fails closed). Profile
classifier copies report MavenClassifierUnpatched, like live ones.
- The same check now runs for the pre-v5 vendored wiring too, where any
executable classifier copy comes from the next repository (the vendored
tree holds only the main jar). formats::maven gains
NON_EXECUTABLE_CLASSIFIERS / is_executable_classifier, worded as in
#2006 so the two merge cleanly.
- A pom-typed (or java-source / javadoc) profile declaration carries no
code and no longer counts against the pin. Other types (test-jar, ejb)
still do: they put a jar on a classpath.
- PomDep now keeps both the version as the pom spells it (`version`, what
hosted writes and rolls back) and as the build resolves it
(`build_version`, -D user properties first). Pins are found from the
former, so a pin .mvn/maven.config moves stays a ref (list / rollback /
remove and liveness keep the wiring) but is marked unattested with the
new UnattestedKind::MavenConfigOverride (CLI note
vex_maven_config_override). A Socket version only maven.config
supplies is diagnosed (DIAG_REF_UNATTRIBUTABLE naming
.mvn/maven.config), never a pom.xml ref, so nothing edits pom.xml for
it.
- The build view leaves project.* / pom.* to the model: -Dproject.version
no longer resolves a ${project.version} dependency version.
- Cleanup: main_jar_declarations / classifier_declarations take a scope
predicate and back ga_versions, the classifier check and the profile
check; group_by_ga is gone (live_gas lists the pin candidates);
trusted_checksums takes the maven.config text extract already read.
Tests: version_property_reads_maven_config_overrides,
profile_declaration_off_the_pin_is_not_attested and
vendored_ref_bypassed_by_a_profile_or_classifier_is_not_attested
(renamed from vendored_ref_under_a_profile_version_is_not_attested) were
extended with a case per finding; each new case failed on the previous
commit. formats::maven gains model_reference_version_ignores_user_properties.
The golden corpus is unchanged.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fixes all nine /code-review high findings on #2006. 1. A typed copy of the GA (`<type>test-jar</type>`, which is the `tests` jar, or `ejb`, `ejb-client`, `maven-plugin`, an unknown type) was skipped with a bare `continue`, so it still attested. A new shared rule, `formats::maven::is_executable_variant(type, classifier)`, maps types to the classifier they imply and treats every type except `pom`, `java-source` and `javadoc` as executable. The reactor now warns `classifier_declared` for such a copy and marks it uncertain unless every reader resolves it to the suffixed version. 2. With `entry_wired` failing closed, `scan`'s overlap takeover classifier read an unreadable pom, or vendored wiring left only in a plugin or profile, as a dead vendored claim. It then emitted `redirect_supersedes_vendored` ("remove <purl>") while the GC still kept the entry. New tri-state: `apply::entry_liveness`, `Discovery::vendor_entry_liveness` and `LedgerLiveness:: vendor_entry_state`. It returns `None` when the claim is undecided (read error, or still referenced without attestable wiring), and the classifier stays silent on `None`. 3. A classifier copy's version now goes through the same override check as the main jar's (`version_moved`, generalized from `version_override`). An inheriting pom or a profile that moves the property off the suffixed version makes it uncertain. 4. A version-less declaration now reads its version from the nearest local `<dependencyManagement>` on its chain, interpolated as the declaring pom reads it. Anything other than the suffixed version, including no local management (a local root left unpinned), is uncertain. One pinned root no longer proves an unpinned sibling root. 5. `vendor --check` now agrees with `vex`. `check_entry` returns `Ok(Some(why))` when the entry is intact and wired but not attestable. `vendor --check` keeps the entry `verified` and adds the new run warning `vendor_jvm_unattested` naming the declarations (documented in CLI_CONTRACT.md). It is a warning rather than a failure because the contract's unattested-references rule says liveness gates keep such wiring live, and re-vendoring cannot clear it. 6. `Reactor::wires` now interpolates each declaration in the declaring pom and in every pom that inherits it, as Maven interpolates after inheritance. A parent's managed `${ct.version}` that each module defines as the pin now counts as wired again, so `refuse_unwired` no longer refuses that plan. 7. The classifier version check uses `Doc::coordinate`. 8. The main-jar predicate is `formats::maven::is_main_jar`, used through `Doc::is_main_jar` by `wires`, `rewrite_declarations`, `warn_unresolved_coordinates` and `jar_declaration`. 9. Removed the uncalled `maven_reactor::wired`. Tests (each fails with its fix neutralized): - plugin_dependencies_exclusions_and_other_types_are_untouched: updated. It pinned the silent `test-jar` pass-through; executable types now warn and don't attest, while `pom` and `java-source` still pass silently. - a_classifier_copy_a_reader_moves_back_to_the_base_is_not_attestable - a_version_less_declaration_under_an_unpinned_root_is_not_attestable - a_parent_declaration_its_modules_resolve_to_the_pin_is_wired - vendor_check_reports_an_unattestable_entry - maven_takeover_stays_silent_on_an_undecided_vendored_claim (CLI) - an_unreadable_pom_is_not_live_but_is_kept also covers the tri-state Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
bugbot run Generated by Claude Code |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Version-less pin ignores profile overrides
- Added managed_version_can_move() to detect when profiles or inheritors provide different managed versions, preventing unpatched JARs from reaching the classpath undetected.
Or push these changes by commenting:
@cursor push 911c1aa479
Preview (911c1aa479)
diff --git a/crates/socket-patch-core/src/vendor/jvm/maven_reactor.rs b/crates/socket-patch-core/src/vendor/jvm/maven_reactor.rs
--- a/crates/socket-patch-core/src/vendor/jvm/maven_reactor.rs
+++ b/crates/socket-patch-core/src/vendor/jvm/maven_reactor.rs
@@ -1808,6 +1808,68 @@
.flatten()
}
+ /// Whether a profile or inheritor can provide a different managed
+ /// version for `g:a` than what `rel` reads from top-level management,
+ /// either through profile property redefinition or profile-specific
+ /// `<dependencyManagement>` entries.
+ fn managed_version_can_move(&self, rel: &str, g: &str, a: &str, base: &str) -> bool {
+ // If the base managed version uses properties, check if profiles
+ // or inheritors redefine them to produce a different version.
+ if base.contains("${") {
+ if self
+ .version_moved(rel, base, &|v: &str| v != base)
+ .is_some()
+ {
+ return true;
+ }
+ }
+
+ // Check if any profile or inheritor has management that provides
+ // a different version (profile management or inheritor management).
+ std::iter::once(rel)
+ .chain(self.inheritors(rel))
+ .any(|reader| {
+ self.chain(reader).any(|at| {
+ let doc = &self.poms[at].doc;
+
+ // Check profile management entries
+ let profile_manages_differently = doc.declarations().into_iter().any(|dep| {
+ !doc.is_top_level_managed(dep) && doc.is_main_jar(dep) && {
+ let resolve = |v: &str| self.interpolate(reader, v, 0);
+ doc.coordinate(dep, "groupId", &resolve).as_deref() == Some(g)
+ && doc.coordinate(dep, "artifactId", &resolve).as_deref() == Some(a)
+ && doc.coordinate(dep, "version", &resolve).as_deref() != Some(base)
+ }
+ });
+
+ if profile_manages_differently {
+ return true;
+ }
+
+ // For inheritors, check if their top-level management differs
+ if reader != rel {
+ let inheritor_manages_differently =
+ doc.declarations().into_iter().any(|dep| {
+ doc.is_top_level_managed(dep) && doc.is_main_jar(dep) && {
+ let resolve = |v: &str| self.interpolate(reader, v, 0);
+ doc.coordinate(dep, "groupId", &resolve).as_deref() == Some(g)
+ && doc.coordinate(dep, "artifactId", &resolve).as_deref()
+ == Some(a)
+ && doc.coordinate(dep, "version", &resolve).as_deref()
+ != Some(base)
+ }
+ });
+
+ if inheritor_manages_differently {
+ return true;
+ }
+ }
+
+ false
+ })
+ })
+ }
+
/// Whether every pom that reads `dep` of `rel` resolves its version
/// to `sv`: `rel` and each pom inheriting it, with or without a
/// profile on its chain active (see [`Self::version_moved`]).
@@ -2050,13 +2112,20 @@
// A version-less declaration reads its management: under a
// local root left unpinned (management conflicting or
// unresolved outside the checkout) that may be the base.
- if !managed_section
- && self
- .managed_version(rel, patch.group_id, patch.artifact_id)
- .as_deref()
- != Some(sv)
- {
- uncertain.insert(at);
+ if !managed_section {
+ let managed = self.managed_version(rel, patch.group_id, patch.artifact_id);
+ if managed.as_deref() != Some(sv)
+ || managed.as_ref().is_some_and(|mv| {
+ self.managed_version_can_move(
+ rel,
+ patch.group_id,
+ patch.artifact_id,
+ mv,
+ )
+ })
+ {
+ uncertain.insert(at);
+ }
}
continue;
};You can send follow-ups to the cloud agent here.
A version-less declaration was treated as reading the suffixed version
whenever the nearest local management resolved to it, but that check
interpolated with no profile overlay and ignored profile management. A
profile that redefines the managed property, or manages g:a itself at
the base, puts the unpatched jar back on the classpath while liveness
still attested. Apply the same version_moved check versioned ${...}
declarations get, and reject profile management of g:a at another
version. Addresses the Bugbot High finding on maven_reactor.rs.
Co-Authored-By: Claude <noreply@anthropic.com>
|
bugbot run Generated by Claude Code |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 98716dd. Configure here.
|
Ready for review at 98716dd.
Generated by Claude Code |
Final review brief (for the human approver)What it does: this PR tightens liveness and attestation for vendored Maven reactor entries.
Risk: low-medium. Almost every change makes attestation stricter, so the likely failure is losing an attestation, not a false one. Deletion paths (GC, Look here
Verified
Changes I made: none. Open questions (non-blocking)
Auto-merge is not armed yet, because CI is red on the main-wide pdm failure. Once #2028 lands and this goes green, the next final-reviewer run arms it, and approving then sends it straight to the merge queue. Generated by Claude Code |
Second review round on vendored Maven liveness. - The vendored reactor judged a `<type>` that implies an executable copy (`test-jar` is the `tests` jar, `ejb-client`, `ejb`, `bundle`, an unknown type) like a classifier, but the hosted rewriter and VEX discovery looked only at `<classifier>`: a `test-jar` declaration at the base release was pinned (hosted) and attested (discovery) while Maven put the unpatched tests jar on the classpath, so the two modes answered differently for one project. `formats::maven::is_variant` / `variant_label` now define the variant both share with the reactor, and both use `is_executable_variant`; discovery's main-jar version grouping skips such variants too, and keys management by type as well as classifier. - `keep_older_vendored_patch` asked the attestation question (`vendor_entry_live`), so an older JVM vendoring that is still wired but not attestable (an executable classifier copy beside it) gave way and every re-run failed `vendor_prebuilt_required`. It now keeps the older patch unless its liveness is provably dead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Vendored JVM (Maven reactor) liveness attested in cases where hosted mode and VEX discovery don't. The owner rule says hosted's stricter behavior is ground truth, so this PR brings vendored mode in line.
What attested wrongly, and the fix
J2: executable classifier and type copies. The project could also declare an executable copy of the patched GA:
<classifier>tests</classifier>, a native classifier, or<type>test-jar</type>. The planner only warned (or, for typed copies, skipped silently), so the entry still attested. The classifier-less pin doesn't retarget the copy, and the vendored tree serves only the main jar, so Maven fetches the public release and its code runs.dependencyManagement) executable copy isuncertain, and so not attestable, unless every pom that reads it resolves the suffixed version. That check uses the same inheritor/profile override check as the main jar.formats::maven:is_main_jar: untyped orjar, no classifier.is_executable_variant: a type counts as the classifier it implies (test-jar→tests,ejb-client→client,java-source/javadoc→ non-executable). Every other type exceptpomis executable.is_executable_classifier: everything exceptsources/javadoc. Hosted mode and discovery use it too.<profile>also blocks attestation, because the profile may be active.J8: unscoped "wired" check.
maven_reactor::wired_checkedsearched every reactor pom's text for the suffixed version. A suffixed version that appeared only in a build plugin's dependencies, a<profile>, an unused property, another artifact's declaration or a classifier copy kept the entry wired.<dependencies>or<dependencyManagement>declaration outside<profiles>that names the main jar at the suffixed version.Doc::declarationsalready excludes<build>and<reporting>.${...}is interpolated in the declaring pom and in every pom that inherits it, as Maven interpolates after inheritance.<dependencyManagement>on its chain. One pinned local root no longer proves an unpinned sibling root.vex,vendor --checkand the planner'srefuse_unwireduse this check.J15: fail-open on read errors.
entry_wiredwasunwrap_or(true), so an unreadable or unparseable pom madeDiscovery::vendor_entry_livereport a JVM entry as live. It now fails closed.GC, the takeover classifier and
vendor --checkscan --pruneGC: unchanged on purpose. It usesentry_references, which still answers "keep" on a read error. The old text search stays asmaven_reactor::references_checkedfor the GC only, because the.mvn/maven.configtail repository also serves plugin resolution: a tree a<build>plugin or a profile still names must not be reverted out from under the build.scan's overlap takeover classifier: now uses a tri-state:apply::entry_liveness,Discovery::vendor_entry_livenessandLedgerLiveness::vendor_entry_state. "Unknown" is not "dead": an unreadable pom, or a tree still referenced without attestable wiring, givesNone. The classifier stays silent then, instead of emittingredirect_supersedes_vendored("remove ") for an entry the GC keeps.vendor --check: now agrees withvex. An intact, wired entry that isn't attestable staysverified, and the run gets the new warningvendor_jvm_unattested, which names the declarations. The new code is documented inCLI_CONTRACT.md. It's a warning, not a failure, because of the contract's unattested-references rule: liveness gates keep such wiring live, and re-vendoring cannot clear it.Other behavior changes
vendor's kept-older-patch rule: an unreadable kept entry now means the unserved upgrade fails asvendor_prebuilt_requiredinstead of being a benign skip.refuse_unwired: a plan whose only wiring is a rewritten profile declaration, with no root pin, is refused asnothing_wired.classifier_declareddegraded warning to the plan.Review findings (/code-review high)
All nine are confirmed and fixed. Each behavior test fails when its fix is neutralized.
test-jarskipped before the classifier check. Fixed with the shared type rule.plugin_dependencies_exclusions_and_other_types_are_untouchedpinned the old silent pass-through; it now expects executable types to warn and not attest, whilepomandjava-sourcestill pass silently.maven_takeover_stays_silent_on_an_undecided_vendored_claim(CLI), with a superseded control case.a_classifier_copy_a_reader_moves_back_to_the_base_is_not_attestable.a_version_less_declaration_under_an_unpinned_root_is_not_attestable.vendor --checkignored J2. It now runsunattestable_checkedand warnsvendor_jvm_unattested. Test:vendor_check_reports_an_unattestable_entry.a_parent_declaration_its_modules_resolve_to_the_pin_is_wired.attestable_checkedstill treats a parent declaration whose${...}the parent itself can't resolve as uncertain (property_unresolved); that rule predates this PR and I left it strict.Doc::coordinate.Doc::is_main_jar/formats::maven::is_main_jar.maven_reactor::wired. Removed.Tests
cargo test -p socket-patch-core --lib -- vendor::jvm:: vex::discover:: formats::maven: 710 passedcommitted_fixture_corpus_matches_golden: passes, output unchanged-- vendor:: patch::redirect:: utils::purl_key: 3756 passed-- takeover commands::vendor::: 76 passed--test vendor_jvm_cli: 54 passed--test maven_sidecar_cli: 28 passed--test e2e_vendor_jvm_build: 35 passed, 6 ignored--test e2e_vendor_maven_build: 4 passed, 1 ignoredcargo clippy --locked --workspace --all-features -- -D warningsandcargo fmt --all -- --checkare clean.Review findings, round 2 (
/code-review highone0c351f+98716dd, cloud routine)Hosted mode and discovery ignored
<type>-implied executable copies: confirmed and fixed. The vendored reactor treats a type liketest-jar(that is, thetestsjar),ejb-client,ejb,bundleor an unknown type as an executable copy. The hosted rewriter and VEX discovery only looked at<classifier>. So atest-jardeclaration at the base release was pinned by hosted mode and attested by discovery, even though Maven puts the unpatched tests jar on the classpath. All three now share one rule:formats::maven::is_variant/variant_labelandis_executable_variant.Tests:
maven_executable_classifier_variant_blocks_the_redirect(hosted) andexecutable_classifier_copy_off_the_pin_is_not_attested(discovery) both gain atest-jarcase.redirect_maven_classifier_unsupported.The kept-patch rule asked the attestation question: confirmed and fixed.
keep_older_vendored_patchusedvendor_entry_live. An older JVM vendoring that was still wired but not attestable (for example, an executable classifier copy beside it) therefore gave way, and every re-run failed withvendor_prebuilt_required. The older patch is now kept unlessvendor_entry_livenessproves it dead. There is no new CLI test: the path needs a full JVM vendor-and-upgrade fixture. It relies onentry_liveness, which is already covered.Declined:
vexreportsvendor_unwiredwithout the reactor's reason for a wired-but-unattestable JVM entry. This affects the note only, and the entry is still withheld. The fix needs a newUnattestedKindpath for JVM entries, which is a follow-up.classifier_declaredforsources/javadocis reported as degraded. That's a code/category nit.<pom>vs<pom>:<line>mix, repeated reactor parses, inheritor walks and a dead match arm. These are report format, performance and style issues.Tests (cloud run):
cargo test -p socket-patch-core --lib -- formats::maven vex::discover:: patch::redirect:: vendor::jvmgives 1581 passed, golden included. The 1 failure is the chmod test, which root ignores. CLI--lib -- vex_sources vendorand--test vendor_jvm_clipass. clippy and fmt are clean.Landing note:
is_executable_classifierandis_executable_variantoverlap with #2005, which also addsis_executable_classifier. When landing #2005 after this PR, keep this PR's shared helpers.Note
Medium Risk
Tightens Maven vendoring, liveness, and
vendor --checkbehavior across reactor parsing and scan flows—could change outcomes for edge-case POMs; no auth or network surface.Overview
Vendored Maven reactor entries are no longer treated as live/attested when the build can still resolve unpatched code: wiring now requires a main-jar declaration (or management the declaration always reads) at the suffixed version outside profiles/build; executable classifier/type copies share one rule with hosted/VEX and block attestation unless every reader resolves the pin.
GC vs attestation is split:
entry_references(text search, fail-open on read errors) still protectsscan --prune;entry_wired/vendor_entry_livefail closed on unreadable poms; a new tri-state (entry_liveness/vendor_entry_liveness) stops the takeover classifier from telling users to remove trees that are merely undecided.vendor --checkemitsvendor_jvm_unattestedwhen an entry is intact but unattestable (not a failure);CLI_CONTRACT.mddocuments the new warning. Largemaven_reactorand CLI/core tests cover classifier copies, version-less roots, profiles, and takeover silence.Reviewed by Cursor Bugbot for commit 98716dd. Configure here.
Generated by Claude Code