-
Notifications
You must be signed in to change notification settings - Fork 215
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 the SQL Injection #330
Conversation
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #330 +/- ##
========================================
Coverage 99.67% 99.68%
========================================
Files 82 83 +1
Lines 5503 5643 +140
========================================
+ Hits 5485 5625 +140
Misses 12 12
Partials 6 6
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
func EscapeQuote(str string) string { | ||
type Escape struct { | ||
From string | ||
To string | ||
} | ||
escape := []Escape{ | ||
{From: "`", To: ""}, // remove the backtick | ||
{From: `\`, To: `\\`}, | ||
{From: `'`, To: `\'`}, | ||
{From: `"`, To: `\"`}, | ||
} | ||
|
||
for _, e := range escape { | ||
str = strings.ReplaceAll(str, e.From, e.To) | ||
} | ||
return str | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as a cornor case, abc\'def
will be escaped to abc\\\'def
, is this a correct result?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's correct. The sql injection always needs a quote to close the previous statement. So escape the quote is key work to prevent injection.
Hi @haoel, great to see the issue is addressed. Could you open a Github security advisory for the SQL injection vulnerability we found? |
@oxeye-daniel , thanks for the reminder, we have open a github security advisory at: GHSA-4c32-w6c7-77x4 please help review and let us know if anything is incorrect, as this is the first time we open such an advisory, thanks. |
Hi @localvar thanks a lot! No problem, you can go ahead and add me as editor for the advisory so I can suggest changes |
@oxeye-daniel , added |
The MySQL/PostgreSQL data checking could have the SQL injection problem, this PR tries to fix it by adding the quotes in SQL and escaping the quotes in configuration.