WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#18637 closed defect (bug) (fixed)

Poor regex used in admin-ajax.php for user UI state

Reported by: MarcusPope Owned by: nacin
Milestone: 3.3 Priority: normal
Severity: normal Version: 3.2.1
Component: Administration Keywords: has-patch
Focuses: Cc:

Description

The regex used to determine if the current page is blacklisted during meta box ordering or screen options changes or box closing actions will filter out custom post types that contain numbers in their names.

In this case all changes to the meta box layout will go unsaved without any notice to the user.

Attached is a patch that abstracts the repetitive / restrictive regex to a constant and updates it to accept 0-9. Not sure if other characters need to be added but the base change is there.

Attachments (5)

admin-ajax.php.patch (1.4 KB) - added by MarcusPope 3 years ago.
18637.patch (888 bytes) - added by ocean90 3 years ago.
18637.2.patch (919 bytes) - added by SergeyBiryukov 3 years ago.
18637.return-0.patch (993 bytes) - added by ocean90 3 years ago.
Return 0 if it's invalid
18637.3.patch (990 bytes) - added by ocean90 3 years ago.

Download all attachments as: .zip

Change History (20)

@MarcusPope3 years ago

comment:1 @ocean903 years ago

  • Severity changed from major to normal

-1 for the constant.

comment:2 @nacin3 years ago

  • Milestone changed from Awaiting Review to 3.3

At a glance, sanitize_key() looks appropriate here.

@ocean903 years ago

comment:3 follow-up: @azaozz3 years ago

-1. I'd really hate to see CPT slugs like '1354345435' or even 'my-page-2344'. IMHO we needed to enforce that on CPT registration.

comment:4 follow-up: @nacin3 years ago

Patch is a bit off, as sanitize_key() isn't a direct replacement for the way preg_match() is being used.

Say you pass in a string that includes valid and invalid characters. sanitize_key() would strip the invalid characters, but return the valid ones. Worse, $page isn't set to the return value, so the unsanitized one would be used.

Setting $page = sanitize_key($page) and then die(-1) if !$page seems fine.

comment:5 in reply to: ↑ 3 @nacin3 years ago

Replying to azaozz:

-1. I'd really hate to see CPT slugs like '1354345435' or even 'my-page-2344'. IMHO we needed to enforce that on CPT registration.

We already do, with sanitize_key(). But there's nothing wrong with numbers, and we shouldn't make it more restrictive, as that'll break things. also think things like 411, thing2thing, etc, where numbers are literally part of the type.

@SergeyBiryukov3 years ago

comment:6 in reply to: ↑ 4 ; follow-up: @SergeyBiryukov3 years ago

Replying to nacin:

Setting $page = sanitize_key($page) and then die(-1) if !$page seems fine.

Done in 18637.2.patch.

comment:7 in reply to: ↑ 6 ; follow-up: @azaozz3 years ago

Replying to SergeyBiryukov:

Shouldn't that be:

if ( $page != sanitize_key( $page ) )

:)

comment:8 in reply to: ↑ 7 ; follow-up: @nacin3 years ago

Replying to azaozz:

Replying to SergeyBiryukov:

Shouldn't that be:

if ( $page != sanitize_key( $page ) )

:)

No. Then it's not doing the assignment. The patch is proper.

comment:9 in reply to: ↑ 8 ; follow-up: @azaozz3 years ago

Replying to nacin:

The current regexp doesn't change/sanitize anything, it only checks whether the passed slug value is 'proper'. Why the change in behaviour? That could bring a mismatch down the line. The slugs are sanitized before loading the page.

@ocean903 years ago

Return 0 if it's invalid

comment:10 in reply to: ↑ 9 @SergeyBiryukov3 years ago

Replying to azaozz:

I thought of that too. Didn't mean to intercept, I guess I'll leave it up to ocean90 :)

comment:11 @azaozz3 years ago

No, that still won't work. sanitize_key( $page ) will always return something (unless all the characters in the slug are bad, i.e. '$%#@%$#@%'), so

$page = sanitize_key( $page )

will always be true and we may end up saving useless inaccessible data as @nacin mentioned.

I don't see a reason to change the behaviour there: we are expecting a page slug that has been sanitized with sanitize_key(). If what we are getting doesn't pass the same sanitization unchanged, it's bad.

Last edited 3 years ago by azaozz (previous) (diff)

comment:12 @nacin3 years ago

In hindsight, azaozz is correct. We're currently validating, rather than sanitizing, and there's no reason to change it. We should use $page != sanitize_key( $page ).

die(0) is proper. We use -1 for permissions, 0 for failures.

@ocean903 years ago

comment:13 @ocean903 years ago

We should use $page != sanitize_key( $page ).

Done in 18637.3.patch.

comment:14 @nacin3 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In [18663]:

Loosen validation regex to use sanitize_key() in a few AJAX locations for things like pages, orders, columns. Change return value to 0 for failure, as -1 is reserved for authentication/intention. props ocean90, azaozz, fixes #18637.

comment:15 @MarcusPope3 years ago

Just curious, but why was the use of a constant inappropriate in my original patch? I completely understand using sanitize_key in lieu of yet another regex (though imo the readability of the intended logic is slightly lowered.) But I don’t understand why replacing three copy-and-pasted string literals with a constant isn’t a good practice.

Had sanitize_key (or variations of it) not existed in the codebase what would be the preferred method for solving this problem?

Hopefully someone can answer me here or email me at marcus.pope at springbox.com

Thanks everyone, especially @azaozz for keeping the behavior consistent :D (btw, I would hate to see slugs like you described as well, but in this case I have a post type with emc2 in the name and even the codex lists:

"ai1m_product" - Products post type provided by the hypothetical "All-in-One Merchant" Plugin.

as a valid naming convention for CPT’s.

Thanks again to everyone else for knocking this out quickly!

Note: See TracTickets for help on using tickets.