Make WordPress Core

Opened 3 weeks ago

Last modified 22 hours ago

#61561 reviewing defect (bug)

autofocus query string parameter in customizer url is broken in WP6.6-RC2

Reported by: jamesros161's profile jamesros161 Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.6.2 Priority: normal
Severity: normal Version: 6.6
Component: Customize Keywords: has-patch has-testing-info changes-requested
Focuses: Cc:

Description

When using a query string parameter to autofocus in the customizer, it is broken by using the sanitize_text_field() function.

Example: customize.php?autofocus%5Bsection%5D=colors

Line 89 of /wp-admin/customize.php uses sanitize_text_field to sanitize the $_REQUEST['autofocus'] variable. However, this variable is an array, not a string. Therefore it will always return an empty string when being sanitized this way.

Change History (17)

#1 @dlh
3 weeks ago

  • Milestone changed from Awaiting Review to 6.6
  • Version set to trunk

Welcome back to Trac, @jamesros161! Thanks for the report.

I can replicate the problem, which appears to have been introduced in [58069].

I'm adding this to the 6.6 milestone for visibility, but I think a fix is more likely a candidate for 6.6.1 at this point.

This ticket was mentioned in PR #6974 on WordPress/wordpress-develop by Debarghya-Banerjee.


3 weeks ago
#2

  • Keywords has-patch added; needs-patch removed

Trac Ticket - Core-61561

## Summary

Fixes an issue with the autofocus query string parameter in the WordPress Customizer (customize.php). Previously, sanitize_text_field() was used on $_REQUESTautofocus?, which is an array, resulting in an empty string.

## Description

### Problem

The Customizer's autofocus functionality was broken due to incorrect sanitization of the $_REQUESTautofocus? variable. When sanitize_text_field() is used on an array, it returns an empty string.

### Example

customize.php?autofocus%5Bsection%5D=colors

### Solution

To resolve this issue, the keys and values of the $_REQUESTautofocus? array are sanitized separately and then combined into a new array.

###Changes Made

  • Sanitize the keys of the $_REQUESTautofocus? array using sanitize_key().
  • Sanitize the values of the $_REQUESTautofocus? array using sanitize_text_field().
  • Combine the sanitized keys and values into a new array using array_combine().

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


2 weeks ago

#4 @audrasjb
2 weeks ago

  • Milestone changed from 6.6 to 6.6.1

Given we're pretty close to 6.6 final release and as it's not a blocker for the release, I'm moving this ticket to 6.6.1.

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


6 days ago

#6 @hellofromTonya
6 days ago

  • Keywords needs-testing needs-testing-info added

Current status:

  • Needs testing instructions.
  • Needs a code review of the patch.
  • Needs testing (test reports).

Likely a candidate for 6.6.2, rather than 6.6.1.

#7 @hellofromTonya
6 days ago

  • Milestone changed from 6.6.1 to 6.6.2

@swissspidy commented on PR #6974:


6 days ago
#8

@hellofromtonya why me? 🤔

@swissspidy commented on PR #6974:


5 days ago
#9

Ah, because of https://core.trac.wordpress.org/changeset/58069. Context would have been helpful.

@debarghyabanerjee commented on PR #6974:


5 days ago
#10

Hi @swissspidy , I have addressed the feedback and made the necessary changes. Assigning you again for re-review.

#11 @swissspidy
5 days ago

  • Keywords commit added

#12 @hellofromTonya
5 days ago

  • Keywords has-testing-info added; needs-testing needs-testing-info removed

Test Report

Patch tested: https://github.com/WordPress/wordpress-develop/pull/6974

Environment

  • OS: macOS
  • Web Server: Nginx
  • PHP: 8.3
  • WordPress: 6.6.1-alpha-58737-src
  • Browser: Firefox 128.0
  • Theme: Twenty Twenty-One
  • Active Plugins: None.

Steps

  1. Open Customizer.
  2. Add the following URL params autofocus%5Bsection%5D=colors to the end of the URL, e.g. http://localhost:8889/wp-admin/customize.php?return=%2Fwp-admin%2F&autofocus%5Bsection%5D=colors).
  3. Observe if the Colors section opens.

Actual Results

When reproducing a bug/defect (before applying the patch:

  • ❌ Error occurs. Can reproduce.

When testing the bugfix patch (after applying the patch):

  • ✅ Issue resolved.

#13 @hellofromTonya
5 days ago

  • Milestone changed from 6.6.2 to 6.6.1
  • Owner set to hellofromTonya
  • Status changed from new to reviewing

Patch: https://github.com/WordPress/wordpress-develop/pull/6974

  • PR is approved by @swissspidy
  • Test report confirms before and after.

Moving this fix back into 6.6.1. Self-assigning for commit.

#14 follow-up: @hellofromTonya
5 days ago

  • Milestone changed from 6.6.1 to 6.7

Tested this on 6.5.4. Can reproduce the issue there too. So this bug was not introduced in 6.6 cycle. Aha. Moving it to 6.7.

Last edited 5 days ago by hellofromTonya (previous) (diff)

#15 @ironprogrammer
4 days ago

Test Report

Patch tested: https://github.com/WordPress/wordpress-develop/pull/6974 (I left a couple of suggestions on the PR so figured I'd drop in a test report as well).

Environment

  • Hardware: MacBook Pro Apple M1 Pro
  • OS: macOS 14.5
  • Browser: Safari 17.5
  • Server: nginx/1.27.0
  • PHP: 8.3.9
  • MySQL: 8.0.27
  • WordPress: 6.7-alpha-58576-src
  • Theme: twentytwenty v2.7

Actual Results

  • ✅ Issue resolved with patch. E.g. /wp-admin/customize.php?return=%2Fwp-admin%2F +
    • Site Identity: &autofocus%5Bsection%5D=title_tagline
    • Colors: &autofocus%5Bsection%5D=colors
    • Additional CSS: &autofocus%5Bsection%5D=custom_css

#16 in reply to: ↑ 14 @hellofromTonya
24 hours ago

  • Keywords changes-requested added; commit removed
  • Milestone changed from 6.7 to 6.6.2

Replying to hellofromTonya:

Tested this on 6.5.4. Can reproduce the issue there too. So this bug was not introduced in 6.6 cycle. Aha. Moving it to 6.7.

After evaluating the fix, [58069] introduced the issue for this fix by treating the autofocus as a string, rather than as a array. As such, the regression of changing the data type was introduced in the 6.6 cycle.

Moving this to 6.6.2 for only the fix that narrowly focuses on restoring the array data type.

Marking it as changes-requested, as the relocating of wp_unslash() was not introduced in the 6.6 cycle.

@swissspidy commented on PR #6974:


22 hours ago
#17

@hellofromtonya As I suggested that change, I disagree. https://core.trac.wordpress.org/changeset/58069 made an incorrect change in that it a) called sanitize_text_field on an array and b) called sanitize_text_field before calling wp_unslash() (rather than after). Both are introduced in the 6.6 cycle and it makes sense to fix this properly, _together_

Note: See TracTickets for help on using tickets.