Skip to content

fix(sbom): an escaped lone surrogate is refused as its own class, and the advice says so - #463

Merged
bomly-guy merged 1 commit into
mainfrom
fix/strict-ingest-lone-surrogates
Sep 13, 2026
Merged

bomly-guy merged 1 commit into
mainfrom
fix/strict-ingest-lone-surrogates

Conversation

@bomly-guy

Copy link
Copy Markdown
Collaborator

Closes #432. Closes #435.

#432. A CycloneDX document with "description":"\ud800" (an escaped unpaired UTF-16 surrogate) was refused as ErrAmbiguousJSON, although ADR-0039 claimed only two classes, and the advice pointed the user at a repeated member that is not there.

The decision, on the specification's text. CycloneDX and SPDX JSON are defined over JSON Schema draft-07, which defers to RFC 7159/8259. RFC 8259 §8.2: a JSON text is interoperable only "when all the strings represented in a JSON text are composed entirely of Unicode characters (however escaped)"; the grammar "allows member names and string values to contain bit sequences that cannot encode Unicode characters; for example, "\uDEAD""; and "the behavior of software that receives JSON texts containing such values is unpredictable". A document its own format says readers will not agree on does not have one reading, so Bomly keeps refusing it and names it as a third class. The spec text is quoted beside the code.

Mechanics (classifyStrictRefusal in internal/sbom/strict.go): decided by what the readers disagree about, never by matching a message. json.Valid still separates malformed; errors.Is(…, jsontext.ErrDuplicateName) says whether a name was repeated; a second scan with AllowDuplicateNames(true) and AllowInvalidUTF8(true) — which retains no names and copies no values, so none of the memory shapes the bounds exist for — confirms text validity was the only objection; utf8.Valid on the raw bytes then separates invalid bytes from an escape that names no character. All on the error path. The main decoder line is unchanged (TestStrictPreflightReadsInPlace still pins bytes.NewBuffer).

User-facing: the detector's advice now says "emits each object member once and writes every string as Unicode text, without unpaired surrogate escapes", and each refusal names its class before the library's location.

#435: the hand-instrumented preflight stays (option 1). The residual — one oversized member name held once by the decoder, at most 1× an input already capped and resident — is the floor for any reader; the durable close is an upstream jsontext token-size option. Recorded in ADR-0039's dated amendment together with the third class.

Tests: TestIngestRejectsEscapedLoneSurrogates, TestStrictIngestAcceptsEscapedCharacters (a paired surrogate and a BMP escape still import), TestStrictRefusalsNameTheirClass (table over the three classes); the fuzz target's determinism check now covers all four sentinels.

Docs: ADR-0039 amendment, docs/SBOM.md third bullet and corrected advice, dev-docs/SECURITY_ASSURANCE.md row and evidence.

make verify passes.

🤖 Generated with Claude Code

… the advice says so

A CycloneDX document carrying "description":"\ud800" -- an escaped unpaired
UTF-16 surrogate -- was refused by the strict preflight as ErrAmbiguousJSON,
which ADR-0039 defined as exactly two classes: a repeated member name, or
bytes that are not valid UTF-8. This document is neither, and the advice
told the user to regenerate it with a producer that emits each member once.

It is a third class, on the specification's own text rather than a library
default. CycloneDX and SPDX JSON are defined over JSON Schema draft-07,
which defers to RFC 7159/8259; RFC 8259 section 8.2 says a JSON text is
interoperable only when every string is composed of Unicode characters
however escaped, names "\uDEAD" as a sequence that cannot encode one, and
says receiver behaviour on such a text is unpredictable. A document its own
format says readers will not agree on does not have one reading, so Bomly
keeps refusing it and names the class.

The class is decided by what the readers disagree about, never by matching
a message: the library says whether a name was repeated; a second scan with
both strict checks waived -- retaining no names and copying no values --
confirms text validity was the only objection; utf8.Valid over the raw
bytes then separates invalid bytes from an escape that names no character.
All of it is on the error path.

Also decided, for #435: the hand-instrumented preflight stays. Its residual
(one oversized member name held once by the decoder, at most 1x an input
already capped and resident) is the floor for any reader; the durable close
is an upstream jsontext token-size option. Recorded in ADR-0039's amendment
with the third class.

Closes #432.
Closes #435.

Co-Authored-By: Claude Fable 5.1 <[email protected]>
@coderabbitai

coderabbitai Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

Next included review available in 43 minutes.

Check out review usage here.

View limit details

Limit 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.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 2f21ee60-eda3-442b-8bfd-20d7973ba7e4

📥 Commits

Reviewing files that changed from the base of the PR and between 6d3bcb2 and ccc97ea.

📒 Files selected for processing (7)
  • dev-docs/SECURITY_ASSURANCE.md
  • dev-docs/adr/0039-both-modules-build-on-go-1-27.md
  • docs/SBOM.md
  • internal/detectors/sbom/detector.go
  • internal/sbom/codec_fuzz_test.go
  • internal/sbom/strict.go
  • internal/sbom/strict_test.go

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

Bomly Diff Summary

Compared 6d3bcb2f74136a114ed78b8c203bf28bbfd97aba to ccc97ea545f010d855afac3956e00561d64b7989.

Overview

Status Manifests Dependencies Findings Duration
✅ Pass +0 / ~0 / -0 0 added / 0 version changed / 0 detail changes / 0 removed 0 introduced / 0 persisted / 0 resolved 1m 23s

Dependency Changes

✅ No dependency changes.

Vulnerabilities

✅ No vulnerability changes.

License Changes

✅ No license changes.

Project Posture

✅ No project posture changes (--matchers +scorecard was not selected).

Policy Findings

✅ No policy differences were identified.

@bomly-guy
bomly-guy merged commit d17559f into main Sep 13, 2026
16 checks passed
@bomly-guy
bomly-guy deleted the fix/strict-ingest-lone-surrogates branch September 13, 2026 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant