mchades commented on code in PR #13721:
URL: https://github.com/apache/gravitino/pull/13721#discussion_r4244783683
##########
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:
Removed the blanket backslash rejection while retaining the NUL,
parent-segment, and normalized-boundary checks. Added regression coverage for
literal backslashes in location lookup and file listing, and updated the API
docs.
##########
docs/open-api/filesets.yaml:
##########
@@ -235,6 +247,8 @@ paths:
responses:
"200":
$ref: "#/components/responses/FileInfoListResponse"
+ "400":
+ $ref: "#/components/responses/InvalidSubPathErrorResponse"
Review Comment:
Updated the 404 description and clarified that file listing also returns 400
for a nonexistent sub-path. The existence condition is explicitly limited to
file listing.
##########
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:
Documented the sub-path semantics and IllegalArgumentException on
FilesetFileOps.listFiles, including nonexistent paths. The wording matches the
updated validation rules.
##########
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:
Removed the two redundant filesystem-access assertions from the
getFileLocation tests and retained the one in the listFiles test, where it
verifies rejection before filesystem access.
--
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]
