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();