fix: prevent ClickHouse SQL injection in ListHooksTraces#2037
fix: prevent ClickHouse SQL injection in ListHooksTraces#2037
Conversation
…House SQL injection ListHooksTraces constructs ClickHouse queries using filter.Path via fmt.Sprintf without validating the input, unlike ListTelemetryLogs which checks against validJSONPath regex. A malicious filter.Path like "x') OR '1'='1" would be interpolated directly into SQL clauses (JSONExtractKeys, attributes access), allowing data exfiltration or query manipulation. Add the same validJSONPath check used elsewhere.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| materializedCol, isMaterialized := materializedColumns[filter.Path] | ||
| var columnRef string | ||
| if isMaterialized { |
There was a problem hiding this comment.
🚩 Inconsistent @-prefix path handling between ListLogs and ListHooksTraces
ListLogs (line 257) delegates column resolution to resolveAttributeColumn() which translates @-prefixed paths to toString(attributes.app.<path>). In contrast, ListHooksTraces (lines 1494-1501) does its own inline resolution that lacks @-prefix handling — an @user.region path would produce toString(attributes.\@user.region`)instead of the intendedtoString(attributes.app.user.region). This is a pre-existing inconsistency not introduced by this PR, but the new validJSONPathregex at line 1491 does accept@-prefixed paths (per the regex at queries.sql.go:25), so callers could pass such paths expecting them to work correctly. Additionally, the exists/not_existsbranches at lines 1524 and 1533 would pass the raw@user.regionstring intoJSONExtractKeys, which likely wouldn't match. Consider refactoring ListHooksTracesto useresolveAttributeColumn` for consistency.
(Refers to lines 1494-1501)
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Summary
ListHooksTracesinserver/internal/telemetry/repo/queries.sql.goconstructs ClickHouse queries usingfilter.Pathviafmt.Sprintfwithout input validation, allowing SQL injectionListTelemetryLogs(line 254) already validates paths againstvalidJSONPathregex — this was simply missing fromListHooksTracesfilter.Pathlikex') OR '1'='1would be interpolated directly intohas(JSONExtractKeys(attributes), '...')andtoString(attributes.\...`)` clausesvalidJSONPath.MatchString(filter.Path)check to skip invalid pathsTest plan
go build ./server/internal/telemetry/...compiles cleanly