I think in set a configuration we should first handle attributes and removeAttributes and afterwards elements, it avoids having to go through all per-element attributes for every global attribute. While I think this shouldn't really be observable from a spec perspective, I think most implementation will want to produce error messages that might make this difference observable in practice.

I have updated the "Configuration Invariants" chapter (but not the actual algorithms) to match our discussion from last Wednesday: https://pr-preview.s3.amazonaws.com/otherdaniel/purification/pull/296.html#invariants

I'd be happy for a rough review whether this matches what we discussed. If so, I'll update the algorithm parts next.

Adding these tables to the spec is a good idea.

An attribute is allowed, if it’s in the local allow list, and not in the global remove list. No duplicate entries between global remove and local allow lists are allowed.

I would word this as

An attribute is only allowed if it’s in the local allow list. No duplicate entries between global remove and local allow lists are allowed.

to make it clearer that this is a strict list. I don't think we need "not in the global remove list" in the first sentences, as this is already enforced by the second.

local attributes: Custom data attributes are allowed. No custom data attributes may be listed in any allow-list, as that would mean a duplicate entry.

I misunderstood this the first time when reading. Maybe "All custom data attributes are allowed..." would help? Not sure.

If a global removeAttributes remove list exists, then:

• dataAttributes must be false or absent.

I am wondering if we should change this to just "must be absent".

A config like { removeAttributes: ["title"], dataAttributes: false } might give to wrong impression of removing data attributes?

If a global removeAttributes remove list exists, then:

• dataAttributes must be false or absent.

I am wondering if we should change this to just "must be absent".

Done.

Adding these tables to the spec is a good idea.

An attribute is allowed, if it’s in the local allow list, and not in the global remove list. No duplicate entries between global remove and local allow lists are allowed.

I would word this as

An attribute is only allowed if it’s in the local allow list. No duplicate entries between global remove and local allow lists are allowed.

to make it clearer that this is a strict list. I don't think we need "not in the global remove list" in the first sentences, as this is already enforced by the second.

Done. Also added a sentence that the global remove list might apply elsewhere.

local attributes: Custom data attributes are allowed. No custom data attributes may be listed in any allow-list, as that would mean a duplicate entry.

I misunderstood this the first time when reading. Maybe "All custom data attributes are allowed..." would help? Not sure.

Done.

I think this latest version represents the current group thinking; and I hope I've incorporated all feedback.

This ends up being more complicated than what is proposed in #294, but largely because there's a few more edge cases this needs to handle. (E.g. neither attributes or removeAttributes present; or the global removeAttributes + local attributes case.)

Did another pass.

I think I've handled all the feedback, but admittedly I haven't been terribly good at marking things as resolved, which makes the comment list here a bit unwiedly. Anything I've overlooked?

Did another pass.

I think I've handled all the feedback, but admittedly I haven't been terribly good at marking things as resolved, which makes the comment list here a bit unwiedly. Anything I've overlooked?

I don't think it can be helped, we have to go through the comments and resolve them.

Sorry, I tried to comment on some of the older comments and Github totally didn't like that.

With the current text in this PR we won't throw for duplicate names in a Sanitizer config, but I think we decided to throw for these cases.

new Sanitizer({ elements: ["br", "br"] });

With the current text in this PR we won't throw for duplicate names in a Sanitizer config, but I think we decided to throw for these cases.

Yes, that's how I remember it too.

[=valid=] now checks for dupes (and thus the constructor will throw on them), while [=canonicalize=] will just copy all elements (without eliminating dupes).

Sorry, I tried to comment on some of the older comments and Github totally didn't like that.

I think I found them: :-)

Based on today's meeting, we think this PR is pretty much good to go. I made another pass through all outstanding comments to resolve them, and indeed found two more I had forgotten! Fixes those, too. :)

It'd likely be best to handle new issues (like #304 + #305) in new PRs.

Great work. I read through everything again and it looks good to me. It's quite possible that I will find something when updating the implementation in Firefox, but that IMO shouldn't block this from landing. Like Daniel, I would prefer tracking problems as individual smaller issues.