SVG and MathML #137

otherdaniel · 2021-11-04T14:41:32Z

Support SVG and MathML embedded content in Sanitizer API.
Plus some cleanup in the spec, to support the main change.

otherdaniel · 2021-11-10T12:59:00Z

I just re-read cure53/DOMPurify#495 which documents a namespace-related issue with DOMPurify. I'll note that in this PR, the allow/block-list items are all namespaced, which would prevent at least their second example.

I do note that the DOMPurify PR goes substantially further and explicitly post-processes the parsed DOM-tree to disallow namespace transisitions except for a small number of allowed transition points. I wonder whether we should adopt a similar rule. I'm presently not convinced, but would appreciate if someone could double-check my thinking here.

securityMB · 2021-11-11T11:03:37Z

As an author of the DOMPurify PR let me add a few cents here:

I do note that the DOMPurify PR goes substantially further and explicitly post-processes the parsed DOM-tree to disallow namespace transisitions except for a small number of allowed transition points. I wonder whether we should adopt a similar rule. I'm presently not convinced, but would appreciate if someone could double-check my thinking here.

I am not really up-to-date with regard to the current state of the Sanitizer API but as far as I understand, currently no method for doing string-to-string sanitization is being provided. If that's the case, then I believe that we don't need to check the namespace transitions. The reasoning for that is that unexpected namespace transitions can basically only hurt if we're re-parsing the HTML.

mozfreddyb · 2021-11-11T11:50:43Z

I am not really up-to-date with regard to the current state of the Sanitizer API but as far as I understand, currently no method for doing string-to-string sanitization is being provided. If that's the case, then I believe that we don't need to check the namespace transitions. The reasoning for that is that unexpected namespace transitions can basically only hurt if we're re-parsing the HTML.

This seems to be aligned with my disapproval of sanitizerFor(el, text)).innerHTML hackery ;)

index.bs

mozfreddyb · 2021-11-11T11:54:17Z

index.bs

+Namespace designator and element names are seperated by a
+whitespace character. `svg` designates elements in the [=SVG namespace=], and


I remember why you said whitespaces, but I wonder if this is too obscure for users of the API. I think it would work better with existing tooling, if we'd stick to the colon character, i.e. using the qname.

Nit: I think you need to say "which" whitespace character, by referencing the unicode code point.

HTML5 makes no use of notion of qnames, and you can actually put a semicolon in an attribute name (not sure if anybody does that; but technically you can). Whitespace resolves the disambiguity as you cannot put it in the name.

mozfreddyb · 2021-11-11T11:55:37Z

index.bs

+Note: The [[HTML]] specification solves the problem of dstinguishing HTML
+  from "foreign" elements largely through the parse context. This means isn't
+  available to the Sanitizer [=configuration object=], since there is no
+  hierarchy or other relationship between configuration items. Therefore,
+  we introduce the explicit namespace designator.


maybe "isn't available to the sanitizer during configuration", because it is later on (but then it's too late)

index.bs

mikewest

Thanks for putting this together! A few comments inline, mostly nits.

index.bs

mikewest · 2021-11-12T07:18:42Z

index.bs

+
+</div>
+
+Note: The [[HTML]] specification solves the problem of dstinguishing HTML


Suggested change

Note: The [[HTML]] specification solves the problem of dstinguishing HTML

Note: The [[HTML]] specification solves the problem of distinguishing HTML

mikewest · 2021-11-12T07:18:53Z

index.bs

+</div>
+
+Note: The [[HTML]] specification solves the problem of dstinguishing HTML
+  from "foreign" elements largely through the parse context. This means isn't


Suggested change

from "foreign" elements largely through the parse context. This means isn't

from "foreign" elements largely through the parse context. This distinction isn't

mikewest · 2021-11-12T07:23:01Z

index.bs

+</div>
+
+Note: This Sanitizer API makes no attempt at supporting arbitrary namespaces
+  or the [XML Namespaces](https://www.w3.org/TR/xml-names/) specification in


Nit: If you write this as [[XML-NAMES]], it'll be brought into the bibliography as an informational reference.

Done. Two questions, though:

Is there a method for finding these short names? In a number of cases, I've tried to find short names and eventually gave up. Is there.. a reference of this?

How does the tool distinguish between informational and normative references?

The shortname is usually whatever shows up after TR in the path. If that doesn't work, you can poke at Specref (which I think is where Bikeshed pulls its reference database).

[[FOO]] is informative. [[!FOO]] is normative. https://tabatkins.github.io/bikeshed/#biblio spells out some of the details.

index.bs

otherdaniel · 2021-11-23T17:41:39Z

I've now updated the PR. Lots of editorial improvements, and particular better links to other specs. The main substance change it that it now uses ":" as separator for element namespaces. That makes it less weird, but prevents sanitization of HTML elements with colones in their names.

mikewest · 2021-11-25T09:38:31Z

Thanks for taking another pass! I left a few more comments, but nothing substantive. There are some open comments left on the PR from the previous review. I'm not sure I'd block on any of them, but it'd be helpful for you to close them out explicitly one way or the other.

otherdaniel · 2021-11-25T16:06:01Z

Thanks for taking another pass! I left a few more comments, but nothing substantive. There are some open comments left on the PR from the previous review. I'm not sure I'd block on any of them, but it'd be helpful for you to close them out explicitly one way or the other.

Yup, the left-over comments are stuff that I overlooked last time. I think I now handled all of them... :-/

I didn't find any new comments, though. All the ones I saw were from "9 days ago". Maybe I'm just confused about the GitHub UI, though...

mikewest

I didn't find any new comments, though. All the ones I saw were from "9 days ago". Maybe I'm just confused about the GitHub UI, though...

Yeah, my fault. I left these comments pending. :/

Thanks for running through the other comments, I think this is the last round of comments I'll have on the PR.

index.bs

mikewest

Thanks for iterating on this. LGTM.

mozfreddyb

Thank you for writing this up and my apologies for the long wait.

index.bs

mozfreddyb

🥇

@otherdaniel

SHA: 1ab795f Reason: push, by @otherdaniel Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

otherdaniel mentioned this pull request Nov 4, 2021

Proposed API: MathML, SVG support, plus localname case-handling. #103

Open

otherdaniel requested a review from mozfreddyb November 10, 2021 10:40

otherdaniel added 4 commits November 10, 2021 15:15

Frist draft.

89d1f77

Contiue refining PR.

770e451

Remove stale issue.

85268a5

Rebase + minor fixups.

a3252ad

otherdaniel force-pushed the svgandmathml branch from bc3069f to a3252ad Compare November 10, 2021 14:22

mozfreddyb reviewed Nov 11, 2021

View reviewed changes

mikewest reviewed Nov 16, 2021

View reviewed changes

otherdaniel added 2 commits November 16, 2021 15:21

Review feedback, editorial

ec53800

Change namespace seperator to colon.

9edf16f

Review feedback

f8f2a83

mikewest reviewed Nov 26, 2021

View reviewed changes

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Show resolved Hide resolved

otherdaniel added 2 commits November 26, 2021 14:22

Review feedback.

792c534

Add issue for sanitizeFor

53489a5

mikewest approved these changes Nov 29, 2021

View reviewed changes

mozfreddyb approved these changes Nov 30, 2021

View reviewed changes

index.bs Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

otherdaniel mentioned this pull request Nov 30, 2021

.sanitizeFor and namespaces #140

Closed

Review feedback.

9ce44a9

mozfreddyb approved these changes Nov 30, 2021

View reviewed changes

otherdaniel merged commit 1ab795f into WICG:main Nov 30, 2021

github-actions bot added a commit that referenced this pull request Nov 30, 2021

SVG and MathML (#137)

1c9ad84

SHA: 1ab795f Reason: push, by @otherdaniel Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

otherdaniel deleted the svgandmathml branch July 8, 2022 11:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SVG and MathML #137

SVG and MathML #137

otherdaniel commented Nov 4, 2021

otherdaniel commented Nov 10, 2021

securityMB commented Nov 11, 2021

mozfreddyb commented Nov 11, 2021 •

edited

mozfreddyb Nov 11, 2021

securityMB Nov 11, 2021

mozfreddyb Nov 11, 2021

mikewest left a comment

mikewest Nov 12, 2021

otherdaniel Nov 16, 2021

mikewest Nov 12, 2021

otherdaniel Nov 16, 2021

mikewest Nov 12, 2021

otherdaniel Nov 16, 2021

mikewest Nov 16, 2021

otherdaniel commented Nov 23, 2021

mikewest commented Nov 25, 2021

otherdaniel commented Nov 25, 2021

mikewest left a comment

mikewest left a comment

mozfreddyb left a comment

mozfreddyb left a comment

		Namespace designator and element names are seperated by a
		whitespace character. `svg` designates elements in the [=SVG namespace=], and


		</div>

		Note: The [[HTML]] specification solves the problem of dstinguishing HTML

	from "foreign" elements largely through the parse context. This means isn't
	from "foreign" elements largely through the parse context. This distinction isn't

SVG and MathML #137

SVG and MathML #137

Conversation

otherdaniel commented Nov 4, 2021

otherdaniel commented Nov 10, 2021

securityMB commented Nov 11, 2021

mozfreddyb commented Nov 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikewest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otherdaniel commented Nov 23, 2021

mikewest commented Nov 25, 2021

otherdaniel commented Nov 25, 2021

mikewest left a comment

Choose a reason for hiding this comment

mikewest left a comment

Choose a reason for hiding this comment

mozfreddyb left a comment

Choose a reason for hiding this comment

mozfreddyb left a comment

Choose a reason for hiding this comment

mozfreddyb commented Nov 11, 2021 •

edited