Cached "Vary: Cookie" responses are improperly revalidated / reused

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: ben, Assigned: bjarne)

Tracking

unspecified
x86
Windows XP
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2

Firefox ignores the "Vary: Cookie" header on a cacheable response, when present.  It revalidates and reuses a single copy of such a response, without regard to cookie value.

We noticed this problem on the login landing page of freebase.com soon after Firefox 3.5.2 was released.  The effect is pretty bad - the user lands on a
"logged out" page instead of the "logged in" one.  It is hard to demonstrate without creating a freebase.com account, so I've attached a tiny server written in Python that demonstrates the problem.

Reproducible: Always

Steps to Reproduce:
1. See comments at top of demo server source file.

Actual Results:  
Incorrect browser cache revalidation/hit.

Expected Results:  
Should instead render a version of page that depends correctly on Cookie value.
(Reporter)

Comment 1

10 years ago
Since the initial reproducible case of this bug requires an account on freebase.com, I've included this tiny server as an alternative way to demonstrate the problem.
(Assignee)

Comment 2

10 years ago
I'll have a look. Very nice testcase. :)

Did this work before 3.5.2 ?
Assignee: nobody → bjarne
Component: General → Networking
Product: Firefox → Core
QA Contact: general → networking
    # The right thing happens for the wrong reason
    # if we use ETag as the validator here instead
    # of Last-Modified.

That wouldn't be for the wrong reason. See RFC 2616 13.6. It explicitly allows conditional requests with ETags in the presence of Vary: headers.
(Assignee)

Comment 4

10 years ago
This problem seems to boil down to necko (erroneously) sending a conditional request (in this case adding an If-Modified-Since header) in cases where the "Vary: Cookie" header is present in a cached response. The patch and test focus on fixing this issue, i.e. the unit-test does not really emulate the test provided by reporter.

It should be noted that in the current approach, the mere presence of a "Vary: cookie" header in a cached response is sufficient to make necko reload the resource. This fact makes "Vary: cookie" work pretty much like a no-cache directive in necko. It could be optimized in at least two steps : 1) ensure that the response in fact has a cookie before reloading, and 2) store cookie-values in the cache and compare values before deciding whether to reload. (See comments in nsHttpChannel::ResponseWouldVary() about the latter, though).

However, the current approach is conservative and will not wrongly reuse a cached resource, although suboptimal in that it will reload resources unnecessarily.
Attachment #395408 - Flags: review?(cbiesinger)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #395408 - Flags: review?(cbiesinger) → review+
Comment on attachment 395408 [details] [diff] [review]
Simple fix to prevent sending IMS header if Vary: Cookie is in response

+            // and we are allowed to do this (see bug #510359)

maybe also mention bug 269303 in this comment, which has much more discussion
Component: Networking → Networking: HTTP
QA Contact: networking → networking.http
(bug 94123 is filed for improving the Vary: implementation, btw. also bug 468426)
(Assignee)

Comment 8

10 years ago
Attachment #395408 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Keywords: checkin-needed

Comment 9

10 years ago
Hi,

See my comment 35 in bug #269303.  I would find the pessimistic solution that causes all/most responses with vary: set as uncacheable unacceptable.  It really is necessary to store the header fields that vary specifies in the cache.

Note to anyone trying to write server-side code: your life will be much simpler if you just use Etags everywhere.

Phil.
Ever since we implemented Vary support a long time ago, that was the situation we were in. It's true that this bug sort of hid that problem in certain cases, but this fix is required. I do agree that a better fix is to store the originally sent headers from the vary: list and only rerequest the data when needed, but that's bug 94123, not this one.
Pushed http://hg.mozilla.org/mozilla-central/rev/8d767fe544bc
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Comment on attachment 395542 [details] [diff] [review]
Updated according to comments from reviewer

I think it's worth fixing this on 1.9.2 as well.
Attachment #395542 - Flags: approval1.9.2?
(Assignee)

Comment 13

10 years ago
(In reply to comment #12)
> (From update of attachment 395542 [details] [diff] [review])
> I think it's worth fixing this on 1.9.2 as well.

Do we need some kind of approval for this or is it just a matter of creating a patch which applies on 1.9.2 ?
You need approval once such a patch exists.
(Assignee)

Comment 15

10 years ago
Ok - the latest patch applies cleanly on 1.9.2 branch and passes TryServer for 1.9.2 as well.

Updated

10 years ago
Attachment #395542 - Flags: approval1.9.2? → approval1.9.2+
You need to log in before you can comment on or make changes to this bug.