Tighten JSON parser#7896
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a single, depth-bounded JSON parsing entrypoint (ccf::parse_json_safe) and switches a broad set of parsing call sites over to it, preventing excessively nested JSON inputs from being parsed throughout the CCF stack (core, governance handlers, JWT handling, samples, and KV JSON serialisation).
Changes:
- Add
ccf::parse_json_safeand aMAX_JSON_NESTING_DEPTHlimit (64) to reject overly nested JSON early. - Replace multiple
nlohmann::json::parse(...)call sites withccf::parse_json_safe(...). - Add unit tests for the new parsing chokepoint and document the behavioural change in
CHANGELOG.md.
Custom instructions used:
.github/copilot-instructions.md.github/instructions/reviewing.instructions.md
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/pal/attestation.cpp |
Use parse_json_safe when decoding quote JSON. |
src/node/uvm_endorsements.cpp |
Use parse_json_safe for JWK and COSE payload JSON parsing. |
src/node/snapshot_serdes.h |
Use parse_json_safe when parsing receipt JSON from snapshot segments. |
src/node/quote.cpp |
Use parse_json_safe for quote/JWK JSON parsing. |
src/node/quote_endorsements_client.h |
Use parse_json_safe for THIM JSON endorsements parsing. |
src/node/node_state.h |
Use parse_json_safe when parsing join error bodies; adds a specific catch (needs adjustment). |
src/node/jwt_key_auto_refresh.h |
Use parse_json_safe for JWKS and issuer metadata parsing. |
src/node/gov/handlers/service_state.h |
Use parse_json_safe for quote-info details and proposal operation parsing. |
src/node/gov/handlers/recovery.h |
Use parse_json_safe for COSE identity content parsing. |
src/node/gov/handlers/proposals.h |
Use parse_json_safe for ballot parameter parsing. |
src/node/gov/handlers/acks.h |
Use parse_json_safe for signed body parsing in acks handler. |
src/js/extensions/ccf/gov_effects.cpp |
Use parse_json_safe for metadata/JWKS JSON parsing. |
src/js/extensions/ccf/crypto.cpp |
Use parse_json_safe when parsing incoming JWK strings. |
src/http/http_jwt.h |
Use parse_json_safe for JWT header/payload parsing and add clearer error for depth-limit failures. |
src/endpoints/json_handler.cpp |
Use parse_json_safe for request body JSON parsing. |
src/ds/test/parse_json_safe.cpp |
New unit tests validating depth limit and error behaviour for parse_json_safe. |
samples/apps/programmability/programmability.cpp |
Use parse_json_safe for sample app request/body parsing. |
samples/apps/logging/logging.cpp |
Use parse_json_safe for sample app request body parsing. |
samples/apps/basic/basic.cpp |
Use parse_json_safe for sample app request body parsing. |
include/ccf/kv/serialisers/json_serialiser.h |
Use parse_json_safe when deserialising JSON-serialised KV entries. |
include/ccf/json_handler.h |
Update documentation snippet to reference parse_json_safe. |
include/ccf/ds/json.h |
Add MAX_JSON_NESTING_DEPTH, JsonTooDeep, callback, and parse_json_safe overloads. |
CMakeLists.txt |
Register new parse_json_safe_test unit test. |
CHANGELOG.md |
Document new JSON nesting-depth rejection (missing PR reference). |
| } | ||
| }; | ||
|
|
||
| inline constexpr int MAX_JSON_NESTING_DEPTH = 64; |
There was a problem hiding this comment.
It's good to have a default but it should be possible to override this at the call site (parse_json_safe) without having to touch CCF.
There was a problem hiding this comment.
@copilot implement this as func param, which witll default to this constant, but can be overwritten at a call site, similar to what is done for cbor parser.
It's a (good) fix for
nlohmann::json::parse, as long as we don't create unauthenticated payloads with other interfaces, e.g.nlohmann::json::from_* (), which we both do not do and do not plan to do.For QuickJS interpreter, we have stack size set here. It only becomes an issue, if someone sets 0 explicitly AND disables
no_lower_than_defaults, in which case they should know what they're doing. Otherwise it will just throw a C++ exception back.