381692 - Page Info Security section should show site-specific saved passwords

Status: VERIFIED FIXED

(Firefox :: Page Info Window, defect)

Description

[(no longer active)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3 Creative ZENcast v1.02.10
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a4) Gecko/20070427 GranParadiso/3.0a4

The new Page Info dialog contains a Security section which tells users whether a site has stored cookies on the users' computers and/or has Firefox saved a password for the site.  In addition, there are the "View Cookies" and "View Saved Passwords" buttons, which I think the user expects to show site-specific cookies and saved passwords (instead of simply showing *all* cookies and saved passwords).  However, the current implementation in FF3.0a4 is the same to the equivalent buttons in the Options dialog.

Reproducible: Always

Steps to Reproduce:
1. Invoke the Page Info dialog for a page.
2. Navigate to the Security section.
3. Click either View Cookies or View Saved Passwords.



about:buildconfig

Build platform
target
i686-pc-cygwin

Build tools
Compiler 	Version 	Compiler flags
$(CYGWIN_WRAPPER) cl 	14.00.50727 	-TC -nologo -W3 -Gy -Fd$(PDBFILE)
$(CYGWIN_WRAPPER) cl 	14.00.50727 	-GR- -TP -nologo -Zc:wchar_t- -W3 -Gy -Fd$(PDBFILE)

Configure arguments
--enable-application=browser --enable-update-channel=beta --enable-optimize --disable-debug --disable-tests --enable-static --disable-shared --enable-svg --enable-canvas --enable-default-toolkit=cairo-windows --enable-update-packaging --with-branding=browser/branding/unofficial

Comment 1

[:Gavin Sharp [email: [e-mail]]]

I believe the cookie part of this request is addressed bug 378668. The password part has no bug that I know of, but requires a fix to bug 327048. Perhaps we could morph this bug to cover the password part of the request?

Comment 2

[(no longer active)]

As suggested in comment 1, this bug is modified to cover the password part of the request.  I've also set a dependency on bug 327048, and I'm taking over that bug.  I think the fix to this bug would be very similar to that of bug 378668.

Comment 3

[Mike Connor [:mconnor]]

This seems more like wanted, not blocking... renominating

Comment 4

[(no longer active)]

Taking over.  I'll post a patch when the fix for bug 327048 lands.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

When 327048 lands, this will become blocking, but for now it has to be wanted since the dependency doesn't block (and is a P2 on the PRD).

Makes me sad, but mconnor convinced me.

Comment 6

[(no longer active)]

Requesting blocking now that bug 327048 landed, as per comment 5...

Comment 7

[(no longer active)]

Let's see if we can land this for M9...

Comment 8

[(no longer active)]

Got to be in Litmus test suite...

Comment 9

[(no longer active)]

Attachment: Patch (v1)

Real simple patch, similar to the patch accepted for the cousin bug 378668.

Comment 10

[(no longer active)]

Attachment: Patch (v1.1)

Oops, caught a nit myself on trailing spaces!  :D

Comment 12

[Ronny Perinke]

Attachment: include fix from bug #378668 comment #31

window.arguments[0] is null when opening the passwords-manager via pref dialog and therefore no passwords would be shown.

Comment 13

[(no longer active)]

Attachment: Patch (v2) (including fix from bug #378668 comment #31)

(In reply to comment #12)
> Created an attachment (id=293310) [details]
> include fix from bug #378668 comment #31
> 
> window.arguments[0] is null when opening the passwords-manager via pref dialog
> and therefore no passwords would be shown.

Good catch Ronny, but the filtering needs to be done after the table is displayed...  This new patch addresses the issue you mentioned correctly.

Comment 14

[Ronny Perinke]

(In reply to comment #13)
> Good catch Ronny, but the filtering needs to be done after the table is
> displayed...  This new patch addresses the issue you mentioned correctly.
> 

*gnarf* of course ... but wouldn't it be smarter to apply this pre-filter only once? Now you are also calling _filterPasswords (through setFilter) when SignonClearFilter() is called, because it calls LoadSignons() too. But who calls SignonClearFilter? _filterPasswords!
So A calls B which again calls A ...
You could end in an endless loop. The other thing is: if manually deleted the search-field, it would then be refilled by setFilter, because the window-argument is still there. So you can not clear the filter anymore, when you opened the pw-manager from the page info dialog!

Here is the part from my version of the patch, which I made for the duplicate bug 408489.


 function SignonsStartup() {
   kSignonBundle = document.getElementById("signonBundle");
   document.getElementById("togglePasswords").label = kSignonBundle.getString("showPasswords");
   document.getElementById("togglePasswords").accessKey = kSignonBundle.getString("showPasswordsAccessKey");
   LoadSignons();
   FocusFilterBox();
+  if (window.arguments && window.arguments[0] && window.arguments[0].filterString)
+    setFilter(window.arguments[0].filterString);
 }

Comment 15

[Asaf Romano (gone)]

Comment on attachment 293313 [details] [diff] [review]
Patch (v2) (including fix from bug #378668 comment #31)

r=mano, needs ui-r.

Comment 16

[Ronny Perinke]

(In reply to comment #15)
> (From update of attachment 293313 [details] [diff] [review])
> r=mano, needs ui-r.
> 

IMO not a good decision.

I know my writings are sometimes confusing, so let me try to make it clear again.
If this patch lands, users will not able to clear the filter (clearing the search-field either using the clear-button or by removing the text), when the password manager dialog has been previously opened from the page info dialog with a non-empty filterString as argument to window.open().

LoadSignons() gets called during startup (of the dialog) and in SignonClearFilter(), when the user clears the filter. In this case LoadSignons() will override the empty value of the search-field with the filterString-argument. If it's empty too, ok, but else ...

Comment 17

[Ronny Perinke]

Attachment: patch v3

addressing the issue mentioned in comment #16

Comment 18

[(no longer active)]

Attachment: Patch (v4)

Mano: Ronny is right in comment 16.  This patch pre-filters the table only on startup.  Otherwise attachment 293341 [details] [diff] [review] is the same to my previous patch (attachment 293313 [details] [diff] [review]).  Thanks to Ronny for spotting this problem.

Another issue that my patch has was that setFilter() would get called both at startup and when the dialog is already open, so we need to check if there's already a filter string present, and clear the filter before setting it again in setFilter().

I'm also asking ui-r on this patch.

Comment 19

[(no longer active)]

Comment on attachment 293313 [details] [diff] [review]
Patch (v2) (including fix from bug #378668 comment #31)

Setting r- because of the serious problem produced by this patch, as mentioned in comment 16.

Comment 20

[Asaf Romano (gone)]

Comment on attachment 293359 [details] [diff] [review]
Patch (v4)

r=mano

Comment 21

[Florian Quèze [:florian]]

Comment on attachment 293359 [details] [diff] [review]
Patch (v4)

>Index: browser/base/content/pageinfo/security.js
>===================================================================

>+			{filterString : this._getSecurityInfo().hostName});

Nit: Would be nice if the tabs on this line could be replaced by spaces before landing.

Comment 22

[(no longer active)]

Attachment: Patch (v4.1)

(In reply to comment #21)
> (From update of attachment 293359 [details] [diff] [review])
> >Index: browser/base/content/pageinfo/security.js
> >===================================================================
> 
> >+			{filterString : this._getSecurityInfo().hostName});
> 
> Nit: Would be nice if the tabs on this line could be replaced by spaces before
> landing.

Nit addressed.

Comment 23

[(no longer active)]

Ping...

Comment 24

[Florian Quèze [:florian]]

(In reply to comment #15)
> (From update of attachment 293313 [details] [diff] [review])
> r=mano, needs ui-r.
> 

Mano, do you really think we need to wait for ui-r here?  As the patch does exactly what the summary says, I would think comment 5 is enough and this only needs approval.

Comment 25

[Johnathan Nightingale [:johnath]]

Comment on attachment 293631 [details] [diff] [review]
Patch (v4.1)

I can ui-r sec-ui-related things like this, and as Florian points out, we could probably get away without it anyhow.  :)

Comment 26

[(no longer active)]

Comment on attachment 293631 [details] [diff] [review]
Patch (v4.1)

Asking for approval...

(According to the comment 5, this should be considered a blocker now.)

Comment 27

[Reed Loden [:reed]]

Checking in toolkit/components/passwordmgr/content/passwordManager.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/content/passwordManager.js,v  <--  passwordManager.js
new revision: 1.22; previous revision: 1.21
done
Checking in browser/base/content/pageinfo/security.js;
/cvsroot/mozilla/browser/base/content/pageinfo/security.js,v  <--  security.js
new revision: 1.14; previous revision: 1.13
done

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008010504 Minefield/3.0b3pre

Comment 29

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=5295 added to Litmus.