From f97daf6deb8702c69857b1480a0b6969edf972d6 Mon Sep 17 00:00:00 2001 From: Dylan Date: Fri, 23 Oct 2026 17:31:37 +0800 Subject: [PATCH 1/4] [fix](fe) Disable LOCAL TVF by default ### What problem does this PR solve? Issue Number: N/A Problem Summary: Add the startup-only enable_local_tvf setting, defaulting to false. Reject LOCAL TVF and FILE TVF with local storage before property parsing or backend access. Explicit opt-in restores each branch's existing behavior. Dedicated regression deployments opt in to retain LOCAL coverage. ### Release note LOCAL TVF and FILE TVF with local storage are 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. ### Check List (For Author) - Test: 11 focused FE unit tests passed with zero failures or skips; focused FE Checkstyle passed. SQL regression coverage added; runtime validation recorded separately. - Behavior changed: Yes. Local TVF access requires explicit startup opt-in. - Does this need documentation: Yes. Release note documents the default and opt-in. --- .../java/org/apache/doris/common/Config.java | 4 + .../LocalTableValuedFunction.java | 6 + .../LocalTableValuedFunctionAuthTest.java | 5 + .../LocalTableValuedFunctionEnableTest.java | 151 ++++++++++++++++++ .../pipeline/cloud_p0/conf/fe_custom.conf | 3 + .../pipeline/cloud_p1/conf/fe_custom.conf | 3 + .../pipeline/external/conf/fe.conf | 3 + .../pipeline/nonConcurrent/conf/fe.conf | 3 + regression-test/pipeline/p0/conf/fe.conf | 3 + regression-test/pipeline/p1/conf/fe.conf | 3 + .../pipeline/vault_p0/conf/fe_custom.conf | 3 + .../account_p0/test_local_tvf_enable.groovy | 33 ++++ .../test_local_tvf_privilege.groovy | 4 +- 13 files changed, 223 insertions(+), 1 deletion(-) create mode 100644 fe/fe-core/src/test/java/org/apache/doris/tablefunction/LocalTableValuedFunctionEnableTest.java create mode 100644 regression-test/suites/account_p0/test_local_tvf_enable.groovy diff --git a/fe/fe-common/src/main/java/org/apache/doris/common/Config.java b/fe/fe-common/src/main/java/org/apache/doris/common/Config.java index d032e271112078..a92117c3b39385 100644 --- a/fe/fe-common/src/main/java/org/apache/doris/common/Config.java +++ b/fe/fe-common/src/main/java/org/apache/doris/common/Config.java @@ -2815,6 +2815,10 @@ public static void validateMtmvCacheConfig() throws ConfigException { + "configuration to prevent command injection attacks.") public static boolean enable_java_udf = true; + @ConfField(description = "Whether to enable LOCAL TVF and FILE TVF with local storage. Disabled by default. " + + "Set this in fe.conf and restart every FE to enable it. Existing access checks still apply.") + public static boolean enable_local_tvf = false; + @ConfField(mutable = true, masterOnly = true, description = "When enabled, data can be processed using the " + "globally created Java UDF function during import. " + "The default setting is false.") public static boolean enable_udf_in_load = false; diff --git a/fe/fe-core/src/main/java/org/apache/doris/tablefunction/LocalTableValuedFunction.java b/fe/fe-core/src/main/java/org/apache/doris/tablefunction/LocalTableValuedFunction.java index b462c395b1c3b1..7a906e5e4023fe 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/tablefunction/LocalTableValuedFunction.java +++ b/fe/fe-core/src/main/java/org/apache/doris/tablefunction/LocalTableValuedFunction.java @@ -21,6 +21,7 @@ import org.apache.doris.analysis.StorageBackend.StorageType; import org.apache.doris.catalog.Env; import org.apache.doris.common.AnalysisException; +import org.apache.doris.common.Config; import org.apache.doris.common.ErrorCode; import org.apache.doris.datasource.storage.StorageAdapter; import org.apache.doris.mysql.privilege.PrivPredicate; @@ -63,6 +64,11 @@ public class LocalTableValuedFunction extends ExternalFileTableValuedFunction { private boolean sharedStorage = false; public LocalTableValuedFunction(Map properties) throws AnalysisException { + if (!Config.enable_local_tvf) { + throw new org.apache.doris.nereids.exceptions.AnalysisException( + "LOCAL TVF is disabled. Set enable_local_tvf=true in fe.conf and restart every FE to enable it."); + } + // Check privileges before listing files or discovering their schema. checkAuth(ConnectContext.get()); diff --git a/fe/fe-core/src/test/java/org/apache/doris/tablefunction/LocalTableValuedFunctionAuthTest.java b/fe/fe-core/src/test/java/org/apache/doris/tablefunction/LocalTableValuedFunctionAuthTest.java index c82cf2eebd93f0..cf400e8918660a 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/tablefunction/LocalTableValuedFunctionAuthTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/tablefunction/LocalTableValuedFunctionAuthTest.java @@ -18,6 +18,7 @@ package org.apache.doris.tablefunction; import org.apache.doris.catalog.Env; +import org.apache.doris.common.Config; import org.apache.doris.mysql.privilege.AccessControllerManager; import org.apache.doris.mysql.privilege.PrivPredicate; import org.apache.doris.nereids.exceptions.AnalysisException; @@ -50,12 +51,15 @@ public class LocalTableValuedFunctionAuthTest { private final AccessControllerManager accessManager = Mockito.mock(AccessControllerManager.class); private final SystemInfoService systemInfo = Mockito.mock(SystemInfoService.class); private final BackendServiceProxy proxy = Mockito.mock(BackendServiceProxy.class); + private boolean originalEnableLocalTvf; private MockedStatic mockedEnv; private MockedStatic mockedContext; private MockedStatic mockedProxy; @BeforeEach public void setUp() throws Exception { + originalEnableLocalTvf = Config.enable_local_tvf; + Config.enable_local_tvf = true; mockedEnv = Mockito.mockStatic(Env.class); mockedEnv.when(Env::getCurrentEnv).thenReturn(env); mockedEnv.when(Env::getCurrentSystemInfo).thenReturn(systemInfo); @@ -76,6 +80,7 @@ public void setUp() throws Exception { @AfterEach public void tearDown() { + Config.enable_local_tvf = originalEnableLocalTvf; mockedProxy.close(); mockedContext.close(); mockedEnv.close(); diff --git a/fe/fe-core/src/test/java/org/apache/doris/tablefunction/LocalTableValuedFunctionEnableTest.java b/fe/fe-core/src/test/java/org/apache/doris/tablefunction/LocalTableValuedFunctionEnableTest.java new file mode 100644 index 00000000000000..5d07ed2ac4c635 --- /dev/null +++ b/fe/fe-core/src/test/java/org/apache/doris/tablefunction/LocalTableValuedFunctionEnableTest.java @@ -0,0 +1,151 @@ +// 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.catalog.Env; +import org.apache.doris.common.Config; +import org.apache.doris.mysql.privilege.AccessControllerManager; +import org.apache.doris.mysql.privilege.PrivPredicate; +import org.apache.doris.nereids.exceptions.AnalysisException; +import org.apache.doris.proto.InternalService.PGlobResponse; +import org.apache.doris.proto.Types.PStatus; +import org.apache.doris.qe.ConnectContext; +import org.apache.doris.qe.SessionVariable; +import org.apache.doris.rpc.BackendServiceProxy; +import org.apache.doris.system.Backend; +import org.apache.doris.system.SystemInfoService; +import org.apache.doris.thrift.TNetworkAddress; + +import com.google.common.collect.ImmutableMap; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.MockedConstruction; +import org.mockito.MockedStatic; +import org.mockito.Mockito; + +import java.util.Map; +import java.util.concurrent.CompletableFuture; + +public class LocalTableValuedFunctionEnableTest { + private static final Map PROPERTIES = ImmutableMap.of( + "file_path", "local_privilege_test.csv", "backend_id", "1", "format", "csv"); + + private final Env env = Mockito.mock(Env.class); + private final ConnectContext context = Mockito.mock(ConnectContext.class); + private final AccessControllerManager accessManager = Mockito.mock(AccessControllerManager.class); + private final SystemInfoService systemInfo = Mockito.mock(SystemInfoService.class); + private final BackendServiceProxy proxy = Mockito.mock(BackendServiceProxy.class); + private boolean originalEnableLocalTvf; + private MockedStatic mockedEnv; + private MockedStatic mockedContext; + private MockedStatic mockedProxy; + + @BeforeEach + public void setUp() throws Exception { + originalEnableLocalTvf = Config.enable_local_tvf; + Config.enable_local_tvf = false; + mockedEnv = Mockito.mockStatic(Env.class); + mockedEnv.when(Env::getCurrentEnv).thenReturn(env); + mockedEnv.when(Env::getCurrentSystemInfo).thenReturn(systemInfo); + mockedContext = Mockito.mockStatic(ConnectContext.class); + mockedContext.when(ConnectContext::get).thenReturn(context); + mockedProxy = Mockito.mockStatic(BackendServiceProxy.class); + mockedProxy.when(BackendServiceProxy::getInstance).thenReturn(proxy); + Mockito.when(env.getAccessManager()).thenReturn(accessManager); + Mockito.when(context.getSessionVariable()).thenReturn(new SessionVariable()); + Mockito.when(context.getQueryTimeoutS()).thenReturn(60); + + Backend backend = Mockito.mock(Backend.class); + Mockito.when(systemInfo.getBackend(1L)).thenReturn(backend); + Mockito.when(backend.getBrpcAddress()).thenReturn(new TNetworkAddress("127.0.0.1", 8060)); + Mockito.when(proxy.glob(Mockito.any(), Mockito.any())).thenReturn(CompletableFuture.completedFuture( + PGlobResponse.newBuilder().setStatus(PStatus.newBuilder().setStatusCode(0)).build())); + } + + @AfterEach + public void tearDown() { + Config.enable_local_tvf = originalEnableLocalTvf; + mockedProxy.close(); + mockedContext.close(); + mockedEnv.close(); + } + + @Test + public void testDisabledRejectsLocalBeforePropertyParsing() { + AnalysisException exception = Assertions.assertThrows(AnalysisException.class, + () -> new LocalTableValuedFunction(ImmutableMap.of())); + Assertions.assertTrue(exception.getMessage().contains("enable_local_tvf=true")); + Mockito.verifyNoInteractions(accessManager, systemInfo, proxy); + } + + @Test + public void testDisabledRejectsCsvAndLanceBeforeFileAccess() { + for (String format : new String[] {"csv", "lance"}) { + Map properties = ImmutableMap.of( + "file_path", "local_enable_test.csv", "backend_id", "1", "format", format); + AnalysisException exception = Assertions.assertThrows(AnalysisException.class, + () -> new LocalTableValuedFunction(properties)); + Assertions.assertTrue(exception.getMessage().contains("LOCAL TVF is disabled")); + } + Mockito.verifyNoInteractions(accessManager, systemInfo, proxy); + } + + @Test + public void testEnabledLocalReachesExistingFileAccess() throws Exception { + Config.enable_local_tvf = true; + Mockito.when(accessManager.checkGlobalPriv(context, PrivPredicate.ADMIN)).thenReturn(true); + new LocalTableValuedFunction(PROPERTIES); + Mockito.verify(proxy).glob(Mockito.any(), Mockito.any()); + } + + @Test + public void testDisabledRejectsFileLocalBeforeFileAccess() { + AnalysisException exception = Assertions.assertThrows(AnalysisException.class, + () -> new FileTableValuedFunction(PROPERTIES)); + Assertions.assertTrue(exception.getMessage().contains("LOCAL TVF is disabled")); + Mockito.verifyNoInteractions(accessManager, systemInfo, proxy); + } + + @Test + public void testEnabledFileLocalReachesExistingFileAccess() throws Exception { + Config.enable_local_tvf = true; + Mockito.when(accessManager.checkGlobalPriv(context, PrivPredicate.ADMIN)).thenReturn(true); + new FileTableValuedFunction(PROPERTIES); + Mockito.verify(proxy).glob(Mockito.any(), Mockito.any()); + } + + @Test + public void testDisabledLocalDoesNotBlockFileS3() { + Map properties = ImmutableMap.builder() + .put("uri", "s3://test-bucket/local_enable_test.csv") + .put("s3.endpoint", "s3.us-east-1.amazonaws.com") + .put("s3.region", "us-east-1") + .put("s3.access_key", "test-access-key") + .put("s3.secret_key", "test-secret-key") + .put("format", "csv") + .build(); + try (MockedConstruction mockedS3 = + Mockito.mockConstruction(S3TableValuedFunction.class)) { + Assertions.assertDoesNotThrow(() -> new FileTableValuedFunction(properties)); + Assertions.assertEquals(1, mockedS3.constructed().size()); + } + Mockito.verifyNoInteractions(accessManager, systemInfo, proxy); + } +} diff --git a/regression-test/pipeline/cloud_p0/conf/fe_custom.conf b/regression-test/pipeline/cloud_p0/conf/fe_custom.conf index 68012788a9eb87..4f34a255a0ca6b 100644 --- a/regression-test/pipeline/cloud_p0/conf/fe_custom.conf +++ b/regression-test/pipeline/cloud_p0/conf/fe_custom.conf @@ -1,3 +1,6 @@ +# Exercise LOCAL TVF explicitly; production defaults to disabled. +enable_local_tvf=true + stream_load_default_timeout_second = 72000 replication_num_forced_in_cloud_mode = true ignore_unsupported_properties_in_cloud_mode = true diff --git a/regression-test/pipeline/cloud_p1/conf/fe_custom.conf b/regression-test/pipeline/cloud_p1/conf/fe_custom.conf index 157f3a07a09b47..e671d0a7dd15b2 100644 --- a/regression-test/pipeline/cloud_p1/conf/fe_custom.conf +++ b/regression-test/pipeline/cloud_p1/conf/fe_custom.conf @@ -1,3 +1,6 @@ +# Exercise LOCAL TVF explicitly; production defaults to disabled. +enable_local_tvf=true + stream_load_default_timeout_second = 72000 replication_num_forced_in_cloud_mode = true ignore_unsupported_properties_in_cloud_mode = true diff --git a/regression-test/pipeline/external/conf/fe.conf b/regression-test/pipeline/external/conf/fe.conf index 34fa5a440a527e..88118ec677cdf6 100644 --- a/regression-test/pipeline/external/conf/fe.conf +++ b/regression-test/pipeline/external/conf/fe.conf @@ -1,3 +1,6 @@ +# Exercise LOCAL TVF explicitly; production defaults to disabled. +enable_local_tvf=true + # 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 diff --git a/regression-test/pipeline/nonConcurrent/conf/fe.conf b/regression-test/pipeline/nonConcurrent/conf/fe.conf index d283b2a15cfa6a..de57276c9bcd79 100644 --- a/regression-test/pipeline/nonConcurrent/conf/fe.conf +++ b/regression-test/pipeline/nonConcurrent/conf/fe.conf @@ -1,3 +1,6 @@ +# Exercise LOCAL TVF explicitly; production defaults to disabled. +enable_local_tvf=true + # 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 diff --git a/regression-test/pipeline/p0/conf/fe.conf b/regression-test/pipeline/p0/conf/fe.conf index 61c6a5a49621bb..4b73152ba7bf7c 100644 --- a/regression-test/pipeline/p0/conf/fe.conf +++ b/regression-test/pipeline/p0/conf/fe.conf @@ -1,3 +1,6 @@ +# Exercise LOCAL TVF explicitly; production defaults to disabled. +enable_local_tvf=true + # 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 diff --git a/regression-test/pipeline/p1/conf/fe.conf b/regression-test/pipeline/p1/conf/fe.conf index be302d2908f7e6..f2932470f237e1 100644 --- a/regression-test/pipeline/p1/conf/fe.conf +++ b/regression-test/pipeline/p1/conf/fe.conf @@ -1,3 +1,6 @@ +# Exercise LOCAL TVF explicitly; production defaults to disabled. +enable_local_tvf=true + # 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 diff --git a/regression-test/pipeline/vault_p0/conf/fe_custom.conf b/regression-test/pipeline/vault_p0/conf/fe_custom.conf index 9b622795168540..88cff4dd920ec5 100644 --- a/regression-test/pipeline/vault_p0/conf/fe_custom.conf +++ b/regression-test/pipeline/vault_p0/conf/fe_custom.conf @@ -1,3 +1,6 @@ +# Exercise LOCAL TVF explicitly; production defaults to disabled. +enable_local_tvf=true + stream_load_default_timeout_second = 72000 replication_num_forced_in_cloud_mode = true ignore_unsupported_properties_in_cloud_mode = true diff --git a/regression-test/suites/account_p0/test_local_tvf_enable.groovy b/regression-test/suites/account_p0/test_local_tvf_enable.groovy new file mode 100644 index 00000000000000..b6853b51ebd5cb --- /dev/null +++ b/regression-test/suites/account_p0/test_local_tvf_enable.groovy @@ -0,0 +1,33 @@ +// 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. + +suite("test_local_tvf_enable", "p0") { + def enabled = (sql "ADMIN SHOW FRONTEND CONFIG LIKE 'enable_local_tvf'")[0][1].toBoolean() + for (def functionName : ["LOCAL", "FILE"]) { + for (def prefix : ["", "EXPLAIN "]) { + test { + sql """${prefix}SELECT * FROM ${functionName}( + "file_path" = "local_enable_test.csv", + "backend_id" = "-1", + "format" = "csv" + )""" + // With opt-in enabled, the existing backend validation must be reached. + exception (enabled ? "'backend_id' is required" : "LOCAL TVF is disabled") + } + } + } +} diff --git a/regression-test/suites/account_p0/test_local_tvf_privilege.groovy b/regression-test/suites/account_p0/test_local_tvf_privilege.groovy index 4dfd4da49c85e4..2f262b3aa17a14 100644 --- a/regression-test/suites/account_p0/test_local_tvf_privilege.groovy +++ b/regression-test/suites/account_p0/test_local_tvf_privilege.groovy @@ -16,6 +16,8 @@ // under the License. suite("test_local_tvf_privilege", "p0") { + def enabled = (sql "ADMIN SHOW FRONTEND CONFIG LIKE 'enable_local_tvf'")[0][1].toBoolean() + def expectedError = enabled ? "ADMIN/OPERATOR" : "LOCAL TVF is disabled" def user = "local_tvf_privilege_user" sql "DROP USER IF EXISTS '${user}'" sql "CREATE USER '${user}' IDENTIFIED BY 'LocalTvf123!'" @@ -36,7 +38,7 @@ suite("test_local_tvf_privilege", "p0") { "backend_id" = "0", "format" = "csv" )""" - exception "ADMIN/OPERATOR" + exception expectedError } } } From 08d01059ac4d99aa9e5a43e825b656bf687ce9e7 Mon Sep 17 00:00:00 2001 From: Dylan Date: Fri, 23 Oct 2026 18:54:50 +0800 Subject: [PATCH 2/4] [fix](tvf) Enforce canonical secure paths for LOCAL reads ### What problem does this PR solve? Issue Number: N/A Related PR: N/A Problem Summary: Glob expansion and symlinks can produce canonical paths outside the configured secure directory. Resolve a nonempty secure root and each local result canonically before checking containment. The shared resolver rejects remote schemes and embedded NULs. Keep the default-disabled startup gate and existing local authorization. Bind the related glob test fixture to its own secure root across build profiles. ### Release note Enabled LOCAL reads remain restricted to user_files_secure_path after glob expansion and symlink resolution. ### Check List (For Author) - Test: 5 focused BE tests passed with zero failures/skips; changed-line clang-format 16 and build hygiene passed. BE tests used the documented private legacy-glibc link adapter with branch/toolchain inputs verified. FE source is unchanged by this supplement; its existing focused tests passed previously. - Behavior changed: Yes, enforce canonical secure-directory containment after explicit startup opt-in. - Does this need documentation: Covered by the PR release note. --- be/src/io/fs/local_file_system.cpp | 33 +++++++-- be/src/io/fs/local_file_system.h | 2 + be/test/io/fs/local_file_system_test.cpp | 90 ++++++++++++++++++++++++ 3 files changed, 118 insertions(+), 7 deletions(-) diff --git a/be/src/io/fs/local_file_system.cpp b/be/src/io/fs/local_file_system.cpp index 421fb71bbbdd51..5c21e0a6d02331 100644 --- a/be/src/io/fs/local_file_system.cpp +++ b/be/src/io/fs/local_file_system.cpp @@ -471,14 +471,26 @@ const std::shared_ptr& global_local_filesystem() { Status LocalFileSystem::canonicalize_local_file(const std::string& dir, const std::string& file_path, std::string* full_path) { - const std::string absolute_path = dir + "/" + file_path; + // An empty root would turn a relative request into an unrestricted absolute path. + if (dir.empty()) { + return Status::InvalidArgument("user_files_secure_path must not be empty"); + } + // Reject embedded NULs before passing paths to filesystem APIs that use C strings. + if (file_path.empty() || file_path.find('\0') != std::string::npos) { + return Status::InvalidArgument("local file path must not be empty or contain NUL"); + } + Path local_path; + // Convert file: URIs to filesystem paths and reject remote schemes before any reader opens them. + RETURN_IF_ERROR(convert_to_abs_path(file_path, local_path)); + std::string secure_path; + RETURN_IF_ERROR(canonicalize(dir, &secure_path)); std::string canonical_path; - RETURN_IF_ERROR(canonicalize(absolute_path, &canonical_path)); - if (!contain_path(dir, canonical_path)) { + RETURN_IF_ERROR(canonicalize(Path(secure_path) / local_path, &canonical_path)); + // Canonicalize both sides: a symlink inside the root may point to a sibling directory. + if (!contain_path(secure_path, canonical_path)) { return Status::InvalidArgument("file path is not allowed: {}", canonical_path); } - - *full_path = canonical_path; + *full_path = std::move(canonical_path); return Status::OK(); } @@ -486,13 +498,20 @@ Status LocalFileSystem::safe_glob(const std::string& path, std::vector if (path.find("..") != std::string::npos) { return Status::InvalidArgument("can not contain '..' in path"); } - std::string full_path = config::user_files_secure_path + "/" + path; + // An empty root would turn the prefix below into "/" and allow any local file. + if (config::user_files_secure_path.empty()) { + return Status::InvalidArgument("user_files_secure_path must not be empty"); + } + std::string secure_path; + RETURN_IF_ERROR(canonicalize(config::user_files_secure_path, &secure_path)); + std::string full_path = secure_path + "/" + path; std::vector files; RETURN_IF_ERROR(_glob(full_path, &files)); for (auto& file : files) { FileInfo fi; fi.is_file = true; - RETURN_IF_ERROR(canonicalize_local_file("", file, &(fi.file_name))); + // Glob classes and symlinks can escape without a literal ".." in the pattern. + RETURN_IF_ERROR(canonicalize_local_file(secure_path, file, &(fi.file_name))); RETURN_IF_ERROR(file_size_impl(fi.file_name, &(fi.file_size))); res->push_back(std::move(fi)); } diff --git a/be/src/io/fs/local_file_system.h b/be/src/io/fs/local_file_system.h index b381e960a7bdba..1c23612f8e282c 100644 --- a/be/src/io/fs/local_file_system.h +++ b/be/src/io/fs/local_file_system.h @@ -75,6 +75,8 @@ class LocalFileSystem final : public FileSystem { static std::filesystem::perms PERMS_OWNER_RW; + // Resolve a local path relative to a nonempty secure root, then check canonical containment. + // Absolute paths and file: URIs must resolve inside the same root; remote schemes are rejected. Status canonicalize_local_file(const std::string& dir, const std::string& file_path, std::string* full_path); diff --git a/be/test/io/fs/local_file_system_test.cpp b/be/test/io/fs/local_file_system_test.cpp index a3b5549f98e893..cbf694f02da0b8 100644 --- a/be/test/io/fs/local_file_system_test.cpp +++ b/be/test/io/fs/local_file_system_test.cpp @@ -31,12 +31,14 @@ #include #include +#include "common/config.h" #include "common/status.h" #include "cpp/sync_point.h" #include "gtest/gtest_pred_impl.h" #include "io/fs/file_reader.h" #include "io/fs/file_writer.h" #include "io/fs/local_file_writer.h" +#include "util/defer_op.h" #include "util/slice.h" namespace doris { @@ -568,6 +570,10 @@ TEST_F(LocalFileSystemTest, AbnormalWriteRead) { } TEST_F(LocalFileSystemTest, TestGlob) { + const std::string original_secure_path = config::user_files_secure_path; + DEFER(config::user_files_secure_path = original_secure_path); + // Bind this fixture to its own files, independent of the runner's build-profile directory. + config::user_files_secure_path = std::filesystem::absolute("./be/ut_build_ASAN/test").string(); std::string path = "./be/ut_build_ASAN/test/file_path/"; EXPECT_TRUE(io::global_local_filesystem()->delete_directory(path).ok()); EXPECT_TRUE(io::global_local_filesystem() @@ -599,6 +605,90 @@ TEST_F(LocalFileSystemTest, TestGlob) { EXPECT_TRUE(io::global_local_filesystem()->delete_directory(path).ok()); } +TEST_F(LocalFileSystemTest, TestSafeGlobPathContainment) { + auto fs = io::global_local_filesystem(); + const std::string original_secure_path = config::user_files_secure_path; + DEFER(config::user_files_secure_path = original_secure_path); + const auto root = std::filesystem::canonical(test_dir); + const auto secure = root / "secure"; + const auto outside = root / "secure-other"; + std::filesystem::create_directories(secure / "nested"); + std::filesystem::create_directory(outside); + ASSERT_TRUE(save_string_file((secure / "nested/inside.csv").string(), "inside").ok()); + ASSERT_TRUE(save_string_file((outside / "outside.csv").string(), "outside").ok()); + std::filesystem::create_directory_symlink(outside, secure / "escape"); + std::filesystem::create_symlink(outside / "outside.csv", secure / "outside.csv"); + std::filesystem::create_directory_symlink(secure / "nested", secure / "inside"); + config::user_files_secure_path = secure.string(); + + for (const auto* pattern : {".[.]/secure-other/outside.csv", ".?/secure-other/outside.csv", + ".[.-.]/secure-other/outside.csv", "escape/*.csv", "outside.csv"}) { + std::vector files; + const auto status = fs->safe_glob(pattern, &files); + EXPECT_FALSE(status.ok()) << pattern; + EXPECT_NE(status.to_string().find("file path is not allowed"), std::string::npos) << status; + } + + for (const auto* pattern : {"nested/*.csv", "inside/*.csv"}) { + std::vector files; + ASSERT_TRUE(fs->safe_glob(pattern, &files).ok()) << pattern; + ASSERT_EQ(1, files.size()); + EXPECT_EQ((secure / "nested/inside.csv").string(), files[0].file_name); + EXPECT_EQ(6, files[0].file_size); + } +} + +TEST_F(LocalFileSystemTest, TestSafeGlobSecureRoot) { + auto fs = io::global_local_filesystem(); + const std::string original_secure_path = config::user_files_secure_path; + DEFER(config::user_files_secure_path = original_secure_path); + const auto root = std::filesystem::canonical(test_dir); + std::filesystem::create_directory(root / "secure"); + std::filesystem::create_directory_symlink(root / "secure", root / "secure-link"); + ASSERT_TRUE(save_string_file((root / "secure/inside.csv").string(), "inside").ok()); + + for (const auto& secure : {std::string(test_dir) + "/secure", (root / "secure/").string(), + (root / "secure-link").string()}) { + config::user_files_secure_path = secure; + std::vector files; + ASSERT_TRUE(fs->safe_glob("*.csv", &files).ok()) << secure; + ASSERT_EQ(1, files.size()); + EXPECT_EQ((root / "secure/inside.csv").string(), files[0].file_name); + } + config::user_files_secure_path = ""; + std::vector files; + EXPECT_FALSE(fs->safe_glob("*.csv", &files).ok()); + config::user_files_secure_path = (root / "missing").string(); + EXPECT_FALSE(fs->safe_glob("*.csv", &files).ok()); +} + +TEST_F(LocalFileSystemTest, TestCanonicalizeSecureLocalPath) { + auto fs = io::global_local_filesystem(); + const auto root = std::filesystem::canonical(test_dir); + const auto secure = root / "secure"; + std::filesystem::create_directories(secure / "nested"); + std::filesystem::create_directory(root / "secure-other"); + std::filesystem::create_directory_symlink(root / "secure-other", secure / "escape"); + std::filesystem::create_directory_symlink(secure / "nested", secure / "inside"); + for (const auto& path : + {std::string("nested"), std::string("nested/../inside"), (secure / "nested").string(), + "file://" + (secure / "nested").string()}) { + std::string resolved; + ASSERT_TRUE(fs->canonicalize_local_file(secure.string(), path, &resolved).ok()) << path; + EXPECT_EQ((secure / "nested").string(), resolved); + } + for (const auto& path : + {std::string("../secure-other"), std::string("escape"), (root / "secure-other").string(), + std::string("https://invalid/dataset"), std::string("s3://bucket/dataset"), + std::string("http://invalid/dataset"), std::string(""), + std::string("nested\0ignored", 14)}) { + std::string resolved; + EXPECT_FALSE(fs->canonicalize_local_file(secure.string(), path, &resolved).ok()) << path; + } + std::string resolved; + EXPECT_FALSE(fs->canonicalize_local_file("", secure.string(), &resolved).ok()); +} + TEST_F(LocalFileSystemTest, TestConvertToAbsPath) { io::Path abs_path; Status st; From d353b6d45bac9dd12a1b388d13d4f19e1e694994 Mon Sep 17 00:00:00 2001 From: Dylan Date: Fri, 23 Oct 2026 19:18:51 +0800 Subject: [PATCH 3/4] [fix](tvf) Redact resolved names from local path errors ### What problem does this PR solve? Issue Number: N/A Related PR: N/A Problem Summary: A rejected glob result could disclose its expanded filename through a canonical-path error or a failed resolution of a dangling symlink. Return one generic target-validation error for both cases before the status reaches SQL clients. Extend temporary-directory tests to assert that neither real target names nor paths appear in errors. ### Release note LOCAL path validation errors no longer reveal expanded or resolved target filenames. ### Check List (For Author) - Test: 5 focused BE tests passed with zero failures/skips, including external wildcard, dangling symlink and error redaction assertions. Modified lines pass clang-format 16; independent Standards and Spec review found no remaining issue in this supplement. - Behavior changed: Yes, target-validation errors use a generic message. - Does this need documentation: Covered by the PR release note. --- be/src/io/fs/local_file_system.cpp | 9 +++++---- be/test/io/fs/local_file_system_test.cpp | 15 +++++++++++++-- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/be/src/io/fs/local_file_system.cpp b/be/src/io/fs/local_file_system.cpp index 5c21e0a6d02331..4afc32cf77a2ea 100644 --- a/be/src/io/fs/local_file_system.cpp +++ b/be/src/io/fs/local_file_system.cpp @@ -485,10 +485,11 @@ Status LocalFileSystem::canonicalize_local_file(const std::string& dir, std::string secure_path; RETURN_IF_ERROR(canonicalize(dir, &secure_path)); std::string canonical_path; - RETURN_IF_ERROR(canonicalize(Path(secure_path) / local_path, &canonical_path)); - // Canonicalize both sides: a symlink inside the root may point to a sibling directory. - if (!contain_path(secure_path, canonical_path)) { - return Status::InvalidArgument("file path is not allowed: {}", canonical_path); + const auto status = canonicalize(Path(secure_path) / local_path, &canonical_path); + // A glob result can reveal external names even when resolution fails on a dangling symlink. + // Reject both failed resolution and out-of-root targets without exposing expanded names. + if (!status.ok() || !contain_path(secure_path, canonical_path)) { + return Status::InvalidArgument("file path is not allowed"); } *full_path = std::move(canonical_path); return Status::OK(); diff --git a/be/test/io/fs/local_file_system_test.cpp b/be/test/io/fs/local_file_system_test.cpp index cbf694f02da0b8..45fc6695162490 100644 --- a/be/test/io/fs/local_file_system_test.cpp +++ b/be/test/io/fs/local_file_system_test.cpp @@ -618,15 +618,24 @@ TEST_F(LocalFileSystemTest, TestSafeGlobPathContainment) { ASSERT_TRUE(save_string_file((outside / "outside.csv").string(), "outside").ok()); std::filesystem::create_directory_symlink(outside, secure / "escape"); std::filesystem::create_symlink(outside / "outside.csv", secure / "outside.csv"); + std::filesystem::create_symlink(outside / "missing.csv", outside / "broken.csv"); std::filesystem::create_directory_symlink(secure / "nested", secure / "inside"); config::user_files_secure_path = secure.string(); for (const auto* pattern : {".[.]/secure-other/outside.csv", ".?/secure-other/outside.csv", - ".[.-.]/secure-other/outside.csv", "escape/*.csv", "outside.csv"}) { + ".[.-.]/secure-other/outside.csv", "escape/*.csv", "outside.csv", + ".[.]/secure-other/*.csv", "escape/broken*.csv"}) { std::vector files; const auto status = fs->safe_glob(pattern, &files); EXPECT_FALSE(status.ok()) << pattern; EXPECT_NE(status.to_string().find("file path is not allowed"), std::string::npos) << status; + // The pattern need not name the matched file, so errors must not reveal that name. + const auto message = status.to_string(); + EXPECT_EQ(std::string::npos, message.find(root.string())) << message; + for (const auto* name : {"outside.csv", "broken.csv", "missing.csv"}) { + EXPECT_EQ(std::string::npos, message.find(name)) << message; + } + EXPECT_TRUE(files.empty()); } for (const auto* pattern : {"nested/*.csv", "inside/*.csv"}) { @@ -683,7 +692,9 @@ TEST_F(LocalFileSystemTest, TestCanonicalizeSecureLocalPath) { std::string("http://invalid/dataset"), std::string(""), std::string("nested\0ignored", 14)}) { std::string resolved; - EXPECT_FALSE(fs->canonicalize_local_file(secure.string(), path, &resolved).ok()) << path; + const auto status = fs->canonicalize_local_file(secure.string(), path, &resolved); + EXPECT_FALSE(status.ok()) << path; + EXPECT_EQ(std::string::npos, status.to_string().find(root.string())) << status; } std::string resolved; EXPECT_FALSE(fs->canonicalize_local_file("", secure.string(), &resolved).ok()); From 545e69a9c7a0cfda377caacb24b13e96e0def606 Mon Sep 17 00:00:00 2001 From: Dylan Date: Sat, 24 Oct 2026 11:56:22 +0800 Subject: [PATCH 4/4] [fix](tvf) Close LOCAL admission and path review gaps ### What problem does this PR solve? Issue Number: N/A Related PR: #69526 Problem Summary: Apply the startup-only LOCAL gate to INSERT and EXPLAIN INSERT. Escape the canonical secure root as a literal glob prefix. ### Release note LOCAL TVF INSERT and EXPLAIN INSERT honor enable_local_tvf. Secure directories containing glob metacharacters remain usable. ### Check List (For Author) - Test: Unit Test: 15 FE and 4 BE selected tests passed; relevant Java Checkstyle, clang-format 16, and git diff --check passed. Regression assertions added, full Groovy not run. - Behavior changed: Yes, enforce the existing LOCAL startup setting at the write entry and preserve valid local path identity. - Does this need documentation: Yes, the original PR release note describes the startup opt-in requirement; documentation PR remains pending. --- be/src/io/fs/local_file_system.cpp | 11 +++- be/test/io/fs/local_file_system_test.cpp | 11 ++++ .../nereids/rules/analysis/BindSink.java | 6 ++ .../LocalTableValuedFunction.java | 13 +++-- .../rules/analysis/BindTVFTableSinkTest.java | 58 +++++++++++++++++++ .../account_p0/test_local_tvf_enable.groovy | 13 +++++ 6 files changed, 107 insertions(+), 5 deletions(-) diff --git a/be/src/io/fs/local_file_system.cpp b/be/src/io/fs/local_file_system.cpp index 4afc32cf77a2ea..6fad144d195b11 100644 --- a/be/src/io/fs/local_file_system.cpp +++ b/be/src/io/fs/local_file_system.cpp @@ -505,7 +505,16 @@ Status LocalFileSystem::safe_glob(const std::string& path, std::vector } std::string secure_path; RETURN_IF_ERROR(canonicalize(config::user_files_secure_path, &secure_path)); - std::string full_path = secure_path + "/" + path; + // A symlink root may canonicalize to a directory such as "user_files[prod]". + // The root is literal; only the user-supplied suffix may contain glob operators. + std::string full_path; + for (char c : secure_path) { + if (c == '\\' || c == '*' || c == '?' || c == '[' || c == ']') { + full_path += '\\'; + } + full_path += c; + } + full_path += "/" + path; std::vector files; RETURN_IF_ERROR(_glob(full_path, &files)); for (auto& file : files) { diff --git a/be/test/io/fs/local_file_system_test.cpp b/be/test/io/fs/local_file_system_test.cpp index 45fc6695162490..a10ea7f0f9492d 100644 --- a/be/test/io/fs/local_file_system_test.cpp +++ b/be/test/io/fs/local_file_system_test.cpp @@ -664,6 +664,17 @@ TEST_F(LocalFileSystemTest, TestSafeGlobSecureRoot) { ASSERT_EQ(1, files.size()); EXPECT_EQ((root / "secure/inside.csv").string(), files[0].file_name); } + const auto special_root = root / "secure[prod]*?\\root"; + std::filesystem::create_directory(special_root); + std::filesystem::create_directory_symlink(special_root, root / "special-root-link"); + ASSERT_TRUE(save_string_file((special_root / "inside.csv").string(), "inside").ok()); + for (const auto& secure : {special_root, root / "special-root-link"}) { + config::user_files_secure_path = secure.string(); + std::vector matches; + ASSERT_TRUE(fs->safe_glob("*.csv", &matches).ok()) << secure; + ASSERT_EQ(1, matches.size()); + EXPECT_EQ((special_root / "inside.csv").string(), matches[0].file_name); + } config::user_files_secure_path = ""; std::vector files; EXPECT_FALSE(fs->safe_glob("*.csv", &files).ok()); diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindSink.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindSink.java index 4f4ece3d15e23b..6cafa3ce3f15e0 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindSink.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindSink.java @@ -104,6 +104,7 @@ import org.apache.doris.qe.AutoCloseSessionVariable; import org.apache.doris.qe.ConnectContext; import org.apache.doris.qe.SessionVariable; +import org.apache.doris.tablefunction.LocalTableValuedFunction; import org.apache.doris.thrift.TPartialUpdateNewRowPolicy; import com.google.common.annotations.VisibleForTesting; @@ -701,6 +702,11 @@ private Plan bindTVFTableSink(MatchingContext> ctx) { "INSERT INTO TVF only supports local/s3/hdfs, but got: " + tvfName); } + // INSERT/EXPLAIN INSERT bind a sink without constructing the LOCAL read TVF. + if (tvfName.equals("local")) { + LocalTableValuedFunction.checkEnabled(); + } + // Validate required properties if (!properties.containsKey("file_path")) { throw new AnalysisException("TVF sink requires 'file_path' property"); diff --git a/fe/fe-core/src/main/java/org/apache/doris/tablefunction/LocalTableValuedFunction.java b/fe/fe-core/src/main/java/org/apache/doris/tablefunction/LocalTableValuedFunction.java index 7a906e5e4023fe..2c7fab00d4df6b 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/tablefunction/LocalTableValuedFunction.java +++ b/fe/fe-core/src/main/java/org/apache/doris/tablefunction/LocalTableValuedFunction.java @@ -64,10 +64,7 @@ public class LocalTableValuedFunction extends ExternalFileTableValuedFunction { private boolean sharedStorage = false; public LocalTableValuedFunction(Map properties) throws AnalysisException { - if (!Config.enable_local_tvf) { - throw new org.apache.doris.nereids.exceptions.AnalysisException( - "LOCAL TVF is disabled. Set enable_local_tvf=true in fe.conf and restart every FE to enable it."); - } + checkEnabled(); // Check privileges before listing files or discovering their schema. checkAuth(ConnectContext.get()); @@ -143,6 +140,14 @@ private void getFileListFromBackend() throws AnalysisException { } } + /** Check read and write admission before properties or backend access. */ + public static void checkEnabled() { + if (!Config.enable_local_tvf) { + throw new org.apache.doris.nereids.exceptions.AnalysisException( + "LOCAL TVF is disabled. Set enable_local_tvf=true in fe.conf and restart every FE to enable it."); + } + } + @Override public void checkAuth(ConnectContext ctx) { if (!Env.getCurrentEnv().getAccessManager().checkGlobalPriv(ctx, PrivPredicate.ADMIN) diff --git a/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/analysis/BindTVFTableSinkTest.java b/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/analysis/BindTVFTableSinkTest.java index b76ba57ddb5303..b789c4c391fd22 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/analysis/BindTVFTableSinkTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/analysis/BindTVFTableSinkTest.java @@ -17,25 +17,83 @@ package org.apache.doris.nereids.rules.analysis; +import org.apache.doris.common.Config; +import org.apache.doris.nereids.exceptions.AnalysisException; import org.apache.doris.nereids.parser.NereidsParser; import org.apache.doris.nereids.trees.expressions.NamedExpression; import org.apache.doris.nereids.trees.plans.Plan; +import org.apache.doris.nereids.trees.plans.commands.ExplainCommand; import org.apache.doris.nereids.trees.plans.commands.insert.InsertIntoTVFCommand; +import org.apache.doris.nereids.trees.plans.logical.LogicalPlan; import org.apache.doris.nereids.trees.plans.logical.LogicalProject; import org.apache.doris.nereids.trees.plans.logical.LogicalTVFTableSink; import org.apache.doris.nereids.util.MemoTestUtils; import org.apache.doris.nereids.util.PlanChecker; import org.apache.doris.qe.ConnectContext; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import java.util.Arrays; import java.util.List; class BindTVFTableSinkTest { + private boolean originalEnableLocalTvf; + + @BeforeEach + void setUp() { + originalEnableLocalTvf = Config.enable_local_tvf; + Config.enable_local_tvf = false; + } + + @AfterEach + void tearDown() { + Config.enable_local_tvf = originalEnableLocalTvf; + } + + @Test + void disabledLocalSinkRejectsInsertAndExplainBeforeProperties() { + for (String prefix : Arrays.asList("", "EXPLAIN ")) { + for (String properties : Arrays.asList("'format'='csv'", + "'file_path'='/tmp/local_gate_', 'backend_id'='1', 'format'='csv'")) { + AnalysisException exception = Assertions.assertThrows(AnalysisException.class, + () -> analyzeInsert(prefix + "INSERT INTO local(" + properties + ") SELECT 1")); + Assertions.assertTrue(exception.getMessage().contains("LOCAL TVF is disabled")); + } + } + } + + @Test + void enabledLocalSinkKeepsPropertyValidation() { + Config.enable_local_tvf = true; + AnalysisException exception = Assertions.assertThrows(AnalysisException.class, + () -> analyzeInsert("INSERT INTO local('format'='csv') SELECT 1")); + Assertions.assertTrue(exception.getMessage().contains("requires 'file_path'")); + } + + @Test + void disabledLocalDoesNotBlockRemoteSinks() { + for (String name : Arrays.asList("s3", "hdfs")) { + Assertions.assertInstanceOf(LogicalTVFTableSink.class, + analyzeInsert("INSERT INTO " + name + "('file_path'='remote/data', 'format'='csv') SELECT 1")); + } + } + + private Plan analyzeInsert(String sql) { + ConnectContext context = MemoTestUtils.createConnectContext(); + LogicalPlan parsed = new NereidsParser().parseSingle(sql); + if (parsed instanceof ExplainCommand) { + parsed = ((ExplainCommand) parsed).getLogicalPlan(); + } + InsertIntoTVFCommand command = (InsertIntoTVFCommand) parsed; + return PlanChecker.from(context).analyze(command.getExplainPlan(context)).getPlan(); + } @Test void duplicateOutputNamesKeepDistinctPositions() { + Config.enable_local_tvf = true; ConnectContext context = MemoTestUtils.createConnectContext(); InsertIntoTVFCommand command = (InsertIntoTVFCommand) new NereidsParser().parseSingle( "INSERT INTO local(" diff --git a/regression-test/suites/account_p0/test_local_tvf_enable.groovy b/regression-test/suites/account_p0/test_local_tvf_enable.groovy index b6853b51ebd5cb..3cc8bf71ce3d92 100644 --- a/regression-test/suites/account_p0/test_local_tvf_enable.groovy +++ b/regression-test/suites/account_p0/test_local_tvf_enable.groovy @@ -30,4 +30,17 @@ suite("test_local_tvf_enable", "p0") { } } } + if (!enabled) { + for (def prefix : ["", "EXPLAIN "]) { + test { + sql """${prefix}INSERT INTO LOCAL( + "file_path" = "/tmp/local_enable_test_", + "backend_id" = "-1", + "format" = "csv" + ) SELECT 1""" + exception "LOCAL TVF is disabled" + } + } + } + }