Make WordPress Core

Opened 4 years ago

Last modified 9 months ago

#41445 assigned defect (bug)

post_parent can prevent media from embedding correctly

Reported by: loboyle Owned by: adamsilverstein
Milestone: Future Release Priority: normal
Severity: normal Version: 4.9.4
Component: Media Keywords: has-patch needs-testing dev-feedback needs-unit-tests
Focuses: rest-api Cc:


If media is uploaded for a post, then used as a featured image on another post, and the original parent is not accessible via the REST API (e.g. because it's in the trash, not published etc), then it cannot be embedded on the post that is accessible.

To reproduce

  • make a new post with a featured image
  • trash the post
  • make a new post, using the first image as the featured image
  • request the second post over the rest API with media embedding enabled

The media will not be embedded, instead a forbidden result will be embedded error

        "message":"You don't have permission to do this.",

See https://github.com/WP-API/WP-API/issues/2596 for the original issue. Also related is https://core.trac.wordpress.org/ticket/30691.

Attachments (3)

41445.diff (2.2 KB) - added by stuartfeldt 4 years ago.
41445.2.diff (2.3 KB) - added by adamsilverstein 16 months ago.
41445.3.diff (1.6 KB) - added by adamsilverstein 9 months ago.

Download all attachments as: .zip

Change History (41)

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

4 years ago

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

4 years ago

#3 @ritter.aaron
4 years ago

we experience the same issue, with the difference that the image was first uploaded to the library via Medialibrary and then attached to a pods post in to a field without any meta data (basicaly using pods with additional fields in table) (which works) and then attached to a blog post which showed the same issue with permission denied 403 (when directly attaching to the post it worked)

one more thing i found is that when attaching to the post the file name gets truncated

when uploading via media library it does not get truncated

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

4 years ago

#5 @benjamincreative
4 years ago

  • Version 4.7.5 deleted

Same here on 4.8.2

Uploaded to one custom post type as draft, added to another public custom post type...

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

4 years ago

#7 @riobahtiar
4 years ago

  • Focuses rest-api added
  • Keywords needs-patch added
  • Version set to 4.8.3

Hi All, Any updates for this ticket?

Because I just experienced the same issue,

WordPress Version 4.8.3

#8 @mythovik
4 years ago

In my case was a jpg error. Some images make this error:

After a while i discover that is about the name of the pic, in my case:


The "ñ" and other special chars, could cause this error.

#9 @Krstarica
4 years ago

Same on 4.9.1.

If post_parent of image is having status other than 'publish', rest_forbidden occurs.

For example, for image having ID 125676, the result of this query has to be 'publish':

SELECT b.post_status FROM wp_posts a, wp_posts b WHERE a.ID = 125676 AND a.post_parent=b.ID

#10 @stuartfeldt
4 years ago

So can we override get_item_permissions_check in WP_REST_Attachments_Controller, and then check if the requested attachment id is also set as _thumbnail_id postmeta -- and then if the request attachment id is a thumbnail on a published post allow it in the wp:featuredmedia response?

Or is that too much overhead to check all of wp_postmeta for each of these calls?

I'm happy to submit a patch if this is an acceptable approach.

#11 @neoian2
4 years ago

This is driving me nuts. The only solution I've found is to upload a new image for each post until this is fixed.

I feel like @stuartfeldt's solution would work, but it seems like a temporary fix to a bigger problem.

#12 @riobahtiar
4 years ago

  • Version 4.8.3 deleted

#13 @riobahtiar
4 years ago

  • Version set to 4.9.4

bugs still occurs in v4.9.4

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

4 years ago

#15 @adamsilverstein
4 years ago

@stuartfeldt Your suggestion to override get_item_permissions_check in WP_REST_Attachments_Controller, and then check if the requested attachment id is also set as _thumbnail_id postmeta sounds reasonable - could you work on a patch?

4 years ago

#16 @stuartfeldt
4 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

#17 @stuartfeldt
4 years ago

  • Keywords dev-feedback added

#18 @adamsilverstein
4 years ago

@stuartfeldt thanks for the patch!

I have a couple of questions/comments about the code:

  • should we check that $requestid? available before using?
  • add a query limit to avoid runaway queries
  • you can remove $has_permission entirely: just return true if $this->check_read_permission( $post ); is true
  • similarly, I don't think need to track $wp_error - instead, since you could be looking thru multiple posts with the same attached image, you are really looking for matches you have access to and can ignore the rest.

Reviewing the approach I also have two basic architectural concerns:

  • performance impact - meta queries are expensive
  • possible unexpected side effects, eg opening permissions where they should not be

This ticket could use feedback/review from a REST API component maintainer about the overall approach. cc: @rmccue @kadamwhite @joehoyle @rachelbaker @jnylen0

#19 @Krstarica
3 years ago

Still present in 4.9.6.

#20 @adamsilverstein
3 years ago

  • Keywords needs-unit-tests added

This could also use unit tests.

#21 @adamsilverstein
3 years ago

  • Milestone changed from Awaiting Review to Future Release
  • Owner set to adamsilverstein
  • Status changed from new to assigned

#22 @danielbachhuber
3 years ago

Here's another GitHub issue with prior conversation on the topic: https://github.com/WP-API/WP-API/issues/1987#issuecomment-247355568

Essentially, this is a flaw in WordPress' permissions model for attachments. The only way around it is with a meta query (querying to see if the attachment is used as _thumbnail_id on any posts), but meta queries have negative performance implications.

#23 @sboisvert
3 years ago

Regarding the query performance concerns. VIP has added an index on meta_key, meta_value(50) to address these types of queries (the index replaces the meta_key index). This makes these reverse lookups go very quickly. Perhaps it would be worth including it in core for this. It seems that reverse lookups on meta_value are getting more and more common from the code we see.

#24 @nickrigby
3 years ago

This issue also occurs if you attach the featured image at draft stage (the post has never been published). The post parent of the attachment retains the post parent id of a revision, which means the same permissions problem occurs.

#25 @jwktje
2 years ago

Any update on this issue?

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

2 years ago

#27 @adamsilverstein
2 years ago

@jwktje Thanks for the ping. Probably too late for this to land in 5.3, I'm milestoning as 5.4 so we make sure to address it then.

#28 @kadamwhite
2 years ago

#48314 was marked as a duplicate.

#29 @danielbachhuber
22 months ago

I've just run into this myself :P

At the very least, we should remove the WP_Error object from the embedded data. I don't think it makes sense to include in any cases.

#30 @jmacon
18 months ago

hey @adamsilverstein wondering if this got folded in to the 5.4 release? Thanks!

#31 @galbaras
16 months ago

This seems to indicate a problem of WordPress not updating media-to-post attachment information when a post changes status to/from "publish".

Basically, if a post is trashed, it's incorrect to show its embedded images as attached to it. If WordPress updated media records with the deletion, there would not be an API issues.

Conversely, if a deleted post is reinstated, its embedded images should be re-attached to it.

This ticket was mentioned in PR #301 on WordPress/wordpress-develop by adamsilverstein.

16 months ago

Trac ticket:

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

16 months ago

#34 @TimothyBlynJacobs
11 months ago

  • Component changed from REST API to Media

Instead of having to do an expensive meta query every time we check read permissions, could we try updating the post parent instead?

So whenever a post is moved to a non-public status, check if it is the parent of any media items. If so, looks for a new parent candidate by doing the _thumbnail_id meta query?

#35 @peterwilsoncc
11 months ago

The get_post_status() function accounts for attachments with an inherit status on trashed and deleted parents:

  • for attachments on trashed posts, it returns the parent's status immediately prior to it been trashed: get_post_meta( $post->post_parent, '_wp_trash_meta_status', true )
  • for attachments on deleted posts, it assumes the attachment has a published status

Could the REST API use get_post_status() in the attachment controller when determining the permissions?

This ticket was mentioned in PR #839 on WordPress/wordpress-develop by adamsilverstein.

9 months ago

Trac ticket:

#37 @adamsilverstein
9 months ago

41445.3.diff uses get_post_status as suggested by @peterwilsoncc. I added this in src/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php (instead of over-riding in the media controller) since it should be safe to use across post types.

Does this approach resolve the issue you reported @loboyle?

#38 @loboyle
9 months ago

Looks like an elegant fix, yes - thanks @peterwilsoncc and @adamsilverstein.

Note: See TracTickets for help on using tickets.