282784 - [FIXr]Warning appears twice when submitting unencrypted form data

Status: VERIFIED FIXED

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

Description

[Daniel C]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b) Gecko/20050218 Firefox/1.0+
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b) Gecko/20050218 Firefox/1.0+

On certain pages with forms, when submitting them, the warning about submitting
unencrypted form data is shown. When 'OK' is pressed, the dialogue disappears,
but then another exact copy reappears. After pressing 'OK' a second time, the
form is submitted. For this to occur, security.warn_submit_insecure must be set
to true, and security.warn_submit_insecure.show_once must be set to false.

Reproducible: Always

Steps to Reproduce:
1. Check your settings in about:config as specified above.
2. Load up the testcase.
3. Submit the form.
4. Select 'OK' in the subsequent warning.
Actual Results:  
The warning appears again. Selecting OK a second time submits the form and no
further warnings are displayed.

Expected Results:  
Only shown the warning once, then submitted the form after pressing 'OK' once.

This occurs with a default profile except for the 2 settings specified above.

Observed with latest trunk in Gentoo Linux and Windows XP.

Attaching testcase...

Comment 1

[Daniel C]

Attachment: Testcase

Testcase. Follow the instructions in the first post to observe the bug.

Comment 2

[Daniel C]

Attachment: Testcase

Testcase. Follow instructions in first post to observe bug.

Comment 3

[Daniel C]

OK, the testcases won't demonstrate the bug because the attachments are on this
secure server. Download the first attachment, and run it locally.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Worksforme with a Seamonkey build from the exact same day.  Just tested with a
Firefox and SeaMonkey built from the same tree, and this is Firefox-only.

Leaving in here for now in case this is an issue in
nsSecurityWarningDialogs.cpp, since the show_once stuff is only done by Firefox,
but ccing some Firefox folks.

DJC, if you get a chance to narrow down when (and whether) this regressed, that
would be most helpful.

Comment 5

[Daniel C]

(In reply to comment #4)

I have tested today for this bug on all the Firefox builds I happen to have on
my system (Linux) at the moment. They happen to be 20050126, 20050218 and
20050223. This bug occurs for me in all 3. The EXACT steps I followed to
reproduce are:

Firstly, save Testcase 1 to your computer.
Start Firefox with the parameters: -safe-mode and -profilemanager.
Create a brand new profile.
Select that profile and start Firefox.
Open the testcase from your computer. I used File -> Open File...
Click the button in that page.
As it's the first time you've used this profile, it should warn you about
submitting unencryped form data (show_once.)
Check the checkbox, and click OK.
When I do this, the warning appears again.

It then appears twice each time I test this bug, or nearly any other form. I
should be able to test on 3 Windows builds tomorrow, although I know I have
definitely witnessed it there.

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK, this is an issue for Firefox 1.0 as well....

That makes it sound like it's just never really worked right.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So.. in Firefox the nsSecureBrowserUIImpl is present twice in the form observer
list.  This happens because nsSecureBrowserUIImpl::Init is called twice on the
same nsSecureBrowserUIImpl object.  This should probably be asserting, since it
leads to incorrect behavior.  kaie, thoughts on that?

The first call comes from the "browser" binding constructor.  Presumably
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/browser.xml&rev=1.53&mark=548#547
is where that happens.  That's been in place since rev 1.1 of the file.

The second call comes from an onload handler.  To be precise, it comes from
prepareForStartup() in browser.js:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.js&rev=1.386&mark=696#693

So looks like this has just never worked (both pieces of code have been in place
since fall 2002).

Over to Firefox.  Raising severity to critical, since this also adds the object
twice as a web progress listener, which may well confuse the lock icon code. 
There seems to be no "security" component, so I guess I'll dump this in general
and hope someone notices...

Comment 8

[Kai Engert (:KaiE:)]

> kaie, thoughts on that?

I have never used FireFox, I only use the suite.
If I understand correctly, the show_once is a FireFox specific feature, and this
bug only occurs with FireFox.

I agree an assertion is a good idea.

However, we could make the code more clever.

mWindow is only assigned a value in the init code.

If mWindow already carries a value, we could skip executing the init code again
(and produce an assertion only).

Comment 9

[Kai Engert (:KaiE:)]

Attachment: protective patch v1

like this

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch for secure browser UI

Well, no reason to assert then.  Just throw NS_ERROR_ALREADY_INITIALIZED if
mWindow is already set.

Note that this patch breaks the firefox UI pretty badly (since browser.js ends
up with an exception and stuff doesn't get set up correctly).  So we'll have to
wait on the front-end to be fixed to check this in....

Comment 11

[Kai Engert (:KaiE:)]

Comment on attachment 175548 [details] [diff] [review]
Patch for secure browser UI

r=kaie

Comment 12

[Kai Engert (:KaiE:)]

Then we should probably have a separate firefox-frontend-change-request-bug and
make it block this one.

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 175548 [details] [diff] [review]
Patch for secure browser UI

sr=jst

Comment 14

[Daniel Veditz [:dveditz]]

Apparently it *hasn't* ever worked right, found bug 180962 which has a patch to
comment out one of the inits.

Comment 15

[Boris Zbarsky [:bzbarsky]]

OK, I guess this is now the PSM bug on making it impossible to make this mistake
again...

Daniel, could we try to get bug 180962 in for 1.8/1.1?

Comment 16

[chris hofmann]

lets try to get this and bug 180962 for 1.1

Comment 17

[Boris Zbarsky [:bzbarsky]]

Per Asa's n.p.m.seamonkey post, this should probably go into b2 (being a bit of
an api change and all).

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 175548 [details] [diff] [review]
Patch for secure browser UI

Requesting approval for 1.8b2, now that bug 180962  is fixed.

Comment 19

[Asa Dotzler [:asa]]

Comment on attachment 175548 [details] [diff] [review]
Patch for secure browser UI

a=asa

Comment 20

[Boris Zbarsky [:bzbarsky]]

Fixed.