WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#19131 closed defect (bug) (fixed)

setting $taxnow in POSTS

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

Description

line 96 and 97 in wp-admin/admin.php:

if ( isset($_GET['taxonomy']) )
	$taxnow = sanitize_key($_GET['taxonomy']);
else
	$taxnow = '';

should be

if ( isset($_REQUEST['taxonomy']) )
	$taxnow = sanitize_key($_REQUEST['taxonomy']);
else
	$taxnow = '';

because in wp-admin/edit-tags.php lines 10, 11:

require_once('./admin.php');
$tax = get_taxonomy( $taxnow );
if ( !current_user_can( $tax->cap->manage_terms ) )
  wp_die( __( 'Cheatin’ uh?' ) );

doesn't work when you edit a tag and POST the changes. The current_user_can() is checked against 'tag_post' instead against the actual taxonomy.

Attachments (5)

19131.diff (672 bytes) - added by nacin 4 years ago.
19131.edit-php.patch (834 bytes) - added by ocean90 4 years ago.
19131.2.diff (1.7 KB) - added by ryan 4 years ago.
'_invalid' post type
19131.3.diff (2.2 KB) - added by nacin 4 years ago.
19131.4.diff (4.8 KB) - added by nacin 4 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 @ocean904 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to worksforme
  • Status changed from new to closed

Done in 3.3, see r19097.

comment:2 @nacin4 years ago

  • Milestone set to 3.3
  • Resolution worksforme deleted
  • Status changed from closed to reopened

Yes, but code changes in edit-tags.php has broken this, as it now only checks $_GET.

Attached a patch as an idea.

@nacin4 years ago

comment:3 @ocean904 years ago

Yes, but code changes in edit-tags.php has broken this, as it now only checks $_GET.

Same for edit.php.

#18716, #18983

comment:4 @nacin4 years ago

This code has changed so much over the last year or two, mainly due to reshuffling from the list tables. [16253], [18966].

We need to make this standardized; it's a mess.

comment:5 @ryan4 years ago

  • Owner set to ryan
  • Resolution set to fixed
  • Status changed from reopened to closed

In [19253]:

Use taxnow to determine the taxonomy for GET and POST requests in edit-tags.php. Props nacin. fixes #19131

comment:6 @nacin4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

edit.php needs this too.

post.php seems fine. post-new.php uses $_GET but I don't think you ever POST to post-new.php.

@ocean904 years ago

comment:7 @ryan4 years ago

There are also post type checks in WP_Posts_List_Table::construct().

The screen code treats a request for ?post_type=foo as a request for post_type = post. This is not good behavior. We should fallback to the default post type only when a particular post type was not requested. Same for taxonomy.

@ryan4 years ago

'_invalid' post type

comment:8 @nacin4 years ago

19131.3.diff takes a slightly different approach. Rather than overloading with _invalid, we just keep it as an empty string if we can't ascertain the post type. We'll die anyway, so it'd probably be better to keep the screen object clean.

@nacin4 years ago

@nacin4 years ago

comment:9 @nacin4 years ago

  • Keywords has-patch added

comment:10 @ryan4 years ago

In [19321]:

Don't fallback to default post type or taxonomy if given an invalid post type or taxonomy. Use typenow as the canonical post type. Props nacin. see #19131

comment:11 follow-up: @ryan4 years ago

This no longer sets _GET[ 'post_type'] to the derived post type. I don't think the lack hurts anything.

Instead of $typenow, standardize on get_current_screen()->post_type?

comment:12 in reply to: ↑ 11 @nacin4 years ago

Replying to ryan:

This no longer sets _GET[ 'post_type'] to the derived post type. I don't think the lack hurts anything.

Agreed. Our penchant to hack stuff into $_GET and $_REQUEST bugs the heck out of me.

Instead of $typenow, standardize on get_current_screen()->post_type?

Definitely. I started using it here and looked at killing off usage. It's why there's an otherwise unnecessary $post_type = $typenow;, that way it wouldn't appear in my greps.

comment:13 @nacin4 years ago

typenow:

./wp-admin/admin.php:92:	$typenow = $_REQUEST['post_type'];
./wp-admin/admin.php:94:	$typenow = '';
./wp-admin/admin.php:114:	if ( !empty($typenow) )
./wp-admin/admin.php:115:		$the_parent = $pagenow . '?post_type=' . $typenow;
./wp-admin/admin.php:217:	if ( $typenow == 'page' ) {
./wp-admin/edit.php:12:if ( ! $typenow )
./wp-admin/edit.php:15:$post_type = $typenow;
./wp-admin/includes/plugin.php:1338:	global $typenow;
./wp-admin/includes/plugin.php:1387:			if ( !empty($typenow) && ($submenu_array[2] == "$pagenow?post_type=$typenow") ) {
./wp-admin/includes/plugin.php:1390:			} elseif ( $submenu_array[2] == $pagenow && empty($typenow) && ( empty($parent_file) || false === strpos($parent_file, '?') ) ) {
./wp-admin/includes/plugin.php:1412:	global $typenow;
./wp-admin/includes/plugin.php:1447:						( !empty($typenow) && $parent == $pagenow . '?post_type=' . $typenow)
./wp-admin/includes/screen.php:531:		global $current_screen, $taxnow, $typenow;
./wp-admin/includes/screen.php:534:		$typenow = $this->post_type;
./wp-admin/menu-header.php:37:	global $self, $parent_file, $submenu_file, $plugin_page, $pagenow, $typenow;
./wp-admin/menu-header.php:56:		if ( ( $parent_file && $item[2] == $parent_file ) || ( empty($typenow) && $self == $item[2] ) )
./wp-admin/menu-header.php:123:				$self_type = ! empty( $typenow ) ? $self . '?post_type=' . $typenow : 'nothing';

taxnow:

./wp-admin/admin.php:97:	$taxnow = $_REQUEST['taxonomy'];
./wp-admin/admin.php:99:	$taxnow = '';
./wp-admin/admin.php:223:		if ( $taxnow == 'category' )
./wp-admin/admin.php:225:		elseif ( $taxnow == 'link_category' )
./wp-admin/edit-tags.php:12:if ( ! $taxnow )
./wp-admin/edit-tags.php:15:$tax = get_taxonomy( $taxnow );
./wp-admin/includes/screen.php:531:		global $current_screen, $taxnow, $typenow;
./wp-admin/includes/screen.php:533:		$taxnow = $this->taxonomy;

Most of these don't really need to be touched for now. Let's just stop propagating it.

comment:14 @nacin4 years ago

In [19324]:

WP_Screen->post_type is always set. see #19131.

comment:15 @nacin4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.