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

WIP: Sniff to check that slashed data is passed to functions that expect it #1222

Draft
wants to merge 21 commits into
base: develop
Choose a base branch
from

Conversation

JDGrimes
Copy link
Contributor

@JDGrimes JDGrimes commented Nov 3, 2017

⚠️ *** THIS IS A WORK IN PROGRESS, DO NOT MERGE! *** ⚠️

Background

In ancient PHP there was a feature called "magic quotes", that would cause escaping to automatically be applied to all $_GET, $_POST, and $_COOKIE data. When enabled, this feature would add a backslash (\) in front of every double-quote ("), single-quote ('), or backslash \ character.

This feature could be turned on or off on any particular server, so for a project like WordPress standardization was necessary. Unfortunately, WordPress made the fateful mistake of standardizing on having magic quotes always be turned on. Thus, the data in $_GET, $_POST, and $_COOKIE (and actually, in all input variables) is always "slashed."

Depending on the context in which this input data is then used, these slashes usually need to be removed. #172 proposed a sniff that would check that all input data was passed through wp_unslash(). This was done in #395.

However, early in WordPress's history, when slashing was the standard, some functions were written to specifically expect slashed data to be passed to them. (Today, these functions usually strip the slashes out with wp_unslash() internally.) Passing data to these functions without slashing it first will cause any intentional slashes to be stripped out. To avoid this, the data should be passed through wp_slash() before it is passed in.

This Sniff

The sniff in this PR ensures that all data passed to functions that expect slashed data is passed through wp_slash().

update_user_meta( $user_id, 'key', $value ); // Bad
update_user_meta( $user_id, 'key', wp_slash( $value ) ); // Good

Unfortunately though, it isn't always as simple as that.

First, there are many functions that accept an array of args (like get_posts()). In many cases, not all of these args need to be slashed, because they are integers or slugs that won't contain slashes anyway. This sniff is smart enough to detect cases like this, and will only issue an error when there are args being passed that need to be slashed.

get_posts( array( 's' => $search ) ); // Bad
get_posts( array( 's' => wp_slash( $search ) ) ); // OK
get_posts( array( 'post_types' => $types ) ); // OK
get_posts( $args ); // Bad
get_posts( wp_slash( $args ) ); // OK (but not really, see below)

Where things get really sticky is when some args are expected to be slashed, and some are specifically expected to not be slashed. In this case again, the sniff is smart about detecting partial slashing when it can.

get_posts( array( 's' => $search, 'meta_value' => $value ) ); // Bad
get_posts( array( 's' => wp_slash( $search ), 'meta_value' => $value ) ); // OK
get_posts( array( 's' => wp_slash( $search ), 'meta_value' => wp_slash( $value ) ) ); // Bad
get_posts( wp_slash( array( 's' => $search, 'meta_value' => $value ) ) ); // Bad
get_posts( $args ); // Bad
get_posts( wp_slash( $args ) ); // Bad

The sniff also warns when it finds a slashed string passed to a function that expects slashed data. Cases like this need to be manually reviewed, to ensure that the slashes are escaped.

update_user_meta( $user_id, 'key', 'slashed\\data' ); // Warning
update_user_meta( $user_id, 'key', wp_slash( 'slashed\\data' ) ); // Good

(Note: I just realized that this is also warning about strings that just contain quotes, which is incorrect. I'll fix that up in a new commit shortly.)

TODO

There is still more work to do here. Most of all, the function lists need to be double-checked, and may need to be changed based on decisions in core (I'll be posting a comment about next steps in this regard in https://core.trac.wordpress.org/ticket/41593).

There are also more features that could be added. For example, in the process I've discovered that some actions and filters receive slashed data. The sniff should probably detect whenever a function is hooked to any of these filters, and issue a warning.

I've gone ahead and pulled this here, in all its horror, to get some more eyes on the sniff implementation. Any suggestions in regard to the sniff itself are welcome; discussion of which functions belong in the list and whether any of them should be fixed is probably better kept to that Trac ticket though.

Fixes #172

⚠️ *** THIS IS A WORK IN PROGRESS, DO NOT MERGE! *** ⚠️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enforce wp_slash() on functions that expect slashed data
2 participants