search

epiphyt / embed-privacy

Embed Privacy prevents loading of embedded external content and allows your site visitors to opt-in.
https://epiph.yt/en/embed-privacy/
GNU General Public License v2.0
19 stars 12 forks source link

Reconsider usage of output buffering #223

Closed kraftner closed 2 months ago

kraftner commented 3 months ago

What feature are you requesting?

Stop using output buffering to put the inline styles into the <header>. Basically revert f603adc / #208 or find a better solution

Why is this feature necessary?

Using output buffering is always risky, especially in public code. Your code is assuming that the buffer is only flashed after all content has been processed. This assumption will not apply in a lot of cases.

You never know who else is using output buffering. A lot of plugins out there are misusing output buffering and if any code clears the output buffer before the end of the content you'll end up with no or incomplete styles.

Some examples

This is a basic example how to break this intentionally:

add_action('wp_head', function(){
    $header = ob_get_clean();
    echo $header;
}, 11);

This will result in nothing being output at all since there is no </head> in the buffer yet.

A more realistic example is if some other plugin is trying to manipulate the body tag like this:

add_action('template_redirect', function (){
    ob_start();
});
add_action('wp_body_open', function(){
    $bodyTag = ob_get_clean();
    // Do something to the tag
    echo $bodyTag;
});

This will also result in nothing being output because now the other plugin starts a buffer first, but then you start a nested one which gets cleared by the other plugin right after the <body> tag. So when your callback runs it now has the </head> element but no style definitions yet since we are way before the content rendering.

And these are only some basic examples which hopefully make it clear that this approach will backfire sooner or later. Output buffering, especially of the whole site, is, if not always, at least 99.9% of the time a ticking time bomb.

So what then?

The original approach of having the style in the <body> might invalidate the HTML, but in practice everyone does it, all browsers support it. And for most people using blocks it will be invalid anyway. Personally, while I am fully in favor of valid HTML, I still think that in this case it would be the far lesser evil.

Another approach would be to go fully inline as proposed by @arnowelzel in https://github.com/epiphyt/embed-privacy/issues/198#issuecomment-1874185283 While I agree fighting against inline styles is annoying it again probably will cause less issues long term. You might even be able to make it easier by carefully crafting the CSS in a way that it uses CSS custom properties that can be overwritten without resorting to !important. Something in the spirit of this:

[data-embed-id="oembed_hash123"] {
    background-image: url(var(--ebd-hash123-background-image, 'https://example.org/wp-content/uploads/embed-privacy/thumbnails/youtube-hash123-maxresdefault.jpg');
}

which can then be easily overwritten by setting --ebd-hash123-background-image.

Are you expecing side-effects?

Depends on the approach - "invalid" HTML or inline CSS.

Code of Conduct

kraftner commented 3 months ago

As a temporary workaround I am using this snippet to go back to rendering the CSS in the <body>:

remove_action( 'wp_head', [ Embed_Privacy::get_instance(), 'start_output_buffer' ] );

add_action('wp_footer', function(){
    $style = Embed_Privacy::get_instance()->get_style();
    echo PHP_EOL . '<style id="embed-privacy-inline-css">' . PHP_EOL . $style . PHP_EOL . '</style>' . PHP_EOL;
});
MatzeKitt commented 3 months ago

While I understand you concern, I cannot reproduce the issue you’ve mentioned.

I created a MU plugin with this content:

<?php
add_action('template_redirect', function (){
    ob_start();
});
add_action('wp_body_open', function(){
    $bodyTag = ob_get_clean();
    // Do something to the tag
    echo $bodyTag;
});

And tested it on a site without Embed Privacy as well on a site with Embed Privacy and see no functional difference here. 🤔

For the first basic example: Of course, if someone misuses code and adds an ob_get_clean() without starting the output buffering, it’s just a misuse of PHP and effectivle an unsupported behavior.

Besides that: output buffering is stackable. As long as you don’t mess them up, they should work inside each other. See also:

Output buffers are stackable, that is, ob_start() may be called while another buffer is active.

Source: https://www.php.net/manual/en/function.ob-start.php

If you have a problem in your code, maybe it’s worth debugging the nesting level with ob_get_level or ob_get_status, see also: https://www.php.net/manual/en/outcontrol.nesting-output-buffers.php

kraftner commented 3 months ago

I am gonna start with the example so we have this out of the way. The steps:

  1. WP with Embed Privacy active. (I have WP 6.5 but shouldn't matter)
  2. Add a YouTube Video to a post
  3. Open the Post. You'll the see <style id="embed-privacy-inline-css"> at the end of <head>.
  4. Add the MU plugin as above and open the post again. The inline CSS will be gone.

The reason for this is that as you noted output buffering is stackable. What happens here is this:

  1. Buffer Level 1 started by MU plugin on template_redirect
  2. Buffer Level 2 started by Embed Privacy plugin on wp_head
  3. Buffer Level 2 cleared by MU plugin on wp_body_open. This triggers your callback. But way too early since the content isn't processed yet and there are no inline styles defined yet.
  4. Buffer Level 1 is cleared at the end of the document but that does nothing any more.

This shows why output buffering is very fragile. There are many ways things can break by not being run in the expected order. You could now of course try to fix this by checking when the callback runs, checking the stacking level, fall back to the <body> if anything is unexpected etc. But this just makes your code more complex and fragile. So, and this is personal opinion: Output buffering should not be used at all in public code. Only in private code where you know the full stack 100% and can fully control the order of things.

So I have no problem in my code, it also didn't break. I just noticed this when auditing the code changes and wanted to mention that from experience I know that this can easily and quite surely will break at some point with other 3rd party code. Especially since we both know that the quality of WP code often isn't the best. And this will then be one of those ugly hard to reproduce bugs and people will blame Embed Privacy for it.

You can of course have a different opinion here and decide not to change anything. If so I'd at least ask you to keep the possibility for the workaround or introduce some kind of filter to be able to decide between the output buffering and an alternative approach (inline styles of styles in <body>. :pray:

MatzeKitt commented 2 months ago

I actually could also find an issue with the first implementation: logos are missing from embeds. Will check for an alternative way.

MatzeKitt commented 2 months ago

I would love to hear your feedback on https://github.com/epiphyt/embed-privacy/commit/c61f68f7c5e6f47c72d3cb24c2f764b9185a6bb9

kraftner commented 2 months ago

I just had a look: TLDR: I generally like it.

I have to say I only skimmed the code, so don't take this is a full code review. Too many things moving around made the diff too hard to read in full. Anyway, let me tell you what I have noticed:

First of all splitting things into smaller, more readable chunks always gets a :+1: from me. Especially when you are taking things out of your God class. This will probably help with future maintenance and is something I'd love to see for more of the plugin code.

Going with inline styles probably also is the best middle ground that keeps the HTML valid while also getting rid of the output buffering.

I have tested the changes with a project that does some bigger customization, so it seems that all the refactoring didn't break any of the hooks. At least not any of those I have been using in any way that breaks my acceptance tests. :grin:

MatzeKitt commented 2 months ago

Thank you for your feedback! ❤️

I absolutely didn’t expect a full review, I just wanted to make sure that the change is probably something that works with what you’ve had in mind.

This will probably help with future maintenance and is something I'd love to see for more of the plugin code.

That’s also my main goal for this release, to split it and deprecate the old version so that I can start working on a clean version 2 with all the deprecations removed, soon.