WordPress / playground-tools

GNU General Public License v2.0
138 stars 39 forks source link

Playground plugin: % signs lost during the import #246

Open adamziel opened 5 months ago

adamziel commented 5 months ago

This is what my Sandbox Site looks like:

CleanShot 2024-04-18 at 14 12 08@2x

I poked around a bit and the % is missing from flex-basis: 60% here:

CleanShot 2024-04-18 at 14 10 54@2x

adamziel commented 5 months ago

It's missing from the .sql database export

adamziel commented 5 months ago

Escaping values using wpdb prepare does something weird:

(await playground.run({
    code: `<?php require '/wordpress/wp-load.php'; var_dump($wpdb->prepare("60%")); `
})).text;
'string(68) "60{af9a6abec7c9b260e27aa2ace1c5391a7c553540c6e87b6723050d04230d874a}"\n'
adamziel commented 5 months ago

It's coming from the $wpdb->prepare call here:

https://github.com/WordPress/playground-tools/blob/9c42b734cf7a42c62d788c73bb19edbdb652d44c/packages/playground/src/playground-db.php#L18-L22

bgrgicak commented 5 months ago

I think that we should drop these prepares. The data is coming from SQL and going into SQL, if there is no way to change the data, we should be fine without prepares.

adamziel commented 5 months ago

@bgrgicak we still need to escape the values, say my database field contains a single quote or ends with a backslash.

adamziel commented 2 months ago

@dmsnell have you ever figured out an offline way of escaping values before interpolating them into MySQL queries? We're solving the problem of creating a dump.sql file in a way that works both in WordPress and in Playground.

dmsnell commented 2 months ago

@adamziel I'm not sure this is related to that problem. Are we generating these values from PHP directly? Could we have the SQLITE database dump them? That would be more ideal as a way of escaping the values.

Based on my incomplete understanding, it's possible to escape MySQL queries fully in the client, however, that also involves some exchange of information with the server; specifically a negotiation of the charset for the session/connection/table/and database default.

It may be possible here to parse each statement and prefix each one with a defined charset, then escape only those bytes which require it for that charset. That would depend, I believe, on ensuring that we split each SQL statement reliably.

This all relates to the set of experiments I want to conduct to resolve this once and for all. Recently I was speaking with @azaozz @dd32 and @aaronjorbin about this for Core, as if we can pull it off reliably we could then eliminate a round-trip to the database for nearly every query in WordPress

dd32 commented 2 months ago

Escaping values using wpdb prepare does something weird:

(await playground.run({
    code: `<?php require '/wordpress/wp-load.php'; var_dump($wpdb->prepare("60%")); `
})).text;
'string(68) "60{af9a6abec7c9b260e27aa2ace1c5391a7c553540c6e87b6723050d04230d874a}"\n'

wpdb::placeholder_escape() runs during the query process to process those. If you're using wpdb::prepare() but not having the query be made via WPDB in the same session, you should call the placeholder_esacpe method on the final query.

These exist to workaround some SQLi attacks that were caused by concatenating multiple prepare'd statements together, while also supporting various printf-style syntaxes https://core.trac.wordpress.org/ticket/41925