garydgregory commented on code in PR #766:
URL: https://github.com/apache/commons-text/pull/766#discussion_r3945306145
##########
src/main/java/org/apache/commons/text/lookup/XmlStringLookup.java:
##########
@@ -128,14 +130,21 @@ public String lookup(final String key) {
}
final String documentPath = keys[0];
final String xpath = StringUtils.substringAfterLast(key, SPLIT_CH);
- final DocumentBuilderFactory dbFactory =
DocumentBuilderFactory.newInstance();
+ // The secure factory installs a non-removable resolver floor that
ignores the JAXP access properties,
+ // so the documented opt-outs keep a plain factory: a feature map
without secure processing, or the
+ // standard javax.xml.accessExternalDTD system property re-allowing
external access.
+ final boolean secure =
Boolean.TRUE.equals(xmlFactoryFeatures.get(XMLConstants.FEATURE_SECURE_PROCESSING))
Review Comment:
Well, right now, you do get secure processing enabled by default in
`master`, and unlike what the Javadoc says, it doesn't appear you can turn it
off with a sys prop, BUT, you can set custom features
(`StringLookupFactory.xmlStringLookup(Map<String, Boolean>, Path...)` which
ends up calling `setFeature()`) so you can shoot yourself in the foot to your
heart's desire.
The simple path would be:
1. Update the Javadoc to reflect the code: no sys prop, unless you can find
it used ;)
2. Redo the main side of this PR to use Commons Secure XML cleanly and
without leaving cruft behind
For example, I think the statics `DEFAULT_*` can go away
Path fences are mixing concerns for this PR. We want to integrate Secure XML
and then see if we need improvements in Secure XML and/or call sites, later if
possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
