WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 3 months ago

#38977 closed defect (bug) (fixed)

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

Reported by: jnylen0 Owned by: 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 4 months ago.

Download all attachments as: .zip

Change History (17)

@jnylen0
4 months ago

#1 @jnylen0
4 months ago

  • Component changed from General to REST API

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


4 months ago

#3 @rachelbaker
4 months ago

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

#4 @jnylen0
4 months 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.


4 months ago

#6 @helen
4 months ago

  • Keywords dev-feedback added

#7 follow-up: @rachelbaker
4 months 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 the nav_menu_item post type when adding Menu endpoints in the future?

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.

Last edited 4 months ago by rachelbaker (previous) (diff)

#8 in reply to: ↑ 7 @jnylen0
4 months 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
4 months 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.


4 months ago

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


4 months ago

#12 @jnylen0
3 months 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
3 months 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
3 months 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
3 months 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
3 months 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.

Note: See TracTickets for help on using tickets.