Defaults for boolean SanitizerConfig items

[otherdaniel]

Give explicit defaults to SanitizerConfig.comments and SanitizerConfig.dataAttributes.

This fixes one of the issues in #249.

---

Preview | Diff

[annevk]

I think this approach has some issues:

1. If boolean fields have a default it should be false, not true.
2. This is strictly less safe.

I think keeping them optional and using that third state to pick a default depending on which method you chose as I suggested in the issue is still a reasonable way forward.

If everyone really wants this behavior we'd have to name them removeComments and removeDataAttributes I think, defaulting to false.

[mozfreddyb]

I was also under the impression that we would make comments & data attributes allowable but not enabled in the setHTML() default.

I think we only discussed a webidl-semantics question on our call on Wednesday, so me skipping this may have contributed to that confusion?

I think we wanted {} to be an "empty sanitizer" but the default to be "a securely configured sanitizer". Is that part of this change?

[otherdaniel]

I seem to recall a quite explicit discussion about a true default in the meeting. But anyways, the observable behaviours are: (Result in the comments is what we have currently specified, as far as i understand.)

[Edit: Using real comment syntax now. :) ]

/* 1*/ new Sanitizer().get().comments;  // false
/* 2*/ new Sanitizer({}).get().comments;  // false
/* 3*/ new Sanitizer({comments: true}).get().comments;  // true
/* 4*/ new Sanitizer({comments: false}).get().comments;  // false

/* 5*/ div.setHTMLUnsafe("<!--bla-->");  // <div></div>
/* 6*/ div.setHTMLUnsafe("<!--bla-->",{sanitizer: {}});  // <div></div>
/* 7*/ div.setHTMLUnsafe("<!--bla-->",{sanitizer:{comments:true}});  // <div><!--bla--></div>
/* 8*/ div.setHTMLUnsafe("<!--bla-->",{sanitizer:{comments:false}});  // <div></div>
 
/* 9*/ div.setHTML("<!--bla-->");  // <div></div>
/*10*/ div.setHTML("<!--bla-->",{sanitizer: {}});  // <div></div>
/*11*/ div.setHTML("<!--bla-->",{sanitizer:{comments:true}});  // <div><!--bla--></div>
/*12*/ div.setHTML("<!--bla-->",{sanitizer:{comments:false}});  // <div></div>

I take it:

• 5 is problematic, as it contradicts existing HTML-spec'ed behaviour of setHTMLUnsafe.
• 5 + 6 are expected to be consistent, because setHTMLUnsafe defaults to {}.
• 9 can be different from 10, due to the new "default" mechanism, but I take it 1 + 2 + 5 + 6 + 10 are meant to be consistent.
• I take it safe/unsafe, i.e. 6-8 and 10-12, are expected to be consistent except for "baseline" behaviour (non-allowable + javascript:-URLs). Comments aren't a "baseline" thing, so I'd expect them to be fully consistent regarding comments.

I'm trying to reconcile these.

I'm guessing the explicit ones (3+4, 7+8, 11+12) are all uncontroversial. The rub is that setHTMLUnsafe already has behaviour. I think that behaviour allows comments. And then we have various requirements of methods to be consistent. In particular, I thought we wanted setHTML and setHTMLUnsafe to be consistent except for clearly articulated differences.

Maybe I'm overlooking something. I think there's other ways to fix this, too, e.g. not defaulting to {} for setHTMLUnsafe.

[annevk]

Nit: you want <!-- bla --> instead of /* bla */.

I think your analysis is correct. I tend to think it's okay for Sanitizer, setHTML(), setHTMLUnsafe() to handle the dictionary members comments and dataAttributes differently, but only when they are not explicitly set. I do see that creates some inelegance in the overall model, but overall that seems preferable over not removing them in setHTML().

As such I think Sanitizer and setHTMLUnsafe() should default them to false, when they are not set. And setHTML() should default them to true.

This gives these results:

1. false
2. false
3. true
4. false
5. false
6. false
7. true
8. false
9. true
10. true
11. true
12. false

(This can only happen when you pass a SanitizerConfig. Once you have a Sanitizer they're always set of course (and they're no longer dictionary members).)

[otherdaniel]

Okay, that makes sense.

I do see that creates some inelegance in the overall model, but overall that seems preferable over not removing them in setHTML(). [...] And setHTML() should default them to true.

This seems contradictory, but I think it's just about notation. I take comments: true as allowing comments, so I read the first bit as default-removing them in setHTML (aka, comments: false) and second as default-allowing them in setHTML (aka, comments: true).

Nit: you want <!-- bla --> instead of /* bla */.

Oh, yes. I'll fix that... :)

[annevk]

Oh yes. I keep inverting them. Hopefully that's just a me problem.

[mozfreddyb]

Ha, good catch. I was also thinking "comments" meaning "allow comments". Interesting that even ourselves can get confused 😬

[otherdaniel]

I change the PR to follows the discussion here. The default now depends on safe/unsafe usage.