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

Add string escape for values #9

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

christianruhstaller
Copy link
Contributor

This will escape the strings for single and double quotes.
Especially single quotes can lead to SQL errors and injections.

If the value in the database is null it
should also be null when inserting it again.
Before an empty string was generated and this
can lead to problems with indexes and constraints.

A combined unique index like locale,uri where the uri
is optional would throw an duplicate Error when you
insert multiple values like this:

'de_ch',''
'de_ch',''

The correct way to insert these:

'de_ch',null
'de_ch',null

And will not throw any errors.
@JamesStewy
Copy link
Owner

Hi and thank you for the PR.

Are there other special characters apart from single and double quotes that should be escaped?
I had a quick look into possibly utilising the SQL escaping that is automatically done when constructing a query but I couldn't figure out how to extract the final command as a string. Do you think that is possible or will we just have to escape it manually?

Finally would you mind rebasing this pull request on master and removing the commit from #11 as I have already merged it.

@christianruhstaller
Copy link
Contributor Author

I found these information in the documentation from MySQL: https://dev.mysql.com/doc/refman/8.0/en/string-literals.html
See table 9.1

@JamesStewy
Copy link
Owner

So, for example, do newlines work currently or will we need to escape them as well?

@christianruhstaller
Copy link
Contributor Author

christianruhstaller commented Jun 25, 2018

For newlines the query runs fine.
The sql dump will contain the newline as a newline in the file:

INSERT INTO test VALUES ('Newline Test
Newline');

MySQL should handle these cases correctly. But I don't know if tools and co. could choke on that.
mysqldump itself escapes the newline.
Navicat for MySQL and their dump escapes the newline also.

The safe approach here would be to escape the listed sequences in https://dev.mysql.com/doc/refman/8.0/en/string-literals.html

The question is if you want to use your own function or somehow use the built in query generator. But as your quick look suggests this will be trickier or not possible at all.

@JamesStewy
Copy link
Owner

The question is if you want to use your own function or somehow use the built in query generator

If this is possible (or becomes in the future) I would prefer to use the built in query generator. But for now lets just extend your escapeString function to include the list in Table 9.1 of the link you gave.

@tpae
Copy link

tpae commented Jul 9, 2018

Any updates on this PR? 🙇

Demitroi pushed a commit to sait/go-mysqldump that referenced this pull request Aug 7, 2018
@samsamm777
Copy link

Anything on this PR?

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.

4 participants