Xml: reject attributes that only exist because of ANTLR error recovery

[timtebeek]

What's wrong

XMLParser.g4 applies attribute : Name '=' STRING to whatever the lexer produces. On malformed markup ANTLR error recovery reaches that rule with tokens that were never written, and XmlParserVisitor.visitAttribute built an Xml.Attribute from the result without checking. That goes wrong three different ways:

Input	Recovery	Result
<t k="" abc "v"/>	inserts the missing =	XmlPrinter re-emits it — the document reprints as <t k="" abc="v"/>
<t k="" a b "v"/>	drops the rule's children	c.EQUALS() is null → NullPointerException in convert
<t k="" x = 1 "v"/>	resyncs past the 1	silently accepted as k="" plus an invented x="v"

The third is the one that matters. The dropped source survives in the neighbouring prefix, so the document reprints byte-for-byte and requirePrintEqualsInput never fires. The file is parsed, cached, and searched with an attribute value that is not what the source says.

Why it shows up in practice

The usual trigger is an embedded expression containing unescaped double quotes — common in generated or hand-written config that carries a scripting snippet in an attribute:

<set-variable name="allowedIps" value="@{
    string ips = "";
    ips += "10.0.0.1,";
    ips += "10.0.0.2";
    return ips;
}" />

STRING is '"' ~[<"]* '"', so this value is not representable — XML 1.0 §2.3 does not permit a literal " inside a "-delimited value, and xmllint rejects the file too. The lexer therefore re-tokenises the value into alternating STRING runs with stray tokens in the gaps, and the table above decides which symptom you get. Which one is essentially arbitrary: it depends only on the token shape of whatever sits inside the expression's string literals.

On main today, that document parses without error, reprints byte-for-byte, and yields:

[set-variable name=<allowedIps>]
[set-variable value=<@{
    string ips = >]

The value stops at the first quote — the addresses are not in the LST at all. A search recipe reading value finds nothing and reports a clean result. Worse, a recipe that writes the attribute writes the truncated value back: ChangeTagAttribute("set-variable", "value", "REDACTED") currently emits

<set-variable name="allowedIps" value="REDACTED"";
    ips += "10.0.0.1,";
    ips += "10.0.0.2";
    return ips;
}" />

which is no longer valid XML.

The fix

Require that an attribute is built only from tokens that actually appeared in the source, and that nothing but whitespace separates them:

• each of Name, EQUALS, STRING must be present and not an ANTLR ErrorNode (recovery marks inserted tokens as error nodes, so this catches the invented = and the null dereference)
• the attribute's prefix, beforeEquals, and the value's prefix must be whitespace only (this catches tokens that recovery dropped, which is what silently truncates the value)
• the same whitespace requirement applies to the trailing prefix of each rule that accepts attributes — an element's beforeTagDelimiterPrefix, and the equivalents in visitXmldecl and visitJspdirective — for the case where recovery produces no attribute at all and the stray tokens are swallowed before the closing delimiter

Otherwise the parse fails with a message naming the actual problem:

Malformed attribute in policy.xml at line 3, column 9. The markup here is not a well-formed
name="value" attribute; common causes are a literal '"' inside a double-quoted value, which
XML 1.0 section 2.3 does not permit (write it as " or delimit the value with single
quotes), an unquoted value, or an HTML-style attribute written without a value.

The separator check matches XML 1.0's S production (#x20 | #x9 | #xD | #xA) exactly, via
StringUtils.indexOfNonWhitespace, rather than StringUtils.isBlank — which also accepts \f, vertical tab
and non-ASCII Unicode spaces. Inside a tag the lexer skips only [ \t\r\n] (XMLLexer.g4, mode INSIDE),
so a separator holding any other "whitespace" is not whitespace the lexer consumed; it is a character it
failed to match and dropped. Both halves are needed, and neither alone is sufficient. The whitespace checks catch tokens recovery dropped — that is what silently truncates the value. The ErrorNode check catches tokens recovery inserted; there is nothing wrong with any prefix in <t k="" abc "v"/>, so a whitespace check cannot see it. Disabling either half locally lets two of the six regression cases through.

XmlPrinter.visitAttribute still appends '=' unconditionally. With this check in place no Xml.Attribute can reach it without a real = in the source, so it is no longer reachable as a bug; making the = optional in the LST would be a much larger change for no gain today.

Behaviour change

Documents matching the third row above previously parsed into an Xml.Document and now become a ParseError. That is the point of the change — they were being parsed into a tree that did not describe the source — but it does mean files that used to ingest "successfully" will start reporting. The first two rows already failed, just less legibly (is not print idempotent, or an NPE); they now carry an actionable message.

Well-formed spellings are unaffected and keep their full value, so there is a clear path for anyone hitting this:

<set-variable value='@{ string ips = ""; ips += "10.0.0.1,"; return ips; }'/>
<set-variable value="@{ string ips = &quot;10.0.0.1&quot;; return ips; }"/>

Notes on provenance

The invented = and the NullPointerException date to the original XML module and only became visible as parse failures once requirePrintEqualsInput landed in 8.1.14 (#3459).

The silent truncation is newer. Bisecting released versions (8.1.14 → 8.91.0-SNAPSHOT) puts it at 8.84.4: before that the same input failed loudly, after it the file is quietly accepted with a truncated value. The only rewrite-xml change in that range is #7906, which left-factored the element rule to support HTML void elements. The feature itself is correctly gated behind htmlMode, but the grammar restructuring it needed is not, so it changed ANTLR's error recovery for the attribute* loop in strict XML too. Worth keeping in mind for future grammar refactors — a change that looks purely structural can move which malformed inputs get rejected.

Testing

Added to XmlParserTest:

• malformedAttributeIsNotSilentlyAccepted — six recovery shapes (including an orphan string in a tag and in an xmldecl) become a ParseError, still print verbatim, and carry the new message
• attributeValueWithUnescapedQuotes — the embedded-expression document above, asserting the message
• wellFormedEmbeddedExpressionsKeepTheirFullValue — " and single-quoted delimiters keep the whole value

:rewrite-xml:test and :rewrite-maven:test pass. The pre-existing malformedBareAmpersandDoesNotThrow / malformedUnterminatedEndTagDoesNotThrow / malformedMissingRootCloseDoesNotThrow cases still pass, so lenient handling of other malformed input is unchanged.

On the separator character set specifically: isBlank and indexOfNonWhitespace differ only on
all-whitespace strings containing at least one of 21 characters (\f, vertical tab, and 19 non-ASCII
whitespace characters). No file under rewrite-xml/src/test or rewrite-maven/src/test contains any of
them, raw or as a Java escape, apart from the case added here — so no existing test input can take a
different path.

Two checks on the blast radius of the added strictness:

• Parsing 12,409 XML and POM files from a local ~/.m2/repository (1,720) and Gradle module cache (10,689) with the patched parser produces zero parse errors, so the guards do not fire on ordinary well-formed XML.
• Differentially probing the old and new visitor over shapes the guards newly reject — HTML boolean attributes, unquoted values, JSP dynamic attributes, <root a/> — shows every one of them was already a ParseError before this change. For those the PR replaces an NPE or an idempotency failure with a readable message. The only previously-successful shapes that now fail are genuinely malformed: <t a=="1"/>, and orphan strings inside a tag.

[MBoegers]

We are only checking if StringUtils.isBlank isn't there a more specific checks of well formed?

[timtebeek]

Good push — there is a more specific check available, and I've switched to it in 40e97b3.

To explain why it started as a blankness check: the four gaps being guarded are exactly XML 1.0's S production, so "holds only whitespace" is the well-formedness condition there rather than a proxy for it.

STag      ::= '<' Name (S Attribute)* S? '>'      -- attribute prefix, beforeTagDelimiterPrefix
Attribute ::= Name Eq AttValue
Eq        ::= S? '=' S?                           -- beforeEquals, value prefix
S         ::= (#x20 | #x9 | #xD | #xA)+

It is also the only check that can see lexer-level dropped characters. Every whitespace rule in the grammar is -> skip, and an unmatched character raises a lexer error and is consumed without ever producing a token — so nothing token-based (adjacency, child count, ErrorNode) can observe it. That is precisely the <t k="" x = 1 "v"/> case: 1 is not a NameStartChar, never enters the token stream, and silently relocates into a prefix. The isSourceToken half catches tokens recovery inserted; the whitespace half catches text it dropped. Neither alone is sufficient — disabling either lets cases through.

But you're right that the character set was wrong. StringUtils.isBlank also accepts \f, vertical tab, and non-ASCII Unicode spaces, while inside a tag the lexer skips only:

mode INSIDE;
S : [ \t\r\n] -> skip ;      // XMLLexer.g4:130

So a gap holding \f was never whitespace the lexer consumed — it is a character it failed to match and dropped, i.e. exactly what this guard exists to catch, waved through. Now using StringUtils.indexOfNonWhitespace, whose character set already matches that rule and the spec.

Differential before/after, all previously accepted:

<t k=""\fabc="v"/>      ACCEPTED  ->  REJECTED
<t k="" \fabc="v"/>     ACCEPTED  ->  REJECTED
<t\fk="v"/>             ACCEPTED  ->  REJECTED
<t k="v"\f/>            ACCEPTED  ->  REJECTED
<t k="v"\v/>            ACCEPTED  ->  REJECTED
<t k="v"　/>        ACCEPTED  ->  REJECTED
<t k=""abc="v"/>        ACCEPTED  ->  ACCEPTED   (deliberate, see below)

Two things I considered and deliberately left out:

Requiring a non-empty separator. XML's (S Attribute)* makes the S mandatory, so <a b="1"c="2"/> is malformed and still parses today. I left it: the LST is faithful and reprints byte-for-byte, so there is no corruption to prevent, and it is unrelated to this bug.

Failing on any ANTLR syntax error. More direct, but no parser in this repo does it — all six ANTLR modules forward to ctx.getOnError() and record nothing, so parseInputs cannot learn an error occurred. It would need a flag shared across the two listener instances, and it is far broader than these five positions: it would also reject the content-level and closing-tag recoveries XmlParserVisitor deliberately tolerates.

Blast radius unchanged: 12,409 XML and POM files from a local ~/.m2/repository and Gradle module cache still parse with zero errors under the stricter set.