search

Qovery / Replibyte

Seed your development database with real data ⚡️
https://www.replibyte.com
GNU General Public License v3.0
4.17k stars 129 forks source link

Bugfix/wild backslash attacks again in insert stmnt #259

Open michalkutrzeba-odrabiamy opened 1 year ago

michalkutrzeba-odrabiamy commented 1 year ago

Database: PostgreSQL 13.7

Replibyte assumed that ' can be escaped by \ and ', but it can only be escaped by ', so this statement select 'test \\' test'; assumed as correct in Replibyte is incorrect in reality. And this select 'test test \'; assumed as incorrect is correct in reality.

select 'test '' test'; # ok
select 'test \\' test';
-- SQL:  select 'test \\' test'
-- unterminated quoted string at or near "'"
select 'test \' test';
-- SQL:  select 'test \' test'
-- unterminated quoted string at or near "'"
michalq commented 1 year ago

Ok I see now that this utils.rs are common for mysql and psql, escaping that works in mysql doesn't work in psql and vice versa, this need to be reconsidered.

evoxmusic commented 1 year ago

Hi guys, some tests are failing. Let me know if you need some help :) Thanks for your contribution

michalq commented 1 year ago

Hi, thanks, I'll left this PR as is for now, but it needs to be done differently and slash escaping must be with different strategy for mysql and postgresql.

I found yet another problem with EOF when in dump script find more than 49 new lines, this seems to be very wrong, at least in my dump there are places with such new lines and dump ends importing too early, for now I commented it out, but I'm not sure if this is ok.

And yet another problem with file order, looks like read_dir returns files in totally random way, so I implemented sorting here.

Example:

Name: ./local-datastore/dump-1674753282544/13.dump
Name: ./local-datastore/dump-1674753282544/3.dump
Name: ./local-datastore/dump-1674753282544/25.dump
Name: ./local-datastore/dump-1674753282544/24.dump
Name: ./local-datastore/dump-1674753282544/2.dump
Name: ./local-datastore/dump-1674753282544/12.dump
Name: ./local-datastore/dump-1674753282544/19.dump
Name: ./local-datastore/dump-1674753282544/23.dump
Name: ./local-datastore/dump-1674753282544/9.dump
Name: ./local-datastore/dump-1674753282544/15.dump
Name: ./local-datastore/dump-1674753282544/5.dump
Name: ./local-datastore/dump-1674753282544/4.dump
Name: ./local-datastore/dump-1674753282544/14.dump
Name: ./local-datastore/dump-1674753282544/8.dump
Name: ./local-datastore/dump-1674753282544/22.dump
Name: ./local-datastore/dump-1674753282544/18.dump
Name: ./local-datastore/dump-1674753282544/21.dump
Name: ./local-datastore/dump-1674753282544/7.dump
Name: ./local-datastore/dump-1674753282544/17.dump
Name: ./local-datastore/dump-1674753282544/16.dump
Name: ./local-datastore/dump-1674753282544/6.dump
Name: ./local-datastore/dump-1674753282544/20.dump
Name: ./local-datastore/dump-1674753282544/1.dump
Name: ./local-datastore/dump-1674753282544/11.dump
Name: ./local-datastore/dump-1674753282544/10.dump
evoxmusic commented 1 year ago

@michalq Do you think it's ready to be merged? (I didn't review it yet)

michalq commented 1 year ago

@michalq Do you think it's ready to be merged? (I didn't review it yet)

It definitely should not be merged, it fixes escape behaviour for postgres but at the same time it breaks mysql since there is one method for both dbs, needs more work.