querybuilder
querybuilder copied to clipboard
sqlkata
Avoid SQL Injection in debug SQL
I was happily making some other contributions to the codebase when I decided to try testing for SQL Injection vulnerabilities. I was dismayed when all my single quotes were not escaped in WHERE clauses and nearly had a panic attack until I realized that the SQL validated in the majority of these tests is just a debug mashup that includes parameter bindings.
Nevertheless, I think it makes sense to escape single quotes properly even in the debug SQL strings, if only to avoid giving some other hapless maintainer a heart attack.
Thank you for this contribution, but as you've said that the ToString() method should only be used for debugging purposes, sanitizing input would encourage developers to use it for execution, what I am actually thinking of is to force some warning string to be sent before the generated SQL to broke it.
something like:
--- UNSAFE SQL: SELECT * FROM [Table]
I think there is still value in sanitizing the debug SQL. I've used the debug string before to spot check problems quickly by copying and pasting into SQL Management Studio. Purposely leaving single quotes unescaped feels like an odd omission that will only cause confusion.
Prefixing the string with UNSAFE SQL: is informative, but I think a little misleading. It may be perfectly valid SQL, but the reality is that the SQL you see is only for debug purposes and not what is being sent to the database server. Perhaps the comment prefix could be broadened and separated by a newline from the actual SQL? Something like,
-- Warning: This SQL is for debugging purposes only. Learn more at https://sometiny.url
SELECT * FROM [Table]
@freakingawesome @ahmad-moussawi , what's the status of this PR? I've noticed it too and this PR would help fix it
If I fix the merge conflicts, can we go ahead with the change?
I've fixed the merge conflict and added a note to the README regarding the purpose of ToString(). I was also going to add that warning comment to prefix all ToString() values, but it would have resulted in a huge number of changes for all tests, so I shied away from it. Perhaps the README warning is sufficient?