From e2cfb5a64150455bf63ad334da9827e7671c9133 Mon Sep 17 00:00:00 2001 From: Mirko Swillus Date: Thu, 23 Jul 2026 14:36:42 +0200 Subject: [PATCH 1/3] #1071: reject invalid XML element names in XML.toString (CWE-91) 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 #294/#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 #1071 payload and an invalid caller-supplied tagName. - Add XMLTest.toStringAcceptsValidXmlNames covering hyphen/dot/ underscore/colon, Latin-1 letters, and the cDataTagName sentinel. Fixes #1071. Also resolves the long-standing well-formedness question in #166 / #294 / #308. Co-Authored-By: Claude --- src/main/java/org/json/XML.java | 76 ++++++++++++++++++ .../org/json/junit/XMLConfigurationTest.java | 43 +++++----- src/test/java/org/json/junit/XMLTest.java | 80 ++++++++++++++----- 3 files changed, 159 insertions(+), 40 deletions(-) diff --git a/src/main/java/org/json/XML.java b/src/main/java/org/json/XML.java index 32475876c..c924a8179 100644 --- a/src/main/java/org/json/XML.java +++ b/src/main/java/org/json/XML.java @@ -232,6 +232,75 @@ public static void noSpace(String string) throws JSONException { } } + /** + * Throw an exception if the string is not a valid XML 1.0 {@code Name} + * (element name). Used by {@link #toString(Object)} to reject JSON keys that + * would otherwise be emitted verbatim between {@code <} and {@code >} and + * could break out of the tag context (element injection, CWE-91; + * see issue #1071 and #294). + * + * @param string the candidate element name + * @throws JSONException if {@code string} is null, empty, or contains a + * character outside the XML 1.0 Name production + */ + static void mustBeXmlName(String string) throws JSONException { + if (string == null || string.isEmpty()) { + throw new JSONException("'" + string + + "' is not a valid XML element name."); + } + int cp = string.codePointAt(0); + if (!isXmlNameStart(cp)) { + throw new JSONException("'" + string + + "' is not a valid XML element name."); + } + for (int i = Character.charCount(cp); i < string.length(); i += Character.charCount(cp)) { + cp = string.codePointAt(i); + if (!isXmlNameChar(cp)) { + throw new JSONException("'" + string + + "' is not a valid XML element name."); + } + } + } + + /** + * XML 1.0 (5th ed.) {@code NameStartChar} production. + * + * @param cp a Unicode code point + * @return true if {@code cp} may start an XML Name + */ + private static boolean isXmlNameStart(int cp) { + return cp == ':' || cp == '_' + || (cp >= 'A' && cp <= 'Z') + || (cp >= 'a' && cp <= 'z') + || (cp >= 0xC0 && cp <= 0xD6) + || (cp >= 0xD8 && cp <= 0xF6) + || (cp >= 0xF8 && cp <= 0x2FF) + || (cp >= 0x370 && cp <= 0x37D) + || (cp >= 0x37F && cp <= 0x1FFF) + || (cp >= 0x200C && cp <= 0x200D) + || (cp >= 0x2070 && cp <= 0x218F) + || (cp >= 0x2C00 && cp <= 0x2FEF) + || (cp >= 0x3001 && cp <= 0xD7FF) + || (cp >= 0xF900 && cp <= 0xFDCF) + || (cp >= 0xFDF0 && cp <= 0xFFFD) + || (cp >= 0x10000 && cp <= 0xEFFFF); + } + + /** + * XML 1.0 (5th ed.) {@code NameChar} production. + * + * @param cp a Unicode code point + * @return true if {@code cp} may appear after the first character of an XML Name + */ + private static boolean isXmlNameChar(int cp) { + return isXmlNameStart(cp) + || cp == '-' || cp == '.' + || (cp >= '0' && cp <= '9') + || cp == 0xB7 + || (cp >= 0x0300 && cp <= 0x036F) + || (cp >= 0x203F && cp <= 0x2040); + } + /** * Scan the content following the named tag, attaching it to the context. * @@ -968,6 +1037,10 @@ private static String toString(final Object object, final String tagName, final JSONObject jo; String string; + if (tagName != null) { + mustBeXmlName(tagName); + } + if (object instanceof JSONObject) { // Emit @@ -986,6 +1059,9 @@ private static String toString(final Object object, final String tagName, final // don't use the new entrySet accessor to maintain Android Support jo = (JSONObject) object; for (final String key : jo.keySet()) { + if (!key.equals(config.getcDataTagName())) { + mustBeXmlName(key); + } Object value = jo.opt(key); if (value == null) { value = ""; diff --git a/src/test/java/org/json/junit/XMLConfigurationTest.java b/src/test/java/org/json/junit/XMLConfigurationTest.java index e8ff3b60c..1a1a2fd58 100755 --- a/src/test/java/org/json/junit/XMLConfigurationTest.java +++ b/src/test/java/org/json/junit/XMLConfigurationTest.java @@ -506,32 +506,31 @@ public void shouldHandleNestedArraytoString() { /** - * Possible bug: - * Illegal node-names must be converted to legal XML-node-names. - * The given example shows 2 nodes which are valid for JSON, but not for XML. - * Therefore illegal arguments should be converted to e.g. an underscore (_). + * JSON keys that are not valid XML 1.0 Names are rejected by + * XML.toString rather than being emitted as (malformed) tag names. + * See issues #166, #294, #308 and #1071. */ @Test public void shouldHandleIllegalJSONNodeNames() { - JSONObject inputJSON = new JSONObject(); - inputJSON.append("123IllegalNode", "someValue1"); - inputJSON.append("Illegal@node", "someValue2"); - - String result = XML.toString(inputJSON, null, - XMLParserConfiguration.KEEP_STRINGS); - - /* - * This is invalid XML. Names should not begin with digits or contain - * certain values, including '@'. One possible solution is to replace - * illegal chars with '_', in which case the expected output would be: - * <___IllegalNode>someValue1someValue2 - */ - String expected = "<123IllegalNode>someValue1someValue2"; - - assertEquals("Length", expected.length(), result.length()); - assertTrue("123IllegalNode", result.contains("<123IllegalNode>someValue1")); - assertTrue("Illegal@node", result.contains("someValue2")); + // Name may not start with a digit + try { + XML.toString(new JSONObject().append("123IllegalNode", "someValue1"), + null, XMLParserConfiguration.KEEP_STRINGS); + fail("expected JSONException for digit-leading element name"); + } catch (JSONException expected) { + assertTrue("message names the offending key", + expected.getMessage().contains("123IllegalNode")); + } + // Name may not contain '@' + try { + XML.toString(new JSONObject().append("Illegal@node", "someValue2"), + null, XMLParserConfiguration.KEEP_STRINGS); + fail("expected JSONException for '@' in element name"); + } catch (JSONException expected) { + assertTrue("message names the offending key", + expected.getMessage().contains("Illegal@node")); + } } /** diff --git a/src/test/java/org/json/junit/XMLTest.java b/src/test/java/org/json/junit/XMLTest.java index 589536fd2..e67b9de59 100644 --- a/src/test/java/org/json/junit/XMLTest.java +++ b/src/test/java/org/json/junit/XMLTest.java @@ -538,31 +538,75 @@ public void shouldHandleNestedArraytoString() { /** - * Possible bug: - * Illegal node-names must be converted to legal XML-node-names. - * The given example shows 2 nodes which are valid for JSON, but not for XML. - * Therefore illegal arguments should be converted to e.g. an underscore (_). + * JSON keys that are not valid XML 1.0 Names are rejected by + * XML.toString rather than being emitted as (malformed) tag names. + * See issues #166, #294, #308 and #1071. */ @Test public void shouldHandleIllegalJSONNodeNames() { - JSONObject inputJSON = new JSONObject(); - inputJSON.append("123IllegalNode", "someValue1"); - inputJSON.append("Illegal@node", "someValue2"); + // Name may not start with a digit + try { + XML.toString(new JSONObject().append("123IllegalNode", "someValue1")); + fail("expected JSONException for digit-leading element name"); + } catch (JSONException expected) { + assertTrue("message names the offending key", + expected.getMessage().contains("123IllegalNode")); + } + // Name may not contain '@' + try { + XML.toString(new JSONObject().append("Illegal@node", "someValue2")); + fail("expected JSONException for '@' in element name"); + } catch (JSONException expected) { + assertTrue("message names the offending key", + expected.getMessage().contains("Illegal@node")); + } + } - String result = XML.toString(inputJSON); + /** + * A JSON key containing XML metacharacters must not be emitted as a raw + * tag name, since doing so allows the key to break out of its element and + * inject sibling structure into the output (CWE-91, issue #1071). + */ + @Test + public void toStringRejectsElementInjectionInKey() + { + JSONObject jo = new JSONObject( + "{\"a/>evil' are outside the XML Name production + } - /* - * This is invalid XML. Names should not begin with digits or contain - * certain values, including '@'. One possible solution is to replace - * illegal chars with '_', in which case the expected output would be: - * <___IllegalNode>someValue1someValue2 - */ - String expected = "<123IllegalNode>someValue1someValue2"; + // caller-supplied tagName is validated too + try { + XML.toString(new JSONObject(), "bad tag"); + fail("expected JSONException for tagName containing whitespace"); + } catch (JSONException expected) { + // expected: space is outside the XML Name production + } + } - assertEquals("length",expected.length(), result.length()); - assertTrue("123IllegalNode",result.contains("<123IllegalNode>someValue1")); - assertTrue("Illegal@node",result.contains("someValue2")); + /** + * Keys that are valid XML 1.0 Names continue to serialise unchanged. + */ + @Test + public void toStringAcceptsValidXmlNames() + { + JSONObject jo = new JSONObject(); + jo.put("simple", "a"); + jo.put("with-hyphen.dot_underscore", "b"); + jo.put("ns:qualified", "c"); + jo.put("élément", "d"); // Latin-1 letters + jo.put("content", "e"); // cDataTagName sentinel, emitted as text + String xml = XML.toString(jo, "root"); + assertTrue(xml.contains("a")); + assertTrue(xml.contains("b")); + assertTrue(xml.contains("c")); + assertTrue(xml.contains("<élément>d")); + assertTrue("cDataTagName still emitted as text content", xml.contains(">e<")); } /** From 6b993e2e47b9a328a9d166a720b33fa7e3e58848 Mon Sep 17 00:00:00 2001 From: Mirko Swillus Date: Fri, 24 Jul 2026 10:57:24 +0200 Subject: [PATCH 2/3] #1071: extract inRange helper to satisfy Sonar S3776 isXmlNameStart's alternating &&/|| chain scored cognitive complexity 28. Extracting inRange(cp, lo, hi) collapses it to a flat || sequence and keeps the range list 1:1 with the XML 1.0 NameStartChar production. isXmlNameChar updated the same way. No behaviour change. Co-Authored-By: Claude --- src/main/java/org/json/XML.java | 44 +++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 19 deletions(-) diff --git a/src/main/java/org/json/XML.java b/src/main/java/org/json/XML.java index c924a8179..5191cdf97 100644 --- a/src/main/java/org/json/XML.java +++ b/src/main/java/org/json/XML.java @@ -262,6 +262,10 @@ static void mustBeXmlName(String string) throws JSONException { } } + private static boolean inRange(int cp, int lo, int hi) { + return cp >= lo && cp <= hi; + } + /** * XML 1.0 (5th ed.) {@code NameStartChar} production. * @@ -269,21 +273,22 @@ static void mustBeXmlName(String string) throws JSONException { * @return true if {@code cp} may start an XML Name */ private static boolean isXmlNameStart(int cp) { - return cp == ':' || cp == '_' - || (cp >= 'A' && cp <= 'Z') - || (cp >= 'a' && cp <= 'z') - || (cp >= 0xC0 && cp <= 0xD6) - || (cp >= 0xD8 && cp <= 0xF6) - || (cp >= 0xF8 && cp <= 0x2FF) - || (cp >= 0x370 && cp <= 0x37D) - || (cp >= 0x37F && cp <= 0x1FFF) - || (cp >= 0x200C && cp <= 0x200D) - || (cp >= 0x2070 && cp <= 0x218F) - || (cp >= 0x2C00 && cp <= 0x2FEF) - || (cp >= 0x3001 && cp <= 0xD7FF) - || (cp >= 0xF900 && cp <= 0xFDCF) - || (cp >= 0xFDF0 && cp <= 0xFFFD) - || (cp >= 0x10000 && cp <= 0xEFFFF); + return cp == ':' + || cp == '_' + || inRange(cp, 'A', 'Z') + || inRange(cp, 'a', 'z') + || inRange(cp, 0xC0, 0xD6) + || inRange(cp, 0xD8, 0xF6) + || inRange(cp, 0xF8, 0x2FF) + || inRange(cp, 0x370, 0x37D) + || inRange(cp, 0x37F, 0x1FFF) + || inRange(cp, 0x200C, 0x200D) + || inRange(cp, 0x2070, 0x218F) + || inRange(cp, 0x2C00, 0x2FEF) + || inRange(cp, 0x3001, 0xD7FF) + || inRange(cp, 0xF900, 0xFDCF) + || inRange(cp, 0xFDF0, 0xFFFD) + || inRange(cp, 0x10000, 0xEFFFF); } /** @@ -294,11 +299,12 @@ private static boolean isXmlNameStart(int cp) { */ private static boolean isXmlNameChar(int cp) { return isXmlNameStart(cp) - || cp == '-' || cp == '.' - || (cp >= '0' && cp <= '9') + || cp == '-' + || cp == '.' || cp == 0xB7 - || (cp >= 0x0300 && cp <= 0x036F) - || (cp >= 0x203F && cp <= 0x2040); + || inRange(cp, '0', '9') + || inRange(cp, 0x0300, 0x036F) + || inRange(cp, 0x203F, 0x2040); } /** From 3dd0ec02f25c7c8f716fd62e1e3ffc6254004275 Mon Sep 17 00:00:00 2001 From: Mirko Swillus Date: Thu, 20 Aug 2026 17:14:07 +0200 Subject: [PATCH 3/3] #1071: narrow tag-name check to XML metachars only Per review on #1072: reject only < > & " ' / in element names to close the CWE-91 injection vector, and drop the full XML 1.0 Name validation to preserve backwards compatibility for callers that emit non-well-formed but non-injecting tag names. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01MTGnYg5v1QxaqqfDKHKTVr --- src/main/java/org/json/XML.java | 83 ++++--------------- .../org/json/junit/XMLConfigurationTest.java | 43 +++++----- src/test/java/org/json/junit/XMLTest.java | 78 ++++++++--------- 3 files changed, 72 insertions(+), 132 deletions(-) diff --git a/src/main/java/org/json/XML.java b/src/main/java/org/json/XML.java index 5191cdf97..0dceb81a4 100644 --- a/src/main/java/org/json/XML.java +++ b/src/main/java/org/json/XML.java @@ -233,80 +233,27 @@ public static void noSpace(String string) throws JSONException { } /** - * Throw an exception if the string is not a valid XML 1.0 {@code Name} - * (element name). Used by {@link #toString(Object)} to reject JSON keys that - * would otherwise be emitted verbatim between {@code <} and {@code >} and - * could break out of the tag context (element injection, CWE-91; - * see issue #1071 and #294). + * Throw an exception if the string contains an XML metacharacter + * ({@code < > & " ' /}). Used by {@link #toString(Object)} to reject JSON + * keys that would otherwise be emitted verbatim between {@code <} and + * {@code >} and could break out of the tag context (element injection, + * CWE-91; see issue #1071). * * @param string the candidate element name - * @throws JSONException if {@code string} is null, empty, or contains a - * character outside the XML 1.0 Name production + * @throws JSONException if {@code string} contains an XML metacharacter */ - static void mustBeXmlName(String string) throws JSONException { - if (string == null || string.isEmpty()) { - throw new JSONException("'" + string - + "' is not a valid XML element name."); - } - int cp = string.codePointAt(0); - if (!isXmlNameStart(cp)) { - throw new JSONException("'" + string - + "' is not a valid XML element name."); - } - for (int i = Character.charCount(cp); i < string.length(); i += Character.charCount(cp)) { - cp = string.codePointAt(i); - if (!isXmlNameChar(cp)) { + static void noXmlMetachars(String string) throws JSONException { + int length = string.length(); + for (int i = 0; i < length; i++) { + char c = string.charAt(i); + if (c == '<' || c == '>' || c == '&' + || c == '"' || c == '\'' || c == '/') { throw new JSONException("'" + string - + "' is not a valid XML element name."); + + "' contains an XML metacharacter and may not be used as an element name."); } } } - private static boolean inRange(int cp, int lo, int hi) { - return cp >= lo && cp <= hi; - } - - /** - * XML 1.0 (5th ed.) {@code NameStartChar} production. - * - * @param cp a Unicode code point - * @return true if {@code cp} may start an XML Name - */ - private static boolean isXmlNameStart(int cp) { - return cp == ':' - || cp == '_' - || inRange(cp, 'A', 'Z') - || inRange(cp, 'a', 'z') - || inRange(cp, 0xC0, 0xD6) - || inRange(cp, 0xD8, 0xF6) - || inRange(cp, 0xF8, 0x2FF) - || inRange(cp, 0x370, 0x37D) - || inRange(cp, 0x37F, 0x1FFF) - || inRange(cp, 0x200C, 0x200D) - || inRange(cp, 0x2070, 0x218F) - || inRange(cp, 0x2C00, 0x2FEF) - || inRange(cp, 0x3001, 0xD7FF) - || inRange(cp, 0xF900, 0xFDCF) - || inRange(cp, 0xFDF0, 0xFFFD) - || inRange(cp, 0x10000, 0xEFFFF); - } - - /** - * XML 1.0 (5th ed.) {@code NameChar} production. - * - * @param cp a Unicode code point - * @return true if {@code cp} may appear after the first character of an XML Name - */ - private static boolean isXmlNameChar(int cp) { - return isXmlNameStart(cp) - || cp == '-' - || cp == '.' - || cp == 0xB7 - || inRange(cp, '0', '9') - || inRange(cp, 0x0300, 0x036F) - || inRange(cp, 0x203F, 0x2040); - } - /** * Scan the content following the named tag, attaching it to the context. * @@ -1044,7 +991,7 @@ private static String toString(final Object object, final String tagName, final String string; if (tagName != null) { - mustBeXmlName(tagName); + noXmlMetachars(tagName); } if (object instanceof JSONObject) { @@ -1066,7 +1013,7 @@ private static String toString(final Object object, final String tagName, final jo = (JSONObject) object; for (final String key : jo.keySet()) { if (!key.equals(config.getcDataTagName())) { - mustBeXmlName(key); + noXmlMetachars(key); } Object value = jo.opt(key); if (value == null) { diff --git a/src/test/java/org/json/junit/XMLConfigurationTest.java b/src/test/java/org/json/junit/XMLConfigurationTest.java index 1a1a2fd58..e8ff3b60c 100755 --- a/src/test/java/org/json/junit/XMLConfigurationTest.java +++ b/src/test/java/org/json/junit/XMLConfigurationTest.java @@ -506,31 +506,32 @@ public void shouldHandleNestedArraytoString() { /** - * JSON keys that are not valid XML 1.0 Names are rejected by - * XML.toString rather than being emitted as (malformed) tag names. - * See issues #166, #294, #308 and #1071. + * Possible bug: + * Illegal node-names must be converted to legal XML-node-names. + * The given example shows 2 nodes which are valid for JSON, but not for XML. + * Therefore illegal arguments should be converted to e.g. an underscore (_). */ @Test public void shouldHandleIllegalJSONNodeNames() { - // Name may not start with a digit - try { - XML.toString(new JSONObject().append("123IllegalNode", "someValue1"), - null, XMLParserConfiguration.KEEP_STRINGS); - fail("expected JSONException for digit-leading element name"); - } catch (JSONException expected) { - assertTrue("message names the offending key", - expected.getMessage().contains("123IllegalNode")); - } - // Name may not contain '@' - try { - XML.toString(new JSONObject().append("Illegal@node", "someValue2"), - null, XMLParserConfiguration.KEEP_STRINGS); - fail("expected JSONException for '@' in element name"); - } catch (JSONException expected) { - assertTrue("message names the offending key", - expected.getMessage().contains("Illegal@node")); - } + JSONObject inputJSON = new JSONObject(); + inputJSON.append("123IllegalNode", "someValue1"); + inputJSON.append("Illegal@node", "someValue2"); + + String result = XML.toString(inputJSON, null, + XMLParserConfiguration.KEEP_STRINGS); + + /* + * This is invalid XML. Names should not begin with digits or contain + * certain values, including '@'. One possible solution is to replace + * illegal chars with '_', in which case the expected output would be: + * <___IllegalNode>someValue1someValue2 + */ + String expected = "<123IllegalNode>someValue1someValue2"; + + assertEquals("Length", expected.length(), result.length()); + assertTrue("123IllegalNode", result.contains("<123IllegalNode>someValue1")); + assertTrue("Illegal@node", result.contains("someValue2")); } /** diff --git a/src/test/java/org/json/junit/XMLTest.java b/src/test/java/org/json/junit/XMLTest.java index e67b9de59..cd67c268e 100644 --- a/src/test/java/org/json/junit/XMLTest.java +++ b/src/test/java/org/json/junit/XMLTest.java @@ -538,29 +538,31 @@ public void shouldHandleNestedArraytoString() { /** - * JSON keys that are not valid XML 1.0 Names are rejected by - * XML.toString rather than being emitted as (malformed) tag names. - * See issues #166, #294, #308 and #1071. + * Possible bug: + * Illegal node-names must be converted to legal XML-node-names. + * The given example shows 2 nodes which are valid for JSON, but not for XML. + * Therefore illegal arguments should be converted to e.g. an underscore (_). */ @Test public void shouldHandleIllegalJSONNodeNames() { - // Name may not start with a digit - try { - XML.toString(new JSONObject().append("123IllegalNode", "someValue1")); - fail("expected JSONException for digit-leading element name"); - } catch (JSONException expected) { - assertTrue("message names the offending key", - expected.getMessage().contains("123IllegalNode")); - } - // Name may not contain '@' - try { - XML.toString(new JSONObject().append("Illegal@node", "someValue2")); - fail("expected JSONException for '@' in element name"); - } catch (JSONException expected) { - assertTrue("message names the offending key", - expected.getMessage().contains("Illegal@node")); - } + JSONObject inputJSON = new JSONObject(); + inputJSON.append("123IllegalNode", "someValue1"); + inputJSON.append("Illegal@node", "someValue2"); + + String result = XML.toString(inputJSON); + + /* + * This is invalid XML. Names should not begin with digits or contain + * certain values, including '@'. One possible solution is to replace + * illegal chars with '_', in which case the expected output would be: + * <___IllegalNode>someValue1someValue2 + */ + String expected = "<123IllegalNode>someValue1someValue2"; + + assertEquals("length",expected.length(), result.length()); + assertTrue("123IllegalNode",result.contains("<123IllegalNode>someValue1")); + assertTrue("Illegal@node",result.contains("someValue2")); } /** @@ -577,36 +579,26 @@ public void toStringRejectsElementInjectionInKey() XML.toString(jo, "root"); fail("expected JSONException for key containing XML metacharacters"); } catch (JSONException expected) { - // expected: '/', '<', '>' are outside the XML Name production + // expected: '/', '<', '>' are rejected in element names } - // caller-supplied tagName is validated too + // caller-supplied tagName is checked too try { - XML.toString(new JSONObject(), "bad tag"); - fail("expected JSONException for tagName containing whitespace"); + XML.toString(new JSONObject(), "bada")); - assertTrue(xml.contains("b")); - assertTrue(xml.contains("c")); - assertTrue(xml.contains("<élément>d")); - assertTrue("cDataTagName still emitted as text content", xml.contains(">e<")); + // each metacharacter is rejected individually + for (char c : new char[] {'<', '>', '&', '"', '\'', '/'}) { + try { + XML.toString(new JSONObject().put("a" + c + "b", "v")); + fail("expected JSONException for key containing '" + c + "'"); + } catch (JSONException expected) { + // expected + } + } } /**