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

[MarcusPope]

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.

[ocean90]

-1 for the constant.

[nacin]

At a glance, sanitize_key() looks appropriate here.

[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.

[nacin]

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.

[nacin]

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.

[SergeyBiryukov]

Replying to nacin:

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

Done in 18637.2.patch​.

[azaozz]

Replying to SergeyBiryukov:

Shouldn't that be:

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

:)

[nacin]

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.

[azaozz]

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.

[ocean90]

Return 0 if it's invalid

[SergeyBiryukov]

Replying to azaozz:

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

[azaozz]

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.

[nacin]

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.

[ocean90]

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

Done in 18637.3.patch​.

[nacin]

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.

[MarcusPope]

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!