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

RESOLVED FIXED

Status

()

Core
Networking: HTTP
P1
normal
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: jrn@jrn.me.uk, Assigned: Biesinger)

Tracking

({perf})

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

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

11 years ago
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.
Reporter, do you still see this problem with the latest Firefox 2? If not, can you please close this bug as WORKSFORME. Thanks!
Whiteboard: CLOSEME 06/27
Version: unspecified → 1.5.0.x Branch
(Reporter)

Comment 2

10 years ago
Still seeing same problem with 2.0.4 on OS X.

Updated

10 years ago
Whiteboard: CLOSEME 06/27
Version: 1.5.0.x Branch → 2.0 Branch
(Reporter)

Comment 3

10 years ago
Just tested with 3.0a5 and present there too, if that's relevant.
Yes it is relevant, thanks!
Version: 2.0 Branch → Trunk
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.
Status: UNCONFIRMED → NEW
Component: General → Networking: HTTP
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → networking.http
Flags: wanted1.8.1.x+
Flags: blocking1.9?
Keywords: perf
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

10 years ago
Moving to blocking for 1.9 as this might be a significant perf win...
Flags: blocking1.9? → blocking1.9+

Updated

10 years ago
Assignee: nobody → sayrer

Updated

10 years ago
Priority: -- → P4
Assignee: sayrer → cbiesinger
Depends on: 262116
Priority: P4 → P1
Created attachment 290421 [details] [diff] [review]
patch

should I add a pref for this?
Attachment #290421 - Flags: superreview?(darin.moz)
Attachment #290421 - Flags: review?(darin.moz)

Comment 9

10 years ago
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.
Attachment #290421 - Flags: superreview?(darin.moz)
Attachment #290421 - Flags: review?(darin.moz)
Attachment #290421 - Flags: review+

Updated

10 years ago
Attachment #290421 - Flags: superreview+
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
per discussion with darin, I'll rename IsPersistentHttpsCachingEnabled to CanCacheAllSSLContent
Created attachment 290542 [details] [diff] [review]
patch v2
Attachment #290421 - Attachment is obsolete: true
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
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Depends on: 406869
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.
Flags: blocking1.8.1.13?
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.13?

Comment 15

10 years ago
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.


Flags: wanted1.8.1.x+
Flags: blocking1.8.1.15?
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.
Flags: blocking1.8.1.15? → blocking1.8.1.16?
Blocks: 420310

Comment 17

9 years ago
(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...
Minusing for the 1.8 branch, although nice to have it doesn't really fit the criteria.
Flags: wanted1.8.1.x-
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.17?
You need to log in before you can comment on or make changes to this bug.