154240 - No warning when redirecting https-http-https at http protocol level

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Kai Engert [:KaiE:]]

This is a constructed test scenario I'm using to test my code.

Prepare a web page B, that gives out a HTTP code 302 and redirects to a new
document location https://C

Prepare a page A that has a link to http://B

Go to https://A
When you click the link to B, it will load a http page, and immediately redirect
to https://C

Expected behaviour: Because the intermediate loading of a http page could be
used to transfer data unencrypted, the browser should show warnings. A warning
should first be shown that an unencrypted web site is loaded. Then another
warning should be shown, that a secure web site is loaded.

Actual behaviour: No warning is shown.

Comment 1

[Kai Engert [:KaiE:]]

A test case is available at
  https://www.kuix.de/misc/test26/redirect302.php

Comment 2

[Kai Engert [:KaiE:]]

Attachment: Patch (looks scary, because indentation changed)

Comment 3

[Kai Engert [:KaiE:]]

Attachment: Same patch, ignoring whitespace

Comment 4

[Paul Wyskoczka]

Adding [adt2 rtm] and nsbeta1+

Comment 5

[Kai Engert [:KaiE:]]

Attachment: Patch v2

This patch fixes the problem.

When I originally rewrote the lock icon behaviour with bug 130949, I did not
test the redirect at the protocol level. This problem has been in since 130949
landed.

Luckily, now that bug 148610 has landed, it is possible to remove some code
from the tracking code, that is no longer needed.

When I wrote 130949, I tried to stick with the "wait with updating until the
request is finished" scheme.

But now, we are updating the lock icon as early as possible. Therefore, it is
ok to check the lock icon state each time a top level document finishes
loading. A top level document does finish loading when doing 302 redirects.

The attached patch simply removes a restriction to test only in some cases, and
makes us check more often. By doing so, we are now detecting the insecure
redirect correctly.

Comment 6

[Javier Delgadillo]

Comment on attachment 89167 [details] [diff] [review]
Patch v2

r=javi looks good.

Comment 7

[Alec Flett]

Comment on attachment 89167 [details] [diff] [review]
Patch v2

sr=alecf

Comment 8

[Kai Engert [:KaiE:]]

Checked in to trunk around 7 am this morning.

Should be fixed in today's builds appearing after 8 am.

Comment 9

[John Unruh]

Verified fixed on the 9:30 AM 6/26 trunk. 
https://www.kuix.de/misc/test26/many.php

Comment 10

[scottputterman]

adding adt1.0.1+.  Please get drivers approval (cc'ing valeski) before checking
into the branch.

Comment 11

[Stephane Saux]

John,
  If you haven't done so already, can you run the full lock icon regression
testing with this additional patch?  We want to make sure that any work on the
lock icon is tested against both the original bug report, but also for any
additional regressions.
  If you have done so already, can you just state so?
thanks.

Comment 12

[John Unruh]

I did full lock icon testing with the trunk build, and all is as expected. The 
patch should be checked into the branch.

Comment 13

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 14

[Stephane Saux]

Kai will check this in tomorrow.

Comment 15

[Kai Engert [:KaiE:]]

Checked in to branch.

Comment 16

[Kai Engert [:KaiE:]]

It's very good that we fixed this bug.
Somebody @netscape.com just pinged me and showed me a site where this
redirection is indeed used:

http://www.tropilab.com/checkout2.html

And click on the "Click here" link. https link redirecting to http.
The now shown page claims it is on secure site, although it is not!

Luckily, we behave correctly on that site.

Comment 17

[John Unruh]

Verified on branch.