This is an automated email from the ASF dual-hosted git repository.
mchades pushed a commit to branch branch-1.3
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/branch-1.3 by this push:
new 52f6362ba1 [Cherry-pick to branch-1.3] [MINOR] fix(fileset): validate
sub-path resolution (#13721) (#13796)
52f6362ba1 is described below
commit 52f6362ba175919d40438ecd8aa7e5f48734bfa6
Author: github-actions[bot]
<41898282+github-actions[bot]@users.noreply.github.com>
AuthorDate: Tue Oct 13 15:43:53 2026 +0800
[Cherry-pick to branch-1.3] [MINOR] fix(fileset): validate sub-path
resolution (#13721) (#13796)
### What changes were proposed in this pull request?
Backport #13721 (878240b75deeeb754e431fb110c6f2553d73c7fb) to
`branch-1.3`.
Resolve the test-file conflict, retain the path-validation coverage, and
adapt the test helper to the 1.3 constructor.
### Why are the changes needed?
Keep fileset sub-path resolution within the selected storage location.
### Does this PR introduce _any_ user-facing change?
File listing and location lookup reject invalid sub-paths with HTTP 400
while preserving valid path behavior.
### How was this patch tested?
- `TestFilesetCatalogOperations`: 225 tests passed.
- `TestFilesetOperations`: 19 tests passed.
- `spotlessApply`, the catalog/server Java format checks, and
`:docs:build` passed.
- Docker integration tests were not run locally.
---------
Co-authored-by: mchades <[email protected]>
---
.../org/apache/gravitino/file/FilesetCatalog.java | 12 +
.../catalog/fileset/FilesetCatalogOperations.java | 64 +++--
.../fileset/TestFilesetCatalogOperations.java | 272 +++++++++++++++++++++
.../apache/gravitino/client/FilesetCatalog.java | 11 +-
.../apache/gravitino/catalog/FilesetFileOps.java | 6 +
docs/open-api/filesets.yaml | 33 ++-
.../server/web/rest/TestFilesetOperations.java | 54 +++-
7 files changed, 418 insertions(+), 34 deletions(-)
diff --git a/api/src/main/java/org/apache/gravitino/file/FilesetCatalog.java
b/api/src/main/java/org/apache/gravitino/file/FilesetCatalog.java
index 448b1990a7..c0c1885917 100644
--- a/api/src/main/java/org/apache/gravitino/file/FilesetCatalog.java
+++ b/api/src/main/java/org/apache/gravitino/file/FilesetCatalog.java
@@ -160,9 +160,15 @@ public interface FilesetCatalog {
* Get the actual location of a file or directory based on the default
storage location of Fileset
* and the sub path.
*
+ * <p>Leading and trailing whitespace in the sub path is trimmed. An empty
path or {@code /}
+ * refers to the default storage location. A leading slash is relative to
that location.
+ *
* @param ident A fileset identifier.
* @param subPath The sub path to the file or directory.
* @return The actual location of the file or directory.
+ * @throws IllegalArgumentException If the sub path contains a complete
{@code ..} path segment or
+ * a NUL character ({@code U+0000}), or its normalized path is outside
the default storage
+ * location.
* @throws NoSuchFilesetException If the fileset does not exist.
*/
default String getFileLocation(NameIdentifier ident, String subPath)
@@ -174,10 +180,16 @@ public interface FilesetCatalog {
* Get the actual location of a file or directory based on the storage
location of Fileset and the
* sub path by the location name.
*
+ * <p>Leading and trailing whitespace in the sub path is trimmed. An empty
path or {@code /}
+ * refers to the selected storage location. A leading slash is relative to
that location.
+ *
* @param ident A fileset identifier.
* @param subPath The sub path to the file or directory.
* @param locationName The location name. If null, the default location will
be used.
* @return The actual location of the file or directory.
+ * @throws IllegalArgumentException If the sub path contains a complete
{@code ..} path segment or
+ * a NUL character ({@code U+0000}), or its normalized path is outside
the selected storage
+ * location.
* @throws NoSuchFilesetException If the fileset does not exist.
* @throws NoSuchLocationNameException If the location name does not exist.
*/
diff --git
a/catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java
b/catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java
index b725c27125..433c52401b 100644
---
a/catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java
+++
b/catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java
@@ -1721,11 +1721,16 @@ public class FilesetCatalogOperations extends
ManagedSchemaOperations
private String getFileLocation(Fileset fileset, String subPath, String
locationName) {
Preconditions.checkArgument(subPath != null, "subPath must not be null");
- String processedSubPath;
- if (!subPath.trim().isEmpty() && !subPath.trim().startsWith(SLASH)) {
- processedSubPath = SLASH + subPath.trim();
- } else {
- processedSubPath = subPath.trim();
+ String processedSubPath = subPath.trim();
+ // Validate before constructing a Hadoop Path, which normalizes away
parent directory segments.
+ Preconditions.checkArgument(
+ subPath.indexOf('\0') < 0, "subPath must not contain null characters");
+ for (String segment : processedSubPath.split(SLASH)) {
+ Preconditions.checkArgument(
+ !"..".equals(segment), "subPath must not contain parent directory
segments");
+ }
+ if (!processedSubPath.isEmpty() && !processedSubPath.startsWith(SLASH)) {
+ processedSubPath = SLASH + processedSubPath;
}
String targetLocationName;
@@ -1744,6 +1749,20 @@ public class FilesetCatalogOperations extends
ManagedSchemaOperations
"Location name %s does not exist in fileset %s", targetLocationName,
fileset.name());
}
+ String storageLocation =
fileset.storageLocations().get(targetLocationName);
+ String fileLocation =
+ processedSubPath.isEmpty()
+ ? storageLocation
+ : removeTrailingSlash(storageLocation) + processedSubPath;
+
+ // Compare normalized Path ancestors, including scheme and authority,
rather than string
+ // prefixes. Keep the original spelling in the response for existing
clients.
+ Path storagePath = normalizeLocationPath(storageLocation);
+ Path filePath = normalizeLocationPath(fileLocation);
+ Preconditions.checkArgument(
+ isWithinStorageLocation(storagePath, filePath),
+ "subPath must stay within the fileset storage location");
+
// do checks for some data operations.
if (hasCallerContext()) {
Map<String, String> contextMap =
CallerContext.CallerContextHolder.get().context();
@@ -1759,10 +1778,9 @@ public class FilesetCatalogOperations extends
ManagedSchemaOperations
FilesetDataOperation dataOperation =
FilesetDataOperation.valueOf(operation);
switch (dataOperation) {
case RENAME:
- // if the sub path is blank, it cannot be renamed otherwise the
metadata in the
- // Gravitino server may be inconsistent.
- if (StringUtils.isBlank(processedSubPath)
- || (processedSubPath.startsWith(SLASH) &&
processedSubPath.length() == 1)) {
+ // Include aliases such as /./ so the fileset root cannot be
renamed independently of
+ // its metadata.
+ if (filePath.equals(storagePath)) {
throw new GravitinoRuntimeException(
"subPath cannot be blank when need to rename a file or a
directory.");
}
@@ -1773,20 +1791,26 @@ public class FilesetCatalogOperations extends
ManagedSchemaOperations
}
}
- String fileLocation;
- // If the processed sub path is blank, we pass the storage location
directly
- if (StringUtils.isBlank(processedSubPath)) {
- fileLocation = fileset.storageLocations().get(targetLocationName);
- } else {
- // the processed sub path always starts with "/" if it is not blank,
- // so we can safely remove the tailing slash if the storage location
ends with "/".
- String storageLocation =
-
removeTrailingSlash(fileset.storageLocations().get(targetLocationName));
- fileLocation = String.format("%s%s", storageLocation, processedSubPath);
- }
return fileLocation;
}
+ private Path normalizeLocationPath(String location) {
+ // Represent authority-only locations (for example s3a://bucket) as
filesystem roots. Hadoop
+ // removes trailing slashes before normalizing dot segments, so parse
again to remove a trailing
+ // slash introduced by normalization of an alias such as /directory/./.
+ Path path = new Path(ensureTrailingSlash(location));
+ return new Path(path.toString());
+ }
+
+ private static boolean isWithinStorageLocation(Path storagePath, Path
filePath) {
+ for (Path current = filePath; current != null; current =
current.getParent()) {
+ if (storagePath.equals(current)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
@VisibleForTesting
Map<String, String> mergeUpLevelConfigurations(
NameIdentifier ident, Map<String, String> entityProperties, Path path) {
diff --git
a/catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java
b/catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java
index d3f670070b..378dd580b7 100644
---
a/catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java
+++
b/catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java
@@ -56,6 +56,7 @@ import com.google.common.collect.Maps;
import java.io.File;
import java.io.IOException;
import java.net.ConnectException;
+import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.Arrays;
import java.util.Collections;
@@ -123,12 +124,16 @@ import org.apache.hadoop.fs.Path;
import org.awaitility.Awaitility;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Assumptions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.Timeout;
+import org.junit.jupiter.api.io.TempDir;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.EnumSource;
import org.junit.jupiter.params.provider.MethodSource;
+import org.junit.jupiter.params.provider.ValueSource;
import org.mockito.MockedStatic;
import org.mockito.Mockito;
@@ -143,6 +148,9 @@ public class TestFilesetCatalogOperations {
private static final String TEST_ROOT_PATH = "file:" +
UNFORMALIZED_TEST_ROOT_PATH;
+ private static final NameIdentifier PATH_VALIDATION_FILESET =
+ NameIdentifier.of("metalake", "catalog", "schema", "fileset");
+
private static final HasPropertyMetadata FILESET_PROPERTIES_METADATA =
new HasPropertyMetadata() {
@Override
@@ -1760,6 +1768,215 @@ public class TestFilesetCatalogOperations {
}
}
+ @ParameterizedTest
+ @ValueSource(
+ strings = {
+ "..",
+ "../outside",
+ "/../outside",
+ "/dir/../../outside",
+ "dir/../file",
+ " /../outside ",
+ "/file\u0000name",
+ "\u0000file",
+ "/file\u0000"
+ })
+ void testRejectUnsafeSubPaths(String subPath) throws IOException {
+ try (FilesetCatalogOperations operations =
+ pathValidationOperations(mockFilesetForPathValidation())) {
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> operations.getFileLocation(PATH_VALIDATION_FILESET, subPath,
null));
+ }
+ }
+
+ @ParameterizedTest
+ @MethodSource("validLocations")
+ void testPreserveValidLocations(String storageLocation, String subPath,
String expected)
+ throws IOException {
+ Fileset fileset = mockFilesetForPathValidation();
+ when(fileset.storageLocations()).thenReturn(ImmutableMap.of("primary",
storageLocation));
+ try (FilesetCatalogOperations operations =
pathValidationOperations(fileset)) {
+ Assertions.assertEquals(
+ expected, operations.getFileLocation(PATH_VALIDATION_FILESET,
subPath, null));
+ }
+ }
+
+ @Test
+ void testSelectedLocationDefinesBoundary() throws IOException {
+ Fileset fileset = mockFilesetForPathValidation();
+ when(fileset.storageLocations())
+ .thenReturn(
+ ImmutableMap.of(
+ "primary", "s3a://bucket/primary", "archive",
"hdfs://namenode:8020/archive"));
+ try (FilesetCatalogOperations operations =
pathValidationOperations(fileset)) {
+ Assertions.assertEquals(
+ "s3a://bucket/primary/data",
+ operations.getFileLocation(PATH_VALIDATION_FILESET, "/data", null));
+ Assertions.assertEquals(
+ "hdfs://namenode:8020/archive/data",
+ operations.getFileLocation(PATH_VALIDATION_FILESET, "/data",
"archive"));
+ for (String location : fileset.storageLocations().keySet()) {
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> operations.getFileLocation(PATH_VALIDATION_FILESET,
"/../outside", location));
+ }
+ }
+ }
+
+ @Test
+ void testRejectAuthorityChangeAtFilesystemRoot() throws IOException {
+ Fileset fileset = mockFilesetForPathValidation();
+ when(fileset.storageLocations()).thenReturn(ImmutableMap.of("primary",
"/"));
+ try (FilesetCatalogOperations operations =
pathValidationOperations(fileset)) {
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> operations.getFileLocation(PATH_VALIDATION_FILESET,
"//other-host/outside", null));
+ }
+ }
+
+ @ParameterizedTest
+ @EnumSource(FilesetDataOperation.class)
+ void testOperationHeadersCannotBypassValidation(FilesetDataOperation
operation)
+ throws IOException {
+ try (FilesetCatalogOperations operations =
+ pathValidationOperations(mockFilesetForPathValidation())) {
+ CallerContext.CallerContextHolder.set(
+ CallerContext.builder()
+ .withContext(
+ ImmutableMap.of(
+
FilesetAuditConstants.HTTP_HEADER_FILESET_DATA_OPERATION, operation.name()))
+ .build());
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> operations.getFileLocation(PATH_VALIDATION_FILESET,
"/../outside", null));
+ } finally {
+ CallerContext.CallerContextHolder.remove();
+ }
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = {"", "/", "/.", "/./", "//", "///"})
+ void testRenameCannotTargetRootAliases(String subPath) throws IOException {
+ try (FilesetCatalogOperations operations =
+ pathValidationOperations(mockFilesetForPathValidation())) {
+ CallerContext.CallerContextHolder.set(
+ CallerContext.builder()
+ .withContext(
+ ImmutableMap.of(
+ FilesetAuditConstants.HTTP_HEADER_FILESET_DATA_OPERATION,
+ FilesetDataOperation.RENAME.name()))
+ .build());
+ Assertions.assertThrows(
+ GravitinoRuntimeException.class,
+ () -> operations.getFileLocation(PATH_VALIDATION_FILESET, subPath,
null));
+ } finally {
+ CallerContext.CallerContextHolder.remove();
+ }
+ }
+
+ @Test
+ void testRenameAllowsNonRootPathAlias() throws IOException {
+ try (FilesetCatalogOperations operations =
+ pathValidationOperations(mockFilesetForPathValidation())) {
+ CallerContext.CallerContextHolder.set(
+ CallerContext.builder()
+ .withContext(
+ ImmutableMap.of(
+ FilesetAuditConstants.HTTP_HEADER_FILESET_DATA_OPERATION,
+ FilesetDataOperation.RENAME.name()))
+ .build());
+ Assertions.assertEquals(
+ "file:///storage/fileset/dir/./file",
+ operations.getFileLocation(PATH_VALIDATION_FILESET, "/dir/./file",
null));
+ } finally {
+ CallerContext.CallerContextHolder.remove();
+ }
+ }
+
+ @Test
+ void testListFilesCannotReachSiblingSentinel(@TempDir File tempDir) throws
IOException {
+ File root = new File(tempDir, "root");
+ File outside = new File(tempDir, "root-sibling");
+ Files.createDirectories(root.toPath());
+ Files.createDirectories(outside.toPath());
+ Files.writeString(new File(outside, "sentinel.txt").toPath(), "outside the
fileset");
+ Fileset fileset = mockFilesetForPathValidation();
+ when(fileset.storageLocations())
+ .thenReturn(ImmutableMap.of("primary", root.toURI().toString()));
+
+ try (FilesetCatalogOperations operations =
pathValidationOperations(fileset);
+ FileSystem fs = FileSystem.newInstanceLocal(new Configuration(false)))
{
+ doReturn(Collections.emptyMap())
+ .when(operations)
+ .mergeUpLevelConfigurations(any(), any(), any());
+ doReturn(fs).when(operations).getFileSystemWithCache(any(), any());
+ Assertions.assertEquals(1, fs.listStatus(new
Path(outside.toURI())).length);
+ Assertions.assertEquals(0, operations.listFiles(PATH_VALIDATION_FILESET,
null, "/").length);
+ Mockito.clearInvocations(operations);
+
+ for (String subPath :
+ new String[] {"/../root-sibling", "/../root-sibling/sentinel.txt",
"/../missing"}) {
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> operations.listFiles(PATH_VALIDATION_FILESET, null,
subPath));
+ }
+ Mockito.verify(operations,
Mockito.never()).mergeUpLevelConfigurations(any(), any(), any());
+ Mockito.verify(operations,
Mockito.never()).getFileSystemWithCache(any(), any());
+ }
+ }
+
+ @Test
+ void testListFilesPreservesLiteralEncodedNames(@TempDir File tempDir) throws
IOException {
+ File root = new File(tempDir, "root");
+ File child = new File(root, "%2e%2e");
+ Files.createDirectories(child.toPath());
+ Files.writeString(new File(child, "a..b + caf\u00e9.txt").toPath(),
"inside the fileset");
+ Fileset fileset = mockFilesetForPathValidation();
+ when(fileset.storageLocations())
+ .thenReturn(ImmutableMap.of("primary", root.toURI().toString()));
+
+ try (FilesetCatalogOperations operations =
pathValidationOperations(fileset);
+ FileSystem fs = FileSystem.newInstanceLocal(new Configuration(false)))
{
+ doReturn(Collections.emptyMap())
+ .when(operations)
+ .mergeUpLevelConfigurations(any(), any(), any());
+ doReturn(fs).when(operations).getFileSystemWithCache(any(), any());
+ FileInfo[] files = operations.listFiles(PATH_VALIDATION_FILESET, null,
"/%2e%2e");
+ Assertions.assertEquals(1, files.length);
+ Assertions.assertEquals("a..b + caf\u00e9.txt", files[0].name());
+ }
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = {"dir\\name", "..\\outside", "dir\\..\\outside"})
+ void testListFilesPreservesLiteralBackslashes(String childName, @TempDir
File tempDir)
+ throws IOException {
+ Assumptions.assumeFalse(Path.WINDOWS, "Backslashes are path separators on
Windows");
+ File root = new File(tempDir, "root");
+ File child = new File(root, childName);
+ Files.createDirectories(child.toPath());
+ Files.writeString(new File(child, "file\\name.txt").toPath(), "inside the
fileset");
+ String storageLocation = root.toURI().toString();
+ Fileset fileset = mockFilesetForPathValidation();
+ when(fileset.storageLocations()).thenReturn(ImmutableMap.of("primary",
storageLocation));
+
+ try (FilesetCatalogOperations operations =
pathValidationOperations(fileset);
+ FileSystem fs = FileSystem.newInstanceLocal(new Configuration(false)))
{
+ doReturn(Collections.emptyMap())
+ .when(operations)
+ .mergeUpLevelConfigurations(any(), any(), any());
+ doReturn(fs).when(operations).getFileSystemWithCache(any(), any());
+ String subPath = "/" + childName;
+ Assertions.assertEquals(
+ storageLocation + childName,
+ operations.getFileLocation(PATH_VALIDATION_FILESET, subPath, null));
+ FileInfo[] files = operations.listFiles(PATH_VALIDATION_FILESET, null,
subPath);
+ Assertions.assertEquals(1, files.length);
+ Assertions.assertEquals("file\\name.txt", files[0].name());
+ }
+ }
+
@Test
public void testGetFileLocation() throws IOException {
final long testId = generateTestId();
@@ -1860,6 +2077,8 @@ public class TestFilesetCatalogOperations {
CallerContext.CallerContextHolder.set(callerContext);
Assertions.assertThrows(
GravitinoRuntimeException.class, () ->
ops.getFileLocation(filesetIdent, ""));
+ } finally {
+ CallerContext.CallerContextHolder.remove();
}
// test storage location end with "/"
@@ -3650,4 +3869,57 @@ public class TestFilesetCatalogOperations {
Assertions.assertNull(result3.get("endpoint"));
}
}
+
+ private static Stream<Arguments> validLocations() {
+ return Stream.of(
+ Arguments.of("file:///storage/fileset", "", "file:///storage/fileset"),
+ Arguments.of("file:///storage/fileset", "/",
"file:///storage/fileset/"),
+ Arguments.of("file:///storage/fileset/", "dir/file",
"file:///storage/fileset/dir/file"),
+ Arguments.of("file:///storage/fileset/.", "/child",
"file:///storage/fileset/./child"),
+ Arguments.of("file:///storage/fileset", "/dir/file",
"file:///storage/fileset/dir/file"),
+ Arguments.of("file:///storage/fileset", "/a..b",
"file:///storage/fileset/a..b"),
+ Arguments.of(
+ "file:///storage/fileset", "/dir/./file",
"file:///storage/fileset/dir/./file"),
+ Arguments.of("file:///storage/fileset", "/%2e%2e",
"file:///storage/fileset/%2e%2e"),
+ Arguments.of("file:///storage/fileset", "/a + b",
"file:///storage/fileset/a + b"),
+ Arguments.of("file:///", "/child", "file:///child"),
+ Arguments.of("/", "/child", "/child"),
+ Arguments.of("/storage/fileset", "child", "/storage/fileset/child"),
+ Arguments.of(
+ "hdfs://namenode:8020/fileset", "/child",
"hdfs://namenode:8020/fileset/child"),
+ Arguments.of(
+ "hdfs://namenode:8020/fileset",
+ "/dir\\name.txt",
+ "hdfs://namenode:8020/fileset/dir\\name.txt"),
+ Arguments.of("s3a://bucket/prefix", "/dir\\name.txt",
"s3a://bucket/prefix/dir\\name.txt"),
+ Arguments.of("gs://bucket/prefix", "/dir\\name.txt",
"gs://bucket/prefix/dir\\name.txt"),
+ Arguments.of("s3a://bucket/prefix", "/..\\outside",
"s3a://bucket/prefix/..\\outside"),
+ Arguments.of("s3a://bucket/prefix", "\\..\\outside",
"s3a://bucket/prefix/\\..\\outside"),
+ Arguments.of(
+ "s3a://bucket/prefix", "/dir\\..\\outside",
"s3a://bucket/prefix/dir\\..\\outside"),
+ Arguments.of("s3a://bucket/prefix", "/child",
"s3a://bucket/prefix/child"),
+ Arguments.of("s3a://bucket", "/child", "s3a://bucket/child"),
+ Arguments.of("s3a://bucket", "", "s3a://bucket"),
+ Arguments.of(
+ "abfs://[email protected]/prefix",
+ "/child",
+ "abfs://[email protected]/prefix/child"));
+ }
+
+ private Fileset mockFilesetForPathValidation() {
+ Fileset fileset = Mockito.mock(Fileset.class);
+ when(fileset.name()).thenReturn(PATH_VALIDATION_FILESET.name());
+ when(fileset.properties())
+ .thenReturn(ImmutableMap.of(PROPERTY_DEFAULT_LOCATION_NAME,
"primary"));
+ when(fileset.storageLocations())
+ .thenReturn(ImmutableMap.of("primary", "file:///storage/fileset"));
+ return fileset;
+ }
+
+ private FilesetCatalogOperations pathValidationOperations(Fileset fileset) {
+ FilesetCatalogOperations operations =
+ Mockito.spy(new
FilesetCatalogOperations(Mockito.mock(EntityStore.class)));
+ doReturn(fileset).when(operations).loadFileset(PATH_VALIDATION_FILESET);
+ return operations;
+ }
}
diff --git
a/clients/client-java/src/main/java/org/apache/gravitino/client/FilesetCatalog.java
b/clients/client-java/src/main/java/org/apache/gravitino/client/FilesetCatalog.java
index 33c9b8d131..9a11e5e919 100644
---
a/clients/client-java/src/main/java/org/apache/gravitino/client/FilesetCatalog.java
+++
b/clients/client-java/src/main/java/org/apache/gravitino/client/FilesetCatalog.java
@@ -229,16 +229,7 @@ class FilesetCatalog extends BaseSchemaCatalog
return resp.dropped();
}
- /**
- * Get the actual path of a file or directory based on the storage location
of Fileset and the sub
- * path.
- *
- * @param ident A fileset identifier.
- * @param subPath The sub path to the file or directory.
- * @param locationName The name of the location to be accessed.
- * @return The actual location of the file or directory.
- * @throws NoSuchFilesetException If the fileset does not exist.
- */
+ /** {@inheritDoc} */
@Override
public String getFileLocation(NameIdentifier ident, String subPath, String
locationName)
throws NoSuchFilesetException, NoSuchLocationNameException {
diff --git
a/core/src/main/java/org/apache/gravitino/catalog/FilesetFileOps.java
b/core/src/main/java/org/apache/gravitino/catalog/FilesetFileOps.java
index c06db4eb42..8c9fd7795d 100644
--- a/core/src/main/java/org/apache/gravitino/catalog/FilesetFileOps.java
+++ b/core/src/main/java/org/apache/gravitino/catalog/FilesetFileOps.java
@@ -34,10 +34,16 @@ public interface FilesetFileOps {
/**
* List the files in a fileset with a specific location name and sub path.
*
+ * <p>Leading and trailing whitespace in the sub path is trimmed. An empty
path or {@code /}
+ * refers to the selected storage location. A leading slash is relative to
that location.
+ *
* @param ident A fileset identifier.
* @param locationName The location name. If null, the default location will
be used.
* @param subPath The sub path under the fileset.
* @return An array of file information objects.
+ * @throws IllegalArgumentException If the sub path contains a complete
{@code ..} path segment or
+ * a NUL character ({@code U+0000}), resolves outside the selected
storage location, or refers
+ * to a path that does not exist.
*/
default FileInfo[] listFiles(NameIdentifier ident, String locationName,
String subPath)
throws NoSuchFilesetException, IOException {
diff --git a/docs/open-api/filesets.yaml b/docs/open-api/filesets.yaml
index 956b9bad63..20ef5d1cf6 100644
--- a/docs/open-api/filesets.yaml
+++ b/docs/open-api/filesets.yaml
@@ -176,7 +176,12 @@ paths:
required: true
schema:
type: string
- description: The sub path to the file or directory
+ description: >-
+ The sub path to the file or directory within the selected storage
location.
+ Leading and trailing whitespace is trimmed. An empty path or `/`
refers to
+ that location, and a leading slash is relative to it. Complete
`..` path
+ segments and NUL characters (U+0000) are not allowed.
+ The normalized path must remain within the selected storage
location.
- name: location_name
in: query
required: false
@@ -187,6 +192,8 @@ paths:
responses:
"200":
$ref: "#/components/responses/FileLocationResponse"
+ "400":
+ $ref: "#/components/responses/InvalidSubPathErrorResponse"
"404":
description: Not Found - The target fileset does not exist
content:
@@ -225,7 +232,13 @@ paths:
schema:
type: string
default: "/"
- description: The sub path within the fileset to list files and
directories from
+ description: >-
+ The sub path within the selected storage location to list files
and directories
+ from. Leading and trailing whitespace is trimmed. An empty path or
`/` refers
+ to that location, and a leading slash is relative to it. Complete
`..` path
+ segments and NUL characters (U+0000) are not allowed.
+ The normalized path must remain within the selected storage
location.
+ The path must exist; otherwise, this operation returns HTTP 400.
- name: location_name
in: query
required: false
@@ -235,8 +248,10 @@ paths:
responses:
"200":
$ref: "#/components/responses/FileInfoListResponse"
+ "400":
+ $ref: "#/components/responses/InvalidSubPathErrorResponse"
"404":
- description: Not Found – the target fileset or subPath or
locationName does not exist
+ description: Not Found – the target fileset or locationName does not
exist
content:
application/vnd.gravitino.v1+json:
schema:
@@ -467,6 +482,18 @@ components:
}
responses:
+ InvalidSubPathErrorResponse:
+ description: >-
+ The sub path does not satisfy the fileset path requirements.
+ content:
+ application/vnd.gravitino.v1+json:
+ schema:
+ $ref: "./openapi.yaml#/components/schemas/ErrorModel"
+ example:
+ code: 1001
+ type: IllegalArgumentException
+ message: subPath must not contain parent directory segments
+
FilesetResponse:
description: The response of fileset object
content:
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestFilesetOperations.java
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestFilesetOperations.java
index 770274b946..544c02a26f 100644
---
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestFilesetOperations.java
+++
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestFilesetOperations.java
@@ -35,9 +35,12 @@ import com.google.common.collect.Maps;
import java.io.IOException;
import java.time.Instant;
import java.util.Map;
+import java.util.stream.Stream;
import javax.servlet.http.HttpServletRequest;
import javax.ws.rs.client.Entity;
import javax.ws.rs.core.Application;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.HttpHeaders;
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.Response;
import org.apache.commons.lang3.reflect.FieldUtils;
@@ -77,16 +80,22 @@ import org.glassfish.jersey.test.TestProperties;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
import org.mockito.Mockito;
import org.mockito.stubbing.Answer;
public class TestFilesetOperations extends BaseOperationsTest {
private static class MockServletRequestFactory extends
ServletRequestFactoryBase {
+ @Context private HttpHeaders headers;
+
@Override
public HttpServletRequest get() {
HttpServletRequest request = mock(HttpServletRequest.class);
when(request.getRemoteUser()).thenReturn(null);
- when(request.getHeader(Version.CLIENT_VERSION_HEADER)).thenReturn(null);
+ when(request.getHeader(Version.CLIENT_VERSION_HEADER))
+ .thenReturn(headers.getHeaderString(Version.CLIENT_VERSION_HEADER));
return request;
}
}
@@ -722,6 +731,49 @@ public class TestFilesetOperations extends
BaseOperationsTest {
Assertions.assertNull(CallerContext.CallerContextHolder.get());
}
+ @ParameterizedTest
+ @MethodSource("invalidSubPathRequests")
+ void testInvalidSubPathsReturnBadRequest(String route, String subPath,
String clientVersion)
+ throws IOException {
+ Mockito.clearInvocations(dispatcher);
+ NameIdentifier ident = NameIdentifier.of(metalake, catalog, schema,
"fileset1");
+ String decodedSubPath = "/../outside";
+ IllegalArgumentException exception =
+ new IllegalArgumentException("subPath must not contain parent
directory segments");
+ doThrow(exception).when(dispatcher).getFileLocation(ident, decodedSubPath,
null);
+ doThrow(exception).when(dispatcher).listFiles(ident, null, decodedSubPath);
+
+ try (Response response =
+ target(filesetPath(metalake, catalog, schema) + "fileset1/" + route)
+ .queryParam("sub_path", subPath)
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .header(Version.CLIENT_VERSION_HEADER, clientVersion)
+ .accept("application/vnd.gravitino.v1+json")
+ .get()) {
+ Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
response.getStatus());
+ ErrorResponse error = response.readEntity(ErrorResponse.class);
+ Assertions.assertEquals(ErrorConstants.ILLEGAL_ARGUMENTS_CODE,
error.getCode());
+ Assertions.assertEquals(IllegalArgumentException.class.getSimpleName(),
error.getType());
+
Assertions.assertTrue(error.getMessage().contains(exception.getMessage()));
+ }
+
+ if (route.equals("location")) {
+ Mockito.verify(dispatcher).getFileLocation(ident, decodedSubPath, null);
+ } else {
+ Mockito.verify(dispatcher).listFiles(ident, null, decodedSubPath);
+ }
+ }
+
+ private static Stream<Arguments> invalidSubPathRequests() {
+ return Stream.of("files", "location")
+ .flatMap(
+ route ->
+ Stream.of(
+ Arguments.of(route, "/../outside", null),
+ Arguments.of(route, "%2F%2e%2E%2Foutside", "1.3.0"),
+ Arguments.of(route, "%252F%252e%252e%252Foutside",
"0.9.1")));
+ }
+
private void assertUpdateFileset(FilesetUpdatesRequest req, Fileset
updatedFileset) {
when(dispatcher.alterFileset(any(),
any(FilesetChange.class))).thenReturn(updatedFileset);
