WordPress.org

Make WordPress Core

Opened 11 years ago

Closed 6 months ago

Last modified 6 months ago

#16773 closed defect (bug) (duplicate)

Unescaped preg_match breaks with PHP 5.3 Namespaced Widget Classes.

Reported by: 5ubliminal Owned by: SergeyBiryukov
Milestone: Priority: normal
Severity: minor Version: 3.1
Component: Widgets Keywords:
Focuses: Cc:

Description

In file /wp-admin/includes/widgets.php at line 118 in function next_widget_id_number we have:

preg_match( '/' . $id_base . '-([0-9]+)$/', $widget_id, $matches )

It generates very ugly warnings for Namespaced Widget Classes as it should cuorrectly be:

preg_match( '/' . preg_quote($id_base, '/') . '-([0-9]+)$/', $widget_id, $matches )

Thanks.

PS: I think you should do a whole sanity check regarding use of Namespaces and Closures. I'm currently switching completely to PHP 5.3 style and I'll keep you updated if I find other... problems.

Attachments (3)

16773.diff (834 bytes) - added by solarissmoke 11 years ago.
widgets.txt (156.2 KB) - added by stevenkword 7 years ago.
Plugin classes that extend WP_Widget
16773.2.diff (884 bytes) - added by drebbits.web 6 years ago.

Download all attachments as: .zip

Change History (31)

#1 @5ubliminal
11 years ago

Spoke too soon. This problem only occurs if you don't specify an id_base. But, if you do, it goes away.

If you don't... the problem starts in

WP_Widget::__construct()

where $this->id_base should be sanitized once more:

$this->id_base = trim(preg_replace('~[^a-z0-9-_]+~i', '-', $this->id_base), '-');

or one could just weed out / and \ characters.

Cheers.

#2 @5ubliminal
11 years ago

  • Severity changed from major to minor

@solarissmoke
11 years ago

#3 @solarissmoke
11 years ago

  • Keywords has-patch added

We already have a function to do just that :)

#4 @tamlyn
9 years ago

  • Cc tamlyn@… added

Patch looks good to me. Any reason this hasn't been accepted yet?

#5 @jdgrimes
7 years ago

Related: #27770

#6 @jorbin
7 years ago

  • Keywords needs-unit-tests needs-testing added

I'd like to ensure that switching from strtolower to sanitize_key doesn't cause any breakage. @stevenkword is going to try and pull the classname of all the widgets currently in the plugin repo and make sure that none of the plugin classes are going to change from the switch.

I would also like to see some unit tests here.

@stevenkword
7 years ago

Plugin classes that extend WP_Widget

#7 @stevenkword
7 years ago

I've attached widgets.txt which contains a list of all classes which extend WP_Widget from the plugin repository.

The list was created using Mark Jaquith's WordPress-Plugin-Directory-Slurper to obtain a directory of plugins.

Then I used the following command to create the file:
ack-grep --php "[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*. extends WP_Widget" ./plugins/ | cut -d' ' -f2 > ./widgets.txt

The pattern was taken from here: http://php.net/manual/en/functions.user-defined.php

Last edited 7 years ago by stevenkword (previous) (diff)

@drebbits.web
6 years ago

#8 @drebbits.web
6 years ago

Since the time the patch was added, widget class had been restructured to class-wp-widget.php. I applied the previous patch to the correct file.

#9 follow-up: @westonruter
6 years ago

Feedback on 16773.2.diff:

<?php
$this->id_base = empty($id_base) ? preg_replace( '/(wp_)?widget_/', '', sanitize_key( get_class( $this ) ) ) : sanitize_key( $id_base ); 

The use of strtolower() should be retained because PHP class names should be have capitalized words. So to ensure that WP_Widget_Foo becomes foo as the default $id_base, use strtolower().

<?php
if ( empty($id_base) ) {
    $this->id_base = preg_replace( '/(wp_)?widget_/', '', strtolower( sanitize_key( get_class( $this ) ) ) );
} else {
    $this->id_base = sanitize_key( get_class( $this ) );
}

This will also ensure back-compat, since strtolower() is currently used in Core.

#10 in reply to: ↑ 9 @SergeyBiryukov
5 years ago

Replying to westonruter:

The use of strtolower() should be retained because PHP class names should be have capitalized words. So to ensure that WP_Widget_Foo becomes foo as the default $id_base, use strtolower().

sanitize_key() already does that, adding a second call seems redundant.

#11 @westonruter
5 years ago

@SergeyBiryukov good point. Another consideration is perhaps it not a good idea to use the sanitize_key function at all because the return value is filtered so in reality we have no idea what it could actually return. As such, maybe it is better to copy the logic from sanitize_key to inline it?

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


17 months ago

#13 @whyisjake
17 months ago

  • Keywords needs-refresh added; has-patch removed
  • Milestone changed from Awaiting Review to Future Release

#14 @SergeyBiryukov
17 months ago

  • Milestone changed from Future Release to 5.6
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#15 @jipmoors
16 months ago

Looking at the original problem and the current code.
I have a hard time deteremining the problem, if there is any.

The original problem was that a variable was used to construct a regular expression query, but that has been rewritten in the meanwhile.

Currenty the id_base is used to construct a string which is most likely (always?) used to fill an HTML ID or FOR attribute. These used to be restricted on how they had to be restricted but since HTML5 this is very loosly defined: https://html.spec.whatwg.org/multipage/dom.html#the-id-attribute

Since everything is working and no other reports have come in, I suggest closing this issue.

#16 @justinahinon
16 months ago

  • Keywords close added

#17 @jipmoors
16 months ago

There seems to be some confusion added to this ticket in https://core.trac.wordpress.org/ticket/16773#comment:8

The code in admin/includes/widget.php is still there and is still using the preg_match.

#18 @jipmoors
16 months ago

Have been reproducing the problem.

In the current code if a widget (in a namespace) is registered without providing a "base id"

  • The widget page will not be able to distinguish between multiple instances of this widget
  • The customizer will only show one instance of the widget
  • The customizer will not show the widget in the "add widget"-selection

Changing the "strtolower" to "sanitize_key" will solve all these problems.

Though this seems to be a backwards-compatibility risk, if the widget name would change the widgets will disappear from the site.

Only changing the strtolower to sanitize_key for classes will solve the problem.
Though widgets with namespaces will be gone from the sites.
So that is a problem to still solve.

Applying the preg_quote, mentioned in the original report seems like a no-brainer to make sure duplicate instances of these widgets can work on the 'widgets' page.

#19 @jipmoors
16 months ago

  • Keywords close removed

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


13 months ago

#21 @helen
13 months ago

  • Milestone changed from 5.6 to 5.7

This does seem like something to fix, I was actually experiencing it on my local testing environment because I had been doing some widget+block testing. However, I hadn't noticed this ticket in the milestone previously and it sounds like the solution needs some more consideration, so I'm going to move this to 5.7 as we are already in 5.6 beta 3.

#22 @lukecarbis
10 months ago

  • Milestone changed from 5.7 to 5.8

We're now just a day away from 5.7 beta 3, so I'm going to move this to 5.8.

#23 @Mte90
9 months ago

I think that it is a duplicate of #44098
In that other ticket I did a patch that works but missing tests.

Last edited 6 months ago by SergeyBiryukov (previous) (diff)

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


7 months ago

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


6 months ago

#26 @hellofromTonya
6 months ago

  • Milestone 5.8 deleted
  • Resolution set to duplicate
  • Status changed from reviewing to closed

@Mte90 you're right. #44098 and this one are the same. Since the solution was committed in that ticket, marking this one as a duplicate and closing.

#27 @SergeyBiryukov
6 months ago

  • Keywords needs-unit-tests needs-testing needs-refresh removed

It looks like I forgot about about this ticket while working on #44098.

Sorry for that! Will do a follow-up commit then to include the props from here :)

#28 @SergeyBiryukov
6 months ago

In 51070:

Coding Standards: Simplify the logic in WP_Widget::get_field_name() and ::get_field_id().

Includes minor code layout fixes for better readability.

Follow-up to [41292], [50953], [50961].

Props 5ubliminal, solarissmoke, tamlyn, jdgrimes, jorbin, stevenkword, drebbits.web, westonruter, jipmoors, justinahinon, helen, lukecarbis, Mte90, hellofromTonya, SergeyBiryukov.
See #16773, #52627.

Note: See TracTickets for help on using tickets.