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

New HTML cleanup in post contents is too extreme #1169

Closed
jankusanagi opened this Issue May 28, 2016 · 11 comments

Comments

Projects
None yet
3 participants
@jankusanagi
Contributor

jankusanagi commented May 28, 2016

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 jankusanagi changed the title from New HTML cleanup is too extreme to New HTML cleanup in post contents is too extreme May 28, 2016

@jankusanagi

This comment has been minimized.

Show comment
Hide comment
@jankusanagi

jankusanagi May 30, 2016

Contributor

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 ;)

Contributor

jankusanagi commented May 30, 2016

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

This comment has been minimized.

Show comment
Hide comment
@erincandescent

erincandescent May 30, 2016

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

erincandescent commented May 30, 2016

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 strugee self-assigned this Jun 1, 2016

strugee added a commit that referenced this issue Jun 1, 2016

@strugee

This comment has been minimized.

Show comment
Hide comment
@strugee

strugee Jun 2, 2016

Member

@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

Member

strugee commented Jun 2, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@strugee
Member

strugee commented Jun 2, 2016

@strugee strugee closed this in #1171 Jun 2, 2016

@strugee

This comment has been minimized.

Show comment
Hide comment
@strugee

strugee Jun 2, 2016

Member

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.

Member

strugee commented Jun 2, 2016

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.

@strugee strugee reopened this Jun 2, 2016

@erincandescent

This comment has been minimized.

Show comment
Hide comment
@erincandescent

erincandescent Jun 2, 2016

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" notifications@github.com 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
.

erincandescent commented Jun 2, 2016

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" notifications@github.com 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

This comment has been minimized.

Show comment
Hide comment
@strugee

strugee Jun 2, 2016

Member

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.

Member

strugee commented Jun 2, 2016

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

This comment has been minimized.

Show comment
Hide comment
@erincandescent

erincandescent Jun 3, 2016

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" notifications@github.com 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
.

erincandescent commented Jun 3, 2016

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" notifications@github.com 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 strugee added this to the 1.0.0 milestone Jul 13, 2016

@strugee

This comment has been minimized.

Show comment
Hide comment
@strugee

strugee Aug 11, 2016

Member

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.

Member

strugee commented Aug 11, 2016

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

This comment has been minimized.

Show comment
Hide comment
@strugee

strugee Aug 16, 2016

Member

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.

Member

strugee commented Aug 16, 2016

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 strugee referenced this issue Aug 20, 2016

Closed

Implement DOMPurify-based XSS scrubber #1184

8 of 8 tasks complete
@strugee

This comment has been minimized.

Show comment
Hide comment
@strugee

strugee Aug 25, 2016

Member

Fixed in master.

Member

strugee commented Aug 25, 2016

Fixed in master.

@strugee strugee closed this Aug 25, 2016

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