WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 3 months ago

#16773 new defect (bug)

Unescaped preg_match breaks with PHP 5.3 Namespaced Widget Classes.

Reported by: 5ubliminal Owned by:
Milestone: Awaiting Review Priority: normal
Severity: minor Version: 3.1
Component: Widgets Keywords: has-patch needs-unit-tests needs-testing
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 (2)

16773.diff (834 bytes) - added by solarissmoke 4 years ago.
widgets.txt (156.2 KB) - added by stevenkword 3 months ago.
Plugin classes that extend WP_Widget

Download all attachments as: .zip

Change History (9)

comment:1 @5ubliminal4 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.

comment:2 @5ubliminal4 years ago

  • Severity changed from major to minor

@solarissmoke4 years ago

comment:3 @solarissmoke4 years ago

  • Keywords has-patch added

We already have a function to do just that :)

comment:4 @tamlyn3 years ago

  • Cc tamlyn@… added

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

comment:6 @jorbin3 months 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.

@stevenkword3 months ago

Plugin classes that extend WP_Widget

comment:7 @stevenkword3 months 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 3 months ago by stevenkword (previous) (diff)
Note: See TracTickets for help on using tickets.