mixed http/https warnings after enabling browser.cache.disk_cache_ssl

RESOLVED FIXED

Status

()

defect
P4
normal
RESOLVED FIXED
15 years ago
8 months ago

People

(Reporter: glob, Assigned: Biesinger)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040926 Firefox/0.10
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040926 Firefox/0.10

after enabling browser.cache.disk_cache_ssl i get warnings that a secure site
contains mixed http and https objects.


Reproducible: Always
Steps to Reproduce:
1. enable browser.cache.disk_cache_ssl, enable security.warn_viewing_mixed 
2. visit https://bugzilla.mozilla.org/show_bug.cgi?id=260682
3. right click on banner image, view image
4. notice that while the url displays https, the addressbar is missing the
yellow background and lock image
5. click back, note the addressbar is still missing https indicators
6. click on the bug url link (Bug #: 260682 ..)
7. you should get the warn_viewing_mixed warning
8. reload page, you should get no warning and everything back to normal

Comment 1

14 years ago
Still there for Mozilla 1.7.11 and Seamonkey 1.1a trunk build 2005090105 on
WinXP. Tested with bugzilla pages, if you open a new tab with a Bugzilla page
you'll get the warning dialog.

Comment 2

13 years ago
-> default owner
Assignee: darin → nobody

Comment 3

13 years ago
Still exists in Firefox 2.0.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20061010 Firefox/2.0

Comment 4

13 years ago
Still exists in Firefox 2.0.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20061010 Firefox/2.0

Comment 5

12 years ago
This still exists, and has suddenly become a major problem because of bug 30968 - https://bugzilla.mozilla.org/show_bug.cgi?id=309368

The default behavior of Firefox is now to give spurious encryption errors on any encrypted site. Since any properly-made shopping site will be encrypted, that means that Firefox will cause loss of sales to every e-commerce site visited by Firefox users. The percentage of lost sales for any given site is probably quite small, but added up the amount of money lost to e-commerce is probably a staggeringly large number. 

I should mention that the behavior of ignoring the no-cache directive mentioned in 309368 was not fixed either, so sites can't fix this by using no-cache. It's a browser-side only bug, and it's going to bite every shopping cart on the planet.

This has just gotta be fixed! Or else bug 309368 needs to be reversed (which wouldn't fix anything but it would get the problem hidden back under the rug where it belongs). 

Updated

12 years ago
Blocks: 309368
OS: Windows XP → All
Hardware: PC → All

Comment 6

12 years ago
Okay - I made a dweebish error: Bug 309368 was not changed after all. That the option appeared to be on by default was actually the result of a misunderstanding on my part. However, there is a growing number of people with the option turned on due to a growing number of FF optimization tips sites suggesting turning it on. While these site aren't giving "official" advice, people are still following it. So it's a real-world effect and needs resolution through the fixing of this bug.
From Darin Fisher - what would need to happen to fix this:

the challenge is to serialize SSL certificate info (the result of nsIChannel::securityInfo) to the disk cache so it can be surfaced again via nsICacheEntryDescriptor::securityInfo.  currently, we only do this in the memory cache b/c we can just hold onto the object w/o serializing. 
Assignee: nobody → cbiesinger
Posted patch netwerk/ + xpcom/ part (obsolete) β€” β€” Splinter Review
Attachment #290412 - Flags: superreview?
Attachment #290412 - Flags: review?
Attachment #290412 - Flags: superreview?(darin.moz)
Attachment #290412 - Flags: superreview?
Attachment #290412 - Flags: review?(darin.moz)
Attachment #290412 - Flags: review?
Posted patch PSM part (obsolete) β€” β€” Splinter Review
Includes the patch for bug 405470. Requires the patch for bug 405481.
Attachment #290413 - Flags: superreview?(darin.moz)
Attachment #290413 - Flags: review?(kengert)
requesting blocking, since this blocks bug 345181 (which is already a blocker)
Flags: blocking1.9?

Comment 11

12 years ago
Comment on attachment 290412 [details] [diff] [review]
netwerk/ + xpcom/ part

>Index: netwerk/base/src/nsBase64Encoder.cpp
...
>+nsresult
>+nsBase64Encoder::Finish(nsCSubstring& result)
>+{
>+  char* b64 = PL_Base64Encode(mData.get(), mData.Length(), nsnull);
>+  if (!b64)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+
>+  result.Assign(b64);
>+  PR_Free(b64);
>+  mData.Truncate();

This .Truncate() call is just to free unneeded memory right?  Might be
good to add a comment explaining that.


>Index: netwerk/base/src/nsSerializationHelper.cpp

>+nsresult
>+NS_DeserializeObject(const nsCSubstring& str, nsISupports** obj)
>+{
>+  PRUint32 size = (str.Length() * 3) / 4;
>+  char* buf = PL_Base64Decode(str.BeginReading(), str.Length(), nsnull);
>+  if (!buf)
>+    return NS_ERROR_OUT_OF_MEMORY;

nit: you should add some comment about the 'size' calculation.
there should really be some macro in NSPR to compute that value :)


>Index: netwerk/base/src/nsSerializationHelper.h
...
>+ * Serialize an object to an ASCII string.
>+ */
>+nsresult NS_SerializeToString(nsISerializable* obj,
>+                              nsCSubstring& str);
>+
>+/**
>+ * Deserialize an object.
>+ */
>+nsresult NS_DeserializeObject(const nsCSubstring& str,
>+                              nsISupports** obj);
>+
>+#endif

It seems like there should be some unit tests for these functions :)



>Index: netwerk/cache/src/nsCacheEntry.h

>     nsresult GetSecurityInfo( nsISupports ** result);
>     void     SetSecurityInfo( nsISupports *  info) { mSecurityInfo = info; }
>+    nsISupports* GetSecurityInfo() { return mSecurityInfo; }

might be nice to name the new getter "SecurityInfo" without the Get
prefix to differentiate it from the getter that returns an out param.


>Index: netwerk/cache/src/nsDiskCacheEntry.cpp

>+    // Restore security info, if present
>+    const char* info = entry->GetMetaDataElement("security-info");
>+    if (info) {
>+      nsCOMPtr<nsISupports> infoobj;
>+      NS_DeserializeObject(nsDependentCString(info), getter_AddRefs(infoobj));
>+      entry->SetSecurityInfo(infoobj);

nit: 4 whitespace indent

nit: infoObj instead of infoobj?


>+    // Store security info, if it is serializable
>+    nsCOMPtr<nsISerializable> serializable =
>+        do_QueryInterface(entry->GetSecurityInfo());
>+    if (serializable) {
>+      nsCString info;
>+      NS_SerializeToString(serializable, info);
>+      entry->SetMetaDataElement("security-info", info.get());
>+    }

nit: 4 whitespace indent
Attachment #290412 - Flags: review?(darin.moz) → review+

Updated

12 years ago
Attachment #290412 - Flags: superreview?(darin.moz) → superreview+

Comment 12

12 years ago
Comment on attachment 290413 [details] [diff] [review]
PSM part

>Index: security/manager/ssl/src/nsNSSCallbacks.cpp
...
>     /* Set the SSL Status information */
>-    nsCOMPtr<nsSSLStatus> status;
>-    infoObject->GetSSLStatus(getter_AddRefs(status));
>+    nsRefPtr<nsSSLStatus> status;
>+    infoObject->GetSSLStatus(reinterpret_cast<nsISupports**>(
>+                               static_cast<void**>(getter_AddRefs(status))));

nit: since you have a pointer to the nsNSSSocketInfo object, perhaps it
would be nicer to just add a simple getter on nsNSSSocketInfo that
returns the nsSSLStatus pointer?  maybe nsNSSSocketInfo::SetSSLStatus
should even take a nsSSLStatus pointer instead of a nsISSLStatus pointer?

otherwise, sr=me
(In reply to comment #11)
> This .Truncate() call is just to free unneeded memory right?  Might be
> good to add a comment explaining that.

I was kind of thinking that someone might also want to reuse the encoder for another encoding... Finish() sounds to me like it should clear the state to allow that.

> >Index: netwerk/base/src/nsSerializationHelper.cpp
> nit: you should add some comment about the 'size' calculation.
> there should really be some macro in NSPR to compute that value :)

Yeah, I'll add that comment. The computation is from the comment in plbase64.h... Really, PL_Base64Decode ought to return the real count.

> >Index: netwerk/base/src/nsSerializationHelper.h
> It seems like there should be some unit tests for these functions :)

Yes, but I can't link to component libraries :/

> might be nice to name the new getter "SecurityInfo" without the Get
> prefix to differentiate it from the getter that returns an out param.

OK. In parts of the tree, a missing Get prefix indicates the method doesn't return null, but I guess we don't really do that in necko.

Posted patch netwerk/ + xpcom/ part, v2 (obsolete) β€” β€” Splinter Review
Attachment #290412 - Attachment is obsolete: true
Posted patch PSM part, v2 (obsolete) β€” β€” Splinter Review
transferring sr=darin
Attachment #290413 - Attachment is obsolete: true
Attachment #290549 - Flags: superreview+
Attachment #290549 - Flags: review?(kengert)
Attachment #290413 - Flags: superreview?(darin.moz)
Attachment #290413 - Flags: review?(kengert)

Comment 16

12 years ago
Comment on attachment 290544 [details] [diff] [review]
netwerk/ + xpcom/ part, v2

I only looked at nsBase64Encoder.{h,cpp}.  I'm surprised that
Mozilla doesn't yet have a base64 encoder class.  Perhaps it
has been calling PL_Base64Encode directly?

In nsBase64Encoder.h

>+ * A base64 encoder. Usage: Instantiate class, write to it using
>+ * nsIOutputStream methods, then call Finish() to get the base64-encoded data.

Since you didn't implement WriteFrom and WriteSegments, perhaps you
should just say "write to it using Write()".

>+    /// The data written to this stream. nsCString has can deal fine with

Remove "has".

In nsCacheEntry.h

>     nsresult GetSecurityInfo( nsISupports ** result);
>     void     SetSecurityInfo( nsISupports *  info) { mSecurityInfo = info; }
>+    nsISupports* SecurityInfo() { return mSecurityInfo; }

Why do you need two getters for mSecurityInfo?  Is it for convenience?
Can we remove the original getter?  Would be nice to list the two getters
together.
Flags: wanted1.9+
Flags: blocking1.9?
Flags: blocking1.9-
Flags: wanted1.9+
Flags: blocking1.9-
Flags: blocking1.9+
Priority: -- → P4
Comment on attachment 290549 [details] [diff] [review]
PSM part, v2

Patch looks good to me, r=kengert, but please address the following.

+  {
+    "NSS IO Layer",
+    NS_NSSIOLAYER_CID,
+    nsnull,
+    nsNSSSocketInfoConstructor
   }

Could you please use something like

+  {
+    "NSS Socket Info",
+    NS_NSSSOCKETINFO_CID,
+    nsnull,


(I wish PSM wouldn't call its own I/O layer the NSS I/O layer, because both PSM and NSS use separate I/O layers...)
Attachment #290549 - Flags: review?(kengert) → review+
Posted patch PSM part, v3 β€” β€” Splinter Review
Attachment #290549 - Attachment is obsolete: true
(In reply to comment #16)
> (From update of attachment 290544 [details] [diff] [review])
> I only looked at nsBase64Encoder.{h,cpp}.  I'm surprised that
> Mozilla doesn't yet have a base64 encoder class.  Perhaps it
> has been calling PL_Base64Encode directly?

Yes, it's been calling it directly.

> Why do you need two getters for mSecurityInfo?  Is it for convenience?
> Can we remove the original getter?  Would be nice to list the two getters
> together.

Good point, I'll remove GetSecurityInfo.
Attachment #290544 - Attachment is obsolete: true

Updated

12 years ago
Attachment #290739 - Flags: review+
checked in
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Depends on: 409500
You need to log in before you can comment on or make changes to this bug.