WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 11 months ago

#23423 reopened defect (bug)

sanitize_title() in dynamic_sidebar() restricts the use of specific characters for sidebar IDs

Reported by: paulvandermeijs Owned by: chriscct7
Milestone: Future Release Priority: normal
Severity: normal Version: 2.2
Component: Widgets Keywords: needs-patch needs-unit-tests
Focuses: Cc:

Description

In the dynamic_sidebar() function in wp-includes/widgets.php uses sanitize_title() on the given index when it looks for a sidebar with a name that matches the index. After that it leaves the index value sanitized making it impossible to use characters not allowed by sanitize_title() in a sidebar ID.

By not overwriting the given index value with the sanitized version it would still be possible to use any character for the ID. To achieve this, lines 847-853

$index = sanitize_title($index);
foreach ( (array) $wp_registered_sidebars as $key => $value ) {
	if ( sanitize_title($value['name']) == $index ) {
		$index = $key;
		break;
	}
}

should be replaced with

$sanitized_index = sanitize_title($index);
foreach ( (array) $wp_registered_sidebars as $key => $value ) {
	if ( sanitize_title($value['name']) == $sanitized_index ) {
		$index = $key;
		break;
	}
}

Attachments (3)

23423.diff (627 bytes) - added by fjarrett 3 years ago.
23423.patch (758 bytes) - added by tyxla 15 months ago.
Refreshing patch to work with the new location of dynamic_sidebar()
23423-test.patch (623 bytes) - added by tyxla 15 months ago.
Attaching a unit test for the reported issue

Download all attachments as: .zip

Change History (15)

#1 @SergeyBiryukov
4 years ago

  • Version changed from 3.5.1 to 2.2

Related: [5473] (for #4258).

#2 @fjarrett
3 years ago

  • Cc fjarrett@… added
  • Keywords has-patch added; needs-patch removed

Patch allowing unsanitized $index

@fjarrett
3 years ago

#4 @chriscct7
15 months ago

  • Keywords needs-refresh added
  • Milestone changed from Awaiting Review to 4.4
  • Owner set to chriscct7
  • Status changed from new to assigned

#5 @wonderboymusic
15 months ago

  • Keywords needs-unit-tests added

Unit tests if possible

@tyxla
15 months ago

Refreshing patch to work with the new location of dynamic_sidebar()

@tyxla
15 months ago

Attaching a unit test for the reported issue

#6 @tyxla
15 months ago

  • Keywords needs-refresh needs-unit-tests removed

I've just uploaded 2 patches:

  • 23423.patch - refreshed version of the old patch to work with the latest location of dynamic_sidebar();
  • 23423-test.patch - contains a unit test of the reported issue.

#7 @chriscct7
15 months ago

  • Status changed from assigned to reviewing

#8 @wonderboymusic
15 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 34465:

Widgets: when passing a string arg value to dynamic_sidebar(), don't reset $index when the arg's sanitized value matches the sanitized name of a sidebar.

Adds unit test.

Props tyxla, fjarrett.
Fixes #23423.

#9 @SergeyBiryukov
12 months ago

This introduced a regression, see #34995.

#10 @SergeyBiryukov
11 months ago

In 36130:

Widgets: Revert [34465], as it introduced a regression, making the $index argument of dynamic_sidebar() case-sensitive.

Fixes #34995 for trunk. See #23423.

#11 @SergeyBiryukov
11 months ago

  • Keywords needs-patch needs-unit-tests added; has-patch removed
  • Milestone changed from 4.4 to Future Release
  • Resolution fixed deleted
  • Status changed from closed to reopened

Needs a new patch and a new test, see comment:7:ticket:34995 and comment:15:ticket:34995.

#12 @dd32
11 months ago

In 36148:

Widgets: Revert [34465], as it introduced a regression, making the $index argument of dynamic_sidebar() case-sensitive.

Merges [36130] to the 4.4 branch.
See #23423.
Fixes #34995.

Note: See TracTickets for help on using tickets.