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 73b6bbf Install a fresh StAX resolver floor per hook instead of
mutating one (#75)
73b6bbf is described below
commit 73b6bbf7ce7f94730fc5023553f709ff5bb64afb
Author: Piotr P. Karwasz <[email protected]>
AuthorDate: Tue Sep 1 13:41:37 2026 +0200
Install a fresh StAX resolver floor per hook instead of mutating one (#75)
Routing a caller resolver by calling setDelegate on the floor already
installed treated that floor as private to the hook being set, which it
is not. Woodstox routes setXMLResolver to both its DTD-subset and
entity hooks, so one floor object sits on several of them, and its
ReaderConfig.createNonShared copies the reference into every reader it
creates. Mutating the object therefore answered hooks the call never
named, and changed the resolution policy of readers created earlier,
including ones already parsing on another thread.
Install a new floor on the named hook instead. Each hook then keeps the
floor it was given and each reader the one it captured, so a resolver
is scoped to the hook it was set on and bound when the reader was made.
Nothing is mutated after publication any more, which also removes the
unsynchronized cross-thread write.
The com.ctc.wstx.* hooks had no tests; they have two now, both verified
to fail without the main-code change. The existing setXMLResolver test
asserted the mutation itself, so it now asserts the contract it was
standing in for: the hook keeps a floor and the caller sits behind it.
Assisted-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01CLnTBsvmYtxzNTWVGNyz33
---
.../commons/xml/secure/SecureXMLInputFactory.java | 20 ++++++------
.../xml/secure/SecureXMLInputFactoryTest.java | 37 +++++++++++++++++++---
2 files changed, 44 insertions(+), 13 deletions(-)
diff --git
a/src/main/java/org/apache/commons/xml/secure/SecureXMLInputFactory.java
b/src/main/java/org/apache/commons/xml/secure/SecureXMLInputFactory.java
index 60a696a..9ea052e 100644
--- a/src/main/java/org/apache/commons/xml/secure/SecureXMLInputFactory.java
+++ b/src/main/java/org/apache/commons/xml/secure/SecureXMLInputFactory.java
@@ -60,12 +60,17 @@ public final class SecureXMLInputFactory {
*
* <p>Every resolver-valued entry point ({@link
#setXMLResolver(XMLResolver)}, {@code setProperty(XMLInputFactory.RESOLVER,
...)} and the Woodstox
* {@code com.ctc.wstx.*Resolver} keys) is routed uniformly: a caller who
supplies their own {@link FallbackIgnoreXMLResolver} takes control and it is
- * passed straight to the delegate; otherwise the current resolver on that
hook is read, and if it is one of our floors the caller's resolver is set as its
- * {@link FallbackIgnoreXMLResolver#setDelegate delegate} (an opt-in the
floor cannot be removed by), or, if the hook is empty, the caller's resolver is
- * wrapped in a new floor. This matters because Woodstox does not chain
resolvers: when a resolver returns {@code null}, {@code DefaultInputResolver}
falls
+ * passed straight to the delegate; otherwise the caller's resolver is
wrapped in a new floor installed on that hook, an opt-in the floor cannot be
removed
+ * by. This matters because Woodstox does not chain resolvers: when a
resolver returns {@code null}, {@code DefaultInputResolver} falls
* through to fetching the systemId URL itself, so a caller-set resolver
that returns {@code null} must still land behind the floor. {@link
#getXMLResolver()} and
* {@code getProperty} report the caller's resolver unwrapped.</p>
*
+ * <p>The floor on a hook is replaced rather than mutated, which is what
keeps each hook and each reader independent. Woodstox routes
+ * {@code setXMLResolver} to both its DTD-subset and entity hooks, so one
floor object sits on several of them, and it copies that reference into every
+ * reader it creates; setting a delegate on the object in place would
therefore also answer hooks the caller never named, and would change the
resolution
+ * policy of readers already created, including ones parsing on another
thread. Installing a new floor leaves both untouched: a hook keeps whatever
floor it
+ * was given, and a reader keeps the one it captured when it was
created.</p>
+ *
* @see org.apache.commons.xml.secure
*/
private static final class Wrapper extends XMLInputFactory {
@@ -225,12 +230,9 @@ private void setResolverProperty(final String name, final
XMLResolver resolver)
// The caller supplies their own floor: hand it to the
delegate as-is.
delegate.setProperty(name, resolver);
} else {
- final Object current = delegate.getProperty(name);
- if (current instanceof FallbackIgnoreXMLResolver) {
- ((FallbackIgnoreXMLResolver)
current).setDelegate(resolver);
- } else {
- delegate.setProperty(name, new
FallbackIgnoreXMLResolver(resolver));
- }
+ // A fresh floor for this hook rather than a new delegate on
the floor already there: Woodstox puts one floor object on several hooks and
copies
+ // the reference into every reader it creates, so mutating it
would reach hooks, and readers already parsing, that this call never named.
+ delegate.setProperty(name, new
FallbackIgnoreXMLResolver(resolver));
}
}
diff --git
a/src/test/java/org/apache/commons/xml/secure/SecureXMLInputFactoryTest.java
b/src/test/java/org/apache/commons/xml/secure/SecureXMLInputFactoryTest.java
index fe60637..05b92e7 100644
--- a/src/test/java/org/apache/commons/xml/secure/SecureXMLInputFactoryTest.java
+++ b/src/test/java/org/apache/commons/xml/secure/SecureXMLInputFactoryTest.java
@@ -20,6 +20,7 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
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;
import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -492,14 +493,14 @@ void setXMLResolverNullClearsCallerDelegate() {
void setXMLResolverRoutesCallerBehindInstalledFloor() {
final RecordingXMLInputFactory fake = new RecordingXMLInputFactory();
final XMLInputFactory secure = SecureXMLInputFactory.secure(fake);
- final FallbackIgnoreXMLResolver floor = (FallbackIgnoreXMLResolver)
fake.resolverHook;
final XMLResolver caller = (publicID, systemID, baseURI, namespace) ->
null;
secure.setXMLResolver(caller);
- assertSame(caller, floor.getDelegate(), "the caller's resolver must
become the delegate of the installed floor");
+ // The hook keeps a floor with the caller behind it; whether that is
the floor already there or a fresh one is the subject of
+ // settingAResolverInstallsAFreshFloorInsteadOfMutatingTheInstalledOne.
+ assertTrue(fake.resolverHook instanceof FallbackIgnoreXMLResolver, "a
caller resolver must land behind a floor, not replace it on the delegate's
hook");
+ assertSame(caller, ((FallbackIgnoreXMLResolver)
fake.resolverHook).getDelegate(), "the caller's resolver must be the floor's
delegate");
assertSame(caller, secure.getXMLResolver(), "getXMLResolver must
report the caller's resolver unwrapped");
assertSame(caller, secure.getProperty(XMLInputFactory.RESOLVER),
"getProperty must report the caller's resolver unwrapped");
- assertFalse(fake.calls.stream().anyMatch(c ->
c.startsWith("setProperty(" + XMLInputFactory.RESOLVER)),
- "a caller resolver must not replace the floor on the
delegate's hook");
}
@Test
@@ -609,6 +610,34 @@ void wrapperDelegatesReaderCreationToDelegate() throws
Exception {
"the exact stream filter must be forwarded");
}
+ @Test
+ void settingAResolverInstallsAFreshFloorInsteadOfMutatingTheInstalledOne()
{
+ // The implementations copy the floor reference into every reader they
create, so mutating the installed floor would change the resolution policy of
+ // readers created before the call, including ones already parsing.
Replacing it leaves what those readers captured alone.
+ final RecordingXMLInputFactory fake = new RecordingXMLInputFactory();
+ final XMLInputFactory secure = SecureXMLInputFactory.secure(fake);
+ final Object captured = fake.resolverHook;
+ secure.setXMLResolver((publicID, systemID, baseURI, namespace) ->
null);
+ assertNotSame(captured, fake.resolverHook, "setting a resolver must
install a fresh floor, not re-delegate the one already on the hook");
+ assertNull(((FallbackIgnoreXMLResolver) captured).getDelegate(), "the
floor an existing reader captured must keep resolving to empty");
+ }
+
+ @Test
+ void woodstoxResolverHooksStayIndependent() {
+ // Woodstox routes setXMLResolver to both its DTD-subset and entity
hooks, so one floor object sits on several of them. Setting one hook must not
+ // answer the others, which it would if the shared floor were mutated
in place.
+ final XMLInputFactory secure = SecureXMLInputFactory.newInstance();
+ final XMLResolver dtd = (publicID, systemID, baseURI, namespace) ->
null;
+ try {
+ secure.setProperty(SecureXMLInputFactory.WSTX_DTD_RESOLVER, dtd);
+ } catch (final IllegalArgumentException notWoodstox) {
+ Assumptions.abort("the implementation does not support " +
SecureXMLInputFactory.WSTX_DTD_RESOLVER);
+ return;
+ }
+ assertSame(dtd,
secure.getProperty(SecureXMLInputFactory.WSTX_DTD_RESOLVER), "the hook the
caller named must report their resolver");
+
assertNull(secure.getProperty(SecureXMLInputFactory.WSTX_ENTITY_RESOLVER), "a
resolver set on the DTD hook must not answer the entity hook");
+ }
+
@Test
void wrapperInstallsFloorOnDelegateHook() {
final RecordingXMLInputFactory fake = new RecordingXMLInputFactory();
