diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e8d71db..402b917a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,34 @@ follows [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Fixed + +- **pyrxd silently refused about a quarter of live mainnet glyph payloads (#576).** + Two write-side limits fired on the READ path and each discarded the entire token: + a 100,000-byte cap in `GlyphMedia.__post_init__`, and `_cbor_str` raising on a + wrong-typed or over-long field. `GlyphInspector.extract_reveal_metadata` catches + `Exception` and returns `None`, so a user saw `metadata: NONE` rather than a + refusal they could act on. + + Measured against an independent verifier on live mainnet: **6 of 25 sampled `gly` + payloads** were refused by pyrxd and decoded fine — four were webp images of + 153,650 / 178,608 / 236,726 bytes, and two were Photonic-minted relationship + glyphs carrying `loc` as an **integer** where our spec says text. + + The media cap guarded nothing pyrxd writes: the decoder is the only code that + constructs a `GlyphMedia`. It also contradicted `_MAX_CBOR_PAYLOAD_BYTES`, which + had been deliberately raised to 256 KB to admit a real 65,569-byte payload — the + inner cap made that headroom unreachable for any media-bearing token. + + Both security properties are unchanged and still asserted: nothing is coerced + (`42` never becomes `"42"`), and nothing oversized reaches a caller. The DoS bound + is the 256 KB payload cap, on the encode path where a policy limit belongs. + + `docs/reference/glyph-token-protocol-spec.md` §4.4–4.5 are corrected. An earlier + revision said pyrxd "decodes media it would refuse to construct" — which was + right — and it had been changed to match the code instead. The spec was made to + follow the defect; it now leads again. + ### Added - **`pyrxd inspect` classifies HashMark records.** HashMark is a THIRD-PARTY diff --git a/docs/reference/glyph-token-protocol-spec.md b/docs/reference/glyph-token-protocol-spec.md index cc361dea..eb674af3 100644 --- a/docs/reference/glyph-token-protocol-spec.md +++ b/docs/reference/glyph-token-protocol-spec.md @@ -304,16 +304,23 @@ Notes on individual fields: | CBOR body | 262,144 bytes (256 KiB) | `src/pyrxd/glyph/payload.py:51` | | `attrs` entries | 64 | `src/pyrxd/glyph/payload.py:58` | | `main.t` (MIME type) | 256 characters | `src/pyrxd/glyph/payload.py:67` | -| `main.b` (media) | 100,000 bytes | `src/pyrxd/glyph/types.py:116-117` | +| `main.b` (media) | bounded only by the CBOR body cap | — | The 256 KiB body cap is a DoS bound chosen above the largest known real payload (the 65,569-byte mainnet body); it is a pyrxd policy limit, not a consensus one. -Both caps are enforced on decode. `decode_payload` constructs a `GlyphMedia` -(`src/pyrxd/glyph/payload.py:122`), so the 100,000-byte `main.b` ceiling checked -in `GlyphMedia.__post_init__` (`src/pyrxd/glyph/types.py:116-117`) rejects an -oversize blob even though the enclosing body is still under 256 KiB. Measured: -100,000 bytes decodes; 100,001 raises `ValidationError`. +**Reading is not writing.** The body cap is the only size bound applied on +decode. There is no separate `main.b` ceiling: media is whatever the enclosing +body can hold. + +An earlier revision of this section said pyrxd "decodes media it would refuse to +construct" — which was correct — and it was then changed to match a 100,000-byte +cap in `GlyphMedia.__post_init__`. That cap was a write-side policy that fired +only on decode, because the decoder is the only code that constructs a +`GlyphMedia`; it refused real mainnet tokens (webp payloads of 153,650 / 178,608 +/ 236,726 bytes, measured), and it made the 256 KiB body budget unreachable for +any media-bearing token. **The spec was right and the code was made authoritative +over it.** The cap is gone; this sentence is restored. ### 4.5 What a decoder MUST reject @@ -323,11 +330,16 @@ oversize blob even though the enclosing body is still under 256 KiB. Measured: - Bytes that are not decodable CBOR. - A top-level value that is not a map. - A map with no `p` key, or whose `p` is not an array. -- Any text field exceeding its length cap in §4.3. -- A `main.t` longer than 256 characters. -- A `main.b` longer than 100,000 bytes (`src/pyrxd/glyph/types.py:116-117`, - reached through the `GlyphMedia` construction at - `src/pyrxd/glyph/payload.py:122`). +- A `main.t` that is not a text string, or longer than 256 characters. + +A decoder MUST NOT reject the whole payload for an unusable OPTIONAL field. A +text field that is the wrong type, or longer than its §4.3 cap, is **dropped** +(the decoder logs it and returns the empty string) so that the rest of the token +still decodes. Dropping preserves both properties the caps exist for — nothing is +coerced (`42` never becomes `"42"`), and nothing oversized reaches a caller — +while a rejection additionally discarded every other field. Measured on live +mainnet: 6 of 25 sampled payloads were rejected outright by the previous +behaviour, including Photonic-minted glyphs carrying `loc` as an integer. - An `attrs` map with more than 64 entries. - A `decimals` that is a float or boolean. diff --git a/src/pyrxd/glyph/payload.py b/src/pyrxd/glyph/payload.py index 39976463..b8c5652f 100644 --- a/src/pyrxd/glyph/payload.py +++ b/src/pyrxd/glyph/payload.py @@ -37,14 +37,31 @@ def encode_payload(metadata: GlyphMetadata) -> tuple[bytes, bytes]: def _cbor_str(d: dict, key: str, max_len: int) -> str: - """Extract a string field from a CBOR dict, enforcing type and length.""" + """Extract a string field from a CBOR dict, dropping it if it is unusable. + + DROPS rather than raises, because this is the READ path for third-party + on-chain data and one bad field used to discard the entire token. `creator`, + `royalty`, `policy`, `rights`, `crypto.timelock`, `in`, `by` and `attrs` were + already defensive; only the string fields and the media raised, and one raise + killed everything. + + Measured on live mainnet: Photonic-minted relationship glyphs carry + ``{'v':2,'p':[2],'loc':0,...}`` — `loc` as an INTEGER. Our spec + (docs/reference/glyph-token-protocol-spec.md:257) says text; the chain + disagrees, and the chain is what we are reading. Refusing those tokens + surfaced as "metadata: NONE", which tells the user nothing they can act on. + + Writers stay strict: nothing on the encode path calls this. + """ v = d.get(key, "") if v == "": return "" if not isinstance(v, str): - raise ValidationError(f"CBOR field {key!r} must be a text string, got {type(v).__name__!r}") + _log.warning("decode_payload: CBOR field %r is %s, not a text string; dropped", key, type(v).__name__) + return "" if len(v) > max_len: - raise ValidationError(f"CBOR field {key!r} too long: {len(v)} > {max_len}") + _log.warning("decode_payload: CBOR field %r is %d chars > %d; dropped", key, len(v), max_len) + return "" return v diff --git a/src/pyrxd/glyph/types.py b/src/pyrxd/glyph/types.py index 80472e69..530825d6 100644 --- a/src/pyrxd/glyph/types.py +++ b/src/pyrxd/glyph/types.py @@ -118,8 +118,22 @@ def __post_init__(self) -> None: raise ValidationError("Invalid MIME type") if len(self.mime_type) > self._MAX_MIME_TYPE_CHARS: raise ValidationError(f"MIME type too long: {len(self.mime_type)} > {self._MAX_MIME_TYPE_CHARS}") - if len(self.data) > 100_000: # 100KB limit for on-chain media - raise ValidationError("On-chain media too large (max 100KB)") + # NO SIZE CAP HERE. This used to refuse data over 100 KB, and the only + # code that ever constructs a GlyphMedia is the DECODER (payload.py) — + # so a write-side policy limit fired exclusively when reading somebody + # else's token off the chain, and took the whole payload down with it: + # `GlyphInspector.extract_reveal_metadata` catches Exception and returns + # None, so the user saw "metadata: NONE" rather than a size refusal. + # + # Measured on live mainnet: 6 of 25 sampled `gly` payloads were refused + # by pyrxd and decoded fine by an independent verifier; four of the six + # were webp images of 153,650 / 178,608 / 236,726 bytes. + # + # It also contradicted the real bound. `_MAX_CBOR_PAYLOAD_BYTES` (256 KB) + # was deliberately raised to admit a genuine 65,569-byte payload, and this + # inner cap made that headroom unreachable for any media-bearing token. + # The 256 KB payload cap is the DoS bound, and it is on the encode path + # where a policy limit belongs. _VALID_PROTOCOL_VALUES = frozenset(p.value for p in GlyphProtocol) diff --git a/tests/test_glyph.py b/tests/test_glyph.py index d45d6434..d45aeb9d 100644 --- a/tests/test_glyph.py +++ b/tests/test_glyph.py @@ -1340,13 +1340,12 @@ def test_build_transfer_ft_passes_classifier(self): class TestSecurityRejection: - def test_glyph_media_data_over_100kb_raises(self): - with pytest.raises(ValidationError, match="too large"): - GlyphMedia(mime_type="image/png", data=bytes(100_001)) - - def test_glyph_media_exactly_100kb_is_valid(self): - media = GlyphMedia(mime_type="image/png", data=bytes(100_000)) - assert len(media.data) == 100_000 + def test_glyph_media_carries_chain_sized_data(self): + """No size cap on GlyphMedia: it is constructed ONLY by the decoder, so a + cap here refused real on-chain tokens and guarded nothing that pyrxd + writes. Live mainnet carries webp payloads well past the old 100 KB.""" + media = GlyphMedia(mime_type="image/png", data=bytes(236_726)) + assert len(media.data) == 236_726 def test_glyph_ref_negative_vout_raises(self): with pytest.raises(ValidationError): @@ -1395,38 +1394,35 @@ def test_decode_payload_caps_hostile_mime_type(self): with pytest.raises(ValidationError, match="main.t.*too long"): decode_payload(evil) - def test_decode_payload_enforces_the_100kb_media_ceiling(self): - """The `main.b` cap applies on DECODE, not just on construction. + def test_decode_accepts_media_the_ENCODER_would_not_choose_to_write(self): + """The read/write asymmetry, restored. + + This case previously asserted the opposite, and its docstring recorded + why: the protocol spec §4.4 had said pyrxd "decodes media it would refuse + to construct", and that sentence was changed to match the code rather + than the code changed to match the sentence. The spec had it right. - ``decode_payload`` builds a ``GlyphMedia`` (``payload.py:122``), so the - 100,000-byte ceiling in ``GlyphMedia.__post_init__`` - (``types.py:116-117``) fires even though the enclosing CBOR body is - comfortably under the 256 KiB limit. Pinned because - ``docs/reference/glyph-token-protocol-spec.md`` §4.4 previously asserted - the opposite — that pyrxd "decodes media it would refuse to construct". + Reading is not writing. A blob is already on chain; refusing to describe + it helps nobody, and one refusal took the whole token's metadata with it. """ import cbor2 from pyrxd.glyph.payload import decode_payload - oversize = cbor2.dumps({"p": [GlyphProtocol.NFT.value], "main": {"t": "image/png", "b": b"\x00" * 150_000}}) - assert len(oversize) < 262_144 # well inside the body cap: the media cap is what fires - with pytest.raises(ValidationError, match="media too large"): - decode_payload(oversize) + body = cbor2.dumps({"p": [GlyphProtocol.NFT.value], "main": {"t": "image/png", "b": b"\x00" * 236_726}}) + meta = decode_payload(body) + assert meta.main is not None and len(meta.main.data) == 236_726 - @pytest.mark.parametrize(("size", "accepted"), [(100_000, True), (100_001, False)]) - def test_decode_payload_media_ceiling_boundary(self, size: int, accepted: bool): + def test_the_dos_bound_is_the_PAYLOAD_cap_and_still_fires(self): + """Paired with the case above: leniency on the media field must not have + removed the bound, only moved it to where it always belonged.""" import cbor2 from pyrxd.glyph.payload import decode_payload - body = cbor2.dumps({"p": [GlyphProtocol.NFT.value], "main": {"t": "image/png", "b": b"\x00" * size}}) - if accepted: - meta = decode_payload(body) - assert meta.main is not None and len(meta.main.data) == size - else: - with pytest.raises(ValidationError, match="media too large"): - decode_payload(body) + body = cbor2.dumps({"p": [GlyphProtocol.NFT.value], "main": {"t": "image/png", "b": b"\x00" * 500_000}}) + with pytest.raises(ValidationError, match="payload too large"): + decode_payload(body) def test_commit_script_wrong_hash_len_raises(self): with pytest.raises(ValidationError, match="32 bytes"): diff --git a/tests/test_glyph_red_team.py b/tests/test_glyph_red_team.py index efde4dd3..1b94330d 100644 --- a/tests/test_glyph_red_team.py +++ b/tests/test_glyph_red_team.py @@ -374,49 +374,64 @@ def test_ft_cbor_with_is_nft_true_raises(self): class TestDecodePayloadSecurityAudit2026: - """Regression tests for MEDIUM-5: decode_payload field-type and length limits.""" - - def test_non_string_name_field_raises(self): - """MEDIUM-5: CBOR 'name' as integer must raise, not silently become str(42)='42'.""" - cbor_bytes = cbor2.dumps({"p": [2], "name": 42}) - with pytest.raises(ValidationError, match="must be a text string"): - decode_payload(cbor_bytes) - - def test_non_string_ticker_field_raises(self): - """MEDIUM-5: CBOR 'ticker' as list must raise.""" - cbor_bytes = cbor2.dumps({"p": [1], "ticker": [1, 2, 3]}) - with pytest.raises(ValidationError, match="must be a text string"): - decode_payload(cbor_bytes) - - def test_non_string_description_raises(self): - """MEDIUM-5: CBOR 'desc' as dict must raise.""" - cbor_bytes = cbor2.dumps({"p": [2], "desc": {"nested": "dict"}}) - with pytest.raises(ValidationError, match="must be a text string"): - decode_payload(cbor_bytes) - - def test_name_too_long_raises(self): - """MEDIUM-5: name > 64 chars must raise — prevents memory exhaustion from on-chain payloads.""" - cbor_bytes = cbor2.dumps({"p": [2], "name": "x" * 65}) - with pytest.raises(ValidationError, match="too long"): - decode_payload(cbor_bytes) - - def test_description_too_long_raises(self): - """MEDIUM-5: desc > 1000 chars must raise.""" - cbor_bytes = cbor2.dumps({"p": [2], "desc": "x" * 1001}) - with pytest.raises(ValidationError, match="too long"): - decode_payload(cbor_bytes) - - def test_ticker_too_long_raises(self): - """MEDIUM-5: ticker > 16 chars must raise.""" - cbor_bytes = cbor2.dumps({"p": [1], "ticker": "x" * 17}) - with pytest.raises(ValidationError, match="too long"): - decode_payload(cbor_bytes) - - def test_image_url_too_long_raises(self): - """MEDIUM-5: image URL > 512 chars must raise.""" - cbor_bytes = cbor2.dumps({"p": [2], "image": "https://example.com/" + "x" * 500}) - with pytest.raises(ValidationError, match="too long"): - decode_payload(cbor_bytes) + """MEDIUM-5: decode_payload field-type and length limits. + + The FAILURE MODE changed on 2026-09-02; the security property did not. + + These cases originally asserted that an unusable field RAISES. The threat + MEDIUM-5 named is type confusion — "must raise, not silently become + str(42)='42'" — and the bound exists to stop an oversized on-chain string + reaching a display path. Both are still enforced: the field is DROPPED, so + nothing is coerced and nothing oversized propagates. + + What raising cost was availability, and it cost it on somebody else's data. + One bad field discarded the ENTIRE token, and `extract_reveal_metadata` + catches Exception and returns None, so a user saw "metadata: NONE" — not a + refusal they could act on. Measured on live mainnet: 6 of 25 sampled `gly` + payloads were refused by pyrxd and decoded fine by an independent verifier, + including Photonic-minted glyphs carrying `loc` as an INTEGER. + + Reading third-party chain data is exactly where a guard that refuses honest + input is a defect. The bound belongs on what we WRITE. + """ + + @pytest.mark.parametrize( + ("field", "bad_value", "protocol"), + [ + ("name", 42, [2]), + ("ticker", [1, 2, 3], [1]), + ("desc", {"nested": "dict"}, [2]), + ], + ) + def test_a_non_string_field_is_dropped_and_NEVER_coerced(self, field, bad_value, protocol): + """The type-confusion half. `str(42)` must not appear anywhere.""" + meta = decode_payload(cbor2.dumps({"p": protocol, field: bad_value})) + attr = {"desc": "description"}.get(field, field) + assert getattr(meta, attr) == "", f"{field} should be dropped, not coerced" + assert str(bad_value) not in (getattr(meta, attr) or "") + + @pytest.mark.parametrize( + ("field", "value", "protocol"), + [ + ("name", "x" * 65, [2]), + ("desc", "x" * 1001, [2]), + ("ticker", "x" * 17, [1]), + ("image", "https://example.com/" + "x" * 500, [2]), + ], + ) + def test_an_oversized_field_is_dropped_so_nothing_oversized_PROPAGATES(self, field, value, protocol): + """The memory-exhaustion half. The bound is on what reaches a caller, and + an empty string satisfies it exactly as a refusal did.""" + meta = decode_payload(cbor2.dumps({"p": protocol, field: value})) + attr = {"desc": "description", "image": "image_url"}.get(field, field) + assert getattr(meta, attr) == "" + + def test_one_bad_field_no_longer_discards_the_WHOLE_token(self): + """The regression this change exists for: a token with one unusable field + must still yield everything else it carries.""" + meta = decode_payload(cbor2.dumps({"v": 2, "p": [2], "loc": 0, "name": "Craig", "ticker": "CRG"})) + assert meta.name == "Craig" and meta.ticker == "CRG" + assert meta.loc == "" # the offending field, and only it def test_valid_fields_at_limits_accepted(self): """Boundary: fields exactly at their limits must be accepted.""" @@ -848,9 +863,28 @@ def test_hex20_wrong_length_rejected(self): with pytest.raises(ValidationError, match="20 bytes"): Hex20(bytes(21)) - def test_glyph_media_over_limit_rejected(self): - with pytest.raises(ValidationError, match="too large"): - GlyphMedia(mime_type="image/png", data=bytes(100_001)) + def test_glyph_media_carries_chain_sized_data(self): + """GlyphMedia is only ever constructed by the DECODER, so a size cap here + refused real on-chain tokens and nothing else. Live mainnet carries webp + payloads of 153,650 / 178,608 / 236,726 bytes. + + The DoS bound lives on the encode path, at `_MAX_CBOR_PAYLOAD_BYTES` + (256 KB) — see `test_the_payload_cap_is_where_the_dos_bound_lives`. The + 100 KB cap this replaced also made that headroom unreachable for any + media-bearing token, so the two limits contradicted each other.""" + m = GlyphMedia(mime_type="image/png", data=bytes(153_650)) + assert len(m.data) == 153_650 + + def test_the_payload_cap_is_where_the_dos_bound_lives(self): + """Paired with the case above so removing one cap cannot quietly remove + the protection: media beyond the payload budget is still refused.""" + import cbor2 + + from pyrxd.glyph.payload import decode_payload + + blob = cbor2.dumps({"v": 1, "p": [2], "main": {"t": "image/webp", "b": bytes(500_000)}}) + with pytest.raises(ValidationError, match="payload too large"): + decode_payload(blob) def test_glyph_media_blank_mime_rejected(self): with pytest.raises(ValidationError, match="Invalid MIME type"):