Merge pull request ClickHouse#30452 from ClickHouse/backport/21.8/30309

Backport ClickHouse#30309 to 21.8: Fix symlinks in file table function

Author: kssenii

src/Common/filesystemHelpers.cpp

@@ -118,7 +118,7 @@ bool pathStartsWith(const std::filesystem::path & path, const std::filesystem::p
     return absolute_path.starts_with(absolute_prefix_path);
 }
 
-bool symlinkStartsWith(const std::filesystem::path & path, const std::filesystem::path & prefix_path)
+bool fileOrSymlinkPathStartsWith(const std::filesystem::path & path, const std::filesystem::path & prefix_path)
 {
     /// Differs from pathStartsWith in how `path` is normalized before comparison.
     /// Make `path` absolute if it was relative and put it into normalized form: remove
@@ -139,13 +139,14 @@ bool pathStartsWith(const String & path, const String & prefix_path)
     return pathStartsWith(filesystem_path, filesystem_prefix_path);
 }
 
-bool symlinkStartsWith(const String & path, const String & prefix_path)
+bool fileOrSymlinkPathStartsWith(const String & path, const String & prefix_path)
 {
     auto filesystem_path = std::filesystem::path(path);
     auto filesystem_prefix_path = std::filesystem::path(prefix_path);
 
-    return symlinkStartsWith(filesystem_path, filesystem_prefix_path);
+    return fileOrSymlinkPathStartsWith(filesystem_path, filesystem_prefix_path);
 }
+
 }
 
 

src/Common/filesystemHelpers.h

@@ -35,7 +35,9 @@ bool pathStartsWith(const std::filesystem::path & path, const std::filesystem::p
 /// Returns true if path starts with prefix path
 bool pathStartsWith(const String & path, const String & prefix_path);
 
-bool symlinkStartsWith(const String & path, const String & prefix_path);
+/// Same as pathStartsWith, but without canonization, i.e. allowed to check symlinks.
+/// (Path is made absolute and normalized.)
+bool fileOrSymlinkPathStartsWith(const String & path, const String & prefix_path);
 
 }
 

src/Dictionaries/FileDictionarySource.cpp

@@ -30,8 +30,9 @@ FileDictionarySource::FileDictionarySource(
     , sample_block{sample_block_}
     , context(context_)
 {
-    if (created_from_ddl && !pathStartsWith(filepath, context->getUserFilesPath()))
-        throw Exception(ErrorCodes::PATH_ACCESS_DENIED, "File path {} is not inside {}", filepath, context->getUserFilesPath());
+    auto user_files_path = context->getUserFilesPath();
+    if (created_from_ddl && !fileOrSymlinkPathStartsWith(filepath, user_files_path))
+        throw Exception(ErrorCodes::PATH_ACCESS_DENIED, "File path {} is not inside {}", filepath, user_files_path);
 }
 
 

src/Dictionaries/LibraryDictionarySource.cpp

@@ -41,14 +41,9 @@ LibraryDictionarySource::LibraryDictionarySource(
     , sample_block{sample_block_}
     , context(Context::createCopy(context_))
 {
-    bool path_checked = false;
-    if (fs::is_symlink(path))
-        path_checked = symlinkStartsWith(path, context->getDictionariesLibPath());
-    else
-        path_checked = pathStartsWith(path, context->getDictionariesLibPath());
-
-    if (created_from_ddl && !path_checked)
-        throw Exception(ErrorCodes::PATH_ACCESS_DENIED, "File path {} is not inside {}", path, context->getDictionariesLibPath());
+    auto dictionaries_lib_path = context->getDictionariesLibPath();
+    if (created_from_ddl && !fileOrSymlinkPathStartsWith(path, dictionaries_lib_path))
+        throw Exception(ErrorCodes::PATH_ACCESS_DENIED, "File path {} is not inside {}", path, dictionaries_lib_path);
 
     if (!fs::exists(path))
         throw Exception(ErrorCodes::FILE_DOESNT_EXIST, "LibraryDictionarySource: Can't load library {}: file doesn't exist", path);

src/Storages/StorageFile.cpp

@@ -22,6 +22,7 @@
 #include <Common/escapeForFileName.h>
 #include <Common/typeid_cast.h>
 #include <Common/parseGlobs.h>
+#include <Common/filesystemHelpers.h>
 #include <Storages/ColumnsDescription.h>
 #include <Storages/StorageInMemoryMetadata.h>
 
@@ -120,8 +121,8 @@ void checkCreationIsAllowed(ContextPtr context_global, const std::string & db_di
         return;
 
     /// "/dev/null" is allowed for perf testing
-    if (!startsWith(table_path, db_dir_path) && table_path != "/dev/null")
-        throw Exception("File is not inside " + db_dir_path, ErrorCodes::DATABASE_ACCESS_DENIED);
+    if (!fileOrSymlinkPathStartsWith(table_path, db_dir_path) && table_path != "/dev/null")
+        throw Exception(ErrorCodes::DATABASE_ACCESS_DENIED, "File `{}` is not inside `{}`", table_path, db_dir_path);
 
     if (fs::exists(table_path) && fs::is_directory(table_path))
         throw Exception("File must not be a directory", ErrorCodes::INCORRECT_FILE_NAME);
@@ -136,7 +137,10 @@ Strings StorageFile::getPathsList(const String & table_path, const String & user
         fs_table_path = user_files_absolute_path / fs_table_path;
 
     Strings paths;
-    const String path = fs::weakly_canonical(fs_table_path);
+    /// Do not use fs::canonical or fs::weakly_canonical.
+    /// Otherwise it will not allow to work with symlinks in `user_files_path` directory.
+    String path = fs::absolute(fs_table_path);
+    path = fs::path(path).lexically_normal(); /// Normalize path.
     if (path.find_first_of("*?{") == std::string::npos)
     {
         std::error_code error;

tests/queries/0_stateless/02051_symlinks_to_user_files.reference

@@ -0,0 +1 @@
+OK

tests/queries/0_stateless/02051_symlinks_to_user_files.sh

@@ -0,0 +1,32 @@
+#!/usr/bin/env bash
+# Tags: no-fasttest, no-parallel
+
+CUR_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
+# shellcheck source=../shell_config.sh
+. "$CUR_DIR"/../shell_config.sh
+
+# See 01658_read_file_to_string_column.sh
+user_files_path=$(clickhouse-client --query "select _path,_file from file('nonexist.txt', 'CSV', 'val1 char')" 2>&1 | grep Exception | awk '{gsub("/nonexist.txt","",$9); print $9}')
+
+FILE_PATH="${user_files_path}/file/"
+mkdir -p ${FILE_PATH}
+chmod 777 ${FILE_PATH}
+
+FILE="test_symlink_${CLICKHOUSE_DATABASE}"
+
+symlink_path=${FILE_PATH}/${FILE}
+file_path=$CUR_DIR/${FILE}
+
+touch ${file_path}
+ln -s ${file_path} ${symlink_path}
+chmod ugo+w ${symlink_path}
+
+function cleanup()
+{
+    rm ${symlink_path} ${file_path}
+}
+trap cleanup EXIT
+
+${CLICKHOUSE_CLIENT} --query="insert into table function file('${symlink_path}', 'Values', 'a String') select 'OK'";
+${CLICKHOUSE_CLIENT} --query="select * from file('${symlink_path}', 'Values', 'a String')";
+