Document every function parameter that expects slashed data

[johnbillion]

There are several inline comments in core which state // expected_slashed, indicating that a variable's value is expected to contain slashed data.

This information should be documented in the corresponding function's parameter documentation, and any "parent" functions that call those functions.

It's more than likely that there are other places in core which expect slashed data which are not documented with // expected_slashed. These need to be identified (comments welcome).

[johnbillion]

@jdgrimes has done some work related to this: ​https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/172#issuecomment-321294881

[jdgrimes]

While I was at it I realized that the same is true for some filters: they expect the result to be slashed. Not to mention action and filter args that are passed slashed instead of unslashed. I don't recall if any of these were documented, but I'm guessing probably not.

[jnylen0]

"slashed data"

I wish we could rename this phrase. At the very least I hope what it means is explained pretty clearly somewhere, because it's not actually a "real" technical term.

[jdgrimes]

An old work-in-progress patch

[jdgrimes]

old-wip.patch​ is a jumble of slashing related changes that I had made a year or more ago when I was looking into this for the WPCS sniff. It includes some added/modified expected_slashed comments, as well as some changed uses of slashing where I discovered it was being done wrong. It also has // WPCS: slashing OK comments in places where I had reviewed a call to a function that expects slashed data and found that the data was handled properly. (These comments were mainly added so that the WPCS sniff wouldn't keep reporting those places as needing slashing.)

I don't know if the patch will still apply cleanly or not, haven't tried it yet. Everything in it probably also needs review; things could have changed or I might have been mistaken.

[jdgrimes]

I'm working on this again, and I should have a new patch and list of functions to share in the next few days. The implications of just a few functions that expect data slashed is so wide-reaching, just because so many other functions in core use them without slashing the data first. I'm still unraveling the whole web.

[jdgrimes]

Catch-all patch; probably needs to be split

[jdgrimes]

41593.diff​ is a snapshot of the changes relating to slashing that I've explored so far. It isn't intended to be committed as it is, or even really be used as the base for future patches. Probably the next step is really splitting it up.

Basically what I've done in the patch is identify a list of functions that expect slashed data, and fed them to a PHPCS sniff. I then ran that sniff over core, to check whether those functions were passed slashed data as expected in each place that they were used. This revealed more functions that expect slashed data because they pass args directly to these functions, and so on.

In the patch, I have added expected_slashed comments for some of these functions. I have also reviewed every place where these functions were used, and either added slashing if necessary, or a // WPCS: slashing OK comment where it isn't (this causes the sniff to longer issue an error there). The sniff is smart about detecting cases where slashing isn't needed, so in appropriate cases I've silenced the error by casting the value in question to an integer (the sniff then ignores that because integers don't need to be slashed). For cases where the value isn't an integer, I've erred on the side of slashing even if it might not be necessary, since it does no harm and decreases the chance of future breakage.

The WPCS sniff I've used to get this far can now be found here: ​https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/pull/1222

That includes a list of functions that expect slashed data.

However, I don't think that it is as simple as just documenting this. Ideally, all of these functions should have slashing-related unit tests, to verify that they do indeed expect slashed data. Only a handful of them actually do:

\Tests_Attachment_Slashes::test_wp_insert_attachment
\Tests_Comment_Slashes::test_wp_new_comment
\Tests_Comment_Slashes::test_wp_insert_comment
\Tests_Comment_Slashes::test_wp_update_comment
\Tests_Meta_Slashes::test_add_comment_meta
\Tests_Meta_Slashes::test_update_comment_meta
\Tests_Meta_Slashes::test_add_user_meta
\Tests_Meta_Slashes::test_update_user_meta
\Tests_Post_Slashes::test_wp_insert_post
\Tests_Post_Slashes::test_wp_update_post
\Tests_Term_Slashes::test_wp_insert_term
\Tests_Term_Slashes::test_wp_update_term
\Tests_User_Slashes::test_wp_insert_user
\Tests_User_Slashes::test_wp_update_user

(In the process I've written tests for a few of the additional functions, which I'll also attach here shortly.)

In some cases, a decision may need to be made as to whether a function should continue to expect slashed data, or whether it should be changed to not. Since this decision will need to be made on a case-by-case basis, I suggest that perhaps we should split this ticket, creating a ticket for each function or set of functions. This may also help to keep the patches to manageable size and scope.

Because some functions use other functions that expect slashed data, it would be necessary to begin with the functions that call wp_unslash() or stripslashes() directly, and then work outward from there.

On that note, I'd like to suggest that all uses of stripslashes() be replaced with wp_unslash() where this is appropriate. It simplifies things. If there are some places that wp_unslash() shouldn't be used though, it would be helpful for me to understand why.

I have more thoughts on particular functions, and I'll probably also think of some other things that I forgot to note, which I'll post tomorrow.

[jdgrimes]

A mess of unit tests for some of the functions that expect slashed data

[jdgrimes]

scratch_50.php​ is a raw collection of unit tests that I wrote for a few of the functions that expect slashed data. These can be used as a basis for patches as we go along here.

---

Next Steps

There is a bit more research that can be done here, but before we can really move too far forward there are some decisions that will need to start being made.

• How do we want to document these parameters that expect slashes, exactly? We'll have to decide that before patches can be made.
• Should we consider possible reasons why it might be good/OK to change slashing expectations in some cases?
• Do we want to commit // WPCS: slashing OK whitelisting comments to core, in the interest of making it easier to run this sniff over it in the future, and ensure more functions expecting slashed data aren't unintentionally added? And what steps do we want to take to reduce the need for these comments (using (int) casts [discussed above], calling wp_slash() where it may not be needed, etc.)?
• Do we want to work on a single ticket, or split this up into several tickets for different functions/groups of functions?

Once some of these things are answered, we basically need to go through all of the functions, write unit tests, make the decisions that need to be made, and prepare a patch that will document the function parameters as needing slashing and also fix all of the places in core that don't abide by that.

Perhaps some of this should be brought up at a core meeting? Now that some of the initial investigative work is done, it would be great to get more people involved. I think this is an effort that would be good to tackle early in 5.0.

[johnbillion]

#38035 was marked as a duplicate.

[TimothyBlynJacobs]

#56832 was marked as a duplicate.