trailofbits / trailofbits/graphtage
Diffing XML or HTML to any non-XML output format crashes
Nobody has claimed this yet.
- 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:
- Build the view from copies, using
TreeNode.copy(), so the wrapper nodes own their children. - Give the JSON formatter a print path that walks the
XMLElementdirectly 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
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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