fix(sbom): the digest vocabulary is the SDK registry's, not local tables - #443
fix(sbom): the digest vocabulary is the SDK registry's, not local tables#443bomly-guy wants to merge 5 commits into
Conversation
Both SBOM formats close their hash enumeration, so an algorithm Bomly
cannot name in the target format cannot be published at all -- which makes
the mapping, not the encoder, the thing that decides whether a digest
survives export. This package had three transcriptions of that mapping and
a fourth site that skipped it, and between them they dropped or corrupted
every algorithm registered after the switches were written.
bomly-sdk v0.9.5 owns the vocabulary in digest.go: ParseDigestAlgorithm
resolves any spelling to the canonical token, SPDXName and CycloneDXName
render each format's, and the registry references spdx/tools-golang's and
cyclonedx-go's own constants rather than copying them -- guarded upstream
by digest_registry_test.go, so a member added to either specification
fails a build instead of disappearing at runtime.
- spdxChecksums delegated; spdxChecksumAlgorithm deleted. It omitted
BLAKE2b-256/384/512, BLAKE3, MD2, MD4, MD6, and ADLER32, all of which
SPDX 2.3 defines, so a document carrying one lost its checksum.
- cycloneDXHashes delegated; cycloneDXHashAlgorithm deleted. Same shape,
same omissions, plus Streebog -- the pair this defect class already
cost once. It now also drops the SPDX-only members (SHA224, MD2, MD4,
MD6, ADLER32) that an ingested SPDX document can carry, rather than
writing an algorithm CycloneDX does not define.
- cycloneDXEmittedHashes wrote the SDK's canonical token straight into
cdx.Hash.Algorithm, so reference hashes exported as "sha256" where the
schema says "SHA-256" and an ingested document changed on its second
export. This is the ExternalReferenceCategory.SPDXName defect in the
digest vocabulary; it now renders through CycloneDXName.
- digestHexSizes is keyed by the canonical token instead of listing a row
per spelling. The lengths stay local -- the SDK deliberately records no
per-algorithm value length, because ecosystems publish digests in hex,
in base64, and over subjects that are not files -- but the spellings
were a fourth copy of the vocabulary.
publishableDigest gives the rule one home: resolve the algorithm, render it
in this format, or omit the digest. Both encoders pass a method expression,
so they ask the same registry the same question and only the rendering
differs.
TestExportProjectsEveryRegisteredDigestAlgorithm is the guard. Referencing
constants makes a rename a compile error but says nothing about an
addition, and an addition is how Streebog was lost: the test enumerates
sdk.DigestAlgorithms() and asserts each member either exports in its
format's spelling or is absent from that format, so a switch written back
in by hand fails here rather than in a user's document.
The three defect tests were confirmed to fail against the pre-change code.
No golden moves: the goldens carry only sha1/sha256/sha512, whose spellings
are unchanged, and the three SBOM smoke tests pass against them.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 1 minute. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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 <noreply@anthropic.com>
…berg-34b10a # Conflicts: # internal/sbom/cyclonedx.go
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e0e275ec4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Bomly Diff SummaryCompared Overview
Dependency Changes✅ No dependency changes. Vulnerabilities✅ No vulnerability changes. License Changes✅ No license changes. Project Posture✅ No project posture changes ( Policy Findings✅ No policy differences were identified. |
publishableDigest checked the algorithm and a non-empty value, which is a weaker gate than the one ingest clears in ingestedDigests. Component digests can be built in memory by a detector or a plugin without ever passing through the SDK's JSON hooks, so a value carrying a control character, an interior Unicode space, or invalid UTF-8 reached the encoders intact. That matters most for the algorithms this branch newly admits, which were previously dropped for want of a mapping. encoding/json rewrites invalid UTF-8 as U+FFFD, so such a digest changes as it is serialized -- a digest that changes when written is worse than no digest. Digest.Normalized is now the gate, so ingest and export clear the same one. What it deliberately does not check is length per algorithm: ecosystems publish digests in hex, in base64 (npm SRI), and over subjects that are not files (a Go module "h1:" dirhash), so a per-algorithm hex length would reject values that are correct for their ecosystem. The new test asserts both directions -- malformed values dropped, a base64 SRI value published. Also pins where CycloneDX spec-version scoping lives. Streebog is a 1.7 addition, so a 1.6 document naming it carries a value outside a closed enumeration -- but cyclonedx-go already owns that conversion: EncodeVersion converts through SpecVersion.supportsHashAlgorithm and strips a hash the requested version cannot name. Verified against the encoder's actual output across all four targets rather than argued from the mapping's shape. A version table in publishableDigest would be a second copy of the library's, wrong the day CycloneDX adds an algorithm -- the defect this whole change removes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
main is red: #436 tightened TestNoDirectPackageURLUse while #437 added internal/output/registry_lookup_guard_test.go, whose own forbidden-string literal names the module the other guard forbids, so the two guards report each other. The failure is inherited by every branch cut from main, including this one, and is unrelated to the digest vocabulary change here. #444 fixes it and is green. Merging that branch rather than writing a second fix: it is the same commit, so when #444 lands on main this history dedupes with no conflict, and #443 never carries a competing version of the same fix. It touches only internal/detectors/guards_test.go, which this PR does not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bde0b5f75
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if isGuardFile(path) { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
filesNamingModule serves both the package-URL and go-spdx guards, but this unconditional skip now exempts internal/output/registry_lookup_guard_test.go from every module check merely because it mentions packageurl-go for its own guard. If that file later imports or otherwise reaches github.com/github/go-spdx, TestNoDirectSPDXExpressionUse will silently pass despite its stated whole-tree coverage; associate each exempt path with only the module it must name rather than skipping the entire file.
AGENTS.md reference: AGENTS.md:L123-L123
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and deliberately not fixed here. isGuardFile is module-blind, so internal/output/registry_lookup_guard_test.go is exempt from TestNoDirectSPDXExpressionUse as well as the packageurl guard it actually needs — a real narrowing of the stated whole-tree coverage. But that code is #444's, not this PR's: it appears in this diff only because I merged #444's branch to unblock CI here (main is red — #436 tightened the PURL guard while #437 added a guard file whose own forbidden-string literal trips it). You have already filed this same finding on #444 at the same file and line, which is where it belongs; fixing it here would put a competing version of guards_test.go on two branches and guarantee the conflict the merge was meant to avoid. Leaving this thread open rather than resolving it, since the fix lands in #444 and this branch will pick it up when that PR updates. Nothing in this PR's own change (internal/sbom) is affected.
🤖 Addressed by Claude Code
1 participant
What
internal/sbomheld three hand-written transcriptions of the digest-algorithm vocabulary and a fourth site that skipped the mapping entirely. This replaces all four with delegation to thebomly-sdkdigest registry.Both SBOM formats close their hash enumeration, so an algorithm Bomly cannot name in the target format cannot be published at all — which makes the mapping, not the encoder, the thing that decides whether a digest survives export. Every one of these copies was correct the day it was written and lossy the day the specification grew.
Why
spdxChecksumAlgorithmomitted BLAKE2b-256/384/512, BLAKE3, MD2, MD4, MD6, and ADLER32 — all defined by SPDX 2.3. A document carrying one had its checksum silently dropped on export, by the gate that exists to reject unpublishable values.cycloneDXHashAlgorithmhad the same shape and the same omissions, plus Streebog: the exact pair this defect class already cost once, which is what motivated sourcing the SDK registry fromspdx/tools-golangandcyclonedx-goconstants in the first place.Changes
spdxChecksums(spdx23.go)ParseDigestAlgorithm+SPDXName(); switch deletedcycloneDXHashes(cyclonedx.go)ParseDigestAlgorithm+CycloneDXName(); switch deletedcycloneDXEmittedHashes(cyclonedx_assertions.go)cdx.Hash.AlgorithmCycloneDXName()digestHexSizes(transform.go)"sha-1","sha1", …)publishableDigest(new, digest.go) gives the rule one home — resolve the algorithm, render it in this format, or omit the digest. Both encoders pass a method expression (sdk.DigestAlgorithm.SPDXName/.CycloneDXName), so they ask the same registry the same question and only the rendering differs.Two behavior fixes beyond the omissions
cycloneDXEmittedHasheswrote"sha256"where CycloneDX defines"SHA-256", so an ingestedSHA-256changed case on its second export. This is theExternalReferenceCategory.SPDXName()defect from feat(sbom): adopt SDK v0.9.7 and close eight limitations #440, in the digest vocabulary.Delegation check
bomly-dev/bomly-sdk@v0.9.6(ParseDigestAlgorithm,SPDXName(),CycloneDXName()). The SDK registry references the two libraries' own constants and is guarded upstream bydigest_registry_test.go.digestHexSizes, used to recognize npm SRI base64): declined — the SDK deliberately records no per-algorithm value length, because ecosystems publish digests in hex, in base64, and over subjects that are not files. Reason recorded in the code; only the lengths stay local, the spellings do not.Tests
Four tests in
internal/sbom/digest_test.go. The three defect tests were confirmed to fail against the pre-change code:The fourth is the guard. Referencing constants makes a rename a compile error but says nothing about an addition — and an addition is how Streebog was lost.
TestExportProjectsEveryRegisteredDigestAlgorithmenumeratessdk.DigestAlgorithms()and asserts each member either exports in its format's spelling or is absent from that format, so a switch written back in by hand fails there rather than in a user's document.Verification
make verifypasses, including the generated-docs drift check.SHA512,SHA-512). Confirmed by runningTestScanSBOMExportGolden,TestScanSBOMExportOrigin, andTestScanSBOMSyftJSONRejectedagainst the existing goldens — all pass. The lowercase"sha256"values in the scan goldens are Bomly's own JSON output (canonical SDK tokens), which this change does not touch.Note for the reviewer
The task description for this change referred to
spdxSourceLinksinspdx23_assertions.goas an existing correct site usingchecksum.Algorithm.SPDXName(). No such function exists in the tree — the one delegating site isExternalReferenceCategory.SPDXName()atspdx23_assertions.go:296(same pattern, different vocabulary), and the CycloneDX emit side was the one carrying the spelling bug fixed here.🤖 Generated with Claude Code