ppkarwasz commented on code in PR #766:
URL: https://github.com/apache/commons-text/pull/766#discussion_r3944902284
##########
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:
This looks like an attempt by the agent to keep the `*ExternalEntityOn`
tests working. Those tests rely on the specifics of the hardening to allow
fetching external entities. On the built-in JDK parser:
- Setting FSP to `false`,
- or setting `javax.xml.accessExternalDTD` to "all",
is enough to reenable external entity fetching.
Commons Secure XML use a resolver to block the fetch instead, so the
technique above will fail, since the resolver will block before
`javax.xml.accessExternalDTD` is even evaluated.
I will of course clean up these (they don't make sense), but we might need a
way to allow users to specify external fetches. My proposal is to use a
resolver that uses the nested `PathFence` to determine if the fetch is allowed
or not. WDYT?
--
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]
