Skip to content

Standardize depth enforcement. - #2092

Open
thomasvl wants to merge 3 commits into
apple:mainfrom
thomasvl:json_depth_enforcement
Open

Standardize depth enforcement.#2092
thomasvl wants to merge 3 commits into
apple:mainfrom
thomasvl:json_depth_enforcement

Conversation

@thomasvl

@thomasvl thomasvl commented Jun 5, 2026

Copy link
Copy Markdown
Collaborator

Use defer to avoid accidentally missing a call on a return path.

This also counts "Value" as an object; that effectively halves the depth of some raw JSON parsed into a "Struct", but it was allowing what might be a stack bomb attempt to be parsed in some cases.

@thomasvl
thomasvl requested review from FranzBusch, Lukasa and tbkka June 5, 2026 14:18
@thomasvl thomasvl added the 🆕 semver/minor Adds new public API. label Jun 5, 2026
@thomasvl

thomasvl commented Jun 5, 2026

Copy link
Copy Markdown
Collaborator Author

@Lukasa @FranzBusch @tbkka - I'm putting this up in draft for thoughts. The Protobuf JSON spec doesn't formally say what counts for depth, but we already had "Struct" and "List" counting, "Value" is the potential gap here. Since this would have impact on things that current parse, but could be dangerous, I'm not sure if we should change things, or just let it go. Thoughts?

@thomasvl
thomasvl force-pushed the json_depth_enforcement branch from 5137862 to 9bf0795 Compare June 5, 2026 14:45
@FranzBusch

Copy link
Copy Markdown
Member

Looks in general fine to me.

@thomasvl

thomasvl commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

@Lukasa @tbkka Any thoughts/concerns on what I flagged in that first comment?

Use `defer` to avoid accidentally missing a call on a `return` path.

This also counts "Value" as an object; that effectively halves the depth of some
raw JSON parsed into a "Struct", but it was allowing what might be a stack bomb
attempt to be parsed in some cases.
@thomasvl
thomasvl force-pushed the json_depth_enforcement branch from 9bf0795 to 19c57c6 Compare July 8, 2026 18:15
@thomasvl
thomasvl marked this pull request as ready for review August 5, 2026 16:26
@thomasvl
thomasvl enabled auto-merge (squash) August 5, 2026 16:27
@thomasvl

Copy link
Copy Markdown
Collaborator Author

@Lukasa ok to land? Please a look at my first comment about the one concern I had.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🆕 semver/minor Adds new public API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants