Spec bugs / clarifications

[otherdaniel]

Collection of issues I encountered when cross-checking our (early-stage) implementation & test results against the spec.

These might well be bugs in the test suite or impl, rather than the spec, though. I'm just listing things here so we won't forget...

[otherdaniel]

Config procesing: We currently throw when an element or global attribute is listed in both allow & remove, but not for per-element attributes.

new Sanitizer({elements: ["p"], removeElements: ["p"]})  // throws
new Sanitizer({elements: [{name: "p", attributes: ["id"], removeAttributes: ["id"]}]})  // does not throw.

I think that's just an oversight.

[otherdaniel]

"comments" and "dataAttribute" keys in the config default to false. That's because WebIDL defaults to false when accessing a missing boolean member. This could be handled differently by checking for presence.

I thought we had decided this differently; but not sure I remember correctly.

bla.setHTML("<p>hello <!-- world -->", {})  // Comment is dropped: <p>hello.

[mozfreddyb]

Config procesing: We currently throw when an element or global attribute is listed in both allow & remove, but not for per-element attributes.

new Sanitizer({elements: ["p"], removeElements: ["p"]})  // throws
new Sanitizer({elements: [{name: "p", attributes: ["id"], removeAttributes: ["id"]}]})  // does not throw.

I think that's just an oversight.

I could see someone arguing that the second case could work for clear semantics on which of the properties is first being checked, leading to either p with id in place, but all other elements without any id *or p element stays but all id attributes removed.

Not sure what to prefer, to be honest.

"comments" and "dataAttribute" keys in the config default to false. That's because WebIDL defaults to false when accessing a missing boolean member. This could be handled differently by checking for presence.

I thought we had decided this differently; but not sure I remember correctly.

bla.setHTML("<p>hello <!-- world -->", {})  // Comment is dropped: <p>hello.

Wait, what is {} doing? I believe we argued the following

• setHTML(foo) and setHTML(foo, undefined) gives a setHTML default policy (which is secure from XSS and then a bit more) but setHTML(foo, {}) should be an "empty" policy which passes the value that would be used for setHTMLUnsafe. However, setHTML would still apply the remove unsafe operation, essentially leading to the "No XSS, but everything else is allowed" list. Is that correct?

In that case, yes. We should allow comments and data attributes with {}.

[annevk]

@mozfreddyb attributes and removeAttributes are both on the same element there so that clearly seems like an error to me.

As for comments and dataAttributes, I agree with the desired behavior for undefined and {}. I'm not sure what the behavior should be for { elements: ["p"] }. I could kinda see an argument for that unless you have a passthrough policy we treat omitting these members as comments/data attributes getting dropped, as that's the safer default.

[otherdaniel]

"comments" and "dataAttribute" keys in the config default to false. That's because WebIDL defaults to false when accessing a missing boolean member. This could be handled differently by checking for presence.
I thought we had decided this differently; but not sure I remember correctly.

bla.setHTML("<p>hello <!-- world -->", {})  // Comment is dropped: <p>hello.

Wait, what is {} doing? I believe we argued the following

• setHTML(foo) and setHTML(foo, undefined) gives a setHTML default policy (which is secure from XSS and then a bit more) but setHTML(foo, {}) should be an "empty" policy which passes the value that would be used for setHTMLUnsafe. However, setHTML would still apply the remove unsafe operation, essentially leading to the "No XSS, but everything else is allowed" list. Is that correct?

In that case, yes. We should allow comments and data attributes with {}.

Ah, good catch. I said bla.setHTML(…, {}), but meant bla.setHTML(…, {sanitizer: {}}).

The former will use the new default "default", which will do whatever we define it as. But I think with bla.setHTML(…, {sanitizer: {}}) and bla.setHTMLUnsafe(…, {}), WebIDL will convert {} to (Sanitizer or SanitizerConfig or SanitizerPresets), which should result in an equivalent of:

{
  elements: [], removeElements: [], replaceWithChildrenElements: [],
  attributes: [], removeAttributes: [],
  comments: false, dataAttributes: false
};