Modify-able config. #237

otherdaniel · 2024-08-13T16:50:47Z

[This isn't quite ready yet. Using a PR for tool support and discussion.]

This re-writes the config logic, based on discussions in the recent meetings, and with the intent to address issues #228, #229, and #233.

The corner stones are:

The Sanitizer (and thus its config) become mutable. It has methods to add stuff to any of the allow or deny lists.
The actual config processing is now defined in terms of those methods.
There is only one "safe-ify" operation, and it gets called once (for safe methods). This is the only different between safe and unsafe versions.
- (Well, this is only a half-truth. The handling of javascript:-navigation URLs remains an odd-ball that is magically different between "safe" and "unsafe" versions.)
The configs have an "other" setting, which allows them to express "everything" and "nothing" filters, which in turn allows us to treat the unsafe defaults as just another regular case.
The entire "known" logic is gone. But in turn, the "baseline" is now back, and is a deny-list.

Preview | Diff

mozfreddyb · 2024-08-21T08:48:26Z

We discussed this in a synchronous meetings. I'm copying the notes verbatim so we can continue async.

Document.parseHTML / Document.parseHTMLUnsafe - How does it handle safety?

We should not need otherMarkup

get/getUnsafe should not be a method

Some method for turning unsafe into safe

Don’t want to call it “safe”, because we don’t know what safe is

Eg. “WithBaseline” (baseline is applied to config)

Element vs allowAttribute?

setComment why set-method instead of an attribute-setter?

Should we use allow instead of allowElement (or element)

Anne: How to undo replaceWithChildren

Tom: element() currently removes from the replaceWithChildren list

Guillaume: “Other name for something captured by allow or block list”

We have a baseline know

Annevk: baseline remove all unsafe stuff

Default “safe list” of allowed elements

Annevk: Expose the default list with a static method

Anne: Name the method removeUnsafe to make a config safe (instead of safeify, because we can’t easily guarantee safeness for all use cases)

It shouldn't be replaceWithChildren but replaceWithChildrenElements (as per earlier group consensus)

Anne: Rename methods to addElement, addRemoveElement, addReplaceWithChildrenElement, etc. (prefix with add to make it clear there is a modification of the instance)

No group concensus for the add prefix. We need to discuss this further.

Tom: Make setComment a property like comment

Anne: This would imply introspection for the properties and not for other like elements, removeElements etc.

Anne: Remove getUnsafe, instead a method that makes sanitizer safe (removeUnsafe)

Guillaume: get() returns whatever the current state is

Anne: Could we use toJSON instead of get?

Guillaume/Frederik: We need to worry about having functions that are not JSON compatible in the future (e.g. if we add a fallback feature in v2)

all: Let’s keep get then.

mozfreddyb

Really great progress so far. We have some nits (and some larger questions) in the review notes, please see above.

otherdaniel · 2024-09-17T13:31:50Z

Added issue links to issues for naming and other markup
Added "unsafe" handling to parseHTMLUnsafe.
Replaced getUnsafe query method with removeUnsafe modifier.
Naming mostly deferred. But made sure all method names start with a verb.
Finished handling of element-dependent attributes.
Various editorial / link-ification improvements.

mozfreddyb · 2024-11-11T12:28:49Z

index.bs

+ISSUE(238): Final naming TBD.
+
+ISSUE(240): "other markup" TBD.
+
+ISSUE: Should these be setter methods -- particularly the setXXX(boolean) --
+    or setters or properties or somesuch?
+
+ISSUE: Should the modifier methods return a reference to [=this=], so that you
+    can 'chain' methods?
+    (e.g. `sanitizer.allowElement("a").allowElement("span")`).
+


Please file issues for those that are only in the file

index.bs

mozfreddyb · 2024-11-11T12:31:20Z

index.bs

@@ -297,6 +418,7 @@ dictionary SanitizerConfig {
 };
 </pre>

+ISSUE: Sould members be required, or have declared defaults?


(Same as above, pls file ISSUE for those that area only in the document)

Done. (Also removed issues that had been resolved, like this one, or otherMarkup.

annevk

Modulo naming the general setup here looks really solid to me. I mainly have editorial concerns. Thanks a lot for your work on this!

annevk · 2024-11-15T09:57:13Z

index.bs

+<div algorithm>
+The <dfn for="Sanitizer" export>allowElement</dfn>(|element|) method steps are:
+
+1. Let |name| be the result of [=canonicalize a sanitizer name=] |element| with [=HTML namespace=] as the default namespace.


I wonder if it would be cleaner to canonicalize the entire element.

Hmm. Canonicalize a sanitizer name is used for both elements and attributes. I could add a "canonicalize an elemnt" and a "canonicale an attribute", both of which would then canonicalize their name, but it seems like that merely adds an indirection. (At least as no other canonicalization steps are performed.)

[Leaving open for now.]

annevk · 2024-11-15T10:02:33Z

index.bs

+The <dfn for="Sanitizer" export>allowElement</dfn>(|element|) method steps are:
+
+1. Let |name| be the result of [=canonicalize a sanitizer name=] |element| with [=HTML namespace=] as the default namespace.
+1. [=list/Remove=] |name| from [=this=]'s [=internal slot=]'s {{SanitizerConfig/elements}}.


This wording needs to be more elaborate to make it clear how this removing actually works. Keep in mind that there's no identity match here and also that the entry in elements may contain other keys. (This applies quite a few times in this PR and maybe also in the existing draft text.)

I think it's also somewhat confusing that the internal slot holds the same dictionary as is used for input, although I guess it can work here. But then you need to address them as dictionary members so it would have to be

[=internal slot=]["{{SanitizerConfig/elements}}"] I think.

It also seems weird to call this "internal slot". Typically we would call it "configuration" or some such.

Added a [=SanitizerConfig/remove=], to explain removal of a name structure.

Removed "internal slot" in favour of configuration; and systematically added dictionary syntax to reference its members.

(I think I cargo-culted the "internal slot" from ECMAScript, which defines an "internal slot" as a hook that the embedder can use to attach data to a script object. I agree that doesn't actually make any sense over here.)

index.bs

annevk · 2024-11-15T10:08:57Z

index.bs

+   {{SanitizerConfig/replaceWithChildrenElements}}, or
+   {{SanitizerConfig/attributes}}.
+1. Let |result| be a copy of |config|.
+1. [=list/For each=] |elem| in


Sometimes you use elem, sometimes, item, and sometimes element. I would settle on element for all of these. I'd generally prefer less abbreviations in prose, so also attribute and configuration instead of attr and config.

Mostly done. I consistently use element, attribute, and configuration now. I still use item now and then, but only for contexts where it could be either an element or an attribute. Happy to rename that, but I couldn't really come up with a nice name for element_or_attribute.

index.bs

…lot, in favour of an associated config object. Use long names. Define list removeal.

mozfreddyb · 2024-11-27T10:03:16Z

copying over from #238

change

replaceWithChildrenElements to replaceElementWithChildren()
setComments() to plural.

keep:

removeElement()
allowElement()
removeAttribute()
allowAttribute()
setDataAttributes()

otherdaniel · 2024-11-27T14:43:28Z

Updated the PR with feedback from today's meeting:

Adopted the updated naming.
Introduced canonicalization methods for element with attributes; elements; and attributes. Rewrote the caller algorithms so that they canonicalize in the first step, and then only use the canonicalized value.

annevk

At this point I would be okay with landing this to get the public draft in a better state, but we'll need follow-up issues for setHTML() defaulting and such (and any comments in this PR that you decide to postpone addressing).

annevk · 2024-11-27T16:41:54Z

index.bs

@@ -217,48 +227,86 @@ dictionary SetHTMLOptions {
 </pre>

 The {{Sanitizer}} configuration object encapsulates a filter configuration.
-The same config can be used with both safe or unsafe methods. The intent is
+The same configuration can be used with both safe or unsafe methods, where


Suggested change

The same configuration can be used with both safe or unsafe methods, where

The same configuration can be used with both safe and unsafe methods, where

Perhaps we should add quotation marks around "safe"?

annevk · 2024-11-27T16:43:02Z

index.bs

@@ -217,48 +227,86 @@ dictionary SetHTMLOptions {
 </pre>

 The {{Sanitizer}} configuration object encapsulates a filter configuration.
-The same config can be used with both safe or unsafe methods. The intent is
+The same configuration can be used with both safe or unsafe methods, where
+the safe methods perform an implicit {{removeUnsafe}} operation. The intent is


Suggested change

the safe methods perform an implicit {{removeUnsafe}} operation. The intent is

the safe methods perform an implicit {{removeUnsafe}} operation on the passed in configuration and have a default configuration when none is passed. The intent is

annevk · 2024-11-27T16:45:20Z

index.bs


 <pre class=idl>
 [Exposed=(Window,Worker)]
 interface Sanitizer {
-  constructor(optional SanitizerConfig config = {});
+  constructor(optional SanitizerConfig configuration = {});
+


Let's add a TODO here for [NewObject] static Sanitizer getDefault()? (I suppose it could also return a SanitizerConfig, but an instance seems better.)

annevk · 2024-11-27T16:46:55Z

index.bs


+<div algorithm>
+The <dfn for="Sanitizer" export>replaceElementWithChildren</dfn>(|element|) method steps are to [=replace an element=] with |element| and [=this=]'s [=Sanitizer/configuration=].


Let's also call the internal operation "replace an element with children"?

annevk · 2024-11-27T16:48:41Z

index.bs

-   |config| and |safe|.
+   Then return  |options|["`sanitizer`"].
+1. [=Assert=]: |options|["`sanitizer`"] is a [=dictionary=].
+1. Return new {{Sanitizer}}(|options|["`sanitizer`"]).


Public API.

annevk · 2024-11-27T16:49:46Z

index.bs

+1. Let |configuration| be the value of |sanitizer|'s [=Sanitizer/configuration=].
+1. If |safe| is true, then set |configuration| to the result of calling [=remove unsafe=] on |configuration|.
+1. Call [=sanitize core=] on |node|, |configuration|, and |safe| (as value for
+    handling javascript navigation urls).


I would remove this parenthetical or switch to named parameters as outlined in the Infra standard.

annevk · 2024-11-27T16:51:09Z

index.bs

+<div algorithm>
+The <dfn>sanitize core</dfn> operation,
+using a {{ParentNode}} |node|, a {{SanitizerConfig}} |configuration|, and a
+[=boolean=] |handle javascript navigation urls|, iterates over the DOM tree


It's usually better to camelCase variable names as otherwise it's unclear if they are separate variables or one long one.

annevk · 2024-11-27T16:51:46Z

index.bs

+</div>
+
+<div algorithm>
+To <dfn for="Sanitizer">allow comments</dfn> with |allow| on a {{SanitizerConfig}} |configuration|, do:


set comments? and set data attributes below?

annevk · 2024-11-27T16:53:03Z

index.bs

+ISSUE(233): Determine if this actually holds.
+
+
+The <dfn>built-in unsafe default config</dfn> is meant to allow anything.


Do we still need this?

annevk · 2024-11-27T16:57:32Z

index.bs

+    { name: "script", namespace: "http://www.w3.org/2000/svg" }
+  ],
+  removeAttributes: [....],
+  comments: true,


comments here isn't needed.

I suspect that we simply want to inline what this is doing in "remove unsafe", but that can probably wait. (As otherwise we'd have to define this microsyntax and such.)

otherdaniel force-pushed the config-extension branch from 4f00015 to 33619b9 Compare August 13, 2024 16:55

mozfreddyb self-requested a review August 21, 2024 08:48

mozfreddyb requested changes Aug 21, 2024

View reviewed changes

mozfreddyb mentioned this pull request Sep 11, 2024

Renaming methods to prefix them with add make it clear there is a modification of the instance #238

Open

otherdaniel force-pushed the config-extension branch from 33619b9 to fad7841 Compare September 13, 2024 15:08

otherdaniel mentioned this pull request Sep 13, 2024

otherMarkup #240

Open

otherdaniel force-pushed the config-extension branch from fad7841 to 6a97bf4 Compare September 17, 2024 13:26

otherdaniel added 3 commits September 18, 2024 13:36

Draft new config extension.

bd33447

Review feedback

d86d83b

Remove otherMarkup

764d394

otherdaniel force-pushed the config-extension branch from 6a97bf4 to 764d394 Compare September 18, 2024 14:06

otherdaniel added 2 commits September 19, 2024 15:24

Review feedback.

c5c79ab

More feedback. Revive error checking for the constructor.

9533b47

mozfreddyb requested review from mozfreddyb and annevk October 16, 2024 08:18

Extend explainer

45d201f

mozfreddyb reviewed Nov 11, 2024

View reviewed changes

annevk reviewed Nov 15, 2024

View reviewed changes

otherdaniel mentioned this pull request Nov 26, 2024

Chainable modifiers? #242

Closed

Review feedback: Don't call public APIs internally. Remove internal s…

72636b0

…lot, in favour of an associated config object. Use long names. Define list removeal.

Review feedback, 2024-11-27

9946e17

annevk approved these changes Nov 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify-able config. #237

Modify-able config. #237

otherdaniel commented Aug 13, 2024 •

edited by pr-preview bot

Loading

mozfreddyb commented Aug 21, 2024

mozfreddyb left a comment

otherdaniel commented Sep 17, 2024

mozfreddyb Nov 11, 2024

mozfreddyb Nov 11, 2024

otherdaniel Nov 26, 2024

annevk left a comment

annevk Nov 15, 2024

otherdaniel Nov 26, 2024

annevk Nov 15, 2024

otherdaniel Nov 26, 2024

annevk Nov 15, 2024

otherdaniel Nov 26, 2024

mozfreddyb commented Nov 27, 2024

otherdaniel commented Nov 27, 2024

annevk left a comment

annevk Nov 27, 2024

annevk Nov 27, 2024

annevk Nov 27, 2024

annevk Nov 27, 2024

annevk Nov 27, 2024

annevk Nov 27, 2024

annevk Nov 27, 2024

annevk Nov 27, 2024

annevk Nov 27, 2024

annevk Nov 27, 2024

	The same configuration can be used with both safe or unsafe methods, where
	The same configuration can be used with both safe and unsafe methods, where

	the safe methods perform an implicit {{removeUnsafe}} operation. The intent is
	the safe methods perform an implicit {{removeUnsafe}} operation on the passed in configuration and have a default configuration when none is passed. The intent is


		<div algorithm>
		The <dfn for="Sanitizer" export>replaceElementWithChildren</dfn>(\|element\|) method steps are to [=replace an element=] with \|element\| and [=this=]'s [=Sanitizer/configuration=].

		ISSUE(233): Determine if this actually holds.


		The <dfn>built-in unsafe default config</dfn> is meant to allow anything.

Modify-able config. #237

Are you sure you want to change the base?

Modify-able config. #237

Conversation

otherdaniel commented Aug 13, 2024 • edited by pr-preview bot Loading

mozfreddyb commented Aug 21, 2024

mozfreddyb left a comment

Choose a reason for hiding this comment

otherdaniel commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk 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

mozfreddyb commented Nov 27, 2024

otherdaniel commented Nov 27, 2024

annevk 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otherdaniel commented Aug 13, 2024 •

edited by pr-preview bot

Loading