You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "ppkarwasz (via GitHub)" <gi...@apache.org> on 2026/08/19 11:28:26 UTC
[PR] Recognize Saxon factories by package prefix [commons-xml]
ppkarwasz opened a new pull request, #45:
URL: https://github.com/apache/commons-xml/pull/45
Recognizes Saxon factories by package prefix instead of an exact class-name set, addressing finding f008.
## Problem
`TransformerHardener` and `XPathHardener` route a factory to `SaxonProvider` (the only recipe that installs Saxon's locked-down `Configuration`) by matching its class name against a fixed set of three names. Saxon-HE ships a second public `TransformerFactory`, `net.sf.saxon.BasicTransformerFactory` (a subclass of the registered `net.sf.saxon.TransformerFactoryImpl`), selectable through the standard `javax.xml.transform.TransformerFactory` system property. It is not in the set, so it falls to the generic branch. That branch wraps the factory (FSP, the URIResolver floor, the Source rewrite) and looks successful, but never installs the empty `CollectionFinder`, so `fn:collection` fetches directly and stays open.
## Changes
- `SaxonProvider` gains a package-private `isSaxon(Class<?>)` that matches the `net.sf.saxon.` and `com.saxonica.` package prefixes. The outer class references no Saxon types (they live in the inner configurer), so it still loads without Saxon on the classpath.
- `TransformerHardener` and `XPathHardener` drop their exact-name sets and call `SaxonProvider.isSaxon(...)`; javadoc updated from "recognized by class name" to "recognized by package prefix", noting that subclasses such as `BasicTransformerFactory` now route to the same recipe.
- `SaxonProvider.configure(...)` now catches the cast `ClassCastException` and rethrows it as a `HardeningException` naming the factory class: with prefix matching, a hypothetical Saxon-package factory that is not castable to `SaxonTransformerFactory`/`XPathFactoryImpl` fails closed with a message rather than a bare `ClassCastException` (the exact-name set previously made the cast infallible).
## Tests
New `SaxonAlternateFactoryTest` (`@Tag("trax")`, reflective load of `BasicTransformerFactory`, skipped when Saxon is absent, so effective on test-saxon and test-saxon-xerces):
- `hardenedBasicFactoryDoesNotLeakCollection`: routes a `BasicTransformerFactory` through `TransformerHardener.harden(...)` and runs a stylesheet whose `collection()` would pull `LEAKED_MARKER` into the output; blocks-or-does-not-leak.
- `unconfiguredBasicFactoryLeaksCollection`: the bare factory resolves it (leak control).
`collection()` is the deliberately chosen channel: it bypasses Saxon's resource-resolution chain, so only the empty `CollectionFinder` in `SaxonProvider` closes it. Verified by probe on the pinned Saxon-HE 13.0 that `collection()` leaks through both the bare factory and the generic `HardeningTransformerFactory` branch but returns empty through `SaxonProvider.configure`. Discrimination confirmed: with the exact-name recognition temporarily restored, the hardened test fails on both Saxon executions while the leak control passes. (The report's suggested `unparsed-text()` probe does not discriminate on Saxon 13, since the generic branch's URIResolver floor already covers that channel; `collection()` is the one it does not.)
Full `mvn clean` and plain `mvn` (checkstyle, spotbugs, pmd, javadoc, whole surefire matrix) green, plus a post-build non-vacuous `spotbugs:check`.
## Note
`TransformerHardener` is also rewritten by open PR #41 (Saxon wrapped like every other implementation) and `XPathHardener` by open PR #44; whichever lands later reconciles small conflicts, and the `SaxonProvider.isSaxon` prefix recognition should be carried into #41's routing.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Recognize Saxon factories by package prefix [commons-xml]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #45:
URL: https://github.com/apache/commons-xml/pull/45#discussion_r3855960994
##########
src/test/java/org/apache/commons/xml/SaxonAlternateFactoryTest.java:
##########
@@ -0,0 +1,105 @@
+/*
+ * 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.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.io.StringWriter;
+
+import javax.xml.transform.TransformerException;
+import javax.xml.transform.TransformerFactory;
+import javax.xml.transform.stream.StreamResult;
+
+import org.junit.jupiter.api.Assumptions;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Checks that Saxon's alternate public {@code TransformerFactory} entry point routes to the same locked-down {@code Configuration} as the registered one.
+ *
+ * <p>Saxon ships {@code net.sf.saxon.BasicTransformerFactory}, a public subclass of the registered {@code net.sf.saxon.TransformerFactoryImpl}, selectable
+ * through the standard TrAX system property. Recognition by package prefix sends it through {@code SaxonProvider}; a name-based recognition would let it fall
+ * to the generic recipe. The probe uses {@code fn:collection}, which bypasses Saxon's resource-resolution chain and fetches directly: only the empty
+ * {@code CollectionFinder} that {@code SaxonProvider} installs closes it, so the generic recipe (which leaves it open even after wrapping) does not. The
+ * factory is instantiated reflectively and the tests skip when Saxon is not on the classpath, so under the surefire group filters the checks are effective on
+ * the test-saxon and test-saxon-xerces executions.</p>
+ */
+@Tag("trax")
+class SaxonAlternateFactoryTest {
+
+ private static final String BASIC_FACTORY_CLASS = "net.sf.saxon.BasicTransformerFactory";
+
+ /** Instantiates {@code BasicTransformerFactory} reflectively, so this test compiles and loads without Saxon on the classpath. */
+ private static TransformerFactory basicSaxonFactory() {
+ try {
+ return (TransformerFactory) Class.forName(BASIC_FACTORY_CLASS).getDeclaredConstructor().newInstance();
+ } catch (final ReflectiveOperationException e) {
+ throw new AssertionError("Cannot instantiate " + BASIC_FACTORY_CLASS, e);
+ }
+ }
+
+ private static void assumeSaxonPresent() {
+ boolean present;
+ try {
+ Class.forName(BASIC_FACTORY_CLASS);
+ present = true;
+ } catch (final ClassNotFoundException e) {
+ present = false;
+ }
+ Assumptions.assumeTrue(present, "Saxon is not on the classpath");
+ }
+
+ /** A {@code collection()} over the test fixtures whose {@code referenced.xml} carries {@link AttackTestSupport#LEAKED_MARKER}. */
+ private static String collectionStylesheet() {
+ final String collection = AttackTestSupport.resourceUrl("referenced.xml").toString().replaceFirst("referenced\\.xml$", "?select=referenced.xml");
+ return "<?xml version=\"1.0\"?>\n"
+ + "<xsl:stylesheet version=\"3.0\" xmlns:xsl=\"http://www.w3.org/1999/XSL/Transform\">\n"
+ + " <xsl:template match=\"/\">\n"
+ + " <leaked><xsl:value-of select=\"string(collection('" + collection + "')/leaked)\"/></leaked>\n"
+ + " </xsl:template>\n"
+ + "</xsl:stylesheet>\n";
+ }
+
+ private static String transform(final TransformerFactory factory) throws TransformerException {
+ final StringWriter sink = new StringWriter();
+ factory.newTemplates(AttackTestSupport.streamSource(collectionStylesheet())).newTransformer()
+ .transform(AttackTestSupport.streamSource("<root/>"), new StreamResult(sink));
+ return sink.toString();
+ }
+
+ @Test
+ void hardenedBasicFactoryDoesNotLeakCollection() {
+ assumeSaxonPresent();
+ final String result;
+ try {
+ result = transform(TransformerHardener.harden(basicSaxonFactory()));
+ } catch (final TransformerException blocked) {
+ return; // Acceptable: the reference was rejected rather than resolved to empty.
Review Comment:
Same comments as in the other PRs WRT `assertThrows()`.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Recognize Saxon factories by package prefix [commons-xml]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #45:
URL: https://github.com/apache/commons-xml/pull/45
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Recognize Saxon factories by package prefix [commons-xml]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #45:
URL: https://github.com/apache/commons-xml/pull/45#discussion_r3857168646
##########
src/test/java/org/apache/commons/xml/SaxonAlternateFactoryTest.java:
##########
@@ -0,0 +1,105 @@
+/*
+ * 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.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.io.StringWriter;
+
+import javax.xml.transform.TransformerException;
+import javax.xml.transform.TransformerFactory;
+import javax.xml.transform.stream.StreamResult;
+
+import org.junit.jupiter.api.Assumptions;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Checks that Saxon's alternate public {@code TransformerFactory} entry point routes to the same locked-down {@code Configuration} as the registered one.
+ *
+ * <p>Saxon ships {@code net.sf.saxon.BasicTransformerFactory}, a public subclass of the registered {@code net.sf.saxon.TransformerFactoryImpl}, selectable
+ * through the standard TrAX system property. Recognition by package prefix sends it through {@code SaxonProvider}; a name-based recognition would let it fall
+ * to the generic recipe. The probe uses {@code fn:collection}, which bypasses Saxon's resource-resolution chain and fetches directly: only the empty
+ * {@code CollectionFinder} that {@code SaxonProvider} installs closes it, so the generic recipe (which leaves it open even after wrapping) does not. The
+ * factory is instantiated reflectively and the tests skip when Saxon is not on the classpath, so under the surefire group filters the checks are effective on
+ * the test-saxon and test-saxon-xerces executions.</p>
+ */
+@Tag("trax")
+class SaxonAlternateFactoryTest {
+
+ private static final String BASIC_FACTORY_CLASS = "net.sf.saxon.BasicTransformerFactory";
+
+ /** Instantiates {@code BasicTransformerFactory} reflectively, so this test compiles and loads without Saxon on the classpath. */
+ private static TransformerFactory basicSaxonFactory() {
+ try {
+ return (TransformerFactory) Class.forName(BASIC_FACTORY_CLASS).getDeclaredConstructor().newInstance();
+ } catch (final ReflectiveOperationException e) {
+ throw new AssertionError("Cannot instantiate " + BASIC_FACTORY_CLASS, e);
+ }
+ }
+
+ private static void assumeSaxonPresent() {
+ boolean present;
+ try {
+ Class.forName(BASIC_FACTORY_CLASS);
+ present = true;
+ } catch (final ClassNotFoundException e) {
+ present = false;
+ }
+ Assumptions.assumeTrue(present, "Saxon is not on the classpath");
+ }
+
+ /** A {@code collection()} over the test fixtures whose {@code referenced.xml} carries {@link AttackTestSupport#LEAKED_MARKER}. */
+ private static String collectionStylesheet() {
+ final String collection = AttackTestSupport.resourceUrl("referenced.xml").toString().replaceFirst("referenced\\.xml$", "?select=referenced.xml");
+ return "<?xml version=\"1.0\"?>\n"
+ + "<xsl:stylesheet version=\"3.0\" xmlns:xsl=\"http://www.w3.org/1999/XSL/Transform\">\n"
+ + " <xsl:template match=\"/\">\n"
+ + " <leaked><xsl:value-of select=\"string(collection('" + collection + "')/leaked)\"/></leaked>\n"
+ + " </xsl:template>\n"
+ + "</xsl:stylesheet>\n";
+ }
+
+ private static String transform(final TransformerFactory factory) throws TransformerException {
+ final StringWriter sink = new StringWriter();
+ factory.newTemplates(AttackTestSupport.streamSource(collectionStylesheet())).newTransformer()
+ .transform(AttackTestSupport.streamSource("<root/>"), new StreamResult(sink));
+ return sink.toString();
+ }
+
+ @Test
+ void hardenedBasicFactoryDoesNotLeakCollection() {
+ assumeSaxonPresent();
+ final String result;
+ try {
+ result = transform(TransformerHardener.harden(basicSaxonFactory()));
+ } catch (final TransformerException blocked) {
+ return; // Acceptable: the reference was rejected rather than resolved to empty.
Review Comment:
Refactored in https://github.com/apache/commons-xml/pull/45/commits/793ab255d64bb376e3a4c4a41c88a10b44ec31f3
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org