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

revamp database escaping fixes #5178 #5180

Open
wants to merge 18 commits into
base: release-2.1
from

Conversation

Projects
None yet
6 participants
@albertlast
Copy link
Collaborator

albertlast commented Nov 29, 2018

Well the pr got more complex/intresting as expected...

The root cause is that mysql know two escaping symbole,
on the one side it know the sql default one '' (double single quote)
and on the other side it know \ blackslash.

This pr disable the last methode -> only one way exists now

Benefits:
the mysql and pg setup could be the same on displayfields (fix the issue)
the amount of \ you insert is the amount which you get back
the undecided smylie work with :-\ instead of two \
in mysql db_query the "// Remove the string escape for better runtime" needs to do this only for one way (otherwise we would had to add both ways) -> less complexity

issue: #5178

albertlast added some commits Nov 29, 2018

valid json for displayfields in postgres
Signed-off-by: albertlast albertlast@hotmail.de
added NO_BACKSLASH_ESCAPES to sql mode
agline mysql with default sql behavior -> \ is no escape char

Signed-off-by: albertlast albertlast@hotmail.de
fix mysql setup
replace \\ with \ in mysql setup file

Signed-off-by: albertlast albertlast@hotmail.de
Fix postgres smylie
Look like a issue which got never reported

Signed-off-by: albertlast albertlast@hotmail.de
fix upgrader
Signed-off-by: albertlast albertlast@hotmail.de
streamline the check with pg
Signed-off-by: albertlast albertlast@hotmail.de

@albertlast albertlast changed the title valid json for displayfields fixes #5178 revamp database escaping fixes #5178 Dec 2, 2018

@Gwenwyfar Gwenwyfar added the Database label Dec 3, 2018

albertlast added some commits Dec 14, 2018

Accidentally approved before completing review

@sbulen

This comment has been minimized.

Copy link
Contributor

sbulen commented Dec 25, 2018

My only concern with this (assuming it's been thoroughly tested) is disallowing backslash escapes in the connect string.

That is basically enforcing a new standard, & I'm not sure what might break as a result.

I am fairly certain some working mods will break.

@albertlast

This comment has been minimized.

Copy link
Collaborator Author

albertlast commented Dec 25, 2018

This change didn't affected cases like the connect string,
Because the backslash is used for php and not mysql.
This is another reason why this change is important to provide a better distinction between the cases.

Since the changes are easy to adept,
I see the mod topic no problem.

@albertlast

This comment has been minimized.

Copy link
Collaborator Author

albertlast commented Feb 16, 2019

@Sesquipedalian the related issue is marked as rc2,
but htis pr doesn't got marked.
what is here the plan?

@Sesquipedalian Sesquipedalian added this to the RC2 milestone Feb 19, 2019

Show resolved Hide resolved Sources/Subs-Db-mysql.php Outdated
Show resolved Hide resolved Sources/Subs-Db-mysql.php Outdated
better reading
Signed-off-by: albertlast albertlast@hotmail.de
@Sesquipedalian

This comment has been minimized.

Copy link
Member

Sesquipedalian commented Feb 20, 2019

Hm. I just noticed a few places in some of the older upgrade files where addslashes() appears to be used to escape SQL strings. Even without the changes in this PR, that's a bad idea and should be fixed. With the changes in this PR, those need to be fixed before this can be merged. Can you check these and fix any that need to be changed, @albertlast?

The files that need to be checked for possible problems:
upgrade_1-0.sql (lines 827 and 911)
upgrade_2-0_mysql.sql (lines 463–476, 1711–1721, and 2033–2034)
upgrade_2-0_postgresql.sql (lines 1081 and 1082)

@albertlast

This comment has been minimized.

Copy link
Collaborator Author

albertlast commented Feb 20, 2019

Postgres did already working fine,
no check is needed.
On the other files i will look into.

removed addslashes
Signed-off-by: albertlast albertlast@hotmail.de
@albertlast

This comment has been minimized.

Copy link
Collaborator Author

albertlast commented Feb 20, 2019

Should be fixed now.

@jdarwood007

This comment has been minimized.

Copy link
Member

jdarwood007 commented Feb 20, 2019

If we are changing the older upgrade files, we need to test upgrading from them.

albertlast added some commits Feb 22, 2019

better sql mode handling
Signed-off-by: albertlast albertlast@hotmail.de
ad quotes
Signed-off-by: albertlast albertlast@hotmail.de
fixing stuff
Signed-off-by: albertlast albertlast@hotmail.de
@albertlast

This comment has been minimized.

Copy link
Collaborator Author

albertlast commented Feb 22, 2019

from my pov the upgrade files for < 2.0 should be deleted here,
because no one is able to test this.

@Sesquipedalian
Copy link
Member

Sesquipedalian left a comment

Missing some single quotes.

Show resolved Hide resolved other/upgrade_2-0_mysql.sql Outdated
Show resolved Hide resolved other/upgrade_2-0_mysql.sql Outdated
add single quotes
Signed-off-by: albertlast albertlast@hotmail.de
@albertlast

This comment has been minimized.

Copy link
Collaborator Author

albertlast commented Feb 28, 2019

got fixed

@Sesquipedalian
Copy link
Member

Sesquipedalian left a comment

Another minor change needed

Show resolved Hide resolved Sources/Subs-Db-mysql.php Outdated
use array
Signed-off-by: albertlast albertlast@hotmail.de
@albertlast

This comment has been minimized.

Copy link
Collaborator Author

albertlast commented Feb 28, 2019

the array idea i like.

@jdarwood007

This comment has been minimized.

Copy link
Member

jdarwood007 commented Mar 1, 2019

While we can't test the 1.0 or 1.1 very well, we still officially support upgrading from yabsee 1.5 all way to the latest version. I do think for 3.0 we should drop this and make it a conversion for yabbse 1.5, SMF 1.0 and 1.1. Only support upgrading from 2.0 or 2.1. Just saying that by changing this, its possible we may introduce a bug with upgrading on those versions. You don't need to run the version with old PHP, just need the database imported into a database server to do the upgrade.

As a very small note, the original devs used single quotes over double quotes because of the slight speed improvements because of the variable handling that double quotes allows is slightly slower. I don't have information for modern PHP or know of a way to do a test on that though.

@live627

This comment has been minimized.

Copy link
Contributor

live627 commented Mar 9, 2019

To fix the relevant issue, ba60e14 and ca3c9dc need cherrypicked . IMO,t the rest off this PR should wait for the next version.

@live627 live627 removed this from the RC2 milestone Mar 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.