Closed Bug 1579663 Opened 7 months ago Closed 4 months ago

Allow data url background-image in styled console logs

Categories

(DevTools :: Console, enhancement, P3)

71 Branch
enhancement

Tracking

(firefox73 fixed)

RESOLVED FIXED
Firefox 73
Tracking Status
firefox73 --- fixed

People

(Reporter: pomax, Assigned: billin22)

Details

Attachments

(2 files)

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.

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!

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.

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

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

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

Assignee: nobody → pomax
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Summary: background-image: url(...) in styled console logs appears to generate a blank <img> → Allow data url background-image in styled console logs

Hello pomax,

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

Type: defect → enhancement
Priority: -- → P3

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

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?

(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

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.

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?

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.

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

(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

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.

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

Flags: needinfo?(pomax)
Assignee: pomax → nobody
Status: ASSIGNED → NEW

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?

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!

Assignee: nobody → billin22
Status: NEW → ASSIGNED
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eb47fe3b8446
Allow data url background-image in styled console logs r=nchevobbe
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 73
You need to log in before you can comment on or make changes to this bug.