From f2c8361093541ca6758a17ebd55ed9de35d6414c Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Tue, 1 Sep 2026 12:38:27 +0200 Subject: [PATCH] Install a fresh StAX resolver floor per hook instead of mutating one 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 Claude-Session: https://claude.ai/code/session_01CLnTBsvmYtxzNTWVGNyz33 --- .../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 60a696a8..9ea052e1 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 { * *

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.

* + *

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.

+ * * @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 fe60637f..05b92e7d 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();