Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix for better escaping to avoid SQL injections #689

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pierresouchay
Copy link

Fix #238

Fix #366

@pierresouchay pierresouchay requested a review from a team as a code owner July 5, 2022 21:05
@pierresouchay
Copy link
Author

This is not perfect and will likely fail in some cases, first proposal

@AzisK
Copy link

AzisK commented Sep 25, 2023

Could you write tests for this?

# Did not find how to do that simply with pypika :(
return (
value.replace("\\", r"\\")
.replace(quote_char, quote_char * 2)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add something like this now, it should be configurable behind a feature flag. This gives off big breaking energy 🥲

Copy link
Author

@pierresouchay pierresouchay Oct 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, why not, how do you see it? A default option set to escape function by default on which you can override and put a no-op one?

I am here because I saw some SQL injections on our platform, IMHO, it should be applied by default

@AzisK
Copy link

AzisK commented Sep 29, 2023

Please also pull the newest code from master for the CI tests to pitch in

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.

Strings with trailing backslashes generate invalid SQL Enhance escape string
3 participants