Closed Bug 1403641 Opened 2 years ago Closed 2 years ago

Download behaviour for data: URL seems different in FF57 compared to 55

Categories

(Core :: DOM: Security, defect, P1)

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 - disabled
firefox58 + fixed

People

(Reporter: jonny, Assigned: ckerschb)

References

Details

(Keywords: regression, site-compat, Whiteboard: [domsecurity-active])

Attachments

(3 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170925150345

Steps to reproduce:

Using Firefox 57.0b3 (64-bit) on mac OS 10.12.6

Visit http://danml.com/download.html#TestUsages
Click the "text dataURL" button

(also visible in the download button on https://makecode.microbit.org)


Actual results:

Firefox 57: ..nothing
Firefox 55: Save file dialog box appears


Expected results:

A save file dialog should appear (or the file should be downloaded, if the user has previously configured that to happen automatically)
Hello I have also observed this - on 55 it is okay > it actually comes to simple testcase like this does not work.
This seems to be quite big regression, which needs fix. A lot of website is using this.

data:text/html;charset=UTF-8,<html><body><a download="text.src" href="data:application/octet-stream;base64,dGV4dCBwcm8gdWxvemVuaQ==">download</a></body></html>
Just to note in my case it does not work on Linux Ubuntu FF developer edition 57 and also on windows 10 and FF developer edition. I first thought that our information system got broken, but then I found out, that this is suddenly broken.
So I have tried to do bisection and it looks like this

2017-09-27T20:20:11: INFO : Narrowed inbound regression window from [01992997, fa1052b9] (3 revisions) to [01992997, 9072e9c0] (2 revisions) (~1 steps left)
2017-09-27T20:20:11: DEBUG : Starting merge handling...
2017-09-27T20:20:11: DEBUG : Using url: https://hg.mozilla.org/integration/mozilla-inbound/json-pushes?changeset=9072e9c0d0aa2bd790634bb0a5c49713b6e27c79&full=1
2017-09-27T20:20:12: DEBUG : Found commit message:
Bug 1399170: Test mozbrowser can not load data URI in iframe. r=smaug

2017-09-27T20:20:12: INFO : The bisection is done.
2017-09-27T20:20:12: INFO : Stopped

Not sure if this is somehow related. Also attaching image of bisection.
[Tracking Requested - why for this release]: Serious web compat regression.

Yeah, this needs fixing.  In Chrome the testcases here work, because they make <a download> go down a totally different codepath from normal loads and apparently do the data: checks _after_ that branchpoint.

The good news is that this is not enabled in late betas, so we're not going to ship this to users until the switch gets flipped in bug 1401895.  Jonny, thank you for noticing the problem and filing!  zdnex, thank you for figuring out what the issue is.
Blocks: 1380959, 1401895
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM: Security
Ever confirmed: true
Flags: needinfo?(ckerschb)
Keywords: regression
Product: Firefox → Core
Keywords: site-compat
Maybe I do not really understand, but FF Developer Edition has this active so in our case it has already affected some people in our office.
Another issue is for example using data: uri for json > which is sometimes used by REST API services to quickly show JSON result and when we have inbuilt JSON viewer it is even better, but this breaks it also.

<html><body><a href='data:application/json,{"my_json_key":"value"}' target='_blank'>show json value</a></body></html>
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #5)
> [Tracking Requested - why for this release]: Serious web compat regression.
> 
> Yeah, this needs fixing.  In Chrome the testcases here work, because they
> make <a download> go down a totally different codepath from normal loads and
> apparently do the data: checks _after_ that branchpoint.

I tested downloading data: URIs but I didn't try <a download>. I'll get that fixed.

I don't think we end up shipping that change in behavior of blocking toplevel data: URI navigations (Bug 1401895) for FF57. At this point I'll rather fix it for FF58 and let it ride the trains. So I am not sure if we need tracking flags for FF57 here.


(In reply to zdnex from comment #7)
> Another issue is for example using data: uri for json > which is sometimes
> used by REST API services to quickly show JSON result and when we have
> inbuilt JSON viewer it is even better, but this breaks it also.
> 
> <html><body><a href='data:application/json,{"my_json_key":"value"}'
> target='_blank'>show json value</a></body></html>

I filed Bug 1403870 to investigate, but I guess we can allow that as well.
Flags: needinfo?(ckerschb)
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [domsecurity-active]
Duplicate of this bug: 1404553
So is there plan to temporary disable option for blocking navigation on data: or this will be solved differently? As we will need to make changes to our apps otherwise - because then it will not work.
(In reply to zdnex from comment #11)
> So is there plan to temporary disable option for blocking navigation on
> data: or this will be solved differently? As we will need to make changes to
> our apps otherwise - because then it will not work.

Please note that this behavior lives behind a pref at the moment which is not flipped for FF57. In other words this behavior of blocking top level data: URI navigations is only active in Nightly and Early Beta, but not release since we are still shaking out bugs like this one. We will definitely fix this Bug before flipping the pref within Bug 1401895.
Hey Boris, I am not sure if there is a better way to distinguish downloads other than checking for the disposition. If there is please let me know, otherwise I think this is what we want.

If you are fine with it, I'll add an automated test.
Attachment #8914454 - Flags: review?(bzbarsky)
Comment on attachment 8914454 [details] [diff] [review]
bug_1403641_data_url_download.patch

I don't see how you can have an "attachment" disposition on aNewChannel in AsyncOnChannelRedirect.  That's a response header, and you don't have those at that point yet.

What policy are you trying to implement, exactly?
Attachment #8914454 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #15)
> I don't see how you can have an "attachment" disposition on aNewChannel in
> AsyncOnChannelRedirect.  That's a response header, and you don't have those
> at that point yet.

My thinking was the following: Within docshell we know it's a download if |!aFileName.IsVoid()|. Later within docshell we call |channel->SetContentDisposition(nsIChannel::DISPOSITION_ATTACHMENT);| in exactly the same case [1]. Hence I was assuming those flags are persistent and copied from the old to the new channel. Maybe I am wrong. If I am wrong, what would be a better way to know whether are are facing a download?
 
> What policy are you trying to implement, exactly?
In the regular dochsell case we determine whether we are hitting a download by inspecting |!aFileName.IsVoid()|. In the redirect case (http(s):// hits a 30X to a data: URI) we should do the same thing, right?

[1] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#11358
Flags: needinfo?(bzbarsky)
> Hence I was assuming those flags are persistent and copied from the old to the new channel.

They're not.  Note discussion at https://github.com/whatwg/html/issues/2622#issuecomment-301061744 and following.

> If I am wrong, what would be a better way to know whether are are facing a download?

If you get a 3xx to a data: URI, it will be a download in Firefox right now only if the type is something we don't handle internally.  This is completely independent of <a download> bits.

> In the regular dochsell case we determine whether we are hitting a download by inspecting |!aFileName.IsVoid()|.

Where "hitting a download" means <a download href="data:....">, yes.

> In the redirect case (http(s):// hits a 30X to a data: URI) we should do the same thing, right?

What same thing?  There is no <a download href="data:...."> in that case.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #17)
> What same thing?  There is no <a download href="data:...."> in that case.

Ok. So I think I was wrong. We can catch the regular case (<a download href="data:...") by inspecting |!aFileName.IsVoid()| and then explicilty allow that data: URI download.

For the redirect case: I don't think we have to worry about the case when a download starts using http(s)://foo and then gets redirected to a data: URI. As far as I know fetch already blocks that kind of cross origin redirect. Ultimately we can simply pass 'false' to AllowTopLevelNavigationToDataURI in the AsyncOnChannelRedirect case. AGreed?
Attachment #8914454 - Attachment is obsolete: true
Attachment #8914843 - Flags: review?(bzbarsky)
Comment on attachment 8914843 [details] [diff] [review]
bug_1403641_data_url_download.patch

r=me
Attachment #8914843 - Flags: review?(bzbarsky) → review+
Comment on attachment 8914844 [details] [diff] [review]
bug_1403641_data_url_download_test.patch

r=me
Attachment #8914844 - Flags: review?(bzbarsky) → review+
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcdbe0c04f23
Allow data: URI downloads even if data: URI navigations are blocked. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/8146509b7d80
Test data: URI download. r=bz
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.