Enforce wp_slash() on functions that expect slashed data #172
Comments
|
The update_post_meta function will also stripslashes: So i'm not sure if all access to $_GET, $_POST, etc. should pass through wp_unslash(). |
|
Yeah, you're right. I'm tempted to sniff for |
|
Ok, that would also work for developers who are using the filter_input function. I'm not sure it's the best approuch, i can imagine that it's sometimes easier to write things like: update_post_meta( $post_id, 'test', $_POST['test'] );It's indeed a pitty that this function expects slashed data. |
|
It is both a pity and a PITA. |
These values are actually being sanitized, but because `wp_unslashed()` is (rightly) being used, WPCS is flagging them. We’ll remove these comments once this is fixed upstream. See WordPress/WordPress-Coding-Standards#172 See #324
This updates the validated/sanitized input sniff to also check for slashing. This could have been made into another sniff instead, however, it would have required lots of duplicated logic and this sniff would need to be updated to accommodate the use of `wp_unslash()` anyway. Currently only `wp_unslash()` is recognized as an unlashing function, but this can be changed in the future if needed. The sniff currently requires that `wp_unslash()` be used *before* the data is passed through a sanitizing function. Sanitizing first and then wrapping that in `wp_unslash()` is not accepted. The error for missing the use of `wp_unslash()` is independent of the missing sanitizing function error, so an error will be given for missing use of an unslashing function whether or not a sanitizing function is used, and vice versa. Unslashing is not required when sanitization is provided via casting, or when certain sanitization functions are used which implicitly or explicitly perform an unslash or for which unslashing isn’t necessary. `absint()` implicitly unslashes, and `sanitize_key()` will remove slashes explicitly. And unslashing isn’t necessary when testing a value with `is_array()`. This list can be expanded in the future, and is configurable via the `customUnslashingSanitizingFunctions` property. See #172
I remember reading some of the discussions, but is there any documentation on the Codex or otherwise about this? |
|
Original ticket: https://core.trac.wordpress.org/ticket/21767 |
Yeah, we'll add a separate sniff that checks that the data is slashed for all functions that expect slashed data. |
westonruter
changed the title
westonruter
changed the title
To date, `WP_SEO::save_post_fields()` has not unslashed $_POST data before processing it or slashed it before passing it to the metadata API (where it is again unslashed). In practice, unslashing and slashing the data is the same as just leaving it. But unslashing $_POST data is a WordPress coding standard, and the metadata API has always expected slashed values. See also WordPress/WordPress-Coding-Standards#172.
|
Note that I still have some old code for a sniff here, that I just haven't finished yet. The sniff itself can be found in this gist: https://gist.github.com/JDGrimes/7a29ec88d533459345565ae3caabe7d2 It needs to be updated before it can actually be added to WPCS, but it does contain some research in regard to functions that require slashing and ones that don't. The list is incomplete, because basically what I've been doing is finding functions that require slashing, adding them to the list, and then running the sniff over core again. This reveals other functions that use those functions, and that therefore also require data passed to them to be slashed. It is really a mess, and it would be nice if we could get this sniff finished so that in the future we can prevent additional functions in core from inadvertently requiring slashed data. I'll try to take a whack at bringing it in line with the latest changes in WPCS soon. |
|
FYI, I'm working on this again. I hope to get an initial version finished this week. Currently I'm still analyzing core to find all of the functions that expect slashed arguments. It's a mess. I'll keep the discussion regarding that on the related trac ticket though. |
£10 says that you'll find a function which needs to accept either slashed or unslashed data depending on some logic inside the function. |
Yes. In addition to functions that accept an array, part of which must be slashed, and part unslashed. I also just ran into a function that used to pass an arg through |
|
Was just thinking that, I'd definitely not take that bet. |
|
Yes, it\'s a very important effort! |
Since WordPress forces global input vars to get magic quoted, any access to them should pass through
wp_unslash(). Likewise, any data data sent into a function that expects pre-slashed input should require an explicitwp_slash().For example,
wp_unslash()andwp_slash()could be enforced in situations like this:As a
WordPress-Extrarule, this will help enforce a discipline of unslashing, sanitizing, and slashing when slashing is required (e.g. inwp_update_post(),update_post_meta(), etc). It's easy to forget and for slashing to sneak in or to get stripped out,\\o/o/, yay.#395 implements the sniff for
wp_unslash()The text was updated successfully, but these errors were encountered: