make addslashes_gpc() use addslashes() fix to use real_escape, rather than addslashes

[Denis-de-Bernardy]

If memory serves, the main problems with making wpdb->escape() point to mysql_real_escape_string() were two:

1. mysql_real_escape_string() doesn't work on all platforms
2. mysql_real_escape_string() doesn't play well with unslashing

Point 1 is covered in wpdb->_real_escape().

The legacy fix to point 2, for some strange reason, led to disabling mysql_real_escape_string() on platforms that could actually use it, instead of using add_magic_quotes() where calls to addslashes()/stripslashes() could occur.

r12961 partially fixes point 2. But only partially.

At the risk of re-opening the can of worms, the attached patch finishes fixing point 2...:

• It changes addslashes_gpc() and esc_sql() so that they use add_magic_quotes() instead.
• This frees wpdb->escape(), which can then be a true alias for wpdb->_real_escape().

I've scanned trunk for occurrences of wpdb->escape() used on arrays -- there were none.

[Denis-de-Bernardy]

Related: #11605

[miqrogroove]

I'll give this +1 if the patch makes esc_sql() deprecated or aliases weak_escape instead of magic_quotes. Otherwise, that can of worms is looking rather large.

[Denis-de-Bernardy]

Replying to miqrogroove:

I'll give this +1 if the patch makes esc_sql() deprecated or aliases weak_escape instead of magic_quotes. Otherwise, that can of worms is looking rather large.

I think that, in the long term esc_sql() should call wpdb->escape(). But for the sake of doing incremental fixes, however, I felt it was better to just keep esc_sql()'s behavior unchanged for now (which is exactly what the patch does).

We'll look into making esc_sql() call _real_escape() once this gets checked in. I think it'll largely be a matter of replacing most calls to esc_sql() into calls to add_magic_quotes().

[miqrogroove]

Replying to Denis-de-Bernardy:

keep esc_sql()'s behavior unchanged for now (which is exactly what the patch does).

Take another look at add_magic_quotes() then. It is not equivalent to what you're trying to do here.

[Denis-de-Bernardy]

err... I might be reading this wrong, but, in trunk at the time of writing this:

function add_magic_quotes( $array ) {
	foreach ( (array) $array as $k => $v ) {
		if ( is_array( $v ) ) {
			$array[$k] = add_magic_quotes( $v );
		} else {
			$array[$k] = addslashes( $v );
		}
	}
	return $array;
}


	function escape( $data ) {
		if ( is_array( $data ) ) {
			foreach ( (array) $data as $k => $v ) {
				if ( is_array( $v ) )
					$data[$k] = $this->escape( $v );
				else
					$data[$k] = $this->_weak_escape( $v );
			}
		} else {
			$data = $this->_weak_escape( $data );
		}

		return $data;
	}

both are poorly written versions of the same thing, i.e.:

function addslashes_deep($data) {
    return is_array($data) ? array_map('addslashes_deep', $data) : addslashes($data);
}

[Denis-de-Bernardy]

actually... nevermind... you're right. add_magic_quotes() only accepts an array, while esc_sql accepts anything.

I'd like to see some dev feedback before updating the patch, though...

[Denis-de-Bernardy]

see also #10041

[westi]

If my memory serves me correctly isn't one of the big issues with mysql_real_escape_string that it just doesn't work well for anything remotely non-ascii/UTF8 like Shift-JIS or Big5.

It is only a distant memory but I thought it worth adding to the discussion so someone could correct me if I was wrong :)

[Denis-de-Bernardy]

My understanding is the real issue was that we'd stripslash() $_POST and $_GET, and then slash them using mysql_real_escape_string(). but by doing so, we add more slashes than addslashes would, so doing the same over again for any reason would return garbage.

[ryan]

addslashes_gpc() should definitely be using addslashes() and not esc_sql(). Can we get that part into 3.0 and punt the rest?

[ryan]

Introduce addslashes_deep() and standardize on it.

[miqrogroove]

You guys are playing with fire here. I would punt it.

[hakre]

Just to make bold what miqrogroove just wrote: Blindly using "real_escape" does not help if there exists no function to revert it on the same data to use decoded values on various places. Currently stripslashes() is used to revert addslashes(). That won't work for real_escape() conceptually and the whole codebase must be changed from strispashes() -> real_unescape() of which later is missing (!).

Related: #14169

[johnbillion]

This was effectively dealt with in r23555 and r23591.