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

mysqli: escape raw string values #598

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@daniele-athome

daniele-athome commented Jan 2, 2017

I've implemented use of pwg_db_real_escape_string in cases where raw strings were injected into SQL code. This will prevent SQL injection and SQL syntax errors when having quotes in filenames (if allowed by the allowed filenames regex).

I've tested it functionally and it works, but I couldn't test it in the system, e.g. I don't know if some input values are subjected to addslashes before being used in SQL. It certainly works for sync (mass_inserts). Anyway I believe that any addslashes use should be removed when dealing with the database and we should always use the dbms escape tools.

Ref: #250, #544

Signed-off-by: Daniele Ricci daniele.athome@gmail.com

@plepe

This comment has been minimized.

Show comment
Hide comment
@plepe

plepe Sep 10, 2018

This branch looks good, and it works in my environment. How are changes that it will be merged soon?

plepe commented Sep 10, 2018

This branch looks good, and it works in my environment. How are changes that it will be merged soon?

@samwilson

This comment has been minimized.

Show comment
Hide comment
@samwilson

samwilson Sep 10, 2018

Contributor

This PR contains lots of commits that are not part of it, and also is not able to merge. It should be rebased and have only topical commits (probably just one).

(Not that I'm a maintainer or anything, but that's what I'd say if I were reviewing this.)

Contributor

samwilson commented Sep 10, 2018

This PR contains lots of commits that are not part of it, and also is not able to merge. It should be rebased and have only topical commits (probably just one).

(Not that I'm a maintainer or anything, but that's what I'd say if I were reviewing this.)

@daniele-athome

This comment has been minimized.

Show comment
Hide comment
@daniele-athome

daniele-athome Sep 11, 2018

My only commit is in fact 26aaea7, probably the upstream branch was rebased or history rewritten, therefore the commit mess. I'll create a new PR - or try to fix this one.

daniele-athome commented Sep 11, 2018

My only commit is in fact 26aaea7, probably the upstream branch was rebased or history rewritten, therefore the commit mess. I'll create a new PR - or try to fix this one.

mysqli: escape raw string values
Signed-off-by: Daniele Ricci <daniele.athome@gmail.com>
@daniele-athome

This comment has been minimized.

Show comment
Hide comment
@daniele-athome

daniele-athome Sep 12, 2018

I've rebased my modifications onto the current master.

daniele-athome commented Sep 12, 2018

I've rebased my modifications onto the current master.

@plepe

This comment has been minimized.

Show comment
Hide comment
@plepe

plepe Sep 13, 2018

With this commit active, synchronization of files which have a ' in their name does work, but on the other hand if you edit albums and add ' to a value (name, description), they will be saved escaped ("foo'bar" will turn into "foo'bar", save again and it will turn into "foo\'bar").

So, I used the commit while synchronizing and disabled it again afterwards, as everything seems to work as expected.

plepe commented Sep 13, 2018

With this commit active, synchronization of files which have a ' in their name does work, but on the other hand if you edit albums and add ' to a value (name, description), they will be saved escaped ("foo'bar" will turn into "foo'bar", save again and it will turn into "foo\'bar").

So, I used the commit while synchronizing and disabled it again afterwards, as everything seems to work as expected.

@daniele-athome

This comment has been minimized.

Show comment
Hide comment
@daniele-athome

daniele-athome Sep 13, 2018

With this commit active, synchronization of files which have a ' in their name does work, but on the other hand if you edit albums and add ' to a value (name, description), they will be saved escaped ("foo'bar" will turn into "foo'bar", save again and it will turn into "foo'bar").

So, I used the commit while synchronizing and disabled it again afterwards, as everything seems to work as expected.

I didn't try it on albums, thanks for the feedback. I'll test it out some more and check for more impacts. I thought it would have been too easy eheh

daniele-athome commented Sep 13, 2018

With this commit active, synchronization of files which have a ' in their name does work, but on the other hand if you edit albums and add ' to a value (name, description), they will be saved escaped ("foo'bar" will turn into "foo'bar", save again and it will turn into "foo'bar").

So, I used the commit while synchronizing and disabled it again afterwards, as everything seems to work as expected.

I didn't try it on albums, thanks for the feedback. I'll test it out some more and check for more impacts. I thought it would have been too easy eheh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment