garydgregory commented on code in PR #112:
URL:
https://github.com/apache/commons-secure-xml/pull/112#discussion_r4506088713
##########
src/main/java/org/apache/commons/xml/secure/SecureXMLInputFactory.java:
##########
@@ -239,6 +240,7 @@ private void setResolverProperty(final String name, final
XMLResolver resolver)
@Override
public void setXMLReporter(final XMLReporter reporter) {
+ floor.setXMLReporter(reporter);
Review Comment:
This updates only the original floor. However, setXMLResolver() and
resolver-valued properties install fresh floors through setResolverProperty().
Those replacements never receive the reporter.
This means a blocked DTD produced one warning initially, zero after
setXMLResolver(...), and still zero after reinstalling the reporter. Reporter
wiring must account for replacement floors while preserving the existing
isolation between resolver hooks and readers.
##########
src/main/java/org/apache/commons/xml/secure/SecureDocumentBuilder.java:
##########
@@ -106,6 +106,7 @@ public void setEntityResolver(final EntityResolver
resolver) {
@Override
public void setErrorHandler(final ErrorHandler eh) {
+ floor.setErrorHandler(eh);
Review Comment:
This now stores the handler in the floor, but `reset()` resets only the
delegate and resolver. The same lifecycle omission exists in `SecureValidator`
and `SecureTransformer`.
After `setErrorHandler(handler); reset();`, parsing a blocked DTD still
invoked that handler. Reused objects can notify a previous caller, and a stale
throwing handler can abort subsequent work. Reset must restore the floor’s
callback state along with the delegate’s state.
##########
src/main/java/org/apache/commons/xml/secure/FallbackIgnoreLSResourceResolver.java:
##########
@@ -75,9 +79,17 @@ public LSInput resolveResource(final String type, final
String namespaceURI, fin
if (resolved != null) {
return resolved;
}
+ final String message = SecureException.forbidden(type, namespaceURI,
publicId, systemId, baseURI);
if (SecureException.throwOnUnresolved()) {
// The interface declares no checked exception; LSException is the
DOM Load/Save runtime failure type.
- throw new LSException(LSException.PARSE_ERR,
SecureException.forbidden(type, namespaceURI, publicId, systemId, baseURI));
+ throw new LSException(LSException.PARSE_ERR, message);
+ }
+ if (errorHandler != null) {
+ try {
+ errorHandler.warning(new SAXParseException(message, null));
+ } catch (SAXException e) {
Review Comment:
Always swallow? A handler throwing a "stop requested" is ignored and the
resolver returns empty content. Shouldn't this preserve the failure through an
appropriate exception mechanism instead of swallowing it? This also conflicts
with the PR’s SAX, URI, and StAX paths, which propagate callback exceptions.
--
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]
