WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 40 hours 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: REST API Keywords: has-patch needs-testing dev-feedback needs-unit-tests
Focuses: rest-api Cc:
PR Number:

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 (1)

41445.diff (2.2 KB) - added by stuartfeldt 20 months ago.

Download all attachments as: .zip

Change History (29)

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


2 years ago

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


2 years ago

#3 @ritter.aaron
2 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.


2 years ago

#5 @benjamincreative
2 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.


2 years ago

#7 @riobahtiar
2 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
2 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
22 months 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
21 months 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
21 months 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
20 months ago

  • Version 4.8.3 deleted

#13 @riobahtiar
20 months 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.


20 months ago

#15 @adamsilverstein
20 months 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
20 months ago

#16 @stuartfeldt
20 months ago

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

#17 @stuartfeldt
20 months ago

  • Keywords dev-feedback added

#18 @adamsilverstein
20 months 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
17 months ago

Still present in 4.9.6.

#20 @adamsilverstein
17 months ago

  • Keywords needs-unit-tests added

This could also use unit tests.

#21 @adamsilverstein
17 months ago

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

#22 @danielbachhuber
13 months 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
13 months 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
7 months 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
10 days ago

Any update on this issue?

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


9 days ago

#27 @adamsilverstein
9 days 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
40 hours ago

#48314 was marked as a duplicate.

Note: See TracTickets for help on using tickets.