trailofbits / trailofbits/graphtage

XML formatter drops inserted attributes and child elements

Open Beginner friendly
#186 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

Adding an attribute to an XML or HTML element that previously had no attributes drops it from the output. The attribute is silently absent from the diff, with no insertion marker and no trace that anything changed.

Reproducer

$ printf '<r><d>x</d></r>'       > b1.xml
$ printf '<r><d a="1">x</d></r>' > b2.xml
$ graphtage --no-status b1.xml b2.xml
<r><d>x</d>
</r>

Expected something like <r><d++ a="1"++>x</d></r>.

Only this case is affected. Adding an attribute to an element that already has one works, as do value changes and removals:

$ graphtage --no-status b2.xml b4.xml   # a="1" -> a="1" b="2"
<r><d a="1"++ b="2"++>x</d>
$ graphtage --no-status b2.xml b3.xml   # a="1" -> a="2"
<r><d a="1" -> "2">x</d>
$ graphtage --no-status b2.xml b1.xml   # a="1" -> (none)
<r><d~~ a="1"~~>x</d>

Cause

The differ is correct; the formatter drops the edit. The Insert is computed:

>>> for e in gx.build_tree("b1.xml").get_all_edits(gx.build_tree("b2.xml")): print(e)
Insert(to_insert=KeyValuePairNode(key=StringNode('a'), value=StringNode('1')), ...)

XMLFormatter.print_XMLElement (graphtage/xml.py:396) guards on the truthiness of the attribute node:

if node.attrib:
    self.print(printer, node.attrib)

node.attrib is the from element's DictNode, and an empty DictNode is falsy (len() == 0). So when the source element has no attributes, the attribute node is never handed to the formatter, and the insert edits attached to it are never applied. This is the failure mode described in CLAUDE.md: insert and remove edits are applied inside SequenceFormatter.print_SequenceNode, so any path that skips that call drops them.

Fix

Removing the guard is sufficient. I tested it by patching print_XMLElement in a throwaway process:

case before after
add to bare element <r><d>x</d> <r><d++ a="1"++>x</d>
element with no attributes at all <r><d>x</d> <r><d>x</d>
change a value <r><d a="1" -> "2">x</d> <r><d a="1" -> "2">x</d>

XMLElementAttribFormatter writes the leading space per key/value pair, so an element with genuinely no attributes still renders as <d> with no stray space, and the other cases are unchanged.

The same guard drops inserted child elements

The condition on the next line, if node._children._children or ... (xml.py:398), has the same shape and the same defect, and this one loses an entire subtree:

$ printf '<r><d>x</d></r>'          > c1.xml
$ printf '<r><d>x<e>y</e></d></r>'  > c2.xml
$ graphtage --no-status c1.xml c2.xml
<r><d>x</d>
</r>

<e>y</e> is gone from the output entirely. When the element starts out empty it is the same story, and the element even renders self-closing:

$ printf '<r><d></d></r>'         > c3.xml
$ printf '<r><d><e>y</e></d></r>' > c4.xml
$ graphtage --no-status c3.xml c4.xml
<r><d />
</r>

Both guards need to consider the to side as well as the from side, or hand the node to the formatter unconditionally and let print_SequenceNode decide what to emit.

A regression test should assert on the rendered diff, not on str(edit) — the edit is present either way, so an assertion over the edit list passes even while the output is wrong.

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 in graphtage/xml.py at XMLFormatter.print_XMLElement around lines 396-398, then run the XML and HTML reproducers from the issue. Add regression coverage that checks the rendered diff for inserted attributes and child elements, rather than only the edit list. Done means both insertions appear in the output while unchanged and existing-attribute cases remain unchanged.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
tooling
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
88/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.