WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 5 months ago

Last modified 2 days ago

#40450 closed enhancement (fixed)

Introduce REST API endpoint for proxying requests to external oEmbed providers

Reported by: westonruter Owned by: jnylen0
Milestone: 4.8 Priority: normal
Severity: normal Version:
Component: Embeds Keywords: has-patch has-unit-tests commit
Focuses: rest-api Cc:

Description (last modified by westonruter)

This essentially proposes adding the functionality from noembed.com into WordPress.

When doing oEmbed previews in the client, both in the media modal embed frame and in the TinyMCE embed preview, a parse-embed admin-ajax request is currently made to do the parsing. This request requires a post_ID parameter because the WP Embed logic caches oEmbed responses in postmeta (see #34115). There is a need for this in the media widgets to be able to obtain the thumbnail_url for a given oEmbed URL. Unfortunately the oEmbed preview in the media modal fails because there is no post_ID for context. Also, the client cannot reliably make requests to oEmbed providers directly since they inconsistently support CORS.

There is currently an wp-json/oembed/1.0/embed endpoint for serving oEmbed responses for posts on a given site. It would be a useful improvement to extend this endpoint to also allow for proxying the oEmbed provider lookup and fetching, as handled internally in \WP_oEmbed::get_html(): https://github.com/WordPress/wordpress-develop/blob/4.7/src/wp-includes/class-oembed.php#L333-L360

I understand there is also a concern for using such an embed endpoint for doing DDoS attacks, so I imagine any external URL lookups would require auth.

Once this is implemented, the use of the embed endpoint can potentially replace the parse-embed request, at least when there is no post_ID for context.

For background, see:
https://wordpress.slack.com/archives/C02RQC26G/p1492038272597233
https://github.com/xwp/wp-core-media-widgets/pull/53#issuecomment-294131744

Attachments (3)

40450.0.diff (2.7 KB) - added by westonruter 6 months ago.
40450.diff (4.5 KB) - added by swissspidy 5 months ago.
40450.1.diff (22.7 KB) - added by jnylen0 5 months ago.
https://patch-diff.githubusercontent.com/raw/xwp/wordpress-develop/pull/225.diff

Download all attachments as: .zip

Change History (27)

@westonruter
6 months ago

#1 @westonruter
6 months ago

  • Keywords has-patch needs-unit-tests added
  • Owner set to swissspidy
  • Status changed from new to reviewing

#2 @westonruter
6 months ago

  • Description modified (diff)
  • Milestone changed from Awaiting Review to 4.8

The patch guards against DDoS by requiring the edit_posts cap. It does't currently cache responses, but I'm guessing this would be added from what lands for #34115.

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


6 months ago

#4 @westonruter
6 months ago

Opened a PR for collaboration and running core unit tests via Travis: https://github.com/xwp/wordpress-develop/pull/225

#5 @timmydcrawford
6 months ago

Once this is implemented, the use of the embed endpoint can potentially replace the parse-embed request, at least when there is no post_ID for context.

@westonruter do we have an idea of what might be needed to make this new endpoint work in the context of a particular post? It appears the current logic in the associated admin-ajax function is to verify the user has access to edit the given post : https://github.com/xwp/wordpress-develop/blob/master/src/wp-admin/includes/ajax-actions.php#L2945-L2951

Would it be worth exploring supporting a post_ID option for the endpoint too? I think it would be great to ship this new endpoint with all the needed features to replace parse_embed usage from the get-go.

#6 @westonruter
6 months ago

@timmydcrawford The only reason for the post_ID for context is to that the embed can be cached in a post's postmeta so that when the post gets viewed on the frontend it won't cause the site to crash with external requests. The capability check then I guess is to prevent some unauthorized user from filling up postmeta on a given post with oEmbed data.

To me it seems that for the purposes of the REST API endpoint, we can just use transients instead, if we even really need caching here. The media library JS can then be updated to just go ahead and use the embed proxy endpoint and ignore whether it has a current post as context. Just like this: https://github.com/xwp/wordpress-develop/pull/225/commits/2bb963d

#7 @westonruter
6 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#8 @westonruter
6 months ago

  • Keywords commit added

I'm proposing for commit. Any last feedback on the patch prior to doing so?

Ref: https://github.com/xwp/wordpress-develop/pull/225

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


6 months ago

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


6 months ago

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


5 months ago

@swissspidy
5 months ago

#12 @swissspidy
5 months ago

40450.diff reuses more parts of the oEmbed class to make things more DRY. Otherwise this looks good to me.

Extending Test_oEmbed_Controller would have been great for this, but we can also do that at a later point.

#13 @jnylen0
5 months ago

The latest version of the PR (https://github.com/xwp/wordpress-develop/pull/225) has a lot more changes than either of the patches here, including unit tests and updating the media modal to use the new proxy endpoint.

Everything tests out well and looks good to me. I think it's ready for commit.

#14 @westonruter
5 months ago

The patches attached to this ticket are just copies of older versions of the PR. Now pending Travis build on PR for green light.

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


5 months ago

#16 @jnylen0
5 months ago

  • Owner changed from swissspidy to jnylen0
  • Status changed from reviewing to accepted

#17 @jnylen0
5 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 40628:

REST API: Add endpoint for proxying requests to external oEmbed providers.

This endpoint is a prerequisite for the media widgets work (see https://github.com/xwp/wp-core-media-widgets).

Also use the new endpoint in the media modal instead of the parse-embed AJAX action.

Props westonruter, timmydcrawford, swissspidy, jnylen0.
Fixes #40450.

#18 @westonruter
5 months ago

In 40641:

Widgets: Utilize WP REST API oEmbed proxy endpoint in media widgets.

Props timmydcrawford.
See #32417, #40450.

This ticket was mentioned in Slack in #core-customize by obenland. View the logs.


5 months ago

#20 @westonruter
3 months ago

In 41035:

REST API: Remove _wpnonce value from being used in hashed oEmbed proxy cache key.

Amends [40628].
Props r-a-y, westonruter.
See #40450.
Fixes #41048.

#21 @westonruter
3 months ago

In 41047:

REST API: Ensure maxwidth and maxheight params are forwarded to oEmbed provider in proxy requests.

Also correct phpdoc return tag on WP_oEmbed_Controller::get_proxy_item() and remove dead code in oEmbed controller phpunit tests.

Amends [40628].
See #40450.
Fixes #41299.

#22 @westonruter
3 months ago

In 41048:

REST API: Remove _wpnonce value from being used in hashed oEmbed proxy cache key.

Merges [41035] to 4.8 branch.
Amends [40628].
Props r-a-y, westonruter.
See #40450.
Fixes #41048 for 4.8.1.

#23 @westonruter
3 months ago

In 41049:

REST API: Ensure maxwidth and maxheight params are forwarded to oEmbed provider in proxy requests.

Also correct phpdoc return tag on WP_oEmbed_Controller::get_proxy_item() and remove dead code in oEmbed controller phpunit tests.

Merges [41047] into 4.8 branch.
Amends [40628].
See #40450.
Fixes #41299 for 4.8.1.

#24 @westonruter
2 days ago

In 41913:

Widgets: Fix previewing embeds in Text widget by allowing parse-embed admin ajax requests with an empty post_ID just as WP_oEmbed_Controller::get_proxy_item_permissions_check() allows.

As of #34115 if there is no post context the oEmbed will be cached in an oembed_cache custom post type, so having a post as context is no longer a requirement for caching.

Props biskobe, westonruter.
See #34115, #40450.
Fixes #40854.

Note: See TracTickets for help on using tickets.