github-actions[bot] commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4361447979
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/S3TableValuedFunction.java:
##########
@@ -89,6 +106,32 @@ public S3TableValuedFunction(Map<String, String>
properties, List<TBrokerFileSta
}
+ private static boolean isRemoteLanceUri(String path) {
+ try {
+ URI uri = new URI(path == null ? "" : path);
+ String scheme = uri.getScheme();
+ String authority = uri.getRawAuthority();
+ if (scheme == null ||
!LANCE_REMOTE_SCHEMES.contains(scheme.toLowerCase(Locale.ROOT))
+ || uri.isOpaque()) {
+ return false;
+ }
+ if (authority == null || authority.isEmpty()) {
+ // GooseFS can resolve its remote master from storage options
or configuration.
+ return "goosefs".equalsIgnoreCase(scheme) && uri.getRawPath()
!= null
+ && uri.getRawPath().startsWith("/");
+ }
+ // Lance falls back to a local path if Rust's URL parser rejects
the URI. Java's URI
+ // accepts registry authorities with invalid ports, which Rust
rejects. Validate the
+ // host/port separately without disallowing legitimate bucket
names with underscores.
+ String host =
HostAndPort.fromString(authority.substring(authority.lastIndexOf('@') + 1))
Review Comment:
[P1] Reject signed-zero ports before Lance falls back to local files
`HostAndPort.fromString` accepts `bucket:-0`: [Guava parses the port as
integer
zero](https://github.com/google/guava/blob/v33.2.1/guava/src/com/google/common/net/HostAndPort.java#L190-L203).
Java `URI` also accepts this authority, so `s3("format"="lance",
"fs.broker.support"="true", "uri"="s3://bucket:-0/../../dataset.lance")` passes
this gate. Rust's [port parser rejects the
minus](https://github.com/servo/rust-url/blob/v2.5.8/url/src/parser.rs#L1114-L1132),
and [Lance then falls back to a local
path](https://github.com/lance-format/lance/blob/v12.0.0/rust/lance-io/src/object_store.rs#L516-L540);
lexical normalization resolves this example to `<FE cwd>/dataset.lance`. An
ordinary user can therefore open an existing FE-local Lance dataset despite
`enable_local_tvf=false` and without ADMIN/OPERATOR. Matching BE-local datasets
also bypass containment because ranges remain `FILE_S3`. Reject signed port
spellings, including negative zero, or validate with the SDK parser, and cover
th
is Broker passthrough case.
##########
be/src/io/fs/local_file_system.cpp:
##########
@@ -455,28 +455,48 @@ const std::shared_ptr<LocalFileSystem>&
global_local_filesystem() {
Status LocalFileSystem::canonicalize_local_file(const std::string& dir,
const std::string& file_path,
std::string* full_path) {
- const std::string absolute_path = dir + "/" + file_path;
+ // An empty root would turn a relative request into an unrestricted
absolute path.
+ if (dir.empty()) {
+ return Status::InvalidArgument("user_files_secure_path must not be
empty");
+ }
+ // Reject embedded NULs before passing paths to filesystem APIs that use C
strings.
+ if (file_path.empty() || file_path.find('\0') != std::string::npos) {
+ return Status::InvalidArgument("local file path must not be empty or
contain NUL");
+ }
+ Path local_path;
+ // Convert file: URIs to filesystem paths and reject remote schemes before
any reader opens them.
+ RETURN_IF_ERROR(convert_to_abs_path(file_path, local_path));
Review Comment:
[P2] Decode file-URI paths before canonical containment checks
For an enabled, authorized LOCAL Lance read with
`user_files_secure_path=/secure`,
`file_path=file:///secure/sample%20data.lance` previously opened
`/secure/sample data.lance`: the [pinned Lance file provider uses
`Url::to_file_path`](https://github.com/lance-format/lance/blob/68c12dfd7efe02f90ad2d7f3a239a7eb64884e57/rust/lance-io/src/object_store/providers/local.rs#L147-L160).
This new call routes the URI through `convert_to_abs_path`, which strips the
scheme/authority without decoding `%20`, so canonicalization instead checks
`/secure/sample%20data.lance`. The valid dataset now fails schema discovery; if
both directory spellings contain datasets, schema discovery and scanning
silently select the percent-named dataset instead. Decode `file:` URI paths
before canonicalization while preserving literal plain-path semantics, and add
escaped-space/percent collision cases to both schema and scan coverage.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
