SBOM: include version-managed and parent-pom Maven dependencies

src/sgraph/converters/sbom_cyclonedx_generator.py

@@ -27,8 +27,26 @@ def slugify_bom_ref(name: str) -> str:
 
 
 def valid_for_bom(elem):
+    # The coordinate branch demands an incoming reference where the version branches do not:
+    # version-management redirection leaves versionless elements behind after re-pointing their
+    # references at versioned ones, and coordinates alone cannot tell those husks apart from a
+    # BOM-managed dependency something still uses.
+    #
+    # parent_version alone deliberately does NOT qualify. Stored models predating
+    # coordinate-carrying parents hold parent_version-only elements, and admitting them splices
+    # the space-bearing element name into a generic purl. On models that do carry coordinates,
+    # the coordinate branch already admits every parent, so a parent_version clause would add
+    # nothing there and regress everything before.
+    #
+    # The coordinates must also be usable, not merely present: charset-rejected ones (a ${}
+    # groupId resolved only in an external parent) build no maven purl, and versionless there
+    # is nothing spec-clean left to emit — the fallback would splice the space-bearing element
+    # name into a generic purl.
     return 'version' in elem.attrs or ' of version ' in elem.name or ' of tag ' in elem.name \
-           or 'license' in elem.attrs
+           or 'license' in elem.attrs \
+           or (is_maven_coordinate(elem.attrs.get('groupId', ''))
+               and is_maven_coordinate(elem.attrs.get('artifactId', ''))
+               and bool(elem.incoming))
 
 
 def extract_version(elem):
@@ -49,6 +67,8 @@ def extract_version(elem):
         version = elem.name.split(' of version ')[-1].strip()
     elif ' of tag ' in elem.name:
         version = elem.name.split(' of tag ')[-1].strip()
+    elif 'parent_version' in elem.attrs:
+        version = elem.attrs['parent_version']
     if version is None:
         return None
     return version.replace(VERSION_PATH_SEPARATOR_ENCODING, '/')
@@ -325,13 +345,17 @@ def maven_purl(elem, version):
     An unresolved version yields a versionless purl rather than one carrying the expression. A
     purl version must be percent-encoded, so the expression would be either non-canonical raw or
     canonical-but-unmatchable encoded; omitting it yields a purl that is canonical and still
-    matches at package level.
+    matches at package level. An empty version takes the same branch: appending it would leave a
+    trailing '@', which is not a canonical versionless purl but a malformed versioned one. So
+    does a semicolon-joined multi-value, the shape attribute transfer produces when two poms
+    name one parent at different versions: it asserts a version that exists nowhere, while the
+    raw value stays disclosed in the component's version field.
     """
     group_id = elem.attrs.get('groupId', '')
     artifact_id = elem.attrs.get('artifactId', '')
     if not (is_maven_coordinate(group_id) and is_maven_coordinate(artifact_id)):
         return None
-    if is_unresolved_version(version):
+    if not version or ';' in version or is_unresolved_version(version):
         return f'pkg:maven/{group_id}/{artifact_id}'
     return f'pkg:maven/{group_id}/{artifact_id}@{version}'
 
@@ -430,6 +454,10 @@ def purl_for(elem, v):
         # and this property records why the purl carries no version.
         properties.append({'name': VERSION_SOURCE_PROPERTY, 'value': v})
         return f'pkg:{pkgtype}/{pkgid}', properties
+    # The versionless-inclusion rule made an empty version reachable here, not only in
+    # maven_purl: charset-rejected coordinates drop a versionless element to this splice.
+    if not v:
+        return f'pkg:{pkgtype}/{pkgid}', properties
     return f'pkg:{pkgtype}/{pkgid}@{v}', properties
 
 

tests/converters/modelfile_for_sbom_maven_coordinates_tests.xml

@@ -11,6 +11,19 @@
             <r r="22" t="ref" />
           </e>
         </e>
+        <!-- These references are the whole difference between managed-lib and husk-lib: the
+             inclusion rule for versionless elements is incoming references, and only one of the
+             pair has any. Removing either reference or pointing one at husk-lib collapses the
+             pair into indistinguishable elements and the rule's control is gone. legacy-parent
+             is referenced too, deliberately: its exclusion must rest on its missing coordinates
+             alone, or the test would pass for the wrong reason on a missing reference. -->
+        <e i="11" n="pom.xml" t="file">
+          <r r="30" t="mvn" />
+          <r r="32" t="mvn_parent" />
+          <r r="33" t="mvn_parent" />
+          <r r="34" t="mvn_parent" />
+          <r r="35" t="mvn" />
+        </e>
       </e>
       <e n="External">
         <!-- The Maven bucket sits under a JVM layer here, as analyzer output does. The branch
@@ -48,6 +61,48 @@
             <e i="24" n="org.example.unresolved unresolved-lib of version ${project.version}"
               artifactId="unresolved-lib" groupId="org.example.unresolved"
               repotype="Maven" version="${project.version}" />
+            <!-- No version anywhere: it is managed by a BOM or parent the analyzer never parses.
+                 The element is referenced from pom.xml, which is what earns it a component.
+                 Giving it a version attribute would turn the versionless-inclusion test into a
+                 restatement of the ordinary versioned case. -->
+            <e i="30" n="org.example.managed managed-lib"
+              artifactId="managed-lib" groupId="org.example.managed"
+              repotype="Maven" />
+            <!-- The unreferenced control for managed-lib: identical shape, no incoming reference.
+                 Version-management redirection leaves elements like this behind after their
+                 references were re-pointed at versioned ones. If this ever appears as a
+                 component, the inclusion rule has decayed into mere coordinate presence. -->
+            <e i="31" n="org.example.husk husk-lib"
+              artifactId="husk-lib" groupId="org.example.husk"
+              repotype="Maven" />
+            <!-- An external parent pom: its exact version is known but arrives under
+                 parent_version, because that is the attribute a <parent> block produces. Renaming
+                 this to version would make the parent_version extraction guard vacuous. -->
+            <e i="32" n="org.example.parentpom parent-pom"
+              artifactId="parent-pom" groupId="org.example.parentpom"
+              parent_version="7.1" repotype="Maven" />
+            <!-- The old-model shape of a parent: every model persisted before the analyzer
+                 started writing coordinates onto parents carries parent_version alone. Emitting
+                 it would splice the space-bearing element name into a generic purl, the exact
+                 class the maven-purl work eliminated. Adding coordinates here would turn this
+                 back into parent-pom and retire the only stored-model regression guard. -->
+            <e i="33" n="org.example.legacyparent legacy-parent"
+              parent_version="5.5" repotype="Maven" />
+            <!-- Two poms naming the same parent at different versions land on one element, and
+                 the attribute transfer joins the values with a semicolon. A purl carrying the
+                 joined value would assert a version that exists nowhere; the raw value must stay
+                 disclosed in the component's version field only. -->
+            <e i="34" n="org.example.multiparent multi-parent"
+              artifactId="multi-parent" groupId="org.example.multiparent"
+              parent_version="4.1.0;3.2.0" repotype="Maven" />
+            <!-- Versionless AND charset-rejected coordinates: a ${} groupId whose property lives
+                 in an external parent. With a version such an element takes the generic residual;
+                 with neither version nor usable identity there is nothing spec-clean to emit, and
+                 the fallback would splice the space-bearing name into the purl. Resolving the
+                 groupId here would collapse this into managed-lib and retire the guard. -->
+            <e i="35" n="org.example.propgroup prop-group-lib"
+              artifactId="prop-group-lib" groupId="${project.groupId}"
+              repotype="Maven" />
           </e>
         </e>
       </e>

tests/converters/sbom_cyclonedx_generator_test.py

@@ -452,16 +452,142 @@ def test_maven_coordinate_guard_does_more_than_reject_whitespace():
     assert len(whitespace_bearing) == 3
 
 
-def test_maven_coordinates_fixture_yields_five_distinct_components():
+def test_maven_coordinates_fixture_yields_eight_distinct_components():
     """Anti-vacuity for the fixture, and the collapse guard for version omission.
 
     A lookup above raises when an element vanishes, but an element added, or two bom-refs
-    collapsed into one when a version is omitted, would otherwise go unnoticed.
+    collapsed into one when a version is omitted, would otherwise go unnoticed. Eight, not
+    ten: husk-lib and legacy-parent exist in the fixture and must not be counted here.
     """
     model, _ = get_model_and_model_api(MAVEN_COORDINATES_MODEL)
     sbom = sbom_cyclonedx_generator.generate_from_sgraph(model)
-    assert len(sbom['components']) == 5
-    assert len({component['bom-ref'] for component in sbom['components']}) == 5
+    assert len(sbom['components']) == 8
+    assert len({component['bom-ref'] for component in sbom['components']}) == 8
+
+
+# --- version-managed dependency tests ---
+
+
+def test_a_referenced_versionless_dependency_is_still_a_component():
+    """A dependency version-managed by an imported BOM has no version anywhere in the model.
+
+    The version is real but lives inside an artifact the analyzer never parses, so requiring a
+    version for inclusion drops exactly the dependencies modern Maven declares: the more a
+    project centralizes versions in parents and BOMs, the emptier its SBOM. A versionless maven
+    purl is canonical and still matches at package level — the same trade the
+    unresolved-expression case above already accepted.
+    """
+    component = get_maven_coordinate_components()['org.example.managed managed-lib']
+    assert component['purl'] == 'pkg:maven/org.example.managed/managed-lib'
+    assert component['version'] == ''
+    assert purl_type_resolution(component) is None
+
+
+def test_an_unreferenced_versionless_element_is_not_swept_in():
+    """The inclusion rule for versionless elements is incoming references, not existence.
+
+    Version-management redirection re-points references at versioned elements and leaves the
+    versionless originals behind with none. husk-lib is the control for managed-lib: identical
+    shape, no incoming reference. Without it the rule could decay into plain coordinate
+    presence and every other assertion would stay green.
+    """
+    assert 'org.example.husk husk-lib' not in get_maven_coordinate_components()
+
+
+def test_parent_version_supplies_the_version_of_an_external_parent_pom():
+    """A parent pom reference records its exact version under parent_version, not version.
+
+    The analyzer read that version out of the <parent> block it parsed, so dropping the
+    component, or emitting it versionless, discards information the model already holds.
+    """
+    component = get_maven_coordinate_components()['org.example.parentpom parent-pom']
+    assert component['purl'] == 'pkg:maven/org.example.parentpom/parent-pom@7.1'
+    assert component['version'] == '7.1'
+
+
+def test_an_explicit_version_outranks_parent_version():
+    """An element that is both a parent and an ordinary dependency keeps its own version.
+
+    No fixture element carries both attributes, deliberately: this ordering is a property of
+    extract_version alone, and a fixture pinning it would couple two orthogonal guards.
+    """
+    elem = SElement(None, 'org.example both')
+    elem.attrs.update(version='1.0', parent_version='2.0')
+    assert sbom_cyclonedx_generator.extract_version(elem) == '1.0'
+
+
+def test_a_legacy_parent_without_coordinates_is_not_emitted():
+    """Models persisted before the analyzer wrote coordinates onto parents must stay excluded.
+
+    SBOMs are generated on demand from stored models with a multi-month lifetime, so the
+    generator meets old shapes long after the analyzer moved on. A parent_version-only element
+    has no coordinates to build a maven purl from; emitting it would splice the space-bearing
+    element name into a generic purl — the exact class the maven-purl work eliminated. The
+    fixture element is referenced, deliberately: exclusion must rest on the missing coordinates,
+    not on a missing reference.
+    """
+    assert 'org.example.legacyparent legacy-parent' not in get_maven_coordinate_components()
+
+
+def test_an_ambiguous_parent_version_stays_out_of_the_purl():
+    """Two poms naming one parent at different versions collide on one versionless element.
+
+    The attribute transfer joins their versions with a semicolon. A purl carrying the joined
+    value asserts a version that exists nowhere and matches nothing; omitting it keeps the purl
+    canonical and package-level matchable, while the raw value stays disclosed in the version
+    field — the same split the unresolved-expression case established.
+    """
+    component = get_maven_coordinate_components()['org.example.multiparent multi-parent']
+    assert component['purl'] == 'pkg:maven/org.example.multiparent/multi-parent'
+    assert component['version'] == '4.1.0;3.2.0'
+
+
+def test_versionless_inclusion_requires_usable_coordinates():
+    """Charset-rejected coordinates plus no version leave nothing spec-clean to emit.
+
+    A ${} groupId whose property lives in an external parent fails the coordinate guard, so no
+    maven purl can be built; versionless, the element predates this feature in no BOM at all,
+    and admitting it now would splice the space-bearing element name into a generic purl. A
+    versioned element with the same broken coordinates still takes the generic residual as
+    before — this rule is about what the versionless-inclusion branch may admit, not about
+    tightening the residual.
+    """
+    assert 'org.example.propgroup prop-group-lib' not in get_maven_coordinate_components()
+
+
+def test_the_generic_fallback_never_splices_an_empty_version():
+    """The versionless-inclusion rule made an empty version reachable on the fallback path.
+
+    Coordinates that fail the charset guard, such as an unresolved ${project.groupId}, drop the
+    element to the generic branch, and a versionless element then reaches the final splice with
+    an empty version. Appending it would emit a trailing '@' — not a canonical versionless purl
+    but a malformed versioned one, the same shape maven_purl already refuses.
+    """
+    maven_bucket = SElement(None, 'Maven')
+    elem = SElement(maven_bucket, 'caffeine')
+    elem.attrs.update(groupId='${project.groupId}', artifactId='caffeine')
+    purl, properties = sbom_cyclonedx_generator.purl_for(elem, '')
+    assert purl == 'pkg:generic/caffeine'
+    assert properties == [{'name': 'purlTypeResolution', 'value': 'maven coordinates unavailable'}]
+
+
+def test_no_fixture_purl_carries_a_space_a_semicolon_or_a_trailing_at():
+    """The cross-shape invariant the individual guards above defend, stated once directly.
+
+    Findings against stored models all took one of these three shapes; asserting the invariant
+    over every fixture generation catches a regression in any of them even if the targeted
+    test for that shape is later weakened.
+    """
+    for model_file in (MAVEN_COORDINATES_MODEL, BINARY_REFS_MODEL,
+                       'converters/modelfile_for_sbom_tests.xml',
+                       'converters/modelfile_for_sbom_multi_tests.xml'):
+        model, _ = get_model_and_model_api(model_file)
+        sbom = sbom_cyclonedx_generator.generate_from_sgraph(model)
+        for component in sbom['components']:
+            for ref in (component['purl'], component['bom-ref']):
+                assert ' ' not in ref, ref
+                assert ';' not in ref, ref
+                assert not ref.endswith('@'), ref
 
 
 def test_a_partly_resolved_version_keeps_its_version():