-
-
Notifications
You must be signed in to change notification settings - Fork 492
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
JDGrimes
wants to merge
21
commits into
develop
Choose a base branch
from
issue/172
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 throughwp_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()
.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.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.
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.
(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