You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@polaris.apache.org by ad...@apache.org on 2026/10/22 18:43:38 UTC
(polaris) branch main updated: Make allowed-location checks consistent for paths with empty segments (#5849)
This is an automated email from the ASF dual-hosted git repository.
adutra pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/polaris.git
The following commit(s) were added to refs/heads/main by this push:
new 1458a90e35 Make allowed-location checks consistent for paths with empty segments (#5849)
1458a90e35 is described below
commit 1458a90e3566f52ae49e234b47945632bd117737
Author: Alexandre Dutra <ad...@apache.org>
AuthorDate: Thu Oct 22 20:43:33 2026 +0200
Make allowed-location checks consistent for paths with empty segments (#5849)
StorageLocationValidator trimmed one trailing slash from allowed
locations before comparing them. This is redundant with
StorageLocation.isChildOf, which already handles trailing slashes, and
it changed the meaning of locations ending with an empty path segment
(e.g. "s3://bucket//" was handled as "s3://bucket/"). Stop trimming so
that allowed locations are interpreted consistently.
Likewise, AzureLocation.isChildOf only re-added the leading slash of
the file path when it was missing, making "abfss://container@host//a"
equivalent to "abfss://container@host/a". Always re-add the separator
slash instead, and remove the now-unused
StorageLocation.ensureLeadingSlash.
---
.../polaris/core/storage/StorageLocation.java | 9 -------
.../core/storage/StorageLocationValidator.java | 13 +++-------
.../polaris/core/storage/azure/AzureLocation.java | 7 +++---
.../core/storage/StorageLocationValidatorTest.java | 29 ++++++++++++++++++++++
.../core/storage/azure/AzureLocationTest.java | 17 +++++++++++++
5 files changed, 53 insertions(+), 22 deletions(-)
diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java
index f965e292f9..d0573a170e 100644
--- a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java
+++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java
@@ -85,15 +85,6 @@ public class StorageLocation {
return path.startsWith("/") ? path.substring(1) : path;
}
- /** If a path doesn't start with `/`, this will add one */
- protected static @NonNull String ensureLeadingSlash(@NonNull String location) {
- if (location.startsWith("/")) {
- return location;
- } else {
- return "/" + location;
- }
- }
-
@Override
public int hashCode() {
return location.hashCode();
diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocationValidator.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocationValidator.java
index dfe81587aa..1d3d4e60ed 100644
--- a/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocationValidator.java
+++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocationValidator.java
@@ -56,18 +56,11 @@ public final class StorageLocationValidator {
@NonNull List<String> allowedLocationsToValid,
@NonNull Set<PolarisStorageActions> actions,
@NonNull Set<String> locations) {
- // trim trailing / from allowed locations so that locations missing the trailing slash still
- // match
+ // Don't trim trailing slashes here: trimming would change the location prefix (e.g.
+ // "s3://bucket//" would become "s3://bucket/"). StorageLocation.isChildOf already takes care of
+ // normalizing the location when doing comparisons.
Set<String> allowedLocationStrings =
allowedLocationsToValid.stream()
- .map(
- str -> {
- if (str.endsWith("/") && str.length() > 1) {
- return str.substring(0, str.length() - 1);
- } else {
- return str;
- }
- })
.map(str -> str.replace("file:///", "file:/"))
.collect(Collectors.toSet());
List<StorageLocation> allowedLocations =
diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/azure/AzureLocation.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/azure/AzureLocation.java
index 711c27deea..e00b93f2cf 100644
--- a/polaris-core/src/main/java/org/apache/polaris/core/storage/azure/AzureLocation.java
+++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/azure/AzureLocation.java
@@ -122,9 +122,10 @@ public class AzureLocation extends StorageLocation {
if (potentialParent instanceof AzureLocation potentialAzureParent) {
if (this.container.equals(potentialAzureParent.container)) {
if (this.storageAccount.equals(potentialAzureParent.storageAccount)) {
- String formattedFilePath = ensureLeadingSlash(ensureTrailingSlash(this.filePath));
- String formattedParentFilePath =
- ensureLeadingSlash(ensureTrailingSlash(potentialAzureParent.filePath));
+ // filePath has its separator slash removed, so always add it back (only adding it when
+ // missing would make "container@host//a" and "container@host/a" equivalent).
+ String formattedFilePath = ensureTrailingSlash("/" + this.filePath);
+ String formattedParentFilePath = ensureTrailingSlash("/" + potentialAzureParent.filePath);
return formattedFilePath.startsWith(formattedParentFilePath);
}
}
diff --git a/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageLocationValidatorTest.java b/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageLocationValidatorTest.java
index b0a748dcfd..376d2de880 100644
--- a/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageLocationValidatorTest.java
+++ b/polaris-core/src/test/java/org/apache/polaris/core/storage/StorageLocationValidatorTest.java
@@ -21,6 +21,7 @@ package org.apache.polaris.core.storage;
import static org.apache.polaris.core.config.RealmConfigurationSource.EMPTY_CONFIG;
import static org.assertj.core.api.Assertions.assertThat;
+import java.util.List;
import java.util.Map;
import java.util.Set;
import org.apache.polaris.core.config.RealmConfig;
@@ -145,6 +146,34 @@ class StorageLocationValidatorTest {
.hasEntrySatisfying("s3://bucket/path/to", map -> assertValidationResult(map, false));
}
+ @ParameterizedTest
+ @ValueSource(
+ strings = {
+ "s3://bucket",
+ "s3a://bucket",
+ "s3://bucket/data",
+ "gs://bucket",
+ "gs://bucket/data",
+ "abfss://container@acct.dfs.core.windows.net",
+ "abfss://container@acct.dfs.core.windows.net/data"
+ })
+ public void testValidateAllowedLocationsWithEmptyTrailingPathSegment(String base) {
+ RealmConfig realmConfig = new RealmConfigImpl(EMPTY_CONFIG, REALM_CONTEXT);
+ String allowedLocation = base + "//";
+ String locationOutside = base + "/path/to/table";
+ String locationInside = base + "//path/to/table";
+ Map<String, Map<PolarisStorageActions, StorageLocationValidator.ValidationResult>> result =
+ StorageLocationValidator.validateAllowedLocations(
+ realmConfig,
+ List.of(allowedLocation),
+ Set.of(PolarisStorageActions.READ),
+ Set.of(locationOutside, locationInside));
+ assertThat(result)
+ .hasSize(2)
+ .hasEntrySatisfying(locationOutside, val -> assertValidationResult(val, false))
+ .hasEntrySatisfying(locationInside, val -> assertValidationResult(val, true));
+ }
+
private void assertValidationResult(
Map<PolarisStorageActions, StorageLocationValidator.ValidationResult> results,
boolean expected) {
diff --git a/polaris-core/src/test/java/org/apache/polaris/core/storage/azure/AzureLocationTest.java b/polaris-core/src/test/java/org/apache/polaris/core/storage/azure/AzureLocationTest.java
index b0f6a3bd09..7a81a12533 100644
--- a/polaris-core/src/test/java/org/apache/polaris/core/storage/azure/AzureLocationTest.java
+++ b/polaris-core/src/test/java/org/apache/polaris/core/storage/azure/AzureLocationTest.java
@@ -72,6 +72,23 @@ public class AzureLocationTest {
Assertions.assertThat(location.isChildOf(parentLocationTrailingSlash)).isTrue();
}
+ @Test
+ public void testLocationComparisonsWithEmptyPathSegment() {
+ StorageLocation root = AzureLocation.of("abfss://container@acc.dfs.core.windows.net/");
+ StorageLocation emptySegment = AzureLocation.of("abfss://container@acc.dfs.core.windows.net//");
+ StorageLocation location =
+ AzureLocation.of("abfss://container@acc.dfs.core.windows.net/some_file/metadata");
+ StorageLocation locationUnderEmptySegment =
+ AzureLocation.of("abfss://container@acc.dfs.core.windows.net//some_file/metadata");
+
+ Assertions.assertThat(emptySegment.isChildOf(root)).isTrue();
+ Assertions.assertThat(root.isChildOf(emptySegment)).isFalse();
+ Assertions.assertThat(location.isChildOf(emptySegment)).isFalse();
+ Assertions.assertThat(locationUnderEmptySegment.isChildOf(emptySegment)).isTrue();
+ Assertions.assertThat(locationUnderEmptySegment.isChildOf(location)).isFalse();
+ Assertions.assertThat(location.isChildOf(locationUnderEmptySegment)).isFalse();
+ }
+
@Test
public void testLocation_negative_cases() {
Assertions.assertThatThrownBy(