Skip to content

Fix paths for bytes dictionary keys - #640

Open
x0Lazarus wants to merge 1 commit into
qlustered:devfrom
x0Lazarus:fix/bytes-dictionary-paths
Open

x0Lazarus wants to merge 1 commit into
qlustered:devfrom
x0Lazarus:fix/bytes-dictionary-paths

Conversation

@x0Lazarus

Copy link
Copy Markdown

DeepDiff({b"foo": 1}, {b"foo": 2}) raises TypeError while building the path for the changed value. The original example in #472 can appear to work now because the default dictionary threshold reports the whole dictionary as changed; comparing values under the same bytes key still reaches the broken path handling.

This reports the change at root[b'foo'] and keeps the resulting paths usable by parse_path, extract, and Delta. Bytes keys are written as Python bytes literals, and the parser handles their escaped quotes and backslashes. The regression cases cover nested additions, removals, and value changes with empty keys, both quote types, trailing backslashes, and non-ASCII bytes, including forward and reverse Delta application.

This builds on the bytes-literal approach proposed by Mukller in #614, with the parsing changes needed to make escaped keys round-trip. Memoryview behavior is unchanged.

Validation on Windows with Python 3.12.14:

  • Path and model suites: 79 passed. The 33 new bytes-key cases fail against unchanged dev.
  • Broader suite, with security-specific tests excluded: 1,342 passed, 8 skipped, 2 deselected, and one existing failure in test_delta_dump_and_read3. The same Windows text-mode pickle failure reproduces on unchanged dev.
  • Targeted Pyright and the blocking flake8 checks pass. Six existing style findings are unchanged. Other Python versions and Linux were not tested locally.

Fixes #472.

@seperman seperman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static review: #640 Fix paths for bytes dictionary keys

Summary: stringify_element now writes bytes keys as Python bytes literals (repr) instead of hitting "'" in b'...', which raised a TypeError. The path tokenizer now honours backslash escapes inside b'...'/b"...", so these paths round-trip through parse_path, extract and Delta. This fixes the crash reported in #472, which still happens on dev whenever a path under a bytes key is built, e.g. DeepDiff({b'foo': 1}, {b'foo': 2}).

Overlap with other PRs

  • #614 (closed without merging, targeted master) only added repr() for bytes/memoryview in stringify_element. That alone breaks round-tripping for keys that contain quotes or backslashes. This PR includes that idea and adds the parser side, so it covers more than #614 did.
  • I found no other open or merged PR touching path.py/stringify_element, and nothing on dev addresses #472.

Security

Path strings can come from untrusted Delta files, so I checked whether the new parsing widens what a path can reach:

  • literal_eval was already applied to most path elements on dev. It is now also applied to elements that start with b'/b" and contain a backslash (deepdiff/path.py:61). The tokenizer hands an element to _add_to_elements as soon as its closing quote is seen (path.py:111-123). So such an element is either one complete bytes literal, which evaluates to bytes, or an unterminated one: literal_eval fails and it stays a plain str, as on dev. literal_eval cannot execute code.
  • The only new element type is bytes, and bytes cannot act as attribute names: getattr/setattr with a bytes name raise TypeError. The setattr(prev_obj, str(prev_elem), ...) fallback (path.py:217) turns bytes into "b'...'", which can never be a dunder. So the GETATTR surface is unchanged.
  • The escape handling only changes state while inside a quoted bytes literal. At worst, a crafted escape merges the rest of the path into a single GET element. It cannot create new GETATTR segments. check_elem still runs on every final element in _get_nested_obj, _get_nested_obj_and_force and Delta's _get_elements_and_details / _get_elem_and_compare_to_old_value.
  • The added code is a linear character scan with no regex, imports or I/O. The only non-ASCII character in the diff is the existing 𝆺𝅥𝅯 marker (U+1D1C0); there are no bidi or zero-width characters.

No security concerns found in static review.

Correctness

  • path.py:325-326: the bytes short-circuit sits before the in checks that raised. repr already chooses the quote style and escapes, so no further quoting is needed.
  • path.py:118: inside_bytes is computed from the text collected before the opening quote. DeepDiff always renders str dict keys starting with ' or ", so string keys such as "b'foo'" are not mistaken for bytes. test_string_key_path_round_trip covers this.
  • stringify_path (path.py:365-368) still formats bytes elements through the f-string, i.e. str(bytes). The output happens to match repr, but under python -b/-bb it emits or raises BytesWarning. Delta uses stringify_path for flat rows and error messages.
  • memoryview keys still produce a path that doesn't round-trip (<memory at 0x…>). As the description says, that is out of scope.

Tests

  • test_bytes_key_paths (tests/test_path.py:94) has 33 cases: added, removed and changed, with empty keys, both quote types, trailing and doubled backslashes, non-ASCII bytes, and ]/. inside keys. Each case checks the reported path, parse_path, stringify_path, extract and bidirectional Delta. Reading dev, these should all fail with the TypeError while the path is built.
  • test_string_key_path_round_trip (tests/test_path.py:126) should already pass on dev. It is a useful guard against the new bytes handling catching string keys.

Suggestions

  • In stringify_path, route bytes through stringify_element too (e.g. isinstance(element, (str, bytes))), so model.py and Delta format bytes the same way and avoid BytesWarning under -b.
  • Optionally add a test showing that an unterminated or malformed bytes literal followed by .attr parses into one GET element, not a GETATTR. That would pin down the security property above.

AI-assisted static review; the code was not executed.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants