1579663 - Allow data url background-image in styled console logs

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P3)

Description

[Pomax]

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

Steps to reproduce:

I ran the following JS in the dev console:

const cvs = document.createElement('canvas');
let d = cvs.width = cvs.height = 200;
let ctx = cvs.getContext('2d');
ctx.fillStyle = "red";
ctx.fillRect(0,0,d,d);
let url = cvs.toDataURL();
let style = display:inline-block; padding: ${d/2}px ${d/2}px; background: blue; background-image: url(${url}); font-size: 0; line-height: 0;;
console.log("%clog", style);

(For context, I ran across this while trying to implement a proof-of-concept console.plot2d() function, for quickly visualising graphable data directly to the console to aid in-browser debugging of mathy/sciency page content)

Actual results:

In Chrome, this yields the correct red rectangle due to the background-image: url(...) style, but in Firefox there is a blue rectangle, suggesting background-image either didn't kick in, or it did not actually set the image source to the specified url.

Expected results:

Firefox should override the initial "background: blue" with the image background specified via the "background-image: url(...)" rule instead.

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Hello pomax,

I think you are hitting Bug 1134512.
We are not allowing any property value containing url : devtools/client/webconsole/components/Output/GripMessageBody.js#101.

The comments on Bug 1134512 seem to indicate we should be able to open up to dataURL.

Do you think you could work on this to unblock your cool proof of concept? You can go through http://docs.firefox-dev.tools/getting-started/ to check how to contribute. Then it should be only about modifying the regex I linked to in order to display data url. Let me know what you think!

Comment 2

[Pomax]

heh, branch 37. But yes, I think you're right, that sounds like the same thing. I have never worked on the FF code before, and my expertise is firmly rooted in JS rather than C++, so I can give it a shot, but it's entirely possible that someone with C++ and FF chops can fix this in an hour rather than me stumbling for a week.

Comment 3

[Pomax]

Having said that, though, I'm not sure it makes a ton of sense to block url(), given that any request is still subject to CORS? Given that one can already use the dev console to trivially build an image element and then inject it into the document, or set a background image on any page element, I'm not sure I understand why url in console log would be any less secure. (Certainly, mailto:, ftp:, file: etc. make no sense and are very sensible to block, but http:, https: and data: should be perfectly fine).

Comment 4

[Pomax]

I just noticed the code you pointed to is entirely JS, so... yeah I'll see what I can do =)

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

(In reply to pomax from comment #4)

I just noticed the code you pointed to is entirely JS, so... yeah I'll see what I can do =)

great!

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Hello pomax,

How is this going? Can I help you in any way?

Comment 7

[Pomax]

I've been having some medical-related issues, I'm hoping to actually try this today.

Comment 8

[Pomax]

In terms of fixing what makes sense: allowing url() and relying on the value regex to then forbid the various questionable protocols feels like the right approach, but it also feels sensible to allow https as url protocol, as requests are still subject to CORS etc. Does that sound reasonable?

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

(In reply to pomax from comment #8)

In terms of fixing what makes sense: allowing url() and relying on the value regex to then forbid the various questionable protocols feels like the right approach, but it also feels sensible to allow https as url protocol, as requests are still subject to CORS etc. Does that sound reasonable?

I think we should only allow url if it's for a data url

Comment 10

[Pomax]

I suppose that keeps it scoped to the use-case this was for, although it seems weird to disallow http/https when CORS and CSP are respected. Then again, that's a different issue, I'll file that after this lands so that discussion can be its own thing.

Comment 11

[:Harald Kirschner :digitarald]

I think we should only allow url if it's for a data url

If I understand bug 1566072, previewing images in DevTools is not inherently insecure. Requests should be made without user credentials of course. Do we have prior art here?

Comment 12

[Pomax]

Attachment: Bug 1579663 - allow url(data:...) in style console logs. r=nchevobbe

Before this change, background-image: url(data:...) would be be rejected in styled
console.log due to url() being a disallowed property value, and the data: protocol
being a disallowed content protocol. This change removes those two restrictions.

Comment 13

[Pomax]

I guess I ran through the whole patch submission process if the patch shows up in this bug?

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

(In reply to pomax from comment #13)

I guess I ran through the whole patch submission process if the patch shows up in this bug?

yes! Thanks a lot, I'm going to review the patch right now

Comment 15

[Patrick Brosset <:pbro>]

Hi pomax, just checking in to see if you're still planning on working on this fix. If you just need more time, that's fine. Just let us know.

Comment 16

[Pomax]

If someone else has more time, I would very much request they please pull this over the finish line. I have so many projects that need my attention that every time I remember this isn't quite done yet, someone else pulls me away.

Comment 17

[Edward Billington]

I'd like to help get this resolved. Already have spoken to Nicolas - is there any more information I will need other than the current information already here and on Phabricator?

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

Hello again Edward :)

I assigned you the bug, so you can start working on it.
I suggest you to start from scratch, taking inspiration on what was done in https://phabricator.services.mozilla.com/D46315 .
Then try to handle the comments I added in https://phabricator.services.mozilla.com/D46315#1431258 and push your commit to Phabricator so I can review it :)

Thanks!

Comment 19

[Edward Billington]

Attachment: Bug 1579663 - Allow data url background-image in styled console logs

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eb47fe3b8446
Allow data url background-image in styled console logs r=nchevobbe

Comment 21

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/eb47fe3b8446