How much should we trust other authors?

etc · 2023-12-02 21:02:54

This post is related to this issue, but is of a broader concern.

We know txp allows any author to insert all kind of html code (frames, scripts, styles etc) in article body/excerpt. Textile is rather permissive re html tags and will not sanitize the content too much. Per se, this is an open door for all kind of XSS (and CVS reports). Fortunately, restricted authors can only save, but not publish their articles.

We have always said that the security is under admins responsibility. They should check the articles content before publishing. The only way to do it safely is to inspect the sources, but that’s cumbersome. Viewing an evil author’s article is risky, even on the public side (scripts, clickjacking etc). And if we sandbox the preview, a potentially unsafe content might not be detected.

So the question is: do we need some kind of code inspector/sanitizer, to help site admins? Or should it rather be a plugin, since most txp admins are the sole authors of their site?

wet · 2023-12-03 01:54:44

etc wrote #335979:

Or should it rather be a plugin, since most txp admins are the sole authors of their site?

+1

phiw13 · 2023-12-03 07:18:42

The plugin route seems more appropriate, or less disruptive¹.

One of your suggestions in that GH issue “Restrict Textile in previews, or even reserve ‘text’ mode to the article author” seems one possible built-in option, if it can easily/safely accomplished, but it is by no means complete, as you note that the Preview is limited (e.g TXP shortcode is not available).

As an editor/admin, I mostly use the Preview when editing articles with a “live” status as a way to verify(sanity check) the edits without saving – to view the edits otherwise requires one to save the article first, which for live articles could be problematic.

¹ with the plugin mentioned in the “security” documentation pages.

Bloke · 2023-12-03 10:41:25

Plugin: com_something so we can keep it up to date with changes and adapt security between releases if necessary.

I hadn’t thought of the issue that sandboxing the preview might allow harmful content through because it wouldn’t be executed. That’s a really important point. Balancing how much exposure to harm we open authors of the admin side up to vs how much potential harm it could wreak if sent into the wild is of primary concern.

I would always rather the limited attack vector of the back end to the front. But that is muddied slightly with the risk of privilege escalation and for sites that use the txp_user table to manage community logins.

etc · 2023-12-03 16:03:57

We seem to agree that body/excerpt preview is only for the (structured) content, not the functionality (scripts). It looks natural then to remove all “unsafe” elements like scripts, frames and so on from preview, even if it’s our own article. Otherwise, “live” previews could be too expensive (script execution, frames loading and so on). It’s even more true when previewing someone else’s article.

So we need some html sanitizer. As native browser options, I’ve tried

<iframe sandbox />: works great, but is terrible UX-wise.
Sanitizer API: works great, but is still experimental.

Other options include either custom scripts or third-party libraries, like DOMPurify. I’d be inclined to the latter, since it is only 20kb minified, works great and can provide feedback when something unsafe is detected. If needed, it can be plugged into “Publish” step, to warn the admin that something is suspicious.

Thoughts?

Bloke · 2023-12-03 16:16:17

If it’s only a small footprint then DomPurify gets my vote.

colak · 2023-12-04 16:26:27

I’m currently administering 2 sites with multiple authors. They add articles and images (though shortcodes) and they are all trustworthy.

I can however understand how some sites may want to open their back-end for communities they may not have an IRL relationship with.

For the sake of backward compatibility, I would instead add another one which has very limited publishing rights. ie, limited textile, no html, no js, no php, etc. The question lies on the shortcodes which are necessary for the formatting of images or the insertion of external content from video and audio hosting platforms.

etc · 2023-12-05 10:57:19

Well, I’m sick of it. Pushed a first round of modifications to dev branch. Includes:

Body/Excerpt preview is put in shadow and sanitized by DOMPurify.
Article preview happens on the public side now, avoiding sending admin cookies. It is ~~put into sandboxed iframe and~~ output in a new sandboxed window ~~the same preview window as body/excerpt~~.
All cookies are httponly by default.

Needs some UX love. Please test by throwing all kind of XSS and previewing.

TBC

etc · 2023-12-05 17:06:00

colak wrote #336007:

For the sake of backward compatibility, I would instead add another one which has very limited publishing rights. ie, limited textile, no html, no js, no php, etc. The question lies on the shortcodes which are necessary for the formatting of images or the insertion of external content from video and audio hosting platforms.

You are right, but txp tags can also be an injection vector. Though we could allow just <txp::shortcodes /> and strip everything else.

phiw13 · 2023-12-06 07:15:59

Pushed a first round of modifications […]

I had a look using various articles and playing around just a little (Safari and Firefox):

Sad: the browser revert to the default stylesheet, except for some text styling (colour, font-family, font-size) – that is probably expected given the sandboxed character of the rendering.
fairly responsive, the “Live preview” still works (I don’t think I ever used it, though)
at one point, while the preview window was open, I got a warning messagepane: “Possibly unsafe body”. The ‘payload” was inline textile formatting such as aligning p>. or indenting p))). and similar (all result appending a style attribute to an element).
a stupid JS onclick=dosomething() was equally kindly dispatched into doing nothing.

Nice work so far.

etc · 2023-12-06 11:12:02

phiw13 wrote #336033:

I had a look using various articles and playing around just a little (Safari and Firefox):

Sad: the browser revert to the default stylesheet, except for some text styling (colour, font-family, font-size) – that is probably expected given the sandboxed character of the rendering.

Thanks for testing. Yes, the preview is isolated, otherwise eventual styles used in article body/excerpt would leak into the main document. We could inject all document styles into the preview, but this is too expensive. Probably, theme authors could provide some small preview.css stylesheet?

phiw13 · 2023-12-06 11:31:05

Probably, theme authors could provide some small preview.css stylesheet?

That would be an eventual option – and it does need to be much as 1/ at least the main colour and font-styling are already in use and 2/ the document is probably better left as simple as possible.

But how would that work to hook it up into the sandboxed document ?

Textpattern CMS

Textpattern CMS support forum

#1 2023-12-02 21:02:54