fix: bound JSON redaction traversal - #398
codeforester wants to merge 3 commits into
Conversation
| identity = id(value) | ||
| if identity in seen: | ||
| return REDACTED | ||
| branch_seen = seen | {identity} |
There was a problem hiding this comment.
Efficiency: cycle detection copies the whole ancestor id-set at every container node (branch_seen = seen | {identity}) instead of mutating one shared set and backtracking (add/discard), so cost scales with nodes × depth rather than nodes. Measured on a synthetic payload of ~2M dict nodes at depth 99: this implementation takes ~2.75s vs ~1.48s for the same traversal using a single mutable active set — the exact pattern lib/python/base_cli/config.py::_validate_config_graph (lines 108-134) already uses for the same cycle+depth-bounded-graph problem. The PR's goal is to stop pathological input from being CPU-expensive, but a large-but-legal payload under the depth cap can still cause multi-second stalls on every JSON envelope emission. Worth reusing the mutable-set-with-backtracking pattern from config.py instead.
| if isinstance(value, tuple): | ||
| return [redact_json_value(item) for item in value] | ||
| if isinstance(value, (Mapping, list, tuple)): | ||
| if _depth >= MAX_JSON_REDACTION_DEPTH: |
There was a problem hiding this comment.
Design concern: the same "[REDACTED]" sentinel is now returned both when a secret-looking key/value is found and when the depth cap or a cycle is hit, so callers can no longer distinguish "this was a secret" from "this was just deep/cyclic structure." A command emitting a legitimately deep (>100 level) but non-sensitive structure (e.g. a recursively-generated debug tree) in details would have everything below level 100 collapse to "[REDACTED]", identical to real secret redaction — a consumer investigating a redaction can't tell a scrub from a truncation. Consider a distinct sentinel (e.g. "[TRUNCATED]") for the depth/cycle case.
This branch has not been deployed
1 participant
Fixes #395