signoz
signoz copied to clipboard
SigNoz
Move db calls to prepared statements with context
Move all db calls to prepared statements and specifically with context if possible to make signoz more secure from sql injections.
A query should not be a string prepared from fmt.sprintf(...) if it has args to pass. We should try to avoid string formatting for args.
Hi @ankitnayan Please assign this issue to me, I will complete the query-service installation today and will start making the changes. Also since we go for the Prepared Statements, here we are only trying to address the insecurities that arise due to malicious SQL data literals.
https://github.com/jshiwam/signoz/commit/f18be5c9f7e1699199861f3ca2b19a63e633ea3c
Hi @ankitnayan please check these comments. I have added them for some clarifications, as my understanding of the whole flow of the project might not be good enough. Please reply on those comments and point out if I have identified all the statments that need to be chagned. Thanks.
@jshiwam I think you are going to the right direction. One suggestion is to apply changes to only a few APIs or maybe 1 single API to start with and get it merged and then keep on adding small PRs for a small set of each APIs.
@srikanthccv please have a look and guide if needed
Sure Actually I have made change to one of the functions and it is working, the testing is also complete. I will remove the comments from those files and only keep that single change and make the first PR.
@srikanthccv @ankitnayan GetService queries clickhouse and we use the clickhouse-driver Conn in order to query the data there. I am not sure if they provide a way to use Prepare Statements. I am new to clickhouse so might be missing something. Please let me know if my observation is correct.
Best regards, the issue is based on changing the calls to the databases by SPS?
Hey @ankitnayan @srikanthccv do we have to migrate the db calls which directly call Exec without preparing the Query Like, the following?
