Closed Bug 510359 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: Networking: HTTP, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: ben, Assigned: bjarne)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
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.
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.
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)
Attachment #395408 - Attachment is obsolete: true
Keywords: checkin-needed
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
Closed: 13 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?
(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.
Ok - the latest patch applies cleanly on 1.9.2 branch and passes TryServer for 1.9.2 as well.
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.