Bug 235836

Summary: Inert subtrees fail to block activation from contained svg elements
Product: WebKit Reporter: Dan Hite <danhite>
Component: Layout and RenderingAssignee: Tim Nguyen (:ntim) <ntim>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, changseok, clopez, dino, esprehn+autocc, ews-watchlist, fmalita, fred.wang, glenn, gyuyoung.kim, koivisto, kondapallykalyan, mmaxfield, ntim, pdr, sabouhallawa, schenney, sergio, simon.fraser, webkit-bug-importer, youennf, zalan
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: All   
OS: All   
See Also: https://github.com/web-platform-tests/wpt/pull/32940
Bug Depends on:    
Bug Blocks: 84635, 165279    
Attachments:
Description Flags
Testcase using modal dialogs
none
Patch
koivisto: review+, ews-feeder: commit-queue-
[fast-cq] Patch none

Description Dan Hite 2022-01-28 14:18:49 PST
I just got ipadOS 15.4beta 1 and was excited to try the new "inert" idl/attribute
which worked as expected wrt the spec (good work, very cool!!);
https://html.spec.whatwg.org/multipage/interaction.html#inert

however the spec itself has a blindspot in that it refers to ~just html elements, ignoring the
<svg> elements integration within html

that is, on very first page I tried testing inert, which describes the inert attribute:
https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/inert

^ then setting document.body.inert = true
Immediately I found an oops-isn't-actually-inert misfeature when tapping around the (inert) page suddenly nav'ed me

you see, MDN does their upper left logo/link html <a> as having an <svg> graphic within it

if you replace the innerHTML of their <a> with text (ie html not svg) then the page was satisfactorily inert

but your hit-test-event-dispatch code for a tap within the svg apparently doesn't follow your new html inert logic
since it dispatches and bubbles into the html <a> and thus navs


if the inert feature, in future, were in widespread use, then this spec foo could be a minor security issue, as page authors might
assume they'd locked down ui on a piece sanitized html, but suddenly a simplistic attack gets an activation ala
<svg><a ...
Comment 1 Sam Sneddon [:gsnedders] 2022-01-29 08:48:10 PST
(In reply to Dan Hite from comment #0)
> I just got ipadOS 15.4beta 1 and was excited to try the new "inert"
> idl/attribute
> which worked as expected wrt the spec (good work, very cool!!);
> https://html.spec.whatwg.org/multipage/interaction.html#inert
> 
> however the spec itself has a blindspot in that it refers to ~just html
> elements, ignoring the
> <svg> elements integration within html

pretty sure from a spec POV it applies to the entire subtree, regardless of whether they're HTML or SVG or MathML or anything else

> 
> that is, on very first page I tried testing inert, which describes the inert
> attribute:
> https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/inert
> 
> ^ then setting document.body.inert = true
> Immediately I found an oops-isn't-actually-inert misfeature when tapping
> around the (inert) page suddenly nav'ed me
> 
> you see, MDN does their upper left logo/link html <a> as having an <svg>
> graphic within it
> 
> if you replace the innerHTML of their <a> with text (ie html not svg) then
> the page was satisfactorily inert
> 
> but your hit-test-event-dispatch code for a tap within the svg apparently
> doesn't follow your new html inert logic
> since it dispatches and bubbles into the html <a> and thus navs
> 
> 
> if the inert feature, in future, were in widespread use, then this spec foo
> could be a minor security issue, as page authors might
> assume they'd locked down ui on a piece sanitized html, but suddenly a
> simplistic attack gets an activation ala
> <svg><a ...

That said, this does sound like a bug in WebKit…
Comment 2 Radar WebKit Bug Importer 2022-02-01 15:46:01 PST
<rdar://problem/88352589>
Comment 3 Tim Nguyen (:ntim) 2022-02-04 07:21:19 PST
Seems like this affects inertness created by modal dialogs as well, not just the inert attribute.
Comment 4 Tim Nguyen (:ntim) 2022-02-04 07:23:13 PST
Created attachment 450893 [details]
Testcase using modal dialogs
Comment 5 Tim Nguyen (:ntim) 2022-02-04 07:30:32 PST
Looks like SVG has its own pointerEvents checks: https://sourcegraph.com/search?q=context:global+repo:%5Egithub%5C.com/WebKit/WebKit%24+pointerEvents%28%29&patternType=literal
Comment 6 Tim Nguyen (:ntim) 2022-02-22 04:04:41 PST
Created attachment 452854 [details]
Patch
Comment 7 Tim Nguyen (:ntim) 2022-02-22 04:05:18 PST
Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/32940
Comment 8 EWS Watchlist 2022-02-22 04:07:12 PST
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 9 Antti Koivisto 2022-02-22 04:30:39 PST
Comment on attachment 452854 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=452854&action=review

> Source/WebCore/ChangeLog:9
> +        Re-using visibleToHitTesting in SVG code isn't possible, because visibleToHitTesting removes visibility: hidden;
> +        content from hit-testing, which we do not want to here, since pointer-events has values for SVG which still

That's pretty silly. SVG spec really makes invisible things hittable?

> Source/WebCore/rendering/style/RenderStyle.h:716
> +    PointerEvents pointerEventsIncludingInert() const { return effectiveInert() ? PointerEvents::None : pointerEvents(); }

I think you proposed effectiveUserSelect() earlier but we then went with *IncludingInert instead. I think you were right and we should just go with effective* with all these (it reads better and is more expandable), wether backed by a fake property or not.
Comment 10 Antti Koivisto 2022-02-22 04:38:24 PST
Comment on attachment 452854 [details]
Patch

visibleToHitTesting() should likely also use the new effective getter. That would fix an apparent bug too where ignoreCSSPointerEventsProperty is not respected for inert. Looking at the call site, it is used for color sampling which should happen on inert elements too (it could use renaming).
Comment 11 Tim Nguyen (:ntim) 2022-02-22 04:48:00 PST
(In reply to Antti Koivisto from comment #9)
> Comment on attachment 452854 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452854&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Re-using visibleToHitTesting in SVG code isn't possible, because visibleToHitTesting removes visibility: hidden;
> > +        content from hit-testing, which we do not want to here, since pointer-events has values for SVG which still
> 
> That's pretty silly. SVG spec really makes invisible things hittable?

As surprising as it is, yes: https://developer.mozilla.org/en-US/docs/Web/CSS/pointer-events#svg_only_experimental_for_html

There are the visibilePainted/visibleFill/visibleStroke/visible values to omit visibility: hidden elements.

(In reply to Antti Koivisto from comment #10)
> Comment on attachment 452854 [details]
> Patch
> 
> visibleToHitTesting() should likely also use the new effective getter. That
> would fix an apparent bug too where ignoreCSSPointerEventsProperty is not
> respected for inert. Looking at the call site, it is used for color sampling
> which should happen on inert elements too (it could use renaming).

Sounds good, will change that then.
Comment 12 Tim Nguyen (:ntim) 2022-02-22 05:57:29 PST
Created attachment 452858 [details]
[fast-cq] Patch
Comment 13 Tim Nguyen (:ntim) 2022-02-22 07:46:08 PST
Committed r290306 (247629@trunk): <https://commits.webkit.org/247629@trunk>
Comment 14 Brent Fulgham 2022-05-26 14:51:58 PDT
This fix shipped with Safari 15.5 (all platforms).