This is an automated email from the ASF dual-hosted git repository. ostinru pushed a commit to branch safe-postgres-jdbc in repository https://gitbox.apache.org/repos/asf/cloudberry-pxf.git
commit cb2f3307325ed40db62b98385df97a154ba9b920 Author: Nikolay Antonov <[email protected]> AuthorDate: Wed Dec 24 12:35:15 2025 +0500 Do not enforce username/passwords for non-postgresql datasources (#17) `java.net.URI` is not proper tool to parse JDBC connections strings: * it fails to parse schemas that contains ':'. For example, `jdbc:postgres://` and `jdbc:sqlserver://` will fail to parse. * Also it won't work with mircosoft jdbc that doesn't use `?` as separator between hostname and query parameters[1]. For example, in jdbcUrl `jdbc:sqlserver://localhost:1433;databaseName=db1;user=admin` it will not extract `user` field. **Solution**: Use `Driver.parseURL` for postgresql and do not check other drivers. [1] https://learn.microsoft.com/en-us/sql/connect/jdbc/using-the-jdbc-driver?view=sql-server-ver17#make-a-simple-connection-to-a-database --- .../pxf/plugins/jdbc/JdbcBasePlugin.java | 72 ++++++++++++---------- .../pxf/plugins/jdbc/JdbcBasePluginTest.java | 47 ++++++++------ 2 files changed, 67 insertions(+), 52 deletions(-) diff --git a/server/pxf-jdbc/src/main/java/org/apache/cloudberry/pxf/plugins/jdbc/JdbcBasePlugin.java b/server/pxf-jdbc/src/main/java/org/apache/cloudberry/pxf/plugins/jdbc/JdbcBasePlugin.java index 1ded13ff..1cda61ed 100644 --- a/server/pxf-jdbc/src/main/java/org/apache/cloudberry/pxf/plugins/jdbc/JdbcBasePlugin.java +++ b/server/pxf-jdbc/src/main/java/org/apache/cloudberry/pxf/plugins/jdbc/JdbcBasePlugin.java @@ -30,12 +30,11 @@ import org.apache.cloudberry.pxf.api.utilities.Utilities; import org.apache.cloudberry.pxf.plugins.jdbc.utils.ConnectionManager; import org.apache.cloudberry.pxf.plugins.jdbc.utils.DbProduct; import org.apache.cloudberry.pxf.plugins.jdbc.utils.HiveJdbcUtils; +import org.postgresql.Driver; +import org.postgresql.PGProperty; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.io.IOException; -import java.net.URI; -import java.net.URISyntaxException; import java.security.PrivilegedExceptionAction; import java.sql.Connection; import java.sql.DatabaseMetaData; @@ -100,6 +99,7 @@ public class JdbcBasePlugin extends BasePlugin { private static final String HIVE_DEFAULT_DRIVER_CLASS = "org.apache.hive.jdbc.HiveDriver"; private static final String MYSQL_DRIVER_PREFIX = "com.mysql."; private static final String JDBC_DATE_WIDE_RANGE = "jdbc.date.wideRange"; + private static final String POSTGRESQL_URL_PREFIX = "jdbc:postgresql://"; private enum TransactionIsolation { READ_UNCOMMITTED(1), @@ -303,25 +303,6 @@ public class JdbcBasePlugin extends BasePlugin { String transactionIsolationString = configuration.get(JDBC_CONNECTION_TRANSACTION_ISOLATION, "NOT_PROVIDED"); transactionIsolation = TransactionIsolation.typeOf(transactionIsolationString); - boolean hasUserInUrl = false; - boolean hasPasswordInUrl = false; - try { - URI uri = new URI(jdbcUrl); - String query = uri.getQuery(); - if (query != null && !query.isEmpty()) { - hasUserInUrl = query.contains("user="); - hasPasswordInUrl = query.contains("password="); - } - String rawUserInfo = uri.getRawUserInfo(); - if (rawUserInfo != null && !rawUserInfo.isEmpty()) { - hasUserInUrl = true; - hasPasswordInUrl = rawUserInfo.contains("/"); // https://www.orafaq.com/wiki/JDBC - } - } catch (URISyntaxException e) { - // If the URL is malformed, it's also an invalid argument - throw new IllegalArgumentException("Invalid JDBC URL format provided: " + jdbcUrl, e); - } - // Set optional user parameter, taking into account impersonation setting for the server. String jdbcUser = configuration.get(JDBC_USER_PROPERTY_NAME); boolean impersonationEnabledForServer = configuration.getBoolean(CONFIG_KEY_SERVICE_USER_IMPERSONATION, false); @@ -340,9 +321,6 @@ public class JdbcBasePlugin extends BasePlugin { if (jdbcUser != null && !jdbcUser.isEmpty()) { LOG.debug("Effective JDBC user {}", jdbcUser); connectionConfiguration.setProperty("user", jdbcUser); - } else if (!hasUserInUrl) { - LOG.debug("JDBC user has not been set"); - throw new IllegalArgumentException("JDBC user has not been set"); } if (LOG.isDebugEnabled()) { @@ -355,18 +333,15 @@ public class JdbcBasePlugin extends BasePlugin { // This must be the last parameter parsed, as we output connectionConfiguration earlier // Optional parameter. By default, corresponding connectionConfiguration property is not set - boolean passwordSetted = false; + String jdbcPassword = configuration.get(JDBC_PASSWORD_PROPERTY_NAME); if (jdbcUser != null) { - String jdbcPassword = configuration.get(JDBC_PASSWORD_PROPERTY_NAME); if (jdbcPassword != null && !jdbcPassword.isEmpty()) { LOG.debug("Connection password: {}", ConnectionManager.maskPassword(jdbcPassword)); connectionConfiguration.setProperty("password", jdbcPassword); - passwordSetted = true; } } - if (!passwordSetted && !hasPasswordInUrl) { - throw new IllegalArgumentException("JDBC password has not been set"); - } + + assertJDBC(jdbcUrl, jdbcUser, jdbcPassword); // connection pool is optional, enabled by default isConnectionPoolUsed = configuration.getBoolean(JDBC_CONNECTION_POOL_ENABLED_PROPERTY_NAME, true); @@ -429,6 +404,38 @@ public class JdbcBasePlugin extends BasePlugin { return connection; } + /** + * Asserts whether all required JDBC URL params/properties set, throws IllegalArgumentException otherwise + * + * @param jdbcUrl connection string + * @param username username from properties (outside of connection string) + * @param password password from properties (outside of connection string) + */ + private void assertJDBC(String jdbcUrl, String username, String password) { + if (!StringUtils.startsWith(jdbcUrl, POSTGRESQL_URL_PREFIX)) { + return; + } + + // For PostgreSQL we enforce users to specify username and password + Properties urlProperties = Driver.parseURL(jdbcUrl, null); + if (urlProperties == null) { + throw new IllegalArgumentException("Invalid PostgreSQL JDBC URL format provided: " + jdbcUrl); + } + + String urlUsername = urlProperties.getProperty(PGProperty.USER.getName()); + if (StringUtils.isEmpty(username) && StringUtils.isEmpty(urlUsername)) { + LOG.debug("PostgreSQL JDBC user has not been set"); + throw new IllegalArgumentException("PostgreSQL JDBC user has not been set"); + } + + String urlPassword = urlProperties.getProperty(PGProperty.PASSWORD.getName()); + if (StringUtils.isEmpty(password) && StringUtils.isEmpty(urlPassword)) { + throw new IllegalArgumentException("PostgreSQL JDBC password has not been set"); + } + + // Ok. It is fine to work with this postgres datasource +} + /** * Prepare a JDBC PreparedStatement * @@ -624,6 +631,3 @@ public class JdbcBasePlugin extends BasePlugin { } } - - - diff --git a/server/pxf-jdbc/src/test/java/org/apache/cloudberry/pxf/plugins/jdbc/JdbcBasePluginTest.java b/server/pxf-jdbc/src/test/java/org/apache/cloudberry/pxf/plugins/jdbc/JdbcBasePluginTest.java index db6cf0bc..2bcb781d 100644 --- a/server/pxf-jdbc/src/test/java/org/apache/cloudberry/pxf/plugins/jdbc/JdbcBasePluginTest.java +++ b/server/pxf-jdbc/src/test/java/org/apache/cloudberry/pxf/plugins/jdbc/JdbcBasePluginTest.java @@ -344,8 +344,8 @@ public class JdbcBasePluginTest { @Test public void testGetConnectionErrorWithoutPassword1() throws SQLException { - configuration.set("jdbc.driver", "org.greenplum.pxf.plugins.jdbc.FakeJdbcDriver"); - configuration.set("jdbc.url", "test-url"); + configuration.set("jdbc.driver", "org.postgresql.Driver"); + configuration.set("jdbc.url", "jdbc:postgresql://example.com/test-url"); configuration.set("jdbc.password", ""); context.setServerName("test-server"); @@ -353,7 +353,7 @@ public class JdbcBasePluginTest { try { getPlugin(mockConnectionManager, mockSecureLogin, context); } catch (IllegalArgumentException e) { - assertEquals("JDBC password has not been set", e.getMessage()); + assertEquals("PostgreSQL JDBC password has not been set", e.getMessage()); return; } Assertions.fail("Expected an exception to be thrown due to missing password, but no exception was thrown."); @@ -362,9 +362,9 @@ public class JdbcBasePluginTest { @Test public void testGetConnectionErrorWithoutPassword2() throws SQLException { configuration = new Configuration(); + configuration.set("jdbc.driver", "org.postgresql.Driver"); + configuration.set("jdbc.url", "jdbc:postgresql://example.com/test-url"); configuration.set("jdbc.user", "test-user"); - configuration.set("jdbc.driver", "org.greenplum.pxf.plugins.jdbc.FakeJdbcDriver"); - configuration.set("jdbc.url", "test-url"); context.setConfiguration(configuration); context.setServerName("test-server"); @@ -372,17 +372,28 @@ public class JdbcBasePluginTest { try { getPlugin(mockConnectionManager, mockSecureLogin, context); } catch (IllegalArgumentException e) { - assertEquals("JDBC password has not been set", e.getMessage()); + assertEquals("PostgreSQL JDBC password has not been set", e.getMessage()); return; } Assertions.fail("Expected an exception to be thrown due to missing password, but no exception was thrown."); } + @Test + public void testGetConnectionErrorWithoutPassword3() throws SQLException { + configuration.set("jdbc.driver", "org.greenplum.pxf.plugins.jdbc.FakeJdbcDriver"); + configuration.set("jdbc.url", "jdbc:sqlserver://localhost:1433;databaseName=db1;user=admin"); + + context.setServerName("test-server"); + + // non-PostgreSQL drivers are not subject to credential validation + getPlugin(mockConnectionManager, mockSecureLogin, context); + } + @Test public void testGetConnectionWithPasswordInUrl() throws SQLException { configuration = new Configuration(); - configuration.set("jdbc.driver", "org.greenplum.pxf.plugins.jdbc.FakeJdbcDriver"); - configuration.set("jdbc.url", "test-url?password=test-password"); + configuration.set("jdbc.driver", "org.postgresql.Driver"); + configuration.set("jdbc.url", "jdbc:postgresql://example.com/test-url?password=test-password"); configuration.set("jdbc.user", "test-user"); context.setServerName("test-server"); @@ -399,13 +410,13 @@ public class JdbcBasePluginTest { Properties properties = new Properties(); properties.setProperty("user", "test-user"); - verify(mockConnectionManager).getConnection("test-server", "test-url?password=test-password", properties, true, poolProps, null); + verify(mockConnectionManager).getConnection("test-server", "jdbc:postgresql://example.com/test-url?password=test-password", properties, true, poolProps, null); } @Test public void testGetConnectionErrorWithoutUser1() throws SQLException { - configuration.set("jdbc.driver", "org.greenplum.pxf.plugins.jdbc.FakeJdbcDriver"); - configuration.set("jdbc.url", "test-url"); + configuration.set("jdbc.driver", "org.postgresql.Driver"); + configuration.set("jdbc.url", "jdbc:postgresql://example.com/test-url"); configuration.set("jdbc.user", ""); configuration.set("jdbc.password", "test-password"); @@ -414,7 +425,7 @@ public class JdbcBasePluginTest { try { getPlugin(mockConnectionManager, mockSecureLogin, context); } catch (IllegalArgumentException e) { - assertEquals("JDBC user has not been set", e.getMessage()); + assertEquals("PostgreSQL JDBC user has not been set", e.getMessage()); return; } Assertions.fail("Expected an exception to be thrown due to missing user, but no exception was thrown."); @@ -424,8 +435,8 @@ public class JdbcBasePluginTest { public void testGetConnectionErrorWithoutUser2() throws SQLException { configuration = new Configuration(); configuration.set("jdbc.password", "test-password"); - configuration.set("jdbc.driver", "org.greenplum.pxf.plugins.jdbc.FakeJdbcDriver"); - configuration.set("jdbc.url", "test-url"); + configuration.set("jdbc.driver", "org.postgresql.Driver"); + configuration.set("jdbc.url", "jdbc:postgresql://example.com/test-url"); context.setConfiguration(configuration); context.setServerName("test-server"); @@ -433,7 +444,7 @@ public class JdbcBasePluginTest { try { getPlugin(mockConnectionManager, mockSecureLogin, context); } catch (IllegalArgumentException e) { - assertEquals("JDBC user has not been set", e.getMessage()); + assertEquals("PostgreSQL JDBC user has not been set", e.getMessage()); return; } Assertions.fail("Expected an exception to be thrown due to missing user, but no exception was thrown."); @@ -442,8 +453,8 @@ public class JdbcBasePluginTest { @Test public void testGetConnectionWithUserAndPasswordInUrl() throws SQLException { configuration = new Configuration(); - configuration.set("jdbc.driver", "org.greenplum.pxf.plugins.jdbc.FakeJdbcDriver"); - configuration.set("jdbc.url", "test-url?user=test-user&password=test-password"); + configuration.set("jdbc.driver", "org.postgresql.Driver"); + configuration.set("jdbc.url", "jdbc:postgresql://example.com/db?user=test-user&password=test-password"); context.setConfiguration(configuration); context.setServerName("test-server"); @@ -456,7 +467,7 @@ public class JdbcBasePluginTest { assertSame(mockConnection, conn); - verify(mockConnectionManager).getConnection("test-server", "test-url?user=test-user&password=test-password", new Properties(), true, poolProps, null); + verify(mockConnectionManager).getConnection("test-server", "jdbc:postgresql://example.com/db?user=test-user&password=test-password", new Properties(), true, poolProps, null); } @Test --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
