buddypress icon indicating copy to clipboard operation
buddypress copied to clipboard

Add/wp unslash input sanitisation

Open dd32 opened this issue 1 year ago • 9 comments

Converting stripslashes( $_REQUEST ) and friends into wp_unslash( $_REQUEST ), and then adding appropriate sanitization of any data.

This is a work in progress, submitted as a PR for unit test autoruns.

Trac ticket: https://buddypress.trac.wordpress.org/ticket/9065

wp_unslash conversions was mostly done with sed without human review, this PR serves as that human review.

Input sanitization added in a few files:


This Pull Request is for code review only. Please keep all other discussion in the BuddyPress Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the WordPress Core Handbook for more details.

dd32 avatar Jan 10 '24 04:01 dd32

As one can see... this is fairly ugly checking for the data types everywhere.

This has redirected me back to https://core.trac.wordpress.org/ticket/22325 / https://core.trac.wordpress.org/ticket/18322 and experimenting with it: https://github.com/WordPress/wordpress-develop/compare/trunk...dd32:wordpress-develop:try/request-abstraction

It might be worthwhile taking that and making a bp_get() / bp_get_request() method that does this sanitisation that can later be deprecated for whatever ends up in Core.

dd32 avatar Jan 10 '24 08:01 dd32

Thanks a lot for your hard work on this @dd32 I'll review the PR asap and will try to include the fix in next minor release (12.1.0).

imath avatar Jan 10 '24 10:01 imath

@imath No urgency here IMHO, this isn't worth rushing, better to get it right and clean instead.

I only did the input sanitization on a few files in the PR, I'll add a note to the issue description for that. There's a bunch more work needed here IMHO.

dd32 avatar Jan 11 '24 00:01 dd32

Thanks for your message @dd32. I had a quick look and saw it was pretty heavy, is it ok if we fix it in next major instead of next minor (I'm always worrying about profiles.w.org)?

imath avatar Jan 11 '24 19:01 imath

is it ok if we fix it in next major

Totally, this isn't a 'light' PR, nor is it urgent.

The only reason for this PR was those warnings from pentesterrs, which I realised are fatals in PHP8, this kind of thing is a "problem" for a lot of plugins, BuddyPress isn't doing something horribly wrong that needs urgent-today-attention.

dd32 avatar Jan 12 '24 01:01 dd32