Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

2023-09-20 meeting notes #199

Closed
evilpie opened this issue Sep 20, 2023 · 2 comments · Fixed by #208
Closed

2023-09-20 meeting notes #199

evilpie opened this issue Sep 20, 2023 · 2 comments · Fixed by #208

Comments

@evilpie
Copy link

evilpie commented Sep 20, 2023

Issue #198: Configuration edge cases
One of the main results of this discussion was that having attributes both for allowElements and blockBlements doesn’t really work.
Should we block an element with a specific attribute, or just drop the attribute? But why would it be called blockElement then?
This is “Block-clauses with element-dependent attributes” in the linked issue.

New design with elements and attribute properties

  • Use elements instead of allowElements
    • Every element can have allowed/dropped attributes
  • Use attributes instead of allowAttributes
    • Every attribute can also have “elements” i.e. the list of elements this attribute is allowed for
{ 
  elements: ["img", // allow all <img> tag
             { name: "animate", namespace: "http://www.w3.org/2000/svg" }, // allow all SVG <animate> elements
             { name: "b", attributes: ["title"] }, // Allow <b> elements, but remove all of the attributes except "title".
             { name: "a", dropAttributes: ["href"] }], // Allow <a> elements, allow all default attributes, but drop "href"

  attributes: ["title", // Allow title on all elements
               {name: "href", elements: ["a"] }] // Allow the href attribute only for <a> elements
}
  • We still have dropAttributes, dropElements and block/flattenElements.
    • dropAttributes doesn't allow elements and drop/flattenElements doesn't allow attributes as filters!
{
  flattenElements: [ /* like dropElements */],
  dropElements: [
    { name: "input"}, // drop all input elements
    { name: "animate", namespace: "http://www.w3.org/2000/svg"}, // drop all SVG animate elements
    "b", // drop all b elements
  ],
  dropAttributes: ["style", // never accept style attrs
                   { name: "type", namespace: "http://www.w3.org/2000/svg"}] // Don't accept type attributes in the SVG namespace
}

Tom: This means the “happy path” uses just the terms “elements” and “attributes”, which is both short and easy to remember

Rename block

  • We don’t have allowElements/allowAttributes anymore so we don’t need the symmetry with allow.
  • Call “block” (replace with children) something like "flat"/"flatten" like JS flatMap
    Anne: my concrete proposal here is flattenElements
  • So we end up with elements/dropElements/flattenElements and attributes/dropAttributes

This is overall a bit similar to the proposal in #181 and in Firefox: https://searchfox.org/mozilla-central/source/dom/webidl/Sanitizer.webidl

@benbucksch
Copy link

benbucksch commented Sep 20, 2023

block vs. allow:

Both block and allow list are needed. Block should have precendence over allow.

E.g.:

  1. For each element and attribute in the document:
  2. If the element/attribute is in the block list, remove it from the document
  3. If there is an allow list, and the element/attribute is not in the allow list, remove it from the document
  4. Otherwise, keep the element/attribute.

block vs. drop

For me personally, the differentiation between "block" and "drop" is too subtle. I understood them as synonyms. If I read "block" for an element, I assume that this element will be dropped entirely, with all attributes and all child elements, i.e. removed completely from the source document. It makes sense to me that child elements will be dropped as well, otherwise the document might become syntactically or semantically invalid. You cannot have a <source> outside a <video>.

For cases like <video>, where you want to forbid the video, but keep the fallback text that is a child, you can keep the <video> element itself, but drop all attributes on it, and drop the <source> elements. Without src (etc.) attributes and without <source>, you've essentially disabled the functionality of the video element, but keep the fallback text.

I think only "block" = 'drop element and its child elements' is needed in practice. I think that almost all realistic cases can be solved this way. Even if there are edge cases which cannot be expressed this way, they should be carefully weighted whether these edge cases are worth the complications in the spec and for authors.

I think "block" is the most intutive term for removing elements or attributes (and their children).

@mozfreddyb
Copy link
Collaborator

block vs. allow:

Both block and allow list are needed. Block should have precendence over allow.

Please note that whatever is being specified as a custom Sanitizer configuration will be held against the secure defaults. This means developers will not be able to allow <script> elements when using setHTML().

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants