This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 9.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit df9e7c2fb42df5721a3e753d349bc89ac6e2320f Author: opencode <[email protected]> AuthorDate: Thu Nov 5 20:00:28 2026 +0100 Fix clearing credentials backed by a defaults table PoolProperties.setPassword(null) and setUsername(null) used Properties.remove, which only removes directly present entries. A value supplied by a defaults table remained resolvable and was materialized into the driver copy, so a cleared credential could still authenticate. setConnectionProperties had the same issue when dropping a contributed key. Remove directly present entries in place as before, and when the removal uncovers a defaults value, continue on a defaults-free copy and publish it. Add regression tests for defaults-backed credentials and connection properties. --- .../apache/tomcat/jdbc/pool/PoolProperties.java | 50 +++++++++++++++++++--- .../tomcat/jdbc/pool/TestPoolProperties.java | 43 +++++++++++++++++++ 2 files changed, 87 insertions(+), 6 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java index eb648ec43c..62dca40e7e 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java @@ -584,16 +584,27 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl connectionPropertyKeys = Collections.emptySet(); return; } + // Removing a key only reaches a directly present entry, so when the + // collection is defaults-backed the removal can uncover the defaults + // value instead of dropping the key. Detect that on each removal and, + // if it happens, continue on a defaults-free copy that is published + // instead of the live collection. + Properties writable = dbProps; // A configuration serialized before this field existed deserializes // with it null, as field initializers do not run on deserialization. if (connectionPropertyKeys != null) { for (String key : connectionPropertyKeys) { - dbProps.remove(key); + writable.remove(key); + if (writable.getProperty(key) != null) { + Properties copy = PoolUtilities.clone(writable); + copy.remove(key); + writable = copy; + } } } Set<String> keys = new HashSet<>(parsed.size()); for (String key : parsed.stringPropertyNames()) { - dbProps.setProperty(key, parsed.getProperty(key)); + writable.setProperty(key, parsed.getProperty(key)); keys.add(key); } // The credential setters' rule outlives the string: while a credential @@ -608,11 +619,12 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl // contributed it; this parse hands the pair back - writing the string // is a fresh declaration of what it contributes. if (username != null) { - dbProps.setProperty(PoolUtilities.PROP_USER, username); + writable.setProperty(PoolUtilities.PROP_USER, username); } if (password != null) { - dbProps.setProperty(PoolUtilities.PROP_PASSWORD, password); + writable.setProperty(PoolUtilities.PROP_PASSWORD, password); } + dbProperties = writable; connectionPropertyKeys = keys; } @@ -745,7 +757,7 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl this.password = password; if (this.dbProperties != null) { if (password == null) { - this.dbProperties.remove(PoolUtilities.PROP_PASSWORD); + removeDbProperty(PoolUtilities.PROP_PASSWORD); warnStringStillNamesCredential(PoolUtilities.PROP_PASSWORD); } else { this.dbProperties.setProperty(PoolUtilities.PROP_PASSWORD, password); @@ -808,7 +820,7 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl this.username = username; if (this.dbProperties != null) { if (username == null) { - this.dbProperties.remove(PoolUtilities.PROP_USER); + removeDbProperty(PoolUtilities.PROP_USER); warnStringStillNamesCredential(PoolUtilities.PROP_USER); } else { this.dbProperties.setProperty(PoolUtilities.PROP_USER, username); @@ -817,6 +829,32 @@ public class PoolProperties implements PoolConfiguration, Cloneable, Serializabl } + /** + * Removes an entry from the database properties, including a value that + * the collection only resolves through a defaults table. A plain + * {@link Properties#remove(Object)} reaches only a directly present entry, + * so it would leave such a value in place - and the connect path + * materializes it into the copy handed to the driver. The direct entry is + * removed in place, and only when that uncovers a defaults value is the + * result materialized into a defaults-free copy and published instead. + * Called with the monitor held. + * + * @param key the property key to remove + */ + private void removeDbProperty(String key) { + Properties props = this.dbProperties; + if (props == null) { + return; + } + props.remove(key); + if (props.getProperty(key) != null) { + Properties copy = PoolUtilities.clone(props); + copy.remove(key); + this.dbProperties = copy; + } + } + + /** * Notes at warn level when clearing a credential has just removed an entry * that the configured {@code connectionProperties} string still names, so diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPoolProperties.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPoolProperties.java index 087be04a3c..b3faf2c14f 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPoolProperties.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPoolProperties.java @@ -338,6 +338,49 @@ class TestPoolProperties { "the string is configuration and is not rewritten by a credential clear"); } + @Test + void clearingCredentialsReachesValuesSuppliedByADefaultsTable() { + Properties defaults = new Properties(); + defaults.setProperty(PoolUtilities.PROP_USER, DEFAULT_USER); + defaults.setProperty(PoolUtilities.PROP_PASSWORD, DEFAULT_PASSWD); + PoolProperties properties = new PoolProperties(); + properties.setDbProperties(new Properties(defaults)); + + properties.setPassword(null); + properties.setUsername(null); + + Assertions.assertNull(properties.getDbProperties().getProperty(PoolUtilities.PROP_PASSWORD), + "clearing the password must remove a value a defaults table supplies"); + Assertions.assertNull(properties.getDbProperties().getProperty(PoolUtilities.PROP_USER), + "clearing the user name must remove a value a defaults table supplies"); + Assertions.assertNull( + PoolUtilities.clone(properties.getDbProperties()).getProperty(PoolUtilities.PROP_PASSWORD), + "the copy handed to the driver must not materialize the cleared password"); + Assertions.assertNull( + PoolUtilities.clone(properties.getDbProperties()).getProperty(PoolUtilities.PROP_USER), + "the copy handed to the driver must not materialize the cleared user name"); + } + + @Test + void droppingAConnectionPropertyReachesADefaultsTable() { + Properties defaults = new Properties(); + defaults.setProperty("sslmode", "require"); + PoolProperties properties = new PoolProperties(); + properties.setDbProperties(new Properties(defaults)); + + properties.setConnectionProperties("sslmode=disable"); + Assertions.assertEquals("disable", + PoolUtilities.clone(properties.getDbProperties()).getProperty("sslmode"), + "the string's pair must shadow the defaults value"); + + properties.setConnectionProperties(null); + + Assertions.assertNull(properties.getDbProperties().getProperty("sslmode"), + "clearing the string must remove the pair it contributed"); + Assertions.assertNull(PoolUtilities.clone(properties.getDbProperties()).getProperty("sslmode"), + "the dropped pair must not fall back to the defaults value the driver copy would send"); + } + @Test void rewritingTheStringAppliesItsCredentialPairAgain() { PoolProperties properties = new PoolProperties(); --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
