Remove stripslashes from API functions

[alexkingorg]

This ticket attempts to bring consistency to the way slashes are handled throughout the WordPress API functions that persist data.

Currently these API functions expect slashed data and strip slashes within their code. Most of the code that invokes these functions understands this and generally passes slashed data to them (sometimes calling add_magic_quotes() when necessary). Most of the code understands this, but not all. While auditing the code for the purposes of creating this patch I found a variety of places that do not slash data before passing it to the model functions as it currently should be done. Further, most developers do not think to manually add slashes to the data they programatically create and pass to these functions. Finally, it is not documented in the Codex that all data passed to these functions should be slashed before being passed in.

As such, we can generally consider such behavior a bug. There are a few potential caveats, which I will cover shortly.

Some of the functions that are being affected here:

• wp_insert_post()
• wp_update_post()
• add_post_meta() [and other meta functions that call add_metadata()]
• update_post_meta() [and other meta functions that call update_metadata()]
• wp_insert_attachment()
• wp_insert_term()
• wp_update_term()
• up_insert_user()
• wp_update_user()
• wp_new_comment()
• etc...

Point being, while it's cool that this stuff can work:

update_post_meta($post->ID, 'key', $_POST['key']);

It's not worth the cost of losing data when someone quite reasonably does something like this:

$post_data = array(
	'post_status' => 'publish',
	'post_type' => 'post',
	'post_title' => 'This is my title',
	'post_content' => 'Crazy with the backslashes \\\\\\',
);
wp_insert_post($post_data);

With the current code they would instead need to do this (to not lose data):

wp_insert_post( add_magic_quotes($post_data) );

Developers just don't think to do this, and they shouldn't have to.

Here's the real life example that got me diving into this:

$my_api_data = get_twitter_data();
update_post_meta($post->ID, '_my_meta_key', $my_api_data);

The data I was saving was in JSON format. I was putting it in like this:

{"profile_image_url":"http:\/\/a0.twimg.com\/profile_images\/17084282\/alex_king_normal.jpg"}

and getting it back liks this:

{"profile_image_url":"http://a0.twimg.com/profile_images/17084282/alex_king_normal.jpg"}

As you can imagine, json_decode() fails on this data.

The functions that do have examples around of passing $_POST data directly to it are the post meta functions. I think changing them might break a fair bit of custom code that is passing in slashed data. I've created wp_update_post_meta() and wp_add_post_meta() accordingly that do not expect slashed data and replaced all instances of update_post_meta() and add_post_meta() with these calls accordingly.

It's possible that we may need to do a similar treatment of the other meta functions (user, comment, etc.). I haven't put anything in place for this in the current patch.

One additional change that was needed was to the default kses filters on a few properties. wp-includes/default-filters.php

// Strip, trim, kses, special chars for string saves
foreach ( array( 'pre_term_name', 'pre_comment_author_name', 'pre_link_name', 'pre_link_target', 'pre_link_rel', 'pre_user_display_name', 'pre_user_first_name', 'pre_user_last_name', 'pre_user_nickname' ) as $filter ) {
	add_filter( $filter, 'sanitize_text_field' );
	add_filter( $filter, 'wp_filter_kses' );
	add_filter( $filter, '_wp_specialchars', 30 );
}

The wp_filter_kses filter has been replaced with wp_kses_data - thanks to @markjaquith for this tip here. I also applied this change to the filters initialized in wp-includes/kses.php:

function kses_init_filters() {
	// Normal filtering
	add_filter('title_save_pre', 'wp_kses_data');

	// Comment filtering
	if ( current_user_can( 'unfiltered_html' ) )
		add_filter( 'pre_comment_content', 'wp_kses_data' );
	else
		add_filter( 'pre_comment_content', 'wp_kses_data' );

	// Post filtering
	add_filter('content_save_pre', 'wp_kses_data');
	add_filter('excerpt_save_pre', 'wp_kses_data');
	add_filter('content_filtered_save_pre', 'wp_kses_data');
}

As this has security implications, it could use extra review and testing.

I changed wp_rel_nofollow() to no longer strip slashes then add them again (and it was adding them wrong, via a call to esc_sql()). Now you can pass unslashed strings to wp_rel_nofollow() without losing data as well.

I have not thoroughly tested XMLRPC calls yet, that is definitely a needed next step. The escape() method within the XMLRPC code looks hinky to me, likely some changes (as well as thorough testing )are needed there.

Obviously, this is a big change. I've done my best to test appropriately. I've tested via the admin UI and with simple code examples (attached) and so far things look good. Unit tests that exercise various slash conditions across all objects would be another smart addition to the mix.

Related: #18322, #20569

Whew! I think that's it for now - when you find something that breaks, please comment accordingly so it can be fixed/discussed. I expect there are likely a few edge cases I missed, but it's definitely time to get more eyeballs on this (as well as help testing).

[alexkingorg]

The patch

[alexkingorg]

Testing steps

[mbijon]

+1 => I've had exactly the same problem with JSON data

OTOH, this is a big change. Hate to drag this out to a later release, but I feel like this needs unit tests to prevent regression issues.

[scribu]

Introducing new, unslashed versions of functions, like wp_add_post_meta() and wp_update_post_meta(), doesn't seem like a scalable approach.

Also, plugins that correctly call add_magic_quotes() or stripslashes() before using the API, such as wp_insert_term(), will get extra slashes in their content.

[johnbillion]

-1 on introducing a slew of new functions just to address this issue.

The only time the current expected slashes behaviour is a problem is when the content you're passing to one of the affected functions contains a backslash which isn't escaping another backslash.

Might it be possible to run a regex within in each of these functions which tests for double backslashes and then treats the content as slashed or not slashed accordingly? Effectively allowing the functions to accept both slashed and non-slashed content?

[johnbillion]

Er, yeah, scrap that last comment. I just got out of bed.

[azaozz]

Sounds like we would need maybe_strip_slashes() and is_slashed() similar to how maybe_unserialize() and is_serialized() work.

[mbijon]

Replying to azaozz:

Sounds like we would need maybe_strip_slashes() and is_slashed() similar to how maybe_unserialize() and is_serialized() work.

I'm not sure if testing for slashed data is necessary -- sometimes data will need to be slashed & sometimes it won't. It would be up to a plugin or site author to decide that, which puts us right back to the current state of things where Alex calls it a bug.

I think I would rather see an extra parameter in the changed methods. With a boolean that defaults to off we could current default actions as-is, but allow someone handling JSON to easily toggle slash removal when needed.

This would resolve @scribu's very valid complaint, & provide a good modification path in the even the preferred default behavior needs to be the Alex' way instead of core's current.

[alexkingorg]

This is in no way limited to JSON data - any time you have strings with backslashes in them and pass them to the wp_insert_*() methods, you lose data. This is why it's a bug.

I don't believe it's possible to test for this data - strings with lots of backslashes are perfectly valid and are not an indicator of slashes having been added or not.

[azaozz]

Replying to alexkingorg:

I don't believe it's possible to test for this data...

Yes, it's hard to test. That's probably why there's no maybe_strip_slashes() yet.

The function addslashes which is used to add the extra slashes only adds them at four places:

"Returns a string with backslashes before characters that need to be quoted in database queries etc. These characters are single quote ('), double quote ("), backslash (\) and NUL (the NULL byte)."

so it's not impossible to determine if a string has been run through addslashes(), just hard to do.

Not sure what's the best solution. Doubling all functions that "expect slashed" (there are a lot of them) seems too drastic. Trying to get a potential is_slashed() to work right without being too slow (it would need to look at user cases) wouldn't be easy. Getting rid of addslashes on the superglobals doesn't seem wise...

[nacin]

Replying to azaozz:

so it's not impossible to determine if a string has been run through addslashes(), just hard to do.

It's pretty much impossible. You can definitively say that something is *not* slashed, but if it is slashed, you don't know whether it was manually slashed, or if the data itself was already slashed.

[nacin]

I don't know how helpful it is to focus on particular solutions — whether its extra functions or parameters doesn't matter much. What matters most is plotting a broad course to saner ground. Very similarly related is #18322. Yes, this will be a major paradigm change, but it is absolutely necessary.

We need to figure out what expected-slashed situations can safely be classified a "bug" (if core is doing it wrong, that's a good indication) versus a breaking-change. Possible example of the former: wp_insert_post(), given that unslashing can/should be moved to edit_post() and the like in admin/includes/post.php. Example of the latter: the metadata functions.

[nacin]

Just going to further emphasize that this needs copious amounts of unit tests. Every single function that is affected, for the complete testing strategy outlined, needs a whole group of tests. This can't go near core without tests, and it is going to be difficult to even understand many of the issues presented here without tests.

[alexkingorg]

There are a number of places in core that are "doing it wrong" based on the current slash assumptions. I'd really like to fix this in 3.5 if we can.

I think unit tests that exercise the model and controller functions (with and without slashed content) are the right next step - will work on creating an outline so others can assist easily.

Related: got PHPUnit installed and and the WP tests working on my new kit earlier this week. :)

[nacin]

Replying to alexkingorg:

The data I was saving was in JSON format. I was putting it in like this:

{"profile_image_url":"http:\/\/a0.twimg.com\/profile_images\/17084282\/alex_king_normal.jpg"}

and getting it back liks this:

{"profile_image_url":"http://a0.twimg.com/profile_images/17084282/alex_king_normal.jpg"}

As you can imagine, json_decode() fails on this data.

Worth pointing out (as pointed out to me by mdawaffe) that json_decode() doesn't fail on that data. JS object notation is pretty liberal when it comes to this. There are certainly good examples of how slash expectations can cause problems, but this isn't it. Needs unit tests. :-)

[mdawaffe]

Replying to nacin:

We need to figure out what expected-slashed situations can safely be classified a "bug" (if core is doing it wrong, that's a good indication) versus a breaking-change.

Is the goal the same for both: ditch the slashes?

[alexkingorg]

In looking through the unit testing code I notice that the create|update_object() methods for comments call $this->addslashes_deep() on the content they then pass to wp_insert|update_comment(). The same steps are *not* taken for the create|update_object() methods for posts, terms, users, etc. All of those tests are _doing_it_wrong() and lose data. If the unit tests passed data with slashes they would fail.

<soapbox>THIS IS WHY THIS PATCH IS IMPORTANT.</soapbox> :)

For now I'm creating new unit tests to explicitly test slash behavior (wrote the first one today - w00T!) in the "model" functions (wp_update|insert_*()) and the controller functions that pull content directly from $_POST (edit_post(), etc.).

[alexkingorg]

Here's a first unit test that handles the post object. It exercises the controller function (edit_post()) as well as the model functions (wp_insert_post(), wp_update_post()).

​https://gist.github.com/3756939

These tests fail without the patch being applied and succeed with the patch applied, so they are working as expected.

Some feedback on the approach and coverage here would be really helpful. Did I miss anything obvious? Are there any "should have" things that were omitted?

Assuming this is the right approach, I'll work on making sure we get the same sort of coverage on the other objects as well. We can track progress here (still needs to be detailed for other objects at this time):

​https://docs.google.com/spreadsheet/ccc?key=0AqJlMmZg82o_dGoxT2pvbTY2VmY3VWpscEZFb1dLSGc

[alexkingorg]

Here are unit tests for post meta - feedback requested:

​https://gist.github.com/3758299

[alexkingorg]

Here are unit tests for comments - feedback requested:

​https://gist.github.com/3758570

[nacin]

These look fantastic. Perfect, even.

The only feedback I have is in addition to @group slashes, also tag these with @ticket 21767. For as long as these tickets are open, the tests will be skipped, unless you run them with --group 21767. This will enable them to be committed before this ticket is fixed.

[alexkingorg]

Awesome, glad we're on the right track. Adding in that @ticket # now and will start generating some patches for the unit tests and attaching them here as well.

[alexkingorg]

I've attached an updated patch (current with trunk as of an hour or so ago), and created a ticket in the unit tests trac instance to begin tracking the tests there:

​http://unit-tests.trac.wordpress.org/ticket/131

[alexkingorg]

Updated to latest 3.5 trunk code.

[alexkingorg]

Ok, I believe we have reasonable unit test coverage added - notes are here:

​http://unit-tests.trac.wordpress.org/ticket/131

I'm also attaching the latest patch (against rev. 22107).

I think this is ready for acceptance testing and (hopefully) merging in!

[ryan]

Thinking that any function that strips GPC should strip at the very top into $post_data rather than situationally throughout the function. For example, edit_link().

[alexkingorg]

I didn't pay too much attention to links since they are going out, but that could definitely be made more consistent. I was thinking we should try to get the change in ASAP to have as long a testing cycle as possible. Would you instead like this to be revisited with a focus on consistency of implementation first?

[ryan]

I'd like to establish the pattern, even if for round two. And I'd like to avoid stripslashes[_deep] and introduce a wp_unslash() function. But that too could wait for a round two, should we decide to do this soon.

[nacin]

Here's a chat between Ryan and I from early this morning. We usually pull these into IRC but just got carried away, so pasting it in full here. Sorry for the lame formatting; it's not easy to make dialogue pretty here.

Nacin: A few years ago we bit the bullet and took slashing out of the options and transients APIs, and no one even blinked.
Nacin: There's a ton there. I'm looking at what we can cherry pick to see what kind of reaction we'd get, then push the rest in for 3.6.

Ryan: Looking at 21767. Not sure I dig adding stripslashes all over the place and forking API. It's switching out one mess for another.

Nacin: Any instance of stripslashes( $var ) where $var is not $_POST is a bad omen.
Nacin: Forking API I don't like either.
Nacin: In order to do this without adding stripslashes or forking the API, we'll likely need to do this in conjunction with some sort of GET and POST non-slashed helpers.

Ryan: Why bother forking the one set of functions when none of the others are forked?
Ryan: Simply removing slashes from the default flow does nothing for plugins.

Nacin: Practically none of them are currently slashing.

Ryan: They are if they too are using POST.

Nacin: "Given how inconsistent core WP itself is about slashing, I'm inclined to go with this. There will be some back-compat grief, but it's looking like more plugins are not escaping than are escaping. We'd be servicing the more popular expectation." (A wise man once said — ​http://core.trac.wordpress.org/ticket/12416#comment:10)

Ryan: A wise man who wasn't going to fork functions.

Nacin: Right. The heavy use of *meta( ... $_POST ... ) is really the only situation where we are stuck with an unclear path forward.

Ryan: I'd say the same about wp_insert_post().

Nacin: I'd be surprised if any WP.com code was doing that right.
Nacin: I mean, we don't. No one does.

Ryan: I dunno. I think the only way we do this is if we stop GPC slashing.

Nacin: That would be a SQL injection floodgate for plugins.
Nacin: If instead of stripslashes() everywhere, it was wp_unslash()?

Ryan: That would help. At least we could mark these things.
Ryan: 1298 wp_insert_post() calls on dotcom.

Nacin: A few hundred might be properly slashed by receiving postdata. If more than 10 are *explicitly* slashed, I'd be surprised.

Ryan: A random sampling of five were pulls from the db that were sent back without slashes.

Nacin: I will say that we did this for option values and I never saw a single bug report or complaint.
Nacin: How many people *really* have \' or \" in everyday content?

Ryan: I like $post_data = stripslashes_deep( $_POST ); at the top of functions.

Nacin: You're right, the big issue is $_POST data.

Ryan: And files. Core admin files that use GPC should just strip at the top instead of situationally.

Nacin: But that could then mean an API function tries to double-strip.

Ryan: Something to make it easier to switch to unslashed GPC?
Ryan: It can't be a copy though. It'd get out of sync.

Nacin: Nah, it'd be a magic object.
Nacin: WP::GET['key'], WP::POST['key']

Ryan: strip on the fly every time? Too slow.

Nacin: I mean, we're basically already doing that.

Ryan: True.
Ryan: Looking through the patch. Any function that strips GPC should probably strip at the very top.
Ryan: I'm coming around.
Ryan: Something like WP::GET/POST would be nice and clean.

Nacin: Hello WP class, nice to see you, time to make you more decorative.
Nacin: The problem with a magic object is that it does not solve $_POST going into _meta()

Ryan: True. Forking there seems okay. It smelled bad a first, but now I can see the reasoning.

Nacin: So I guess our next question is, can we actually get away with just dropping GPC slashing?

Ryan: probably 90% get away with it, 10% sql injection.
Ryan: We can always keep that in our pocket. This is a step towards that.
Ryan: Meanwhile, encourage WP::GET and leave the old GPC slashed.

Nacin: We already see a ton of SQL injection reports in plugins. Ultimately, they are already doing it wrong, and GPC slashing is saving them at the last possible moment. It won't affect any API calls or prepares.

Ryan: Would we want decorated WP::GET even if we stopped slashing GPC?

Nacin: The only way it'd be needed is if people needed a way to determine if GPC was slashed.
Nacin: Something as simple as function wp_gpc_slashed() { return false; } and then toggling that to true in a future release can offer an upgrade path.
Nacin: We'd need to commit this day one of a cycle, announce it before that, and probably do it in coordination with some proactive scans on the plugin directory.
Nacin: You know, we could solve this at the dev summit. I was going to recommend a dev chat, but there, we'd have everyone in the same room. This is exactly the kind of architecture decision we should be making at a summit.

[alexkingorg]

I think this is going to be a hard patch to keep up to date if it isn't merged in soon... what needs to happen for this to go in?

I will commit to going back and refactoring everything to make the $_POST to $post_data conversion at the top of the code sections and converting to a wp_unslash() function, but my preference would be to get this in now to start wider testing and make those changes as additional patches.

[mbijon]

Is there any way we could toggle wp_unslash() off by default?

I think add_theme_support() sets a precedent for this. Then this could go in sooner, but not affect anyone who isn't purposefully enabling it.

---

Why?

As much as I'd like this to go in because it seems right ... the plugin repo is well-aged. So the risk of a security hole that stays open a long time is high.

For preventing security issues manually: I can't imagine finding the time to do a full review & refactor of all plugins on our clients at work plus on my own sites. That's putting aside a few scores of out-of-touch clients who won't have the staff or budget to do updates themselves (multiply that by a few thousand active WP devs).

But if it's disabled by default then there's no need to have Alex maintain this patch long-term, and no need to time it for the beginning of a cycle. Plus, is a single cycle really enough to see even a large minority of plugins updated?

[rmccue]

As discussed at #wpcs, this looks like it's going to be 3.6-early (from memory, please correct me otherwise). We have a long-term plan for stripslashes, including adding an abstraction for superglobals: #22325

If anyone wants to summarise the discussion, please do, otherwise I will attempt to tomorrow.

[ryan]

Here's a somewhat unorganized dump of my current notes on this:

For 3.6, leave GPC slashed.

Introduce wp_gpc_unslash() (or whatever we want to call it) to conditionally unslash strings and arrays of strings if GPC is slashed.

Change core to expect unslashed data everywhere. Functions/methods/files that deal directly with GPC should do something along the lines of $post_data = wp_gpc_unslash( $_POST ) at the top. Functions that do not directly use GPC will have all slash adds and strips removed.

Plugins should move to using wp_gpc_unslash() before passing GPC strings to core API.

Plugins no longer have to worry about slashing data pulled from the DB before passing it back to core. Since most don't anyway, their code suddenly starts working correctly.

Plugins should start expecting all data passed to filters to be unlashed. Most do already, so this too starts working according to expectations.

The only abstraction is wp_gpc_unslash(). No fussing with abstraction of the GPC super globals themselves. The super globals are well known and just fine as an API. We just need to stop littering them with slashes.

With GPC staying slashed and core moving to expecting unslashed, this leaves one major back compat pain point: double slashing of data coming in from GPC.

So, we do a big push during 3.6 to get plugins using wp_gpc_unslash(). Many won't update, but those are the ones that are probably already insecure and badly slashed. The goal would be to get the most popular ones updated and establish the pattern.

In 3.7, turn off GPC slashing. All plugins using wp_gpc_unslash() keep working just fine. This allows us to push the bigger back compat pain -- bare SQL queries directly using GPC -- until after we've established the wp_gpc_unslash() back compat pattern and worked out any double slashing issues.

[ryan]

Thoughts on comment:36? The patches here already do most of the work.

I personally am not much interested in abstracting GPCS as in #22325. Once we stop slashing the super globals, a new API isn't really needed.

[rmccue]

Replying to ryan:

Thoughts on comment:36? The patches here already do most of the work.

I personally am not much interested in abstracting GPCS as in #22325. Once we stop slashing the super globals, a new API isn't really needed.

+1. If we don't need to abstract GPCS to execute this, let's avoid doing so. The plan as above sounds like the best approach, since it means we kill off the slashed data expectation as soon as possible.

(Raw SQL queries using GPC may be related to #21663)

[alexkingorg]

Replying to ryan:

Thoughts on comment:36? The patches here already do most of the work.

I think this is fine. Thanks for helping work through this Ryan!

[ryan]

Right on. Anyone want to get things rolling with a patch refresh?

[ryan]

Quick refresh

[ryan]

Unit test failures:

1) Tests_Meta::test_metadata_slashes
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Testsingleslash'
+'Test\singleslash'

/Users/ryan/Projects/unit-tests/trunk/tests/meta.php:134

2) Tests_Post_Meta::test_update_meta
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-''quote and \slash'
+'\'quote and \\slash'

/Users/ryan/Projects/unit-tests/trunk/tests/post/meta.php:222

3) Tests_XMLRPC_wp_editPost::test_edit_custom_fields
Failed asserting that stdClass Object (
    'meta_id' => '14097'
    'post_id' => '14085'
    'meta_key' => 'custom_field_to_delete'
    'meta_value' => '12345678'
) is false.

/Users/ryan/Projects/unit-tests/trunk/tests/xmlrpc/wp/editPost.php:194

[ryan]

Unit test fixes

[ryan]

That fixes all the unit tests except the xmlrpc ones. xmlrpc needs a full audit to get rid of $this->escape() and strip/addslashes().

[ryan]

Tidying

[ryan]

Introduce wp_slash()/wp_unslash() and start using them in ajax-actions.php and wp-comments-post.php

[ryan]

That gets a start on introducing and using two new slash functions. Any stripslashes() or stripslashes_deep() calls on GPC data should be converted to wp_unslash(). If a function makes lots of unlsash calls on $_POST data, consider adding $post_data = wp_unslash( $_POST ) at the top of the function and use $post_data instead of $_POST everywhere in the function. This does have the drawback that once wp_unslash() no longer unslashes, the $post_data assignment will be pointless. It's a matter of style whether we want to add wp_unslash() calls for each use of $_POST in a function or change $_POST to $post_data everywhere in the function.

[ryan]

Calls to esc_sql(), $wpdb->escape(), addslashes(), add_magic_quotes() on data passed to core API also need to be audited and probably removed. Escaping should be done with $wpdb->prepare() ( or update() and insert() ) right before the data goes to the DB.

[ryan]

More stripslashes/stripslashes_deep to wp_unslash conversion

[ryan]

There are still a couple hundred instances of stripslashes/stripslashes_deep. Most are run directly on GPCS, making them obvious candidates for wp_unslash conversion.

[ryan]

While we're about this we will need to audit all wpdb calls to make sure the queries are properly prepare()d. esc_sql() and $wdb->escape() should not be used in core code making queries. They are both weak. When they are used where addslashes/stripslashes is intended, they should be converted to wpslash/unslash(). If they are escaping a query that is going to the DB, prepare() should be used instead.

[dave1010]

Are any functions / methods going to be deprecated as part of this change or is that something for the future? Can $wpdb->escape() etc be deprecated in favour of using $wpdb->prepare()?

[ryan]

Replying to dave1010:

Are any functions / methods going to be deprecated as part of this change or is that something for the future? Can $wpdb->escape() etc be deprecated in favour of using $wpdb->prepare()?

Maybe. *_post_meta() will probably be deprecated in favor of wp_*_post_meta() or whatever we end up naming the no-slashes-expected variant. In IRC we tossed around the idea of adding meta methods to WP_Post instead of introducing new wp_ prefixed functions. It is likely time to deprecate $wpdb->escape() and esc_sql().

[ryan]

wp_xmlrpc_server::escape() skips over objects whereas wp_unslash()/stripslashes_deep() unslashes them. Something to keep in mind. wp_unslash() may need an arg to control unslashing of objects.

[ryan]

Do we want to wp_unslash() functions such as wp_get_referer() and wp_get_original_referer() that return GPCS values that are often stuck in attributes or continue leaving that to the caller? For now I'll leave it to the caller as is currently done.

[ryan]

default-widgets.php, esc_js(), wp_parse_str(), get_search_link(), delete_metadata(), wpmu_validate_blog_signup(), install_blog(), wpmu_welcome_notification(), wp_new_user_notification(), add_ping(), get_pages(), WP_Query::get_posts(), get_term_by(), and WP_Widget::update_callback() need closer looks. I skipped them for now.

I removed stripping from MS functions that stripped get_site_option() as well as MS email and message strings.

[ryan]

Single site and multisite unit tests pass except for xmlrpc. --group ajax isn't working so ajax unit tests have not been run. Light free form testing with slashes and quotes in content looks good. No warnings after clicking every menu in the site and network admins.

[ryan]

Remove some addslashes() calls

[ryan]

Not many calls to addslashes() remain. It is used in some inline js that perhaps should be using esc_js() instead. default-widgets.php still does some strip/add juggling with kses that can probably be replaced with a straight call to wp_kses_data as is now done in default-filters.php. query.php uses addslashes_gpc() in a few places.

addslashes() and stripslashes() should be retained in a few places. Notably the upgrade routines in wp-admin/includes/upgrade.php and in the slashing utility functions in formatting.php, wp-db.php, and kses.php.

[ryan]

Update the wp_xmlrpc_server::wp_editPost() stack. Tests_XMLRPC_wp_editPost now runs clean.

[ryan]

Remove all escape() calls from xmlrpc. Remove more stripslashes() calls elsewhere. Use wp_kses_post() instead of wp_filter_post_kses().

[ryan]

Having WP_Widget::update_callback() wp_unslash() post would allows removing stripslashes calls from the update() methods in widgets. First lets see how many widgets are bothering to strip slashes in update().

[ryan]

Changing wp_xmlrpc_server::escape() to do nothing could provide easy back compat for those who have added custom methods and actually remembered to escape.

[ryan]

WP_Widget::update_callback() already stripslashes_deep() $new_instance before it is passed along to each widget's update() handlers. The handful of places in default-widgets.php where stripslashes() is run in update() seem to be holdovers from pre WP_Widget days. Removing them and changing update_callback() to use wp_unslash().

[ryan]

Remove stripslashes() from WP_Widget::update() handlers.

[ryan]

wp_unslash() GPCS in class-wp.php so that query_vars and query_string are not polluted with slashes. This allows removing more stripslashes() calls.

[ryan]

wp_unslash() in wp_get_referer() and wp_get_original_referer(). Few places were stripslashing the return value.

[ryan]

addslashes() is used in inline js in admin-header.php, media_send_to_editor(), iframe_header().

[ryan]

addslashes() in ms-settings.php probably should become wp_slash(), however this would require moving wp_slash() out of formatting.php and into functions.php.

[ryan]

Does addslashes() in url_to_postid() need to remain?

[ryan]

Places that might need to prepare().

edit-comments.php, remove escape() calls
user-edit.php
WP_MS_Sites_List_Table::prepare_items()
meta_form()
site-users.php
check_comment()
wp_untrash_post_comments()
get_page_by_path()
_get_last_post_time()
_prime_post_caches()
get_objects_in_term()
WP_Tax_Query::transform_query()
wp_delete_term()
clean_term_cache()
_pad_term_counts()
count_many_users_posts()

[ryan]

Remove some $wpdb->escape() calls.

[ryan]

Wondering what havoc would ensue in plugin land if esc_sql() switched to doing a real escape rather than a weak escape. Core almost always uses it when actually constructing sql. This is an appropriate use. wp_create_user() uses esc_sql() to prepare two variables for passing to wp_insert_user(). addslashes() would have been a better choice. Those two calls are removed in the patches here since wp_insert_user() will no longer expect slashed arguments. esc_sql() is also used in addslashes_gpc(). esc_sql() calls $wpdb->escape(). All of this implies escaping for the DB when really it is just adding slashes for the purposes of GPCS back compat. addslashes_gpc() should likely just pull the code from $wpdb->escape() into itself so as to avoid implications that addslashes_gpc() is about db escaping.

[ryan]

Remove some esc_sql() calls.

[ryan]

I think this is ready for commit consideration. I still want to reduce the esc_sql() and $wpdb->escape() calls and possibly convert addslashes() to esc_js() in a few places, but those can wait for later rounds.

[ryan]

wp_signon() and the entire auth flow still need to be sorted, but I'm saving that for round two.

[ryan]

With changes from review in IRC

[ryan]

Every variable handle with wp_reset_vars() needs an audit. wp_reset_vars() will now start wp_unslash()ing.

[ryan]

In 23416:

Change all core API to expect unslashed rather than slashed arguments.

The exceptions to this are update_post_meta() and add_post_meta() which are often used by plugins in POST handlers and will continue accepting slashed data for now.

Introduce wp_upate_post_meta() and wp_add_post_meta() as unslashed alternatives to update_post_meta() and add_post_meta(). These functions could become methods in WP_Post so don't use them too heavily yet.

Remove all escape() calls from wp_xmlrpc_server. Now that core expects unslashed data this is no longer needed.

Remove addslashes(), addslashes_gpc(), add_magic_quotes() calls on data being prepared for handoff to core functions that until now expected slashed data. Adding slashes in no longer necessary.

Introduce wp_unslash() and use to it remove slashes from GPCS data before using it in core API. Almost every instance of stripslashes() in core should now be wp_unslash(). In the future (a release or three) when GPCS is no longer slashed, wp_unslash() will stop stripping slashes and simply return what is passed. At this point wp_unslash() calls can be removed from core.

Introduce wp_slash() for slashing GPCS data. This will also turn into a noop once GPCS is no longer slashed. wp_slash() should almost never be used. It is mainly of use in unit tests.

Plugins should use wp_unslash() on data being passed to core API.

Plugins should no longer slash data being passed to core. So when you get_post() and then wp_insert_post() the post data from get_post() no longer needs addslashes(). Most plugins were not bothering with this. They will magically start doing the right thing. Unfortunately, those few souls who did it properly will now have to avoid calling addslashes() for 3.6 and newer.

Use wp_kses_post() and wp_kses_data(), which expect unslashed data, instead of wp_filter_post_kses() and wp_filter_kses(), which expect slashed data. Filters are no longer passed slashed data.

Remove many no longer necessary calls to $wpdb->escape() and esc_sql().

In wp_get_referer() and wp_get_original_referer(), return unslashed data.

Remove old stripslashes() calls from WP_Widget::update() handlers. These haven't been necessary since WP_Widget.

Switch several queries over to prepare().

Expect something to break.

Props alexkingorg
see #21767

[ryan]

​[UT1215]

[nacin]

In 23419:

Do not unslash variables reset by wp_reset_vars(). Remove variables that have never been used in plugin-editor.php. see #21767.

[nacin]

Clean up wp_reset_vars() calls. Quite a bit of cruft that can be traced back to b2. Still to do: Use the referer API in user-edit, instead of manual global vars.

[Ipstenu]

I think this is related. I just svn'd up and was writing a post. All my apostrophoes and quotes are prefaced by a slash.

So it's became it\'s

[ryanduff]

Replying to Ipstenu:

I think this is related. I just svn'd up and was writing a post. All my apostrophoes and quotes are prefaced by a slash.

So it's became it\'s

Seeing this on the make blogs as well-- and IIRC, I think they're running trunk.

[Ipstenu]

Figured out it's a plugin (duh). Syntax Highlighter :) I left Viper an email.

[ryanduff]

Replying to Ipstenu:

Figured out it's a plugin (duh). Syntax Highlighter :) I left Viper an email.

Interesting. I had a feeling it may have been something in P2... or a plugin running on make. Thanks for the heads up. Drops the chances of it being a core issue significantly ;)

[scribu]

I'm getting extra slashes too now. Here's what my plugin is doing, in a nutshell:

$value = $_POST['my_plugin'];

$value = wp_filter_kses( $value );

update_metadata( 'post', $post_id, 'my_plugin', $value );

foo' becomes foo\' becomes foo\\\' becomes foo\\\\\\\' etc.

[nacin]

In 23445:

Remove unused variables reset by wp_reset_vars(). Many of these haven't been used since b2. see #21767.

[alexkingorg]

Replying to scribu:

I'm getting extra slashes too now

This situation is why I have advocated for new wp_update_postmeta() and wp_insert_postmeta() API methods (and included them in my original patch). The update/insert_postmeta() calls have too much existing documentation that show passing the $_POST data directly in as a value.

[scribu]

Actually, wp_filter_kses() also adds a pair of slashes.

[nacin]

Replying to alexkingorg:

Replying to scribu:

I'm getting extra slashes too now

This situation is why I have advocated for new wp_update_postmeta() and wp_insert_postmeta() API methods (and included them in my original patch). The update/insert_postmeta() calls have too much existing documentation that show passing the $_POST data directly in as a value.

We did add new wp_update_postmeta() and wp_add_post_meta() functions (the previous ones did not have the wp_ prefix), in [23416]. scribu, though, was using the lower update_metadata() directly, which was changed from expecting slashed data to expected unslashed data.

[alexkingorg]

Ah, yup! :)

[Viper007Bond]

Replying to Ipstenu:

Figured out it's a plugin (duh). Syntax Highlighter :) I left Viper an email.

Yeah, I need to set aside some time to track it down. I did a quick test but was unable to reproduce it locally while I could on my live blog. Weird.

[johnjamesjacoby]

bbPress has similar issues as scribu above. Added some support for 2.3 in ​[BB4771].

[ocean90]

#23563 was marked as a duplicate.

[Viper007Bond]

Replying to Viper007Bond:

Replying to Ipstenu:

Figured out it's a plugin (duh). Syntax Highlighter :) I left Viper an email.

Yeah, I need to set aside some time to track it down. I did a quick test but was unable to reproduce it locally while I could on my live blog. Weird.

Fixed my plugin in ​http://plugins.trac.wordpress.org/changeset/672129/syntaxhighlighter

This ticket's commit certainly is backwards breaking for a plugin that was doing things correctly (stripping, modifying, adding).

[nacin]

Replying to Viper007Bond:

This ticket's commit certainly is backwards breaking for a plugin that was doing things correctly (stripping, modifying, adding).

​http://make.wordpress.org/core/2013/02/17/slashing-insanity/

As many have you have noticed, changeset r23416 (ticket #21767, Remove stripslashes from API functions) has caused issues with quite a few plugins. (For more, read the detailed commit message.)

Yes, trunk is very alpha right now. Yes, expect breakage.

These aren’t “bugs” in plugins per se. At least not yet. For now, plugin authors should be alerted to follow #21767.

We’re not jumping to revert things as @ryan and I want to take a wait-and-see approach on this. By going all-out initially, it provides us a lot of data to help us plot a modified course of action. We’re working to compile a list of grievances, so please, if something you were doing broke, please post specific code to #21767. We can then make adjustments to avoid major compatibility issues.

We strive to remain compatible from version to version, but sometimes it just requires dropping a commit bomb to see how certain changes will actually play out in the wild. Thanks for testing, and no need to panic. We’ll clean this up before beta. Bear with us, and help us out if you can!

[ryan]

We are changing tack on this. Functions such as wp_insert_post() that accept argument arrays will accept a "slashed" argument. slashed will default to true. Plugins and core functions that wish to pass unslashed data will need to pass slashed => false. When slashed is true, wp_unslash() will be run on the passed in data. When it is false wp_unslash will not be run. As I am still on the road, could someone patch wp_insert_post() to handle the slashed argument and have all core calls to wp_insert_post() pass slashed => false since we'd like to maintain passing unslashed data in core as a new standard? Once that is in, we can roll out to other functions that used to be slashed and get things back compat again. The new wp_update_post_meta() and wp_insert_post_meta() functions can be removed.

It is clear that we'll have to fork a bunch of functions to reach a slashless future. We'll probably take this opportunity to add methods to WP_Post and introduce WP_Comment. That's a discussion for 3.7, however.

Possibly back on the table for 3.6 is moving away from the GPCS supergloblals. It's looking like we may never be able to stop slashing GPCS, so let's consider something like WP:GET, WP:POST. Something light with just what we need. This can be discussed on #22325.

[alexkingorg]

Where can I read more about the issues that are causing the change in direction?

[ryan]

This broke p2 posts and comments and broke several popular plugins right off the bat.

[ryan]

slashed arg for wp_insert_post, wp_insert_attachment

[alexkingorg]

We knew there would be a small number of folks who were "doing it right" that would have to update their themes/plugins accordingly. I have several plugins I would need to change now as well. In discussing this with Nacin at WCSF, he put it this way "yes, but you'll update your plugins". I think he's right - people that knew enough to do it "right" by slashing stuff are pretty likely to update their code accordingly.

The mass benefit to the change was to "fix" the majority of usage which is incorrect. In talking with other WP devs about this issue at a number of WordCamps over the last year, I am pretty confident that most custom site build usage of wp_insert|update_post() is currently incorrect. Adding another parameter misses that mark.

I don't have the plugin repo checked out anymore (and won't be on a fast connection to do so for a week or so), but if someone could do a search for wp_insert|update_post through the full repo, it might give us a better sense of "general" usage. I do think much of the problem is a somewhat "hidden" failure though, as much of the usage I've discussed with people is in custom sites/implementations.

[ryan]

There's a ton of stuff coming straight from POST into API. Mass carnage. Not worth it. IMO. Either we stop slashing GPCS right now and take that pain or we use the arg until we can fork pretty much everything.

[ryan]

Revert 23416, 23419, 23445 except for wp_reset_vars() changes.

[ryan]

In 23554:

Revert 23416, 23419, 23445 except for wp_reset_vars() changes. We are going a different direction with the slashing cleanup, so resetting to a clean slate. see #21767

[ryan]

In 23555:

Introduce wp_slash() and wp_unslash(). This will be used to cleanup the myriad calls to addslashes*, add_magic_quotes, stripslashes*. see #21767

[ryan]

In 23556:

There is no need to strip the output of get_site_option().

see #21767

[ryan]

In 23563:

Use wp_unslash() instead of stripslashes() and stripslashes_deep(). Use wp_slash() instead of add_magic_quotes().

see #21767

[ryan]

In 23564:

Use prepare instead of escape.

see #21767

[ryan]

In 23570:

Always wp_unslash() the return of wp_get_referer().

see #21767

[nacin]

In 23573:

Remove double-strip on HTTP_IF_NONE_MATCH, which was done years ago (in #2597). see #21767.

[nacin]

In 23576:

Unslash early, directly on the superglobal. see #21767.

[nacin]

In 23577:

Remove an unslash in the deprecated WP_User_Search, as search_term is already unslashed in the constructor. see #21767.

[nacin]

In 23578:

Ensure the referer functions operate completely on unslashed data: wp_referer_field(), wp_original_referer_field(), wp_get_referer(), wp_get_original_referer().

Use wp_slash() instead of addslashes().

see #21767.

[nacin]

In 23580:

Don't unslash variables that came from wp_reset_vars(). see #21767.

[Viper007Bond]

EDIT: Nacin pinged me on IRC and let me know to revert. All good. :)

So from a plugin perspective, we're back to 3.5 for filters (slashes)? It's just some functions that will have a "I'm not passing slashed data" argument?

Trying to figure out if I need to revert my change to my plugin or hang tight. :)

[SergeyBiryukov]

In 23584:

Use correct variable. see [23575]. see #21767.

[ryan]

Replying to Viper007Bond:

EDIT: Nacin pinged me on IRC and let me know to revert. All good. :)

So from a plugin perspective, we're back to 3.5 for filters (slashes)? It's just some functions that will have a "I'm not passing slashed data" argument?

We're going back to 3.5 behavior all around with the new 'slashed' argument coming soon for those who'd like to pass unslashed data.

Trying to figure out if I need to revert my change to my plugin or hang tight. :)

Revert.

[ryan]

In 23592:

In ms-functions.php, remove unnecessary slashing, don't strip the return of get_site_option, s/stripslashes*/wp_unslash/.

see #21767

[ryan]

[23591]

[ryan]

In 23593:

Remove unnecessary stripslashes().

see #21767

[ryan]

​[UT1230]

[ryan]

In 23594:

Use wp_unslash() instead of stripslashes() and stripslashes_deep(). Use wp_slash() instead of add_magic_quotes().

see #21767

[SergeyBiryukov]

In 23595:

Use correct variable. see [23592]. see #21767.

[ryan]

slashed arg for wp_insert_post, wp_update_post, wp_insert_attachment

[nacin]

In 23925:

Remove dead code. Removed in [23445], accidentally reinstated in [23554]. This was found during wp_reset_vars() cleanup. see #21767.

[ryan]

Use prepare() in wp_allow_comment(). Barely tested.

[ryan]

In 23973:

Use prepare() for the duplicate comment query in wp_allow_comment().

see #21767

[ryan]

In 23974:

Use API instead of bare SQL queries in site-users.php.

see #21767

[ryan]

We cleaned up what we could and decided on a new direction. Resolving as fixed.

[nacin]

In 24713:

Use wp_slash() in places where we improperly used the DB API instead. see #21767.

[nacin]

In 24714:

Use sanitize_key() instead of esc_sql() when 'escaping' variable DB field names. see #21767.

[nacin]

In 24715:

More clear and concise escaping in get_page_by_path(). see #21767.

[nacin]

In 24716:

Use wp_slash() instead of the DB layer in XML-RPC. see #21767.

[nacin]

In 24719:

Avoid a sanitize_key() call on ID, as this causes it to be lowercased. wp_dropdown_users() requires user_login as a fallback; specify it for get_users(). see #21767.

[koke]

Since the switch to wp_slash, XML-RPC fails to parse dates. From the error log:

[18-Jul-2013 13:06:18 UTC] PHP Warning:  addslashes() expects parameter 1 to be string, object given in /srv/apache/wptest/wpt/wp-includes/formatting.php on line 3391
[18-Jul-2013 13:06:18 UTC] PHP Stack trace:
[18-Jul-2013 13:06:18 UTC] PHP   1. {main}() /srv/apache/wptest/wpt/xmlrpc.php:0
[18-Jul-2013 13:06:18 UTC] PHP   2. wp_xmlrpc_server->serve_request() /srv/apache/wptest/wpt/xmlrpc.php:69
[18-Jul-2013 13:06:18 UTC] PHP   3. IXR_Server->IXR_Server() /srv/apache/wptest/wpt/wp-includes/class-wp-xmlrpc-server.php:133
[18-Jul-2013 13:06:18 UTC] PHP   4. IXR_Server->serve() /srv/apache/wptest/wpt/wp-includes/class-IXR.php:364
[18-Jul-2013 13:06:18 UTC] PHP   5. IXR_Server->call() /srv/apache/wptest/wpt/wp-includes/class-IXR.php:391
[18-Jul-2013 13:06:18 UTC] PHP   6. wp_xmlrpc_server->mw_newPost() /srv/apache/wptest/wpt/wp-includes/class-IXR.php:441
[18-Jul-2013 13:06:18 UTC] PHP   7. wp_xmlrpc_server->escape() /srv/apache/wptest/wpt/wp-includes/class-wp-xmlrpc-server.php:4039
[18-Jul-2013 13:06:18 UTC] PHP   8. wp_slash() /srv/apache/wptest/wpt/wp-includes/class-wp-xmlrpc-server.php:227
[18-Jul-2013 13:06:18 UTC] PHP   9. addslashes() /srv/apache/wptest/wpt/wp-includes/formatting.php:3391

Running trunk r24727

As a side effect of this, user-provided dates are ignored and posts that should be scheduled are published immediately.

[nacin]

In 24731:

XML-RPC: Recursively escape arrays as before, to avoid stomping nested objects. fixes #21767.

[westonruter]

See related #35898: Customizer eats backslashes when sanitizing (including updates for nav menus and some widgets)

[boonebgorges]

In 36787:

Remove tests related to wp_*_post_meta() functions.

These tests functions were introduced as part of the attempt to straighten out
metadata slashing. See [23416]. The functions were later pulled out [23554],
but the tests were not removed at the same time. Since that point, they've
done nothing, so they can safely be removed.

See #21767. See #36016.