From f28b8867fc7f534668742a3d003a67342fecc170 Mon Sep 17 00:00:00 2001 From: Lazreg Raed <160362805+RaidLZ@users.noreply.github.com> Date: Mon, 6 Jul 2026 03:16:20 +0100 Subject: [PATCH] fix(clickhouse): reconcile default port when `secure` is toggled via kwargs or query params (#2426) --- core/wren/src/wren/connector/clickhouse.py | 45 ++++- core/wren/tests/connectors/test_clickhouse.py | 42 ----- .../tests/unit/test_clickhouse_helpers.py | 165 ++++++++++++++++++ 3 files changed, 204 insertions(+), 48 deletions(-) diff --git a/core/wren/src/wren/connector/clickhouse.py b/core/wren/src/wren/connector/clickhouse.py index ad872dc83..3e608f422 100644 --- a/core/wren/src/wren/connector/clickhouse.py +++ b/core/wren/src/wren/connector/clickhouse.py @@ -231,6 +231,19 @@ def _build_clickhouse_column(values: list, arrow_type: pa.DataType) -> pa.Array: # Client kwargs assembly # -------------------------------------------------------------------------- +_FALSY_SECURE_VALUES = frozenset({"", "0", "false", "no", "off"}) + + +def _parse_secure_flag(value: Any) -> bool: + """Interpret a ``secure`` override from kwargs or a URL query string. + + Query parameters always arrive as strings, so ``?secure=false`` must not + be treated as truthy the way ``bool("false")`` would. + """ + if isinstance(value, str): + return value.strip().lower() not in _FALSY_SECURE_VALUES + return bool(value) + def _build_clickhouse_client_kwargs(connection_info: Any) -> dict: """Translate ``ClickHouseConnectionInfo`` / ``ConnectionUrl`` into @@ -246,7 +259,17 @@ def _build_clickhouse_client_kwargs(connection_info: Any) -> dict: "ClickHouse connection URL must use clickhouse:// scheme", ) - kwargs: dict = dict(parse_qsl(parsed.query)) + # ``keep_blank_values`` so a blank parameter (``?secure=``) still + # reaches the override handling below instead of being dropped — + # blank means "falsy", matching ``_parse_secure_flag``. Blank is only + # meaningful for the params handled below, though: any other blank + # param keeps the old dropped behaviour rather than leaking + # ``key=""`` into the client kwargs via ``out.update(kwargs)``. + kwargs: dict = { + key: value + for key, value in parse_qsl(parsed.query, keep_blank_values=True) + if value != "" or key in ("secure", "statement_timeout") + } info_kwargs = getattr(connection_info, "kwargs", None) if info_kwargs: kwargs.update(info_kwargs) @@ -255,17 +278,27 @@ def _build_clickhouse_client_kwargs(connection_info: Any) -> dict: dict(kwargs.pop("settings", {})) if "settings" in kwargs else {} ) statement_timeout = kwargs.pop("statement_timeout", None) - if statement_timeout is not None: + # Blank (``?statement_timeout=``) is treated the same as absent. + if statement_timeout not in (None, ""): settings["max_execution_time"] = int(statement_timeout) # urlparse leaves percent-encoded characters in userinfo, so decode # them before clickhouse-connect sees the credentials. Matches the # mssql / postgres URL handling elsewhere in this package. secure = parsed.scheme == "clickhouse+https" - # Pick the port-less default from the scheme: ClickHouse listens for - # HTTPS on 8443 and plaintext HTTP on 8123. Defaulting an https URL to - # 8123 silently dialed the plaintext port with ``secure=True``, so the - # TLS handshake hit a non-TLS listener and the connection failed. + # TLS can also be toggled *after* the scheme is inspected — via a + # ``?secure=...`` query parameter or a ``kwargs`` override. Reconcile + # the effective value here (kwargs win over query params, which win + # over the scheme, matching the merge above) so the port-less default + # tracks the real TLS setting instead of just the scheme. + secure_override = kwargs.pop("secure", None) + if secure_override is not None: + secure = _parse_secure_flag(secure_override) + # Pick the port-less default from the effective TLS setting: ClickHouse + # listens for HTTPS on 8443 and plaintext HTTP on 8123. Defaulting a + # secure connection to 8123 silently dialed the plaintext port with + # ``secure=True``, so the TLS handshake hit a non-TLS listener and the + # connection failed. An explicit URL port always wins. default_port = 8443 if secure else 8123 out: dict = { "host": parsed.hostname, diff --git a/core/wren/tests/connectors/test_clickhouse.py b/core/wren/tests/connectors/test_clickhouse.py index 934a09cfb..c37433408 100644 --- a/core/wren/tests/connectors/test_clickhouse.py +++ b/core/wren/tests/connectors/test_clickhouse.py @@ -289,45 +289,3 @@ class TestClickHouseClientKwargs: info = _FakeChInfo(kwargs={"statement_timeout": 10}) out = _build_clickhouse_client_kwargs(info) assert out["settings"] == {"max_execution_time": 10} - - -class _FakeChUrl: - """Stand-in exposing a ``connection_url`` for the URL kwargs path.""" - - def __init__(self, url: str) -> None: - from pydantic import SecretStr - - self.connection_url = SecretStr(url) - self.kwargs = None - - -@pytest.mark.clickhouse -class TestClickHouseUrlKwargs: - """Pure-Python tests for the ``connection_url`` branch of the kwargs builder.""" - - def test_https_url_without_port_uses_secure_default_port(self) -> None: - """A port-less clickhouse+https URL must dial 8443 (TLS), not 8123. - - ClickHouse serves HTTPS on 8443 and plaintext HTTP on 8123. Defaulting - an https URL to 8123 while also setting ``secure=True`` made the TLS - client connect to the plaintext listener — the handshake fails. - """ - out = _build_clickhouse_client_kwargs( - _FakeChUrl("clickhouse+https://user:pw@host/db") - ) - assert out["secure"] is True - assert out["port"] == 8443 - - def test_http_url_without_port_uses_plaintext_default_port(self) -> None: - out = _build_clickhouse_client_kwargs( - _FakeChUrl("clickhouse+http://user:pw@host/db") - ) - assert "secure" not in out - assert out["port"] == 8123 - - def test_explicit_port_is_respected_for_https(self) -> None: - out = _build_clickhouse_client_kwargs( - _FakeChUrl("clickhouse+https://user:pw@host:9440/db") - ) - assert out["secure"] is True - assert out["port"] == 9440 diff --git a/core/wren/tests/unit/test_clickhouse_helpers.py b/core/wren/tests/unit/test_clickhouse_helpers.py index aecd3d728..b0fed656b 100644 --- a/core/wren/tests/unit/test_clickhouse_helpers.py +++ b/core/wren/tests/unit/test_clickhouse_helpers.py @@ -228,3 +228,168 @@ def test_clickhouse_arrow_table_nullable_columns_preserve_none_values() -> None: assert table.column("id").to_pylist() == [1, None, 3] assert table.column("name").to_pylist() == ["Alice", "Bob", None] assert table.column("score").to_pylist() == [9.5, None, 7.0] + + +# --------------------------------------------------------------------------- +# 5. Default port tracks the effective ``secure`` value (connection_url branch) +# +# Regression for Canner/WrenAI#2412 / #2416: the port-less default must follow +# the *effective* TLS setting (kwargs > query param > scheme), not the scheme +# alone, and string overrides like ``?secure=false`` must not be truthy. +# --------------------------------------------------------------------------- + + +class TestClickHouseUrlKwargs: + """Pure-Python tests for the ``connection_url`` branch of the kwargs builder.""" + + def test_https_url_without_port_uses_secure_default_port(self) -> None: + """A port-less clickhouse+https URL must dial 8443 (TLS), not 8123. + + ClickHouse serves HTTPS on 8443 and plaintext HTTP on 8123. Defaulting + an https URL to 8123 while also setting ``secure=True`` made the TLS + client connect to the plaintext listener — the handshake fails. + """ + out = _build_clickhouse_client_kwargs( + _FakeConnInfoFromUrl("clickhouse+https://user:pw@host/db") + ) + assert out["secure"] is True + assert out["port"] == 8443 + + def test_http_url_without_port_uses_plaintext_default_port(self) -> None: + out = _build_clickhouse_client_kwargs( + _FakeConnInfoFromUrl("clickhouse+http://user:pw@host/db") + ) + assert "secure" not in out + assert out["port"] == 8123 + + def test_explicit_port_is_respected_for_https(self) -> None: + out = _build_clickhouse_client_kwargs( + _FakeConnInfoFromUrl("clickhouse+https://user:pw@host:9440/db") + ) + assert out["secure"] is True + assert out["port"] == 9440 + + # --- secure enabled after the scheme is inspected (#2416) --------------- + + def test_secure_query_param_uses_secure_default_port(self) -> None: + """``?secure=true`` on a plain scheme must dial 8443, not 8123. + + The scheme alone said plaintext, so the old code picked 8123 — then + ``secure=True`` arrived via the query string and the TLS handshake + hit the plaintext listener. + """ + out = _build_clickhouse_client_kwargs( + _FakeConnInfoFromUrl("clickhouse://user:pw@host/db?secure=true") + ) + assert out["secure"] is True + assert out["port"] == 8443 + + def test_secure_kwargs_override_uses_secure_default_port(self) -> None: + out = _build_clickhouse_client_kwargs( + _FakeConnInfoFromUrl( + "clickhouse://user:pw@host/db", kwargs={"secure": True} + ) + ) + assert out["secure"] is True + assert out["port"] == 8443 + + def test_secure_false_query_param_uses_plaintext_default_port(self) -> None: + """``?secure=false`` must not be truthy just because it is a string.""" + out = _build_clickhouse_client_kwargs( + _FakeConnInfoFromUrl("clickhouse+https://user:pw@host/db?secure=false") + ) + assert "secure" not in out + assert out["port"] == 8123 + + def test_secure_false_kwargs_override_uses_plaintext_default_port(self) -> None: + out = _build_clickhouse_client_kwargs( + _FakeConnInfoFromUrl( + "clickhouse+https://user:pw@host/db", kwargs={"secure": False} + ) + ) + assert "secure" not in out + assert out["port"] == 8123 + + def test_explicit_port_wins_over_secure_query_param(self) -> None: + out = _build_clickhouse_client_kwargs( + _FakeConnInfoFromUrl("clickhouse://user:pw@host:9000/db?secure=true") + ) + assert out["secure"] is True + assert out["port"] == 9000 + + def test_explicit_port_wins_over_secure_false_override(self) -> None: + out = _build_clickhouse_client_kwargs( + _FakeConnInfoFromUrl( + "clickhouse+https://user:pw@host:9440/db", kwargs={"secure": "false"} + ) + ) + assert "secure" not in out + assert out["port"] == 9440 + + def test_secure_kwargs_win_over_query_param(self) -> None: + """kwargs are merged after query params, so they take precedence.""" + out = _build_clickhouse_client_kwargs( + _FakeConnInfoFromUrl( + "clickhouse://user:pw@host/db?secure=true", kwargs={"secure": False} + ) + ) + assert "secure" not in out + assert out["port"] == 8123 + + @pytest.mark.parametrize("raw", ["1", "true", "TRUE", "yes", "on"]) + def test_truthy_secure_strings(self, raw: str) -> None: + out = _build_clickhouse_client_kwargs( + _FakeConnInfoFromUrl(f"clickhouse://user:pw@host/db?secure={raw}") + ) + assert out["secure"] is True + assert out["port"] == 8443 + + @pytest.mark.parametrize("raw", ["0", "false", "FALSE", "no", "off", ""]) + def test_falsy_secure_strings(self, raw: str) -> None: + out = _build_clickhouse_client_kwargs( + _FakeConnInfoFromUrl( + "clickhouse+https://user:pw@host/db", kwargs={"secure": raw} + ) + ) + assert "secure" not in out + assert out["port"] == 8123 + + def test_blank_secure_query_param_is_a_falsy_override(self) -> None: + """``?secure=`` must reach the override handling, not be dropped. + + ``parse_qsl`` discards blank values by default, which silently turned + ``?secure=`` into "unspecified". Blank is falsy, like everywhere else + in ``_parse_secure_flag``. + """ + out = _build_clickhouse_client_kwargs( + _FakeConnInfoFromUrl("clickhouse+https://user:pw@host/db?secure=") + ) + assert "secure" not in out + assert out["port"] == 8123 + + def test_blank_statement_timeout_query_param_is_ignored(self) -> None: + """``?statement_timeout=`` is treated as absent, not ``int("")``.""" + out = _build_clickhouse_client_kwargs( + _FakeConnInfoFromUrl("clickhouse://user:pw@host/db?statement_timeout=") + ) + assert out["settings"] == {} + assert "statement_timeout" not in out + + def test_blank_unhandled_query_param_is_dropped(self) -> None: + """``?connect_timeout=`` must not reach the client kwargs as ``""``. + + ``keep_blank_values`` exists so ``?secure=`` / ``?statement_timeout=`` + can act as explicit "unset" overrides; every other blank param keeps + the old dropped behaviour instead of leaking ``key=""`` into + ``clickhouse_connect.get_client``. + """ + out = _build_clickhouse_client_kwargs( + _FakeConnInfoFromUrl("clickhouse://user:pw@host/db?connect_timeout=") + ) + assert "connect_timeout" not in out + + def test_non_blank_query_param_passes_through(self) -> None: + out = _build_clickhouse_client_kwargs( + _FakeConnInfoFromUrl("clickhouse://user:pw@host/db?connect_timeout=10") + ) + assert out["connect_timeout"] == "10"