812167 - 302 Redirect Responses are Cached to disk despite "Cache-control: no-cache", no-store", "Pragma: no-cache" and "Expires: -1" HTTP header being set

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[lsmercer]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.11 (KHTML, like Gecko) Chrome/23.0.1271.64 Safari/537.11

Steps to reproduce:

Using firefox 16.0.2 I navigated to a page over HTTPS that responds with a 302 redirect response.

The 302 response had the following HTTP Headers set:
Cache-control: no-cache, no-store
Pragma: no-cache
Expires: -1

This can be recreated with the following php page:
<?php
header('Cache-control: no-cache, no-store, private');
header('Pragma: no-cache');
header('Expires: -1');
header('Location: http://localhost/page.php', true, 302);
exit;
?>

The entire response was:
HTTP/1.1 302 Found
Date: Thu, 15 Nov 2012 10:34:29 GMT
Server: Apache
Cache-control: no-cache, no-store
Pragma: no-cache
Expires: -1
Location: https://localhost/page.php
Content-Length: 0
Keep-Alive: timeout=5, max=100
Connection: Keep-Alive
Content-Type: text/html


Actual results:

The browser redirected to the target location successfully. However, in the about:cache page the 302 is listed under the about:cache?device=disk

Furthermore, after restarting the browser session, the entry persisted.

The output of about:cache?device=disk can be found below. I would not expect to see redirect_test_1.php in the cache:

Information about the Cache Service

Disk cache device

Number of entries: 3
Maximum storage size: 1048576 KiB
Storage in use: 14 KiB
Cache Directory: C:\...\Cache
________________________________
Key Data size Fetch count Last modified Expires
https://localhost/redirect_test_1.php 0 bytes 1 2012-11-15 10:44:53 1970-01-01 00:00:00
http://localhost/page.php 217 bytes 1 2012-11-15 10:44:54 1970-01-01 00:00:00
http://localhost/favicon.ico 7782 bytes 3 2012-11-15 10:44:54 2012-12-06 17:28:21
----------------------------------

After restarting Firefox, these entries persist (despite the 'Expires' value being in the past).


Expected results:

If the following HTTP headers are set, the page or request should not be cached to disk:

Cache-control: no-cache, no-store
Pragma: no-cache
Expires: -1

On firefox 10.0.2, the 302 response is cached to 'memory'. It can be seen in the about:cache?device=memory page and is not present on the about:cache?device=disk page.

When the user exits the browser, the memory cache is cleared and the cache entry does not persist.

This behaviour would be preferable.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Honza, Patrick, shouldn't we be calling UpdateInhibitPersistentCachingFlag in the redirect case?  We don't seem to be....

Comment 2

[Honza Bambas (:mayhemer)]

Yep, we should do it at this line probably:

http://hg.mozilla.org/mozilla-central/annotate/0ef0c9db3b2b/netwerk/protocol/http/nsHttpChannel.cpp#l3958

Comment 3

[Honza Bambas (:mayhemer)]

We would like to get fixed sometime at this cycle.

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking only so we can make sure it got into trunk by the time 20 moves to Aurora.  Tracking on trunk version doesn't really get much release-driver attention on the bug so please prioritize this and get an assignee on it so that the likelihood of a fix in the next 5 weeks is higher.

Comment 5

[Honza Bambas (:mayhemer)]

I'll fix this.

Comment 6

[Honza Bambas (:mayhemer)]

Attachment: v1

coding: 10 minutes, writing the test: 3 hours...

https://tbpl.mozilla.org/?tree=Try&rev=0cf261b0f837

Comment 7

[Michal Novotny [:michal]]

Comment on attachment 707932 [details] [diff] [review]
v1

Review of attachment 707932 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/test/unit/test_bug812167.js
@@ +12,5 @@
> +- check cache entry for the 302 response is not cached at all
> +*/
> +
> +var httpserver = null;
> +// Need to randomize, because apparently no one clears our cache

Wouldn't it be better to simply clear the cache by calling evict_cache_entries()?

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #7)
> Wouldn't it be better to simply clear the cache by calling
> evict_cache_entries()?

It may just slow things down, IMO.  This randomization is copied from other tests.

Thanks for the review and help with this patch!

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 707932 [details] [diff] [review]
v1

https://hg.mozilla.org/integration/mozilla-inbound/rev/c4887bfe1e9a

This is tracked for Fx20 (aurora at this time).  Do I have to ask for approval for this patch or just land?

Comment 10

[:Gavin Sharp [email: [e-mail]]]

You need to ask for approval.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c4887bfe1e9a

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 707932 [details] [diff] [review]
v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): since ever
User impact if declined: storing potentially private information despite server forbade storing it
Testing completed (on m-c, etc.): landed on m-c recently
Risk to taking this patch (and alternatives if risky): low, IMO, but I don't have an argument why ; however, the same technique of adding the flag to inhibit caching is used on other places of this code, not causing troubles
String or UUID changes made by this patch: none/none

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 707932 [details] [diff] [review]
v1

Approving, assuming we have enough time to bake this on Aurora and Beta before shipping to shake out potential regressions.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a298397005cd