Closed
Bug 510359
Opened 15 years ago
Closed 15 years ago
Cached "Vary: Cookie" responses are improperly revalidated / reused
Categories
(Core :: Networking: HTTP, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta1-fixed |
People
(Reporter: ben, Assigned: bjarne)
References
Details
Attachments
(2 files, 1 obsolete file)
6.68 KB,
text/x-python
|
Details | |
5.82 KB,
patch
|
benjamin
:
approval1.9.2+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 2•15 years ago
|
||
I'll have a look. Very nice testcase. :)
Did this work before 3.5.2 ?
Assignee: nobody → bjarne
Updated•15 years ago
|
Component: General → Networking
Product: Firefox → Core
QA Contact: general → networking
Comment 3•15 years ago
|
||
# 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•15 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)
Updated•15 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•15 years ago
|
Attachment #395408 -
Flags: review?(cbiesinger) → review+
Comment 6•15 years ago
|
||
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
Updated•15 years ago
|
Component: Networking → Networking: HTTP
QA Contact: networking → networking.http
Comment 7•15 years ago
|
||
(bug 94123 is filed for improving the Vary: implementation, btw. also bug 468426)
Assignee | ||
Comment 8•15 years ago
|
||
Attachment #395408 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Comment 9•15 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.
Comment 10•15 years ago
|
||
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•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Updated•15 years ago
|
Keywords: checkin-needed
Comment 12•15 years ago
|
||
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•15 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 ?
Comment 14•15 years ago
|
||
You need approval once such a patch exists.
Assignee | ||
Comment 15•15 years ago
|
||
Ok - the latest patch applies cleanly on 1.9.2 branch and passes TryServer for 1.9.2 as well.
Updated•15 years ago
|
Attachment #395542 -
Flags: approval1.9.2? → approval1.9.2+
Comment 16•15 years ago
|
||
status1.9.2:
--- → beta1-fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•