1604562 - mask-image disappear because of CORS Origin header missing if edited via the style attribute in inspector

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[nightmaster]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:72.0) Gecko/20100101 Firefox/72.0

Steps to reproduce:

Set a mask-image on an element with another origin than current; e.g. I am seeing my page on localhost and my SVG image comes from localhost:8080.

Example code: <div style="mask-image:'demo.svg'></div>

Actual results:

When you work on this mask image (adding, changing or removing other CSS properties linked to it), the image disappears, and console print an error message pointing on this page: https://developer.mozilla.org/fr/docs/Web/HTTP/CORS/Errors/CORSOriginHeaderNotAdded
But it happens only on Aurora channel (Developer Edition: v72), it works fine on stable release (v70), and worked fine on previous version (71)

Expected results:

The image should not disappear at all, unless the mask-image is removed, or the colour applied to it is the same as background.

Comment 1

[Hani Yacoub, Desktop QA]

Could you please provide a test page example in order to reproduce this issue?
Thanks.

Comment 2

[nightmaster]

you can use this page
And I need to specify that the problem only happens when using mask in Shadow DOM.

Demo fully shows the fact: mask on standard element works (navy one) perfectly initially, as well as the Shadow DOM (green one).
But try to make any action on the green, via inspector, that has an impact on the mask image, and it will disappear and refuse to display again unless you refresh the page. But it works if you do it programmatically...

Comment 3

[nightmaster]

Attachment: demo HTML file

Comment 4

[Hani Yacoub, Desktop QA]

Assigning "Core: Layout" component for this bug.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

[Tracking Requested - why for this release]: last-minute web-visible regression.

Thanks for reporting this.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Mozregression points to bug 1391994, so this is a devtools-only regression, which is something at least.

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=6dc1bde1256e5e2160e6437766c60ec1213542d7&tochange=70d2c0d4a3e89d42a7d16c0c1cb3aa23c3cfbfbe

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Can you sanity-check me and confirm that this only happens with devtools if changing the style attribute?

Or do you see it in any scenario where the inspector isn't involved?

Comment 8

[nightmaster]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)

Can you sanity-check me and confirm that this only happens with devtools if changing the style attribute?

Or do you see it in any scenario where the inspector isn't involved?

As far as I can see, it only happens with inspector. When I adjust via JS there is no problem, and initial rendering is OK

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Thanks, yeah... it also reproduces in the non-shadow-dom case if you put the mask in the style attribute rather than the CSS rule.

Comment 10

[Julien Cristau [:jcristau]]

Not tracking because it's late for 72 and this is "just" devtools.

Comment 11

[Brad Werth [:bradwerth]]

I'll figure it out. Thanks for the testcase.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:gl, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 13

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #12)

The priority flag is not set for this bug.
:gl, could you have a look please?

For more information, please visit auto_nag documentation.

Passing this over to Brad since he's already on it.

Comment 14

[Pascal Chevrel:pascalc]

Brad, could you give us an update on this bug please? Thanks

Comment 15

[Brad Werth [:bradwerth]]

I am struggling to reproduce with the testcase. I made a more permanently-hosted version of the testcase at http://www.bradleyjwerth.com/webtest/missingMask.html. What are the steps to reproduce with this testcase?

Comment 16

[Pascal Chevrel:pascalc]

Too late for 74.

Comment 17

[Brad Werth [:bradwerth]]

I'm not working in this area anymore. Taking myself off the bug to make it clear that somebody else can pick it up.

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

Redirect a needinfo that is pending on an inactive user to the triage owner.
:jdescottes, since the bug has high priority and recent activity, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 19

[Julian Descottes [:jdescottes]]

Here's yet another test case (since all the links have expired)
https://bug1604562-test-case.glitch.me/

STRs:

• open devtools
• select the DIV
• update anything in the rule view (eg change the height from 100px to 101px)

ER: The mask should still apply
AR: The mask disappears, and the console shows:

Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at https://bug1604562-svg-origin.glitch.me/mask.svg. (Reason: CORS header ‘Origin’ cannot be added).

Comment 20

[Julian Descottes [:jdescottes]]

It seems the issue comes down to calling rawNode.setAttributeDevtools in style-rule.js. Using the regular setAttribute, we don't get any issue here.

setAttributeDevtools uses a dedicated principal which bypasses CSP issues, but doesn't seem to be considered with the same origin as the page's principal (https://searchfox.org/mozilla-central/rev/4c3f6e8bf87fffb7c62feb4c76a14e0eb0b94c1f/dom/base/Element.cpp#1562).

The limitation comes from here:
https://searchfox.org/mozilla-central/rev/4c3f6e8bf87fffb7c62feb4c76a14e0eb0b94c1f/netwerk/protocol/http/nsCORSListenerProxy.cpp#986-992 where we explicitly disallow doing CORS for an expanded principal.

I am not sure if this is actionable, can we technically relax the restriction?
Emilio, Christoph, what do you think?

Comment 21

[Emilio Cobos Álvarez (:emilio)]

So that code basically comes from bug 1307730. Maybe expanded principals with only one principal in the list like the one we use for devtools could use the origin of the single expanded principal.

That said, since we only use this for the inline attribute check, is there any reason why just using the system principal as the triggering wouldn't work? In particular, if a malicious actor can run system principal code, it can also call setAttributeDevTools to bypass this check, can't it?

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1604562 - Stop using an expanded principal for DevTools. r=ckerschb

This is one alternative for the fix.

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1604562 - Use wrapped principal as needed for CORS requests triggered by expanded principals. r=freddyb

As long as they only wrap one principal, we have a meaningful origin to
use for the CORS request.

This is another alternative for the fix.

Comment 24

[Emilio Cobos Álvarez (:emilio)]

I sent two potential fixes. Both fix the issue but I'm less sure about the trade-offs with the patch from comment 22.

Comment 25

[Frederik Braun [:freddy]]

Thanks for proposing both patches. I see a lot of value in both approaches.

1. D148395 Using the SystemPrincipal for DevTools, instead of this one-list ExpandedPrincipal is cutting through a lot of complexity and removes the skipInlineChecks. That would make the intent and level of privilege by DevTools-specific actions clear. However, switching to the SystemPrincipal bypasses a lot of other checks which we might want to require down the line?
   Especially for CSS-specifics, I am not entirely sure if this would have an effect on any kind of state (e.g., CSS variables, subsequent resource loads through @import, font etc.)? I am a bit worried about unintentional side-effects and haven't followed the call-chain too deep after SetAttribute, SetAttr, ParseAttr... Maybe parsing is already stateful?

2. D148396 This looks like it's closer to fixing the actual bug and choosing the right kind of Principal for a multi-context situation, which is less broad and therefore less error prone. But if we go down that route, we might have to do lots of quirky auto* ep = BasePrincipal::Cast(aPrincipal)->As<ExpandedPrincipal>(); if (ep->AllowList().Length() != 1) { ... snippets, which are an odd way to say that something is a special-cased DevTools principal..

@emilio: I believe you might just know this from the top of your head. Can you share some thoughts about side-effects of using the SystemPrincipal (option 1). Maybe this is all fine after all?

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Yeah, so thinking a bit deeper about it, even though (1) is definitely simpler, it probably has a bunch of (probably not terrible but weird) side-effects.

For example (I haven't tested but I'm happy to do so) I'd expect the opposite case from this one to break (a CORS request that should be blocked would stop being blocked after you interact with devtools).

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Julian, do you know how to best test this / where I could crib from?

Comment 28

[Julian Descottes [:jdescottes]]

Attachment: Bug 1604562 - [devtools] Add mochitest for CORS request issue from DevTools style update

Comment 29

[Julian Descottes [:jdescottes]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #27)

Julian, do you know how to best test this / where I could crib from?

I ended up writing a small test :) https://phabricator.services.mozilla.com/D148657
It should fail for now, and should pass if the issue gets resolved. And thanks a lot for working on the fix!

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/89f4150cac69
Use wrapped principal as needed for CORS requests triggered by expanded principals. r=necko-reviewers,freddyb,kershaw

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1362c690f9d0
[devtools] Add mochitest for CORS request issue from DevTools style update r=emilio,nchevobbe

Comment 32

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/89f4150cac69

Comment 33

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/1362c690f9d0

Comment 34

[Kris Maglione [:kmag]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)

Mozregression points to bug 1391994, so this is a devtools-only regression, which is something at least.

I don't think that's strictly true. This should also apply to extension content scripts changing the style attribute, and their allow lists won't be one element long. I think it would be better for this code to use PrincipalToInherit, or to at least pick a principal when the allow list is more than one element long.

Emilio, are you sure this only applies to devtools?

Comment 35

[Emilio Cobos Álvarez (:emilio)]

No, I think you're right. Shouldn't be hard to create a test-case to confirm? I wasn't familiar with principalToInherit, but it seems like the right thing to use here.