Upstream sanitizer api#12395
Conversation
223a4d1 to
d2034e5
Compare
|
@zcorpan @evilpie @mozfreddyb @otherdaniel |
|
Amazing, thanks for working on this. The built-in safe default configuration is pretty integral to the API, where did I go? For anyone else looking at this, the gist of the changes are in dynamic-markup-insertion.html. |
Oh you're right I had it on my todo list and forgot. Getting to it. Thanks! |
Done. |
|
I thought as part of moving this into the HTML standard we'd also address the parser integration issue? |
This is a huge PR so I thought doing it in two stages, the first one being a purely technical upstream, would be easier to review? Open and happy to incorporate the stream-while-parsing changes in this PR if you and @zcorpan are ok to review that in one go. |
|
I prefer doing the parser integration in a follow-up PR. |
|
I think these three PRs would be good to merge before merging into the HTML standard: |
Since some security sensitive changes rely on "sanitizing while parsing", and that in turn relies on the current post-processing sanitizer being upstreamed, I don't think we should delay upstreaming any further. Can we race it? If any of these go in before the upstream PR is in I'll incorporate them into the HTML PR. |
| data-x="dom-SanitizerProcessingInstruction-target">target</code> member.</p> | ||
| </div> | ||
|
|
||
| <div algorithm> |
There was a problem hiding this comment.
Opened whatwg/infra#709 for now.
I'm not sure about the comparator thing - infra doesn't really say what it means that two items in a list are the same. Would it be enough to mention here whaht makes items of attributes/elements/... lists "equal"
otherdaniel
left a comment
There was a problem hiding this comment.
Thank you, and I'm super happy to see this happening!
I wonder if we can link to the "Security Considerations" section in the current spec; or have them in a supplementary document somewhere?
otherdaniel
left a comment
There was a problem hiding this comment.
Thank you, and I'm super happy to see this happening!
I wonder if we can link to the "Security Considerations" section in the current spec; or have them in a supplementary document somewhere?
ea79a5b to
1e065df
Compare
I've upstreamed them instead into a security consideration subsection |
|
I've refactored some of the sanitization constants to go into each element's definition instead of being in one huge table. I think that makes it less error prone when we add new elements in the future. If that's undesirable I'm happy to revert. |
| data-x="dom-SanitizerProcessingInstruction-target">target</code> member.</p> | ||
| </div> | ||
|
|
||
| <div algorithm> |
…nition lists near the top of the file
5866163 to
b3593af
Compare
|
I believe everything normative is now equivalent to the WICG spec. |
otherdaniel
left a comment
There was a problem hiding this comment.
Gave this a complete read, and this looks great!
Have some (entirely optional) comments below.
| <ol> | ||
| <li><p>Set <var>element</var>["<code | ||
| data-x="dom-SanitizerElementNamespaceWithAttributes-attributes">attributes</code>"] to the | ||
| result of <span data-x="set create">removing duplicates</span> from |
There was a problem hiding this comment.
"remove duplicates" is now mapped to "create a set", which does the same operation. If so, I think we can just call it that.
| };</code></pre> | ||
|
|
||
| <p><code>SanitizerElementNamespace</code>, <code>SanitizerAttributeNamespace</code>, | ||
| <code>SanitizerAttribute</code>, and <code>SanitizerProcessingInstruction</code> dictionaries are |
There was a problem hiding this comment.
SanitizerAttribute isn't a dictionary; it's a typedef of a union. Is SanitizerElementNamespaceWithAttributes meant?
| <p>If all of the following are true:</p> | ||
|
|
||
| <ul class="brief"> | ||
| <li><p><var>safe</var> is true;</p></li> |
There was a problem hiding this comment.
The paragraph above asks for 'all of the following' to be true, and this asks for |safe| to be true. While correct, I think the condition is redundant.
| </tr> | ||
| <tr> | ||
| <td><code data-x="SVG title">title</code></td> | ||
| <td><span data-x="SVG namespace">SVG</span></td> |
There was a problem hiding this comment.
Missing an empty <td></td>. The third column is currently "merged" with the following box; or at least it looks that way.
|
|
||
| <!-- NON-NORMATIVE SECTION --> | ||
|
|
||
| <p>Mutation XSS or mXSS describes an attack that exploits cases where the parsed DOM structure is |
There was a problem hiding this comment.
I think readability would benefit from splitting this up in paragraphs (as it originally was, https://wicg.github.io/sanitizer-api/#mutated-xss)
Convert the incubated spec in https://wicg.github.io/sanitizer-api/ to the HTML format and make it part of the HTML standard.
(See WHATWG Working Mode: Changes for more details.)
/canvas.html ( diff )
/comms.html ( diff )
/dom.html ( diff )
/dynamic-markup-insertion.html ( diff )
/edits.html ( diff )
/embedded-content-other.html ( diff )
/form-elements.html ( diff )
/forms.html ( diff )
/grouping-content.html ( diff )
/iframe-embed-object.html ( diff )
/image-maps.html ( diff )
/imagebitmap-and-animations.html ( diff )
/index.html ( diff )
/indices.html ( diff )
/infrastructure.html ( diff )
/input.html ( diff )
/interaction.html ( diff )
/microdata.html ( diff )
/parsing.html ( diff )
/references.html ( diff )
/rendering.html ( diff )
/scripting.html ( diff )
/sections.html ( diff )
/semantics.html ( diff )
/system-state.html ( diff )
/tables.html ( diff )
/text-level-semantics.html ( diff )
/timers-and-user-prompts.html ( diff )
/web-messaging.html ( diff )
/webstorage.html ( diff )
/workers.html ( diff )