DOC: Add docs for read_sql to avoid sql injection
State: closed

- [ ] closes https://github.com/pandas-dev/pandas/issues/10899
- [ ] All [code checks passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#pre-commit).

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.


Labels:
name: Docs
name: IO SQL
description: to_sql, read_sql, read_sql_query

Comments:
Does this really matter for any other drivers? I think it is the responsibility of the driver to properly escape strings
> 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. 
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. 
> 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 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.
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 
> 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
I agree with Will's comment by just adding https://github.com/pandas-dev/pandas/pull/56546#discussion_r1439751772 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.
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.


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 https://github.com/pandas-dev/pandas/pull/56546#discussion_r1439751772
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.
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
Do you want to add an example with `params` in this PR @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.
Thanks @erichxchen