BUG Fix regression in IE no-cache https file downloads.

[mateusz]

Currently IE6-8 will refuse to download files over HTTPS with default
Framework settings.

Currently the HTTP::add_cache_headers competely overrides Cache-Control
headers on each request, so there is no way to inject custom headers
from the API-consuning methods.

Also of note: adding no-store header also fixes the issue but will
prevent proxies from caching the request body (which they do when using
no-cache). So the setting max-age to some low number is a better choice
here.

[mateusz]

@halkyon can you have a look? I don't really like how this if is structured - trying to detect the file download situation but it will break when someone changes Content-disposition to Content-Disposition and we will not notice it broke (again). Any ideas how to make it better?

[hafriedlander]

but will prevent proxies from caching the request body (which they do when using no-cache

I think I prefer no-store, at least on all HTTPS requests. http://stackoverflow.com/questions/866822/why-both-no-cache-and-no-store-should-be-used-in-http-response for what it does (note that no-cache already stops intermediate proxies from caching, no-store just stops them from saving it to non-volatile storage as well)

[hafriedlander]

So, to be clear, keep all the existing checks (UA, content disposition is attachment), but also check if https. If so, use no-store.

[mateusz]

@hafriedlander AFAIK no-cache forces each cache to reissue a request, but it is possible for the server to reply with "not modified" content-less response, which allows the cache in turn to respond using locally cached body. This saves on transmission time between server and the cache.

If we use no-store, my understanding is that caches will need to transmit body on each request, which will put the server under more load.

Good idea with checking for HTTPS!

[hafriedlander]

@mateusz Yeah, it's possible to reply with a 304. But we never do - we always just regenerate the request (unless I'm missing a code path somewhere).

[mateusz]

@hafriedlander

Have added an is_https check, and made it not break when someone decides to fix the header in HTTPRequest. I don't want to really change the header here because it seems to be an API change someone can rely on.

Regarding the 304 - someone might actually implement 304s in the future :-) Do you have any reason why max-age=3 would be worse than no-store in this scenario? Max-age is time tested in this case.

[mateusz]

Seems it has passed the build, not sure why it's still yellow https://travis-ci.org/silverstripe/silverstripe-framework/builds/10611827

[tractorcow]

@mateusz There is a regression introduced in this fix where HTTP::add_cache_headers(); is called without the optional $body parameter. The blog RSS widget is one such case where this occurs.

Fatal error: Call to a member function getHeader() on a non-object

Will follow up with a PR.

[tractorcow]

Fixed with #2445