This is an automated email from the ASF dual-hosted git repository.
garydgregory pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/commons-secure-xml.git
The following commit(s) were added to refs/heads/main by this push:
new 6fcb571 Keep the resolver floor on a reader obtained before reset()
(#72)
6fcb571 is described below
commit 6fcb5718dede6858ad1c13a96121bc24b40d3353
Author: Piotr P. Karwasz <[email protected]>
AuthorDate: Tue Sep 1 13:31:35 2026 +0200
Keep the resolver floor on a reader obtained before reset() (#72)
A JAXP parser hands out one reader for its lifetime, and reset() reverts
that reader to its just-created state, which removes the floor the
wrapper installed after creation. SecureSAXParser only dropped its
cached views, so the floor came back on the next getXMLReader() call
while a reader a caller already held kept parsing without one.
Re-secure the reader in place instead, through the view already vended,
as SecureDocumentBuilder.reset() has always done. Keeping one wrapper
also keeps a caller resolver set on that view routed through the floor
that is actually installed.
ResetSecureTest covers the retained reader; the SecureSAXParser unit
test now asserts the views survive a reset with the floor in place,
rather than that they are recreated.
Assisted-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01CLnTBsvmYtxzNTWVGNyz33
---
.../org/apache/commons/xml/secure/SecureSAXParser.java | 11 +++++++----
.../org/apache/commons/xml/secure/SecureXMLReader.java | 8 ++++++++
.../org/apache/commons/xml/secure/ResetSecureTest.java | 17 +++++++++++++++++
.../apache/commons/xml/secure/SecureSAXParserTest.java | 11 +++++++----
4 files changed, 39 insertions(+), 8 deletions(-)
diff --git a/src/main/java/org/apache/commons/xml/secure/SecureSAXParser.java
b/src/main/java/org/apache/commons/xml/secure/SecureSAXParser.java
index 81272a0..941ba34 100644
--- a/src/main/java/org/apache/commons/xml/secure/SecureSAXParser.java
+++ b/src/main/java/org/apache/commons/xml/secure/SecureSAXParser.java
@@ -104,10 +104,13 @@ public boolean isXIncludeAware() {
@Override
public void reset() {
delegate.reset();
- // The JAXP reset contract reverts the delegate to its just-created
state, which strips the post-creation reader securing.
- // We reset the cached readers, so securing can be applied again.
- secureXMLReader = null;
- secureParser = null;
+ // The JAXP reset contract reverts the delegate to its just-created
state, which strips the securing from the one reader it hands out for its
lifetime.
+ if (secureXMLReader instanceof SecureXMLReader) {
+ ((SecureXMLReader) secureXMLReader).restoreFloor();
+ } else {
+ secureXMLReader = null;
+ secureParser = null;
+ }
}
@Override
diff --git a/src/main/java/org/apache/commons/xml/secure/SecureXMLReader.java
b/src/main/java/org/apache/commons/xml/secure/SecureXMLReader.java
index 98817f6..6531bd0 100644
--- a/src/main/java/org/apache/commons/xml/secure/SecureXMLReader.java
+++ b/src/main/java/org/apache/commons/xml/secure/SecureXMLReader.java
@@ -107,6 +107,14 @@ public void parse(final String systemId) throws
IOException, SAXException {
delegate.parse(systemId);
}
+ /**
+ * Re-installs the floor as the wrapped reader's entity resolver and drops
any caller-supplied resolver, restoring the just-created state.
+ */
+ void restoreFloor() {
+ floor.setDelegate(null);
+ delegate.setEntityResolver(floor);
+ }
+
@Override
public void setContentHandler(final ContentHandler handler) {
delegate.setContentHandler(handler);
diff --git a/src/test/java/org/apache/commons/xml/secure/ResetSecureTest.java
b/src/test/java/org/apache/commons/xml/secure/ResetSecureTest.java
index 5bf3ebe..b6f2f80 100644
--- a/src/test/java/org/apache/commons/xml/secure/ResetSecureTest.java
+++ b/src/test/java/org/apache/commons/xml/secure/ResetSecureTest.java
@@ -86,6 +86,23 @@ void saxParserResetKeepsEntityResolverFloor() throws
Exception {
assertFalse(text.contains(AttackTestSupport.LEAKED_MARKER), "external
entity leaked after reset:\n" + text);
}
+ @Test
+ @Tag("sax")
+ void saxParserResetKeepsFloorOnReaderVendedBeforeReset() throws Exception {
+ final SAXParser parser =
SecureSAXParserFactory.newInstance().newSAXParser();
+ // The handle a caller keeps across the reset. A JAXP parser hands out
one reader for its lifetime, so re-fetching it after the reset (as the test
+ // above does) hides the case pooling code actually hits: reset the
parser, keep parsing through the reader you already hold.
+ final XMLReader reader = parser.getXMLReader();
+ AttackTestSupport.assumeDoesNotThrow(parser::reset);
+ final String text;
+ try {
+ text = AttackTestSupport.captureCharacters(reader,
entityPayload(UNLISTED));
+ } catch (final SAXException blocked) {
+ return; // Acceptable: rejected at parse rather than resolved to
empty.
+ }
+ assertFalse(text.contains(AttackTestSupport.LEAKED_MARKER), "external
entity leaked through a reader obtained before reset:\n" + text);
+ }
+
@Test
@Tag("trax")
void transformerResetKeepsUriResolverFloor() throws Exception {
diff --git
a/src/test/java/org/apache/commons/xml/secure/SecureSAXParserTest.java
b/src/test/java/org/apache/commons/xml/secure/SecureSAXParserTest.java
index da593ae..c731a0d 100644
--- a/src/test/java/org/apache/commons/xml/secure/SecureSAXParserTest.java
+++ b/src/test/java/org/apache/commons/xml/secure/SecureSAXParserTest.java
@@ -17,8 +17,8 @@
package org.apache.commons.xml.secure;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
import static org.junit.jupiter.api.Assertions.assertNotNull;
-import static org.junit.jupiter.api.Assertions.assertNotSame;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertSame;
@@ -108,7 +108,7 @@ public void setProperty(final String name, final Object
value) throws SAXNotReco
}
@Test
- void cachesSecureViewsThenRecreatesThemAfterReset() throws Exception {
+ void cachesSecureViewsAndKeepsThemSecuredAfterReset() throws Exception {
final SecureSAXParser parser = new
SecureSAXParser(SAXParserFactory.newInstance().newSAXParser());
final XMLReader firstReader = parser.getXMLReader();
final Parser firstParser = parser.getParser();
@@ -117,8 +117,11 @@ void cachesSecureViewsThenRecreatesThemAfterReset() throws
Exception {
parser.setProperty("http://xml.org/sax/properties/lexical-handler";,
null);
assertNull(parser.getProperty("http://xml.org/sax/properties/lexical-handler";));
parser.reset();
- assertNotSame(firstReader, parser.getXMLReader());
- assertNotSame(firstParser, parser.getParser());
+ // The views survive the reset rather than being recreated: a caller
holding one from before keeps parsing on the floor the reset stripped.
+ assertSame(firstReader, parser.getXMLReader());
+ assertSame(firstParser, parser.getParser());
+ assertInstanceOf(FallbackIgnoreEntityResolver2.class,
((SecureXMLReader) firstReader).getDelegate().getEntityResolver(),
+ "the reset must put the floor back on the underlying reader");
}
@Test
