trailofbits / trailofbits/graphtage

Diffing XML or HTML to any non-XML output format crashes

Open
#187 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
Python
Stars
2.5k
Forks
61
Avg merge
6h 45m
Merged PRs (30d)
45

Description

Diffing XML or HTML with --format set to anything other than xml or html crashes with an unhandled ValueError. Cross-format output is one of Graphtage's headline features, and it does not work in the XML direction at all.

Reproducer

Two identical files are enough:

$ printf '<r><d>x</d></r>' > a.xml
$ graphtage --no-status --format json a.xml a.xml
Traceback (most recent call last):
  ...
  File "graphtage/xml.py", line 462, in _json_print_XMLElement
    KeyValuePairNode(StringNode('tag'), node.tag),
  File "graphtage/tree.py", line 705, in wrapped
    child.parent = self
  File "graphtage/tree.py", line 454, in parent
    raise ValueError(...)
ValueError: Error while setting EditedStringNode('r').parent = KeyValuePairNode(key=StringNode('tag'), value=EditedStringNode('r')): Parent is already assigned to EditedXMLElement(...)

Scope

Every non-XML output format fails, for both XML and HTML input. The reverse direction is fine.

input xml html json json5 yaml toml csv ini plist
xml ok ok crash crash crash crash crash crash crash
html ok ok crash crash crash crash crash crash crash
json ok ok ok ok ok ok ok ok ok

The process also exits 1, which is the code for "differences found", so a script cannot tell the crash apart from a successful diff. It should be 2.

Cause

_json_print_XMLElement (graphtage/xml.py:460-470) builds a throwaway DictNode view of the element by wrapping the element's existing children in new nodes:

kvps = [KeyValuePairNode(StringNode('tag'), node.tag)]
...
self.print(printer, DictNode(kvps))

node.tag is already a child of the XMLElement, so it already has a parent. The TreeNode.parent setter (tree.py:449-455) rejects reassignment, and the constructor assigns parents to its children (tree.py:705), so the first KeyValuePairNode raises before anything is printed.

This is a regression, not an original defect. _json_print_XMLElement was written in b3da9cf (2020-05-27), when nodes had no parents. The check that rejects it arrived over two years later in 9bc2342 "Add parents to nodes" (2022-09-30), first released in v0.2.7. Nothing has exercised the path since: there is no test anywhere in test/ that renders XML through a non-XML formatter, which is why this has gone unnoticed across all six releases from v0.2.7 to v0.4.0.

I did not run v0.2.6 to confirm the before state, because releases from that era do not import on a current Python.

Fix

The printing path needs a representation it owns rather than one that re-parents live nodes. Options, roughly in increasing order of effort:

  1. Build the view from copies, using TreeNode.copy(), so the wrapper nodes own their children.
  2. Give the JSON formatter a print path that walks the XMLElement directly and emits the key/value syntax, without constructing intermediate nodes at all.

Whichever route, the regression test to add is a matrix one: render a small diff of each input filetype through every registered formatter and assert it neither raises nor exits non-zero. That would have caught this in 2022 and would cover the other filetypes at the same time.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with _json_print_XMLElement in graphtage/xml.py and the parent assignment logic in tree.py, then run the XML-to-JSON reproducer. Add a regression test under test/ covering the input and formatter matrix described in the issue. Done means XML and HTML render through every registered non-XML formatter without raising, and crashes return exit code 2 rather than 1.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
cli, testing-qa
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.