Skip to content

Conversation

jwintel
Copy link
Contributor

@jwintel jwintel commented May 28, 2025

Description

To fix the issue, we need to replace the unsafe query construction with a parameterized query using placeholders. This ensures that user-controlled data is safely embedded into the query without risking SQL injection. Specifically:

Replace the fmt.Sprintf-based query construction with a query string that uses placeholders (?) for parameters.
Pass the user-controlled values as arguments to the Query method, which will safely escape and embed them into the query.

Related Issue

See code scanning issues numbers 4-17

Motivation and Context

To ensure that we sanitize input from user

How Has This Been Tested?

  • Should pass CI checks

@jwintel jwintel requested review from mlim19 and skamerintel May 28, 2025 21:12
@jwintel jwintel self-assigned this May 28, 2025
@mlim19
Copy link
Contributor

mlim19 commented May 29, 2025

@skamerintel, please review

@jwintel jwintel requested a review from dkorlovs May 29, 2025 18:49
@dkorlovs
Copy link

@jwintel how user can provide something to sql request? e.g. params.ServiceId?
or this data is visible from frontend as request to backend?

Copy link
Contributor

@skamerintel skamerintel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI suggesting that '?' is not valid for clickhouse

@jwintel
Copy link
Contributor Author

jwintel commented May 30, 2025

@jwintel how user can provide something to sql request? e.g. params.ServiceId? or this data is visible from frontend as request to backend?

@dkorlovs That's a good question actually. I tested this a bit and from what I've seen so far the arguments passed to the backend in the payload are serviceName, endTime, startTime, and a boolean or two:

image

But the times get passed through various date methods which would break if you stuck sql in them, and the serviceID is never one of the arguments--that name to ID translation must happen somewhere else within the backend, before the clickhouse interface.

What concerns me are the hostname and container name and deployment name parameters. I don't have an example that I can test from unfortunately, but there is a filter where the user can pick these.

They are supplied by the backend, but presumably if the user sets this filter it would include them in a backend call because the clickhouse code does use these as query parameters. So I think there is still a potential vulnerability.

@jwintel
Copy link
Contributor Author

jwintel commented May 31, 2025

AI suggesting that '?' is not valid for clickhouse

Yes, it appears this breaks the backend. Working on another solution.

@jwintel
Copy link
Contributor Author

jwintel commented Jun 2, 2025

@skamerintel

Found the documentation for the Go SQL interface, the '?' looks acceptable:

https://pkg.go.dev/database/sql#DB.Query

But there are internal server errors being returned. Further debugging is needed to determine why.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants