Ben Sizer Reporter
	Description • 20 years ago

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.2) Gecko/20040819 Firefox/0.8 StumbleUpon/1.9 (MOOX M2)
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.2) Gecko/20040819 Firefox/0.8 (MOOX M2)

If I'm on an http site, then I click a link on that site to a totally unrelated
https server which provides a downloadable file - not a file that can be viewed
in Firefox, and not where the link target is a new window - then the address bar
for the currently viewed HTTP site goes yellow, and the security box on the
status bar shows the http site with a padlock. In other words, the HTTP site is
reported as taking on the security characteristics of the HTTPS server that is
supplying the file rather than the currently-viewed page.

Further discussion here: http://forums.mozillazine.org/viewtopic.php?t=117908

Reproducible: Always
Steps to Reproduce:
1.Find or create a link on a HTML page hosted on a HTTP server that points to a
downloadable file on an unrelated HTTPS server.
2.Click that link.
3.Accept the file or cancel (the visual indications change before you make the
choice anyway.)

Actual Results:  
Address bar background changes yellow.
Status bar security field displays the HTTP server name with a padlock, and
changes the tool-tip to mention the HTTPS's certificate issuing authority.

Expected Results:  
Made none of the above changes in the UI.

Perhaps the relevant section of the download manager window should go yellow
instead, but I'm sure that's a separate issue...

The linked-to discussion leads me to believe that the yellow-bar feature may
only be visible in more recent builds.

	kstahl
	Comment 1 • 20 years ago

Confirming. Using the link in that thread to download a file from a secure
server gives me the padlock on the page I'm currently on.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040803 Firefox/0.9.3

	logan
	Comment 2 • 20 years ago

This appears to be quite old. I've been able to duplicate this with Firefox
branch, 0.9.3, 0.9.1, 0.8 and Mozilla 1.7.2. It's possible this is a dupe of
another bug with the security flag.

Marking NEW, requesting blocking.

Thanks for filing man. :)

	logan
	Comment 3 • 20 years ago

HW/OS -> All, Severity -> critical

Well, it's pretty serious anyway heh..

	Mike Connor [:mconnor]
	Comment 4 • 20 years ago

If we've been shipping with this behaviour for a while, I doubt we'll block on this.

	chris hofmann
	Comment 5 • 20 years ago

darin will take a look to see what we can do here.

	chris hofmann
	Comment 6 • 20 years ago

if this can make PR it would be good.

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

This is a dup.

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 8 • 20 years ago

happens in mozilla 2004090105/winxp too, -> browser

	Daniel Veditz [:dveditz]
	Comment 9 • 20 years ago

On the surface this is sort of the opposite of bug 258048

	Daniel Veditz [:dveditz]
	Comment 10 • 20 years ago

*** Bug 238566 has been marked as a duplicate of this bug. ***

	Daniel Veditz [:dveditz]
	Comment 11 • 20 years ago

Are you sure this is a dupe? We've fixed similar bugs, but I can't find one
currently like this.

bug 240053 was fixed in Moz1.7 / ff0.9 (redirects)
bug 253121 was fixed in Moz1.7.2 / ff0.9.3 (onunload document.write)

bug 258048 describes an opposite situation, security indicators for the current
site stick around too long.

	Darin Fisher Assignee
	Comment 13 • 20 years ago

Load this attachment over plaintext HTTP.  Then, click the link named "click
me".  Notice that you get a download dialog and the lock icon is present on the
browser window, but the original page contents are still showing.

	Darin Fisher Assignee
	Comment 14 • 20 years ago

I can reproduce this bug using Mozilla 1.0 and 1.4, so this has been around for
a long time.  I also don't see any evidence in the code that there was ever a
design intended to handle this case :-(

	Darin Fisher Assignee
	Comment 15 • 20 years ago

OK, this seems like a safe patch.  The solution is to basically make retargeted
loads behave more like redirected loads as far as the webprogresslistener
notifications are concerned.

With this patch, we pass NS_BINDING_RETARGETED as the status code when calling
RemoveRequest from inside nsExternalAppHandler::RetargetLoadNotifications.  It
makes sense to pass an error code here because the load is actually not
successful with respect to loading inside the load group.  It is really pretty
wrong to say that the request has completed normally, which is what the current
code does.

The other change this patch makes is to cause the DocLoader to not synthesize a
STATE_TRANSFERRING event when the loadgroup calls its OnStopRequest method
(triggered from RemoveRequest).  This is what we currently do for redirected
requests, and it makes sense to do the same for retargeted requests.

One more thing to note: NS_BINDING_RETARGETED is not used anywhere else.  I
recall back in the day that it was used.  Used by code in uriloader land if I'm
not mistaken.  The fact that it is not used is good from the point of view of
this patch, since it means that we aren't co-opting an existing meaning for
this error code.

	Boris Zbarsky [:bzbarsky]
	Comment 16 • 20 years ago

Comment on attachment 158258 [details] [diff] [review]
v1 patch

sr=bzbarsky if you also fix the similar code at
http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#4546
(which is retargeting from one docshell to another, but the effect is the
same).

	Darin Fisher Assignee
	Comment 17 • 20 years ago

thanks bz!

	Darin Fisher Assignee
	Comment 18 • 20 years ago

fixed-on-trunk

	Daniel Veditz [:dveditz]
	Comment 19 • 20 years ago

Needed on 1.7 also

	Mike Kaply [:mkaply]
	Comment 20 • 20 years ago

Comment on attachment 158283 [details] [diff] [review]
v1.1 patch - with similar nsDocShell changes

a=mkaply for both

	Bill Gianopoulos [:WG9s]
	Comment 21 • 20 years ago

Needed on Aviary also.

	Darin Fisher Assignee
	Comment 22 • 20 years ago

fixed1.7.x, fixed-aviary1.0

	Ben Sizer Reporter
	Comment 23 • 20 years ago

This isn't totally fixed, assuming it's supposed to be on my build (Firefox
branch, 20040912). I can't give out the test URL I see this on for copyright and
practicality reasons, but clicking a link hosted on http://www.livejournal.com
leading to an mp3 file hosted at https://home.comcast.net still makes
livejournal's location bar go yellow and padlocked, and the status bar shows
livejournal.com as being a secure site.

It seems possibly dependent on mime type as the behaviour seems fixed for zip
files on the secure server but still broken for mp3s.

	logan
	Comment 25 • 20 years ago

Hrm, ok I'm not sure that worked right heh.. I was trying to make a testcase
with test.mp3, but the file is displaying in the browser.

Thought maybe if the content-type was being sent as text/plain, Firefox would
start out wanting to view the file, but once it sees binary data it'd change to
download instead.

I setup a test at work and didn't have any problems with content-type bits.

http://www.gozer.org/test/mozilla/257308/

	Ben Sizer Reporter
	Comment 26 • 20 years ago

Ok, that one worked fine for me too. Maybe it's something Comcast-specific.

	Robert Parenton
	Comment 27 • 20 years ago

The testcase should work now.  A mimetype of application/octet-stream should
force Firefox to always ask to download.

	Darin Fisher Assignee
	Comment 28 • 20 years ago

Hmm... I just downloaded the latest aviary branch build from ftp.mozilla.org
(Mozilla/5.0 (X11; U; Linux i686; rv:1.7.3) Gecko/20040910 Firefox/0.10), and I
couldn't reproduce the bug using the testcases at
http://www.gozer.org/test/mozilla/257308/.

	logan
	Comment 29 • 20 years ago

(In reply to comment #26)
> Ok, that one worked fine for me too. Maybe it's something Comcast-specific.

Since you can't put up the urls, could you get the headers for the file that's
causing this?

	Ben Sizer Reporter
	Comment 30 • 20 years ago

I don't know how to get the headers for an mp3 download. Page Info and the Web
Developer extension's 'view response headers' function don't work because the
page stays blank while it downloads in the download manager. Any suggestions?

	logan
	Comment 31 • 20 years ago

There's lotsa ways to do this..

wget -sqO- http://www.example.com/some.mp3

-(~) telnet www.example.com 80
Trying 127.0.0.1...
Connected to www.example.com.
Escape character is '^]'.
HEAD /some.mp3 HTTP/1.0
Host: www.example.com

HTTP/1.1 200 OK
...

telnet won't work on an ssl-enabled server, so...

openssl s_client -connect www.example.com:443
<send http request>

Or, you could even make a small libcurl or perl client. :P

	Ben Sizer Reporter
	Comment 32 • 20 years ago

Interesting... :

HTTP/1.1 200 OK
Server: Netscape-Enterprise/4.1
Date: Tue, 14 Sep 2004 01:29:16 GMT
Content-type: text/plain
Last-modified: Mon, 13 Sep 2004 20:15:51 GMT
Content-length: 2740429
Accept-ranges: bytes
Connection: close

It's sending text/plain, yet it's definitely an mp3 file, and Firefox correctly
identifies it as such (giving me the save/open dialog) when I download it.

	Ben Sizer Reporter
	Comment 33 • 20 years ago

Ok! I have a new testcase for you to try:
http://homepage.ntlworld.com/ben.sizer/testmoz/mp3test.html
Click the mp3 link there, which leads to an mp3 file hosted on a Comcast server,
accessed with https. The actual/expected results are as I originally reported at
the start of this bug. I have just tested with a 20040916 branch build of
Firefox. Can anyone confirm this?

	logan
	Comment 34 • 20 years ago

Man, I feel like a dumbass - Content-Type: test/plain

I've fixed my testcase and am able to see it now, must be due to the text/plain
[oh it's binary] stuff (bug 220807).

Reopened

	logan
	Comment 35 • 20 years ago

Ok, I can't get my bugzilla test.mp3 attachment to work right, I think it's
content-disposition: inline header, so we'll just have to rely on the external
testcases.

	Darin Fisher Assignee
	Comment 36 • 20 years ago

> http://homepage.ntlworld.com/ben.sizer/testmoz/mp3test.html
> Can anyone confirm this?

Yes, I can reproduce the problem too using the 9/14 aviary branch build w/ your
testcase.  Investigating...

	Darin Fisher Assignee
	Comment 37 • 20 years ago

It looks like we are getting an OnProgress event prior to OnStartRequest when
loading http://homepage.ntlworld.com/ben.sizer/testmoz/mp3test.html.  I think
that would explain the lock icon bug because the nsSecureBrowserUI uses the
OnProgress event to determine that secure content is being shown to the user. 
Hmm...

It looks like nsHttpChannel does not ensure that OnProgress occur only after
OnStartRequest.  There's a side issue since we use OnProgress to report upload
progress as well as download progress.  Clearly, nsSecureBrowserUI should only
apply its OnProgress logic to downloads, so we should verify that.

	Darin Fisher Assignee
	Comment 38 • 20 years ago

ok, after further analysis i understand the problem.  the channel cannot solve
this problem.  it is in fact delaying OnProgress until after OnStartRequest, but
that is not enough.  in this case, the content-type from the server is
text/plain.    in this case we invoke a content sniffer to determine if the
content is really text because it is often the case that servers such as this
one are misconfigured.  the content sniffer requires data to be read from the
channel, which triggers the OnProgress notification.

to solve this problem i think we need to suppress sending
OnStateChange(STATE_TRANSFERRING) until after a load has been targetted.  this
only applies to toplevel loads with the LOAD_DOCUMENT_URI load flag.  one way to
do that would be to invent a new load flag that gets set by the
nsDocumentOpenInfo once it has targetted a load.  nsDocLoader::OnProgress would
then check for that load flag and suppress the OnStateChange call if the load
flag is not set (for the case in which LOAD_DOCUMENT_URI is set).

i discussed this plan with biesi over IRC and he agrees.  this seems like a more
robust fix that should help minimize the chance of this bug ever coming up again.

patch coming up...

	Darin Fisher Assignee
	Comment 39 • 20 years ago

Here's a patch for the solution I mentioned.  I still need to document the load
flags used by the URILoader, DocLoader, and DocShell better.  That
documentation probably doesn't belong in nsIChannel.idl ;-)

	Boris Zbarsky [:bzbarsky]
	Comment 40 • 20 years ago

Comment on attachment 159229 [details] [diff] [review]
v2 patch

>Index: uriloader/base/nsDocLoader.cpp
>-  nsLoadFlags loadFlags = 0;
>+  PRUint32 loadFlags = 0;

Why?  The loadFlags attribute of nsIRequest is of type "nsLoadFlags"...

>+    newLoadFlags = nsIChannel::LOAD_RETARGETED_DOCUMENT_URI;

This should be an |=, no?

sr=bzbarsky with those fixed (and perhaps the other GetLoadFlags callers also
using nsLoadFlags?)

	Darin Fisher Assignee
	Comment 41 • 20 years ago

nsLoadFlags vs. PRUint32 ... no good reason really.  much of necko uses
PRUint32 (despite nsIRequest.idl), and so I went with that here too.  But, I
don't care much either way, so I changed all of uriloader/base over to
nsLoadFlags.

Thanks for catching the |= mistake.  That's corrected in this patch too.

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 42 • 20 years ago

Comment on attachment 159235 [details] [diff] [review]
v2.1 patch - revised per bz's comments

looks good

	Darin Fisher Assignee
	Comment 43 • 20 years ago

fixed-on-trunk

	Daniel Veditz [:dveditz]
	Comment 44 • 20 years ago

Comment on attachment 159235 [details] [diff] [review]
v2.1 patch - revised per bz's comments

a=dveditz for 1.7x contingent on aviary approval. Would like some trunk baking
first.

	Asa Dotzler [:asa]
	Comment 45 • 20 years ago

Comment on attachment 159235 [details] [diff] [review]
v2.1 patch - revised per bz's comments

a=asa for aviary checkin.

	Darin Fisher Assignee
	Comment 46 • 20 years ago

fixed-aviary1.0 and fixed1.7.x for v2.1 patch

	Darin Fisher Assignee
	Comment 48 • 20 years ago

Comment on attachment 172289 [details] [diff] [review]
Complete patch, backported to 1.4

>Index: uriloader/base/nsURILoader.cpp

>     }
> 
>+    aChannel->SetLoadFlags(loadFlags | newLoadFlags);
>+
>+
>     const char* contentTypeToUse;

nit: no need for extra newline


>     if (helperAppService)
>     {
>+      // Set these flags to indicate that the channel has been targeted and that
>+      // we are not using the original consumer.
>+      nsLoadFlags loadFlags = 0;
>+      request->GetLoadFlags(&loadFlags);
>+      request->SetLoadFlags(loadFlags | nsIChannel::LOAD_RETARGETED_DOCUMENT_URI
>+                                    | nsIChannel::LOAD_TARGETED);
>+
>       rv = helperAppService->DoContent(contentType.get(),
>                                        request,
>                                        m_originalContext,
>                                        getter_AddRefs(contentStreamListener));
>       if (NS_SUCCEEDED(rv) && contentStreamListener) {
>         m_targetStreamListener = contentStreamListener;
>+        request->SetLoadFlags(loadFlags);
>         return NS_OK;
>       }

I think it is wrong to call SetLoadFlags in the success case here.
The trunk does so in the failure case.

	Darin Fisher Assignee
	Comment 49 • 20 years ago

also, the last patch didn't include the definition of nsIChannel::LOAD_TARGETED.

	Christopher Aillon (sabbatical, not receiving bugmail)
	Comment 50 • 20 years ago

addressing darin's comments.  I also think we need to null out
m_targetStreamListener (if DoContent() fails) looking at the current trunk.  It
seems like the correct thing to do here, what do you think darin?

	Darin Fisher Assignee
	Comment 51 • 20 years ago

Comment on attachment 174321 [details] [diff] [review]
Backport for 1.4 take 2

sure, this looks fine to me.  r=darin