Make WordPress Core

Opened 5 years ago

Last modified 4 weeks ago

#41445 assigned defect (bug)

post_parent can prevent media from embedding correctly

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

Description

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

{
  "wp:featuredmedia":[  
     {  
        "code":"rest_forbidden",
        "message":"You don't have permission to do this.",
        "data":{  
           "status":403
        }
     }
  ]
}

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 20 months ago.
41445.3.diff (1.6 KB) - added by adamsilverstein 13 months ago.

Download all attachments as: .zip

Change History (45)

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:

PeñaNieto.jpg

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?

@stuartfeldt
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
4 years ago

Still present in 4.9.6.

#20 @adamsilverstein
4 years ago

  • Keywords needs-unit-tests added

This could also use unit tests.

#21 @adamsilverstein
4 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
2 years 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
22 months ago

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

#31 @galbaras
21 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.


20 months ago

Trac ticket:

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


20 months ago

#34 @TimothyBlynJacobs
15 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
15 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.


13 months ago

Trac ticket:

#37 @adamsilverstein
13 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
13 months ago

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

#39 @orhanc
6 weeks ago

@adamsilverstein We're facing the same issue with the latest release of wordpress version 5.8.2.

#40 follow-up: @adamsilverstein
6 weeks ago

  • Milestone changed from Future Release to 6.0

Hey @orhanc thanks for the ping, apologies for letting this ticket linger... I had overlooked this 'future release' ticket and it is probably too late to land this change for 5.9, so I will tag it for 6.0.

Although the code in 41445.3.diff works in testing, it is not quite ready to land...

First, @TimothyBlynJacobs raised an important point in this comment - calling get_post_status can add an additional meta query (specifically when the parent is in the trash) so it might be worth considering what Timothy suggested:

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?

So that would be another post that had used the image as featured?

Also from @galbaras -

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

That would be good to support if possible, especially since it is the current behavior. The exception would be the case where the image has already been attached to a different post (which isn't possible now).

In addition, this change needs some backing unit tests to validate the expected behavior.

#41 @galbaras
6 weeks ago

Here's a bit of a radical idea: the relationship between media and posts is currently many-to-one due the use of post_parent to handle it. However, in real life, it's many-to-many, because there's nothing stopping users from embedding the same item in multiple posts.

WordPress handles many-to-many relationships via wp_postmeta, not wp_posts fields. Perhaps it's time to review this relationship?

Having said that, I've seen cases where images are linked to posts (by plugins, e.g. within a gallery or custom field) without updating their respective post_parent fields. So there may be a need for a way (e.g. scheduled task) to clean that up.

Subsequently, if a parent post is deleted, the association with the published post can still be used to check whether the media should be accessible.

#42 in reply to: ↑ 40 @szabolcsgleszer
4 weeks ago

Hi @galbaras , @adamsilverstein,

thanks for your comments.

If I understood it well, the whole concept for the relationship between media and posts has to be revalidated and only after that we can expect to have a fix for this?
Either a new one or the one from Adam.

I am asking it again, because we are dependent from it and would like to know if we can calculate with this feature or not.

Note: See TracTickets for help on using tickets.