diff --git a/src/sgraph/converters/sbom_cyclonedx_generator.py b/src/sgraph/converters/sbom_cyclonedx_generator.py index eaee68c..1ad9dbc 100644 --- a/src/sgraph/converters/sbom_cyclonedx_generator.py +++ b/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 diff --git a/tests/converters/modelfile_for_sbom_maven_coordinates_tests.xml b/tests/converters/modelfile_for_sbom_maven_coordinates_tests.xml index a17424d..ea3abda 100644 --- a/tests/converters/modelfile_for_sbom_maven_coordinates_tests.xml +++ b/tests/converters/modelfile_for_sbom_maven_coordinates_tests.xml @@ -11,6 +11,19 @@ + + + + + + + + + + + + + + + + + + + diff --git a/tests/converters/sbom_cyclonedx_generator_test.py b/tests/converters/sbom_cyclonedx_generator_test.py index 7d93a3e..711ac06 100644 --- a/tests/converters/sbom_cyclonedx_generator_test.py +++ b/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 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():