Copilot commented on code in PR #18:
URL: https://github.com/apache/commons-xml/pull/18#discussion_r3535791917
##########
src/test/java/org/apache/commons/xml/AttackTestSupport.java:
##########
@@ -859,6 +860,18 @@ static XMLReader strictXMLReader(final XMLReader reader) {
return reader;
}
+ /**
+ * Runs the action and, if it throws, aborts (skips rather than fails) the
calling test. Used to guard platform-optional configuration such as
+ * {@code setXIncludeAware}, which the Android JAXP implementations do not
support.
+ */
+ static void assumeDoesNotThrow(final Executable action) {
+ try {
+ action.execute();
+ } catch (final Throwable t) {
+ Assumptions.assumeTrue(false, "platform does not support this
configuration: " + t);
+ }
+ }
Review Comment:
assumeDoesNotThrow catches Throwable and skips the test on any failure,
including AssertionError/Errors that would typically indicate a real regression
rather than an unsupported platform feature. Catching Exception keeps the
intended ‘unsupported configuration’ skip behavior while still letting serious
Errors surface.
##########
src/main/java/org/apache/commons/xml/HardeningXMLInputFactory.java:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.commons.xml;
+
+import javax.xml.stream.XMLInputFactory;
+import javax.xml.stream.XMLResolver;
+
+/**
+ * {@link XMLInputFactory} wrapper that keeps the {@link
Resolvers.FallbackDenyXMLResolver} floors {@link StaxHardener} installs on the
entity-resolution hooks
+ * non-removable by the caller.
+ *
+ * <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 Resolvers.FallbackDenyXMLResolver} 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 Resolvers.FallbackDenyXMLResolver#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 fresh floor. 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>
+ */
+final class HardeningXMLInputFactory extends DelegatingXMLInputFactory {
+
+ HardeningXMLInputFactory(final XMLInputFactory delegate) {
+ super(delegate);
+ }
+
+ @Override
+ public void setXMLResolver(final XMLResolver resolver) {
+ if (resolver instanceof Resolvers.FallbackDenyXMLResolver) {
+ // The caller supplies their own floor: hand it to the delegate
as-is.
+ super.setXMLResolver(resolver);
+ } else {
+ final XMLResolver current = super.getXMLResolver();
+ if (current instanceof Resolvers.FallbackDenyXMLResolver) {
+ ((Resolvers.FallbackDenyXMLResolver)
current).setDelegate(resolver);
+ } else {
+ super.setXMLResolver(new
Resolvers.FallbackDenyXMLResolver(resolver));
+ }
+ }
+ }
+
+ @Override
+ public XMLResolver getXMLResolver() {
+ return unwrap(super.getXMLResolver());
+ }
+
+ @Override
+ public void setProperty(final String name, final Object value) {
+ if (XMLInputFactory.RESOLVER.equals(name)) {
+ setXMLResolver((XMLResolver) value);
+ } else if (isWstxResolverProperty(name)) {
+ setResolverProperty(name, (XMLResolver) value);
+ } else {
+ super.setProperty(name, value);
+ }
+ }
+
+ @Override
+ public Object getProperty(final String name) {
+ if (XMLInputFactory.RESOLVER.equals(name)) {
+ return getXMLResolver();
+ }
+ if (isWstxResolverProperty(name)) {
+ return unwrap((XMLResolver) super.getProperty(name));
+ }
+ return super.getProperty(name);
+ }
Review Comment:
getProperty assumes the underlying value for Woodstox resolver keys is
always an XMLResolver and casts unconditionally. If an implementation returns
null or a different sentinel type, this will throw ClassCastException.
Returning the raw delegate value when it isn’t an XMLResolver keeps behavior
compatible while still unwrapping floors when present.
##########
src/main/java/org/apache/commons/xml/HardeningXMLInputFactory.java:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.commons.xml;
+
+import javax.xml.stream.XMLInputFactory;
+import javax.xml.stream.XMLResolver;
+
+/**
+ * {@link XMLInputFactory} wrapper that keeps the {@link
Resolvers.FallbackDenyXMLResolver} floors {@link StaxHardener} installs on the
entity-resolution hooks
+ * non-removable by the caller.
+ *
+ * <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 Resolvers.FallbackDenyXMLResolver} 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 Resolvers.FallbackDenyXMLResolver#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 fresh floor. 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>
+ */
+final class HardeningXMLInputFactory extends DelegatingXMLInputFactory {
+
+ HardeningXMLInputFactory(final XMLInputFactory delegate) {
+ super(delegate);
+ }
+
+ @Override
+ public void setXMLResolver(final XMLResolver resolver) {
+ if (resolver instanceof Resolvers.FallbackDenyXMLResolver) {
+ // The caller supplies their own floor: hand it to the delegate
as-is.
+ super.setXMLResolver(resolver);
+ } else {
+ final XMLResolver current = super.getXMLResolver();
+ if (current instanceof Resolvers.FallbackDenyXMLResolver) {
+ ((Resolvers.FallbackDenyXMLResolver)
current).setDelegate(resolver);
+ } else {
+ super.setXMLResolver(new
Resolvers.FallbackDenyXMLResolver(resolver));
+ }
+ }
+ }
+
+ @Override
+ public XMLResolver getXMLResolver() {
+ return unwrap(super.getXMLResolver());
+ }
+
+ @Override
+ public void setProperty(final String name, final Object value) {
+ if (XMLInputFactory.RESOLVER.equals(name)) {
+ setXMLResolver((XMLResolver) value);
+ } else if (isWstxResolverProperty(name)) {
+ setResolverProperty(name, (XMLResolver) value);
+ } else {
+ super.setProperty(name, value);
+ }
+ }
Review Comment:
setProperty casts resolver values to XMLResolver unconditionally for the
RESOLVER and Woodstox resolver keys. If a caller passes a non-XMLResolver value
(or an implementation returns a non-XMLResolver from getProperty for a Woodstox
key), this wrapper will throw a ClassCastException instead of deferring to the
delegate’s usual IllegalArgumentException/behavior. Consider type-checking and
delegating for non-XMLResolver values to preserve the underlying factory’s
contract.
##########
src/test/java/org/apache/commons/xml/SchemaLocationPropertyTest.java:
##########
@@ -0,0 +1,187 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.commons.xml;
+
+import static org.apache.commons.xml.AttackTestSupport.assertParseFails;
+import static org.apache.commons.xml.AttackTestSupport.assertParseSucceeds;
+import static org.apache.commons.xml.AttackTestSupport.inputSource;
+import static org.apache.commons.xml.AttackTestSupport.resourceUrl;
+import static org.apache.commons.xml.AttackTestSupport.strictDocumentBuilder;
+import static org.apache.commons.xml.AttackTestSupport.strictXMLReader;
+
+import javax.xml.XMLConstants;
+import javax.xml.parsers.DocumentBuilder;
+import javax.xml.parsers.DocumentBuilderFactory;
+import javax.xml.parsers.SAXParser;
+import javax.xml.parsers.SAXParserFactory;
+
+import org.junit.jupiter.api.Assumptions;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.function.ThrowingSupplier;
+import org.xml.sax.SAXException;
+import org.xml.sax.XMLReader;
+
+/**
+ * Checks that a hardened, schema-validating parser does not fetch a schema
named only through a Xerces schema-location
+ * property: {@code external-noNamespaceSchemaLocation} (no-namespace schema)
and {@code external-schemaLocation}
+ * (namespaced schema).
+ *
+ * <p>The fixtures declare the instance's root element, so a parser that
fetches the schema validates the instance
+ * cleanly and one that does not cannot. The permissive controls prove the
external schema is reachable in principle, so
+ * the hardened side throwing means the fetch was refused, not merely
misconfigured. The stock JDK refuses it through
+ * {@code accessExternalSchema=""}; external Apache Xerces, which ignores that
property, refuses it through the deny-all
+ * entity-resolver floor.</p>
+ *
+ * <p>Not every parser supports these schema-validation knobs (Android's
KXmlParser and Expat do not), so the whole
+ * configuration runs through {@link #configureOrSkip}: a parser that rejects
validation, the schema language, or the
+ * schema-location property skips the test rather than failing it.</p>
+ */
+@Tag("schema")
+class SchemaLocationPropertyTest {
+
+ private static final String SCHEMA_LANGUAGE =
"http://java.sun.com/xml/jaxp/properties/schemaLanguage";;
+ private static final String SCHEMA_FEATURE =
"http://apache.org/xml/features/validation/schema";;
+ private static final String EXTERNAL_NO_NS =
"http://apache.org/xml/properties/schema/external-noNamespaceSchemaLocation";;
+ private static final String EXTERNAL_SCHEMA_LOCATION =
"http://apache.org/xml/properties/schema/external-schemaLocation";;
+
+ /** Instance whose root, {@code <root>}, is declared by {@code
no-namespace.xsd}. */
+ private static final String NO_NS_INSTANCE = "<root>x</root>";
+
+ private static final String LEAKED_NS = "http://example.org/leaked";;
+
+ /** Instance whose root, {@code l:leaked}, is declared by {@code
included.xsd} in the {@value #LEAKED_NS} namespace. */
+ private static final String NAMESPACED_INSTANCE = "<l:leaked xmlns:l=\"" +
LEAKED_NS + "\">x</l:leaked>";
+
+ private static String noNamespaceLocation() {
+ return resourceUrl("no-namespace.xsd").toString();
+ }
+
+ private static String namespacedLocation() {
+ return LEAKED_NS + " " + resourceUrl("included.xsd");
+ }
+
+ /**
+ * Runs the parser setup, skipping the test (rather than failing it) on
parsers that do not accept these
+ * schema-validation features/properties, such as Android's KXmlParser and
Expat.
+ */
+ private static <T> T configureOrSkip(final ThrowingSupplier<T> setup) {
+ try {
+ return setup.get();
+ } catch (final Throwable t) {
+ return Assumptions.abort("Parser does not support schema
validation through these features/properties: " + t);
+ }
+ }
Review Comment:
configureOrSkip currently catches Throwable and aborts the test. Catching
Throwable will also skip on AssertionError or other serious Errors, which can
hide real regressions in test setup. Narrowing to Exception still skips
unsupported parser configurations but lets Errors surface as failures.
--
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]
