Improve and update the existing Security Considerations by mozfreddyb · Pull Request #351 · WICG/sanitizer-api

mozfreddyb · 2025-10-31T15:49:03Z

**Improve and update the existing Security Considerations - fixes Document threat models #287 **

index.bs

annevk · 2025-11-03T08:01:55Z

index.bs

@@ -1453,10 +1453,15 @@ URLs, is as follows:
 # Security Considerations # {#security-considerations}

 The Sanitizer API is intended to prevent DOM-based Cross-Site Scripting


In the heading below we just use "XSS". We should probably be somewhat consistent in this document about that. And we might also need a reference for this term I suppose if we want to use it.

Is MDN an acceptable reference for web specifications?

Maybe, because it's only for non-normative text. But it gets kinda circular and doesn't seem ideal.

do you think we need to define it ourselves instead?

index.bs

annevk · 2025-11-03T08:12:42Z

index.bs

+: `<math>` MathML math
+:: The reasoning for the inclusion of MathML is solely based on [[SafeMathML]].
+
+: `<svg>`
+:: This element is not allowed, because it leads to HTTP requests to arbitrary hosts.


I thought we did allow some Math and SVG by default?

MathML is some allowed as can only be seen when following the reference. I'll make it explicit

You're right and I think I got that wrong. Will correct this in a follow-up.

The PR is getting a bit too unwieldy for my personal preference. I'll do the SVG/MATHML/HTML tables in a follow-up

index.bs

…s in a cohesive way with a table and footnotes.

annevk

I'm not sure how I feel about the "Index" section. It largely duplicates the existing lists, with some words added. That would probably be better inline?

index.bs

mozfreddyb · 2025-11-06T10:01:33Z

I'm not sure how I feel about the "Index" section. It largely duplicates the existing lists, with some words added. That would probably be better inline?

Well, I guess we need a place for detailed reasoning that's going to come in the #354 follow-up. I can add this to Security Considerations instead if that's preferable?

evilpie · 2025-11-06T12:47:16Z

index.bs

+    <tr>
+     <td><code>object</code></td>
+     <td>http://www.w3.org/1999/xhtml</td>
+     <td>This element is disallowed because it can include other, untrusted documents and including scripts.</td>


"including scripts" doesn't seem grammatically correct to me.

evilpie · 2025-11-06T12:47:58Z

index.bs

+
+While the Sanitizer aims to disallow Cross-Site Scripting attacks [[XSS]], the default
+settings are a bit stricter and do not include elements that may change page
+settings, layout or fetch resources from other origins. The following elements


We don't/can't really differentiate between fetching resources from same vs. other origins, so this seems too specific.

evilpie · 2025-11-06T12:49:22Z

index.bs

+## Elements and attributes not allowed in the default config ##  {#default-disallowed-elements}
+
+While the Sanitizer aims to disallow Cross-Site Scripting attacks [[XSS]], the default
+settings are a bit stricter and do not include elements that may change page


"a bit" feels a bit off here. Maybe "the default settings are purposefully stricter"

evilpie · 2025-11-06T12:53:03Z

The definition of the kinds of XSS we try to prevent is central to defining our threat model, so I would prefer if it was clearly laid out as part of the specification instead of a link to MDN.

otherdaniel · 2025-11-06T15:20:09Z

index.bs

-the construction of a Sanitizer object that leaves script-capable markup in
-and doing so would be a bug in the threat model.
+The Sanitizer API is intended to prevent DOM-based Cross-Site Scripting [[XSS]]
+by traversing supplied HTML content and removing elements and attributes


Maybe clarify that sanitzation happens before inserting. E.g.:

[...] by traversing supplied HTML content, after parsing but before insertion into any document, and removing [...]

I think this is obvious to us, but I think a lot of people haven't entirely thought through the "when" aspect of sanitiaztion.

otherdaniel · 2025-11-06T15:24:46Z

index.bs


-The Sanitizer API offers only functions that turn a string into a node tree.
-The context is supplied implicitly by all sanitizer functions:
+The Sanitizer API offers functions that combine parsing, sanitization and insertion.


This is very true. I wonder if it bears saying at the top of this section, in the paragraph "The Sanitizer API is intended to [...]". Maybe instead of the comment I've proposed there.

mozfreddyb requested review from evilpie and otherdaniel October 31, 2025 15:49

mozfreddyb force-pushed the improved-threat-mode-descr-fix-287 branch from 5e625d3 to db63861 Compare October 31, 2025 15:50

Improve and update the existing Security Considerations - fixes WICG#287

3a5c90f

mozfreddyb force-pushed the improved-threat-mode-descr-fix-287 branch from db63861 to 3a5c90f Compare October 31, 2025 15:51

annevk reviewed Nov 3, 2025

View reviewed changes

evilpie reviewed Nov 3, 2025

View reviewed changes

index.bs Outdated Show resolved Hide resolved

mozfreddyb added 4 commits November 3, 2025 13:30

review feedback - part 1

7423640

no comma before because#

c706019

rewrite to ensure we include ideally all elements - part 1

e518c72

reduce PR to defer tables of allowed/disallowed elements for a follow-up

d65965d

mozfreddyb mentioned this pull request Nov 5, 2025

Include additional reasoning for all elements not allowed in the default config #354

Open

showcase how we could reason about html/svg/mathml elements/attribute…

0f24e33

…s in a cohesive way with a table and footnotes.

mozfreddyb mentioned this pull request Nov 5, 2025

Give allow/disallow reason in a big long tables (see #354) #355

Draft

annevk reviewed Nov 5, 2025

View reviewed changes

index.bs Outdated Show resolved Hide resolved

clarify ARIA

1287ee6

evilpie reviewed Nov 6, 2025

View reviewed changes

otherdaniel reviewed Nov 6, 2025

View reviewed changes

		@@ -1453,10 +1453,15 @@ URLs, is as follows:
		# Security Considerations # {#security-considerations}

		The Sanitizer API is intended to prevent DOM-based Cross-Site Scripting

Conversation

mozfreddyb commented Oct 31, 2025 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

annevk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mozfreddyb commented Nov 6, 2025

Uh oh!

evilpie Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evilpie Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evilpie commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mozfreddyb commented Oct 31, 2025 •

edited by pr-preview bot

Loading

evilpie Nov 6, 2025 •

edited

Loading

evilpie Nov 6, 2025 •

edited

Loading

evilpie commented Nov 6, 2025 •

edited

Loading