Skip to content

GROOVY-12120: FactorySupport: log a warning when an XML parser securi… #2650

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
paulk-asert merged 1 commit into from
Jun 30, 2026
+138 −15
Merged

GROOVY-12120: FactorySupport: log a warning when an XML parser securi… #2650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 52 additions & 15 deletions subprojects/groovy-xml/src/main/java/groovy/xml/FactorySupport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import javax.xml.xpath.XPathFactory;
import javax.xml.xpath.XPathFactoryConfigurationException;
import java.security.PrivilegedExceptionAction;
import java.util.logging.Logger;

/**
* Support class for creating hardened JAXP factories.
Expand All @@ -43,8 +44,28 @@
*/
public class FactorySupport {

private static final Logger LOG = Logger.getLogger(FactorySupport.class.getName());

private static final String DISALLOW_DOCTYPE_DECL_FEATURE = "http://apache.org/xml/features/disallow-doctype-decl";

/**
* Logs that a security-hardening setting could not be applied to a JAXP factory. These settings
* are applied on a best-effort basis (a non-standard JAXP provider may not recognise them), but a
* silent failure would leave the factory more permissive than the caller expects, so it is logged
* rather than swallowed.
*
* @param factory the factory the setting was applied to
* @param name the feature/attribute/property name
* @param value the value that could not be applied
* @param cause the failure, or {@code null} if the value was simply not retained after being set
*/
private static void warnHardeningNotApplied(Object factory, String name, Object value, Exception cause) {
LOG.warning("Unable to apply XML security setting '" + name + "'=" + value + " on "
+ factory.getClass().getName()
+ (cause != null ? ": " + cause : " (value was not retained after being set)")
+ "; the factory may be more permissive than intended (e.g. weaker XXE/DTD protection).");
}

/**
* Runs the supplied factory creation action and normalizes checked failures.
*
Expand Down Expand Up @@ -241,59 +262,75 @@ public static XPathFactory createXPathFactory() {
return factory;
}

private static void setFeatureQuietly(DocumentBuilderFactory factory, String feature, boolean value) {
// package-private for testing
static void setFeatureQuietly(DocumentBuilderFactory factory, String feature, boolean value) {
try {
factory.setFeature(feature, value);
} catch (ParserConfigurationException ignored) {
// feature is not supported, ignore
if (factory.getFeature(feature) != value) {
warnHardeningNotApplied(factory, feature, value, null);
}
} catch (ParserConfigurationException e) {
warnHardeningNotApplied(factory, feature, value, e);
}
}

private static void setFeatureQuietly(SAXParserFactory factory, String feature, boolean value) {
try {
factory.setFeature(feature, value);
} catch (ParserConfigurationException | SAXNotSupportedException | SAXNotRecognizedException ignored) {
// feature is not supported, ignore
if (factory.getFeature(feature) != value) {
warnHardeningNotApplied(factory, feature, value, null);
}
} catch (ParserConfigurationException | SAXNotSupportedException | SAXNotRecognizedException e) {
warnHardeningNotApplied(factory, feature, value, e);
}
}

private static void setFeatureQuietly(TransformerFactory factory, String feature, boolean value) {
try {
factory.setFeature(feature, value);
} catch (TransformerConfigurationException ignored) {
// feature is not supported, ignore
if (factory.getFeature(feature) != value) {
warnHardeningNotApplied(factory, feature, value, null);
}
} catch (TransformerConfigurationException e) {
warnHardeningNotApplied(factory, feature, value, e);
}
}

private static void setFeatureQuietly(SchemaFactory factory, String feature, boolean value) {
try {
factory.setFeature(feature, value);
} catch (SAXNotSupportedException | SAXNotRecognizedException ignored) {
// feature is not supported, ignore
if (factory.getFeature(feature) != value) {
warnHardeningNotApplied(factory, feature, value, null);
}
} catch (SAXNotSupportedException | SAXNotRecognizedException e) {
warnHardeningNotApplied(factory, feature, value, e);
}
}

private static void setFeatureQuietly(XPathFactory factory, String feature, boolean value) {
try {
factory.setFeature(feature, value);
} catch (XPathFactoryConfigurationException ignored) {
// feature is not supported, ignore
if (factory.getFeature(feature) != value) {
warnHardeningNotApplied(factory, feature, value, null);
}
} catch (XPathFactoryConfigurationException e) {
warnHardeningNotApplied(factory, feature, value, e);
}
}

private static void setAttributeQuietly(TransformerFactory factory, String attribute, Object value) {
try {
factory.setAttribute(attribute, value);
} catch (IllegalArgumentException ignored) {
// attribute is not supported, ignore
} catch (IllegalArgumentException e) {
warnHardeningNotApplied(factory, attribute, value, e);
}
}

private static void setPropertyQuietly(XMLInputFactory factory, String property, Object value) {
try {
factory.setProperty(property, value);
} catch (IllegalArgumentException ignored) {
// property is not supported, ignore
} catch (IllegalArgumentException e) {
warnHardeningNotApplied(factory, property, value, e);
}
}
}
86 changes: 86 additions & 0 deletions subprojects/groovy-xml/src/test/groovy/groovy/xml/FactorySupportTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import org.junit.jupiter.api.Test;

import javax.xml.XMLConstants;
import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.parsers.ParserConfigurationException;
import javax.xml.parsers.SAXParserFactory;
Expand All @@ -30,6 +31,14 @@
import javax.xml.xpath.XPathFactory;
import java.security.PrivilegedActionException;
import java.security.PrivilegedExceptionAction;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.logging.Handler;
import java.util.logging.Level;
import java.util.logging.LogRecord;
import java.util.logging.Logger;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
Expand Down Expand Up @@ -161,4 +170,81 @@ public void hardenedXPathFactoryEnablesSecureProcessing() throws Exception {
XPathFactory factory = FactorySupport.createXPathFactory();
assertTrue(factory.getFeature(XMLConstants.FEATURE_SECURE_PROCESSING));
}

// GROOVY-12120: a failure to apply a security feature must be logged, not silently swallowed

@Test
public void warnsWhenSecurityFeatureSetThrows() {
MockDocumentBuilderFactory factory = new MockDocumentBuilderFactory();
factory.throwOnSetFeature = true;
List<LogRecord> logs = captureFactorySupportLogs(() ->
FactorySupport.setFeatureQuietly(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true));
assertTrue(logs.stream().anyMatch(r -> r.getLevel() == Level.WARNING
&& r.getMessage().contains(XMLConstants.FEATURE_SECURE_PROCESSING)),
"expected a warning when the security feature could not be set");
}

@Test
public void warnsWhenSecurityFeatureNotRetained() {
MockDocumentBuilderFactory factory = new MockDocumentBuilderFactory();
factory.forcedGetFeature = Boolean.FALSE; // pretend the value never took effect
List<LogRecord> logs = captureFactorySupportLogs(() ->
FactorySupport.setFeatureQuietly(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true));
assertTrue(logs.stream().anyMatch(r -> r.getLevel() == Level.WARNING
&& r.getMessage().contains(XMLConstants.FEATURE_SECURE_PROCESSING)),
"expected a warning when the security feature was not retained after being set");
}

@Test
public void doesNotWarnWhenSecurityFeatureApplied() {
MockDocumentBuilderFactory factory = new MockDocumentBuilderFactory();
List<LogRecord> logs = captureFactorySupportLogs(() ->
FactorySupport.setFeatureQuietly(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true));
assertTrue(logs.isEmpty(), "no warning expected when the feature applies cleanly");
}

private static List<LogRecord> captureFactorySupportLogs(Runnable action) {
Logger logger = Logger.getLogger(FactorySupport.class.getName());
List<LogRecord> records = new ArrayList<>();
Handler handler = new Handler() {
public void publish(LogRecord record) { records.add(record); }
public void flush() { }
public void close() { }
};
boolean useParent = logger.getUseParentHandlers();
Level oldLevel = logger.getLevel();
logger.addHandler(handler);
logger.setUseParentHandlers(false);
logger.setLevel(Level.ALL);
try {
action.run();
} finally {
logger.removeHandler(handler);
logger.setUseParentHandlers(useParent);
logger.setLevel(oldLevel);
}
return records;
}

/** A non-standard JAXP factory whose feature handling can be made to fail or no-op. */
private static final class MockDocumentBuilderFactory extends DocumentBuilderFactory {
boolean throwOnSetFeature;
Boolean forcedGetFeature; // when non-null, getFeature returns this regardless of what was set
final Map<String, Boolean> features = new HashMap<>();

public DocumentBuilder newDocumentBuilder() { return null; }
public void setAttribute(String name, Object value) { }
public Object getAttribute(String name) { return null; }

public void setFeature(String name, boolean value) throws ParserConfigurationException {
if (throwOnSetFeature) {
throw new ParserConfigurationException("unsupported feature: " + name);
}
features.put(name, value);
}

public boolean getFeature(String name) {
return forcedGetFeature != null ? forcedGetFeature : Boolean.TRUE.equals(features.get(name));
}
}
}
Loading