This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 056d79bf12c93d090eed5ad06d111e23ee022795 Author: remm <[email protected]> AuthorDate: Wed Oct 28 16:24:48 2026 +0100 Fail the JDBC resource creation on an unusable boolean attribute The JNDI object factory of the JDBC connection pool converted every boolean attribute of a Resource with Boolean.valueOf or Boolean.parseBoolean, which yield false for any input that is not case-insensitively "true": a misspelled testOnBorrow="ture" silently disabled borrow-time validation, a value padded with whitespace lost its meaning, and an empty attribute read as an explicit false. The tri-state pair defaultAutoCommit and defaultReadOnly was worse, as their null state means "leave the driver's own default alone" and the factory could never produce it: any configured value, however meaningless, forced an explicit true or false onto every connection of the pool, while an unusable numeric attribute of the very same resource already failed loudly with a NumberFormatException. Parse boolean attributes strictly through one helper that all twenty one sites now share. A value equal to "true" or "false", ignoring case and surrounding whitespace, sets the property; an empty attribute leaves it unset, at its PoolProperties default for a plain boolean and at the "use the driver default" state for the tri-state pair; anything else fails the creation of the resource with an IllegalArgumentException that names the property and quotes its value, reaching the container the way the numeric failures already do. Valid configurations are unaffected. --- .../apache/tomcat/jdbc/pool/DataSourceFactory.java | 138 +++++++++++++++++---- .../tomcat/jdbc/naming/TestNamingFactory.java | 132 ++++++++++++++++++++ 2 files changed, 248 insertions(+), 22 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceFactory.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceFactory.java index 4f10020c2d..d32b545eec 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceFactory.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceFactory.java @@ -453,12 +453,18 @@ public class DataSourceFactory implements ObjectFactory { value = properties.getProperty(PROP_DEFAULTAUTOCOMMIT); if (value != null) { - poolProperties.setDefaultAutoCommit(Boolean.valueOf(value)); + Boolean b = parseBoolean(PROP_DEFAULTAUTOCOMMIT, value); + if (b != null) { + poolProperties.setDefaultAutoCommit(b); + } } value = properties.getProperty(PROP_DEFAULTREADONLY); if (value != null) { - poolProperties.setDefaultReadOnly(Boolean.valueOf(value)); + Boolean b = parseBoolean(PROP_DEFAULTREADONLY, value); + if (b != null) { + poolProperties.setDefaultReadOnly(b); + } } value = properties.getProperty(PROP_DEFAULTTRANSACTIONISOLATION); @@ -524,17 +530,26 @@ public class DataSourceFactory implements ObjectFactory { value = properties.getProperty(PROP_TESTONBORROW); if (value != null) { - poolProperties.setTestOnBorrow(Boolean.parseBoolean(value)); + Boolean b = parseBoolean(PROP_TESTONBORROW, value); + if (b != null) { + poolProperties.setTestOnBorrow(b.booleanValue()); + } } value = properties.getProperty(PROP_TESTONRETURN); if (value != null) { - poolProperties.setTestOnReturn(Boolean.parseBoolean(value)); + Boolean b = parseBoolean(PROP_TESTONRETURN, value); + if (b != null) { + poolProperties.setTestOnReturn(b.booleanValue()); + } } value = properties.getProperty(PROP_TESTONCONNECT); if (value != null) { - poolProperties.setTestOnConnect(Boolean.parseBoolean(value)); + Boolean b = parseBoolean(PROP_TESTONCONNECT, value); + if (b != null) { + poolProperties.setTestOnConnect(b.booleanValue()); + } } value = properties.getProperty(PROP_TIMEBETWEENEVICTIONRUNSMILLIS); @@ -554,7 +569,10 @@ public class DataSourceFactory implements ObjectFactory { value = properties.getProperty(PROP_TESTWHILEIDLE); if (value != null) { - poolProperties.setTestWhileIdle(Boolean.parseBoolean(value)); + Boolean b = parseBoolean(PROP_TESTWHILEIDLE, value); + if (b != null) { + poolProperties.setTestWhileIdle(b.booleanValue()); + } } value = properties.getProperty(PROP_PASSWORD); @@ -594,12 +612,18 @@ public class DataSourceFactory implements ObjectFactory { value = properties.getProperty(PROP_ACCESSTOUNDERLYINGCONNECTIONALLOWED); if (value != null) { - poolProperties.setAccessToUnderlyingConnectionAllowed(Boolean.parseBoolean(value)); + Boolean b = parseBoolean(PROP_ACCESSTOUNDERLYINGCONNECTIONALLOWED, value); + if (b != null) { + poolProperties.setAccessToUnderlyingConnectionAllowed(b.booleanValue()); + } } value = properties.getProperty(PROP_REMOVEABANDONED); if (value != null) { - poolProperties.setRemoveAbandoned(Boolean.parseBoolean(value)); + Boolean b = parseBoolean(PROP_REMOVEABANDONED, value); + if (b != null) { + poolProperties.setRemoveAbandoned(b.booleanValue()); + } } value = properties.getProperty(PROP_REMOVEABANDONEDTIMEOUT); @@ -609,7 +633,10 @@ public class DataSourceFactory implements ObjectFactory { value = properties.getProperty(PROP_LOGABANDONED); if (value != null) { - poolProperties.setLogAbandoned(Boolean.parseBoolean(value)); + Boolean b = parseBoolean(PROP_LOGABANDONED, value); + if (b != null) { + poolProperties.setLogAbandoned(b.booleanValue()); + } } value = properties.getProperty(PROP_POOLPREPAREDSTATEMENTS); @@ -652,17 +679,26 @@ public class DataSourceFactory implements ObjectFactory { value = properties.getProperty(PROP_JMX_ENABLED); if (value != null) { - poolProperties.setJmxEnabled(Boolean.parseBoolean(value)); + Boolean b = parseBoolean(PROP_JMX_ENABLED, value); + if (b != null) { + poolProperties.setJmxEnabled(b.booleanValue()); + } } value = properties.getProperty(PROP_FAIR_QUEUE); if (value != null) { - poolProperties.setFairQueue(Boolean.parseBoolean(value)); + Boolean b = parseBoolean(PROP_FAIR_QUEUE, value); + if (b != null) { + poolProperties.setFairQueue(b.booleanValue()); + } } value = properties.getProperty(PROP_USE_EQUALS); if (value != null) { - poolProperties.setUseEquals(Boolean.parseBoolean(value)); + Boolean b = parseBoolean(PROP_USE_EQUALS, value); + if (b != null) { + poolProperties.setUseEquals(b.booleanValue()); + } } value = properties.getProperty(OBJECT_NAME); @@ -682,7 +718,10 @@ public class DataSourceFactory implements ObjectFactory { value = properties.getProperty(PROP_USE_CON_LOCK); if (value != null) { - poolProperties.setUseLock(Boolean.parseBoolean(value)); + Boolean b = parseBoolean(PROP_USE_CON_LOCK, value); + if (b != null) { + poolProperties.setUseLock(b.booleanValue()); + } } value = properties.getProperty(PROP_DATASOURCE); @@ -704,46 +743,101 @@ public class DataSourceFactory implements ObjectFactory { value = properties.getProperty(PROP_ALTERNATE_USERNAME_ALLOWED); if (value != null) { - poolProperties.setAlternateUsernameAllowed(Boolean.parseBoolean(value)); + Boolean b = parseBoolean(PROP_ALTERNATE_USERNAME_ALLOWED, value); + if (b != null) { + poolProperties.setAlternateUsernameAllowed(b.booleanValue()); + } } value = properties.getProperty(PROP_COMMITONRETURN); if (value != null) { - poolProperties.setCommitOnReturn(Boolean.parseBoolean(value)); + Boolean b = parseBoolean(PROP_COMMITONRETURN, value); + if (b != null) { + poolProperties.setCommitOnReturn(b.booleanValue()); + } } value = properties.getProperty(PROP_ROLLBACKONRETURN); if (value != null) { - poolProperties.setRollbackOnReturn(Boolean.parseBoolean(value)); + Boolean b = parseBoolean(PROP_ROLLBACKONRETURN, value); + if (b != null) { + poolProperties.setRollbackOnReturn(b.booleanValue()); + } } value = properties.getProperty(PROP_USEDISPOSABLECONNECTIONFACADE); if (value != null) { - storeDeprecatedUseDisposableConnectionFacade(poolProperties, - Boolean.parseBoolean(value)); + Boolean b = parseBoolean(PROP_USEDISPOSABLECONNECTIONFACADE, value); + if (b != null) { + storeDeprecatedUseDisposableConnectionFacade(poolProperties, + b.booleanValue()); + } } value = properties.getProperty(PROP_LOGVALIDATIONERRORS); if (value != null) { - poolProperties.setLogValidationErrors(Boolean.parseBoolean(value)); + Boolean b = parseBoolean(PROP_LOGVALIDATIONERRORS, value); + if (b != null) { + poolProperties.setLogValidationErrors(b.booleanValue()); + } } value = properties.getProperty(PROP_PROPAGATEINTERRUPTSTATE); if (value != null) { - poolProperties.setPropagateInterruptState(Boolean.parseBoolean(value)); + Boolean b = parseBoolean(PROP_PROPAGATEINTERRUPTSTATE, value); + if (b != null) { + poolProperties.setPropagateInterruptState(b.booleanValue()); + } } value = properties.getProperty(PROP_IGNOREEXCEPTIONONPRELOAD); if (value != null) { - poolProperties.setIgnoreExceptionOnPreLoad(Boolean.parseBoolean(value)); + Boolean b = parseBoolean(PROP_IGNOREEXCEPTIONONPRELOAD, value); + if (b != null) { + poolProperties.setIgnoreExceptionOnPreLoad(b.booleanValue()); + } } value = properties.getProperty(PROP_USESTATEMENTFACADE); if (value != null) { - poolProperties.setUseStatementFacade(Boolean.parseBoolean(value)); + Boolean b = parseBoolean(PROP_USESTATEMENTFACADE, value); + if (b != null) { + poolProperties.setUseStatementFacade(b.booleanValue()); + } } return poolProperties; } + /** + * Parses the value configured for a boolean property. Unlike + * {@link Boolean#parseBoolean(String)}, which silently yields + * {@code false} for any input, an unrecognized value is reported + * instead of being applied as {@code false}. + * @param name the property name, for the error message + * @param value the configured value, never {@code null} + * @return {@link Boolean#TRUE} or {@link Boolean#FALSE}, or {@code null} + * when the value is empty and the property is therefore to be + * left unset: at its {@link PoolProperties} default for a plain + * boolean, at its "use the driver default" state for a tri-state + * property + * @throws IllegalArgumentException if the value is neither + * {@code true} nor {@code false}, ignoring case and surrounding + * whitespace + */ + private static Boolean parseBoolean(String name, String value) { + String trimmed = value.trim(); + if (trimmed.isEmpty()) { + return null; + } + if ("true".equalsIgnoreCase(trimmed)) { + return Boolean.TRUE; + } + if ("false".equalsIgnoreCase(trimmed)) { + return Boolean.FALSE; + } + throw new IllegalArgumentException("Invalid boolean value '" + value + + "' for property " + name); + } + /** * Creates and configures a {@link DataSource} instance based on the * given properties. diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/naming/TestNamingFactory.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/naming/TestNamingFactory.java index 5610f9dac2..01b7026ec3 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/naming/TestNamingFactory.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/naming/TestNamingFactory.java @@ -35,6 +35,7 @@ import org.junit.jupiter.api.Test; import org.apache.tomcat.jdbc.JdbcPoolBaseTest; import org.apache.tomcat.jdbc.pool.DataSource; import org.apache.tomcat.jdbc.pool.DataSourceFactory; +import org.apache.tomcat.jdbc.pool.PoolConfiguration; import org.apache.tomcat.jdbc.pool.XADataSource; /** @@ -93,6 +94,137 @@ public class TestNamingFactory extends JdbcPoolBaseTest { } } + /* + * Boolean attributes are applied strictly: only "true" and "false", + * ignoring case and surrounding whitespace, configure the property; + * an empty attribute means "unset" - the PoolProperties default, and + * for the tri-state defaultAutoCommit/defaultReadOnly the "leave the + * driver default alone" state - and anything else fails the factory + * instead of being silently applied as false, the way the numeric + * attributes already fail on a bad value. + */ + + @Test + void misspelledBooleanAttributeFailsTheFactory() + throws Exception { + Reference ref = dataSourceReference("javax.sql.DataSource"); + // pre-fix this was silently applied as testOnBorrow=false + ref.add(new StringRefAddr("testOnBorrow", "ture")); + + IllegalArgumentException e = Assertions.assertThrows( + IllegalArgumentException.class, + () -> new DataSourceFactory().getObjectInstance( + ref, new CompositeName("jdbc/BadBooleanDS"), + null, null), + "a misspelled boolean value must fail the factory call"); + Assertions.assertTrue(e.getMessage().contains("testOnBorrow"), + "the failure must name the misconfigured attribute, was: " + + e.getMessage()); + Assertions.assertTrue(e.getMessage().contains("ture"), + "the failure must show the offending value, was: " + + e.getMessage()); + } + + @Test + void misspelledTriStateAttributeFailsTheFactory() + throws Exception { + Reference ref = dataSourceReference("javax.sql.DataSource"); + // pre-fix this was silently applied as defaultAutoCommit=FALSE, + // forcing auto-commit off on every connection of the pool + ref.add(new StringRefAddr("defaultAutoCommit", "Yes")); + + IllegalArgumentException e = Assertions.assertThrows( + IllegalArgumentException.class, + () -> new DataSourceFactory().getObjectInstance( + ref, new CompositeName("jdbc/BadTriStateDS"), + null, null), + "a misspelled tri-state boolean must fail the factory call " + + "instead of forcing an explicit value"); + Assertions.assertTrue(e.getMessage().contains("defaultAutoCommit"), + "the failure must name the misconfigured attribute, was: " + + e.getMessage()); + Assertions.assertTrue(e.getMessage().contains("Yes"), + "the failure must show the offending value, was: " + + e.getMessage()); + } + + @Test + void emptyBooleanAttributeLeavesThePropertyUnset() { + java.util.Properties attrs = new java.util.Properties(); + attrs.setProperty("defaultAutoCommit", ""); + attrs.setProperty("defaultReadOnly", ""); + // fairQueue defaults to true: pre-fix, the empty attribute forced + // false and silently changed the pool's behavior + attrs.setProperty("fairQueue", ""); + + PoolConfiguration configured = + DataSourceFactory.parsePoolProperties(attrs); + + Assertions.assertNull(configured.getDefaultAutoCommit(), + "an empty tri-state attribute must keep the " + + "\"use the driver default\" state"); + Assertions.assertNull(configured.getDefaultReadOnly(), + "an empty tri-state attribute must keep the " + + "\"use the driver default\" state"); + Assertions.assertTrue(configured.isFairQueue(), + "an empty plain boolean attribute must leave the " + + "PoolProperties default in place"); + } + + @Test + void emptyDefaultAutoCommitLeavesConnectionsAtTheDriverDefault() + throws Exception { + DataSource pool = factoryPoolWithAutoCommit(""); + try (java.sql.Connection c = pool.getConnection()) { + Assertions.assertTrue(c.getAutoCommit(), + "an empty defaultAutoCommit must leave the connections " + + "at the driver's own auto-commit default"); + } + } + + @Test + void configuredDefaultAutoCommitStillForcesTheStateOnConnections() + throws Exception { + DataSource pool = factoryPoolWithAutoCommit("false"); + try (java.sql.Connection c = pool.getConnection()) { + Assertions.assertFalse(c.getAutoCommit(), + "an explicitly configured defaultAutoCommit must still " + + "be applied to the connections"); + } + } + + @Test + void booleanValuesApplyIgnoringCaseAndSurroundingWhitespace() { + java.util.Properties attrs = new java.util.Properties(); + attrs.setProperty("testOnBorrow", "TRUE"); + // pre-fix, the trailing space made this false + attrs.setProperty("testOnReturn", "True "); + attrs.setProperty("testWhileIdle", " False "); + attrs.setProperty("removeAbandoned", "false"); + + PoolConfiguration configured = + DataSourceFactory.parsePoolProperties(attrs); + + Assertions.assertTrue(configured.isTestOnBorrow(), + "\"TRUE\" must be accepted as true"); + Assertions.assertTrue(configured.isTestOnReturn(), + "\"True \" must be accepted as true, whitespace included"); + Assertions.assertFalse(configured.isTestWhileIdle(), + "\" False \" must be accepted as false, whitespace included"); + Assertions.assertFalse(configured.isRemoveAbandoned(), + "the exact lowercase spelling must keep working"); + } + + /* A factory-built pool, H2-backed, with one extra attribute. */ + private DataSource factoryPoolWithAutoCommit(String value) + throws Exception { + Reference ref = dataSourceReference("javax.sql.DataSource"); + ref.add(new StringRefAddr("jmxEnabled", "false")); + ref.add(new StringRefAddr("defaultAutoCommit", value)); + return manage((DataSource) new DataSourceFactory().getObjectInstance( + ref, new CompositeName("jdbc/AutoCommitDS"), null, null)); + } + @Test void namingFactoryAppliesDocumentedPropertyTypes() throws Exception { Reference ref = new Reference(NamingTarget.class.getName(), null, null); --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
