DOC: Add docs for read_sql to avoid sql injection

[erichxchen]

• closes read_sql should accept a sql_params parameter #10899
• All code checks passed.

Added examples and notes to the documents of pandas.read_sql (https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.read_sql.html) as a warning to avoid SQL injection.

[lithomas1]

Thanks for the PR, this looks correct at a first glance to me!

I left a small comment.

cc @WillAyd

[WillAyd]

Does this really matter for any other drivers? I think it is the responsibility of the driver to properly escape strings

[erichxchen]

Does this really matter for any other drivers? I think it is the responsibility of the driver to properly escape strings

Thanks @WillAyd. Yeah, I also think it is the responsibility of the driver to properly escape strings. When I test with other drivers, I do see that some of them will raise exceptions if there are multiple statements in one query.

But I think it would be nice to have a note/warning along with this API.

[WillAyd]

I think a note is fine but directing users to use params to solve this is not always the purpose of that argument, so I would remove any reference to that.

[erichxchen]

I think a note is fine but directing users to use params to solve this is not always the purpose of that argument, so I would remove any reference to that.

Removed the reference directing users to use params

[WillAyd]

I was thinking something along the lines of

pandas does not attempt to sanitize SQL statements; instead it simply forwards the statement you are executing to the underlying driver, which may or may not sanitize from there. Please refer to the underlying driver documentation for any details. Generally, be wary when accepting statements from arbitrary sources

Is all that we need to say

[datapythonista]

@WillAyd I do think we should be pointing users to use params. In the current version of this PR we are telling them what not to do, but we don't provide any solution. The right way to deal with user input in SQL is to use params. I don't think it's so useful to explain in the docs that regular string formatting is not great for SQL. I think we should simply mention that users should use params instead of string formatting, and show an example.

[WillAyd]

My hesitation in doing that is that we are making promises about params doing something that isn't true. I don't see anything in PEP 249 about params helping in this case and our code base does nothing to change that.

Is the promise of that avoiding injection made somewhere else? I am still under the impression that that would be driver-specific behavior

[WillAyd]

In the current version of this PR we are telling them what not to do, but we don't provide any solution

My latest feedback was that we should get rid of the DO NOT DO THIS comment and replace with a general warning around injection

[mroeschke]

I agree with Will's comment by just adding #56546 (comment) and showing an example of using param. AFAICT in the docstring we don't have one.

Since pandas essentially uses execute + user supplied input instead of ORM methods, pandas can't defend against SQL injection.

[datapythonista]

Makes sense. I guess pandas users are still expected to use params. Fully agree that we can't make promises on how that will protect from SQL injection. If what you both are proposing is to make clearer that for user input params should be used, and then warn about possible SQL injection and recommend users to check the driver documentation for more information, and see if they need to sanitize the output themselves, I fully agree on that.

[erichxchen]

Even though I still think it would be useful to add some examples to tell users what kind of actions are not safe, and suggest them to use params. But yes, we cannot make a promise for this.

I changed the docs to the general warning mentioned in #56546 (comment)

[datapythonista]

Thank you for the changes and adding the warning. I think adding an example of params and providing more info that makes clear that it's the way to provide data not directly hardcoded in the SQL is fine. I think the feedback above was to make sure we don't make users believe that using params will prevent SQL injection, which seems to be false for some drivers.

Maybe it would even be helpful to mention examples of drivers tbat do and don't sanitize params. I expected all them to do, I think it may be helpful for users to have an idea.

In any case, great changes, if you are happy with the PR as is, happy to get it merged, but I think you can add what you mention about params.

[mroeschke]

Yeah I would support an example in the Examples section that uses params noting it's the recommended way to parameterize a query over f-strings

[datapythonista]

Do you want to add an example with params in this PR @erichxchen ?

[erichxchen]

Do you want to add an example with params in this PR @erichxchen ?

Yes, I added an example with params in the Example Section.

In the example, I only say that using params is recommended over string interpolation. We do not make the promise that it will prevent the injection. So I think it should be okay.

[mroeschke]

Thanks @erichxchen