github-actions[bot] commented on code in PR #65987:
URL: https://github.com/apache/doris/pull/65987#discussion_r3649345799
##########
fe/fe-connector/fe-connector-jdbc/src/main/java/org/apache/doris/connector/jdbc/JdbcConnectorProvider.java:
##########
@@ -58,6 +58,11 @@ public void validateProperties(Map<String, String>
properties) {
}
}
+ // 1b. Mandatory, non-configurable driver_url security rule.
checkProperties() runs this on
+ // both CREATE and ALTER CATALOG (both !isReplay), so a malicious
driver_url cannot be
+ // introduced by either; metadata replay of existing catalogs is never
affected.
+ JdbcDorisConnector.checkDriverUrlSecurityRule(resolve(properties,
JdbcConnectorProperties.DRIVER_URL));
Review Comment:
[P1] Do not revalidate unchanged legacy URLs on unrelated ALTERs
`checkProperties()` receives the entire catalog map after
`CatalogMgr.replayAlterCatalogProps()` has merged the requested delta, so this
unconditional call checks the old `driver_url` even when the ALTER did not
touch it. For example, a catalog created before upgrade with the previously
valid bare name `legacy+patched.jar` still replays and can load that file, but
`ALTER CATALOG ... SET PROPERTIES ("password"="rotated")` now fails because `+`
is outside the new grammar. That makes routine administration of an otherwise
usable existing catalog impossible. Please make this new rule delta-aware for
ALTER (while retaining full CREATE validation), and add a
replayed-legacy-catalog test; actual driver URL changes still need the
allowlist/checksum validation from the existing thread.
##########
fe/fe-common/src/main/java/org/apache/doris/common/Config.java:
##########
@@ -3256,9 +3256,11 @@ public static int metaServiceRpcRetryTimes() {
@ConfField(mutable = true)
public static int mow_calculate_delete_bitmap_retry_times = 10;
- @ConfField(mutable = true, description = {
+ @ConfField(description = {
Review Comment:
[P1] Keep all-FE config updates safe during rolling upgrades
During a rolling upgrade, an old FE still treats this key (and
`jdbc_driver_url_white_list` / `force_sqlserver_jdbc_encrypt_false`) as
mutable. `Env.setConfig()` applies the receiving FE first and only then
forwards `ADMIN SET ALL FRONTENDS CONFIG` to peers. If an old FE receives the
statement, it changes its local value, an upgraded FE rejects the forwarded
statement as `is not mutable`, and the command returns failure without rolling
the old FE back. The cluster is then enforcing different S3/JDBC allowlists or
SQL Server encryption behavior despite the failed command. Please preserve a
rolling-compatible transition (or preflight/rollback the all-FE update) and
cover the mixed-version path.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/JdbcResource.java:
##########
@@ -297,35 +301,142 @@ private static void checkCloudWhiteList(String
driverUrl) throws IllegalArgument
}
}
+ // A scheme-less driver_url must be a plain jar file name: letters,
digits, dot, underscore, hyphen.
+ // No path separator (encoded or not) and no other characters, so it can
never escape jdbc_drivers_dir.
+ private static final Pattern SAFE_BARE_JAR_NAME =
Pattern.compile("^[A-Za-z0-9._-]+\\.jar$");
+
public static String getFullDriverUrl(String driverUrl) throws
IllegalArgumentException {
if (!(driverUrl.startsWith("file://") ||
driverUrl.startsWith("http://";)
|| driverUrl.startsWith("https://";) ||
driverUrl.matches("^[^:/]+\\.jar$"))) {
throw new IllegalArgumentException("Invalid driver URL format.
Supported formats are: "
+ "file://xxx.jar, http://xxx.jar, https://xxx.jar, or
xxx.jar (without prefix).");
}
+ URI uri;
try {
- URI uri = new URI(driverUrl);
- String schema = uri.getScheme();
- checkCloudWhiteList(driverUrl);
- if (schema == null && !driverUrl.startsWith("/")) {
- return checkAndReturnDefaultDriverUrl(driverUrl);
+ uri = new URI(driverUrl);
+ } catch (URISyntaxException e) {
+ // Fail closed: an unparsable URL must never be silently accepted,
otherwise the
+ // allowed-path check below could be bypassed by a malformed URL.
+ LOG.warn("invalid jdbc driver url: {}", driverUrl, e);
+ throw new IllegalArgumentException("Invalid driver URL: " +
driverUrl);
+ }
+
+ String schema = uri.getScheme();
+ checkCloudWhiteList(driverUrl);
+ if (schema == null && !driverUrl.startsWith("/")) {
+ // Enforce the safe bare-name grammar in this shared resolver,
before resolving under
+ // jdbc_drivers_dir. Otherwise an encoded separator (e.g.
"%2e%2e%2Fevil.jar") passes the
+ // loose format check above and, once resolved and decoded by the
loader, escapes the
+ // directory. The connector rule catches "%" too, but
Iceberg/Paimon/legacy JDBC consumers
+ // call getFullDriverUrl directly and bypass that rule, so it must
be enforced here.
+ if (!SAFE_BARE_JAR_NAME.matcher(driverUrl).matches()) {
+ throw new IllegalArgumentException(
+ "Invalid driver_url: a driver file name must match
[A-Za-z0-9._-]+.jar: " + driverUrl);
}
+ return checkAndReturnDefaultDriverUrl(driverUrl);
+ }
+
+ // "*" or an empty/blank value means allow all (the documented,
backward-compatible contract).
+ String securePath = Config.jdbc_driver_secure_path;
+ if (securePath == null || securePath.trim().isEmpty() ||
"*".equals(securePath.trim())) {
+ return driverUrl;
+ }
- if ("*".equals(Config.jdbc_driver_secure_path)) {
- return driverUrl;
+ if (!isDriverUrlAllowed(driverUrl, uri)) {
+ throw new IllegalArgumentException("Driver URL does not match any
allowed paths: " + driverUrl);
+ }
+ return driverUrl;
+ }
+
+ /**
+ * Check whether {@code driverUrl} falls under one of the
semicolon-separated prefixes configured in
+ * {@link Config#jdbc_driver_secure_path}. Matching is structural
(component-based) rather than a raw string
+ * prefix, so that neither prefix confusion ({@code /opt/drivers} vs
{@code /opt/drivers-evil}) nor path
+ * traversal ({@code /opt/drivers/../etc}) can slip a driver outside the
allowed location.
+ */
+ private static boolean isDriverUrlAllowed(String driverUrl, URI uri) {
+ String scheme = uri.getScheme();
+ List<String> allowedPaths = new ArrayList<>();
+ for (String p : Config.jdbc_driver_secure_path.split(";")) {
+ String trimmed = p.trim();
+ if (!trimmed.isEmpty()) {
+ allowedPaths.add(trimmed);
}
+ }
+ if ("http".equalsIgnoreCase(scheme) ||
"https".equalsIgnoreCase(scheme)) {
+ URI candidate = uri.normalize();
+ return allowedPaths.stream().anyMatch(allowed ->
remoteUrlMatches(candidate, allowed));
+ }
+ // Only file:// reaches here; bare absolute paths and bare "*.jar" are
handled earlier.
+ // A local file URL must carry no authority, query or fragment.
Otherwise validation (which
+ // looks only at URI.getPath()) and the consumers (URLClassLoader /
checksum, which act on the
+ // whole original URL) would address different objects — e.g.
"file://attacker/dir/x.jar" is
+ // fetched from a remote authority, and "file:///dir/x.jar?evil" maps
to a sibling file.
+ String authority = uri.getRawAuthority();
+ if ((authority != null && !authority.isEmpty())
Review Comment:
[P1] Preserve local file://localhost URLs after upgrade
This guard rejects every non-empty file authority, but
`file://localhost/...` is a standard spelling for a local file: JDK 17 opens it
from the local filesystem. Before this hunk, matching values such as
`jdbc_driver_secure_path=file://localhost/opt/doris/jdbc_drivers` and
`driver_url=file://localhost/opt/doris/jdbc_drivers/x.jar` passed the
raw-prefix check. With a concrete secure path, the upgraded resolver now
rejects that same persisted URL during CREATE or lazy Iceberg/Paimon/legacy
JDBC initialization. Please canonicalize an absent authority and an exact
`localhost` authority to the same local form while continuing to reject all
non-local authorities, and add a replay compatibility test.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/JdbcResource.java:
##########
@@ -297,35 +301,142 @@ private static void checkCloudWhiteList(String
driverUrl) throws IllegalArgument
}
}
+ // A scheme-less driver_url must be a plain jar file name: letters,
digits, dot, underscore, hyphen.
+ // No path separator (encoded or not) and no other characters, so it can
never escape jdbc_drivers_dir.
+ private static final Pattern SAFE_BARE_JAR_NAME =
Pattern.compile("^[A-Za-z0-9._-]+\\.jar$");
+
public static String getFullDriverUrl(String driverUrl) throws
IllegalArgumentException {
if (!(driverUrl.startsWith("file://") ||
driverUrl.startsWith("http://";)
|| driverUrl.startsWith("https://";) ||
driverUrl.matches("^[^:/]+\\.jar$"))) {
throw new IllegalArgumentException("Invalid driver URL format.
Supported formats are: "
+ "file://xxx.jar, http://xxx.jar, https://xxx.jar, or
xxx.jar (without prefix).");
}
+ URI uri;
try {
- URI uri = new URI(driverUrl);
- String schema = uri.getScheme();
- checkCloudWhiteList(driverUrl);
- if (schema == null && !driverUrl.startsWith("/")) {
- return checkAndReturnDefaultDriverUrl(driverUrl);
+ uri = new URI(driverUrl);
+ } catch (URISyntaxException e) {
+ // Fail closed: an unparsable URL must never be silently accepted,
otherwise the
+ // allowed-path check below could be bypassed by a malformed URL.
+ LOG.warn("invalid jdbc driver url: {}", driverUrl, e);
+ throw new IllegalArgumentException("Invalid driver URL: " +
driverUrl);
+ }
+
+ String schema = uri.getScheme();
+ checkCloudWhiteList(driverUrl);
+ if (schema == null && !driverUrl.startsWith("/")) {
+ // Enforce the safe bare-name grammar in this shared resolver,
before resolving under
+ // jdbc_drivers_dir. Otherwise an encoded separator (e.g.
"%2e%2e%2Fevil.jar") passes the
+ // loose format check above and, once resolved and decoded by the
loader, escapes the
+ // directory. The connector rule catches "%" too, but
Iceberg/Paimon/legacy JDBC consumers
+ // call getFullDriverUrl directly and bypass that rule, so it must
be enforced here.
+ if (!SAFE_BARE_JAR_NAME.matcher(driverUrl).matches()) {
+ throw new IllegalArgumentException(
+ "Invalid driver_url: a driver file name must match
[A-Za-z0-9._-]+.jar: " + driverUrl);
}
+ return checkAndReturnDefaultDriverUrl(driverUrl);
+ }
+
+ // "*" or an empty/blank value means allow all (the documented,
backward-compatible contract).
+ String securePath = Config.jdbc_driver_secure_path;
+ if (securePath == null || securePath.trim().isEmpty() ||
"*".equals(securePath.trim())) {
+ return driverUrl;
+ }
- if ("*".equals(Config.jdbc_driver_secure_path)) {
- return driverUrl;
+ if (!isDriverUrlAllowed(driverUrl, uri)) {
+ throw new IllegalArgumentException("Driver URL does not match any
allowed paths: " + driverUrl);
+ }
+ return driverUrl;
+ }
+
+ /**
+ * Check whether {@code driverUrl} falls under one of the
semicolon-separated prefixes configured in
+ * {@link Config#jdbc_driver_secure_path}. Matching is structural
(component-based) rather than a raw string
+ * prefix, so that neither prefix confusion ({@code /opt/drivers} vs
{@code /opt/drivers-evil}) nor path
+ * traversal ({@code /opt/drivers/../etc}) can slip a driver outside the
allowed location.
+ */
+ private static boolean isDriverUrlAllowed(String driverUrl, URI uri) {
+ String scheme = uri.getScheme();
+ List<String> allowedPaths = new ArrayList<>();
+ for (String p : Config.jdbc_driver_secure_path.split(";")) {
+ String trimmed = p.trim();
+ if (!trimmed.isEmpty()) {
+ allowedPaths.add(trimmed);
}
+ }
+ if ("http".equalsIgnoreCase(scheme) ||
"https".equalsIgnoreCase(scheme)) {
+ URI candidate = uri.normalize();
+ return allowedPaths.stream().anyMatch(allowed ->
remoteUrlMatches(candidate, allowed));
+ }
+ // Only file:// reaches here; bare absolute paths and bare "*.jar" are
handled earlier.
+ // A local file URL must carry no authority, query or fragment.
Otherwise validation (which
+ // looks only at URI.getPath()) and the consumers (URLClassLoader /
checksum, which act on the
+ // whole original URL) would address different objects — e.g.
"file://attacker/dir/x.jar" is
+ // fetched from a remote authority, and "file:///dir/x.jar?evil" maps
to a sibling file.
+ String authority = uri.getRawAuthority();
+ if ((authority != null && !authority.isEmpty())
+ || uri.getRawQuery() != null || uri.getRawFragment() != null) {
+ return false;
+ }
+ Path candidate = toLocalPath(driverUrl).normalize();
+ return allowedPaths.stream()
+ .map(allowed -> toLocalPath(allowed).normalize())
+ .anyMatch(candidate::startsWith);
+ }
- boolean isAllowed =
Arrays.stream(Config.jdbc_driver_secure_path.split(";"))
- .anyMatch(allowedPath ->
driverUrl.startsWith(allowedPath.trim()));
- if (!isAllowed) {
- throw new IllegalArgumentException("Driver URL does not match
any allowed paths: " + driverUrl);
+ /**
+ * Turn a {@code file://} URL or a plain filesystem path into a {@link
Path} for structural comparison.
+ * A {@code file://} URL is decoded exactly once via {@link URI#getPath()}
so that percent-encoded
+ * segments (e.g. {@code %2e%2e}) are resolved into the same
representation the driver-loading
+ * consumers ({@code URL.openStream} / {@code URLClassLoader}) will use;
otherwise an encoded parent
+ * segment would survive normalization and escape the allowed directory.
+ */
+ private static Path toLocalPath(String pathOrUrl) {
+ if (pathOrUrl.startsWith("file:")) {
+ try {
+ String decoded = new URI(pathOrUrl).getPath();
+ if (decoded != null && !decoded.isEmpty()) {
+ return Paths.get(decoded);
+ }
+ } catch (URISyntaxException ignored) {
+ // fall through to literal stripping below
}
- return driverUrl;
+ int sep = pathOrUrl.indexOf("//");
+ return Paths.get(sep >= 0 ? pathOrUrl.substring(sep + 2) :
pathOrUrl.substring("file:".length()));
+ }
+ return Paths.get(pathOrUrl);
+ }
+
+ /**
+ * Structural match for remote (http/https) driver URLs: scheme, host and
port must be equal, and the
+ * candidate path must sit under the allowed path (component-based). A
bare path prefix (no scheme) can
+ * never authorize a remote URL.
+ */
+ private static boolean remoteUrlMatches(URI candidate, String allowedPath)
{
+ URI base;
+ try {
+ base = new URI(allowedPath).normalize();
} catch (URISyntaxException e) {
- LOG.warn("invalid jdbc driver url: " + driverUrl);
- return driverUrl;
+ return false;
}
+ if (base.getScheme() == null) {
+ return false;
+ }
+ // Scheme/host/port and the path prefix must match, and the
resource-selecting components
+ // (user-info and query) that the checksum/classloader consumers act
on must match exactly too,
+ // otherwise e.g. ".../download?id=approved" would authorize
".../download?id=evil".
+ return base.getScheme().equalsIgnoreCase(candidate.getScheme())
+ && base.getHost() != null &&
base.getHost().equalsIgnoreCase(candidate.getHost())
+ && base.getPort() == candidate.getPort()
+ && Objects.equals(base.getUserInfo(), candidate.getUserInfo())
+ && Objects.equals(base.getRawQuery(), candidate.getRawQuery())
+ && pathIsUnder(candidate.getPath(), base.getPath());
+ }
+
+ private static boolean pathIsUnder(String candidatePath, String basePath) {
+ Path candidate = Paths.get(candidatePath == null ||
candidatePath.isEmpty() ? "/" : candidatePath).normalize();
Review Comment:
[P1] Match remote paths in the representation actually requested
This comparison uses decoded `URI.getPath()` values. With
`jdbc_driver_secure_path=http://host/drivers/`, the candidate
`http://host/drivers%2Fevil.jar` is decoded to `/drivers/evil.jar` and passes
`pathIsUnder()`, but `getFullDriverUrl()` returns the original URL and JDK 17
sends the raw request path `/drivers%2Fevil.jar`. An origin can route that raw
path as a resource distinct from `/drivers/evil.jar`, so validation can
authorize a different object from the one checksum/classloader consumers fetch.
Please compare the raw request representation, reject encoded path delimiters,
or return the exact canonical URL that was authorized, with a server-backed
negative test.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/JdbcResource.java:
##########
@@ -297,35 +301,142 @@ private static void checkCloudWhiteList(String
driverUrl) throws IllegalArgument
}
}
+ // A scheme-less driver_url must be a plain jar file name: letters,
digits, dot, underscore, hyphen.
+ // No path separator (encoded or not) and no other characters, so it can
never escape jdbc_drivers_dir.
+ private static final Pattern SAFE_BARE_JAR_NAME =
Pattern.compile("^[A-Za-z0-9._-]+\\.jar$");
+
public static String getFullDriverUrl(String driverUrl) throws
IllegalArgumentException {
if (!(driverUrl.startsWith("file://") ||
driverUrl.startsWith("http://";)
|| driverUrl.startsWith("https://";) ||
driverUrl.matches("^[^:/]+\\.jar$"))) {
throw new IllegalArgumentException("Invalid driver URL format.
Supported formats are: "
+ "file://xxx.jar, http://xxx.jar, https://xxx.jar, or
xxx.jar (without prefix).");
}
+ URI uri;
try {
- URI uri = new URI(driverUrl);
- String schema = uri.getScheme();
- checkCloudWhiteList(driverUrl);
- if (schema == null && !driverUrl.startsWith("/")) {
- return checkAndReturnDefaultDriverUrl(driverUrl);
+ uri = new URI(driverUrl);
+ } catch (URISyntaxException e) {
+ // Fail closed: an unparsable URL must never be silently accepted,
otherwise the
+ // allowed-path check below could be bypassed by a malformed URL.
+ LOG.warn("invalid jdbc driver url: {}", driverUrl, e);
+ throw new IllegalArgumentException("Invalid driver URL: " +
driverUrl);
+ }
+
+ String schema = uri.getScheme();
+ checkCloudWhiteList(driverUrl);
+ if (schema == null && !driverUrl.startsWith("/")) {
+ // Enforce the safe bare-name grammar in this shared resolver,
before resolving under
+ // jdbc_drivers_dir. Otherwise an encoded separator (e.g.
"%2e%2e%2Fevil.jar") passes the
+ // loose format check above and, once resolved and decoded by the
loader, escapes the
+ // directory. The connector rule catches "%" too, but
Iceberg/Paimon/legacy JDBC consumers
+ // call getFullDriverUrl directly and bypass that rule, so it must
be enforced here.
+ if (!SAFE_BARE_JAR_NAME.matcher(driverUrl).matches()) {
Review Comment:
[P1] Preserve valid legacy bare filenames after replay
Before this hunk, a bare filename only had to match `^[^:/]+\.jar$`, so
names such as `legacy+patched.jar` were valid and resolved under
`jdbc_drivers_dir`. The new shared ASCII-only check rejects them. Iceberg,
Paimon, and legacy JDBC consumers call `getFullDriverUrl()` lazily after
metadata replay, so an existing catalog using such a harmless filename can fail
on first access after upgrade even though no CREATE or ALTER is being
validated. Please preserve the old non-separator filename compatibility while
rejecting raw/decoded separators and parent segments, or provide an explicit
migration path, and cover a replayed direct consumer.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/JdbcResource.java:
##########
@@ -297,35 +301,142 @@ private static void checkCloudWhiteList(String
driverUrl) throws IllegalArgument
}
}
+ // A scheme-less driver_url must be a plain jar file name: letters,
digits, dot, underscore, hyphen.
+ // No path separator (encoded or not) and no other characters, so it can
never escape jdbc_drivers_dir.
+ private static final Pattern SAFE_BARE_JAR_NAME =
Pattern.compile("^[A-Za-z0-9._-]+\\.jar$");
+
public static String getFullDriverUrl(String driverUrl) throws
IllegalArgumentException {
if (!(driverUrl.startsWith("file://") ||
driverUrl.startsWith("http://";)
|| driverUrl.startsWith("https://";) ||
driverUrl.matches("^[^:/]+\\.jar$"))) {
throw new IllegalArgumentException("Invalid driver URL format.
Supported formats are: "
+ "file://xxx.jar, http://xxx.jar, https://xxx.jar, or
xxx.jar (without prefix).");
}
+ URI uri;
try {
- URI uri = new URI(driverUrl);
- String schema = uri.getScheme();
- checkCloudWhiteList(driverUrl);
- if (schema == null && !driverUrl.startsWith("/")) {
- return checkAndReturnDefaultDriverUrl(driverUrl);
+ uri = new URI(driverUrl);
+ } catch (URISyntaxException e) {
+ // Fail closed: an unparsable URL must never be silently accepted,
otherwise the
+ // allowed-path check below could be bypassed by a malformed URL.
+ LOG.warn("invalid jdbc driver url: {}", driverUrl, e);
+ throw new IllegalArgumentException("Invalid driver URL: " +
driverUrl);
+ }
+
+ String schema = uri.getScheme();
+ checkCloudWhiteList(driverUrl);
+ if (schema == null && !driverUrl.startsWith("/")) {
+ // Enforce the safe bare-name grammar in this shared resolver,
before resolving under
+ // jdbc_drivers_dir. Otherwise an encoded separator (e.g.
"%2e%2e%2Fevil.jar") passes the
+ // loose format check above and, once resolved and decoded by the
loader, escapes the
+ // directory. The connector rule catches "%" too, but
Iceberg/Paimon/legacy JDBC consumers
+ // call getFullDriverUrl directly and bypass that rule, so it must
be enforced here.
+ if (!SAFE_BARE_JAR_NAME.matcher(driverUrl).matches()) {
+ throw new IllegalArgumentException(
+ "Invalid driver_url: a driver file name must match
[A-Za-z0-9._-]+.jar: " + driverUrl);
}
+ return checkAndReturnDefaultDriverUrl(driverUrl);
+ }
+
+ // "*" or an empty/blank value means allow all (the documented,
backward-compatible contract).
+ String securePath = Config.jdbc_driver_secure_path;
+ if (securePath == null || securePath.trim().isEmpty() ||
"*".equals(securePath.trim())) {
+ return driverUrl;
+ }
- if ("*".equals(Config.jdbc_driver_secure_path)) {
- return driverUrl;
+ if (!isDriverUrlAllowed(driverUrl, uri)) {
+ throw new IllegalArgumentException("Driver URL does not match any
allowed paths: " + driverUrl);
+ }
+ return driverUrl;
+ }
+
+ /**
+ * Check whether {@code driverUrl} falls under one of the
semicolon-separated prefixes configured in
+ * {@link Config#jdbc_driver_secure_path}. Matching is structural
(component-based) rather than a raw string
+ * prefix, so that neither prefix confusion ({@code /opt/drivers} vs
{@code /opt/drivers-evil}) nor path
+ * traversal ({@code /opt/drivers/../etc}) can slip a driver outside the
allowed location.
+ */
+ private static boolean isDriverUrlAllowed(String driverUrl, URI uri) {
+ String scheme = uri.getScheme();
+ List<String> allowedPaths = new ArrayList<>();
+ for (String p : Config.jdbc_driver_secure_path.split(";")) {
+ String trimmed = p.trim();
+ if (!trimmed.isEmpty()) {
+ allowedPaths.add(trimmed);
}
+ }
+ if ("http".equalsIgnoreCase(scheme) ||
"https".equalsIgnoreCase(scheme)) {
+ URI candidate = uri.normalize();
+ return allowedPaths.stream().anyMatch(allowed ->
remoteUrlMatches(candidate, allowed));
+ }
+ // Only file:// reaches here; bare absolute paths and bare "*.jar" are
handled earlier.
+ // A local file URL must carry no authority, query or fragment.
Otherwise validation (which
+ // looks only at URI.getPath()) and the consumers (URLClassLoader /
checksum, which act on the
+ // whole original URL) would address different objects — e.g.
"file://attacker/dir/x.jar" is
+ // fetched from a remote authority, and "file:///dir/x.jar?evil" maps
to a sibling file.
+ String authority = uri.getRawAuthority();
+ if ((authority != null && !authority.isEmpty())
+ || uri.getRawQuery() != null || uri.getRawFragment() != null) {
+ return false;
+ }
+ Path candidate = toLocalPath(driverUrl).normalize();
+ return allowedPaths.stream()
+ .map(allowed -> toLocalPath(allowed).normalize())
+ .anyMatch(candidate::startsWith);
+ }
- boolean isAllowed =
Arrays.stream(Config.jdbc_driver_secure_path.split(";"))
- .anyMatch(allowedPath ->
driverUrl.startsWith(allowedPath.trim()));
- if (!isAllowed) {
- throw new IllegalArgumentException("Driver URL does not match
any allowed paths: " + driverUrl);
+ /**
+ * Turn a {@code file://} URL or a plain filesystem path into a {@link
Path} for structural comparison.
+ * A {@code file://} URL is decoded exactly once via {@link URI#getPath()}
so that percent-encoded
+ * segments (e.g. {@code %2e%2e}) are resolved into the same
representation the driver-loading
+ * consumers ({@code URL.openStream} / {@code URLClassLoader}) will use;
otherwise an encoded parent
+ * segment would survive normalization and escape the allowed directory.
+ */
+ private static Path toLocalPath(String pathOrUrl) {
+ if (pathOrUrl.startsWith("file:")) {
+ try {
+ String decoded = new URI(pathOrUrl).getPath();
+ if (decoded != null && !decoded.isEmpty()) {
+ return Paths.get(decoded);
+ }
+ } catch (URISyntaxException ignored) {
+ // fall through to literal stripping below
}
- return driverUrl;
+ int sep = pathOrUrl.indexOf("//");
+ return Paths.get(sep >= 0 ? pathOrUrl.substring(sep + 2) :
pathOrUrl.substring("file:".length()));
+ }
+ return Paths.get(pathOrUrl);
+ }
+
+ /**
+ * Structural match for remote (http/https) driver URLs: scheme, host and
port must be equal, and the
+ * candidate path must sit under the allowed path (component-based). A
bare path prefix (no scheme) can
+ * never authorize a remote URL.
+ */
+ private static boolean remoteUrlMatches(URI candidate, String allowedPath)
{
+ URI base;
+ try {
+ base = new URI(allowedPath).normalize();
} catch (URISyntaxException e) {
- LOG.warn("invalid jdbc driver url: " + driverUrl);
- return driverUrl;
+ return false;
}
+ if (base.getScheme() == null) {
+ return false;
+ }
+ // Scheme/host/port and the path prefix must match, and the
resource-selecting components
+ // (user-info and query) that the checksum/classloader consumers act
on must match exactly too,
+ // otherwise e.g. ".../download?id=approved" would authorize
".../download?id=evil".
+ return base.getScheme().equalsIgnoreCase(candidate.getScheme())
+ && base.getHost() != null &&
base.getHost().equalsIgnoreCase(candidate.getHost())
+ && base.getPort() == candidate.getPort()
Review Comment:
[P1] Treat explicit default ports as the same remote endpoint
`URI.getPort()` returns `-1` when the URL omits its default port, so this
exact comparison rejects a semantically identical endpoint. For example, with
`jdbc_driver_secure_path=https://repo.example`, the previously accepted driver
`https://repo.example:443/jdbc/vendor.jar` now fails even though both URLs
address the same HTTPS endpoint; JDK 17 also reports the same effective default
port for them. Iceberg, Paimon, and legacy JDBC resolve these URLs lazily after
replay, so an existing catalog can fail on first access after upgrade. Please
normalize omitted HTTP/HTTPS ports to `80`/`443` before comparing, retain exact
matching for non-default ports, and cover the replayed direct-consumer case.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
