miniflux / v2

Minimalist and opinionated feed reader
https://miniflux.app
Apache License 2.0
6.66k stars 708 forks source link

Render feed's entries in a sandboxed iframe #2396

Open jvoisin opened 6 months ago

jvoisin commented 6 months ago

The web is a scary place, and processing untrusted data from websites to display them in a browser window is delicate security-wise to say the least.

Enter sandboxed iframes! Designed to make it easy for everyone to render untrusted data in a trusted context. It would thus be nice to make use of one to render feed's entries. I know that there is some CSP in place, but there are myriads of way a webpage could to eldritch things, so having a simple yet comprehensive/browser-enforced security layer would make everyone sleep better at night.

The change itself it pretty trivial, in internal/template/templates/views/entry.html, change it to something like this:

+        <iframe credentialless sandbox="allow-scripts allow-same-origin" frameborder="0" srcdoc="
+        <link rel='stylesheet' type='text/css' href='{{ route "stylesheet" "name" .theme "checksum" .theme_checksum }}'>
        {{ if .user }}
+       {{ (proxyFilter .entry.Content) }}
-       {{ noescape (proxyFilter .entry.Content) }}
        {{ else }}
+       {{ .entry.Content }}    
-       {{ noescape .entry.Content }}
        {{ end }}
+        "></iframe>

Some CSS should likely be add too to make it looks less awful. The main issue is that there is no way to properly size an iframe, meaning that it'll have a scrollbar.

fguillot commented 6 months ago

There is a HTML sanitizer in addition to the CSP. External iframes are also sandboxed.

Loading entry content in a sandboxed iframe might be a good idea. Having an extra layer of security cannot hurt. However, I'm wondering if there are any downsides in terms of accessibility, especially for screen readers.

jvoisin commented 6 months ago

The sanitizer looks scary and a bit brittle: parsing html/mathml/svg like browsers do is highly non-trivial; see all the hoops DOMPurify has to jump through. Moreover, it seems that it's manually concatenating strings to create html, with a mix of whitelist and blacklists, … while I loath adding new dependencies as much as the next person, Google's safehtml might be able to help a bit there.

As for accessibility, it's indeed a valid concern. The simple solution implementation-wise would be to add a preferences ("Enable sandboxed rendering of feed entries (might impact accessibility)"), but it means adding yet another option to miniflux.