Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#38977 closed defect (bug) (fixed)

REST API: `password` is incorrectly included in arguments to get a media item

Reported by: jnylen0's profile jnylen0 Owned by: jnylen0's profile jnylen0
Milestone: 4.7.1 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch has-unit-tests commit fixed-major
Focuses: Cc:

Description

Media items in the API do not support passwords, so we need to check for the presence of password in the item schema before adding this argument.

Attachments (1)

38977.diff (3.6 KB) - added by jnylen0 8 years ago.

Download all attachments as: .zip

Change History (18)

@jnylen0
8 years ago

#1 @jnylen0
8 years ago

  • Component changed from General to REST API

This ticket was mentioned in Slack in #core-restapi by jnylen. View the logs.


8 years ago

#3 @rachelbaker
8 years ago

@jnylen0 What if the media is attached to a post that is password protected? What should happen then?

#4 @jnylen0
8 years ago

I think this change is still correct.

Here's such a post: https://nylen.io/wp-dev/2016/11/30/this-includes-attachment-tim-berners-lee-favorite-websites/

and here's the attachment page (does not require or accept a password): https://nylen.io/wp-dev/2016/11/30/this-includes-attachment-tim-berners-lee-favorite-websites/tim-berners-lee-favorite-websites/

I'm wondering whether it is worth calling all of get_item_schema again, maybe we should special-case 'attachment' === $this->post_type there instead.

This ticket was mentioned in Slack in #core by helen. View the logs.


8 years ago

#6 @helen
8 years ago

  • Keywords dev-feedback added

#7 follow-up: @rachelbaker
8 years ago

I am concerned about the potential performance impact of reloading the schema. We *could* special case attachment, but then shouldn't we also special case revisions? And what about when Menu endpoints are added that use the nav_menu_item post type?

Seems like, ideally, we would want to use something like a post_type_supports() conditional within the schema. Which is a bigger fix than we can get into 4.7, which brings me to question "is anything broken here?"

I agree that this is something we should fix, but I also don't want to rush a half-baked solution.

@jnylen0 @helen I recommend moving this ticket out of the 4.7 milestone, and into 4.8 so it can be addressed thoroughly.

Version 0, edited 8 years ago by rachelbaker (next)

#8 in reply to: ↑ 7 @jnylen0
8 years ago

Replying to rachelbaker:

I am concerned about the potential performance impact of reloading the schema.

Worth noting that we're already grabbing the schema twice for each post type during route registration (via get_endpoint_args_for_item_schema for create and edit).

We *could* special case attachment, but then shouldn't we also special case the nav_menu_item post type when adding Menu endpoints in the future?

Agreed I don't much like the idea of special casing here either.

Seems like, ideally, we would want to use something like a post_type_supports() conditional within the schema. Which is a bigger fix than we can get into 4.7

There's no post_type_supports for passwords, it's odd like that.

which brings me to question "is anything broken here?"

I think so, we have an argument that does nothing for no clear reason. If we leave it in place for 4.7, are we saying that it should be left in place for future releases also?

@jnylen0 @helen I recommend moving this ticket out of the 4.7 milestone, and into 4.8 so it can be addressed thoroughly.

I would prefer that we go with the fix in 38977.diff for 4.7 and get the interface behaving as expected, then we can circle back around to a more complete and performant fix in 4.8.

In any case, I would not like to move this out of 4.7 unless we can go ahead and decide that we will definitely fix it in 4.8.

#9 @pento
8 years ago

  • Keywords dev-feedback removed
  • Milestone changed from 4.7 to 4.7.1

The worst case is that someone sends a password that won't do anything. We can do this in 4.7.1.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


8 years ago

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


8 years ago

#12 @jnylen0
8 years ago

  • Owner set to jnylen0
  • Status changed from new to accepted

Agree with getting this into 4.7.1. While it's not the ideal fix, performance impact from calling the schema 3 times instead of 2 is going to be neglibible - it doesn't call out to the DB or anything and we recommend that people upgrade to PHP 7 which will also help.

#13 @pento
8 years ago

  • Keywords commit added

Let's go with 38977.diff.

I'm not worried about the performance impact, and while I would like to see post_type_supports() support password, that'll be a fairly significant amount of work, way outside of the scope of this ticket.

#14 @jnylen0
8 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 39595:

REST API: Do not include the password argument when getting media items

Currently, attachment is the only post type exposed via the REST API that
does not support password protection, but it's possible for other post types to
remove password support.

Fixes #38977.

#15 @jnylen0
8 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Needs to be ported to the 4.7 branch.

#16 @dd32
8 years ago

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

In 39610:

REST API: Do not include the password argument when getting media items

Currently, attachment is the only post type exposed via the REST API that
does not support password protection, but it's possible for other post types to
remove password support.

Props jnylen0.
Merges [39595] to the 4.7 branch.
Fixes #38977.

This ticket was mentioned in Slack in #core-restapi by jnylen. View the logs.


8 years ago

Note: See TracTickets for help on using tickets.