#1071: reject invalid XML element names in XML.toString (CWE-91)#1072
Open
mechko wants to merge 1 commit into
Open
#1071: reject invalid XML element names in XML.toString (CWE-91)#1072mechko wants to merge 1 commit into
mechko wants to merge 1 commit into
Conversation
XML.toString emitted JSONObject keys verbatim as tag names, so a key containing '<', '>' or '/' broke out of its element and injected arbitrary sibling structure into the output. Per stleary#294/stleary#123 the agreed approach is to throw on invalid input rather than mangle it. - Add mustBeXmlName / isXmlNameStart / isXmlNameChar implementing the XML 1.0 (5th ed.) Name production, code-point aware. - Validate tagName at method entry and each key at the top of the key loop (skipping the cDataTagName sentinel). - Rewrite XMLTest.shouldHandleIllegalJSONNodeNames and XMLConfigurationTest.shouldHandleIllegalJSONNodeNames (previously documenting the pass-through behaviour) to assert the throw. - Add XMLTest.toStringRejectsElementInjectionInKey covering the stleary#1071 payload and an invalid caller-supplied tagName. - Add XMLTest.toStringAcceptsValidXmlNames covering hyphen/dot/ underscore/colon, Latin-1 letters, and the cDataTagName sentinel. Fixes stleary#1071. Also resolves the long-standing well-formedness question in stleary#166 / stleary#294 / stleary#308. Co-Authored-By: Claude <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Fixes #1071. Implements the throw-on-invalid-name approach discussed in #294 / #123, which also resolves the long-standing well-formedness question in #166 / #308.
Problem
XML.toStringemits JSONObject keys verbatim as XML tag names. A key containing<,>or/breaks out of its element and injects arbitrary sibling structure:Values already go through
escape(); keys/tagNamedo not.Fix
Validate every key and
tagNameagainst the XML 1.0 (5th ed.)Nameproduction before emitting; throwJSONExceptionon mismatch.mustBeXmlName(String)— throws if the string is null/empty or contains a code point outside theNameproductionisXmlNameStart(int)/isXmlNameChar(int)— the two productions, code-point aware (handles supplementary planes)tagNameat the top of the privatetoStringoverload, and once perkeyat the top of the key loop (thecDataTagNamesentinel is skipped since it never becomes a tag)<,>,/, whitespace,",&,@are all outsideNameChar, so the injection vector is closed. Keys that are already valid XML Names are unaffected —mustBeXmlNameis a no-op for them.Behaviour change
XML.toStringnow throws for keys that are not valid XML Names, where previously it emitted malformed XML:"a/><injected>…"JSONException"123foo"<123foo>(invalid XML)JSONException"has space"<has space>(invalid XML)JSONException"foo@bar"<foo@bar>(invalid XML)JSONException"ns:name","a-b.c_d","élément"The two existing
shouldHandleIllegalJSONNodeNamestests already documented the old output as "invalid XML" / "possible bug"; they've been rewritten to assert the throw.Verification
mvn test: 791 run, 0 fail, 0 error, 6 skipped (pre-existing).Tests
XMLTest.shouldHandleIllegalJSONNodeNames/XMLConfigurationTest.shouldHandleIllegalJSONNodeNames— rewritten to assertJSONExceptionfor123IllegalNodeandIllegal@nodeXMLTest.toStringRejectsElementInjectionInKey— the XML.toString: unescaped keys allow XML element injection (CWE-91) — proposal to throw per #294 #1071 payload throws; invalid caller-suppliedtagNamethrowsXMLTest.toStringAcceptsValidXmlNames— hyphen/dot/underscore/colon, Latin-1 letters, andcDataTagNamesentinel still serialise🤖 Generated with Claude Code