Last Comment Bug 345181 - Firefox ignores "Cache-Control: public" header on TLS connections
: Firefox ignores "Cache-Control: public" header on TLS connections
Status: RESOLVED FIXED
: perf
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: Trunk
: All All
: P1 normal with 1 vote (vote)
: ---
Assigned To: Christian :Biesinger (don't email me, ping me on IRC)
:
Mentors:
https://bowmore.dcs.st-andrews.ac.uk/...
Depends on: 262116 406869
Blocks: 420310
  Show dependency treegraph
 
Reported: 2006-07-19 05:53 PDT by James Ross Nicoll
Modified: 2008-07-09 11:55 PDT (History)
14 users (show)
mtschrep: blocking1.9+
dveditz: wanted1.8.1.x-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (7.28 KB, patch)
2007-11-27 11:25 PST, Christian :Biesinger (don't email me, ping me on IRC)
darin.moz: review+
darin.moz: superreview+
Details | Diff | Review
patch v2 (8.47 KB, patch)
2007-11-28 08:49 PST, Christian :Biesinger (don't email me, ping me on IRC)
no flags Details | Diff | Review

Description James Ross Nicoll 2006-07-19 05:53:30 PDT
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] 2007-06-12 08:28:17 PDT
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 James Ross Nicoll 2007-06-12 08:35:45 PDT
Still seeing same problem with 2.0.4 on OS X.
Comment 3 James Ross Nicoll 2007-06-12 08:43:21 PDT
Just tested with 3.0a5 and present there too, if that's relevant.
Comment 4 Steve England [:stevee] 2007-06-12 08:44:28 PDT
Yes it is relevant, thanks!
Comment 5 Daniel Veditz [:dveditz] 2007-10-11 02:36:37 PDT
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] 2007-10-11 02:42:49 PDT
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 2007-11-01 14:10:40 PDT
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) 2007-11-27 11:25:47 PST
Created attachment 290421 [details] [diff] [review]
patch

should I add a pref for this?
Comment 9 Darin Fisher 2007-11-27 11:40:07 PST
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) 2007-11-27 11:52:35 PST
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) 2007-11-27 12:53:15 PST
per discussion with darin, I'll rename IsPersistentHttpsCachingEnabled to CanCacheAllSSLContent
Comment 12 Christian :Biesinger (don't email me, ping me on IRC) 2007-11-28 08:49:21 PST
Created attachment 290542 [details] [diff] [review]
patch v2
Comment 13 Christian :Biesinger (don't email me, ping me on IRC) 2007-11-30 10:09:18 PST
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] 2008-02-11 10:50:02 PST
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 2008-04-17 22:19:48 PDT
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] 2008-06-05 14:32:42 PDT
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 2008-06-05 17:46:52 PDT
(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] 2008-07-09 11:55:57 PDT
Minusing for the 1.8 branch, although nice to have it doesn't really fit the criteria.

Note You need to log in before you can comment on or make changes to this bug.