-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Relax <select>
parser
#10557
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Relax <select>
parser
#10557
Conversation
Why does this also make a bunch of changes to the processing model of |
The changes in the processing model do support the new proposed content model for select, but they also mitigate compat issues for websites which are already putting tags in between For example, without these changes to the processing model, the following <select>
<div>
<option>...</option>
...
</div>
</select> In my compat analysis, I found a lot of websites which are doing this, so in order to ship the parser changes separately from customizable select in chrome, I plan to ship the parser changes and these processing model changes together, because otherwise there would be too much breakage. I'm happy to put them in a separate PR if you want, or keep them here and update the commit message (sorry for not putting it in there). Which would you prefer? |
Thanks! Given that rationale I think it's good to couple the changes, but that should be in the commit message as well. |
This doesn't define optional tags for The definition for "have a particular element in select scope" may be needed for that, but should be changed to be similar to "have a particular element in button scope" (but for In particular, allow these without a parse error: <select>
<optgroup>
<option>
<optgroup>
</select> The second <select>
<option><p>
<option>
</select> This should generate implied end tags and pop the <select>
<optgroup>
<hr>
<option>
<hr>
</select> The See how the parser deals with |
cf37251
to
afb732d
Compare
Done.
@zcorpan thanks for the feedback! I did some experimenting and added some logic to the start tags for option, optgroup, and hr. What do you think? |
afb732d
to
e08df7e
Compare
|
I thought that this is covered by "While the stack of open elements has an option element in select scope". What exactly should I change?
Done
I'm guessing this is from "If the current node is not now a rtc element or a ruby element, this is a parse error," right? Should I add "If the current node is not now an option element, this is a parse error" after "While the stack of open elements has an option element in select scope, pop an element from the stack of open elements"? |
ea6d608
to
21bc5e6
Compare
The "in select scope" I think should be removed altogether since it assumes the stack will not have other elements when in a High-level of what I think should happen: when parsing option or optgroup start tag: check that select is in scope, check that option or optgroup is in scope, generate implied end tags (except for optgroup when handling I can look into this more next week and suggest more specific changes. |
21bc5e6
to
a57c84e
Compare
Thanks! I gave this a try |
@zcorpan how does the latest text look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the option/optgroup cases are right after these changes.
a57c84e
to
a1edb00
Compare
a1edb00
to
9507110
Compare
I was talking to @mfreed7 about the changes we've made in the PR so far, and I feel like I couldn't provide a good explanation for why we are making the parser not support cases like these:
Is it just compat reasons? Is there a good justification? |
This patch makes the parser allow additional tags in <select> besides <option>, <optgroup>, and <hr>, mostly by removing the "in select" and "in select in table" parser modes. In order to replicate the behavior where opening a <select> tag within another open <select> tag inserts a </select> close tag, a traversal through the stack of open elements was added which I borrowed from the <button> part of the parser. This will need test changes to be implemented in html5lib. Fixes whatwg#10310
9507110
to
1330ad5
Compare
It would be a breaking change from what is conforming HTML today, and break compat for sites that omit |
This was pointed out here: whatwg/html#10557 (comment) Change-Id: I443ddd71e48ecefe598a7be07d0705a68db36d02
This was pointed out here: whatwg/html#10557 (comment) Change-Id: I443ddd71e48ecefe598a7be07d0705a68db36d02 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6077768 Reviewed-by: Traian Captan <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Commit-Queue: Traian Captan <[email protected]> Auto-Submit: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1423995}
This was pointed out here: whatwg/html#10557 (comment) Change-Id: I443ddd71e48ecefe598a7be07d0705a68db36d02 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6077768 Reviewed-by: Traian Captan <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Commit-Queue: Traian Captan <[email protected]> Auto-Submit: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1423995}
This was pointed out here: whatwg/html#10557 (comment) Change-Id: I443ddd71e48ecefe598a7be07d0705a68db36d02 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6077768 Reviewed-by: Traian Captan <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Commit-Queue: Traian Captan <[email protected]> Auto-Submit: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1423995}
source
Outdated
|
||
<li><p>Pop elements from the <span>stack of open elements</span> until a <code>select</code> | ||
element has been popped from the stack.</p></li> | ||
</ol> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in the fragment case you could still end up with an input
element child? That will result in mutation XSS or some such, right? cc @zcorpan @mozfreddyb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think that might cause some XSS-related issues downstream, but then all parser changes like these do... 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I add a fragment parsing context element check for input elements, would that fix this? I'd do the same thing as I did for select elements here: 9d86011
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we dropped the input
element on the floor in the fragment case it might fix it I suppose.
I think we should be very careful that we don't introduce any new opportunities where serialize-then-parse results in a different tree (assuming the original tree was the result of parsing + innerHTML calls).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @zcorpan here. I don't believe this change introduces any new mXSS technique (even if some mutations exist).
I would even go as far as saying that this change could be security-positive. Internally at Google we're aware of HTML sanitizer bypass existing because of the fact that the sanitizer wasn't aware of the special parsing rules for <select>
(so the discrepancy could be exploited). When this change is in effect, the bypass doesn't work anymore because <select>
is now parsed as any other tag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like a conclusion from an incomplete understanding of the current state of this change. Because it's still not parsed quite like any other tag.
I will grant that I'm not sure we need to do it, but I think it would be good if innerHTML
could not be used to generate a different tree here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that there are a lot of cases where innerHTML
can be used to create a DOM that is mutated upon serialize-parse, and we likely can't start to drop elements without breaking the web. Doing so only for input
in select
but not input
in option
in select
or h2
in h1
or any foster-parented content in table
, and so on, doesn't seem like it solves anything. So my conclusion is that we should let input
be inserted.
Also see #10310 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There could be a web compat reason to drop input
though, similar to #10310 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @zcorpan, we have been keeping <input>
in <select>
for assigning to select.innerHTML in stable chrome for a while now with no issues, so I am comfortable keeping it that way.
Thanks for the analysis @securityMB!
Might be a silly question but does the check for HTML fragment parsing algorithm mean that this doesn't apply if you're parsing in an XHTML document? |
@lukewarlow XHTML documents use the XML parser. |
…Relaxation, a=testonly Automatic update from web-platform-tests Update select-value WPT for SelectParserRelaxation This was pointed out here: whatwg/html#10557 (comment) Change-Id: I443ddd71e48ecefe598a7be07d0705a68db36d02 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6077768 Reviewed-by: Traian Captan <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Commit-Queue: Traian Captan <[email protected]> Auto-Submit: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1423995} -- wpt-commits: dcbc65f3c164e3a47ed07a0d216f8a7fc5b0d54d wpt-pr: 50882
…Relaxation, a=testonly Automatic update from web-platform-tests Update select-value WPT for SelectParserRelaxation This was pointed out here: whatwg/html#10557 (comment) Change-Id: I443ddd71e48ecefe598a7be07d0705a68db36d02 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6077768 Reviewed-by: Traian Captan <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Commit-Queue: Traian Captan <[email protected]> Auto-Submit: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1423995} -- wpt-commits: dcbc65f3c164e3a47ed07a0d216f8a7fc5b0d54d wpt-pr: 50882
…Relaxation, a=testonly Automatic update from web-platform-tests Update select-value WPT for SelectParserRelaxation This was pointed out here: whatwg/html#10557 (comment) Change-Id: I443ddd71e48ecefe598a7be07d0705a68db36d02 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6077768 Reviewed-by: Traian Captan <tcaptanchromium.org> Commit-Queue: Joey Arhar <jarharchromium.org> Commit-Queue: Traian Captan <tcaptanchromium.org> Auto-Submit: Joey Arhar <jarharchromium.org> Cr-Commit-Position: refs/heads/main{#1423995} -- wpt-commits: dcbc65f3c164e3a47ed07a0d216f8a7fc5b0d54d wpt-pr: 50882 UltraBlame original commit: 5aa6c6ca474e72bba059f5cca3e877950d617dc7
…Relaxation, a=testonly Automatic update from web-platform-tests Update select-value WPT for SelectParserRelaxation This was pointed out here: whatwg/html#10557 (comment) Change-Id: I443ddd71e48ecefe598a7be07d0705a68db36d02 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6077768 Reviewed-by: Traian Captan <tcaptanchromium.org> Commit-Queue: Joey Arhar <jarharchromium.org> Commit-Queue: Traian Captan <tcaptanchromium.org> Auto-Submit: Joey Arhar <jarharchromium.org> Cr-Commit-Position: refs/heads/main{#1423995} -- wpt-commits: dcbc65f3c164e3a47ed07a0d216f8a7fc5b0d54d wpt-pr: 50882 UltraBlame original commit: 5aa6c6ca474e72bba059f5cca3e877950d617dc7
…Relaxation, a=testonly Automatic update from web-platform-tests Update select-value WPT for SelectParserRelaxation This was pointed out here: whatwg/html#10557 (comment) Change-Id: I443ddd71e48ecefe598a7be07d0705a68db36d02 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6077768 Reviewed-by: Traian Captan <tcaptanchromium.org> Commit-Queue: Joey Arhar <jarharchromium.org> Commit-Queue: Traian Captan <tcaptanchromium.org> Auto-Submit: Joey Arhar <jarharchromium.org> Cr-Commit-Position: refs/heads/main{#1423995} -- wpt-commits: dcbc65f3c164e3a47ed07a0d216f8a7fc5b0d54d wpt-pr: 50882 UltraBlame original commit: 5aa6c6ca474e72bba059f5cca3e877950d617dc7
…Relaxation, a=testonly Automatic update from web-platform-tests Update select-value WPT for SelectParserRelaxation This was pointed out here: whatwg/html#10557 (comment) Change-Id: I443ddd71e48ecefe598a7be07d0705a68db36d02 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6077768 Reviewed-by: Traian Captan <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Commit-Queue: Traian Captan <[email protected]> Auto-Submit: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1423995} -- wpt-commits: dcbc65f3c164e3a47ed07a0d216f8a7fc5b0d54d wpt-pr: 50882
…Relaxation, a=testonly Automatic update from web-platform-tests Update select-value WPT for SelectParserRelaxation This was pointed out here: whatwg/html#10557 (comment) Change-Id: I443ddd71e48ecefe598a7be07d0705a68db36d02 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6077768 Reviewed-by: Traian Captan <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Commit-Queue: Traian Captan <[email protected]> Auto-Submit: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1423995} -- wpt-commits: dcbc65f3c164e3a47ed07a0d216f8a7fc5b0d54d wpt-pr: 50882
This spec PR currently disallows This would allow this to be parsed without the parser changing things: <select>
<div>
<input> |
That would mean |
Good point, I'd be OK with doing the |
That doesn't answer the more significant question. (And I don't really have the data to pick between that and what you suggested. I was just pointing out a consequence.) |
I have UseCounters for this which haven't reached stable yet, and I am working with accessibility folks on this right now. I can share more once the UseCounters hit stable and when I get more accessibility feedback. |
Based on feedback in the <select> parser relaxation HTML spec PR, we might want to close the <select> on these tags when parsing an <input>: whatwg/html#10557 (comment) If the UseCounter shows that usage is low enough, then we could remove this behavior. Bug: 402429384 Change-Id: Id8809d40162c7fd06a446e146d65aaf6258d2e5b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6506819 Reviewed-by: Mason Freed <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1459707}
Based on feedback in the <select> parser relaxation HTML spec PR, we might want to close the <select> on these tags when parsing an <input>: whatwg/html#10557 (comment) If the UseCounter shows that usage is low enough, then we could remove this behavior. Bug: 402429384 Change-Id: Id8809d40162c7fd06a446e146d65aaf6258d2e5b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6506819 Reviewed-by: Mason Freed <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1459707}
Based on feedback in the <select> parser relaxation HTML spec PR, we might want to close the <select> on these tags when parsing an <input>: whatwg/html#10557 (comment) If the UseCounter shows that usage is low enough, then we could remove this behavior. Bug: 402429384 Change-Id: Id8809d40162c7fd06a446e146d65aaf6258d2e5b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6506819 Reviewed-by: Mason Freed <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1459707}
This patch makes the parser allow additional tags in
<select>
besides<option>
,<optgroup>
, and<hr>
, mostly by removing the "in select" and "in select in table" parser modes.In order to replicate the behavior where opening a
<select>
tag within another open<select>
tag inserts a</select>
close tag, a traversal through the stack of open elements was added which I borrowed from the<button>
part of the parser.This patch also changes the processing model to make
<select>
look through all its descendants in the DOM tree for<option>
elements, rather than just children and optgroup children which conform to the content model. This is needed for compat reasons because there are websites which put other tags in between their<select>
and<option>
s which would break with this parser change unless we also update this processing model. More context here and here.Fixes #10310
<select>
parser mozilla/standards-positions#1086<select>
parser WebKit/standards-positions#414(See WHATWG Working Mode: Changes for more details.)
/form-elements.html ( diff )
/index.html ( diff )
/infrastructure.html ( diff )
/parsing.html ( diff )