Closed Bug 1604562 Opened 3 years ago Closed 2 months ago

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

Categories

(DevTools :: Inspector, defect, P2)

72 Branch
defect

Tracking

(firefox-esr68 unaffected, firefox-esr91 wontfix, firefox-esr102 wontfix, firefox71 unaffected, firefox72- wontfix, firefox73 wontfix, firefox74 wontfix, firefox75 wontfix, firefox76 wontfix, firefox77 verified, firefox78 wontfix, firefox101 wontfix, firefox102 wontfix, firefox103 fixed)

RESOLVED FIXED
103 Branch
Tracking Status
firefox-esr68 --- unaffected
firefox-esr91 --- wontfix
firefox-esr102 --- wontfix
firefox71 --- unaffected
firefox72 - wontfix
firefox73 --- wontfix
firefox74 --- wontfix
firefox75 --- wontfix
firefox76 --- wontfix
firefox77 --- verified
firefox78 --- wontfix
firefox101 --- wontfix
firefox102 --- wontfix
firefox103 --- fixed

People

(Reporter: nightmaster, Assigned: emilio)

References

(Regression)

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

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.

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

Flags: needinfo?(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...

Flags: needinfo?(nightmaster)
Attached file demo HTML file

Assigning "Core: Layout" component for this bug.

Component: Untriaged → Layout
Product: Firefox → Core

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

Thanks for reporting this.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Summary: mask-image disappear because of CORS Origin header missing → mask-image disappear because of CORS Origin header missing if in Shadow DOM
Flags: needinfo?(bwerth)
Regressed by: 1391994
Has Regression Range: --- → yes
Attachment #9117569 - Attachment mime type: application/octet-stream → text/html

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?

Flags: needinfo?(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

Flags: needinfo?(nightmaster)

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.

Component: Layout → Inspector
Product: Core → DevTools
Summary: mask-image disappear because of CORS Origin header missing if in Shadow DOM → mask-image disappear because of CORS Origin header missing if edited via the style attribute in inspector

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

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

Assignee: nobody → bwerth
Flags: needinfo?(bwerth)

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(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.

Flags: needinfo?(gl) → needinfo?(bwerth)
Flags: needinfo?(bwerth)
Priority: -- → P2

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

Flags: needinfo?(bwerth)

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?

Flags: needinfo?(bwerth) → needinfo?(nightmaster)

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

Assignee: bwerth → nobody

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.

Flags: needinfo?(nightmaster) → needinfo?(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).

Flags: needinfo?(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?

Flags: needinfo?(emilio)
Flags: needinfo?(ckerschb)

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?

Flags: needinfo?(emilio)
See Also: → 1307730

This is one alternative for the fix.

Assignee: nobody → emilio
Status: NEW → ASSIGNED

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.

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

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?

Flags: needinfo?(ckerschb)

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

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

Flags: needinfo?(jdescottes)
Attachment #9279871 - Attachment description: Bug 1604562 - Use wrapped principal as needed for CORS requests triggered by expanded principals. r=ckerschb → Bug 1604562 - Use wrapped principal as needed for CORS requests triggered by expanded principals. r=freddyb

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

Flags: needinfo?(jdescottes)
Pushed by ealvarez@mozilla.com:
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
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1362c690f9d0
[devtools] Add mochitest for CORS request issue from DevTools style update r=emilio,nchevobbe
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 103 Branch
Regressions: 1773397
Attachment #9279870 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.