wp-shortcake / shortcake-bakery

A fine selection of shortcodes for WordPress
42 stars 16 forks source link

Allow plugins to filter iFrame and Script embed sources. #189

Closed ryanboswell closed 7 years ago

ryanboswell commented 7 years ago

This makes it easy to retroactively apply HTTPS schemes to existing embeds when prepping a site to serve correctly over HTTPS.

goldenapples commented 7 years ago

I keep trying to think of ways in which this can be be extended to cover more use cases.

For one, even using this same API, if the filter were applied in the callback function rather than in the force_ssl() method, it could get passed the script/iframe url as context, in case that matters. You might want to enforce SSL for all script URLs, except that you know there's one or two that don't work.

Beyond that, if the filter was just a filter around the src, then sites could do whatever they wanted with it, like proxying it through another domain if needed, or changing the hostname in the case on embed providers that use a different hostname for ssl vs non-ssl. (We could provide a helper function to cover the most common case, replacing http: with https:. If the theme could just do something like add_filter( 'shortcake_bakery_iframe_src', 'shortcake_bakery_force_ssl' );, that would be just as simple from the theme as this is, but it could also be extended easily to cover edge cases.

Do either of these make sense as suggestions? Or is it better to keep it simpler like this?

ryanboswell commented 7 years ago

I keep trying to think of ways in which this can be be extended to cover more use cases.

I was actually giving it some more thought myself late last night and this morning. My main thoughts centered around passing better context to this filter to allow plugins to determine more conditionally whether to force SSL or not.

But I also kinda like your thought of being able to filter the src itself, and as you point-out some providers might use a different hostname for SSL so this implementation wouldn't cover that. And it covers the force SSL case we want here. So I'm in favor of moving in that direction.

I guess the question is how this should play with the existing whitelist domains filter. Should that filter get run before or after we filter the URL? I'm inclined to say after, for the sake of being extra cautious with security, but open to any arguments for putting it before.

goldenapples commented 7 years ago

Should that filter get run before or after we filter the URL? I'm inclined to say after, for the sake of being extra cautious with security, but open to any arguments for putting it before.

Oh, good question. Hmm.

The main use case I'm thinking of is something like this: someone builds infographics that are hosted on an S3 bucket with a domain like assets.domain.com to be iframed into a post. But that CNAME doesn't have SSL set up on it, so over https it has to be served through a Cloudfront distribution like https://domain.cloudfront.net. In theory the WP author entering this could enter the embed with either of those domains, and so both would have to be added to the whitelist.

I guess I'm with you on running the whitelist domains filter after filtering the src. This would allow for a fallback, like redirecting to a proxy host or just a "not available" page from the iframe src if an https version isn't available or the src isn't whitelisted.

ryanboswell commented 7 years ago

@goldenapples ok, check out what I've got now. I wasn't sure where to put a helper function in the plugin, but something like this should work to force SSL blindly:

add_filter( 'shortcake_bakery_script_src', function( $src ){
    return set_url_scheme( $src, 'https' );
});
goldenapples commented 7 years ago

Oh, yeah. That usage is simple enough that it really doesn't need a helper function. :+1:

I like it. :ship: