Closed Bug 1396798 Opened 7 years ago Closed 7 years ago

Don't block top level data: URIs when loading an image

Categories

(Core :: DOM: Security, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(2 files)

Currently, all toplevel data: URI navigations are blocked when the pref |security.data_uri.block_toplevel_data_uri_navigations| is set to true.

I think we can exclude data images from that restriction.
Assignee: nobody → ckerschb
Blocks: 1380959
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
Comment on attachment 8904991 [details] [diff] [review]
bug_1396798_do_not_block_toplevel_data_img.patch

Oddly simple.
Attachment #8904991 - Flags: review?(bugs) → review+
Comment on attachment 8904992 [details] [diff] [review]
bug_1396798_test_toplevel_data_img.patch

The svg part here is wrong. SVG is loaded as a document, so there wouldn't be any img elements.
I guess you could check for example that 
wrappedWin2.document.documentElement.localName != "svg"

With that, r+
Attachment #8904992 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #4)
> Comment on attachment 8904992 [details] [diff] [review]
> bug_1396798_test_toplevel_data_img.patch
> 
> The svg part here is wrong. SVG is loaded as a document, so there wouldn't
> be any img elements.
> I guess you could check for example that 
> wrappedWin2.document.documentElement.localName != "svg"
> 
> With that, r+

Thanks, I just checked that we spit out the warning and don't load the svg image. I'll fix that.
Comment on attachment 8904991 [details] [diff] [review]
bug_1396798_do_not_block_toplevel_data_img.patch

Given how large image data: URIs can be, it might be nice to not do a copy of the entire URI here if we can avoid it.  Since we know the scheme is "data" already, could we GetFilePath instead of GetSpec?  That will just bump the stringbuffer refcount on mPath instead of copying...
Also, what is the "spec" used for right now?  If this is the code added in bug 1394554, then it looks like it's dead code, just doing a copy of a large string for no reason.
Flags: needinfo?(ckerschb)
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #7)
> Also, what is the "spec" used for right now?  If this is the code added in
> bug 1394554, then it looks like it's dead code, just doing a copy of a large
> string for no reason.

Thanks Boris - Good catch - I'll get both things fixed.
Flags: needinfo?(ckerschb)
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff412c116b9b
Do not block toplevel data: navigation to image (except svgs). r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/79bf8a92a0ea
Test toplevel data: URI navigation to images. r=smaug
https://hg.mozilla.org/mozilla-central/rev/ff412c116b9b
https://hg.mozilla.org/mozilla-central/rev/79bf8a92a0ea
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1418642
You need to log in before you can comment on or make changes to this bug.