Keep readonly-pinned settings from failing under client-side compatibility

A native-protocol client resolves `compatibility` on its own side and then
transmits every reverted setting as an explicit change. For a setting a profile
pins with a `readonly` (CONST) constraint this failed the whole query with
SETTING_CONSTRAINT_VIOLATION, even though the server keeps the pinned value
regardless. The HTTP interface, which resolves `compatibility` server-side, was
never affected.

When a settings batch contains `compatibility`, reproduce the values it derives
and drop a change to a CONST-pinned setting when it merely carries that derived
value. A value differing from the derived one is a genuine override attempt of a
read-only setting and still fails, so the lockdown guarantee is preserved.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

src/Access/SettingsConstraints.cpp

@@ -9,7 +9,9 @@
 #include <Common/SettingSource.h>
 #include <IO/WriteHelpers.h>
 
+#include <algorithm>
 #include <bitset>
+#include <optional>
 #include <string_view>
 #include <unordered_map>
 
@@ -242,10 +244,31 @@ void SettingsConstraints::checkOrClamp(const Settings & current_settings, Settin
 {
     /// If we filter out settings that match the current default here, `compatibility` will silently override them.
     /// So when `compatibility` is present, we keep unchanged settings so they are applied after `compatibility`.
-    bool has_compatibility_setting = changes.tryGet("compatibility") != nullptr;
+    const Field * compatibility_value = changes.tryGet("compatibility");
+    bool has_compatibility_setting = compatibility_value != nullptr;
+
+    /// A native-protocol client resolves `compatibility` on its own side and transmits every reverted setting as an
+    /// explicit change (see `SettingsImpl::applyCompatibilitySetting`). For a setting a profile pins with `readonly`
+    /// (a CONST constraint) this would fail the whole query, even though the server keeps the pinned value regardless.
+    /// Reproduce the values that `compatibility` derives so `checkImpl` can drop such compatibility-derived changes
+    /// instead of throwing, matching how the server resolves `compatibility` itself (and how HTTP already behaves).
+    /// Only build the snapshot when a CONST-pinned setting is actually present, to keep the common case free.
+    std::optional<Settings> compatibility_settings;
+    if (has_compatibility_setting
+        && std::any_of(changes.begin(), changes.end(), [&](const SettingChange & change)
+           {
+               auto it = constraints.find(resolveSettingNameWithCache(Settings::resolveName(change.name)));
+               return it != constraints.end() && it->second.writability == SettingConstraintWritability::CONST;
+           }))
+    {
+        compatibility_settings.emplace();
+        compatibility_settings->set("compatibility", *compatibility_value);
+    }
+
     std::erase_if(changes, [&](SettingChange & change)
     {
-        return !checkImpl(current_settings, change, reaction, source, /*ignore_unchanged_settings=*/has_compatibility_setting);
+        return !checkImpl(current_settings, change, reaction, source, /*ignore_unchanged_settings=*/has_compatibility_setting,
+                          compatibility_settings ? &*compatibility_settings : nullptr);
     });
 }
 
@@ -285,7 +308,8 @@ bool SettingsConstraints::checkImpl(const Settings & current_settings,
                                     SettingChange & change,
                                     ReactionOnViolation reaction,
                                     SettingSource source,
-                                    bool ignore_unchanged_settings) const
+                                    bool ignore_unchanged_settings,
+                                    const Settings * compatibility_settings) const
 {
     std::string_view setting_name = Settings::resolveName(change.name);
 
@@ -324,7 +348,20 @@ bool SettingsConstraints::checkImpl(const Settings & current_settings,
             return true;
     }
 
-    return getChecker(current_settings, setting_name).check(change, new_value, reaction, source);
+    auto checker = getChecker(current_settings, setting_name);
+
+    /// Drop a change that merely carries the value `compatibility` derived on the client side for a setting the
+    /// profile pins read-only (see `checkOrClamp`): the pinned value stays in force and the query no longer fails.
+    /// A value differing from the derived one is a genuine override attempt of a read-only setting and still fails.
+    if (compatibility_settings && checker.explain.text.empty()
+        && checker.constraint.writability == SettingConstraintWritability::CONST)
+    {
+        Field compatibility_value;
+        if (compatibility_settings->tryGet(change.name, compatibility_value) && new_value == compatibility_value)
+            return false;
+    }
+
+    return checker.check(change, new_value, reaction, source);
 }
 
 bool SettingsConstraints::checkImpl(const MergeTreeSettings & current_settings, SettingChange & change, ReactionOnViolation reaction) const

src/Access/SettingsConstraints.h

@@ -166,7 +166,8 @@ private:
         SettingChange & change,
         ReactionOnViolation reaction,
         SettingSource source,
-        bool ignore_unchanged_settings = false) const;
+        bool ignore_unchanged_settings = false,
+        const Settings * compatibility_settings = nullptr) const;
 
     bool checkImpl(const MergeTreeSettings & current_settings, SettingChange & change, ReactionOnViolation reaction) const;
 

tests/integration/test_compatibility_readonly_constrained_setting/configs/users.xml

@@ -0,0 +1,28 @@
+<clickhouse>
+    <!--
+        Reproduces the recommended lockdown of `s3_allow_server_credentials_in_user_queries`: pin it to 0
+        with an explicit value and a `readonly` (CONST) constraint, so untrusted users cannot enable it.
+
+        A native-protocol client that sets an older `compatibility` resolves it on its own side and then
+        transmits the reverted value of the pinned setting as an explicit change. The server must keep the
+        pinned value instead of failing every query with SETTING_CONSTRAINT_VIOLATION.
+    -->
+    <profiles>
+        <default>
+            <s3_allow_server_credentials_in_user_queries>0</s3_allow_server_credentials_in_user_queries>
+            <constraints>
+                <s3_allow_server_credentials_in_user_queries>
+                    <readonly/>
+                </s3_allow_server_credentials_in_user_queries>
+            </constraints>
+        </default>
+    </profiles>
+
+    <users>
+        <default>
+            <password></password>
+            <profile>default</profile>
+            <access_management>1</access_management>
+        </default>
+    </users>
+</clickhouse>

tests/integration/test_compatibility_readonly_constrained_setting/test.py

@@ -0,0 +1,48 @@
+import pytest
+
+from helpers.cluster import ClickHouseCluster
+
+cluster = ClickHouseCluster(__file__)
+node = cluster.add_instance("node", user_configs=["configs/users.xml"])
+
+# Any version before `s3_allow_server_credentials_in_user_queries` was introduced (26.7), so that
+# `compatibility` reverts it to its old default and the client transmits that reverted value.
+OLD_COMPATIBILITY = "24.1"
+
+SETTING = "s3_allow_server_credentials_in_user_queries"
+
+
+@pytest.fixture(scope="module")
+def start_cluster():
+    try:
+        cluster.start()
+        yield cluster
+    finally:
+        cluster.shutdown()
+
+
+def test_old_compatibility_keeps_readonly_pinned_setting(start_cluster):
+    # The native client resolves `compatibility` locally and sends the reverted value of the pinned
+    # setting as an explicit change. With the pin in place the query must still succeed, and the
+    # pinned value must stay in force.
+    assert node.query("SELECT 1", settings={"compatibility": OLD_COMPATIBILITY}).strip() == "1"
+    assert (
+        node.query(
+            f"SELECT getSetting('{SETTING}')", settings={"compatibility": OLD_COMPATIBILITY}
+        ).strip()
+        == "false"
+    )
+
+
+def test_explicit_override_of_readonly_setting_is_rejected(start_cluster):
+    # A genuine attempt to change the read-only setting (no `compatibility` involved) is still refused.
+    error = node.query_and_get_error("SELECT 1", settings={SETTING: 1})
+    assert "should not be changed" in error
+
+
+def test_http_compatibility_unaffected(start_cluster):
+    # Over HTTP `compatibility` is resolved server-side against the already-pinned value, so this path
+    # was never broken; guard it so the native and HTTP paths stay consistent.
+    assert (
+        node.http_query("SELECT 1", params={"compatibility": OLD_COMPATIBILITY}).strip() == "1"
+    )

tests/queries/0_stateless/04612_compatibility_readonly_constrained_setting.reference

@@ -0,0 +1,2 @@
+false
+SETTING_CONSTRAINT_VIOLATION

tests/queries/0_stateless/04612_compatibility_readonly_constrained_setting.sh

@@ -0,0 +1,26 @@
+#!/usr/bin/env bash
+
+CUR_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
+# shellcheck source=../shell_config.sh
+. "$CUR_DIR"/../shell_config.sh
+
+user="user_${CLICKHOUSE_DATABASE}"
+profile="profile_${CLICKHOUSE_DATABASE}"
+
+${CLICKHOUSE_CLIENT} --query "DROP USER IF EXISTS ${user}"
+${CLICKHOUSE_CLIENT} --query "DROP SETTINGS PROFILE IF EXISTS ${profile}"
+
+# Pin a setting read-only in a profile: an explicit value plus a CONST constraint. This mirrors the recommended
+# lockdown of s3_allow_server_credentials_in_user_queries for untrusted users.
+${CLICKHOUSE_CLIENT} --query "CREATE SETTINGS PROFILE ${profile} SETTINGS s3_allow_server_credentials_in_user_queries = 0 CONST"
+${CLICKHOUSE_CLIENT} --query "CREATE USER ${user} IDENTIFIED WITH no_password SETTINGS PROFILE ${profile}"
+
+# An old compatibility is resolved by the native client, which then transmits the reverted value of the pinned
+# setting as an explicit change. This must not fail the query; the pinned value stays in force.
+${CLICKHOUSE_CLIENT} --user "${user}" --compatibility 24.1 --query "SELECT getSetting('s3_allow_server_credentials_in_user_queries')"
+
+# A genuine attempt to change the read-only setting is still rejected.
+${CLICKHOUSE_CLIENT} --user "${user}" --s3_allow_server_credentials_in_user_queries 1 --query "SELECT 1" 2>&1 | grep -o -m1 "SETTING_CONSTRAINT_VIOLATION"
+
+${CLICKHOUSE_CLIENT} --query "DROP USER IF EXISTS ${user}"
+${CLICKHOUSE_CLIENT} --query "DROP SETTINGS PROFILE IF EXISTS ${profile}"