This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 11.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 4cf5d2c9536cbe13ec49b4bad027f4a59917d6a1 Author: opencode <[email protected]> AuthorDate: Thu Nov 5 23:06:19 2026 +0100 Tighten the DataSourceProxy bean and JMX surface Mask the parsed interceptor definitions returned by getJdbcInterceptorsAsArray(), like the string getter and the pool MBean already do, so a bean/JMX client cannot recover a credential from the parsed property map. The masking copy shares no definition, property or slot with the live configuration, so a caller cannot mutate the chain a later connection builds. Also catch RuntimeException in getPool() so a malformed interceptor configuration returns null as documented instead of escaping from the getter, clamp a negative login timeout to zero, and reject setJmxEnabled() after close() as well as after pool creation. Add regression tests for each. --- .../apache/tomcat/jdbc/pool/DataSourceProxy.java | 30 ++++++++++--- .../tomcat/jdbc/pool/util/LocalStrings.properties | 1 + .../jdbc/pool/TestDataSourceProxyCredentials.java | 51 ++++++++++++++++++++++ .../pool/TestJdbcInterceptorConfigParsing.java | 11 +++++ .../tomcat/jdbc/pool/jmx/TestJmxRegistration.java | 8 ++++ 5 files changed, 94 insertions(+), 7 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceProxy.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceProxy.java index d0d867513d..8de9d82641 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceProxy.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceProxy.java @@ -395,12 +395,16 @@ public class DataSourceProxy implements PoolConfiguration { public ConnectionPool getPool() { try { return createPool(); - } catch (SQLException x) { + } catch (SQLException | RuntimeException x) { if (closed) { // A closed data source legitimately has no pool; this is // a normal lifecycle state, not a creation failure. return null; } + // A configuration error before the pool is built - a malformed + // interceptor value, for example - is a creation failure like any + // other to this contract: report it and answer no pool rather than + // let it escape from a getter documented to return null. log.error(MessageFactory.get("dataSourceProxy.errorCreatingPool"), x); return null; } @@ -795,14 +799,18 @@ public class DataSourceProxy implements PoolConfiguration { /** * Sets whether JMX is enabled, before the pool is created. Whether the pool registers an MBean is decided when * the pool is created, so a write after that point is rejected rather than stored without effect, matching the - * pool MBean's own setter. + * pool MBean's own setter. A closed data source is rejected as well: it can never create a pool again, so a write + * there would be stored without any possible effect. * * @param enabled whether JMX is enabled * - * @throws UnsupportedOperationException if the pool has already been created + * @throws UnsupportedOperationException if the pool has already been created or this data source has been closed */ @Override public synchronized void setJmxEnabled(boolean enabled) { + if (closed) { + throw new UnsupportedOperationException(MessageFactory.get("dataSourceProxy.jmxEnabledAfterClose")); + } if (pool != null) { throw new UnsupportedOperationException(MessageFactory.get("dataSourceProxy.jmxEnabledAfterStart")); } @@ -898,12 +906,13 @@ public class DataSourceProxy implements PoolConfiguration { /** * The pool's borrow wait timeout, {@link PoolConfiguration#getMaxWait()}, expressed in whole seconds: * {@link javax.sql.DataSource#getLoginTimeout()} is mapped onto that property rather than ignored. A - * sub-second <code>maxWait</code> therefore reads back as zero. + * sub-second <code>maxWait</code> therefore reads back as zero, and so does a negative one, which the pool reads + * as "wait indefinitely"; a negative login timeout is not a legal JDBC value. * - * @return the timeout in whole seconds + * @return the timeout in whole seconds, never negative */ public int getLoginTimeout() { - return poolProperties.getMaxWait() / 1000; + return Math.max(0, poolProperties.getMaxWait() / 1000); } /** @@ -1425,9 +1434,16 @@ public class DataSourceProxy implements PoolConfiguration { } + /** + * Returns the parsed interceptor definitions with their credential values masked, the display counterpart of + * {@link #getJdbcInterceptors()}. This object doubles as its own JMX/bean surface, so the raw definitions - which + * carry the pairs the pool builds its interceptor chains from - stay reachable only through + * {@link #getPoolProperties()}. The copy is also defensive: it shares no definition, property or array slot with the + * live configuration, so a caller cannot mutate the chain a later connection will build. {@inheritDoc} + */ @Override public InterceptorDefinition[] getJdbcInterceptorsAsArray() { - return getPoolProperties().getJdbcInterceptorsAsArray(); + return PoolProperties.maskInterceptorCredentials(getPoolProperties().getJdbcInterceptorsAsArray()); } diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties index 1b5e4b79dd..631c562cf3 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties @@ -123,6 +123,7 @@ dataSourceProxy.connectionNotXA=Connection from pool does not implement javax.sq dataSourceProxy.dataSourceClosed=DataSource has been closed. dataSourceProxy.errorClosingPool=Error during connection pool closure. dataSourceProxy.errorCreatingPool=Error during connection pool creation. +dataSourceProxy.jmxEnabledAfterClose=jmxEnabled cannot be changed after the data source has been closed dataSourceProxy.jmxEnabledAfterStart=jmxEnabled must be set before the pool is started dataSourceProxy.notAWrapper=Not a wrapper of {0} dataSourceProxy.poolConfigurationNull=PoolConfiguration cannot be null. diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestDataSourceProxyCredentials.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestDataSourceProxyCredentials.java index d02e8cec8d..a050dab669 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestDataSourceProxyCredentials.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestDataSourceProxyCredentials.java @@ -130,6 +130,57 @@ class TestDataSourceProxyCredentials { Assertions.assertNotSame(live, view, "the copy must not be the live store"); } + @Test + void getLoginTimeoutNeverReturnsANegativeValue() { + DataSourceProxy proxy = new DataSourceProxy(); + proxy.getPoolProperties().setMaxWait(-5000); + + Assertions.assertEquals(0, proxy.getLoginTimeout(), + "a negative maxWait means wait indefinitely, and a negative login timeout is" + + " not a legal JDBC value"); + proxy.setLoginTimeout(5); + Assertions.assertEquals(5, proxy.getLoginTimeout(), "a normal timeout must still round-trip"); + } + + @Test + void parsedInterceptorDefinitionsAreMaskedAndAreNotTheLiveDefinitions() { + DataSourceProxy proxy = new DataSourceProxy(); + proxy.setJdbcInterceptors("ResetAbandonedTimer(password=s3cret);StatementCache(pass=s3cret2)"); + + PoolProperties.InterceptorDefinition[] view = proxy.getJdbcInterceptorsAsArray(); + PoolProperties.InterceptorDefinition timer = null; + PoolProperties.InterceptorDefinition cache = null; + for (PoolProperties.InterceptorDefinition definition : view) { + if (definition.getClassName().endsWith("ResetAbandonedTimer")) { + timer = definition; + } else if (definition.getClassName().endsWith("StatementCache")) { + cache = definition; + } + } + Assertions.assertNotNull(timer, "the parsed view must carry the configured timer"); + Assertions.assertNotNull(cache, "the parsed view must carry the configured cache"); + Assertions.assertEquals("********", timer.getProperties().get("password").getValue(), + "the parsed bean/JMX view must mask a credential exactly like the string getter"); + Assertions.assertEquals("********", cache.getProperties().get("pass").getValue(), + "the parsed bean/JMX view must mask every credential-shaped key"); + + // The raw definitions stay reachable through getPoolProperties() for + // trusted in-process code, and mutating the display copy cannot change + // the chain the pool builds. + PoolProperties.InterceptorDefinition liveTimer = null; + for (PoolProperties.InterceptorDefinition definition : proxy.getPoolProperties() + .getJdbcInterceptorsAsArray()) { + if (definition.getClassName().endsWith("ResetAbandonedTimer")) { + liveTimer = definition; + } + } + Assertions.assertNotNull(liveTimer); + Assertions.assertEquals("s3cret", liveTimer.getProperties().get("password").getValue()); + timer.getProperties().put("password", new PoolProperties.InterceptorProperty("password", "changed")); + Assertions.assertEquals("s3cret", liveTimer.getProperties().get("password").getValue(), + "the display copy must share no property map with the live definitions"); + } + /** * Database properties whose {@code password} write, once armed, lets a * concurrent credential change run inside it and then completes the write - diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestJdbcInterceptorConfigParsing.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestJdbcInterceptorConfigParsing.java index 99548b1808..9ea43d0762 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestJdbcInterceptorConfigParsing.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestJdbcInterceptorConfigParsing.java @@ -153,6 +153,17 @@ class TestJdbcInterceptorConfigParsing { } } + @Test + void getPoolReturnsNullForAnUncheckedConfigurationFailure() { + DataSourceProxy proxy = new DataSourceProxy(); + proxy.setJdbcInterceptors("MalformedParmsInterceptor(a=1,b)"); + + Assertions.assertDoesNotThrow(proxy::getPool, + "getPool documents null on a creation failure, including an unchecked" + + " configuration error before the pool is built"); + Assertions.assertNull(proxy.getPool(), "an unchecked creation failure must read as no pool"); + } + @Test void otherMalformedConfigsOnlyFailLater() { String[] noExceptionButInvalidConfigs = { diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxRegistration.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxRegistration.java index 90d58a8c35..2a9b5869ac 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxRegistration.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxRegistration.java @@ -966,6 +966,14 @@ class TestJmxRegistration extends JdbcPoolBaseTest { () -> ds.setJmxEnabled(false), "a post-start write would otherwise be stored without creating or removing anything"); Assertions.assertTrue(ds.isJmxEnabled(), "the refused write must not change the configuration"); + + ds.close(); + Assertions.assertThrows(UnsupportedOperationException.class, + () -> ds.setJmxEnabled(false), + "a closed data source can never create a pool again, so a write there would be" + + " stored without any possible effect"); + Assertions.assertTrue(ds.isJmxEnabled(), + "the refused write on a closed data source must not change the configuration"); } @Test --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
