Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Render feed's entries in a sandboxed iframe #2396

Open
1 task done
jvoisin opened this issue Feb 24, 2024 · 2 comments
Open
1 task done

Render feed's entries in a sandboxed iframe #2396

jvoisin opened this issue Feb 24, 2024 · 2 comments

Comments

@jvoisin
Copy link
Contributor

jvoisin commented Feb 24, 2024

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
Copy link
Member

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
Copy link
Contributor Author

jvoisin commented Feb 25, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants