Make WordPress Core

Opened 2 years ago

Last modified 2 years ago

#55456 new defect (bug)

Double escaping wp_user-settings

Reported by: phatkoala's profile phatkoala Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 2.7
Component: Users Keywords: has-patch dev-feedback has-testing-info needs-testing
Focuses: Cc:

Description

Fresh install of WordPress with no plugins and using default Twenty Twenty Two theme.

Either directly insert test data;

INSERT INTO wp_usermeta (user_id, meta_key, meta_value) VALUES (1, 'wp_user-settings', 'foo=1&bar=1');

Or activate this plugin;

<?php
/*
Plugin Name: Ampersand Test
*/
register_activation_hook( __FILE__, function(){
    set_user_setting( 'foo', 1 );
    set_user_setting( 'bar', 1 );
} );

The name of the attributes and their values are completely arbitrary and have no impact on the behaviour.

Now go to your "Profile" page (/wp-admin/profile.php) and press "Update Profile". Wait 5 seconds and repeat.

The value of wp-user_settings in the database and COOKIE is being double escaped (escaped on read and escaped on write).

Therefore the value of wp-user_settings does this;

  • foo=1&bar=1
  • foo=1&ampbar=1
  • foo=1&ampampbar=1
  • foo=1&ampampampbar=1
  • and so forth

Once the value of wp-user_settings becomes too long and/or combined with a user using the same browser with multiple logins, the length of the COOKIE(s) will become too large and the request header will be rejected by Apache/Nginx.

Change History (6)

#1 @azouamauriac
2 years ago

  • Keywords needs-patch added

Hi @phatkoala welcome to trac! Thanks for the report and the steps to reproduce it. I'm able to reproduce it.

Hope it will be fixed ASAP.

This ticket was mentioned in PR #2458 on WordPress/wordpress-develop by PhatKoala.


2 years ago
#2

  • Keywords has-patch added; needs-patch removed

Ampersands (&) are being double escaped in wp_user-settings. This bug fix adds a regular expression to remove escaped ampersands from wp_user-settings.

Trac ticket: https://core.trac.wordpress.org/ticket/55456

#3 follow-up: @costdev
2 years ago

  • Keywords dev-feedback has-testing-info needs-testing added
  • Milestone changed from Awaiting Review to 6.0
  • Version changed from 5.9.2 to 2.7

Test Report

Env

  • Server: Apache (Linux)
  • WordPress: 6.0-alpha-52448-src
  • Browser: Chrome 99.0.4844.51
  • OS: Windows 10
  • Theme: Twenty Twenty-One
  • Plugins: None activated.

Steps to reproduce

  1. Save the plugin code in this ticket's description to wp-content/plugins/ampersand_test/ampersand_test.php.
  2. Navigate to Plugins > Installed Plugins.
  3. Activate the Ampersand Test plugin.
  4. Navigate to Users > Profile.
  5. Open the database and navigate to {prefix}_usermeta.
  6. Filter for wp_user-settings.
  7. See that the value contains &ampfoo=1&ampbar=1.
  8. Refresh the Profile page.
  9. Repeat steps 5 and 6.
  10. See that the value contains &amp&ampfoo=1&amp&ampbar=1.

Cleanup

  1. Edit the database entry and remove &amp&ampfoo=1&amp&ampbar=1.
  2. Save.
  3. Navigate to Plugins > Installed Plugins.
  4. Deactivate the Ampersand Test plugin, then reactivate it.

Steps to test PR 2458

  1. Checkout PR 2458.
  2. Open the database and navigate to the {prefix}_usermeta table.
  3. Filter for wp_user-settings.
  4. See that the value contains &foo=1&bar=1.
  5. Refresh the Profile page.
  6. See that the value still contains &foo=1&bar=1.

Results

  1. Issue reproduced. ✅
  2. PR 2458 resolves the issue. ✅

Notes

  1. Introduced in [8784].
  2. Milestoning for 6.0 to get this some visibility.
  3. Adding dev-feedback to verify that this approach has no unintended side effects / BC breaks.
  4. Adding has-testing-info and needs-testing to get some tester creativity on this.

#4 in reply to: ↑ 3 @azouamauriac
2 years ago

Hi thanks for test report, and thanks @phatkoala for the PR

Replying to costdev:

  1. PR 2458 resolves the issue. ✅

He updated the PR, and when I tested it, here is the result: before the last PR: https://prnt.sc/Bej0-MVvbgSj ; after the last PR : https://prnt.sc/ZZ53foh-Xs3b; So I think you should use commit link instead of PR since it can be updated anytime and commiters can be involved in mistakes. I think it is this commit you have tested and agreed(I guess). I tested it too, and it fixes the bug, but I am not so fan of the way, while I am still thinking about how to fix it properly, I agree with the commit though.

  1. Introduced in [8784].

IMO the bug is due to this line in this function
WP_User::__get
(if you comment this line everything will work fine) introduced in [18597].
Also, just for information, I've made some tests on 4.9.20 version and the bug is not present there, while it's present in 5.8 where I've done some tests too.

This ticket was mentioned in Slack in #core by chaion07. View the logs.


2 years ago

#6 @costdev
2 years ago

  • Milestone changed from 6.0 to Future Release

With 6.0 RC1 tomorrow and as this ticket still needs more investigation, I'm moving this to the Future Release milestone.

Last edited 2 years ago by costdev (previous) (diff)
Note: See TracTickets for help on using tickets.