145730 - repeated "encrypted page" alerts

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P3)

Description

[K Chayka]

win32 trunk build 2002051708 (and the 05/19 build), win98se

At the main login page for Wells Fargo online banking, an alert displays that an
encrypted page was requested, as expected.  In the online bill paying area,
virtually every linked page results in 2 alerts - the first is "you are leaving
an encrypted page", the second is "you have requested an encrypted page".   The
expectation is that once logged into the secure site, the encrypted page alerts
shouldn't display within the same banking session.  builds 2002051508 and
2002051204 behave as expected.

Steps to reproduce:
1.  In Prefs - Privacy & Security, turn on SSL warnings
2.  Go to https://banking.wellsfargo.com and log in
3.  Select Bill Pay
4.  Select View Pending Payments - the 2 alerts display.  The expectation is
that they do not.

Observation 1: The repeated alerts do no occur in the "Account Summary" area of
the site.

Observation 2: The Account Summary area of the site uses absolute paths for
virtually every link, i.e.
<form...
ACTION="https://mn1-gx4-ib.banking.wellsfargo.com/cgi-bin/session.cgi?sessargs=<whatever>"
<a
href="https://mn1-gx4-ib.banking.wellsfargo.com/cgi-bin/session.cgi?sessargs=<whatever>"

The Bill Paying area uses many js links and relative paths, i.e.
<form... ACTION="../SummaryInformation/pgUpdateStatus.<whatever>"
<a
href="JavaScript:Validate('bnReviewPayments','bnReviewPayments_onWebEvent(bnReviewPayments,0)','0');">

Comment 1

[Stephane Saux]

cc aruner
We can't investigate this unless we have a log in to wellsfargo.com

Comment 2

[John Unruh]

Reporter, can you try today's build and see if this is still happening?

Comment 3

[K Chayka]

win32 trunk build 061408 - still happening.

Click on a link or submit button that goes to a javascript:"function()" rather
than another https://url seems to be at the root of it.

Immediately after clicking the link, the "you are leaving an encrypted page"
message pops up and the security icon in the status bar switches to unlocked. 
When the next page starts loading, the "you have requested an encrypted page"
message pops up and the icon changes back to locked.

Comment 4

[John Unruh]

Confirming. This appears to be a regression, since I already have an in-house 
test case setup. 
1.) Visit https://pki.mcom.com/tests.html - a secure site.
2.) In the lower left, click on the link "Secure Popup" - 
JavaScript:openNewWindow('https://pki.mcom.com/popup.html','popup',400,400)
What is expected: Since the original page is secure and the popup is secure, 
there should be no warning or change in lock icon status.
What happens: I get a warning stating that I am leaving a secure site, and also 
the lock on the main page changes to a broken lock icon.

Comment 5

[Kai Engert (:KaiE:)]

-> me

Comment 6

[Kai Engert (:KaiE:)]

John, I confirm that I see the same behaviour, and we should look into it.
However, I would like to confirm that both web sites are indeed the same problems.
Your testcases uses JavaScript.

K Chayka: When you have logged in to that site, please do the following: Move
the mouse over one of those links that trigger that double alert - but do not
click it. In the status bar of the window (at the bottom of the window) there
should be information about the link. Does that text start with "JavaScript:"?

Comment 7

[Kai Engert (:KaiE:)]

John, I would like to separate your comment into two different issues:
1.) The "leaving security" issue, which is shown for the currently displayed
(already open) window.
2.) The issue that opening a secure window from a secure window does again show
an "encrypted" alert.

It seems to me 2.) is not a new behaviour. I can see that in old 0.9.4 builds as
well. If you agree and think we should change that, I suggest we could file a
new RFE bug.


I looked into 1.)
The behaviour is indeed a regression and was caused by the patch to bug 144056.
That bug tried to make sure that the lock icon will get reset, whatever content
type will be loaded in the top level document. Obviously, the JavaScript type
should have been filtered out, since no new content gets loaded in that case.

I made a test with a trunk build and having the patch from 144056 reverted, and
the test case did not show a "leaving crypto" warning for the already open page,
and the lock icon stayed secure. (However, the new opened window still shows the
warning / 2.))

Comment 8

[Kai Engert (:KaiE:)]

I should have read this bug more carefully, I realize that K already reported
that JavaScript: links are the cause of the problem. Thanks.

Comment 9

[Kai Engert (:KaiE:)]

Attachment: Patch, but probably too slow?

This patch fixes the problem for me.
It makes the assumption, that we can ignore the progress event triggered by the
JavaScript action. It assumes that we will further events if the JS actually
triggers loading new content into the displayed window. I don't know whether
that assumption can be made or not.

Comment 10

[Kai Engert (:KaiE:)]

The problem with my patch is, that we receive many many progress events, and we
now have to construct and stringcompare("javascript:") the nsIRequest.name of
every request.

I was unable to find a different way to decide, whether the received nsIRequest
is indicating a JS action. bbaetz and biesi on IRC suggested, I could look look
at the URI and use schemeIs - but for URLs like 
  javascript:openNewWindow('https://pki.mcom.com/popup.html','popup',400,400)
I do NOT get "javascript" as the scheme, I get "https"...

Background: The security code is tracking all loading progress, to decide what
the current summarized security status is, in order to show a correct lock icon
state and correct security transition warnings.

Comment 11

[Kai Engert (:KaiE:)]

Attachment: Suggested alternate patch, probably faster?

This patch creates a new (empty) interface that nsJSChannel implements, so we
can QI to it. If we can, we know it's JS.

Comment 12

[Kai Engert (:KaiE:)]

Attachment: Alternate patch, probably faster? (v2)

bbaetz suggested, we nsIJSChannel should inherit from nsIChannel, and
nsJSChannel should inherit only from nsIJSChannel.

Comment 13

[Kai Engert (:KaiE:)]

Attachment: Simpler patch

Actually I made a mistake while trying out my patch. I accidentially looked at
the wrong URI instance :-(

This is a simple patch that uses the approach that was suggested by many
people, checking the scheme for the notified URI in the request, and ignore it
if it is JavaScript. To my testing, if the JS causes the document to change, we
will receive more progress events for the loaded content, and because of that,
ignoring the JS event seems safe to do.

Comment 14

[Kai Engert (:KaiE:)]

Javi, can you please review?

Comment 15

[Darin Fisher]

Comment on attachment 87957 [details] [diff] [review]
Simpler patch

sr=darin

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 87957 [details] [diff] [review]
Simpler patch

r/sr=jst too if you rename aURI to uri. Naming local variables a_something_
conflicts with our naming scheme for method aruments.

Comment 17

[Kai Engert (:KaiE:)]

Attachment: Updated simpler patch, aUri renamed to uri

As requested

Comment 18

[Kai Engert (:KaiE:)]

Comment on attachment 88124 [details] [diff] [review]
Updated simpler patch, aUri renamed to uri

carrying forward reviews

Comment 19

[Kai Engert (:KaiE:)]

Checked in to trunk.

Comment 20

[Kai Engert (:KaiE:)]

John, can you please verify? Can you try to find regressions on JavaScript pages?
Thanks.

Comment 21

[K Chayka]

win32 build 2002061808

Just tried Wells Fargo online banking where I originally experienced this.
No more alerts on js links, just on entering and leaving the secure site.
Groovy :-)

Comment 22

[John Unruh]

Verified on trunk. Looks good to me.

Comment 23

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch, pending
driver's approval. pls check this in asap, then add the "fixed1.0.1" keyword.

Comment 24

[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 25

[Kai Engert (:KaiE:)]

Checked in to 1_0 branch.

Comment 26

[John Unruh]

Verified on 6/23 branch.