Skip to content

Fix tool.SBOM.cyclonedx with empty components#17925

Merged
memsharded merged 5 commits intoconan-io:develop2from
NokiDev:fix-tool.sbom.cyclonedx-with-empty-components
Mar 17, 2025
Merged

Fix tool.SBOM.cyclonedx with empty components#17925
memsharded merged 5 commits intoconan-io:develop2from
NokiDev:fix-tool.sbom.cyclonedx-with-empty-components

Conversation

@NokiDev
Copy link
Contributor

@NokiDev NokiDev commented Mar 10, 2025

Changelog: BugFix: Add correct info in metadata + prevent crash when no component is associated to root_node.
Docs: Omit

Fix issue #17924

  • Refer to the issue that supports this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

@CLAassistant
Copy link

CLAassistant commented Mar 10, 2025

CLA assistant check
All committers have signed the CLA.

@ErniGH
Copy link
Contributor

ErniGH commented Mar 12, 2025

Hi @NokiDev, I’ve been looking into the issue, and there is indeed a problem. The line that fixes it is:
"bom-ref": special_id if has_special_root_node else f"pkg:conan/{conanfile.name}@{conanfile.ref.version}?rref={conanfile.ref.revision}",
so I think it would be best to minimize the pull request, focusing on fixing the issue so we can merge it and resolve the bug.

I see too many changes unrelated to the main issue, and I believe it would be better to discuss those in a separate PR.

Regarding the author field, it is indeed incorrect to use "conan" since the standard refers to "The person(s) or organization(s) that authored the component." I think it would be best to remove it altogether, as it is not a mandatory field in the standard, and it is not filled in the Conan Center Index recipes, which could cause issues.

@NokiDev NokiDev force-pushed the fix-tool.sbom.cyclonedx-with-empty-components branch from ddbc133 to cb5750a Compare March 12, 2025 16:13
Copy link
Contributor

@ErniGH ErniGH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks for your PR 😄

@NokiDev
Copy link
Contributor Author

NokiDev commented Mar 12, 2025

@ErniGH, I've followed your advice and limited PR change to the fix.
I'm open to discuss all other changes in another issue / PR if there is one yet

@ErniGH
Copy link
Contributor

ErniGH commented Mar 12, 2025

@NokiDev I feel a bit uneasy about not having had a test that could catch this error. I'm trying to add a small test to cover this case, but I'm unable to replicate the issue you had when the component list was empty. Could you help me? What was your use case for triggering an empty component list?

@NokiDev
Copy link
Contributor Author

NokiDev commented Mar 12, 2025

Sure, in my case I was using a conanfile with tool_requires and calling the function with use_build=False (the default I believe)

But without any requires that should also work no ?
Otherwise having a test that check the outputed spdx.json file content shall be better.
And then you can check that in the metadata section you have intended info ?
IMO better to check intended output than all error ones.

If you still struggle, I can add a test tomorrow - not sure how testing a hook with conan TestClient tough

@ErniGH
Copy link
Contributor

ErniGH commented Mar 13, 2025

I have added a test that has a "post_package" hook with add_build set to false and add_test set to false as well. I create a conanfile.py without any requirements and build it using conan create. This test does not generate an empty component list in the SBOM. How did you do it? 🤔

@NokiDev NokiDev force-pushed the fix-tool.sbom.cyclonedx-with-empty-components branch from 0b6f0ba to 4552ed2 Compare March 13, 2025 13:47
class FooPackage(ConanFile):
name = "foo"
version = "1.0"
package_type = "build-scripts"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I achieved to reproduce the issue when there is no components by setting package-type = "build-scripts"
Not really sure why, it triggers correctly the variable has_special_root_node which in turn would strip 1st component from the graph and triggers the issue.

bar_layout = tc.created_layout()
assert os.path.exists(os.path.join(bar_layout.metadata(), "sbom.cdx.json"))

def test_sbom_with_dependencies(hook_setup_post_package_default):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified this test to ensure sbom is generated with appropriate component ref in the metadata.

@NokiDev
Copy link
Contributor Author

NokiDev commented Mar 13, 2025

@ErniGH Achieved to reproduce, I believe I might have tried to use the post_generate hook too when issue occured. My bad.

I updated the tests to reflect the changes I made.

BTW if you have a less weird way to trigger the has_special_root_node behaviour, feel free to update the test. I found that having package_type set as build scripts works. IMHO stripping the 1st component shall be the default behaviour since information will be present in the metadata section

Edit: This seems really weird because has_special_root_node is True, thus the special id is used in bom-ref and it shouldn't execute the else clause where c.name etc... are used but it seems to evaluate the else clause as well.
Is this a python interpreter normal behaviour ?

@NokiDev NokiDev force-pushed the fix-tool.sbom.cyclonedx-with-empty-components branch from 4552ed2 to f01d7f9 Compare March 13, 2025 15:49
@memsharded memsharded added this to the 2.15.0 milestone Mar 13, 2025
@ErniGH
Copy link
Contributor

ErniGH commented Mar 14, 2025

@NokiDev The problem here is that the subgraph generated of a build-script without requirements generates a single node, the context of which is built and is not used by the SBOM. The test is able to capture the error, and the fix resolves the bug.
Good job!

Copy link
Contributor

@ErniGH ErniGH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@memsharded memsharded merged commit 306aee0 into conan-io:develop2 Mar 17, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants