Closed Bug 1136055 Opened 9 years ago Closed 9 years ago

"Save link as" does not follow HTTP 302 response / redirection

Categories

(Firefox :: General, defect)

38 Branch
x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 41
Tracking Status
firefox38 --- wontfix
firefox39 + verified
firefox38.0.5 --- wontfix
firefox40 + verified
firefox41 + verified
firefox-esr38 39+ affected
relnote-firefox --- 39+

People

(Reporter: flore, Assigned: ckerschb)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

Since a few nightlies in 38 branch (and still in nightly 39), I noticed a change in "save link as" behavior.

On this particular website: https://interfacelift.com/wallpaper/downloads/date/any/ , I usually just right-click on the download button, select "Save link as" and the picture was then downloaded on my computer. Since a few nightlies (I don't know the date exactly), if I do that, I only get a 0 byte file. But it works correctly on version 35.0.1 and I get the picture, correct size.

All download links on this site point to the same directory (example: https://interfacelift.com/wallpaper/7yz4ma1/03850_stormyroad_1440x900.jpg), but then, it is rewritten to replace "7yz4ma1" by another string of characters that change often (I can imagine that it's to avoid automated download scripts).
On previous versions of Firefox (and still in 35.0.1 release), the "save link as" got the change in the URL (can be checked by copying the download link in the download window)

BuildID (nightly showing with bug): 20150223103044
Summary: "Save link as" does not completely follow link if it's changed dynamically → "Save link as" does not follow HTTP 302 response / redirection
I just want to precise that the change in URL is done by a 302 redirection
I'm seeing this too (and have been for a while).  I setup https://jsfiddle.net/bdukes/mgw1mqyu/1/show/ with a couple of test links (just based on a couple of sites where I ran into the issue).  This also shows it occurring for 301 redirects.

On that test page, I incorrectly get the 0 byte response in FF 40.0a2 (Dev Edition), as well FF 38, but I correctly get the full file on FF 37.
Me too..noticed the change after upgrade fro 37 to 38.0.1.

This is a big pain, I use this feature to download media file from their page links.
OK, this is becoming a serious issue.
Apparently, the problem is not only with "save link as" but also following 302 redirections!

I was filing my income tax returns (in France). First there was a 302 redirection that was not followed. I got around that by just copy pasting the URL. When I was finished, I just click on the link to download the pdf... And guess what it's a 302 redirection and no way to download it because of this bug!

Seriously, this is becoming worse with every new version
[Tracking Requested - why for this release]: regression since Firefox38

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0a91847e50e6&tochange=a62c7b38ad6b

Suspect:  Bug 1087726
Blocks: 1087726
Flags: needinfo?(mozilla)
Version: 39 Branch → 38 Branch
(In reply to Brian Dukes from comment #2)
> I'm seeing this too (and have been for a while).  I setup
> https://jsfiddle.net/bdukes/mgw1mqyu/1/show/ with a couple of test links
> (just based on a couple of sites where I ran into the issue).  This also
> shows it occurring for 301 redirects.

Thanks Brian for setting up that testpage, made it real easy to figure out what's going on here. Let me explain:

(1) In Bug 418354 Firefox started to block MixedContent that follows any kind of redirection.
(2) Within Bug 1006868 we started to revamp the way content security checks are performed within Gecko. As a first step we attach a loadinfo to every channel at channel creation time which is also propagated if a channel goes through a redirect. A serious of patches (see Bug 1087720) makes sure that we attach such a loadinfo to every channel. One of them is indeed Bug 1087726.

What does that mean?
If you click on any of the links provided on the testpage in Comment 2, you see the following message in the console:
> Blocked loading mixed active content "http://fdlyr.co/d/changelog/cdn.5by5.tv/audio/broadcasts/changelog/2015/changelog-155.mp3"

Which is correct, because the main page is https and the redirect ends up at
> http://cdn.5by5.tv/audio/broadcasts/changelog/2015/changelog-155.mp3

Now why that message does not show up in the console if you 'right click - save link as ...' I can't tell - maybe Tanvi knows.
Flags: needinfo?(mozilla) → needinfo?(tanvi)
We could still probably take a fix for this in 39. We don't have a lot of time left in the beta cycle though. 

Lawrence can you suggest anyone who might have a look at this?
Flags: needinfo?(lmandel)
I suspect that I've also been hit by this but didn't realize what had happened until reading this bug.

Dave/Justin - Can either of you recommend an owner? We'll need one in the next day or so if there's any chance of fixing this in 39.
Flags: needinfo?(lmandel)
Flags: needinfo?(dolske)
Flags: needinfo?(dcamp)
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #8)
> I suspect that I've also been hit by this but didn't realize what had
> happened until reading this bug.
> 
> Dave/Justin - Can either of you recommend an owner? We'll need one in the
> next day or so if there's any chance of fixing this in 39.

Lawrence, as explained in Comment 6, this bug is more of a web compatibility issue. Firefox (in particular the mixed content blocker) is blocking that redirected request correctly. The only thing missing is a web console error message if you do a 'right click - save as ...'. If you just click the link, the mixed content error message is correctly displayed in the console. I don't know how important we consider a missing console message. Just wanted to clarify once again, that nothing is broken with the redirect handling itself.
Flags: needinfo?(lmandel)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #9)
> (In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #8)
> > I suspect that I've also been hit by this but didn't realize what had
> > happened until reading this bug.
> > 
> > Dave/Justin - Can either of you recommend an owner? We'll need one in the
> > next day or so if there's any chance of fixing this in 39.
> 
> Lawrence, as explained in Comment 6, this bug is more of a web compatibility
> issue. Firefox (in particular the mixed content blocker) is blocking that
> redirected request correctly. The only thing missing is a web console error
> message if you do a 'right click - save as ...'. If you just click the link,
> the mixed content error message is correctly displayed in the console. I
> don't know how important we consider a missing console message. Just wanted
> to clarify once again, that nothing is broken with the redirect handling
> itself.

Ehm, but if I turn off mixed content blocking on the page in comment #0, that doesn't fix the issue. Why not?
Flags: needinfo?(mozilla)
(it also seems that for a "save as" download, it isn't really mixed content - it should be treated the same way as opening https://www.example.com/ from the URL bar, when the main document at that location redirects to http://www.example.com/ and the referrer was whatever linked to https://www.example.com/ (potentially getting lost through the redirect depending on the referrer policy)
(In reply to :Gijs Kruitbosch from comment #10)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #9)
> > (In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #8)
> > > I suspect that I've also been hit by this but didn't realize what had
> > > happened until reading this bug.
> > > 
> > > Dave/Justin - Can either of you recommend an owner? We'll need one in the
> > > next day or so if there's any chance of fixing this in 39.
> > 
> > Lawrence, as explained in Comment 6, this bug is more of a web compatibility
> > issue. Firefox (in particular the mixed content blocker) is blocking that
> > redirected request correctly. The only thing missing is a web console error
> > message if you do a 'right click - save as ...'. If you just click the link,
> > the mixed content error message is correctly displayed in the console. I
> > don't know how important we consider a missing console message. Just wanted
> > to clarify once again, that nothing is broken with the redirect handling
> > itself.
> 
> Ehm, but if I turn off mixed content blocking on the page in comment #0,
> that doesn't fix the issue. Why not?

You are absolutely correct. Thanks for pointing that out, I just disabled mixed content and get the same error. So, after the channel gets redirected mixed content receives a call to AsyncOnChannelRedirect [1], which then calls ::ShouldLoad, which then returns an error because it cannot query a docshell [2]. That error than causes the channel to be aborted.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#210
[2] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#589
Flags: needinfo?(mozilla)
So, Christoph, do you have time to investigate if we can change how we pass parameters for "save link as" so that we don't break this case? I'm a little bit swamped myself and the networking internals aren't really my expertise, but I can promise fast reviews if you have suggestions for alternative parameters we can use for the browser callsite here (even if it just gets us back to status-quo for 39).
(In reply to :Gijs Kruitbosch from comment #13)
> So, Christoph, do you have time to investigate if we can change how we pass
> parameters for "save link as" so that we don't break this case?

Working on it...

I'm a little
> bit swamped myself and the networking internals aren't really my expertise,
> but I can promise fast reviews if you have suggestions for alternative
> parameters we can use for the browser callsite here (even if it just gets us
> back to status-quo for 39).

It seems we are spinning in circles.
The initial patch [1] set a |doc| within contextmenu.js where we do the 'save link as ... ' [2], but that caused problems within e10s, so we removed it again [3].

Now we have to find a solution that works in e10s as well as in regular mode.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1087726
[2] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#1304
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=1127927
Copying comment over from
https://bugzilla.mozilla.org/show_bug.cgi?id=1127927#c12

> Unfortunately, when I rebased this patch, the "Save link" functionality
> stopped working again. It was broken by changes that landed in bug 1087726.
> Christoph, could you please look over the changes I've made to
> newChannelFromURI2? In e10s, |doc| is actually a cross-process wrapper
> around a document in the content process. It's not safe to pass it into a
> chrome C++ method like this. So instead I passed the document principal. Is
> there any reason why you really need the node instead of just the principal?

Apparently there is a case now. Bill any suggestions?
Flags: needinfo?(wmccloskey)
Blocks: 1127927
Per comment #11, can we not treat this as a toplevel load, however that would work in terms of actual parameters? I am not sure why for "save link as" we really need the reference of the original document/node in the first place, except maybe the referrer policy, which it seems to me we should be able to extract...

(clearing some of the needinfos)
Flags: needinfo?(lmandel)
Flags: needinfo?(dolske)
I am taking this bug and clearing needinfos...
Assignee: nobody → mozilla
Flags: needinfo?(wmccloskey)
Flags: needinfo?(tanvi)
Attached patch bug_1136055_save_link_as.patch (obsolete) — Splinter Review
As discussed on IRC we should treat 'save link as ...' similar to a toplevel load. What we do about the principal with toplevel loads is still up in the air, at the moment we are fine with this change. I also verified that it works correctly now.
Attachment #8622557 - Flags: review?(gijskruitbosch+bugs)
Once we start working on Bug 1105556, we should also revisit this bug and search for the right principal (if necessary).
Blocks: 1105556
Attachment #8622557 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/9dc97d73b64f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
A little late to the game, but I have some questions here.

Using TYPE_DOCUMENT seems like a bit of a hack and we may end up revisiting this bug in the future when another consumer of loadInfo is looking for the loadingNode.

In bug 1127927, we stopped passing the doc to loadInfo:
https://bugzilla.mozilla.org/attachment.cgi?id=8561791&action=diff#a/browser/base/content/nsContextMenu.js_sec3
Why?  We can set the doc in loadInfo so that the child process has access to it, even though the parent doesn't.

Since in e10s, the parent process doesn't have a doc, nsMixedContentBlocker::AsyncOnChannelRedirect returns early - http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#222.  The child process should have access to the doc though and complete AsyncOnChannelRedirect.  Why did we have to remove the doc from loadInfo?
Flags: needinfo?(wmccloskey)
Christoph, is this safe for uplift for beta/aurora?
Flags: needinfo?(mozilla)
(I think so, and that will get us out of the fire on 39, but hey...)
This nsContextMenu.js code you quoted is a bit unusual. We're asked to save a document that lives in the content process. However, the content process can't write to disk. So, instead, the parent process loads and renders the document itself and saves it to disk. This is bad for security because we never want to be rendering content in the parent process. Eventually we'd like to render the page in the content process and serialize it to the parent somehow to be written to disk.

In any case, the parent process has no access to the triggeringNode when we start saving since it lives in the content process. (We do have a CPOW, but it's not usable from C++ code.)

One thing I don't understand here is why the request is getting blocked, and why TYPE_DOCUMENT fixes it. Tanvi, it sounds like your last paragraph in comment 22 is trying to explain that, but I don't understand the explanation. The channel we're creating is a normal HTTP channel as far as I know, not an nsIParentChannel.
Flags: needinfo?(wmccloskey)
Comment on attachment 8622557 [details] [diff] [review]
bug_1136055_save_link_as.patch

Approval Request Comment
[Feature/regressing bug #]:
It's actually two bugs:
* Bug 1087726 - Make JS callers of ios.newChannel call ios.newChannel2 in browser/
* Bug 1127927 - [e10s] Save Page As... doesn't work

[User impact if declined]:
* Right click 'save as ...' does not work if the channel gets redirected. File size ends up being 0.

[Describe test coverage new/current, TreeHerder]:
Only the testcoverage provided in the bug (see comment 2. No automatic tests.

[Risks and why]: 
Low, because we are only changing the content type in the loadInfo for downloads which are triggered by 'save link as '''. The contenttype in the loadInfo is actually unused, besides for redirects for mixedcontenblocker.

[String/UUID change made/needed]:
no
Flags: needinfo?(mozilla)
Attachment #8622557 - Flags: approval-mozilla-beta?
Attachment #8622557 - Flags: approval-mozilla-aurora?
Tanvi, Christoph, can we discuss this one in tomorrow's content policy meeting?
(In reply to Jonas Sicking (:sicking) from comment #27)
> Tanvi, Christoph, can we discuss this one in tomorrow's content policy
> meeting?

Most certainly. But we needed a hotfix for 39, so this definitely seems reasonable at the moment. Let's discuss tomorrow. I will come back to this bug and post the outcome of our meeting.
(In reply to Bill McCloskey (:billm) from comment #25)
> One thing I don't understand here is why the request is getting blocked, and
> why TYPE_DOCUMENT fixes it. Tanvi, it sounds like your last paragraph in
> comment 22 is trying to explain that, but I don't understand the
> explanation. The channel we're creating is a normal HTTP channel as far as I
> know, not an nsIParentChannel.

Hey Bill,

I'm having trouble reproducing this with https://jsfiddle.net/bdukes/mgw1mqyu/1/show/.  Not sure what the steps to reproduce are.  But from what I can gather:
* You click "save link as" on a link that is https and redirects to http
* When it redirects to http, nsMixedContentBlocker::AsyncOnChannelRedirect gets called.  AsyncOnChannelRedirect gets called on the child and the parent.
* When it is called in the parent process, we skip the mixed content check (since 1. the child will perform the check for us and 2. we don't have the document).
* When it is called in the child, we use the channel's loadInfo to extract the parameters needed to call nsMixedContentBlocker::ShouldLoad().  Specifically, we use the loadingNode as the aRequestingContext in ShouldLoad().  ShouldLoad() returns early for TYPE_DOCUMENT since mixed content pertains to a top level documents subresources, and not the top level document itself.  For TYPE_OTHER however, the ShouldLoad() code continues to execute.  In order for the code to do all the things it needs to do, we need to query the docshell here http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#591.  This fails because we don't have aRequestingContext (loadInfo->loadingNode) since we didn't set one for "Save Link As".


On a separate note, can we consider "Save Link As" as TYPE_DOCUMENT?  We consider HTTP downloads from HTTPS pages as mixed content.  So isn't this similar?  Should we be able to do an HTTP "Save Link As" on an HTTPS page with a lock?  Will the user assume that the content is fetched over an encrypted connection?
Hi Tanvi,
I'm definitely seeing something different than you. I went to the jsfiddle, right-clicked on "with 302 redirect" and selected "save link as". nsMixedContentBlocker::AsyncOnChannelRedirect doesn't run at all in the child process. It runs in the parent, but it doesn't return early at the nsIParentChannel check. It proceeds into ShouldLoad. I'm not sure what happens inside there since I only have an opt build sitting around and it's hard to debug. It might be that it's denying the request because of the docshell thing you pointed out (but in the parent)?
http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#591

In terms of TYPE_DOCUMENT being okay, it doesn't seem unreasonable. In my mind, "Save link as" is similar to clicking on the link and then doing "Save as" when it's loaded.
Yeah, just checked in a debug build, and it is the docshell check that's failing in the parent.
Comment on attachment 8622557 [details] [diff] [review]
bug_1136055_save_link_as.patch

Taking it in aurora & esr38 because it is an important regression.
Attachment #8622557 - Flags: approval-mozilla-esr38+
Attachment #8622557 - Flags: approval-mozilla-aurora?
Attachment #8622557 - Flags: approval-mozilla-aurora+
(In reply to Sylvestre Ledru [:sylvestre] from comment #32)
> Comment on attachment 8622557 [details] [diff] [review]
> bug_1136055_save_link_as.patch
> 
> Taking it in aurora & esr38 because it is an important regression.

Let's wait to uplift this until after our 2pm PST meeting.
Flags: needinfo?(dcamp)
I'd like to take this for 39 if you feel comfortable that you can verify the fix in beta 7 over this weekend and fix problems or back out by Sunday night. 

If we don't take this for 39 we shouldn't take it for this ESR either.
Flags: needinfo?(tanvi)
Flags: needinfo?(sledru)
There are multiple issues going on in this bug.

When you "save link as" without a redirect, we don't go through nsMixedContentBlocker because Content Policies are not getting called.  (Maybe they should be).  When you "save link as" with a redirect, AsyncOnChannelRedirect gets called, which triggers Mixed Content Blocker.  Mixed Content Blocker looks for the aRequestingContext (the loadingNode in loadInfo).  If one doesn't exist, it errors out and causes this problem.

When you are on an https page and you attempt to download an http resource, Mixed Content Blocker will block that resource, as it should.  If I go to https://mozilla.org and download Firefox, I expect that the download is also HTTPS and can't be switched to malware by a MITM.  Do we want the same behavior for save-link-as?  Using 'save-link-as' in the example in comment 2 results in downloading an mp3.  Shouldn't it get the Mixed Content check the same way downloads do?  And hence TYPE_OTHER is probably the right content type.

I still don't quite understand why we can't pass the doc here - https://bugzilla.mozilla.org/attachment.cgi?id=8561791&action=diff#a/browser/base/content/nsContextMenu.js_sec3

From Bill's comment 30, it sounds like the check to see if the process is a child or a parent in nsMixedContentBlocker::AsyncOnChannelRedirect is not robust enough.

Separately, we should create a bug for better UI / error messaging when Mixed Content Blocker blocks something because of a missing docshell.  Not sure what that messaging should be yet, because a missing docshell isn't exactly a website error.
(In reply to Tanvi Vyas [:tanvi] from comment #33)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #32)
> > Comment on attachment 8622557 [details] [diff] [review]
> > bug_1136055_save_link_as.patch
> > 
> > Taking it in aurora & esr38 because it is an important regression.
> 
> Let's wait to uplift this until after our 2pm PST meeting.

So we just finished our meeting and we decided to go with a slightly different approach. It's also a one line fix. I will past a patch today which we then can uplift to 39.
Status: RESOLVED → REOPENED
Flags: needinfo?(tanvi)
Flags: needinfo?(sledru)
Resolution: FIXED → ---
Comment on attachment 8622557 [details] [diff] [review]
bug_1136055_save_link_as.patch

Clearing the uplift flags for this patch.
Attachment #8622557 - Flags: approval-mozilla-esr38+
Attachment #8622557 - Flags: approval-mozilla-beta?
Attachment #8622557 - Flags: approval-mozilla-aurora+
Attached patch bug_1136055_save_link_as.patch (obsolete) — Splinter Review
Gijs, today in our meeting we decided for the other option, instead of using TYPE_DOCUMENT, we decided to rather use the systemPrincipal to bypass mixed content blocking after redirects. I verified that the patch fixes the problem described. If you remember, we discussed the two options we have to fix the bug:
* either use TYPE_DOCUMENT, or
* use the SystemPrincipal in the loadInfo when performing a 'save link as...'.

Tanvi, Jonas, and Billm are rather for option two, which is fine by me and I am pretty sure fine by you as well. I am not sure if the sheriffs would rather back out the old patch or land this one on top.
Attachment #8622557 - Attachment is obsolete: true
Attachment #8624031 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8624031 [details] [diff] [review]
bug_1136055_save_link_as.patch

Review of attachment 8624031 [details] [diff] [review]:
-----------------------------------------------------------------

I mean, r+ assuming this doesn't make things work that shouldn't. For instance, it looks like save link as on something like this:

http://jsbin.com/tenihukimu/edit?html,output

is currently denied. Will that continue to be the case? Do we care?
Attachment #8624031 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #39)
> Comment on attachment 8624031 [details] [diff] [review]
> bug_1136055_save_link_as.patch
> 
> Review of attachment 8624031 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I mean, r+ assuming this doesn't make things work that shouldn't. For
> instance, it looks like save link as on something like this:
> 
> http://jsbin.com/tenihukimu/edit?html,output
> 
> is currently denied. Will that continue to be the case? Do we care?

So to sum it up again. For 'save link as ...' we don't perform any content security checks before we start the download. In other words ::ShouldLoad which internally calls mixed content blocker is not called. For redirects however, mixed content blocker implements ::AsyncOnChannelRedirect [1]. In simpler words, for 'save link as...' we do not evaluate mixed content before starting the download but because of the way our implementation works we do evaluate mixed content if that download is redirected. Using the systemPrincipal as the loadingprincipal allows us to bypass mixed content blocking after redirects for 'save link as ...'. This is the behavior we would like to have for now. It has been that way for a long time and we are not making things any worse than they are right now.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#210
Just chatted with Gijs on IRC again to make sure everything we do here does not introduce any other side effects, errors, or regressions.

> (In reply to :Gijs Kruitbosch from comment #39)
> > it looks like save link as on something like this:
> > 
> > http://jsbin.com/tenihukimu/edit?html,output
> > 
> > is currently denied. Will that continue to be the case? Do we care?

The case should still be denied because we do a URI sanity check. Before the download is started we receive a call to urlSecurityCheck [1] to make sure to not access e.g. chrome://.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/BrowserUtils.jsm#56
Great. We should get this landed. Considering that this being broken didn't break any tests, and the previous patch should have the same net effect and was already green, I don't think this needs a trypush, but the tree is closed.

For when we're tying this "back down" some other way in future: can we ensure that we add a regression test for this case then? It would have caught both the e10s issue and this.
Keywords: checkin-needed
Carrying over r+ from Gijs and rebased the patch on current beta.
Attachment #8624031 - Attachment is obsolete: true
Attachment #8624338 - Flags: review+
Here is what would need to happen:
We would need a backout of
> https://bugzilla.mozilla.org/show_bug.cgi?id=1136055#c20
and then reland this patch
> bug_1136055_save_link_as_after_backout.patch
which we can then uplift.

Ryan, could you do that for me please?
Flags: needinfo?(ryanvm)
Flags: needinfo?(ryanvm)
There are a number of followup items from this bug:

* should save link as go through any security checks?  Should we be calling the Content Policy API?
* should we be passing the doc principal or system principal to loadInfo.
* We need to find a better way to error out of nsMixedContentBlocker when no docshell is available.  I'm not sure if a console message makes sense. 
* Are there other places in the code where we will run into similar problems - we don’t have a node in the loadInfo and its content that could do through a redirect. (ex: any loads that comes from a worker, at least shared worker if not all workers)
* save-link-as and urlSecurityCheck
(In reply to Tanvi Vyas [:tanvi] from comment #46)
> There are a number of followup items from this bug:
>
> * save-link-as and urlSecurityCheck
save-link-as goes through urlSecurityCheck.  But if save-link-as goes through a redirect, urlSecurityCheck won't get called on it and we could potentially let something through we shouldn't.  Gijs and I both tried testing this and have thankfully had no luck.  But its something to look closer at.  When we move security checks, we should set appropriate flags on LoadInfo to make sure a redirect goes through the same security checks that the original link went through for save-link-as.
https://people.mozilla.org/~tvyas/https_302_chrome.html
https://hg.mozilla.org/mozilla-central/rev/a206ed49ec46
https://hg.mozilla.org/mozilla-central/rev/e8c9a6fdad6a
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Comment on attachment 8624338 [details] [diff] [review]
bug_1136055_save_link_as_after_backout.patch

The initial patch was already approved for uplifiting. But since we have decided to back out the first patch and use this new patch we have to re-set the flags for uplift approval.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #26)
> Comment on attachment 8622557 [details] [diff] [review]
> bug_1136055_save_link_as.patch
> 
> Approval Request Comment
> [Feature/regressing bug #]:
> It's actually two bugs:
> * Bug 1087726 - Make JS callers of ios.newChannel call ios.newChannel2 in
> browser/
> * Bug 1127927 - [e10s] Save Page As... doesn't work
> 
> [User impact if declined]:
> * Right click 'save as ...' does not work if the channel gets redirected.
> File size ends up being 0.
> 
> [Describe test coverage new/current, TreeHerder]:
> Only the testcoverage provided in the bug (see comment 2. No automatic tests.
> 
> [Risks and why]: 
> Low, because we are only changing the content type in the loadInfo for
> downloads which are triggered by 'save link as '''. The contenttype in the
> loadInfo is actually unused, besides for redirects for mixedcontenblocker.
> 
> [String/UUID change made/needed]:
> no
Attachment #8624338 - Flags: approval-mozilla-beta?
Attachment #8624338 - Flags: approval-mozilla-aurora?
Comment on attachment 8624338 [details] [diff] [review]
bug_1136055_save_link_as_after_backout.patch

Approved for uplift to aurora and beta because it has an impact on many users. Very last minute!
Attachment #8624338 - Flags: approval-mozilla-beta?
Attachment #8624338 - Flags: approval-mozilla-beta+
Attachment #8624338 - Flags: approval-mozilla-aurora?
Attachment #8624338 - Flags: approval-mozilla-aurora+
Christoph can you follow up and help us test this when it's in the RC build?
Flags: needinfo?(mozilla)
(In reply to Liz Henry (:lizzard) from comment #51)
> Christoph can you follow up and help us test this when it's in the RC build?

Chatted with Kamil (from QA) in person, he offered to test and make sure it works correctly.
@Kamil: Comment 2 has a nice testpage. Thanks for your help!
Flags: needinfo?(mozilla) → needinfo?(kjozwiak)
Reproduced the original issue using the following build(s):
- http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/38.0.1/win32/en-US/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015/02/2015-02-23-03-02-31-mozilla-central/

Used the following test cases to reproduce original issue:
- https://interfacelift.com/wallpaper/details/3918/drifting_away.html
- https://jsfiddle.net/bdukes/mgw1mqyu/1/show/

When right clicking and selecting "Save Link As.." on 302/301 redirects, fx downloaded a 0kbs file rather than the full expected file.

Went through verification using the following builds:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-07-02-03-02-07-mozilla-central/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-07-02-00-40-06-mozilla-aurora/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/39.0-candidates/build6/

OS's Used: (went through each channel on each individual OS)

- Win 8.1 x64 VM - PASSED
- Ubuntu 14.04.2 x64 VM - PASSED
- OSX 10.10.4 - PASSED

Went through the two websites and ensured that selecting "Save Link As.." correctly downloaded the content. Made sure both direct and redirect links worked without any issues.

Chris, let me know if there's anything that I've missed!
See Also: → 1179947
(In reply to Tanvi Vyas [:tanvi] from comment #46)
> There are a number of followup items from this bug:
> 
> * should save link as go through any security checks?  Should we be calling
> the Content Policy API?
> * should we be passing the doc principal or system principal to loadInfo.
> * save-link-as and urlSecurityCheck
Gijs, can you file followup bugs for these?

> * We need to find a better way to error out of nsMixedContentBlocker when no
> docshell is available.  I'm not sure if a console message makes sense. 
I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1179947

> * Are there other places in the code where we will run into similar problems
> - we don’t have a node in the loadInfo and its content that could do through
> a redirect. (ex: any loads that comes from a worker, at least shared worker
> if not all workers)
We need to look for places where no loadingNode is set and the loadingPrincipal is not system principal.  Christoph, you may have already done this?
(In reply to Tanvi Vyas [:tanvi] from comment #57)
> (In reply to Tanvi Vyas [:tanvi] from comment #46)
> > There are a number of followup items from this bug:
> > 
> > * should save link as go through any security checks?  Should we be calling
> > the Content Policy API?
> > * should we be passing the doc principal or system principal to loadInfo.
> > * save-link-as and urlSecurityCheck
> Gijs, can you file followup bugs for these?

I'm not sure I understand what you mean by some of these questions well enough to be the best person to file these. Please needinfo me if you still think it'd be better if I did that than if it was you or Christoph.
(In reply to Kamil Jozwiak [:kjozwiak] from comment #56)
> Reproduced the original issue using the following build(s):
> - http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/38.0.1/win32/en-US/
> -
> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015/02/2015-02-23-03-
> 02-31-mozilla-central/
> 
> Used the following test cases to reproduce original issue:
> - https://interfacelift.com/wallpaper/details/3918/drifting_away.html
> - https://jsfiddle.net/bdukes/mgw1mqyu/1/show/
> 
> When right clicking and selecting "Save Link As.." on 302/301 redirects, fx
> downloaded a 0kbs file rather than the full expected file.
> 
> Went through verification using the following builds:
> -
> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-07-02-03-02-07-
> mozilla-central/
> -
> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-07-02-00-40-06-
> mozilla-aurora/
> -
> http://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/39.0-candidates/
> build6/
> 
> OS's Used: (went through each channel on each individual OS)
> 
> - Win 8.1 x64 VM - PASSED
> - Ubuntu 14.04.2 x64 VM - PASSED
> - OSX 10.10.4 - PASSED
> 
> Went through the two websites and ensured that selecting "Save Link As.."
> correctly downloaded the content. Made sure both direct and redirect links
> worked without any issues.
> 
> Chris, let me know if there's anything that I've missed!

Thanks so much Kamil.

Liz, since Kamil verified it's not an issue anymore, I don't think there is anything left to do here, right? Just making sure we are not missing something.
Flags: needinfo?(lhenry)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #59)
> I don't think there is
> anything left to do here, right? Just making sure we are not missing
> something.

Clearing my needinfo queue at the moment. Since I haven't heard any complaints I think everything turned out to be ok in this bug. (Clearing needinfo now).
Flags: needinfo?(lhenry)
Hi guys,

this issue occurs with the latest Firefox version, 40.0.2, on some selected urls with redirected links.
save as link works flawlessly, but with direct mouse click, there's no redirect and no download.
in some cases, there's an empty redirect response.
By now we've found out the following details:
If the user updates or upgrades to FF version 40.0.2 (clean machine) he can successfully download the files from the sites mentioned above in a period of ~ 20 min.
After that period it is no longer possible to download the files.
Restarting PC has no effect.
link example:
http://download.jzip.com/jZipSetup.exe
when downloading from firefox directly, the issue occurs. on any other browser and with save as link, there's no problem.
(In reply to Pavel Zeldin from comment #61)
> Hi guys,
> 
> this issue occurs with the latest Firefox version, 40.0.2, on some selected
> urls with redirected links.
> save as link works flawlessly, but with direct mouse click, there's no
> redirect and no download.
> in some cases, there's an empty redirect response.

Hey Pavel, can you file a new (separate) bug for this issue with the details you provided, please?
Flags: needinfo?(pavel)
sure, no problems.
Flags: needinfo?(pavel)
(In reply to Tanvi Vyas [:tanvi] from comment #57)
> (In reply to Tanvi Vyas [:tanvi] from comment #46)
> > There are a number of followup items from this bug:
> > 
> > * should save link as go through any security checks?  Should we be calling
> > the Content Policy API?
> > * should we be passing the doc principal or system principal to loadInfo.
> > * save-link-as and urlSecurityCheck

I filed 1195555 for these followups.
Blocks: 1195555
Depends on: 1195242
You need to log in before you can comment on or make changes to this bug.