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

Define the supported HTML subset for message events #1562

Merged
merged 6 commits into from Aug 28, 2018

Conversation

4 participants
@turt2live
Member

turt2live commented Aug 26, 2018

Rendered: see 'docs' status check


Also clarify that m.notice messages can support HTML.

Reference: https://github.com/matrix-org/matrix-react-sdk/blob/2dc94ac277bfaed6e7a8116ff08bba22ee8fb642/src/HtmlUtils.js#L179-L291

Fixes #1559
Fixes #1560

Define the supported HTML subset for message events
Also clarify that `m.notice` messages can support HTML.

Fixes #1559
Fixes #1560

@turt2live turt2live requested a review from matrix-org/spec-core-team Aug 26, 2018

@turt2live turt2live added this to In review (just the PRs) in August 2018 r0 via automation Aug 26, 2018

``class`` (only classes which start with ``language-`` for syntax highlighting)
Additionally, clients should ensure that *all* ``a`` tags get a ``rel="noopener"``

This comment has been minimized.

@ara4n

ara4n Aug 27, 2018

Member

s/clients/web clients/

This comment has been minimized.

@Half-Shot

Half-Shot Aug 27, 2018

Contributor

+1 to keeping tables, lots of bots n things use it. The discord bridge will be using it to display discords tables in clients.


Apparently FastHub (android client) has rendered all inline comments in one thread, will move this comment when at a PC

injection, and similar attacks. The strongly suggested set of HTML tags to permit,
denying the use and rendering of anything else, is: ``font``, ``del``, ``h1``,
``h2``, ``h3``, ``h4``, ``h5``, ``h6``, ``blockquote``, ``p``, ``a``, ``ul``,
``ol``, ``sup``, ``sub``, ``nl``, ``li``, ``b``, ``i``, ``u``, ``strong``, ``em``,

This comment has been minimized.

@ara4n

ara4n Aug 27, 2018

Member

i have no idea what nl is doing in there. i'd be inclined to remove it.

:``img``:
``width``, ``height``, ``alt``, ``title``, ``src`` (provided it is a Matrix Content
URI)

This comment has been minimized.

@ara4n

ara4n Aug 27, 2018

Member

"provided it uses the mxc:// scheme" perhaps

This comment has been minimized.

@turt2live

turt2live Aug 27, 2018

Member

My concern is that people generally don't know how the media repository works, or what an "mxc://" scheme is. To be fair, a "Matrix Content URI" also doesn't click.

A link on mxc:// maybe?

This comment has been minimized.

@uhoreg

uhoreg Aug 27, 2018

Member

I think that making "Matrix Content URI" to the section where it's defined (#module-content) would be helpful. In that section, it's called "Matrix Content (MXC) URI", so I think we should go with that.

plain text version of the HTML should be provided in the ``body``.
Clients should limit the HTML they render to avoid Cross-Site Scripting, HTML
injection, and similar attacks. The strongly suggested set of HTML tags to permit,

This comment has been minimized.

@ara4n

ara4n Aug 27, 2018

Member

we should also specify a maximum nesting of the tags, to avoid thousands of nested b-tags crashing browsers.

This comment has been minimized.

@turt2live

turt2live Aug 27, 2018

Member

Does arbitrarily picking the number 42 seem reasonable?

This comment has been minimized.

@turt2live

turt2live Aug 27, 2018

Member

ftr went with 100 further down

This comment has been minimized.

@ara4n

ara4n Aug 27, 2018

Member

(i was worried with 42 being too small given scope for legitimate nesting from RTE copy-paste antics)

to display if available. Currently ``m.text``, ``m.emote``, and ``m.notice``
support an additional ``format`` parameter of ``org.matrix.custom.html``. When
this field is present, a ``formatted_body`` with the HTML must be provided. The
plain text version of the HTML should be provided in the ``body``.

This comment has been minimized.

@ara4n

ara4n Aug 27, 2018

Member

might be worth a informational link through to #1225 as an FYI on how this might evolve in general, to reassure people we're not baking in the crappy formatted_body thing forever. (do we have a way of doing informational blocks in the spec atm, which aren't normative but just FYI?)

This comment has been minimized.

@turt2live

turt2live Aug 27, 2018

Member

We can do a "note" which ends up being a dark gray box of text.

This comment has been minimized.

@turt2live

turt2live Aug 27, 2018

Member

added a note further down

``h2``, ``h3``, ``h4``, ``h5``, ``h6``, ``blockquote``, ``p``, ``a``, ``ul``,
``ol``, ``sup``, ``sub``, ``nl``, ``li``, ``b``, ``i``, ``u``, ``strong``, ``em``,
``strike``, ``code``, ``hr``, ``br``, ``div``, ``table``, ``thead``, ``tbody``,
``tr``, ``th``, ``td``, ``caption``, ``pre``, ``span``, ``img``.

This comment has been minimized.

@ara4n

ara4n Aug 27, 2018

Member

I suggest we remove the table stuff (table, thead, tbody, tr, th, td, caption) from this list as nobody uses it, and it makes implementation much harder on non-web-based clients. We could include it as a "you could always whitelist this if your client can render it safely, but it's not currently recommended" perhaps.

This comment has been minimized.

@turt2live

turt2live Aug 27, 2018

Member

I use tables all the time in various alerts from things. It's the closest we have to arbitrary fields like slack.

This comment has been minimized.

@ara4n

ara4n Aug 27, 2018

Member

okay.

@ara4n

This comment has been minimized.

Member

ara4n commented Aug 27, 2018

lgtm other than comments

@turt2live turt2live requested a review from ara4n Aug 27, 2018

@turt2live turt2live removed their assignment Aug 27, 2018

August 2018 r0 automation moved this from In review (just the PRs) to Reviewer approved Aug 27, 2018

@ara4n

ara4n approved these changes Aug 27, 2018

lgtm!

@turt2live turt2live merged commit e4f8c23 into matrix-org:master Aug 28, 2018

4 checks passed

ci/circleci: build-docs Your tests passed on CircleCI!
Details
ci/circleci: build-swagger Your tests passed on CircleCI!
Details
docs Click details to preview the HTML documentation.
Details
swagger Click to preview the swagger build.
Details

August 2018 r0 automation moved this from Reviewer approved to Done (this list will be incomplete) Aug 28, 2018

@turt2live turt2live deleted the turt2live:travis/c2s/html-formatting branch Aug 28, 2018

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