Forbid creating directories in non-user directories by scanhex12 · Pull Request #86844 · ClickHouse/ClickHouse

scanhex12 · 2025-09-08T17:33:47Z

Changelog category (leave one):

Critical Bug Fix (crash, data loss, RBAC) or LOGICAL_ERROR

clickhouse-gh · 2025-09-08T17:34:14Z

Workflow [PR], commit [74ba30f]

nikitamikhaylov · 2025-09-09T07:00:50Z

Test?

Algunenano · 2025-09-09T12:23:54Z

Not for changelog (changelog entry is not required)

Please set the proper category. This is a critical bugfix

divanik · 2025-09-09T12:38:25Z

I am not against of merging this PR (at least it is mitigate crtical issue). But I am really worried that LocalObjectStorage interface allows us to do such actions. If so, the bug can be a great deal more dangerous

divanik

Also address the tests and create the test which checks that security bug is not longer reproduced

divanik · 2025-09-09T14:58:18Z

src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp

+    if (object_storage->getType() == ObjectStorageType::Local)
+    {
+        auto user_files_path = local_context->getUserFilesPath();
+        if (!fileOrSymlinkPathStartsWith(configuration_ptr->getPathForRead().path, user_files_path))
+            throw Exception(ErrorCodes::PATH_ACCESS_DENIED, "File path {} is not inside {}", configuration_ptr->getPathForRead().path, user_files_path);
+    }


Move the logic to

ClickHouse/src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h

Line 112 in 91dde0a

{

to also cover DeltaLake, referencing existing table (both by table engine and table function)

Any reasons why you think this is a bad suggestion?

src/Core/Settings.cpp

michael-anastasakis · 2025-09-10T10:02:10Z

From a security perspective, we cannot have a critical security check as an optional setting. That validation needs to be mandatory similarly as @Algunenano commented. I understand that there may be some extra effort needed to fix the failing tests but we need to have a proper and clean solution rather than a partial one.

src/Disks/ObjectStorages/Local/LocalObjectStorage.cpp

divanik · 2025-09-12T13:24:18Z

tests/integration/test_storage_iceberg_schema_evolution/test_array_evolved_nested.py

-            f"/iceberg_data/default/{TABLE_NAME}/",
-            f"/iceberg_data/default/{TABLE_NAME}/",


Is it true that default_upload_directory is always used with prefix "/var/lib/clickhouse/user_files/" now? Let's patch arguments in the function itself. This will dramatically change number of backported changes

divanik · 2025-09-12T13:25:27Z

tests/integration/test_storage_iceberg_with_spark/test_iceberg_snapshot_reads.py

    assert int(instance.query(f"SELECT count() FROM {TABLE_NAME}")) == 100
    snapshot1_timestamp = datetime.now(timezone.utc)
-    snapshot1_id = get_last_snapshot(f"/iceberg_data/default/{TABLE_NAME}/")
+    snapshot1_id = get_last_snapshot(f"/var/lib/clickhouse/user_files/iceberg_data/default/{TABLE_NAME}/")


In this function as well

This function is used 2 times with prefix /var/lib/clickhouse/user_files, so it's useless to move those changes in get_last_snapshot.

divanik · 2025-09-12T13:29:37Z

src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp

+    if (object_storage->getType() == ObjectStorageType::Local)
+    {
+        auto user_files_path = local_context->getUserFilesPath();
+        if (!fileOrSymlinkPathStartsWith(configuration_ptr->getPathForRead().path, user_files_path))
+            throw Exception(ErrorCodes::PATH_ACCESS_DENIED, "File path {} is not inside {}", configuration_ptr->getPathForRead().path, user_files_path);
+    }


Any reasons why you think this is a bad suggestion?

divanik · 2025-09-12T16:41:34Z

src/Storages/ObjectStorage/DataLakes/DeltaLakeMetadata.cpp

+
+    auto log = getLogger("IcebergMetadata");


divanik · 2025-09-12T16:43:41Z

tests/integration/test_storage_iceberg_with_spark/test_writes_field_partitioning.py

    )

-    df = spark.read.format("iceberg").load(f"/iceberg_data/default/{TABLE_NAME}").collect()
+    df = spark.read.format("iceberg").load(f"/var/lib/clickhouse/user_files/iceberg_data/default/{TABLE_NAME}").collect()


Let's move this to a separate function (in backported code it also will be easy to replace such line with a function call)

divanik · 2025-09-12T16:44:51Z

tests/integration/test_storage_iceberg_with_spark/test_writes.py

+    with open(f"/var/lib/clickhouse/user_files/iceberg_data/default/{TABLE_NAME}/metadata/version-hint.text", "wb") as f:
        f.write(b"4")


Also this to a separate function which takes table name and int version

divanik · 2025-09-12T16:47:27Z

tests/integration/test_storage_iceberg_with_spark/test_writes_create_partitioned_table.py

@@ -28,12 +28,12 @@ def test_writes_create_partitioned_table(started_cluster_iceberg_with_spark, for
    default_download_directory(


default_download_directory can be changed as a default_upload_directory

…o check_right

Algunenano · 2025-09-15T11:40:29Z

Please, change the changelog category to "Critical bug fix"

pamarcos · 2025-09-15T13:56:34Z

Please, change the changelog category to "Critical bug fix"

+1, this seems relevant enough not only to be considered as a critical bug fix, but also IMHO a changelog entry is the bare minimum we should add as well

otselnik · 2026-02-12T10:43:32Z

@scanhex12 Will the work on backporting this pull request to the 25.8 version be continued?

clickhouse-gh bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Sep 8, 2025

kssenii self-assigned this Sep 9, 2025

divanik self-assigned this Sep 9, 2025

divanik requested changes Sep 9, 2025

View reviewed changes

Algunenano reviewed Sep 10, 2025

View reviewed changes

src/Core/Settings.cpp Outdated Show resolved Hide resolved

kssenii reviewed Sep 10, 2025

View reviewed changes

src/Disks/ObjectStorages/Local/LocalObjectStorage.cpp Outdated Show resolved Hide resolved

scanhex12 force-pushed the check_right branch from b64ecf6 to 7680331 Compare September 11, 2025 21:38

scanhex12 requested a review from divanik September 11, 2025 23:31

divanik requested changes Sep 12, 2025

View reviewed changes

scanhex12 added 12 commits September 12, 2025 15:21

start

f95ddd4

fix

a79a457

style

efd8eaa

fix status

5ce47a1

fixes

9e9e747

fixes

272410b

fix

4b136b9

fix azure

1c91e4b

fix test

34cc089

fix tests

6cebaf2

fix

ca3a85a

fix

7ae21a6

scanhex12 force-pushed the check_right branch from ade470f to 7ae21a6 Compare September 12, 2025 15:32

scanhex12 added 2 commits September 12, 2025 15:34

fix

78bf097

fix

2072431

scanhex12 requested a review from divanik September 12, 2025 16:14

divanik reviewed Sep 12, 2025

View reviewed changes

scanhex12 added 3 commits September 12, 2025 18:28

fix

ba834a1

Merge branch 'master' of https://github.com/ClickHouse/ClickHouse int…

a034ecb

…o check_right

more fix

229471c

scanhex12 force-pushed the check_right branch from 905b621 to 229471c Compare September 13, 2025 12:20

scanhex12 and others added 4 commits September 14, 2025 10:19

fix

e95c1d2

fix test

9365ad5

another fix

484b6a1

Update DeltaLakeMetadata.cpp

74ba30f

divanik approved these changes Sep 15, 2025

View reviewed changes

scanhex12 added pr-must-backport Pull request should be backported intentionally. Use this label with great care! v25.8-must-backport labels Sep 15, 2025

scanhex12 added this pull request to the merge queue Sep 15, 2025

Merged via the queue into ClickHouse:master with commit 5ec7d6d Sep 15, 2025
120 of 122 checks passed

scanhex12 deleted the check_right branch September 15, 2025 12:39

robot-ch-test-poll1 added the pr-synced-to-cloud The PR is synced to the cloud repo label Sep 15, 2025

Algunenano removed the v25.8-must-backport label Sep 15, 2025

scanhex12 added the pr-critical-bugfix label Sep 15, 2025

robot-ch-test-poll1 added pr-backports-created-cloud deprecated label, NOOP pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR labels Sep 15, 2025

Algunenano mentioned this pull request Sep 16, 2025

test_disks_app_interactive/test.py are flaky/broken #87183

Closed

Felixoid mentioned this pull request Nov 4, 2025

Fix cherry_pick.py by adding update:>last_90_days #89498

Merged

		f"/iceberg_data/default/{TABLE_NAME}/",
		f"/iceberg_data/default/{TABLE_NAME}/",

		with open(f"/var/lib/clickhouse/user_files/iceberg_data/default/{TABLE_NAME}/metadata/version-hint.text", "wb") as f:
		f.write(b"4")

		@@ -28,12 +28,12 @@ def test_writes_create_partitioned_table(started_cluster_iceberg_with_spark, for
		default_download_directory(

Conversation

scanhex12 commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

Uh oh!

clickhouse-gh bot commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikitamikhaylov commented Sep 9, 2025

Uh oh!

Algunenano commented Sep 9, 2025

Uh oh!

divanik commented Sep 9, 2025

Uh oh!

divanik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michael-anastasakis commented Sep 10, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Algunenano commented Sep 15, 2025

Uh oh!

Uh oh!

pamarcos commented Sep 15, 2025

Uh oh!

otselnik commented Feb 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

scanhex12 commented Sep 8, 2025 •

edited

Loading

clickhouse-gh bot commented Sep 8, 2025 •

edited

Loading