Skip to content

Commit 3dd0ec0

Browse files
mechkoclaude
mechko
and
claude
committed
stleary#1071: narrow tag-name check to XML metachars only
Per review on stleary#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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MTGnYg5v1QxaqqfDKHKTVr
1 parent 6b993e2 commit 3dd0ec0

3 files changed

Lines changed: 72 additions & 132 deletions

File tree

‎src/main/java/org/json/XML.java‎

Lines changed: 15 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -233,80 +233,27 @@ public static void noSpace(String string) throws JSONException {
233233
}
234234

235235
/**
236-
* Throw an exception if the string is not a valid XML 1.0 {@code Name}
237-
* (element name). Used by {@link #toString(Object)} to reject JSON keys that
238-
* would otherwise be emitted verbatim between {@code <} and {@code >} and
239-
* could break out of the tag context (element injection, CWE-91;
240-
* see issue #1071 and #294).
236+
* Throw an exception if the string contains an XML metacharacter
237+
* ({@code < > & " ' /}). Used by {@link #toString(Object)} to reject JSON
238+
* keys that would otherwise be emitted verbatim between {@code <} and
239+
* {@code >} and could break out of the tag context (element injection,
240+
* CWE-91; see issue #1071).
241241
*
242242
* @param string the candidate element name
243-
* @throws JSONException if {@code string} is null, empty, or contains a
244-
* character outside the XML 1.0 Name production
243+
* @throws JSONException if {@code string} contains an XML metacharacter
245244
*/
246-
static void mustBeXmlName(String string) throws JSONException {
247-
if (string == null || string.isEmpty()) {
248-
throw new JSONException("'" + string
249-
+ "' is not a valid XML element name.");
250-
}
251-
int cp = string.codePointAt(0);
252-
if (!isXmlNameStart(cp)) {
253-
throw new JSONException("'" + string
254-
+ "' is not a valid XML element name.");
255-
}
256-
for (int i = Character.charCount(cp); i < string.length(); i += Character.charCount(cp)) {
257-
cp = string.codePointAt(i);
258-
if (!isXmlNameChar(cp)) {
245+
static void noXmlMetachars(String string) throws JSONException {
246+
int length = string.length();
247+
for (int i = 0; i < length; i++) {
248+
char c = string.charAt(i);
249+
if (c == '<' || c == '>' || c == '&'
250+
|| c == '"' || c == '\'' || c == '/') {
259251
throw new JSONException("'" + string
260-
+ "' is not a valid XML element name.");
252+
+ "' contains an XML metacharacter and may not be used as an element name.");
261253
}
262254
}
263255
}
264256

265-
private static boolean inRange(int cp, int lo, int hi) {
266-
return cp >= lo && cp <= hi;
267-
}
268-
269-
/**
270-
* XML 1.0 (5th ed.) {@code NameStartChar} production.
271-
*
272-
* @param cp a Unicode code point
273-
* @return true if {@code cp} may start an XML Name
274-
*/
275-
private static boolean isXmlNameStart(int cp) {
276-
return cp == ':'
277-
|| cp == '_'
278-
|| inRange(cp, 'A', 'Z')
279-
|| inRange(cp, 'a', 'z')
280-
|| inRange(cp, 0xC0, 0xD6)
281-
|| inRange(cp, 0xD8, 0xF6)
282-
|| inRange(cp, 0xF8, 0x2FF)
283-
|| inRange(cp, 0x370, 0x37D)
284-
|| inRange(cp, 0x37F, 0x1FFF)
285-
|| inRange(cp, 0x200C, 0x200D)
286-
|| inRange(cp, 0x2070, 0x218F)
287-
|| inRange(cp, 0x2C00, 0x2FEF)
288-
|| inRange(cp, 0x3001, 0xD7FF)
289-
|| inRange(cp, 0xF900, 0xFDCF)
290-
|| inRange(cp, 0xFDF0, 0xFFFD)
291-
|| inRange(cp, 0x10000, 0xEFFFF);
292-
}
293-
294-
/**
295-
* XML 1.0 (5th ed.) {@code NameChar} production.
296-
*
297-
* @param cp a Unicode code point
298-
* @return true if {@code cp} may appear after the first character of an XML Name
299-
*/
300-
private static boolean isXmlNameChar(int cp) {
301-
return isXmlNameStart(cp)
302-
|| cp == '-'
303-
|| cp == '.'
304-
|| cp == 0xB7
305-
|| inRange(cp, '0', '9')
306-
|| inRange(cp, 0x0300, 0x036F)
307-
|| inRange(cp, 0x203F, 0x2040);
308-
}
309-
310257
/**
311258
* Scan the content following the named tag, attaching it to the context.
312259
*
@@ -1044,7 +991,7 @@ private static String toString(final Object object, final String tagName, final
1044991
String string;
1045992

1046993
if (tagName != null) {
1047-
mustBeXmlName(tagName);
994+
noXmlMetachars(tagName);
1048995
}
1049996

1050997
if (object instanceof JSONObject) {
@@ -1066,7 +1013,7 @@ private static String toString(final Object object, final String tagName, final
10661013
jo = (JSONObject) object;
10671014
for (final String key : jo.keySet()) {
10681015
if (!key.equals(config.getcDataTagName())) {
1069-
mustBeXmlName(key);
1016+
noXmlMetachars(key);
10701017
}
10711018
Object value = jo.opt(key);
10721019
if (value == null) {

‎src/test/java/org/json/junit/XMLConfigurationTest.java‎

Lines changed: 22 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -506,31 +506,32 @@ public void shouldHandleNestedArraytoString() {
506506

507507

508508
/**
509-
* JSON keys that are not valid XML 1.0 Names are rejected by
510-
* XML.toString rather than being emitted as (malformed) tag names.
511-
* See issues #166, #294, #308 and #1071.
509+
* Possible bug:
510+
* Illegal node-names must be converted to legal XML-node-names.
511+
* The given example shows 2 nodes which are valid for JSON, but not for XML.
512+
* Therefore illegal arguments should be converted to e.g. an underscore (_).
512513
*/
513514
@Test
514515
public void shouldHandleIllegalJSONNodeNames()
515516
{
516-
// Name may not start with a digit
517-
try {
518-
XML.toString(new JSONObject().append("123IllegalNode", "someValue1"),
519-
null, XMLParserConfiguration.KEEP_STRINGS);
520-
fail("expected JSONException for digit-leading element name");
521-
} catch (JSONException expected) {
522-
assertTrue("message names the offending key",
523-
expected.getMessage().contains("123IllegalNode"));
524-
}
525-
// Name may not contain '@'
526-
try {
527-
XML.toString(new JSONObject().append("Illegal@node", "someValue2"),
528-
null, XMLParserConfiguration.KEEP_STRINGS);
529-
fail("expected JSONException for '@' in element name");
530-
} catch (JSONException expected) {
531-
assertTrue("message names the offending key",
532-
expected.getMessage().contains("Illegal@node"));
533-
}
517+
JSONObject inputJSON = new JSONObject();
518+
inputJSON.append("123IllegalNode", "someValue1");
519+
inputJSON.append("Illegal@node", "someValue2");
520+
521+
String result = XML.toString(inputJSON, null,
522+
XMLParserConfiguration.KEEP_STRINGS);
523+
524+
/*
525+
* This is invalid XML. Names should not begin with digits or contain
526+
* certain values, including '@'. One possible solution is to replace
527+
* illegal chars with '_', in which case the expected output would be:
528+
* <___IllegalNode>someValue1</___IllegalNode><Illegal_node>someValue2</Illegal_node>
529+
*/
530+
String expected = "<123IllegalNode>someValue1</123IllegalNode><Illegal@node>someValue2</Illegal@node>";
531+
532+
assertEquals("Length", expected.length(), result.length());
533+
assertTrue("123IllegalNode", result.contains("<123IllegalNode>someValue1</123IllegalNode>"));
534+
assertTrue("Illegal@node", result.contains("<Illegal@node>someValue2</Illegal@node>"));
534535
}
535536

536537
/**

‎src/test/java/org/json/junit/XMLTest.java‎

Lines changed: 35 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -538,29 +538,31 @@ public void shouldHandleNestedArraytoString() {
538538

539539

540540
/**
541-
* JSON keys that are not valid XML 1.0 Names are rejected by
542-
* XML.toString rather than being emitted as (malformed) tag names.
543-
* See issues #166, #294, #308 and #1071.
541+
* Possible bug:
542+
* Illegal node-names must be converted to legal XML-node-names.
543+
* The given example shows 2 nodes which are valid for JSON, but not for XML.
544+
* Therefore illegal arguments should be converted to e.g. an underscore (_).
544545
*/
545546
@Test
546547
public void shouldHandleIllegalJSONNodeNames()
547548
{
548-
// Name may not start with a digit
549-
try {
550-
XML.toString(new JSONObject().append("123IllegalNode", "someValue1"));
551-
fail("expected JSONException for digit-leading element name");
552-
} catch (JSONException expected) {
553-
assertTrue("message names the offending key",
554-
expected.getMessage().contains("123IllegalNode"));
555-
}
556-
// Name may not contain '@'
557-
try {
558-
XML.toString(new JSONObject().append("Illegal@node", "someValue2"));
559-
fail("expected JSONException for '@' in element name");
560-
} catch (JSONException expected) {
561-
assertTrue("message names the offending key",
562-
expected.getMessage().contains("Illegal@node"));
563-
}
549+
JSONObject inputJSON = new JSONObject();
550+
inputJSON.append("123IllegalNode", "someValue1");
551+
inputJSON.append("Illegal@node", "someValue2");
552+
553+
String result = XML.toString(inputJSON);
554+
555+
/*
556+
* This is invalid XML. Names should not begin with digits or contain
557+
* certain values, including '@'. One possible solution is to replace
558+
* illegal chars with '_', in which case the expected output would be:
559+
* <___IllegalNode>someValue1</___IllegalNode><Illegal_node>someValue2</Illegal_node>
560+
*/
561+
String expected = "<123IllegalNode>someValue1</123IllegalNode><Illegal@node>someValue2</Illegal@node>";
562+
563+
assertEquals("length",expected.length(), result.length());
564+
assertTrue("123IllegalNode",result.contains("<123IllegalNode>someValue1</123IllegalNode>"));
565+
assertTrue("Illegal@node",result.contains("<Illegal@node>someValue2</Illegal@node>"));
564566
}
565567

566568
/**
@@ -577,36 +579,26 @@ public void toStringRejectsElementInjectionInKey()
577579
XML.toString(jo, "root");
578580
fail("expected JSONException for key containing XML metacharacters");
579581
} catch (JSONException expected) {
580-
// expected: '/', '<', '>' are outside the XML Name production
582+
// expected: '/', '<', '>' are rejected in element names
581583
}
582584

583-
// caller-supplied tagName is validated too
585+
// caller-supplied tagName is checked too
584586
try {
585-
XML.toString(new JSONObject(), "bad tag");
586-
fail("expected JSONException for tagName containing whitespace");
587+
XML.toString(new JSONObject(), "bad<tag");
588+
fail("expected JSONException for tagName containing '<'");
587589
} catch (JSONException expected) {
588-
// expected: space is outside the XML Name production
590+
// expected: '<' is rejected in element names
589591
}
590-
}
591592

592-
/**
593-
* Keys that are valid XML 1.0 Names continue to serialise unchanged.
594-
*/
595-
@Test
596-
public void toStringAcceptsValidXmlNames()
597-
{
598-
JSONObject jo = new JSONObject();
599-
jo.put("simple", "a");
600-
jo.put("with-hyphen.dot_underscore", "b");
601-
jo.put("ns:qualified", "c");
602-
jo.put("élément", "d"); // Latin-1 letters
603-
jo.put("content", "e"); // cDataTagName sentinel, emitted as text
604-
String xml = XML.toString(jo, "root");
605-
assertTrue(xml.contains("<simple>a</simple>"));
606-
assertTrue(xml.contains("<with-hyphen.dot_underscore>b</with-hyphen.dot_underscore>"));
607-
assertTrue(xml.contains("<ns:qualified>c</ns:qualified>"));
608-
assertTrue(xml.contains("<élément>d</élément>"));
609-
assertTrue("cDataTagName still emitted as text content", xml.contains(">e<"));
593+
// each metacharacter is rejected individually
594+
for (char c : new char[] {'<', '>', '&', '"', '\'', '/'}) {
595+
try {
596+
XML.toString(new JSONObject().put("a" + c + "b", "v"));
597+
fail("expected JSONException for key containing '" + c + "'");
598+
} catch (JSONException expected) {
599+
// expected
600+
}
601+
}
610602
}
611603

612604
/**

0 commit comments

Comments
 (0)