409174 - View page info -> Security: doesn't show cookies were set for sites with ip:8080 address

Status: VERIFIED FIXED

(Firefox :: Page Info Window, defect)

Description

[Fedge]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.9) Gecko/20071025 Firefox/2.0.0.9
Build Identifier: 3.0b3pre

Just noticed working on my own test server that view cookies won't show the cookies I set server-side. If I go through the Options dialog I can see them, but in Page info they do not show up.

Reproducible: Always

Steps to Reproduce:
1. Go to http://142.59.78.134:8080/
2. Look in View page info -> Security -> View cookies
3. Says no cookies were set when there are
Actual Results:  
Confused me for a second as to why my sessions were working!

Expected Results:  
Would like to see cookies I set, even on port 8080

Comment 1

[[e-mail]]

confirmed. there are actually two bugs here:
1) pageinfo/security tab says there are no stored cookies.
2) clicking 'view cookies' prefills the cookieviewer search bar with "78.134:8080", which is why no matches are returned.

the call that determines 1) is at:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/browser/base/content/pageinfo/security.js&rev=1.13&mark=233#233
probably what's happening is that info.hostName includes the port, which the cookieservice doesn't expect. thus it won't return any matches. this needs to use a raw host string here, and it needs to be ACE encoded if it's IDN. we could either roll an nsIURI out of it and then get the ASCII host, which will "just work"(tm), or maybe the |info| object has what we need...

2) is probably related:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/browser/base/content/pageinfo/security.js&rev=1.13&mark=143#143
the eTLD service might be choking on the port also and treating it like a regular domain, giving us back the last two chunks. i'm saddened NSPR choked so easily on that. :(

cc'ing ronny and johnath since their work in bug 378668 is intimately related (also see bug 378668 comment 41).

Comment 2

[[e-mail]]

uh, so now i'm confused: http://developer.mozilla.org/en/docs/DOM:window.location says |hostName| isn't supposed to include the port. so why is it?

Comment 3

[[e-mail]]

oh, my bad, _getSecurityInfo() just gets |host| but calls the return variable |hostName|.

we should just get the whole spec and roll our own nsIURI out of it, use |uri.asciiHost| for the cookie call, and pass the URI wholesale into eTLD.

Comment 4

[Ronny Perinke]

(In reply to comment #3)
> we should just get the whole spec and roll our own nsIURI out of it, use
> |uri.asciiHost| for the cookie call, and pass the URI wholesale into eTLD.
> 

Or we rename _getSecurityInfo().hostName to _getSecurityInfo().host and add _getSecurityInfo().hostName with that what the name suggest, the hostname gWindow.location.hostname.
This involves changes in /security/manager/pki/resources/content/PageInfoOverlay.xul.

Changing _getSecurityInfo().hostName to gWindow.location.hostname without the rest would give more search results than expected for the passwords filter thing, because the port is saved too. Let's say your are visiting 1.2.3.4:5678 and have also stored passwords for 1.2.3.4:5679. Without the port you would get passwords for both. Ok, it doesn't greatly matter but I just wanted to mention it.

Comment 5

[Florian Quèze [:florian]]

Hmm... I would prefer that the viewCookies function does not call _getSecurityInfo() (and anyway it should not be called twice from the same function).

You can get the URI with gDocument.documentURIObject.

Comment 6

[[e-mail]]

Attachment: like so

thanks florian! i thought we'd have a document URI floating about somewhere :)

this tweaks the cookie and login indicators to use said URI, instead of mucking things up; and fixes the eTLD call to do the right thing with IP addresses. i've tested this with the reporter's testcase, and with regular sites, and it works fine.

of course, i'm assuming here that gDocument.documentURIObject is always the URI we want. also, the |if (!uri)| nullchecks in those functions might not be necessary; i'm not sure.

florian, care to review? ;)

Comment 7

[Florian Quèze [:florian]]

I think you don't need the |if (!uri)| nullchecks. You may want to nullcheck uri.asciiPath but apparently both cookieManager.countCookiesFromHost and passwordManager.countLogins don't care if you give them an empty string.

>+  return passwordManager.countLogins(uri.prePath, "", "");

Are you sure about this? When I look in nsILoginManager.idl the parameters of countLogins are aHostname, aActionURL and aHttpRealm.
But your patch doesn't really change this code, so maybe this is more a question for Johnath.

Comment 8

[Justin Dolske [:Dolske]]

The .countLogins call is right, the empty args are treated as wildcards because the dialog is counting logins for the whole site (so limiting the scope to a specific form submit URL or realm isn't needed).

Comment 9

[[e-mail]]

(In reply to comment #7)
> I think you don't need the |if (!uri)| nullchecks.

ok, consider them removed - i'll fix that up on checkin.

Comment 10

[Florian Quèze [:florian]]

(In reply to comment #8)
> The .countLogins call is right, the empty args are treated as wildcards because
> the dialog is counting logins for the whole site (so limiting the scope to a
> specific form submit URL or realm isn't needed).
> 

Yeah I saw that the empty strings were right. I was more wondering about giving prePath to a parameter called aHostname. I should have been more clear in my previous comment, sorry.

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 294185 [details] [diff] [review]
like so

This looks good to me.  I'm not a browser/ peer though ;-).

Comment 12

[Johnathan Nightingale [:johnath]]

(In reply to comment #10)
> (In reply to comment #8)
> > The .countLogins call is right, the empty args are treated as wildcards because
> > the dialog is counting logins for the whole site (so limiting the scope to a
> > specific form submit URL or realm isn't needed).
> > 
> 
> Yeah I saw that the empty strings were right. I was more wondering about giving
> prePath to a parameter called aHostname. I should have been more clear in my
> previous comment, sorry.

Man, go away for the holidays and all kinds of fun things happen.  Sorry I'm late to the party here guys.

TTBOMK, prePath is the right thing to provide here, since pwmgr stores scheme://host:port for each entry.  But dolske would know for sure.  The "hostname" parameter is a little misleading, you're right, we might want to update the IDL to reflect its intended usage.  But on the other hand, given my host/hostName ickyness documented above, I can hardly throw stones.

Comment 13

[Florian Quèze [:florian]]

(In reply to comment #12)

> TTBOMK, prePath is the right thing to provide here

Yeah, I saw that when looking at the tests.

> The "hostname" parameter is a little misleading, you're right, we might 
> want to update the IDL to reflect its intended usage.

We may want to file another bug to improve the comments in the IDL files for this parameter. Justin, do you think it's worth it?

Comment 14

[Justin Dolske [:Dolske]]

(In reply to comment #12)

> TTBOMK, prePath is the right thing to provide here, since pwmgr stores
> scheme://host:port for each entry.  But dolske would know for sure.  

Without rereading context here, .prePath is basically what you want, but has the annoying problem of including the username/password when present. ("http://user:[e-mail]"). That's why nsLoginMananger._getPasswordOrigin() constructs it from scheme+host+port.

> The
> "hostname" parameter is a little misleading, you're right, we might want to
> update the IDL to reflect its intended usage.

Sure, I guess. Looks like countLogins() is the only interface where that isn't really mentioned. The others have it, as does nsILoginInfo.idl. Also on http://developer.mozilla.org/en/docs/Using_nsILoginManager. :-)

Comment 15

[Asaf Romano (gone)]

Comment on attachment 294185 [details] [diff] [review]
like so


> /**
>- * Return true iff we have cookies for hostName
>+ * Return true iff we have cookies for uri
>  */
>-function hostHasCookies(hostName) {
>-  if (!hostName)
>+function hostHasCookies(uri) {
>+  if (!uri)
>     return false;
>   
>   var cookieManager = Components.classes["@mozilla.org/cookiemanager;1"]
>                                 .getService(Components.interfaces.nsICookieManager2);
> 
>-  return cookieManager.countCookiesFromHost(hostName) > 0;
>+  return cookieManager.countCookiesFromHost(uri.asciiHost) > 0;
> }
> 
> /**
>- * Return true iff realm (proto://host:port) (extracted from location) has
>+ * Return true iff realm (proto://host:port) (extracted from uri) has
>  * saved passwords
>  */
>-function realmHasPasswords(location) {
>-  if (!location) 
>+function realmHasPasswords(uri) {
>+  if (!uri) 
>     return false;
>   
>-  var realm = makeURI(location).prePath;
>   var passwordManager = Components.classes["@mozilla.org/login-manager;1"]
>                                   .getService(Components.interfaces.nsILoginManager);
>-  return passwordManager.countLogins(realm, "", "");
>+  return passwordManager.countLogins(uri.prePath, "", "");

Please add a  !=/> 0 here as well (0 !== false).

r=mano otherwise.

Comment 16

[[e-mail]]

Comment on attachment 294185 [details] [diff] [review]
like so

will do! requesting approval - fixes broken pageinfo cookieviewer behavior.

Comment 18

[Mike Connor [:mconnor]]

Comment on attachment 294185 [details] [diff] [review]
like so

a=mconnor on behalf of drivers for 1.9 checkin

Comment 19

[[e-mail]]

fixed.

Comment 20

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2008010904 Minefield/3.0b3pre. I verified by testing the site in the URL.