From 10811826e2d05aa99b44cf0666ef37ca760bd627 Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Sat, 5 Sep 2026 01:15:43 -0700 Subject: [PATCH 1/6] feat(sbom): carry a document's own component assertions through ingest Phase 2.5, first half of #396. CycloneDX and the graph hop; SPDX, document assertions, the fixed-point test and hostile-document fuzz follow on this branch. `bomly scan --sbom --format spdx` silently dropped most of what its input asserted, because only coordinates, scope, copyright and licenses survived Document -> Graph -> Document. Supplier, originator, description, homepage, checksums, CPEs and the document's own external references now cross that hop as the typed SDK fields ADR-0037 defined for it. Every value clears an SDK gate at every crossing -- ingest and export both, not just the boundary that happens to be upstream. An ingested document is untrusted input Bomly re-emits under its own name, and the node in between is not a trusted carrier either: a detector or external plugin can write these fields directly, so a value can reach export having passed no ingest gate at all. That asymmetry was #391's last unfixed finding. The gates are the SDK's, never local: Contact.Normalized, ExternalReference .Normalized, Digest.Normalized, MergeExternalReferences for the set class. CPE grammar is reachable only through the reference gate, so a CPE is offered as a reference under both CPE types and admitted if either accepts it -- #396 records a hand-rolled CPE validator being retrofitted once already, its fuzz target breaking it immediately. Origin-derived reference types are deliberately NOT ingested. Bomly renders each origin as a distribution or vcs reference, and nothing in a document distinguishes ours from a third party's -- so ingesting them would let a detector assertion return as a source assertion and be re-emitted as one on the next hop, a detector claim laundered into a document claim across two conversions. TestOriginIsNotReadBackFromAnIngestedDocument already pinned the origin half of that rule and caught this when the reference half was missing. The cost is real and stated rather than hidden: a third-party document's own vcs or distribution reference is not preserved. Also bumps bomly-sdk to v0.9.3 for its merge-capacity hardening (bomly-dev/bomly-sdk#53), and applies the same bound to the CLI's own two `len(a)+len(b)` allocation hints in the SPDX encoder. Those lengths are attacker-influenced now that ingest carries a foreign document's assertions. Co-Authored-By: Claude Opus 5 --- go.mod | 6 +- go.sum | 20 +-- internal/sbom/cyclonedx.go | 18 +- internal/sbom/cyclonedx_assertions.go | 245 ++++++++++++++++++++++++++ internal/sbom/graph.go | 120 +++++++++++++ internal/sbom/model.go | 26 +++ internal/sbom/spdx23.go | 27 ++- internal/sbom/transform.go | 51 ++++++ 8 files changed, 496 insertions(+), 17 deletions(-) create mode 100644 internal/sbom/cyclonedx_assertions.go diff --git a/go.mod b/go.mod index 58aa397d..4161d651 100644 --- a/go.mod +++ b/go.mod @@ -4,7 +4,7 @@ go 1.27.0 require ( github.com/BurntSushi/toml v1.6.0 - github.com/CycloneDX/cyclonedx-go v0.11.0 + github.com/CycloneDX/cyclonedx-go v0.12.0 github.com/Masterminds/semver/v3 v3.5.0 github.com/bomly-dev/bomly-plugin-depsdev-license-matcher v0.2.0 github.com/bomly-dev/bomly-plugin-govulncheck-analyzer v0.2.0 @@ -15,7 +15,7 @@ require ( github.com/bomly-dev/bomly-plugin-pyreach-analyzer v0.2.0 github.com/bomly-dev/bomly-plugin-scorecard-matcher v0.2.0 github.com/bomly-dev/bomly-plugin-syft-detector v0.2.0 - github.com/bomly-dev/bomly-sdk v0.9.2 + github.com/bomly-dev/bomly-sdk v0.9.3 github.com/charmbracelet/bubbles v1.0.0 github.com/charmbracelet/bubbletea v1.3.10 github.com/charmbracelet/lipgloss v1.1.0 @@ -27,7 +27,7 @@ require ( github.com/spf13/pflag v1.0.10 go.uber.org/zap v1.28.0 golang.org/x/term v0.45.0 - google.golang.org/grpc v1.83.0 + google.golang.org/grpc v1.83.2 gopkg.in/yaml.v3 v3.0.1 ) diff --git a/go.sum b/go.sum index 0852b8e1..5b353842 100644 --- a/go.sum +++ b/go.sum @@ -77,8 +77,8 @@ github.com/BurntSushi/toml v0.4.1/go.mod h1:CxXYINrC8qIiEnFrOxCa7Jy5BFHlXnUU2pbi github.com/BurntSushi/toml v1.6.0 h1:dRaEfpa2VI55EwlIW72hMRHdWouJeRF7TPYhI+AUQjk= github.com/BurntSushi/toml v1.6.0/go.mod h1:ukJfTF/6rtPPRCnwkur4qwRxa8vTRFBF0uk2lLoLwho= github.com/BurntSushi/xgb v0.0.0-20160522181843-27f122750802/go.mod h1:IVnqGOEym/WlBOVXweHU+Q+/VP0lqqI8lqeDx9IjBqo= -github.com/CycloneDX/cyclonedx-go v0.11.0 h1:GokP8FiRC+foiuwWhSSLpSD5H4hSWtGnR3wo7apkBFI= -github.com/CycloneDX/cyclonedx-go v0.11.0/go.mod h1:vUvbCXQsEm48OI6oOlanxstwNByXjCZ2wuleUlwGEO8= +github.com/CycloneDX/cyclonedx-go v0.12.0 h1:/7Jum36UA6V043tQZ/fE3jf+Nf9gn/qxUFfd7QReMy8= +github.com/CycloneDX/cyclonedx-go v0.12.0/go.mod h1:V2577HhxDDCDLYfkm55WJrz16nHTfyQZwcWUBSG7Z28= github.com/DataDog/datadog-go v3.2.0+incompatible/go.mod h1:LButxg5PwREeZtORoXG3tL4fMGNddJ+vMq1mwgfaqoQ= github.com/DataDog/zstd v1.5.7 h1:ybO8RBeh29qrxIhCA9E8gKY6xfONU9T6G6aP9DTKfLE= github.com/DataDog/zstd v1.5.7/go.mod h1:g4AWEaM3yOg3HYfnJ3YIawPnVdXJh9QME85blwSAmyw= @@ -257,8 +257,8 @@ github.com/bomly-dev/bomly-plugin-scorecard-matcher v0.2.0 h1:VOK4+GfVGukbccabjD github.com/bomly-dev/bomly-plugin-scorecard-matcher v0.2.0/go.mod h1:3ux1Su5UCCKeF+wtttQrlw7r+B7sc6UXrK98Ybuq5gw= github.com/bomly-dev/bomly-plugin-syft-detector v0.2.0 h1:UlKwTqp+ZWu25leky9J6NITCKDdePpdgJQl/8dInApg= github.com/bomly-dev/bomly-plugin-syft-detector v0.2.0/go.mod h1:NVVrSMHkjC3VEDPtCI7+1c4tWBzwI1eve8tynO1pRoM= -github.com/bomly-dev/bomly-sdk v0.9.2 h1:nVOdoEV5pKzv/4Pze1WXyfdS6o/wnLTBA1Y+VRxvcVw= -github.com/bomly-dev/bomly-sdk v0.9.2/go.mod h1:Ulgp/NHlOLmbUvbKpuXylPf74LdCA54gpg+syK90Do8= +github.com/bomly-dev/bomly-sdk v0.9.3 h1:ynj+EDxdSZHwZaFmhNAFMAQJwQEgdW5JVrAm3kYgNBs= +github.com/bomly-dev/bomly-sdk v0.9.3/go.mod h1:gAGOEa88BE8gELt2rM/ZrqGIJnOzXiR6ocuuKW6AaJ0= github.com/bradleyjkemp/cupaloy/v2 v2.8.0 h1:any4BmKE+jGIaMpnU8YgH/I2LPiLBufr6oMMlVBbn9M= github.com/bradleyjkemp/cupaloy/v2 v2.8.0/go.mod h1:bm7JXdkRd4BHJk9HpwqAI8BoAY1lps46Enkdqw6aRX0= github.com/bufbuild/protocompile v0.14.1 h1:iA73zAf/fyljNjQKwYzUHD6AD4R8KMasmwa/FBatYVw= @@ -979,8 +979,8 @@ github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/ github.com/stretchr/testify v1.7.2/go.mod h1:R6va5+xMeoiuVRoj+gSkQ7d3FALtqAAGI1FQKckRals= github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4= -github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= -github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= +github.com/stretchr/testify v1.12.1 h1:EuwCh5fleGS7H32xRwO3wRGT7DxrDhLAT6FF8MpWDWE= +github.com/stretchr/testify v1.12.1/go.mod h1:MDEgiDPPsNp5cuIrHPPCyornHKgEVbtFUmoNlxoYthg= github.com/subosito/gotenv v1.2.0/go.mod h1:N0PQaV/YGNqwC0u51sEeR/aUtSLEXKX9iv69rRypqCw= github.com/subosito/gotenv v1.6.0 h1:9NlTDc1FTs4qu0DDq7AEtTPNw6SVm7uBMsUCUjABIf8= github.com/subosito/gotenv v1.6.0/go.mod h1:Dk4QP5c2W3ibzajGcXpNraDfq2IrhjMIvMSWPKKo0FU= @@ -990,8 +990,8 @@ github.com/sylabs/squashfs v1.0.6 h1:PvJcDzxr+vIm2kH56mEMbaOzvGu79gK7P7IX+R7BDZI github.com/sylabs/squashfs v1.0.6/go.mod h1:DlDeUawVXLWAsSRa085Eo0ZenGzAB32JdAUFaB0LZfE= github.com/tailscale/hujson v0.0.0-20260302212456-ecc657c15afd h1:Rf9uhF1+VJ7ZHqxrG8pJ6YacmHvVCmByDmGbAWCc/gA= github.com/tailscale/hujson v0.0.0-20260302212456-ecc657c15afd/go.mod h1:EbW0wDK/qEUYI0A5bqq0C2kF8JTQwWONmGDBbzsxxHo= -github.com/terminalstatic/go-xsd-validate v0.1.6 h1:TenYeQ3eY631qNi1/cTmLH/s2slHPRKTTHT+XSHkepo= -github.com/terminalstatic/go-xsd-validate v0.1.6/go.mod h1:18lsvYFofBflqCrvo1umpABZ99+GneNTw2kEEc8UPJw= +github.com/terminalstatic/go-xsd-validate v0.1.8 h1:UVrTCy1j3DhwaYTTUF+QYO/Nan13S0tf+Jwi+p45Bf0= +github.com/terminalstatic/go-xsd-validate v0.1.8/go.mod h1:1kb47fi2c6onlf+B7UrrQ9VYraOhcYwFm3iG+J6F4Zo= github.com/therootcompany/xz v1.0.1 h1:CmOtsn1CbtmyYiusbfmhmkpAAETj0wBIH6kCYaX+xzw= github.com/therootcompany/xz v1.0.1/go.mod h1:3K3UH1yCKgBneZYhuQUvJ9HPD19UEXEI0BWbMn8qNMY= github.com/tidwall/gjson v1.19.0 h1:xwxm7n691Uf3u5OFjzngavjGTh55KX5q/9w9xHW88JU= @@ -1536,8 +1536,8 @@ google.golang.org/grpc v1.39.1/go.mod h1:PImNr+rS9TWYb2O4/emRugxiyHZ5JyHW5F+RPnD google.golang.org/grpc v1.40.0/go.mod h1:ogyxbiOoUXAkP+4+xa6PZSE9DZgIHtSpzjDTB9KAK34= google.golang.org/grpc v1.40.1/go.mod h1:ogyxbiOoUXAkP+4+xa6PZSE9DZgIHtSpzjDTB9KAK34= google.golang.org/grpc v1.42.0/go.mod h1:k+4IHHFw41K8+bbowsex27ge2rCb65oeWqe4jJ590SU= -google.golang.org/grpc v1.83.0 h1:JeNZEKJFbQxArAMl+hiytHauacDNqJUllNfmIMmpqnQ= -google.golang.org/grpc v1.83.0/go.mod h1:kDyl6SKsiHKt0uylY5gtn5cEjkrIOhQOGDgIc4JGwzQ= +google.golang.org/grpc v1.83.2 h1:EManeRomTObA0BU7I8vXgg/78uE5MJ9M8B39EX2WscU= +google.golang.org/grpc v1.83.2/go.mod h1:YPI1hK3kDked6iHvgX3tR0y+nX/qpMFKhPgFsokw1S8= google.golang.org/grpc/cmd/protoc-gen-go-grpc v1.1.0/go.mod h1:6Kw0yEErY5E/yWrBtf03jp27GLLJujG4z/JK95pnjjw= google.golang.org/protobuf v0.0.0-20200109180630-ec00e32a8dfd/go.mod h1:DFci5gLYBciE7Vtevhsrf46CRTquxDuWsQurQQe4oz8= google.golang.org/protobuf v0.0.0-20200221191635-4d8936d0db64/go.mod h1:kwYJMbMJ01Woi6D6+Kah6886xMZcty6N08ah7+eCXa0= diff --git a/internal/sbom/cyclonedx.go b/internal/sbom/cyclonedx.go index 2b83c11d..4bdfe6da 100644 --- a/internal/sbom/cyclonedx.go +++ b/internal/sbom/cyclonedx.go @@ -8,6 +8,7 @@ import ( "time" cdx "github.com/CycloneDX/cyclonedx-go" + "github.com/bomly-dev/bomly-sdk" "github.com/bomly-dev/bomly-sdk/spdxkit" ) @@ -105,7 +106,7 @@ func (c cycloneDXCodec) decodeJSON(data []byte) (*Document, error) { componentByID := make(map[string]Component) if bom.Components != nil { for _, comp := range *bom.Components { - componentByID[comp.BOMRef] = Component{ + component := Component{ ID: comp.BOMRef, Name: comp.Name, Org: comp.Group, @@ -116,6 +117,8 @@ func (c cycloneDXCodec) decodeJSON(data []byte) (*Document, error) { Copyright: comp.Copyright, Licenses: parseCycloneDXLicenses(comp.Licenses), } + applyCycloneDXAssertions(&component, comp) + componentByID[comp.BOMRef] = component } } @@ -460,9 +463,20 @@ func cycloneDXComponent(comp Component) cdx.Component { if props := cycloneDXEOLProperties(comp.EOL); len(props) > 0 { component.Properties = &props } - if refs := cycloneDXComponentReferences(comp); len(refs) > 0 { + // Origin-derived references first, then the ones the source document + // asserted. Both are references about the same component and the format + // carries one list, so they concatenate; the emitted set is deduplicated + // by the SDK's reference identity before it is written. + refs := cycloneDXComponentReferences(comp) + refs = append(refs, cycloneDXEmittedReferences(comp.ExternalReferences)...) + if len(refs) > 0 { component.ExternalReferences = &refs } + component.Supplier = cycloneDXEntityFor(comp.Supplier) + if originator := cycloneDXEntityFor(comp.Originator); originator != nil { + component.Publisher = originator.Name + } + component.Description = sdk.NormalizeDescription(comp.Description) return component } diff --git a/internal/sbom/cyclonedx_assertions.go b/internal/sbom/cyclonedx_assertions.go new file mode 100644 index 00000000..3c11c3da --- /dev/null +++ b/internal/sbom/cyclonedx_assertions.go @@ -0,0 +1,245 @@ +package sbom + +import ( + "strings" + + cdx "github.com/CycloneDX/cyclonedx-go" + "github.com/bomly-dev/bomly-sdk" +) + +// This file carries a CycloneDX document's own component assertions in and +// out: supplier, originator, description, homepage, checksums, CPE, and the +// document's external references (ADR-0037, issue #396). +// +// Every value crosses the trust boundary in both directions, so every value +// clears an SDK gate at each crossing. An ingested document is untrusted +// input that Bomly re-emits under its own name. + +// cycloneDXSupplier reads a component's supplier as a contact. +// +// CycloneDX models an organization, so the kind is organization; a value that +// does not survive Contact.Normalized -- a control character that would +// corrupt the SPDX projection, an embedded address -- is dropped rather than +// repaired. +func cycloneDXSupplier(entity *cdx.OrganizationalEntity) *sdk.Contact { + if entity == nil { + return nil + } + contact := sdk.Contact{ + Kind: sdk.ContactKindOrganization, + Name: entity.Name, + } + if entity.URL != nil && len(*entity.URL) > 0 { + contact.URL = (*entity.URL)[0] + } + normalized, ok := contact.Normalized() + if !ok { + return nil + } + return &normalized +} + +// cycloneDXOriginator reads the party that authored the component. +// +// CycloneDX spells this three ways across versions: `publisher` (a string), +// the deprecated `author`, and `authors`. Publisher is preferred because it +// is the field 1.5 and 1.6 documents actually carry; the others are read only +// when it is absent, so an older document is not silently less preserved. +func cycloneDXOriginator(comp cdx.Component) *sdk.Contact { + candidates := []struct { + kind sdk.ContactKind + value string + }{ + {sdk.ContactKindOrganization, comp.Publisher}, + {sdk.ContactKindPerson, comp.Author}, + } + if comp.Authors != nil { + for _, author := range *comp.Authors { + candidates = append(candidates, struct { + kind sdk.ContactKind + value string + }{sdk.ContactKindPerson, author.Name}) + } + } + for _, candidate := range candidates { + if strings.TrimSpace(candidate.value) == "" { + continue + } + contact := sdk.Contact{Kind: candidate.kind, Name: candidate.value} + if normalized, ok := contact.Normalized(); ok { + return &normalized + } + } + return nil +} + +// cycloneDXComponentDigests reads a component's own checksums. +func cycloneDXComponentDigests(hashes *[]cdx.Hash) []Digest { + if hashes == nil { + return nil + } + digests := make([]Digest, 0, len(*hashes)) + for _, hash := range *hashes { + digests = append(digests, Digest{Algorithm: string(hash.Algorithm), Value: hash.Value}) + } + return digests +} + +// cycloneDXIngestedReferences reads the document's external references. +// +// CycloneDX has no category axis, so the category stays empty -- which is +// itself the assertion that this reference came from a format that has none, +// and is what lets the SPDX projection stay faithful rather than inventing +// one. The type is the document's own token, kept verbatim: both +// specifications keep adding types, and a reference this build does not +// recognize still round-trips. +func cycloneDXIngestedReferences(refs *[]cdx.ExternalReference) []sdk.ExternalReference { + if refs == nil { + return nil + } + converted := make([]sdk.ExternalReference, 0, len(*refs)) + for _, ref := range *refs { + if isOriginDerivedReferenceType(string(ref.Type)) { + continue + } + converted = append(converted, sdk.ExternalReference{ + Type: string(ref.Type), + Locator: ref.URL, + Comment: ref.Comment, + Hashes: cycloneDXReferenceHashes(ref.Hashes), + }) + } + return sdk.MergeExternalReferences(nil, converted) +} + +// isOriginDerivedReferenceType reports whether a reference type is one Bomly +// emits from a package origin. +// +// These are not ingested, and the reason is a laundering hazard rather than +// squeamishness. Origin is detector-asserted (ADR-0033): a detector says +// where it actually resolved a package, and that claim drives the +// dependency-confusion signal. Bomly renders each origin as a distribution or +// vcs reference on export. Nothing in the document distinguishes a reference +// Bomly derived from an origin from one a third party asserted, so ingesting +// them would let Bomly's own detector assertion return as a source assertion +// and be re-emitted as one on the next hop -- a detector claim promoted to a +// document claim across two conversions, with no detector behind it. +// +// The cost is real and stated rather than hidden: a third-party document's +// own vcs or distribution reference is not preserved. Every other category +// is. Recovering these needs a way to tell our emission from a source's, +// which the formats do not offer today; docs/SBOM.md records the limit. +// +// The same rule already applies to origins themselves -- +// TestOriginIsNotReadBackFromAnIngestedDocument pins it -- and this keeps the +// two halves of one decision from drifting apart. +func isOriginDerivedReferenceType(referenceType string) bool { + switch strings.ToLower(strings.TrimSpace(referenceType)) { + case string(cdx.ERTypeDistribution), string(cdx.ERTypeVCS): + return true + default: + return false + } +} + +// cycloneDXReferenceHashes reads a reference's own integrity claims, which +// CycloneDX carries natively and SPDX 2.3 has no slot for. +func cycloneDXReferenceHashes(hashes *[]cdx.Hash) []sdk.Digest { + if hashes == nil { + return nil + } + converted := make([]sdk.Digest, 0, len(*hashes)) + for _, hash := range *hashes { + converted = append(converted, sdk.Digest{ + Algorithm: sdk.DigestAlgorithm(hash.Algorithm), + Value: hash.Value, + }) + } + return converted +} + +// applyCycloneDXAssertions fills a component with what the document asserted +// about it, each value through its gate. +func applyCycloneDXAssertions(component *Component, comp cdx.Component) { + if component == nil { + return + } + component.Supplier = cycloneDXSupplier(comp.Supplier) + component.Originator = cycloneDXOriginator(comp) + component.Description = sdk.NormalizeDescription(comp.Description) + component.ExternalReferences = cycloneDXIngestedReferences(comp.ExternalReferences) + if digests := cycloneDXComponentDigests(comp.Hashes); len(digests) > 0 { + component.Digests = digests + } + if cpe := strings.TrimSpace(comp.CPE); cpe != "" { + component.CPEs = append(component.CPEs, cpe) + } +} + +// cycloneDXEntityFor renders a contact as a CycloneDX organizational entity. +func cycloneDXEntityFor(contact *sdk.Contact) *cdx.OrganizationalEntity { + if contact == nil { + return nil + } + normalized, ok := contact.Normalized() + if !ok || strings.TrimSpace(normalized.Name) == "" { + return nil + } + entity := &cdx.OrganizationalEntity{Name: normalized.Name} + if url := strings.TrimSpace(normalized.URL); url != "" { + urls := []string{url} + entity.URL = &urls + } + return entity +} + +// cycloneDXEmittedReferences renders the component's references, dropping any +// that no longer clears the gate. +// +// The gate runs again on the way out. The node these came from is not a +// trusted carrier -- a detector or an external plugin can write the field +// directly, and such a value never passed an ingest gate at all. +func cycloneDXEmittedReferences(refs []sdk.ExternalReference) []cdx.ExternalReference { + if len(refs) == 0 { + return nil + } + emitted := make([]cdx.ExternalReference, 0, len(refs)) + for _, ref := range refs { + normalized, ok := ref.Normalized() + if !ok { + continue + } + emitted = append(emitted, cdx.ExternalReference{ + Type: cdx.ExternalReferenceType(normalized.Type), + URL: normalized.Locator, + Comment: normalized.Comment, + Hashes: cycloneDXEmittedHashes(normalized.Hashes), + }) + } + if len(emitted) == 0 { + return nil + } + return emitted +} + +// cycloneDXEmittedHashes renders a reference's integrity claims. +func cycloneDXEmittedHashes(digests []sdk.Digest) *[]cdx.Hash { + if len(digests) == 0 { + return nil + } + hashes := make([]cdx.Hash, 0, len(digests)) + for _, digest := range digests { + normalized, ok := digest.Normalized() + if !ok { + continue + } + hashes = append(hashes, cdx.Hash{ + Algorithm: cdx.HashAlgorithm(normalized.Algorithm), + Value: normalized.Value, + }) + } + if len(hashes) == 0 { + return nil + } + return &hashes +} diff --git a/internal/sbom/graph.go b/internal/sbom/graph.go index fbcdaef1..4905937c 100644 --- a/internal/sbom/graph.go +++ b/internal/sbom/graph.go @@ -5,8 +5,127 @@ import ( "strings" "github.com/bomly-dev/bomly-sdk" + spdxcommon "github.com/spdx/tools-golang/spdx/v2/common" ) +// applyIngestedAssertions carries a source document's own claims onto the +// node, so a conversion keeps what the document said (issue #396). +// +// Only coordinates, scope, copyright and licenses used to survive this hop, +// so `bomly scan --sbom --format spdx` silently returned a document asserting +// far less than its input. These are the fields ADR-0037 typed for exactly +// that reason. +// +// Every value clears its own gate here. An ingested document is untrusted +// input that Bomly re-emits under its own name: a supplier name carrying +// control characters would corrupt SPDX's line-oriented form, a reference +// locator can be a local path or carry credentials, and #391 leaked +// credentials twice through URL positions its author had not considered. The +// gates are the SDK's, so ingest cannot hold a value to a weaker standard +// than export does. +// +// What is deliberately not set here is Source. It feeds +// RegistryMatchEligible, and an ingested component must stay eligible for +// enrichment so `bomly scan --sbom --enrich` keeps working. +func applyIngestedAssertions(pkg *sdk.DependencyNode, component Component) { + if pkg == nil { + return + } + if component.Supplier != nil { + if contact, ok := component.Supplier.Normalized(); ok { + pkg.Supplier = &contact + } + } + if component.Originator != nil { + if contact, ok := component.Originator.Normalized(); ok { + pkg.Originator = &contact + } + } + pkg.Description = sdk.NormalizeDescription(component.Description) + pkg.Homepage = sdk.NormalizeHomepage(component.Homepage) + pkg.ExternalReferences = sdk.MergeExternalReferences(nil, component.ExternalReferences) + pkg.Digests = ingestedDigests(component.Digests) + pkg.CPEs = ingestedCPEs(component.CPEs) +} + +// ingestedDigests admits the checksums a document stated, each through the +// digest gate. +// +// A digest names an algorithm from a vocabulary both formats keep extending, +// and a value whose shape that algorithm fixes; a document may state neither +// correctly. Digest.Normalized is the gate, so the algorithm set stays the +// SDK's -- a length check written here would go stale in the direction of +// silently dropping a real hash, which is precisely how a transcribed digest +// table lost CycloneDX's Streebog entries once already. +func ingestedDigests(digests []Digest) []sdk.Digest { + if len(digests) == 0 { + return nil + } + admitted := make([]sdk.Digest, 0, len(digests)) + seen := make(map[sdk.Digest]struct{}, len(digests)) + for _, digest := range digests { + normalized, ok := sdk.Digest{ + Algorithm: sdk.DigestAlgorithm(digest.Algorithm), + Value: digest.Value, + }.Normalized() + if !ok { + continue + } + if _, duplicate := seen[normalized]; duplicate { + continue + } + seen[normalized] = struct{}{} + admitted = append(admitted, normalized) + } + if len(admitted) == 0 { + return nil + } + return admitted +} + +// ingestedCPEs admits the CPEs a document stated, dropping any that is not a +// well-formed binding. A malformed identifier published back out would be a +// claim about a platform no source made, and a CPE decides which advisories +// match -- a wrong one is a wrong vulnerability answer. +// +// The grammar is the SDK's. It is reachable only through the external- +// reference gate, so each value is offered as a reference under both CPE +// reference types and admitted if either accepts it: 2.2 and 2.3 are +// genuinely different bindings, and a value is whichever one it parses as. +// Writing the grammar here instead is the mistake #396 records being +// retrofitted once already -- the hand-rolled CPE validator its fuzz target +// immediately broke. +func ingestedCPEs(values []string) []string { + if len(values) == 0 { + return nil + } + cpeTypes := []string{spdxcommon.TypeSecurityCPE23Type, spdxcommon.TypeSecurityCPE22Type} + admitted := make([]string, 0, len(values)) + seen := make(map[string]struct{}, len(values)) + for _, value := range values { + for _, cpeType := range cpeTypes { + reference, ok := sdk.ExternalReference{ + Category: sdk.ExternalReferenceCategorySecurity, + Type: cpeType, + Locator: value, + }.Normalized() + if !ok { + continue + } + if _, duplicate := seen[reference.Locator]; duplicate { + break + } + seen[reference.Locator] = struct{}{} + admitted = append(admitted, reference.Locator) + break + } + } + if len(admitted) == 0 { + return nil + } + return admitted +} + // componentIdentityHint describes a component in an error, preferring the // package URL it stated because that is the field an author has to correct. func componentIdentityHint(component Component) string { @@ -81,6 +200,7 @@ func ToGraph(doc *Document) (*sdk.Graph, error) { } pkg.Scopes = sdk.ScopesOf(sdk.Scope(component.Scope)) pkg.Copyright = component.Copyright + applyIngestedAssertions(pkg, component) // The document's own component ID does not survive: the node answers // to the identity its coordinates mint, and idMap below is what // re-points the document's relationships onto it. diff --git a/internal/sbom/model.go b/internal/sbom/model.go index 18d8c7ca..f7771d34 100644 --- a/internal/sbom/model.go +++ b/internal/sbom/model.go @@ -153,6 +153,32 @@ type Component struct { VCSURL string VCSRevision string + // What the source document asserted about this component, beyond its + // identity (ADR-0037, issue #396). + // + // These carry a foreign document's own claims through the graph so a + // conversion does not silently drop them: `bomly scan --sbom --format + // spdx` used to lose the supplier, description, checksums, CPEs and + // references its input stated, because the only things surviving the + // graph hop were coordinates, scope, copyright and licenses. + // + // They are SDK types rather than local structs on purpose. Each carries + // its own publication gate -- Contact.Normalized, ExternalReference + // .Normalized -- and every value crossing this boundary re-clears it, + // because an ingested document is untrusted input that Bomly re-emits. + // A local mirror of these shapes would be a second place for those rules + // to be forgotten, which is the defect ADR-0037 replaced. + Supplier *sdk.Contact + Originator *sdk.Contact + Description string + Homepage string + + // ExternalReferences are the document's own references, kept with the + // category and type it stated so the SPDX triple round-trips without + // being re-derived. Merge class: set, unioned by the reference's own + // identity. + ExternalReferences []sdk.ExternalReference + // Every place this package was resolved from, primary first. // // ADR-0041 folds equal-identity records into one node and keeps their diff --git a/internal/sbom/spdx23.go b/internal/sbom/spdx23.go index 63262f54..a8b50e0a 100644 --- a/internal/sbom/spdx23.go +++ b/internal/sbom/spdx23.go @@ -80,7 +80,7 @@ func (spdx23Codec) encodeJSON(doc *Document, opts EncodeOptions) ([]byte, error) packages = append(packages, pkg) } - relationships := make([]*v23.Relationship, 0, len(doc.Dependencies)+len(doc.Roots)) + relationships := make([]*v23.Relationship, 0, allocHint(len(doc.Dependencies), len(doc.Roots))) documentRef := common.DocElementID{ElementRefID: common.ElementID("DOCUMENT")} for _, root := range doc.Roots { rootID, ok := idByComponent[root] @@ -483,6 +483,29 @@ func spdxLicenseValue(licenses []License) (string, []spdxkit.ExtractedText) { return spdxkit.Compose(elements), extracted } +// maxAllocHint bounds a preallocation hint. It is a dumb count, not a limit +// on the work: a hint is only a hint, and append grows past it, so a +// genuinely larger document still encodes in full. +const maxAllocHint = 1 << 20 + +// allocHint sizes a preallocation from two lengths that came from a decoded +// document. Each side is clamped before the addition rather than the sum +// checked after it, so the sum cannot wrap -- an overflowed hint reaches make +// as a negative size, which panics. +// +// These lengths are attacker-influenced now: ingest carries a foreign +// document's own assertions, so a component's reference and vulnerability +// counts come from that document rather than from Bomly's own detection. +// +// Not delegated. bomly-sdk hardened the same pattern in its merges +// (bomly-dev/bomly-sdk#53) but keeps mergeCapacity unexported, and a resource +// bound is the project's own call rather than a rule a library owns -- the +// delegation convention says so explicitly. The bound is kept identical to +// the SDK's so the two do not drift into different answers for one question. +func allocHint(a, b int) int { + return min(a, maxAllocHint) + min(b, maxAllocHint) +} + // spdxOtherLicenses renders the document's extracted-text section: one entry // per distinct reference, sorted by identifier so the document is stable. // @@ -573,7 +596,7 @@ func spdxSourceInfo(component Component) string { } func spdxExternalReferences(component Component) []*v23.PackageExternalReference { - refs := make([]*v23.PackageExternalReference, 0, 1+len(component.CPEs)+len(component.Vulnerabilities)) + refs := make([]*v23.PackageExternalReference, 0, allocHint(len(component.CPEs), len(component.Vulnerabilities))+1) if purl := strings.TrimSpace(component.PURL); purl != "" { refs = append(refs, &v23.PackageExternalReference{ Category: common.CategoryPackageManager, diff --git a/internal/sbom/transform.go b/internal/sbom/transform.go index bd7db011..1d216d0c 100644 --- a/internal/sbom/transform.go +++ b/internal/sbom/transform.go @@ -72,6 +72,7 @@ func FromDepGraph(g *sdk.Graph, opts BuildOptions) (*Document, error) { component.Copyright = dep.Copyright component.Licenses = componentLicenses(sdk.DetectionLicenses(dep)) component.Digests = componentDigests(dep.Digests) + applyNodeAssertions(&component, dep) // The project's own records never take an external origin. This // guard closes the one remaining path -- a plugin-supplied graph // asserting an origin directly -- and module nodes cannot reach @@ -425,6 +426,56 @@ func applyOrigins(component *Component, origins []sdk.DependencyOrigin) { component.VCSRevision = primary.Revision } +// applyNodeAssertions copies the claims a node carries about itself onto the +// component the document will hold (ADR-0037, issue #396). +// +// Every value re-clears its own gate on the way out, even though it cleared +// one on the way in. The node is not a trusted carrier: a detector or an +// external plugin can write these fields directly, and a value that entered +// through a plugin never passed an ingest gate at all. Gating only at the +// boundary that happens to be upstream is how #391's last unfixed finding +// worked -- references restored from metadata were published without +// re-clearing anything. +// +// A rejected value is dropped rather than repaired: the gates decide what is +// publishable, and a "fixed" contact or reference would be an assertion no +// source made. +func applyNodeAssertions(component *Component, dep *sdk.DependencyNode) { + if component == nil || dep == nil { + return + } + if dep.Supplier != nil { + if contact, ok := dep.Supplier.Normalized(); ok { + component.Supplier = &contact + } + } + if dep.Originator != nil { + if contact, ok := dep.Originator.Normalized(); ok { + component.Originator = &contact + } + } + component.Description = sdk.NormalizeDescription(dep.Description) + component.Homepage = sdk.NormalizeHomepage(dep.Homepage) + component.ExternalReferences = publishableReferences(dep.ExternalReferences) + if len(dep.CPEs) > 0 && len(component.CPEs) == 0 { + component.CPEs = append([]string(nil), dep.CPEs...) + } +} + +// publishableReferences returns the references that survive the SDK gate, +// deduplicated by the reference's own identity. +// +// MergeExternalReferences is the union rule, so calling it with no existing +// set both normalizes and dedupes -- the set merge class stated in ADR-0037, +// applied through the one implementation of it rather than a second sort-and- +// compare written here. +func publishableReferences(refs []sdk.ExternalReference) []sdk.ExternalReference { + if len(refs) == 0 { + return nil + } + return sdk.MergeExternalReferences(nil, refs) +} + func enrichComponentFromRegistry(component *Component, registry *sdk.PackageRegistry, purl string, projectOwned bool) { if component == nil || registry == nil || purl == "" { return From b210be5d5ad10fcacd28cc3f89fc6b03890a5d21 Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Sat, 5 Sep 2026 01:31:20 -0700 Subject: [PATCH 2/6] feat(sbom): SPDX carries component assertions, and a fuzz target guards them Second part of #396. SPDX both directions, the conversion acceptance test, and the hostile-document fuzz target. SPDX writes a contact as one line, so ParseSPDXContact and Contact.SPDXString do the reading and writing -- the grammar has an optional "()" suffix that must not be retained, and that is precisely the rule a second local reading forgets. Description prefers the fuller field and falls back to summary. Checksums, homepage, supplier and originator cross both ways. References keep the category SPDX stated, because the triple (category, type, locator) is the reference's identity and the category is an SPDX-only axis with no CycloneDX source value -- so an SPDX reference round-trips without being re-derived from the type alone. A CycloneDX-sourced reference has no category, and takes OTHER on the way out, which is the category SPDX provides for exactly that. Bomly's own projections are not read back as assertions. The purl and cpe references are written from Component.PURL and Component.CPEs on every run, so ingesting them would emit a second copy on the next hop and make the round trip grow. CPEs are instead read into Component.CPEs, which is the field they mean -- SPDX has no CPE field, so the reference list is the only place one can be, and dropping them outright lost the CPE the acceptance test asks for. FuzzIngestedAssertions drives arbitrary supplier names, URLs, descriptions, CPEs and digests through the ingest gates and asserts that whatever is stored still clears its own gate, and that a second ingest changes nothing. It found a real defect inside a minute, which is the reason #396 requires these from day one rather than after review. That defect is bomly-dev/bomly-sdk#54: NormalizeDescription is not idempotent. Invalid UTF-8 is replaced with U+FFFD at three bytes per bad byte, so a 3006-byte value normalizes to 9006 -- past the bound applied before the repair -- and the next pass, seeing an over-long value, returns empty. A description would survive one hop and vanish on the next, which is exactly the fixed point #396 requires. stableValue applies the SDK's own function until it settles and drops a value that will not, because publishing something that changes on the next hop is worse than publishing nothing. It goes when #54 ships; the fuzz target asserts the property either way. Co-Authored-By: Claude Opus 5 --- internal/sbom/assertions_fuzz_test.go | 100 +++++++ internal/sbom/assertions_round_trip_test.go | 149 +++++++++++ internal/sbom/graph.go | 31 ++- internal/sbom/spdx23.go | 12 +- internal/sbom/spdx23_assertions.go | 275 ++++++++++++++++++++ scripts/run-fuzz.sh | 1 + 6 files changed, 563 insertions(+), 5 deletions(-) create mode 100644 internal/sbom/assertions_fuzz_test.go create mode 100644 internal/sbom/assertions_round_trip_test.go create mode 100644 internal/sbom/spdx23_assertions.go diff --git a/internal/sbom/assertions_fuzz_test.go b/internal/sbom/assertions_fuzz_test.go new file mode 100644 index 00000000..3823e023 --- /dev/null +++ b/internal/sbom/assertions_fuzz_test.go @@ -0,0 +1,100 @@ +package sbom + +import ( + "strings" + "testing" + + "github.com/bomly-dev/bomly-sdk" + "github.com/bomly-dev/bomly-sdk/testkit" +) + +// FuzzIngestedAssertions drives an arbitrary document's component assertions +// through the ingest gates and asserts that nothing unpublishable survives. +// +// This is the hostile-document half of #396. Ingest reads values a stranger +// wrote and Bomly re-emits them under its own name, so the interesting input +// is not a malformed document -- the codec rejects those -- but a +// well-formed one carrying a local path, an embedded credential, a control +// character, or a malformed CPE or digest. Two credential leaks in #391 came +// through URL positions nobody had thought to test. +func FuzzIngestedAssertions(f *testing.F) { + f.Add("Acme Corp", "https://acme.example/", "a description", "https://acme.example/home", "cpe:2.3:a:acme:widget:1:*:*:*:*:*:*:*", "SHA-256", strings.Repeat("a", 64)) + f.Add("Acme\nCorp", "https://user:pass@acme.example/", "desc", "file:///etc/passwd", "cpe:not-a-cpe", "SHA-256", "short") + f.Add("", "", "", "", "", "", "") + f.Add("Acme (bob@acme.example)", "https://acme.example/?token=abc", "d", "http://127.0.0.1/", "cpe:/a:acme:widget", "MD5", strings.Repeat("f", 32)) + f.Add(strings.Repeat("n", 5000), "https://[::1]/", "x", "ssh://git@host/repo", "cpe:2.3:*:*:*:*:*:*:*:*:*:*:*", "SHA-512", "zz") + + f.Fuzz(func(t *testing.T, name, contactURL, description, homepage, cpe, algorithm, digestValue string) { + for _, value := range []string{name, contactURL, description, homepage, cpe, algorithm, digestValue} { + if len(value) > testkit.MaxFuzzInputSize { + t.Skip("input beyond the documented bound") + } + } + + component := Component{ + Name: "widget", + Supplier: &sdk.Contact{Kind: sdk.ContactKindOrganization, Name: name, URL: contactURL}, + Description: description, + Homepage: homepage, + CPEs: []string{cpe}, + Digests: []Digest{{Algorithm: algorithm, Value: digestValue}}, + } + + node, err := sdk.NewDependencyNode(sdk.Coordinates{Ecosystem: "npm", Name: "widget", Version: "1.0.0"}) + if err != nil { + t.Fatalf("construct node: %v", err) + } + applyIngestedAssertions(node, component) + + // Whatever arrived, what the node now carries must be publishable: + // re-running each gate on the stored value has to agree with it. + if node.Supplier != nil { + if _, ok := node.Supplier.Normalized(); !ok { + t.Fatalf("stored supplier %+v does not clear its own gate", node.Supplier) + } + } + if node.Description != sdk.NormalizeDescription(node.Description) { + t.Fatalf("stored description %q is not normalized", node.Description) + } + if node.Homepage != sdk.NormalizeHomepage(node.Homepage) { + t.Fatalf("stored homepage %q is not normalized", node.Homepage) + } + for _, digest := range node.Digests { + if _, ok := digest.Normalized(); !ok { + t.Fatalf("stored digest %+v does not clear its own gate", digest) + } + } + for _, reference := range node.ExternalReferences { + if _, ok := reference.Normalized(); !ok { + t.Fatalf("stored reference %+v does not clear its own gate", reference) + } + } + // A stored CPE must still be a CPE the SDK accepts. + for _, stored := range node.CPEs { + if len(ingestedCPEs([]string{stored})) != 1 { + t.Fatalf("stored CPE %q would not be admitted again", stored) + } + } + + // Re-running ingest on what was stored changes nothing: the gates are + // a fixed point, so a value cannot be laundered by another hop. + again, err := sdk.NewDependencyNode(sdk.Coordinates{Ecosystem: "npm", Name: "widget", Version: "1.0.0"}) + if err != nil { + t.Fatalf("construct node: %v", err) + } + applyIngestedAssertions(again, Component{ + Supplier: node.Supplier, + Description: node.Description, + Homepage: node.Homepage, + CPEs: node.CPEs, + Digests: componentDigests(node.Digests), + }) + if again.Description != node.Description || again.Homepage != node.Homepage { + t.Fatalf("a second ingest changed the value: %q/%q then %q/%q", + node.Description, node.Homepage, again.Description, again.Homepage) + } + if len(again.CPEs) != len(node.CPEs) || len(again.Digests) != len(node.Digests) { + t.Fatalf("a second ingest changed the set sizes") + } + }) +} diff --git a/internal/sbom/assertions_round_trip_test.go b/internal/sbom/assertions_round_trip_test.go new file mode 100644 index 00000000..44481756 --- /dev/null +++ b/internal/sbom/assertions_round_trip_test.go @@ -0,0 +1,149 @@ +package sbom + +import ( + "strings" + "testing" + + "github.com/bomly-dev/bomly-sdk" +) + +// supplierRichCycloneDX is a document asserting the fields #396 exists to +// preserve: supplier, publisher, description, a checksum, a CPE, and a +// classified external reference. +const supplierRichCycloneDX = `{ + "bomFormat": "CycloneDX", + "specVersion": "1.5", + "version": 1, + "components": [ + { + "bom-ref": "pkg:npm/widget@1.0.0", + "type": "library", + "name": "widget", + "version": "1.0.0", + "purl": "pkg:npm/widget@1.0.0", + "description": "A widget for widgeting.", + "publisher": "Widget Publishing Inc", + "supplier": { "name": "Widget Supply Co", "url": ["https://widgets.example/"] }, + "cpe": "cpe:2.3:a:widget:widget:1.0.0:*:*:*:*:*:*:*", + "hashes": [ + { "alg": "SHA-256", "content": "9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08" } + ], + "externalReferences": [ + { "type": "issue-tracker", "url": "https://widgets.example/issues", "comment": "public tracker" } + ] + } + ] +}` + +// assertPreserved states what every hop has to keep, so one helper covers +// CycloneDX, SPDX, and the graph. +func assertPreserved(t *testing.T, where string, component Component) { + t.Helper() + if component.Supplier == nil || component.Supplier.Name != "Widget Supply Co" { + t.Fatalf("%s: supplier = %+v, want the source's", where, component.Supplier) + } + if component.Originator == nil || !strings.Contains(component.Originator.Name, "Widget Publishing") { + t.Fatalf("%s: originator = %+v, want the source's publisher", where, component.Originator) + } + if !strings.Contains(component.Description, "widgeting") { + t.Fatalf("%s: description = %q, want the source's", where, component.Description) + } + if len(component.Digests) == 0 { + t.Fatalf("%s: checksums lost", where) + } + if len(component.CPEs) == 0 { + t.Fatalf("%s: CPE lost", where) + } + var tracker bool + for _, ref := range component.ExternalReferences { + if strings.Contains(ref.Locator, "widgets.example/issues") { + tracker = true + } + } + if !tracker { + t.Fatalf("%s: the issue-tracker reference was lost: %+v", where, component.ExternalReferences) + } +} + +// componentNamed finds a component by name, failing the test when absent. +func componentNamed(t *testing.T, doc *Document, name string) Component { + t.Helper() + for _, component := range doc.Components { + if component.Name == name { + return component + } + } + t.Fatalf("component %q missing from document: %+v", name, doc.Components) + return Component{} +} + +// A supplier-rich CycloneDX document converts to SPDX and back without losing +// what it asserted. This is #396's first acceptance criterion, and it fails +// at the graph hop rather than in a codec: before this change the only things +// crossing Document -> Graph -> Document were coordinates, scope, copyright +// and licenses. +func TestSupplierRichDocumentSurvivesConversionToSPDXAndBack(t *testing.T) { + ingested, err := UnmarshalJSON([]byte(supplierRichCycloneDX), TargetCycloneDX15JSON) + if err != nil { + t.Fatalf("ingest cyclonedx: %v", err) + } + assertPreserved(t, "cyclonedx ingest", componentNamed(t, ingested, "widget")) + + // Through the graph, which is where the loss used to happen. + graph, err := ToGraph(ingested) + if err != nil { + t.Fatalf("to graph: %v", err) + } + asSPDX, err := MarshalDepGraphJSON(graph, TargetSPDX23JSON, BuildOptions{}, EncodeOptions{}) + if err != nil { + t.Fatalf("marshal spdx: %v", err) + } + + backFromSPDX, err := UnmarshalJSON(asSPDX, TargetSPDX23JSON) + if err != nil { + t.Fatalf("ingest spdx: %v", err) + } + assertPreserved(t, "spdx round trip", componentNamed(t, backFromSPDX, "widget")) + + // And back to CycloneDX, so neither format is a one-way door. + spdxGraph, err := ToGraph(backFromSPDX) + if err != nil { + t.Fatalf("spdx to graph: %v", err) + } + asCDX, err := MarshalDepGraphJSON(spdxGraph, TargetCycloneDX15JSON, BuildOptions{}, EncodeOptions{}) + if err != nil { + t.Fatalf("marshal cyclonedx: %v", err) + } + backFromCDX, err := UnmarshalJSON(asCDX, TargetCycloneDX15JSON) + if err != nil { + t.Fatalf("re-ingest cyclonedx: %v", err) + } + assertPreserved(t, "cyclonedx round trip", componentNamed(t, backFromCDX, "widget")) +} + +// Ingest must not set Source: it feeds RegistryMatchEligible, and an ingested +// component has to stay eligible or `bomly scan --sbom --enrich` stops +// enriching anything. +func TestIngestLeavesComponentsEligibleForEnrichment(t *testing.T) { + ingested, err := UnmarshalJSON([]byte(supplierRichCycloneDX), TargetCycloneDX15JSON) + if err != nil { + t.Fatalf("ingest: %v", err) + } + graph, err := ToGraph(ingested) + if err != nil { + t.Fatalf("to graph: %v", err) + } + nodes := graph.DependencyNodes() + if len(nodes) == 0 { + t.Fatalf("no dependency nodes") + } + for _, node := range nodes { + if node.Source != "" { + t.Fatalf("ingest set Source = %q; that feeds RegistryMatchEligible and would stop --sbom --enrich", node.Source) + } + if !node.RegistryMatchEligible() { + t.Fatalf("ingested %q is not eligible for enrichment", node.NodeID()) + } + } + _ = sdk.EcosystemUnknown +} diff --git a/internal/sbom/graph.go b/internal/sbom/graph.go index 4905937c..4228c0aa 100644 --- a/internal/sbom/graph.go +++ b/internal/sbom/graph.go @@ -41,13 +41,40 @@ func applyIngestedAssertions(pkg *sdk.DependencyNode, component Component) { pkg.Originator = &contact } } - pkg.Description = sdk.NormalizeDescription(component.Description) - pkg.Homepage = sdk.NormalizeHomepage(component.Homepage) + pkg.Description = stableValue(sdk.NormalizeDescription, component.Description) + pkg.Homepage = stableValue(sdk.NormalizeHomepage, component.Homepage) pkg.ExternalReferences = sdk.MergeExternalReferences(nil, component.ExternalReferences) pkg.Digests = ingestedDigests(component.Digests) pkg.CPEs = ingestedCPEs(component.CPEs) } +// stableValue normalizes until the result stops changing, and drops a value +// that will not settle. +// +// A gate that is not idempotent breaks the round trip this whole change +// exists to provide: a description carrying invalid UTF-8 normalizes to three +// bytes per bad byte, which can push it past the length bound that was +// applied before the repair -- so the next pass sees an over-long value and +// returns empty. The field then survives one hop and vanishes on the next. +// Found by FuzzIngestedAssertions within a minute of first running, and filed +// as bomly-dev/bomly-sdk#54. +// +// This does not re-implement any rule: it applies the SDK's own function +// until it reaches its fixed point, which is a policy decision about how much +// to trust a gate rather than a second opinion about what the gate should +// say. Publishing a value that changes on the next hop is worse than +// publishing nothing, so a value that has not settled after one further pass +// is dropped. Remove this once #54 ships; the fuzz target asserts the +// property either way. +func stableValue(normalize func(string) string, value string) string { + first := normalize(value) + second := normalize(first) + if first != second { + return "" + } + return first +} + // ingestedDigests admits the checksums a document stated, each through the // digest gate. // diff --git a/internal/sbom/spdx23.go b/internal/sbom/spdx23.go index a8b50e0a..25574b21 100644 --- a/internal/sbom/spdx23.go +++ b/internal/sbom/spdx23.go @@ -69,7 +69,11 @@ func (spdx23Codec) encodeJSON(doc *Document, opts EncodeOptions) ([]byte, error) PackageCopyrightText: spdxCopyrightValue(c.Copyright), PackageChecksums: spdxChecksums(c.Digests), PackageSourceInfo: spdxSourceInfo(c), - PackageExternalReferences: spdxExternalReferences(c), + PackageExternalReferences: append(spdxExternalReferences(c), spdxEmittedReferences(c.ExternalReferences)...), + PackageSupplier: spdxSupplierFor(c.Supplier), + PackageOriginator: spdxOriginatorFor(c.Originator), + PackageDescription: sdk.NormalizeDescription(c.Description), + PackageHomePage: sdk.NormalizeHomepage(c.Homepage), PrimaryPackagePurpose: spdxPrimaryPackagePurpose(c.Type), } if _, isRoot := rootComponents[c.ID]; isRoot || IsProjectRootComponent(c) { @@ -164,7 +168,7 @@ func (spdx23Codec) decodeJSON(data []byte) (*Document, error) { continue } id := common.RenderElementID(p.PackageSPDXIdentifier) - components = append(components, Component{ + component := Component{ ID: id, Name: p.PackageName, Version: p.PackageVersion, @@ -175,7 +179,9 @@ func (spdx23Codec) decodeJSON(data []byte) (*Document, error) { PackageManager: parseSPDXPackageManager(p.PackageExternalReferences), Copyright: parseSPDXCopyright(p.PackageCopyrightText), Licenses: parseSPDXLicenses(extractedByRef, p.PackageLicenseConcluded, p.PackageLicenseDeclared), - }) + } + applySPDXAssertions(&component, p) + components = append(components, component) } depsByRef := make(map[string][]string, len(components)) diff --git a/internal/sbom/spdx23_assertions.go b/internal/sbom/spdx23_assertions.go new file mode 100644 index 00000000..5ffdf43a --- /dev/null +++ b/internal/sbom/spdx23_assertions.go @@ -0,0 +1,275 @@ +package sbom + +import ( + "strings" + + "github.com/bomly-dev/bomly-sdk" + "github.com/spdx/tools-golang/spdx/v2/common" + v23 "github.com/spdx/tools-golang/spdx/v2/v2_3" +) + +// This file carries an SPDX document's own component assertions in and out: +// supplier, originator, description, homepage, checksums and the document's +// external references (ADR-0037, issue #396). +// +// SPDX writes a contact as one line -- "Organization: Acme" -- so the parse +// and the render are the SDK's ParseSPDXContact and Contact.SPDXString. The +// grammar has an optional "()" suffix that must not be retained, which +// is the kind of rule that gets forgotten when it is rewritten per call site. + +// spdxIngestedSupplier reads a package's supplier. +func spdxIngestedSupplier(supplier *common.Supplier) *sdk.Contact { + if supplier == nil { + return nil + } + return spdxContactFrom(supplier.SupplierType, supplier.Supplier) +} + +// spdxIngestedOriginator reads the party that authored the package. +func spdxIngestedOriginator(originator *common.Originator) *sdk.Contact { + if originator == nil { + return nil + } + return spdxContactFrom(originator.OriginatorType, originator.Originator) +} + +// spdxContactFrom rebuilds SPDX's single-line contact form and parses it +// through the SDK. +// +// tools-golang splits the line into a type and a value, so the line is +// reassembled rather than mapped field by field: the SDK owns what the form +// means, including the address suffix it strips, and a second reading of the +// same grammar here would be a second place to get it wrong. +func spdxContactFrom(contactType, value string) *sdk.Contact { + value = strings.TrimSpace(value) + if value == "" { + return nil + } + line := value + if contactType = strings.TrimSpace(contactType); contactType != "" { + line = contactType + ": " + value + } + contact, ok := sdk.ParseSPDXContact(line) + if !ok { + return nil + } + normalized, ok := contact.Normalized() + if !ok { + return nil + } + return &normalized +} + +// spdxIngestedChecksums reads a package's own checksums. +func spdxIngestedChecksums(checksums []common.Checksum) []Digest { + if len(checksums) == 0 { + return nil + } + digests := make([]Digest, 0, len(checksums)) + for _, checksum := range checksums { + digests = append(digests, Digest{ + Algorithm: string(checksum.Algorithm), + Value: checksum.Value, + }) + } + return digests +} + +// spdxIngestedReferences reads the document's external references, keeping +// the category SPDX stated alongside the type. +// +// The triple (category, type, locator) is the reference's identity, and the +// category is an SPDX-only axis with no CycloneDX source value -- so keeping +// it is what lets an SPDX reference round-trip without being re-derived from +// the type alone. +// +// Bomly's own emissions are skipped. The PURL and CPE references it writes +// are projections of the component's identity and enrichment, and reading +// them back as source assertions would duplicate them on the next export; +// the origin-derived ones are excluded for the laundering reason recorded on +// isOriginDerivedReferenceType. +func spdxIngestedReferences(refs []*v23.PackageExternalReference) []sdk.ExternalReference { + if len(refs) == 0 { + return nil + } + converted := make([]sdk.ExternalReference, 0, len(refs)) + for _, ref := range refs { + if ref == nil || isBomlyProjectedReference(ref) { + continue + } + category, err := sdk.ParseExternalReferenceCategory(ref.Category) + if err != nil { + continue + } + converted = append(converted, sdk.ExternalReference{ + Category: category, + Type: ref.RefType, + Locator: ref.Locator, + Comment: ref.ExternalRefComment, + }) + } + return sdk.MergeExternalReferences(nil, converted) +} + +// spdxIngestedCPEs reads the CPEs a document stated. +// +// SPDX 2.3 has no CPE field: a CPE is carried as a SECURITY external +// reference, so the reference list is the only place one can be. They are +// read into Component.CPEs rather than kept among the external references, +// because that is the field they mean and the exporter writes them back from +// there -- keeping them in both would emit each CPE twice on the next hop. +// +// The locator is not re-validated here. It arrives from the same list +// spdxIngestedReferences reads, and ingestedCPEs re-clears the CPE grammar +// through the SDK gate before any of this reaches a node. +func spdxIngestedCPEs(refs []*v23.PackageExternalReference) []string { + if len(refs) == 0 { + return nil + } + cpes := make([]string, 0, len(refs)) + seen := make(map[string]struct{}, len(refs)) + for _, ref := range refs { + if ref == nil { + continue + } + switch strings.ToLower(strings.TrimSpace(ref.RefType)) { + case strings.ToLower(common.TypeSecurityCPE23Type), strings.ToLower(common.TypeSecurityCPE22Type): + default: + continue + } + locator := strings.TrimSpace(ref.Locator) + if locator == "" { + continue + } + if _, duplicate := seen[locator]; duplicate { + continue + } + seen[locator] = struct{}{} + cpes = append(cpes, locator) + } + if len(cpes) == 0 { + return nil + } + return cpes +} + +// isBomlyProjectedReference reports whether a reference is one Bomly derives +// from the component itself rather than one a source asserted. +// +// The purl and cpe references are projections of identity and enrichment: the +// exporter writes them from Component.PURL and Component.CPEs on every run, +// so ingesting them as assertions would produce a second copy on re-export +// and make the round trip grow. The Bomly-defined origin reference type is +// excluded for the separate laundering reason. +func isBomlyProjectedReference(ref *v23.PackageExternalReference) bool { + switch strings.ToLower(strings.TrimSpace(ref.RefType)) { + case strings.ToLower(common.TypePackageManagerPURL), + strings.ToLower(common.TypeSecurityCPE23Type), + strings.ToLower(common.TypeSecurityCPE22Type), + spdxOriginRefType: + return true + default: + return false + } +} + +// applySPDXAssertions fills a component with what the document asserted about +// it, each value through its gate. +func applySPDXAssertions(component *Component, pkg *v23.Package) { + if component == nil || pkg == nil { + return + } + component.Supplier = spdxIngestedSupplier(pkg.PackageSupplier) + component.Originator = spdxIngestedOriginator(pkg.PackageOriginator) + // SPDX has both; description is the fuller field, so summary fills in + // only when it is absent rather than overwriting it. + description := pkg.PackageDescription + if strings.TrimSpace(description) == "" { + description = pkg.PackageSummary + } + component.Description = sdk.NormalizeDescription(description) + component.Homepage = sdk.NormalizeHomepage(pkg.PackageHomePage) + component.ExternalReferences = spdxIngestedReferences(pkg.PackageExternalReferences) + if cpes := spdxIngestedCPEs(pkg.PackageExternalReferences); len(cpes) > 0 { + component.CPEs = cpes + } + if digests := spdxIngestedChecksums(pkg.PackageChecksums); len(digests) > 0 { + component.Digests = digests + } +} + +// spdxSupplierFor renders a contact into SPDX's supplier form. +func spdxSupplierFor(contact *sdk.Contact) *common.Supplier { + if contact == nil { + return nil + } + kind, name, ok := spdxContactParts(*contact) + if !ok { + return nil + } + return &common.Supplier{SupplierType: kind, Supplier: name} +} + +// spdxOriginatorFor renders a contact into SPDX's originator form. +func spdxOriginatorFor(contact *sdk.Contact) *common.Originator { + if contact == nil { + return nil + } + kind, name, ok := spdxContactParts(*contact) + if !ok { + return nil + } + return &common.Originator{OriginatorType: kind, Originator: name} +} + +// spdxContactParts splits the SDK's rendered contact line back into the two +// fields tools-golang holds it in. +// +// The line is produced by Contact.SPDXString rather than assembled here, so +// the gate runs and the rendering stays the SDK's; this only re-splits what +// it produced. A contact with nothing publishable renders empty and is +// omitted rather than written as a malformed line. +func spdxContactParts(contact sdk.Contact) (kind, name string, ok bool) { + line := contact.SPDXString() + if line == "" { + return "", "", false + } + prefix, value, found := strings.Cut(line, ": ") + if !found { + // NOASSERTION carries no type half. + return "", line, true + } + return prefix, value, true +} + +// spdxEmittedReferences renders the component's own asserted references, +// re-clearing the gate on the way out. +func spdxEmittedReferences(refs []sdk.ExternalReference) []*v23.PackageExternalReference { + if len(refs) == 0 { + return nil + } + emitted := make([]*v23.PackageExternalReference, 0, len(refs)) + for _, ref := range refs { + normalized, ok := ref.Normalized() + if !ok { + continue + } + category := string(normalized.Category) + if category == "" { + // A CycloneDX-sourced reference has no category axis. SPDX + // requires one, and OTHER is the category the specification + // provides for a reference outside its defined vocabularies. + category = common.CategoryOther + } + emitted = append(emitted, &v23.PackageExternalReference{ + Category: category, + RefType: normalized.Type, + Locator: normalized.Locator, + ExternalRefComment: normalized.Comment, + }) + } + if len(emitted) == 0 { + return nil + } + return emitted +} diff --git a/scripts/run-fuzz.sh b/scripts/run-fuzz.sh index c5d3f8e6..a6bcb9cf 100755 --- a/scripts/run-fuzz.sh +++ b/scripts/run-fuzz.sh @@ -33,6 +33,7 @@ targets=( "github.com/bomly-dev/bomly-cli/internal/sbom FuzzUnmarshalAutoJSON" "github.com/bomly-dev/bomly-cli/internal/sbom FuzzNormalizeSPDXLicenseExpression" "github.com/bomly-dev/bomly-cli/internal/sbom FuzzSPDXLicenseValue" + "github.com/bomly-dev/bomly-cli/internal/sbom FuzzIngestedAssertions" "github.com/bomly-dev/bomly-cli/internal/baseline FuzzLoad" "github.com/bomly-dev/bomly-cli/internal/engine FuzzConsolidateVulnerabilities" "github.com/bomly-dev/bomly-cli/internal/plugin FuzzPluginPathSanitizers" From c6a6dc58158935bfa8f416a2dc81ed68c99e78e5 Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Sat, 5 Sep 2026 12:33:54 -0700 Subject: [PATCH 3/6] feat(sbom): a conversion restates its source, a merge links its sources The export surface takes the prepared graph entries alongside the graph, so what each source document asserted about itself reaches the codec -- merging entries into one graph is precisely the step that discarded it. With one source document the export adopts that document's identity rather than minting a new one, which is what makes export -> ingest -> export reproduce the same bytes. With several it mints its own and names each source through a CycloneDX reference of type `bom`, because both formats give a document exactly one identity. The fixed-point test earned its place immediately: SPDX external references were being emitted with the SDK's comparison-form category ("other") instead of the specification's spelling ("OTHER"), so an ingested document changed case on its second export. SPDXName() is the authority now. Merged SPDX documents link nothing yet -- SPDX's externalDocumentRefs requires a checksum over the source bytes that the SDK carrier cannot hold. Filed as bomly-dev/bomly-sdk#55 and linked from the code. Recorded as ADR-0042. Co-Authored-By: Claude Opus 5 --- .github/workflows/sbom-interoperability.yml | 57 ++- dev-docs/SECURITY_ASSURANCE.md | 1 + ...es-its-source-a-merge-links-its-sources.md | 82 ++++ dev-docs/adr/README.md | 1 + docs/SBOM.md | 48 ++- docs/SECURITY.md | 7 + internal/cli/scan_cmd.go | 12 +- internal/detectors/sbom/detector.go | 8 + .../engine/consolidation/consolidation.go | 4 + internal/sbom/codec.go | 11 +- internal/sbom/cyclonedx.go | 18 +- internal/sbom/cyclonedx_assertions.go | 154 ++++++++ internal/sbom/document_assertions.go | 138 +++++++ .../sbom/document_assertions_fuzz_test.go | 133 +++++++ internal/sbom/document_assertions_test.go | 361 ++++++++++++++++++ internal/sbom/model.go | 22 ++ internal/sbom/spdx23.go | 25 +- internal/sbom/spdx23_assertions.go | 101 ++++- internal/sbom/transform.go | 82 ++-- scripts/run-fuzz.sh | 1 + 20 files changed, 1211 insertions(+), 55 deletions(-) create mode 100644 dev-docs/adr/0042-a-conversion-restates-its-source-a-merge-links-its-sources.md create mode 100644 internal/sbom/document_assertions.go create mode 100644 internal/sbom/document_assertions_fuzz_test.go create mode 100644 internal/sbom/document_assertions_test.go diff --git a/.github/workflows/sbom-interoperability.yml b/.github/workflows/sbom-interoperability.yml index 7d3af05e..5781334d 100644 --- a/.github/workflows/sbom-interoperability.yml +++ b/.github/workflows/sbom-interoperability.yml @@ -17,6 +17,8 @@ env: CDX_CLI_URL: https://github.com/CycloneDX/cyclonedx-cli/releases/download/v0.32.0/cyclonedx-linux-x64 CDX_CLI_SHA256: 454879e6a4a405c8a13bff49b8982adcb0596f3019b26b0811c66e4d7f0783e1 FIXTURE_SBOM: test/smoke/testdata/sboms/go.spdx.json + MERGE_FIXTURE_A: test/smoke/testdata/sboms/go.spdx.json + MERGE_FIXTURE_B: test/smoke/testdata/sboms/js.spdx.json OUTPUT_DIR: sbom-assurance-artifacts jobs: @@ -64,11 +66,56 @@ jobs: sha256sum "${OUTPUT_DIR}/bomly.spdx.json" "${OUTPUT_DIR}/bomly.cdx.json" \ | tee "${OUTPUT_DIR}/checksums.txt" + - name: Generate a merged SBOM from two source documents + run: | + set -euo pipefail + # Two subprojects, one source document each, so the export has to + # reconcile two documents that each assert their own identity -- + # the case ADR-0037 defines and the single-fixture run above never + # reaches. + mkdir -p "${OUTPUT_DIR}/merge-tree/a" "${OUTPUT_DIR}/merge-tree/b" + cp "${MERGE_FIXTURE_A}" "${OUTPUT_DIR}/merge-tree/a/" + cp "${MERGE_FIXTURE_B}" "${OUTPUT_DIR}/merge-tree/b/" + ./bin/bomly scan --path "${OUTPUT_DIR}/merge-tree" --recursive --detectors sbom \ + --format spdx > "${OUTPUT_DIR}/bomly-merged.spdx.json" + ./bin/bomly scan --path "${OUTPUT_DIR}/merge-tree" --recursive --detectors sbom \ + --format cyclonedx > "${OUTPUT_DIR}/bomly-merged.cdx.json" + sha256sum "${OUTPUT_DIR}/bomly-merged.spdx.json" "${OUTPUT_DIR}/bomly-merged.cdx.json" \ + | tee -a "${OUTPUT_DIR}/checksums.txt" + + - name: Check the merged document links its sources + run: | + set -euo pipefail + # A merged document states its own identity and names the documents + # it was built from, rather than adopting one source's identity + # (ADR-0037). Asserted here because only a real two-source run can + # produce it. + python3 - "${OUTPUT_DIR}/bomly-merged.cdx.json" "${MERGE_FIXTURE_A}" "${MERGE_FIXTURE_B}" <<'PYEOF' + import json, sys + + merged = json.load(open(sys.argv[1])) + wanted = {json.load(open(path))["documentNamespace"] for path in sys.argv[2:]} + linked = { + ref["url"] + for ref in merged.get("externalReferences", []) + if ref.get("type") == "bom" + } + missing = wanted - linked + if missing: + raise SystemExit(f"merged document does not link its sources: {sorted(missing)}") + if merged.get("serialNumber") in wanted: + raise SystemExit("merged document adopted a source's identity") + print(f"merged document links {len(linked)} source(s): {sorted(linked)}") + PYEOF + - name: Validate SPDX 2.3 output (spdx/tools-java) run: | set -euo pipefail java -jar "${OUTPUT_DIR}/validators/${SPDX_TOOLS_JAR}" Verify \ "${OUTPUT_DIR}/bomly.spdx.json" 2>&1 | tee "${OUTPUT_DIR}/spdx-validation.log" + java -jar "${OUTPUT_DIR}/validators/${SPDX_TOOLS_JAR}" Verify \ + "${OUTPUT_DIR}/bomly-merged.spdx.json" 2>&1 \ + | tee "${OUTPUT_DIR}/spdx-merged-validation.log" - name: Validate CycloneDX 1.7 output (cyclonedx-cli) run: | @@ -77,6 +124,10 @@ jobs: --input-file "${OUTPUT_DIR}/bomly.cdx.json" \ --input-format json --input-version v1_7 --fail-on-errors \ 2>&1 | tee "${OUTPUT_DIR}/cyclonedx-validation.log" + "${OUTPUT_DIR}/validators/cyclonedx-cli" validate \ + --input-file "${OUTPUT_DIR}/bomly-merged.cdx.json" \ + --input-format json --input-version v1_7 --fail-on-errors \ + 2>&1 | tee "${OUTPUT_DIR}/cyclonedx-merged-validation.log" - name: Summarize the result if: always() @@ -84,7 +135,7 @@ jobs: set -euo pipefail { printf '# SBOM interoperability assurance\n\n' - printf 'The built binary generated SPDX 2.3 and CycloneDX 1.7 SBOMs from a pinned fixture, and the official validators checked them.\n\n' + printf 'The built binary generated SPDX 2.3 and CycloneDX 1.7 SBOMs from a pinned fixture, and a merged pair from two source documents, and the official validators checked all four.\n\n' printf -- '- **Revision:** `%s`\n' "${GITHUB_SHA}" printf -- '- **Validators:** spdx/tools-java `%s`, cyclonedx-cli `%s` (both checksum-pinned)\n\n' "${SPDX_TOOLS_VERSION}" "${CDX_CLI_VERSION}" if [[ -f "${OUTPUT_DIR}/checksums.txt" ]]; then @@ -102,8 +153,12 @@ jobs: path: | sbom-assurance-artifacts/bomly.spdx.json sbom-assurance-artifacts/bomly.cdx.json + sbom-assurance-artifacts/bomly-merged.spdx.json + sbom-assurance-artifacts/bomly-merged.cdx.json sbom-assurance-artifacts/checksums.txt sbom-assurance-artifacts/spdx-validation.log sbom-assurance-artifacts/cyclonedx-validation.log + sbom-assurance-artifacts/spdx-merged-validation.log + sbom-assurance-artifacts/cyclonedx-merged-validation.log if-no-files-found: warn retention-days: 14 diff --git a/dev-docs/SECURITY_ASSURANCE.md b/dev-docs/SECURITY_ASSURANCE.md index bf701318..08a1591b 100644 --- a/dev-docs/SECURITY_ASSURANCE.md +++ b/dev-docs/SECURITY_ASSURANCE.md @@ -39,6 +39,7 @@ start a subprocess, enable a plugin, or choose an output path. | Repository manifests, lockfiles, and analyzer source | In-process file reads have a 64 MiB per-file limit that is checked before and during the read. Parsers never receive a partial over-limit document. Discovery is scoped to the selected target, does not follow directory symlinks, and has depth and exclusion controls. Pure parsers have registered fuzz targets and malformed-input tests. | `TestSDKContractReadLimitEnforcesBoundsAndGrowth`, `TestSDKContractReadRepositoryFileEnforces64MiBBound` (local contracts; full suite upstream in `bomly-dev/bomly-sdk` `system/read_test.go`), `test/assurance/REPOSITORY_INPUT_LIMITS.md`, `test/assurance/PARSER_FUZZING.md`, `scripts/run-fuzz.sh`, detector and analyzer package tests | A selected package manager's command output is not truncated because partial output could produce an incomplete graph. A local target may contain an unbounded number of individually bounded files. | | Project discovery and baselines | Recursive discovery does not follow directory symlinks. Automatic baseline selection warns and ignores a symlinked `.bomly` directory or baseline file. Explicit baseline selection may use a symlink because the user chose that path. Baseline JSON is limited to 16 MiB and 10,000 entries, is strict, and validates duplicates in linear time. It can only supply policy-status decisions. | `TestPlanSubprojectsRecursiveDoesNotFollowSymlinkedDirs`, `TestResolversForTargetIgnoresAutomaticSymlinksAndAllowsExplicitSelection`, `TestResolversForTargetAllowsUserSelectedSymlinkAsProjectRoot`, `TestLoadRejectsMalformedAndUnsupportedDocuments`, `TestLoadRejectsOversizedBaseline`, `TestDocumentEntryLimit`, `TestDocumentRejectsIndexedAdvisoryOverlap`, `FuzzLoad` | An explicitly selected baseline path is trusted and may be a symlink. | | SBOM and configuration input | Configuration reads are limited to 4 MiB and SBOM reads to 256 MiB before parsing. Strict configuration parsing rejects unknown keys. SBOM parsers are fuzzed and oversized documents fail clearly. | `TestSDKContractReadLimitEnforcesBoundsAndGrowth`, `TestLoadFileRejectsOversizedFile`, `TestDetectorResolveGraph_RejectsOversizedSBOM`, `FuzzLoadFile`, `FuzzUnmarshalAutoJSON` | A user-selected file can still consume work up to its limit. | +| SBOM assertions re-published on export | An ingested document's component and document-level assertions are re-emitted under Bomly's name, so each clears its SDK publication gate on ingest *and* again on export -- a `GraphEntry` is reachable by any detector or plugin, so a value written straight onto one never passed a decoder. Contacts drop email addresses; unpublishable identities, names, comments, references, CPEs and digests are dropped rather than written. Reference types `distribution` and `vcs` are deliberately not ingested, so a detector's origin assertion cannot re-enter as a document assertion (ADR-0033, ADR-0042). Gates are idempotent, so a value cannot be laundered by another hop. | `FuzzIngestedAssertions`, `FuzzDocumentAssertions`, `TestSourceClaimsAreRegatedOnExport`, `TestOriginIsNotReadBackFromAnIngestedDocument`, `TestSingleSourceExportIsAFixedPoint`, `TestIngestLeavesComponentsEligibleForEnrichment` | A published value is only as safe as the SDK gate that admitted it; a gate defect is an SDK defect (see bomly-dev/bomly-sdk#54). A merged SPDX export cannot yet name its sources (bomly-dev/bomly-sdk#55). | | Plugin download and extraction | Direct URL packages require a checksum unless the user explicitly bypasses it. GitHub release metadata is limited to 4 MiB. ZIP and tar extraction reject traversal, links, and special files. Downloads are limited to 256 MiB; archives to 4,096 entries, 256 MiB per expanded file, and 512 MiB total. Partial files are removed. | `TestResolveGitHubReleaseRejectsOversizedMetadata`, `TestExtractZipArchiveRejectsEscapingAndSymlinkEntries`, `TestExtractTarGzArchiveRejectsEscapingLinksAndSpecialFiles`, `TestCopyDownloadWithLimit`, `TestInstallRemoteArchiveRejectsDeclaredDownloadOverLimit`, `TestArchiveExtractionLimitsAtBoundary`, `TestArchiveExtractionRejectsResourceLimits`, `TestWriteArchiveFileRemovesPartialFileAtLimit` | `--insecure-skip-checksum` is an explicit integrity bypass. | | Plugin metadata and lifecycle | Manifests and runtime snapshots are limited to 1 MiB; the installed database is limited to 16 MiB. Plugins are installed disabled. Only enabled plugins register or run. The managed environment is allowlisted. | `TestReadFileWithLimitAcceptsExactBoundary`, `TestReadFileWithLimitRejectsOverBoundary`, `TestPluginJSONReadersRejectOversizedFiles`, `TestInstallDevBinaryVerifyEnableDisableAndUninstall`, `TestPrepareLoadsAndRunsExternalDetector`, `TestProtocolV1DetectorSnapshotDefaultsAbsentOptionalCapabilities`, `TestPluginEnvDoesNotForwardUnrelatedHostEnvironment` | Enabled plugins are trusted native processes with the user's privileges. The protocol is not an OS sandbox. | | Package-manager and detector subprocesses | Debug logs contain executable, sanitized arguments, working directory, and raw subprocess stderr. Credential-like flag values and URL user information are redacted from command arguments. Raw stderr is hidden below debug verbosity and is not added to structured results. Build-tool commands have existing timeouts where their detector contract supplies one. | `TestSDKContractSanitizeArgsRedactsCredentials`, `TestSDKContractCommandStderrHiddenBelowDebug` (local contracts; full suites upstream in `bomly-dev/bomly-sdk` `logkit/command_test.go` and `logkit/stderr_test.go`), `TestNewConsole` (`internal/logging/logger_test.go`), `TestInstallLogsReproducibleCommandAndDebugStderr`, `TestRunGitLogsStderrAtDebug`, Java readiness command tests, representative detector command-log tests | The selected executable, its credential store, registry configuration, network traffic, and filesystem behavior are trusted host concerns. Bomly cannot reliably sanitize arbitrary tool output. Debug stderr may contain credentials or other sensitive values and users must handle debug logs as sensitive data. | diff --git a/dev-docs/adr/0042-a-conversion-restates-its-source-a-merge-links-its-sources.md b/dev-docs/adr/0042-a-conversion-restates-its-source-a-merge-links-its-sources.md new file mode 100644 index 00000000..893cd525 --- /dev/null +++ b/dev-docs/adr/0042-a-conversion-restates-its-source-a-merge-links-its-sources.md @@ -0,0 +1,82 @@ +# ADR-0042: A conversion restates its source; a merge links its sources + +- **Date:** 2026-09-05 +- **Status:** Accepted + +## Context + +ADR-0037 gave document-level assertions a model home on `GraphEntry` and +stated the shape of the export projection: a merged document asserts its own +aggregate identity and links each source, while a single ingested document +re-exported "reproduces its own assertions". Implementing that (issue #396, +phase 2.5) turned two of its sentences into decisions that had to be made +concretely, because both formats give a document exactly one identity and the +model can hold several. + +The first is what a *conversion* does — one source document in, one document +out. Minting a fresh identity is the safe-looking answer, and it is the wrong +one: the source identity then has to be carried as a link, and on the next +ingest that link has nowhere to live, because `DocumentAssertions` describes +what a document says about *itself* and has no field for the documents behind +it. Each hop would therefore differ from the last, and the fixed point #396 +requires — Bomly's own export, re-ingested and re-exported, byte-identical — +would be unreachable by construction. + +The second is what the export surface receives. The pipeline merges entries +into one `sdk.Graph` before formatting, and that merge is exactly the step +that discards which document each part came from. + +## Decision + +**The export surface takes the prepared entries alongside the graph.** +`FromGraphEntries(graph, entries, opts)` and `MarshalGraphEntriesJSON` are the +entry points; `FromDepGraph` and `MarshalDepGraphJSON` remain as the no-source +form. Both arguments are passed and neither is derived from the other: the +graph is the one already selected for output — consolidation renamed its +identities and the scope filter decided what stays — so rebuilding it from the +entries would export a different graph than the rest of the command reports. +The entries are there for the one thing only they carry. + +**A conversion restates its source.** With exactly one source document, the +exported document adopts that document's identity rather than minting one, in +whichever identity slot the target format can hold it: an SPDX +`documentNamespace` takes any URI, a CycloneDX `serialNumber` takes only a +UUID URN, so a BOM-Link is parsed back to its serial and anything else is +linked instead. A caller-pinned identity always wins over both. The document +*name* is not adopted at the CLI, which names a document after the scanned +project. + +**A merge links its sources.** With two or more, the document mints its own +identity and names each source through a reference of type `bom` carrying a +BOM-Link or the source's namespace URI. Creators and tools union in both +cases, which is the SDK's declared merge class for them. + +**The claims are re-gated on export, not trusted from the entry.** A +`GraphEntry` is reachable by any detector or external plugin, so a value +written straight onto one never passed a decoder. Everything runs through +`DocumentAssertions.Normalized` on the way out as well as on the way in. + +## Consequences + +- `export → ingest → export` is a fixed point for both formats, asserted by + `TestSingleSourceExportIsAFixedPoint`. It found a real defect on its first + run: SPDX external references were emitted with the SDK's comparison-form + category (`other`) instead of the specification's spelling (`OTHER`), so an + ingested document changed case on its second export. `SPDXName()` is the + authority now. +- The fixed point holds within a format, not across one. A CycloneDX document + cannot hold a non-UUID identity, so an SPDX source converted to CycloneDX is + linked rather than adopted, and the chain back is a different document. +- A merged SPDX document links nothing. SPDX names another document through + `externalDocumentRefs`, and every entry requires a checksum over that + document's bytes — computable only at ingest, and `DocumentAssertions` has + nowhere to keep it. Filed as bomly-dev/bomly-sdk#55; the CycloneDX half + ships now, and the SPDX projection belongs beside it when the carrier grows + the field. Merged SPDX documents still preserve every component assertion. +- Consolidation carries `GraphEntry.Document` through, which it previously + dropped when rebuilding entries. That was silent: nothing downstream read + the field yet. +- `FuzzDocumentAssertions` covers the export projection with hostile claims + and asserts idempotence, the property whose failure in the component half + surfaced bomly-dev/bomly-sdk#54. The merged case is validated end to end by + the SBOM interoperability workflow, through both official validators. diff --git a/dev-docs/adr/README.md b/dev-docs/adr/README.md index 12c27784..b71c54d7 100644 --- a/dev-docs/adr/README.md +++ b/dev-docs/adr/README.md @@ -62,3 +62,4 @@ status to `Superseded by [ADR-NNNN](NNNN-slug.md)`; do not rewrite history | ADR-0039 | 2026-08-26 | [Both modules build on Go 1.27; untrusted JSON parses strictly](0039-both-modules-build-on-go-1-27.md) | Accepted | | ADR-0040 | 2026-08-26 | [The SDK is the default home for behavior](0040-the-sdk-is-the-default-home-for-behavior.md) | Accepted | | ADR-0041 | 2026-08-29 | [Identity is the canonical PURL on typed graph nodes](0041-identity-is-the-canonical-purl-on-typed-nodes.md) | Accepted | +| ADR-0042 | 2026-09-05 | [A conversion restates its source; a merge links its sources](0042-a-conversion-restates-its-source-a-merge-links-its-sources.md) | Accepted | diff --git a/docs/SBOM.md b/docs/SBOM.md index 6342c184..71869abe 100644 --- a/docs/SBOM.md +++ b/docs/SBOM.md @@ -67,6 +67,23 @@ This is fast, offline, and useful for: Format is auto-detected by content. The supported ingest formats are SPDX 2.3 JSON and CycloneDX 1.4–1.7 JSON; anything else is rejected as an unsupported format. Most SBOM producers, including Syft, can emit one of the supported formats directly (for example `syft -o spdx-json`). +### What an ingested document keeps + +Ingest reads more than package coordinates. What a source document asserted +about each component is carried through the scan and written back out: +supplier, originator or publisher, description, homepage, checksums, CPEs, +and the document's own external references with the category and type it +stated. + +The document's claims about *itself* are kept too — its identity, name, data +license, timestamp, credited people, organizations and tools, and its comment. + +Every one of those values is untrusted input that Bomly re-publishes under its +own name, so each is re-checked on the way in *and* on the way out. A value +that cannot be published — a credential in a URL, a local path, a control +character, a malformed CPE or digest — is dropped rather than passed along. +Email addresses are never retained on a contact. + ## Diffing SBOMs Compare two SBOM files without re-running detectors on either side: @@ -241,7 +258,22 @@ Every generated document carries a stable identity: - A generated `urn:uuid` serial number (CycloneDX `serialNumber`; the same nonce forms the SPDX document namespace, so the two exports of one scan are - correlatable). + correlatable). Two cases differ, and both apply only when the scan read + SBOMs rather than lockfiles: + - **One source document** (a conversion). The output keeps that document's + identity instead of minting a new one, because it restates one document + rather than describing a new subject. This is what makes + export → ingest → export reproduce the same bytes. A CycloneDX serial can + only hold a UUID URN, so an SPDX namespace that is not one is linked + instead of adopted (see below). The document *name* is unaffected: it + stays the scanned project's name. + - **Several source documents** (a merge). The output mints its own identity + — both formats give a document exactly one, and adopting a source's would + name a document that is not this one — and *links* each source: a + CycloneDX external reference of type `bom`, carrying a BOM-Link + (`urn:cdx:/`) for a CycloneDX source or the namespace URI + for an SPDX one. People and tools credited by any source are credited by + the merged document too. - The producing tool with its version (CycloneDX `metadata.tools[]`; SPDX `Creator: Tool: bomly-cli-`), plus one tool entry per detector that contributed to the graph. @@ -325,7 +357,13 @@ Some information necessarily becomes less specific during conversion: - Package origin is written on export but not read back on ingest: scanning an SBOM produces packages with no origin, so re-exporting that graph emits `NOASSERTION` and no distribution or vcs reference. Origin comes from a - lockfile, and an ingested document is not one. + lockfile, and an ingested document is not one. This is why references of + type `distribution` and `vcs` are the two Bomly does not preserve from a + source document: they are the shape Bomly's own origin export takes, and + nothing in a document distinguishes Bomly's emission from a third party's — + so reading them back would let a detector's guess about where a package came + from re-enter as though the document had asserted it. Every other reference + type is preserved with the category and type the source stated. - Bomly relationship confidence (`direct`, `transitive`, or `unknown`), source provenance, reachability analysis, policy findings, and run diagnostics are report data rather than portable SBOM fields. Use JSON when those distinctions @@ -338,6 +376,12 @@ Some information necessarily becomes less specific during conversion: written with the same detail as an inventory entry, so a package that is both the document's subject and a component describes itself the same way in both places. +- A merged SPDX document does not link its sources. SPDX names another + document through `externalDocumentRefs`, and every entry there requires a + checksum over that document's bytes, which Bomly's document carrier cannot + yet hold (tracked as bomly-dev/bomly-sdk#55). Merged CycloneDX documents do + link their sources, and merged SPDX documents still preserve every + component assertion. - The CycloneDX `group` namespace survives a CycloneDX round trip. SPDX 2.3 has no group field, so an SPDX round trip recovers the namespace only from the PURL. diff --git a/docs/SECURITY.md b/docs/SECURITY.md index 7758d799..fa610828 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -60,6 +60,13 @@ responses as untrusted input. plugin metadata have fixed size limits. - Configuration, baselines, SBOM input, and deps.dev responses have fixed limits. Oversized input fails with an error instead of being partially used. +- What an ingested SBOM asserted — component supplier, description, homepage, + checksums, CPEs, references, and the document's own identity, creators and + tools — is re-published under Bomly's name, so every one of those values is + re-checked on the way out as well as on the way in. A value carrying a + credential, a local path, a control character, or a malformed identifier is + dropped rather than written into the output document, and an email address + is never retained on a contact. - Manifests, lockfiles, workspace files, and source files parsed in process have a 64 MiB per-file limit. Local JSON cache entries have the same limit; an oversized or corrupt cache entry becomes a cache miss. diff --git a/internal/cli/scan_cmd.go b/internal/cli/scan_cmd.go index a4062b26..ac4cf687 100644 --- a/internal/cli/scan_cmd.go +++ b/internal/cli/scan_cmd.go @@ -130,6 +130,14 @@ func newScanCmd() *cobra.Command { return output.WriteSARIF(w, findings, pipeResult.Registry, "bomly", cmd.Root().Version, output.SARIFOptions{IncludeReachability: commandCtx.ResolvedConfig.Analyze, LocationGraphs: []*sdk.Graph{pipeResult.Graph}}) } + // The entries, not just the merged graph: a graph consolidated + // from ingested SBOMs no longer records what each source document + // said about itself, and the export needs that to restate it + // rather than credit only Bomly (ADR-0037). + var sbomEntries []sdk.GraphEntry + if pipeResult.Consolidated.Graphs != nil { + sbomEntries = pipeResult.Consolidated.Graphs.Entries + } sbomBuildOpts := scanSBOMBuildOptions(logger, payload.Project, commandCtx.ResolvedConfig, cmd.Root().Version, resolved, pipeResult.Registry, selectedScope, len(pipeResult.DetectorWarnings) > 0) if len(outputSpecs) > 0 { @@ -138,7 +146,7 @@ func newScanCmd() *cobra.Command { for _, spec := range outputSpecs { switch { case spec.IsSBOM(): - rawDocument, err := sbom.MarshalDepGraphJSON(selectedGraph, spec.Target, sbomBuildOpts, sbom.EncodeOptions{Pretty: true}) + rawDocument, err := sbom.MarshalGraphEntriesJSON(selectedGraph, sbomEntries, spec.Target, sbomBuildOpts, sbom.EncodeOptions{Pretty: true}) if err != nil { return fmt.Errorf("marshal %s sbom: %w", spec.Label, err) } @@ -162,7 +170,7 @@ func newScanCmd() *cobra.Command { if !ok { return exit.InvalidInputError("output format %q is not supported by scan", graphOutputFormat) } - rawDocument, err := sbom.MarshalDepGraphJSON(selectedGraph, target, sbomBuildOpts, sbom.EncodeOptions{Pretty: true}) + rawDocument, err := sbom.MarshalGraphEntriesJSON(selectedGraph, sbomEntries, target, sbomBuildOpts, sbom.EncodeOptions{Pretty: true}) if err != nil { return fmt.Errorf("marshal %s sbom: %w", graphOutputFormat, err) } diff --git a/internal/detectors/sbom/detector.go b/internal/detectors/sbom/detector.go index aa48c5c2..ad01c1bf 100644 --- a/internal/detectors/sbom/detector.go +++ b/internal/detectors/sbom/detector.go @@ -98,6 +98,12 @@ func (d Detector) ResolveGraph(_ context.Context, req sdk.DetectionRequest) (sdk return sdk.DetectionResult{}, fmt.Errorf("convert sbom %q to graph: %w", sbomPath, err) } graphs := sdk.SingleGraphContainer(depsGraph, detectorkit.InferManifestMetadata(req, evidencePatterns)) + // What the document said about itself rides the entry it became, so a + // later export can restate it instead of crediting only Bomly for a + // document Bomly only converted (ADR-0037). + if assertions := doc.Assertions; !assertions.IsEmpty() && len(graphs.Entries) == 1 { + graphs.Entries[0].Document = &assertions + } logger.Debug("resolved explicit sbom file", zap.String("path", sbomPath), zap.String("format", string(target))) return sdk.DetectionResult{ @@ -125,6 +131,7 @@ func normalizeSBOMManifestMetadata(container *sdk.GraphContainer, req sdk.Detect normalized.Entries = append(normalized.Entries, sdk.GraphEntry{ Graph: entry.Graph, Manifest: manifest, + Document: entry.Document, }) } return normalized @@ -143,6 +150,7 @@ func normalizeSBOMGraphContainer(container *sdk.GraphContainer) *sdk.GraphContai normalized.Entries = append(normalized.Entries, sdk.GraphEntry{ Graph: normalizedGraph, Manifest: entry.Manifest, + Document: entry.Document, }) } return normalized diff --git a/internal/engine/consolidation/consolidation.go b/internal/engine/consolidation/consolidation.go index 68fc82dc..995ab5be 100644 --- a/internal/engine/consolidation/consolidation.go +++ b/internal/engine/consolidation/consolidation.go @@ -106,6 +106,10 @@ func selectManifestEntries(results []sdk.DetectionResult) (sdk.ExecutionTarget, entry: sdk.GraphEntry{ Graph: normalizedGraph, Manifest: manifest, + // Carried, not dropped: this is the only record of what + // the source document said about itself, and export is + // downstream of here (ADR-0037). + Document: entry.Document, }, subproject: result.SubprojectInfo, detectorName: result.DetectorName, diff --git a/internal/sbom/codec.go b/internal/sbom/codec.go index 003621b6..2a60d072 100644 --- a/internal/sbom/codec.go +++ b/internal/sbom/codec.go @@ -100,8 +100,17 @@ func UnmarshalAutoJSON(data []byte) (*Document, Target, error) { } // MarshalDepGraphJSON converts a dependency graph directly into a target JSON SBOM. +// +// For a graph built from ingested SBOMs, prefer MarshalGraphEntriesJSON: see +// FromDepGraph for what this entry point cannot see. func MarshalDepGraphJSON(g *sdk.Graph, target Target, buildOpts BuildOptions, encodeOpts EncodeOptions) ([]byte, error) { - doc, err := FromDepGraph(g, buildOpts) + return MarshalGraphEntriesJSON(g, nil, target, buildOpts, encodeOpts) +} + +// MarshalGraphEntriesJSON converts the prepared graph entries and the graph +// they consolidated into directly into a target JSON SBOM. +func MarshalGraphEntriesJSON(g *sdk.Graph, entries []sdk.GraphEntry, target Target, buildOpts BuildOptions, encodeOpts EncodeOptions) ([]byte, error) { + doc, err := FromGraphEntries(g, entries, buildOpts) if err != nil { return nil, err } diff --git a/internal/sbom/cyclonedx.go b/internal/sbom/cyclonedx.go index 4bdfe6da..038fcec3 100644 --- a/internal/sbom/cyclonedx.go +++ b/internal/sbom/cyclonedx.go @@ -70,12 +70,11 @@ func (c cycloneDXCodec) encodeJSON(doc *Document, opts EncodeOptions) ([]byte, e } if doc.Provenance.Manufacturer != "" { metadata.Manufacturer = &cdx.OrganizationalEntity{Name: doc.Provenance.Manufacturer} - author := cdx.OrganizationalContact{Name: doc.Provenance.Manufacturer} - if email := bareEmail(doc.Provenance.SecurityContact); email != "" { - author.Email = email - } - metadata.Authors = &[]cdx.OrganizationalContact{author} } + if authors := cycloneDXDocumentAuthors(doc); len(authors) > 0 { + metadata.Authors = &authors + } + metadata.Tools = cycloneDXSourceTools(doc, metadata.Tools) if props := cycloneDXMetadataProperties(doc.Provenance); len(props) > 0 { metadata.Properties = &props } @@ -88,6 +87,14 @@ func (c cycloneDXCodec) encodeJSON(doc *Document, opts EncodeOptions) ([]byte, e bom.Compositions = &[]cdx.Composition{{Aggregate: aggregate}} } + // The documents this one was built from, named rather than inherited. + // Empty for a native scan and for a conversion that adopted its single + // source's identity; populated for a merge, and for a conversion whose + // source identity this format cannot hold. + if links := cycloneDXSourceLinks(doc); len(links) > 0 { + bom.ExternalReferences = &links + } + var out bytes.Buffer enc := cdx.NewBOMEncoder(&out, cdx.BOMFileFormatJSON).SetPretty(opts.Pretty) if err := enc.EncodeVersion(bom, toCycloneDXVersion(c.version)); err != nil { @@ -200,6 +207,7 @@ func (c cycloneDXCodec) decodeJSON(data []byte) (*Document, error) { return &Document{ Name: defaultDocumentName, + Assertions: cycloneDXDocumentAssertions(bom), Tool: cycloneDXPrimaryToolName(bom.Metadata), Tools: cycloneDXToolNames(bom.Metadata), Created: created, diff --git a/internal/sbom/cyclonedx_assertions.go b/internal/sbom/cyclonedx_assertions.go index 3c11c3da..099767f2 100644 --- a/internal/sbom/cyclonedx_assertions.go +++ b/internal/sbom/cyclonedx_assertions.go @@ -243,3 +243,157 @@ func cycloneDXEmittedHashes(digests []sdk.Digest) *[]cdx.Hash { } return &hashes } + +// cycloneDXDocumentAssertions reads what a CycloneDX document says about +// itself. +// +// The identity is a BOM-Link, built by cyclonedx-go from the serial and +// version rather than formatted here: the URN's shape is the library's to +// own, down to stripping the "urn:uuid:" prefix. A document without a serial +// has no identity to state, which is legal -- serialNumber is optional. +func cycloneDXDocumentAssertions(bom *cdx.BOM) sdk.DocumentAssertions { + if bom == nil { + return sdk.DocumentAssertions{} + } + var assertions sdk.DocumentAssertions + version := bom.Version + if version < 1 { + // ADR-0037's default: a document that did not number itself is + // version 1, which is also the only value NewBOMLink accepts below. + version = 1 + } + if link, err := cdx.NewBOMLink(bom.SerialNumber, version, nil); err == nil { + assertions.Identity = link.String() + } + if bom.Metadata != nil { + assertions.Created = bom.Metadata.Timestamp + if bom.Metadata.Manufacturer != nil { + assertions.Creators = appendEntityContact(assertions.Creators, *bom.Metadata.Manufacturer) + } + if bom.Metadata.Authors != nil { + for _, author := range *bom.Metadata.Authors { + assertions.Creators = appendContact(assertions.Creators, sdk.Contact{ + Kind: sdk.ContactKindPerson, + Name: author.Name, + }) + } + } + assertions.Tools = cycloneDXIngestedTools(bom.Metadata.Tools) + } + normalized, ok := assertions.Normalized() + if !ok { + return sdk.DocumentAssertions{} + } + return normalized +} + +// cycloneDXIngestedTools reads both shapes of the tools field: the component +// list CycloneDX 1.5 introduced and the deprecated flat list before it. +func cycloneDXIngestedTools(tools *cdx.ToolsChoice) []sdk.DocumentTool { + if tools == nil { + return nil + } + var ingested []sdk.DocumentTool + if tools.Components != nil { + for _, tool := range *tools.Components { + vendor := "" + if tool.Manufacturer != nil { + vendor = tool.Manufacturer.Name + } + ingested = append(ingested, sdk.DocumentTool{Vendor: vendor, Name: tool.Name, Version: tool.Version}) + } + } + if tools.Tools != nil { + for _, tool := range *tools.Tools { + ingested = append(ingested, sdk.DocumentTool{Vendor: tool.Vendor, Name: tool.Name, Version: tool.Version}) + } + } + return ingested +} + +func appendEntityContact(contacts []sdk.Contact, entity cdx.OrganizationalEntity) []sdk.Contact { + contact := sdk.Contact{Kind: sdk.ContactKindOrganization, Name: entity.Name} + if entity.URL != nil && len(*entity.URL) > 0 { + contact.URL = (*entity.URL)[0] + } + return appendContact(contacts, contact) +} + +// appendContact keeps only what the SDK's contact gate passes. A name that +// arrived with an email address loses the address there, not here. +func appendContact(contacts []sdk.Contact, contact sdk.Contact) []sdk.Contact { + normalized, ok := contact.Normalized() + if !ok { + return contacts + } + return append(contacts, normalized) +} + +// cycloneDXDocumentCreators renders the document's credited parties: the +// organizations become the manufacturer, the people become authors. +func cycloneDXDocumentAuthors(doc *Document) []cdx.OrganizationalContact { + var authors []cdx.OrganizationalContact + if doc.Provenance.Manufacturer != "" { + author := cdx.OrganizationalContact{Name: doc.Provenance.Manufacturer} + if email := bareEmail(doc.Provenance.SecurityContact); email != "" { + author.Email = email + } + authors = append(authors, author) + } + for _, creator := range doc.Assertions.Creators { + if creator.Kind != sdk.ContactKindPerson { + continue + } + authors = append(authors, cdx.OrganizationalContact{Name: creator.Name}) + } + return authors +} + +// cycloneDXSourceTools folds the tools the source documents credited in with +// Bomly's own, deduplicated on the triple the SDK keys them by. +func cycloneDXSourceTools(doc *Document, tools *cdx.ToolsChoice) *cdx.ToolsChoice { + if len(doc.Assertions.Tools) == 0 { + return tools + } + components := make([]cdx.Component, 0, len(doc.Assertions.Tools)) + if tools != nil && tools.Components != nil { + components = append(components, *tools.Components...) + } + seen := make(map[string]struct{}, len(components)) + for _, component := range components { + seen[component.Name+"\x00"+component.Version] = struct{}{} + } + for _, tool := range doc.Assertions.Tools { + key := tool.Name + "\x00" + tool.Version + if _, dup := seen[key]; dup { + continue + } + seen[key] = struct{}{} + component := cdx.Component{Type: cdx.ComponentTypeApplication, Name: tool.Name, Version: tool.Version} + if tool.Vendor != "" { + component.Manufacturer = &cdx.OrganizationalEntity{Name: tool.Vendor} + } + components = append(components, component) + } + if len(components) == 0 { + return tools + } + return &cdx.ToolsChoice{Components: &components} +} + +// cycloneDXSourceLinks renders the links naming the documents this one was +// built from, as external references of type "bom" on the document itself. +func cycloneDXSourceLinks(doc *Document) []cdx.ExternalReference { + links := documentSourceLinks(doc) + if len(links) == 0 { + return nil + } + refs := make([]cdx.ExternalReference, 0, len(links)) + for _, link := range links { + refs = append(refs, cdx.ExternalReference{ + Type: cdx.ExternalReferenceType(link.Type), + URL: link.Locator, + }) + } + return refs +} diff --git a/internal/sbom/document_assertions.go b/internal/sbom/document_assertions.go new file mode 100644 index 00000000..225f7c4a --- /dev/null +++ b/internal/sbom/document_assertions.go @@ -0,0 +1,138 @@ +package sbom + +import ( + cdx "github.com/CycloneDX/cyclonedx-go" + "github.com/bomly-dev/bomly-sdk" +) + +// This file decides what an export says about itself when the graph it +// describes was read from other documents (ADR-0037, issue #396). +// +// The rule turns on how many documents were ingested, because both formats +// give a document exactly one identity: +// +// - none: a native scan. Bomly asserts everything itself; nothing here +// applies. +// - one: a conversion. The document restates that source's assertions, +// identity included, so `export -> ingest -> export` reproduces its own +// bytes -- the fixed point #396 asks for. +// - many: a merge. The document is a new one and says so: it keeps its own +// identity and *links* each source instead of adopting one of them. +// +// Creators and tools union in every case, which is the SDK's declared merge +// class for them: two documents having produced this one is the normal case +// and both deserve credit. + +// applySourceAssertions folds the source documents' own claims into the +// document being built, and records the sources for link emission. +func applySourceAssertions(doc *Document, sources []sdk.DocumentAssertions) { + if doc == nil { + return + } + cleaned := make([]sdk.DocumentAssertions, 0, len(sources)) + for _, source := range sources { + // Re-gated here rather than trusted from the entry: these arrived + // from an untrusted document, crossed the plugin boundary as part of + // a detection result, and are about to be written into an SBOM. That + // is exactly the re-clearing rule ADR-0037 states. + normalized, ok := source.Normalized() + if !ok { + continue + } + cleaned = append(cleaned, normalized) + } + if len(cleaned) == 0 { + return + } + doc.Sources = cleaned + + aggregate := cleaned[0] + for _, source := range cleaned[1:] { + aggregate = sdk.MergeDocumentAssertions(aggregate, source) + } + doc.Assertions.Creators = aggregate.Creators + doc.Assertions.Tools = aggregate.Tools + + if len(cleaned) > 1 { + // A merged document asserts its own identity. Its sources are named + // by documentSourceLinks, not adopted here. + return + } + + only := cleaned[0] + doc.Assertions.Comment = only.Comment + if doc.Name == "" { + doc.Name = only.Name + } + // The data license is deliberately not inherited: SPDX 2.3 fixes it at + // CC0-1.0 for the document itself, so re-asserting a source's value would + // write an invalid document. It stays preserved in the model. + inheritDocumentIdentity(doc, only.Identity) +} + +// inheritDocumentIdentity adopts a single source's identity as this +// document's own, in whichever of the two formats' identity slots can hold +// it. +// +// A CycloneDX serial number is a UUID URN and nothing else, so an identity +// that is not a BOM-Link cannot become one; the SPDX namespace is any URI and +// takes either form. An identity that cannot be adopted is not lost -- it is +// linked instead, by documentSourceLinks. +func inheritDocumentIdentity(doc *Document, identity string) { + if identity == "" { + return + } + if doc.Namespace == "" { + doc.Namespace = identity + } + if doc.SerialNumber != "" { + return + } + // Parsed by the library that owns the grammar, so the serial comes back + // in the urn:uuid form CycloneDX wants without this package taking a + // position on how a BOM-Link is spelled. + link, err := cdx.ParseBOMLink(identity) + if err != nil { + return + } + doc.SerialNumber = link.SerialNumber() +} + +// documentSourceLinks returns the references that name each source document +// this one was built from, for the sources whose identity this document did +// not adopt as its own. +// +// A source whose identity became this document's identity is not linked: the +// document would be pointing at itself. +// +// Only CycloneDX renders these today. SPDX links documents through +// externalDocumentRefs, whose every entry requires a checksum over the source +// document's bytes -- and DocumentAssertions has nowhere to carry one, so a +// merged SPDX export names no sources. Tracked as bomly-dev/bomly-sdk#55; +// when that field ships, an SPDX projection belongs here beside this one. +func documentSourceLinks(doc *Document) []sdk.ExternalReference { + if doc == nil || len(doc.Sources) == 0 { + return nil + } + links := make([]sdk.ExternalReference, 0, len(doc.Sources)) + for _, source := range doc.Sources { + if source.Identity == "" || source.Identity == doc.Namespace { + continue + } + // Category stays unknown: this is CycloneDX's axis, and SPDX's + // referenceCategory has no member that means "another document" -- + // SPDX links documents through externalDocumentRefs instead. + ref, ok := sdk.ExternalReference{ + Type: string(cdx.ERTypeBOM), + Locator: source.Identity, + }.Normalized() + if !ok { + continue + } + links = append(links, ref) + } + if len(links) == 0 { + return nil + } + return sdk.MergeExternalReferences(nil, links) +} diff --git a/internal/sbom/document_assertions_fuzz_test.go b/internal/sbom/document_assertions_fuzz_test.go new file mode 100644 index 00000000..83326a3c --- /dev/null +++ b/internal/sbom/document_assertions_fuzz_test.go @@ -0,0 +1,133 @@ +package sbom + +import ( + "strings" + "testing" + + "github.com/bomly-dev/bomly-sdk" + "github.com/bomly-dev/bomly-sdk/testkit" +) + +// FuzzDocumentAssertions drives arbitrary document-level claims through the +// export projection and asserts that nothing unpublishable reaches a +// document, and that a second pass changes nothing. +// +// The component half of this lives in FuzzIngestedAssertions. This half +// covers the other trust boundary ADR-0037 opens: a graph entry's Document +// field is reachable by any detector or external plugin, so a value written +// straight onto it -- never having passed a decoder -- is exactly as +// untrusted as one that arrived in a document, and gets re-gated on export. +// +// Idempotence is asserted rather than assumed. bomly-dev/bomly-sdk#54 was +// found by the component target this way: a gate that repairs a value and +// then bounds it can grow the value past its own bound, and the next pass +// empties it -- which would break the fixed point #396 requires. +func FuzzDocumentAssertions(f *testing.F) { + f.Add("https://acme.example/spdx/doc-1", "acme-bom", "CC0-1.0", "2026-01-02T03:04:05Z", "Acme Corp", "acme-sbom-2.4.1", "a comment") + f.Add("urn:cdx:3e671687-395b-41f5-a30f-a58921a69b79/1", "n", "MIT", "not a date", "Person\nName", "tool", "line\nbreak") + f.Add("", "", "", "", "", "", "") + f.Add("file:///etc/passwd", "name\x00", "not-a-license", "2026", "bob@acme.example", "", strings.Repeat("c", 9000)) + f.Add("https://user:pass@acme.example/ns", strings.Repeat("n", 9000), "", "", "Acme (bob@acme.example)", "t\x07ool", "") + + f.Fuzz(func(t *testing.T, identity, name, dataLicense, created, creator, tool, comment string) { + for _, value := range []string{identity, name, dataLicense, created, creator, tool, comment} { + if len(value) > testkit.MaxFuzzInputSize { + t.Skip("input beyond the documented bound") + } + } + + hostile := sdk.DocumentAssertions{ + Identity: identity, + Name: name, + DataLicense: dataLicense, + Created: created, + Creators: []sdk.Contact{{Kind: sdk.ContactKindOrganization, Name: creator}}, + Tools: []sdk.DocumentTool{{Name: tool}}, + Comment: comment, + } + + g := mustFuzzGraph(t) + entry := sdk.GraphEntry{Graph: g, Document: &hostile} + doc, err := FromGraphEntries(g, []sdk.GraphEntry{entry}, BuildOptions{}) + if err != nil { + t.Fatalf("export: %v", err) + } + + // A document always identifies itself, whatever the source claimed. + if doc.Namespace == "" || doc.SerialNumber == "" { + t.Fatalf("document has no identity: ns=%q serial=%q", doc.Namespace, doc.SerialNumber) + } + // Every value the document now carries must clear its own gate: an + // unpublishable claim must not become publishable by being written + // onto an entry instead of parsed from a document. + if _, ok := (sdk.DocumentAssertions{Identity: doc.Namespace}).Normalized(); !ok { + t.Fatalf("document namespace %q does not clear the identity gate", doc.Namespace) + } + stored, _ := doc.Assertions.Normalized() + if stored.Comment != doc.Assertions.Comment { + t.Fatalf("stored comment %q is not normalized (%q)", doc.Assertions.Comment, stored.Comment) + } + for _, contact := range doc.Assertions.Creators { + if _, ok := contact.Normalized(); !ok { + t.Fatalf("stored creator %+v does not clear its own gate", contact) + } + if strings.Contains(contact.Name, "@") { + t.Fatalf("an address survived into a creator: %q", contact.Name) + } + } + for _, stored := range doc.Assertions.Tools { + if _, ok := stored.Normalized(); !ok { + t.Fatalf("stored tool %+v does not clear its own gate", stored) + } + } + for _, link := range documentSourceLinks(doc) { + if _, ok := link.Normalized(); !ok { + t.Fatalf("source link %+v does not clear its own gate", link) + } + } + + // Feeding the projection back its own output changes nothing. + second := sdk.GraphEntry{Graph: g, Document: &sdk.DocumentAssertions{ + Identity: doc.Namespace, + Name: doc.Name, + DataLicense: doc.Assertions.DataLicense, + Created: doc.Assertions.Created, + Creators: doc.Assertions.Creators, + Tools: doc.Assertions.Tools, + Comment: doc.Assertions.Comment, + }} + again, err := FromGraphEntries(g, []sdk.GraphEntry{second}, BuildOptions{}) + if err != nil { + t.Fatalf("second export: %v", err) + } + if again.Namespace != doc.Namespace { + t.Fatalf("a second pass changed the identity: %q then %q", doc.Namespace, again.Namespace) + } + if again.Name != doc.Name { + t.Fatalf("a second pass changed the name: %q then %q", doc.Name, again.Name) + } + if again.Assertions.Comment != doc.Assertions.Comment { + t.Fatalf("a second pass changed the comment: %q then %q", doc.Assertions.Comment, again.Assertions.Comment) + } + if len(again.Assertions.Creators) != len(doc.Assertions.Creators) || + len(again.Assertions.Tools) != len(doc.Assertions.Tools) { + t.Fatalf("a second pass changed the credited set sizes") + } + }) +} + +// mustFuzzGraph returns the smallest graph an export accepts, so the fuzzer +// spends its budget on the document claims rather than on graph shapes the +// component target already covers. +func mustFuzzGraph(t *testing.T) *sdk.Graph { + t.Helper() + g := sdk.New() + node, err := sdk.NewDependencyNode(sdk.Coordinates{Ecosystem: "npm", Name: "widget", Version: "1.0.0"}) + if err != nil { + t.Fatalf("construct node: %v", err) + } + if err := g.AddNode(node); err != nil { + t.Fatalf("add node: %v", err) + } + return g +} diff --git a/internal/sbom/document_assertions_test.go b/internal/sbom/document_assertions_test.go new file mode 100644 index 00000000..dc73161a --- /dev/null +++ b/internal/sbom/document_assertions_test.go @@ -0,0 +1,361 @@ +package sbom + +import ( + "bytes" + "encoding/json" + "strings" + "testing" + "time" + + cdx "github.com/CycloneDX/cyclonedx-go" + "github.com/bomly-dev/bomly-sdk" +) + +// documentRichSPDX asserts document-level claims: an identity, a name, named +// creators of both kinds, a tool, and a comment. +const documentRichSPDX = `{ + "spdxVersion": "SPDX-2.3", + "dataLicense": "CC0-1.0", + "SPDXID": "SPDXRef-DOCUMENT", + "name": "acme-platform-bom", + "documentNamespace": "https://acme.example/spdx/acme-platform-7f3c", + "comment": "Produced for the quarterly release review.", + "creationInfo": { + "created": "2026-01-02T03:04:05Z", + "creators": ["Organization: Acme Corp", "Person: Dana Scully (dana@acme.example)", "Tool: acme-sbom-2.4.1"] + }, + "packages": [ + { + "SPDXID": "SPDXRef-widget", + "name": "widget", + "versionInfo": "1.0.0", + "externalRefs": [ + {"referenceCategory": "PACKAGE-MANAGER", "referenceType": "purl", "referenceLocator": "pkg:npm/widget@1.0.0"} + ] + } + ] +}` + +// serialCycloneDX is a CycloneDX document that identifies itself: it carries +// a serial number, which is what a BOM-Link is built from. +const serialCycloneDX = `{ + "bomFormat": "CycloneDX", + "specVersion": "1.5", + "serialNumber": "urn:uuid:3e671687-395b-41f5-a30f-a58921a69b79", + "version": 1, + "metadata": { + "timestamp": "2026-01-03T04:05:06Z", + "tools": { "components": [ { "type": "application", "name": "cdx-gen", "version": "9.1.0" } ] } + }, + "components": [ + { + "bom-ref": "pkg:npm/gadget@2.0.0", + "type": "library", + "name": "gadget", + "version": "2.0.0", + "purl": "pkg:npm/gadget@2.0.0" + } + ] +}` + +// fixedExportTime pins the timestamp an export stamps, so two runs differ +// only in what they preserved. +func fixedExportTime() time.Time { + return time.Date(2026, 2, 28, 12, 0, 0, 0, time.UTC) +} + +// ingestDocument reads a document and returns the graph entry it becomes, +// the way the sbom detector builds one. +func ingestDocument(t *testing.T, raw string) (*sdk.Graph, sdk.GraphEntry) { + t.Helper() + doc, _, err := UnmarshalAutoJSON([]byte(raw)) + if err != nil { + t.Fatalf("ingest: %v", err) + } + g, err := ToGraph(doc) + if err != nil { + t.Fatalf("to graph: %v", err) + } + entry := sdk.GraphEntry{Graph: g} + if assertions := doc.Assertions; !assertions.IsEmpty() { + entry.Document = &assertions + } + return g, entry +} + +// An ingested document's own claims survive the graph hop, which is where +// they used to be dropped: a merged graph has no record of which document it +// came from. +func TestDocumentClaimsSurviveTheGraphHop(t *testing.T) { + _, entry := ingestDocument(t, documentRichSPDX) + if entry.Document == nil { + t.Fatal("the entry carries no document assertions") + } + got := *entry.Document + if got.Identity != "https://acme.example/spdx/acme-platform-7f3c" { + t.Errorf("identity = %q", got.Identity) + } + if got.Name != "acme-platform-bom" { + t.Errorf("name = %q", got.Name) + } + if got.DataLicense != "CC0-1.0" { + t.Errorf("data license = %q", got.DataLicense) + } + if got.Created != "2026-01-02T03:04:05Z" { + t.Errorf("created = %q", got.Created) + } + if !strings.Contains(got.Comment, "quarterly release") { + t.Errorf("comment = %q", got.Comment) + } + var org, person bool + for _, creator := range got.Creators { + switch creator.Kind { + case sdk.ContactKindOrganization: + org = org || creator.Name == "Acme Corp" + case sdk.ContactKindPerson: + person = person || creator.Name == "Dana Scully" + } + } + if !org || !person { + t.Errorf("creators = %+v, want both Acme Corp and Dana Scully", got.Creators) + } + if len(got.Tools) != 1 || got.Tools[0].Name != "acme-sbom-2.4.1" { + t.Errorf("tools = %+v", got.Tools) + } + // The address the source stated is not retained -- the SDK's contact gate + // strips it, and this asserts the gate is actually on this path. + for _, creator := range got.Creators { + if strings.Contains(creator.Name, "@") { + t.Errorf("an email address survived into a creator: %q", creator.Name) + } + } +} + +// The fixed point issue #396 asks for: a single-source export, re-ingested +// and re-exported, is byte-identical. +// +// The freshly minted values a second run would differ on -- the timestamp and +// the serial -- are pinned through BuildOptions, so what this actually +// compares is the preserved claims. Identity is deliberately not pinned: a +// conversion adopts its source's, so a drifting identity would show up here. +func TestSingleSourceExportIsAFixedPoint(t *testing.T) { + for _, target := range []Target{TargetSPDX23JSON, TargetCycloneDX16JSON} { + t.Run(string(target), func(t *testing.T) { + opts := BuildOptions{ToolVersion: "0.0.0-test"} + opts.Created = fixedExportTime() + + _, entry := ingestDocument(t, supplierRichCycloneDX) + first, err := MarshalGraphEntriesJSON(entry.Graph, []sdk.GraphEntry{entry}, target, opts, EncodeOptions{Pretty: true}) + if err != nil { + t.Fatalf("first export: %v", err) + } + + _, reingested := ingestDocument(t, string(first)) + second, err := MarshalGraphEntriesJSON(reingested.Graph, []sdk.GraphEntry{reingested}, target, opts, EncodeOptions{Pretty: true}) + if err != nil { + t.Fatalf("second export: %v", err) + } + + if !bytes.Equal(first, second) { + t.Errorf("export -> ingest -> export is not a fixed point.\nfirst:\n%s\nsecond:\n%s", first, second) + } + }) + } +} + +// A conversion adopts its single source's identity, so the document it +// produces still says which document it restates. +func TestSingleSourceExportAdoptsTheSourceIdentity(t *testing.T) { + _, entry := ingestDocument(t, documentRichSPDX) + doc, err := FromGraphEntries(entry.Graph, []sdk.GraphEntry{entry}, BuildOptions{}) + if err != nil { + t.Fatalf("export: %v", err) + } + if doc.Namespace != "https://acme.example/spdx/acme-platform-7f3c" { + t.Errorf("namespace = %q, want the source's", doc.Namespace) + } + if doc.Name != "acme-platform-bom" { + t.Errorf("name = %q, want the source's", doc.Name) + } + if len(doc.Sources) != 1 { + t.Fatalf("sources = %+v, want the one ingested document", doc.Sources) + } + // Not linked: the link would point at this document itself. + if links := documentSourceLinks(doc); len(links) != 0 { + t.Errorf("a source whose identity was adopted was also linked: %+v", links) + } +} + +// A caller-pinned identity wins over the source's, so `--sbom-namespace` +// still means what it says. +func TestPinnedIdentityWinsOverTheSource(t *testing.T) { + _, entry := ingestDocument(t, documentRichSPDX) + doc, err := FromGraphEntries(entry.Graph, []sdk.GraphEntry{entry}, BuildOptions{ + DocumentNS: "https://pinned.example/ns", + DocumentName: "pinned-name", + }) + if err != nil { + t.Fatalf("export: %v", err) + } + if doc.Namespace != "https://pinned.example/ns" { + t.Errorf("namespace = %q, want the pinned one", doc.Namespace) + } + if doc.Name != "pinned-name" { + t.Errorf("name = %q, want the pinned one", doc.Name) + } +} + +// A merged export states its own identity and links each source, rather than +// adopting one of them: both formats give a document exactly one identity, +// and picking a source's would name a document that is not this one. +func TestMergedExportLinksItsSourcesInsteadOfAdoptingOne(t *testing.T) { + _, spdxEntry := ingestDocument(t, documentRichSPDX) + _, cdxEntry := ingestDocument(t, serialCycloneDX) + entries := []sdk.GraphEntry{spdxEntry, cdxEntry} + + merged := sdk.New() + for _, entry := range entries { + if err := sdk.MergeGraph(merged, entry.Graph); err != nil { + t.Fatalf("merge: %v", err) + } + } + + doc, err := FromGraphEntries(merged, entries, BuildOptions{}) + if err != nil { + t.Fatalf("export: %v", err) + } + if doc.Namespace == "https://acme.example/spdx/acme-platform-7f3c" { + t.Error("the merged document adopted a source's identity") + } + if !strings.HasPrefix(doc.Namespace, "https://bomly.dev/spdx/") { + t.Errorf("namespace = %q, want a freshly minted one", doc.Namespace) + } + if len(doc.Sources) != 2 { + t.Fatalf("sources = %d, want both documents", len(doc.Sources)) + } + + // Both sources' creators and tools are credited: that is the SDK's + // declared merge class for these fields, and a merged document that + // dropped one source's credit would be asserting authorship it does not + // have. + var acme, widgetTool bool + for _, creator := range doc.Assertions.Creators { + acme = acme || creator.Name == "Acme Corp" + } + for _, tool := range doc.Assertions.Tools { + widgetTool = widgetTool || tool.Name == "acme-sbom-2.4.1" + } + if !acme || !widgetTool { + t.Errorf("merged credit lost: creators=%+v tools=%+v", doc.Assertions.Creators, doc.Assertions.Tools) + } + + links := documentSourceLinks(doc) + if len(links) != 2 { + t.Fatalf("links = %+v, want one per source", links) + } + for _, link := range links { + if link.Type != string(cdx.ERTypeBOM) { + t.Errorf("link type = %q, want %q", link.Type, cdx.ERTypeBOM) + } + } +} + +// The CycloneDX projection of those links: root-level external references of +// type "bom", carrying a BOM-Link for a CycloneDX source and the namespace +// URI for an SPDX one, exactly as ADR-0037 states. +func TestMergedCycloneDXExportCarriesSourceBOMLinks(t *testing.T) { + _, spdxEntry := ingestDocument(t, documentRichSPDX) + _, cdxEntry := ingestDocument(t, serialCycloneDX) + entries := []sdk.GraphEntry{spdxEntry, cdxEntry} + + merged := sdk.New() + for _, entry := range entries { + if err := sdk.MergeGraph(merged, entry.Graph); err != nil { + t.Fatalf("merge: %v", err) + } + } + raw, err := MarshalGraphEntriesJSON(merged, entries, TargetCycloneDX16JSON, BuildOptions{}, EncodeOptions{Pretty: true}) + if err != nil { + t.Fatalf("export: %v", err) + } + + var bom cdx.BOM + if err := json.Unmarshal(raw, &bom); err != nil { + t.Fatalf("decode export: %v", err) + } + if bom.ExternalReferences == nil { + t.Fatalf("the merged document links no sources:\n%s", raw) + } + var namespaceLink, bomLink bool + for _, ref := range *bom.ExternalReferences { + if ref.Type != cdx.ERTypeBOM { + continue + } + namespaceLink = namespaceLink || ref.URL == "https://acme.example/spdx/acme-platform-7f3c" + bomLink = bomLink || cdx.IsBOMLink(ref.URL) + } + if !namespaceLink { + t.Errorf("the SPDX source's namespace is not linked: %+v", *bom.ExternalReferences) + } + if !bomLink { + t.Errorf("the CycloneDX source is not linked as a BOM-Link: %+v", *bom.ExternalReferences) + } +} + +// A source document's claims are re-gated on the way out, not trusted because +// they were gated on the way in. The entry is reachable by any detector or +// external plugin, so a value written straight onto it must still be refused. +func TestSourceClaimsAreRegatedOnExport(t *testing.T) { + _, entry := ingestDocument(t, supplierRichCycloneDX) + entry.Document = &sdk.DocumentAssertions{ + Identity: "not a valid iri at all", + Name: "line\nbreak", + Comment: strings.Repeat("x", 1<<20), + Creators: []sdk.Contact{{Kind: sdk.ContactKindPerson, Name: "ctrl\x00char"}}, + } + doc, err := FromGraphEntries(entry.Graph, []sdk.GraphEntry{entry}, BuildOptions{}) + if err != nil { + t.Fatalf("export: %v", err) + } + if strings.Contains(doc.Namespace, "not a valid iri") { + t.Errorf("an unpublishable identity reached the document: %q", doc.Namespace) + } + if strings.Contains(doc.Name, "\n") { + t.Errorf("a line break reached the document name: %q", doc.Name) + } + if strings.Contains(doc.Assertions.Comment, "xxxx") { + t.Error("an over-long comment reached the document") + } + for _, creator := range doc.Assertions.Creators { + if strings.ContainsRune(creator.Name, 0) { + t.Errorf("a control character reached a creator: %q", creator.Name) + } + } +} + +// Bomly's own credit is not duplicated when a source already credited the +// same tool at the same version, which is what would otherwise make each hop +// of a round trip grow the creator list. +func TestBomlyCreditIsNotDuplicatedAcrossHops(t *testing.T) { + opts := BuildOptions{ToolVersion: "0.0.0-test", Created: fixedExportTime()} + _, entry := ingestDocument(t, supplierRichCycloneDX) + first, err := MarshalGraphEntriesJSON(entry.Graph, []sdk.GraphEntry{entry}, TargetSPDX23JSON, opts, EncodeOptions{Pretty: true}) + if err != nil { + t.Fatalf("first export: %v", err) + } + _, second := ingestDocument(t, string(first)) + doc, err := FromGraphEntries(second.Graph, []sdk.GraphEntry{second}, opts) + if err != nil { + t.Fatalf("second export: %v", err) + } + creators := spdxDocumentCreators(doc) + seen := map[string]int{} + for _, creator := range creators { + seen[creator.CreatorType+": "+creator.Creator]++ + } + for line, count := range seen { + if count > 1 { + t.Errorf("creator %q appears %d times: %+v", line, count, creators) + } + } +} diff --git a/internal/sbom/model.go b/internal/sbom/model.go index f7771d34..f73eb090 100644 --- a/internal/sbom/model.go +++ b/internal/sbom/model.go @@ -104,6 +104,28 @@ type Document struct { Components []Component Dependencies []Dependency Roots []string + + // Assertions are the claims this document makes about itself: its + // identity, name, data license, creators, tools, and comment. A decoder + // fills them; ingest carries them onto the graph entry the document + // became, so a later export can say what the source said (ADR-0037). + // + // This is what a document says about *itself*, which is why it is not the + // same field as Sources below. + Assertions sdk.DocumentAssertions + + // Sources are the documents this one was built from, one per ingested + // SBOM, in entry order. + // + // Empty for a native scan: nothing was ingested, so Bomly's own document + // asserts everything itself. One source is the conversion case, where the + // document restates that source's assertions -- the fixed point issue + // #396 requires. Two or more is the merge case, where the document + // asserts its own aggregate identity and *links* each source rather than + // inheriting one, because both formats give a document exactly one + // identity and picking a source's would name a document that is not this + // one. + Sources []sdk.DocumentAssertions } // IsProjectRootComponent reports whether a component is a synthesized pseudo diff --git a/internal/sbom/spdx23.go b/internal/sbom/spdx23.go index 25574b21..f0af19d2 100644 --- a/internal/sbom/spdx23.go +++ b/internal/sbom/spdx23.go @@ -78,7 +78,7 @@ func (spdx23Codec) encodeJSON(doc *Document, opts EncodeOptions) ([]byte, error) } if _, isRoot := rootComponents[c.ID]; isRoot || IsProjectRootComponent(c) { if doc.Provenance.Manufacturer != "" { - pkg.PackageSupplier = &common.Supplier{SupplierType: "Organization", Supplier: doc.Provenance.Manufacturer} + pkg.PackageSupplier = &common.Supplier{SupplierType: spdxOrganizationCreatorType, Supplier: doc.Provenance.Manufacturer} } } packages = append(packages, pkg) @@ -116,25 +116,8 @@ func (spdx23Codec) encodeJSON(doc *Document, opts EncodeOptions) ([]byte, error) } } - creators := make([]common.Creator, 0, len(doc.ToolNamesOrDefault())+1) - for _, tool := range doc.ToolNamesOrDefault() { - // SPDX creator convention appends the tool version as "name-version". - if tool == doc.ToolOrDefault() && doc.ToolVersion != "" { - tool += "-" + doc.ToolVersion - } - creators = append(creators, common.Creator{ - CreatorType: "Tool", - Creator: tool, - }) - } - if doc.Provenance.Manufacturer != "" { - creators = append(creators, common.Creator{ - CreatorType: "Organization", - Creator: doc.Provenance.Manufacturer, - }) - } creation := &v23.CreationInfo{ - Creators: creators, + Creators: spdxDocumentCreators(doc), Created: doc.CreatedOrNow().Format("2006-01-02T15:04:05Z"), CreatorComment: spdxCreatorComment(doc.Provenance), } @@ -146,6 +129,7 @@ func (spdx23Codec) encodeJSON(doc *Document, opts EncodeOptions) ([]byte, error) DocumentName: doc.NameOrDefault(), DocumentNamespace: doc.NamespaceOrDefault(), CreationInfo: creation, + DocumentComment: doc.Assertions.Comment, Packages: packages, Relationships: relationships, OtherLicenses: spdxOtherLicenses(extractedLicenses), @@ -233,6 +217,7 @@ func (spdx23Codec) decodeJSON(data []byte) (*Document, error) { return &Document{ Name: spdxDoc.DocumentName, Namespace: spdxDoc.DocumentNamespace, + Assertions: spdxDocumentAssertions(&spdxDoc), Tool: extractSPDXToolName(spdxDoc.CreationInfo), Tools: extractSPDXToolNames(spdxDoc.CreationInfo), Created: parseSPDXCreated(spdxDoc.CreationInfo), @@ -282,7 +267,7 @@ func extractSPDXToolNames(ci *v23.CreationInfo) []string { } tools := make([]string, 0, len(ci.Creators)) for _, c := range ci.Creators { - if c.CreatorType == "Tool" { + if c.CreatorType == spdxToolCreatorType { tools = append(tools, c.Creator) } } diff --git a/internal/sbom/spdx23_assertions.go b/internal/sbom/spdx23_assertions.go index 5ffdf43a..f8a658fe 100644 --- a/internal/sbom/spdx23_assertions.go +++ b/internal/sbom/spdx23_assertions.go @@ -17,6 +17,15 @@ import ( // grammar has an optional "()" suffix that must not be retained, which // is the kind of rule that gets forgotten when it is rewritten per call site. +// SPDX names a creator by its type. tools-golang splits the line into type +// and value but declares no constants for the three spellings the +// specification allows, so they are named here once rather than repeated as +// literals at every site that reads or writes one. +const ( + spdxToolCreatorType = "Tool" + spdxOrganizationCreatorType = "Organization" +) + // spdxIngestedSupplier reads a package's supplier. func spdxIngestedSupplier(supplier *common.Supplier) *sdk.Contact { if supplier == nil { @@ -254,7 +263,13 @@ func spdxEmittedReferences(refs []sdk.ExternalReference) []*v23.PackageExternalR if !ok { continue } - category := string(normalized.Category) + // SPDXName, not the category's own string: the SDK holds the + // category in its comparison form ("package-manager") and renders + // SPDX's spelling ("PACKAGE-MANAGER") separately. Emitting the + // comparison form wrote a category the specification does not define, + // and only showed up as an ingested document changing case on its + // second export -- which the fixed-point test is there to catch. + category := normalized.Category.SPDXName() if category == "" { // A CycloneDX-sourced reference has no category axis. SPDX // requires one, and OTHER is the category the specification @@ -273,3 +288,87 @@ func spdxEmittedReferences(refs []sdk.ExternalReference) []*v23.PackageExternalR } return emitted } + +// spdxDocumentAssertions reads what an SPDX document says about itself. +// +// tools-golang has already split each creator line into its type and value, +// so the "Person: name (email)" grammar is never re-parsed here -- the type +// selects the slot, and the value goes through the SDK's own contact gate. +func spdxDocumentAssertions(doc *v23.Document) sdk.DocumentAssertions { + if doc == nil { + return sdk.DocumentAssertions{} + } + assertions := sdk.DocumentAssertions{ + Identity: doc.DocumentNamespace, + Name: doc.DocumentName, + DataLicense: doc.DataLicense, + Comment: doc.DocumentComment, + } + if doc.CreationInfo != nil { + assertions.Created = doc.CreationInfo.Created + for _, creator := range doc.CreationInfo.Creators { + if strings.EqualFold(strings.TrimSpace(creator.CreatorType), spdxToolCreatorType) { + // Kept whole: see documentSourceTools for why the + // "name-version" convention is not split apart. + assertions.Tools = append(assertions.Tools, sdk.DocumentTool{Name: creator.Creator}) + continue + } + if contact := spdxContactFrom(creator.CreatorType, creator.Creator); contact != nil { + assertions.Creators = append(assertions.Creators, *contact) + } + } + } + normalized, ok := assertions.Normalized() + if !ok { + return sdk.DocumentAssertions{} + } + return normalized +} + +// spdxDocumentCreators renders the creator lines for a document, folding the +// parties and tools the source documents credited in with Bomly's own. +// +// Deduplicated on the rendered line, because that is what the format carries: +// a tool the source credited as "bomly-cli-0.19.0" and Bomly crediting itself +// under the same version are one creator, not two. +func spdxDocumentCreators(doc *Document) []common.Creator { + creators := make([]common.Creator, 0, len(doc.ToolNamesOrDefault())+1) + seen := make(map[string]struct{}) + add := func(creatorType, value string) { + value = strings.TrimSpace(value) + if value == "" { + return + } + line := creatorType + ": " + value + if _, dup := seen[line]; dup { + return + } + seen[line] = struct{}{} + creators = append(creators, common.Creator{CreatorType: creatorType, Creator: value}) + } + + for _, tool := range doc.ToolNamesOrDefault() { + // SPDX creator convention appends the tool version as "name-version". + if tool == doc.ToolOrDefault() && doc.ToolVersion != "" { + tool += "-" + doc.ToolVersion + } + add(spdxToolCreatorType, tool) + } + for _, tool := range doc.Assertions.Tools { + add(spdxToolCreatorType, tool.Name) + } + if doc.Provenance.Manufacturer != "" { + add(spdxOrganizationCreatorType, doc.Provenance.Manufacturer) + } + for _, creator := range doc.Assertions.Creators { + // Rendered by the SDK so the line matches the one it would parse + // back, rather than being assembled from the parts a second time. + line := creator.SPDXString() + creatorType, value, found := strings.Cut(line, ": ") + if !found { + continue + } + add(creatorType, value) + } + return creators +} diff --git a/internal/sbom/transform.go b/internal/sbom/transform.go index 1d216d0c..a154f9d0 100644 --- a/internal/sbom/transform.go +++ b/internal/sbom/transform.go @@ -20,10 +20,35 @@ import ( var ErrNilGraph = errors.New("dependency graph is nil") // FromDepGraph builds a neutral SBOM document from a dependency DAG. +// +// For a graph that came from ingested SBOMs, prefer FromGraphEntries: this +// entry point has no way to see what those documents said about themselves, +// and so exports a document that credits only Bomly. func FromDepGraph(g *sdk.Graph, opts BuildOptions) (*Document, error) { + return FromGraphEntries(g, nil, opts) +} + +// FromGraphEntries builds a neutral SBOM document from the prepared graph +// entries and the consolidated graph they produced. +// +// Both are passed, and neither is derived from the other. The graph is the +// one already selected for output -- consolidation renamed its identities and +// the scope filter decided what stays -- so re-merging the entries here would +// export a different graph than the rest of the command reports. The entries +// are here for the one thing only they carry: what each source document +// asserted about itself, which the merge into a single graph necessarily +// discards (ADR-0037). +func FromGraphEntries(g *sdk.Graph, entries []sdk.GraphEntry, opts BuildOptions) (*Document, error) { if g == nil { return nil, ErrNilGraph } + sources := make([]sdk.DocumentAssertions, 0, len(entries)) + for _, entry := range entries { + if entry.Document == nil { + continue + } + sources = append(sources, *entry.Document) + } componentCount := g.Size() components := make([]Component, 0, componentCount) @@ -148,11 +173,6 @@ func FromDepGraph(g *sdk.Graph, opts BuildOptions) (*Document, error) { created = time.Now().UTC() } - documentName := opts.DocumentName - if documentName == "" { - documentName = defaultDocumentName - } - // When the graph has no single root (multiple manifests, multiple // ecosystems) the primary component would otherwise be an arbitrary // manifest node. Synthesize a pseudo root that represents the scanned @@ -172,40 +192,56 @@ func FromDepGraph(g *sdk.Graph, opts BuildOptions) (*Document, error) { rootIDs = []string{root.ID} } - serialNumber := strings.TrimSpace(opts.SerialNumber) - nonce := "" - if serialNumber == "" { - nonce = newUUIDv4() - serialNumber = "urn:uuid:" + nonce - } - documentNS := opts.DocumentNS - if documentNS == "" { - if nonce == "" { - nonce = newUUIDv4() - } - documentNS = "https://bomly.dev/spdx/" + nonce - } toolName := opts.ToolName if toolName == "" { toolName = defaultToolName } toolNames := uniqueToolNames(append([]string{toolName}, opts.ToolNames...)) - return &Document{ - Name: documentName, - Namespace: documentNS, + doc := &Document{ + Name: opts.DocumentName, + Namespace: opts.DocumentNS, Tool: toolName, Tools: toolNames, ToolVersion: strings.TrimSpace(opts.ToolVersion), Created: created, - SerialNumber: serialNumber, + SerialNumber: strings.TrimSpace(opts.SerialNumber), Provenance: opts.Provenance, Lifecycle: strings.TrimSpace(opts.Lifecycle), Aggregate: strings.TrimSpace(opts.Aggregate), Components: components, Dependencies: dependencies, Roots: rootIDs, - }, nil + } + + // Before the identity is minted, not after: a conversion adopts its + // single source's identity, and it can only do that while the slot is + // still empty. An identity the caller pinned always wins over both. + applySourceAssertions(doc, sources) + mintDocumentIdentity(doc) + if doc.Name == "" { + doc.Name = defaultDocumentName + } + return doc, nil +} + +// mintDocumentIdentity fills whichever identity slots are still empty with a +// freshly generated one, so a document always identifies itself. +// +// Both slots share a nonce when both are minted, which keeps an export's +// SPDX namespace and CycloneDX serial recognizably the same document. +func mintDocumentIdentity(doc *Document) { + nonce := "" + if doc.SerialNumber == "" { + nonce = newUUIDv4() + doc.SerialNumber = "urn:uuid:" + nonce + } + if doc.Namespace == "" { + if nonce == "" { + nonce = newUUIDv4() + } + doc.Namespace = "https://bomly.dev/spdx/" + nonce + } } // projectRootComponent synthesizes the pseudo component representing the diff --git a/scripts/run-fuzz.sh b/scripts/run-fuzz.sh index a6bcb9cf..815d27e7 100755 --- a/scripts/run-fuzz.sh +++ b/scripts/run-fuzz.sh @@ -34,6 +34,7 @@ targets=( "github.com/bomly-dev/bomly-cli/internal/sbom FuzzNormalizeSPDXLicenseExpression" "github.com/bomly-dev/bomly-cli/internal/sbom FuzzSPDXLicenseValue" "github.com/bomly-dev/bomly-cli/internal/sbom FuzzIngestedAssertions" + "github.com/bomly-dev/bomly-cli/internal/sbom FuzzDocumentAssertions" "github.com/bomly-dev/bomly-cli/internal/baseline FuzzLoad" "github.com/bomly-dev/bomly-cli/internal/engine FuzzConsolidateVulnerabilities" "github.com/bomly-dev/bomly-cli/internal/plugin FuzzPluginPathSanitizers" From e03cb157e20ca5b98dcc956fe64b0b4bfef4473b Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Sat, 5 Sep 2026 21:31:52 -0700 Subject: [PATCH 4/6] fix(sbom): eight preservation defects review found, and the guard that missed one Every one reproduced before it was fixed and fails a test after. Identity and sources. Converting SPDX to CycloneDX adopted the source namespace into the model, minted a fresh serial because CycloneDX has no namespace slot, and then suppressed the source link by comparing against the namespace -- so the export named its source neither way. The comparison is now against the identity the format actually writes. A document that asserted nothing about itself is also a source now: dropping it made a merge of two documents look like a conversion of one and publish the merged inventory under the other's identity. A conversion keeps the source's creation time. It adopts the source's identity, and claiming that identity while stating a different creation time is two claims that disagree. The fixed-point test pinned the timestamp, which is exactly what hid this; it pins nothing now. Component data. A homepage vanished on any CycloneDX hop -- the format has no homepage field, so it travels as the website reference CycloneDX offers for the same claim, both ways. A document whose only component is its primary one lost supplier, description, hashes, CPE and references, since the ingest fallback never applied assertions. Configured provenance overwrote a root component's own supplier: one names who supplied the component, the other who produced the project. Two components minting one canonical package URL discarded the second along with everything it asserted. They fold through the SDK's InsertNode now. The guard that exists to catch exactly this spelled the lookup as `.Node(node.ID)`, so it never saw `.Node(packageID)` -- widened, which immediately found a second site in explain. An ingested organization disappeared even on a CycloneDX round trip: the export filled the one manufacturer slot from configured provenance alone. Co-Authored-By: Claude Opus 5 --- internal/detectors/guards_test.go | 8 +- internal/detectors/sbom/detector.go | 7 +- internal/engine/explain/why.go | 17 +- internal/sbom/assertions_round_trip_test.go | 165 +++++++++++++++- internal/sbom/cyclonedx.go | 25 ++- internal/sbom/cyclonedx_assertions.go | 72 ++++++- internal/sbom/document_assertions.go | 103 ++++++++-- .../sbom/document_assertions_fuzz_test.go | 2 +- internal/sbom/document_assertions_test.go | 180 ++++++++++++++++-- internal/sbom/graph.go | 12 +- internal/sbom/spdx23.go | 7 +- internal/sbom/transform.go | 13 +- 12 files changed, 548 insertions(+), 63 deletions(-) diff --git a/internal/detectors/guards_test.go b/internal/detectors/guards_test.go index 3d7052c5..c923336b 100644 --- a/internal/detectors/guards_test.go +++ b/internal/detectors/guards_test.go @@ -44,7 +44,13 @@ func walkInternalGo(t *testing.T, visit func(path, body string)) { func TestNodeInsertionGoesThroughTheSharedHelper(t *testing.T) { // A lookup on the graph followed by an insert, which is the shape that // silently discards the duplicate. - lookupThenAdd := regexp.MustCompile(`(?s)\.Node\(node\.ID\).{0,200}?\.AddNode\(`) + // Any receiver and any identifier, not the one variable name this rule was + // first written against. The regex used to spell the lookup as + // `.Node(node.ID)`, so internal/sbom's `.Node(packageID)` walked straight + // past it and silently discarded a duplicate component's assertions for as + // long as the guard has existed. A guard that only catches the shape you + // already fixed is not a guard. + lookupThenAdd := regexp.MustCompile(`(?s)\.Node\([A-Za-z_][\w.]*\).{0,200}?\.AddNode\(`) var offenders []string walkInternalGo(t, func(path, body string) { diff --git a/internal/detectors/sbom/detector.go b/internal/detectors/sbom/detector.go index ad01c1bf..72297d0c 100644 --- a/internal/detectors/sbom/detector.go +++ b/internal/detectors/sbom/detector.go @@ -100,9 +100,10 @@ func (d Detector) ResolveGraph(_ context.Context, req sdk.DetectionRequest) (sdk graphs := sdk.SingleGraphContainer(depsGraph, detectorkit.InferManifestMetadata(req, evidencePatterns)) // What the document said about itself rides the entry it became, so a // later export can restate it instead of crediting only Bomly for a - // document Bomly only converted (ADR-0037). - if assertions := doc.Assertions; !assertions.IsEmpty() && len(graphs.Entries) == 1 { - graphs.Entries[0].Document = &assertions + // document Bomly only converted (ADR-0037). The codec decides what that + // record contains, including for a document that asserted nothing. + if len(graphs.Entries) == 1 { + graphs.Entries[0].Document = sbom.DocumentAssertionsFor(doc) } logger.Debug("resolved explicit sbom file", zap.String("path", sbomPath), zap.String("format", string(target))) diff --git a/internal/engine/explain/why.go b/internal/engine/explain/why.go index a57451d8..c00b6d5a 100644 --- a/internal/engine/explain/why.go +++ b/internal/engine/explain/why.go @@ -113,16 +113,19 @@ func GraphFromPaths(source *sdk.Graph, paths []Path) (*sdk.Graph, error) { if source == nil { return focused, nil } + // Nodes repeat across paths -- a shared dependency is on every path that + // reaches it -- so insertion folds rather than skipping. Every witness here + // is a clone of one source node, so the fold is a no-op in practice; it is + // the shared entry point regardless, because "skipping is fine here" is a + // judgement each site otherwise re-makes on its own. for _, path := range paths { for i, ref := range path.Packages { pkg, ok := source.Node(ref.ID) if !ok || pkg == nil { continue } - if _, exists := focused.Node(pkg.NodeID()); !exists { - if err := focused.AddNode(pkg.CloneNode()); err != nil { - return nil, err - } + if _, err := focused.InsertNode(pkg.CloneNode()); err != nil { + return nil, err } if i == 0 { continue @@ -132,10 +135,8 @@ func GraphFromPaths(source *sdk.Graph, paths []Path) (*sdk.Graph, error) { if !ok || parent == nil { continue } - if _, exists := focused.Node(parent.NodeID()); !exists { - if err := focused.AddNode(parent.CloneNode()); err != nil { - return nil, err - } + if _, err := focused.InsertNode(parent.CloneNode()); err != nil { + return nil, err } if err := focused.AddEdge(parent.NodeID(), pkg.NodeID()); err != nil && !errors.Is(err, sdk.ErrCycleDetected) { return nil, err diff --git a/internal/sbom/assertions_round_trip_test.go b/internal/sbom/assertions_round_trip_test.go index 44481756..a992ffa9 100644 --- a/internal/sbom/assertions_round_trip_test.go +++ b/internal/sbom/assertions_round_trip_test.go @@ -48,16 +48,31 @@ func assertPreserved(t *testing.T, where string, component Component) { if !strings.Contains(component.Description, "widgeting") { t.Fatalf("%s: description = %q, want the source's", where, component.Description) } - if len(component.Digests) == 0 { - t.Fatalf("%s: checksums lost", where) + // Values, not presence. A conversion that kept a checksum entry while + // changing its algorithm or digest, or kept a reference while relabelling + // its type, would satisfy a presence check and still have corrupted the + // claim -- and the algorithm names differ between the two formats, which + // is exactly where such a slip would hide. + var digest Digest + for _, candidate := range component.Digests { + if strings.EqualFold(strings.ReplaceAll(candidate.Algorithm, "-", ""), "sha256") { + digest = candidate + } + } + if digest.Value != "9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08" { + t.Fatalf("%s: sha-256 checksum = %+v, want the source's", where, component.Digests) } - if len(component.CPEs) == 0 { - t.Fatalf("%s: CPE lost", where) + if len(component.CPEs) == 0 || component.CPEs[0] != "cpe:2.3:a:widget:widget:1.0.0:*:*:*:*:*:*:*" { + t.Fatalf("%s: CPE = %v, want the source's", where, component.CPEs) } var tracker bool for _, ref := range component.ExternalReferences { - if strings.Contains(ref.Locator, "widgets.example/issues") { - tracker = true + if ref.Locator != "https://widgets.example/issues" { + continue + } + tracker = true + if !strings.EqualFold(ref.Type, "issue-tracker") { + t.Fatalf("%s: the issue-tracker reference was relabelled %q", where, ref.Type) } } if !tracker { @@ -147,3 +162,141 @@ func TestIngestLeavesComponentsEligibleForEnrichment(t *testing.T) { } _ = sdk.EcosystemUnknown } + +// A homepage survives a CycloneDX hop. The format has no homepage field, so it +// travels as the website reference CycloneDX offers for the same claim -- and +// used to travel nowhere at all, disappearing on any SPDX-to-CycloneDX +// conversion. +func TestHomepageSurvivesCycloneDX(t *testing.T) { + const spdxWithHomepage = `{ + "spdxVersion": "SPDX-2.3", "dataLicense": "CC0-1.0", "SPDXID": "SPDXRef-DOCUMENT", + "name": "h", "documentNamespace": "https://h.example/spdx/1", + "creationInfo": {"created": "2026-01-01T00:00:00Z", "creators": ["Tool: t"]}, + "packages": [{ + "SPDXID": "SPDXRef-w", "name": "widget", "versionInfo": "1.0.0", + "homepage": "https://widget.example/", + "externalRefs": [{"referenceCategory": "PACKAGE-MANAGER", "referenceType": "purl", + "referenceLocator": "pkg:npm/widget@1.0.0"}] + }] +}` + doc, _, err := UnmarshalAutoJSON([]byte(spdxWithHomepage)) + if err != nil { + t.Fatalf("ingest: %v", err) + } + if got := componentNamed(t, doc, "widget").Homepage; got != "https://widget.example/" { + t.Fatalf("ingested homepage = %q", got) + } + graph, err := ToGraph(doc) + if err != nil { + t.Fatalf("to graph: %v", err) + } + cyclone, err := MarshalDepGraphJSON(graph, TargetCycloneDX16JSON, BuildOptions{}, EncodeOptions{Pretty: true}) + if err != nil { + t.Fatalf("cyclonedx export: %v", err) + } + back, _, err := UnmarshalAutoJSON(cyclone) + if err != nil { + t.Fatalf("re-ingest: %v", err) + } + if got := componentNamed(t, back, "widget").Homepage; got != "https://widget.example/" { + t.Fatalf("homepage after the CycloneDX hop = %q\n%s", got, cyclone) + } +} + +// A document whose only component is its primary one keeps that component's +// assertions. This is legal CycloneDX, and the ingest fallback that handles it +// used to read half the fields. +func TestMetadataOnlyComponentKeepsItsAssertions(t *testing.T) { + const metadataOnly = `{ + "bomFormat": "CycloneDX", "specVersion": "1.5", "version": 1, + "metadata": {"component": { + "bom-ref": "pkg:npm/solo@1.0.0", "type": "application", "name": "solo", "version": "1.0.0", + "purl": "pkg:npm/solo@1.0.0", "description": "the only component", + "supplier": {"name": "Solo Supply Co"}, + "hashes": [{"alg": "SHA-256", "content": "9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08"}] + }} +}` + doc, _, err := UnmarshalAutoJSON([]byte(metadataOnly)) + if err != nil { + t.Fatalf("ingest: %v", err) + } + solo := componentNamed(t, doc, "solo") + if solo.Supplier == nil || solo.Supplier.Name != "Solo Supply Co" { + t.Errorf("supplier = %+v", solo.Supplier) + } + if !strings.Contains(solo.Description, "only component") { + t.Errorf("description = %q", solo.Description) + } + if len(solo.Digests) == 0 { + t.Error("the checksum was dropped") + } +} + +// Two components that mint one canonical package URL fold into one node whose +// assertions are the union of both, rather than the first one's alone. +func TestDuplicateComponentsFoldTheirAssertions(t *testing.T) { + const duplicated = `{ + "bomFormat": "CycloneDX", "specVersion": "1.5", "version": 1, + "components": [ + {"bom-ref": "a", "type": "library", "name": "widget", "version": "1.0.0", + "purl": "pkg:npm/widget@1.0.0", + "externalReferences": [{"type": "issue-tracker", "url": "https://one.example/issues"}]}, + {"bom-ref": "b", "type": "library", "name": "widget", "version": "1.0.0", + "purl": "pkg:npm/widget@1.0.0", "description": "the second says more", + "supplier": {"name": "Second Supply Co"}, + "externalReferences": [{"type": "chat", "url": "https://two.example/chat"}]} + ] +}` + doc, _, err := UnmarshalAutoJSON([]byte(duplicated)) + if err != nil { + t.Fatalf("ingest: %v", err) + } + graph, err := ToGraph(doc) + if err != nil { + t.Fatalf("to graph: %v", err) + } + if graph.Size() != 1 { + t.Fatalf("size = %d, want the two components folded into one node", graph.Size()) + } + node := graph.DependencyNodes()[0] + if !strings.Contains(node.Description, "second says more") { + t.Errorf("description = %q, want the second component's -- a gap the first left", node.Description) + } + if node.Supplier == nil || node.Supplier.Name != "Second Supply Co" { + t.Errorf("supplier = %+v, want the second component's", node.Supplier) + } + var tracker, chat bool + for _, ref := range node.ExternalReferences { + tracker = tracker || strings.Contains(ref.Locator, "one.example") + chat = chat || strings.Contains(ref.Locator, "two.example") + } + if !tracker || !chat { + t.Errorf("references = %+v, want the union of both components'", node.ExternalReferences) + } +} + +// A root component's own supplier is not replaced by configured provenance: +// one names who supplied the component, the other who produced the project. +func TestConfiguredProvenanceDoesNotOverwriteARootSupplier(t *testing.T) { + doc, _, err := UnmarshalAutoJSON([]byte(supplierRichCycloneDX)) + if err != nil { + t.Fatalf("ingest: %v", err) + } + // The lone component is the document's root. + raw, err := MarshalJSON(doc, TargetSPDX23JSON, EncodeOptions{Pretty: true}) + if err != nil { + t.Fatalf("export: %v", err) + } + if !strings.Contains(string(raw), "Widget Supply Co") { + t.Fatalf("the source supplier is missing:\n%s", raw) + } + + doc.Provenance = Provenance{Manufacturer: "Operator Ltd"} + withProvenance, err := MarshalJSON(doc, TargetSPDX23JSON, EncodeOptions{Pretty: true}) + if err != nil { + t.Fatalf("export with provenance: %v", err) + } + if !strings.Contains(string(withProvenance), "Widget Supply Co") { + t.Errorf("configured provenance overwrote the component's own supplier:\n%s", withProvenance) + } +} diff --git a/internal/sbom/cyclonedx.go b/internal/sbom/cyclonedx.go index 038fcec3..66fef645 100644 --- a/internal/sbom/cyclonedx.go +++ b/internal/sbom/cyclonedx.go @@ -68,9 +68,7 @@ func (c cycloneDXCodec) encodeJSON(doc *Document, opts EncodeOptions) ([]byte, e } metadata.Component = &primary } - if doc.Provenance.Manufacturer != "" { - metadata.Manufacturer = &cdx.OrganizationalEntity{Name: doc.Provenance.Manufacturer} - } + metadata.Manufacturer = cycloneDXDocumentManufacturer(doc) if authors := cycloneDXDocumentAuthors(doc); len(authors) > 0 { metadata.Authors = &authors } @@ -163,7 +161,7 @@ func (c cycloneDXCodec) decodeJSON(data []byte) (*Document, error) { if len(componentByID) == 0 && bom.Metadata != nil && bom.Metadata.Component != nil { root := bom.Metadata.Component - componentByID[root.BOMRef] = Component{ + component := Component{ ID: root.BOMRef, Name: root.Name, Org: root.Group, @@ -174,6 +172,12 @@ func (c cycloneDXCodec) decodeJSON(data []byte) (*Document, error) { Copyright: root.Copyright, Licenses: parseCycloneDXLicenses(root.Licenses), } + // The same assertions the inventory loop applies. A document whose + // only component is its primary one is legal, and reading it with + // half the fields was a silent hole: supplier, description, hashes, + // CPE and references all stopped here. + applyCycloneDXAssertions(&component, *root) + componentByID[root.BOMRef] = component } components := make([]Component, 0, len(componentByID)) @@ -476,7 +480,7 @@ func cycloneDXComponent(comp Component) cdx.Component { // carries one list, so they concatenate; the emitted set is deduplicated // by the SDK's reference identity before it is written. refs := cycloneDXComponentReferences(comp) - refs = append(refs, cycloneDXEmittedReferences(comp.ExternalReferences)...) + refs = append(refs, cycloneDXEmittedReferences(cycloneDXComponentAssertedReferences(comp))...) if len(refs) > 0 { component.ExternalReferences = &refs } @@ -488,6 +492,17 @@ func cycloneDXComponent(comp Component) cdx.Component { return component } +// cycloneDXComponentAssertedReferences returns the references a source +// document asserted about a component, plus the website reference its homepage +// is carried in. +func cycloneDXComponentAssertedReferences(comp Component) []sdk.ExternalReference { + refs := comp.ExternalReferences + if homepage, ok := cycloneDXHomepageReference(comp); ok { + refs = sdk.MergeExternalReferences(refs, []sdk.ExternalReference{homepage}) + } + return refs +} + // cycloneDXLicenses renders a component's licenses into CycloneDX. // // The format offers three shapes and scores them differently: `license.id` is diff --git a/internal/sbom/cyclonedx_assertions.go b/internal/sbom/cyclonedx_assertions.go index 099767f2..73513f81 100644 --- a/internal/sbom/cyclonedx_assertions.go +++ b/internal/sbom/cyclonedx_assertions.go @@ -168,6 +168,7 @@ func applyCycloneDXAssertions(component *Component, comp cdx.Component) { component.Originator = cycloneDXOriginator(comp) component.Description = sdk.NormalizeDescription(comp.Description) component.ExternalReferences = cycloneDXIngestedReferences(comp.ExternalReferences) + component.Homepage = cycloneDXIngestedHomepage(component.ExternalReferences) if digests := cycloneDXComponentDigests(comp.Hashes); len(digests) > 0 { component.Digests = digests } @@ -176,6 +177,42 @@ func applyCycloneDXAssertions(component *Component, comp cdx.Component) { } } +// cycloneDXIngestedHomepage recovers a component's homepage from its website +// reference. +// +// CycloneDX has no homepage field; SPDX does. A reference of type "website" is +// what CycloneDX offers for the same claim, so that is where a homepage goes +// on the way out and where it is read on the way in -- otherwise converting +// SPDX to CycloneDX and back dropped PackageHomePage, since the value survived +// as a reference nobody read as a homepage. +func cycloneDXIngestedHomepage(refs []sdk.ExternalReference) string { + for _, ref := range refs { + if !strings.EqualFold(ref.Type, string(cdx.ERTypeWebsite)) { + continue + } + if homepage := sdk.NormalizeHomepage(ref.Locator); homepage != "" { + return homepage + } + } + return "" +} + +// cycloneDXHomepageReference renders a component's homepage as the website +// reference CycloneDX carries it in, or nothing when the component already +// states the same website itself. +func cycloneDXHomepageReference(comp Component) (sdk.ExternalReference, bool) { + homepage := sdk.NormalizeHomepage(comp.Homepage) + if homepage == "" { + return sdk.ExternalReference{}, false + } + for _, ref := range comp.ExternalReferences { + if strings.EqualFold(ref.Type, string(cdx.ERTypeWebsite)) && ref.Locator == homepage { + return sdk.ExternalReference{}, false + } + } + return sdk.ExternalReference{Type: string(cdx.ERTypeWebsite), Locator: homepage}.Normalized() +} + // cycloneDXEntityFor renders a contact as a CycloneDX organizational entity. func cycloneDXEntityFor(contact *sdk.Contact) *cdx.OrganizationalEntity { if contact == nil { @@ -329,8 +366,34 @@ func appendContact(contacts []sdk.Contact, contact sdk.Contact) []sdk.Contact { return append(contacts, normalized) } -// cycloneDXDocumentCreators renders the document's credited parties: the -// organizations become the manufacturer, the people become authors. +// cycloneDXDocumentManufacturer names the organization credited with the +// document. +// +// Configured provenance wins, because that is the operator stating who +// produced this run. Failing that, the first organization a source document +// credited: CycloneDX has one slot for an organization and no other place to +// put one, so an ingested manufacturer used to vanish even on a CycloneDX to +// CycloneDX round trip -- the export read only provenance, and every non-person +// creator was skipped. +func cycloneDXDocumentManufacturer(doc *Document) *cdx.OrganizationalEntity { + if doc.Provenance.Manufacturer != "" { + return &cdx.OrganizationalEntity{Name: doc.Provenance.Manufacturer} + } + for _, creator := range doc.Assertions.Creators { + if creator.Kind != sdk.ContactKindOrganization { + continue + } + entity := &cdx.OrganizationalEntity{Name: creator.Name} + if creator.URL != "" { + urls := []string{creator.URL} + entity.URL = &urls + } + return entity + } + return nil +} + +// cycloneDXDocumentAuthors renders the people credited with the document. func cycloneDXDocumentAuthors(doc *Document) []cdx.OrganizationalContact { var authors []cdx.OrganizationalContact if doc.Provenance.Manufacturer != "" { @@ -384,7 +447,10 @@ func cycloneDXSourceTools(doc *Document, tools *cdx.ToolsChoice) *cdx.ToolsChoic // cycloneDXSourceLinks renders the links naming the documents this one was // built from, as external references of type "bom" on the document itself. func cycloneDXSourceLinks(doc *Document) []cdx.ExternalReference { - links := documentSourceLinks(doc) + // CycloneDX writes a serial and no namespace, so that is the only identity + // a reader of this document can see, and the only one a source link could + // be redundant with. + links := documentSourceLinks(doc, documentIdentity{Serial: doc.SerialNumber}) if len(links) == 0 { return nil } diff --git a/internal/sbom/document_assertions.go b/internal/sbom/document_assertions.go index 225f7c4a..40289532 100644 --- a/internal/sbom/document_assertions.go +++ b/internal/sbom/document_assertions.go @@ -1,6 +1,8 @@ package sbom import ( + "time" + cdx "github.com/CycloneDX/cyclonedx-go" "github.com/bomly-dev/bomly-sdk" ) @@ -23,27 +25,52 @@ import ( // class for them: two documents having produced this one is the normal case // and both deserve credit. +// DocumentAssertionsFor returns the record of what a source document asserted +// about itself, for attaching to the graph entry that document became. +// +// It is never nil for a document that was read, even one that asserted +// nothing. The presence of the record is itself a fact -- that this entry came +// from an SBOM -- and a merged export needs it: CycloneDX permits a document +// with neither a serial number nor metadata, and treating that as "no source" +// made a merge of two documents look like a conversion of one, publishing the +// merged inventory under the other document's identity. +// +// Exported so the detector and this package agree by construction rather than +// by both remembering the same rule. +func DocumentAssertionsFor(doc *Document) *sdk.DocumentAssertions { + if doc == nil { + return nil + } + assertions := doc.Assertions + return &assertions +} + // applySourceAssertions folds the source documents' own claims into the // document being built, and records the sources for link emission. func applySourceAssertions(doc *Document, sources []sdk.DocumentAssertions) { if doc == nil { return } + if len(sources) == 0 { + return + } cleaned := make([]sdk.DocumentAssertions, 0, len(sources)) for _, source := range sources { // Re-gated here rather than trusted from the entry: these arrived // from an untrusted document, crossed the plugin boundary as part of // a detection result, and are about to be written into an SBOM. That // is exactly the re-clearing rule ADR-0037 states. - normalized, ok := source.Normalized() - if !ok { - continue - } + // + // A source that normalizes to nothing is kept as a placeholder rather + // than dropped. Whether this export is a conversion or a merge is a + // question about how many documents were read, not about how many of + // them had something publishable to say: CycloneDX permits a document + // with neither a serial number nor metadata, and dropping it here made + // a two-document merge look like a one-document conversion and adopt + // the other source's identity. + normalized, _ := source.Normalized() cleaned = append(cleaned, normalized) } - if len(cleaned) == 0 { - return - } doc.Sources = cleaned aggregate := cleaned[0] @@ -64,6 +91,20 @@ func applySourceAssertions(doc *Document, sources []sdk.DocumentAssertions) { if doc.Name == "" { doc.Name = only.Name } + // The source's own timestamp, not this run's clock. A conversion adopts + // the source's identity, and a document claiming to be that document while + // stating a different creation time is two claims that disagree. It is + // also what makes the fixed point hold without a caller pinning the + // timestamp: re-exporting twice used to differ by wall clock alone. + // + // Parsed rather than copied because the model holds a time; the SDK keeps + // the source spelling verbatim, and the first hop settles on the encoders' + // rendering of it. + if doc.Created.IsZero() { + if created, err := time.Parse(time.RFC3339, only.Created); err == nil { + doc.Created = created.UTC() + } + } // The data license is deliberately not inherited: SPDX 2.3 fixes it at // CC0-1.0 for the document itself, so re-asserting a source's value would // write an invalid document. It stays preserved in the model. @@ -98,25 +139,63 @@ func inheritDocumentIdentity(doc *Document, identity string) { doc.SerialNumber = link.SerialNumber() } +// documentIdentity is the identity a format actually writes into the document +// it is producing. +// +// Which slot is filled depends on the format: SPDX writes a namespace URI, +// CycloneDX a serial number, and neither writes the other. That distinction is +// the whole reason this type exists -- a source is only redundant with an +// identity the reader will actually see. +type documentIdentity struct { + Namespace string + Serial string +} + +// names reports whether an identity refers to this same document. +// +// One document has two spellings across the formats: an SPDX namespace, and a +// BOM-Link over a CycloneDX serial. Parsed by the library that owns the link +// grammar, and compared on the serial rather than the rendered link, because a +// source that numbered itself version 2 spells the same document differently +// than the version this export would write. +func (d documentIdentity) names(identity string) bool { + if identity == "" { + return false + } + if d.Namespace != "" && identity == d.Namespace { + return true + } + if d.Serial != "" { + if link, err := cdx.ParseBOMLink(identity); err == nil && link.SerialNumber() == d.Serial { + return true + } + } + return false +} + // documentSourceLinks returns the references that name each source document // this one was built from, for the sources whose identity this document did -// not adopt as its own. +// not adopt as the identity it is about to write. // -// A source whose identity became this document's identity is not linked: the -// document would be pointing at itself. +// The comparison is against what the format emits, not against the model's +// namespace field. Converting an SPDX source to CycloneDX adopts the source +// namespace into the model and then writes a freshly minted serial, because +// CycloneDX has no namespace slot -- so comparing against the namespace +// suppressed the link for a document that had not in fact adopted anything, +// and the export named its source neither way. // // Only CycloneDX renders these today. SPDX links documents through // externalDocumentRefs, whose every entry requires a checksum over the source // document's bytes -- and DocumentAssertions has nowhere to carry one, so a // merged SPDX export names no sources. Tracked as bomly-dev/bomly-sdk#55; // when that field ships, an SPDX projection belongs here beside this one. -func documentSourceLinks(doc *Document) []sdk.ExternalReference { +func documentSourceLinks(doc *Document, emitted documentIdentity) []sdk.ExternalReference { if doc == nil || len(doc.Sources) == 0 { return nil } links := make([]sdk.ExternalReference, 0, len(doc.Sources)) for _, source := range doc.Sources { - if source.Identity == "" || source.Identity == doc.Namespace { + if emitted.names(source.Identity) || source.Identity == "" { continue } // Category stays unknown: this is CycloneDX's axis, and SPDX's diff --git a/internal/sbom/document_assertions_fuzz_test.go b/internal/sbom/document_assertions_fuzz_test.go index 83326a3c..abd57198 100644 --- a/internal/sbom/document_assertions_fuzz_test.go +++ b/internal/sbom/document_assertions_fuzz_test.go @@ -80,7 +80,7 @@ func FuzzDocumentAssertions(f *testing.F) { t.Fatalf("stored tool %+v does not clear its own gate", stored) } } - for _, link := range documentSourceLinks(doc) { + for _, link := range documentSourceLinks(doc, documentIdentity{Serial: doc.SerialNumber}) { if _, ok := link.Normalized(); !ok { t.Fatalf("source link %+v does not clear its own gate", link) } diff --git a/internal/sbom/document_assertions_test.go b/internal/sbom/document_assertions_test.go index dc73161a..5e5e887a 100644 --- a/internal/sbom/document_assertions_test.go +++ b/internal/sbom/document_assertions_test.go @@ -76,11 +76,7 @@ func ingestDocument(t *testing.T, raw string) (*sdk.Graph, sdk.GraphEntry) { if err != nil { t.Fatalf("to graph: %v", err) } - entry := sdk.GraphEntry{Graph: g} - if assertions := doc.Assertions; !assertions.IsEmpty() { - entry.Document = &assertions - } - return g, entry + return g, sdk.GraphEntry{Graph: g, Document: DocumentAssertionsFor(doc)} } // An ingested document's own claims survive the graph hop, which is where @@ -134,15 +130,15 @@ func TestDocumentClaimsSurviveTheGraphHop(t *testing.T) { // The fixed point issue #396 asks for: a single-source export, re-ingested // and re-exported, is byte-identical. // -// The freshly minted values a second run would differ on -- the timestamp and -// the serial -- are pinned through BuildOptions, so what this actually -// compares is the preserved claims. Identity is deliberately not pinned: a -// conversion adopts its source's, so a drifting identity would show up here. +// Nothing is pinned. Neither the identity nor the timestamp is supplied by the +// caller, because a conversion takes both from its source -- so a value that +// drifted, or was silently replaced by this run's clock, shows up here as a +// byte difference. Pinning them would have made this test pass over exactly +// the defects it exists to catch. func TestSingleSourceExportIsAFixedPoint(t *testing.T) { for _, target := range []Target{TargetSPDX23JSON, TargetCycloneDX16JSON} { t.Run(string(target), func(t *testing.T) { opts := BuildOptions{ToolVersion: "0.0.0-test"} - opts.Created = fixedExportTime() _, entry := ingestDocument(t, supplierRichCycloneDX) first, err := MarshalGraphEntriesJSON(entry.Graph, []sdk.GraphEntry{entry}, target, opts, EncodeOptions{Pretty: true}) @@ -180,8 +176,9 @@ func TestSingleSourceExportAdoptsTheSourceIdentity(t *testing.T) { if len(doc.Sources) != 1 { t.Fatalf("sources = %+v, want the one ingested document", doc.Sources) } - // Not linked: the link would point at this document itself. - if links := documentSourceLinks(doc); len(links) != 0 { + // Not linked in the format that adopted it: the link would point at this + // document itself. + if links := documentSourceLinks(doc, documentIdentity{Namespace: doc.Namespace}); len(links) != 0 { t.Errorf("a source whose identity was adopted was also linked: %+v", links) } } @@ -249,7 +246,7 @@ func TestMergedExportLinksItsSourcesInsteadOfAdoptingOne(t *testing.T) { t.Errorf("merged credit lost: creators=%+v tools=%+v", doc.Assertions.Creators, doc.Assertions.Tools) } - links := documentSourceLinks(doc) + links := documentSourceLinks(doc, documentIdentity{Serial: doc.SerialNumber}) if len(links) != 2 { t.Fatalf("links = %+v, want one per source", links) } @@ -359,3 +356,160 @@ func TestBomlyCreditIsNotDuplicatedAcrossHops(t *testing.T) { } } } + +// Converting SPDX to CycloneDX names the source. The namespace is adopted into +// the model, but CycloneDX has no namespace slot and writes a freshly minted +// serial instead -- so the source has to be linked, or the exported document +// says nothing at all about where it came from. +func TestCycloneDXConversionLinksAnSPDXSourceItCannotAdopt(t *testing.T) { + _, entry := ingestDocument(t, documentRichSPDX) + raw, err := MarshalGraphEntriesJSON(entry.Graph, []sdk.GraphEntry{entry}, TargetCycloneDX16JSON, BuildOptions{}, EncodeOptions{Pretty: true}) + if err != nil { + t.Fatalf("export: %v", err) + } + var bom cdx.BOM + if err := json.Unmarshal(raw, &bom); err != nil { + t.Fatalf("decode: %v", err) + } + if bom.ExternalReferences == nil { + t.Fatalf("the conversion names its source neither by identity nor by link:\n%s", raw) + } + var linked bool + for _, ref := range *bom.ExternalReferences { + linked = linked || (ref.Type == cdx.ERTypeBOM && ref.URL == "https://acme.example/spdx/acme-platform-7f3c") + } + if !linked { + t.Errorf("the SPDX source is not linked: %+v", *bom.ExternalReferences) + } +} + +// A CycloneDX source Bomly *can* adopt is not also linked -- the link would +// point at this document itself. +func TestCycloneDXConversionDoesNotLinkTheIdentityItAdopted(t *testing.T) { + _, entry := ingestDocument(t, serialCycloneDX) + raw, err := MarshalGraphEntriesJSON(entry.Graph, []sdk.GraphEntry{entry}, TargetCycloneDX16JSON, BuildOptions{}, EncodeOptions{Pretty: true}) + if err != nil { + t.Fatalf("export: %v", err) + } + var bom cdx.BOM + if err := json.Unmarshal(raw, &bom); err != nil { + t.Fatalf("decode: %v", err) + } + if bom.SerialNumber != "urn:uuid:3e671687-395b-41f5-a30f-a58921a69b79" { + t.Fatalf("serial = %q, want the source's", bom.SerialNumber) + } + if bom.ExternalReferences != nil { + for _, ref := range *bom.ExternalReferences { + if ref.Type == cdx.ERTypeBOM { + t.Errorf("the document links itself: %+v", ref) + } + } + } +} + +// A document that asserted nothing about itself still counts as a source, so +// merging it with an identified document is a merge and not a conversion. +// +// CycloneDX permits a document with neither a serial number nor metadata, and +// treating it as absent made the export adopt the other source's identity -- +// publishing a merged inventory under the name of one of its inputs. +func TestAnAnonymousSourceStillCountsAsASource(t *testing.T) { + const anonymous = `{ + "bomFormat": "CycloneDX", + "specVersion": "1.5", + "version": 1, + "components": [ + {"bom-ref": "pkg:npm/quiet@1.0.0", "type": "library", "name": "quiet", + "version": "1.0.0", "purl": "pkg:npm/quiet@1.0.0"} + ] +}` + _, anonEntry := ingestDocument(t, anonymous) + if anonEntry.Document == nil { + t.Fatal("an ingested document with no claims left no record that it was read") + } + _, spdxEntry := ingestDocument(t, documentRichSPDX) + entries := []sdk.GraphEntry{anonEntry, spdxEntry} + + merged := sdk.New() + for _, entry := range entries { + if err := sdk.MergeGraph(merged, entry.Graph); err != nil { + t.Fatalf("merge: %v", err) + } + } + doc, err := FromGraphEntries(merged, entries, BuildOptions{}) + if err != nil { + t.Fatalf("export: %v", err) + } + if doc.Namespace == "https://acme.example/spdx/acme-platform-7f3c" { + t.Error("a merge of two documents adopted one source's identity") + } + if len(doc.Sources) != 2 { + t.Errorf("sources = %d, want both documents counted", len(doc.Sources)) + } +} + +// A conversion states the source's creation time, not this run's clock. The +// document claims the source's identity; claiming its identity and a different +// creation time is two statements that disagree. +func TestConversionKeepsTheSourceCreationTime(t *testing.T) { + _, entry := ingestDocument(t, documentRichSPDX) + doc, err := FromGraphEntries(entry.Graph, []sdk.GraphEntry{entry}, BuildOptions{}) + if err != nil { + t.Fatalf("export: %v", err) + } + if got := doc.Created.UTC().Format(time.RFC3339); got != "2026-01-02T03:04:05Z" { + t.Errorf("created = %q, want the source's", got) + } +} + +// An organization a source credited survives a CycloneDX round trip. The +// format has one slot for it, and the export used to fill that slot only from +// configured provenance. +func TestCycloneDXCreditsAnIngestedOrganization(t *testing.T) { + _, entry := ingestDocument(t, documentRichSPDX) + raw, err := MarshalGraphEntriesJSON(entry.Graph, []sdk.GraphEntry{entry}, TargetCycloneDX16JSON, BuildOptions{}, EncodeOptions{Pretty: true}) + if err != nil { + t.Fatalf("export: %v", err) + } + var bom cdx.BOM + if err := json.Unmarshal(raw, &bom); err != nil { + t.Fatalf("decode: %v", err) + } + if bom.Metadata == nil || bom.Metadata.Manufacturer == nil || bom.Metadata.Manufacturer.Name != "Acme Corp" { + t.Errorf("manufacturer = %+v, want the source's organization", bom.Metadata.Manufacturer) + } +} + +// Configured provenance still wins over an ingested organization: that is the +// operator saying who produced this run. +func TestConfiguredProvenanceOutranksAnIngestedOrganization(t *testing.T) { + _, entry := ingestDocument(t, documentRichSPDX) + doc, err := FromGraphEntries(entry.Graph, []sdk.GraphEntry{entry}, BuildOptions{ + Provenance: Provenance{Manufacturer: "Operator Ltd"}, + }) + if err != nil { + t.Fatalf("export: %v", err) + } + if got := cycloneDXDocumentManufacturer(doc); got == nil || got.Name != "Operator Ltd" { + t.Errorf("manufacturer = %+v, want the configured one", got) + } +} + +// The record exists for a document that asserted nothing, which is what makes +// a merge involving such a document read as a merge. +func TestDocumentAssertionsForAlwaysRecordsThatADocumentWasRead(t *testing.T) { + doc, _, err := UnmarshalAutoJSON([]byte(`{"bomFormat":"CycloneDX","specVersion":"1.5","version":1,"components":[]}`)) + if err != nil { + t.Fatalf("ingest: %v", err) + } + got := DocumentAssertionsFor(doc) + if got == nil { + t.Fatal("a document that asserted nothing left no record that it was read") + } + if !got.IsEmpty() { + t.Errorf("assertions = %+v, want the empty record", *got) + } + if DocumentAssertionsFor(nil) != nil { + t.Error("no document must mean no record") + } +} diff --git a/internal/sbom/graph.go b/internal/sbom/graph.go index 4228c0aa..69a72a20 100644 --- a/internal/sbom/graph.go +++ b/internal/sbom/graph.go @@ -234,10 +234,14 @@ func ToGraph(doc *Document) (*sdk.Graph, error) { packageID := pkg.NodeID() sdk.SetDetectionLicenses(pkg, graphLicenses(component.Licenses)) - if _, exists := depsGraph.Node(packageID); !exists { - if err := depsGraph.AddNode(pkg); err != nil { - return nil, fmt.Errorf("add package %q: %w", component.ID, err) - } + // Through the SDK's fold, not a lookup followed by an insert. Two + // components can mint one canonical package URL -- the same package + // listed twice, or listed once per manifest -- and skipping the second + // discarded everything it asserted: its supplier, its references, its + // digests. InsertNode applies the declared merge classes instead, + // scalars filling gaps and sets unioning (ADR-0041). + if _, err := depsGraph.InsertNode(pkg); err != nil { + return nil, fmt.Errorf("add package %q: %w", component.ID, err) } idMap[component.ID] = packageID } diff --git a/internal/sbom/spdx23.go b/internal/sbom/spdx23.go index f0af19d2..0d54a9d1 100644 --- a/internal/sbom/spdx23.go +++ b/internal/sbom/spdx23.go @@ -77,7 +77,12 @@ func (spdx23Codec) encodeJSON(doc *Document, opts EncodeOptions) ([]byte, error) PrimaryPackagePurpose: spdxPrimaryPackagePurpose(c.Type), } if _, isRoot := rootComponents[c.ID]; isRoot || IsProjectRootComponent(c) { - if doc.Provenance.Manufacturer != "" { + // Only when the component has no supplier of its own. Configured + // provenance names who produced the project; a supplier the + // source document asserted names who supplied that component, and + // overwriting the second with the first loses a claim someone + // actually made in favor of a default. + if doc.Provenance.Manufacturer != "" && pkg.PackageSupplier == nil { pkg.PackageSupplier = &common.Supplier{SupplierType: spdxOrganizationCreatorType, Supplier: doc.Provenance.Manufacturer} } } diff --git a/internal/sbom/transform.go b/internal/sbom/transform.go index a154f9d0..bf98c1f2 100644 --- a/internal/sbom/transform.go +++ b/internal/sbom/transform.go @@ -168,11 +168,6 @@ func FromGraphEntries(g *sdk.Graph, entries []sdk.GraphEntry, opts BuildOptions) } } - created := opts.Created.UTC() - if created.IsZero() { - created = time.Now().UTC() - } - // When the graph has no single root (multiple manifests, multiple // ecosystems) the primary component would otherwise be an arbitrary // manifest node. Synthesize a pseudo root that represents the scanned @@ -204,7 +199,7 @@ func FromGraphEntries(g *sdk.Graph, entries []sdk.GraphEntry, opts BuildOptions) Tool: toolName, Tools: toolNames, ToolVersion: strings.TrimSpace(opts.ToolVersion), - Created: created, + Created: opts.Created.UTC(), SerialNumber: strings.TrimSpace(opts.SerialNumber), Provenance: opts.Provenance, Lifecycle: strings.TrimSpace(opts.Lifecycle), @@ -219,6 +214,12 @@ func FromGraphEntries(g *sdk.Graph, entries []sdk.GraphEntry, opts BuildOptions) // still empty. An identity the caller pinned always wins over both. applySourceAssertions(doc, sources) mintDocumentIdentity(doc) + if doc.Created.IsZero() { + // Only once nothing else supplied one: a caller's pinned timestamp + // first, then the source document's on a conversion, and this run's + // clock last. + doc.Created = time.Now().UTC() + } if doc.Name == "" { doc.Name = defaultDocumentName } From b10d495f3f9237e6b8c678bf27c9c008cbbdebe3 Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Sat, 5 Sep 2026 22:02:32 -0700 Subject: [PATCH 5/6] fix(sbom): a person stays a person, and a credited tool keeps its identity Three more from review, each reproduced first. An SPDX "Person: Alice" came back from a CycloneDX hop as "Organization: Alice". CycloneDX reads `publisher` as an organization and `author` as a person, and every originator went through `publisher` -- so the claim was not lost but changed, which is worse, because nothing downstream can tell it happened. Persons now go through `author` and `authors`; cyclonedx-go emits whichever the target spec version defines, so 1.4 still carries it. A source crediting "Acme / cdx-gen / 9.1.0" became "Tool: cdx-gen" on SPDX export. SPDX writes a tool as name-version, the same convention Bomly already used for its own creator line. The vendor is dropped there because SPDX models none, and folding it into the name would produce a tool called "Acme/cdx-gen" on the next read -- the same corrupt-versus-drop choice as above, resolved the same way. On CycloneDX export that tool was suppressed outright: the deduplication key was written here by hand and omitted the vendor, so a vendorless entry of the same name and version swallowed it. The union is the SDK's now, which keys on the whole triple, and the local key is gone. Merged source links remain write-only -- there is nowhere to put them on ingest, so a merged export converted again names no sources. Filed as bomly-dev/bomly-sdk#61, linked from the code and documented, rather than smuggled through Metadata. Co-Authored-By: Claude Opus 5 --- docs/SBOM.md | 5 ++ internal/sbom/assertions_round_trip_test.go | 80 +++++++++++++++++++++ internal/sbom/cyclonedx.go | 31 +------- internal/sbom/cyclonedx_assertions.go | 73 +++++++++++++------ internal/sbom/document_assertions.go | 7 ++ internal/sbom/document_assertions_test.go | 50 +++++++++++++ internal/sbom/spdx23_assertions.go | 21 +++++- 7 files changed, 215 insertions(+), 52 deletions(-) diff --git a/docs/SBOM.md b/docs/SBOM.md index 71869abe..9ed707b7 100644 --- a/docs/SBOM.md +++ b/docs/SBOM.md @@ -382,6 +382,11 @@ Some information necessarily becomes less specific during conversion: yet hold (tracked as bomly-dev/bomly-sdk#55). Merged CycloneDX documents do link their sources, and merged SPDX documents still preserve every component assertion. +- Bomly does not read those source links back. A merged CycloneDX document + names the documents it was built from, but converting that document again + produces one that names no sources: Bomly records what a document says about + itself and has no place for the documents behind it (tracked as + bomly-dev/bomly-sdk#61). Keep the merged output if you need its provenance. - The CycloneDX `group` namespace survives a CycloneDX round trip. SPDX 2.3 has no group field, so an SPDX round trip recovers the namespace only from the PURL. diff --git a/internal/sbom/assertions_round_trip_test.go b/internal/sbom/assertions_round_trip_test.go index a992ffa9..136f3b16 100644 --- a/internal/sbom/assertions_round_trip_test.go +++ b/internal/sbom/assertions_round_trip_test.go @@ -300,3 +300,83 @@ func TestConfiguredProvenanceDoesNotOverwriteARootSupplier(t *testing.T) { t.Errorf("configured provenance overwrote the component's own supplier:\n%s", withProvenance) } } + +// A person stays a person through a CycloneDX hop. +// +// CycloneDX reads `publisher` as an organization and `author` as a person, so +// routing every originator through `publisher` did not lose the distinction -- +// it asserted the wrong one, turning "Person: Alice" into "Organization: +// Alice". Corrupting a claim is worse than dropping it, because nothing +// downstream can tell it happened. +func TestOriginatorKeepsItsContactKindThroughCycloneDX(t *testing.T) { + const personOriginator = `{ + "spdxVersion": "SPDX-2.3", "dataLicense": "CC0-1.0", "SPDXID": "SPDXRef-DOCUMENT", + "name": "p", "documentNamespace": "https://p.example/spdx/1", + "creationInfo": {"created": "2026-01-01T00:00:00Z", "creators": ["Tool: t"]}, + "packages": [{ + "SPDXID": "SPDXRef-w", "name": "widget", "versionInfo": "1.0.0", + "originator": "Person: Alice Example", + "externalRefs": [{"referenceCategory": "PACKAGE-MANAGER", "referenceType": "purl", + "referenceLocator": "pkg:npm/widget@1.0.0"}] + }] +}` + doc, _, err := UnmarshalAutoJSON([]byte(personOriginator)) + if err != nil { + t.Fatalf("ingest: %v", err) + } + graph, err := ToGraph(doc) + if err != nil { + t.Fatalf("to graph: %v", err) + } + // Every supported spec version: `authors` is the 1.6 form and `author` the + // older one, so a 1.4 document has to carry the claim too. + for _, target := range []Target{TargetCycloneDX14JSON, TargetCycloneDX15JSON, TargetCycloneDX16JSON, TargetCycloneDX17JSON} { + t.Run(string(target), func(t *testing.T) { + raw, err := MarshalDepGraphJSON(graph, target, BuildOptions{}, EncodeOptions{Pretty: true}) + if err != nil { + t.Fatalf("export: %v", err) + } + back, _, err := UnmarshalAutoJSON(raw) + if err != nil { + t.Fatalf("re-ingest: %v", err) + } + got := componentNamed(t, back, "widget").Originator + if got == nil { + t.Fatalf("the originator was lost:\n%s", raw) + } + if got.Kind != sdk.ContactKindPerson { + t.Errorf("originator kind = %q, want person -- the claim was changed, not just dropped", got.Kind) + } + if got.Name != "Alice Example" { + t.Errorf("originator name = %q", got.Name) + } + }) + } +} + +// An organization originator still goes to `publisher`, which is the field +// CycloneDX reads back as an organization. +func TestOrganizationOriginatorStillUsesPublisher(t *testing.T) { + doc, _, err := UnmarshalAutoJSON([]byte(supplierRichCycloneDX)) + if err != nil { + t.Fatalf("ingest: %v", err) + } + graph, err := ToGraph(doc) + if err != nil { + t.Fatalf("to graph: %v", err) + } + raw, err := MarshalDepGraphJSON(graph, TargetCycloneDX16JSON, BuildOptions{}, EncodeOptions{Pretty: true}) + if err != nil { + t.Fatalf("export: %v", err) + } + if !strings.Contains(string(raw), `"publisher": "Widget Publishing Inc"`) { + t.Errorf("an organization originator did not go to publisher:\n%s", raw) + } + back, _, err := UnmarshalAutoJSON(raw) + if err != nil { + t.Fatalf("re-ingest: %v", err) + } + if got := componentNamed(t, back, "widget").Originator; got == nil || got.Kind != sdk.ContactKindOrganization { + t.Errorf("originator = %+v, want an organization", got) + } +} diff --git a/internal/sbom/cyclonedx.go b/internal/sbom/cyclonedx.go index 66fef645..b3d8d77a 100644 --- a/internal/sbom/cyclonedx.go +++ b/internal/sbom/cyclonedx.go @@ -50,7 +50,7 @@ func (c cycloneDXCodec) encodeJSON(doc *Document, opts EncodeOptions) ([]byte, e metadata := &cdx.Metadata{ Timestamp: doc.CreatedOrNow().Format(time.RFC3339), - Tools: cycloneDXTools(doc.ToolNamesOrDefault(), doc.ToolOrDefault(), doc.ToolVersion), + Tools: cycloneDXMetadataTools(doc), } if root := chooseRoot(doc); root != nil { // The primary component is built the same way as an inventory entry. @@ -72,7 +72,6 @@ func (c cycloneDXCodec) encodeJSON(doc *Document, opts EncodeOptions) ([]byte, e if authors := cycloneDXDocumentAuthors(doc); len(authors) > 0 { metadata.Authors = &authors } - metadata.Tools = cycloneDXSourceTools(doc, metadata.Tools) if props := cycloneDXMetadataProperties(doc.Provenance); len(props) > 0 { metadata.Properties = &props } @@ -222,30 +221,6 @@ func (c cycloneDXCodec) decodeJSON(data []byte) (*Document, error) { }, nil } -func cycloneDXTools(names []string, primaryTool, toolVersion string) *cdx.ToolsChoice { - if len(names) == 0 { - return nil - } - components := make([]cdx.Component, 0, len(names)) - for _, name := range names { - if strings.TrimSpace(name) == "" { - continue - } - component := cdx.Component{ - Type: cdx.ComponentTypeApplication, - Name: name, - } - if name == primaryTool { - component.Version = toolVersion - } - components = append(components, component) - } - if len(components) == 0 { - return nil - } - return &cdx.ToolsChoice{Components: &components} -} - // cycloneDXSecurityReferences maps provenance contact fields onto external // references attached to the primary component. // cycloneDXComponentReferences renders where a package came from: the exact @@ -485,9 +460,7 @@ func cycloneDXComponent(comp Component) cdx.Component { component.ExternalReferences = &refs } component.Supplier = cycloneDXEntityFor(comp.Supplier) - if originator := cycloneDXEntityFor(comp.Originator); originator != nil { - component.Publisher = originator.Name - } + cycloneDXApplyOriginator(&component, comp.Originator) component.Description = sdk.NormalizeDescription(comp.Description) return component } diff --git a/internal/sbom/cyclonedx_assertions.go b/internal/sbom/cyclonedx_assertions.go index 73513f81..4d0593d2 100644 --- a/internal/sbom/cyclonedx_assertions.go +++ b/internal/sbom/cyclonedx_assertions.go @@ -213,6 +213,32 @@ func cycloneDXHomepageReference(comp Component) (sdk.ExternalReference, bool) { return sdk.ExternalReference{Type: string(cdx.ERTypeWebsite), Locator: homepage}.Normalized() } +// cycloneDXApplyOriginator writes the party that authored a component into the +// field CycloneDX reserves for that kind of party. +// +// The kind is not decoration. `publisher` is read back as an organization and +// `author` as a person -- by this package's own decoder and by other tools -- +// so routing a person through `publisher` did not merely lose the +// distinction, it asserted the wrong one: an SPDX "Person: Alice" came back +// from a CycloneDX hop as "Organization: Alice". Corrupting a claim is worse +// than dropping it, because nothing downstream can tell it happened. +// +// Both author fields are set for a person. `authors` is the 1.6 form and +// `author` the older one; cyclonedx-go emits whichever the target spec version +// defines, so a 1.4 document still carries the claim. +func cycloneDXApplyOriginator(component *cdx.Component, originator *sdk.Contact) { + entity := cycloneDXEntityFor(originator) + if entity == nil { + return + } + if originator.Kind == sdk.ContactKindPerson { + component.Author = entity.Name + component.Authors = &[]cdx.OrganizationalContact{{Name: entity.Name}} + return + } + component.Publisher = entity.Name +} + // cycloneDXEntityFor renders a contact as a CycloneDX organizational entity. func cycloneDXEntityFor(contact *sdk.Contact) *cdx.OrganizationalEntity { if contact == nil { @@ -412,35 +438,38 @@ func cycloneDXDocumentAuthors(doc *Document) []cdx.OrganizationalContact { return authors } -// cycloneDXSourceTools folds the tools the source documents credited in with -// Bomly's own, deduplicated on the triple the SDK keys them by. -func cycloneDXSourceTools(doc *Document, tools *cdx.ToolsChoice) *cdx.ToolsChoice { - if len(doc.Assertions.Tools) == 0 { - return tools - } - components := make([]cdx.Component, 0, len(doc.Assertions.Tools)) - if tools != nil && tools.Components != nil { - components = append(components, *tools.Components...) - } - seen := make(map[string]struct{}, len(components)) - for _, component := range components { - seen[component.Name+"\x00"+component.Version] = struct{}{} - } - for _, tool := range doc.Assertions.Tools { - key := tool.Name + "\x00" + tool.Version - if _, dup := seen[key]; dup { - continue +// cycloneDXMetadataTools renders the tools credited with the document: the +// ones this run used, plus the ones its source documents credited. +// +// The union is the SDK's, not a local one. Its merge class for tools keys on +// the whole (vendor, name, version) triple, and a key written here by hand +// omitted the vendor -- so a source crediting "Acme / cdx-gen / 9.1.0" was +// silently suppressed by a vendorless entry of the same name and version, +// discarding the one field the source added. +func cycloneDXMetadataTools(doc *Document) *cdx.ToolsChoice { + own := make([]sdk.DocumentTool, 0, len(doc.ToolNamesOrDefault())) + for _, name := range doc.ToolNamesOrDefault() { + tool := sdk.DocumentTool{Name: name} + if name == doc.ToolOrDefault() { + tool.Version = doc.ToolVersion } - seen[key] = struct{}{} + own = append(own, tool) + } + merged := sdk.MergeDocumentAssertions( + sdk.DocumentAssertions{Tools: own}, + sdk.DocumentAssertions{Tools: doc.Assertions.Tools}, + ) + if len(merged.Tools) == 0 { + return nil + } + components := make([]cdx.Component, 0, len(merged.Tools)) + for _, tool := range merged.Tools { component := cdx.Component{Type: cdx.ComponentTypeApplication, Name: tool.Name, Version: tool.Version} if tool.Vendor != "" { component.Manufacturer = &cdx.OrganizationalEntity{Name: tool.Vendor} } components = append(components, component) } - if len(components) == 0 { - return tools - } return &cdx.ToolsChoice{Components: &components} } diff --git a/internal/sbom/document_assertions.go b/internal/sbom/document_assertions.go index 40289532..e6d204db 100644 --- a/internal/sbom/document_assertions.go +++ b/internal/sbom/document_assertions.go @@ -189,6 +189,13 @@ func (d documentIdentity) names(identity string) bool { // document's bytes -- and DocumentAssertions has nowhere to carry one, so a // merged SPDX export names no sources. Tracked as bomly-dev/bomly-sdk#55; // when that field ships, an SPDX projection belongs here beside this one. +// +// These links are also write-only for now: DocumentAssertions records what a +// document says about itself, with no field for the documents behind it, so +// ingesting a merged export cannot restore them and a second export names no +// sources. Tracked as bomly-dev/bomly-sdk#61. Carrying them through Metadata +// instead is exactly what ADR-0037 closed off, so the gap is documented rather +// than worked around. func documentSourceLinks(doc *Document, emitted documentIdentity) []sdk.ExternalReference { if doc == nil || len(doc.Sources) == 0 { return nil diff --git a/internal/sbom/document_assertions_test.go b/internal/sbom/document_assertions_test.go index 5e5e887a..61123a94 100644 --- a/internal/sbom/document_assertions_test.go +++ b/internal/sbom/document_assertions_test.go @@ -513,3 +513,53 @@ func TestDocumentAssertionsForAlwaysRecordsThatADocumentWasRead(t *testing.T) { t.Error("no document must mean no record") } } + +// A credited tool keeps everything the format can hold. A CycloneDX source +// naming "Acme / cdx-gen / 9.1.0" was reduced to "Tool: cdx-gen" on SPDX +// export, and suppressed entirely on CycloneDX export by a deduplication key +// that ignored the vendor. +func TestCreditedToolsKeepWhatEachFormatCanHold(t *testing.T) { + doc := &Document{ + Tool: defaultToolName, + Tools: []string{defaultToolName}, + ToolVersion: "1.2.3", + Assertions: sdk.DocumentAssertions{Tools: []sdk.DocumentTool{ + {Vendor: "Acme", Name: "cdx-gen", Version: "9.1.0"}, + // Same name and version as Bomly's own entry, but with a vendor + // the source added: the SDK's merge class keys on the whole + // triple, so this is a distinct tool and must not be swallowed. + {Vendor: "Acme", Name: defaultToolName, Version: "1.2.3"}, + }}, + } + + t.Run("spdx renders name and version", func(t *testing.T) { + var found bool + for _, creator := range spdxDocumentCreators(doc) { + found = found || creator.Creator == "cdx-gen-9.1.0" + } + if !found { + t.Errorf("creators = %+v, want the tool's version kept", spdxDocumentCreators(doc)) + } + }) + + t.Run("cyclonedx keeps the vendor", func(t *testing.T) { + tools := cycloneDXMetadataTools(doc) + if tools == nil || tools.Components == nil { + t.Fatal("no tools rendered") + } + var vendored, ownVendored bool + for _, component := range *tools.Components { + if component.Manufacturer == nil { + continue + } + vendored = vendored || (component.Name == "cdx-gen" && component.Manufacturer.Name == "Acme") + ownVendored = ownVendored || (component.Name == defaultToolName && component.Manufacturer.Name == "Acme") + } + if !vendored { + t.Errorf("tools = %+v, want the source's vendor kept", *tools.Components) + } + if !ownVendored { + t.Errorf("a source tool differing only by vendor was suppressed: %+v", *tools.Components) + } + }) +} diff --git a/internal/sbom/spdx23_assertions.go b/internal/sbom/spdx23_assertions.go index f8a658fe..04477844 100644 --- a/internal/sbom/spdx23_assertions.go +++ b/internal/sbom/spdx23_assertions.go @@ -325,6 +325,25 @@ func spdxDocumentAssertions(doc *v23.Document) sdk.DocumentAssertions { return normalized } +// spdxToolCreator renders a tool as SPDX's creator value. +// +// SPDX writes a tool as "name-version", which is the same convention Bomly +// already uses for its own creator line a few lines below, so a source tool +// carrying a version keeps it rather than being reduced to a bare name. +// +// The vendor is dropped, because SPDX models no vendor for a creator and the +// alternatives are worse: folding it into the name produces a string that +// reads back as a tool called "Acme/cdx-gen", which corrupts the name instead +// of preserving the vendor. A claim the format cannot hold is better lost +// than misfiled -- the CycloneDX projection keeps all three fields. +func spdxToolCreator(tool sdk.DocumentTool) string { + name := strings.TrimSpace(tool.Name) + if version := strings.TrimSpace(tool.Version); name != "" && version != "" { + return name + "-" + version + } + return name +} + // spdxDocumentCreators renders the creator lines for a document, folding the // parties and tools the source documents credited in with Bomly's own. // @@ -355,7 +374,7 @@ func spdxDocumentCreators(doc *Document) []common.Creator { add(spdxToolCreatorType, tool) } for _, tool := range doc.Assertions.Tools { - add(spdxToolCreatorType, tool.Name) + add(spdxToolCreatorType, spdxToolCreator(tool)) } if doc.Provenance.Manufacturer != "" { add(spdxOrganizationCreatorType, doc.Provenance.Manufacturer) From 37efbed22103eb3612bfbc6506581b4f063c065c Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Sat, 5 Sep 2026 22:23:59 -0700 Subject: [PATCH 6/6] fix(sbom): keep the BOM revision, the CPE binding, and one author per party A serial names a BOM and the revision names which issue of it, so adopting a source's serial without its revision produced a document claiming to be revision 1 of a BOM whose revision 4 it had converted -- and the self-link check, comparing serials alone, then linked nothing either. Both travel together now, and a different revision of the same serial is correctly a different document. CPE 2.2 and 2.3 are different bindings and the reference type declares which. Every CPE went out as cpe23Type, so an ingested "cpe:/a:..." was republished under the 2.3 type -- a claim the source never made, looking authoritative on the far side. The type is chosen by asking the SDK's own gate which binding accepts the value, the same mechanism that admits these on the way in; 2.3 is tried first so a value valid in both keeps the current form. Configured provenance is written as an author as well as the manufacturer, so re-ingesting Bomly's own output read that name back as a person creator and appended it beside the configured value again -- one extra author per hop, in a flow advertised as a fixed point. The fixed-point test used no provenance, which is why it did not see this; there is one now that does. Co-Authored-By: Claude Opus 5 --- internal/sbom/assertions_round_trip_test.go | 58 ++++++++++++++++ internal/sbom/cyclonedx.go | 1 + internal/sbom/cyclonedx_assertions.go | 26 ++++++- internal/sbom/document_assertions.go | 10 ++- internal/sbom/document_assertions_test.go | 75 +++++++++++++++++++++ internal/sbom/model.go | 22 +++++- internal/sbom/spdx23.go | 6 +- internal/sbom/spdx23_assertions.go | 24 +++++++ 8 files changed, 214 insertions(+), 8 deletions(-) diff --git a/internal/sbom/assertions_round_trip_test.go b/internal/sbom/assertions_round_trip_test.go index 136f3b16..a87f3fff 100644 --- a/internal/sbom/assertions_round_trip_test.go +++ b/internal/sbom/assertions_round_trip_test.go @@ -380,3 +380,61 @@ func TestOrganizationOriginatorStillUsesPublisher(t *testing.T) { t.Errorf("originator = %+v, want an organization", got) } } + +// A CPE keeps the binding it was written in. 2.2 and 2.3 are different +// bindings and the reference type declares which; labelling every CPE +// cpe23Type published a 2.2 binding under the 2.3 type, which is a claim the +// source never made and one that looks authoritative on the far side. +func TestCPEKeepsItsBindingThroughSPDX(t *testing.T) { + const cpe22Document = `{ + "spdxVersion": "SPDX-2.3", "dataLicense": "CC0-1.0", "SPDXID": "SPDXRef-DOCUMENT", + "name": "c", "documentNamespace": "https://c.example/spdx/1", + "creationInfo": {"created": "2026-01-01T00:00:00Z", "creators": ["Tool: t"]}, + "packages": [{ + "SPDXID": "SPDXRef-w", "name": "widget", "versionInfo": "1.0.0", + "externalRefs": [ + {"referenceCategory": "PACKAGE-MANAGER", "referenceType": "purl", + "referenceLocator": "pkg:npm/widget@1.0.0"}, + {"referenceCategory": "SECURITY", "referenceType": "cpe22Type", + "referenceLocator": "cpe:/a:widget:widget:1.0.0"} + ] + }] +}` + doc, _, err := UnmarshalAutoJSON([]byte(cpe22Document)) + if err != nil { + t.Fatalf("ingest: %v", err) + } + graph, err := ToGraph(doc) + if err != nil { + t.Fatalf("to graph: %v", err) + } + raw, err := MarshalDepGraphJSON(graph, TargetSPDX23JSON, BuildOptions{}, EncodeOptions{Pretty: true}) + if err != nil { + t.Fatalf("export: %v", err) + } + if !strings.Contains(string(raw), "cpe22Type") { + t.Errorf("a 2.2 binding was not written as cpe22Type:\n%s", raw) + } + if strings.Contains(string(raw), `"cpe23Type"`) { + t.Errorf("a 2.2 binding was published under the 2.3 type:\n%s", raw) + } +} + +// A 2.3 binding still goes out as cpe23Type. +func TestModernCPEStillUsesTheCurrentType(t *testing.T) { + doc, _, err := UnmarshalAutoJSON([]byte(supplierRichCycloneDX)) + if err != nil { + t.Fatalf("ingest: %v", err) + } + graph, err := ToGraph(doc) + if err != nil { + t.Fatalf("to graph: %v", err) + } + raw, err := MarshalDepGraphJSON(graph, TargetSPDX23JSON, BuildOptions{}, EncodeOptions{Pretty: true}) + if err != nil { + t.Fatalf("export: %v", err) + } + if !strings.Contains(string(raw), "cpe23Type") { + t.Errorf("a 2.3 binding was not written as cpe23Type:\n%s", raw) + } +} diff --git a/internal/sbom/cyclonedx.go b/internal/sbom/cyclonedx.go index b3d8d77a..88f9f527 100644 --- a/internal/sbom/cyclonedx.go +++ b/internal/sbom/cyclonedx.go @@ -19,6 +19,7 @@ type cycloneDXCodec struct { func (c cycloneDXCodec) encodeJSON(doc *Document, opts EncodeOptions) ([]byte, error) { bom := cdx.NewBOM() bom.SerialNumber = doc.SerialNumber + bom.Version = doc.SerialVersionOrDefault() components := make([]cdx.Component, 0, len(doc.Components)) for _, comp := range doc.Components { diff --git a/internal/sbom/cyclonedx_assertions.go b/internal/sbom/cyclonedx_assertions.go index 4d0593d2..4bd691ed 100644 --- a/internal/sbom/cyclonedx_assertions.go +++ b/internal/sbom/cyclonedx_assertions.go @@ -420,20 +420,37 @@ func cycloneDXDocumentManufacturer(doc *Document) *cdx.OrganizationalEntity { } // cycloneDXDocumentAuthors renders the people credited with the document. +// +// One entry per party. Configured provenance is written as an author as well +// as the manufacturer, so re-ingesting Bomly's own output reads that name back +// as a person creator -- and appending it beside the configured value again +// grew the author list by one on every hop, which is a fixed point that is not +// fixed. The configured value is kept first so its contact details survive. func cycloneDXDocumentAuthors(doc *Document) []cdx.OrganizationalContact { var authors []cdx.OrganizationalContact + seen := make(map[string]struct{}, len(doc.Assertions.Creators)+1) + add := func(author cdx.OrganizationalContact) { + if author.Name == "" { + return + } + if _, dup := seen[author.Name]; dup { + return + } + seen[author.Name] = struct{}{} + authors = append(authors, author) + } if doc.Provenance.Manufacturer != "" { author := cdx.OrganizationalContact{Name: doc.Provenance.Manufacturer} if email := bareEmail(doc.Provenance.SecurityContact); email != "" { author.Email = email } - authors = append(authors, author) + add(author) } for _, creator := range doc.Assertions.Creators { if creator.Kind != sdk.ContactKindPerson { continue } - authors = append(authors, cdx.OrganizationalContact{Name: creator.Name}) + add(cdx.OrganizationalContact{Name: creator.Name}) } return authors } @@ -479,7 +496,10 @@ func cycloneDXSourceLinks(doc *Document) []cdx.ExternalReference { // CycloneDX writes a serial and no namespace, so that is the only identity // a reader of this document can see, and the only one a source link could // be redundant with. - links := documentSourceLinks(doc, documentIdentity{Serial: doc.SerialNumber}) + links := documentSourceLinks(doc, documentIdentity{ + Serial: doc.SerialNumber, + SerialVersion: doc.SerialVersionOrDefault(), + }) if len(links) == 0 { return nil } diff --git a/internal/sbom/document_assertions.go b/internal/sbom/document_assertions.go index e6d204db..ccac5f9b 100644 --- a/internal/sbom/document_assertions.go +++ b/internal/sbom/document_assertions.go @@ -136,7 +136,10 @@ func inheritDocumentIdentity(doc *Document, identity string) { if err != nil { return } + // The revision travels with the serial. Keeping only the serial made the + // export claim to be revision 1 of a document whose revision 2 it read. doc.SerialNumber = link.SerialNumber() + doc.SerialVersion = link.Version() } // documentIdentity is the identity a format actually writes into the document @@ -149,6 +152,10 @@ func inheritDocumentIdentity(doc *Document, identity string) { type documentIdentity struct { Namespace string Serial string + // SerialVersion is the revision written beside Serial. A link naming a + // different revision of the same serial names a different document, so it + // is still a link and not a self-reference. + SerialVersion int } // names reports whether an identity refers to this same document. @@ -166,7 +173,8 @@ func (d documentIdentity) names(identity string) bool { return true } if d.Serial != "" { - if link, err := cdx.ParseBOMLink(identity); err == nil && link.SerialNumber() == d.Serial { + if link, err := cdx.ParseBOMLink(identity); err == nil && + link.SerialNumber() == d.Serial && link.Version() == d.SerialVersion { return true } } diff --git a/internal/sbom/document_assertions_test.go b/internal/sbom/document_assertions_test.go index 61123a94..bd5ac497 100644 --- a/internal/sbom/document_assertions_test.go +++ b/internal/sbom/document_assertions_test.go @@ -563,3 +563,78 @@ func TestCreditedToolsKeepWhatEachFormatCanHold(t *testing.T) { } }) } + +// A serial names a BOM; the revision names which issue of it. Adopting one +// without the other produced a document claiming to be revision 1 of a BOM +// whose revision 2 it had actually converted -- and, because the self-link +// check compared serials alone, it linked nothing either. +func TestConversionKeepsTheSourceBOMRevision(t *testing.T) { + source := strings.Replace(serialCycloneDX, `"version": 1,`, `"version": 4,`, 1) + _, entry := ingestDocument(t, source) + if entry.Document.Identity != "urn:cdx:3e671687-395b-41f5-a30f-a58921a69b79/4" { + t.Fatalf("identity = %q, want the revision kept", entry.Document.Identity) + } + raw, err := MarshalGraphEntriesJSON(entry.Graph, []sdk.GraphEntry{entry}, TargetCycloneDX16JSON, BuildOptions{}, EncodeOptions{Pretty: true}) + if err != nil { + t.Fatalf("export: %v", err) + } + var bom cdx.BOM + if err := json.Unmarshal(raw, &bom); err != nil { + t.Fatalf("decode: %v", err) + } + if bom.Version != 4 { + t.Errorf("version = %d, want the source's revision", bom.Version) + } + if bom.SerialNumber != "urn:uuid:3e671687-395b-41f5-a30f-a58921a69b79" { + t.Errorf("serial = %q", bom.SerialNumber) + } + // Adopted, so not also linked. + if bom.ExternalReferences != nil { + for _, ref := range *bom.ExternalReferences { + if ref.Type == cdx.ERTypeBOM { + t.Errorf("the document links itself: %+v", ref) + } + } + } +} + +// A different revision of the same serial is a different document, so it is +// linked rather than treated as this document itself. +func TestADifferentRevisionOfTheSameSerialIsStillASource(t *testing.T) { + doc := &Document{ + SerialNumber: "urn:uuid:3e671687-395b-41f5-a30f-a58921a69b79", + SerialVersion: 1, + Sources: []sdk.DocumentAssertions{ + {Identity: "urn:cdx:3e671687-395b-41f5-a30f-a58921a69b79/7"}, + }, + } + links := documentSourceLinks(doc, documentIdentity{Serial: doc.SerialNumber, SerialVersion: doc.SerialVersionOrDefault()}) + if len(links) != 1 { + t.Fatalf("links = %+v, want revision 7 named as a source of revision 1", links) + } +} + +// Bomly's own output re-ingests without growing its author list. Configured +// provenance is written as an author as well as the manufacturer, so the name +// comes back as a person creator and used to be appended a second time -- one +// extra author per hop, in a flow advertised as a fixed point. +func TestConfiguredProvenanceDoesNotDuplicateAuthorsAcrossHops(t *testing.T) { + opts := BuildOptions{ + Provenance: Provenance{Manufacturer: "Operator Ltd"}, + Created: fixedExportTime(), + SerialNumber: "urn:uuid:11111111-2222-4333-8444-555555555555", + } + _, entry := ingestDocument(t, supplierRichCycloneDX) + first, err := MarshalGraphEntriesJSON(entry.Graph, []sdk.GraphEntry{entry}, TargetCycloneDX16JSON, opts, EncodeOptions{Pretty: true}) + if err != nil { + t.Fatalf("first export: %v", err) + } + _, again := ingestDocument(t, string(first)) + second, err := MarshalGraphEntriesJSON(again.Graph, []sdk.GraphEntry{again}, TargetCycloneDX16JSON, opts, EncodeOptions{Pretty: true}) + if err != nil { + t.Fatalf("second export: %v", err) + } + if !bytes.Equal(first, second) { + t.Errorf("a provenance-configured export is not a fixed point.\nfirst:\n%s\nsecond:\n%s", first, second) + } +} diff --git a/internal/sbom/model.go b/internal/sbom/model.go index f73eb090..36e7d124 100644 --- a/internal/sbom/model.go +++ b/internal/sbom/model.go @@ -97,9 +97,15 @@ type Document struct { ToolVersion string Created time.Time SerialNumber string - Provenance Provenance - Lifecycle string - Aggregate string + // SerialVersion is the CycloneDX document revision that goes with + // SerialNumber. A serial names a BOM; the revision names which issue of + // it, so adopting a source's serial without its revision produced a + // document claiming to be revision 1 of a BOM whose revision 2 it had + // actually converted. Zero means unset and encodes as 1. + SerialVersion int + Provenance Provenance + Lifecycle string + Aggregate string Components []Component Dependencies []Dependency @@ -308,6 +314,16 @@ func (d *Document) ToolNamesOrDefault() []string { return []string{d.ToolOrDefault()} } +// SerialVersionOrDefault returns the CycloneDX document revision, defaulting to +// the first revision. CycloneDX numbers a document from 1, and a BOM-Link has +// to name a revision, so there is no "unset" to write. +func (d *Document) SerialVersionOrDefault() int { + if d.SerialVersion > 0 { + return d.SerialVersion + } + return 1 +} + // CreatedOrNow returns the document timestamp in UTC, defaulting to the current time. func (d *Document) CreatedOrNow() time.Time { if !d.Created.IsZero() { diff --git a/internal/sbom/spdx23.go b/internal/sbom/spdx23.go index 0d54a9d1..3b75d7b7 100644 --- a/internal/sbom/spdx23.go +++ b/internal/sbom/spdx23.go @@ -605,9 +605,13 @@ func spdxExternalReferences(component Component) []*v23.PackageExternalReference if cpe == "" { continue } + refType := spdxCPEReferenceType(cpe) + if refType == "" { + continue + } refs = append(refs, &v23.PackageExternalReference{ Category: common.CategorySecurity, - RefType: common.TypeSecurityCPE23Type, + RefType: refType, Locator: cpe, }) } diff --git a/internal/sbom/spdx23_assertions.go b/internal/sbom/spdx23_assertions.go index 04477844..66d7c848 100644 --- a/internal/sbom/spdx23_assertions.go +++ b/internal/sbom/spdx23_assertions.go @@ -120,6 +120,30 @@ func spdxIngestedReferences(refs []*v23.PackageExternalReference) []sdk.External return sdk.MergeExternalReferences(nil, converted) } +// spdxCPEReferenceType names the reference type a CPE actually belongs to. +// +// 2.2 and 2.3 are different bindings, and the type declares which one the +// locator is written in. Labelling every CPE as cpe23Type published a 2.2 +// binding under the 2.3 type -- a claim the source never made, and one that +// survived a same-format round trip looking authoritative. Corrupting a claim +// is worse than dropping it. +// +// The classifier is the SDK's gate, not a prefix test here: it owns both +// grammars, and the same mechanism admits these values on the way in. 2.3 is +// tried first so an identifier valid in both is written in the current form. +func spdxCPEReferenceType(value string) string { + for _, cpeType := range []string{common.TypeSecurityCPE23Type, common.TypeSecurityCPE22Type} { + if _, ok := (sdk.ExternalReference{ + Category: sdk.ExternalReferenceCategorySecurity, + Type: cpeType, + Locator: value, + }).Normalized(); ok { + return cpeType + } + } + return "" +} + // spdxIngestedCPEs reads the CPEs a document stated. // // SPDX 2.3 has no CPE field: a CPE is carried as a SECURITY external