WordPress.org

Make WordPress Core

Opened 11 months ago

Closed 10 months ago

Last modified 5 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 11 months ago.

Download all attachments as: .zip

Change History (18)

@jnylen0
11 months ago

#1 @jnylen0
11 months ago

  • Component changed from General to REST API

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


11 months ago

#3 @rachelbaker
11 months ago

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

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


11 months ago

#6 @helen
11 months ago

  • Keywords dev-feedback added

#7 follow-up: @rachelbaker
11 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 11 months ago by rachelbaker (previous) (diff)

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


11 months ago

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


11 months ago

#12 @jnylen0
11 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
10 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
10 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
10 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
10 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.

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


5 months ago

Note: See TracTickets for help on using tickets.