You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by "OIiveirra (via GitHub)" <gi...@apache.org> on 2026/10/23 09:45:19 UTC
[PR] [fix](fe) Disable LOCAL TVF by default (branch-4.1) [doris]
OIiveirra opened a new pull request, #69528:
URL: https://github.com/apache/doris/pull/69528
### What problem does this PR solve?
Issue Number: N/A
Related PR: #69526
Problem Summary: LOCAL TVF and FILE TVF with local storage currently remains available without explicit startup opt-in. Add `enable_local_tvf=false` and reject use at the LOCAL constructor before property parsing, backend selection, or file access. Setting it to true restores this branch's existing behavior.
## Summary
```text
LOCAL / FILE-local → startup opt-in check → existing processing
```
Dedicated regression deployments opt in to retain their existing LOCAL coverage.
### Release note
LOCAL TVF and FILE TVF with local storage is disabled by default. Set `enable_local_tvf=true` in `fe.conf` on every query-serving FE and restart those FEs to restore the existing behavior. The setting is not runtime-mutable.
## Evidence
- **Before:** LOCAL construction proceeded directly into existing processing.
- **After:** Disabled LOCAL/FILE-local access is rejected before file-service interaction; explicit opt-in reaches existing processing. Non-local FILE/S3 delegation remains available.
- 7 focused FE unit tests passed, with zero failures or skips.
- Focused Checkstyle passed for the changed Java files.
- Private branch-4.1 FE: 22 real SQL checks passed across default-disabled and restart-enabled profiles (LOCAL/FILE, CSV/Lance, SELECT/EXPLAIN, ordinary/admin users, and startup-only configuration); the private FE was stopped.
- SQL regression coverage added for SELECT and EXPLAIN under either configuration state; the Groovy runner was not executed locally.
### Check List (For Author)
- Test:
- [x] Unit Test
- [x] Manual test (private FE SQL validation described above)
- Behavior changed:
- [x] Yes. Local TVF access requires startup opt-in.
- Does this need documentation:
- [x] Yes. Default and opt-in are documented in the release note above.
## Merge Danger
**Door:** Two-way; explicit startup opt-in restores existing behavior.
**Blast Radius:** LOCAL TVF consumers, including FILE with local storage.
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6368402795
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6381426208
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4366573408
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/LocalTableValuedFunction.java:
##########
@@ -135,6 +143,38 @@ private void getFileListFromBackend() throws AnalysisException {
}
}
+ /** Apply LOCAL admission to sinks that select the built-in local JNI writer as well. */
+ public static void checkSinkEnabled(String tvfName, Map<String, String> properties) {
+ String writerClass = properties.getOrDefault("writer_class", "");
+ // JNI consumes a C string, and ScannerLoader accepts slash-separated class names.
+ int nul = writerClass.indexOf('\0');
+ if (nul >= 0) {
+ writerClass = writerClass.substring(0, nul);
+ }
+ if ("local".equalsIgnoreCase(tvfName)
Review Comment:
Fixed in de692c0306f780a7a327f53d89700aa77182c753. The existing shared gate now checks the native HDFS NameNode and selected Hadoop provider before analysis; final sink binding rechecks normalized backend properties, including configuration resources. FE cleanup checks its actual storage binding and path/default-filesystem selection before FileSystemFactory is called, including when a nominal S3 sink selects Hadoop storage for cleanup.
The checks cover `file:///`, selected LocalFileSystem/RawLocalFileSystem, Hadoop key trimming and canonical/legacy default-FS aliases. Indirect filesystem selectors require the existing startup opt-in because aliases and BE runtime configuration can change their effective values. Explicit remote filesystems and unselected `fs.file.impl` settings remain permitted; enabled requests retain the existing Hadoop implementation/configuration.
All 45 selected FE tests passed with fresh XML and unchanged sources. Cases cover INSERT/EXPLAIN, direct binding, normalized resources, disabled/enabled and remote controls, plus global LOAD with cleanup enabled. Mock assertions verify no filesystem-factory or executor interaction on rejection; no real directory deletion was performed. Java Checkstyle passed. This closes the original LOCAL default-off requirement without changing BE/Lance or global storage code.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4370544042
##########
be/src/io/fs/local_file_system.cpp:
##########
@@ -452,31 +452,206 @@ const std::shared_ptr<LocalFileSystem>& global_local_filesystem() {
return local_fs;
}
+Status LocalFileSystem::parse_local_path(const std::string& file_path, Path& local_path) {
+ // 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");
+ }
+ // Match URL preprocessing only for actual file URIs; bare paths keep literal whitespace.
+ const auto is_c0_or_space = [](unsigned char c) { return c <= 0x20; };
+ size_t begin = 0;
+ size_t end = file_path.size();
+ while (begin < end && is_c0_or_space(file_path[begin])) {
+ ++begin;
+ }
+ while (end > begin && is_c0_or_space(file_path[end - 1])) {
+ --end;
+ }
+ std::string uri_input;
+ for (size_t i = begin; i < end; ++i) {
+ if (file_path[i] != '\t' && file_path[i] != '\r' && file_path[i] != '\n') {
+ uri_input += file_path[i];
+ }
+ }
+ const bool is_file_uri = uri_input.size() >= 5 &&
+ (uri_input[0] == 'f' || uri_input[0] == 'F') &&
+ (uri_input[1] == 'i' || uri_input[1] == 'I') &&
+ (uri_input[2] == 'l' || uri_input[2] == 'L') &&
+ (uri_input[3] == 'e' || uri_input[3] == 'E') && uri_input[4] == ':';
+ std::string local_input = is_file_uri ? std::move(uri_input) : file_path;
+ if (is_file_uri) {
+ local_input.replace(0, 4, "file");
+ // File URLs treat raw backslashes as separators, including before the authority.
+ // Do this before conversion; percent-encoded backslashes must remain literal.
+ for (size_t i = 5; i < local_input.size(); ++i) {
+ if (local_input[i] == '\\') {
+ local_input[i] = '/';
+ }
+ }
+ }
+ const auto hex_digit = [](char c) -> int {
+ if (c >= '0' && c <= '9') return c - '0';
+ if (c >= 'a' && c <= 'f') return c - 'a' + 10;
+ if (c >= 'A' && c <= 'F') return c - 'A' + 10;
+ return -1;
+ };
+ const auto percent_decode = [&](const std::string& encoded) {
+ std::string decoded;
+ decoded.reserve(encoded.size());
+ for (size_t i = 0; i < encoded.size(); ++i) {
+ if (encoded[i] == '%' && i + 2 < encoded.size()) {
+ const int high = hex_digit(encoded[i + 1]);
+ const int low = hex_digit(encoded[i + 2]);
+ if (high >= 0 && low >= 0) {
+ decoded += static_cast<char>((high << 4) | low);
+ i += 2;
+ continue;
+ }
+ }
+ decoded += encoded[i];
+ }
+ return decoded;
+ };
+ const auto is_drive = [](const std::string& segment) {
+ return segment.size() == 2 &&
+ ((segment[0] >= 'a' && segment[0] <= 'z') ||
+ (segment[0] >= 'A' && segment[0] <= 'Z')) &&
+ (segment[1] == ':' || segment[1] == '|');
+ };
+ bool empty_file_host = false;
+ if (is_file_uri && local_input.compare(5, 2, "//") == 0) {
+ const auto host_end = local_input.find_first_of("/?#", 7);
+ auto host = local_input.substr(7, host_end == std::string::npos ? host_end : host_end - 7);
+ if (is_drive(host)) {
+ // The URL parser treats a drive in the host position as the first path segment.
+ local_input.erase(5, 2);
+ } else {
+ host = percent_decode(host);
+ std::transform(host.begin(), host.end(), host.begin(), [](unsigned char c) {
+ return c >= 'A' && c <= 'Z' ? c + ('a' - 'A') : c;
+ });
+ empty_file_host = host.empty() || host == "localhost";
+ }
+ }
+ // Convert file: URIs to filesystem paths and reject remote schemes before any reader opens them.
+ RETURN_IF_ERROR(convert_to_abs_path(local_input, local_path));
Review Comment:
[P2] Preserve SDK fallback identity for malformed file authorities
With LOCAL enabled, an ADMIN/OPERATOR, and both BE cwd and user_files_secure_path set to /secure, consider file_path='file://host:80/secure/data.lance'. The [pinned URL parser rejects a colon in a file host](https://github.com/servo/rust-url/blob/v2.5.8/idna/src/uts46.rs#L343-L351), so [Lance falls back to treating the entire original string as a cwd-relative filesystem path](https://github.com/lance-format/lance/blob/68c12dfd7efe02f90ad2d7f3a239a7eb64884e57/rust/lance-io/src/object_store.rs#L528-L545): /secure/file:/host:80/secure/data.lance. This call instead strips host:80 without validating it and resolves /secure/data.lance; resolve_dataset_uri sees success and skips the SDK parser. If those two in-root datasets contain different rows, schema discovery and scanning now silently select the other dataset. Preserve the SDK's parse/fallback identity before containment, or reject the malformed authority instead of rewriting it, and add a distinguishable-dataset collision case.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4366160077
##########
be/src/io/fs/local_file_system.cpp:
##########
@@ -455,28 +455,89 @@ 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));
+ if (file_path.rfind("file:", 0) == 0) {
+ // file:a%20b names "a b", but a bare path containing "%20" must stay literal.
+ // Decode once, preserve '+' and invalid escapes, matching Lance's file provider.
+ auto encoded = local_path.string();
+ // Query and fragment are URI components, not part of the filesystem path.
+ encoded = encoded.substr(0, encoded.find_first_of("?#"));
+ const auto hex_digit = [](char c) -> int {
+ if (c >= '0' && c <= '9') return c - '0';
+ if (c >= 'a' && c <= 'f') return c - 'a' + 10;
+ if (c >= 'A' && c <= 'F') return c - 'A' + 10;
+ return -1;
+ };
+ std::string decoded;
+ decoded.reserve(encoded.size());
+ for (size_t i = 0; i < encoded.size(); ++i) {
+ if (encoded[i] == '%' && i + 2 < encoded.size()) {
+ const int high = hex_digit(encoded[i + 1]);
+ const int low = hex_digit(encoded[i + 2]);
+ if (high >= 0 && low >= 0) {
+ decoded += static_cast<char>((high << 4) | low);
+ i += 2;
+ continue;
+ }
+ }
+ decoded += encoded[i];
+ }
+ // %00 must not truncate the path when canonicalization reaches C filesystem APIs.
+ if (decoded.find('\0') != std::string::npos) {
+ return Status::InvalidArgument("local file path must not contain NUL");
+ }
+ local_path = std::move(decoded);
+ }
+ 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);
Review Comment:
The latest review's bare-path variant is now fixed in 0c4fa140e43bd3be71e097713f0cbd3f25d38c8c. LOCAL Lance/version-zero paths use the pinned PathAbs order: anchor relative paths to the canonical secure root, remove bare dot segments lexically, then resolve symlinks and enforce containment.
The earlier Lance bare-path test expectation was incorrect and has been changed from the nested dataset (42) to the root dataset (7), with absolute and relative controls. Ordinary LocalFS/glob physical-path semantics remain unchanged. Empty/NUL paths, including a NUL before a removable parent segment, remain rejected. All 13 selected BE tests passed.
##########
be/src/io/fs/local_file_system.cpp:
##########
@@ -455,28 +455,142 @@ 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");
+ }
+ // URI schemes are ASCII case-insensitive; do not alter spelling in bare paths.
+ const bool is_file_uri = file_path.size() >= 5 &&
+ (file_path[0] == 'f' || file_path[0] == 'F') &&
+ (file_path[1] == 'i' || file_path[1] == 'I') &&
+ (file_path[2] == 'l' || file_path[2] == 'L') &&
+ (file_path[3] == 'e' || file_path[3] == 'E') && file_path[4] == ':';
+ std::string local_input = file_path;
+ if (is_file_uri) {
+ local_input.replace(0, 4, "file");
+ // File URLs treat raw backslashes as separators, including before the authority.
+ // Do this before conversion; percent-encoded backslashes must remain literal.
+ for (size_t i = 5; i < local_input.size(); ++i) {
+ if (local_input[i] == '\\') {
+ local_input[i] = '/';
+ }
+ }
+ }
+ 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(local_input, local_path));
Review Comment:
Fixed in 0c4fa140e43bd3be71e097713f0cbd3f25d38c8c. URL whitespace/control preprocessing now occurs before recognizing an actual file URI. Raw surrounding C0/space and URL tab/newline preprocessing follow the pinned parser; literal bare-path whitespace and percent-encoded spaces are preserved. Existing NUL rejection and canonical containment remain in place.
Tests distinguish normal row 7, a literal-space dataset with row 99, and the other path-identity collisions, checking schema and actual BE scans. Raw-space file URIs select row 7; `%20` URIs and bare paths with a literal trailing space select row 99. All 13 selected filesystem/Lance tests passed with fresh XML and unchanged source.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6380544307
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4368441150
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/LocalTableValuedFunction.java:
##########
@@ -135,6 +143,38 @@ private void getFileListFromBackend() throws AnalysisException {
}
}
+ /** Apply LOCAL admission to sinks that select the built-in local JNI writer as well. */
+ public static void checkSinkEnabled(String tvfName, Map<String, String> properties) {
+ String writerClass = properties.getOrDefault("writer_class", "");
+ // JNI consumes a C string, and ScannerLoader accepts slash-separated class names.
+ int nul = writerClass.indexOf('\0');
+ if (nul >= 0) {
+ writerClass = writerClass.substring(0, nul);
+ }
+ if ("local".equalsIgnoreCase(tvfName)
Review Comment:
Fixed in 02f2ddded5c1f13c9a59cca1b04b02c582f4c831. Disabled LOCAL writes now reach the actual Hadoop consumer guard, including implicit core-site/hdfs-site provider overrides. The runtime registers defaults before freezing its configuration and rejects local, unknown and overloaded routes before provider initialization. BE also requires the matching patched runtime and binds cached handlers to exact parameters/authority and the captured Context. The startup opt-in for explicitly enabled writes is retained.
Validation: final standard FE/BE targeted suites passed with fresh XML and unchanged source fingerprints (100 FE / 15 BE). Real pinned libhdfs controls reject inherited local providers, overloaded providers and inherited local child mounts before file IO; cold-JVM controls cover resource registration and all default-URI entrypoints. No storage-data read/write/delete was used in these runtime controls. The same mechanism is synchronized across #69526, #69527 and #69528.
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/LocalTableValuedFunction.java:
##########
@@ -135,6 +150,332 @@ private void getFileListFromBackend() throws AnalysisException {
}
}
+ /** Apply LOCAL admission to the effective sink before planning or filesystem side effects. */
+ public static void checkSinkEnabled(String tvfName, Map<String, String> properties) {
+ if ("local".equalsIgnoreCase(tvfName) || isLocalWriter(properties.getOrDefault("writer_class", ""))) {
+ checkEnabled();
+ }
+ if ("hdfs".equalsIgnoreCase(tvfName)) {
+ if (!properties.containsKey("writer_class")) {
+ checkHdfsSinkEnabled(properties);
+ }
+ if (Boolean.parseBoolean(properties.getOrDefault("delete_existing_files", "false"))) {
+ checkHadoopFileSystemEnabled(
+ BrokerUtil.extractParentDirectory(properties.getOrDefault("file_path", "")), properties);
+ }
+ }
+ }
+
+ private static boolean isLocalWriter(String writer) {
+ // ScannerLoader accepts slashes, and JNI passes the class name as a C string.
+ return "org.apache.doris.writer.LocalFileJniWriter".equals(nativeStringPrefix(writer).replace('/', '.'));
+ }
+
+ /** Check the NameNode and provider used by the native FILE_HDFS writer. */
+ public static void checkHdfsSinkEnabled(Map<String, String> properties) {
+ checkHdfsSinkEnabled(properties.getOrDefault("file_path", ""), properties);
+ }
+
+ public static void checkHdfsSinkEnabled(String outputPrefix, Map<String, String> properties) {
+ String nameNode = nativeStringPrefix(properties.getOrDefault("fs.defaultFS", "")).trim();
+ if ("default".equals(nameNode)) {
+ // libhdfs delegates this selector to the BE's defaults, which may select local storage.
+ checkEnabled();
+ }
+ // libhdfs prefixes bare NameNodes with hdfs:// rather than using the output path's scheme.
+ checkHadoopSchemeEnabled(nameNode.contains("://") ? uriScheme(nameNode) : "hdfs", properties);
+ if (!Config.enable_local_tvf) {
+ try {
+ URI filesystem = new URI(nameNode.contains("://") ? nameNode : "hdfs://" + nameNode);
+ // BE appends a generated filename directly to this prefix, then convert_path
+ // removes the NameNode. Resolve its parent, not an exact mount-point filename.
+ String generated = outputPrefix + "doris-tvf-file";
+ String rawNameNode = properties.getOrDefault("fs.defaultFS", "");
+ if (!rawNameNode.isEmpty() && generated.contains(rawNameNode)) {
+ generated = generated.substring(rawNameNode.length());
+ }
+ if (!generated.startsWith("/")) {
+ generated = "/" + generated;
+ }
+ String path = hadoopPathUri(nativeStringPrefix(generated)).getPath();
+ path = path.substring(0, path.lastIndexOf('/') + 1);
+ if (path.length() > 1) {
+ path = path.substring(0, path.length() - 1);
+ }
+ checkViewFsEnabled(filesystem, path, properties, new HashSet<>());
+ } catch (URISyntaxException e) {
+ // Invalid NameNodes remain subject to the existing storage validation.
+ }
+ }
+ }
+
+ /** Check Hadoop's path/default-filesystem selection before FE-side cleanup. */
+ public static void checkHadoopFileSystemEnabled(String path, Map<String, String> properties) {
Review Comment:
Fixed in 02f2ddded5c1f13c9a59cca1b04b02c582f4c831. Hadoop TVF reads are guarded at FE listing and the actual BE consumer, including cached native connections and Avro schema/data readers. The actual frozen configuration resolves inherited/overloaded providers and ViewFS mounts; each real path is rechecked instead of trusting FE-only properties. S3A bucket-option and JobConf configuration copies preserve the request guard. Unmarked catalogs retain their previous behavior; local reads use constrained local().
Validation: final standard FE/BE targeted suites passed with fresh XML and unchanged source fingerprints (100 FE / 15 BE). Real pinned libhdfs controls reject inherited local providers, overloaded providers and inherited local child mounts before file IO; cold-JVM controls cover resource registration and all default-URI entrypoints. No storage-data read/write/delete was used in these runtime controls. The same mechanism is synchronized across #69526, #69527 and #69528.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4366036114
##########
be/src/io/fs/local_file_system.cpp:
##########
@@ -455,28 +455,89 @@ 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));
+ if (file_path.rfind("file:", 0) == 0) {
+ // file:a%20b names "a b", but a bare path containing "%20" must stay literal.
+ // Decode once, preserve '+' and invalid escapes, matching Lance's file provider.
+ auto encoded = local_path.string();
+ // Query and fragment are URI components, not part of the filesystem path.
+ encoded = encoded.substr(0, encoded.find_first_of("?#"));
+ const auto hex_digit = [](char c) -> int {
+ if (c >= '0' && c <= '9') return c - '0';
+ if (c >= 'a' && c <= 'f') return c - 'a' + 10;
+ if (c >= 'A' && c <= 'F') return c - 'A' + 10;
+ return -1;
+ };
+ std::string decoded;
+ decoded.reserve(encoded.size());
+ for (size_t i = 0; i < encoded.size(); ++i) {
+ if (encoded[i] == '%' && i + 2 < encoded.size()) {
+ const int high = hex_digit(encoded[i + 1]);
+ const int low = hex_digit(encoded[i + 2]);
+ if (high >= 0 && low >= 0) {
+ decoded += static_cast<char>((high << 4) | low);
+ i += 2;
+ continue;
+ }
+ }
+ decoded += encoded[i];
+ }
+ // %00 must not truncate the path when canonicalization reaches C filesystem APIs.
+ if (decoded.find('\0') != std::string::npos) {
+ return Status::InvalidArgument("local file path must not contain NUL");
+ }
+ local_path = std::move(decoded);
+ }
+ 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);
Review Comment:
Fixed in 71be77f48222334cf3311ed71ba9f4b6142043e4. Actual file URI dot segments, including encoded-dot forms, are normalized before percent decoding and before filesystem/symlink resolution. Empty URL segments and encoded separator ordering are preserved; bare paths keep their filesystem semantics. Canonical secure-root containment still runs afterward.
The new regression creates two distinguishable datasets (`root_only`/row 7 versus `nested_only`/row 42) and verifies schema, canonical dataset identity and actual scans. Before the fix it reproduced the wrong schema and row 42; afterward all 13 selected BE filesystem/Lance tests passed. Coverage includes missing intermediate directories, encoded dots/slashes, case-insensitive schemes, raw URI separators and bare-path controls.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6380540538
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4367751428
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/HadoopTvfAdmission.java:
##########
@@ -0,0 +1,502 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.tablefunction;
+
+import org.apache.doris.common.Config;
+import org.apache.doris.common.util.LocationPath;
+import org.apache.doris.nereids.exceptions.AnalysisException;
+
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * TVF-only admission for Hadoop consumers. Input properties have already been normalized by storage.
+ * This module neither loads Hadoop resources nor constructs a filesystem. Java strings and native
+ * C strings are projected separately before filesystem selection and mount-table interpretation.
+ */
+public final class HadoopTvfAdmission {
+ public enum Access {
+ NATIVE_WRITE,
+ JAVA_DELETE,
+ HADOOP_READ
+ }
+
+ private enum Provider {
+ REMOTE,
+ LOCAL,
+ MOUNT_TABLE,
+ DYNAMIC,
+ UNKNOWN
+ }
+
+ private static final Set<String> REMOTE_SCHEMES = new HashSet<>(Arrays.asList(
+ "hdfs", "s3a", "s3n", "oss", "ofs", "jfs", "abfs", "abfss", "wasb", "wasbs", "gs", "cosn"));
+ private static final Set<String> REMOTE_IMPLEMENTATIONS = new HashSet<>(Arrays.asList(
+ "org.apache.hadoop.hdfs.DistributedFileSystem",
+ "org.apache.hadoop.fs.s3a.S3AFileSystem",
+ "org.apache.hadoop.fs.azurebfs.AzureBlobFileSystem",
+ "org.apache.hadoop.fs.azurebfs.SecureAzureBlobFileSystem",
+ "org.apache.hadoop.fs.azure.NativeAzureFileSystem",
+ "org.apache.hadoop.fs.aliyun.oss.AliyunOSSFileSystem",
+ "com.aliyun.jindodata.oss.JindoOssFileSystem",
+ "org.apache.hadoop.fs.obs.OBSFileSystem",
+ "com.google.cloud.hadoop.fs.gcs.GoogleHadoopFileSystem"));
+
+ private final Access access;
+ private final Map<String, String> properties = new HashMap<>();
+
+ private HadoopTvfAdmission(Access access, Map<String, String> normalized, boolean nativeConsumer) {
+ this.access = access;
+ Set<String> nativeRoutingKeys = new HashSet<>();
+ for (Map.Entry<String, String> entry : normalized.entrySet()) {
+ // parse_properties / generateHdfsParam consumes the canonical NameNode before JNI.
+ if (nativeConsumer && "fs.defaultFS".equalsIgnoreCase(entry.getKey())) {
+ continue;
+ }
+ String key = (nativeConsumer ? nativeStringPrefix(entry.getKey()) : entry.getKey()).trim();
+ // Native writes first pass through StringCaseMap, before JNI truncates strings.
+ // Its comparator uses strncasecmp followed by the full UTF-8 byte length.
+ if (nativeConsumer && access == Access.NATIVE_WRITE && isRoutingKey(asciiCaseFold(key))) {
+ String identity = asciiCaseFold(nativeStringPrefix(entry.getKey())) + "\0"
+ + entry.getKey().getBytes(StandardCharsets.UTF_8).length;
+ if (!nativeRoutingKeys.add(identity)) {
+ reject();
+ }
+ }
+ String value = nativeConsumer ? nativeStringPrefix(entry.getValue()) : entry.getValue();
+ String previous = properties.put(key, value);
+ if (previous != null && !previous.equals(value) && isRoutingKey(key)) {
+ reject();
+ }
+ }
+ }
+
+ /** Reject local/unknown Hadoop TVF routes before listing, deletion, schema discovery or data IO. */
+ public static void check(Access access, String path, Map<String, String> normalized) {
+ if (access != Access.HADOOP_READ && Config.enable_local_tvf) {
+ return;
+ }
+ try {
+ if (access == Access.NATIVE_WRITE) {
+ String nameNode = nativeNameNode(access, normalized);
+ URI output = nativeFileUri(access, path + "doris-tvf-file", nameNode);
+ String parent = output.getPath();
+ parent = parent.substring(0, parent.lastIndexOf('/') + 1);
+ if (parent.length() > 1) {
+ parent = parent.substring(0, parent.length() - 1);
+ }
+ new HadoopTvfAdmission(access, normalized, true).checkNative(nameNode, parent);
+ } else {
+ HadoopTvfAdmission javaConsumer = new HadoopTvfAdmission(access, normalized, false);
+ // Project the Java consumer independently from native convert_path/JNI.
+ URI javaPath = hadoopPathUri(path.trim());
+ javaConsumer.checkRoute(javaPath, javaPath.getPath(), new HashSet<>());
+ if (access == Access.HADOOP_READ) {
+ // FileQueryScanNode sends this filesystem identifier in range.fs_name. BE
+ // falls back to hdfs_params.fs_name only when that range field is empty.
+ URI fileUri = LocationPath.of(path).getPath().toUri();
+ String nameNode = fileUri.getScheme() + "://" + fileUri.getAuthority();
+ if (nameNode.isEmpty()) {
+ nameNode = nativeNameNode(access, normalized);
+ }
+ HadoopTvfAdmission nativeConsumer = new HadoopTvfAdmission(access, normalized, true);
+ nativeConsumer.checkNativeRead(nameNode, path);
+ // Schema RPCs have no range.fs_name; they use THdfsParams.fs_name instead.
+ nativeConsumer.checkNativeRead(nativeNameNode(access, normalized), path);
+ }
+ }
+ } catch (URISyntaxException e) {
+ new HadoopTvfAdmission(access, normalized, false).reject();
+ }
+ }
+
+ private static String nativeNameNode(Access access, Map<String, String> normalized) {
+ String result = null;
+ for (Map.Entry<String, String> entry : normalized.entrySet()) {
+ if ("fs.defaultFS".equalsIgnoreCase(entry.getKey())) {
+ if (result != null && !result.equals(entry.getValue())) {
+ reject(access);
+ }
+ result = entry.getValue();
+ }
+ }
+ return result == null ? "" : result;
+ }
+
+ private void checkNative(String nameNode, String path) throws URISyntaxException {
+ nameNode = selector(nativeStringPrefix(nameNode)).trim();
+ if (nameNode.isEmpty() && access == Access.NATIVE_WRITE) {
+ // BE passes fs_name.c_str(), never NULL. libhdfs prefixes this empty string
+ // with hdfs://, whose URI construction fails before any filesystem is selected.
+ // Preserve existing analysis; execution reports the invalid NameNode as before.
+ return;
+ }
+ if ("default".equals(nameNode) || nameNode.isEmpty()) {
+ reject();
+ }
+ checkRoute(new URI(nameNode.contains("://") ? nameNode : "hdfs://" + nameNode),
+ path, new HashSet<>());
+ }
+
+ private void checkNativeRead(String nameNode, String path) throws URISyntaxException {
+ checkNative(nameNode, nativeFileUri(access, path, nameNode).getPath());
+ }
+
+ private List<URI> defaults() throws URISyntaxException {
+ List<URI> defaults = new ArrayList<>();
+ for (String key : Arrays.asList("fs.defaultFS", "fs.default.name")) {
+ String value = property(key);
+ if (value != null) {
+ value = selector(value).trim();
+ // FileSystem.fixName treats slash-free host[:port] strings as HDFS defaults.
+ defaults.add(new URI("local".equals(value) ? "file:///"
+ : (value.indexOf('/') < 0 ? "hdfs://" + value : value)));
+ }
+ }
+ if (defaults.isEmpty()) {
+ defaults.add(new URI("file:///"));
+ }
+ return defaults;
+ }
+
+ private void checkRoute(URI filesystem, String path, Set<String> activeTables) throws URISyntaxException {
+ if (filesystem.getScheme() == null) {
+ for (URI defaultFs : defaults()) {
+ checkViewFsEnabled(defaultFs, path, activeTables);
+ }
+ return;
+ }
+ if (filesystem.getAuthority() == null) {
+ boolean completed = false;
+ for (URI defaultFs : defaults()) {
+ if (filesystem.getScheme().equals(defaultFs.getScheme()) && defaultFs.getAuthority() != null) {
+ checkViewFsEnabled(defaultFs, path, activeTables);
+ completed = true;
+ }
+ }
+ if (completed) {
+ return;
+ }
+ }
+ checkViewFsEnabled(filesystem, path, activeTables);
+ }
+
+ private Provider provider(String scheme) {
+ if ("file".equalsIgnoreCase(scheme)) {
+ return Provider.LOCAL;
+ }
+ String implementation = property("fs." + scheme + ".impl");
+ if (implementation != null) {
+ implementation = selector(implementation).trim();
+ if ("org.apache.hadoop.fs.LocalFileSystem".equals(implementation)
+ || "org.apache.hadoop.fs.RawLocalFileSystem".equals(implementation)) {
+ return Provider.LOCAL;
+ }
+ if ("org.apache.hadoop.fs.viewfs.ViewFileSystem".equals(implementation)) {
+ return Provider.MOUNT_TABLE;
+ }
+ if ("org.apache.hadoop.fs.viewfs.ViewFileSystemOverloadScheme".equals(implementation)
+ || "org.apache.hadoop.hdfs.ViewDistributedFileSystem".equals(implementation)) {
+ return Provider.DYNAMIC;
+ }
+ return REMOTE_IMPLEMENTATIONS.contains(implementation) ? Provider.REMOTE : Provider.UNKNOWN;
+ }
+ if ("viewfs".equals(scheme)) {
+ return Provider.MOUNT_TABLE;
+ }
+ return REMOTE_SCHEMES.contains(scheme) ? Provider.REMOTE : Provider.UNKNOWN;
+ }
+
+ private void checkGlobTargets(String key, Set<String> activeTables) throws URISyntaxException {
+ for (String target : selector(property(key)).split(",", -1)) {
+ if (target.isEmpty()) {
+ reject();
+ }
+ URI uri = new URI(target);
+ // Listing can touch any descendant of a selected mount. Reuse the same route
+ // interpreter for nested tables instead of assuming their root is representative.
+ String path = uri.getPath() == null ? "/" : uri.getPath();
+ checkRoute(uri, (path.endsWith("/") ? path : path + "/") + "*", activeTables);
+ }
+ }
+
+ private void checkViewFsEnabled(URI filesystem, String path, Set<String> activeTables) {
+ String scheme = filesystem.getScheme();
+ Provider provider = provider(scheme);
+ if (provider == Provider.LOCAL || provider == Provider.UNKNOWN || provider == Provider.DYNAMIC) {
+ reject();
+ }
+ if (provider != Provider.MOUNT_TABLE) {
+ return;
+ }
+ if (!path.startsWith("/")) {
+ // Java resolves relative paths against the selected filesystem's working directory.
+ // Its user/home configuration is not a proven absolute mount-table path here.
+ reject();
+ }
+ String table = filesystem.getAuthority();
+ String ignorePort = property("fs.viewfs.ignore.port.in.mount.table.name");
+ if (ignorePort != null && Boolean.parseBoolean(selector(ignorePort).trim())
+ && table != null && filesystem.getPort() != -1) {
+ table = filesystem.getHost();
+ if (table == null) {
+ reject();
+ }
+ }
+ if (table == null || table.isEmpty()) {
+ table = property("fs.viewfs.mounttable.default.name.key");
+ table = table == null ? "default" : selector(table);
+ }
+ // Nested ViewFS cycles or arbitrarily deep delegation cannot establish remote storage.
+ if (activeTables.size() >= 32 || !activeTables.add(table)) {
+ reject();
+ }
+ try {
+ String prefix = "fs.viewfs.mounttable." + table + ".";
+ String source = "";
+ List<String> selectedKeys = new ArrayList<>();
+ List<String> mountSources = new ArrayList<>();
+ String fallback = null;
+ String rootMerge = null;
+ for (String key : properties.keySet()) {
+ if (!key.startsWith(prefix)) {
+ continue;
+ }
+ String entry = key.substring(prefix.length());
+ if (entry.equals("linkFallback")) {
+ fallback = key;
+ continue;
+ }
+ if (entry.equals("linkMergeSlash")) {
+ rootMerge = key;
+ continue;
+ }
+ if (entry.startsWith("linkRegex")) {
+ // Regex interception rewrites runtime paths before ordinary mount resolution.
+ reject();
+ }
+ int slash = entry.indexOf('/');
+ if (slash < 0 || !(entry.startsWith("link.") || entry.startsWith("linkMerge.")
+ || entry.startsWith("linkNfly."))) {
+ continue;
+ }
+ String candidate = entry.substring(slash);
+ // InodeTree splits source components and discards trailing empty components.
+ while (candidate.length() > 1 && candidate.endsWith("/")) {
+ candidate = candidate.substring(0, candidate.length() - 1);
+ }
+ mountSources.add(candidate);
+ if (!(path.equals(candidate) || path.startsWith(candidate + "/"))) {
+ continue;
+ }
+ if (candidate.length() > source.length()) {
+ source = candidate;
+ selectedKeys.clear();
+ }
+ if (candidate.equals(source)) {
+ selectedKeys.add(key);
+ }
+ }
+ if (access == Access.HADOOP_READ && hasGlob(path)) {
+ String literal = path.substring(0, firstGlob(path));
+ int slash = literal.lastIndexOf('/');
+ literal = slash < 0 ? "/" : literal.substring(0, slash + 1);
+ for (String key : properties.keySet()) {
+ if (!key.startsWith(prefix)) {
+ continue;
+ }
+ String entry = key.substring(prefix.length());
+ int sourceStart = entry.indexOf('/');
+ if (sourceStart >= 0 && (entry.startsWith("link.") || entry.startsWith("linkMerge.")
+ || entry.startsWith("linkNfly."))) {
+ String mount = entry.substring(sourceStart);
+ if (mount.startsWith(literal) || literal.startsWith(mount + "/")) {
+ checkGlobTargets(key, activeTables);
+ }
+ }
+ }
+ }
+ if (access == Access.HADOOP_READ && !hasGlob(path)) {
+ String subtree = path.endsWith("/") ? path : path + "/";
+ for (String key : properties.keySet()) {
+ if (key.startsWith(prefix)) {
+ String entry = key.substring(prefix.length());
+ int sourceStart = entry.indexOf('/');
+ if (sourceStart >= 0 && entry.substring(sourceStart).startsWith(subtree)) {
+ checkGlobTargets(key, activeTables);
+ }
+ }
+ }
+ }
+ if (rootMerge != null) {
+ selectedKeys.add(rootMerge);
+ source = "";
+ } else if (selectedKeys.isEmpty() && fallback != null) {
+ selectedKeys.add(fallback);
+ }
+ if (selectedKeys.isEmpty()) {
+ // Missing inline fallback does not prove absence of one in default resources.
+ // Keep rejecting unproven routes, including globs that cross mount names.
+ reject();
+ }
+ for (String key : selectedKeys) {
+ String targets = selector(property(key));
+ for (String target : targets.split(",", -1)) {
Review Comment:
[P2] Preserve commas in single-target ViewFS mount URIs
A regular `link./data=hdfs://namenode/base,archive` names one remote directory: [Hadoop InodeTree parses SINGLE links as one URI](https://github.com/apache/doris-thirdparty/blob/hadoop-3.4.2.2-for-doris/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/InodeTree.java). With `enable_local_tvf=false`, `INSERT INTO hdfs('file_path'='/data/prefix_', 'format'='csv', 'fs.defaultFS'='viewfs://cluster', 'fs.viewfs.mounttable.cluster.link./data'='hdfs://namenode/base,archive') SELECT 1` now fails with `LOCAL TVF is disabled`: this loop splits off `archive` and rejects its missing scheme. HDFS/FILE reads through the same mount also fail, even with LOCAL enabled. Preserve the complete URI for SINGLE/fallback/root-merge links, and apply comma-list parsing only to multi-target link types; `checkGlobTargets` needs the same distinction. Add remote comma-directory read/write/cleanup controls.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6398585557
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4366627104
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/S3TableValuedFunction.java:
##########
@@ -89,6 +107,43 @@ public S3TableValuedFunction(Map<String, String> properties, List<TBrokerFileSta
}
+ private static boolean isRemoteLanceUri(String path) {
+ try {
+ URI uri = new URI(path == null ? "" : path);
Review Comment:
[P2] Preserve SDK-supported spaces in remote Lance paths
With ordinary endpoint/region/credentials and a dataset stored under `sample data.lance`, `s3("uri"="s3://bucket/sample data.lance", "format"="lance", ...)` previously reached the remote dataset: `S3PropertyUtils.validateAndNormalizeUri` returns `s3://` paths unchanged, and Lance passes them to Rust's URL parser, which [percent-encodes spaces in path components](https://github.com/servo/rust-url/blob/v2.5.8/url/src/parser.rs#L1138-L1165). The new `java.net.URI` constructor throws on the raw space, so this guard rejects the query before metadata loading even though the SDK parses it successfully as remote storage. This also affects `file()` when it delegates to S3. Preserve SDK-supported path spelling while validating the scheme/authority against local fallback, and replace the new `s3://bucket/bad path` rejection expectation with positive raw-space/escaped-space metadata 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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6381420126
# BE UT Coverage Report
Increment line coverage `98.67% (148/150)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/0c4fa140e43bd3be71e097713f0cbd3f25d38c8c_0c4fa140e43bd3be71e097713f0cbd3f25d38c8c/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/0c4fa140e43bd3be71e097713f0cbd3f25d38c8c_0c4fa140e43bd3be71e097713f0cbd3f25d38c8c/report/index.html)
| Category | Coverage |
|-------------------|--------------------|
| Function Coverage | 61.48% (26987/43896) |
| Line Coverage | 46.17% (281419/609592) |
| Region Coverage | 41.79% (222672/532863) |
| Branch Coverage | 43.35% (103586/238933) |
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4369911685
##########
fe/be-java-extensions/hadoop-deps/src/main/java/org/apache/hadoop/fs/FileSystem.java:
##########
@@ -3622,13 +3633,670 @@ public static Class<? extends FileSystem> getFileSystemClass(String scheme,
* @return the initialized filesystem.
* @throws IOException problems loading or initializing the FileSystem
*/
+ // DORIS-PATCH: TVF policy is checked against the frozen configuration used by the consumer.
+ public static int getDorisTvfGuardVersion() {
+ return 1;
+ }
+
+ public static final class TvfGuard {
+ public static final String POLICY_KEY = "fs.doris.tvf.remote.paths";
+ public static final String NATIVE_POLICY = "native";
+ public static final String JAVA_READ_POLICY = "java-read";
+ public static final String ROOT_KEY = "fs.doris.tvf.root";
+ public static final String PATH_KEY = "fs.doris.tvf.path";
+ public static final String CONTEXT_KEY = "fs.doris.tvf.context";
+ private static final ThreadLocal<Context> LAST_NATIVE_CONTEXT = new ThreadLocal<>();
+
+ private TvfGuard() {
+ }
+
+ public static final class Rule {
+ private final String filesystem;
+ private final String path;
+ private final boolean descendants;
+
+ public Rule(String filesystem, String path, boolean descendants) {
+ this.filesystem = filesystem;
+ this.path = path;
+ this.descendants = descendants;
+ }
+ }
+
+ public static String encode(List<Rule> rules) {
+ if (FileSystem.getDorisTvfGuardVersion() != 1) {
+ throw new IllegalStateException("TVF Hadoop runtime policy version mismatch");
+ }
+ try {
+ java.io.ByteArrayOutputStream bytes = new java.io.ByteArrayOutputStream();
+ java.io.DataOutputStream out = new java.io.DataOutputStream(bytes);
+ out.writeInt(1);
+ out.writeInt(rules.size());
+ for (Rule rule : rules) {
+ out.writeUTF(rule.filesystem);
+ out.writeUTF(rule.path);
+ out.writeBoolean(rule.descendants);
+ }
+ out.flush();
+ return java.util.Base64.getEncoder().encodeToString(bytes.toByteArray());
+ } catch (IOException e) {
+ throw new IllegalArgumentException("Cannot encode TVF Hadoop route policy", e);
+ }
+ }
+
+ /** Routes touched by Globber, without constructing a filesystem or listing data. */
+ public static List<Rule> globRules(URI filesystem, String pattern) throws IOException {
+ List<Rule> rules = new ArrayList<>();
+ rules.add(new Rule(filesystem.toString(), pattern, true));
+ if (pattern.indexOf('\\') < 0) {
+ return rules;
+ }
+ for (String expanded : GlobExpander.expand(pattern)) {
+ Path flattened = new Path(expanded.isEmpty() ? Path.CUR_DIR : expanded);
+ String path = flattened.toUri().getPath();
+ Path candidate = new Path(flattened.isAbsolute() ? "/" : Path.CUR_DIR);
+ for (String component : path.split("/")) {
+ if (component.isEmpty()) {
+ continue;
+ }
+ GlobFilter filter = new GlobFilter(component);
+ if (filter.hasPattern()) {
+ // Globber lists this parent before it knows the matching child names. Keep
+ // prior wildcard components as a conservative subtree route, including when
+ // a later escaped '..' moves the final candidate outside that subtree.
+ rules.add(new Rule(filesystem.toString(), new Path(candidate, "*").toUri().getPath(), true));
+ }
+ component = component.replaceAll("\\\\(.)", "$1");
+ candidate = new Path(candidate, component);
+ }
+ rules.add(new Rule(filesystem.toString(), candidate.toUri().getPath(), true));
+ }
+ return rules;
+ }
+
+ private static IOException rejected() {
+ return new IOException("Hadoop TVF requires a proven remote filesystem route. "
+ + "Use local() with LOCAL TVF privileges and secure-path restrictions.");
+ }
+
+ private static Configuration freeze(Configuration conf) throws IOException {
+ // Service providers (notably HDFS) register their default/site resources on first load.
+ // Load them before copying raw values, as an ordinary FileSystem lookup does.
+ if (!FILE_SYSTEMS_LOADED) {
+ loadFileSystems();
+ }
+ if (conf instanceof FrozenConfiguration) {
+ return conf;
+ }
+ return new FrozenConfiguration(conf);
+ }
+
+ private static Configuration prepare(Configuration conf) throws IOException {
+ String policy = conf.getRaw(POLICY_KEY);
+ if (policy == null || policy.isEmpty()) {
+ return conf;
+ }
+ Configuration frozen = freeze(conf);
+ Context context = ((FrozenConfiguration) frozen).context;
+ if (NATIVE_POLICY.equals(policy)) {
+ context.checkNative(frozen.getRaw(ROOT_KEY), frozen.getRaw(PATH_KEY));
+ } else if (JAVA_READ_POLICY.equals(policy)) {
+ context.checkJava(new Path(frozen.getRaw(PATH_KEY)));
+ } else {
+ context.checkRules(policy);
+ }
+ return frozen;
+ }
+
+ public static void check(URI ignored, Configuration conf) throws IOException {
+ prepare(conf);
+ }
+
+ public static void clearNativeContext() {
+ LAST_NATIVE_CONTEXT.remove();
+ }
+
+ public static Context takeNativeContext() {
+ Context result = LAST_NATIVE_CONTEXT.get();
+ LAST_NATIVE_CONTEXT.remove();
+ return result;
+ }
+
+ public static Configuration prepareJavaRead(Configuration conf, Path path, String marker) throws IOException {
+ if (marker == null || marker.isEmpty()) {
+ return conf;
+ }
+ if (!NATIVE_POLICY.equals(marker)) {
+ throw rejected();
+ }
+ Configuration request = new Configuration(conf);
+ request.set(POLICY_KEY, JAVA_READ_POLICY);
+ request.set(ROOT_KEY, path.toUri().toString());
+ request.set(PATH_KEY, path.toString());
+ return prepare(request);
+ }
+
+ public static void checkJavaRead(FileSystem fs, Path path, String marker) throws IOException {
+ if (marker == null || marker.isEmpty()) {
+ return;
+ }
+ if (!NATIVE_POLICY.equals(marker)) {
+ throw rejected();
+ }
+ // The marker is request-owned, never inferred from an unmarked cached filesystem.
+ Configuration conf = fs.getConf();
+ if (JAVA_READ_POLICY.equals(conf.getRaw(POLICY_KEY))) {
+ // S3A derives a Configuration for bucket options. Its copy preserves our internal
+ // marker and raw snapshot values, but not the private Configuration subclass.
+ ((FrozenConfiguration) freeze(conf)).context.checkJava(path);
+ } else {
+ throw rejected();
+ }
+ }
+
+ private static final class FrozenConfiguration extends Configuration {
+ private final Context context;
+
+ private FrozenConfiguration(Configuration source) throws IOException {
+ super(false);
+ setClassLoader(source.getClassLoader());
+ java.util.SortedMap<String, String> values = new java.util.TreeMap<>();
+ for (Map.Entry<String, String> entry : source) {
+ String key = entry.getKey();
+ String value = source.getRaw(key);
+ set(key, value);
+ if (!PATH_KEY.equals(key) && !ROOT_KEY.equals(key) && !CONTEXT_KEY.equals(key)) {
+ values.put(key, value);
+ }
+ }
+ // ViewFS's private InnerCache folds authority case (A/a). Guarded children must
+ // instead use FileSystem's exact-authority/profile cache below.
+ set("fs.viewfs.enable.inner.cache", "false");
Review Comment:
Fixed in 01684e21fca82408df7817df0aeb8e83defa0a8b.
Guarded Java ViewFS requests now own their outer instance and all actual FsGetter child clients. The request-local map preserves exact raw scheme/authority identity, getNewInstance children are identity-tracked, and close performs base cleanup before closing every owned client, including partial initialization and exceptional close paths. These children no longer accumulate in Hadoop global caching across distinct request paths. Tests cover many paths, simultaneous equal-policy requests, nested A/a endpoints, Nfly peers, initialization failure, delete-on-exit and idempotent close. Unmarked and native ownership remain unchanged. The fix is synchronized across the three PRs.
Validation: final standard JDK17 FE selected tests: 112 passed, zero failures/errors/skips with fresh XML and unchanged source. Standard Clang21/ASAN HdfsMgrTest: 11 passed; native source/test bytes remain unchanged. Actual-file Checkstyle passed. Full community CI is not claimed.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6397669486
# FE UT Coverage Report
Increment line coverage ` 84.81% (229/270)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/69528_f293dcebdba35652d127a186ef304d66936a8b23/fe_increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/69528_f293dcebdba35652d127a186ef304d66936a8b23/fe_report/index.html)
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4366195704
##########
be/src/format_v2/table/lance_reader.cpp:
##########
@@ -35,11 +35,50 @@
#include "format_v2/lance/lance_reader_helper.h"
#include "format_v2/lance/lance_runtime_filter_helper.h"
#include "format_v2/lance/lance_session_manager.h"
+#include "io/fs/local_file_system.h"
#include "runtime/file_scan_profile.h"
#include "runtime/runtime_state.h"
namespace doris::format::lance {
+namespace {
+Status resolve_dataset_uri(const TFileRangeDesc& range, const TFileScanRangeParams& scan_params,
+ std::string* uri) {
+ const auto& requested_uri = range.table_format_params.lance_params.dataset_uri;
+ // Modern plans tag each range; older plans put the type in the scan parameters.
+ // Untagged catalog scans retain their original storage-provider selection.
+ const bool is_local = range.__isset.file_type
+ ? range.file_type == TFileType::FILE_LOCAL
+ : scan_params.__isset.file_type &&
+ scan_params.file_type == TFileType::FILE_LOCAL;
+ // LOCAL TVF uses version zero for both schema discovery and whole-dataset scans.
+ // Catalog scans pin a positive snapshot version, including file:// catalogs whose
+ // operator-configured warehouse is independent of the LOCAL TVF secure directory.
+ if (is_local && range.table_format_params.lance_params.version == 0) {
+ // LOCAL Lance bypasses glob because it opens a dataset directory. Apply the same
+ // canonical secure-root check before either schema discovery or a cached dataset open.
+ std::string local_input = requested_uri;
+ const auto colon = requested_uri.find(':');
+ const auto slash = requested_uri.find('/');
+ const bool bare_path =
+ colon == std::string::npos || (slash != std::string::npos && slash < colon);
+ if (bare_path && !requested_uri.empty() && requested_uri.find('\0') == std::string::npos &&
+ !config::user_files_secure_path.empty()) {
+ // Lance's PathAbs fallback resolves bare dot segments before following symlinks.
+ // Anchor relative TVF paths to the canonical secure root, not the process cwd.
+ std::string secure_root;
+ RETURN_IF_ERROR(io::global_local_filesystem()->canonicalize(
+ config::user_files_secure_path, &secure_root));
+ local_input = (io::Path(secure_root) / requested_uri).lexically_normal().string();
Review Comment:
[P2] Preserve Lance's home expansion before anchoring bare paths
With `enable_local_tvf=true`, an ADMIN/OPERATOR, BE home `/home/doris`, and `user_files_secure_path=/home/doris/user_files`, `local("file_path"="~/user_files/data.lance", "backend_id"="1", "format"="lance")` previously opened `/home/doris/user_files/data.lance`, inside the secure root. The [pinned SDK expands `~` and `~/` before PathAbs](https://github.com/lance-format/lance/blob/68c12dfd7efe02f90ad2d7f3a239a7eb64884e57/rust/lance-io/src/object_store.rs#L539-L564). Prefixing the raw input here instead produces `/home/doris/user_files/~/user_files/data.lance`, suppressing that expansion. Schema discovery now fails for a valid dataset, or silently discovers and scans the other dataset if both locations exist. Both the schema RPC and LanceSplit retain the original string and call this resolver. This home-alias case is distinct from the existing dot-segment findings and needs no dots or symlinks. Expand supported bare home aliases before lexical normalization and canonical containment
; add schema/scan cases with distinguishable datasets and an out-of-root expanded target.
##########
be/src/io/fs/local_file_system.cpp:
##########
@@ -455,28 +455,158 @@ 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");
+ }
+ // Match URL preprocessing only for actual file URIs; bare paths keep literal whitespace.
+ const auto is_c0_or_space = [](unsigned char c) { return c <= 0x20; };
+ size_t begin = 0;
+ size_t end = file_path.size();
+ while (begin < end && is_c0_or_space(file_path[begin])) {
+ ++begin;
+ }
+ while (end > begin && is_c0_or_space(file_path[end - 1])) {
+ --end;
+ }
+ std::string uri_input;
+ for (size_t i = begin; i < end; ++i) {
+ if (file_path[i] != '\t' && file_path[i] != '\r' && file_path[i] != '\n') {
+ uri_input += file_path[i];
+ }
+ }
+ const bool is_file_uri = uri_input.size() >= 5 &&
+ (uri_input[0] == 'f' || uri_input[0] == 'F') &&
+ (uri_input[1] == 'i' || uri_input[1] == 'I') &&
+ (uri_input[2] == 'l' || uri_input[2] == 'L') &&
+ (uri_input[3] == 'e' || uri_input[3] == 'E') && uri_input[4] == ':';
+ std::string local_input = is_file_uri ? std::move(uri_input) : file_path;
+ if (is_file_uri) {
+ local_input.replace(0, 4, "file");
+ // File URLs treat raw backslashes as separators, including before the authority.
+ // Do this before conversion; percent-encoded backslashes must remain literal.
+ for (size_t i = 5; i < local_input.size(); ++i) {
+ if (local_input[i] == '\\') {
+ local_input[i] = '/';
+ }
+ }
+ }
+ 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(local_input, local_path));
+ if (is_file_uri) {
+ // file:a%20b names "a b", but a bare path containing "%20" must stay literal.
+ // Decode once, preserve '+' and invalid escapes, matching Lance's file provider.
+ auto encoded = local_path.string();
+ // Query and fragment are URI components, not part of the filesystem path.
+ encoded = encoded.substr(0, encoded.find_first_of("?#"));
+ // A file URL without a leading slash still names a filesystem-rooted path.
+ if (encoded.empty() || encoded[0] != '/') {
+ encoded.insert(0, "/");
+ }
+ // URL dot segments are removed before symlinks are followed and before %2F
+ // becomes a path separator. Keep empty segments: "a//../b" denotes "a/b".
+ std::vector<std::string> segments;
+ for (size_t start = 1;;) {
+ const size_t end = encoded.find('/', start);
+ const auto segment =
+ encoded.substr(start, end == std::string::npos ? end : end - start);
+ const bool dot = segment == "." || segment == "%2e" || segment == "%2E";
+ const bool parent = segment == ".." || segment == ".%2e" || segment == ".%2E" ||
+ segment == "%2e." || segment == "%2E." || segment == "%2e%2e" ||
+ segment == "%2e%2E" || segment == "%2E%2e" || segment == "%2E%2E";
+ if (parent) {
+ if (!segments.empty()) {
+ segments.pop_back();
+ }
+ } else if (!dot) {
+ segments.push_back(segment);
Review Comment:
[P2] Preserve initial drive-segment normalization for file URIs
For enabled, authorized LOCAL Lance on Linux with `user_files_secure_path=/C:` and an existing `/C:/data.lance`, `file_path="file:///C|/data.lance"` previously opened that in-root dataset. The [pinned URL parser normalizes an initial file-URI `C|` segment to `C:` on every OS](https://github.com/servo/rust-url/blob/v2.5.8/url/src/parser.rs#L1343-L1355). This loop retains `C|`, so the new pre-open canonicalization resolves `/C|/data.lance` and fails or rejects the valid read. With secure root `/` and datasets at both spellings, schema discovery and scanning silently select the pipe-named dataset instead. FE preserves the original dataset URI, so this input reaches both BE paths. This is a distinct first-segment grammar rule from the existing percent, dot and whitespace cases; it requires the uncommon root-level drive-name layout described above. Apply SDK-compatible drive normalization to actual file URIs before filesystem resolution, retaining literal bare Unix names, and add a sch
ema/scan identity case.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4366428146
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/LocalTableValuedFunction.java:
##########
@@ -135,6 +143,38 @@ private void getFileListFromBackend() throws AnalysisException {
}
}
+ /** Apply LOCAL admission to sinks that select the built-in local JNI writer as well. */
+ public static void checkSinkEnabled(String tvfName, Map<String, String> properties) {
+ String writerClass = properties.getOrDefault("writer_class", "");
+ // JNI consumes a C string, and ScannerLoader accepts slash-separated class names.
+ int nul = writerClass.indexOf('\0');
+ if (nul >= 0) {
+ writerClass = writerClass.substring(0, nul);
+ }
+ if ("local".equalsIgnoreCase(tvfName)
Review Comment:
[P1] Gate native Hadoop sinks that select local storage
With `enable_local_tvf=false` on a standard Linux BE with Java/Hadoop support, a global LOAD user can still execute `INSERT INTO hdfs('file_path'='/tmp/local_gate_', 'format'='csv', 'fs.defaultFS'='file:///', 'fs.file.impl'='org.apache.hadoop.fs.LocalFileSystem') SELECT 1`. Neither condition here matches, so both sink admission calls permit it. `TVFTableSink` preserves these Hadoop properties without URI validation; the default native writer passes `FILE_HDFS` through `FileFactory`, which forwards `file:///` as the NameNode. The [pinned libhdfs preserves that scheme and calls generic FileSystem.newInstance](https://github.com/apache/doris-thirdparty/blob/hadoop-3.4.2.2-for-doris/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfs/hdfs.c#L745), selecting `LocalFileSystem`; `HdfsFileWriter::create` then opens the generated `/tmp/local_gate_<query>_0.csv` as the BE OS user without LOCAL admission or secure-root containment. Thus the new default-off switch leaves nat
ive local writes available. Apply the admission check to the effective Hadoop filesystem, including local provider overrides, and add disabled-switch INSERT/EXPLAIN coverage for this route.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4366084340
##########
be/src/io/fs/local_file_system.cpp:
##########
@@ -455,28 +455,142 @@ 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");
+ }
+ // URI schemes are ASCII case-insensitive; do not alter spelling in bare paths.
+ const bool is_file_uri = file_path.size() >= 5 &&
+ (file_path[0] == 'f' || file_path[0] == 'F') &&
+ (file_path[1] == 'i' || file_path[1] == 'I') &&
+ (file_path[2] == 'l' || file_path[2] == 'L') &&
+ (file_path[3] == 'e' || file_path[3] == 'E') && file_path[4] == ':';
+ std::string local_input = file_path;
+ if (is_file_uri) {
+ local_input.replace(0, 4, "file");
+ // File URLs treat raw backslashes as separators, including before the authority.
+ // Do this before conversion; percent-encoded backslashes must remain literal.
+ for (size_t i = 5; i < local_input.size(); ++i) {
+ if (local_input[i] == '\\') {
+ local_input[i] = '/';
+ }
+ }
+ }
+ 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(local_input, local_path));
Review Comment:
[P2] Preserve Lance file-URI parsing before resolving local paths
With `enable_local_tvf=true`, an ADMIN/OPERATOR and `user_files_secure_path=/secure`, `file_path="file:///secure/sample.lance "` (one raw trailing space) previously opened `/secure/sample.lance`: [Lance parses the URI first](https://github.com/lance-format/lance/blob/68c12dfd7efe02f90ad2d7f3a239a7eb64884e57/rust/lance-io/src/object_store.rs#L494-L502), and its [URL parser trims trailing spaces](https://github.com/servo/rust-url/blob/v2.5.8/url/src/parser.rs#L206-L220). This conversion instead preserves the space and canonicalizes `/secure/sample.lance `, a different directory. With same-schema datasets holding different rows at those two locations, schema discovery and scans now silently select the second dataset; if only the original exists, schema discovery fails. FE property parsing and LanceSplit retain the original URI, and Hadoop accepts this absolute form, so the input reaches both BE paths. This differs from the existing percent/dot threads: no escapes, dot segments or sym
links are involved. Apply SDK-compatible parsing to actual `file:` URIs before containment, retain literal bare-path whitespace, and add a schema/scan collision test with different rows.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6385361325
# FE Regression Coverage Report
Increment line coverage ` 24.90% (61/245)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/69528_9df0508783e6b1da6be6ef9909de9ebc0db45db8_merge_fe/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/69528_9df0508783e6b1da6be6ef9909de9ebc0db45db8_merge_fe/report/index.html)
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4371079593
##########
be/src/io/fs/local_file_system.cpp:
##########
@@ -452,31 +452,206 @@ const std::shared_ptr<LocalFileSystem>& global_local_filesystem() {
return local_fs;
}
+Status LocalFileSystem::parse_local_path(const std::string& file_path, Path& local_path) {
+ // 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");
+ }
+ // Match URL preprocessing only for actual file URIs; bare paths keep literal whitespace.
+ const auto is_c0_or_space = [](unsigned char c) { return c <= 0x20; };
+ size_t begin = 0;
+ size_t end = file_path.size();
+ while (begin < end && is_c0_or_space(file_path[begin])) {
+ ++begin;
+ }
+ while (end > begin && is_c0_or_space(file_path[end - 1])) {
+ --end;
+ }
+ std::string uri_input;
+ for (size_t i = begin; i < end; ++i) {
+ if (file_path[i] != '\t' && file_path[i] != '\r' && file_path[i] != '\n') {
+ uri_input += file_path[i];
+ }
+ }
+ const bool is_file_uri = uri_input.size() >= 5 &&
+ (uri_input[0] == 'f' || uri_input[0] == 'F') &&
+ (uri_input[1] == 'i' || uri_input[1] == 'I') &&
+ (uri_input[2] == 'l' || uri_input[2] == 'L') &&
+ (uri_input[3] == 'e' || uri_input[3] == 'E') && uri_input[4] == ':';
+ std::string local_input = is_file_uri ? std::move(uri_input) : file_path;
+ if (is_file_uri) {
+ local_input.replace(0, 4, "file");
+ // File URLs treat raw backslashes as separators, including before the authority.
+ // Do this before conversion; percent-encoded backslashes must remain literal.
+ for (size_t i = 5; i < local_input.size(); ++i) {
+ if (local_input[i] == '\\') {
+ local_input[i] = '/';
+ }
+ }
+ }
+ const auto hex_digit = [](char c) -> int {
+ if (c >= '0' && c <= '9') return c - '0';
+ if (c >= 'a' && c <= 'f') return c - 'a' + 10;
+ if (c >= 'A' && c <= 'F') return c - 'A' + 10;
+ return -1;
+ };
+ const auto percent_decode = [&](const std::string& encoded) {
+ std::string decoded;
+ decoded.reserve(encoded.size());
+ for (size_t i = 0; i < encoded.size(); ++i) {
+ if (encoded[i] == '%' && i + 2 < encoded.size()) {
+ const int high = hex_digit(encoded[i + 1]);
+ const int low = hex_digit(encoded[i + 2]);
+ if (high >= 0 && low >= 0) {
+ decoded += static_cast<char>((high << 4) | low);
+ i += 2;
+ continue;
+ }
+ }
+ decoded += encoded[i];
+ }
+ return decoded;
+ };
+ const auto is_drive = [](const std::string& segment) {
+ return segment.size() == 2 &&
+ ((segment[0] >= 'a' && segment[0] <= 'z') ||
+ (segment[0] >= 'A' && segment[0] <= 'Z')) &&
+ (segment[1] == ':' || segment[1] == '|');
+ };
+ bool empty_file_host = false;
+ if (is_file_uri && local_input.compare(5, 2, "//") == 0) {
+ const auto host_end = local_input.find_first_of("/?#", 7);
+ auto host = local_input.substr(7, host_end == std::string::npos ? host_end : host_end - 7);
+ if (is_drive(host)) {
+ // The URL parser treats a drive in the host position as the first path segment.
+ local_input.erase(5, 2);
+ } else {
+ host = percent_decode(host);
+ std::transform(host.begin(), host.end(), host.begin(), [](unsigned char c) {
+ return c >= 'A' && c <= 'Z' ? c + ('a' - 'A') : c;
+ });
+ empty_file_host = host.empty() || host == "localhost";
+ }
+ }
+ // Convert file: URIs to filesystem paths and reject remote schemes before any reader opens them.
+ RETURN_IF_ERROR(convert_to_abs_path(local_input, local_path));
Review Comment:
Fixed in 95b42fa44940649bc4492d5201ca79f3778154d4. Every non-bare LOCAL Lance URI now uses the actual local_path returned by the existing pinned SDK metadata parser before canonical containment and tree validation. This retains the SDK's whole-original-string fallback identity for malformed file authorities. Bare relative/home paths keep secure-root anchoring, and aliases keep their selected local provider.
The real two-dataset regression reproduced the wrong schema/key/row 7 before the repair; schema discovery, cache identity and actual scanning now select the SDK fallback dataset and row 42. Out-of-root fallback rejects. Both releases passed 20 selected filesystem/Lance tests; generic LocalFS, SDK ABI and manifest-base policy remain unchanged. The pinned SDK also confirmed that the old decoded-parent success expectation was invalid, so it is covered as a schema/scan rejection.
##########
fe/be-java-extensions/hadoop-deps/src/main/java/org/apache/hadoop/fs/FileSystem.java:
##########
@@ -3622,13 +3633,670 @@ public static Class<? extends FileSystem> getFileSystemClass(String scheme,
* @return the initialized filesystem.
* @throws IOException problems loading or initializing the FileSystem
*/
+ // DORIS-PATCH: TVF policy is checked against the frozen configuration used by the consumer.
+ public static int getDorisTvfGuardVersion() {
+ return 1;
+ }
+
+ public static final class TvfGuard {
+ public static final String POLICY_KEY = "fs.doris.tvf.remote.paths";
+ public static final String NATIVE_POLICY = "native";
+ public static final String JAVA_READ_POLICY = "java-read";
+ public static final String ROOT_KEY = "fs.doris.tvf.root";
+ public static final String PATH_KEY = "fs.doris.tvf.path";
+ public static final String CONTEXT_KEY = "fs.doris.tvf.context";
+ private static final ThreadLocal<Context> LAST_NATIVE_CONTEXT = new ThreadLocal<>();
+
+ private TvfGuard() {
+ }
+
+ public static final class Rule {
+ private final String filesystem;
+ private final String path;
+ private final boolean descendants;
+
+ public Rule(String filesystem, String path, boolean descendants) {
+ this.filesystem = filesystem;
+ this.path = path;
+ this.descendants = descendants;
+ }
+ }
+
+ public static String encode(List<Rule> rules) {
+ if (FileSystem.getDorisTvfGuardVersion() != 1) {
+ throw new IllegalStateException("TVF Hadoop runtime policy version mismatch");
+ }
+ try {
+ java.io.ByteArrayOutputStream bytes = new java.io.ByteArrayOutputStream();
+ java.io.DataOutputStream out = new java.io.DataOutputStream(bytes);
+ out.writeInt(1);
+ out.writeInt(rules.size());
+ for (Rule rule : rules) {
+ out.writeUTF(rule.filesystem);
+ out.writeUTF(rule.path);
+ out.writeBoolean(rule.descendants);
+ }
+ out.flush();
+ return java.util.Base64.getEncoder().encodeToString(bytes.toByteArray());
+ } catch (IOException e) {
+ throw new IllegalArgumentException("Cannot encode TVF Hadoop route policy", e);
+ }
+ }
+
+ /** Routes touched by Globber, without constructing a filesystem or listing data. */
+ public static List<Rule> globRules(URI filesystem, String pattern) throws IOException {
+ List<Rule> rules = new ArrayList<>();
+ rules.add(new Rule(filesystem.toString(), pattern, true));
+ if (pattern.indexOf('\\') < 0) {
+ return rules;
+ }
+ for (String expanded : GlobExpander.expand(pattern)) {
+ Path flattened = new Path(expanded.isEmpty() ? Path.CUR_DIR : expanded);
+ String path = flattened.toUri().getPath();
+ Path candidate = new Path(flattened.isAbsolute() ? "/" : Path.CUR_DIR);
+ for (String component : path.split("/")) {
+ if (component.isEmpty()) {
+ continue;
+ }
+ GlobFilter filter = new GlobFilter(component);
+ if (filter.hasPattern()) {
+ // Globber lists this parent before it knows the matching child names. Keep
+ // prior wildcard components as a conservative subtree route, including when
+ // a later escaped '..' moves the final candidate outside that subtree.
+ rules.add(new Rule(filesystem.toString(), new Path(candidate, "*").toUri().getPath(), true));
+ }
+ component = component.replaceAll("\\\\(.)", "$1");
+ candidate = new Path(candidate, component);
+ }
+ rules.add(new Rule(filesystem.toString(), candidate.toUri().getPath(), true));
+ }
+ return rules;
+ }
+
+ private static IOException rejected() {
+ return new IOException("Hadoop TVF requires a proven remote filesystem route. "
+ + "Use local() with LOCAL TVF privileges and secure-path restrictions.");
+ }
+
+ private static Configuration freeze(Configuration conf) throws IOException {
+ // Service providers (notably HDFS) register their default/site resources on first load.
+ // Load them before copying raw values, as an ordinary FileSystem lookup does.
+ if (!FILE_SYSTEMS_LOADED) {
+ loadFileSystems();
+ }
+ if (conf instanceof FrozenConfiguration) {
+ return conf;
+ }
+ return new FrozenConfiguration(conf);
+ }
+
+ private static Configuration prepare(Configuration conf) throws IOException {
+ String policy = conf.getRaw(POLICY_KEY);
+ if (policy == null || policy.isEmpty()) {
+ return conf;
+ }
+ Configuration frozen = freeze(conf);
+ Context context = ((FrozenConfiguration) frozen).context;
+ if (NATIVE_POLICY.equals(policy)) {
+ context.checkNative(frozen.getRaw(ROOT_KEY), frozen.getRaw(PATH_KEY));
+ } else if (JAVA_READ_POLICY.equals(policy)) {
+ context.checkJava(new Path(frozen.getRaw(PATH_KEY)));
+ } else {
+ context.checkRules(policy);
+ }
+ return frozen;
+ }
+
+ public static void check(URI ignored, Configuration conf) throws IOException {
+ prepare(conf);
+ }
+
+ public static void clearNativeContext() {
+ LAST_NATIVE_CONTEXT.remove();
+ }
+
+ public static Context takeNativeContext() {
+ Context result = LAST_NATIVE_CONTEXT.get();
+ LAST_NATIVE_CONTEXT.remove();
+ return result;
+ }
+
+ public static Configuration prepareJavaRead(Configuration conf, Path path, String marker) throws IOException {
+ if (marker == null || marker.isEmpty()) {
+ return conf;
+ }
+ if (!NATIVE_POLICY.equals(marker)) {
+ throw rejected();
+ }
+ Configuration request = new Configuration(conf);
+ request.set(POLICY_KEY, JAVA_READ_POLICY);
+ request.set(ROOT_KEY, path.toUri().toString());
+ request.set(PATH_KEY, path.toString());
+ return prepare(request);
+ }
+
+ public static void checkJavaRead(FileSystem fs, Path path, String marker) throws IOException {
+ if (marker == null || marker.isEmpty()) {
+ return;
+ }
+ if (!NATIVE_POLICY.equals(marker)) {
+ throw rejected();
+ }
+ // The marker is request-owned, never inferred from an unmarked cached filesystem.
+ Configuration conf = fs.getConf();
+ if (JAVA_READ_POLICY.equals(conf.getRaw(POLICY_KEY))) {
+ // S3A derives a Configuration for bucket options. Its copy preserves our internal
+ // marker and raw snapshot values, but not the private Configuration subclass.
+ ((FrozenConfiguration) freeze(conf)).context.checkJava(path);
+ } else {
+ throw rejected();
+ }
+ }
+
+ private static final class FrozenConfiguration extends Configuration {
+ private final Context context;
+
+ private FrozenConfiguration(Configuration source) throws IOException {
+ super(false);
+ setClassLoader(source.getClassLoader());
+ java.util.SortedMap<String, String> values = new java.util.TreeMap<>();
+ for (Map.Entry<String, String> entry : source) {
+ String key = entry.getKey();
+ String value = source.getRaw(key);
+ set(key, value);
+ if (!PATH_KEY.equals(key) && !ROOT_KEY.equals(key) && !CONTEXT_KEY.equals(key)) {
+ values.put(key, value);
+ }
+ }
+ // ViewFS's private InnerCache folds authority case (A/a). Guarded children must
+ // instead use FileSystem's exact-authority/profile cache below.
+ set("fs.viewfs.enable.inner.cache", "false");
Review Comment:
The retained native BrokerProperties-passthrough variant is also addressed in 95b42fa44940649bc4492d5201ca79f3778154d4. The actual native TVF backend payload now excludes the known output-path transport field after the native first-NUL prefix, Configuration.trim and case-insensitive TVF-field projection. The dedicated Thrift output path stays correct, while all real Hadoop properties and authority/configuration isolation are retained.
The production sender/FrozenConfiguration/real ViewFS-cache test uses ordered uppercase-before-lowercase keys, NUL aliases and pre-NUL whitespace. Before fixture cleanup, two different output prefixes share the same profile/client; a real Hadoop setting change remains isolated (client counts 1,1,2). All 125 selected FE/Avro tests pass. Native connection close/disconnect retains its existing lifetime behavior; this removes the newly introduced per-output-prefix cache dimension.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6396151772
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6379574960
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4365918988
##########
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:
Fixed in 398acc158bbb9e2a9525a0960a9186f351d73fc6. The final remote Lance URI guard now validates the original port spelling as ASCII digits, rejecting signed zero before metadata loading. IPv6 is also validated without DNS using `InetAddresses`, rejecting the leading-zero IPv4-tail case.
`S3TableValuedFunctionTest` covers `:-0`, `:-00`, `:+0`, the malformed mapped IPv6 address, and valid port/IPv6 cases. Rejected inputs do not reach the mocked metadata loader. All 35 selected FE tests passed.
##########
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:
Fixed in 398acc158bbb9e2a9525a0960a9186f351d73fc6. LOCAL path validation now decodes actual `file:` URI paths once before canonicalization, after removing raw query/fragment components. Bare paths retain literal percent characters and `+`; decoded NUL and out-of-root targets are rejected.
`LanceTableReaderLocalPathTest.FileUrisPreserveEscapedDatasetIdentity` checks the canonical dataset key, real schema discovery and scanning for escaped-space/literal-percent collisions, `%2520`, plus/percent names and query/fragment handling. All 11 selected BE filesystem/Lance tests passed under Clang21/ASAN; scans returned row IDs {1,2,3,4}.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6386541747
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4371394437
##########
fe/be-java-extensions/hadoop-deps/src/main/java/org/apache/hadoop/fs/FileSystem.java:
##########
@@ -3622,16 +3640,769 @@ public static Class<? extends FileSystem> getFileSystemClass(String scheme,
* @return the initialized filesystem.
* @throws IOException problems loading or initializing the FileSystem
*/
+ // DORIS-PATCH: TVF policy is checked against the frozen configuration used by the consumer.
+ public static int getDorisTvfGuardVersion() {
+ return 1;
+ }
+
+ public static final class TvfGuard {
+ public static final String POLICY_KEY = "fs.doris.tvf.remote.paths";
+ public static final String NATIVE_POLICY = "native";
+ public static final String NATIVE_WRITE_POLICY = "native-write";
+ public static final String JAVA_READ_POLICY = "java-read";
+ public static final String ROOT_KEY = "fs.doris.tvf.root";
+ public static final String PATH_KEY = "fs.doris.tvf.path";
+ public static final String CONTEXT_KEY = "fs.doris.tvf.context";
+ private static final ThreadLocal<Context> LAST_NATIVE_CONTEXT = new ThreadLocal<>();
+
+ private TvfGuard() {
+ }
+
+ public static final class Rule {
+ private final String filesystem;
+ private final String path;
+ private final boolean descendants;
+
+ public Rule(String filesystem, String path, boolean descendants) {
+ this.filesystem = filesystem;
+ this.path = path;
+ this.descendants = descendants;
+ }
+ }
+
+ public static String encode(List<Rule> rules) {
+ if (FileSystem.getDorisTvfGuardVersion() != 1) {
+ throw new IllegalStateException("TVF Hadoop runtime policy version mismatch");
+ }
+ try {
+ java.io.ByteArrayOutputStream bytes = new java.io.ByteArrayOutputStream();
+ java.io.DataOutputStream out = new java.io.DataOutputStream(bytes);
+ out.writeInt(1);
+ out.writeInt(rules.size());
+ for (Rule rule : rules) {
+ out.writeUTF(rule.filesystem);
+ out.writeUTF(rule.path);
+ out.writeBoolean(rule.descendants);
+ }
+ out.flush();
+ return java.util.Base64.getEncoder().encodeToString(bytes.toByteArray());
+ } catch (IOException e) {
+ throw new IllegalArgumentException("Cannot encode TVF Hadoop route policy", e);
+ }
+ }
+
+ /** Literal probes and subtree prefixes touched by Globber, without filesystem IO. */
+ public static List<Rule> globRules(URI filesystem, String pattern) throws IOException {
+ List<Rule> rules = new ArrayList<>();
+ for (String expanded : GlobExpander.expand(pattern)) {
+ Path flattened = new Path(expanded.isEmpty() ? Path.CUR_DIR : expanded);
+ String path = flattened.toUri().getPath();
+ Path root = new Path(flattened.isAbsolute() ? "/" : Path.CUR_DIR);
+ Path candidate = root;
+ List<String> components = new ArrayList<>();
+ List<Boolean> patterned = new ArrayList<>();
+ for (String component : path.split("/")) {
+ if (component.isEmpty()) {
+ continue;
+ }
+ GlobFilter filter = new GlobFilter(component);
+ if (filter.hasPattern()) {
+ // Keep every real listing parent, even if later escaped '..' removes it.
+ rules.add(new Rule(filesystem.toString(),
+ globLiteralPrefix(root, components, patterned).toUri().getPath(), true));
+ }
+ component = component.replaceAll("\\\\(.)", "$1");
+ candidate = new Path(candidate, component);
+ if ("..".equals(component) && !components.isEmpty()
+ && !"..".equals(components.get(components.size() - 1))) {
+ components.remove(components.size() - 1);
+ patterned.remove(patterned.size() - 1);
+ } else if (!".".equals(component)) {
+ components.add(component);
+ patterned.add(filter.hasPattern());
+ }
+ }
+ boolean subtree = patterned.contains(Boolean.TRUE);
+ Path probe = subtree ? globLiteralPrefix(root, components, patterned) : candidate;
+ rules.add(new Rule(filesystem.toString(), probe.toUri().getPath(), subtree));
+ }
+ return rules;
+ }
+
+ private static Path globLiteralPrefix(Path root, List<String> components, List<Boolean> patterned) {
+ Path prefix = root;
+ for (int i = 0; i < components.size() && !patterned.get(i); i++) {
+ prefix = new Path(prefix, components.get(i));
+ }
+ return prefix;
+ }
+
+ /** Java requests own their mount clients; native connections keep their existing cache. */
+ private static final class RequestViewFileSystem extends org.apache.hadoop.fs.viewfs.ViewFileSystem {
+ private final Map<List<String>, FileSystem> children = new HashMap<>();
+ private final Set<FileSystem> owned = Collections.newSetFromMap(new java.util.IdentityHashMap<>());
+ private boolean closed;
+
+ private RequestViewFileSystem() throws IOException {
+ }
+
+ private final FsGetter getter = new FsGetter() {
+ @Override
+ public FileSystem get(URI uri, Configuration conf) throws IOException {
+ synchronized (RequestViewFileSystem.this) {
+ List<String> key = Arrays.asList(uri.getScheme(), uri.getRawAuthority());
+ FileSystem child = children.get(key);
+ if (child == null) {
+ child = getNewInstance(uri, conf);
+ children.put(key, child);
+ }
+ return child;
+ }
+ }
+
+ @Override
+ public FileSystem getNewInstance(URI uri, Configuration conf) throws IOException {
+ synchronized (RequestViewFileSystem.this) {
+ if (closed) {
+ throw new IOException("TVF ViewFS request is closed");
+ }
+ FileSystem child = FileSystem.newInstance(uri, conf);
+ owned.add(child);
+ return child;
+ }
+ }
+ };
+
+ @Override
+ protected FsGetter fsGetter() {
+ return getter;
+ }
+
+ @Override
+ public synchronized void close() throws IOException {
+ if (closed) {
+ return;
+ }
+ List<IOException> failures = new ArrayList<>();
+ try {
+ // delete-on-exit may resolve a previously unused mount, so do this before sealing.
+ ((FileSystem) this).closeBase();
+ } catch (IOException e) {
+ failures.add(e);
+ } finally {
+ closed = true;
+ // Enumerating ViewFS target.getChildFileSystems() misses Nfly's actual peers.
+ // The stable getter owns those peers directly, including partial initialization.
+ for (FileSystem child : owned) {
+ try {
+ child.close();
+ } catch (IOException e) {
+ failures.add(e);
+ } catch (RuntimeException e) {
+ failures.add(new IOException("Cannot close a TVF mount client", e));
+ }
+ }
+ owned.clear();
+ children.clear();
+ }
+ if (!failures.isEmpty()) {
+ throw MultipleIOException.createIOException(failures);
+ }
+ }
+ }
+
+ private static boolean isNative(String policy) {
+ return NATIVE_POLICY.equals(policy) || NATIVE_WRITE_POLICY.equals(policy);
+ }
+
+ private static boolean isJavaRequest(Configuration conf) {
+ String policy = conf.getRaw(POLICY_KEY);
+ return policy != null && !policy.isEmpty() && !isNative(policy);
+ }
+
+ private static boolean isRequestViewFs(Class<? extends FileSystem> implementation, Configuration conf) {
+ return isJavaRequest(conf) && implementation == org.apache.hadoop.fs.viewfs.ViewFileSystem.class;
+ }
+
+ private static IOException rejected() {
+ return new IOException("Hadoop TVF requires a proven remote filesystem route. "
+ + "Use local() with LOCAL TVF privileges and secure-path restrictions.");
+ }
+
+ private static Configuration freeze(Configuration conf) throws IOException {
+ // Service providers (notably HDFS) register their default/site resources on first load.
+ // Load them before copying raw values, as an ordinary FileSystem lookup does.
+ if (!FILE_SYSTEMS_LOADED) {
+ loadFileSystems();
+ }
+ if (conf instanceof FrozenConfiguration) {
+ return conf;
+ }
+ return new FrozenConfiguration(conf);
+ }
+
+ private static Configuration prepare(Configuration conf) throws IOException {
+ String policy = conf.getRaw(POLICY_KEY);
+ if (policy == null || policy.isEmpty()) {
+ return conf;
+ }
+ Configuration frozen = freeze(conf);
+ Context context = ((FrozenConfiguration) frozen).context;
+ if (isNative(policy)) {
+ context.checkNative(frozen.getRaw(ROOT_KEY), frozen.getRaw(PATH_KEY));
+ } else if (JAVA_READ_POLICY.equals(policy)) {
+ context.checkJava(new Path(frozen.getRaw(PATH_KEY)));
+ } else {
+ context.checkRules(policy);
+ }
+ return frozen;
+ }
+
+ public static void check(URI ignored, Configuration conf) throws IOException {
+ prepare(conf);
+ }
+
+ public static void clearNativeContext() {
+ LAST_NATIVE_CONTEXT.remove();
+ }
+
+ public static Context takeNativeContext() {
+ Context result = LAST_NATIVE_CONTEXT.get();
+ LAST_NATIVE_CONTEXT.remove();
+ return result;
+ }
+
+ public static Configuration prepareJavaRead(Configuration conf, Path path, String marker) throws IOException {
+ if (marker == null || marker.isEmpty()) {
+ return conf;
+ }
+ if (!NATIVE_POLICY.equals(marker)) {
+ throw rejected();
+ }
+ Configuration request = new Configuration(conf);
+ request.set(POLICY_KEY, JAVA_READ_POLICY);
+ request.set(ROOT_KEY, path.toUri().toString());
+ request.set(PATH_KEY, path.toString());
+ return prepare(request);
+ }
+
+ public static void checkJavaRead(FileSystem fs, Path path, String marker) throws IOException {
+ if (marker == null || marker.isEmpty()) {
+ return;
+ }
+ if (!NATIVE_POLICY.equals(marker)) {
+ throw rejected();
+ }
+ // The marker is request-owned, never inferred from an unmarked cached filesystem.
+ Configuration conf = fs.getConf();
+ if (JAVA_READ_POLICY.equals(conf.getRaw(POLICY_KEY))) {
+ // S3A derives a Configuration for bucket options. Its copy preserves our internal
+ // marker and raw snapshot values, but not the private Configuration subclass.
+ ((FrozenConfiguration) freeze(conf)).context.checkJava(path);
+ } else {
+ throw rejected();
+ }
+ }
+
+ private static final class FrozenConfiguration extends Configuration {
+ private final Context context;
+
+ private FrozenConfiguration(Configuration source) throws IOException {
+ super(false);
+ setClassLoader(source.getClassLoader());
+ java.util.SortedMap<String, String> values = new java.util.TreeMap<>();
+ for (Map.Entry<String, String> entry : source) {
+ String key = entry.getKey();
+ String value = source.getRaw(key);
+ set(key, value);
+ if (!PATH_KEY.equals(key) && !ROOT_KEY.equals(key) && !CONTEXT_KEY.equals(key)) {
+ values.put(key, value);
+ }
+ }
+ // ViewFS's private InnerCache folds authority case (A/a). Guarded children must
+ // instead use FileSystem's exact-authority/profile cache below.
+ set("fs.viewfs.enable.inner.cache", "false");
+ values.put("fs.viewfs.enable.inner.cache", "false");
+ try {
+ java.security.MessageDigest digest = java.security.MessageDigest.getInstance("SHA-256");
+ java.io.DataOutputStream output = new java.io.DataOutputStream(
+ new java.security.DigestOutputStream(new java.io.OutputStream() {
+ @Override
+ public void write(int value) {
+ }
+
+ @Override
+ public void write(byte[] values, int offset, int length) {
+ }
+ }, digest));
+ for (Map.Entry<String, String> entry : values.entrySet()) {
+ for (String value : new String[] {entry.getKey(), entry.getValue()}) {
+ // Preserve raw Java code units, including unpaired surrogates. UTF-8 replacement
+ // would give a literal '?' and an unpaired surrogate the same cache profile.
+ output.writeInt(value.length());
+ output.writeChars(value);
+ }
+ }
+ output.flush();
+ StringBuilder identity = new StringBuilder();
+ for (byte value : digest.digest()) {
+ identity.append(Character.forDigit((value & 255) >>> 4, 16));
+ identity.append(Character.forDigit(value & 15, 16));
+ }
+ set(CONTEXT_KEY, identity.toString());
+ } catch (java.security.NoSuchAlgorithmException e) {
+ throw new IOException("SHA-256 is required for TVF Hadoop cache isolation");
+ }
+ context = new Context(this);
+ }
+ }
+
+ public static final class Context {
+ private final Configuration conf;
+ private final Map<String, String> routingKeys = new HashMap<>();
+
+ private Context(Configuration conf) {
+ this.conf = conf;
+ for (Map.Entry<String, String> entry : conf) {
+ String key = entry.getKey();
+ if ("fs.defaultFS".equals(key) || "fs.default.name".equals(key)
+ || key.startsWith("fs.viewfs.") || (key.startsWith("fs.") && key.endsWith(".impl"))) {
+ routingKeys.put(key, entry.getValue());
+ }
+ }
+ }
+
+ private EffectiveRoute route(boolean descendants) {
+ return new EffectiveRoute(routingKeys, descendants, conf);
+ }
+
+ public String profile() {
+ return conf.getRaw(CONTEXT_KEY);
+ }
+
+ public boolean isDirectRemote() throws IOException {
+ String root = conf.getRaw(ROOT_KEY);
+ if (root == null) {
+ throw rejected();
+ }
+ try {
+ URI uri = nativeUri(root);
+ return route(false).provider(uri.getScheme()) == Route.Provider.REMOTE;
+ } catch (RuntimeException | URISyntaxException e) {
+ throw rejected();
+ }
+ }
+
+ private static URI nativeUri(String root) throws URISyntaxException {
+ root = root.trim();
+ if (root.isEmpty() || "default".equals(root)) {
+ throw new URISyntaxException("", "Unproven native filesystem");
+ }
+ return new URI(root.contains("://") ? root : "hdfs://" + root);
+ }
+
+ public void checkNative(String root, String path) throws IOException {
+ if (root == null || path == null || path.isEmpty()) {
+ throw rejected();
+ }
+ try {
+ URI uri = nativeUri(root);
+ Path actualPath = new Path(path);
+ if (NATIVE_WRITE_POLICY.equals(conf.getRaw(POLICY_KEY))) {
+ // ViewFS.create resolves the parent, without following the final component.
+ Path parent = actualPath.getParent();
+ actualPath = parent == null ? new Path("/") : parent;
+ }
+ route(false).checkRoute(uri, actualPath.toUri().getPath(), new HashSet<>());
+ } catch (RuntimeException | URISyntaxException e) {
+ throw rejected();
+ }
+ }
+
+ private void checkJava(Path path) throws IOException {
+ try {
+ URI uri = path.toUri();
+ route(false).checkRoute(uri, uri.getPath(), new HashSet<>());
+ } catch (RuntimeException | URISyntaxException e) {
+ throw rejected();
+ }
+ }
+
+ private void checkRules(String policy) throws IOException {
+ try {
+ byte[] bytes = java.util.Base64.getDecoder().decode(policy);
+ java.io.DataInputStream in = new java.io.DataInputStream(new java.io.ByteArrayInputStream(bytes));
+ int version = in.readInt();
+ int count = in.readInt();
+ if (version != 1 || count <= 0 || count > in.available() / 5) {
+ throw rejected();
+ }
+ for (int i = 0; i < count; i++) {
+ URI filesystem = new URI(in.readUTF());
+ String path = in.readUTF();
+ boolean descendants = in.readBoolean();
+ route(descendants).checkRoute(filesystem, path, new HashSet<>());
+ }
+ if (in.available() != 0) {
+ throw rejected();
+ }
+ } catch (RuntimeException | URISyntaxException e) {
+ throw rejected();
+ }
+ }
+ }
+
+ private static final class EffectiveRoute extends Route {
+ private final Configuration conf;
+
+ private EffectiveRoute(Map<String, String> properties, boolean descendants, Configuration conf) {
+ super(properties, descendants);
+ this.conf = conf;
+ }
+
+ private EffectiveRoute(EffectiveRoute original, boolean descendants) {
+ super(original, descendants);
+ conf = original.conf;
+ }
+
+ @Override
+ protected Route withDescendants(boolean value) {
+ return new EffectiveRoute(this, value);
+ }
+
+ @Override
+ protected String property(String key) {
+ return conf.get(key);
+ }
+
+ @Override
+ protected Provider provider(String scheme) {
+ if ("file".equalsIgnoreCase(scheme)) {
+ return Provider.LOCAL;
+ }
+ try {
+ String implementation = FileSystem.getFileSystemClass(scheme, conf).getName();
+ return implementationProvider(implementation);
+ } catch (IOException | RuntimeException e) {
+ return Provider.UNKNOWN;
+ }
+ }
+ }
+
+ public static class Route {
+ protected enum Provider {
+ REMOTE,
+ LOCAL,
+ MOUNT_TABLE,
+ DYNAMIC,
+ UNKNOWN
+ }
+
+ // JuiceFS loads its authoritative backend from mutable namespace metadata; neither
+ // its scheme nor its class proves remote storage for a guarded TVF.
+ protected static final Set<String> REMOTE_SCHEMES = new HashSet<>(Arrays.asList(
+ "hdfs", "s3a", "s3n", "oss", "ofs", "abfs", "abfss", "wasb", "wasbs", "gs", "cosn"));
+ protected static final Set<String> REMOTE_IMPLEMENTATIONS = new HashSet<>(Arrays.asList(
+ "org.apache.hadoop.hdfs.DistributedFileSystem",
+ "org.apache.hadoop.fs.s3a.S3AFileSystem",
+ "org.apache.hadoop.fs.azurebfs.AzureBlobFileSystem",
+ "org.apache.hadoop.fs.azurebfs.SecureAzureBlobFileSystem",
+ "org.apache.hadoop.fs.azure.NativeAzureFileSystem",
+ "org.apache.hadoop.fs.azure.NativeAzureFileSystem$Secure",
+ "org.apache.hadoop.fs.ozone.OzoneFileSystem",
+ "org.apache.hadoop.fs.ozone.RootedOzoneFileSystem",
+ "org.apache.hadoop.fs.cosn.CosNFileSystem",
+ "com.qcloud.chdfs.fs.CHDFSHadoopFileSystemAdapter",
+ "org.apache.hadoop.fs.aliyun.oss.AliyunOSSFileSystem",
+ "com.aliyun.jindodata.oss.JindoOssFileSystem",
+ "org.apache.hadoop.fs.obs.OBSFileSystem",
+ "com.google.cloud.hadoop.fs.gcs.GoogleHadoopFileSystem"));
+
+ private final boolean descendants;
+ private final Map<String, String> properties;
+
+ protected Route(Route original, boolean descendants) {
+ this(original.properties, descendants);
+ }
+
+ protected Route withDescendants(boolean value) {
+ return new Route(this, value);
+ }
+
+ public Route(Map<String, String> properties, boolean descendants) {
+ // Both callers supply a statement-local snapshot that is never mutated afterward.
+ this.properties = properties;
+ this.descendants = descendants;
+ }
+
+ private List<URI> defaults() throws URISyntaxException {
+ List<URI> defaults = new ArrayList<>();
+ for (String key : Arrays.asList("fs.defaultFS", "fs.default.name")) {
+ String value = property(key);
+ if (value != null) {
+ value = selector(value).trim();
+ // FileSystem.fixName treats slash-free host[:port] strings as HDFS defaults.
+ defaults.add(new URI("local".equals(value) ? "file:///"
+ : (value.indexOf('/') < 0 ? "hdfs://" + value : value)));
+ }
+ }
+ if (defaults.isEmpty()) {
+ defaults.add(new URI("file:///"));
+ }
+ return defaults;
+ }
+
+ public void checkRoute(URI filesystem, String path, Set<String> activeTables) throws URISyntaxException {
+ if (filesystem.getScheme() == null) {
+ for (URI defaultFs : defaults()) {
+ checkViewFsEnabled(defaultFs, path, activeTables);
+ }
+ return;
+ }
+ if (filesystem.getAuthority() == null) {
+ boolean completed = false;
+ for (URI defaultFs : defaults()) {
+ if (filesystem.getScheme().equals(defaultFs.getScheme()) && defaultFs.getAuthority() != null) {
+ checkViewFsEnabled(defaultFs, path, activeTables);
+ completed = true;
+ }
+ }
+ if (completed) {
+ return;
+ }
+ }
+ checkViewFsEnabled(filesystem, path, activeTables);
+ }
+
+ public void checkGlobRoute(URI filesystem, String pattern, Set<String> activeTables)
+ throws IOException, URISyntaxException {
+ for (Rule rule : globRules(filesystem, pattern)) {
+ withDescendants(rule.descendants).checkRoute(filesystem, rule.path, activeTables);
+ }
+ }
+
+ protected Provider provider(String scheme) {
+ if ("file".equalsIgnoreCase(scheme)) {
+ return Provider.LOCAL;
+ }
+ String implementation = property("fs." + scheme + ".impl");
+ if (implementation != null) {
+ implementation = selector(implementation).trim();
+ return implementationProvider(implementation);
+ }
+ if ("viewfs".equals(scheme)) {
+ return Provider.MOUNT_TABLE;
+ }
+ return REMOTE_SCHEMES.contains(scheme) ? Provider.REMOTE : Provider.UNKNOWN;
+ }
+
+ protected Provider implementationProvider(String implementation) {
+ if ("org.apache.hadoop.fs.LocalFileSystem".equals(implementation)
+ || "org.apache.hadoop.fs.RawLocalFileSystem".equals(implementation)) {
+ return Provider.LOCAL;
+ }
+ if ("org.apache.hadoop.fs.viewfs.ViewFileSystem".equals(implementation)) {
+ return Provider.MOUNT_TABLE;
+ }
+ if ("org.apache.hadoop.fs.viewfs.ViewFileSystemOverloadScheme".equals(implementation)
+ || "org.apache.hadoop.hdfs.ViewDistributedFileSystem".equals(implementation)) {
+ return Provider.DYNAMIC;
+ }
+ return REMOTE_IMPLEMENTATIONS.contains(implementation) ? Provider.REMOTE : Provider.UNKNOWN;
+ }
+
+ private String[] targets(String key, String prefix) {
+ String value = selector(property(key));
+ // InodeTree treats SINGLE, SINGLE_FALLBACK and MERGE_SLASH as one URI.
+ // Commas delimit targets only for MERGE and NFLY links.
+ String entry = key.substring(prefix.length());
+ boolean multiple = entry.startsWith("linkMerge.") || entry.startsWith("linkNfly.");
+ return multiple ? value.split(",", -1) : new String[] {value};
+ }
+
+ private void checkGlobTargets(String key, String prefix, Set<String> activeTables) throws URISyntaxException {
+ for (String target : targets(key, prefix)) {
+ if (target.isEmpty()) {
+ reject();
+ }
+ URI uri = new URI(target);
+ // Listing can touch any descendant of a selected mount. Reuse the same route
+ // interpreter for nested tables instead of assuming their root is representative.
+ String path = uri.getPath() == null ? "/" : uri.getPath();
+ checkRoute(uri, path, activeTables);
+ }
+ }
+
+ private void checkViewFsEnabled(URI filesystem, String path, Set<String> activeTables) {
+ String scheme = filesystem.getScheme();
+ Provider provider = provider(scheme);
+ if (provider == Provider.LOCAL || provider == Provider.UNKNOWN || provider == Provider.DYNAMIC) {
+ reject();
+ }
+ if (provider != Provider.MOUNT_TABLE) {
+ return;
+ }
+ if (!path.startsWith("/")) {
+ // Java resolves relative paths against the selected filesystem's working directory.
+ // Its user/home configuration is not a proven absolute mount-table path here.
+ reject();
+ }
+ String table = filesystem.getAuthority();
+ String ignorePort = property("fs.viewfs.ignore.port.in.mount.table.name");
+ if (ignorePort != null && Boolean.parseBoolean(selector(ignorePort).trim())
+ && table != null && filesystem.getPort() != -1) {
+ table = filesystem.getHost();
+ if (table == null) {
+ reject();
+ }
+ }
+ if (table == null || table.isEmpty()) {
+ table = property("fs.viewfs.mounttable.default.name.key");
+ table = table == null ? "default" : selector(table);
+ }
+ // Nested ViewFS cycles or arbitrarily deep delegation cannot establish remote storage.
+ if (activeTables.size() >= 32 || !activeTables.add(table)) {
Review Comment:
Fixed in b1fb0a3fe44334d62eacb8590dbae3aa1ac49e72. The shared route interpreter now identifies an active ViewFS route by its resolved table and path, using a length-prefixed table name to avoid ambiguous state keys. Re-entering a table at a different path is allowed; repeated route states and the separate 32-step depth bound still reject actual and path-growing cycles. Local, unknown and dynamic targets retain their existing rejection.
The pinned Hadoop SINGLE-link implementation was verified to initialize its target lazily. An actual unguarded ViewFS /a -> same-table /b -> HDFS /base/file.csv succeeds; before the repair the new guarded finite-alias and depth-bound positive tests failed. The repaired code passes all 129 selected FE/Avro tests with fresh required reports, including actual ViewFS reads and literal cleanup, native retained-context reuse, LOCAL-enabled/disabled read admission, local targets, true cycles, expanding routes, and the 32/33-step boundary. All 26 applicable Doris Java files pass Checkstyle. No SDK, C++ or Thrift change is involved.
Exact-head formal review: https://github.com/apache/doris/pull/69528#issuecomment-6399562602.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6401440869
Closing this PR because the implementation scope is being reduced to a focused FE change: disable LOCAL TVF by default, retain explicit startup opt-in, and preserve the existing implementation. Replacement work will start from the latest target branch. The broader storage/runtime hardening is outside that replacement scope. The original branch, local changes and review history are preserved.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6398460518
<!-- doris-repo-review:v1:begin -->
### Local pipeline review — ✅ PASS
```yaml
schema: doris-repo-review/v1
status: PASS
pr: apache/doris#69528
commit: 95b42fa44940649bc4492d5201ca79f3778154d4
base: 00a9e1cb6dd99355f1b385e3c3ec5c23ae5a83cf
reviewed_at: 2026-10-25T21:49+08:00
reviewer: OIiveirra
model: gpt-6.1-sol
effort: xhigh
findings: {blocker: 0, major: 0, minor: 0, nit: 0}
rounds: 1
converged: true
```
**Notes for maintainers**
- `fe/fe-core/src/main/java/org/apache/doris/planner/TVFTableSink.java:141` — native output-path aliases are removed using first-NUL prefix, Configuration trim and case-insensitive field identity; dedicated output and real configuration/profile isolation remain. Actual ordered sender/profile/client gate observes 1,1,2 before cleanup.
- `fe/be-java-extensions/hadoop-deps/src/main/java/org/apache/hadoop/fs/FileSystem.java:4101` — guarded JuiceFS denial, including remote-backed namespaces, is an explicitly declared security limitation; mutable authoritative backend/reloads cannot be proved remote by class/scheme. Ordinary unmarked/catalog behavior remains.
- `be/src/format_v2/table/lance_reader.cpp:220` — every non-bare LOCAL URI uses the pinned SDK actual local_path, including whole-string malformed-file fallback, before canonical/tree/base policy. Distinct real schema/key/row identity and narrow-root rejection cover the repair.
- Review covers all65 files, converged in round1 with zero findings. Parent source-bound selected evidence is125 FE and20 BE tests per release; this review ran no builds/tests and does not claim complete CI/SQL/mixed-version or successful optional file+uring execution.
<sub>Reviewed locally with the `doris-repo-review` pipeline. Repository policy may accept this receipt for the matching commit; it is not a human Apache approval.</sub>
<!-- doris-repo-review:v1:end -->
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6377100900
# FE Regression Coverage Report
Increment line coverage ` 33.33% (9/27)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/69528_c4103b6ec3f366ce9096ea20424e998585edbe60_merge_fe/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/69528_c4103b6ec3f366ce9096ea20424e998585edbe60_merge_fe/report/index.html)
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6386528284
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6383030951
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6389262069
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4368505894
##########
be/src/format_v2/table/lance_reader.cpp:
##########
@@ -22,24 +22,218 @@
#include <arrow/record_batch.h>
#include <arrow/type.h>
#include <lance/lance.h>
+#include <pwd.h>
+#include <rapidjson/document.h>
+#include <unistd.h>
#include <algorithm>
#include <bit>
+#include <cerrno>
+#include <cstdlib>
#include <cstring>
+#include <filesystem>
#include <limits>
#include <memory>
+#include <unordered_set>
#include "common/config.h"
#include "common/logging.h"
#include "exec/common/endian.h"
#include "format_v2/lance/lance_reader_helper.h"
#include "format_v2/lance/lance_runtime_filter_helper.h"
#include "format_v2/lance/lance_session_manager.h"
+#include "io/fs/local_file_system.h"
#include "runtime/file_scan_profile.h"
#include "runtime/runtime_state.h"
+#ifndef LANCE_STORAGE_BASES_JSON_VERSION
+#error "Rebuild lance-c with TVF storage-base metadata support before building Doris"
+#endif
+
namespace doris::format::lance {
+namespace {
+std::string expand_home_path(const std::string& path) {
+ if (path != "~" && !path.starts_with("~/")) {
+ return path;
+ }
+ // Match the SDK's std::env::home_dir: HOME first, then the current user's passwd entry.
+ std::string home;
+ if (const char* value = std::getenv("HOME"); value != nullptr && value[0] != '\0') {
+ home = value;
+ } else {
+ std::vector<char> buffer(1024);
+ struct passwd entry {};
+ struct passwd* result = nullptr;
+ int error;
+ while ((error = getpwuid_r(getuid(), &entry, buffer.data(), buffer.size(), &result)) ==
+ ERANGE) {
+ buffer.resize(buffer.size() * 2);
+ }
+ if (error == 0 && result != nullptr && entry.pw_dir != nullptr) {
+ home = entry.pw_dir;
+ }
+ }
+ if (home.empty()) {
+ return path;
+ }
+ return path == "~" ? home : (io::Path(home) / path.substr(2)).string();
+}
+
+Status validate_local_dataset_files(const std::string& dataset_uri) {
+ // Lance opens manifests and fragments itself, so a confined dataset directory is not
+ // enough: its descendants can redirect those opens through symlinks.
+ std::error_code error;
+ const auto secure_root = std::filesystem::canonical(config::user_files_secure_path, error);
+ if (error) {
+ return Status::InvalidArgument("local Lance dataset path is not allowed");
+ }
+ std::vector<std::filesystem::path> pending {dataset_uri};
+ std::unordered_set<std::string> visited;
+ while (!pending.empty()) {
+ auto directory = std::move(pending.back());
+ pending.pop_back();
+ // Canonical directory identity bounds cycles and repeated in-root aliases without
+ // imposing a file-count or nesting limit on otherwise valid datasets.
+ if (!visited.insert(directory.string()).second) {
+ continue;
+ }
+ std::filesystem::directory_iterator entries(directory, error);
+ if (error) {
+ return Status::InvalidArgument("local Lance dataset path is not allowed");
+ }
+ const std::filesystem::directory_iterator end;
+ while (entries != end) {
+ auto target = entries->path();
+ auto status = entries->symlink_status(error);
+ if (error) {
+ return Status::InvalidArgument("local Lance dataset path is not allowed");
+ }
+ if (std::filesystem::is_symlink(status)) {
+ target = std::filesystem::canonical(target, error);
+ if (error || !io::LocalFileSystem::contain_path(secure_root, target)) {
+ return Status::InvalidArgument("local Lance dataset path is not allowed");
+ }
+ status = std::filesystem::status(target, error);
+ if (error) {
+ return Status::InvalidArgument("local Lance dataset path is not allowed");
+ }
+ }
+ if (std::filesystem::is_directory(status)) {
+ pending.push_back(std::move(target));
+ }
+ entries.increment(error);
+ if (error) {
+ return Status::InvalidArgument("local Lance dataset path is not allowed");
+ }
+ }
+ }
+ return Status::OK();
+}
+
+bool is_local_lance_tvf(const TFileRangeDesc& range, const TFileScanRangeParams& scan_params) {
+ const bool local = range.__isset.file_type
+ ? range.file_type == TFileType::FILE_LOCAL
+ : scan_params.__isset.file_type &&
+ scan_params.file_type == TFileType::FILE_LOCAL;
+ const auto& params = range.table_format_params.lance_params;
+ return local && (params.version == 0 || (params.__isset.is_tvf && params.is_tvf));
+}
+
+Status resolve_dataset_uri(const TFileRangeDesc& range, const TFileScanRangeParams& scan_params,
+ std::string* uri) {
+ const auto& requested_uri = range.table_format_params.lance_params.dataset_uri;
+ // Explicit TVF identity applies regardless of snapshot selection. Older LOCAL TVFs
+ // used version zero; configured fixed-version catalogs do not send the TVF marker.
+ if (is_local_lance_tvf(range, scan_params)) {
+ // LOCAL Lance bypasses glob because it opens a dataset directory. Apply the same
+ // canonical secure-root check before either schema discovery or a cached dataset open.
+ std::string local_input = requested_uri;
+ const auto colon = requested_uri.find(':');
+ const auto slash = requested_uri.find('/');
+ const bool bare_path =
+ colon == std::string::npos || (slash != std::string::npos && slash < colon);
+ if (bare_path && !requested_uri.empty() && requested_uri.find('\0') == std::string::npos &&
+ !config::user_files_secure_path.empty()) {
+ // Lance's PathAbs fallback resolves bare dot segments before following symlinks.
+ // Anchor relative TVF paths to the canonical secure root, not the process cwd.
+ std::string secure_root;
+ RETURN_IF_ERROR(io::global_local_filesystem()->canonicalize(
+ config::user_files_secure_path, &secure_root));
+ local_input = (io::Path(secure_root) / expand_home_path(requested_uri))
+ .lexically_normal()
+ .string();
+ }
+ RETURN_IF_ERROR(io::global_local_filesystem()->canonicalize_local_file(
Review Comment:
[P2] Preserve Lance's supported local object-store alias
With LOCAL enabled, an ADMIN/OPERATOR, `user_files_secure_path=/secure` and an existing `/secure/data.lance`, `local('file_path'='file-object-store:///secure/data.lance', 'backend_id'='...', 'format'='lance')` previously reached schema discovery and scanning. [The pinned SDK registers this alias as a local FileStoreProvider](https://github.com/lance-format/lance/blob/68c12dfd7efe02f90ad2d7f3a239a7eb64884e57/rust/lance-io/src/object_store/providers.rs#L371-L380), and FE preserves this valid URI through split construction. This new call sends it through `canonicalize_local_file()` and `convert_to_abs_path()`, which accepts only the `file` scheme, so schema discovery now fails before the SDK opens the in-root dataset. Resolve this registered local provider to its SDK filesystem path before canonical containment, preserve the requested provider when reopening, and add schema/scan coverage. This is a distinct supported-provider case rather than the existing file-URI escaping, dot, whit
espace or home-expansion cases.
##########
fe/be-java-extensions/hadoop-deps/src/main/java/org/apache/hadoop/fs/FileSystem.java:
##########
@@ -3622,13 +3633,628 @@ public static Class<? extends FileSystem> getFileSystemClass(String scheme,
* @return the initialized filesystem.
* @throws IOException problems loading or initializing the FileSystem
*/
+ // DORIS-PATCH: TVF policy is checked against the frozen configuration used by the consumer.
+ public static int getDorisTvfGuardVersion() {
+ return 1;
+ }
+
+ public static final class TvfGuard {
+ public static final String POLICY_KEY = "fs.doris.tvf.remote.paths";
+ public static final String NATIVE_POLICY = "native";
+ public static final String JAVA_READ_POLICY = "java-read";
+ public static final String ROOT_KEY = "fs.doris.tvf.root";
+ public static final String PATH_KEY = "fs.doris.tvf.path";
+ public static final String CONTEXT_KEY = "fs.doris.tvf.context";
+ private static final ThreadLocal<Context> LAST_NATIVE_CONTEXT = new ThreadLocal<>();
+
+ private TvfGuard() {
+ }
+
+ public static final class Rule {
+ private final String filesystem;
+ private final String path;
+ private final boolean descendants;
+
+ public Rule(String filesystem, String path, boolean descendants) {
+ this.filesystem = filesystem;
+ this.path = path;
+ this.descendants = descendants;
+ }
+ }
+
+ public static String encode(List<Rule> rules) {
+ if (FileSystem.getDorisTvfGuardVersion() != 1) {
+ throw new IllegalStateException("TVF Hadoop runtime policy version mismatch");
+ }
+ try {
+ java.io.ByteArrayOutputStream bytes = new java.io.ByteArrayOutputStream();
+ java.io.DataOutputStream out = new java.io.DataOutputStream(bytes);
+ out.writeInt(1);
+ out.writeInt(rules.size());
+ for (Rule rule : rules) {
+ out.writeUTF(rule.filesystem);
+ out.writeUTF(rule.path);
+ out.writeBoolean(rule.descendants);
+ }
+ out.flush();
+ return java.util.Base64.getEncoder().encodeToString(bytes.toByteArray());
+ } catch (IOException e) {
+ throw new IllegalArgumentException("Cannot encode TVF Hadoop route policy", e);
+ }
+ }
+
+ private static IOException rejected() {
+ return new IOException("Hadoop TVF requires a proven remote filesystem route. "
+ + "Use local() with LOCAL TVF privileges and secure-path restrictions.");
+ }
+
+ private static Configuration freeze(Configuration conf) throws IOException {
+ // Service providers (notably HDFS) register their default/site resources on first load.
+ // Load them before copying raw values, as an ordinary FileSystem lookup does.
+ if (!FILE_SYSTEMS_LOADED) {
+ loadFileSystems();
+ }
+ if (conf instanceof FrozenConfiguration) {
+ return conf;
+ }
+ return new FrozenConfiguration(conf);
+ }
+
+ private static Configuration prepare(Configuration conf) throws IOException {
+ String policy = conf.getRaw(POLICY_KEY);
+ if (policy == null || policy.isEmpty()) {
+ return conf;
+ }
+ Configuration frozen = freeze(conf);
+ Context context = ((FrozenConfiguration) frozen).context;
+ if (NATIVE_POLICY.equals(policy)) {
+ context.checkNative(frozen.getRaw(ROOT_KEY), frozen.getRaw(PATH_KEY));
+ } else if (JAVA_READ_POLICY.equals(policy)) {
+ context.checkJava(new Path(frozen.getRaw(PATH_KEY)));
+ } else {
+ context.checkRules(policy);
+ }
+ return frozen;
+ }
+
+ public static void check(URI ignored, Configuration conf) throws IOException {
+ prepare(conf);
+ }
+
+ public static void clearNativeContext() {
+ LAST_NATIVE_CONTEXT.remove();
+ }
+
+ public static Context takeNativeContext() {
+ Context result = LAST_NATIVE_CONTEXT.get();
+ LAST_NATIVE_CONTEXT.remove();
+ return result;
+ }
+
+ public static Configuration prepareJavaRead(Configuration conf, Path path, String marker) throws IOException {
+ if (marker == null || marker.isEmpty()) {
+ return conf;
+ }
+ if (!NATIVE_POLICY.equals(marker)) {
+ throw rejected();
+ }
+ Configuration request = new Configuration(conf);
+ request.set(POLICY_KEY, JAVA_READ_POLICY);
+ request.set(ROOT_KEY, path.toUri().toString());
+ request.set(PATH_KEY, path.toString());
+ return prepare(request);
+ }
+
+ public static void checkJavaRead(FileSystem fs, Path path, String marker) throws IOException {
+ if (marker == null || marker.isEmpty()) {
+ return;
+ }
+ if (!NATIVE_POLICY.equals(marker)) {
+ throw rejected();
+ }
+ // The marker is request-owned, never inferred from an unmarked cached filesystem.
+ Configuration conf = fs.getConf();
+ if (JAVA_READ_POLICY.equals(conf.getRaw(POLICY_KEY))) {
+ // S3A derives a Configuration for bucket options. Its copy preserves our internal
+ // marker and raw snapshot values, but not the private Configuration subclass.
+ ((FrozenConfiguration) freeze(conf)).context.checkJava(path);
+ } else {
+ throw rejected();
+ }
+ }
+
+ private static final class FrozenConfiguration extends Configuration {
+ private final Context context;
+
+ private FrozenConfiguration(Configuration source) throws IOException {
+ super(false);
+ setClassLoader(source.getClassLoader());
+ java.util.SortedMap<String, String> values = new java.util.TreeMap<>();
+ for (Map.Entry<String, String> entry : source) {
+ String key = entry.getKey();
+ String value = source.getRaw(key);
+ set(key, value);
+ if (!PATH_KEY.equals(key) && !ROOT_KEY.equals(key) && !CONTEXT_KEY.equals(key)) {
+ values.put(key, value);
+ }
+ }
+ try {
+ java.security.MessageDigest digest = java.security.MessageDigest.getInstance("SHA-256");
+ java.io.DataOutputStream output = new java.io.DataOutputStream(
+ new java.security.DigestOutputStream(new java.io.OutputStream() {
+ @Override
+ public void write(int value) {
+ }
+
+ @Override
+ public void write(byte[] values, int offset, int length) {
+ }
+ }, digest));
+ for (Map.Entry<String, String> entry : values.entrySet()) {
+ for (String value : new String[] {entry.getKey(), entry.getValue()}) {
+ // Preserve raw Java code units, including unpaired surrogates. UTF-8 replacement
+ // would give a literal '?' and an unpaired surrogate the same cache profile.
+ output.writeInt(value.length());
+ output.writeChars(value);
+ }
+ }
+ output.flush();
+ StringBuilder identity = new StringBuilder();
+ for (byte value : digest.digest()) {
+ identity.append(Character.forDigit((value & 255) >>> 4, 16));
+ identity.append(Character.forDigit(value & 15, 16));
+ }
+ set(CONTEXT_KEY, identity.toString());
+ } catch (java.security.NoSuchAlgorithmException e) {
+ throw new IOException("SHA-256 is required for TVF Hadoop cache isolation");
+ }
+ context = new Context(this);
+ }
+ }
+
+ public static final class Context {
+ private final Configuration conf;
+ private final Map<String, String> routingKeys = new HashMap<>();
+
+ private Context(Configuration conf) {
+ this.conf = conf;
+ for (Map.Entry<String, String> entry : conf) {
+ String key = entry.getKey();
+ if ("fs.defaultFS".equals(key) || "fs.default.name".equals(key)
+ || key.startsWith("fs.viewfs.") || (key.startsWith("fs.") && key.endsWith(".impl"))) {
+ routingKeys.put(key, entry.getValue());
+ }
+ }
+ }
+
+ private EffectiveRoute route(boolean descendants) {
+ return new EffectiveRoute(routingKeys, descendants, conf);
+ }
+
+ public String profile() {
+ return conf.getRaw(CONTEXT_KEY);
+ }
+
+ public boolean isDirectRemote() throws IOException {
+ String root = conf.getRaw(ROOT_KEY);
+ if (root == null) {
+ throw rejected();
+ }
+ try {
+ URI uri = nativeUri(root);
+ return route(false).provider(uri.getScheme()) == Route.Provider.REMOTE;
+ } catch (RuntimeException | URISyntaxException e) {
+ throw rejected();
+ }
+ }
+
+ private static URI nativeUri(String root) throws URISyntaxException {
+ root = root.trim();
+ if (root.isEmpty() || "default".equals(root)) {
+ throw new URISyntaxException("", "Unproven native filesystem");
+ }
+ return new URI(root.contains("://") ? root : "hdfs://" + root);
+ }
+
+ public void checkNative(String root, String path) throws IOException {
+ if (root == null || path == null || path.isEmpty()) {
+ throw rejected();
+ }
+ try {
+ URI uri = nativeUri(root);
+ String actual = new Path(path).toUri().getPath();
+ route(false).checkRoute(uri, actual, new HashSet<>());
+ } catch (RuntimeException | URISyntaxException e) {
+ throw rejected();
+ }
+ }
+
+ private void checkJava(Path path) throws IOException {
+ try {
+ URI uri = path.toUri();
+ route(false).checkRoute(uri, uri.getPath(), new HashSet<>());
+ } catch (RuntimeException | URISyntaxException e) {
+ throw rejected();
+ }
+ }
+
+ private void checkRules(String policy) throws IOException {
+ try {
+ byte[] bytes = java.util.Base64.getDecoder().decode(policy);
+ java.io.DataInputStream in = new java.io.DataInputStream(new java.io.ByteArrayInputStream(bytes));
+ int version = in.readInt();
+ int count = in.readInt();
+ if (version != 1 || count <= 0 || count > in.available() / 5) {
+ throw rejected();
+ }
+ for (int i = 0; i < count; i++) {
+ URI filesystem = new URI(in.readUTF());
+ String path = in.readUTF();
+ boolean descendants = in.readBoolean();
+ route(descendants).checkRoute(filesystem, path, new HashSet<>());
+ }
+ if (in.available() != 0) {
+ throw rejected();
+ }
+ } catch (RuntimeException | URISyntaxException e) {
+ throw rejected();
+ }
+ }
+ }
+
+ private static final class EffectiveRoute extends Route {
+ private final Configuration conf;
+
+ private EffectiveRoute(Map<String, String> properties, boolean descendants, Configuration conf) {
+ super(properties, descendants);
+ this.conf = conf;
+ }
+
+ @Override
+ protected String property(String key) {
+ return conf.get(key);
+ }
+
+ @Override
+ protected Provider provider(String scheme) {
+ if ("file".equalsIgnoreCase(scheme)) {
+ return Provider.LOCAL;
+ }
+ try {
+ String implementation = FileSystem.getFileSystemClass(scheme, conf).getName();
+ if (Arrays.asList("io.juicefs.JuiceFileSystem", "org.apache.hadoop.fs.ozone.OzoneFileSystem",
+ "org.apache.hadoop.fs.ozone.RootedOzoneFileSystem", "org.apache.hadoop.fs.cosn.CosNFileSystem",
+ "com.qcloud.chdfs.fs.CHDFSHadoopFileSystemAdapter").contains(implementation)) {
+ return Provider.REMOTE;
+ }
+ return implementationProvider(implementation);
+ } catch (IOException | RuntimeException e) {
+ return Provider.UNKNOWN;
+ }
+ }
+ }
+
+ public static class Route {
+ protected enum Provider {
+ REMOTE,
+ LOCAL,
+ MOUNT_TABLE,
+ DYNAMIC,
+ UNKNOWN
+ }
+
+ protected static final Set<String> REMOTE_SCHEMES = new HashSet<>(Arrays.asList(
+ "hdfs", "s3a", "s3n", "oss", "ofs", "jfs", "abfs", "abfss", "wasb", "wasbs", "gs", "cosn"));
+ protected static final Set<String> REMOTE_IMPLEMENTATIONS = new HashSet<>(Arrays.asList(
+ "org.apache.hadoop.hdfs.DistributedFileSystem",
+ "org.apache.hadoop.fs.s3a.S3AFileSystem",
+ "org.apache.hadoop.fs.azurebfs.AzureBlobFileSystem",
+ "org.apache.hadoop.fs.azurebfs.SecureAzureBlobFileSystem",
+ "org.apache.hadoop.fs.azure.NativeAzureFileSystem",
Review Comment:
[P2] Admit Hadoop's standard secure WASB implementation
With LOCAL disabled, a native `INSERT INTO hdfs('file_path'='/exports/prefix_', 'format'='csv', 'fs.defaultFS'='wasbs://container@account.blob.core.windows.net', ...)` with valid Azure credentials passes FE scheme admission, but the consumer rejects it before initialization. [Hadoop's default provider](https://github.com/apache/doris-thirdparty/blob/hadoop-3.4.2.2-for-doris/hadoop-common-project/hadoop-common/src/main/resources/core-default.xml#L2231-L2234) is `org.apache.hadoop.fs.azure.NativeAzureFileSystem$Secure`; `EffectiveRoute.provider()` resolves that exact class and `implementationProvider()` returns UNKNOWN because this set includes only the base class. Valid ViewFS links to `wasbs` fail for the same reason. Include the standard Secure provider and add default/inherited runtime controls for direct and delegated secure WASB routes.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4369659423
##########
fe/be-java-extensions/hadoop-deps/src/main/java/org/apache/hadoop/fs/FileSystem.java:
##########
@@ -3622,13 +3633,628 @@ public static Class<? extends FileSystem> getFileSystemClass(String scheme,
* @return the initialized filesystem.
* @throws IOException problems loading or initializing the FileSystem
*/
+ // DORIS-PATCH: TVF policy is checked against the frozen configuration used by the consumer.
+ public static int getDorisTvfGuardVersion() {
+ return 1;
+ }
+
+ public static final class TvfGuard {
+ public static final String POLICY_KEY = "fs.doris.tvf.remote.paths";
+ public static final String NATIVE_POLICY = "native";
+ public static final String JAVA_READ_POLICY = "java-read";
+ public static final String ROOT_KEY = "fs.doris.tvf.root";
+ public static final String PATH_KEY = "fs.doris.tvf.path";
+ public static final String CONTEXT_KEY = "fs.doris.tvf.context";
+ private static final ThreadLocal<Context> LAST_NATIVE_CONTEXT = new ThreadLocal<>();
+
+ private TvfGuard() {
+ }
+
+ public static final class Rule {
+ private final String filesystem;
+ private final String path;
+ private final boolean descendants;
+
+ public Rule(String filesystem, String path, boolean descendants) {
+ this.filesystem = filesystem;
+ this.path = path;
+ this.descendants = descendants;
+ }
+ }
+
+ public static String encode(List<Rule> rules) {
+ if (FileSystem.getDorisTvfGuardVersion() != 1) {
+ throw new IllegalStateException("TVF Hadoop runtime policy version mismatch");
+ }
+ try {
+ java.io.ByteArrayOutputStream bytes = new java.io.ByteArrayOutputStream();
+ java.io.DataOutputStream out = new java.io.DataOutputStream(bytes);
+ out.writeInt(1);
+ out.writeInt(rules.size());
+ for (Rule rule : rules) {
+ out.writeUTF(rule.filesystem);
+ out.writeUTF(rule.path);
+ out.writeBoolean(rule.descendants);
+ }
+ out.flush();
+ return java.util.Base64.getEncoder().encodeToString(bytes.toByteArray());
+ } catch (IOException e) {
+ throw new IllegalArgumentException("Cannot encode TVF Hadoop route policy", e);
+ }
+ }
+
+ private static IOException rejected() {
+ return new IOException("Hadoop TVF requires a proven remote filesystem route. "
+ + "Use local() with LOCAL TVF privileges and secure-path restrictions.");
+ }
+
+ private static Configuration freeze(Configuration conf) throws IOException {
+ // Service providers (notably HDFS) register their default/site resources on first load.
+ // Load them before copying raw values, as an ordinary FileSystem lookup does.
+ if (!FILE_SYSTEMS_LOADED) {
+ loadFileSystems();
+ }
+ if (conf instanceof FrozenConfiguration) {
+ return conf;
+ }
+ return new FrozenConfiguration(conf);
+ }
+
+ private static Configuration prepare(Configuration conf) throws IOException {
+ String policy = conf.getRaw(POLICY_KEY);
+ if (policy == null || policy.isEmpty()) {
+ return conf;
+ }
+ Configuration frozen = freeze(conf);
+ Context context = ((FrozenConfiguration) frozen).context;
+ if (NATIVE_POLICY.equals(policy)) {
+ context.checkNative(frozen.getRaw(ROOT_KEY), frozen.getRaw(PATH_KEY));
+ } else if (JAVA_READ_POLICY.equals(policy)) {
+ context.checkJava(new Path(frozen.getRaw(PATH_KEY)));
+ } else {
+ context.checkRules(policy);
+ }
+ return frozen;
+ }
+
+ public static void check(URI ignored, Configuration conf) throws IOException {
+ prepare(conf);
+ }
+
+ public static void clearNativeContext() {
+ LAST_NATIVE_CONTEXT.remove();
+ }
+
+ public static Context takeNativeContext() {
+ Context result = LAST_NATIVE_CONTEXT.get();
+ LAST_NATIVE_CONTEXT.remove();
+ return result;
+ }
+
+ public static Configuration prepareJavaRead(Configuration conf, Path path, String marker) throws IOException {
+ if (marker == null || marker.isEmpty()) {
+ return conf;
+ }
+ if (!NATIVE_POLICY.equals(marker)) {
+ throw rejected();
+ }
+ Configuration request = new Configuration(conf);
+ request.set(POLICY_KEY, JAVA_READ_POLICY);
+ request.set(ROOT_KEY, path.toUri().toString());
+ request.set(PATH_KEY, path.toString());
+ return prepare(request);
+ }
+
+ public static void checkJavaRead(FileSystem fs, Path path, String marker) throws IOException {
+ if (marker == null || marker.isEmpty()) {
+ return;
+ }
+ if (!NATIVE_POLICY.equals(marker)) {
+ throw rejected();
+ }
+ // The marker is request-owned, never inferred from an unmarked cached filesystem.
+ Configuration conf = fs.getConf();
+ if (JAVA_READ_POLICY.equals(conf.getRaw(POLICY_KEY))) {
+ // S3A derives a Configuration for bucket options. Its copy preserves our internal
+ // marker and raw snapshot values, but not the private Configuration subclass.
+ ((FrozenConfiguration) freeze(conf)).context.checkJava(path);
+ } else {
+ throw rejected();
+ }
+ }
+
+ private static final class FrozenConfiguration extends Configuration {
+ private final Context context;
+
+ private FrozenConfiguration(Configuration source) throws IOException {
+ super(false);
+ setClassLoader(source.getClassLoader());
+ java.util.SortedMap<String, String> values = new java.util.TreeMap<>();
+ for (Map.Entry<String, String> entry : source) {
+ String key = entry.getKey();
+ String value = source.getRaw(key);
+ set(key, value);
+ if (!PATH_KEY.equals(key) && !ROOT_KEY.equals(key) && !CONTEXT_KEY.equals(key)) {
+ values.put(key, value);
+ }
+ }
+ try {
+ java.security.MessageDigest digest = java.security.MessageDigest.getInstance("SHA-256");
+ java.io.DataOutputStream output = new java.io.DataOutputStream(
+ new java.security.DigestOutputStream(new java.io.OutputStream() {
+ @Override
+ public void write(int value) {
+ }
+
+ @Override
+ public void write(byte[] values, int offset, int length) {
+ }
+ }, digest));
+ for (Map.Entry<String, String> entry : values.entrySet()) {
+ for (String value : new String[] {entry.getKey(), entry.getValue()}) {
+ // Preserve raw Java code units, including unpaired surrogates. UTF-8 replacement
+ // would give a literal '?' and an unpaired surrogate the same cache profile.
+ output.writeInt(value.length());
+ output.writeChars(value);
+ }
+ }
+ output.flush();
+ StringBuilder identity = new StringBuilder();
+ for (byte value : digest.digest()) {
+ identity.append(Character.forDigit((value & 255) >>> 4, 16));
+ identity.append(Character.forDigit(value & 15, 16));
+ }
+ set(CONTEXT_KEY, identity.toString());
+ } catch (java.security.NoSuchAlgorithmException e) {
+ throw new IOException("SHA-256 is required for TVF Hadoop cache isolation");
+ }
+ context = new Context(this);
+ }
+ }
+
+ public static final class Context {
+ private final Configuration conf;
+ private final Map<String, String> routingKeys = new HashMap<>();
+
+ private Context(Configuration conf) {
+ this.conf = conf;
+ for (Map.Entry<String, String> entry : conf) {
+ String key = entry.getKey();
+ if ("fs.defaultFS".equals(key) || "fs.default.name".equals(key)
+ || key.startsWith("fs.viewfs.") || (key.startsWith("fs.") && key.endsWith(".impl"))) {
+ routingKeys.put(key, entry.getValue());
+ }
+ }
+ }
+
+ private EffectiveRoute route(boolean descendants) {
+ return new EffectiveRoute(routingKeys, descendants, conf);
+ }
+
+ public String profile() {
+ return conf.getRaw(CONTEXT_KEY);
+ }
+
+ public boolean isDirectRemote() throws IOException {
+ String root = conf.getRaw(ROOT_KEY);
+ if (root == null) {
+ throw rejected();
+ }
+ try {
+ URI uri = nativeUri(root);
+ return route(false).provider(uri.getScheme()) == Route.Provider.REMOTE;
+ } catch (RuntimeException | URISyntaxException e) {
+ throw rejected();
+ }
+ }
+
+ private static URI nativeUri(String root) throws URISyntaxException {
+ root = root.trim();
+ if (root.isEmpty() || "default".equals(root)) {
+ throw new URISyntaxException("", "Unproven native filesystem");
+ }
+ return new URI(root.contains("://") ? root : "hdfs://" + root);
+ }
+
+ public void checkNative(String root, String path) throws IOException {
+ if (root == null || path == null || path.isEmpty()) {
+ throw rejected();
+ }
+ try {
+ URI uri = nativeUri(root);
+ String actual = new Path(path).toUri().getPath();
+ route(false).checkRoute(uri, actual, new HashSet<>());
+ } catch (RuntimeException | URISyntaxException e) {
+ throw rejected();
+ }
+ }
+
+ private void checkJava(Path path) throws IOException {
+ try {
+ URI uri = path.toUri();
+ route(false).checkRoute(uri, uri.getPath(), new HashSet<>());
+ } catch (RuntimeException | URISyntaxException e) {
+ throw rejected();
+ }
+ }
+
+ private void checkRules(String policy) throws IOException {
+ try {
+ byte[] bytes = java.util.Base64.getDecoder().decode(policy);
+ java.io.DataInputStream in = new java.io.DataInputStream(new java.io.ByteArrayInputStream(bytes));
+ int version = in.readInt();
+ int count = in.readInt();
+ if (version != 1 || count <= 0 || count > in.available() / 5) {
+ throw rejected();
+ }
+ for (int i = 0; i < count; i++) {
+ URI filesystem = new URI(in.readUTF());
+ String path = in.readUTF();
+ boolean descendants = in.readBoolean();
+ route(descendants).checkRoute(filesystem, path, new HashSet<>());
+ }
+ if (in.available() != 0) {
+ throw rejected();
+ }
+ } catch (RuntimeException | URISyntaxException e) {
+ throw rejected();
+ }
+ }
+ }
+
+ private static final class EffectiveRoute extends Route {
+ private final Configuration conf;
+
+ private EffectiveRoute(Map<String, String> properties, boolean descendants, Configuration conf) {
+ super(properties, descendants);
+ this.conf = conf;
+ }
+
+ @Override
+ protected String property(String key) {
+ return conf.get(key);
+ }
+
+ @Override
+ protected Provider provider(String scheme) {
+ if ("file".equalsIgnoreCase(scheme)) {
+ return Provider.LOCAL;
+ }
+ try {
+ String implementation = FileSystem.getFileSystemClass(scheme, conf).getName();
+ if (Arrays.asList("io.juicefs.JuiceFileSystem", "org.apache.hadoop.fs.ozone.OzoneFileSystem",
+ "org.apache.hadoop.fs.ozone.RootedOzoneFileSystem", "org.apache.hadoop.fs.cosn.CosNFileSystem",
+ "com.qcloud.chdfs.fs.CHDFSHadoopFileSystemAdapter").contains(implementation)) {
+ return Provider.REMOTE;
+ }
+ return implementationProvider(implementation);
+ } catch (IOException | RuntimeException e) {
+ return Provider.UNKNOWN;
+ }
+ }
+ }
+
+ public static class Route {
+ protected enum Provider {
+ REMOTE,
+ LOCAL,
+ MOUNT_TABLE,
+ DYNAMIC,
+ UNKNOWN
+ }
+
+ protected static final Set<String> REMOTE_SCHEMES = new HashSet<>(Arrays.asList(
+ "hdfs", "s3a", "s3n", "oss", "ofs", "jfs", "abfs", "abfss", "wasb", "wasbs", "gs", "cosn"));
+ protected static final Set<String> REMOTE_IMPLEMENTATIONS = new HashSet<>(Arrays.asList(
+ "org.apache.hadoop.hdfs.DistributedFileSystem",
+ "org.apache.hadoop.fs.s3a.S3AFileSystem",
+ "org.apache.hadoop.fs.azurebfs.AzureBlobFileSystem",
+ "org.apache.hadoop.fs.azurebfs.SecureAzureBlobFileSystem",
+ "org.apache.hadoop.fs.azure.NativeAzureFileSystem",
Review Comment:
Fixed in fb92e7d8c337c86e6e90fe135bbd4a5d995115b0. The shared exact-provider set includes Hadoop's standard NativeAzureFileSystem$Secure. Controls cover actual default/inherited direct and delegated WASBS routes, plus local override rejection. The same fix is synchronized across all three PRs.
Validation: Final standard JDK17 FE: 107 selected tests passed with fresh XML, zero failures/errors/skips and frozen source. Final standard Clang21/ASAN BE: 28 selected tests passed. Each release also passed 16 Lance and 19 Paimon build-control tests; the new SDK unit/C API tests, matching Rust std/ABI checks and private standard build passed. Independent Standards/Spec rechecks found no remaining confirmed issue in this follow-up.
##########
be/src/format_v2/table/lance_reader.cpp:
##########
@@ -22,24 +22,218 @@
#include <arrow/record_batch.h>
#include <arrow/type.h>
#include <lance/lance.h>
+#include <pwd.h>
+#include <rapidjson/document.h>
+#include <unistd.h>
#include <algorithm>
#include <bit>
+#include <cerrno>
+#include <cstdlib>
#include <cstring>
+#include <filesystem>
#include <limits>
#include <memory>
+#include <unordered_set>
#include "common/config.h"
#include "common/logging.h"
#include "exec/common/endian.h"
#include "format_v2/lance/lance_reader_helper.h"
#include "format_v2/lance/lance_runtime_filter_helper.h"
#include "format_v2/lance/lance_session_manager.h"
+#include "io/fs/local_file_system.h"
#include "runtime/file_scan_profile.h"
#include "runtime/runtime_state.h"
+#ifndef LANCE_STORAGE_BASES_JSON_VERSION
+#error "Rebuild lance-c with TVF storage-base metadata support before building Doris"
+#endif
+
namespace doris::format::lance {
+namespace {
+std::string expand_home_path(const std::string& path) {
+ if (path != "~" && !path.starts_with("~/")) {
+ return path;
+ }
+ // Match the SDK's std::env::home_dir: HOME first, then the current user's passwd entry.
+ std::string home;
+ if (const char* value = std::getenv("HOME"); value != nullptr && value[0] != '\0') {
+ home = value;
+ } else {
+ std::vector<char> buffer(1024);
+ struct passwd entry {};
+ struct passwd* result = nullptr;
+ int error;
+ while ((error = getpwuid_r(getuid(), &entry, buffer.data(), buffer.size(), &result)) ==
+ ERANGE) {
+ buffer.resize(buffer.size() * 2);
+ }
+ if (error == 0 && result != nullptr && entry.pw_dir != nullptr) {
+ home = entry.pw_dir;
+ }
+ }
+ if (home.empty()) {
+ return path;
+ }
+ return path == "~" ? home : (io::Path(home) / path.substr(2)).string();
+}
+
+Status validate_local_dataset_files(const std::string& dataset_uri) {
+ // Lance opens manifests and fragments itself, so a confined dataset directory is not
+ // enough: its descendants can redirect those opens through symlinks.
+ std::error_code error;
+ const auto secure_root = std::filesystem::canonical(config::user_files_secure_path, error);
+ if (error) {
+ return Status::InvalidArgument("local Lance dataset path is not allowed");
+ }
+ std::vector<std::filesystem::path> pending {dataset_uri};
+ std::unordered_set<std::string> visited;
+ while (!pending.empty()) {
+ auto directory = std::move(pending.back());
+ pending.pop_back();
+ // Canonical directory identity bounds cycles and repeated in-root aliases without
+ // imposing a file-count or nesting limit on otherwise valid datasets.
+ if (!visited.insert(directory.string()).second) {
+ continue;
+ }
+ std::filesystem::directory_iterator entries(directory, error);
+ if (error) {
+ return Status::InvalidArgument("local Lance dataset path is not allowed");
+ }
+ const std::filesystem::directory_iterator end;
+ while (entries != end) {
+ auto target = entries->path();
+ auto status = entries->symlink_status(error);
+ if (error) {
+ return Status::InvalidArgument("local Lance dataset path is not allowed");
+ }
+ if (std::filesystem::is_symlink(status)) {
+ target = std::filesystem::canonical(target, error);
+ if (error || !io::LocalFileSystem::contain_path(secure_root, target)) {
+ return Status::InvalidArgument("local Lance dataset path is not allowed");
+ }
+ status = std::filesystem::status(target, error);
+ if (error) {
+ return Status::InvalidArgument("local Lance dataset path is not allowed");
+ }
+ }
+ if (std::filesystem::is_directory(status)) {
+ pending.push_back(std::move(target));
+ }
+ entries.increment(error);
+ if (error) {
+ return Status::InvalidArgument("local Lance dataset path is not allowed");
+ }
+ }
+ }
+ return Status::OK();
+}
+
+bool is_local_lance_tvf(const TFileRangeDesc& range, const TFileScanRangeParams& scan_params) {
+ const bool local = range.__isset.file_type
+ ? range.file_type == TFileType::FILE_LOCAL
+ : scan_params.__isset.file_type &&
+ scan_params.file_type == TFileType::FILE_LOCAL;
+ const auto& params = range.table_format_params.lance_params;
+ return local && (params.version == 0 || (params.__isset.is_tvf && params.is_tvf));
+}
+
+Status resolve_dataset_uri(const TFileRangeDesc& range, const TFileScanRangeParams& scan_params,
+ std::string* uri) {
+ const auto& requested_uri = range.table_format_params.lance_params.dataset_uri;
+ // Explicit TVF identity applies regardless of snapshot selection. Older LOCAL TVFs
+ // used version zero; configured fixed-version catalogs do not send the TVF marker.
+ if (is_local_lance_tvf(range, scan_params)) {
+ // LOCAL Lance bypasses glob because it opens a dataset directory. Apply the same
+ // canonical secure-root check before either schema discovery or a cached dataset open.
+ std::string local_input = requested_uri;
+ const auto colon = requested_uri.find(':');
+ const auto slash = requested_uri.find('/');
+ const bool bare_path =
+ colon == std::string::npos || (slash != std::string::npos && slash < colon);
+ if (bare_path && !requested_uri.empty() && requested_uri.find('\0') == std::string::npos &&
+ !config::user_files_secure_path.empty()) {
+ // Lance's PathAbs fallback resolves bare dot segments before following symlinks.
+ // Anchor relative TVF paths to the canonical secure root, not the process cwd.
+ std::string secure_root;
+ RETURN_IF_ERROR(io::global_local_filesystem()->canonicalize(
+ config::user_files_secure_path, &secure_root));
+ local_input = (io::Path(secure_root) / expand_home_path(requested_uri))
+ .lexically_normal()
+ .string();
+ }
+ RETURN_IF_ERROR(io::global_local_filesystem()->canonicalize_local_file(
Review Comment:
Fixed in fb92e7d8c337c86e6e90fe135bbd4a5d995115b0. Unsupported LOCAL URI aliases now use a small pre-open SDK metadata API that reuses the pinned SDK's own URL parser/default registry path extraction without constructing a store or reading a manifest. BE canonicalizes and checks the extracted path and reachable tree, then reopens the canonical URI with the original local provider and percent-encoded path. It does not rewrite the alias to file: before parsing. Established file: URI parsing stays unchanged. Actual schema and row-value controls cover mandatory file-object-store, encoded space/percent/plus, authority/query/fragment, in-root symlink, and out-of-root rejection; the dataset key preserves provider identity. Extra file+uring scans are explicitly opt-in and not claimed here because this host denies io_uring setup. Matching header/archive macros and defined exports are enforced.
Validation: Final standard JDK17 FE: 107 selected tests passed with fresh XML, zero failures/errors/skips and frozen source. Final standard Clang21/ASAN BE: 28 selected tests passed. Each release also passed 16 Lance and 19 Paimon build-control tests; the new SDK unit/C API tests, matching Rust std/ABI checks and private standard build passed. Independent Standards/Spec rechecks found no remaining confirmed issue in this follow-up.
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/LocalTableValuedFunction.java:
##########
@@ -135,6 +143,38 @@ private void getFileListFromBackend() throws AnalysisException {
}
}
+ /** Apply LOCAL admission to sinks that select the built-in local JNI writer as well. */
+ public static void checkSinkEnabled(String tvfName, Map<String, String> properties) {
+ String writerClass = properties.getOrDefault("writer_class", "");
+ // JNI consumes a C string, and ScannerLoader accepts slash-separated class names.
+ int nul = writerClass.indexOf('\0');
+ if (nul >= 0) {
+ writerClass = writerClass.substring(0, nul);
+ }
+ if ("local".equalsIgnoreCase(tvfName)
Review Comment:
Fixed in fb92e7d8c337c86e6e90fe135bbd4a5d995115b0. The retained nested ViewFS variant is confirmed and fixed: guarded FrozenConfiguration forces fs.viewfs.enable.inner.cache=false and includes that effective value in its profile. Nested children therefore use the existing FileSystem cache, whose guarded identity preserves exact authority and configuration. The folding InnerCache can no longer reuse A for a; ordinary unmarked requests keep their configured cache behavior. Native write admission still checks the actual path before IO and enabled-write opt-in is unchanged.
Validation: Final standard JDK17 FE: 107 selected tests passed with fresh XML, zero failures/errors/skips and frozen source. Final standard Clang21/ASAN BE: 28 selected tests passed. Each release also passed 16 Lance and 19 Paimon build-control tests; the new SDK unit/C API tests, matching Rust std/ABI checks and private standard build passed. Independent Standards/Spec rechecks found no remaining confirmed issue in this follow-up.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4371468958
##########
be/src/format_v2/table/lance_reader.cpp:
##########
@@ -628,6 +887,10 @@ Status LanceTableReader::_ensure_dataset_open(const TFileRangeDesc& range,
} else if (!_opened_dataset_key.has_value() || *_opened_dataset_key != key) {
return Status::InvalidArgument(
"Lance reader cannot mix dataset snapshots or storage options");
+ } else {
+ // Revalidate reused handles before scanners, indexes or row-id reads. Fresh handles
+ // are checked inside _open_dataset before they become reader state.
+ RETURN_IF_ERROR(_validate_dataset_storage(_dataset, key.storage_policy));
Review Comment:
[P2] Reuse remote storage admission for an unchanged snapshot
For a normal S3 Lance scan with F fragments and B registered remote bases, `TVFScanNode.getLanceSplits()` emits one fragment per split, and each split returns here with the same `_dataset`/`DatasetKey`. This call nevertheless reconstructs, sorts, serializes, parses and validates every base through the new JSON API again. The pinned SDK's [public add_bases API](https://github.com/lance-format/lance/blob/68c12dfd7efe02f90ad2d7f3a239a7eb64884e57/rust/lance/src/dataset.rs#L2013-L2043) can register many distinct remote bases, including unused ones, so 1,000 fragments and 1,000 registrations cause about a million base descriptions instead of one admission pass per opened handle. The added work scales with F times B, plus sorting, despite an unchanged immutable snapshot and policy. Cache successful REMOTE validation for that handle/key after `_open_dataset`, preserving LOCAL rechecks needed for filesystem changes.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra closed pull request #69528: [fix](tvf) Secure LOCAL TVF access (4.1)
URL: https://github.com/apache/doris/pull/69528
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6368399326
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6380066570
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6380070275
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6381435929
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4366829078
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/LocalTableValuedFunction.java:
##########
@@ -135,6 +143,38 @@ private void getFileListFromBackend() throws AnalysisException {
}
}
+ /** Apply LOCAL admission to sinks that select the built-in local JNI writer as well. */
+ public static void checkSinkEnabled(String tvfName, Map<String, String> properties) {
+ String writerClass = properties.getOrDefault("writer_class", "");
+ // JNI consumes a C string, and ScannerLoader accepts slash-separated class names.
+ int nul = writerClass.indexOf('\0');
+ if (nul >= 0) {
+ writerClass = writerClass.substring(0, nul);
+ }
+ if ("local".equalsIgnoreCase(tvfName)
Review Comment:
The delegated ViewFS variant is fixed in 9df0508783e6b1da6be6ef9909de9ebc0db45db8. Native admission uses the actual generated file's parent; FE cleanup checks its actual parent directory. The raw and normalized checks follow the selected mount table and path-component link, including trailing-source slashes, authority/default table selection, fallback and multiple targets. Nested targets are checked before filesystem creation. Unused local mounts do not block an unambiguous remote route.
Runtime-overloaded, regex, cyclic, inherited or ambiguous nested delegation requires the existing LOCAL startup opt-in instead of assuming the inline configuration proves remote storage. Enabled requests retain their existing Hadoop behavior.
All 51 selected FE tests passed with fresh XML and unchanged sources, including INSERT/EXPLAIN, direct binding, resource-derived mounts, off/on and remote controls, and a global-LOAD command entry with cleanup enabled. Factory/executor mocks verify no side effect before rejection. No real directory deletion was executed; targeted Checkstyle passed.
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/S3TableValuedFunction.java:
##########
@@ -89,6 +107,43 @@ public S3TableValuedFunction(Map<String, String> properties, List<TBrokerFileSta
}
+ private static boolean isRemoteLanceUri(String path) {
+ try {
+ URI uri = new URI(path == null ? "" : path);
Review Comment:
Fixed in 9df0508783e6b1da6be6ef9909de9ebc0db45db8 and mirrored to the existing 4.2 PR. The remote-Lance guard validates scheme/authority without applying java.net.URI's stricter path grammar to the full string. The SDK receives the original raw-space or percent-escaped path; malformed/signed ports and local aliases remain rejected.
Pinned url 2.5.8 independently parses raw and escaped spaces as the same remote path, while rejecting the invalid-port controls. Passing FE tests verify exact metadata-loader requests through S3 and FILE, plus scan-range URI identity for both spellings. All 51 selected FE tests passed with fresh XML and unchanged sources. These are mocked FE metadata/range tests and a pinned URL probe; no real remote-service read is claimed.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4367403255
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/LocalTableValuedFunction.java:
##########
@@ -135,6 +150,332 @@ private void getFileListFromBackend() throws AnalysisException {
}
}
+ /** Apply LOCAL admission to the effective sink before planning or filesystem side effects. */
+ public static void checkSinkEnabled(String tvfName, Map<String, String> properties) {
+ if ("local".equalsIgnoreCase(tvfName) || isLocalWriter(properties.getOrDefault("writer_class", ""))) {
+ checkEnabled();
+ }
+ if ("hdfs".equalsIgnoreCase(tvfName)) {
+ if (!properties.containsKey("writer_class")) {
+ checkHdfsSinkEnabled(properties);
+ }
+ if (Boolean.parseBoolean(properties.getOrDefault("delete_existing_files", "false"))) {
+ checkHadoopFileSystemEnabled(
+ BrokerUtil.extractParentDirectory(properties.getOrDefault("file_path", "")), properties);
+ }
+ }
+ }
+
+ private static boolean isLocalWriter(String writer) {
+ // ScannerLoader accepts slashes, and JNI passes the class name as a C string.
+ return "org.apache.doris.writer.LocalFileJniWriter".equals(nativeStringPrefix(writer).replace('/', '.'));
+ }
+
+ /** Check the NameNode and provider used by the native FILE_HDFS writer. */
+ public static void checkHdfsSinkEnabled(Map<String, String> properties) {
+ checkHdfsSinkEnabled(properties.getOrDefault("file_path", ""), properties);
+ }
+
+ public static void checkHdfsSinkEnabled(String outputPrefix, Map<String, String> properties) {
+ String nameNode = nativeStringPrefix(properties.getOrDefault("fs.defaultFS", "")).trim();
+ if ("default".equals(nameNode)) {
+ // libhdfs delegates this selector to the BE's defaults, which may select local storage.
+ checkEnabled();
+ }
+ // libhdfs prefixes bare NameNodes with hdfs:// rather than using the output path's scheme.
+ checkHadoopSchemeEnabled(nameNode.contains("://") ? uriScheme(nameNode) : "hdfs", properties);
+ if (!Config.enable_local_tvf) {
+ try {
+ URI filesystem = new URI(nameNode.contains("://") ? nameNode : "hdfs://" + nameNode);
+ // BE appends a generated filename directly to this prefix, then convert_path
+ // removes the NameNode. Resolve its parent, not an exact mount-point filename.
+ String generated = outputPrefix + "doris-tvf-file";
+ String rawNameNode = properties.getOrDefault("fs.defaultFS", "");
+ if (!rawNameNode.isEmpty() && generated.contains(rawNameNode)) {
+ generated = generated.substring(rawNameNode.length());
+ }
+ if (!generated.startsWith("/")) {
+ generated = "/" + generated;
+ }
+ String path = hadoopPathUri(nativeStringPrefix(generated)).getPath();
+ path = path.substring(0, path.lastIndexOf('/') + 1);
+ if (path.length() > 1) {
+ path = path.substring(0, path.length() - 1);
+ }
+ checkViewFsEnabled(filesystem, path, properties, new HashSet<>());
+ } catch (URISyntaxException e) {
+ // Invalid NameNodes remain subject to the existing storage validation.
+ }
+ }
+ }
+
+ /** Check Hadoop's path/default-filesystem selection before FE-side cleanup. */
+ public static void checkHadoopFileSystemEnabled(String path, Map<String, String> properties) {
Review Comment:
Fixed in 2d7318284d162ad1dc9b1912a8e1150e1f74c674, with the same TVF-only policy on the other active branches. HDFS TVF checks normalized routes before listing and checks every returned path before schema/data use; FILE delegation reaches the same checks. Java listing, native range NameNode and native schema NameNode are checked using their separate consumer semantics.
Hadoop TVFs reject local/unknown delegation even when LOCAL is enabled, because the generic Hadoop reader cannot enforce LOCAL privileges and secure-root containment. The error directs local reads to constrained local(); generic Hadoop/catalog behavior is unchanged. Tests verify disabled/enabled rejection before filesystem creation, actual normalizer/provider controls, remote listing and returned-path checks. All 86 selected FE tests passed, including the release Java trim and native schema-path controls; actual Hadoop callbacks were recording-only.
##########
be/src/format_v2/table/lance_reader.cpp:
##########
@@ -22,24 +22,150 @@
#include <arrow/record_batch.h>
#include <arrow/type.h>
#include <lance/lance.h>
+#include <pwd.h>
+#include <unistd.h>
#include <algorithm>
#include <bit>
+#include <cerrno>
+#include <cstdlib>
#include <cstring>
+#include <filesystem>
#include <limits>
#include <memory>
+#include <unordered_set>
#include "common/config.h"
#include "common/logging.h"
#include "exec/common/endian.h"
#include "format_v2/lance/lance_reader_helper.h"
#include "format_v2/lance/lance_runtime_filter_helper.h"
#include "format_v2/lance/lance_session_manager.h"
+#include "io/fs/local_file_system.h"
#include "runtime/file_scan_profile.h"
#include "runtime/runtime_state.h"
namespace doris::format::lance {
+namespace {
+std::string expand_home_path(const std::string& path) {
+ if (path != "~" && !path.starts_with("~/")) {
+ return path;
+ }
+ // Match the SDK's std::env::home_dir: HOME first, then the current user's passwd entry.
+ std::string home;
+ if (const char* value = std::getenv("HOME"); value != nullptr && value[0] != '\0') {
+ home = value;
+ } else {
+ std::vector<char> buffer(1024);
+ struct passwd entry {};
+ struct passwd* result = nullptr;
+ int error;
+ while ((error = getpwuid_r(getuid(), &entry, buffer.data(), buffer.size(), &result)) ==
+ ERANGE) {
+ buffer.resize(buffer.size() * 2);
+ }
+ if (error == 0 && result != nullptr && entry.pw_dir != nullptr) {
+ home = entry.pw_dir;
+ }
+ }
+ if (home.empty()) {
+ return path;
+ }
+ return path == "~" ? home : (io::Path(home) / path.substr(2)).string();
+}
+
+Status validate_local_dataset_files(const std::string& dataset_uri) {
+ // Lance opens manifests and fragments itself, so a confined dataset directory is not
+ // enough: its descendants can redirect those opens through symlinks.
+ std::error_code error;
+ const auto secure_root = std::filesystem::canonical(config::user_files_secure_path, error);
+ if (error) {
+ return Status::InvalidArgument("local Lance dataset path is not allowed");
+ }
+ std::vector<std::filesystem::path> pending {dataset_uri};
+ std::unordered_set<std::string> visited;
+ while (!pending.empty()) {
+ auto directory = std::move(pending.back());
+ pending.pop_back();
+ // Canonical directory identity bounds cycles and repeated in-root aliases without
+ // imposing a file-count or nesting limit on otherwise valid datasets.
+ if (!visited.insert(directory.string()).second) {
+ continue;
+ }
+ std::filesystem::directory_iterator entries(directory, error);
+ if (error) {
+ return Status::InvalidArgument("local Lance dataset path is not allowed");
+ }
+ const std::filesystem::directory_iterator end;
+ while (entries != end) {
+ auto target = entries->path();
+ auto status = entries->symlink_status(error);
+ if (error) {
+ return Status::InvalidArgument("local Lance dataset path is not allowed");
+ }
+ if (std::filesystem::is_symlink(status)) {
+ target = std::filesystem::canonical(target, error);
+ if (error || !io::LocalFileSystem::contain_path(secure_root, target)) {
+ return Status::InvalidArgument("local Lance dataset path is not allowed");
+ }
+ status = std::filesystem::status(target, error);
+ if (error) {
+ return Status::InvalidArgument("local Lance dataset path is not allowed");
+ }
+ }
+ if (std::filesystem::is_directory(status)) {
+ pending.push_back(std::move(target));
+ }
+ entries.increment(error);
+ if (error) {
+ return Status::InvalidArgument("local Lance dataset path is not allowed");
+ }
+ }
+ }
+ return Status::OK();
+}
+
+Status resolve_dataset_uri(const TFileRangeDesc& range, const TFileScanRangeParams& scan_params,
+ std::string* uri) {
+ const auto& requested_uri = range.table_format_params.lance_params.dataset_uri;
+ // Modern plans tag each range; older plans put the type in the scan parameters.
+ // Untagged catalog scans retain their original storage-provider selection.
+ const bool is_local = range.__isset.file_type
+ ? range.file_type == TFileType::FILE_LOCAL
+ : scan_params.__isset.file_type &&
+ scan_params.file_type == TFileType::FILE_LOCAL;
+ // LOCAL TVF uses version zero for both schema discovery and whole-dataset scans.
+ // Catalog scans pin a positive snapshot version, including file:// catalogs whose
+ // operator-configured warehouse is independent of the LOCAL TVF secure directory.
+ if (is_local && range.table_format_params.lance_params.version == 0) {
+ // LOCAL Lance bypasses glob because it opens a dataset directory. Apply the same
+ // canonical secure-root check before either schema discovery or a cached dataset open.
+ std::string local_input = requested_uri;
+ const auto colon = requested_uri.find(':');
+ const auto slash = requested_uri.find('/');
+ const bool bare_path =
+ colon == std::string::npos || (slash != std::string::npos && slash < colon);
+ if (bare_path && !requested_uri.empty() && requested_uri.find('\0') == std::string::npos &&
+ !config::user_files_secure_path.empty()) {
+ // Lance's PathAbs fallback resolves bare dot segments before following symlinks.
+ // Anchor relative TVF paths to the canonical secure root, not the process cwd.
+ std::string secure_root;
+ RETURN_IF_ERROR(io::global_local_filesystem()->canonicalize(
+ config::user_files_secure_path, &secure_root));
+ local_input = (io::Path(secure_root) / expand_home_path(requested_uri))
+ .lexically_normal()
+ .string();
+ }
+ RETURN_IF_ERROR(io::global_local_filesystem()->canonicalize_local_file(
+ config::user_files_secure_path, local_input, uri));
+ return validate_local_dataset_files(*uri);
Review Comment:
Fixed in 2d7318284d162ad1dc9b1912a8e1150e1f74c674. Java TVF metadata loading inspects manifest bases immediately after opening the primary snapshot and before BASIC metadata use. BE TVFs use the new metadata-only Lance C API to inspect SDK-decoded primary and every registered base before schema/scanner/index/row-id work: remote TVFs reject local bases; LOCAL bases must canonicalize inside the secure root and their reachable trees are checked. Both dataset-root and direct-data-directory bases are covered. Policy is part of the dataset key and is enforced on cached/reused handles; unmarked fixed-version catalogs retain their configured policy.
All 86 selected FE and 22 selected BE tests passed with zero failures/errors/skips. Official SDK-generated fixtures prove outside-root rejection and actual in-root rows 1–4. Isolated S3 tests execute Java rejection, BE remote/local-base rejection and actual remote rows 1–4. The C ABI/SDK tests and matching header/archive checks passed. Complete enforcement requires upgraded serving FE/BE versions; the PR describes the optional TVF marker and mixed-version boundary.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6385363148
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6377825768
# BE Regression && UT Coverage Report
Increment line coverage `100.00% (40/40)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/69528_c4103b6ec3f366ce9096ea20424e998585edbe60_merge/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/69528_c4103b6ec3f366ce9096ea20424e998585edbe60_merge/report/index.html)
| Category | Coverage |
|-------------------|--------------------|
| Function Coverage | 75.04% (32028/42684) |
| Line Coverage | 59.41% (359163/604593) |
| Region Coverage | 56.07% (299258/533763) |
| Branch Coverage | 56.98% (135883/238489) |
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](fe) Disable LOCAL TVF by default (branch-4.1) [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6362101427
Thank you for your contribution to Apache Doris.
Don't know what should be done next? See [How to process your PR](https://doris.apache.org/community/how-to-contribute/review-and-ci).
Please clearly describe your PR:
1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
3. What features were added. Why was this function added?
4. Which code was refactored and why was this part of the code refactored?
5. Which functions were optimized and what is the difference before and after the optimization?
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4369659737
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/LocalTableValuedFunction.java:
##########
@@ -135,6 +150,332 @@ private void getFileListFromBackend() throws AnalysisException {
}
}
+ /** Apply LOCAL admission to the effective sink before planning or filesystem side effects. */
+ public static void checkSinkEnabled(String tvfName, Map<String, String> properties) {
+ if ("local".equalsIgnoreCase(tvfName) || isLocalWriter(properties.getOrDefault("writer_class", ""))) {
+ checkEnabled();
+ }
+ if ("hdfs".equalsIgnoreCase(tvfName)) {
+ if (!properties.containsKey("writer_class")) {
+ checkHdfsSinkEnabled(properties);
+ }
+ if (Boolean.parseBoolean(properties.getOrDefault("delete_existing_files", "false"))) {
+ checkHadoopFileSystemEnabled(
+ BrokerUtil.extractParentDirectory(properties.getOrDefault("file_path", "")), properties);
+ }
+ }
+ }
+
+ private static boolean isLocalWriter(String writer) {
+ // ScannerLoader accepts slashes, and JNI passes the class name as a C string.
+ return "org.apache.doris.writer.LocalFileJniWriter".equals(nativeStringPrefix(writer).replace('/', '.'));
+ }
+
+ /** Check the NameNode and provider used by the native FILE_HDFS writer. */
+ public static void checkHdfsSinkEnabled(Map<String, String> properties) {
+ checkHdfsSinkEnabled(properties.getOrDefault("file_path", ""), properties);
+ }
+
+ public static void checkHdfsSinkEnabled(String outputPrefix, Map<String, String> properties) {
+ String nameNode = nativeStringPrefix(properties.getOrDefault("fs.defaultFS", "")).trim();
+ if ("default".equals(nameNode)) {
+ // libhdfs delegates this selector to the BE's defaults, which may select local storage.
+ checkEnabled();
+ }
+ // libhdfs prefixes bare NameNodes with hdfs:// rather than using the output path's scheme.
+ checkHadoopSchemeEnabled(nameNode.contains("://") ? uriScheme(nameNode) : "hdfs", properties);
+ if (!Config.enable_local_tvf) {
+ try {
+ URI filesystem = new URI(nameNode.contains("://") ? nameNode : "hdfs://" + nameNode);
+ // BE appends a generated filename directly to this prefix, then convert_path
+ // removes the NameNode. Resolve its parent, not an exact mount-point filename.
+ String generated = outputPrefix + "doris-tvf-file";
+ String rawNameNode = properties.getOrDefault("fs.defaultFS", "");
+ if (!rawNameNode.isEmpty() && generated.contains(rawNameNode)) {
+ generated = generated.substring(rawNameNode.length());
+ }
+ if (!generated.startsWith("/")) {
+ generated = "/" + generated;
+ }
+ String path = hadoopPathUri(nativeStringPrefix(generated)).getPath();
+ path = path.substring(0, path.lastIndexOf('/') + 1);
+ if (path.length() > 1) {
+ path = path.substring(0, path.length() - 1);
+ }
+ checkViewFsEnabled(filesystem, path, properties, new HashSet<>());
+ } catch (URISyntaxException e) {
+ // Invalid NameNodes remain subject to the existing storage validation.
+ }
+ }
+ }
+
+ /** Check Hadoop's path/default-filesystem selection before FE-side cleanup. */
+ public static void checkHadoopFileSystemEnabled(String path, Map<String, String> properties) {
Review Comment:
Fixed in fb92e7d8c337c86e6e90fe135bbd4a5d995115b0. The retained nested ViewFS read variant is confirmed and fixed with the same guarded-only inner-cache override. Exact authority/profile caching keeps A and a separate after warming the outer handler. In an isolated old/new dispatch control the old Context allowed /evil while the actual child selected the local leaf; the fixed code selected the lowercase table's remote fallback. The successful control uses simulated leaf endpoints, without storage content IO. Runtime tests execute warm-up followed by repeated lower-table dispatch, alongside existing native and Avro per-path checks.
Validation: Final standard JDK17 FE: 107 selected tests passed with fresh XML, zero failures/errors/skips and frozen source. Final standard Clang21/ASAN BE: 28 selected tests passed. Each release also passed 16 Lance and 19 Paimon build-control tests; the new SDK unit/C API tests, matching Rust std/ABI checks and private standard build passed. Independent Standards/Spec rechecks found no remaining confirmed issue in this follow-up.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4371132112
##########
fe/be-java-extensions/hadoop-deps/src/main/java/org/apache/hadoop/fs/FileSystem.java:
##########
@@ -3622,16 +3640,769 @@ public static Class<? extends FileSystem> getFileSystemClass(String scheme,
* @return the initialized filesystem.
* @throws IOException problems loading or initializing the FileSystem
*/
+ // DORIS-PATCH: TVF policy is checked against the frozen configuration used by the consumer.
+ public static int getDorisTvfGuardVersion() {
+ return 1;
+ }
+
+ public static final class TvfGuard {
+ public static final String POLICY_KEY = "fs.doris.tvf.remote.paths";
+ public static final String NATIVE_POLICY = "native";
+ public static final String NATIVE_WRITE_POLICY = "native-write";
+ public static final String JAVA_READ_POLICY = "java-read";
+ public static final String ROOT_KEY = "fs.doris.tvf.root";
+ public static final String PATH_KEY = "fs.doris.tvf.path";
+ public static final String CONTEXT_KEY = "fs.doris.tvf.context";
+ private static final ThreadLocal<Context> LAST_NATIVE_CONTEXT = new ThreadLocal<>();
+
+ private TvfGuard() {
+ }
+
+ public static final class Rule {
+ private final String filesystem;
+ private final String path;
+ private final boolean descendants;
+
+ public Rule(String filesystem, String path, boolean descendants) {
+ this.filesystem = filesystem;
+ this.path = path;
+ this.descendants = descendants;
+ }
+ }
+
+ public static String encode(List<Rule> rules) {
+ if (FileSystem.getDorisTvfGuardVersion() != 1) {
+ throw new IllegalStateException("TVF Hadoop runtime policy version mismatch");
+ }
+ try {
+ java.io.ByteArrayOutputStream bytes = new java.io.ByteArrayOutputStream();
+ java.io.DataOutputStream out = new java.io.DataOutputStream(bytes);
+ out.writeInt(1);
+ out.writeInt(rules.size());
+ for (Rule rule : rules) {
+ out.writeUTF(rule.filesystem);
+ out.writeUTF(rule.path);
+ out.writeBoolean(rule.descendants);
+ }
+ out.flush();
+ return java.util.Base64.getEncoder().encodeToString(bytes.toByteArray());
+ } catch (IOException e) {
+ throw new IllegalArgumentException("Cannot encode TVF Hadoop route policy", e);
+ }
+ }
+
+ /** Literal probes and subtree prefixes touched by Globber, without filesystem IO. */
+ public static List<Rule> globRules(URI filesystem, String pattern) throws IOException {
+ List<Rule> rules = new ArrayList<>();
+ for (String expanded : GlobExpander.expand(pattern)) {
+ Path flattened = new Path(expanded.isEmpty() ? Path.CUR_DIR : expanded);
+ String path = flattened.toUri().getPath();
+ Path root = new Path(flattened.isAbsolute() ? "/" : Path.CUR_DIR);
+ Path candidate = root;
+ List<String> components = new ArrayList<>();
+ List<Boolean> patterned = new ArrayList<>();
+ for (String component : path.split("/")) {
+ if (component.isEmpty()) {
+ continue;
+ }
+ GlobFilter filter = new GlobFilter(component);
+ if (filter.hasPattern()) {
+ // Keep every real listing parent, even if later escaped '..' removes it.
+ rules.add(new Rule(filesystem.toString(),
+ globLiteralPrefix(root, components, patterned).toUri().getPath(), true));
+ }
+ component = component.replaceAll("\\\\(.)", "$1");
+ candidate = new Path(candidate, component);
+ if ("..".equals(component) && !components.isEmpty()
+ && !"..".equals(components.get(components.size() - 1))) {
+ components.remove(components.size() - 1);
+ patterned.remove(patterned.size() - 1);
+ } else if (!".".equals(component)) {
+ components.add(component);
+ patterned.add(filter.hasPattern());
+ }
+ }
+ boolean subtree = patterned.contains(Boolean.TRUE);
+ Path probe = subtree ? globLiteralPrefix(root, components, patterned) : candidate;
+ rules.add(new Rule(filesystem.toString(), probe.toUri().getPath(), subtree));
+ }
+ return rules;
+ }
+
+ private static Path globLiteralPrefix(Path root, List<String> components, List<Boolean> patterned) {
+ Path prefix = root;
+ for (int i = 0; i < components.size() && !patterned.get(i); i++) {
+ prefix = new Path(prefix, components.get(i));
+ }
+ return prefix;
+ }
+
+ /** Java requests own their mount clients; native connections keep their existing cache. */
+ private static final class RequestViewFileSystem extends org.apache.hadoop.fs.viewfs.ViewFileSystem {
+ private final Map<List<String>, FileSystem> children = new HashMap<>();
+ private final Set<FileSystem> owned = Collections.newSetFromMap(new java.util.IdentityHashMap<>());
+ private boolean closed;
+
+ private RequestViewFileSystem() throws IOException {
+ }
+
+ private final FsGetter getter = new FsGetter() {
+ @Override
+ public FileSystem get(URI uri, Configuration conf) throws IOException {
+ synchronized (RequestViewFileSystem.this) {
+ List<String> key = Arrays.asList(uri.getScheme(), uri.getRawAuthority());
+ FileSystem child = children.get(key);
+ if (child == null) {
+ child = getNewInstance(uri, conf);
+ children.put(key, child);
+ }
+ return child;
+ }
+ }
+
+ @Override
+ public FileSystem getNewInstance(URI uri, Configuration conf) throws IOException {
+ synchronized (RequestViewFileSystem.this) {
+ if (closed) {
+ throw new IOException("TVF ViewFS request is closed");
+ }
+ FileSystem child = FileSystem.newInstance(uri, conf);
+ owned.add(child);
+ return child;
+ }
+ }
+ };
+
+ @Override
+ protected FsGetter fsGetter() {
+ return getter;
+ }
+
+ @Override
+ public synchronized void close() throws IOException {
+ if (closed) {
+ return;
+ }
+ List<IOException> failures = new ArrayList<>();
+ try {
+ // delete-on-exit may resolve a previously unused mount, so do this before sealing.
+ ((FileSystem) this).closeBase();
+ } catch (IOException e) {
+ failures.add(e);
+ } finally {
+ closed = true;
+ // Enumerating ViewFS target.getChildFileSystems() misses Nfly's actual peers.
+ // The stable getter owns those peers directly, including partial initialization.
+ for (FileSystem child : owned) {
+ try {
+ child.close();
+ } catch (IOException e) {
+ failures.add(e);
+ } catch (RuntimeException e) {
+ failures.add(new IOException("Cannot close a TVF mount client", e));
+ }
+ }
+ owned.clear();
+ children.clear();
+ }
+ if (!failures.isEmpty()) {
+ throw MultipleIOException.createIOException(failures);
+ }
+ }
+ }
+
+ private static boolean isNative(String policy) {
+ return NATIVE_POLICY.equals(policy) || NATIVE_WRITE_POLICY.equals(policy);
+ }
+
+ private static boolean isJavaRequest(Configuration conf) {
+ String policy = conf.getRaw(POLICY_KEY);
+ return policy != null && !policy.isEmpty() && !isNative(policy);
+ }
+
+ private static boolean isRequestViewFs(Class<? extends FileSystem> implementation, Configuration conf) {
+ return isJavaRequest(conf) && implementation == org.apache.hadoop.fs.viewfs.ViewFileSystem.class;
+ }
+
+ private static IOException rejected() {
+ return new IOException("Hadoop TVF requires a proven remote filesystem route. "
+ + "Use local() with LOCAL TVF privileges and secure-path restrictions.");
+ }
+
+ private static Configuration freeze(Configuration conf) throws IOException {
+ // Service providers (notably HDFS) register their default/site resources on first load.
+ // Load them before copying raw values, as an ordinary FileSystem lookup does.
+ if (!FILE_SYSTEMS_LOADED) {
+ loadFileSystems();
+ }
+ if (conf instanceof FrozenConfiguration) {
+ return conf;
+ }
+ return new FrozenConfiguration(conf);
+ }
+
+ private static Configuration prepare(Configuration conf) throws IOException {
+ String policy = conf.getRaw(POLICY_KEY);
+ if (policy == null || policy.isEmpty()) {
+ return conf;
+ }
+ Configuration frozen = freeze(conf);
+ Context context = ((FrozenConfiguration) frozen).context;
+ if (isNative(policy)) {
+ context.checkNative(frozen.getRaw(ROOT_KEY), frozen.getRaw(PATH_KEY));
+ } else if (JAVA_READ_POLICY.equals(policy)) {
+ context.checkJava(new Path(frozen.getRaw(PATH_KEY)));
+ } else {
+ context.checkRules(policy);
+ }
+ return frozen;
+ }
+
+ public static void check(URI ignored, Configuration conf) throws IOException {
+ prepare(conf);
+ }
+
+ public static void clearNativeContext() {
+ LAST_NATIVE_CONTEXT.remove();
+ }
+
+ public static Context takeNativeContext() {
+ Context result = LAST_NATIVE_CONTEXT.get();
+ LAST_NATIVE_CONTEXT.remove();
+ return result;
+ }
+
+ public static Configuration prepareJavaRead(Configuration conf, Path path, String marker) throws IOException {
+ if (marker == null || marker.isEmpty()) {
+ return conf;
+ }
+ if (!NATIVE_POLICY.equals(marker)) {
+ throw rejected();
+ }
+ Configuration request = new Configuration(conf);
+ request.set(POLICY_KEY, JAVA_READ_POLICY);
+ request.set(ROOT_KEY, path.toUri().toString());
+ request.set(PATH_KEY, path.toString());
+ return prepare(request);
+ }
+
+ public static void checkJavaRead(FileSystem fs, Path path, String marker) throws IOException {
+ if (marker == null || marker.isEmpty()) {
+ return;
+ }
+ if (!NATIVE_POLICY.equals(marker)) {
+ throw rejected();
+ }
+ // The marker is request-owned, never inferred from an unmarked cached filesystem.
+ Configuration conf = fs.getConf();
+ if (JAVA_READ_POLICY.equals(conf.getRaw(POLICY_KEY))) {
+ // S3A derives a Configuration for bucket options. Its copy preserves our internal
+ // marker and raw snapshot values, but not the private Configuration subclass.
+ ((FrozenConfiguration) freeze(conf)).context.checkJava(path);
+ } else {
+ throw rejected();
+ }
+ }
+
+ private static final class FrozenConfiguration extends Configuration {
+ private final Context context;
+
+ private FrozenConfiguration(Configuration source) throws IOException {
+ super(false);
+ setClassLoader(source.getClassLoader());
+ java.util.SortedMap<String, String> values = new java.util.TreeMap<>();
+ for (Map.Entry<String, String> entry : source) {
+ String key = entry.getKey();
+ String value = source.getRaw(key);
+ set(key, value);
+ if (!PATH_KEY.equals(key) && !ROOT_KEY.equals(key) && !CONTEXT_KEY.equals(key)) {
+ values.put(key, value);
+ }
+ }
+ // ViewFS's private InnerCache folds authority case (A/a). Guarded children must
+ // instead use FileSystem's exact-authority/profile cache below.
+ set("fs.viewfs.enable.inner.cache", "false");
+ values.put("fs.viewfs.enable.inner.cache", "false");
+ try {
+ java.security.MessageDigest digest = java.security.MessageDigest.getInstance("SHA-256");
+ java.io.DataOutputStream output = new java.io.DataOutputStream(
+ new java.security.DigestOutputStream(new java.io.OutputStream() {
+ @Override
+ public void write(int value) {
+ }
+
+ @Override
+ public void write(byte[] values, int offset, int length) {
+ }
+ }, digest));
+ for (Map.Entry<String, String> entry : values.entrySet()) {
+ for (String value : new String[] {entry.getKey(), entry.getValue()}) {
+ // Preserve raw Java code units, including unpaired surrogates. UTF-8 replacement
+ // would give a literal '?' and an unpaired surrogate the same cache profile.
+ output.writeInt(value.length());
+ output.writeChars(value);
+ }
+ }
+ output.flush();
+ StringBuilder identity = new StringBuilder();
+ for (byte value : digest.digest()) {
+ identity.append(Character.forDigit((value & 255) >>> 4, 16));
+ identity.append(Character.forDigit(value & 15, 16));
+ }
+ set(CONTEXT_KEY, identity.toString());
+ } catch (java.security.NoSuchAlgorithmException e) {
+ throw new IOException("SHA-256 is required for TVF Hadoop cache isolation");
+ }
+ context = new Context(this);
+ }
+ }
+
+ public static final class Context {
+ private final Configuration conf;
+ private final Map<String, String> routingKeys = new HashMap<>();
+
+ private Context(Configuration conf) {
+ this.conf = conf;
+ for (Map.Entry<String, String> entry : conf) {
+ String key = entry.getKey();
+ if ("fs.defaultFS".equals(key) || "fs.default.name".equals(key)
+ || key.startsWith("fs.viewfs.") || (key.startsWith("fs.") && key.endsWith(".impl"))) {
+ routingKeys.put(key, entry.getValue());
+ }
+ }
+ }
+
+ private EffectiveRoute route(boolean descendants) {
+ return new EffectiveRoute(routingKeys, descendants, conf);
+ }
+
+ public String profile() {
+ return conf.getRaw(CONTEXT_KEY);
+ }
+
+ public boolean isDirectRemote() throws IOException {
+ String root = conf.getRaw(ROOT_KEY);
+ if (root == null) {
+ throw rejected();
+ }
+ try {
+ URI uri = nativeUri(root);
+ return route(false).provider(uri.getScheme()) == Route.Provider.REMOTE;
+ } catch (RuntimeException | URISyntaxException e) {
+ throw rejected();
+ }
+ }
+
+ private static URI nativeUri(String root) throws URISyntaxException {
+ root = root.trim();
+ if (root.isEmpty() || "default".equals(root)) {
+ throw new URISyntaxException("", "Unproven native filesystem");
+ }
+ return new URI(root.contains("://") ? root : "hdfs://" + root);
+ }
+
+ public void checkNative(String root, String path) throws IOException {
+ if (root == null || path == null || path.isEmpty()) {
+ throw rejected();
+ }
+ try {
+ URI uri = nativeUri(root);
+ Path actualPath = new Path(path);
+ if (NATIVE_WRITE_POLICY.equals(conf.getRaw(POLICY_KEY))) {
+ // ViewFS.create resolves the parent, without following the final component.
+ Path parent = actualPath.getParent();
+ actualPath = parent == null ? new Path("/") : parent;
+ }
+ route(false).checkRoute(uri, actualPath.toUri().getPath(), new HashSet<>());
+ } catch (RuntimeException | URISyntaxException e) {
+ throw rejected();
+ }
+ }
+
+ private void checkJava(Path path) throws IOException {
+ try {
+ URI uri = path.toUri();
+ route(false).checkRoute(uri, uri.getPath(), new HashSet<>());
+ } catch (RuntimeException | URISyntaxException e) {
+ throw rejected();
+ }
+ }
+
+ private void checkRules(String policy) throws IOException {
+ try {
+ byte[] bytes = java.util.Base64.getDecoder().decode(policy);
+ java.io.DataInputStream in = new java.io.DataInputStream(new java.io.ByteArrayInputStream(bytes));
+ int version = in.readInt();
+ int count = in.readInt();
+ if (version != 1 || count <= 0 || count > in.available() / 5) {
+ throw rejected();
+ }
+ for (int i = 0; i < count; i++) {
+ URI filesystem = new URI(in.readUTF());
+ String path = in.readUTF();
+ boolean descendants = in.readBoolean();
+ route(descendants).checkRoute(filesystem, path, new HashSet<>());
+ }
+ if (in.available() != 0) {
+ throw rejected();
+ }
+ } catch (RuntimeException | URISyntaxException e) {
+ throw rejected();
+ }
+ }
+ }
+
+ private static final class EffectiveRoute extends Route {
+ private final Configuration conf;
+
+ private EffectiveRoute(Map<String, String> properties, boolean descendants, Configuration conf) {
+ super(properties, descendants);
+ this.conf = conf;
+ }
+
+ private EffectiveRoute(EffectiveRoute original, boolean descendants) {
+ super(original, descendants);
+ conf = original.conf;
+ }
+
+ @Override
+ protected Route withDescendants(boolean value) {
+ return new EffectiveRoute(this, value);
+ }
+
+ @Override
+ protected String property(String key) {
+ return conf.get(key);
+ }
+
+ @Override
+ protected Provider provider(String scheme) {
+ if ("file".equalsIgnoreCase(scheme)) {
+ return Provider.LOCAL;
+ }
+ try {
+ String implementation = FileSystem.getFileSystemClass(scheme, conf).getName();
+ return implementationProvider(implementation);
+ } catch (IOException | RuntimeException e) {
+ return Provider.UNKNOWN;
+ }
+ }
+ }
+
+ public static class Route {
+ protected enum Provider {
+ REMOTE,
+ LOCAL,
+ MOUNT_TABLE,
+ DYNAMIC,
+ UNKNOWN
+ }
+
+ // JuiceFS loads its authoritative backend from mutable namespace metadata; neither
+ // its scheme nor its class proves remote storage for a guarded TVF.
+ protected static final Set<String> REMOTE_SCHEMES = new HashSet<>(Arrays.asList(
+ "hdfs", "s3a", "s3n", "oss", "ofs", "abfs", "abfss", "wasb", "wasbs", "gs", "cosn"));
+ protected static final Set<String> REMOTE_IMPLEMENTATIONS = new HashSet<>(Arrays.asList(
+ "org.apache.hadoop.hdfs.DistributedFileSystem",
+ "org.apache.hadoop.fs.s3a.S3AFileSystem",
+ "org.apache.hadoop.fs.azurebfs.AzureBlobFileSystem",
+ "org.apache.hadoop.fs.azurebfs.SecureAzureBlobFileSystem",
+ "org.apache.hadoop.fs.azure.NativeAzureFileSystem",
+ "org.apache.hadoop.fs.azure.NativeAzureFileSystem$Secure",
+ "org.apache.hadoop.fs.ozone.OzoneFileSystem",
+ "org.apache.hadoop.fs.ozone.RootedOzoneFileSystem",
+ "org.apache.hadoop.fs.cosn.CosNFileSystem",
+ "com.qcloud.chdfs.fs.CHDFSHadoopFileSystemAdapter",
+ "org.apache.hadoop.fs.aliyun.oss.AliyunOSSFileSystem",
+ "com.aliyun.jindodata.oss.JindoOssFileSystem",
+ "org.apache.hadoop.fs.obs.OBSFileSystem",
+ "com.google.cloud.hadoop.fs.gcs.GoogleHadoopFileSystem"));
+
+ private final boolean descendants;
+ private final Map<String, String> properties;
+
+ protected Route(Route original, boolean descendants) {
+ this(original.properties, descendants);
+ }
+
+ protected Route withDescendants(boolean value) {
+ return new Route(this, value);
+ }
+
+ public Route(Map<String, String> properties, boolean descendants) {
+ // Both callers supply a statement-local snapshot that is never mutated afterward.
+ this.properties = properties;
+ this.descendants = descendants;
+ }
+
+ private List<URI> defaults() throws URISyntaxException {
+ List<URI> defaults = new ArrayList<>();
+ for (String key : Arrays.asList("fs.defaultFS", "fs.default.name")) {
+ String value = property(key);
+ if (value != null) {
+ value = selector(value).trim();
+ // FileSystem.fixName treats slash-free host[:port] strings as HDFS defaults.
+ defaults.add(new URI("local".equals(value) ? "file:///"
+ : (value.indexOf('/') < 0 ? "hdfs://" + value : value)));
+ }
+ }
+ if (defaults.isEmpty()) {
+ defaults.add(new URI("file:///"));
+ }
+ return defaults;
+ }
+
+ public void checkRoute(URI filesystem, String path, Set<String> activeTables) throws URISyntaxException {
+ if (filesystem.getScheme() == null) {
+ for (URI defaultFs : defaults()) {
+ checkViewFsEnabled(defaultFs, path, activeTables);
+ }
+ return;
+ }
+ if (filesystem.getAuthority() == null) {
+ boolean completed = false;
+ for (URI defaultFs : defaults()) {
+ if (filesystem.getScheme().equals(defaultFs.getScheme()) && defaultFs.getAuthority() != null) {
+ checkViewFsEnabled(defaultFs, path, activeTables);
+ completed = true;
+ }
+ }
+ if (completed) {
+ return;
+ }
+ }
+ checkViewFsEnabled(filesystem, path, activeTables);
+ }
+
+ public void checkGlobRoute(URI filesystem, String pattern, Set<String> activeTables)
+ throws IOException, URISyntaxException {
+ for (Rule rule : globRules(filesystem, pattern)) {
+ withDescendants(rule.descendants).checkRoute(filesystem, rule.path, activeTables);
+ }
+ }
+
+ protected Provider provider(String scheme) {
+ if ("file".equalsIgnoreCase(scheme)) {
+ return Provider.LOCAL;
+ }
+ String implementation = property("fs." + scheme + ".impl");
+ if (implementation != null) {
+ implementation = selector(implementation).trim();
+ return implementationProvider(implementation);
+ }
+ if ("viewfs".equals(scheme)) {
+ return Provider.MOUNT_TABLE;
+ }
+ return REMOTE_SCHEMES.contains(scheme) ? Provider.REMOTE : Provider.UNKNOWN;
+ }
+
+ protected Provider implementationProvider(String implementation) {
+ if ("org.apache.hadoop.fs.LocalFileSystem".equals(implementation)
+ || "org.apache.hadoop.fs.RawLocalFileSystem".equals(implementation)) {
+ return Provider.LOCAL;
+ }
+ if ("org.apache.hadoop.fs.viewfs.ViewFileSystem".equals(implementation)) {
+ return Provider.MOUNT_TABLE;
+ }
+ if ("org.apache.hadoop.fs.viewfs.ViewFileSystemOverloadScheme".equals(implementation)
+ || "org.apache.hadoop.hdfs.ViewDistributedFileSystem".equals(implementation)) {
+ return Provider.DYNAMIC;
+ }
+ return REMOTE_IMPLEMENTATIONS.contains(implementation) ? Provider.REMOTE : Provider.UNKNOWN;
+ }
+
+ private String[] targets(String key, String prefix) {
+ String value = selector(property(key));
+ // InodeTree treats SINGLE, SINGLE_FALLBACK and MERGE_SLASH as one URI.
+ // Commas delimit targets only for MERGE and NFLY links.
+ String entry = key.substring(prefix.length());
+ boolean multiple = entry.startsWith("linkMerge.") || entry.startsWith("linkNfly.");
+ return multiple ? value.split(",", -1) : new String[] {value};
+ }
+
+ private void checkGlobTargets(String key, String prefix, Set<String> activeTables) throws URISyntaxException {
+ for (String target : targets(key, prefix)) {
+ if (target.isEmpty()) {
+ reject();
+ }
+ URI uri = new URI(target);
+ // Listing can touch any descendant of a selected mount. Reuse the same route
+ // interpreter for nested tables instead of assuming their root is representative.
+ String path = uri.getPath() == null ? "/" : uri.getPath();
+ checkRoute(uri, path, activeTables);
+ }
+ }
+
+ private void checkViewFsEnabled(URI filesystem, String path, Set<String> activeTables) {
+ String scheme = filesystem.getScheme();
+ Provider provider = provider(scheme);
+ if (provider == Provider.LOCAL || provider == Provider.UNKNOWN || provider == Provider.DYNAMIC) {
+ reject();
+ }
+ if (provider != Provider.MOUNT_TABLE) {
+ return;
+ }
+ if (!path.startsWith("/")) {
+ // Java resolves relative paths against the selected filesystem's working directory.
+ // Its user/home configuration is not a proven absolute mount-table path here.
+ reject();
+ }
+ String table = filesystem.getAuthority();
+ String ignorePort = property("fs.viewfs.ignore.port.in.mount.table.name");
+ if (ignorePort != null && Boolean.parseBoolean(selector(ignorePort).trim())
+ && table != null && filesystem.getPort() != -1) {
+ table = filesystem.getHost();
+ if (table == null) {
+ reject();
+ }
+ }
+ if (table == null || table.isEmpty()) {
+ table = property("fs.viewfs.mounttable.default.name.key");
+ table = table == null ? "default" : selector(table);
+ }
+ // Nested ViewFS cycles or arbitrarily deep delegation cannot establish remote storage.
+ if (activeTables.size() >= 32 || !activeTables.add(table)) {
Review Comment:
[P2] Allow finite routes that revisit a ViewFS table
With `fs.defaultFS=viewfs://cluster`, `fs.viewfs.mounttable.cluster.link./a=viewfs://cluster/b` and `fs.viewfs.mounttable.cluster.link./b=hdfs://namenode/base`, reading `viewfs://cluster/a/file.csv` resolves to remote `/base/file.csv`. [Pinned Hadoop initializes SINGLE-link targets lazily](https://github.com/apache/doris-thirdparty/blob/hadoop-3.4.2.2-for-doris/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/InodeTree.java), so this finite alias works without initialization recursion. This set rejects the second `cluster` before examining `/b`, making HDFS/FILE reads fail even with LOCAL enabled; native writes and cleanup through `/a` also fail with LOCAL disabled. Track resolved route state, including the path or selected link, with a separate depth bound, and cover this remote alias alongside an actual cycle.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6379619371
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4366382140
##########
be/src/format_v2/table/lance_reader.cpp:
##########
@@ -35,11 +35,50 @@
#include "format_v2/lance/lance_reader_helper.h"
#include "format_v2/lance/lance_runtime_filter_helper.h"
#include "format_v2/lance/lance_session_manager.h"
+#include "io/fs/local_file_system.h"
#include "runtime/file_scan_profile.h"
#include "runtime/runtime_state.h"
namespace doris::format::lance {
+namespace {
+Status resolve_dataset_uri(const TFileRangeDesc& range, const TFileScanRangeParams& scan_params,
+ std::string* uri) {
+ const auto& requested_uri = range.table_format_params.lance_params.dataset_uri;
+ // Modern plans tag each range; older plans put the type in the scan parameters.
+ // Untagged catalog scans retain their original storage-provider selection.
+ const bool is_local = range.__isset.file_type
+ ? range.file_type == TFileType::FILE_LOCAL
+ : scan_params.__isset.file_type &&
+ scan_params.file_type == TFileType::FILE_LOCAL;
+ // LOCAL TVF uses version zero for both schema discovery and whole-dataset scans.
+ // Catalog scans pin a positive snapshot version, including file:// catalogs whose
+ // operator-configured warehouse is independent of the LOCAL TVF secure directory.
+ if (is_local && range.table_format_params.lance_params.version == 0) {
+ // LOCAL Lance bypasses glob because it opens a dataset directory. Apply the same
+ // canonical secure-root check before either schema discovery or a cached dataset open.
+ std::string local_input = requested_uri;
+ const auto colon = requested_uri.find(':');
+ const auto slash = requested_uri.find('/');
+ const bool bare_path =
+ colon == std::string::npos || (slash != std::string::npos && slash < colon);
+ if (bare_path && !requested_uri.empty() && requested_uri.find('\0') == std::string::npos &&
+ !config::user_files_secure_path.empty()) {
+ // Lance's PathAbs fallback resolves bare dot segments before following symlinks.
+ // Anchor relative TVF paths to the canonical secure root, not the process cwd.
+ std::string secure_root;
+ RETURN_IF_ERROR(io::global_local_filesystem()->canonicalize(
+ config::user_files_secure_path, &secure_root));
+ local_input = (io::Path(secure_root) / requested_uri).lexically_normal().string();
Review Comment:
Fixed in 66d2b918a4cbcc4a5dca2271396700da91bbe365. Only LOCAL/version-zero bare paths expand SDK-supported `~` and `~/` before lexical normalization and canonical secure-root containment. HOME is used first with a thread-safe passwd fallback, matching the SDK; named-user spellings and file-URI literal tildes are preserved.
All 15 selected BE tests passed. Distinct real Lance fixtures verify schema, canonical dataset key and actual rows for the home target versus the literal-tilde target, plus an expanded target outside the secure root. The tests use a unique fixture under the existing home and never override HOME. Remote ranges and fixed-version catalogs retain their routing.
##########
be/src/io/fs/local_file_system.cpp:
##########
@@ -455,28 +455,158 @@ 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");
+ }
+ // Match URL preprocessing only for actual file URIs; bare paths keep literal whitespace.
+ const auto is_c0_or_space = [](unsigned char c) { return c <= 0x20; };
+ size_t begin = 0;
+ size_t end = file_path.size();
+ while (begin < end && is_c0_or_space(file_path[begin])) {
+ ++begin;
+ }
+ while (end > begin && is_c0_or_space(file_path[end - 1])) {
+ --end;
+ }
+ std::string uri_input;
+ for (size_t i = begin; i < end; ++i) {
+ if (file_path[i] != '\t' && file_path[i] != '\r' && file_path[i] != '\n') {
+ uri_input += file_path[i];
+ }
+ }
+ const bool is_file_uri = uri_input.size() >= 5 &&
+ (uri_input[0] == 'f' || uri_input[0] == 'F') &&
+ (uri_input[1] == 'i' || uri_input[1] == 'I') &&
+ (uri_input[2] == 'l' || uri_input[2] == 'L') &&
+ (uri_input[3] == 'e' || uri_input[3] == 'E') && uri_input[4] == ':';
+ std::string local_input = is_file_uri ? std::move(uri_input) : file_path;
+ if (is_file_uri) {
+ local_input.replace(0, 4, "file");
+ // File URLs treat raw backslashes as separators, including before the authority.
+ // Do this before conversion; percent-encoded backslashes must remain literal.
+ for (size_t i = 5; i < local_input.size(); ++i) {
+ if (local_input[i] == '\\') {
+ local_input[i] = '/';
+ }
+ }
+ }
+ 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(local_input, local_path));
+ if (is_file_uri) {
+ // file:a%20b names "a b", but a bare path containing "%20" must stay literal.
+ // Decode once, preserve '+' and invalid escapes, matching Lance's file provider.
+ auto encoded = local_path.string();
+ // Query and fragment are URI components, not part of the filesystem path.
+ encoded = encoded.substr(0, encoded.find_first_of("?#"));
+ // A file URL without a leading slash still names a filesystem-rooted path.
+ if (encoded.empty() || encoded[0] != '/') {
+ encoded.insert(0, "/");
+ }
+ // URL dot segments are removed before symlinks are followed and before %2F
+ // becomes a path separator. Keep empty segments: "a//../b" denotes "a/b".
+ std::vector<std::string> segments;
+ for (size_t start = 1;;) {
+ const size_t end = encoded.find('/', start);
+ const auto segment =
+ encoded.substr(start, end == std::string::npos ? end : end - start);
+ const bool dot = segment == "." || segment == "%2e" || segment == "%2E";
+ const bool parent = segment == ".." || segment == ".%2e" || segment == ".%2E" ||
+ segment == "%2e." || segment == "%2E." || segment == "%2e%2e" ||
+ segment == "%2e%2E" || segment == "%2E%2e" || segment == "%2E%2E";
+ if (parent) {
+ if (!segments.empty()) {
+ segments.pop_back();
+ }
+ } else if (!dot) {
+ segments.push_back(segment);
Review Comment:
The exact triple-slash example has been checked against pinned url 2.5.8 on Unix: `file:///C|/data.lance` produces `/C|/data.lance`, whereas `file:/C|/data.lance`, `file:C|/data.lance` and `file://C|/data.lance` produce `/C:/data.lance`. The triple-slash identity is therefore intentionally retained.
The related drive-parser gaps are fixed in 66d2b918a4cbcc4a5dca2271396700da91bbe365: single-slash/rootless/authority drive handling, and raw-drive protection from a following parent segment, are matched to the SDK before containment. The legacy converter and literal bare Unix names are unchanged.
All 15 selected BE tests passed. A 19-form pure-parser matrix distinguishes these spellings, encoded pipes and localhost authorities (18 URL expectations independently checked against the pinned SDK). Real nested-drive datasets also verify distinct schemas and actual scanned rows. No root-level fixture is created.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6382169704
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6385290864
# BE UT Coverage Report
Increment line coverage `90.15% (238/264)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/9df0508783e6b1da6be6ef9909de9ebc0db45db8_9df0508783e6b1da6be6ef9909de9ebc0db45db8/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/9df0508783e6b1da6be6ef9909de9ebc0db45db8_9df0508783e6b1da6be6ef9909de9ebc0db45db8/report/index.html)
| Category | Coverage |
|-------------------|--------------------|
| Function Coverage | 61.49% (26994/43902) |
| Line Coverage | 46.18% (281587/609732) |
| Region Coverage | 41.82% (222914/532982) |
| Branch Coverage | 43.38% (103695/239031) |
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4367453269
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/HadoopTvfAdmission.java:
##########
@@ -0,0 +1,477 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.tablefunction;
+
+import org.apache.doris.common.Config;
+import org.apache.doris.common.util.LocationPath;
+import org.apache.doris.nereids.exceptions.AnalysisException;
+
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * TVF-only admission for Hadoop consumers. Input properties have already been normalized by storage.
+ * This module neither loads Hadoop resources nor constructs a filesystem. Java strings and native
+ * C strings are projected separately before filesystem selection and mount-table interpretation.
+ */
+public final class HadoopTvfAdmission {
+ public enum Access {
+ NATIVE_WRITE,
+ JAVA_DELETE,
+ HADOOP_READ
+ }
+
+ private enum Provider {
+ REMOTE,
+ LOCAL,
+ MOUNT_TABLE,
+ DYNAMIC,
+ UNKNOWN
+ }
+
+ private static final Set<String> REMOTE_SCHEMES = new HashSet<>(Arrays.asList(
+ "hdfs", "s3a", "s3n", "oss", "ofs", "jfs", "abfs", "abfss", "wasb", "wasbs", "gs", "cosn"));
+ private static final Set<String> REMOTE_IMPLEMENTATIONS = new HashSet<>(Arrays.asList(
+ "org.apache.hadoop.hdfs.DistributedFileSystem",
+ "org.apache.hadoop.fs.s3a.S3AFileSystem",
+ "org.apache.hadoop.fs.azurebfs.AzureBlobFileSystem",
+ "org.apache.hadoop.fs.azurebfs.SecureAzureBlobFileSystem",
+ "org.apache.hadoop.fs.azure.NativeAzureFileSystem",
+ "org.apache.hadoop.fs.aliyun.oss.AliyunOSSFileSystem",
+ "com.aliyun.jindodata.oss.JindoOssFileSystem",
+ "org.apache.hadoop.fs.obs.OBSFileSystem",
+ "com.google.cloud.hadoop.fs.gcs.GoogleHadoopFileSystem"));
+
+ private final Access access;
+ private final Map<String, String> properties = new HashMap<>();
+
+ private HadoopTvfAdmission(Access access, Map<String, String> normalized, boolean nativeConsumer) {
+ this.access = access;
+ for (Map.Entry<String, String> entry : normalized.entrySet()) {
+ // parse_properties / generateHdfsParam consumes the canonical NameNode before JNI.
+ if (nativeConsumer && "fs.defaultFS".equalsIgnoreCase(entry.getKey())) {
+ continue;
+ }
+ String key = (nativeConsumer ? nativeStringPrefix(entry.getKey()) : entry.getKey()).trim();
+ String value = nativeConsumer ? nativeStringPrefix(entry.getValue()) : entry.getValue();
+ String previous = properties.put(key, value);
+ if (previous != null && !previous.equals(value) && isRoutingKey(key)) {
+ reject();
+ }
+ }
+ }
+
+ /** Reject local/unknown Hadoop TVF routes before listing, deletion, schema discovery or data IO. */
+ public static void check(Access access, String path, Map<String, String> normalized) {
+ if (access != Access.HADOOP_READ && Config.enable_local_tvf) {
+ return;
+ }
+ try {
+ if (access == Access.NATIVE_WRITE) {
+ String nameNode = nativeNameNode(access, normalized);
+ URI output = nativeFileUri(access, path + "doris-tvf-file", nameNode);
+ String parent = output.getPath();
+ parent = parent.substring(0, parent.lastIndexOf('/') + 1);
+ if (parent.length() > 1) {
+ parent = parent.substring(0, parent.length() - 1);
+ }
+ new HadoopTvfAdmission(access, normalized, true).checkNative(nameNode, parent);
+ } else {
+ HadoopTvfAdmission javaConsumer = new HadoopTvfAdmission(access, normalized, false);
+ // Project the Java consumer independently from native convert_path/JNI.
+ URI javaPath = hadoopPathUri(path.trim());
+ javaConsumer.checkRoute(javaPath, javaPath.getPath(), new HashSet<>());
+ if (access == Access.HADOOP_READ) {
+ // FileQueryScanNode sends this filesystem identifier in range.fs_name. BE
+ // falls back to hdfs_params.fs_name only when that range field is empty.
+ URI fileUri = LocationPath.of(path).getPath().toUri();
+ String nameNode = fileUri.getScheme() + "://" + fileUri.getAuthority();
+ if (nameNode.isEmpty()) {
+ nameNode = nativeNameNode(access, normalized);
+ }
+ HadoopTvfAdmission nativeConsumer = new HadoopTvfAdmission(access, normalized, true);
+ nativeConsumer.checkNativeRead(nameNode, path);
+ // Schema RPCs have no range.fs_name; they use THdfsParams.fs_name instead.
+ nativeConsumer.checkNativeRead(nativeNameNode(access, normalized), path);
+ }
+ }
+ } catch (URISyntaxException e) {
+ new HadoopTvfAdmission(access, normalized, false).reject();
+ }
+ }
+
+ private static String nativeNameNode(Access access, Map<String, String> normalized) {
+ String result = null;
+ for (Map.Entry<String, String> entry : normalized.entrySet()) {
+ if ("fs.defaultFS".equalsIgnoreCase(entry.getKey())) {
+ if (result != null && !result.equals(entry.getValue())) {
+ reject(access);
+ }
+ result = entry.getValue();
+ }
+ }
+ return result == null ? "" : result;
+ }
+
+ private void checkNative(String nameNode, String path) throws URISyntaxException {
+ nameNode = selector(nativeStringPrefix(nameNode)).trim();
+ if (nameNode.isEmpty() && access == Access.NATIVE_WRITE) {
+ // BE passes fs_name.c_str(), never NULL. libhdfs prefixes this empty string
+ // with hdfs://, whose URI construction fails before any filesystem is selected.
+ // Preserve existing analysis; execution reports the invalid NameNode as before.
+ return;
+ }
+ if ("default".equals(nameNode) || nameNode.isEmpty()) {
+ reject();
+ }
+ checkRoute(new URI(nameNode.contains("://") ? nameNode : "hdfs://" + nameNode),
+ path, new HashSet<>());
+ }
+
+ private void checkNativeRead(String nameNode, String path) throws URISyntaxException {
+ checkNative(nameNode, nativeFileUri(access, path, nameNode).getPath());
+ }
+
+ private List<URI> defaults() throws URISyntaxException {
+ List<URI> defaults = new ArrayList<>();
+ for (String key : Arrays.asList("fs.defaultFS", "fs.default.name")) {
+ String value = property(key);
+ if (value != null) {
+ value = selector(value).trim();
+ defaults.add(new URI("local".equals(value) ? "file:///"
+ : (uriScheme(value).isEmpty() ? "hdfs://" + value : value)));
Review Comment:
[P2] Preserve Hadoop's host:port default-filesystem shorthand
With the default `enable_local_tvf=false`, `INSERT INTO hdfs('file_path'='/exports/prefix_', 'format'='csv', 'fs.defaultFS'='namenode:8020', 'delete_existing_files'='true') SELECT 1` now fails analysis with `LOCAL TVF is disabled`, although both the write and cleanup select remote HDFS. `HdfsProperties` preserves this value and the native gate prefixes `hdfs://`, but cleanup reaches `defaults()`: `uriScheme()` treats `namenode` as a scheme, so `provider()` rejects it as unknown. Hadoop's [getDefaultUri/fixName](https://github.com/apache/doris-thirdparty/blob/hadoop-3.4.2.2-for-doris/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java#L487-L497) instead converts slash-free `namenode:8020` to `hdfs://namenode:8020`. Mirror that default-filesystem normalization before interpreting the scheme, including the legacy default-FS alias, and cover remote cleanup with LOCAL disabled. This remote compatibility regression is distinct from the earlier local-pr
ovider bypass finding.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6386849879
# Cloud UT Coverage Report
Increment line coverage ` ` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/75386c739acf34ecda23c1964d47dcc110457906_75386c739acf34ecda23c1964d47dcc110457906_cloud/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/75386c739acf34ecda23c1964d47dcc110457906_75386c739acf34ecda23c1964d47dcc110457906_cloud/report/index.html)
| Category | Coverage |
|-------------------|--------------------|
| Function Coverage | 78.52% (1963/2500) |
| Line Coverage | 65.52% (35412/54050) |
| Region Coverage | 53.07% (34020/64104) |
| Branch Coverage | 55.86% (10483/18767) |
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4367889998
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/HadoopTvfAdmission.java:
##########
@@ -0,0 +1,502 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.tablefunction;
+
+import org.apache.doris.common.Config;
+import org.apache.doris.common.util.LocationPath;
+import org.apache.doris.nereids.exceptions.AnalysisException;
+
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * TVF-only admission for Hadoop consumers. Input properties have already been normalized by storage.
+ * This module neither loads Hadoop resources nor constructs a filesystem. Java strings and native
+ * C strings are projected separately before filesystem selection and mount-table interpretation.
+ */
+public final class HadoopTvfAdmission {
+ public enum Access {
+ NATIVE_WRITE,
+ JAVA_DELETE,
+ HADOOP_READ
+ }
+
+ private enum Provider {
+ REMOTE,
+ LOCAL,
+ MOUNT_TABLE,
+ DYNAMIC,
+ UNKNOWN
+ }
+
+ private static final Set<String> REMOTE_SCHEMES = new HashSet<>(Arrays.asList(
+ "hdfs", "s3a", "s3n", "oss", "ofs", "jfs", "abfs", "abfss", "wasb", "wasbs", "gs", "cosn"));
+ private static final Set<String> REMOTE_IMPLEMENTATIONS = new HashSet<>(Arrays.asList(
+ "org.apache.hadoop.hdfs.DistributedFileSystem",
+ "org.apache.hadoop.fs.s3a.S3AFileSystem",
+ "org.apache.hadoop.fs.azurebfs.AzureBlobFileSystem",
+ "org.apache.hadoop.fs.azurebfs.SecureAzureBlobFileSystem",
+ "org.apache.hadoop.fs.azure.NativeAzureFileSystem",
+ "org.apache.hadoop.fs.aliyun.oss.AliyunOSSFileSystem",
+ "com.aliyun.jindodata.oss.JindoOssFileSystem",
+ "org.apache.hadoop.fs.obs.OBSFileSystem",
+ "com.google.cloud.hadoop.fs.gcs.GoogleHadoopFileSystem"));
+
+ private final Access access;
+ private final Map<String, String> properties = new HashMap<>();
+
+ private HadoopTvfAdmission(Access access, Map<String, String> normalized, boolean nativeConsumer) {
+ this.access = access;
+ Set<String> nativeRoutingKeys = new HashSet<>();
+ for (Map.Entry<String, String> entry : normalized.entrySet()) {
+ // parse_properties / generateHdfsParam consumes the canonical NameNode before JNI.
+ if (nativeConsumer && "fs.defaultFS".equalsIgnoreCase(entry.getKey())) {
+ continue;
+ }
+ String key = (nativeConsumer ? nativeStringPrefix(entry.getKey()) : entry.getKey()).trim();
+ // Native writes first pass through StringCaseMap, before JNI truncates strings.
+ // Its comparator uses strncasecmp followed by the full UTF-8 byte length.
+ if (nativeConsumer && access == Access.NATIVE_WRITE && isRoutingKey(asciiCaseFold(key))) {
+ String identity = asciiCaseFold(nativeStringPrefix(entry.getKey())) + "\0"
+ + entry.getKey().getBytes(StandardCharsets.UTF_8).length;
+ if (!nativeRoutingKeys.add(identity)) {
+ reject();
+ }
+ }
+ String value = nativeConsumer ? nativeStringPrefix(entry.getValue()) : entry.getValue();
+ String previous = properties.put(key, value);
+ if (previous != null && !previous.equals(value) && isRoutingKey(key)) {
+ reject();
+ }
+ }
+ }
+
+ /** Reject local/unknown Hadoop TVF routes before listing, deletion, schema discovery or data IO. */
+ public static void check(Access access, String path, Map<String, String> normalized) {
+ if (access != Access.HADOOP_READ && Config.enable_local_tvf) {
+ return;
+ }
+ try {
+ if (access == Access.NATIVE_WRITE) {
+ String nameNode = nativeNameNode(access, normalized);
+ URI output = nativeFileUri(access, path + "doris-tvf-file", nameNode);
+ String parent = output.getPath();
+ parent = parent.substring(0, parent.lastIndexOf('/') + 1);
+ if (parent.length() > 1) {
+ parent = parent.substring(0, parent.length() - 1);
+ }
+ new HadoopTvfAdmission(access, normalized, true).checkNative(nameNode, parent);
+ } else {
+ HadoopTvfAdmission javaConsumer = new HadoopTvfAdmission(access, normalized, false);
+ // Project the Java consumer independently from native convert_path/JNI.
+ URI javaPath = hadoopPathUri(path.trim());
+ javaConsumer.checkRoute(javaPath, javaPath.getPath(), new HashSet<>());
+ if (access == Access.HADOOP_READ) {
+ // FileQueryScanNode sends this filesystem identifier in range.fs_name. BE
+ // falls back to hdfs_params.fs_name only when that range field is empty.
+ URI fileUri = LocationPath.of(path).getPath().toUri();
+ String nameNode = fileUri.getScheme() + "://" + fileUri.getAuthority();
+ if (nameNode.isEmpty()) {
+ nameNode = nativeNameNode(access, normalized);
+ }
+ HadoopTvfAdmission nativeConsumer = new HadoopTvfAdmission(access, normalized, true);
+ nativeConsumer.checkNativeRead(nameNode, path);
+ // Schema RPCs have no range.fs_name; they use THdfsParams.fs_name instead.
+ nativeConsumer.checkNativeRead(nativeNameNode(access, normalized), path);
+ }
+ }
+ } catch (URISyntaxException e) {
+ new HadoopTvfAdmission(access, normalized, false).reject();
+ }
+ }
+
+ private static String nativeNameNode(Access access, Map<String, String> normalized) {
+ String result = null;
+ for (Map.Entry<String, String> entry : normalized.entrySet()) {
+ if ("fs.defaultFS".equalsIgnoreCase(entry.getKey())) {
+ if (result != null && !result.equals(entry.getValue())) {
+ reject(access);
+ }
+ result = entry.getValue();
+ }
+ }
+ return result == null ? "" : result;
+ }
+
+ private void checkNative(String nameNode, String path) throws URISyntaxException {
+ nameNode = selector(nativeStringPrefix(nameNode)).trim();
+ if (nameNode.isEmpty() && access == Access.NATIVE_WRITE) {
+ // BE passes fs_name.c_str(), never NULL. libhdfs prefixes this empty string
+ // with hdfs://, whose URI construction fails before any filesystem is selected.
+ // Preserve existing analysis; execution reports the invalid NameNode as before.
+ return;
+ }
+ if ("default".equals(nameNode) || nameNode.isEmpty()) {
+ reject();
+ }
+ checkRoute(new URI(nameNode.contains("://") ? nameNode : "hdfs://" + nameNode),
+ path, new HashSet<>());
+ }
+
+ private void checkNativeRead(String nameNode, String path) throws URISyntaxException {
+ checkNative(nameNode, nativeFileUri(access, path, nameNode).getPath());
+ }
+
+ private List<URI> defaults() throws URISyntaxException {
+ List<URI> defaults = new ArrayList<>();
+ for (String key : Arrays.asList("fs.defaultFS", "fs.default.name")) {
+ String value = property(key);
+ if (value != null) {
+ value = selector(value).trim();
+ // FileSystem.fixName treats slash-free host[:port] strings as HDFS defaults.
+ defaults.add(new URI("local".equals(value) ? "file:///"
+ : (value.indexOf('/') < 0 ? "hdfs://" + value : value)));
+ }
+ }
+ if (defaults.isEmpty()) {
+ defaults.add(new URI("file:///"));
+ }
+ return defaults;
+ }
+
+ private void checkRoute(URI filesystem, String path, Set<String> activeTables) throws URISyntaxException {
+ if (filesystem.getScheme() == null) {
+ for (URI defaultFs : defaults()) {
+ checkViewFsEnabled(defaultFs, path, activeTables);
+ }
+ return;
+ }
+ if (filesystem.getAuthority() == null) {
+ boolean completed = false;
+ for (URI defaultFs : defaults()) {
+ if (filesystem.getScheme().equals(defaultFs.getScheme()) && defaultFs.getAuthority() != null) {
+ checkViewFsEnabled(defaultFs, path, activeTables);
+ completed = true;
+ }
+ }
+ if (completed) {
+ return;
+ }
+ }
+ checkViewFsEnabled(filesystem, path, activeTables);
+ }
+
+ private Provider provider(String scheme) {
+ if ("file".equalsIgnoreCase(scheme)) {
+ return Provider.LOCAL;
+ }
+ String implementation = property("fs." + scheme + ".impl");
+ if (implementation != null) {
+ implementation = selector(implementation).trim();
+ if ("org.apache.hadoop.fs.LocalFileSystem".equals(implementation)
+ || "org.apache.hadoop.fs.RawLocalFileSystem".equals(implementation)) {
+ return Provider.LOCAL;
+ }
+ if ("org.apache.hadoop.fs.viewfs.ViewFileSystem".equals(implementation)) {
+ return Provider.MOUNT_TABLE;
+ }
+ if ("org.apache.hadoop.fs.viewfs.ViewFileSystemOverloadScheme".equals(implementation)
+ || "org.apache.hadoop.hdfs.ViewDistributedFileSystem".equals(implementation)) {
+ return Provider.DYNAMIC;
+ }
+ return REMOTE_IMPLEMENTATIONS.contains(implementation) ? Provider.REMOTE : Provider.UNKNOWN;
+ }
+ if ("viewfs".equals(scheme)) {
+ return Provider.MOUNT_TABLE;
+ }
+ return REMOTE_SCHEMES.contains(scheme) ? Provider.REMOTE : Provider.UNKNOWN;
+ }
+
+ private void checkGlobTargets(String key, Set<String> activeTables) throws URISyntaxException {
+ for (String target : selector(property(key)).split(",", -1)) {
+ if (target.isEmpty()) {
+ reject();
+ }
+ URI uri = new URI(target);
+ // Listing can touch any descendant of a selected mount. Reuse the same route
+ // interpreter for nested tables instead of assuming their root is representative.
+ String path = uri.getPath() == null ? "/" : uri.getPath();
+ checkRoute(uri, (path.endsWith("/") ? path : path + "/") + "*", activeTables);
+ }
+ }
+
+ private void checkViewFsEnabled(URI filesystem, String path, Set<String> activeTables) {
+ String scheme = filesystem.getScheme();
+ Provider provider = provider(scheme);
+ if (provider == Provider.LOCAL || provider == Provider.UNKNOWN || provider == Provider.DYNAMIC) {
+ reject();
+ }
+ if (provider != Provider.MOUNT_TABLE) {
+ return;
+ }
+ if (!path.startsWith("/")) {
+ // Java resolves relative paths against the selected filesystem's working directory.
+ // Its user/home configuration is not a proven absolute mount-table path here.
+ reject();
+ }
+ String table = filesystem.getAuthority();
+ String ignorePort = property("fs.viewfs.ignore.port.in.mount.table.name");
+ if (ignorePort != null && Boolean.parseBoolean(selector(ignorePort).trim())
+ && table != null && filesystem.getPort() != -1) {
+ table = filesystem.getHost();
+ if (table == null) {
+ reject();
+ }
+ }
+ if (table == null || table.isEmpty()) {
+ table = property("fs.viewfs.mounttable.default.name.key");
+ table = table == null ? "default" : selector(table);
+ }
+ // Nested ViewFS cycles or arbitrarily deep delegation cannot establish remote storage.
+ if (activeTables.size() >= 32 || !activeTables.add(table)) {
+ reject();
+ }
+ try {
+ String prefix = "fs.viewfs.mounttable." + table + ".";
+ String source = "";
+ List<String> selectedKeys = new ArrayList<>();
+ List<String> mountSources = new ArrayList<>();
+ String fallback = null;
+ String rootMerge = null;
+ for (String key : properties.keySet()) {
+ if (!key.startsWith(prefix)) {
+ continue;
+ }
+ String entry = key.substring(prefix.length());
+ if (entry.equals("linkFallback")) {
+ fallback = key;
+ continue;
+ }
+ if (entry.equals("linkMergeSlash")) {
+ rootMerge = key;
+ continue;
+ }
+ if (entry.startsWith("linkRegex")) {
+ // Regex interception rewrites runtime paths before ordinary mount resolution.
+ reject();
+ }
+ int slash = entry.indexOf('/');
+ if (slash < 0 || !(entry.startsWith("link.") || entry.startsWith("linkMerge.")
+ || entry.startsWith("linkNfly."))) {
+ continue;
+ }
+ String candidate = entry.substring(slash);
+ // InodeTree splits source components and discards trailing empty components.
+ while (candidate.length() > 1 && candidate.endsWith("/")) {
+ candidate = candidate.substring(0, candidate.length() - 1);
+ }
+ mountSources.add(candidate);
+ if (!(path.equals(candidate) || path.startsWith(candidate + "/"))) {
+ continue;
+ }
+ if (candidate.length() > source.length()) {
+ source = candidate;
+ selectedKeys.clear();
+ }
+ if (candidate.equals(source)) {
+ selectedKeys.add(key);
+ }
+ }
+ if (access == Access.HADOOP_READ && hasGlob(path)) {
+ String literal = path.substring(0, firstGlob(path));
+ int slash = literal.lastIndexOf('/');
+ literal = slash < 0 ? "/" : literal.substring(0, slash + 1);
+ for (String key : properties.keySet()) {
+ if (!key.startsWith(prefix)) {
+ continue;
+ }
+ String entry = key.substring(prefix.length());
+ int sourceStart = entry.indexOf('/');
+ if (sourceStart >= 0 && (entry.startsWith("link.") || entry.startsWith("linkMerge.")
+ || entry.startsWith("linkNfly."))) {
+ String mount = entry.substring(sourceStart);
+ if (mount.startsWith(literal) || literal.startsWith(mount + "/")) {
+ checkGlobTargets(key, activeTables);
+ }
+ }
+ }
+ }
+ if (access == Access.HADOOP_READ && !hasGlob(path)) {
+ String subtree = path.endsWith("/") ? path : path + "/";
+ for (String key : properties.keySet()) {
+ if (key.startsWith(prefix)) {
+ String entry = key.substring(prefix.length());
+ int sourceStart = entry.indexOf('/');
+ if (sourceStart >= 0 && entry.substring(sourceStart).startsWith(subtree)) {
+ checkGlobTargets(key, activeTables);
+ }
+ }
+ }
+ }
+ if (rootMerge != null) {
+ selectedKeys.add(rootMerge);
+ source = "";
+ } else if (selectedKeys.isEmpty() && fallback != null) {
+ selectedKeys.add(fallback);
+ }
+ if (selectedKeys.isEmpty()) {
+ // Missing inline fallback does not prove absence of one in default resources.
+ // Keep rejecting unproven routes, including globs that cross mount names.
+ reject();
+ }
+ for (String key : selectedKeys) {
+ String targets = selector(property(key));
+ for (String target : targets.split(",", -1)) {
Review Comment:
Confirmed Hadoop SINGLE, SINGLE_FALLBACK and MERGE_SLASH each consume one URI. Admission now preserves their complete comma-containing paths in both selected and glob-target checks; only MERGE/NFLY use comma lists. Updated controls cover read/write/cleanup/glob and retain rejection of local/malformed multi-target routes and truly empty single targets.
Follow-up commit: aad01e0da033db903a8cd088cd733ee0623c8709. 73 selected FE tests passed with fresh XML, no failures/errors/skips and unchanged inputs; targeted Checkstyle passed.
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/LocalTableValuedFunction.java:
##########
@@ -135,6 +143,38 @@ private void getFileListFromBackend() throws AnalysisException {
}
}
+ /** Apply LOCAL admission to sinks that select the built-in local JNI writer as well. */
+ public static void checkSinkEnabled(String tvfName, Map<String, String> properties) {
+ String writerClass = properties.getOrDefault("writer_class", "");
+ // JNI consumes a C string, and ScannerLoader accepts slash-separated class names.
+ int nul = writerClass.indexOf('\0');
+ if (nul >= 0) {
+ writerClass = writerClass.substring(0, nul);
+ }
+ if ("local".equalsIgnoreCase(tvfName)
Review Comment:
Reopening for the retained inherited-provider variant. The explicit local provider/NameNode examples are fixed, but the BE-only core-site LocalFileSystem case remains valid: the FE map does not represent that effective runtime Configuration. Earlier replies overstated coverage of inherited configuration. A forced provider substitution would change administrator configuration semantics and still would not close ViewFS overlays, so it is not included as an unproven minimal fix. The PR description now states this remaining P1; complete runtime enforcement is not claimed.
Follow-up commit: aad01e0da033db903a8cd088cd733ee0623c8709. 73 selected FE tests passed with fresh XML, no failures/errors/skips and unchanged inputs; targeted Checkstyle passed.
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/LocalTableValuedFunction.java:
##########
@@ -135,6 +150,332 @@ private void getFileListFromBackend() throws AnalysisException {
}
}
+ /** Apply LOCAL admission to the effective sink before planning or filesystem side effects. */
+ public static void checkSinkEnabled(String tvfName, Map<String, String> properties) {
+ if ("local".equalsIgnoreCase(tvfName) || isLocalWriter(properties.getOrDefault("writer_class", ""))) {
+ checkEnabled();
+ }
+ if ("hdfs".equalsIgnoreCase(tvfName)) {
+ if (!properties.containsKey("writer_class")) {
+ checkHdfsSinkEnabled(properties);
+ }
+ if (Boolean.parseBoolean(properties.getOrDefault("delete_existing_files", "false"))) {
+ checkHadoopFileSystemEnabled(
+ BrokerUtil.extractParentDirectory(properties.getOrDefault("file_path", "")), properties);
+ }
+ }
+ }
+
+ private static boolean isLocalWriter(String writer) {
+ // ScannerLoader accepts slashes, and JNI passes the class name as a C string.
+ return "org.apache.doris.writer.LocalFileJniWriter".equals(nativeStringPrefix(writer).replace('/', '.'));
+ }
+
+ /** Check the NameNode and provider used by the native FILE_HDFS writer. */
+ public static void checkHdfsSinkEnabled(Map<String, String> properties) {
+ checkHdfsSinkEnabled(properties.getOrDefault("file_path", ""), properties);
+ }
+
+ public static void checkHdfsSinkEnabled(String outputPrefix, Map<String, String> properties) {
+ String nameNode = nativeStringPrefix(properties.getOrDefault("fs.defaultFS", "")).trim();
+ if ("default".equals(nameNode)) {
+ // libhdfs delegates this selector to the BE's defaults, which may select local storage.
+ checkEnabled();
+ }
+ // libhdfs prefixes bare NameNodes with hdfs:// rather than using the output path's scheme.
+ checkHadoopSchemeEnabled(nameNode.contains("://") ? uriScheme(nameNode) : "hdfs", properties);
+ if (!Config.enable_local_tvf) {
+ try {
+ URI filesystem = new URI(nameNode.contains("://") ? nameNode : "hdfs://" + nameNode);
+ // BE appends a generated filename directly to this prefix, then convert_path
+ // removes the NameNode. Resolve its parent, not an exact mount-point filename.
+ String generated = outputPrefix + "doris-tvf-file";
+ String rawNameNode = properties.getOrDefault("fs.defaultFS", "");
+ if (!rawNameNode.isEmpty() && generated.contains(rawNameNode)) {
+ generated = generated.substring(rawNameNode.length());
+ }
+ if (!generated.startsWith("/")) {
+ generated = "/" + generated;
+ }
+ String path = hadoopPathUri(nativeStringPrefix(generated)).getPath();
+ path = path.substring(0, path.lastIndexOf('/') + 1);
+ if (path.length() > 1) {
+ path = path.substring(0, path.length() - 1);
+ }
+ checkViewFsEnabled(filesystem, path, properties, new HashSet<>());
+ } catch (URISyntaxException e) {
+ // Invalid NameNodes remain subject to the existing storage validation.
+ }
+ }
+ }
+
+ /** Check Hadoop's path/default-filesystem selection before FE-side cleanup. */
+ public static void checkHadoopFileSystemEnabled(String path, Map<String, String> properties) {
Review Comment:
Reopening for the retained implicit overloaded-provider variant. Explicit ViewFS/local routes are rejected, but a provider inherited from FE/BE default resources can preserve an hdfs:// path while delegating locally, so repeated URI/map admission is insufficient. Loading only FE defaults cannot establish BE behavior. This remains unresolved pending validation at the actual consumer/configuration boundary. No blanket rejection of valid remote ViewFS or generic Hadoop/catalog behavior change is included, and the PR description now records this limitation.
Follow-up commit: aad01e0da033db903a8cd088cd733ee0623c8709. 73 selected FE tests passed with fresh XML, no failures/errors/skips and unchanged inputs; targeted Checkstyle passed.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6396114833
<!-- doris-repo-review:v1:begin -->
### Local pipeline review — ✅ PASS
```yaml
schema: doris-repo-review/v1
status: PASS
pr: apache/doris#69528
commit: f293dcebdba35652d127a186ef304d66936a8b23
base: 00a9e1cb6dd99355f1b385e3c3ec5c23ae5a83cf
reviewed_at: 2026-10-25T17:46+08:00
reviewer: OIiveirra
model: gpt-6.1-sol
effort: xhigh
findings: {blocker: 0, major: 0, minor: 0, nit: 0}
rounds: 1
converged: true
```
**Notes for maintainers**
- `fe/be-java-extensions/avro-scanner/src/main/java/org/apache/doris/avro/AvroJNIScanner.java:171` — Rechecked failed production open cleanup, preassignment schema input and original/suppressed failures; previous runtime-03 is fixed.
- `fe/be-java-extensions/hadoop-deps/src/main/java/org/apache/hadoop/fs/FileSystem.java:3695` — Literal metacharacters and later real wildcard prefixes retain their identity; previous runtime-04 is fixed while actual-local negatives remain.
- Full aggregate review covers 65 files and converged in round 1 with zero findings. Source-bound validation records 124 selected FE tests and 26 applicable Java Checkstyle files passing; full community CI and mixed-version behavior were not run in this review.
<sub>Reviewed locally with the `doris-repo-review` pipeline. Repository policy may accept this receipt for the matching commit; it is not a human Apache approval.</sub>
<!-- doris-repo-review:v1:end -->
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6399728253
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6393511027
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4369693987
##########
fe/be-java-extensions/hadoop-deps/src/main/java/org/apache/hadoop/fs/FileSystem.java:
##########
@@ -3622,13 +3633,670 @@ public static Class<? extends FileSystem> getFileSystemClass(String scheme,
* @return the initialized filesystem.
* @throws IOException problems loading or initializing the FileSystem
*/
+ // DORIS-PATCH: TVF policy is checked against the frozen configuration used by the consumer.
+ public static int getDorisTvfGuardVersion() {
+ return 1;
+ }
+
+ public static final class TvfGuard {
+ public static final String POLICY_KEY = "fs.doris.tvf.remote.paths";
+ public static final String NATIVE_POLICY = "native";
+ public static final String JAVA_READ_POLICY = "java-read";
+ public static final String ROOT_KEY = "fs.doris.tvf.root";
+ public static final String PATH_KEY = "fs.doris.tvf.path";
+ public static final String CONTEXT_KEY = "fs.doris.tvf.context";
+ private static final ThreadLocal<Context> LAST_NATIVE_CONTEXT = new ThreadLocal<>();
+
+ private TvfGuard() {
+ }
+
+ public static final class Rule {
+ private final String filesystem;
+ private final String path;
+ private final boolean descendants;
+
+ public Rule(String filesystem, String path, boolean descendants) {
+ this.filesystem = filesystem;
+ this.path = path;
+ this.descendants = descendants;
+ }
+ }
+
+ public static String encode(List<Rule> rules) {
+ if (FileSystem.getDorisTvfGuardVersion() != 1) {
+ throw new IllegalStateException("TVF Hadoop runtime policy version mismatch");
+ }
+ try {
+ java.io.ByteArrayOutputStream bytes = new java.io.ByteArrayOutputStream();
+ java.io.DataOutputStream out = new java.io.DataOutputStream(bytes);
+ out.writeInt(1);
+ out.writeInt(rules.size());
+ for (Rule rule : rules) {
+ out.writeUTF(rule.filesystem);
+ out.writeUTF(rule.path);
+ out.writeBoolean(rule.descendants);
+ }
+ out.flush();
+ return java.util.Base64.getEncoder().encodeToString(bytes.toByteArray());
+ } catch (IOException e) {
+ throw new IllegalArgumentException("Cannot encode TVF Hadoop route policy", e);
+ }
+ }
+
+ /** Routes touched by Globber, without constructing a filesystem or listing data. */
+ public static List<Rule> globRules(URI filesystem, String pattern) throws IOException {
+ List<Rule> rules = new ArrayList<>();
+ rules.add(new Rule(filesystem.toString(), pattern, true));
+ if (pattern.indexOf('\\') < 0) {
+ return rules;
+ }
+ for (String expanded : GlobExpander.expand(pattern)) {
+ Path flattened = new Path(expanded.isEmpty() ? Path.CUR_DIR : expanded);
+ String path = flattened.toUri().getPath();
+ Path candidate = new Path(flattened.isAbsolute() ? "/" : Path.CUR_DIR);
+ for (String component : path.split("/")) {
+ if (component.isEmpty()) {
+ continue;
+ }
+ GlobFilter filter = new GlobFilter(component);
+ if (filter.hasPattern()) {
+ // Globber lists this parent before it knows the matching child names. Keep
+ // prior wildcard components as a conservative subtree route, including when
+ // a later escaped '..' moves the final candidate outside that subtree.
+ rules.add(new Rule(filesystem.toString(), new Path(candidate, "*").toUri().getPath(), true));
+ }
+ component = component.replaceAll("\\\\(.)", "$1");
+ candidate = new Path(candidate, component);
+ }
+ rules.add(new Rule(filesystem.toString(), candidate.toUri().getPath(), true));
+ }
+ return rules;
+ }
+
+ private static IOException rejected() {
+ return new IOException("Hadoop TVF requires a proven remote filesystem route. "
+ + "Use local() with LOCAL TVF privileges and secure-path restrictions.");
+ }
+
+ private static Configuration freeze(Configuration conf) throws IOException {
+ // Service providers (notably HDFS) register their default/site resources on first load.
+ // Load them before copying raw values, as an ordinary FileSystem lookup does.
+ if (!FILE_SYSTEMS_LOADED) {
+ loadFileSystems();
+ }
+ if (conf instanceof FrozenConfiguration) {
+ return conf;
+ }
+ return new FrozenConfiguration(conf);
+ }
+
+ private static Configuration prepare(Configuration conf) throws IOException {
+ String policy = conf.getRaw(POLICY_KEY);
+ if (policy == null || policy.isEmpty()) {
+ return conf;
+ }
+ Configuration frozen = freeze(conf);
+ Context context = ((FrozenConfiguration) frozen).context;
+ if (NATIVE_POLICY.equals(policy)) {
+ context.checkNative(frozen.getRaw(ROOT_KEY), frozen.getRaw(PATH_KEY));
+ } else if (JAVA_READ_POLICY.equals(policy)) {
+ context.checkJava(new Path(frozen.getRaw(PATH_KEY)));
+ } else {
+ context.checkRules(policy);
+ }
+ return frozen;
+ }
+
+ public static void check(URI ignored, Configuration conf) throws IOException {
+ prepare(conf);
+ }
+
+ public static void clearNativeContext() {
+ LAST_NATIVE_CONTEXT.remove();
+ }
+
+ public static Context takeNativeContext() {
+ Context result = LAST_NATIVE_CONTEXT.get();
+ LAST_NATIVE_CONTEXT.remove();
+ return result;
+ }
+
+ public static Configuration prepareJavaRead(Configuration conf, Path path, String marker) throws IOException {
+ if (marker == null || marker.isEmpty()) {
+ return conf;
+ }
+ if (!NATIVE_POLICY.equals(marker)) {
+ throw rejected();
+ }
+ Configuration request = new Configuration(conf);
+ request.set(POLICY_KEY, JAVA_READ_POLICY);
+ request.set(ROOT_KEY, path.toUri().toString());
+ request.set(PATH_KEY, path.toString());
+ return prepare(request);
+ }
+
+ public static void checkJavaRead(FileSystem fs, Path path, String marker) throws IOException {
+ if (marker == null || marker.isEmpty()) {
+ return;
+ }
+ if (!NATIVE_POLICY.equals(marker)) {
+ throw rejected();
+ }
+ // The marker is request-owned, never inferred from an unmarked cached filesystem.
+ Configuration conf = fs.getConf();
+ if (JAVA_READ_POLICY.equals(conf.getRaw(POLICY_KEY))) {
+ // S3A derives a Configuration for bucket options. Its copy preserves our internal
+ // marker and raw snapshot values, but not the private Configuration subclass.
+ ((FrozenConfiguration) freeze(conf)).context.checkJava(path);
+ } else {
+ throw rejected();
+ }
+ }
+
+ private static final class FrozenConfiguration extends Configuration {
+ private final Context context;
+
+ private FrozenConfiguration(Configuration source) throws IOException {
+ super(false);
+ setClassLoader(source.getClassLoader());
+ java.util.SortedMap<String, String> values = new java.util.TreeMap<>();
+ for (Map.Entry<String, String> entry : source) {
+ String key = entry.getKey();
+ String value = source.getRaw(key);
+ set(key, value);
+ if (!PATH_KEY.equals(key) && !ROOT_KEY.equals(key) && !CONTEXT_KEY.equals(key)) {
+ values.put(key, value);
+ }
+ }
+ // ViewFS's private InnerCache folds authority case (A/a). Guarded children must
+ // instead use FileSystem's exact-authority/profile cache below.
+ set("fs.viewfs.enable.inner.cache", "false");
Review Comment:
[P2] Release ViewFS child clients when disabling its inner cache
For a remote mount `/data -> s3a://bucket/base`, repeated `hdfs('uri'='viewfs://cluster/data/fileN.csv', 'fs.defaultFS'='viewfs://cluster', 'format'='csv', 'csv_schema'='id:int', ...)` calls with unchanged credentials now create a distinct S3A client for each path: `protectJava()` includes the listing URI/path in the policy, and `Cache.Key` distinguishes those policies. This override routes children into Hadoop's global cache, but `DFSFileSystem` only disables HDFS caching; [the pinned ViewFS close path](https://github.com/apache/doris-thirdparty/blob/hadoop-3.4.2.2-for-doris/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java#L1828-L1866) closes children only when their scheme's cache is disabled. Default-cached S3A children therefore survive closing/collecting the outer filesystem and remain in the global HashMap without eviction, accumulating clients, HTTP pools and memory for the FE lifetime. Preserve exact authority/profile isolat
ion while adding bounded ownership and cleanup for these request-scoped children, with a many-distinct-paths lifecycle regression.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6401467116
# FE UT Coverage Report
Increment line coverage ` 84.98% (232/273)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/69528_b1fb0a3fe44334d62eacb8590dbae3aa1ac49e72/fe_increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/69528_b1fb0a3fe44334d62eacb8590dbae3aa1ac49e72/fe_report/index.html)
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4365938520
##########
be/src/io/fs/local_file_system.cpp:
##########
@@ -455,28 +455,89 @@ 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));
+ if (file_path.rfind("file:", 0) == 0) {
+ // file:a%20b names "a b", but a bare path containing "%20" must stay literal.
+ // Decode once, preserve '+' and invalid escapes, matching Lance's file provider.
+ auto encoded = local_path.string();
+ // Query and fragment are URI components, not part of the filesystem path.
+ encoded = encoded.substr(0, encoded.find_first_of("?#"));
+ const auto hex_digit = [](char c) -> int {
+ if (c >= '0' && c <= '9') return c - '0';
+ if (c >= 'a' && c <= 'f') return c - 'a' + 10;
+ if (c >= 'A' && c <= 'F') return c - 'A' + 10;
+ return -1;
+ };
+ std::string decoded;
+ decoded.reserve(encoded.size());
+ for (size_t i = 0; i < encoded.size(); ++i) {
+ if (encoded[i] == '%' && i + 2 < encoded.size()) {
+ const int high = hex_digit(encoded[i + 1]);
+ const int low = hex_digit(encoded[i + 2]);
+ if (high >= 0 && low >= 0) {
+ decoded += static_cast<char>((high << 4) | low);
+ i += 2;
+ continue;
+ }
+ }
+ decoded += encoded[i];
+ }
+ // %00 must not truncate the path when canonicalization reaches C filesystem APIs.
+ if (decoded.find('\0') != std::string::npos) {
+ return Status::InvalidArgument("local file path must not contain NUL");
+ }
+ local_path = std::move(decoded);
+ }
+ 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);
Review Comment:
[P2] Normalize file-URI dot segments before resolving symlinks
With `enable_local_tvf=true`, an ADMIN/OPERATOR, `user_files_secure_path=/secure`, and `/secure/link -> /secure/nested/child`, `file:///secure/link/../sample.lance` previously opened `/secure/sample.lance`: [Lance parses the URI first](https://github.com/lance-format/lance/blob/68c12dfd7efe02f90ad2d7f3a239a7eb64884e57/rust/lance-io/src/object_store.rs#L494-L502), and its [URL parser removes `link/..` lexically](https://github.com/servo/rust-url/blob/v2.5.8/url/src/parser.rs#L1264-L1275). This new filesystem canonicalization instead follows `link` before processing `..`, producing `/secure/nested/sample.lance`. If both datasets exist, schema discovery and scanning silently read the other dataset; otherwise a valid URI fails. Both targets are inside the secure root. This is distinct from the fixed percent-decoding thread: it needs no escaped characters and arises at the new canonicalization step. Normalize file-URI path segments before filesystem resolution and containment, preserve
bare-path semantics, and add a schema/scan case with different datasets on the two paths.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4366880442
##########
be/src/format_v2/table/lance_reader.cpp:
##########
@@ -22,24 +22,150 @@
#include <arrow/record_batch.h>
#include <arrow/type.h>
#include <lance/lance.h>
+#include <pwd.h>
+#include <unistd.h>
#include <algorithm>
#include <bit>
+#include <cerrno>
+#include <cstdlib>
#include <cstring>
+#include <filesystem>
#include <limits>
#include <memory>
+#include <unordered_set>
#include "common/config.h"
#include "common/logging.h"
#include "exec/common/endian.h"
#include "format_v2/lance/lance_reader_helper.h"
#include "format_v2/lance/lance_runtime_filter_helper.h"
#include "format_v2/lance/lance_session_manager.h"
+#include "io/fs/local_file_system.h"
#include "runtime/file_scan_profile.h"
#include "runtime/runtime_state.h"
namespace doris::format::lance {
+namespace {
+std::string expand_home_path(const std::string& path) {
+ if (path != "~" && !path.starts_with("~/")) {
+ return path;
+ }
+ // Match the SDK's std::env::home_dir: HOME first, then the current user's passwd entry.
+ std::string home;
+ if (const char* value = std::getenv("HOME"); value != nullptr && value[0] != '\0') {
+ home = value;
+ } else {
+ std::vector<char> buffer(1024);
+ struct passwd entry {};
+ struct passwd* result = nullptr;
+ int error;
+ while ((error = getpwuid_r(getuid(), &entry, buffer.data(), buffer.size(), &result)) ==
+ ERANGE) {
+ buffer.resize(buffer.size() * 2);
+ }
+ if (error == 0 && result != nullptr && entry.pw_dir != nullptr) {
+ home = entry.pw_dir;
+ }
+ }
+ if (home.empty()) {
+ return path;
+ }
+ return path == "~" ? home : (io::Path(home) / path.substr(2)).string();
+}
+
+Status validate_local_dataset_files(const std::string& dataset_uri) {
+ // Lance opens manifests and fragments itself, so a confined dataset directory is not
+ // enough: its descendants can redirect those opens through symlinks.
+ std::error_code error;
+ const auto secure_root = std::filesystem::canonical(config::user_files_secure_path, error);
+ if (error) {
+ return Status::InvalidArgument("local Lance dataset path is not allowed");
+ }
+ std::vector<std::filesystem::path> pending {dataset_uri};
+ std::unordered_set<std::string> visited;
+ while (!pending.empty()) {
+ auto directory = std::move(pending.back());
+ pending.pop_back();
+ // Canonical directory identity bounds cycles and repeated in-root aliases without
+ // imposing a file-count or nesting limit on otherwise valid datasets.
+ if (!visited.insert(directory.string()).second) {
+ continue;
+ }
+ std::filesystem::directory_iterator entries(directory, error);
+ if (error) {
+ return Status::InvalidArgument("local Lance dataset path is not allowed");
+ }
+ const std::filesystem::directory_iterator end;
+ while (entries != end) {
+ auto target = entries->path();
+ auto status = entries->symlink_status(error);
+ if (error) {
+ return Status::InvalidArgument("local Lance dataset path is not allowed");
+ }
+ if (std::filesystem::is_symlink(status)) {
+ target = std::filesystem::canonical(target, error);
+ if (error || !io::LocalFileSystem::contain_path(secure_root, target)) {
+ return Status::InvalidArgument("local Lance dataset path is not allowed");
+ }
+ status = std::filesystem::status(target, error);
+ if (error) {
+ return Status::InvalidArgument("local Lance dataset path is not allowed");
+ }
+ }
+ if (std::filesystem::is_directory(status)) {
+ pending.push_back(std::move(target));
+ }
+ entries.increment(error);
+ if (error) {
+ return Status::InvalidArgument("local Lance dataset path is not allowed");
+ }
+ }
+ }
+ return Status::OK();
+}
+
+Status resolve_dataset_uri(const TFileRangeDesc& range, const TFileScanRangeParams& scan_params,
+ std::string* uri) {
+ const auto& requested_uri = range.table_format_params.lance_params.dataset_uri;
+ // Modern plans tag each range; older plans put the type in the scan parameters.
+ // Untagged catalog scans retain their original storage-provider selection.
+ const bool is_local = range.__isset.file_type
+ ? range.file_type == TFileType::FILE_LOCAL
+ : scan_params.__isset.file_type &&
+ scan_params.file_type == TFileType::FILE_LOCAL;
+ // LOCAL TVF uses version zero for both schema discovery and whole-dataset scans.
+ // Catalog scans pin a positive snapshot version, including file:// catalogs whose
+ // operator-configured warehouse is independent of the LOCAL TVF secure directory.
+ if (is_local && range.table_format_params.lance_params.version == 0) {
+ // LOCAL Lance bypasses glob because it opens a dataset directory. Apply the same
+ // canonical secure-root check before either schema discovery or a cached dataset open.
+ std::string local_input = requested_uri;
+ const auto colon = requested_uri.find(':');
+ const auto slash = requested_uri.find('/');
+ const bool bare_path =
+ colon == std::string::npos || (slash != std::string::npos && slash < colon);
+ if (bare_path && !requested_uri.empty() && requested_uri.find('\0') == std::string::npos &&
+ !config::user_files_secure_path.empty()) {
+ // Lance's PathAbs fallback resolves bare dot segments before following symlinks.
+ // Anchor relative TVF paths to the canonical secure root, not the process cwd.
+ std::string secure_root;
+ RETURN_IF_ERROR(io::global_local_filesystem()->canonicalize(
+ config::user_files_secure_path, &secure_root));
+ local_input = (io::Path(secure_root) / expand_home_path(requested_uri))
+ .lexically_normal()
+ .string();
+ }
+ RETURN_IF_ERROR(io::global_local_filesystem()->canonicalize_local_file(
+ config::user_files_secure_path, local_input, uri));
+ return validate_local_dataset_files(*uri);
Review Comment:
[P1] Validate manifest-selected storage before Lance opens fragments
This walk checks physical descendants, but valid Lance manifests can select another storage base without symlinks. The [pinned SDK's shallow clone](https://github.com/lance-format/lance/blob/68c12dfd7efe02f90ad2d7f3a239a7eb64884e57/rust/lance-table/src/format/manifest.rs#L241-L305) assigns `DataFile.base_id` and records the source URI. With LOCAL enabled, `/secure/clone.lance` can contain only regular metadata yet reference `/private/source.lance`: schema discovery and this walk succeed, then the SDK [resolves the external base/provider](https://github.com/lance-format/lance/blob/68c12dfd7efe02f90ad2d7f3a239a7eb64884e57/rust/lance/src/dataset.rs#L2441-L2493) and [opens its fragments](https://github.com/lance-format/lance/blob/68c12dfd7efe02f90ad2d7f3a239a7eb64884e57/rust/lance/src/dataset/fragment.rs#L1420-L1454) outside `user_files_secure_path`.
The same mechanism also bypasses the default-off switch: an ordinary S3 TVF caller can supply a valid `s3://bucket/clone.lance` manifest with a compatible scalar schema, known readable local fragment, and `base_id -> file:///private/source.lance`. `LanceMetadataLoader` BASIC loading only reads version/schema/fragment fields, and the top-level remote-URI guard accepts it; the `FILE_S3` fixed-version BE range skips this local branch and opens that local base. This needs no malformed URI and is distinct from the existing parser threads. Validate effective manifest bases before TVF data opens: reject local bases for remote TVFs and enforce canonical containment for LOCAL bases, or reject external-base TVFs until that boundary is enforceable. Cover out-of-root and entirely in-root shallow clones.
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/LocalTableValuedFunction.java:
##########
@@ -135,6 +150,332 @@ private void getFileListFromBackend() throws AnalysisException {
}
}
+ /** Apply LOCAL admission to the effective sink before planning or filesystem side effects. */
+ public static void checkSinkEnabled(String tvfName, Map<String, String> properties) {
+ if ("local".equalsIgnoreCase(tvfName) || isLocalWriter(properties.getOrDefault("writer_class", ""))) {
+ checkEnabled();
+ }
+ if ("hdfs".equalsIgnoreCase(tvfName)) {
+ if (!properties.containsKey("writer_class")) {
+ checkHdfsSinkEnabled(properties);
+ }
+ if (Boolean.parseBoolean(properties.getOrDefault("delete_existing_files", "false"))) {
+ checkHadoopFileSystemEnabled(
+ BrokerUtil.extractParentDirectory(properties.getOrDefault("file_path", "")), properties);
+ }
+ }
+ }
+
+ private static boolean isLocalWriter(String writer) {
+ // ScannerLoader accepts slashes, and JNI passes the class name as a C string.
+ return "org.apache.doris.writer.LocalFileJniWriter".equals(nativeStringPrefix(writer).replace('/', '.'));
+ }
+
+ /** Check the NameNode and provider used by the native FILE_HDFS writer. */
+ public static void checkHdfsSinkEnabled(Map<String, String> properties) {
+ checkHdfsSinkEnabled(properties.getOrDefault("file_path", ""), properties);
+ }
+
+ public static void checkHdfsSinkEnabled(String outputPrefix, Map<String, String> properties) {
+ String nameNode = nativeStringPrefix(properties.getOrDefault("fs.defaultFS", "")).trim();
+ if ("default".equals(nameNode)) {
+ // libhdfs delegates this selector to the BE's defaults, which may select local storage.
+ checkEnabled();
+ }
+ // libhdfs prefixes bare NameNodes with hdfs:// rather than using the output path's scheme.
+ checkHadoopSchemeEnabled(nameNode.contains("://") ? uriScheme(nameNode) : "hdfs", properties);
+ if (!Config.enable_local_tvf) {
+ try {
+ URI filesystem = new URI(nameNode.contains("://") ? nameNode : "hdfs://" + nameNode);
+ // BE appends a generated filename directly to this prefix, then convert_path
+ // removes the NameNode. Resolve its parent, not an exact mount-point filename.
+ String generated = outputPrefix + "doris-tvf-file";
+ String rawNameNode = properties.getOrDefault("fs.defaultFS", "");
+ if (!rawNameNode.isEmpty() && generated.contains(rawNameNode)) {
+ generated = generated.substring(rawNameNode.length());
+ }
+ if (!generated.startsWith("/")) {
+ generated = "/" + generated;
+ }
+ String path = hadoopPathUri(nativeStringPrefix(generated)).getPath();
+ path = path.substring(0, path.lastIndexOf('/') + 1);
+ if (path.length() > 1) {
+ path = path.substring(0, path.length() - 1);
+ }
+ checkViewFsEnabled(filesystem, path, properties, new HashSet<>());
+ } catch (URISyntaxException e) {
+ // Invalid NameNodes remain subject to the existing storage validation.
+ }
+ }
+ }
+
+ /** Check Hadoop's path/default-filesystem selection before FE-side cleanup. */
+ public static void checkHadoopFileSystemEnabled(String path, Map<String, String> properties) {
Review Comment:
[P1] Apply effective-filesystem admission to Hadoop TVF reads
With `enable_local_tvf=false`, an ordinary user can still execute `SELECT * FROM file("uri"="viewfs://review/local/data.csv", "fs.defaultFS"="viewfs://review", "fs.viewfs.mounttable.review.link./local"="file:///tmp", "format"="csv", "csv_schema"="id:int")`. Given a readable `/tmp/data.csv` on the FE and selected BE, this reads the BE-local file even when `/tmp` is outside `user_files_secure_path`. FILE delegates to `HdfsTableValuedFunction`; its `init -> parseFile -> DFSFileSystem.globList` selects ViewFS without calling this admission helper or LOCAL's ADMIN/OPERATOR check. The range stays `FILE_HDFS`, and `FileFactory -> HdfsFileReader` reaches the [pinned ViewFS target open](https://github.com/apache/doris-thirdparty/blob/hadoop-3.4.2.2-for-doris/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java#L686-L690), bypassing BE local containment too. `hdfs(...)` exposes the same read route. This is distinct from the existing native INSERT
/cleanup thread: its fixes only invoke these helpers on writes/deletion. Check the effective normalized filesystem before Hadoop TVF listing, including FILE delegation, and enforce LOCAL read privileges/containment for local targets; add disabled-switch and truly remote read controls.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6385348042
# BE Regression && UT Coverage Report
Increment line coverage `80.30% (212/264)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/69528_9df0508783e6b1da6be6ef9909de9ebc0db45db8_merge/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/69528_9df0508783e6b1da6be6ef9909de9ebc0db45db8_merge/report/index.html)
| Category | Coverage |
|-------------------|--------------------|
| Function Coverage | 75.10% (32060/42692) |
| Line Coverage | 59.48% (359773/604850) |
| Region Coverage | 56.13% (299780/534039) |
| Branch Coverage | 57.08% (136269/238732) |
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6385353927
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6379573679
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6383035420
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6396462446
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6399562602
<!-- doris-repo-review:v1:begin -->
### Local pipeline review — ✅ PASS
```yaml
schema: doris-repo-review/v1
status: PASS
pr: apache/doris#69528
commit: b1fb0a3fe44334d62eacb8590dbae3aa1ac49e72
base: 00a9e1cb6dd99355f1b385e3c3ec5c23ae5a83cf
reviewed_at: 2026-10-25T23:31+08:00
reviewer: OIiveirra
model: gpt-6.1-sol
effort: xhigh
findings: {blocker: 0, major: 0, minor: 0, nit: 0}
rounds: 1
converged: true
```
**Notes for maintainers**
- `fe/be-java-extensions/hadoop-deps/src/main/java/org/apache/hadoop/fs/FileSystem.java:4265` — finite same-table ViewFS aliases now use unambiguous resolved table/path identity. Actual pinned Hadoop3.4.2 SINGLE lazy initialization and real unguarded/guarded dispatch independently establish the known route.
- `fe/be-java-extensions/hadoop-deps/src/main/java/org/apache/hadoop/fs/FileSystem.java:4373` — successful state insertion has matching finally removal; repeated states, growing-path cycles, local targets and the independent32-state bound remain rejected as appropriate.32 mount states may terminate remotely;33 rejects.
- `fe/fe-core/src/test/java/org/apache/doris/tablefunction/HadoopTvfRuntimeAdmissionTest.java:391` — new controls cover actual ViewFS stat/literal-delete paths, native retained-Context admission, both LOCAL settings and static32/33 boundary. Only recognized remote leaf storage is mocked; real native writer IO or32-hop network execution is not claimed.
- Full65-file review converged in round1 with zero findings. Parent source-bound129 FE tests/nine fresh required XML and26-file style pass; review ran no tests/builds. Unchanged C++/SDK/Thrift evidence is reused by bytes, without claiming full CI/SQL/mixed-version execution. Master and4.2 are excluded.
<sub>Reviewed locally with the `doris-repo-review` pipeline. Repository policy may accept this receipt for the matching commit; it is not a human Apache approval.</sub>
<!-- doris-repo-review:v1:end -->
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4367696772
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/HadoopTvfAdmission.java:
##########
@@ -0,0 +1,477 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.tablefunction;
+
+import org.apache.doris.common.Config;
+import org.apache.doris.common.util.LocationPath;
+import org.apache.doris.nereids.exceptions.AnalysisException;
+
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * TVF-only admission for Hadoop consumers. Input properties have already been normalized by storage.
+ * This module neither loads Hadoop resources nor constructs a filesystem. Java strings and native
+ * C strings are projected separately before filesystem selection and mount-table interpretation.
+ */
+public final class HadoopTvfAdmission {
+ public enum Access {
+ NATIVE_WRITE,
+ JAVA_DELETE,
+ HADOOP_READ
+ }
+
+ private enum Provider {
+ REMOTE,
+ LOCAL,
+ MOUNT_TABLE,
+ DYNAMIC,
+ UNKNOWN
+ }
+
+ private static final Set<String> REMOTE_SCHEMES = new HashSet<>(Arrays.asList(
+ "hdfs", "s3a", "s3n", "oss", "ofs", "jfs", "abfs", "abfss", "wasb", "wasbs", "gs", "cosn"));
+ private static final Set<String> REMOTE_IMPLEMENTATIONS = new HashSet<>(Arrays.asList(
+ "org.apache.hadoop.hdfs.DistributedFileSystem",
+ "org.apache.hadoop.fs.s3a.S3AFileSystem",
+ "org.apache.hadoop.fs.azurebfs.AzureBlobFileSystem",
+ "org.apache.hadoop.fs.azurebfs.SecureAzureBlobFileSystem",
+ "org.apache.hadoop.fs.azure.NativeAzureFileSystem",
+ "org.apache.hadoop.fs.aliyun.oss.AliyunOSSFileSystem",
+ "com.aliyun.jindodata.oss.JindoOssFileSystem",
+ "org.apache.hadoop.fs.obs.OBSFileSystem",
+ "com.google.cloud.hadoop.fs.gcs.GoogleHadoopFileSystem"));
+
+ private final Access access;
+ private final Map<String, String> properties = new HashMap<>();
+
+ private HadoopTvfAdmission(Access access, Map<String, String> normalized, boolean nativeConsumer) {
+ this.access = access;
+ for (Map.Entry<String, String> entry : normalized.entrySet()) {
+ // parse_properties / generateHdfsParam consumes the canonical NameNode before JNI.
+ if (nativeConsumer && "fs.defaultFS".equalsIgnoreCase(entry.getKey())) {
+ continue;
+ }
+ String key = (nativeConsumer ? nativeStringPrefix(entry.getKey()) : entry.getKey()).trim();
+ String value = nativeConsumer ? nativeStringPrefix(entry.getValue()) : entry.getValue();
+ String previous = properties.put(key, value);
+ if (previous != null && !previous.equals(value) && isRoutingKey(key)) {
+ reject();
+ }
+ }
+ }
+
+ /** Reject local/unknown Hadoop TVF routes before listing, deletion, schema discovery or data IO. */
+ public static void check(Access access, String path, Map<String, String> normalized) {
+ if (access != Access.HADOOP_READ && Config.enable_local_tvf) {
+ return;
+ }
+ try {
+ if (access == Access.NATIVE_WRITE) {
+ String nameNode = nativeNameNode(access, normalized);
+ URI output = nativeFileUri(access, path + "doris-tvf-file", nameNode);
+ String parent = output.getPath();
+ parent = parent.substring(0, parent.lastIndexOf('/') + 1);
+ if (parent.length() > 1) {
+ parent = parent.substring(0, parent.length() - 1);
+ }
+ new HadoopTvfAdmission(access, normalized, true).checkNative(nameNode, parent);
+ } else {
+ HadoopTvfAdmission javaConsumer = new HadoopTvfAdmission(access, normalized, false);
+ // Project the Java consumer independently from native convert_path/JNI.
+ URI javaPath = hadoopPathUri(path.trim());
+ javaConsumer.checkRoute(javaPath, javaPath.getPath(), new HashSet<>());
+ if (access == Access.HADOOP_READ) {
+ // FileQueryScanNode sends this filesystem identifier in range.fs_name. BE
+ // falls back to hdfs_params.fs_name only when that range field is empty.
+ URI fileUri = LocationPath.of(path).getPath().toUri();
+ String nameNode = fileUri.getScheme() + "://" + fileUri.getAuthority();
+ if (nameNode.isEmpty()) {
+ nameNode = nativeNameNode(access, normalized);
+ }
+ HadoopTvfAdmission nativeConsumer = new HadoopTvfAdmission(access, normalized, true);
+ nativeConsumer.checkNativeRead(nameNode, path);
+ // Schema RPCs have no range.fs_name; they use THdfsParams.fs_name instead.
+ nativeConsumer.checkNativeRead(nativeNameNode(access, normalized), path);
+ }
+ }
+ } catch (URISyntaxException e) {
+ new HadoopTvfAdmission(access, normalized, false).reject();
+ }
+ }
+
+ private static String nativeNameNode(Access access, Map<String, String> normalized) {
+ String result = null;
+ for (Map.Entry<String, String> entry : normalized.entrySet()) {
+ if ("fs.defaultFS".equalsIgnoreCase(entry.getKey())) {
+ if (result != null && !result.equals(entry.getValue())) {
+ reject(access);
+ }
+ result = entry.getValue();
+ }
+ }
+ return result == null ? "" : result;
+ }
+
+ private void checkNative(String nameNode, String path) throws URISyntaxException {
+ nameNode = selector(nativeStringPrefix(nameNode)).trim();
+ if (nameNode.isEmpty() && access == Access.NATIVE_WRITE) {
+ // BE passes fs_name.c_str(), never NULL. libhdfs prefixes this empty string
+ // with hdfs://, whose URI construction fails before any filesystem is selected.
+ // Preserve existing analysis; execution reports the invalid NameNode as before.
+ return;
+ }
+ if ("default".equals(nameNode) || nameNode.isEmpty()) {
+ reject();
+ }
+ checkRoute(new URI(nameNode.contains("://") ? nameNode : "hdfs://" + nameNode),
+ path, new HashSet<>());
+ }
+
+ private void checkNativeRead(String nameNode, String path) throws URISyntaxException {
+ checkNative(nameNode, nativeFileUri(access, path, nameNode).getPath());
+ }
+
+ private List<URI> defaults() throws URISyntaxException {
+ List<URI> defaults = new ArrayList<>();
+ for (String key : Arrays.asList("fs.defaultFS", "fs.default.name")) {
+ String value = property(key);
+ if (value != null) {
+ value = selector(value).trim();
+ defaults.add(new URI("local".equals(value) ? "file:///"
+ : (uriScheme(value).isEmpty() ? "hdfs://" + value : value)));
Review Comment:
Fixed in 75386c739acf34ecda23c1964d47dcc110457906. Default-filesystem parsing now follows Hadoop FileSystem.fixName: local remains the local alias, while a slash-free host[:port] value is normalized to hdfs://host[:port] before scheme/provider interpretation. Both canonical and legacy default-FS keys are covered.
Before tests reproduced the erroneous rejection. The new release cleanup control reaches a mocked remote filesystem with LOCAL disabled and verifies the expected parent-directory call; it performs no real deletion or NameNode access. Actual installed Hadoop classes confirm canonical/legacy host:port normalization. Local defaults remain rejected. All 65 selected FE tests and Checkstyle pass with fresh XML and unchanged inputs. This does not change storage URI normalization elsewhere; master's separate pre-existing normalization limit is not claimed fixed.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6398586940
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6399730784
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6389267945
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on PR #69528:
URL: https://github.com/apache/doris/pull/69528#issuecomment-6393506658
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](tvf) Secure LOCAL TVF access (4.1) [doris]
Posted by "OIiveirra (via GitHub)" <gi...@apache.org>.
OIiveirra commented on code in PR #69528:
URL: https://github.com/apache/doris/pull/69528#discussion_r4371828806
##########
be/src/format_v2/table/lance_reader.cpp:
##########
@@ -628,6 +887,10 @@ Status LanceTableReader::_ensure_dataset_open(const TFileRangeDesc& range,
} else if (!_opened_dataset_key.has_value() || *_opened_dataset_key != key) {
return Status::InvalidArgument(
"Lance reader cannot mix dataset snapshots or storage options");
+ } else {
+ // Revalidate reused handles before scanners, indexes or row-id reads. Fresh handles
+ // are checked inside _open_dataset before they become reader state.
+ RETURN_IF_ERROR(_validate_dataset_storage(_dataset, key.storage_policy));
Review Comment:
Fixed in 4fadc819b29d2af9da19a23f983e70a3655421e0. Successful REMOTE storage admission is now reused for the same opened handle and complete DatasetKey. The pinned SDK exposes registered bases from the immutable opened snapshot; _open_dataset still validates all bases before publishing the handle. A changed URI, version, storage options or policy still fails the existing key comparison. LOCAL handles continue revalidating paths and registered bases on every reuse because filesystem paths and links can change. No new cache state or SDK interface was added.
All 31 selected Clang21/ASAN filesystem and Lance tests passed with fresh XML and frozen sources. Coverage includes reusing a LOCAL handle after narrowing its secure root around a registered sibling base, initial REMOTE rejection of a local primary before handle publication, snapshot/key checks, multiple fragment splits and row-id materialization. Changed C++ ranges pass clang-format16. No live S3 throughput benchmark is claimed.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org