Skip to content

Conversation

@freakingawesome
Copy link
Contributor

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.

@ahmad-moussawi
Copy link
Contributor

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]

@freakingawesome
Copy link
Contributor Author

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]

@ahmad-moussawi ahmad-moussawi force-pushed the master branch 6 times, most recently from f9a6417 to a30bb49 Compare January 1, 2020 11:39
@ahmad-moussawi ahmad-moussawi force-pushed the master branch 7 times, most recently from 838f8ca to 47633a3 Compare March 14, 2021 14:51
@v15h41
Copy link

v15h41 commented Apr 9, 2021

@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?

@freakingawesome
Copy link
Contributor Author

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?

@freakingawesome freakingawesome force-pushed the disallow-sql-injection-in-debug-sql branch from 65dc982 to 1daa869 Compare April 9, 2021 14:14
@ahmad-moussawi ahmad-moussawi force-pushed the master branch 2 times, most recently from 748c2bf to bad6733 Compare June 12, 2021 03:48
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.

3 participants