security: use parameterized queries for ClickHouse model comparison

[kolega-ai-dev]

Vulnerability identified and fix provided by Kolega.dev

SQL Injection via String Concatenation in ClickHouse Queries

Location

ModelComparisonManager.ts:71, 106, 110, 118, 122, 254-255, 276-277, 293-294, 315-316, 341-342

Description

Multiple ClickHouse queries are constructed using string concatenation with unsanitized model names and provider values. The formattedModelNames variable is built by joining user-provided model names directly into SQL: '${model.names.join("','")}'. While the provider is toUpperCase'd, it's still concatenated directly into queries without parameterization. Although ClickHouse uses positional parameters in some places, the model names are not properly escaped or parameterized.

Analysis Notes

Confirmed SQL injection vulnerability. At line 71, user-provided model names from the API are directly concatenated into SQL using template literals: const formattedModelNames = ('${model.names.join("','")}')``. This is then used in 5 different ClickHouse queries (lines 254, 276, 293, 315, 341) without parameterization. The endpoint at /v1/public/compare/models accepts a `ModelsToCompare[]` body where `names: string[]` is attacker-controlled. A malicious model name like `foo'); DROP TABLE request_response_rmt; --` could escape the string context. The provider value is `toUpperCase()`'d but this provides no SQL injection protection. ClickHouse supports parameterized queries which should be used instead. This is exploitable by any authenticated API user.

Fix Applied

Replaced all string concatenation with ClickHouse's native parameterized query placeholders ({val_N:String}). Model names and provider values are now passed through the dbQuery parameter array, where the @clickhouse/client library handles safe value binding and escaping. A helper method buildModelNameParams generates individual {val_N:String} placeholders for each model name in the IN clause, eliminating any possibility of SQL injection through user-controlled input.

Tests/Linters Ran

• TypeScript type check (npx tsc --noEmit): Passed with no errors
• Build (yarn build): Passed successfully
• Jest test suite (npx jest --detectOpenHandles): 221 passed, 38 failed, 2 skipped — all 38 failures are pre-existing (emoji/sanitize test issues unrelated to this change), confirmed identical results on clean main branch
• No ModelComparison-specific tests exist in the codebase
• Jawn backend has no dedicated lint/format scripts; TypeScript compilation serves as the primary static check

[greptile-apps]

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.