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

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Ben Drees]

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.

Comment 1

[Ben Drees]

Attachment: A tiny web server in Python that demonstrates the problem

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.

Comment 2

[Bjarne (:bjarne)]

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

Did this work before 3.5.2 ?

Comment 3

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

    # 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.

Comment 4

[Bjarne (:bjarne)]

Attachment: Simple fix to prevent sending IMS header if Vary: Cookie is in response

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.

Comment 6

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

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

Comment 7

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

(bug 94123 is filed for improving the Vary: implementation, btw. also bug 468426)

Comment 8

[Bjarne (:bjarne)]

Attachment: Updated according to comments from reviewer

Comment 9

[Phil Endecott]

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.

Comment 10

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

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/8d767fe544bc

Comment 12

[Boris Zbarsky [:bzbarsky]]

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.

Comment 13

[Bjarne (:bjarne)]

(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 ?

Comment 14

[Boris Zbarsky [:bzbarsky]]

You need approval once such a patch exists.

Comment 15

[Bjarne (:bjarne)]

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

Comment 16

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/dd64afe08c41