This is an automated email from the ASF dual-hosted git repository.
yuqi1129 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 3dab3db67a [#13238] fix(common): accept JDBC URLs with a literal
percent without weakening the unsafe-parameter scan (#13239)
3dab3db67a is described below
commit 3dab3db67a4d73632a8f1a9559f30123172089d0
Author: YangJie <[email protected]>
AuthorDate: Sat Sep 19 22:40:02 2026 -0400
[#13238] fix(common): accept JDBC URLs with a literal percent without
weakening the unsafe-parameter scan (#13239)
### What changes were proposed in this pull request?
Decoding now falls back to the last fully decoded form instead of
throwing, and the unsafe-parameter scan (and the
`jdbc:mysql`/`mariadb`/`postgresql`/`h2` prefix gates) additionally
scans a sanitized form in which malformed percent escapes are
re-sanitized before every decode pass. `DataSourceUtils`'s duplicate
throwing decode is removed and its H2 check uses the shared helper.
### Why are the changes needed?
The throwing decode rejected legal JDBC URLs with a literal `%` or a
once-encoded `%25`. Sanitizing on every pass is required for security:
MySQL Connector/J decodes query tokens independently and ignores the URL
fragment, so a malformed escape in the fragment (e.g. `#%zz`) must not
halt decoding before a legitimately multi-encoded unsafe parameter name
in the query resolves (e.g. `?%2561utoDeserialize=true#%zz`, where
`%2561` decodes to `%61` then to `a`).
Fix: #13238
### Does this PR introduce _any_ user-facing change?
No API change. Legal JDBC URLs containing a literal `%` (e.g.
`password=100%`) or a once-encoded `%25` are now accepted instead of
rejected. The unsafe-parameter scan is not weakened.
### How was this patch tested?
Extended `TestJdbcUrlUtils`: the literal-percent acceptance cases fail
on the pre-fix tree and pass after the fix; a double-encoded unsafe
parameter hidden behind a poisoned fragment
(`?%2561utoDeserialize=true#%zz`, plus mysql/mariadb and no-fragment
variants) is rejected, and it was accepted before the every-pass
sanitization; and the existing unsafe-parameter cases (raw, single- and
double-encoded, `connectionProperties` smuggling, fragment poison) still
reject.
---
.../catalog/jdbc/utils/DataSourceUtils.java | 27 ++--
.../jdbc/utils/TestDataSourceUrlValidation.java | 17 +++
.../org/apache/gravitino/utils/JdbcUrlUtils.java | 100 ++++++++++++---
.../apache/gravitino/utils/TestJdbcUrlUtils.java | 139 +++++++++++++++++++--
4 files changed, 239 insertions(+), 44 deletions(-)
diff --git
a/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/utils/DataSourceUtils.java
b/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/utils/DataSourceUtils.java
index b585464a0f..7a7847ebf5 100644
---
a/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/utils/DataSourceUtils.java
+++
b/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/utils/DataSourceUtils.java
@@ -21,6 +21,7 @@ package org.apache.gravitino.catalog.jdbc.utils;
import java.sql.SQLException;
import java.time.Duration;
import java.util.List;
+import java.util.Locale;
import java.util.Map;
import java.util.Properties;
import javax.sql.DataSource;
@@ -76,11 +77,14 @@ public class DataSourceUtils {
// 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")) {
+ String lowerUrl = jdbcConfig.getJdbcUrl().toLowerCase(Locale.ROOT);
+ boolean isH2Url =
+ JdbcUrlUtils.decodedFormsForScan(lowerUrl).stream()
+ .anyMatch(form -> form.startsWith("jdbc:h2"));
+ if (isH2Url) {
throw new GravitinoRuntimeException("H2 JDBC URL is not allowed in
catalog configuration");
}
- if (jdbcConfig.getJdbcDriver().toLowerCase().startsWith("org.h2.")) {
+ if
(jdbcConfig.getJdbcDriver().toLowerCase(Locale.ROOT).startsWith("org.h2.")) {
throw new GravitinoRuntimeException("H2 JDBC driver is not allowed in
catalog configuration");
}
// Reject DBCP2 pool properties that load arbitrary classes via reflection
before handing the
@@ -205,23 +209,6 @@ public class DataSourceUtils {
}
}
- private static String recursiveDecode(String url) {
- String prev;
- String decoded = url;
- int max = 5;
-
- do {
- prev = decoded;
- try {
- decoded = java.net.URLDecoder.decode(prev, "UTF-8");
- } catch (Exception e) {
- throw new GravitinoRuntimeException("Unable to decode JDBC URL");
- }
- } while (!prev.equals(decoded) && --max > 0);
-
- return decoded;
- }
-
public static void closeDataSource(DataSource dataSource) {
if (null != dataSource) {
try {
diff --git
a/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/utils/TestDataSourceUrlValidation.java
b/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/utils/TestDataSourceUrlValidation.java
index 128c3c1ade..d11e87a179 100644
---
a/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/utils/TestDataSourceUrlValidation.java
+++
b/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/utils/TestDataSourceUrlValidation.java
@@ -178,6 +178,23 @@ public class TestDataSourceUrlValidation {
"H2 JDBC URL is not allowed in catalog configuration",
gre.getMessage());
}
+ @Test
+ public void testRejectH2UrlWithMalformedPercent() {
+ // A malformed percent escape in an H2 URL must not derail the
URL-decoding scan (it falls back
+ // instead of throwing), so the H2 guard still fires on the untouched
"jdbc:h2" prefix.
+ 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?password=100%");
+ 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 testRejectH2Driver() {
HashMap<String, String> properties = Maps.newHashMap();
diff --git a/common/src/main/java/org/apache/gravitino/utils/JdbcUrlUtils.java
b/common/src/main/java/org/apache/gravitino/utils/JdbcUrlUtils.java
index b451ed7507..fcc2b1046f 100644
--- a/common/src/main/java/org/apache/gravitino/utils/JdbcUrlUtils.java
+++ b/common/src/main/java/org/apache/gravitino/utils/JdbcUrlUtils.java
@@ -22,6 +22,7 @@ package org.apache.gravitino.utils;
import com.google.common.base.Preconditions;
import java.io.IOException;
import java.io.StringReader;
+import java.io.UnsupportedEncodingException;
import java.net.URLDecoder;
import java.util.Arrays;
import java.util.Collections;
@@ -86,23 +87,56 @@ public class JdbcUrlUtils {
Preconditions.checkArgument(StringUtils.isNotBlank(url), "JDBC URL can't
be blank");
String lowerUrl = url.toLowerCase(Locale.ROOT);
- String decodedUrl = recursiveDecode(lowerUrl);
-
- if (decodedUrl.startsWith("jdbc:mysql")) {
- checkUnsafeParameters(decodedUrl, all, UNSAFE_MYSQL_PARAMETERS, "MySQL");
- } else if (decodedUrl.startsWith("jdbc:mariadb")) {
- checkUnsafeParameters(decodedUrl, all, UNSAFE_MYSQL_PARAMETERS,
"MariaDB");
- } else if (decodedUrl.startsWith("jdbc:postgresql")) {
- checkUnsafeParameters(decodedUrl, all, UNSAFE_POSTGRES_PARAMETERS,
"PostgreSQL");
+ List<String> decodedForms = decodedFormsForScan(lowerUrl);
+
+ if (anyFormStartsWith(decodedForms, "jdbc:mysql")) {
+ checkUnsafeParameters(decodedForms, all, UNSAFE_MYSQL_PARAMETERS,
"MySQL");
+ } else if (anyFormStartsWith(decodedForms, "jdbc:mariadb")) {
+ checkUnsafeParameters(decodedForms, all, UNSAFE_MYSQL_PARAMETERS,
"MariaDB");
+ } else if (anyFormStartsWith(decodedForms, "jdbc:postgresql")) {
+ checkUnsafeParameters(decodedForms, all, UNSAFE_POSTGRES_PARAMETERS,
"PostgreSQL");
}
}
- private static void checkUnsafeParameters(
- String url, Map<String, String> config, List<String> unsafeParams,
String dbType) {
+ /**
+ * Returns the decoded forms of a JDBC URL that unsafe-parameter scans must
cover. Drivers such as
+ * MySQL Connector/J decode query tokens independently and ignore the URL
fragment, so a malformed
+ * percent escape in one part of the URL must not stop the scan from
revealing parameter names
+ * hidden behind valid encodings in another part. The returned forms are the
URL decoded until the
+ * first undecodable escape, plus the fully decoded form of the URL with
malformed escapes treated
+ * as literal '{@code %}' characters.
+ *
+ * @param url the JDBC URL, already lower-cased by the caller.
+ * @return the candidate decoded forms, never empty.
+ */
+ public static List<String> decodedFormsForScan(String url) {
+ // Percent-decoding can reintroduce upper-case characters (e.g. "%4a" ->
'J'), so the
+ // returned forms are lower-cased for substring and prefix matching.
+ String stoppedAtMalformed = recursiveDecode(url).toLowerCase(Locale.ROOT);
+ String fullyDecoded =
recursiveDecodeSanitizingEachPass(url).toLowerCase(Locale.ROOT);
+ if (fullyDecoded.equals(stoppedAtMalformed)) {
+ return Collections.singletonList(stoppedAtMalformed);
+ }
+ return Arrays.asList(stoppedAtMalformed, fullyDecoded);
+ }
- // Percent-decoding in recursiveDecode can reintroduce upper-case
characters (e.g. "%4a" ->
- // 'J'), so lower-case again here rather than relying on the pre-decode
lower-casing.
- String lowerUrl = url.toLowerCase(Locale.ROOT);
+ /**
+ * Replaces every '{@code %}' that is not followed by two hex digits with
the escape for a literal
+ * '{@code %}', making the URL decodable by {@link URLDecoder}.
+ */
+ private static String sanitizeMalformedPercentEscapes(String url) {
+ return url.replaceAll("%(?![0-9a-fA-F]{2})", "%25");
+ }
+
+ private static boolean anyFormStartsWith(List<String> forms, String prefix) {
+ return forms.stream().anyMatch(form -> form.startsWith(prefix));
+ }
+
+ private static void checkUnsafeParameters(
+ List<String> decodedForms,
+ Map<String, String> config,
+ List<String> unsafeParams,
+ String dbType) {
// Parameter names that reach the JDBC driver through the config map: the
config keys
// themselves (defense in depth) plus any names embedded in the DBCP2
"connectionProperties"
@@ -111,7 +145,8 @@ public class JdbcUrlUtils {
for (String param : unsafeParams) {
String lowerParam = param.toLowerCase(Locale.ROOT);
- if (lowerUrl.contains(lowerParam) ||
configParamNames.contains(lowerParam)) {
+ boolean inUrl = decodedForms.stream().anyMatch(form ->
form.contains(lowerParam));
+ if (inUrl || configParamNames.contains(lowerParam)) {
throw new GravitinoRuntimeException(
"Unsafe %s parameter '%s' detected in JDBC configuration", dbType,
param);
}
@@ -127,8 +162,41 @@ public class JdbcUrlUtils {
prev = decoded;
try {
decoded = URLDecoder.decode(prev, "UTF-8");
- } catch (Exception e) {
- throw new GravitinoRuntimeException("Unable to decode JDBC URL");
+ } catch (UnsupportedEncodingException e) {
+ // UTF-8 is guaranteed to be supported by the JVM specification.
+ throw new RuntimeException(e);
+ } catch (IllegalArgumentException e) {
+ // The URL contains a literal '%' that is not part of a valid escape
(e.g. a password
+ // like "100%"). JDBC URLs are not required to be validly
percent-encoded, so keep the
+ // last fully decoded form instead of rejecting the URL: any parameter
name hidden
+ // behind valid encodings was already revealed by the previous passes,
and a malformed
+ // escape cannot additionally hide a readable name from this check.
+ return prev;
+ }
+ } while (!prev.equals(decoded) && --max > 0);
+
+ return decoded;
+ }
+
+ /**
+ * Fully decodes {@code url}, neutralizing malformed percent escapes before
EVERY decode pass.
+ * Sanitizing only once is not enough: decoding "{@code %25zz}" yields
"{@code %zz}" again, so a
+ * later pass would halt at the regenerated malformed escape and leave a
doubly-encoded parameter
+ * name elsewhere (e.g. "{@code %2561utoDeserialize}", which needs two
passes to reveal 'a') only
+ * partially decoded. Because each pass is sanitized first, {@link
URLDecoder} never throws here.
+ */
+ private static String recursiveDecodeSanitizingEachPass(String url) {
+ String prev;
+ String decoded = url;
+ int max = 5;
+
+ do {
+ prev = decoded;
+ try {
+ decoded = URLDecoder.decode(sanitizeMalformedPercentEscapes(prev),
"UTF-8");
+ } catch (UnsupportedEncodingException e) {
+ // UTF-8 is guaranteed to be supported by the JVM specification.
+ throw new RuntimeException(e);
}
} while (!prev.equals(decoded) && --max > 0);
diff --git
a/common/src/test/java/org/apache/gravitino/utils/TestJdbcUrlUtils.java
b/common/src/test/java/org/apache/gravitino/utils/TestJdbcUrlUtils.java
index 056178ca72..93557de8c6 100644
--- a/common/src/test/java/org/apache/gravitino/utils/TestJdbcUrlUtils.java
+++ b/common/src/test/java/org/apache/gravitino/utils/TestJdbcUrlUtils.java
@@ -20,6 +20,7 @@
package org.apache.gravitino.utils;
import java.util.Collections;
+import java.util.List;
import java.util.Locale;
import org.apache.gravitino.exceptions.GravitinoRuntimeException;
import org.junit.jupiter.api.Assertions;
@@ -31,14 +32,15 @@ import org.junit.jupiter.api.parallel.Resources;
public class TestJdbcUrlUtils {
@Test
- public void whenMalformedUrlGiven_ShouldThrowGravitinoRuntimeException() {
- GravitinoRuntimeException gre =
- Assertions.assertThrows(
- GravitinoRuntimeException.class,
- () ->
- JdbcUrlUtils.validateJdbcConfig(
- "testDriver", "malformed%ZZurl",
Collections.singletonMap("test", "test")));
- Assertions.assertEquals("Unable to decode JDBC URL", gre.getMessage());
+ public void whenMalformedUrlGiven_ShouldFallBackToLastDecodedForm() {
+ // A percent escape that URLDecoder cannot decode (e.g. a literal '%' in a
password) no
+ // longer rejects the whole URL: JDBC URLs are not required to be
percent-encoded. The
+ // validation still scans the last successfully decoded form, so unsafe
parameters remain
+ // detectable (see
unsafeParameterBehindEncodingIsStillDetectedWithLiteralPercent).
+ Assertions.assertDoesNotThrow(
+ () ->
+ JdbcUrlUtils.validateJdbcConfig(
+ "testDriver", "malformed%ZZurl",
Collections.singletonMap("test", "test")));
}
@Test
@@ -62,6 +64,127 @@ public class TestJdbcUrlUtils {
Collections.singletonMap("test", "test")));
}
+ @Test
+ public void testValidateJdbcConfigWithLiteralPercentInUrl() {
+ // A literal '%' (e.g. a password like "100%") is legal in a JDBC URL;
before the fix the
+ // decoder threw "Unable to decode JDBC URL" for it.
+ Assertions.assertDoesNotThrow(
+ () ->
+ JdbcUrlUtils.validateJdbcConfig(
+ "testDriver",
+ "jdbc:mysql://localhost:3306/test?password=100%",
+ Collections.emptyMap()));
+
+ // A once-encoded '%25' decodes to a literal '%', which must not be
rejected either.
+ Assertions.assertDoesNotThrow(
+ () ->
+ JdbcUrlUtils.validateJdbcConfig(
+ "testDriver",
+ "jdbc:postgresql://localhost:5432/test?password=pa%25ss",
+ Collections.emptyMap()));
+ }
+
+ @Test
+ public void unsafeParameterBehindEncodingWithFragmentPoisonIsStillDetected()
{
+ // MySQL Connector/J decodes query tokens independently and ignores the
URL fragment, so a
+ // malformed escape in the fragment must not stop the scan from revealing
an encoded unsafe
+ // parameter name in the query.
+ Assertions.assertThrows(
+ GravitinoRuntimeException.class,
+ () ->
+ JdbcUrlUtils.validateJdbcConfig(
+ "testDriver",
+ "jdbc:mysql://localhost:3306/test?%61utoDeserialize=true#%zz",
+ Collections.emptyMap()));
+ Assertions.assertThrows(
+ GravitinoRuntimeException.class,
+ () ->
+ JdbcUrlUtils.validateJdbcConfig(
+ "testDriver",
+
"jdbc:mysql://localhost:3306/test?%71ueryInterceptors=x#frag%25",
+ Collections.emptyMap()));
+ }
+
+ @Test
+ public void doubleEncodedUnsafeParamWithPoisonedFragmentIsRejected() {
+ // Copilot's alleged bypass on PR #13239: a double-encoded parameter name
combined with a
+ // malformed '#%zz' fragment. The discriminating case hides the FIRST
letter of the name behind
+ // the double encoding (%2561 -> %61 -> 'a'), so no scanned form contains
the literal name until
+ // the query token is fully decoded. Because a single pre-pass
sanitization turns '%zz' into
+ // '%25zz' but the first decode pass regenerates '%zz', the second pass
would halt at the
+ // malformed fragment before '%61' becomes 'a' -- unless malformed escapes
are sanitized on
+ // every pass. MySQL Connector/J decodes query tokens independently and
ignores the fragment.
+ assertUnsafeRejected("jdbc:mysql://h/db?%2561utoDeserialize=true#%zz");
+ assertUnsafeRejected("jdbc:mariadb://h/db?%2561utoDeserialize=true#%zz");
+ // Without the poisoned fragment, recursive decoding already reveals the
hidden name.
+ assertUnsafeRejected("jdbc:mysql://h/db?%2561utoDeserialize=true");
+
+ // The exact strings from the report keep the literal 'autoDeserialize'
after the double
+ // encoding, so they are rejected on the cleartext substring alone (a
weaker path than the one
+ // under test); kept as a regression anchor for the reported input.
+ assertUnsafeRejected("jdbc:mysql://h/db?%2561autoDeserialize=true#%zz");
+ assertUnsafeRejected("jdbc:mysql://h/db?%2561autoDeserialize=true");
+ }
+
+ private static void assertUnsafeRejected(String url) {
+ Assertions.assertThrows(
+ GravitinoRuntimeException.class,
+ () -> JdbcUrlUtils.validateJdbcConfig("testDriver", url,
Collections.emptyMap()),
+ () -> "Expected unsafe URL to be rejected but it was accepted: " +
url);
+ }
+
+ @Test
+ public void unsafeParamBehindUpperCaseHexEscapeMidDecodeIsRejected() {
+ // Regression for a case-sensitivity gap in the malformed-escape
sanitizer. A decode pass can
+ // regenerate a percent escape whose hex digits include an upper-case
letter: "%25%36%46"
+ // decodes to "%6F", the escape for 'o'. The caller only lower-cases the
ORIGINAL URL, so this
+ // "%6F" appears mid-decode. A sanitizer that recognizes only lower-case
hex treats "%6F" as
+ // malformed and rewrites it to "%256F", so it never decodes to 'o' and
the hidden
+ // 'autoDeserialize' is missed. The '#%zz' fragment poisons the
non-sanitizing decode path
+ // (MySQL Connector/J decodes query tokens independently and ignores the
fragment), forcing
+ // detection through the sanitizing path under test.
+ assertUnsafeRejected("jdbc:mysql://h/db?aut%25%36%46deserialize=true#%zz");
+
assertUnsafeRejected("jdbc:mariadb://h/db?aut%25%36%46deserialize=true#%zz");
+ }
+
+ @Test
+ public void validUpperCaseHexEscapeIsNotMangledBySanitizer() {
+ // A valid percent escape whose hex digits include an upper-case letter
("%2F", identical to
+ // "%2f") must decode the same as its lower-case form and must not be
mangled into a literal
+ // '%'. Such an escape reaches the sanitizer only mid-decode, past the
caller's initial
+ // lower-casing, so exercise the sanitizing decode path directly:
"%25%32%46" -> "%2F" -> '/',
+ // and "%25%32%66" -> "%2f" -> '/'. The '#%zz' fragment keeps the
non-sanitizing path from
+ // decoding, so the fully decoded form is produced by the path under test.
+ List<String> upperHexForms =
+
JdbcUrlUtils.decodedFormsForScan("jdbc:mysql://h/db?p=a%25%32%46b#%zz");
+ List<String> lowerHexForms =
+
JdbcUrlUtils.decodedFormsForScan("jdbc:mysql://h/db?p=a%25%32%66b#%zz");
+
+ String upperDecoded = upperHexForms.get(upperHexForms.size() - 1);
+ String lowerDecoded = lowerHexForms.get(lowerHexForms.size() - 1);
+ Assertions.assertEquals(
+ lowerDecoded,
+ upperDecoded,
+ "Upper-case hex escape must decode identically to its lower-case
form");
+ Assertions.assertTrue(
+ upperDecoded.contains("a/b"),
+ () -> "Expected '%2F' to decode to '/', but the sanitizer mangled it:
" + upperDecoded);
+ }
+
+ @Test
+ public void unsafeParameterBehindEncodingIsStillDetectedWithLiteralPercent()
{
+ // The decoded fallback must not weaken detection: an unsafe parameter
that remains readable
+ // after the last successful decode is still rejected even when the URL
also carries a
+ // literal '%'.
+ Assertions.assertThrows(
+ GravitinoRuntimeException.class,
+ () ->
+ JdbcUrlUtils.validateJdbcConfig(
+ "testDriver",
+
"jdbc:mysql://localhost:3306/test?password=100%&autoDeserialize=true",
+ Collections.emptyMap()));
+ }
+
@Test
public void
whenUnsafeParameterGivenForMySQL_ShouldThrowGravitinoRuntimeException() {
