Found via ClickGap automated review. Please close or comment if this is incorrect or needs adjustment.
Retrospective finding from a historical scan of PR #65238 (merged 2024-07-17). Confirmed on current codebase — close with a note if already fixed.
Describe what's wrong
When a user explicitly sets <accept-invalid-certificate>0</accept-invalid-certificate> in their client config file, the client still accepts invalid certificates instead of rejecting them.
Root cause: Client.cpp:292: if (config().has("accept-invalid-certificate")) only checks if the key exists, not whether its boolean value is true. So <accept-invalid-certificate>0</accept-invalid-certificate> triggers the acceptance logic.
Why we believe this is a bug: Client::initialize (Client.cpp:292) → config().has("accept-invalid-certificate") checks key existence, not value → Lines 294-295 set AcceptCertificateHandler even when value is 0
Affected locations:
programs/client/Client.cpp:292 — accept-invalid-certificate config check
Impact: Users who explicitly set <accept-invalid-certificate>0</accept-invalid-certificate> intending to reject invalid certificates will have their certificates accepted, potentially creating a security vulnerability where self-signed or invalid certificates are trusted when they should not be.
Does it reproduce on most recent release?
Yes — confirmed on current master (commit 5068a189c05).
How to reproduce
Run: cd tests/integration && CLICKHOUSE_TESTS_SERVER_BIN_PATH=../../build/programs/clickhouse CLICKHOUSE_TESTS_BASE_CONFIG_DIR=../../programs/server pytest test_accept_invalid_certificate/test_explicit_zero.py -v -s
Expected behavior
Exception: certificate verify failed
Error message and/or stacktrace
Connection succeeded with output '1\n' when it should have failed with 'certificate verify failed'
Additional context
Suggested fix: Change line 292 from if (config().has("accept-invalid-certificate")) to if (config().getBool("accept-invalid-certificate", false))
Analysis details: Confidence HIGH | Severity P2 | Testability: INTEGRATION_TEST
Found during automated review of PR #65238.
ClickGapAI · Confidence: HIGH · Severity: P2 · Finding: h_pr65238_001
Found via ClickGap automated review. Please close or comment if this is incorrect or needs adjustment.
Retrospective finding from a historical scan of PR #65238 (merged 2024-07-17). Confirmed on current codebase — close with a note if already fixed.
Describe what's wrong
When a user explicitly sets
<accept-invalid-certificate>0</accept-invalid-certificate>in their client config file, the client still accepts invalid certificates instead of rejecting them.Root cause: Client.cpp:292:
if (config().has("accept-invalid-certificate"))only checks if the key exists, not whether its boolean value is true. So<accept-invalid-certificate>0</accept-invalid-certificate>triggers the acceptance logic.Why we believe this is a bug: Client::initialize (Client.cpp:292) → config().has("accept-invalid-certificate") checks key existence, not value → Lines 294-295 set AcceptCertificateHandler even when value is 0
Affected locations:
programs/client/Client.cpp:292— accept-invalid-certificate config checkImpact: Users who explicitly set
<accept-invalid-certificate>0</accept-invalid-certificate>intending to reject invalid certificates will have their certificates accepted, potentially creating a security vulnerability where self-signed or invalid certificates are trusted when they should not be.Does it reproduce on most recent release?
Yes — confirmed on current
master(commit5068a189c05).How to reproduce
Expected behavior
Error message and/or stacktrace
Additional context
Suggested fix: Change line 292 from
if (config().has("accept-invalid-certificate"))toif (config().getBool("accept-invalid-certificate", false))Analysis details: Confidence HIGH | Severity P2 | Testability:
INTEGRATION_TESTFound during automated review of PR #65238.
ClickGapAI · Confidence: HIGH · Severity: P2 · Finding:
h_pr65238_001