Closed Bug 1391994 Opened 7 years ago Closed 5 years ago

CSP blocks editing style

Categories

(DevTools :: Inspector: Rules, defect, P2)

Product:
DevTools
Component:
Inspector: Rules
Type:
defect

Tracking

(firefox72 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 72
Tracking Flags:
Tracking Status
firefox72 --- fixed

People

(Reporter: jrmuizel, Assigned: bradwerth)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [dt-q])

Attachments

(3 files, 9 obsolete files)

Bug 1391994 Part 1: Add new methods to Element to set attributes via devtools.
47 bytes, text/x-phabricator-request
bzbarsky
: feedback+
Details | Review
Bug 1391994 Part 2: Use the new SetAttributeDevtools methods in callsites where devtools modifies a node it hasn't just created.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1391994 Part 3: Expand devtools tests to verify interactive style edits work on pages with CSP restrictions.
47 bytes, text/x-phabricator-request
Details | Review 
I get something like:

Content Security Policy: The page’s settings blocked the loading of a resource at self (“style-src https://crates.io https://www.google.com https://ajax.googleapis.com”). Source: width: 100;.

When trying to edit the element style in the rules pane on https://crates.io/crates/goblin
Component: Developer Tools → Developer Tools: CSS Rules Inspector
Just to make it clear for people trying to reproduce this:

1. Open the browser console (ctrl+shift+J)
2. open https://crates.io/crates/goblin in a new tab
3. open the inspector in the tab
4. with the <body> selected (default), make sure the "Rules" panel is visible in the sidebar
5. in the element { ... } section, type any CSS declaration, like width: 100px;

At each character you type, the following error is logged in the browser console:

Content Security Policy: The page’s settings blocked the loading of a resource at self (“style-src https://crates.io https://www.google.com https://ajax.googleapis.com”). Source: width: 100px;

Hey Christoph, in bug 1185351 we did something similar but only for native anonymous content, so we sort of took a shortcut.
Here, I think we would have to detect whether the caller is chrome or content in this case.

I believe there are (at least) 2 places in DevTools that trigger this problem.

1. If you follow the steps above, the following code ends up being called:
http://searchfox.org/mozilla-central/rev/05c4c3bc0cfb9b0fc66bdfc8c47cac674e45f151/devtools/server/actors/styles.js#1266-1268

2. If, instead, you try to set an inline style="color:red" attribute on a node in the inspector, then this code is called:
http://searchfox.org/mozilla-central/rev/05c4c3bc0cfb9b0fc66bdfc8c47cac674e45f151/devtools/server/actors/inspector.js#728-745

Can you take a look at see whether a CSP fix (skipping CSP if the caller is chrome sounds like what we should do) would be needed for this?
Blocks: top-inspector-bugs
status-firefox57: --- → fix-optional
Flags: needinfo?(ckerschb)
Priority: -- → P2
(In reply to Patrick Brosset <:pbro> from comment #1)
> Can you take a look at see whether a CSP fix (skipping CSP if the caller is
> chrome sounds like what we should do) would be needed for this?

While I agree that we should skip the CSP check if the caller is chrome, I think it's not something the CSP code can figure out using the current setup. It would have to happen elsewhere and most importantly earlier.

In more detail, e.g., when calling CSPAllowsInlineStyle() [1] we would have to pass a SystemPrincipal instead of the NodePrincipal for those particular cases which would then cause the CSP checks to wave those kinds of loads through.

Does that make sense?

[1] https://dxr.mozilla.org/mozilla-central/source/layout/style/nsStyleUtil.h#176
Flags: needinfo?(ckerschb)
Any progress on this? It's quite a pain to not be able to edit style on pages with CSP.
Flags: needinfo?(ckerschb)
(In reply to Jeff Muizelaar [:jrmuizel] from comment #4)
> Any progress on this? It's quite a pain to not be able to edit style on
> pages with CSP.

Happy to provide help and guidance but don't have the cycles to take on that work on at the moment. Patrick, maybe someone from your team?
Flags: needinfo?(ckerschb) → needinfo?(pbrosset)
Brad, I know you already have a lot going on, but do you think you'd have some time for this one? Christoph can help with navigating the CSP code if needed. And I can help with the DevTools JS code causing this (see comment 1).
Flags: needinfo?(pbrosset) → needinfo?(bwerth)
I've finally started looking at this. Sorry for the delay.
Assignee: nobody → bwerth
Flags: needinfo?(bwerth)
The provided patch doesn't solve the problem yet, but it's a start for implementing the strategy laid out in comment 2.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> In more detail, e.g., when calling CSPAllowsInlineStyle() [1] we would have
> to pass a SystemPrincipal instead of the NodePrincipal for those particular
> cases which would then cause the CSP checks to wave those kinds of loads
> through.

Christoph, is this the only way to solve the problem? I ask because the idea of the triggering principal overriding the subject principal seems promising and is relatively easy to implement. The proposed patch attempts this, but the SystemPrincipal doesn't override the document Principal in BasePrincipal::OverridesCSP(). I confess that I don't understand Principals that well. Is there a reason why the SystemPrincipal shouldn't override the document Principal?

If this override approach won't work, then the fix for this will involve either:

1) Changing the document Principal to a SystemPrincipal while the devtools are open -- which would seem to allow other kinds of content security violations, so is probably a non-starter.
2) Passing a flag from ElementBinding::SetAttribute down through 7 dependent calls in order to supply a SystemPrincipal instead of the document Principal within nsStyledElement::ParseStyleAttribute. This seems really intrusive and I'd like to avoid it.

All insights appreciated!
Flags: needinfo?(ckerschb)
https://developer.mozilla.org/en-US/docs/Mozilla/Gecko/Script_security#Privileged_to_unprivileged_code says that...

> The system principal passes all security checks. It subsumes itself and all other principals. Chrome code, by definition, runs with the system principal, as do frame scripts.

If this is the case, and BasePrincipal::OverridesCSP() implements the concept of "subsumes", then it seems safe and correct for SystemPrincipal objects to override all other principals (and return true). I'll add that to the patch.
Attachment #8940896 - Flags: review?(bzbarsky)
Attachment #8940897 - Flags: review?(ttromey)
Attachment #8941649 - Flags: review?(bzbarsky)
Attachment #8941650 - Flags: review?(ttromey)
Comment on attachment 8940896 [details]
Bug 1391994 Part 1: Define Element::setAttributeSystem and ::setAttributeSystemNS to allow Chrome callers to set attributes using the System Principal.

https://reviewboard.mozilla.org/r/211158/#review217712

r=me with the issues below fixed.

::: commit-message-ca379:1
(Diff revision 2)
> +Bug 1391994 Part 1: Define Element::setAttributeSystem and ::setAttributeSystemNS to allow Chrome callers to set attributes using the System Principal.

The commit message should probably explain why setAttribute/setAttributeNS doesn't do what is needed here.

And s/Chrome/system/.

::: dom/base/Element.cpp:1455
(Diff revision 2)
> +                            ErrorResult& aError)
> +{
> +  // Run this through SetAttribute with the System Principal.
> +  nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager();
> +  nsCOMPtr<nsIPrincipal> sysPrincipal;
> +  ssm->GetSystemPrincipal(getter_AddRefs(sysPrincipal));

nsContentUtils::GetSystemPrincipal instead of all this, please.  Both places.

::: dom/webidl/Element.webidl:48
(Diff revision 2)
>    [CEReactions, NeedsSubjectPrincipal=NonSystem, Throws]
>    void setAttribute(DOMString name, DOMString value);
>    [CEReactions, NeedsSubjectPrincipal=NonSystem, Throws]
>    void setAttributeNS(DOMString? namespace, DOMString name, DOMString value);
> +  [ChromeOnly, CEReactions, Throws]
> +  void setAttributeSystem(DOMString name, DOMString value);

This needs documentation to explain why it's there, as does setAttributeSystemNS.

The new methods should also go on a partial interface documented as being Gecko extensions, not in the middle of a bunch of standard stuff.
Attachment #8940896 - Flags: review?(bzbarsky) → review+
Attachment #8941649 - Flags: review?(bzbarsky) → review?(kmaglione+bmo)
Comment on attachment 8940897 [details]
Bug 1391994 Part 2: Use the new Element::setAttributeSystem and ::setAttributeSystemNS methods for rule changes made in the inspector.

https://reviewboard.mozilla.org/r/211160/#review217718

::: devtools/server/actors/inspector.js:754
(Diff revision 2)
>          }
>        } else if (change.attributeNamespace) {
> -        rawNode.setAttributeNS(change.attributeNamespace, change.attributeName,
> +        rawNode.setAttributeSystemNS(change.attributeNamespace, change.attributeName,
>                                 change.newValue);
>        } else {
> -        rawNode.setAttribute(change.attributeName, change.newValue);
> +        rawNode.setAttributeSystem(change.attributeName, change.newValue);

Will this regress bug 1424474?  If not, why not?

::: devtools/server/actors/styles.js:1274
(Diff revision 2)
>        throw new Error("invalid call to setRuleText");
>      }
>  
>      if (this.type === ELEMENT_STYLE) {
>        // For element style rules, set the node's style attribute.
> -      this.rawNode.setAttribute("style", newText);
> +      this.rawNode.setAttributeSystem("style", newText);

Again, this is basically regressing part of bug 1424474.  That might be OK depending on what values come through here, but it's not obvious why it's OK.  It certainly needs documentation.
(In reply to Brad Werth [:bradwerth] from comment #12)
> Christoph, is this the only way to solve the problem? I ask because the idea
> of the triggering principal overriding the subject principal seems promising
> and is relatively easy to implement. The proposed patch attempts this, but
> the SystemPrincipal doesn't override the document Principal in
> BasePrincipal::OverridesCSP(). I confess that I don't understand Principals
> that well. Is there a reason why the SystemPrincipal shouldn't override the
> document Principal?

The changes within OverridesCSP() to also account for the systemPrincipal seem fine with me. If the triggeringPrincipal is the SystemPrincipal then it's fine to bypass CSP.
Flags: needinfo?(ckerschb)
Comment on attachment 8940897 [details]
Bug 1391994 Part 2: Use the new Element::setAttributeSystem and ::setAttributeSystemNS methods for rule changes made in the inspector.

https://reviewboard.mozilla.org/r/211160/#review217818

I think the idea behind this looks fine -- IIUC the series introduces a new chrome-only API that bypasses CSP, then changes the inspector actors to use it.  However, given that I can't see the bug mentioned by :bz in the review, I think I should probably not be reviewing this.  So, I'm blanking the review.  It's fine, IMO, to just rely on his review.  Thanks.
Attachment #8940897 - Flags: review?(ttromey)
Comment on attachment 8941650 [details]
Bug 1391994 Part 4: Expand devtools tests to verify interactive style edits work on pages with CSP restrictions.

https://reviewboard.mozilla.org/r/211888/#review217820

Thank you.
Attachment #8941650 - Flags: review?(ttromey) → review+
> However, given that I can't see the bug mentioned by :bz in the review

You can now.
Comment on attachment 8941649 [details]
Bug 1391994 Part 3: Make SystemPrincipals override all other principals in BasePrincipal::OverridesCSP.

https://reviewboard.mozilla.org/r/211886/#review217884

This change is OK with me, but I don't think it's really going to do what you want.

I agree with bz that this is going to regress bug 1424474, for a start. I think you could make an argument that that's the desirable behavior, but it's not ideal.

I think what you really want is an expanded principal that inherits from the page principal, with the page principal's CSP, but altered to allow inline styles. Otherwise, developers will be able to make changes via the style editor that won't work when they apply them to their actual page CSS.

Also, ideally, you should really be making those changes via the element `style` property. That would allow us to take into account which properties were set by the inspector, rather than applying the system principal to loads of URLs that were already present in the attribute.
Attachment #8941649 - Flags: review?(kmaglione+bmo) → review+
Thanks to all for reviews and comments. I've a lot to learn about proper application of CSP. I'll change strategy to the proposed solution in comment 25.
Product: Firefox → DevTools
status-firefox57: fix-optional → ---
See Also: → 1476439
See Also: → 1228985
Whiteboard: [dt-q]
I am working on an updated patch. I just now checked the Steps to Reproduce and I'm not able to replicate this anymore. Jeff, is this still occurring for you?
Flags: needinfo?(jmuizelaar)
It looks like crates.io is not setting a content security policy anymore. I can reproduce something similar on https://content-security-policy.com/
Flags: needinfo?(jmuizelaar)
Comment on attachment 8941649 [details]
Bug 1391994 Part 3: Make SystemPrincipals override all other principals in BasePrincipal::OverridesCSP.

This change was put in the tree as part of Bug 1432358.
Attachment #8941649 - Attachment is obsolete: true
Attachment #8941650 - Attachment is obsolete: true
Attachment #8940897 - Attachment is obsolete: true
Attachment #8940896 - Attachment is obsolete: true
DevtoolsPrincipal wraps another principal and passes through all
calls to the wrapped principal. The only difference i that it
returns true in BasePrincipal::IsDevtoolsPrincipal(). This can be
used when evaluating a principal for special security processing.
Element is expanded with chrome-only setAttribute methods that give devtools
callers an elevated principal with appropriate permissions for documents with
a Content-Security-Policy.

Depends on D8139
Ugh, Part 1 of this patch is not correct. I'm finding it very difficult to make a principal that can mimic another principal. The options I can see are:

1) Continue deriving the new class from BasePrincipal. This requires MANY more overrides to be defined, and that forces many methods of BasePrincipal to become virtual, and that forces changes in other principal classes (at least in their headers).

2) Similar to the above, but first make a new interface for nsIBasePrincipal. This would substantially change the BasePrincipal class header and change the calling semantics of the BasePrincipal::Cast method. But it would simplify defining a pass-though principal since it wouldn't have to inherit from BasePrincipal anymore.

3) Instead of generating a new principal, set and unset a flag on a BasePrincipal to indicate it is currently being used by devtools. This mutates the principal, but only temporarily.

Option 3 is by far the easiest to implement, so I'll attempt that next. Option 2 might be the cleanest -- I'll let reviewers decide.
FWIW, we are going to move the CSP from the Principal into the Client over in Bug 965637 which is already in review and should land soon. It's *not* going to fix the problem described within this bug, but will definitely interfere with the proposed changes within this bug.

I would like to find a more general solution to this problem, because we have had similar issues with editing style and CSP in the past. Unfortunately, I don't have a good solution on the tips of my fingers though, but hopefully things become a little easier once the CSP does not hang off the principal anymore.
Depends on: 965637
When we enable CSP for about pages (https://bugzilla.mozilla.org/show_bug.cgi?id=1492063) this will start affecting development of about: pages with DevTools. For DevTools team it means about:devtools, about:devtools-toolbox and about:debugging (especially the new one for the new remote debugging initiative) but I am sure other teams maintaining about:pages will be affected as well.
I was discussing with nchevobbe and nicofrand on IRC#devtools just now and this problem was mentioned again. In this case, it was particularly bad because it took them both a really long time to even figure out that CSP was blocking editing. So there was a lot of time lost.
Now, this bug is still on our "top bugs" list, and Brad has been making progress here, so I'm hopeful that we can resume the work and find a solution in the near future, but it will take a while longer.
And because of this, nchevobbe suggested an interesting option: warning users that CSS changes aren't possible when the right CSP is detected. If devtools could somehow detect that CSP is in effect on the page and is blocking css changes, then it could make the rule-view read-only and show a warning of some sort. At least to avoid people wasting time, while we look for the proper long-term solution.
See bug 1513169.
See Also: → 1513169

Taking myself off for now. I'll re-evaluate when Bug 965637 lands.

Assignee: bwerth → nobody

(In reply to Brad Werth [:bradwerth] (PTO until 7/15) from comment #41)

Taking myself off for now. I'll re-evaluate when Bug 965637 lands.

Small ping since Bug 965637 has now landed :)

Also CSP starts to be enabled on various about: pages (eg https://bugzilla.mozilla.org/show_bug.cgi?id=1497197), so development on about: pages will slightly degrade.

Flags: needinfo?(bwerth)
See Also: → 1570720

I'm going to make another attempt at this, based off the earlier patches.

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

It looks like the separation of CSP from the principal doesn't fundamentally change the required approach. nsStyleUtil::CSPAllowsInlineStyle still checks aTriggeringPrincipal, but now only for the purpose of using its CSP intead of the document CSP. So it still may be possible to reach that function with an ExpandedPrincipal with no CSP specified, which will allow the editing of inline style.

Attachment #9015675 - Attachment is obsolete: true
Attachment #9015676 - Attachment is obsolete: true
Attachment #9015679 - Attachment is obsolete: true
Attachment #9015681 - Attachment is obsolete: true
Attachment #9015683 - Attachment is obsolete: true

This expands Element with chrome-only setAttribute methods that give devtools
callers an ExtendedPrincipal without a nsIContentSecurityPolicy. This gives
devtools the ability to modify documents with a Content-Security-Policy.

Attachment #9084199 - Flags: feedback?(kmaglione+bmo)

(In reply to Kris Maglione [:kmag] from comment #25)

I think what you really want is an expanded principal that inherits from the
page principal, with the page principal's CSP, but altered to allow inline
styles. Otherwise, developers will be able to make changes via the style
editor that won't work when they apply them to their actual page CSS.

I think attachment 9084199 [details] nearly does what Kris was recommending some time ago. The difference is that the ExpandedPrincipal has NO CSP -- the patch doesn't attempt to clone and mutate the document CSP. I'm not sure if doing so would be safer/better. Kris, can you please provide feedback?

Attachment #9084199 - Flags: feedback?(bzbarsky)

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #20)

Again, this is basically regressing part of bug 1424474. That might be OK
depending on what values come through here, but it's not obvious why it's
OK. It certainly needs documentation.

Boris, I'm hoping the new approach in attachment 9084199 [details] addresses your concerns here. Instead of applying a SystemPrincipal to the edits being done by the new devtools-only methods, it's now using an ExpandedPrincipal with no CSP. My intention is only to provide a safe bypass to the check in nsStyleUtil::CSPAllowsInlineStyle at https://searchfox.org/mozilla-central/source/layout/style/nsStyleUtil.cpp#380 without making other actions unsafe or unchecked. Is this adequate?

So what is the exact intended behavior of the new methods?

As a concrete example, if the page has a CSP that prevents loads of images from hostname "foo" and devtools is used to set the src of an <img> to `http://foo/something", should the load happen? Using the new methods it would, right?

Also, it's not 100% clear to me whether the "use the principal's CSP if it's expanded" carveout is permanent. Christoph, do you recall what the general plan is there?

Flags: needinfo?(ckerschb)
Flags: needinfo?(bwerth)

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #53)

So what is the exact intended behavior of the new methods?

We want the ability to make any change to inline style and have it stick without a CSP violation. Whether or not the change might result in a load that is disallowed per the CSP should not be affected.

As a concrete example, if the page has a CSP that prevents loads of images from hostname "foo" and devtools is used to set the src of an <img> to `http://foo/something", should the load happen? Using the new methods it would, right?

I don't believe that it would have this effect, since the principal only exists for the duration of the SetAttribute call, and I don't see how the load code would have access to it. But it's worth checking and I'll attempt to do that.

Flags: needinfo?(bwerth)

I don't believe that it would have this effect, since the principal only exists for the duration of the SetAttribute call,

The attribute setter passes along the principal through the load code. That's what enables extensions to do loads in web pages that would normally be blocked by the page CSP, for example.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #55)

The attribute setter passes along the principal through the load code. That's what enables extensions to do loads in web pages that would normally be blocked by the page CSP, for example.

Since that's the case, then perhaps the fix is to create an ExtendedPrincipal that clones the document CSP and mutates the clone to add a policy that specifically allows inline style. That shouldn't allow anything else beyond the intended effect. I'll change the patches to do this, and attempt to make a test that confirms your example from comment 53 is appropriately disallowed.

We should start by writing the test and seeing whether it fails or passes with the patches as they are.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #53)

Also, it's not 100% clear to me whether the "use the principal's CSP if it's expanded" carveout is permanent. Christoph, do you recall what the general plan is there?

The carveout is not permanent and Jonathan will try to move the CSP off the ExpandedPrincipal within Bug 1548468 for the bigger effort of making Principals threadsafe (See Bug 1443925). At the moment we don't have a clear plan where the CSP should be stored after removing it from the ExpandedPrincipal though, if you have any suggestions please leave a comment here: https://bugzilla.mozilla.org/show_bug.cgi?id=1548468#c1. That reminds me that I should follow up on that.

Flags: needinfo?(ckerschb)

Overall, the problem described here is very similar to the problem described within Bug 1228985. I think we should try to find a more generic solution. Can we tag everything created by devtools somehow and then simply don't apply any CSP to it?

Can we tag everything created by devtools somehow and then simply don't apply any CSP to it?

That's more or less what this patch does, no? In the sense that we can "tag" by using an expanded principal as the principal involved...

Comment on attachment 9084199 [details]
Bug 1391994 Part 1: Add new methods to Element to set attributes via devtools.

So I think this is fine if we're OK with bypassing CSP for all callsites that use the new APIs, but not bypassing CheckLoadURI (see bug 1424474 for why we would not want to do that).

The documentation for the new methods should clearly say that it will bypass all CSP directives, not just the ones that have to do with "inline" content.

Attachment #9084199 - Flags: feedback?(bzbarsky) → feedback+

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #61)

So I think this is fine if we're OK with bypassing CSP for all callsites that use the new APIs, but not bypassing CheckLoadURI (see bug 1424474 for why we would not want to do that).

I've uploaded an updated set of patches that explicitly clone and mutate the existing document CSP to add "style-src 'unsafe-inline'", which I hope will prevent any unwanted bypassing of the CSP. I have not added a test that verifies a CheckLoadURI violation, but I did manually check the Steps to Reproduce on Bug 1424474 to confirm that the inline source change is still forbidden with these patches.

Attachment #9084199 - Flags: feedback?(kmaglione+bmo)

which I hope will prevent any unwanted bypassing of the CSP

I would really like us to think through which CSP bypasses are or are not wanted here, please. If we just want to allow inline styles and nothing else, that's OK, but I'd like it to be an informed decision, ideally, so we don't have people coming by later and saying "I changed the image url in devtools but it did not load; why not?" My personal take on it is that devtools-triggered changes should probably NOT be subject to page CSP, just like extensions that modify the page are not, but won't claim to know what devtools users expect to happen.

I'd probably agree with Boris: if the developer tools don't do what developers expect to happen, they might come to perceive the tools as only half useful, aka 'unreliable' from their point of view. I think we should assume that developers know what they're doing; they should be responsible themselves if they mess up, shouldn't need hand-holding?

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #64)

I would really like us to think through which CSP bypasses are or are not wanted here, please. If we just want to allow inline styles and nothing else, that's OK, but I'd like it to be an informed decision, ideally, so we don't have people coming by later and saying "I changed the image url in devtools but it did not load; why not?" My personal take on it is that devtools-triggered changes should probably NOT be subject to page CSP, just like extensions that modify the page are not, but won't claim to know what devtools users expect to happen.

Sure. The current version of the patch makes the additional privileges explicit and I hope easy to add to, but you're right that we should also have a clear goal in mind.

Harald, would you please respond with your vision on how permissive we want Devtools to be on pages with a CSP? For example, do we want the user to be able to make changes interactively that would not be allowed if the changes to the page were permanent (presuming the CSP does not also change)?

For reference, here's the devtools csp meta bug: Bug 1479015

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #64)

I would really like us to think through which CSP bypasses are or are not wanted here, please.

I'm attempting to address this point in this document: https://docs.google.com/document/d/1eP3sDsn3hG1lSRwztIagXPG5neITr0uDcSVvVaYAYYs/edit#
Please take a look and let me know of any questions.

Flags: needinfo?(bzbarsky)

Sorry for the lag here....

I understand that we want to bypass style-src for things devtools does. The question is what other things devtools wants to bypass and in which cases.

Flags: needinfo?(bzbarsky)

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #68)

The question is what other things devtools wants to bypass and in which cases.

Nothing else in DevTools injects things in the page in a way that gets blocked by CSP. Bypassing CSP for editing element styles and adding stylesheets is what we need first and foremost.
So I was going to reply that we do not want to bypass anything else.

I just stumbled upon bug 1439390 however. This one is about the "edit & resend" feature of the network panel being blocked (by connect-src I think). So this would be the only other use case I am aware of.

Nothing else in DevTools injects things in the page in a way that gets blocked by CSP.

OK. So if the user uses devtools to edit, say, the src of an image, does that not get blocked by CSP? Or does it get blocked but we want it to get blocked?

What about editing non-inline url-valued CSS properties?

What about executing manual fetch() or XHR via the console?

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #70)

OK. So if the user uses devtools to edit, say, the src of an image, does that not get blocked by CSP? Or does it get blocked but we want it to get blocked?

It does get blocked, and we do want it to get blocked. This is also what Chrome does, and I don't think is a compelling enough use case to warrant lowering security for devtools.

What about editing non-inline url-valued CSS properties?

Same thing here, if the editing leads to loading external resources that would normally be blocked, then those should remain blocked.

What about executing manual fetch() or XHR via the console?

And same here, if CSP policies on the current page block this, then they should remain blocked in the console too.
The idea again is that we do not want to force unnecessary changes on the CSP engine that would only allow a few minor use cases in devtools. The major ones are about editing CSS in a simple way (as described in the doc in comment 67).

See Also: → 1580514
No longer blocks: top-inspector-bugs

I believe I have updated the patches to match all reviewer feedback.

Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f225d8e16072
Part 1: Add new methods to Element to set attributes via devtools. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/fa4bb19a3d19
Part 2: Use the new SetAttributeDevtools methods in callsites where devtools modifies a node it hasn't just created. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/70d2c0d4a3e8
Part 3: Expand devtools tests to verify interactive style edits work on pages with CSP restrictions. r=bzbarsky
Regressions: 1604562
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: