github-actions[bot] commented on code in PR #69526:
URL: https://github.com/apache/doris/pull/69526#discussion_r4361423895
##########
be/src/io/fs/local_file_system.cpp:
##########
@@ -471,28 +471,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));
+ std::string secure_path;
+ RETURN_IF_ERROR(canonicalize(dir, &secure_path));
std::string canonical_path;
- RETURN_IF_ERROR(canonicalize(absolute_path, &canonical_path));
- if (!contain_path(dir, canonical_path)) {
- return Status::InvalidArgument("file path is not allowed: {}",
canonical_path);
+ const auto status = canonicalize(Path(secure_path) / local_path,
&canonical_path);
+ // A glob result can reveal external names even when resolution fails on a
dangling symlink.
+ // Reject both failed resolution and out-of-root targets without exposing
expanded names.
+ if (!status.ok() || !contain_path(secure_path, canonical_path)) {
+ return Status::InvalidArgument("file path is not allowed");
}
-
- *full_path = canonical_path;
+ *full_path = std::move(canonical_path);
return Status::OK();
}
Status LocalFileSystem::safe_glob(const std::string& path,
std::vector<FileInfo>* res) {
if (path.find("..") != std::string::npos) {
return Status::InvalidArgument("can not contain '..' in path");
}
- std::string full_path = config::user_files_secure_path + "/" + path;
+ // An empty root would turn the prefix below into "/" and allow any local
file.
+ if (config::user_files_secure_path.empty()) {
+ return Status::InvalidArgument("user_files_secure_path must not be
empty");
+ }
+ std::string secure_path;
+ RETURN_IF_ERROR(canonicalize(config::user_files_secure_path,
&secure_path));
+ std::string full_path = secure_path + "/" + path;
Review Comment:
[P2] Escape the canonical root before glob expansion
A configured root such as `/srv/user-files` can be a symlink to
`/mnt/user_files[prod]`. With `enable_local_tvf=true` and `file_path=*.csv`,
this change sends `/mnt/user_files[prod]/*.csv` to `glob()`, which interprets
`[prod]` as a character class and returns no matches even when `inside.csv`
exists. Previously glob used the literal `/srv/user-files` prefix and the read
succeeded. Keep the canonical root for containment, but escape its glob
metacharacters before prefixing the user pattern, and add a symlink-root
regression case.
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/LocalTableValuedFunction.java:
##########
@@ -63,6 +64,11 @@ public class LocalTableValuedFunction extends
ExternalFileTableValuedFunction {
private boolean sharedStorage = false;
public LocalTableValuedFunction(Map<String, String> properties) throws
AnalysisException {
+ if (!Config.enable_local_tvf) {
Review Comment:
[P2] Apply the LOCAL startup gate to the write sink too
With `enable_local_tvf=false`, an ADMIN can still run `INSERT INTO
local("file_path"="/tmp/local_optin_", "backend_id"="1", "format"="csv") SELECT
1` against a live backend and create a local CSV file. That path goes through
`InsertIntoTVFCommand -> BindSink.bindTVFTableSink -> TVFTableSink ->
VTVFTableWriter` and never constructs this class, so the new constructor check
is bypassed. The config description and release note promise that LOCAL TVF is
disabled by default, but currently only reads are disabled. Apply the startup
check to local sink binding as well and cover INSERT/EXPLAIN INSERT with the
flag off.
--
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]
