jerryshao commented on code in PR #13721:
URL: https://github.com/apache/gravitino/pull/13721#discussion_r4245334783
##########
docs/open-api/filesets.yaml:
##########
@@ -550,6 +564,19 @@ components:
}
responses:
+ InvalidSubPathErrorResponse:
+ description: >-
+ The sub path does not satisfy the fileset path requirements.
+ For file listing, this response also applies when the sub path does
not exist.
Review Comment:
[Nit] This listing-only sentence also shows up on the location endpoint.
`InvalidSubPathErrorResponse` is referenced by both operations
(filesets.yaml:195-196 for `/location` and :250-251 for `/files`), and the spec
is OpenAPI 3.0.3 (docs/open-api/openapi.yaml:19), where a `description` written
next to a `$ref` is ignored. So a reader of `GET .../location` is told "For
file listing, this response also applies when the sub path does not exist",
which does not apply to that endpoint - `getFileLocation` never touches a
filesystem and so never reports a missing path.
Suggest either keeping the shared component generic and stating the
existence condition in the `files` endpoint's own `sub_path` description
(filesets.yaml:235-240, already being reworded in this PR), or defining a
second response component for the listing endpoint.
Verified by: read both `$ref` sites and the component in this checkout, and
confirmed the spec version in docs/open-api/openapi.yaml; `yaml.safe_load` on
filesets.yaml parses and the component resolves.
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java:
##########
@@ -1796,11 +1796,16 @@ 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.
+ Preconditions.checkArgument(
+ subPath.indexOf('\0') < 0, "subPath must not contain null characters");
Review Comment:
[Question] With the blanket backslash ban gone, the normalized-ancestor
check is now the only defense - which is right server-side, but the accepted
sub path is still echoed back verbatim.
On a POSIX server this is safe: `Path.WINDOWS` is false, so `\` is an
ordinary character, `/dir\..\outside` is one literal segment, and the boundary
check keeps it inside the storage location. The residual case is the consumer
of the returned string. `getFileLocation` now returns, by design and asserted
in this PR, `s3a://bucket/prefix/..\outside`
(TestFilesetCatalogOperations.java:4245) and `s3a://bucket/prefix/\..\outside`
(:4246). Any client that treats `\` as a separator - a Hadoop `Path` built on a
Windows JVM, where `normalizePath` rewrites `\` to `/` - collapses those to
`s3a://bucket/outside`, i.e. outside the location the server just validated.
GVFS reaches the same code through `BaseGVFSOperations#getActualFilePath`
(clients/filesystem-hadoop3/src/main/java/org/apache/gravitino/filesystem/hadoop/BaseGVFSOperations.java:526),
which passes the sub path through unchanged.
The same hole exists for a segment that another layer trims, e.g. `/dir/..
/outside`: `.. ` is not equal to `..`, so the segment loop at line 1803 lets it
through and the boundary check sees a literal directory name.
Two questions, either answer is fine by me:
1. Is the Windows-client case simply out of scope? If so it is worth one
sentence in the PR's user-facing-change section, since the earlier discussion
concluded the opposite way.
2. If you want to keep it closed without losing literal names, a narrower
guard is possible: reject a sub path that has a `..` segment after splitting on
both `/` and `\`. `dir\name.txt` and `file\name.txt` stay addressable (the
cases this commit restored), while only the traversal-shaped `..\` spellings
are refused.
Verified by: read the current check at
FilesetCatalogOperations.java:1799-1806 and the boundary walk in
`isWithinStorageLocation`/`normalizeLocationPath` in this checkout; read the
new expectations at TestFilesetCatalogOperations.java:4239-4248; read the GVFS
sub-path derivation at the line cited above. Hadoop's `Path.normalizePath` only
substitutes `\` when `Path.WINDOWS` is true, which the new test acknowledges
with `Assumptions.assumeFalse(Path.WINDOWS, ...)` at
TestFilesetCatalogOperations.java:2120.
--
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]
