WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#20629 closed defect (bug) (fixed)

wp.getPages response includes bogus content for pages lacking edit permission

Reported by: redsweater Owned by: ryan
Milestone: 3.4 Priority: normal
Severity: normal Version: 3.4
Component: XML-RPC Keywords: has-patch
Focuses: Cc:

Description

In the event that the authenticated user for wp.getPages does not have editing permissions for a given page, instead of that page being completely omitted from the resulting array, the array is filled with bogus entries. The practical implication of this bug is that for XMLRPC clients that do not expect the bogus values to appear, they will strain to interpret the error blocks as actual page entries.

The bogus entries appear like this:

<value><struct>
  <member><name>code</name><value><int>401</int></value></member>
  <member><name>message</name><value><string>Sorry, you cannot edit this page.</string></value></member>
</struct></value>

This doesn't match the documented behavior that the result should be an array of page structures.

I believe the expected behavior would be for the resulting array to simply omit any pages the user doesn't have permission to edit.

The crux of this issue is in the implementation of wp.getPages, and that it reuses the wp_getPage() method to fulfill the getPages request:

for ( $i = 0; $i < $num_pages; $i++ ) {
	$page = wp_xmlrpc_server::wp_getPage(array(
		$blog_id, $pages[$i]->ID, $username, $password
	));
	$pages_struct[] = $page;
}

It is within this wp_getPage() method that the page-specific permission check is made, and a bogus error is returned instead of the page.

Probably the wp_getPages() method should check for this bogus error result and, if present, omit the page from the return $pages_struct.

Attachments (3)

FixGetPages.diff (681 bytes) - added by redsweater 2 years ago.
Patch to omit pages from wp_getPages when wp_getPage has returned an error for the given page
20629.patch (5.4 KB) - added by maxcutler 2 years ago.
20629.2.patch (5.6 KB) - added by maxcutler 2 years ago.
Tabs for indent, spaces for alignment.

Download all attachments as: .zip

Change History (12)

redsweater2 years ago

Patch to omit pages from wp_getPages when wp_getPage has returned an error for the given page

comment:1 redsweater2 years ago

  • Keywords has-patch added

comment:2 maxcutler2 years ago

This should really be fixed. While the current patch is the easy way, I wonder if a better solution would be to extract most of wp_getPage into a _prepare_page method like we've done in #20409 and the new 3.4 methods. That way the nested calls to wp_getPage from wp_getPages don't have to re-query for the page (even though it's a likely cache hit).

comment:3 maxcutler2 years ago

  • Cc maxcutler added

comment:4 nacin2 years ago

  • Milestone changed from Awaiting Review to 3.4

comment:5 maxcutler2 years ago

Added unit test in [UT715]. The attached patch is broken because $page is an array and get_class will fail on arrays. I'll work on a new patch.

comment:6 redsweater2 years ago

Aha! Great catch. I only tested the behavior from the other side of the API, where it did work, presumably because whatever value is returned by get_class(array()) is still not equal to IXR_Error.

comment:7 nacin2 years ago

is_a() or instanceof are both better in this case.

maxcutler2 years ago

comment:8 maxcutler2 years ago

Added a new patch that adds a _prepare_page method as mentioned in comment:2. Cuts out double queries in wp.getPages and allows for filtering of the output array for both methods.

maxcutler2 years ago

Tabs for indent, spaces for alignment.

comment:9 ryan2 years ago

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

In [20807]:

Fix wp.getPages response when fetching pages the current user does not have caps for. Props maxcutler, redsweater. fixes #20629

Note: See TracTickets for help on using tickets.