| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
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>
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 <noreply@anthropic.com>
|
@mechko Sorry for the long wait, will get to this before next week. While reviewing the PR, I found that strict mode is not filtering incoming text for valid JSON chars. I will fix this for strict mode, but keep the current behavior for non-strict mode. |
Sorry, something went wrong.
|
@mechko Thanks for the PR. Can you please restrict this fix to the security issues mentioned in #1071. Enforcing well-formed tags could break backwards compatibility. Fixing the security issues also does this, but is justified because the fix prevents an injection attack.
|
Sorry, something went wrong.
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
|
Sorry, something went wrong.
|
@stleary I adjusted the fix according to your comment, please let me know if you still want anything to be changed. |
Sorry, something went wrong.
|
What problem does this code solve? Risks Changes to the Existing Behavior Changes to the API Will this require a new release? Should the documentation be updated? Unit Tests Refactoring Review status Starting 3-day comment window |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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.toString emits JSONObject keys verbatim as XML tag names. A key containing <, > or / breaks out of its element and injects arbitrary sibling structure:
{"a/><injected>evil</injected><a": ""} → <root><a/><injected>evil</injected><a/></root>Values already go through escape(); keys/tagName do not.
Fix
Validate every key and tagName against the XML 1.0 (5th ed.) Name production before emitting; throw JSONException on mismatch.
<, >, /, whitespace, ", &, @ are all outside NameChar, so the injection vector is closed. Keys that are already valid XML Names are unaffected — mustBeXmlName is a no-op for them.
Behaviour change
XML.toString now throws for keys that are not valid XML Names, where previously it emitted malformed XML:
The two existing shouldHandleIllegalJSONNodeNames tests already documented the old output as "invalid XML" / "possible bug"; they've been rewritten to assert the throw.
Verification
$ java -cp target/classes:. XmlRepro Exception in thread "main" org.json.JSONException: 'a/><injected>evil</injected><a' is not a valid XML element name. at org.json.XML.mustBeXmlName(XML.java:259) at org.json.XML.toString(XML.java:1063)mvn test: 791 run, 0 fail, 0 error, 6 skipped (pre-existing).
Tests
🤖 Generated with Claude Code