jerryshao commented on code in PR #13721:
URL: https://github.com/apache/gravitino/pull/13721#discussion_r4236231208
##########
catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java:
##########
@@ -1925,6 +1932,172 @@ void testTrailSlash() throws IOException {
}
}
+ @ParameterizedTest
+ @ValueSource(
+ strings = {
+ "..",
+ "../outside",
+ "/../outside",
+ "/dir/../../outside",
+ "dir/../file",
+ " /../outside ",
+ "/..\\outside",
+ "\\..\\outside",
+ "/dir\\..\\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));
+ Mockito.verify(operations,
Mockito.never()).getFileSystemWithCache(any(), any());
Review Comment:
[Nit] This assertion cannot fail.
`getFileLocation` never touches a filesystem on any path - it only reads
`fileset.storageLocations()`, builds `Path` objects and validates - so
`verify(never()).getFileSystemWithCache(...)` holds regardless of whether the
validation works. Same for the copy at line 1970.
The assertion is meaningful in `testListFilesCannotReachSiblingSentinel`
(line 2075), where `listFiles` would otherwise reach the filesystem; here it
reads as a guarantee the test is not actually providing. Suggest dropping it
from these two tests, or asserting the exception message instead so the test
distinguishes the backslash/NUL rejection from the `..` rejection from the
boundary rejection - right now all three parameter groups would pass even if a
single over-broad check swallowed them all.
Verified by: read the whole method body at
FilesetCatalogOperations.java:1797-1872 in this checkout; there is no
filesystem access before the return.
##########
api/src/main/java/org/apache/gravitino/file/FilesetCatalog.java:
##########
@@ -249,10 +255,16 @@ default String getFileLocation(NameIdentifier ident,
String subPath)
* 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, a
+ * backslash, or a NUL character ({@code U+0000}), or its normalized
path is outside the
+ * selected storage location.
Review Comment:
[Nit] The file-listing contract did not get the same treatment.
Both `getFileLocation` overloads here and both OpenAPI operations now
describe the constraints, but `FilesetFileOps#listFiles` - the Java-side
declaration of the other endpoint that validates the sub path - still documents
`@param subPath The sub path under the fileset.` with no constraints and no
`IllegalArgumentException`
(core/src/main/java/org/apache/gravitino/catalog/FilesetFileOps.java:34-45).
It is an internal interface ("not exposed to public client APIs"), so this
is low priority, but it is the only remaining place where the new behaviour is
undocumented, and copying the two sentences added here would close the gap.
Verified by: grepped for `FileInfo[] listFiles` across `api/`, `core/` and
`catalogs/` - `FilesetFileOps:42` is the sole declaration, and it is unchanged
in this PR.
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java:
##########
@@ -1796,11 +1796,18 @@ private String failureCategory(Throwable throwable) {
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.
+ // Backslashes can be interpreted as separators by a client on a different
operating system.
+ Preconditions.checkArgument(
+ subPath.indexOf('\\') < 0 && subPath.indexOf('\0') < 0,
+ "subPath must not contain backslashes or null characters");
Review Comment:
[Question] Is breaking sub paths that contain a backslash an accepted cost?
A backslash is a legal character in POSIX file names, HDFS paths and S3/GCS
object keys, and before this change such a path was addressable:
`getFileLocation` only concatenated, so `/dir\name.txt` resolved to the key
`prefix/dir\name.txt`. After this change every `getFileLocation`/`listFiles`
call for such an object fails with HTTP 400, and there is no way to reach it -
including through GVFS, where `BaseGVFSOperations#getActualFilePath`
(clients/filesystem-hadoop3/src/main/java/org/apache/gravitino/filesystem/hadoop/BaseGVFSOperations.java:526)
hands the sub path through verbatim because `Path.WINDOWS` is false on a Linux
client, so the backslash survives.
Note that the backslash alone cannot cause server-side traversal: on a Linux
server `/dir\..\outside` is one literal segment and the new normalized-ancestor
check already keeps it inside the storage location. The ban only defends a
client that would re-split on `\`, which is the right instinct, but it is a
silent data-access regression for existing filesets rather than a security fix.
If the trade-off is intended, please call it out explicitly in the PR's
user-facing-change section (today it only says "unsupported sub-paths") and in
the release notes, so operators with such keys learn about it before upgrading.
If it is not worth the break, the boundary check on its own is already
sufficient server-side.
Verified by: read the pre-change concatenation in `git diff
origin/main...HEAD` for this method, traced `Path(String)`/`normalizePath` to
confirm `\` is not a separator when `Path.WINDOWS` is false, and read the GVFS
sub-path derivation at the line cited above.
##########
docs/open-api/filesets.yaml:
##########
@@ -235,6 +247,8 @@ paths:
responses:
"200":
$ref: "#/components/responses/FileInfoListResponse"
+ "400":
+ $ref: "#/components/responses/InvalidSubPathErrorResponse"
Review Comment:
[Nit] Adding `400` here makes the neighbouring `404` description
self-contradictory.
Line 253 still reads "the target fileset or subPath or locationName does not
exist", but a sub path that simply does not exist has never produced a 404 on
this endpoint: `FilesetCatalogOperations#listFiles` throws
`IllegalArgumentException`
(catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java:421-423,
"Path %s does not exist in fileset %s"), which `FilesetExceptionHandler` turns
into 400. The PR's own `testListFilesCannotReachSiblingSentinel` relies on this
for the `/../missing` case.
Since this PR is specifically documenting the error responses, suggest
narrowing the `404` description to the fileset and the location name, and
extending the `400` description to cover a non-existent sub path alongside an
invalid one.
Verified by: read the throw site and
`ExceptionHandlers$FilesetExceptionHandler` in this checkout;
`IllegalArgumentException` is the first branch and returns
`Utils.illegalArguments`.
--
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]
