fix: java-archive-cataloger returns unknown version for uber-jar - #5164
Conversation
…anchore#5163 Signed-off-by: Rez Moss <hi@rezmoss.com>
|
@wagoodman could plz review this |
CAOShurong
left a comment
There was a problem hiding this comment.
Independent exact-head verification of this PR — the fix works and the guardrails hold. All findings below reproduced locally.
Disclosure: I have an overlapping open PR for #5163 (#5219) implementing the same fallback idea, plus verification comments on the issue itself. This is an independent review of this implementation on its own merits; maintainers will likely want to pick one.
What I verified (head a7bc3ea0, base bf82010f, go1.26 windows-amd64)
- All three new table cases of
Test_archiveParser_versionFromPropertiesFilePASS, plusTest_releaseVersionPattern(10 subcases) and the nil-manifest test. The other failing tests insyft/pkg/cataloger/javaare environment-only on this host (make-generatedtestdata/java-buildsfixtures require real maven/gradle builds); they fail identically without this patch's changes involved. go vet ./...clean on the touched package; gofmt deviations are pre-existing CRLF artifacts identical at base.
End-to-end probe through the full cataloger (both binaries built from head and base)
Five synthetic jars replicating shapes discussed on the issue:
| Probe | Shape | base | head |
|---|---|---|---|
| A | app uber-jar, Main-Class + Multi-Release, root version.properties with tag=v0.63.5 |
UNKNOWN |
0.63.5 ✅ closes #5163 |
| B | library jar (no Main-Class) carrying a shaded dep's com/vendor/dep/version.properties |
UNKNOWN |
UNKNOWN ✅ correctly rejected |
| B2 | app uber-jar where the only version.properties is nested (clojure/version.properties) |
UNKNOWN |
UNKNOWN ✅ root gate holds |
| C | root version.properties with version=${revision} |
UNKNOWN |
UNKNOWN ✅ placeholder rejected |
| D | root tag=v2.19.0 |
UNKNOWN |
2.19.0 |
The exact-root glob gate (GlobMatch(true, "/version.properties")) does exactly what the issue thread said was necessary: I previously counted ~40 nested third-party */version.properties entries inside the real metabase.jar, and probes B/B2 confirm none of them can leak into the version now. Case-insensitivity also comes along for free from GlobMatch(true, …) (normalizeZipEntryName lowercases entries), which is a nice bonus over an exact string compare. manifest.Main.MustGet("Main-Class") is nil-safe (KeyValues.MustGet returns ""), so the nil-manifest path is covered twice over.
Two non-blocking notes
- Date-shaped values pass the regex. Empirically probed: a root
version.propertieswithversion=2024-08-05yields package version2024-08-05(and purl…/example.core.bootstrap/2024-08-05).releaseVersionPatternaccepts any digit-leading run including-, so ISO dates sail through. Since Metabase's own file ships adate=key (correctly ignored today because you whitelistversion/tag), a build that instead wrote the date intoversion=would get a nonsense-but-plausible-looking version. Cheap hardening: reject values matching^\d{4}-\d{2}-\d{2}$. v-prefix stripping is an opinion worth stating explicitly.tag=v2.19.0→2.19.0keeps the purl well-formed (matches the normalization I suggested on the issue), but it silently rewrites what the build declared. Fine either way — just worth a sentence in the doc comment so future readers know it's intentional.
Nice touch keeping LIB_WITH_VERSION_PROPERTIES (the no-Main-Class negative case) in the fixture set — that's the FP vector most quick implementations would miss. Functionally ready from where I sit; merge decision obviously yours, and feel free to close in favor of whichever implementation you prefer given the duplicate.
fixed #5163
Description
fixed java-archive-cataloger returns unknown ver for uber-jar
before
after
Type of change
Checklist
Issue references