345181 - Firefox ignores "Cache-Control: public" header on TLS connections

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P1)

Description

[u39892]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4

Firefox does not appear to cache responses sent with "Cache-Control: public" header, if they are sent over SSL/TLS. While I'm aware that Firefox generally doesn't cache responses over TLS for security, in cases where the response is explicitely defined to be public, this would seem to be incorrect behaviour. I'm testing caching behaviour by use of the "Cache Source" entry from the page info window.

For reference, we're using TLS in this instance to ensure integrity, rather than security (it's important that the user receives the response we're sending, it's not important if anyone else can read that).


Reproducible: Always

Steps to Reproduce:
1. Go to https://bowmore.dcs.st-andrews.ac.uk/mms/images/uparrow.png
2. Right click -> View Page Info
Actual Results:  
Cache source is listed as "Not cached".

Expected Results:  
Cache source should be "Disk cache". For example, go to
http://bowmore.dcs.st-andrews.ac.uk/mms/images/uparrow.png and then View Page Info.

Comment 1

[Steve England [:stevee]]

Reporter, do you still see this problem with the latest Firefox 2? If not, can you please close this bug as WORKSFORME. Thanks!

Comment 2

[u39892]

Still seeing same problem with 2.0.4 on OS X.

Comment 3

[u39892]

Just tested with 3.0a5 and present there too, if that's relevant.

Comment 4

[Steve England [:stevee]]

Yes it is relevant, thanks!

Comment 5

[Daniel Veditz [:dveditz]]

Confirming bug, moving to networking component.

Couldn't load the reporter's file, but tested with URIs from my webmail which can be loaded over SSL or not.

The spec says Cache-Control: public "MAY" be cached, not MUST or SHOULD. We're not violating a spec, but we're also not taking advantage of optimization hints the site is giving us.
http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9.1

bug 205921 added a pref that puts SSL files in the cache, but that stores more than just the explicitly "public" pages. Plus no one's going to find and use that pref, Firefox should "just work".

Unless that pref is enabled we go ahead and set the load flag INHIBIT_PERSISTENT_CACHING on all SSL loads, overriding any Cache-Control header.

Comment 6

[Daniel Veditz [:dveditz]]

Adding "perf" keyword: this would speed loading of sites that use SSL to protect user-data, but also load large static resources that are not private (logos/images, stylesheets, scripts). If we're going to cache any sites at all, a site where a user has sensitive data stored is one they're likely to return to so caching what we can safely would be a win.

Comment 7

[Mike Schroepfer]

Moving to blocking for 1.9 as this might be a significant perf win...

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

should I add a pref for this?

Comment 9

[Darin Fisher]

Comment on attachment 290421 [details] [diff] [review]
patch

>Index: netwerk/protocol/http/src/nsHttpChannel.cpp
>+    // Only cache SSL content on disk if the server sent a
>+    // Cache-Control: public header, or if the user set the pref
>+    if (!gHttpHandler->IsPersistentHttpsCachingEnabled() &&
>+        mConnectionInfo->UsingSSL() && !mResponseHead->CacheControlPublic())
>+        mLoadFlags |= INHIBIT_PERSISTENT_CACHING;

It seems like you want to always set INHIBIT_PERSISTENT_CACHING if
IsPersistentHttpCachingEnabled() returns false.  So, I think you want this
condition instead:

  if (!gHttpHandler->IsPersistentHttpsCachingEnabled() || (
      mConnectionInfo->UsingSSL() && !mResponseHead->CacheControlPublic()))

Otherwise, this change looks good to me.  r+sr=darin with that fix.

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Well, those aren't the semantics I wanted to implement. I wanted:

- If the pref is set, cache all SSL to disk (except no-store)
- otherwise, cache only Cache-Control: public SSL content

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

per discussion with darin, I'll rename IsPersistentHttpsCachingEnabled to CanCacheAllSSLContent

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in nsHttpChannel.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,v  <--  nsHttpChannel.cpp
new revision: 1.324; previous revision: 1.323
done
Checking in nsHttpHandler.h;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpHandler.h,v  <--  nsHttpHandler.h
new revision: 1.53; previous revision: 1.52
done
Checking in nsHttpResponseHead.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpResponseHead.cpp,v  <--  nsHttpResponseHead.cpp
new revision: 1.48; previous revision: 1.47
done
Checking in nsHttpResponseHead.h;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpResponseHead.h,v  <--  nsHttpResponseHead.h
new revision: 1.28; previous revision: 1.27
done

Comment 14

[Daniel Veditz [:dveditz]]

Is this worth taking on the 1.8 branch? We'd also need the fixes for minor regression 406869 and for bug 262116 which starts mattering if caching is the default.

Comment 15

[Denis Altudov]

It would be really nice if this got fixed in FF 2.0

As a result of this bug I have to dial-down security in my application in order to  allow reasonable load times, which in return reduces security for the users. 

Please consider this!
Thanks.

Comment 16

[Daniel Veditz [:dveditz]]

Not going to get a branch patch before the cut-off, and not clear this fits our branch criteria anyway. Will look in the next release.

Comment 17

[Mike Schroepfer]

(In reply to comment #16)
> Not going to get a branch patch before the cut-off, and not clear this fits our
> branch criteria anyway. Will look in the next release.
> 

Agreed - we should be branch landing only critical security and stability issues - this is a perf win and there are hundreds of others in FF3 that are not easily backported.  But don't worry - folks will upgrade to FF3 quickly...

Comment 18

[Daniel Veditz [:dveditz]]

Minusing for the 1.8 branch, although nice to have it doesn't really fit the criteria.