Closed Bug 257308 Opened 15 years ago Closed 15 years ago

Visual indicators of site security appear for the wrong site (lock icon)


(Core :: Networking, defect, critical)

Not set





(Reporter: bugzilla, Assigned: darin.moz)


(Blocks 1 open bug, )


(Keywords: fixed-aviary1.0, fixed1.4.4, fixed1.7.5, Whiteboard: [sg:fix])


(6 files, 3 obsolete files)

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:

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.
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
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. :)
Ever confirmed: true
Flags: blocking-aviary1.0PR?
Flags: blocking-aviary1.0?
HW/OS -> All, Severity -> critical

Well, it's pretty serious anyway heh..
Severity: normal → critical
OS: Windows 98 → All
Hardware: PC → All
If we've been shipping with this behaviour for a while, I doubt we'll block on this.
darin will take a look to see what we can do here.
Assignee: firefox → darin
if this can make PR it would be good.
Flags: blocking-aviary1.0PR?
Flags: blocking-aviary1.0PR+
Flags: blocking-aviary1.0?
Flags: blocking-aviary1.0+
This is a dup.
Whiteboard: DUPEME
happens in mozilla 2004090105/winxp too, -> browser
Component: General → Networking
Product: Firefox → Browser
QA Contact: firefox.general → benc
Version: unspecified → Trunk
Target Milestone: --- → mozilla1.8beta
On the surface this is sort of the opposite of bug 258048
Blocks: lockicon
*** Bug 238566 has been marked as a duplicate of this bug. ***
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.
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.
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 :-(
Attached patch v1 patch (obsolete) — Splinter Review
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.
Attachment #158258 - Flags: superreview?(bzbarsky)
Attachment #158258 - Flags: review?(cbiesinger)
Attachment #158258 - Flags: review?(cbiesinger) → review+
Comment on attachment 158258 [details] [diff] [review]
v1 patch

sr=bzbarsky if you also fix the similar code at
(which is retargeting from one docshell to another, but the effect is the
Attachment #158258 - Flags: superreview?(bzbarsky) → superreview+
thanks bz!
Attachment #158258 - Attachment is obsolete: true
Closed: 15 years ago
Resolution: --- → FIXED
Attachment #158283 - Flags: approval1.7.x?
Attachment #158283 - Flags: approval-aviary?
Needed on 1.7 also
Flags: blocking1.7.x+
Whiteboard: DUPEME → [sg:fix]
Comment on attachment 158283 [details] [diff] [review]
v1.1 patch - with similar nsDocShell changes

a=mkaply for both
Attachment #158283 - Flags: approval1.7.x?
Attachment #158283 - Flags: approval1.7.x+
Attachment #158283 - Flags: approval-aviary?
Attachment #158283 - Flags: approval-aviary+
Needed on Aviary also.
fixed1.7.x, fixed-aviary1.0
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
leading to an mp3 file hosted at still makes
livejournal's location bar go yellow and padlocked, and the status bar shows 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.
Attached file 30kb mp3
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.
Ok, that one worked fine for me too. Maybe it's something Comcast-specific.
Attachment #158694 - Attachment mime type: text/plain → application/octet-stream
The testcase should work now.  A mimetype of application/octet-stream should
force Firefox to always ask to download.
Hmm... I just downloaded the latest aviary branch build from
(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
(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?
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?
There's lotsa ways to do this..

wget -sqO-

-(~) telnet 80
Connected to
Escape character is '^]'.
HEAD /some.mp3 HTTP/1.0

HTTP/1.1 200 OK

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

openssl s_client -connect
<send http request>

Or, you could even make a small libcurl or perl client. :P
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.
Ok! I have a new testcase for you to try:
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?
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).

Resolution: FIXED → ---
Attachment #158694 - Attachment mime type: application/octet-stream → text/plain
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
> Can anyone confirm this?

Yes, I can reproduce the problem too using the 9/14 aviary branch build w/ your
testcase.  Investigating...
It looks like we are getting an OnProgress event prior to OnStartRequest when
loading  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. 

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.
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...
Attached patch v2 patch (obsolete) — Splinter Review
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 ;-)
Attachment #159229 - Flags: superreview?(bzbarsky)
Attachment #159229 - Flags: review?(cbiesinger)
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?)
Attachment #159229 - Flags: superreview?(bzbarsky) → superreview+
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

Thanks for catching the |= mistake.  That's corrected in this patch too.
Attachment #159229 - Attachment is obsolete: true
Attachment #159229 - Flags: review?(cbiesinger)
Attachment #159235 - Flags: review?(cbiesinger)
Comment on attachment 159235 [details] [diff] [review]
v2.1 patch - revised per bz's comments

looks good
Attachment #159235 - Flags: review?(cbiesinger) → review+
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Attachment #159235 - Flags: approval1.7.x?
Attachment #159235 - Flags: approval-aviary?
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
Attachment #159235 - Flags: approval1.7.x? → approval1.7.x+
Comment on attachment 159235 [details] [diff] [review]
v2.1 patch - revised per bz's comments

a=asa for aviary checkin.
Attachment #159235 - Flags: approval-aviary? → approval-aviary+
fixed-aviary1.0 and fixed1.7.x for v2.1 patch
Summary: Visual indicators of site security appear for the wrong site → Visual indicators of site security appear for the wrong site (lock icon)
Attachment #158251 - Attachment description: sample test case → sample test case (view on non-secure server)
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.
Attachment #172289 - Flags: review?(darin) → review-
also, the last patch didn't include the definition of nsIChannel::LOAD_TARGETED.
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?
Attachment #172289 - Attachment is obsolete: true
Attachment #174321 - Flags: review?(darin)
Comment on attachment 174321 [details] [diff] [review]
Backport for 1.4 take 2

sure, this looks fine to me.  r=darin
Attachment #174321 - Flags: review?(darin) → review+
You need to log in before you can comment on or make changes to this bug.