From 069d0b440293633693df35fa255888c1629ff6f0 Mon Sep 17 00:00:00 2001 From: Dylan Date: Fri, 23 Oct 2026 17:27:53 +0800 Subject: [PATCH 1/3] [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 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 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: 3 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 + .../LocalTableValuedFunctionEnableTest.java | 116 ++++++++++++++++++ .../pipeline/external/conf/fe.conf | 3 + regression-test/pipeline/p0/conf/fe.conf | 3 + .../account_p0/test_local_tvf_enable.groovy | 33 +++++ 6 files changed, 165 insertions(+) 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 be15741dcc8f33..42e89cee72f0f8 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 @@ -2619,6 +2619,10 @@ public class Config extends ConfigBase { }) public static boolean enable_java_udf = true; + @ConfField(description = "Whether to enable LOCAL TVF. 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(description = { "是否忽略 Image 文件中未知的模块。如果为 true，不在 PersistMetaModules.MODULE_NAMES 中的元数据模块将被忽略并跳过。" + "默认为 false，如果 Image 文件中包含未知的模块，Doris 将会抛出异常。" 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 df1bf6b61b2d91..4a22b75ae64e66 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.proto.InternalService; import org.apache.doris.proto.InternalService.PGlobResponse; import org.apache.doris.proto.InternalService.PGlobResponse.PFileInfo; @@ -65,6 +66,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."); + } + // 1. analyze common properties Map otherProps = super.parseCommonProperties(properties); 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..c1e7a73bb6713e --- /dev/null +++ b/fe/fe-core/src/test/java/org/apache/doris/tablefunction/LocalTableValuedFunctionEnableTest.java @@ -0,0 +1,116 @@ +// 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.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()); + } +} diff --git a/regression-test/pipeline/external/conf/fe.conf b/regression-test/pipeline/external/conf/fe.conf index 0a4966d3a66134..4af5b26ef0afd7 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/p0/conf/fe.conf b/regression-test/pipeline/p0/conf/fe.conf index 43e2ee9155e67f..88d69566677e1a 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/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..d0fd04295ab5b8 --- /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"]) { + 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") + } + } + } +} From 9ee7ac5828c76c49b56a0f3ea556f53bbebb2e0a Mon Sep 17 00:00:00 2001 From: Dylan Date: Fri, 23 Oct 2026 18:52:22 +0800 Subject: [PATCH 2/3] [fix](tvf) Enforce secure paths for enabled LOCAL reads ### What problem does this PR solve? Issue Number: N/A Related PR: #69526 Problem Summary: Canonical glob results were not checked against the secure root. Validate canonical local paths against a nonempty canonical secure root, reject remote schemes in the shared local resolver, and enforce ADMIN/OPERATOR before local file access and during privilege checking. Keep the existing default-disabled startup gate. Give the existing glob fixture an explicit secure root so profile-specific test directories do not change its assumptions. ### Release note Enabled LOCAL reads remain restricted to user_files_secure_path and require ADMIN/OPERATOR privileges. ### Check List (For Author) - Test: 6 focused FE tests and 4 focused BE tests passed with zero failures/skips; Java Checkstyle and changed-line clang-format 16 passed. BE tests used the supported Clang16 ASAN profile. - Behavior changed: Yes, canonical secure-root containment and local authorization after explicit 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 +++++++++++++++++++ .../LocalTableValuedFunction.java | 16 ++++ .../LocalTableValuedFunctionEnableTest.java | 26 ++++++ 5 files changed, 160 insertions(+), 7 deletions(-) diff --git a/be/src/io/fs/local_file_system.cpp b/be/src/io/fs/local_file_system.cpp index 80d70abcd0e176..44bfc285717b1d 100644 --- a/be/src/io/fs/local_file_system.cpp +++ b/be/src/io/fs/local_file_system.cpp @@ -426,14 +426,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(); } @@ -441,13 +453,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 d765f2daeeca50..361d852c21ec0c 100644 --- a/be/src/io/fs/local_file_system.h +++ b/be/src/io/fs/local_file_system.h @@ -67,6 +67,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 e15ba34a2545e4..ef4a677a0b4027 100644 --- a/be/test/io/fs/local_file_system_test.cpp +++ b/be/test/io/fs/local_file_system_test.cpp @@ -29,11 +29,13 @@ #include #include +#include "common/config.h" #include "common/status.h" #include "common/sync_point.h" #include "gtest/gtest_pred_impl.h" #include "io/fs/file_reader.h" #include "io/fs/file_writer.h" +#include "util/defer_op.h" #include "util/slice.h" namespace doris { @@ -376,6 +378,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() @@ -407,6 +413,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; 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 4a22b75ae64e66..777bd461f53081 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 @@ -22,9 +22,12 @@ 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.mysql.privilege.PrivPredicate; import org.apache.doris.proto.InternalService; import org.apache.doris.proto.InternalService.PGlobResponse; import org.apache.doris.proto.InternalService.PGlobResponse.PFileInfo; +import org.apache.doris.qe.ConnectContext; import org.apache.doris.rpc.BackendServiceProxy; import org.apache.doris.system.Backend; import org.apache.doris.thrift.TBrokerFileStatus; @@ -71,6 +74,9 @@ public LocalTableValuedFunction(Map properties) throws AnalysisE "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()); + // 1. analyze common properties Map otherProps = super.parseCommonProperties(properties); @@ -135,6 +141,16 @@ private void getFileListFromBackend() throws AnalysisException { } } + @Override + public void checkAuth(ConnectContext ctx) { + if (!Env.getCurrentEnv().getAccessManager().checkGlobalPriv(ctx, PrivPredicate.ADMIN) + && !Env.getCurrentEnv().getAccessManager().checkGlobalPriv(ctx, PrivPredicate.OPERATOR)) { + String message = ErrorCode.ERR_SPECIFIC_ACCESS_DENIED_ERROR.formatErrorMsg("ADMIN/OPERATOR"); + throw new org.apache.doris.nereids.exceptions.AnalysisException(message); + } + super.checkAuth(ctx); + } + @Override public TFileType getTFileType() { return TFileType.FILE_LOCAL; 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 index c1e7a73bb6713e..d1e7fdc15529ea 100644 --- 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 @@ -113,4 +113,30 @@ public void testEnabledLocalReachesExistingFileAccess() throws Exception { new LocalTableValuedFunction(PROPERTIES); Mockito.verify(proxy).glob(Mockito.any(), Mockito.any()); } + + @Test + public void testEnabledLocalRejectsUnprivilegedUserBeforeFileAccess() { + Config.enable_local_tvf = true; + AnalysisException exception = Assertions.assertThrows(AnalysisException.class, + () -> new LocalTableValuedFunction(PROPERTIES)); + Assertions.assertTrue(exception.getMessage().contains("ADMIN/OPERATOR")); + Mockito.verifyNoInteractions(systemInfo, proxy); + } + + @Test + public void testOperatorCanAccessLocalFiles() throws Exception { + Config.enable_local_tvf = true; + Mockito.when(accessManager.checkGlobalPriv(context, PrivPredicate.OPERATOR)).thenReturn(true); + new LocalTableValuedFunction(PROPERTIES); + Mockito.verify(proxy).glob(Mockito.any(), Mockito.any()); + } + + @Test + public void testCheckAuthRejectsRevokedPrivileges() throws Exception { + Config.enable_local_tvf = true; + Mockito.when(accessManager.checkGlobalPriv(context, PrivPredicate.ADMIN)).thenReturn(true); + LocalTableValuedFunction local = new LocalTableValuedFunction(PROPERTIES); + Mockito.when(accessManager.checkGlobalPriv(context, PrivPredicate.ADMIN)).thenReturn(false); + Assertions.assertThrows(AnalysisException.class, () -> local.checkAuth(context)); + } } From 7a993f06b6e8edfe3af7bd625fe5ad7fbc288abd Mon Sep 17 00:00:00 2001 From: Dylan Date: Fri, 23 Oct 2026 19:18:51 +0800 Subject: [PATCH 3/3] [fix](tvf) Redact resolved names from local path errors ### What problem does this PR solve? Issue Number: N/A Related PR: #69526 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: 4 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 44bfc285717b1d..b94e5bfff4cf24 100644 --- a/be/src/io/fs/local_file_system.cpp +++ b/be/src/io/fs/local_file_system.cpp @@ -440,10 +440,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 ef4a677a0b4027..4c8facd1d73aca 100644 --- a/be/test/io/fs/local_file_system_test.cpp +++ b/be/test/io/fs/local_file_system_test.cpp @@ -426,15 +426,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"}) { @@ -491,7 +500,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());