Re-write default handling and supply default values. #60
Conversation
index.bs
Outdated
| @@ -198,6 +208,14 @@ dictionary which describes modifications to the sanitze operation. | |||
| : dropAttributes | |||
| :: The <dfn>attribute drop list</dfn> is an [=attribute match list=], which | |||
| determines whether an attribute (on a given element) should be dropped. | |||
| : allowUnknownElements | |||
There was a problem hiding this comment.
I don't think we want a blanket allow-boolean for this case (and the attribute case below). I think we should ask developers to specifically list the elements & attributes they need.
I know this puts an extra onus on e.g., framework authors, but I'd rather do this than provide a footgun that turns this into a security bug. Happy to debate this further.
There was a problem hiding this comment.
I'm not sure I agree, honestly. I'll remove them in the next update, but I guess I'll make an attempt at getting them back in again. :)
index.bs
Outdated
| "allowUnknownElements": false | ||
| "allowUnknownAttributes": false, | ||
| "allowCustomElements": false, | ||
| "allowElements": [ |
There was a problem hiding this comment.
Same nit as above, plus I'd prefer we figure out a way to name it once in the spec and then reference it from there instead of having it fully enumerated in two places.
There was a problem hiding this comment.
Not sure I agree. The baseline and the default config will differ in subtle ways. Otherwise, we could just merge them again.
Arguably, it'd make sense to have an easier way to construct them, like:
default = { allowElement: baseline.allowElements + ["bla", "bla", "blubb"] }
Or so. That'd likely be much easier to understand.
Incidentally, today someone brought up exactly that point when discussing allowUnknownElements. So if someone wants to add an "unknown" element to the list, they shouldn't have to research and repeat the entire allow-list. I guess we're missing a mechanism there.
(Could be: Default config exposed, so people can easily build on it. Or every Sanitizer exposes their config. Or "inheritance", where you name a "parent" Sanitizer. Or... )
There was a problem hiding this comment.
Would it make sense to add a (non-normative, I guess?) section that explains how the values were constructed? Ideally in executable form.
Something like: https://github.com/otherdaniel/purification/blob/default-construction/resources/configurations.html
The idea is that one can copy&paste the results into the spec (or generate them as part of the deploy script). If all goes well, the code version could be more easily readable & modifyable than the spec version, but we'd still have a definite version in the spec.
resources/baseline-config.json
Outdated
| @@ -0,0 +1,755 @@ | |||
| { | |||
There was a problem hiding this comment.
I've run these files through a tool called json_pp (JSON pretty print) that I found on my machine. I'm not sure this format is the best, but it's very diff-able.
Change the default lists and sanitizing logics according to: WICG/sanitizer-api#38 and WICG/sanitizer-api#60 Bug: 1116418 Change-Id: Ie4866b09ec3cd94a6bb38075ba7068ddd2a448a6 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2698917 Commit-Queue: Yifan Luo <[email protected]> Reviewed-by: Daniel Vogelheim <[email protected]> Cr-Commit-Position: refs/heads/master@{#856648}
|
I've uploaded a re-write of this PR. I'll re-add actual default values next. |
|
I've also (re-) added actual default values, as well as a script that computes them. The intention is that the script is easier to understand and modify, but we'd still have exact values in the spec. I'd appreciate another round of reviews, looking both at the actual default values, as well as the mechanics for how we construct them. |
otherdaniel
force-pushed
the
new-default
branch
from
8005b1c to
b7077cd
Compare
|
oh darn. merging #70 before this made this a revision with merge conflicts. 😖 |
|
let's land this :) |
otherdaniel
force-pushed
the
new-default
branch
from
3362204 to
b877ea9
Compare
|
Landed! :) |
Change the default lists and sanitizing logics according to: WICG/sanitizer-api#38 and WICG/sanitizer-api#60 Bug: 1116418 Change-Id: Ie4866b09ec3cd94a6bb38075ba7068ddd2a448a6 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2698917 Commit-Queue: Yifan Luo <[email protected]> Reviewed-by: Daniel Vogelheim <[email protected]> Cr-Commit-Position: refs/heads/master@{#856648} GitOrigin-RevId: 716ecaeea7b768608631d61682b3fdf9e086ec78
This PR
The first commit defined the built-ins as a block-list; the second also as an allow-list.
I find the second one to be somewhat harder to read. (E.g. it's more difficutl to find the difference between built-in and default lists.)