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

Quotes shouldn't be escaped #220

Closed
tacoverdo opened this issue Jan 20, 2016 · 9 comments
Closed

Quotes shouldn't be escaped #220

tacoverdo opened this issue Jan 20, 2016 · 9 comments
Labels
[Priority] High [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature is broken.
Milestone

Comments

@tacoverdo
Copy link

Problem description
When adding a translation that contains quotes, or when filtering on something containing a quote, the quote is automagically escaped. This can lead to problems in the translated languages.

Example
translations___dutch___yoast_seo__for_wordpress____glotpress
The entered translation is Het focus-zoekwoord '%1$s' komt niet voor in de pagina titel.. It's added as expected. However, when going back to the string, or when looking at the waiting strings, it shows as
Het focus-zoekwoord \'%1$s\' komt niet voor in de pagina titel..
(link to live example)

That's annoying, but in this string, things are getting problematic:
Het redirect-type is de HTTP respons code die naar de browser wordt gezonden om aan te geven welk type redirect er wordt uitgeserveerd. <br/><br/>Lees <a href=\'%s\' target=\'_blank\'>deze pagina</a> voor meer informatie. means the HTML is affected and thus causes invalid HTML output.

Possible fix
Do not escape every input.

Tested versions
WordPress 4.4.1
GlotPress plugin 1.0

@ocean90 ocean90 added [Type] Bug An existing feature is broken. [Priority] High labels Jan 20, 2016
@ocean90 ocean90 added this to the 1.0.1 milestone Jan 20, 2016
@ocean90
Copy link
Member

ocean90 commented Jan 20, 2016

Thanks @tacoverdo!

It's added as expected.

Have you checked the database entry? It should be escaped there which is wrong.

@ocean90
Copy link
Member

ocean90 commented Jan 20, 2016

Changing https://github.com/GlotPress/GlotPress-WP/blob/7d805093cb79edbce019cf75d8525f096c24bfb2/gp-includes/routes/translation.php#L179 to

if ( isset( $translations[$i] ) ) $data["translation_$i"] = wp_unslash( $translations[$i] );

should prevent the extra slashes.

This requires some more testing/investigation as it probably affects all POST actions.

@toolstack
Copy link
Contributor

Agreed, BackPress probably didn't enable magic quotes and WordPress does so we'll have to check any time we get post/get data.

@ocean90
Copy link
Member

ocean90 commented Jan 20, 2016

From https://glotpress.trac.wordpress.org/browser/trunk/gp-settings.php#L76

// alleviate the magic_quotes_gpc effects
if ( get_magic_quotes_gpc() ) {
    $_GET    = stripslashes_deep( $_GET );
    $_POST   = stripslashes_deep( $_POST );
    $_COOKIE = stripslashes_deep( $_COOKIE );
}

@ocean90
Copy link
Member

ocean90 commented Jan 20, 2016

From https://github.com/WordPress/WordPress/blob/ededb78efc523cd48d8f94dfce0a8425ace55027/wp-includes/load.php#L606

/**
 * Add magic quotes to `$_GET`, `$_POST`, `$_COOKIE`, and `$_SERVER`.
 *
 * Also forces `$_REQUEST` to be `$_GET + $_POST`. If `$_SERVER`,
 * `$_COOKIE`, or `$_ENV` are needed, use those superglobals directly.
 *
 * @since 3.0.0
 * @access private
 */
function wp_magic_quotes() {
    // If already slashed, strip.
    if ( get_magic_quotes_gpc() ) {
        $_GET    = stripslashes_deep( $_GET    );
        $_POST   = stripslashes_deep( $_POST   );
        $_COOKIE = stripslashes_deep( $_COOKIE );
    }

    // Escape with wpdb.
    $_GET    = add_magic_quotes( $_GET    );
    $_POST   = add_magic_quotes( $_POST   );
    $_COOKIE = add_magic_quotes( $_COOKIE );
    $_SERVER = add_magic_quotes( $_SERVER );

    // Force REQUEST to be GET + POST.
    $_REQUEST = array_merge( $_GET, $_POST );
}

😢

@toolstack
Copy link
Contributor

Yea, I remember taking those out as we didn't want to mess with the global.

Fortunately it looks like it should be an easy fix as gp_post() is used to get post data and we can add the strip slashses here https://github.com/GlotPress/GlotPress-WP/blob/master/gp-includes/misc.php#L10

@toolstack
Copy link
Contributor

...and in gp_get() as well.

@ocean90
Copy link
Member

ocean90 commented Jan 20, 2016

Yeah, that should work. Although some functions are accessing the superglobals directly, like $_COOKIE for the notices or $_SERVER.

@toolstack
Copy link
Contributor

Those should be relatively few and far between, scrubbing the code for them should be easy enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Priority] High [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature is broken.
Projects
None yet
Development

No branches or pull requests

3 participants