From 23daea3e3a5f9edc1d7875c20bcc2a8b66fb1c3c Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Sun, 6 Sep 2026 13:09:38 -0700 Subject: [PATCH 1/7] feat(sbom): adopt SDK v0.9.5 and close the five limitations it unblocks Five gaps in the SBOM preservation work were documented in code and in docs/SBOM.md with an SDK issue attached to each, because the fix belonged in the shared model rather than here (ADR-0040). SDK v0.9.5 ships all five, so this consumes them and removes the notes. **The description gate is idempotent again (sdk#54).** A gate that repairs invalid UTF-8 and then bounds the result can push a value past its own bound, so the next pass empties it: a description survived one conversion and vanished on the next. `internal/sbom/graph.go` carried a local normalize-until-it-settles loop for exactly that, with an instruction to delete it when the fix shipped. It is gone; the ingest path calls the SDK gates directly, and a regression test pins the fixed-point property with the input that found the defect. **A merged SPDX document names its sources (sdk#55).** SPDX links a document through externalDocumentRefs, and section 6.6 requires a checksum over that document's bytes on every entry -- so ADR-0042 shipped the CycloneDX half and left the SPDX half open. `DocumentAssertions` now carries a document version and a source checksum, and ingest computes that checksum where the original bytes are: once, in the codec entry point, for every format including one added later. It cannot be recovered from the parsed model afterwards, which is why it has to be captured there. **A source document's own scope word survives (sdk#57).** A component a CycloneDX document marked `optional` imported as runtime and re-exported as `required` -- a claim about shipping code that the source deliberately had not made -- because the model had nowhere to keep a source-asserted scope beside the derived set. `DependencyNode.SourceScope` is that slot, and `CycloneDXScopeForExport` decides when the word is re-emitted and when the projection is. That decision stays the SDK's: it is the same mapping that read the word in, and a second copy here is how the two directions came to disagree before. **Source links are read back (sdk#61).** They were write-only: an export wrote them and ingest read nothing, so converting a merged document again produced one that claimed to be built from nothing. `DocumentAssertions.Sources` gives them a home, both codecs read and re-emit them, and each source contributes its own link tuple plus the tuples it recorded -- the SDK's declared merge class for the set, not a rule re-decided here. The CycloneDX `bom` reference now carries the checksum too, so a merged CycloneDX document converted to SPDX can still name every source. **An unreadable scope token no longer unscopes a component (sdk#64).** The strict decode was a forward-compatibility trap: one token a newer Bomly wrote made an older one drop the whole assertion, and SPDX has no native scalar to fall back on, so the loss there was total. The lenient read keeps the scopes this build knows and reports the rest; the SBOM detector turns them into a warning naming the file, which is the channel the ingest path has -- the codec has no logger and the SDK deliberately does not log. Delegation check: every rule here is the SDK's or a pinned library's -- the scope vocabulary and carrier, the link tuple and its gates, folding and bounding, the BOM-Link grammar (cyclonedx-go), the digest registry and its SPDX spelling, and tools-golang's `DocumentRef-` prefixing. One decline is recorded in the code: nothing mints an SPDX idstring from a document identity, so the reference id reuses this package's existing package-id rule with its collision suffix. Co-Authored-By: Claude Opus 5 --- dev-docs/SECURITY_ASSURANCE.md | 2 +- ...es-its-source-a-merge-links-its-sources.md | 24 ++ docs/SBOM.md | 54 +-- go.mod | 2 +- go.sum | 4 +- internal/detectors/sbom/detector.go | 13 + internal/detectors/sbom/detector_test.go | 88 +++++ internal/sbom/codec.go | 38 ++- internal/sbom/codec_fuzz_test.go | 7 + internal/sbom/cyclonedx.go | 79 +++-- internal/sbom/cyclonedx_assertions.go | 82 ++++- internal/sbom/document_assertions.go | 65 ++-- .../sbom/document_assertions_fuzz_test.go | 10 + internal/sbom/document_assertions_test.go | 23 +- internal/sbom/document_sources_test.go | 318 ++++++++++++++++++ internal/sbom/graph.go | 34 +- internal/sbom/graph_test.go | 54 +++ internal/sbom/model.go | 39 ++- internal/sbom/scope_carrier.go | 63 ++++ internal/sbom/scope_round_trip_test.go | 165 +++++++++ internal/sbom/spdx23.go | 63 ++-- internal/sbom/spdx23_assertions.go | 101 ++++++ internal/sbom/transform.go | 1 + 23 files changed, 1174 insertions(+), 155 deletions(-) create mode 100644 internal/sbom/document_sources_test.go create mode 100644 internal/sbom/scope_carrier.go diff --git a/dev-docs/SECURITY_ASSURANCE.md b/dev-docs/SECURITY_ASSURANCE.md index fcebcb78..fa43e06b 100644 --- a/dev-docs/SECURITY_ASSURANCE.md +++ b/dev-docs/SECURITY_ASSURANCE.md @@ -39,7 +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. SBOM ingest additionally refuses a document without a single unambiguous reading -- a repeated object member name or invalid UTF-8 (ADR-0039) -- with an error naming the member path or byte offset; the guarantee is exactly those two classes, and the check validates rather than re-decodes so no other parsing behavior changes. Duplicate detection retains an object's member names until that object closes, so the cost is set by every object open at once; the check is bounded by refusing a document that would hold more than 100,000 member names, or more than 16 MiB of them, simultaneously -- failing closed, since skipping the check on the largest inputs would place the gap where a crafted document would aim. The preflight also reads the document in place through a `*bytes.Buffer` rather than a generic reader, which would grow a buffer of its own and materialize each token into it -- a single 150 MiB string value retained 447 MiB that way and nothing at all in place; the name bounds do not reach values, so this is handled by not copying them. Both name bounds were needed and each was found by measurement rather than reasoning: bounding only the widest single object missed nested objects that accumulate (2.5 GiB from a 411 MiB document), and bounding names by count alone missed long names (900 MiB from a 194 MiB document, now 116 MiB and refused). An oversized name is refused on its source span before it is read, because reading one costs a copy of it -- measured at a full extra copy, escaped or not, on top of what the decoder retains; the span gates, the name accounts, and using either for both was wrong in a different direction each time. The strict reader is confined to `internal/sbom` by a guard test, so the trusted plugin wire cannot be tightened by a decoder swap. | `TestSDKContractReadLimitEnforcesBoundsAndGrowth`, `TestLoadFileRejectsOversizedFile`, `TestDetectorResolveGraph_RejectsOversizedSBOM`, `FuzzLoadFile`, `FuzzUnmarshalAutoJSON`, `TestIngestRejectsDuplicateObjectNames`, `TestIngestRejectsNestedDuplicateObjectNames`, `TestIngestRejectsInvalidUTF8`, `TestStrictIngestDoesNotRejectMerelyUnusualDocuments`, `TestBomlyOutputPassesTheStrictGate`, `TestStrictJSONStaysInSBOMIngest`, `TestADocumentTooLargeToCheckIsRefused`, `TestNestedObjectsCannotAccumulatePastTheBound`, `TestSiblingObjectsDoNotAccumulate`, `TestLongMemberNamesAreBoundedByTheirBytes`, `TestClosedObjectsReleaseTheirNameBytes`, `TestOrdinaryNamesDoNotApproachTheByteBound`, `TestStrictPreflightReadsInPlace`, `TestAnOversizedNameIsRefusedBeforeItIsRead`, `TestManyComponentsDoNotTripTheWidthBound`, `TestAmbiguityIsReportedBeforeFormatDetection` | A user-selected file can still consume work up to its limit. Case-variant field matching and other reader-divergence classes are outside the stated guarantee. | -| 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). | +| 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 -- bomly-dev/bomly-sdk#54 was one, and the local normalize-until-it-settles workaround it forced is gone since SDK v0.9.5. A merged export now names its sources in both formats, so a source identity and a source checksum are two more published values, each re-gated on export. | | 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 index 893cd525..35fafb02 100644 --- 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 @@ -80,3 +80,27 @@ written straight onto one never passed a decoder. Everything runs through 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. + +## Update (2026-09-06): both gaps this ADR left open are closed + +Two consequences above were open questions with SDK issues attached. SDK +v0.9.5 closed both, and this repository consumes them. + +**A merged SPDX document now links its sources.** The blocker was that +`DocumentAssertions` had nowhere to keep a checksum over a source document's +bytes, which SPDX requires on every `externalDocumentRefs` entry +(bomly-dev/bomly-sdk#55). The carrier grew a document version and a source +checksum, and ingest computes that checksum where the original bytes are -- +in the codec entry point, once, for every format including one added later. +The SPDX projection now sits beside the CycloneDX one. A source that reached +a graph entry without passing through ingest has no checksum and is left +unnamed rather than written as an invalid reference. + +**Those links are read back.** `DocumentAssertions.Sources` +(bomly-dev/bomly-sdk#61) gives the documents behind a document a home, so +both codecs read the links on ingest and re-emit them on export. A merged +export converted again still names its inputs. Each source contributes its +own link tuple and the tuples it recorded, which is the SDK's declared merge +class for the set -- inheritance, not a rule re-decided here. The CycloneDX +`bom` reference carries the checksum too, so converting a merged CycloneDX +document to SPDX can still name every source. diff --git a/docs/SBOM.md b/docs/SBOM.md index 9c82c516..6037941a 100644 --- a/docs/SBOM.md +++ b/docs/SBOM.md @@ -301,11 +301,22 @@ Every generated document carries a stable identity: 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 + name a document that is not this one — and *links* each source. CycloneDX + writes a document-level external reference of type `bom`, carrying a + BOM-Link (`urn:cdx:/`) for a CycloneDX source or the + namespace URI for an SPDX one. SPDX writes an `externalDocumentRefs` + entry naming the same identity, with a SHA-256 checksum over the source + document's bytes, which the specification requires on every entry; the + CycloneDX reference carries that checksum too, so the link stays usable + if the merged document is later converted to SPDX. The checksum is + computed while the source document is being read, because it cannot be + recovered from the parsed model afterwards — a source that reached Bomly + without those bytes is left unnamed in SPDX rather than written as an + invalid reference. People and tools credited by any source are credited by the merged document too. + - Source links are read back on import, so provenance survives more than one + conversion: converting a merged document again produces a document that + still names the documents behind it, in either format. - 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. @@ -394,11 +405,21 @@ Some information necessarily becomes less specific during conversion: exactly, and a document from any other producer still yields a usable scope. Reading the other way, CycloneDX `required` and `optional` both become runtime and `excluded` becomes development. -- A source document's own scope word is not preserved verbatim. A component - a CycloneDX document marked `optional` imports as runtime and re-exports as - `required`, because Bomly's model has no slot for a source-asserted scope - beside the set it derives (tracked as bomly-dev/bomly-sdk#57). Bomly's own - documents are unaffected: their scope sets survive through the carrier. +- A scope carrier naming a token this build does not recognize keeps the + scopes it does recognize. The carrier is Bomly's own, so an unreadable token + is almost always one a newer Bomly wrote; refusing the whole value would have + left the component unscoped, which in SPDX — where there is no native scope + field to fall back on — meant losing the scope entirely. The tokens that were + not read are reported as a warning naming the file. +- A source document's own scope word is preserved and written back. A + component a CycloneDX document marked `optional` re-exports as `optional`, + not as Bomly's projection of the set it derives — so `optional` and + `excluded` do not collapse into `required` across a round trip that asserted + neither. The word yields to the projection when Bomly's own scope set stops + meaning what the word meant, for example once the package turns out to be + reachable from a development root as well. SPDX 2.3 has no scope field, so + the word is carried only through a CycloneDX export; an SPDX document still + carries the full set in its package comment. - 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 @@ -421,17 +442,10 @@ 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. -- 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. +- A conversion is a fixed point within a format, not across one. A CycloneDX + serial can hold only a UUID URN, so an SPDX source converted to CycloneDX is + linked rather than adopted, and converting back produces a different + document. See "Document identity" above for how sources are named. - 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/go.mod b/go.mod index 4161d651..c0ef5451 100644 --- a/go.mod +++ b/go.mod @@ -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.3 + github.com/bomly-dev/bomly-sdk v0.9.5 github.com/charmbracelet/bubbles v1.0.0 github.com/charmbracelet/bubbletea v1.3.10 github.com/charmbracelet/lipgloss v1.1.0 diff --git a/go.sum b/go.sum index 5b353842..98f0dad1 100644 --- a/go.sum +++ b/go.sum @@ -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.3 h1:ynj+EDxdSZHwZaFmhNAFMAQJwQEgdW5JVrAm3kYgNBs= -github.com/bomly-dev/bomly-sdk v0.9.3/go.mod h1:gAGOEa88BE8gELt2rM/ZrqGIJnOzXiR6ocuuKW6AaJ0= +github.com/bomly-dev/bomly-sdk v0.9.5 h1:T4ivCbedRShbuvPUx0J4be+jug0dLRnX06BifHdZZyk= +github.com/bomly-dev/bomly-sdk v0.9.5/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= diff --git a/internal/detectors/sbom/detector.go b/internal/detectors/sbom/detector.go index 613fd5b3..914e9edb 100644 --- a/internal/detectors/sbom/detector.go +++ b/internal/detectors/sbom/detector.go @@ -105,6 +105,19 @@ func (d Detector) ResolveGraph(_ context.Context, req sdk.DetectionRequest) (sdk } } + // A token in Bomly's own scope carrier that this build cannot read is + // almost always one a newer Bomly wrote. The scopes beside it are kept -- + // the SDK reads the carrier leniently -- so the graph is sound and the + // scan continues; what a user needs to know is that this binary is + // reading a document written by a later one, because that is the thing + // they can act on. This logger is the channel the ingest path has: the + // codec has none, and the SDK deliberately does not log. + if len(doc.UnknownScopeTokens) > 0 { + logger.Warn(fmt.Sprintf("sbom: ignored %d unrecognized scope token(s) in %q; a newer Bomly may have written them", + len(doc.UnknownScopeTokens), sbomPath), + zap.Strings("tokens", doc.UnknownScopeTokens)) + } + depsGraph, err := sbom.ToGraph(doc) if err != nil { return sdk.DetectionResult{}, fmt.Errorf("convert sbom %q to graph: %w", sbomPath, err) diff --git a/internal/detectors/sbom/detector_test.go b/internal/detectors/sbom/detector_test.go index d242f12f..d49e3217 100644 --- a/internal/detectors/sbom/detector_test.go +++ b/internal/detectors/sbom/detector_test.go @@ -3,6 +3,7 @@ package sbom import ( "context" "errors" + "fmt" "os" "path/filepath" "strings" @@ -13,6 +14,8 @@ import ( "github.com/bomly-dev/bomly-cli/internal/testnodes" "github.com/bomly-dev/bomly-sdk" "github.com/bomly-dev/bomly-sdk/system" + "go.uber.org/zap" + "go.uber.org/zap/zaptest/observer" ) func TestDetectorResolveGraph_SPDXJSON(t *testing.T) { @@ -249,3 +252,88 @@ func verifyResolvedGraph(t *testing.T, result sdk.DetectionResult, wantDependenc } t.Fatalf("expected graph to contain stable package id %q, got %s", wantDependencyID, g.PrettyString()) } + +// A carrier token this build cannot read reaches the user as a warning, and +// the scopes beside it still reach the graph. +// +// The SDK reads the carrier leniently so an older Bomly does not lose every +// scope to one token a newer one wrote (bomly-dev/bomly-sdk#64). Dropping the +// token silently would leave a user with no way to learn that the document was +// written by a later build; this detector's logger is the channel the ingest +// path has, since the codec has none and the SDK deliberately does not log. +func TestDetectorWarnsAboutUnreadableScopeTokens(t *testing.T) { + raw := `{ + "spdxVersion": "SPDX-2.3", + "dataLicense": "CC0-1.0", + "SPDXID": "SPDXRef-DOCUMENT", + "name": "n", + "documentNamespace": "https://acme.example/spdx/n", + "creationInfo": {"created": "2026-01-02T03:04:05Z", "creators": ["Tool: t"]}, + "packages": [ + { + "SPDXID": "SPDXRef-widget", + "name": "widget", + "versionInfo": "1.0.0", + "comment": "bomly:scope=runtime,future-scope", + "externalRefs": [ + {"referenceCategory": "PACKAGE-MANAGER", "referenceType": "purl", "referenceLocator": "pkg:npm/widget@1.0.0"} + ] + } + ] +}` + path := filepath.Join(t.TempDir(), "carrier.spdx.json") + if err := os.WriteFile(path, []byte(raw), 0o644); err != nil { + t.Fatalf("write fixture: %v", err) + } + + core, logs := observer.New(zap.WarnLevel) + detector := Detector{Logger: zap.New(core)} + result, err := detector.ResolveGraph(context.Background(), requestForSBOMPath(path)) + if err != nil { + t.Fatalf("ResolveGraph() error = %v", err) + } + + entries := logs.All() + if len(entries) != 1 { + t.Fatalf("warnings = %+v, want exactly one", entries) + } + if !strings.Contains(entries[0].Message, "scope token") { + t.Errorf("warning = %q, want it to name the unreadable tokens", entries[0].Message) + } + var named bool + for _, field := range entries[0].Context { + if field.Key == "tokens" && strings.Contains(fmt.Sprint(field.Interface), "future-scope") { + named = true + } + } + if !named { + t.Errorf("warning fields = %+v, want the token itself", entries[0].Context) + } + + // The scope this build can read still reached the graph: the warning is + // about what was dropped, not about what was kept. + g, err := result.ConsolidatedGraph() + if err != nil { + t.Fatalf("ConsolidatedGraph() error = %v", err) + } + nodes := g.DependencyNodes() + if len(nodes) != 1 { + t.Fatalf("nodes = %d, want the component", len(nodes)) + } + if len(nodes[0].Scopes) != 1 || nodes[0].Scopes[0] != sdk.ScopeRuntime { + t.Errorf("scopes = %v, want runtime", nodes[0].Scopes) + } +} + +// A document with a carrier this build reads completely warns about nothing. +func TestDetectorStaysQuietForAReadableCarrier(t *testing.T) { + path := writeSBOMFixture(t, sbom.TargetSPDX23JSON) + core, logs := observer.New(zap.WarnLevel) + detector := Detector{Logger: zap.New(core)} + if _, err := detector.ResolveGraph(context.Background(), requestForSBOMPath(path)); err != nil { + t.Fatalf("ResolveGraph() error = %v", err) + } + if entries := logs.All(); len(entries) != 0 { + t.Errorf("warnings = %+v, want none", entries) + } +} diff --git a/internal/sbom/codec.go b/internal/sbom/codec.go index 8feaa5e6..5e4082a4 100644 --- a/internal/sbom/codec.go +++ b/internal/sbom/codec.go @@ -2,6 +2,8 @@ package sbom import ( "bytes" + "crypto/sha256" + "encoding/hex" "encoding/json" "errors" "fmt" @@ -54,7 +56,7 @@ func UnmarshalJSON(data []byte, target Target) (*Document, error) { if err := requireUnambiguousJSON(data); err != nil { return nil, err } - return c.decodeJSON(data) + return decodeDocument(c, data) } // unmarshalValidated decodes a document whose bytes the caller has already @@ -64,7 +66,39 @@ func unmarshalValidated(data []byte, target Target) (*Document, error) { if !ok { return nil, fmt.Errorf("%w: %s", ErrUnsupportedTarget, target) } - return c.decodeJSON(data) + return decodeDocument(c, data) +} + +// decodeDocument runs a codec and stamps the document with a checksum over the +// bytes it was decoded from. +// +// Every ingest path goes through here, which is the point: the checksum can +// only be computed while the original bytes are in hand, and it cannot be +// recovered from the parsed model afterwards. An SPDX externalDocumentRef is +// invalid without one, so a merged SPDX export that has to name its sources +// has exactly one chance to capture it -- here, for every format, including +// one added later (ADR-0037). +// +// SHA-256 because both formats define it and both validators accept it; the +// spelling each writes is the SDK's to render, not this package's. +func decodeDocument(c codec, data []byte) (*Document, error) { + doc, err := c.decodeJSON(data) + if err != nil { + return nil, err + } + if doc == nil { + return nil, ErrNilDocument + } + sum := sha256.Sum256(data) + // The gate runs here rather than at the export site, so a checksum that + // could not be published never reaches the model at all. + if checksum, ok := (sdk.Digest{ + Algorithm: sdk.DigestAlgorithmSHA256, + Value: hex.EncodeToString(sum[:]), + }).Normalized(); ok { + doc.Assertions.Checksum = &checksum + } + return doc, nil } // DetectJSONTarget identifies the supported SBOM JSON format represented by data. diff --git a/internal/sbom/codec_fuzz_test.go b/internal/sbom/codec_fuzz_test.go index 21e9b6ef..b234316c 100644 --- a/internal/sbom/codec_fuzz_test.go +++ b/internal/sbom/codec_fuzz_test.go @@ -16,6 +16,13 @@ func FuzzUnmarshalAutoJSON(f *testing.F) { `{"bomFormat":"CycloneDX","specVersion":"1.5","version":1,"components":[]}`, `{"bomFormat":"CycloneDX","specVersion":"1.6","version":1,"components":[]}`, `{"artifacts":[],"artifactRelationships":[],"source":{"type":"directory","target":"."},"descriptor":{"name":"syft","version":"seed"},"schema":{"version":"16.0.34","url":"https://raw.githubusercontent.com/anchore/syft/main/schema/json/schema-16.0.34.json"}}`, + // Documents that name their own sources, so the fuzzer reaches the + // link read-back: an SPDX externalDocumentRef and a CycloneDX + // reference of type "bom", each with and without a usable checksum. + `{"spdxVersion":"SPDX-2.3","SPDXID":"SPDXRef-DOCUMENT","name":"demo","documentNamespace":"https://example.com/spdx/demo","externalDocumentRefs":[{"externalDocumentId":"DocumentRef-a","spdxDocument":"https://example.com/spdx/a","checksum":{"algorithm":"SHA256","checksumValue":"0000000000000000000000000000000000000000000000000000000000000000"}}],"creationInfo":{"created":"2026-01-01T00:00:00Z","creators":["Tool: bomly-fuzz"]},"packages":[]}`, + `{"spdxVersion":"SPDX-2.3","SPDXID":"SPDXRef-DOCUMENT","name":"demo","documentNamespace":"https://example.com/spdx/demo","externalDocumentRefs":[{"externalDocumentId":"DocumentRef-a","spdxDocument":"../../etc/passwd","checksum":{"algorithm":"NOPE","checksumValue":""}}],"packages":[]}`, + `{"bomFormat":"CycloneDX","specVersion":"1.6","version":1,"serialNumber":"urn:uuid:3e671687-395b-41f5-a30f-a58921a69b79","externalReferences":[{"type":"bom","url":"urn:cdx:3e671687-395b-41f5-a30f-a58921a69b79/2","hashes":[{"alg":"SHA-256","content":"0000000000000000000000000000000000000000000000000000000000000000"}]}],"components":[]}`, + `{"bomFormat":"CycloneDX","specVersion":"1.6","version":1,"externalReferences":[{"type":"bom","url":"file:///etc/passwd"}],"components":[{"bom-ref":"a","name":"a","version":"1","purl":"pkg:npm/a@1","scope":"optional","properties":[{"name":"bomly:scopes","value":"runtime,future"}]}]}`, // Malformed inputs: rejection paths must be deterministic, never panic. ``, `{}`, diff --git a/internal/sbom/cyclonedx.go b/internal/sbom/cyclonedx.go index a0d48568..33893141 100644 --- a/internal/sbom/cyclonedx.go +++ b/internal/sbom/cyclonedx.go @@ -109,18 +109,25 @@ func (c cycloneDXCodec) decodeJSON(data []byte) (*Document, error) { } componentByID := make(map[string]Component) + var unknownScopes []string if bom.Components != nil { for _, comp := range *bom.Components { + unknownScopes = mergeUnknownScopeTokens(unknownScopes, unknownScopeTokens(cycloneDXCarriedScopes(comp.Properties))) component := Component{ - ID: comp.BOMRef, - Name: comp.Name, - Org: comp.Group, - Type: string(comp.Type), - Scopes: sdk.ScopesFromCycloneDXComponent(string(comp.Scope), cycloneDXCarriedScopes(comp.Properties)), - Version: comp.Version, - PURL: comp.PackageURL, - Copyright: comp.Copyright, - Licenses: parseCycloneDXLicenses(comp.Licenses), + ID: comp.BOMRef, + Name: comp.Name, + Org: comp.Group, + Type: string(comp.Type), + Scopes: sdk.ScopesFromCycloneDXComponent(string(comp.Scope), cycloneDXCarriedScopes(comp.Properties)), + // The word beside the set it derives, so an export can say + // what this document said rather than Bomly's projection of + // it. Gated by the SDK, which is also what refuses a value + // that is not a scope word at all. + SourceScope: sdk.NormalizeSourceScope(string(comp.Scope)), + Version: comp.Version, + PURL: comp.PackageURL, + Copyright: comp.Copyright, + Licenses: parseCycloneDXLicenses(comp.Licenses), } applyCycloneDXAssertions(&component, comp) componentByID[comp.BOMRef] = component @@ -161,16 +168,18 @@ func (c cycloneDXCodec) decodeJSON(data []byte) (*Document, error) { if len(componentByID) == 0 && bom.Metadata != nil && bom.Metadata.Component != nil { root := bom.Metadata.Component + unknownScopes = mergeUnknownScopeTokens(unknownScopes, unknownScopeTokens(cycloneDXCarriedScopes(root.Properties))) component := Component{ - ID: root.BOMRef, - Name: root.Name, - Org: root.Group, - Type: string(root.Type), - Scopes: sdk.ScopesFromCycloneDXComponent(string(root.Scope), cycloneDXCarriedScopes(root.Properties)), - Version: root.Version, - PURL: root.PackageURL, - Copyright: root.Copyright, - Licenses: parseCycloneDXLicenses(root.Licenses), + ID: root.BOMRef, + Name: root.Name, + Org: root.Group, + Type: string(root.Type), + Scopes: sdk.ScopesFromCycloneDXComponent(string(root.Scope), cycloneDXCarriedScopes(root.Properties)), + SourceScope: sdk.NormalizeSourceScope(string(root.Scope)), + Version: root.Version, + PURL: root.PackageURL, + 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 @@ -210,15 +219,16 @@ 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, - SerialNumber: bom.SerialNumber, - Components: components, - Dependencies: dependencies, - Roots: roots, + Name: defaultDocumentName, + Assertions: cycloneDXDocumentAssertions(bom), + Tool: cycloneDXPrimaryToolName(bom.Metadata), + Tools: cycloneDXToolNames(bom.Metadata), + Created: created, + SerialNumber: bom.SerialNumber, + Components: components, + Dependencies: dependencies, + Roots: roots, + UnknownScopeTokens: unknownScopes, }, nil } @@ -442,11 +452,16 @@ func toCycloneDXVersion(target Target) cdx.SpecVersion { // the document describes a package the same way wherever it appears. func cycloneDXComponent(comp Component) cdx.Component { component := cdx.Component{ - BOMRef: comp.ID, - Type: cycloneDXComponentType(comp.Type), - Name: comp.NameOrID(), - Group: comp.Org, - Scope: cdx.Scope(sdk.CycloneDXScope(comp.Scopes)), + BOMRef: comp.ID, + Type: cycloneDXComponentType(comp.Type), + Name: comp.NameOrID(), + Group: comp.Org, + // The source document's own word when Bomly's scope set still means + // what that word meant, and the projection of the set otherwise. The + // SDK owns that decision: it is the same mapping that read the word + // in, and only it can say whether the word still describes the set + // (ADR-0037). + Scope: cdx.Scope(sdk.CycloneDXScopeForExport(comp.Scopes, comp.SourceScope)), Version: comp.Version, PackageURL: comp.PURL, Copyright: comp.Copyright, diff --git a/internal/sbom/cyclonedx_assertions.go b/internal/sbom/cyclonedx_assertions.go index 4bd691ed..0e65bc15 100644 --- a/internal/sbom/cyclonedx_assertions.go +++ b/internal/sbom/cyclonedx_assertions.go @@ -328,6 +328,14 @@ func cycloneDXDocumentAssertions(bom *cdx.BOM) sdk.DocumentAssertions { if link, err := cdx.NewBOMLink(bom.SerialNumber, version, nil); err == nil { assertions.Identity = link.String() } + // The version the document stated, not the default applied above: zero + // means the source numbered nothing, and a BOM-Link identity's tail says + // so anyway. Writing the default here would put a version onto every + // document that never claimed one. + assertions.Version = bom.Version + // What this document says it was built from, so a second conversion can + // still name them. + assertions.Sources = cycloneDXIngestedSources(bom.ExternalReferences) if bom.Metadata != nil { assertions.Created = bom.Metadata.Timestamp if bom.Metadata.Manufacturer != nil { @@ -492,6 +500,13 @@ func cycloneDXMetadataTools(doc *Document) *cdx.ToolsChoice { // cycloneDXSourceLinks renders the links naming the documents this one was // built from, as external references of type "bom" on the document itself. +// +// The checksum rides along in the reference's hashes. CycloneDX does not +// require one, but SPDX's externalDocumentRef does, and a merged CycloneDX +// document converted to SPDX has nowhere else to recover it from -- so +// dropping it here would make the provenance survive one format and die on the +// next hop. The category-free reference gate is the SDK's; the digest gate +// runs again on the way out for the reason every export re-gates. 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 @@ -505,10 +520,69 @@ func cycloneDXSourceLinks(doc *Document) []cdx.ExternalReference { } refs := make([]cdx.ExternalReference, 0, len(links)) for _, link := range links { - refs = append(refs, cdx.ExternalReference{ - Type: cdx.ExternalReferenceType(link.Type), - URL: link.Locator, - }) + // Category stays unset: this is CycloneDX's axis, and the reference + // is written from the link tuple rather than from a stored reference. + ref, ok := sdk.ExternalReference{ + Type: string(cdx.ERTypeBOM), + Locator: link.Identity, + }.Normalized() + if !ok { + continue + } + emitted := cdx.ExternalReference{ + Type: cdx.ExternalReferenceType(ref.Type), + URL: ref.Locator, + } + if link.Checksum != nil { + emitted.Hashes = cycloneDXEmittedHashes([]sdk.Digest{*link.Checksum}) + } + refs = append(refs, emitted) + } + if len(refs) == 0 { + return nil } return refs } + +// cycloneDXIngestedSources reads the documents a CycloneDX document says it +// was built from: its own external references of type "bom". +// +// These were write-only until now -- the export wrote them and nothing read +// them back, so converting a merged document again produced one that named no +// sources at all (bomly-dev/bomly-sdk#61). The reference type is the +// library's constant, and every field is gated by DocumentSource.Normalized +// when the parent record is normalized, so a reference naming nothing +// publishable drops rather than becoming an empty link. +func cycloneDXIngestedSources(refs *[]cdx.ExternalReference) []sdk.DocumentSource { + if refs == nil { + return nil + } + sources := make([]sdk.DocumentSource, 0, len(*refs)) + for _, ref := range *refs { + if !strings.EqualFold(strings.TrimSpace(string(ref.Type)), string(cdx.ERTypeBOM)) { + continue + } + source := sdk.DocumentSource{Identity: ref.URL} + // The first hash that clears the digest gate. A reference may carry + // several; the record holds one, and the SPDX projection it feeds has + // one slot too. + if ref.Hashes != nil { + for _, hash := range *ref.Hashes { + checksum, ok := (sdk.Digest{ + Algorithm: sdk.DigestAlgorithm(hash.Algorithm), + Value: hash.Value, + }).Normalized() + if !ok { + continue + } + source.Checksum = &checksum + break + } + } + sources = append(sources, source) + } + if len(sources) == 0 { + return nil + } + return sources +} diff --git a/internal/sbom/document_assertions.go b/internal/sbom/document_assertions.go index ccac5f9b..109512f4 100644 --- a/internal/sbom/document_assertions.go +++ b/internal/sbom/document_assertions.go @@ -181,9 +181,9 @@ func (d documentIdentity) names(identity string) bool { 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 the identity it is about to write. +// documentSourceLinks returns the link tuples naming each source document this +// one was built from, for the sources whose identity this document did not +// adopt as the identity it is about to write. // // The comparison is against what the format emits, not against the model's // namespace field. Converting an SPDX source to CycloneDX adopts the source @@ -192,41 +192,48 @@ func (d documentIdentity) names(identity string) bool { // 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. +// Each source contributes two things: its own link tuple, and the tuples it +// recorded for the documents *it* was built from. That second half is what +// makes provenance survive more than one hop, and it is the SDK's declared +// merge class for the set rather than a rule invented here -- a document +// built from a merged document inherits that document's sources beside its +// own identity. Without it a merged export converted again named nothing: the +// links were write-only, which is what bomly-dev/bomly-sdk#61 recorded. // -// 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 { +// The result is folded, gated, sorted and bounded by the SDK, by handing the +// candidates back to DocumentAssertions.Normalized. Doing it here would be a +// second copy of the set's key, its self-reference rule and its bound -- the +// three things that decide whether the merge stays associative. +// +// Both formats render these now. CycloneDX writes an external reference of +// type "bom"; SPDX writes an externalDocumentRef, which requires a checksum +// over the source document's bytes -- captured at ingest by decodeDocument, so +// a source that arrived without one is skipped there rather than written as an +// invalid reference. +func documentSourceLinks(doc *Document, emitted documentIdentity) []sdk.DocumentSource { if doc == nil || len(doc.Sources) == 0 { return nil } - links := make([]sdk.ExternalReference, 0, len(doc.Sources)) + candidates := make([]sdk.DocumentSource, 0, len(doc.Sources)*2) for _, source := range doc.Sources { - if emitted.names(source.Identity) || source.Identity == "" { - 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 { + candidates = append(candidates, sdk.DocumentSource{ + Identity: source.Identity, + Version: source.Version, + Checksum: source.Checksum, + }) + candidates = append(candidates, source.Sources...) + } + folded, _ := sdk.DocumentAssertions{Sources: candidates}.Normalized() + + links := make([]sdk.DocumentSource, 0, len(folded.Sources)) + for _, source := range folded.Sources { + if emitted.names(source.Identity) { continue } - links = append(links, ref) + links = append(links, source) } if len(links) == 0 { return nil } - return sdk.MergeExternalReferences(nil, links) + return links } diff --git a/internal/sbom/document_assertions_fuzz_test.go b/internal/sbom/document_assertions_fuzz_test.go index abd57198..5313beaa 100644 --- a/internal/sbom/document_assertions_fuzz_test.go +++ b/internal/sbom/document_assertions_fuzz_test.go @@ -44,6 +44,16 @@ func FuzzDocumentAssertions(f *testing.F) { Creators: []sdk.Contact{{Kind: sdk.ContactKindOrganization, Name: creator}}, Tools: []sdk.DocumentTool{{Name: tool}}, Comment: comment, + // The documents this one claims to be built from, asserted just + // as hostilely: they become external references and SPDX + // externalDocumentRefs on export, so an identity that is a local + // path or a digest that is not one must not reach a document. + // The self-reference -- the same identity this record claims -- + // is the cycle the SDK's gate drops. + Sources: []sdk.DocumentSource{ + {Identity: identity}, + {Identity: name, Checksum: &sdk.Digest{Algorithm: sdk.DigestAlgorithm(dataLicense), Value: tool}}, + }, } g := mustFuzzGraph(t) diff --git a/internal/sbom/document_assertions_test.go b/internal/sbom/document_assertions_test.go index bd5ac497..9001db54 100644 --- a/internal/sbom/document_assertions_test.go +++ b/internal/sbom/document_assertions_test.go @@ -250,9 +250,15 @@ func TestMergedExportLinksItsSourcesInsteadOfAdoptingOne(t *testing.T) { if len(links) != 2 { t.Fatalf("links = %+v, want one per source", links) } + // Each link is a full tuple, not a bare identity: the checksum is what + // SPDX's externalDocumentRef requires on every entry, and it can only be + // computed while the source's original bytes are in hand. for _, link := range links { - if link.Type != string(cdx.ERTypeBOM) { - t.Errorf("link type = %q, want %q", link.Type, cdx.ERTypeBOM) + if link.Identity == "" { + t.Errorf("link %+v names no document", link) + } + if link.Checksum == nil { + t.Errorf("link %q carries no checksum, so SPDX cannot name it", link.Identity) } } } @@ -497,6 +503,11 @@ func TestConfiguredProvenanceOutranksAnIngestedOrganization(t *testing.T) { // The record exists for a document that asserted nothing, which is what makes // a merge involving such a document read as a merge. +// +// "Asserted nothing" is about the claims the document made: no identity, no +// name, no creators, no tools. The record is no longer empty even then, +// because ingest stamps a checksum over the bytes it read -- the value SPDX's +// externalDocumentRef requires and that nothing downstream can recompute. func TestDocumentAssertionsForAlwaysRecordsThatADocumentWasRead(t *testing.T) { doc, _, err := UnmarshalAutoJSON([]byte(`{"bomFormat":"CycloneDX","specVersion":"1.5","version":1,"components":[]}`)) if err != nil { @@ -506,8 +517,12 @@ func TestDocumentAssertionsForAlwaysRecordsThatADocumentWasRead(t *testing.T) { 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 got.Identity != "" || got.Name != "" || got.DataLicense != "" || got.Comment != "" || + len(got.Creators) != 0 || len(got.Tools) != 0 || len(got.Sources) != 0 { + t.Errorf("assertions = %+v, want no claims of its own", *got) + } + if got.Checksum == nil { + t.Errorf("assertions = %+v, want the ingest-captured checksum", *got) } if DocumentAssertionsFor(nil) != nil { t.Error("no document must mean no record") diff --git a/internal/sbom/document_sources_test.go b/internal/sbom/document_sources_test.go new file mode 100644 index 00000000..ad7be2a6 --- /dev/null +++ b/internal/sbom/document_sources_test.go @@ -0,0 +1,318 @@ +package sbom + +import ( + "crypto/sha256" + "encoding/hex" + "encoding/json" + "strings" + "testing" + + cdx "github.com/CycloneDX/cyclonedx-go" + "github.com/bomly-dev/bomly-sdk" +) + +// mergedExport ingests two documents, merges their graphs, and exports the +// result to one target -- the merge case ADR-0042 defines, where the document +// mints its own identity and links its sources. +func mergedExport(t *testing.T, target Target, raws ...string) ([]byte, []sdk.GraphEntry) { + t.Helper() + entries := make([]sdk.GraphEntry, 0, len(raws)) + merged := sdk.New() + for _, raw := range raws { + _, entry := ingestDocument(t, raw) + entries = append(entries, entry) + if err := sdk.MergeGraph(merged, entry.Graph); err != nil { + t.Fatalf("merge: %v", err) + } + } + raw, err := MarshalGraphEntriesJSON(merged, entries, target, BuildOptions{Created: fixedExportTime()}, EncodeOptions{Pretty: true}) + if err != nil { + t.Fatalf("export: %v", err) + } + return raw, entries +} + +// spdxExternalDocumentRefs reads the externalDocumentRefs off a rendered SPDX +// document, as a consumer would. +func spdxExternalDocumentRefs(t *testing.T, raw []byte) []struct { + ID string `json:"externalDocumentId"` + URI string `json:"spdxDocument"` + Checksum struct { + Algorithm string `json:"algorithm"` + Value string `json:"checksumValue"` + } `json:"checksum"` +} { + t.Helper() + var doc struct { + Namespace string `json:"documentNamespace"` + Refs []struct { + ID string `json:"externalDocumentId"` + URI string `json:"spdxDocument"` + Checksum struct { + Algorithm string `json:"algorithm"` + Value string `json:"checksumValue"` + } `json:"checksum"` + } `json:"externalDocumentRefs"` + } + if err := json.Unmarshal(raw, &doc); err != nil { + t.Fatalf("decode spdx: %v", err) + } + return doc.Refs +} + +// cycloneDXSourceRefs reads the document-level references of type "bom". +func cycloneDXSourceRefs(t *testing.T, raw []byte) []cdx.ExternalReference { + t.Helper() + var bom cdx.BOM + if err := json.Unmarshal(raw, &bom); err != nil { + t.Fatalf("decode cyclonedx: %v", err) + } + if bom.ExternalReferences == nil { + return nil + } + refs := make([]cdx.ExternalReference, 0, len(*bom.ExternalReferences)) + for _, ref := range *bom.ExternalReferences { + if ref.Type == cdx.ERTypeBOM { + refs = append(refs, ref) + } + } + return refs +} + +// A merged SPDX document names the documents it was built from. +// +// It could not before: SPDX links a document through externalDocumentRefs, +// every entry there requires a checksum over that document's bytes, and the +// document carrier had nowhere to hold one -- so the CycloneDX half of +// ADR-0042 shipped and the SPDX half was left open as +// bomly-dev/bomly-sdk#55. The checksum is captured at ingest, which is the +// only moment those bytes exist. +func TestMergedSPDXExportNamesItsSources(t *testing.T) { + raw, _ := mergedExport(t, TargetSPDX23JSON, documentRichSPDX, serialCycloneDX) + + var doc struct { + Namespace string `json:"documentNamespace"` + } + if err := json.Unmarshal(raw, &doc); err != nil { + t.Fatalf("decode: %v", err) + } + if doc.Namespace == "https://acme.example/spdx/acme-platform-7f3c" { + t.Error("the merged document adopted a source's identity instead of minting its own") + } + + refs := spdxExternalDocumentRefs(t, raw) + if len(refs) != 2 { + t.Fatalf("externalDocumentRefs = %+v, want one per source\n%s", refs, raw) + } + byURI := make(map[string]string, len(refs)) + seenIDs := make(map[string]struct{}, len(refs)) + for _, ref := range refs { + if !strings.HasPrefix(ref.ID, "DocumentRef-") { + t.Errorf("externalDocumentId = %q, want the mandatory DocumentRef- prefix", ref.ID) + } + if _, duplicate := seenIDs[ref.ID]; duplicate { + t.Errorf("externalDocumentId %q is used twice", ref.ID) + } + seenIDs[ref.ID] = struct{}{} + if ref.Checksum.Algorithm != "SHA256" { + t.Errorf("checksum algorithm = %q, want SPDX's own spelling", ref.Checksum.Algorithm) + } + if len(ref.Checksum.Value) != 64 { + t.Errorf("checksum for %q = %q, want a SHA-256 hex digest", ref.URI, ref.Checksum.Value) + } + byURI[ref.URI] = ref.Checksum.Value + } + if _, ok := byURI["https://acme.example/spdx/acme-platform-7f3c"]; !ok { + t.Errorf("the SPDX source is not named: %+v", refs) + } + if _, ok := byURI["urn:cdx:3e671687-395b-41f5-a30f-a58921a69b79/1"]; !ok { + t.Errorf("the CycloneDX source is not named by its BOM-Link: %+v", refs) + } + + // The two sources are different documents, so their checksums differ. A + // single digest reused across every entry would be the shape of a bug + // that hashes the export rather than each source. + values := make(map[string]struct{}, len(byURI)) + for _, value := range byURI { + values[value] = struct{}{} + } + if len(values) != len(byURI) { + t.Errorf("the sources share a checksum: %+v", byURI) + } +} + +// The checksum a source link carries is a digest of that source document's +// own bytes, not of anything else. Asserted against the digest computed here +// so a change that hashes the wrong buffer cannot pass. +func TestSourceLinkChecksumCoversTheSourceBytes(t *testing.T) { + raw, _ := mergedExport(t, TargetSPDX23JSON, documentRichSPDX, serialCycloneDX) + + want := map[string]string{ + "https://acme.example/spdx/acme-platform-7f3c": sha256Hex(documentRichSPDX), + "urn:cdx:3e671687-395b-41f5-a30f-a58921a69b79/1": sha256Hex(serialCycloneDX), + } + for _, ref := range spdxExternalDocumentRefs(t, raw) { + expected, known := want[ref.URI] + if !known { + t.Errorf("unexpected source %q", ref.URI) + continue + } + if ref.Checksum.Value != expected { + t.Errorf("checksum for %q = %q, want a digest of that document's bytes %q", ref.URI, ref.Checksum.Value, expected) + } + } +} + +// Converting a merged document again still names the documents behind it. +// +// The links used to be write-only: an export wrote them and ingest read +// nothing back, so the second conversion produced a document that claimed to +// be built from nothing (bomly-dev/bomly-sdk#61). Provenance now survives more +// than one hop, which is the SDK's declared merge class for the source set. +func TestMergedSourceLinksSurviveASecondConversion(t *testing.T) { + for _, target := range []Target{TargetSPDX23JSON, TargetCycloneDX16JSON} { + t.Run(string(target), func(t *testing.T) { + first, _ := mergedExport(t, target, documentRichSPDX, serialCycloneDX) + + // The merged document, converted a second time. One source now, + // so this is a conversion: it restates that document, and the + // documents that document named are still named. + _, entry := ingestDocument(t, string(first)) + second, err := MarshalGraphEntriesJSON(entry.Graph, []sdk.GraphEntry{entry}, target, + BuildOptions{Created: fixedExportTime()}, EncodeOptions{Pretty: true}) + if err != nil { + t.Fatalf("second export: %v", err) + } + + var named []string + switch target { + case TargetSPDX23JSON: + for _, ref := range spdxExternalDocumentRefs(t, second) { + named = append(named, ref.URI) + } + default: + for _, ref := range cycloneDXSourceRefs(t, second) { + named = append(named, ref.URL) + } + } + for _, want := range []string{ + "https://acme.example/spdx/acme-platform-7f3c", + "urn:cdx:3e671687-395b-41f5-a30f-a58921a69b79/1", + } { + if !containsStringValue(named, want) { + t.Errorf("second export names %v, missing %q\n%s", named, want, second) + } + } + }) + } +} + +// A merged CycloneDX document carries each source's checksum on the link, so +// the tuple SPDX needs survives a CycloneDX hop. Without it, converting a +// merged CycloneDX document to SPDX could name no source at all -- the format +// requires the checksum and there would be nowhere left to recover it from. +func TestCycloneDXSourceLinksCarryTheirChecksum(t *testing.T) { + raw, _ := mergedExport(t, TargetCycloneDX16JSON, documentRichSPDX, serialCycloneDX) + refs := cycloneDXSourceRefs(t, raw) + if len(refs) != 2 { + t.Fatalf("bom references = %+v, want one per source\n%s", refs, raw) + } + for _, ref := range refs { + if ref.Hashes == nil || len(*ref.Hashes) == 0 { + t.Fatalf("the link to %q carries no hash\n%s", ref.URL, raw) + } + } + + // And converting that document to SPDX still names both sources, which is + // the whole reason the hash is written. + _, entry := ingestDocument(t, string(raw)) + converted, err := MarshalGraphEntriesJSON(entry.Graph, []sdk.GraphEntry{entry}, TargetSPDX23JSON, + BuildOptions{Created: fixedExportTime()}, EncodeOptions{Pretty: true}) + if err != nil { + t.Fatalf("convert to spdx: %v", err) + } + if got := len(spdxExternalDocumentRefs(t, converted)); got != 2 { + t.Errorf("externalDocumentRefs = %d, want both sources\n%s", got, converted) + } +} + +// A native scan names no sources: there were none. +func TestNativeExportNamesNoSources(t *testing.T) { + g := scopedGraph(t, sdk.ScopeRuntime) + for _, target := range []Target{TargetSPDX23JSON, TargetCycloneDX16JSON} { + t.Run(string(target), func(t *testing.T) { + raw, err := MarshalDepGraphJSON(g, target, BuildOptions{Created: fixedExportTime()}, EncodeOptions{Pretty: true}) + if err != nil { + t.Fatalf("export: %v", err) + } + switch target { + case TargetSPDX23JSON: + if refs := spdxExternalDocumentRefs(t, raw); len(refs) != 0 { + t.Errorf("externalDocumentRefs = %+v, want none", refs) + } + default: + if refs := cycloneDXSourceRefs(t, raw); len(refs) != 0 { + t.Errorf("bom references = %+v, want none", refs) + } + } + }) + } +} + +// A conversion does not link the document it restates: it *is* that document. +// Only the documents behind it are named. +func TestConversionDoesNotLinkTheDocumentItRestates(t *testing.T) { + _, entry := ingestDocument(t, documentRichSPDX) + raw, err := MarshalGraphEntriesJSON(entry.Graph, []sdk.GraphEntry{entry}, TargetSPDX23JSON, + BuildOptions{Created: fixedExportTime()}, EncodeOptions{Pretty: true}) + if err != nil { + t.Fatalf("export: %v", err) + } + if refs := spdxExternalDocumentRefs(t, raw); len(refs) != 0 { + t.Errorf("externalDocumentRefs = %+v, want none: the export adopted that identity", refs) + } +} + +func containsStringValue(values []string, want string) bool { + for _, value := range values { + if value == want { + return true + } + } + return false +} + +// sha256Hex is the digest form the ingest checksum is written in. +func sha256Hex(value string) string { + sum := sha256.Sum256([]byte(value)) + return hex.EncodeToString(sum[:]) +} + +// Two sources whose identities reduce to the same SPDX idstring still get +// distinct reference ids. +// +// The idstring alphabet is narrow -- letters, digits, "." and "-" -- so two +// different URIs can sanitize to one id, and SPDX requires each +// externalDocumentId to be unique within a document. Nothing in tools-golang +// or the SDK mints one, so this package reuses its own package-id rule +// including the collision suffix; this is the case that says so. +func TestCollidingSourceIdentitiesGetDistinctReferenceIDs(t *testing.T) { + checksum := sdk.Digest{ + Algorithm: sdk.DigestAlgorithmSHA256, + Value: "0000000000000000000000000000000000000000000000000000000000000000", + } + doc := &Document{ + Namespace: "https://bomly.dev/spdx/merged", + Sources: []sdk.DocumentAssertions{ + {Identity: "https://acme.example/a_b", Checksum: &checksum}, + {Identity: "https://acme.example/a+b", Checksum: &checksum}, + }, + } + refs := spdxSourceLinks(doc) + if len(refs) != 2 { + t.Fatalf("refs = %+v, want one per source", refs) + } + if refs[0].DocumentRefID == refs[1].DocumentRefID { + t.Errorf("both sources share the id %q; SPDX requires them to be distinct", refs[0].DocumentRefID) + } +} diff --git a/internal/sbom/graph.go b/internal/sbom/graph.go index c0a1850f..527088ae 100644 --- a/internal/sbom/graph.go +++ b/internal/sbom/graph.go @@ -41,40 +41,13 @@ func applyIngestedAssertions(pkg *sdk.DependencyNode, component Component) { pkg.Originator = &contact } } - pkg.Description = stableValue(sdk.NormalizeDescription, component.Description) - pkg.Homepage = stableValue(sdk.NormalizeHomepage, component.Homepage) + 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) } -// 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. // @@ -226,6 +199,9 @@ func ToGraph(doc *Document) (*sdk.Graph, error) { return nil, fmt.Errorf("sbom component %q (%s): %w", component.ID, componentIdentityHint(component), err) } pkg.Scopes = append([]sdk.Scope(nil), component.Scopes...) + // Through the gate, not copied: the field crosses the same trust + // boundary every other ingested assertion does. + pkg.SourceScope = sdk.NormalizeSourceScope(component.SourceScope) pkg.Copyright = component.Copyright applyIngestedAssertions(pkg, component) // The document's own component ID does not survive: the node answers diff --git a/internal/sbom/graph_test.go b/internal/sbom/graph_test.go index f5e80fff..04171e23 100644 --- a/internal/sbom/graph_test.go +++ b/internal/sbom/graph_test.go @@ -149,3 +149,57 @@ func TestExportedDependenciesNameOnlyComponents(t *testing.T) { t.Fatalf("child module dependsOn = %v, want %q", got, leaf.NodeID()) } } + +// The ingest gates are fixed points, so the values they admit survive every +// further hop. +// +// This is the property bomly-dev/bomly-sdk#54 broke and the SDK's v0.9.5 +// NormalizeDescription restored, and it is the reason applyIngestedAssertions +// no longer wraps the gates in a local normalize-until-it-settles loop. The +// input is the shape that found the defect: bytes that are not valid UTF-8, +// short enough to pass the gate's input bound and long enough that repairing +// each of them into U+FFFD -- three bytes apiece -- carries the result past +// that same bound. Under the old gate the first pass returned the repaired, +// over-long value and the second returned "", so a description survived one +// conversion and vanished on the next. +// +// Asserted on the gate itself rather than only through the node, because it +// is the gate's promise; the node assertion below is what the ingest path +// actually depends on. +func TestIngestGatesAreFixedPoints(t *testing.T) { + for _, testCase := range []struct { + name string + value string + }{ + {"invalid utf-8 that triples on repair", strings.Repeat("\xff", 5000)}, + {"invalid utf-8 within the bound", strings.Repeat("\xff", 16)}, + {"control characters", "a\x00b\x07c"}, + {"plain text", "a widget for widgets"}, + } { + t.Run(testCase.name, func(t *testing.T) { + once := sdk.NormalizeDescription(testCase.value) + if twice := sdk.NormalizeDescription(once); twice != once { + t.Fatalf("NormalizeDescription is not idempotent: %d bytes then %d bytes", len(once), len(twice)) + } + + 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{Name: "widget", Description: testCase.value}) + if node.Description != once { + t.Fatalf("ingested description = %q, want the gate's own answer %q", node.Description, once) + } + // And a second hop -- export back into a component, ingest again -- + // keeps it, which is what the deleted workaround was protecting. + second, err := sdk.NewDependencyNode(sdk.Coordinates{Ecosystem: "npm", Name: "widget", Version: "1.0.0"}) + if err != nil { + t.Fatalf("construct node: %v", err) + } + applyIngestedAssertions(second, Component{Name: "widget", Description: node.Description}) + if second.Description != node.Description { + t.Fatalf("description changed on the second hop: %q then %q", node.Description, second.Description) + } + }) + } +} diff --git a/internal/sbom/model.go b/internal/sbom/model.go index b9fe9f99..314985ba 100644 --- a/internal/sbom/model.go +++ b/internal/sbom/model.go @@ -120,6 +120,19 @@ type Document struct { // same field as Sources below. Assertions sdk.DocumentAssertions + // UnknownScopeTokens are the scope-carrier tokens an ingested document + // named that this build does not recognize, deduplicated and sorted. + // + // The carrier is Bomly's own, so an unreadable token is most likely one a + // newer Bomly wrote. The scopes beside it are still kept -- that is the + // point of the SDK's lenient read -- but the fact that something was not + // read is worth telling a user, and this package has no logger of its + // own. The SBOM detector is where it becomes a warning. + // + // Empty for a document Bomly wrote and for a document with no carrier at + // all. + UnknownScopeTokens []string + // Sources are the documents this one was built from, one per ingested // SBOM, in entry order. // @@ -171,13 +184,29 @@ type Component struct { // and a second copy here is how the forward and reverse directions came // to disagree in the first place. // - // One clause of ADR-0037 is not implemented here: a source document's own - // scalar -- CycloneDX's "optional", say -- is not preserved for verbatim - // re-emission, because DependencyNode has nowhere to carry a source- - // asserted scope alongside the derived set. "optional" therefore reads as - // runtime and re-exports as "required". Tracked as bomly-dev/bomly-sdk#57. + // A source document's own scalar rides beside the set in SourceScope, so + // the derived set is never the only record of what the document said. Scopes []sdk.Scope + // SourceScope is the scope word the source document used, in that + // document's own vocabulary -- CycloneDX's "optional", say. It is a + // preserved claim and never an input to filtering: Scopes is what the + // pipeline reads, and the SDK derives it from this. + // + // ADR-0037 asks that the word be re-emitted verbatim unless Bomly's own + // scope set changed, so "optional" and "excluded" do not collapse into + // "required" across a round trip that asserted neither. Whether the word + // still means what the set means is sdk.CycloneDXScopeForExport's + // decision, not this package's -- the mapping that read the word in is + // the only thing that can say whether it still describes the set, and a + // second copy of it here is how the two directions came to disagree + // before. + // + // Only CycloneDX has a scalar to hold it. SPDX 2.3 has no scope field at + // all, so an SPDX export carries the set in its comment carrier and the + // source word stops there. + SourceScope string + PURL string Ecosystem string PackageManager string diff --git a/internal/sbom/scope_carrier.go b/internal/sbom/scope_carrier.go new file mode 100644 index 00000000..7cb3bd03 --- /dev/null +++ b/internal/sbom/scope_carrier.go @@ -0,0 +1,63 @@ +package sbom + +import ( + "sort" + + "github.com/bomly-dev/bomly-sdk" +) + +// This file reads Bomly's own scope carrier out of an ingested document and +// collects the tokens this build could not read, so they reach a user instead +// of vanishing (ADR-0037). +// +// The carrier is a `bomly:scopes` CycloneDX property and the `scope=` field of +// the SPDX package comment. Both hold the same value, and the SDK owns what it +// means in both directions -- nothing here parses a token. + +// unknownScopeTokens returns the scope tokens a carrier named that this build +// does not recognize. +// +// The read is lenient because the strict one was a forward-compatibility trap: +// a newer Bomly writing one token an older one does not know made the older +// one drop the whole assertion. CycloneDX could fall back to its native +// scalar; SPDX has no scalar, so a component the document scoped +// "runtime,future" ended up unscoped altogether -- the loss +// bomly-dev/bomly-sdk#64 recorded, closed by the SDK's DecodeScopeSetLenient. +// +// A carrier that is malformed outright -- an empty entry, a token that is not +// shaped like a scope token at all -- yields nothing rather than a list of +// junk to warn about. The SDK draws that line; this only reports what it +// hands back. +func unknownScopeTokens(carrier string) []string { + decoded, err := sdk.DecodeScopeSetLenient(carrier) + if err != nil { + return nil + } + return decoded.Unknown +} + +// mergeUnknownScopeTokens folds tokens into a document's collected set, +// deduplicated and sorted. +// +// Sorted because the list reaches a log line and a test golden, and one +// document's components are read in map order in one codec and slice order in +// the other; an unstable list would make the same document warn differently +// between runs. +func mergeUnknownScopeTokens(collected, tokens []string) []string { + if len(tokens) == 0 { + return collected + } + seen := make(map[string]struct{}, len(collected)+len(tokens)) + for _, token := range collected { + seen[token] = struct{}{} + } + for _, token := range tokens { + if _, duplicate := seen[token]; duplicate { + continue + } + seen[token] = struct{}{} + collected = append(collected, token) + } + sort.Strings(collected) + return collected +} diff --git a/internal/sbom/scope_round_trip_test.go b/internal/sbom/scope_round_trip_test.go index 6295ffe4..9262a642 100644 --- a/internal/sbom/scope_round_trip_test.go +++ b/internal/sbom/scope_round_trip_test.go @@ -188,3 +188,168 @@ func TestSPDXPackageCommentCarriesTheScopeSet(t *testing.T) { t.Fatalf("the package comment does not carry the sorted set:\n%s", raw) } } + +// foreignCycloneDX is a document Bomly did not write: a native scalar scope +// and no carrier property beside it. +func foreignCycloneDX(scope string) string { + return `{ + "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", "scope": "` + scope + `"} + ] +}` +} + +// cycloneDXComponentScope reads the scalar scope a rendered document wrote. +func cycloneDXComponentScope(t *testing.T, raw []byte) cdx.Scope { + t.Helper() + var bom cdx.BOM + if err := json.Unmarshal(raw, &bom); err != nil { + t.Fatalf("decode: %v", err) + } + if bom.Components == nil || len(*bom.Components) == 0 { + t.Fatalf("no components:\n%s", raw) + } + return (*bom.Components)[0].Scope +} + +// A source document's own scope word comes back out unchanged. +// +// It could not before: the model had nowhere to keep a source-asserted scope +// beside the set it derives, so a component a document marked "optional" was +// re-exported as "required" -- a claim about shipping code that the source had +// deliberately not made (bomly-dev/bomly-sdk#57). Every word in CycloneDX's +// vocabulary is covered, including "excluded", which read as development and +// projected back to "excluded" by luck rather than by preservation. +func TestSourceScopeWordSurvivesACycloneDXRoundTrip(t *testing.T) { + for _, word := range []string{"required", "optional", "excluded"} { + t.Run(word, func(t *testing.T) { + doc, _, err := UnmarshalAutoJSON([]byte(foreignCycloneDX(word))) + if err != nil { + t.Fatalf("ingest: %v", err) + } + if got := componentNamed(t, doc, "widget").SourceScope; got != word { + t.Fatalf("ingested source scope = %q, want %q", got, word) + } + + g, err := ToGraph(doc) + if err != nil { + t.Fatalf("to graph: %v", err) + } + raw, err := MarshalDepGraphJSON(g, TargetCycloneDX16JSON, BuildOptions{}, EncodeOptions{Pretty: true}) + if err != nil { + t.Fatalf("export: %v", err) + } + if got := cycloneDXComponentScope(t, raw); string(got) != word { + t.Errorf("re-exported scope = %q, want the source's own word %q\n%s", got, word, raw) + } + }) + } +} + +// When Bomly's own scope set no longer means what the source's word meant, +// the projection is written instead. The word is preserved, not obeyed: it +// describes a set, and a set that changed is no longer the one it describes. +func TestSourceScopeYieldsToTheProjectionWhenTheSetChanges(t *testing.T) { + doc, _, err := UnmarshalAutoJSON([]byte(foreignCycloneDX("optional"))) + if err != nil { + t.Fatalf("ingest: %v", err) + } + g, err := ToGraph(doc) + if err != nil { + t.Fatalf("to graph: %v", err) + } + nodes := g.DependencyNodes() + if len(nodes) != 1 { + t.Fatalf("nodes = %d", len(nodes)) + } + // What propagation does when the package turns out to be reachable from a + // development root as well: the set now says something "optional" does not. + nodes[0].Scopes = append(nodes[0].Scopes, sdk.ScopeDevelopment) + + raw, err := MarshalDepGraphJSON(g, TargetCycloneDX16JSON, BuildOptions{}, EncodeOptions{Pretty: true}) + if err != nil { + t.Fatalf("export: %v", err) + } + if got := cycloneDXComponentScope(t, raw); got != cdx.ScopeRequired { + t.Errorf("scope = %q, want the projection %q once the set changed\n%s", got, cdx.ScopeRequired, raw) + } +} + +// A carrier naming one token this build does not know keeps the scopes it +// does know, and reports the token. +// +// The strict read was a forward-compatibility trap: one unknown token from a +// newer Bomly dropped the whole assertion. CycloneDX could fall back to its +// scalar; SPDX has no scalar, so the component ended up unscoped altogether -- +// the loss bomly-dev/bomly-sdk#64 recorded. +func TestUnknownScopeTokenKeepsTheKnownScopes(t *testing.T) { + spdxWithCarrier := `{ + "spdxVersion": "SPDX-2.3", + "dataLicense": "CC0-1.0", + "SPDXID": "SPDXRef-DOCUMENT", + "name": "n", + "documentNamespace": "https://acme.example/spdx/n", + "creationInfo": {"created": "2026-01-02T03:04:05Z", "creators": ["Tool: t"]}, + "packages": [ + { + "SPDXID": "SPDXRef-widget", + "name": "widget", + "versionInfo": "1.0.0", + "comment": "bomly:scope=runtime,future-scope", + "externalRefs": [ + {"referenceCategory": "PACKAGE-MANAGER", "referenceType": "purl", "referenceLocator": "pkg:npm/widget@1.0.0"} + ] + } + ] +}` + cycloneDXWithCarrier := `{ + "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", "scope": "excluded", + "properties": [{"name": "bomly:scopes", "value": "runtime,future-scope"}]} + ] +}` + for name, raw := range map[string]string{"spdx": spdxWithCarrier, "cyclonedx": cycloneDXWithCarrier} { + t.Run(name, func(t *testing.T) { + doc, _, err := UnmarshalAutoJSON([]byte(raw)) + if err != nil { + t.Fatalf("ingest: %v", err) + } + got := componentScopes(t, doc, "widget") + if len(got) != 1 || got[0] != sdk.ScopeRuntime { + t.Fatalf("scopes = %v, want the token this build can read", got) + } + if len(doc.UnknownScopeTokens) != 1 || doc.UnknownScopeTokens[0] != "future-scope" { + t.Errorf("unknown tokens = %v, want the one that was dropped", doc.UnknownScopeTokens) + } + }) + } +} + +// A document Bomly wrote names no unknown tokens, so the warning stays quiet +// on the ordinary path. +func TestAKnownCarrierReportsNoUnknownTokens(t *testing.T) { + for _, target := range []Target{TargetSPDX23JSON, TargetCycloneDX16JSON} { + t.Run(string(target), func(t *testing.T) { + g := scopedGraph(t, sdk.ScopeRuntime, sdk.ScopeDevelopment) + raw, err := MarshalDepGraphJSON(g, target, BuildOptions{}, EncodeOptions{Pretty: true}) + if err != nil { + t.Fatalf("export: %v", err) + } + doc, _, err := UnmarshalAutoJSON(raw) + if err != nil { + t.Fatalf("ingest: %v", err) + } + if len(doc.UnknownScopeTokens) != 0 { + t.Errorf("unknown tokens = %v, want none", doc.UnknownScopeTokens) + } + }) + } +} diff --git a/internal/sbom/spdx23.go b/internal/sbom/spdx23.go index 6f754985..d068a8e4 100644 --- a/internal/sbom/spdx23.go +++ b/internal/sbom/spdx23.go @@ -133,11 +133,15 @@ func (spdx23Codec) encodeJSON(doc *Document, opts EncodeOptions) ([]byte, error) SPDXIdentifier: common.ElementID("DOCUMENT"), DocumentName: doc.NameOrDefault(), DocumentNamespace: doc.NamespaceOrDefault(), - CreationInfo: creation, - DocumentComment: doc.Assertions.Comment, - Packages: packages, - Relationships: relationships, - OtherLicenses: spdxOtherLicenses(extractedLicenses), + // 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. + ExternalDocumentReferences: spdxSourceLinks(doc), + CreationInfo: creation, + DocumentComment: doc.Assertions.Comment, + Packages: packages, + Relationships: relationships, + OtherLicenses: spdxOtherLicenses(extractedLicenses), } return marshalJSON(spdxDoc, opts.Pretty) @@ -152,16 +156,19 @@ func (spdx23Codec) decodeJSON(data []byte) (*Document, error) { extractedByRef := spdxExtractedTexts(spdxDoc.OtherLicenses) components := make([]Component, 0, len(spdxDoc.Packages)) + var unknownScopes []string for _, p := range spdxDoc.Packages { if p == nil { continue } id := common.RenderElementID(p.PackageSPDXIdentifier) + scopes, unknown := spdxCommentScopes(p.PackageComment) + unknownScopes = mergeUnknownScopeTokens(unknownScopes, unknown) component := Component{ ID: id, Name: p.PackageName, Version: p.PackageVersion, - Scopes: spdxCommentScopes(p.PackageComment), + Scopes: scopes, Type: parseSPDXComponentType(p), PURL: parseSPDXPURL(p.PackageExternalReferences), Ecosystem: parseSPDXYcosystem(p.PackageExternalReferences), @@ -220,15 +227,16 @@ func (spdx23Codec) decodeJSON(data []byte) (*Document, error) { sort.Strings(roots) return &Document{ - Name: spdxDoc.DocumentName, - Namespace: spdxDoc.DocumentNamespace, - Assertions: spdxDocumentAssertions(&spdxDoc), - Tool: extractSPDXToolName(spdxDoc.CreationInfo), - Tools: extractSPDXToolNames(spdxDoc.CreationInfo), - Created: parseSPDXCreated(spdxDoc.CreationInfo), - Components: components, - Dependencies: dependencies, - Roots: roots, + Name: spdxDoc.DocumentName, + Namespace: spdxDoc.DocumentNamespace, + Assertions: spdxDocumentAssertions(&spdxDoc), + Tool: extractSPDXToolName(spdxDoc.CreationInfo), + Tools: extractSPDXToolNames(spdxDoc.CreationInfo), + Created: parseSPDXCreated(spdxDoc.CreationInfo), + Components: components, + Dependencies: dependencies, + Roots: roots, + UnknownScopeTokens: unknownScopes, }, nil } @@ -407,24 +415,27 @@ func parseSPDXComponentType(p *v23.Package) string { return strings.ToLower(strings.TrimSpace(p.PrimaryPackagePurpose)) } -// spdxCommentScopes reads the scope set back out of the package comment. +// spdxCommentScopes reads the scope set back out of the package comment, and +// reports the tokens this build could not read. // // A carrier that will not parse is treated as absent rather than as an error: // this is a comment field on someone else's document, and refusing the whole // component because a Bomly-shaped comment was malformed would lose more than // it protects. The SDK owns what the carrier means in both directions. -// One consequence is not what ADR-0037 asks for. The SDK's decode is -// all-or-nothing, so a carrier naming one known scope beside one unknown token -// yields nothing rather than the known scope plus a warning, and unlike -// CycloneDX there is no native scalar here to fall back to -- the component -// ends up unscoped. Tracked as bomly-dev/bomly-sdk#64; partial decode belongs -// beside the grammar in the SDK rather than being re-derived here. -func spdxCommentScopes(comment string) []sdk.Scope { - scopes, err := sdk.DecodeScopeSet(parseSPDXCommentField(comment, "scope")) +// +// The read is the SDK's lenient one, which is what ADR-0037 asks for: the +// scopes this build knows are kept even when a token beside them is not. SPDX +// has no native scalar to fall back on, so the strict read made a component +// scoped "runtime,future" unscoped outright -- total loss from one token a +// newer Bomly wrote. The tokens that were not read come back so a caller can +// say so; they are the SDK's warning channel, since it does not log. +func spdxCommentScopes(comment string) ([]sdk.Scope, []string) { + carrier := parseSPDXCommentField(comment, "scope") + decoded, err := sdk.DecodeScopeSetLenient(carrier) if err != nil { - return nil + return nil, nil } - return scopes + return decoded.Scopes, decoded.Unknown } func parseSPDXCommentField(comment, field string) string { diff --git a/internal/sbom/spdx23_assertions.go b/internal/sbom/spdx23_assertions.go index 66d7c848..c31f24a5 100644 --- a/internal/sbom/spdx23_assertions.go +++ b/internal/sbom/spdx23_assertions.go @@ -1,6 +1,7 @@ package sbom import ( + "fmt" "strings" "github.com/bomly-dev/bomly-sdk" @@ -327,6 +328,7 @@ func spdxDocumentAssertions(doc *v23.Document) sdk.DocumentAssertions { Name: doc.DocumentName, DataLicense: doc.DataLicense, Comment: doc.DocumentComment, + Sources: spdxIngestedSources(doc.ExternalDocumentReferences), } if doc.CreationInfo != nil { assertions.Created = doc.CreationInfo.Created @@ -415,3 +417,102 @@ func spdxDocumentCreators(doc *Document) []common.Creator { } return creators } + +// spdxSourceLinks renders the documents this one was built from as SPDX +// externalDocumentRefs. +// +// This is the half ADR-0042 had to leave out. SPDX names another document +// through externalDocumentRefs, and section 6.6 makes the checksum mandatory +// on every entry -- so until DocumentAssertions could carry one +// (bomly-dev/bomly-sdk#55), a merged SPDX document could name nothing at all +// while the CycloneDX half already linked its sources. The checksum is +// captured at ingest by decodeDocument, over the source document's original +// bytes, because it cannot be recovered from the parsed model afterwards. +// +// A source with no checksum is skipped rather than written with an empty one: +// an entry missing the field is an invalid document, and a validator rejecting +// the whole export is a worse outcome than one unnamed source. That happens +// only for a source that reached the entry without passing through ingest -- +// a detector or plugin writing DocumentAssertions directly. +// +// The reference id is minted by sanitizeSPDXID, the same rule this package +// already uses for package element ids, with the same collision suffix. It is +// declined delegation deliberately: tools-golang owns the "DocumentRef-" +// prefix and the wire shape (common.DocumentID applies both), and the SDK's +// spdxkit mints only LicenseRef ids from license text; neither offers an +// idstring minter for a document identity. Reusing this package's existing +// rule keeps one answer rather than a second one. +func spdxSourceLinks(doc *Document) []v23.ExternalDocumentRef { + // SPDX writes a namespace and no serial, so the namespace 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{Namespace: doc.NamespaceOrDefault()}) + if len(links) == 0 { + return nil + } + refs := make([]v23.ExternalDocumentRef, 0, len(links)) + usedIDs := make(map[string]int, len(links)) + for _, link := range links { + if link.Checksum == nil { + continue + } + // Re-gated on the way out: a link tuple reaching here from a graph + // entry never passed a decoder. + checksum, ok := link.Checksum.Normalized() + if !ok { + continue + } + algorithm := checksum.Algorithm.SPDXName() + if algorithm == "" { + // The SDK's registry says SPDX does not define this algorithm. + // Writing the canonical token instead would publish a value the + // specification does not list. + continue + } + base := sanitizeSPDXID(link.Identity) + seq := usedIDs[base] + usedIDs[base] = seq + 1 + if seq > 0 { + base = fmt.Sprintf("%s-%d", base, seq) + } + refs = append(refs, v23.ExternalDocumentRef{ + // Without the "DocumentRef-" prefix: common.DocumentID adds it on + // the way out and strips it on the way in. + DocumentRefID: common.DocumentID(base), + URI: link.Identity, + Checksum: common.Checksum{ + Algorithm: common.ChecksumAlgorithm(algorithm), + Value: checksum.Value, + }, + }) + } + if len(refs) == 0 { + return nil + } + return refs +} + +// spdxIngestedSources reads the documents an SPDX document says it was built +// from, so a second conversion can name them too (bomly-dev/bomly-sdk#61). +// +// The checksum comes back with the reference, which is what makes an SPDX +// source usable as a link on the next export in either format. The algorithm +// spelling is SPDX's; the SDK's digest registry resolves it, so no mapping is +// written here. +func spdxIngestedSources(refs []v23.ExternalDocumentRef) []sdk.DocumentSource { + if len(refs) == 0 { + return nil + } + sources := make([]sdk.DocumentSource, 0, len(refs)) + for _, ref := range refs { + source := sdk.DocumentSource{Identity: ref.URI} + if checksum, ok := (sdk.Digest{ + Algorithm: sdk.DigestAlgorithm(ref.Checksum.Algorithm), + Value: ref.Checksum.Value, + }).Normalized(); ok { + source.Checksum = &checksum + } + sources = append(sources, source) + } + return sources +} diff --git a/internal/sbom/transform.go b/internal/sbom/transform.go index 05b00ad4..0718a396 100644 --- a/internal/sbom/transform.go +++ b/internal/sbom/transform.go @@ -94,6 +94,7 @@ func FromGraphEntries(g *sdk.Graph, entries []sdk.GraphEntry, opts BuildOptions) } if dep, isDep := pkg.(*sdk.DependencyNode); isDep { component.Scopes = append([]sdk.Scope(nil), dep.Scopes...) + component.SourceScope = dep.SourceScope component.Copyright = dep.Copyright component.Licenses = componentLicenses(sdk.DetectionLicenses(dep)) component.Digests = componentDigests(dep.Digests) From 5b64d7bc520dde5e18bc1375d26ff89ac7a7ddcb Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Sun, 6 Sep 2026 13:18:52 -0700 Subject: [PATCH 2/7] fix(sbom): the SPDX digest spelling comes from the registry, not a local switch A hand-written switch here knew nine algorithms against the SDK registry's nineteen, so a document carrying BLAKE2b, BLAKE3, MD2, MD4, MD6, ADLER32 or Streebog had that checksum silently dropped on export. Correct the day it was written, quietly lossy once the vocabulary grew. This is the failure the delegation rule exists to prevent, and Streebog is the example it cites -- the registry now contains exactly those two constants, and this table did not. Adopting v0.9.5 left the package with two mappings for one vocabulary, which is the moment to delete the older one rather than note it. The guard is differential rather than another list: it walks sdk.DigestAlgorithms() and requires every algorithm SPDX defines a spelling for to render as that spelling. Referencing constants would make a rename a compile error and do nothing about an addition, which is how the gap opened. An algorithm SPDX does not define still renders empty; that is the format's limit, not a gap in the mapping. Co-Authored-By: Claude Opus 5 --- internal/sbom/license_emission_test.go | 26 +++++++++++++++++++ internal/sbom/spdx23.go | 35 +++++++++++--------------- 2 files changed, 41 insertions(+), 20 deletions(-) diff --git a/internal/sbom/license_emission_test.go b/internal/sbom/license_emission_test.go index 32e26e1d..a738c185 100644 --- a/internal/sbom/license_emission_test.go +++ b/internal/sbom/license_emission_test.go @@ -396,3 +396,29 @@ func TestLicenseExpressionsAreEmittedCanonically(t *testing.T) { }) } } + +// Every algorithm the SDK knows an SPDX spelling for must survive export. +// +// This is a differential test on purpose. Referencing constants makes a rename +// a compile error and does nothing about an addition, which is exactly how a +// hand-written switch here came to know nine algorithms against the registry's +// nineteen -- dropping BLAKE2b, BLAKE3, MD2, MD4, MD6, ADLER32 and Streebog +// checksums on export without a word. Reading the registry means the next +// algorithm added upstream fails this test rather than disappearing. +func TestEverySPDXKnownDigestAlgorithmIsEmitted(t *testing.T) { + var checked int + for _, algorithm := range sdk.DigestAlgorithms() { + spdxName := algorithm.SPDXName() + if spdxName == "" { + continue // SPDX does not define this one; the format's limit, not ours. + } + checked++ + got := spdxChecksumAlgorithm(string(algorithm)) + if string(got) != spdxName { + t.Errorf("%s renders as %q, want SPDX's %q", algorithm, got, spdxName) + } + } + if checked < 10 { + t.Fatalf("only %d algorithms were checked; the registry looks unread", checked) + } +} diff --git a/internal/sbom/spdx23.go b/internal/sbom/spdx23.go index d068a8e4..54f6fc2c 100644 --- a/internal/sbom/spdx23.go +++ b/internal/sbom/spdx23.go @@ -380,29 +380,24 @@ func spdxChecksums(digests []Digest) []common.Checksum { return out } +// spdxChecksumAlgorithm renders a digest algorithm in SPDX's spelling. +// +// The registry is the SDK's, not a list here. A hand-written switch stood in +// this spot and knew nine algorithms against the registry's nineteen, so a +// document carrying BLAKE2b, BLAKE3, MD2, MD4, MD6, ADLER32 or Streebog had +// that checksum silently dropped on export -- correct the day it was written +// and quietly lossy once the vocabulary grew. That is the failure this +// project's delegation rule exists to prevent, and Streebog is the example it +// cites. +// +// An algorithm SPDX does not define returns "", which the caller drops. That +// is a real limit of the format rather than a gap in this mapping. func spdxChecksumAlgorithm(algorithm string) common.ChecksumAlgorithm { - switch strings.ToLower(strings.TrimSpace(algorithm)) { - case "md5": - return common.MD5 - case "sha1", "sha-1": - return common.SHA1 - case "sha224", "sha-224": - return common.SHA224 - case "sha256", "sha-256": - return common.SHA256 - case "sha384", "sha-384": - return common.SHA384 - case "sha512", "sha-512": - return common.SHA512 - case "sha3-256": - return common.SHA3_256 - case "sha3-384": - return common.SHA3_384 - case "sha3-512": - return common.SHA3_512 - default: + parsed, err := sdk.ParseDigestAlgorithm(algorithm) + if err != nil { return "" } + return common.ChecksumAlgorithm(parsed.SPDXName()) } func parseSPDXComponentType(p *v23.Package) string { From b2118bf7ab04409919004cd0f5fc679a1db62a6d Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Sun, 6 Sep 2026 18:25:59 -0700 Subject: [PATCH 3/7] feat(sbom): move to SDK v0.9.7 and settle the scope contract v0.9.6 and v0.9.7 close three more of the issues this work filed, and one of them resolves a disagreement this repo had recorded as open. The scope one is a contract change, not a bump. ADR-0037 said a bare CycloneDX `optional` means development; the SDK read it as runtime, and the conflict was written into that ADR rather than settled in passing. It is settled now in the ADR's favour, so the test that pinned the SDK's old reading flips and the clarification note records the resolution. The objection that made it a real question was answered rather than overruled, which is worth keeping in view: what risked hiding a shipped dependency was never `optional` but the *unasserted* case, and that now reads as runtime explicitly. A component nobody classified is no longer the one that disappears from `--scope runtime`. TestSourceScopeYieldsToTheProjectionWhenTheSetChanges needed its premise repaired rather than its expectation. It added development to a set the word "optional" now already describes, so nothing changed and the word was rightly re-emitted -- it adds runtime now, which is a set the word genuinely no longer describes. Also in: a single-segment Go module mints pkg:golang instead of pkg:generic, so go4.org and its like stop missing golang advisories. No goldens move -- no smoke fixture depends on such a module, which is precisely why a self-scan found it and the suite did not. Co-Authored-By: Claude Opus 5 --- ...m-assertions-are-typed-sdk-model-fields.md | 46 ++++++++++--------- go.mod | 2 +- go.sum | 4 +- internal/sbom/scope_round_trip_test.go | 27 +++++++---- 4 files changed, 45 insertions(+), 34 deletions(-) diff --git a/dev-docs/adr/0037-sbom-assertions-are-typed-sdk-model-fields.md b/dev-docs/adr/0037-sbom-assertions-are-typed-sdk-model-fields.md index f2193154..11c69264 100644 --- a/dev-docs/adr/0037-sbom-assertions-are-typed-sdk-model-fields.md +++ b/dev-docs/adr/0037-sbom-assertions-are-typed-sdk-model-fields.md @@ -195,28 +195,30 @@ genuinely component-private data, with the `bomly.` prefix documented as reserved. No new user-visible feature may ship its data as a metadata key; the migration path for a metadata key is a typed field. -## Clarification (2026-09-06): the `optional` scalar mapping is contested - -The rule above says a bare CycloneDX scalar maps `optional` → development. -The SDK maps `optional` → runtime, and argues the case in -`scope_cyclonedx.go`: an optional component provides additional -functionality at runtime rather than being development-only, so `required` -and `optional` both land on runtime. - -The two fail in opposite directions. Under the SDK's rule an optional -component is included by `--scope runtime`, which is conservative for a -tool whose job is to report vulnerabilities in what ships. Under this ADR's -rule it is dropped, which hides a shipped dependency from triage. On the -merits the SDK's reading looks like the safer one and this clause may -simply be wrong — but that is a contract change, so it is recorded here as -open rather than settled in passing. - -Tracked as bomly-dev/bomly-sdk#63. Until it is resolved the shipped -behavior is the SDK's, and the CLI test that covers it says so explicitly -rather than presenting it as this ADR's rule. The scope carrier -(`bomly:scopes`) means the question only affects documents Bomly did not -write; its own documents round-trip the full scope set without consulting -the scalar. +## Clarification (2026-09-06): the `optional` scalar mapping, resolved + +This ADR says a bare CycloneDX scalar maps `optional` → development. The +SDK shipped `optional` → runtime, arguing that an optional component +provides additional functionality at runtime rather than being +development-only. The two were recorded here as an open conflict, because +they fail in opposite directions and the choice is security-relevant: under +the ADR's rule an optional component is dropped by `--scope runtime`, which +hides a shipped dependency from triage. + +Resolved in this ADR's favour by bomly-dev/bomly-sdk#63, released in SDK +v0.9.6. `optional` and `excluded` both read as development, as stated +above. + +The objection that made it a real question was answered rather than +overruled. What actually risked hiding a shipped dependency was not +`optional` but the *unasserted* case — a component whose scope nobody +stated. That now reads as runtime explicitly, so the component nobody +classified is no longer the one that disappears from the shipped set. The +same change stopped scope filters dropping unasserted dependencies. + +The scope carrier (`bomly:scopes`) means the scalar only decides for +documents Bomly did not write; its own documents round-trip the full scope +set without consulting it. ## Consequences diff --git a/go.mod b/go.mod index c0ef5451..8680a59c 100644 --- a/go.mod +++ b/go.mod @@ -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.5 + github.com/bomly-dev/bomly-sdk v0.9.7 github.com/charmbracelet/bubbles v1.0.0 github.com/charmbracelet/bubbletea v1.3.10 github.com/charmbracelet/lipgloss v1.1.0 diff --git a/go.sum b/go.sum index 98f0dad1..43cff82e 100644 --- a/go.sum +++ b/go.sum @@ -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.5 h1:T4ivCbedRShbuvPUx0J4be+jug0dLRnX06BifHdZZyk= -github.com/bomly-dev/bomly-sdk v0.9.5/go.mod h1:gAGOEa88BE8gELt2rM/ZrqGIJnOzXiR6ocuuKW6AaJ0= +github.com/bomly-dev/bomly-sdk v0.9.7 h1:kYRSCR5jjzuAM81xyXExKC8yDstaUwxWKR/ml5gAQmQ= +github.com/bomly-dev/bomly-sdk v0.9.7/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= diff --git a/internal/sbom/scope_round_trip_test.go b/internal/sbom/scope_round_trip_test.go index 9262a642..cad4ed26 100644 --- a/internal/sbom/scope_round_trip_test.go +++ b/internal/sbom/scope_round_trip_test.go @@ -101,20 +101,24 @@ func TestCycloneDXStillWritesItsScalarScope(t *testing.T) { // straight through, minting nodes scoped to a value no SDK filter matches // (survey defect 1). // -// The "optional" row pins the SDK's shipped mapping, which is optional → -// runtime. ADR-0037 states optional → development, so the two disagree; the -// conflict is recorded in that ADR's 2026-09-06 clarification and tracked as -// bomly-dev/bomly-sdk#63. This asserts what the code does today rather than -// settling which rule is right -- the safer default, since the SDK's reading -// keeps an optional component inside --scope runtime instead of hiding it. +// The "optional" row was the one place ADR-0037 and the shipped SDK disagreed: +// the ADR said development, the SDK read it as runtime. Resolved in the ADR's +// favour by bomly-dev/bomly-sdk#63, and the objection that made it a real +// question -- that development would hide a shipped component from +// --scope runtime -- was answered separately rather than waved away: an +// unasserted scope now reads as runtime, so a component nobody classified is +// no longer the one that disappears. func TestForeignCycloneDXScopesMapIntoTheSDKVocabulary(t *testing.T) { for _, testCase := range []struct { native string want sdk.Scope }{ {"required", sdk.ScopeRuntime}, - {"optional", sdk.ScopeRuntime}, + {"optional", sdk.ScopeDevelopment}, {"excluded", sdk.ScopeDevelopment}, + // Unasserted is runtime, which is what keeps a component nobody + // classified from being filtered out of the shipped set. + {"", sdk.ScopeRuntime}, } { t.Run(testCase.native, func(t *testing.T) { raw := `{ @@ -267,8 +271,13 @@ func TestSourceScopeYieldsToTheProjectionWhenTheSetChanges(t *testing.T) { t.Fatalf("nodes = %d", len(nodes)) } // What propagation does when the package turns out to be reachable from a - // development root as well: the set now says something "optional" does not. - nodes[0].Scopes = append(nodes[0].Scopes, sdk.ScopeDevelopment) + // runtime root as well: the set now says something "optional" does not. + // + // It has to be runtime specifically. "optional" already means development + // on its own, so adding development leaves the set exactly what the word + // described and the word is rightly re-emitted -- which is the case the + // test above covers, not this one. + nodes[0].Scopes = append(nodes[0].Scopes, sdk.ScopeRuntime) raw, err := MarshalDepGraphJSON(g, TargetCycloneDX16JSON, BuildOptions{}, EncodeOptions{Pretty: true}) if err != nil { From 33a699c772923cc5debed101ccbd304c4c28ea85 Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Sun, 6 Sep 2026 18:29:56 -0700 Subject: [PATCH 4/7] refactor(sbom): the purl-type to ecosystem join is the SDK's v0.9.7 exports EcosystemForPURLType, which closes the decline recorded when three drifted copies of this mapping were consolidated into one: the SDK answered the question already and kept it unexported, so the CLI could hold one copy instead of three but not zero. What stood here was not a table but a reassembly -- purlkit calls plus a fallback of this package's own -- which is the same drift in a thinner disguise. The SDK had grown a second lookup for manager-name aliases and this had not, so "swiftpm" resolved to unknown here and to swift there. A differential run over thirty-nine inputs found that one difference and nothing else, which is why delegating is an improvement rather than a behavior change made on purpose. Co-Authored-By: Claude Opus 5 --- internal/sbom/identity.go | 26 +++++++------------------- internal/sbom/sbom_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 19 deletions(-) diff --git a/internal/sbom/identity.go b/internal/sbom/identity.go index ef964df6..ba0140db 100644 --- a/internal/sbom/identity.go +++ b/internal/sbom/identity.go @@ -29,26 +29,14 @@ func parsePURL(value string) *purlkit.PURL { // it and SPDX rebuilds it from the PURL — guessing here would relabel every // round-tripped Erlang dependency as Elixir, and packageManagerForPURLType // would then call it Mix. Leaving it unknown keeps the ambiguity visible. -// The purl-type -> ecosystem table that lived here is purlkit's -// (EcosystemForType). It was a second copy of the SDK's mapping, and a second -// copy is a mapping that drifts: purl-spec adds a type, one table learns it -// and the other keeps answering unknown. Phase 2.1 deletes the copy. +// The purl-type -> ecosystem question is the SDK's, answered by +// EcosystemForPURLType. It was reassembled here out of purlkit calls plus a +// fallback of this package's own, which is the same drift in a thinner +// disguise: the SDK grew a second lookup for manager-name aliases and this +// copy did not, so "swiftpm" resolved to unknown here and to swift there. +// Delegating picked that up rather than any change being made for it. func ecosystemFromPURLType(purlType string) sdk.Ecosystem { - normalized := strings.ToLower(strings.TrimSpace(purlType)) - if normalized == "" { - return sdk.EcosystemUnknown - } - if name, ok := purlkit.EcosystemForType(normalized); ok { - if ecosystem, err := sdk.ParseEcosystem(name); err == nil { - return ecosystem - } - } - // A type purlkit does not map may still be an ecosystem name Bomly knows. - ecosystem, err := sdk.ParseEcosystem(normalized) - if err != nil { - return sdk.EcosystemUnknown - } - return ecosystem + return sdk.EcosystemForPURLType(purlType) } func packageManagerForPURL(value string, ecosystemHint, packageManagerHint string) sdk.PackageManager { diff --git a/internal/sbom/sbom_test.go b/internal/sbom/sbom_test.go index 78588a53..6bb21b0d 100644 --- a/internal/sbom/sbom_test.go +++ b/internal/sbom/sbom_test.go @@ -866,3 +866,29 @@ func mustDep(t testing.TB, node sdk.GraphNode) *sdk.DependencyNode { } return dep } + +// The purl-type to ecosystem join is the SDK's, not a local reassembly of +// purlkit calls. A CLI copy had already fallen behind: the SDK consults a +// second lookup for manager-name aliases, so "swiftpm" resolved to the swift +// ecosystem there and to unknown here. +func TestEcosystemFromPURLTypeDelegatesToTheSDK(t *testing.T) { + for _, testCase := range []struct { + purlType string + want sdk.Ecosystem + }{ + {"golang", sdk.EcosystemGo}, + {"npm", sdk.EcosystemNPM}, + {"swiftpm", sdk.EcosystemSwift}, + {"", sdk.EcosystemUnknown}, + {"nothing-like-this", sdk.EcosystemUnknown}, + } { + t.Run(testCase.purlType, func(t *testing.T) { + if got := ecosystemFromPURLType(testCase.purlType); got != testCase.want { + t.Errorf("ecosystem = %q, want %q", got, testCase.want) + } + if got, want := ecosystemFromPURLType(testCase.purlType), sdk.EcosystemForPURLType(testCase.purlType); got != want { + t.Errorf("diverged from the SDK: %q vs %q", got, want) + } + }) + } +} From 48f1a0e4aab88ac045ab83ceb795fde8ca4a5646 Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Tue, 8 Sep 2026 23:46:10 -0700 Subject: [PATCH 5/7] fix(sbom): the CycloneDX digest spelling comes from the registry too The mirror of the SPDX fix earlier in this branch, and the same defect in two shapes one file apart. A hand-written switch knew eight algorithms against the registry's nineteen, so a component carrying BLAKE2b, BLAKE3 or Streebog had its checksum silently dropped. And the external-reference path cast the SDK token straight into CycloneDX's enum, writing "sha256" where the schema says "SHA-256" -- invalid for every algorithm, not only the ones the format has no name for. That cast predates this branch. Both render through DigestAlgorithm.CycloneDXName() now, and an algorithm CycloneDX does not define is omitted rather than written in a spelling the schema rejects: MD2, MD4, MD6 and ADLER32 are SPDX spellings with no CycloneDX equivalent. The guard is differential, walking sdk.DigestAlgorithms(), so an algorithm added upstream fails a test instead of vanishing -- the same shape as the SPDX guard, which is what made this one easy to see. Also corrects the scope line in docs/SBOM.md: optional and excluded both read as development, per the resolution recorded in ADR-0037. Co-Authored-By: Claude Opus 5 --- docs/SBOM.md | 6 ++-- internal/sbom/cyclonedx.go | 32 ++++++++---------- internal/sbom/cyclonedx_assertions.go | 10 +++++- internal/sbom/license_emission_test.go | 46 ++++++++++++++++++++++++++ 4 files changed, 73 insertions(+), 21 deletions(-) diff --git a/docs/SBOM.md b/docs/SBOM.md index 6037941a..b238c825 100644 --- a/docs/SBOM.md +++ b/docs/SBOM.md @@ -403,8 +403,10 @@ Some information necessarily becomes less specific during conversion: prefers: a `bomly:scopes` CycloneDX property, and the `scope=` field of the SPDX package comment. A Bomly document therefore round-trips its scope set exactly, and a document from any other producer still yields a usable scope. - Reading the other way, CycloneDX `required` and `optional` both become - runtime and `excluded` becomes development. + Reading the other way, CycloneDX `required` becomes runtime, while + `optional` and `excluded` become development — the specification defines an + optional component as one that is not installed or otherwise reachable, so + it is absent from what runs. - A scope carrier naming a token this build does not recognize keeps the scopes it does recognize. The carrier is Bomly's own, so an unreadable token is almost always one a newer Bomly wrote; refusing the whole value would have diff --git a/internal/sbom/cyclonedx.go b/internal/sbom/cyclonedx.go index 33893141..72a6ef61 100644 --- a/internal/sbom/cyclonedx.go +++ b/internal/sbom/cyclonedx.go @@ -597,27 +597,23 @@ func cycloneDXHashes(digests []Digest) []cdx.Hash { // cycloneDXHashAlgorithm maps a digest algorithm string onto a CycloneDX hash // algorithm constant. Returns "" when the algorithm is unsupported so the // digest is dropped rather than emitting an invalid BOM. +// cycloneDXHashAlgorithm renders a digest algorithm in CycloneDX's spelling. +// +// The registry is the SDK's, not a list here. A hand-written switch stood in +// this spot and knew eight algorithms against the registry's nineteen, so a +// component carrying BLAKE2b, BLAKE3 or Streebog had that checksum silently +// dropped -- the same defect this PR already fixed on the SPDX side, in the +// same shape, one file away. +// +// An algorithm CycloneDX does not define returns "", which the caller drops. +// That is a real limit of the format: MD2, MD4, MD6 and ADLER32 are SPDX +// spellings with no CycloneDX equivalent. func cycloneDXHashAlgorithm(algorithm string) cdx.HashAlgorithm { - switch strings.ToLower(strings.TrimSpace(algorithm)) { - case "md5": - return cdx.HashAlgoMD5 - case "sha1", "sha-1": - return cdx.HashAlgoSHA1 - case "sha256", "sha-256": - return cdx.HashAlgoSHA256 - case "sha384", "sha-384": - return cdx.HashAlgoSHA384 - case "sha512", "sha-512": - return cdx.HashAlgoSHA512 - case "sha3-256": - return cdx.HashAlgoSHA3_256 - case "sha3-384": - return cdx.HashAlgoSHA3_384 - case "sha3-512": - return cdx.HashAlgoSHA3_512 - default: + parsed, err := sdk.ParseDigestAlgorithm(algorithm) + if err != nil { return "" } + return cdx.HashAlgorithm(parsed.CycloneDXName()) } func cycloneDXEOLProperties(eol *EOL) []cdx.Property { diff --git a/internal/sbom/cyclonedx_assertions.go b/internal/sbom/cyclonedx_assertions.go index 0e65bc15..717f67bf 100644 --- a/internal/sbom/cyclonedx_assertions.go +++ b/internal/sbom/cyclonedx_assertions.go @@ -296,8 +296,16 @@ func cycloneDXEmittedHashes(digests []sdk.Digest) *[]cdx.Hash { if !ok { continue } + // Through the same rendering the component hashes use. Casting the + // SDK token straight into CycloneDX's enum wrote "sha256" where the + // schema says "SHA-256" -- invalid for every algorithm, not only the + // ones CycloneDX has no name for. + algorithm := cycloneDXHashAlgorithm(string(normalized.Algorithm)) + if algorithm == "" { + continue + } hashes = append(hashes, cdx.Hash{ - Algorithm: cdx.HashAlgorithm(normalized.Algorithm), + Algorithm: algorithm, Value: normalized.Value, }) } diff --git a/internal/sbom/license_emission_test.go b/internal/sbom/license_emission_test.go index a738c185..2af8497e 100644 --- a/internal/sbom/license_emission_test.go +++ b/internal/sbom/license_emission_test.go @@ -422,3 +422,49 @@ func TestEverySPDXKnownDigestAlgorithmIsEmitted(t *testing.T) { t.Fatalf("only %d algorithms were checked; the registry looks unread", checked) } } + +// Every algorithm the SDK knows a CycloneDX spelling for must survive export, +// and must go out in that spelling. +// +// The mirror of TestEverySPDXKnownDigestAlgorithmIsEmitted, and it exists for +// the same reason twice over: a hand-written switch here knew eight algorithms +// against the registry's nineteen, and the external-reference path cast the +// SDK token straight into the enum, writing "sha256" where the schema says +// "SHA-256". Differential on purpose -- a constant reference makes a rename a +// compile error and says nothing about an addition. +func TestEveryCycloneDXKnownDigestAlgorithmIsEmitted(t *testing.T) { + var checked int + for _, algorithm := range sdk.DigestAlgorithms() { + name := algorithm.CycloneDXName() + if name == "" { + continue // CycloneDX does not define this one; the format's limit. + } + checked++ + if got := cycloneDXHashAlgorithm(string(algorithm)); string(got) != name { + t.Errorf("%s renders as %q, want CycloneDX's %q", algorithm, got, name) + } + } + if checked < 10 { + t.Fatalf("only %d algorithms were checked; the registry looks unread", checked) + } +} + +// An algorithm CycloneDX has no name for is omitted rather than written in a +// spelling the schema rejects. +func TestUnmappableDigestIsOmittedFromCycloneDXReferences(t *testing.T) { + hashes := cycloneDXEmittedHashes([]sdk.Digest{ + {Algorithm: sdk.DigestAlgorithmADLER32, Value: "0badf00d"}, + {Algorithm: sdk.DigestAlgorithmSHA256, Value: "9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08"}, + }) + if hashes == nil { + t.Fatal("the sha-256 hash was dropped along with the unmappable one") + } + for _, h := range *hashes { + if h.Algorithm == "adler32" || h.Algorithm == "ADLER32" { + t.Errorf("an algorithm CycloneDX does not define was emitted: %+v", h) + } + if h.Algorithm == "sha256" { + t.Errorf("the SDK token was emitted instead of CycloneDX's spelling: %+v", h) + } + } +} From 1ac23e7305bc6386716d6ac408d349307066d952 Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Tue, 8 Sep 2026 23:50:57 -0700 Subject: [PATCH 6/7] test(smoke): refresh the swiftpm golden for an upstream release swift-http-types moved 1.7.0 -> 1.8.0 upstream. Unrelated to this branch and the drift #425 tracks; committed only so the suite is green, and kept as its own commit so it reads as what it is. Nothing else moved. The digest change in this branch touched no golden at all, which says no smoke fixture carries an external-reference hash -- worth a fixture, since that is the path the cast was corrupting. Co-Authored-By: Claude Opus 5 --- .../smoke/testdata/golden/scan-swiftpm.golden.json | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/test/smoke/testdata/golden/scan-swiftpm.golden.json b/test/smoke/testdata/golden/scan-swiftpm.golden.json index 5c4a0d67..8f1d7abe 100644 --- a/test/smoke/testdata/golden/scan-swiftpm.golden.json +++ b/test/smoke/testdata/golden/scan-swiftpm.golden.json @@ -10,7 +10,7 @@ "pkg:swift/github.com/apple/swift-atomics@1.3.1", "pkg:swift/github.com/apple/swift-collections@1.1.4", "pkg:swift/github.com/apple/swift-crypto@3.3.0", - "pkg:swift/github.com/apple/swift-http-types@1.7.0", + "pkg:swift/github.com/apple/swift-http-types@1.8.0", "pkg:swift/github.com/apple/swift-log@1.5.4", "pkg:swift/github.com/apple/swift-metrics@2.11.0", "pkg:swift/github.com/apple/swift-nio-extras@1.22.0", @@ -144,7 +144,7 @@ }, { "depends_on": [], - "id": "pkg:swift/github.com/apple/swift-http-types@1.7.0", + "id": "pkg:swift/github.com/apple/swift-http-types@1.8.0", "licenses": [], "locations": [ { @@ -157,12 +157,12 @@ } ], "name": "github.com/apple:swift-http-types", - "package_ref": "pkg:swift/github.com/apple/swift-http-types@1.7.0", - "purl": "pkg:swift/github.com/apple/swift-http-types@1.7.0", + "package_ref": "pkg:swift/github.com/apple/swift-http-types@1.8.0", + "purl": "pkg:swift/github.com/apple/swift-http-types@1.8.0", "scopes": [ "runtime" ], - "version": "1.7.0" + "version": "1.8.0" }, { "depends_on": [], @@ -586,8 +586,8 @@ "licenses": [], "name": "github.com/apple:swift-http-types", "org": "github.com/apple", - "purl": "pkg:swift/github.com/apple/swift-http-types@1.7.0", - "version": "1.7.0", + "purl": "pkg:swift/github.com/apple/swift-http-types@1.8.0", + "version": "1.8.0", "vulnerabilities": [] }, { From 4e99cea8f6db6221ec861185e7a908b7ae1eb784 Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Wed, 9 Sep 2026 00:11:04 -0700 Subject: [PATCH 7/7] fix(test): a guard file may name the module it forbids main is red. #436 and #437 each added a guard and merged independently: the presentation-layer guard has to spell packageurl-go in order to ban it, and the module-boundary guard reports any file under internal/ that names it. Two rules doing their job, one flagging the other. The exemption is a set of canonical paths now. Not a name -- exempting anything called guards_test.go was the earlier bug in this same line, and it hid a forbidden import in a second guard file. Not one hard-coded path either, which is what made the guards collide the moment a second one existed. Adding a guard costs one line in that set, deliberately: a new exemption should be an edit somebody reviews, not a pattern that widens on its own. The predicate is extracted so the property can be pinned rather than described. TestGuardExemptionIsByPathNotByName fails if a file becomes exempt for being *named* like a guard, and if an entry names a file that no longer exists -- a dead exemption is a rule nobody is applying. The first mutation I ran against the old shape passed, which is how the missing test surfaced. Co-Authored-By: Claude Opus 5 --- internal/detectors/guards_test.go | 55 +++++++++++++++++++++++++++++-- 1 file changed, 53 insertions(+), 2 deletions(-) diff --git a/internal/detectors/guards_test.go b/internal/detectors/guards_test.go index f49f6920..9728a846 100644 --- a/internal/detectors/guards_test.go +++ b/internal/detectors/guards_test.go @@ -113,6 +113,35 @@ func TestNoDirectSPDXExpressionUse(t *testing.T) { } } +// guardFiles are the files whose job is to forbid a module, so they are the +// files that have to spell it. Exempting them is not a loophole -- naming a +// module in a rule that bans it is the opposite of reaching for it. +// +// An explicit set of canonical paths, for two reasons learned the hard way. +// Matching a name instead exempted every guards_test.go under internal/, so a +// second guard file anywhere could name a forbidden module unnoticed. And +// exempting only this one file made the guards flag each other the moment a +// second one existed: internal/output grew its own presentation-layer guard, +// which must name packageurl-go to forbid it, and this rule reported it. +// +// Adding a guard therefore costs one line here, on purpose. That is the point: +// a new exemption should be a deliberate edit somebody reviews, not a pattern +// that silently widens. +var guardFiles = map[string]struct{}{ + filepath.Clean(filepath.Join(internalRoot, "detectors", "guards_test.go")): {}, + filepath.Clean(filepath.Join(internalRoot, "output", "registry_lookup_guard_test.go")): {}, +} + +// isGuardFile reports whether a path is one of the guard files above. +// +// By path. Exempting anything named guards_test.go was the earlier bug: it +// meant a second guard file in any package could name a forbidden module and +// no rule would report it. +func isGuardFile(path string) bool { + _, ok := guardFiles[filepath.Clean(path)] + return ok +} + // filesNamingModule returns every Go file under internal/ -- test files // included -- whose text names the module path. Tests count because a test // reaching a library directly proves the hazard is still reachable, and a test @@ -127,13 +156,15 @@ func TestNoDirectSPDXExpressionUse(t *testing.T) { // the right direction to fail. func filesNamingModule(t *testing.T, module string) []string { t.Helper() - self := filepath.Clean(filepath.Join(internalRoot, "detectors", "guards_test.go")) var offenders []string err := filepath.Walk(internalRoot, func(path string, info os.FileInfo, err error) error { if err != nil { return err } - if info.IsDir() || !strings.HasSuffix(path, ".go") || filepath.Clean(path) == self { + if info.IsDir() || !strings.HasSuffix(path, ".go") { + return nil + } + if isGuardFile(path) { return nil } body, err := os.ReadFile(path) @@ -279,3 +310,23 @@ func TestDetectionResultsCarryingGraphsAreAttributed(t *testing.T) { "wrap the result in detectors.Attributed: %v", offenders) } } + +// The exemption is a set of paths, and a file is not exempt for being named +// like a guard. Both halves have been wrong here before: matching the basename +// hid a forbidden import in a second guards_test.go, and exempting only one +// file made two guards report each other. +func TestGuardExemptionIsByPathNotByName(t *testing.T) { + for path := range guardFiles { + if _, err := os.Stat(path); err != nil { + t.Errorf("guard file %q does not exist; a dead exemption is a rule nobody is applying", path) + } + if !isGuardFile(path) { + t.Errorf("guard file %q is not recognized by its own predicate", path) + } + } + // A file named like a guard, in a package that has none, must not be + // exempt -- whether or not it exists today. + if impostor := filepath.Join(internalRoot, "sbom", "guards_test.go"); isGuardFile(impostor) { + t.Errorf("%q is exempt for being named guards_test.go rather than for being a guard", impostor) + } +}