Copilot commented on code in PR #10801:
URL: https://github.com/apache/gravitino/pull/10801#discussion_r3107940119
##########
catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/utils/DataSourceUtils.java:
##########
@@ -45,6 +45,17 @@ public static DataSource createDataSource(Map<String,
String> properties) {
public static DataSource createDataSource(JdbcConfig jdbcConfig)
throws GravitinoRuntimeException {
+ // H2 is bundled as an embedded backend and must not be used through
user-facing catalog
+ // configuration. Its INIT parameter allows arbitrary SQL (and Java code
via CREATE ALIAS)
+ // to execute at connection time, and the H2 driver class must also be
blocked to prevent
+ // bypassing this check via a mismatched driver and URL combination.
+ String decodedUrl = recursiveDecode(jdbcConfig.getJdbcUrl().toLowerCase());
+ if (decodedUrl.startsWith("jdbc:h2")) {
+ throw new GravitinoRuntimeException("H2 JDBC URL is not allowed in
catalog configuration");
+ }
+ if (jdbcConfig.getJdbcDriver().toLowerCase().startsWith("org.h2.")) {
+ throw new GravitinoRuntimeException("H2 JDBC driver is not allowed in
catalog configuration");
Review Comment:
Use locale-independent lowercasing for case-insensitive comparisons here
(e.g., Locale.ROOT) to avoid edge cases on systems with non-English default
locales.
##########
catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/utils/TestDataSourceUrlValidation.java:
##########
@@ -84,4 +84,67 @@ public void testRejectEncodedAllowLoadLocalInfile() {
Assertions.assertThrows(
GravitinoRuntimeException.class, () ->
DataSourceUtils.createDataSource(properties));
}
+
+ @Test
+ public void testRejectH2Url() {
+ HashMap<String, String> properties = Maps.newHashMap();
+ properties.put(JdbcConfig.JDBC_DRIVER.getKey(), "org.postgresql.Driver");
+ properties.put(
+ JdbcConfig.JDBC_URL.getKey(),
+ "jdbc:h2:mem:test;INIT=CREATE ALIAS EXEC AS 'String f() throws
Exception"
+ + " { Runtime.getRuntime().exec(\"id\"); return \"ok\"; }'\\;CALL
EXEC()");
+ properties.put(JdbcConfig.USERNAME.getKey(), "test");
+ properties.put(JdbcConfig.PASSWORD.getKey(), "test");
+
+ GravitinoRuntimeException gre =
+ Assertions.assertThrows(
+ GravitinoRuntimeException.class, () ->
DataSourceUtils.createDataSource(properties));
+ Assertions.assertEquals(
+ "H2 JDBC URL is not allowed in catalog configuration",
gre.getMessage());
+ }
+
+ @Test
+ public void testRejectH2UrlCaseInsensitive() {
+ HashMap<String, String> properties = Maps.newHashMap();
+ properties.put(JdbcConfig.JDBC_DRIVER.getKey(), "org.postgresql.Driver");
+ properties.put(JdbcConfig.JDBC_URL.getKey(), "JDBC:H2:mem:test");
+ properties.put(JdbcConfig.USERNAME.getKey(), "test");
+ properties.put(JdbcConfig.PASSWORD.getKey(), "test");
+
+ GravitinoRuntimeException gre =
+ Assertions.assertThrows(
+ GravitinoRuntimeException.class, () ->
DataSourceUtils.createDataSource(properties));
+ Assertions.assertEquals(
+ "H2 JDBC URL is not allowed in catalog configuration",
gre.getMessage());
+ }
Review Comment:
The H2 URL check relies on recursiveDecode() to prevent percent-encoded
bypasses, but the new tests only cover plain/case-variant URLs. Add a test that
verifies an encoded H2 URL (e.g., encoded ':' characters, and ideally
double-encoded) is also rejected, to ensure the decode path is actually
enforced.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
