Inert subtrees fail to block activation from contained svg elements

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Dan Hite]

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 ...

[Sam Sneddon [:gsnedders]]

(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…

[Radar WebKit Bug Importer]

<rdar://problem/88352589>

[Tim Nguyen (:ntim)]

Seems like this affects inertness created by modal dialogs as well, not just the inert attribute.

[Tim Nguyen (:ntim)]

Created attachment 450893 [details]
Testcase using modal dialogs

[Tim Nguyen (:ntim)]

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

[Tim Nguyen (:ntim)]

Created attachment 452854 [details]
Patch

[Tim Nguyen (:ntim)]

Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/32940

[EWS Watchlist]

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

[Antti Koivisto]

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.

[Antti Koivisto]

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).

[Tim Nguyen (:ntim)]

(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.

[Tim Nguyen (:ntim)]

Created attachment 452858 [details]
[fast-cq] Patch

[Tim Nguyen (:ntim)]

Committed r290306 (247629@trunk): <https://commits.webkit.org/247629@trunk>

[Brent Fulgham]

This fix shipped with Safari 15.5 (all platforms).