From 0f3b5e3b82e8937341187ffa86a3aa5264fdbc74 Mon Sep 17 00:00:00 2001 From: xgreenx Date: Thu, 10 Sep 2026 22:04:54 +0100 Subject: [PATCH] fix(ceremony): read JSON fields with the whitespace beside structure removed GitHub pretty-prints /user for the media type the profile pins: a newline and two spaces before every member, a space after every colon. The readers matched compact delimiters byte for byte, so every real identity read was refused. Rather than teach each reader the places whitespace may sit, the bytes are normalized once before any reader looks, the way `normalizeHeaderBytes` already serves the request head: a run of JSON whitespace that touches `:` `,` `{` `}` `[` or `]` is removed, which is exactly where JSON puts insignificant whitespace, and a run between two tokens stays, so `123 4` does not read as `1234` and a trailing space is still the byte the terminator check judges. Stateless on purpose: a reader that tracks strings is one a prover desynchronises by cutting a range mid-value. Every reader keeps its one template, and a member in any spelling is the same member, so a duplicate spelled with spaces is counted as one. The bearer framing follows: the revealed range ending where the commitment starts is the anchor, its normalized bytes end with the prefix, and the prefix appears at most once across everything revealed. One range, never a join -- a prefix assembled across a seam is one the platform never wrote. The prover reveals the whitespace inside the member at its offsets; libid-rs follows in its own change. An alternative to #36, which reaches the same acceptance with a tolerant matcher per reader. Assisted-by: Claude Opus 5 Signed-off-by: xgreenx --- .../ceremony/CeremonyAttestation.sol | 50 +++++++++++++-- .../contracts/ceremony/CeremonyFields.sol | 61 +++++++++++++++++++ .../ceremony/TlsNotaryVerifierBase.sol | 8 ++- .../ceremony/test/CeremonyAttestation.t.sol | 60 ++++++++++++++++++ .../ceremony/test/CeremonyFields.t.sol | 37 +++++++++++ .../test/GitHubPlatformVerifier.t.sol | 29 ++++++++- 6 files changed, 237 insertions(+), 8 deletions(-) diff --git a/solidity/contracts/ceremony/CeremonyAttestation.sol b/solidity/contracts/ceremony/CeremonyAttestation.sol index 74249dc..1334575 100644 --- a/solidity/contracts/ceremony/CeremonyAttestation.sol +++ b/solidity/contracts/ceremony/CeremonyAttestation.sol @@ -1,6 +1,8 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.24; +import {CeremonyFields} from "./CeremonyFields.sol"; + /// @title CeremonyAttestation /// @notice Decoder for the attested-data layout the launch profiles pin. /// @dev THE LAYOUT IS THE PROFILE'S, NOT THE SPECIFICATION'S. REQ-COMMON-18 @@ -142,7 +144,8 @@ library CeremonyAttestation { /// @dev The normalized, line-anchored needle REQ-COMMON-39 counts. bytes internal constant AUTHORIZATION_NEEDLE = "\r\nauthorization:bearer"; - /// @notice The one commitment framed by exactly these revealed bytes. + /// @notice The one commitment framed by these revealed bytes, JSON + /// whitespace aside. /// /// @dev For a direction that is NOT exactly covered, where several ranges /// are hidden and only the anchors around one of them are revealed. @@ -163,12 +166,21 @@ library CeremonyAttestation { pure returns (RangeCommitment memory framed) { + // The prefix at most once across everything revealed, JSON whitespace + // removed: a second one, in any spelling, is a second place the framing + // could point, whether or not a commitment sits behind it. + if (_occurrences(CeremonyFields.normalizeJsonBytes(concatRevealed(block_)), prefix) > 1) { + revert AmbiguousFraming(); + } + uint256 found = type(uint256).max; for (uint256 i = 0; i < block_.commitments.length; ++i) { RangeCommitment memory c = block_.commitments[i]; - if (c.start < prefix.length) continue; - bytes memory before_ = _revealedSlice(block_, c.start - uint32(prefix.length), c.start); - if (keccak256(before_) != keccak256(prefix)) continue; + // The one revealed range ending where the commitment starts is the + // anchor, and its bytes with the JSON whitespace removed end with + // the prefix. One range, never a join: a prefix assembled across a + // seam is one the platform never wrote in one piece. + if (!_anchoredBy(block_, c.start, prefix)) continue; bytes memory after_ = _revealedSlice(block_, c.end, c.end + uint32(suffix.length)); if (keccak256(after_) != keccak256(suffix)) continue; @@ -179,6 +191,36 @@ library CeremonyAttestation { return block_.commitments[found]; } + /// @dev Whether a revealed range ends exactly at `at` and, JSON whitespace + /// removed, ends with `prefix`. The whitespace stays revealed at its + /// offsets -- the range is the wire -- and is only ignored to compare. + function _anchoredBy(DirectionBlock memory block_, uint32 at, bytes memory prefix) private pure returns (bool) { + for (uint256 i = 0; i < block_.revealed.length; ++i) { + RevealedRange memory range = block_.revealed[i]; + if (range.end != at) continue; + bytes memory normalized = CeremonyFields.normalizeJsonBytes(range.value); + if (normalized.length < prefix.length) return false; + for (uint256 j = 0; j < prefix.length; ++j) { + if (normalized[normalized.length - prefix.length + j] != prefix[j]) return false; + } + return true; + } + return false; + } + + function _occurrences(bytes memory haystack, bytes memory needle) private pure returns (uint256 count) { + for (uint256 i = 0; i + needle.length <= haystack.length; ++i) { + bool hit = true; + for (uint256 j = 0; j < needle.length; ++j) { + if (haystack[i + j] != needle[j]) { + hit = false; + break; + } + } + if (hit) ++count; + } + } + /// @notice Every check REQ-COMMON-35, -39 and -40 require of an /// identity-session request that commits a credential in an HTTP /// `Authorization` header. diff --git a/solidity/contracts/ceremony/CeremonyFields.sol b/solidity/contracts/ceremony/CeremonyFields.sol index 98e9872..b4dd3a1 100644 --- a/solidity/contracts/ceremony/CeremonyFields.sol +++ b/solidity/contracts/ceremony/CeremonyFields.sol @@ -51,6 +51,7 @@ library CeremonyFields { pure returns (Found found, bytes memory value) { + data = normalizeJsonBytes(data); bytes memory needle = abi.encodePacked('"', name, '":"'); uint256 at; (found, at) = _findUnique(data, needle); @@ -88,6 +89,7 @@ library CeremonyFields { pure returns (Found found, bytes memory digits) { + data = normalizeJsonBytes(data); bytes memory needle = abi.encodePacked('"', name, '":'); uint256 at; (found, at) = _findUnique(data, needle); @@ -110,6 +112,65 @@ library CeremonyFields { return (Found.One, digits); } + /// @notice `data` with the JSON whitespace that touches a structural + /// byte removed. + /// + /// @dev The four bytes JSON lets a writer put between tokens (RFC 8259 + /// section 2): space, tab, line feed, carriage return. GitHub + /// pretty-prints `/user` for the media type the profile pins, so the + /// compact delimiters the readers above match are a grammar, not the + /// bytes on the wire. Removing the whitespace first, the way + /// `CeremonyAttestation.normalizeHeaderBytes` does for a request head, + /// leaves every reader its one exact template and makes a member in + /// any spelling the same member -- so a duplicate spelled with spaces + /// is still counted as one. + /// + /// Only a run that touches `:` `,` `{` `}` `[` or `]` on either side + /// goes, which is exactly where JSON puts insignificant whitespace. + /// A run between two tokens stays: `123 456` must not read as + /// `123456`, and a trailing space after digits must still be the + /// byte the terminator check judges. Stateless on purpose, with no + /// notion of being inside a string: a reader with one is a reader a + /// prover desynchronises by cutting a revealed range mid-value, and a + /// needle then hides where the reader believes a string is open. No + /// needle can be manufactured by this either -- one needs unescaped + /// quotes, and this removes none -- and nothing this reads carries + /// whitespace beside a structural byte inside its value. + function normalizeJsonBytes(bytes memory data) internal pure returns (bytes memory out) { + out = new bytes(data.length); + uint256 n; + uint256 i; + while (i < data.length) { + if (!_isJsonWhitespace(data[i])) { + out[n++] = data[i]; + ++i; + continue; + } + uint256 j = i; + while (j < data.length && _isJsonWhitespace(data[j])) { + ++j; + } + bool touches = (n != 0 && _isStructural(out[n - 1])) || (j < data.length && _isStructural(data[j])); + if (!touches) { + for (uint256 k = i; k < j; ++k) { + out[n++] = data[k]; + } + } + i = j; + } + assembly ("memory-safe") { + mstore(out, n) + } + } + + function _isJsonWhitespace(bytes1 c) private pure returns (bool) { + return c == 0x20 || c == 0x09 || c == 0x0a || c == 0x0d; + } + + function _isStructural(bytes1 c) private pure returns (bool) { + return c == ":" || c == "," || c == "{" || c == "}" || c == "[" || c == "]"; + } + function _findUnique(bytes memory data, bytes memory needle) private pure returns (Found found, uint256 at) { uint256 hit = type(uint256).max; for (uint256 i = 0; i + needle.length <= data.length; ++i) { diff --git a/solidity/contracts/ceremony/TlsNotaryVerifierBase.sol b/solidity/contracts/ceremony/TlsNotaryVerifierBase.sol index f1a24a5..c303e7d 100644 --- a/solidity/contracts/ceremony/TlsNotaryVerifierBase.sol +++ b/solidity/contracts/ceremony/TlsNotaryVerifierBase.sol @@ -171,7 +171,9 @@ abstract contract TlsNotaryVerifierBase is IPlatformVerifier, PlatformVerifierBa /// through a second delimiter and the duplicate REQ-COMMON-19A exists /// to reject becomes invisible to it. So the value is read per range /// and the occurrences are counted over the concatenation -- where a - /// seam can only over-count, which fails closed. + /// seam can only over-count, which fails closed. Both read bytes with + /// the JSON whitespace removed, so a copy spelled with spaces is a + /// copy. function _delimiterCount(bytes memory joined, bytes memory delimiter) private pure returns (uint256 count) { for (uint256 i = 0; i + delimiter.length <= joined.length; ++i) { bool hit = true; @@ -215,7 +217,7 @@ abstract contract TlsNotaryVerifierBase is IPlatformVerifier, PlatformVerifierBa if (matches != 1) revert FieldNotUnique(name, matches); // And the delimiter appears once across the whole revealed set, so a // second copy cannot hide under a range boundary. - uint256 seen = _delimiterCount(joined, abi.encodePacked('"', name, '":"')); + uint256 seen = _delimiterCount(CeremonyFields.normalizeJsonBytes(joined), abi.encodePacked('"', name, '":"')); if (seen != 1) revert FieldNotUnique(name, seen); } @@ -235,7 +237,7 @@ abstract contract TlsNotaryVerifierBase is IPlatformVerifier, PlatformVerifierBa } } if (matches != 1) revert FieldNotUnique(name, matches); - uint256 seen = _delimiterCount(joined, abi.encodePacked('"', name, '":')); + uint256 seen = _delimiterCount(CeremonyFields.normalizeJsonBytes(joined), abi.encodePacked('"', name, '":')); if (seen != 1) revert FieldNotUnique(name, seen); } diff --git a/solidity/contracts/ceremony/test/CeremonyAttestation.t.sol b/solidity/contracts/ceremony/test/CeremonyAttestation.t.sol index ab6a885..1b1a12c 100644 --- a/solidity/contracts/ceremony/test/CeremonyAttestation.t.sol +++ b/solidity/contracts/ceremony/test/CeremonyAttestation.t.sol @@ -192,4 +192,64 @@ contract CeremonyAttestationTest is Test { ) ); } + + // ─── Framing behind JSON whitespace ───────────────────────────── + + function framed(CeremonyAttestation.DirectionBlock memory block_) external pure returns (bytes32) { + return CeremonyAttestation.requireFramedCommitment(block_, '"access_token":"', '"').commitment; + } + + /// @dev The anchor range carries the prefix with the platform's spaces, + /// tab and newline inside, ends where the commitment starts, and the + /// closing quote follows. The whitespace stays revealed at its + /// offsets -- the range is the wire -- and is only ignored to compare. + function test_framesABearerBehindJsonWhitespace() public { + bytes memory prefix = bytes('"access_token" \t: \r\n"'); + uint32 start = uint32(prefix.length); + CeremonyAttestation.DirectionBlock memory block_; + block_.revealed = new CeremonyAttestation.RevealedRange[](2); + block_.revealed[0] = CeremonyAttestation.RevealedRange({start: 0, end: start, value: prefix}); + block_.revealed[1] = CeremonyAttestation.RevealedRange({start: start + 5, end: start + 6, value: '"'}); + block_.commitments = new CeremonyAttestation.RangeCommitment[](1); + block_.commitments[0] = + CeremonyAttestation.RangeCommitment({start: start, end: start + 5, commitment: bytes32(uint256(7))}); + assertEq(this.framed(block_), bytes32(uint256(7))); + + // One byte off: no revealed range ends where the commitment starts. + block_.commitments[0].start = start + 1; + vm.expectRevert(CeremonyAttestation.NoFramedCommitment.selector); + this.framed(block_); + + // A second prefix, compact, elsewhere among the revealed bytes is + // ambiguous even with no second commitment behind it. + block_.commitments[0].start = start; + block_.revealed[1].value = bytes('" "access_token":"'); + block_.revealed[1].end = block_.revealed[1].start + uint32(block_.revealed[1].value.length); + vm.expectRevert(CeremonyAttestation.AmbiguousFraming.selector); + this.framed(block_); + } + + /// @dev The prefix split across two revealed ranges around a committed + /// byte. Joined and normalized it would read as the prefix; it is + /// refused, because no single range ends at the commitment with it, + /// and a prefix assembled across a seam is one the platform never + /// wrote in one piece. + function test_refusesAPrefixSplitAcrossRanges() public { + bytes memory first = bytes('"access_token" '); + bytes memory second = bytes(': \t"'); + uint32 split = uint32(first.length); + uint32 bearer = split + 1 + uint32(second.length); + CeremonyAttestation.DirectionBlock memory block_; + block_.revealed = new CeremonyAttestation.RevealedRange[](3); + block_.revealed[0] = CeremonyAttestation.RevealedRange({start: 0, end: split, value: first}); + block_.revealed[1] = CeremonyAttestation.RevealedRange({start: split + 1, end: bearer, value: second}); + block_.revealed[2] = CeremonyAttestation.RevealedRange({start: bearer + 5, end: bearer + 6, value: '"'}); + block_.commitments = new CeremonyAttestation.RangeCommitment[](2); + block_.commitments[0] = + CeremonyAttestation.RangeCommitment({start: split, end: split + 1, commitment: bytes32(uint256(8))}); + block_.commitments[1] = + CeremonyAttestation.RangeCommitment({start: bearer, end: bearer + 5, commitment: bytes32(uint256(7))}); + vm.expectRevert(CeremonyAttestation.NoFramedCommitment.selector); + this.framed(block_); + } } diff --git a/solidity/contracts/ceremony/test/CeremonyFields.t.sol b/solidity/contracts/ceremony/test/CeremonyFields.t.sol index e7689b5..0b1b6fc 100644 --- a/solidity/contracts/ceremony/test/CeremonyFields.t.sol +++ b/solidity/contracts/ceremony/test/CeremonyFields.t.sol @@ -176,4 +176,41 @@ contract CeremonyFieldsTest is Test { assertFalse(CeremonyFields.isSerializerSafe(bytes(""))); assertFalse(CeremonyFields.isSerializerSafe(hex"c3a9")); // non-ASCII } + + // ─── JSON whitespace ──────────────────────────────────────────── + + /// @dev JSON whitespace between tokens is not part of any token. The + /// readers remove it before they look, so a pretty-printed member + /// reads as its compact spelling does. + function test_readsMembersThroughJsonWhitespace() public view { + bytes memory body = bytes('{\n "login" \t: "alice",\r\n "id" : 123 \n}'); + assertEq(string(this.jsonString(body, "login")), "alice"); + assertEq(string(this.jsonInteger(body, "id")), "123"); + } + + /// @dev And a duplicate in another spelling is still a duplicate, for a + /// string and for an integer alike. + function test_countsADuplicateInAnotherWhitespaceSpelling() public { + vm.expectRevert(abi.encodeWithSelector(CeremonyFields.AmbiguousField.selector, "login")); + this.jsonString(bytes('{"login":"alice","login" : "bob"}'), "login"); + vm.expectRevert(abi.encodeWithSelector(CeremonyFields.AmbiguousField.selector, "id")); + this.jsonInteger(bytes('{"id":123,"id" : 456}'), "id"); + } + + /// @dev Only the four bytes JSON calls whitespace are removed. A vertical + /// tab is not one of them, and a member spelled with it is no member. + function test_removesOnlyJsonWhitespace() public { + vm.expectRevert(abi.encodeWithSelector(CeremonyFields.FieldNotFound.selector, "login")); + this.jsonString(bytes.concat(bytes('{"login":'), hex"0b", bytes('"alice"}')), "login"); + } + + /// @dev Whitespace before a brace is JSON's and goes; whitespace between + /// two runs of digits touches no structural byte, stays, and is the + /// terminator the reader then refuses. `123 4` does not read as + /// `1234`. + function test_stillRefusesDigitsAfterWhitespace() public { + assertEq(string(this.jsonInteger(bytes('{"id":123 }'), "id")), "123"); + vm.expectRevert(abi.encodeWithSelector(CeremonyFields.BadIntegerTerminator.selector, "id", bytes1(0x20))); + this.jsonInteger(bytes('{"id":123 4}'), "id"); + } } diff --git a/solidity/contracts/ceremony/test/GitHubPlatformVerifier.t.sol b/solidity/contracts/ceremony/test/GitHubPlatformVerifier.t.sol index 0868daf..9deb9f2 100644 --- a/solidity/contracts/ceremony/test/GitHubPlatformVerifier.t.sol +++ b/solidity/contracts/ceremony/test/GitHubPlatformVerifier.t.sol @@ -277,7 +277,10 @@ contract GitHubPlatformVerifierTest is Test { /// rather than a prefix of a longer one. function test_rejectsAnIdWithoutAStructuralTerminator() public { TlsNotaryVerifierBase.TlsNotaryProof memory s = _payload(); - s.identitySession = _identity('{"login":"octocat","id":583231 }', CeremonyProfile.AUTHORITY_GITHUB_API); + // A space before the brace is JSON's own and reads through; a space + // before more digits touches no structural byte and is the + // terminator, which is not one. + s.identitySession = _identity('{"login":"octocat","id":583231 4}', CeremonyProfile.AUTHORITY_GITHUB_API); vm.expectPartialRevert(CeremonyFields.BadIntegerTerminator.selector); this.run{value: quote}(s); } @@ -331,6 +334,30 @@ contract GitHubPlatformVerifierTest is Test { this.run{value: quote}(s); } + /// @dev GitHub pretty-prints `/user` for the media type the profile pins: + /// a newline and two spaces before every member, a space after every + /// colon. The readers remove JSON whitespace before they look, so the + /// compact delimiters they match are the grammar, not the bytes. + function test_readsTheIdentityGitHubPrettyPrints() public { + TlsNotaryVerifierBase.TlsNotaryProof memory s = _payload(); + s.identitySession = _identity( + '{\n "login": "octocat",\n "id": 583231,\n "node_id": "MDQ6VXNlcjU4MzIzMQ==",\n "name": "The Octocat"\n}', + CeremonyProfile.AUTHORITY_GITHUB_API + ); + ICeremony.VerifiedClaim memory f = this.run{value: quote}(s); + assertEq(f.userId, "583231"); + assertEq(f.handle, "octocat"); + } + + /// @dev A second `login` in another spelling is a second `login`. + function test_rejectsADuplicateMemberInAnotherWhitespaceSpelling() public { + TlsNotaryVerifierBase.TlsNotaryProof memory s = _payload(); + s.identitySession = + _identity('{"login":"octocat","id":583231,"login" : "mallory"}', CeremonyProfile.AUTHORITY_GITHUB_API); + vm.expectRevert(abi.encodeWithSelector(TlsNotaryVerifierBase.FieldNotUnique.selector, "login", 2)); + this.run{value: quote}(s); + } + /// @dev The wrong authority is still refused before any field is read. function test_rejectsAnIdentityReadFromTheWrongAuthority() public { TlsNotaryVerifierBase.TlsNotaryProof memory s = _payload();