boonebgorges / invite-anyone

Invite Anyone - Expands on BuddyPress's invitation features
http://teleogistic.net/code/buddypress/invite-anyone
29 stars 29 forks source link

Invitation links invalid #146

Closed Jon007 closed 7 years ago

Jon007 commented 7 years ago

Several people reported on wordpress.org support forum and I reproduced the same, in the invitation messages the text appears as:

To accept this invitation, please visit PTURL%%

There is no URL instead the token %%ACCEPTURL%% appears to be replaced with PTURL%% in the body of the message.

The other links, to the inviter profile and to opt out of invitations work ok...

debugging... $iaoptions['addl_invitation_message'] is returning the string:

To accept this invitation, please visit PTURL%%

To opt out of future invitations to this site, please visit %%OPTOUTURL%%

to the token is invalid and cannot then be replaced

Jon007 commented 7 years ago

While the text can be changed by visiting the settings page at

/wp-admin/admin.php?page=invite-anyone

Unfortunately if the text is edited to %%ACCEPTURL%% then on save this is replaced with PTURL%%

Jon007 commented 7 years ago

in function invite_anyone_settings_check() %%ACCEPTURL%% stripped by Wordpress sanitize_textarea_field( and then saved as please visit PTURL%%:

s:117:"You have been invited by %INVITERNAME% to join the xxxx community.

Visit %INVITERNAME%\'s profile at %INVITERURL%.";s:23:"addl_invitation_message";
s:124:"To accept this invitation, please visit PTURL%%

To opt out of future invitations to this site, please visit %%OPTOUTURL%%";}

here it is caught in _sanitize_text_fields() :

    $found = false;
    while ( preg_match('/%[a-f0-9]{2}/i', $filtered, $match) ) {
        $filtered = str_replace($match[0], '', $filtered);

and strips out the matching values:

$match[0] = '%AC'
$match[0] = '%CE'

The intention of this code is to catch hexadecimal character escapes of the form %FF. Other parameters such as %%OPTOUTURL%% are not caught by this because they do not start with one of the characters A-F.

Well, that's the problem but, @boonebgorges can you give an opinion on the correct solution??

Oddly this can't be fixed in github, because github doesn't actually have this code, the github code for admin-panel.php only has empty function for settings check whereas the release version from wordpress.org has a 70 line function with a foreach .. select case ..:

function invite_anyone_settings_check($input) {
    return $input;
}
Jon007 commented 7 years ago

ok so this is security fix 1.3.16 released 2 months ago:

Security fix: Improved output escaping of user-provided content in the Dashboard and on the front end

the code for this is not in github but is in SVN: https://plugins.svn.wordpress.org/invite-anyone/trunk/admin/admin-panel.php

Please would you resync github and consider fix? After all this breaks email invitations which is the core functionality..

Noting that the Footer text cannot be changed by users (group admins), it is not general user input text, it is text input by site admin, who has many and easier opportunities to break their own system if they want to... then this extra check could be removed from this field, for example by moving the check up one line it would only apply to the default invitation message, which is editable by users:

            case 'default_invitation_message' :
                if ( function_exists( 'sanitize_textarea_field' ) ) {
                    $value = sanitize_textarea_field( $value );
                }
            case 'addl_invitation_message' :
            break;

This support thread refers: https://wordpress.org/support/topic/general-settings-problem/

boonebgorges commented 7 years ago

@Jon007 Thanks very much for doing the research here.

I'd prefer not to remove the sanitization altogether.

In c1b69df I've introduced a juggling act that seems to fix the problem. Can you verify?

I've also added an update routine in a75b90b that will fix strings in the database that have been improperly saved due to this bug. I think this is probably worth doing, but I'd value your feedback.

Jon007 commented 7 years ago

Hi, confirmed yes that tests out ok!

boonebgorges commented 7 years ago

Thanks! I'm going to push out an update.