Make WordPress Core

Opened 13 years ago

Last modified 9 months ago

#18322 reopened defect (bug)

The Road to Magic Quotes Sanity

Reported by: ryan's profile ryan Owned by: jorbin's profile jorbin
Milestone: Future Release Priority: normal
Severity: normal Version: 3.2.1
Component: Bootstrap/Load Keywords: needs-patch dev-feedback
Focuses: Cc:

Description

For back compat reasons, wp_magic_quotes() performs addslashes() on GPCS data. This is a pain, especially given that some core API expects slashes and some doesn't. In hopes of someday losing the automatic GPCS slashing, let's introduce a flag to turn off the slashing as well as slash and unslash functions that consult the flag. If slashing is on, these functions add and strip slashes. If slashing is off, they return data unchanged. Plugin authors can start using these functions and testing their code with GPCS slashing turned off and on. Eventually, GPCS slashing would default to off and all calls to the slash and unslash functions could be removed from core.

Attachments (3)

18322.diff (45.2 KB) - added by ryan 13 years ago.
Incomplete patch showing the gist
WP_Request.php (3.2 KB) - added by dd32 12 years ago.
18322.2.diff (6.4 KB) - added by NathanAtmoz 6 years ago.

Download all attachments as: .zip

Change History (56)

@ryan
13 years ago

Incomplete patch showing the gist

#1 @ryan
13 years ago

Patch introduces a $wp_magic_quotes global flag, which defaults to on. wp_magic_quotes() slashes all GPCS if the flag is on. wp_slash() and wp_unslash() also consult this flag. If on then addslashes() and stripslashes() are performed by wp_slash() and wp_unslash(), respectively. If off then they simply return the data without adding or stripping.

Last edited 13 years ago by ryan (previous) (diff)

#2 @nacin
13 years ago

JJJ would want me to recommend that these functions are named maybe_unslash() and maybe_slash().

I like this - let's consider encapsulating the global inside a function that can be called to A) set and B) get the value. As in, one less global.

#3 @scribu
13 years ago

+1 on maybe_*()

I like this - let's consider encapsulating the global inside a function that can be called to A) set and B) get the value. As in, one less global.

Static variable?

#4 @ryan
13 years ago

I thought about maybe. I'm not so convinced that I want to propagate maybe everywhere.

#5 @WraithKenny
13 years ago

  • Cc Ken@… added

#6 @CaptainN
13 years ago

  • Cc CaptainN added

#7 @CaptainN
13 years ago

I wrote a small proof of concept for an idea I had to make runtime compatibility patching easier. The idea is you would use an ArrayObject, and overwrite the $_GET, etc. vars, to make it easy to switch between slashed and unslashed in different contexts.

The problem with this approach is that it changes those vars to an object type, so they'll fail is_array and from what I've read also won't work in some places where array is required (through php 5 type hinting), so I didn't flush it out any further. I thought I'd share, in case there is any merit to the idea, or a PHP wizard more magical than me could smooth the edges (or if PHP changes their core to accept ArrayObject in places where array is currently required).

<?php
// From formatting.php ln: 1233

/**
 * Navigates through an array and removes slashes from the values.
 *
 * If an array is passed, the array_map() function causes a callback to pass the
 * value back to the function. The slashes from this value will removed.
 *
 * @since 2.0.0
 *
 * @param array|string $value The array or string to be stripped.
 * @return array|string Stripped array (or string in the callback).
 */
function stripslashes_deep($value) {
	if ( is_array($value) ) {
		$value = array_map('stripslashes_deep', $value);
	} elseif ( is_object($value) ) {
		$vars = get_object_vars( $value );
		foreach ($vars as $key=>$data) {
			$value->{$key} = stripslashes_deep( $data );
		}
	} else {
		$value = stripslashes($value);
	}
	
	return $value;
}

function addslashes_deep( $value )
{
	if ( is_array( $value ) ) {
		$value = array_map( 'addslashes_deep', $value );
	}
	elseif ( is_object( $value ) )
	{
		$vars = get_object_vars( $value );
		foreach ( $vars as $key=>$data ) {
			$value->{$key} = addslashes_deep( $data );
		}
	}
	else {
		$value = addslashes( $value );
	}
	return $value;
}

class WP_GPC extends ArrayObject
{
	// set the default magicness here
	public $magic_quotes = true;
	
	private $raw;
	
	public function __construct( $data )
	{
		$raw = $data;
		
		// Check ini setting, get rid of built in slashes
		if ( get_magic_quotes_gpc() ) {
			$data = stripslashes_deep( $data );
		}
		// set the props
		parent::__construct( $data );
	}
	
	public function offsetGet($offset)
	{
		$value = null;
		if ( parent::offsetGet( $offset ) )
		{
			if ( $this->magic_quotes ) {
				$value = addslashes_deep( parent::offsetGet( $offset ) );
			}
			else {
				$value = parent::offsetGet( $offset );
			}
		}
		return $value;
	}
}

header('Content-Type: text/plain');
ini_set("html_errors", "0");

print_r($_GET);

$_GET = new WP_GPC( $_GET );
$_POST = new WP_GPC( $_POST );
$_COOKIE = new WP_GPC( $_COOKIE );
$_REQUEST = new WP_GPC( $_REQUEST );

echo $_GET['some_query_var'];

$_GET->magic_quotes = false;

echo $_GET['some_query_var'];

?>

#9 @Mick P.
12 years ago

Please remember. From an end user point of view, the main problem is having to constantly unslash some data that is wanted to be pristine. The easiest way to deal with it is to overwrite the data in the array at some point, but it has to be after whenever WP does its version of Magic Quotes (assuming WP uses the same algorithm as PHP) but to do that you need to be certain that WP is doing its MQ, which you cannot assume it always will.

http://wordpress.org/support/topic/wp-automatically-escaping-get-and-post-etc-globals?replies=3#post-2368782

Last edited 12 years ago by Mick P. (previous) (diff)

@dd32
12 years ago

#10 @dd32
12 years ago

attachment WP_Request.php added

The ArrayObject work above replacing $_GET inspired my thought, so I've whipped this class up.

It doesn't deal with the functions expecting slashed or otherwise, but reinvents the wheel by giving a wrapper for the superglobals - but in doing so, adds functionality which most plugins require (is the variable set? Else we've got a default for that), along with extracting only a selection of the fields (ie. ?key=1&key2=2&key3=3 we could use GET::get( array('key', 'key3', 'key99'), array('key99' => 'Agent 99') ); to only get those 3 fields, droping back to the $defaults for unset data)

Quick fun half hour project to see what was possible with PHP5 magic.

#11 @markjaquith
12 years ago

Ryan — I think it's futile to hope for a future where we can flip magic quotes off. There's a non-negligible chance that we'll introduce security issues for the plugins that don't change. And you know they won't. So what's the problem we're trying to solve? I think you outlined it here:

This is a pain, especially given that some core API expects slashes and some doesn't.

So why not give them a new way to access those superglobals that is non-slashed by default? Proposal (for how you'd use it... you can infer the implementation):

$foo = _GET( 'bar' ); // OLD: $foo = $_GET['bar'];
_GET( 'bar', 'newvalue' ); // OLD: $_GET['bar'] = 'newvalue';

And so forth for the other magic-quoted superglobals. Yes, we're doing double work in terms of pre-slashing and then unslashing on access... but we are already. And this is simpler than remembering to wrap everything with stripslashes(). Also, we can do fancy stuff, if we need to, and filter superglobal access through this API. Don't have a use case for that... but we'd have the option.

We could do complicated stuff with implementing the ArrayObject interface, like Dion's code did... which gives us iteration, etc. But I don't think we need to. Frankly, even setting superglobals is sketchy.

#12 @ryan
12 years ago

I think of this primarily as an audit where we mark all core slashing and unslashing mischief with new functions. Being able to set policy is bonus.

#13 @ryan
12 years ago

Auditing and using maybe_slash/unslash() would always make clear where esc_sql() and wpdb:escape() are being used incorrectly. Those are fine in a maybe slash/unslash context, but not when wanting a real escape.

#14 @hakre
12 years ago

About a new API, I'd like to give some pointers: Probably it's feasibly to create a request object which abstracts and encapsulates the request properly:

$request = WP_Request::createFromGlobals();

The key point here is that you don't have static GET::/POST::/WHATEVER:: functions (e.g. replacing $_GET with GET::get()). That's more or less only offering a new interface for the same thing. Instead encapsulate what varies and offer a real interface in form of an instance. If you need a global access to it, put it in a global variable as it's common within Worpdress.

That's then one point instead of (additional) global (static class) functions.

I suggest to take a look into the API of the Symfony2 Request, the sourcecode or the docs for some inspiration.

The current dependency of wordpress is $_GET, $_POST, $_SERVER, $_COOKIE and $_REQUEST afaik. So the request class must not be equally evolved as the Symfony2 Request.

The key point is to start moving away from the (super)global dependencies. Then you can change things easier over time including the slashes domain.

You want to change that, the root problem aren't the quotes technically, but to decide whether they are in or not globally. If we remove the many superglobals by encapsulating them, the problem of the superglobal depency is gone, it's easy to set policy. And you're even able to test the application as the request can be injected. That will become crucial if you want to change the processing of slashes for input data.

And take care: Every level of indirection comes with a price, take it lightly.


For the status quo, I think it's a good idea to go through the functions and find out if the data that is processed by these functions is native/raw, slash-encoded or undefined (often called maybe). One way could be to add a docblock tag that documents it so you can run a simple static code-analysis over all files afterwards to index functions used and to get a better impression.

The parts that are maybe are a smell then, that the usage of the function is not clear.

Render a dependency tree of all functions that are calling a maybe function and it would be more visible how maybe affects the codebase and how large the problem is.

Related: #17018, #12402, #10360, #5791

Last edited 12 years ago by hakre (previous) (diff)

#15 @scribu
12 years ago

Related: #20569

#16 @johnbillion
12 years ago

  • Cc johnbillion added

#17 @scribu
11 years ago

Related: #22325

#18 @rmccue
11 years ago

Also related: #21767

#19 @aaroncampbell
11 years ago

  • Cc aaroncampbell added

#20 @andyadams
11 years ago

  • Cc andyadamscp@… added

#21 @sc0ttkclark
11 years ago

  • Cc lol@… added

#22 @tomdxw
11 years ago

  • Cc tom@… added

#23 @dave1010
11 years ago

  • Cc dave1010@… added

#24 @johnbillion
11 years ago

#24130 was marked as a duplicate.

#25 @thanatica2
11 years ago

So the biggest problem, realistically speaking, is that any sane standard test for magic quotes will always result in FALSE as of PHP 5.4, while magic quotes are still being applied. There seems to be no test for WP's magic quotes. So even in the future when WP fixes this, scripts that actually depend on it will fail. But I couldn't care less, because this is wrong behaviour. And wrong behaviour should not be maintained for the sake of old scripts and more bad code, not to mention performance hits. Just fix it and eat the consequences. You should have known it was coming to a point like this.

So here's a solution.

DO NOT apply magic quotes. Seriously. Remove them when PHP added them, but NEVER ADD THEM BACK. Quite simple really. I could even propose a piece of code that fixes everything. But it's so blazingly simple it shouldn't be neccesary.

#26 @rlange
11 years ago

addslashes()—called by add_magic_quotes(), which is itself called by wp_magic_quotes()—has the nasty side effect of silently converting integers to strings.

This is most obvious when working with $_SERVER['REQUEST_TIME'], which is an integer.

I had written code outside of WordPress that attempts to parse the given value using strtotime() if it's a string, or accept it as a timestamp if it's an integer. Unfortunately, strtotime() doesn't parse timestamps that are strings. This code failed when used within a WordPress site.

The workaround is simple enough, but I think this qualifies as unexpected behavior. A possible solution is for WordPress to check if the value is a string before applying addslashes().

#27 @westonruter
10 years ago

  • Cc weston@… added

#28 @Denis-de-Bernardy
10 years ago

  • Cc ddebernardy@… added

#29 @jdwire
10 years ago

  • Cc jdwire added

#30 @nacin
10 years ago

  • Component changed from General to Bootstrap/Load

#31 follow-up: @arman.poghosyan
10 years ago

  • Severity changed from normal to major

I was having issues with slashes in my plugin while sending JSON string in POST to manipulate object data in php. It's nonsense to have magic_quotes on in Wordpress and it's very hard to debug first time.

I'm using PHP > 5.4 both on local servers and production servers. Majority of shared hostings are already on PHP > 5.4, so magic_quotes are disabled and deprecated by default and nobody expects that it can cause problem.

Further checking magic_quotes in PHP with get_magic_quotes_gpc() always returns 0, so I couldn't even imagine that Wordpress could add quotes on its own.

I think it's time to change this behavior and or turn it off by default. Or intorduce some flag in config, that will make it obvious, that it's Wordpress issue.

#32 in reply to: ↑ 31 ; follow-up: @Denis-de-Bernardy
10 years ago

Replying to arman.poghosyan:

I'm using PHP > 5.4 both on local servers and production servers. Majority of shared hostings are already on PHP > 5.4, so magic_quotes are disabled and deprecated by default and nobody expects that it can cause problem.

It's closer to 10%-ish of hosts, actually… :-(

http://wordpress.org/about/stats/

#33 in reply to: ↑ 32 @arman.poghosyan
10 years ago

Replying to Denis-de-Bernardy:

Replying to arman.poghosyan:

I'm using PHP > 5.4 both on local servers and production servers. Majority of shared hostings are already on PHP > 5.4, so magic_quotes are disabled and deprecated by default and nobody expects that it can cause problem.

It's closer to 10%-ish of hosts, actually… :-(

http://wordpress.org/about/stats/

Are these charts up to date?
On all shared hostings that we use for our clients (I don't know if it is appropriate to name hosting companies here, but they are actually top (read most advertised and used) hostings)) PHP 5.4 is set up by default (on a few of them, PHP 5.2 is on, but you can easily change to 5.4 from CPanel) and on some of them you can even switch up to PHP 5.6.

On VPS and Dedis that we use we always update to latest stable version available (CentOS/RedHat default repository) and it's also PHP > 5.4.

Unfortunately this issue doesn't affect everybody even with lates PHP versions, as majority only uses GET and POST to update entries in MySQL database with $wpdb, in that case everything is always OK. But in case if you escape data on your own or if you use JSON data as me, you will for sure get acquanted with strange core behavior.

I think that it will be more appropriate to say that even if 100% use PHP > 5.4, only some 10% will be affected by the issue.

#34 follow-ups: @thanatica2
10 years ago

Why are we still discussing this? Just remove the magic quotes. They are not neccesary.

#35 in reply to: ↑ 34 ; follow-up: @aaroncampbell
10 years ago

Replying to arman.poghosyan:

Are these charts up to date?
On all shared hostings that we use for our clients (I don't know if it is appropriate to name hosting companies here, but they are actually top (read most advertised and used) hostings)) PHP 5.4 is set up by default (on a few of them, PHP 5.2 is on, but you can easily change to 5.4 from CPanel) and on some of them you can even switch up to PHP 5.6.

Yes, those charts are up to date. Even if 5.4 were the default on all hosts (and it definitely isn't), that would only affect new sites. All existing sites would still be on whatever the default was when they first signed up for hosting. Very few people ever change their PHP version unless a host forces them so, and hosts don't change it for you because it could break things. Unfortunately it will be quite a while until a majority of WordPress sites are on PHP 5.4+

Replying to thanatica2:

Why are we still discussing this? Just remove the magic quotes. They are not neccesary.

I know this is a pretty long ticket (especially if you account for all the related ones that are linked), but it's definitely worth taking the time to carefully read each comment. Currently magic quotes *are* necessary because removing them could easily open us to unexpected security vulnerabilities. And even if we fix all those in core, there would likely be hundreds (conservative estimate) of plugins that would be suddenly vulnerable because they were assuming slashed data and it wasn't.

I think we'd all like to get rid of the forced slashing, but we need to come up with a way to do it that doesn't result in thousands of vulnerable sites. We haven't yet found an elegant way to do that.

#36 in reply to: ↑ 34 @knutsp
10 years ago

Replying to thanatica2:

Why are we still discussing this? Just remove the magic quotes. They are not neccesary.

If you mean magic_quotes_gpc = Off in PHP (<5.4) that's the default, and the whole thing removed in PHP 5.4. That INI-setting doesn't affect WordPress. WordPress has it's own magic quotes function, if I have got it right. And then that usage is what's being discussed.

#37 @nacin
10 years ago

#27421 was marked as a duplicate.

#38 follow-up: @dhjw
10 years ago

Please put in a constant or function that can be checked to determine when slashes are added. Otherwise there is no elegant way to work around this issue and the hole just gets deeper and deeper as everyone continues to blindly assume quotes are added. As it stands, I'm doing

if(function_exists('wp_magic_quotes')) $gpc=true; else $gpc=false;
Last edited 10 years ago by dhjw (previous) (diff)

#39 @thanatica2
10 years ago

I know this is a pretty long ticket (especially if you account for all the related ones that are linked), but it's definitely worth taking the time to carefully read each comment. Currently magic quotes *are* necessary because removing them could easily open us to unexpected security vulnerabilities. And even if we fix all those in core, there would likely be hundreds (conservative estimate) of plugins that would be suddenly vulnerable because they were assuming slashed data and it wasn't.

You must be confusing PHP magic quotes with WP magic quotes. The latter ones must be removed, because they mess up plugins that assume PHP can be queried for magic quotes having been applied. This flag will be false even if they have been applied, because it's a WP function.

This is reinventing the wheel (the square wheel, imo) and therefore completely bollocks.

#40 in reply to: ↑ 38 @thanatica2
10 years ago

Replying to dhjw:

Please put in a constant or function that can be checked to determine when slashes are added. Otherwise there is no elegant way to work around this issue and the hole just gets deeper and deeper as everyone continues to blindly assume quotes are added. As it stands, I'm doing

if(function_exists('wp_magic_quotes')) $gpc=true; else $gpc=false;

That's what we have to put up with, because the WP devs have created this insane problem for themselves in the past. It should not be "our" problem to work around something like that.

#41 in reply to: ↑ 35 @najamelan
9 years ago

Replying to thanatica2:

Why are we still discussing this? Just remove the magic quotes. They are not neccesary.

I know this is a pretty long ticket (especially if you account for all the related ones that are linked), but it's definitely worth taking the time to carefully read each comment. Currently magic quotes *are* necessary because removing them could easily open us to unexpected security vulnerabilities. And even if we fix all those in core, there would likely be hundreds (conservative estimate) of plugins that would be suddenly vulnerable because they were assuming slashed data and it wasn't.

I think we'd all like to get rid of the forced slashing, but we need to come up with a way to do it that doesn't result in thousands of vulnerable sites. We haven't yet found an elegant way to do that.

I'm sorry, but you're turning things around imo. Messing with the raw php server variables is bad practice for anything but the most innocuous php script. People expect these variables to be pristine. The only reason they would not do so is because it has caused them troubles and they figured out it was WP messing with them. If plugin authors write broken plugins based on broken WP code, than there's not so much you can do, other than investing in making php less of a security hazard and education. However, if WP wants to move on from it's medieval heritage, it does need to fix stuff like this. If PHP can move away from magic quotes without breaking the internet, so can WP.

If you need slashed versions of these variables, you copy them into your own data structure and slash them. Say: $_GET_SLASHED. Right now the workaround for people struggling with this is the reverse, namely copying these variables into some safe haven before WP nukes them. You can do this in wp-config.php or in plugins, cause they are loaded before WP does it's "magic". Note: you can't do this in a theme, because they are loaded afterwards. (note that this means my plugin won't have security issues if WP get's rid of its magic quotes) If you want to publish a theme that needs pristine data, you're basically out of luck as far as I can tell, because stripping the slashes is also dangerous if you need some degree of slashing, albeit not an arbitrary degree (json_encoded data for example).

Now instead of slashing stuff ad nauseum, what WP really misses is a clean OOP escaping class. This is what I use: https://gist.github.com/najamelan/e2bc8ed92911537e4475. It's not finished, nor unit tested, so it's for educational purposes only.

#42 @chriscct7
9 years ago

  • Keywords needs-patch dev-feedback added
  • Severity changed from major to normal

#43 @chriscct7
9 years ago

#23905 was marked as a duplicate.

This ticket was mentioned in Slack in #core by johnbillion. View the logs.


8 years ago

#45 @dd32
8 years ago

#36438 was marked as a duplicate.

#46 @MikeSchinkel
8 years ago

Given how we have been bit hard by this issue, isn't it time to address if finally?

https://make.wordpress.org/core/2016/04/06/rest-api-slashed-data-in-wordpress-4-4-and-4-5/#respond

See #36438 for yet another solution and #22325 for a duplicate of this where much discussions was had.

#47 @dd32
7 years ago

#40476 was marked as a duplicate.

#48 @NathanAtmoz
6 years ago

There's been discussion that the super globals might become read-only in PHP8. While this is a long way off and certainly not a certainty, the idea of immutable super globals is intriguing.

The attached patch creates a new class that stores the super globals before wp_magic_quotes() is called, so the data is all unslashed. It also adds some helper function to get the data, but there's no way to modify the data - ensuring that the data is always unslashed.

This would allow core a way forward by offering an API to get the unslashed data but still maintaining the quoted super globals. Unsupported plugins that were written to work with the actual PHP super globals would still work and not have security compromised. Supported plugins could change to use the new API if they want to work exclusively with unslashed data.

In core, the new way to access the super globals would be:

// Old way
if( isset( $_POST[ 'var' ] ) ) {
  do_something_with( $_POST[ 'var' ] );
}

// New way
if( ! is_null( $var = _POST( 'var' ) ) ) {
  do_something_with( $var );
}

and it would be up to each individual case to decide if it should be slashed before using. This also has the added benefit of not quoting and unquoting unnecessarily.

The downside is obviously that there will be two sets of the super globals available: the actual super globals that have been slashed and the un-modified, unslashed, actually-not super globals.

@NathanAtmoz
6 years ago

#49 @jorbin
5 years ago

  • Milestone changed from Future Release to 5.3

PHP 7.4 is deprecating get_magic_quotes_gpc, which means we need to address this for 5.3.

#50 @jorbin
5 years ago

  • Owner set to jorbin
  • Resolution set to fixed
  • Status changed from new to closed

In 46105:

GENERAL: Remove magic quote functions

The path to magic quote sanity took a fun and exciting turn: PHP core removed it and WordPress updated the minimum version.

For the formally external pclzip, the code is commented out to make investigating easier and in case we ever need to merge upstream (if that still exists) changes.

Props ayeshrajans, jrf, jorbin.
See #47783.
Fixes #18322.

#51 @jrf
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

While the deprecation of the magic quotes functions allowed us to get rid of some of the code related to this issue, the patch which went into WP 5.3 does not address the real issue of the values in all superglobals being slash escaped with addslashes() by WP.

#52 @SergeyBiryukov
4 years ago

  • Milestone changed from 5.3 to Future Release

#53 @lucasbustamante
9 months ago

Is there any foreseeable impact to deprecate and stop calling wp_magic_quotes(); as part of the WordPress load process?

This way, we can stop requiring the use of wp_unslash() on every interaction with super globals like $_POST, $_GET, which adds complexity and becomes a barrier to the adoption of static code analysis tools such as PHPCS.

The goal is to make developers feel like quality tools such as PHPCS improve their code and worfklows without getting in their way with "meaningless nitpicks". I think the ecosystem has a lot to gain in making the development experience more enjoyable overall.

Note: See TracTickets for help on using tickets.