FreshRSS / FreshRSS

A free, self-hostable news aggregator…
https://freshrss.org
GNU Affero General Public License v3.0
9.36k stars 806 forks source link

[BUG] Large emojis for WordPress blogs (again) #4104

Open Krinkle opened 2 years ago

Krinkle commented 2 years ago

Describe the bug

To Reproduce Steps to reproduce the behavior:

  1. Subscribe to a WordPress blog, such as the WordPress development blog: https://make.wordpress.org/core, https://make.wordpress.org/core/feed/
  2. View post containing an emoji via web client.
  3. Connect to FreshRSS API in NetNewsWire, and view same post there.

Expected behavior Native emoji, or otherwise an appropiately-sized image.

Screenshots

FreshRSS web client (Firefox) NetNewsWire on macOS NetNewsWire on iOS
Screenshot 2021-12-31 at 18 19 06 capture IMG

Environment:

Additional context

It seems FreshRSS strips the class attributes and all inline styles, and does so not just in its web client, but also in the content served through the API for other clients that have their own formatting. Although having said that, the first screenshot shows that this appears to be an issue in the default web client as well, which suggests the current fix has also stopped working.

Finally, I note that the example WordPress post in question, is serving native emojis from the served HTML:

<p>… <strong>305 people made their very first contribution to WordPress
Core</strong> ♥️</p>

However, the RSS feed at https://make.wordpress.org/core/feed/ serves an image instead, which suggests upstream WordPress is either applying the img transform only to the feed and/or applying a native re-transform to the theme rendering only.

<strong>305 people made their very first contribution to WordPress Core</strong>
<img src="https://s.w.org/images/core/emoji/13.1.0/72x72/2665.png" alt="♥" class="wp-smiley" style="height: 1em; max-height: 1em;" /></p>
Frenzie commented 2 years ago

There is no such thing as a current fix. The linked issue was closed because WP switched to Unicode emojis, but perhaps they never actually did for feeds.

Alkarex commented 2 years ago

Hello, In the CustomCSS extension (see https://github.com/FreshRSS/Extensions ), you can add such a style as a workaround:

img[src^="https://s.w.org/images/core/emoji/"] {
    height: 1em;
    max-height: 1em;
}
Alkarex commented 2 years ago

P.S. Allowing custom classes and styles in the HTML we render and/or serve back is not trivial from a security and robustness point of view

Frenzie commented 2 years ago

@Alkarex What about security? Robustness is simple enough with a prefix.

Krinkle commented 2 years ago

P.S. Allowing custom classes and styles in the HTML we render and/or serve back is not trivial from a security and robustness point of view

I recognise this for the web client where the HTML is served from an authenticated origin (i.e. the user's FreshRSS server), however would it be possible to apply the class stripping only in the web rendering and not in the general feeds/APIs?

Robustness is simple enough with a prefix.

Ack. Potentially a prefix like freshrss- or fr- could be reserved for the web client's own styles, and allow any other classes to pass through unaltered. However, this is non-trivial currently given the use of Bootstrap CSS, which is well-known for its generic class names. And while those may be harmless from a security prespective (e.g. anything specific to FreshRSS or used by its JavaScript could use a prefixed class), they are also prone to unintended matching due to their generic nature.

I think it's fair to strip classes in the web client as a final step, knowing that no foreign CSS/JS is applied anyhow, and that any common styles are already accomodated for by the web client with its own stylesheets etc. However when the feed API is consumed by another feed reader, that contexts doesn't apply, and in particular the FreshRSS styles aren't applied, as the feed reader has its own way of doing this, and like FreshRSS, similarly expects access to the original content to process it accordingly with various sanitiziation steps and/or added CSS.

Frenzie commented 2 years ago

Ack. Potentially a prefix like freshrss- or fr- could be reserved for the web client's own styles, and allow any other classes to pass through unaltered. However, this is non-trivial currently given the use of Bootstrap CSS, which is well-known for its generic class names. And while those may be harmless from a security prespective (e.g. anything specific to FreshRSS or used by its JavaScript could use a prefixed class), they are also prone to unintended matching due to their generic nature.

That's a non-issue. You would however want to change class="flux_content" to class="flux_content f_##". Keep in mind I'm talking from a CustomCSS perspective. There's nothing that can be done here generically.

By robustness I mean it always works exactly the way you expect it to work. I don't mean you don't have to do anything.

I think it's fair to strip classes in the web client as a final step

I think it's annoying to strip it. Prefix it and all is well.

Krinkle commented 2 years ago

think it's fair to strip classes in the web client as a final step, […].

I think it's annoying to strip it. Prefix it and all is well.

Ah, you mean to prefix the classes in the content (rather than adopt a prefix for the UI).

I hadn't considered that. I suppose the problem there might be that it is effectively the same as stripping, in that any downstream consumer of it will no longer recognise it. Not for workarounds like wp-smiley, nor for any semantic use of classes (e.g. hCard and other such HTML/RDF microformats).

However, if limited to the web client's own rendering, then (from my POV) prefixing and stripping are equally fine. If prefixing is preferred, that'd work for me. So long as e.g. the greader API and responses such as from a=rss pass-through the class attribute unchanged.

Alkarex commented 2 years ago

Maybe a whitelist of allowed classes could be acceptable.

math-GH commented 2 years ago

My plan is to implemente in SimplePie a new function, that rename attributes, so that class could be renamed to data-class and tags like noscript to data-tag. Than you can work with it in customCSS without a risk.

math-GH commented 2 years ago

For the class, I made a first solution, see #4175

Alkarex commented 1 year ago

Related article: https://danq.me/2023/09/04/wordpress-stop-emoji-images/

math-GH commented 1 year ago

Related article: https://danq.me/2023/09/04/wordpress-stop-emoji-images/

strange. I cannot reproduce it.

grafik

It is UTF8 and not an image grafik

Alkarex commented 1 year ago

strange. I cannot reproduce it.

If I am not mistaken, the article is precisely about how Dan fixed it (server side)

math-GH commented 1 year ago

ah ok. Dan fixed it in Wordpress and not in FreshRSS.