JSON decoding ignores DecodeOptions, so element-memory and size limits bound only the protobuf codec
- Lingua principale
- Rust
- Stelle
- 883
- Fork
- 88
- Merge medio
- 3g 19h
- PR unite (30g)
- 42
Descrizione
`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.
Guida per i contributori
Apri la guida per i contributori
Valutazione
Questa issue non è ancora stata valutata.