New HTML cleanup in post contents is too extreme

[jankusanagi]

I noticed that certain posts containing img tags don't display the expected image, when seen from an up-to-date server, running the git version.

Then I checked, and I saw that the "content" field in the JSON itself was being "sanitized". It's not a matter of webUI display, the received JSON itself is different, for any client.

The culprit seems to be this commit: 49dcced

The relevant line has this comment:
// using defaults
// { allowedTags: ['b', 'i', 'em', 'strong', 'a'], allowedAttributes: { a: ['href']}}

So this could probably be easily configured to allow style and img tags without compromising security.

As it is now, users of up-to-date git servers are not seeing posts with img tags in them, which is quite common to have in the Pump network.

Also, less serious but still annoying, the fact that the style tags are removed makes most of the styling produced by the Dianara client invalid. Dianara uses Qt's richtext engine, which means that most of the visual stuff depends on style tags. Even b is turned into a style someParams.

P.S.- BTW, I've now checked that this alterations happen also with outgoing stuff, so posting with an img tag is not possible either. Those will be silently dropped from people's posts.

P.S.2.- Even if the post doesn't use those 'forbidden' tags, the 'sanitation' seems to add extra p tags between lines which were separated by br, or something like that. However it's done, it seems to result in posts with more spaces between lines. It's not very nice.

[jankusanagi]

I'd say it's critical to fix this before 1.0

I think the relevant commit was by @profOnno. Mentioning him and @strugee here so they can take a look ;)

[erincandescent]

Whoa whoa whoa. It's stripping the content of messages coming from other servers? This is totally wrong.

Post content should be left unmodified. Sanitization is a client (in this case Web UI) matter

[strugee]

@oshepherd AFAIK that's the intended behavior. The reason it's done is for anti-XSS - the intention isn't to screw around with anyone's content.

/cc @evanp

[strugee]

@jankusanagi related issue: we can't allow arbitrary CSS.

http://stackoverflow.com/questions/3607894/cross-site-scripting-in-css-stylesheets

[strugee]

I merged this because now master is at least less broken than it used to be, but that being said, we still need feedback from @evanp. Reopening.

[erincandescent]

Yes. Sanitising the HTML that is displayed is fine and correct.

Modifying the data sent over the wire is wrong. The JSON and embedded
content of an object sent over the wire should be unchanged.

Sanitising content before display is a client problem, not a server
problem. Note that the web UI and clients render raw content from remote,
untrusted servers. You cannot perform this sanitising server side.
On 2 Jun 2016 2:38 a.m., "Alex Jordan" [email protected] wrote:

@oshepherd https://github.com/oshepherd AFAIK that's the intended
behavior. The reason it's done is for anti-XSS - the intention isn't to
screw around with anyone's content.

/cc @evanp https://github.com/evanp

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#1169 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAhH3ALaSvI1vteyzKSgnAWz-SpJvsNGks5qHjP5gaJpZM4IpJ95
.

[strugee]

Relevant spec sections:

http://activitystrea.ms/specs/json/1.0/#republisher
http://activitystrea.ms/specs/json/1.0/#security

Honestly, I'm not quite sure if we're violating spec or not.

In any case, I sent an email to Evan. It's his call to make, not mine.

[erincandescent]

It seems pretty clear to me from:

When a Re-publisher transmits an object, the Re-publisher MUST maintain the
full integrity of theobject, including any extension properties, and retain
the original id value

That modifying an object is prohibited.

Regardless, as said, the web UI accesses data from remote servers hence
must be capable of local sanitisation of objects
On 2 Jun 2016 9:53 p.m., "Alex Jordan" [email protected] wrote:

Relevant spec sections:

http://activitystrea.ms/specs/json/1.0/#republisher
http://activitystrea.ms/specs/json/1.0/#security

Honestly, I'm not quite sure if we're violating spec or not.

In any case, I sent an email to Evan. It's his call to make, not mine.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1169 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAhH3IhH6VQwzpKO9V7O31GeiYKW1xHRks5qH0LIgaJpZM4IpJ95
.

[strugee]

This was discussed in the last meeting and we're going to continue to sanitize incoming objects, even though it's a violation of spec.

@oshepherd I'll look into the web UI too, and make sure it handles this properly.

[strugee]

FYI the plan for this is at cure53/DOMPurify#176

CSS will be retained and there will be very little to no impact on "good" objects.

[strugee]

Fixed in master.