Merge pull request #30309 from kssenii/fix-file-symlinks

Fix symlinks in file table function

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
@@ -140,13 +140,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,8 +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);
 
-/// Returns true if symlink starts with prefix path
+/// Same as pathStartsWith, but without canonization, i.e. allowed to check symlinks.
-bool symlinkStartsWith(const String & path, const String & prefix_path);
+/// (Path is made absolute and normalized.)
+bool fileOrSymlinkPathStartsWith(const String & path, const String & prefix_path);
 
 }
 

src/Dictionaries/FileDictionarySource.cpp

@@ -31,7 +31,7 @@ FileDictionarySource::FileDictionarySource(
     , context(context_)
 {
     auto user_files_path = context->getUserFilesPath();
-    if (created_from_ddl && !pathStartsWith(filepath, user_files_path))
+    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,13 +41,7 @@ LibraryDictionarySource::LibraryDictionarySource(
     , context(Context::createCopy(context_))
 {
     auto dictionaries_lib_path = context->getDictionariesLibPath();
-    bool path_checked = false;
+    if (created_from_ddl && !fileOrSymlinkPathStartsWith(path, dictionaries_lib_path))
-    if (fs::is_symlink(path))
-        path_checked = symlinkStartsWith(path, dictionaries_lib_path);
-    else
-        path_checked = pathStartsWith(path, dictionaries_lib_path);
-
-    if (created_from_ddl && !path_checked)
         throw Exception(ErrorCodes::PATH_ACCESS_DENIED, "File path {} is not inside {}", path, dictionaries_lib_path);
 
     if (!fs::exists(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>
 
@@ -124,8 +125,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")
+    if (!fileOrSymlinkPathStartsWith(table_path, db_dir_path) && table_path != "/dev/null")
-        throw Exception("File is not inside " + db_dir_path, ErrorCodes::DATABASE_ACCESS_DENIED);
+        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);
@@ -140,7 +141,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')";
+