Christoph Kerschbaumer [:ckerschb] Assignee
	Description • 7 years ago

Within Bug 1380959 we flipped that behavior on for Nightly and Early Beta. Within this bug we should switch it on for real.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 2 • 7 years ago

Boris, now that we only block data URIs that are opened in the browser (see Bug 1403814) and also got Bug 1407891 (view-image problem) out of the way, can we ship this behavioral change of blocking top-level data URI navigations?

Please note that we already enabled this change in Nightly and Early Beta within the 57 cycle (see Bug 1380959).

What do you think?

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 7 years ago

Comment on attachment 8926437 [details] [diff] [review]
bug_1401895_block_toplevel_data_uris.patch

The comment on this pref needs to be updated to match reality.

It looks like we block data:text/plain in toplevel.  Why?  In general, I would think we would want to whitelist everything in nsContentUtils::IsPlainTextType, not just application/json.  It's particularly weird to whitelist application/json but not text/json...

r=me if we whitelist all the plaintext types, I think.  If we don't want to do that, I'd like to understand why.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 4 • 7 years ago

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #3)
> Comment on attachment 8926437 [details] [diff] [review]
> bug_1401895_block_toplevel_data_uris.patch
> 
> The comment on this pref needs to be updated to match reality.
> 
> It looks like we block data:text/plain in toplevel.  Why?  In general, I
> would think we would want to whitelist everything in
> nsContentUtils::IsPlainTextType, not just application/json.  It's
> particularly weird to whitelist application/json but not text/json...

I guess that makes sense. Thanks for the hint. I filed Bug 1415612 to get that done and to not pollute this bug with those changes.

	Pulsebot
	Comment 5 • 7 years ago

Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/479f3105ad3b
Block top-level navigations to data: URIs. r=bz

	Dorel Luca [:dluca]
	Comment 6 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/479f3105ad3b

	Kohei Yoshino [:kohei]
	Comment 7 • 7 years ago

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/data-url-navigations-on-top-level-window-will-now-be-blocked/

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 8 • 7 years ago

(In reply to Kohei Yoshino [:kohei] from comment #7)
> Posted the site compatibility note:
> https://www.fxsitecompat.com/en-CA/docs/2017/data-url-navigations-on-top-
> level-window-will-now-be-blocked/

Thanks!

	Chris Mills [:cmills]
	Comment 9 • 6 years ago

I've documented this by adding a note to our browser compat data (see https://github.com/mdn/browser-compat-data/pull/857), and a note to the Fx59 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/59#Security

Let me know if that is OK. Thanks!

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 10 • 6 years ago

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #9)
> I've documented this by adding a note to our browser compat data (see
> https://github.com/mdn/browser-compat-data/pull/857), and a note to the Fx59
> rel notes:
> 
> https://developer.mozilla.org/en-US/Firefox/Releases/59#Security
> 
> Let me know if that is OK. Thanks!

That looks good, thanks. Can you also link to the blogpost, which in my opinion is more comprehensive:
https://blog.mozilla.org/security/2017/11/27/blocking-top-level-navigations-data-urls-firefox-58/

Thanks!

	Chris Mills [:cmills]
	Comment 11 • 6 years ago

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #10)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #9)
> > I've documented this by adding a note to our browser compat data (see
> > https://github.com/mdn/browser-compat-data/pull/857), and a note to the Fx59
> > rel notes:
> > 
> > https://developer.mozilla.org/en-US/Firefox/Releases/59#Security
> > 
> > Let me know if that is OK. Thanks!
> 
> That looks good, thanks. Can you also link to the blogpost, which in my
> opinion is more comprehensive:
> https://blog.mozilla.org/security/2017/11/27/blocking-top-level-navigations-
> data-urls-firefox-58/
> 
> Thanks!

OK, I've updated the link.

	Martin S.
	Comment 12 • 6 years ago

I've been using top-level document data URI as a workaround for the https://bugzilla.mozilla.org/show_bug.cgi?id=1344465 bug (POST does not work in webextensions). Disabling the top-level data URI kills this workaround.

	:Gijs (he/him)
	Comment 13 • 6 years ago

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #10)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #9)
> > I've documented this by adding a note to our browser compat data (see
> > https://github.com/mdn/browser-compat-data/pull/857), and a note to the Fx59
> > rel notes:
> > 
> > https://developer.mozilla.org/en-US/Firefox/Releases/59#Security
> > 
> > Let me know if that is OK. Thanks!
> 
> That looks good, thanks. Can you also link to the blogpost, which in my
> opinion is more comprehensive:
> https://blog.mozilla.org/security/2017/11/27/blocking-top-level-navigations-
> data-urls-firefox-58/
> 
> Thanks!

This blogpost suggests we did this in 58. Can we update it to say 59 instead? I was very confused when a reporter pointed out that toplevel data: URI navigation still worked in bug 1441819.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 14 • 6 years ago

(In reply to Martin S. from comment #12)
> This blogpost suggests we did this in 58. Can we update it to say 59
> instead? I was very confused when a reporter pointed out that toplevel data:
> URI navigation still worked in bug 1441819.

Heh, interesting that no one of my reviewers caught that. Thanks for pointing it out - I already updated it [1].

[1] https://blog.mozilla.org/security/2017/11/27/blocking-top-level-navigations-data-urls-firefox-59/