Skip to content

Update XMLHandler#438

Open
kevin-mcgoldrick wants to merge 9 commits into
masterfrom
XMLHandler
Open

Update XMLHandler#438
kevin-mcgoldrick wants to merge 9 commits into
masterfrom
XMLHandler

Conversation

@kevin-mcgoldrick

@kevin-mcgoldrick kevin-mcgoldrick commented Oct 30, 2025

Copy link
Copy Markdown
Collaborator

Update XMLHandler

Removes the jdom2 dependency and rewrites GenericXMLHandler on top of the JDK's built-in java.xml APIs (org.w3c.dom DOM + javax.xml XPath/Transformer). jdom2 is being dropped because the XML exception types it throws have been removed from the Java runtime.

What changed

  • jdom2 removed from all POMs that declared it (pom.xml, agent_common, script_processor, web/web_ui).
  • GenericXMLHandler rewritten using DOM / XPath / Transformer:
    • Read (GetElementText, GetElementAttr, GetElementList, xPathExists), write (SetElementText, SetElementAttribute, Save), and toString() reimplemented with no jdom2 types.
    • Create-on-write preservedSetElementText/SetElementAttribute still build any missing path segments (and the root element) via a new resolveOrCreateElement helper, so XMLRequest can construct a body from an empty document as before.
  • XXE hardening on the shared parser/transformer factories: disallow-doctype-decl, external general/parameter entities disabled, XInclude off, entity-reference expansion off, and external DTD/stylesheet access cleared on the transformer.
  • Dead code removed — the empty JsonHandler / JsonHandlerTest classes are deleted.

Tests

  • Replaced the disabled, auto-generated (@generatedBy CodePro) stubs in GenericXMLHandlerTest / XMLRequestTest with real assertions covering both the string- and file-based constructor paths, get/set of text & attributes, GetElementList, xPathExists (positive and negative), isXMLValid (valid + invalid), clone, and the path helpers.
  • mvn clean test for agent_common is green.

Please make sure these check boxes are checked before submitting

  • ** Squashed Commits **
  • ** All Tests Passed ** - mvn clean test -P default

** PR review process **

  • Requires one +1 from a reviewer
  • Repository owners will merge your PR once it is approved.

@kevin-mcgoldrick kevin-mcgoldrick added dependencies Update one or more dependencies version java Pull requests that update java code labels Mar 19, 2026
@kevin-mcgoldrick kevin-mcgoldrick added the patch Increment the patch version when merged label Jul 14, 2026
} catch (Exception ex) {
LOG.error("Error in handler: " + ex.getMessage(), ex);
return "";
Node node = (Node) XPATH_FACTORY.newXPath()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetElementAttr breaks its existing attribute-XPath contract. Expressions such as root/child/@first now return ""; element expressions instead concatenate every attribute value.

return (org.jdom2.Element) XPath.selectSingleNode(this.xmlDocument, xPathExpression);
String currentPath = String.join("/", Arrays.copyOfRange(segments, 0, currentNode + 1));

Node existing = (Node) XPATH_FACTORY.newXPath()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Namespace handling is nonfunctional. XPath instances never receive a NamespaceContext, parsing is not namespace-aware, and createElementNS receives a name with the prefix removed. The user-facing ns: request configuration therefore cannot create namespaced XML.

* @return TRUE if the xPath expression exists; false otherwise
*/
public boolean xPathExists(String xpathExpr)
public boolean xPathExists(String xPathExpression)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

evaluate(...) returns the XPath string value here, not the matched node. This makes existing empty elements report false, unlike the previous node-based implementation. Could this evaluate with XPathConstants.NODE and check for null? Please add coverage for <root><empty/></root>.

private static final XPathFactory XPATH_FACTORY = XPathFactory.newInstance();
private static final TransformerFactory TRANSFORMER_FACTORY;

static {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XXE hardening fails open. If one provider-specific feature is unsupported, the remaining features are skipped and parsing continues. Standard ACCESS_EXTERNAL_DTD/ACCESS_EXTERNAL_SCHEMA parser restrictions are also absent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Update one or more dependencies version enhancement java Pull requests that update java code patch Increment the patch version when merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants