anthropics / anthropics/buffa

JSON decoding ignores DecodeOptions, so element-memory and size limits bound only the protobuf codec

Offen
#330 1 Kommentar 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen
Vorherrschende Sprache
Rust
Sterne
883
Forks
88
Ø Merge
3 T. 19 Std.
Gemergte PRs (30 T.)
42

Beschreibung

`DecodeOptions` is not consulted anywhere on the JSON path, so a message decoded from JSON is bounded by none of the limits that bound the same message decoded from protobuf.

## The two paths

The protobuf path threads a `DecodeContext` carrying the remaining budgets through every merge call. Both `DecodeOptions::decode_from_slice` and `DecodeOptions::decode_view` build one:

```rust
let limit = core::cell::Cell::new(self.unknown_field_limit);
let elem_budget = core::cell::Cell::new(self.element_memory_limit);
V::decode_view_with_ctx(
buf,
DecodeContext::new(self.recursion_limit, &limit).with_element_memory(&elem_budget),
)
```

The JSON path has no equivalent. Generated messages get `#[derive(::serde::Serialize, ::serde::Deserialize)]` (`buffa-codegen/src/message.rs:517-522`), with a hand-written `Deserialize` where derive cannot express the shape (oneofs, extension keys). The caller invokes them through their own `serde_json::from_str`, which is the idiom the guide documents (`docs/guide.md:1434`). Grepping `recursion_limit|max_message_size|unknown_field_limit|element_memory_limit|DecodeContext|DecodeOptions` across `buffa/src/json.rs`, `buffa/src/json_helpers.rs`, `buffa/src/json_helpers/` and `buffa-descriptor/src/reflect/json.rs` returns nothing. Every terminal method on `DecodeOptions` is binary-wire only.

## Why this is more than a doc gap

`element_memory_limit` was added in 0.9.0 precisely because element footprint is not proportional to encoded size. The 0.9.0 changelog entry makes the case in wire terms:

> an empty repeated message element is 2 bytes on the wire and `size_of::()` in the `Vec` it lands in — measured at 256 bytes for a message of a few `Vec`/`String` fields, a 128x ratio, so 4 MiB of them forced ~512 MiB

None of that reasoning is protobuf-specific. Against the same `repeated Payload items` field, `{},` is three JSON bytes for the same ~256 bytes of `Vec` footprint — about 85x, versus 128x on the wire. The JSON encoding is within a small constant of the exact case the limit was introduced to stop, and has no ceiling at all. `""` into `repeated string` and `"a":{}` into `map` amplify by the same route.

serde_json's own 128-deep recursion default is the one accidental substitute, and it is a poor one: it is looser than buffa's own `RECURSION_LIMIT` of 100, it is not reachable through `DecodeOptions`, and it disappears entirely for a non-`serde_json` `Deserializer` — this repo already ships one in `buffa-yaml`.

## Documentation that currently reads the wrong way

- `DecodeOptions`' rustdoc says "Use this to set custom recursion depth limits or maximum message sizes when decoding from untrusted input" without naming a codec. With `json` enabled, that is a reasonable thing to believe and it is not true.
- The 0.9.0 changelog says "The owned, view and reflective (`DynamicMessage`) decoders are all bounded." True of the binary decoders; `DynamicMessage::from_json` is not bounded.
- The guide's decode-options table (`docs/guide.md:1050-1056`) does not list `with_element_memory_limit` at all, and the JSON section says nothing about limits.

## What a fix has to get past, and what already exists

buffa has already hit this constraint and documented it, in `buffa/src/json.rs`:

> Serde's `Deserialize` trait has no context parameter, so runtime options must be passed through ambient state.

That module exists because of it: `JsonParseOptions` is carried by a thread-local on `std` (`with_json_parse_options`) and a leaked `AtomicPtr` global on `no_std` (`set_global_json_parse_options`). So there is a working precedent for getting per-call state into the derive-generated deserializers, which makes this cheaper than it first looks.

The two surfaces differ a lot:

**Reflective JSON is a contained, additive fix.** `DynamicMessage::from_json` owns its `serde_json::Deserializer` and already goes through a real seed, `DynamicMessageSeed`, whose doc says new parse options belong on it as builder-style setters. A budget setter plus charging in the seq/map visitors covers it.

**Generated-message JSON is the hard half**, because buffa never sees the call. Options, in increasing cost:

1. **Extend `JsonParseOptions`** with the limit fields and charge from the container visitors in `buffa/src/json_helpers.rs` (the `Vec::with_capacity(clamp_size_hint(..))` sites, the map visitors, `DefaultDeserializeSeed`/`NullableDeserializeSeed`). Reuses machinery that already exists. Inherits its warts: ambient state, and the `no_std` variant is process-wide set-once.
2. **Add a buffa-owned entry point** — `DecodeOptions::decode_json_from_slice::` — that installs the budget for the duration of the call and caps input size first. Additive, but opt-in: the documented idiom stays unbounded, so most callers stay unprotected.
3. **Move generated JSON off serde-derive** to a seed-based codec, as the reflective path already uses. This is the rewrite. It changes the public shape of generated code, and it would also remove the `serde_json::Value` buffering the extension path does today.

I do not have a strong view between these, and the choice looks like a real design decision rather than an obvious one. What I would argue is that doing nothing is not tenable while `element_memory_limit` is presented as codec-independent — at minimum `DecodeOptions`, `with_element_memory_limit` and the guide should say which codec they bound.

One thing that does *not* carry over, for scope: unknown-field amplification. `__buffa_unknown_fields` is `#[serde(skip)]` for messages without extension ranges, and unknown JSON keys are dropped rather than materialized.

## Where this came from

Found while wiring the 0.9 element-memory budget through to user-facing configuration in connect-rust (connectrpc/connect-rust#235 for the server, #236 for the client). Both sides normalize JSON to protobuf bytes and decode *that* under the caller's limits, which is enough to make the knob behave the same on both codecs, but the `serde_json` parse ahead of it is unbounded and only the second materialization is charged.

Beitragsleitfaden

Beitragsleitfaden öffnen

Bewertung

Dieses Issue wurde noch nicht bewertet.

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.