WIP NEW Add better HTTP cache-control manipulation

[dhensby]

SS3 code for silverstripe/silverstripe-versioned#146

@chillu / @sminnee please don't hate me with this. This is just an initial POC of how we can get Cache-Control working in a more controlled way. Once the general approach is accepted I'd then start applying HTTPCacheControl::inst()->disableCaching() (or even HTTPCacheControl::inst()->privateCache()`) to the relevant parts of the framework that indicate to us that there's some globally non-cachable content being passed around.

1. I've addded a new class HTTPCacheControl which keeps better track of the Cache-Control directives and adds some helper methods to set up the cache-control to what we'd want (eg: disableCaching to add the holy trinity of no-cache, no-store, must-revalidate and privateCache to set the cache-control to the sensible defaults for private cache-control headers.
2. I've reworked HTTP::add_cache_headers() to fix the issues outlined here: Enforce no-cache when viewing draft or private content silverstripe-versioned#146 (comment)
3. I've added case-insenstive queries of SS_HTTPRequest::getHeader so that we can simplify a small piece of silly code here
   4, Fixed our inconsistent ETag generation

[chillu]

Isn't this an implicit API change? There'll be configurations which relied on those defaults, however bad they might be. At least this will need a pretty solid upgrading guide.

• Removing Vary: Cookie: Leaving this out could be dangerous unless HTTP proxies implement "safe but not spec compliant behaviour", namely that they ignore caching instructions if detecting cookie that aren't whitelisted, regardless of Vary (CloudFlare and Incapsula do that). The proper answer is not to send Cache-Control headers in the first place with those cookies, so Vary doesn't apply. I think as long as we stick with "PHP session = potentially private content", we're good. There's some edge cases around cookie-based a/b testing, but I think they can be covered in the upgrading guide rather than 100% backwards compat.
• Removing Vary: Accept: That seems fine, it's auto-added by mod_gzip etc anyway
• Removing Vary: X-Forwarded-Protocol: Do you have a reference on why this is bad practice? It seems common practice for a cache to include the full URI (incl. protocol) in computing the cache key, making this header redundant. It's more relevant for the application processing a request.
• Adding Vary: X-Requested-With: That seems overkill as a default? It's fairly uncommon to vary responses between ajax and non-ajax requests. We might do this in the CMS, but those are never cacheable requests anyway.

[dhensby]

They may be an implicit API change and yes some sites may rely on these, but this combination is practically "no-cache" as far as a proxy is concerned because these Headers are likely to be unique per visitor so if we want to provide meaningful HTTP caches we just can't have this set up.

Vary: Cookie: If we denote the content as private using the private directive then this vary is not needed. Fastly explicitly say Vary: Cookie is no good. If we use Vary: Cookie we may as well not use Cache-Control it seems.

Vary: Accept: Note this is different from Accept-Encoding (which is added by mod_deflate and other server layers that do compression). Accept is the mime types the browser understands (eg: text/html or application/json). I can't seem to find many references online to support using Vary: Accept anywhere.

Vary: X-Forwarded-Protocol: This is a header that should only be set by proxies, load balancers, CDNs and is not a header the browser sends, this means it provides no value as a Vary.

Vary: X-Requested-With: I've added that purely because of our HTTP.cache_ajax_requests config option as it seems sensible; though I think you're right that it's not needed... Happy to remove it if you think even with our HTTP.cache_ajax_requests option it is not needed.

[chillu]

All good, thanks for taking the time to respond!

cache_ajax_requests is such a whacky behaviour, I think we should deprecate it in 4.x. But in the meantime, I would only add this Vary header if its turned off - because that's the only time the behaviour "varies" based on that metadata.

[dhensby]

I would only add this Vary header if its turned off - because that's the only time the behaviour "varies" based on that metadata.

That's true, so I'm happy to take it out, it does make managing the Vary stuff a bit harder, though

[chillu]

Mozilla says:

No transformations or conversions should be made to the resource. The Content-Encoding, Content-Range, Content-Type headers must not be modified by a proxy. A non- transparent proxy might, for example, convert between image formats in order to save cache space or to reduce the amount of traffic on a slow link. The no-transform directive disallows this.

That seems like over-reach for framework defaults, and should be left to the specific implementation. Happy to have that removed.

[chillu]

I think given the security implications of getting this change wrong, it'd be good to have quite granular commits for specific stuff, for example one commit for "move isDev below headers_sent check" to make that explicit.

[dhensby]

I've split this PR into smaller commits that are hopefully a bit easier to follow

[sminnee]

Hi Dan,

This is a good start and I like the general direction, although I've got a lot of feedback. :-)

General:

• IMO any APIs we add to 3.7 should be as close as possible to APIs we add in 4.2 or 4.3. The one possible exception would be whether we namespace the relevant class. Still, we need to consider how this will look in 4.
• I think the entirety of HTTP::add_cache_headers should be moved to HTTPCacheControl. The split of responsibility right now is a little arbitrary and less extensible than would be ideal.
• Cache control headers are hard, and we should expect that in 95% of cases, developers aren't going to want to tweak things at this level. Our goal as a framework should be to provide some sensible defaults that cover most cases, with the ability to tweak things as needed. My view on what is a good "normal" set of tools is:
  • Say whether content is public or private. "Private" means any content that is session-specific. (setPublic, setPrivate)
  • Say how long content can be cached for. (setMaxAge)
  • Say which headers content varies by. (setVary, clearVary, addVary, removeVary)
• I would suggest refactoring this alongside controllerpolicy, and considering how controllerpolicy will be work with 3.7.

Specific:

• HTTPCacheControl should use Injector and not its own hand-rolled inst. In SS3 it can extend SS_Object; in SS4 it can use Extensible. It can then use self::singleton() to get its inst.
• Move as much code from HTTP::add_cache_headers() as possible. The one thing I would leave inside HTTP::add_cache_headers() is the behaviour to work without an HTTPResponse; this seems like a cluttering distraction. HTTPCacheControl can have a method such as applyToResponse(&$response).
• If a response isn't provided then add_cache_headers can pass an empty HTTPResponse object, extract any headers added to that object by HTTPCacheControl, and send them straight to the browser.
• register_modification_date, register_modification_timestamp, and register_etag should pass over to HTTPCacheControl.
• In SS4, HTTPCacheControl might best act as a middleware, bypassing add_cache_headers entirely, which can be deprecated.
• HTTPCacheControl should be designed to have a number of "modes", in the first instance "public" and "private" but in principle developers could add their own custom modes. Each mode should have its own blob of configuration code that can be swapped out – there could potentially be a whole separate HTTPCacheControl-style object for each of these, maybe HTTPCacheControl.public and HTTPCacheControl.private within Injector, with the top-level object simply acting as a dispatcher. This will become more important for modules like controllerpolicy, that will also need to respect the same modes. In particular, it's critical that controllerpolicy's current configuration only be applied when in "public" mode. This will likely result in a new minor release of controllerpolicy designed to work with SS3.7.
• Note that any setters called on the dispatcher such as setMaxAge should be passed to both the private and the public mode, rather than merely to the currently active one, otherwise you'll get confusing bugs if you setMaxAge and then switch to a private mode.
• The disabling of cache on dev is a good default, but do it as a flag in yml config - that is, a block of config that sets a "disableCaching" property if in dev mode - not hardcoded into the bowels of the app.
• It would probably be worth adding some flags to enable/disable the blocks of functionality such as your special handling of IE6/8 no-cache/no-store, e-tag, application, and auto-generation of 304 responses.

[chillu]

I'm not sure about separating HTTPCacheControl from the HTTP class. Maybe we should just handle the state statically in HTTP instead, and then move to an instance-based (and more testable) HTTP class in 5.x? It seems weird to have HTTP::add_cache_headers() and HTTP::set_cache_age() controlling the Vary, ETag and Max-Age aspects of HTTP caching, and then a separate HTTPCacheControl class specifically for the Cache-Control header. These headers conceptually belong together, right?

[dhensby]

tl;dr: The Cache-Control header is quite complicated / specialised I felt it needed a dedicated class to manage its state.

The main reason I went with a separate class was just for my sanity in keeping this closely related logic together.

The Cache-Control header is a special kind of header with these multiple directives that can be applied, a bit like a header of headers (like Cookies in that respect) and I just felt this was best contained in one single class. Symfony do something semi-similar where their "Header Bag" has a special treatment of the cache control header (https://github.com/symfony/http-foundation/blob/master/HeaderBag.php).

I don't much like this approach either, but I prefer it to adding it onto HTTP and making that more bloated and less focused. Really, I'd have preferred to be managing this on the response object itself, but that seemed a bit too much of a shift.

[chillu]

I think Symfony has it right, treating all headers in one API/class, and then specialising with methods like addCacheControlDirective().

[chillu]

Spelling

[chillu]

This could lead to double ups (from the specific response and the generic config), but that's acceptable - I assume it's unlikely to throw off HTTP caches. So before generic config would override specific Vary headers on the response, which makes that feature useless. Now it appends both. Ideally you could opt out of certain Vary headers set in config (e.g. instruct to vary on cookie, apart from specific responses). But that's quite specialised, can't think of a great use case.

[dhensby]

Making this unique shouldn't be too tough; I'll look at adding that in

[dhensby]

Made this unique for now given the current approach, but this may change with some refactors

[chillu]

You removed the else case from the original implementation which added the no-cache and no-store flags. Why wasn't this replaced with HTTPCacheControl::inst()->privateCache() instead? Are you intentionally removing those defaults for non-content-disposition responses, and rely on other logic paths to mark this as a private cache? I can't see any invocations of setPrivate() or privateCache() in your changeset?

[dhensby]

The old logic flow was essentially [logic to figure out $cacheAge value] -> [if $cacheAge > 0] cache response [else if MSIE HTTPS attachment] cache response for short period [ else ] no cache.

The new logic flow is: [ work out if response should be cache-able and modify HTTPCacheControl as appropriate ] -> [ fix edge case bugs ]

The previous logic was "if this is a non-cacheable resource ($cacheAge is <= 0) AND this is MSIE requesting a file download over HTTPS, make it cacheable but for a short period of time).

In my refactor I'm no longer using $cacheAge as a way of determining if a response should be cached, instead I'm directly calling HTTPCacheControl::inst()->disableCaching();.

So what happens now is we are saying "if this is MSIE requesting a file download over HTTPS and we have the no-cache or no-store directives, then make this a private cache response. This is more inline with fixing the actual bug as described by MS: "When you try to open a Microsoft Office document or a PDF file by typing an HTTPS URL for the document in the Address bar ..., you may receive [an error message]. ... This issue occurs if the server sends a "Cache-control:no-store" header or sends a "Cache-control:no-cache" header."

[dhensby]

I can't see any invocations of setPrivate() or privateCache() in your changeset?

3 lines above this review comment I use privateCache :)

[chillu]

I don't understand why this logic branch exists. Why does the presence or lack of a response body influence the way the response code is sent?

[sminnee]

This is pre-existing behaviour of HTTP::add_cache_headers() that should be deprecated in 4 but we still need to support in SS3:

You can call it without an SS_HTTPResponse object and it will instead perform direct header() calls. Yuck, but it's used in RSSFeed.php in SS3 and in principle could be used by project code.

[chillu]

Oh right, it's checking for the presence of a response object implicitly by checking for $body

[tractorcow]

RSSFeed could still take a response here, and we could make the missing response arg a deprecation notice.

[sminnee]

We should also have, say, HTTPCacheControl.temporary to cover definitely-use-only-once responses such as that after a POST request.

"temporary" could also be "dynamic", "uncacheable", or even "ephemeral" (okay that last one is kind of a job)

[chillu]

Do we need to make setPrivate() "sticky"? Otherwise a later setPublic() call might accidentally undo flags sent from other logic paths already. For example, userforms might enforce setPrivate() (regardless of using CSRF tokens) as part of the controller execution, but your PageController might overrule it later with a setPublic() call. You could force devs to check for the current mode before using setPublic(), but that's prone to human error. Is there any case where a logic path has requested private mode, but there's a valid reason for it being overwritten later on? If PHP sessions trigger setPrivate(), there should be a way to overwrite this behaviour if a dev is sure there's no private content associated with this particular execution path. Do we need $force flags?

On modes, this sounds a bit like gold plating the API? I think a "temporary" mode would be an advanced enough use case that we should rely on the implementor to understand the HTTP caching behaviour and use the more lowlevel HTTP APIs like addHeader, addVary etc? Basically, "public" and "private" are dev-friendly abstractions which avoid mistakes, but anything beyond that should clearly require more understanding. In the case of a "temporary" mode, this would be a matter of knowing the effects of no-store, right?

[sminnee]

We should also have, say, HTTPCacheControl.temporary to cover definitely-use-only-once responses such as that after a POST request.

"temporary" could also be "dynamic", "uncacheable", or even "ephemeral" (okay that last one is kind of a job)

[sminnee]

Do we need to make setPrivate() "sticky"? Otherwise a later setPublic() call might accidentally undo flags sent from other logic paths already.

This is a non-trivial problem. It's almost like you need a prioritised list of "set private" and "set public" (or set temporary) declarations and the highest priority one wins.

For example, if we say "set private if a session cookie is present", then a conscientious developer might choose to override that with something more targeted.

In general, however, temporary > private > public for any given priority level.

[sminnee]

Temporary is not advanced. It occurs on pretty well every POST request, and we need to put the default cache headers for these situations somewhere.

[dhensby]

Just a quick summary for any further Vary discussions:

The way I understand the Vary header is like this:

For simplicity I'm only going to consider the flow for a browser. When a browser makes a request to a server, if the response is cache-able (ie: Cache-Control has does not contain the no-store directive) the browser will store the response in it's cache and build a cache key using the request URL (protocol, domain, path and query string). However, if the server sends back extra information in it's response via the Vary header it will also use the values it sent in those headers to form part of the cache key.

If we look at a typical HTTP request that chrome might make:

GET /home?stage=Live HTTP/1.1
Host: example.com
Connection: keep-alive
Upgrade-Insecure-Requests: 1
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.181 Safari/537.36
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8
Accept-Encoding: gzip, deflate, br
Accept-Language: en-GB,en-US;q=0.9,en;q=0.8

And the server responds with:

HTTP/1.1 200 OK
Date: Wed, 23 May 2018 09:36:33 GMT
Vary: Accept-Encoding
Content-Encoding: gzip
Cache-Control: max-age=0
Expires: Wed, 23 May 2018 09:36:33 GMT
Content-Length: 9549
Keep-Alive: timeout=5, max=100
Connection: Keep-Alive
Content-Type: text/html; charset=UTF-8

Then the browsers cache key is no longer just the requested URL it will now also contain the value the browser sent with it's Accept-Encoding header. We can imagine the cache key now being comprised of the URL and gzip, deflate, br (the value of the Accept-Encoding header that was sent).

For CDNs it's similar except they'll ignore more requests (like those marked as private or with specific directives aimed at proxies) and may have some other internal logic to discard requests they feel shouldn't be cached.

Therefore when thinking of what goes into a Vary header we need to consider:

1. Does this header change so frequently as to make HTTP Caching useless (eg: Cookie)
2. Does the browser even send this header (eg: X-Forwarded-Protocol)

Especially when considering CDNs, we want to be caching resources that can be shared and using Cookie (which will be unique to each user most of the time) we are essentially making CDN caches useless. If that's the intention, then we should be simply using private so that the browsers can do some caching even if the CDNs can't.

[dhensby]

IMO any APIs we add to 3.7 should be as close as possible to APIs we add in 4.2 or 4.3. The one possible exception would be whether we namespace the relevant class. Still, we need to consider how this will look in 4.

It looks like must of this API has stayed the same in 4.x, with HTTP::add_cache_headers() being sprinkled around framework code. The code in HTTP::add_cache_headers() looks unchanged from 3.x too, which means it'll have the same issues I've identified in the 3.x branch too. Given that's the case, I think we'd keep the implementation for 3 and 4 basically the same as I think using a middleware approach would be too risky in terms of comparability, HTTP::add_cache_headers() is a supported API it seems.

I think the entirety of HTTP::add_cache_headers should be moved to HTTPCacheControl. The split of responsibility right now is a little arbitrary and less extensible than would be ideal.

My intention was the HTTPCacheControl handles the Cache-Control only and nothing else. HTTP currently has the responsibility for other headers and seems to have quite mixed responsibilities there. Given that Cache-Control is basically the fundamental header when controlling HTTP caches, it seemed right to me that it control that part and HTTP defer that to another class whilst maintaining its current purpose.

Cache control headers are hard, and we should expect that in 95% of cases, developers aren't going to want to tweak things at this level. Our goal as a framework should be to provide some sensible defaults that cover most cases, with the ability to tweak things as needed. My view on what is a good "normal" set of tools is:

I think that's true, I've added HTTPCacheControl::makePrivate(), HTTPCacheControl::makePublic() and HTTPCacheControl::disableCaching() to cover those common use-cases off, HTTP::set_cache_age() still works.

Say which headers content varies by. (setVary, clearVary, addVary, removeVary)

I hadn't thought about this, mostly because this isn't possible without modifying config at the moment anyway. I was thinking we'd leave this as is for the current change / allowing users to directly set the vary header on the response object...

[chillu]

So looks like we've got a few unresolved points:

• Should we abstract to "modes", or are the three methods Dan already implemented enough?
• Cache control logic in HTTPCacheControl, or HTTP? Or all logic from HTTP moved into HTTPCacheControl?
• How do we handle "sticky" private mode?
• Making vary headers more composable (set/add/remove), without creating duplicates
• Middleware in SS4, or staying close to the 3.x-style implementation?
• Some detail feedback from Sam's comment: Disable cache on dev mode via YAML, register_*() calls

[sminnee]

Should we abstract to "modes", or are the three methods Dan already implemented enough?

I think to answer this we need to see a PR to silverstripe-controllerpolicy designed to work with this that ensure that the configuration currently being provided to controllerpolicy only applies to public responses.

This is absolutely critical; all of this work is meaningless unless that is achieved.

@dhensby can you do some work on a controllerpolicy PR to go alongside this and see how you get on? Make sure that it doesn't require any change to preexisting controllerpolicy configuration so that an upgrade to both framework and controllerpolicy can be dropped in-place to an existing site.

[sminnee]

It looks like must of this API has stayed the same in 4.x, with HTTP::add_cache_headers() being sprinkled around framework code.

The design of HTTP::add_cache_headers() is completely out of keeping with the rest of 4.x and I think it would be absolutely fine to deprecate it in 4.2 and keep it functioning merely as a shim that passes control over to a middleware or something. You could probably structure it so that the middleware was applied a maximum of once; if HTTP::add_cache_headers() was called manually then a subsequent execution was a no-op.

Given the lifetime of 4, we're going to need to be able to undertake refactorings in a non-breaking way and it's not in the interests of the project to leave them 'til 5.

[dhensby]

<moved tasks from this comment to silverstripe/silverstripe-versioned#146 >

[chillu]

@dhensby Can you please prioritise the rest of your checklist for your working day today? We need to get this fully into peer review (incl. any changes requires to controllerpolicy) by Thursday NZST, if we want to release 3.7.0-rc1 by Tuesday NZST (Monday is a public holiday here)

Sam said: In SS4, HTTPCacheControl might best act as a middleware, bypassing add_cache_headers entirely, which can be deprecated.
Dan said: this I'm not so sure about this one. HTTP hasn't had any relevant changes to it between 3 and 4 so I'm worried about just deprecating and moving away from it, especially given all the methods are static...

I'm on the fence with this. We want to teach developers good caching habits, and having consistent API that can be memorised across releases is a big part of that. But then again, the 3.x release line won't have a big impact on developer habits any more. I don't see a whole lot of practical difference between e.g. HTTP::setCachePrivate(), Injector::inst()->get('HTTPCacheControl')->setPrivate() and Injector::inst()->get('HTTPCacheMiddleware')->setPrivate() in 4.x for developers, in terms of API use. For core architecture, middlewares give us more consistency (execution order) and composability. For developers, it means they can debug and opt out of this in the same way as other middlewares. It would also be a more consistent path to more advanced HTTP caching, e.g. filesystem-based caches like https://github.com/tractorcow/silverstripe-dynamiccache.

Ingo summarised: Cache control logic in HTTPCacheControl, or HTTP? Or all logic from HTTP moved into HTTPCacheControl?

I note you haven't added that to your checklist ;) I think if we go to a middleware based approach in 4.x, I wouldn't expect there to be another API called HTTPCacheControl - the configuration and state would happen on one middleware object. IMHO we should only introduce HTTPCacheControl in 3.x if we expect it to survive in 4.x - which means not doing middleware. If we're not expecting it to survive in 4.x (and implement middleware), then we should just continue using the HTTP class for this, and embrace the static-ness.

We have slightly longer for 4.2.0-rc1 than we have for 3.7.0-rc1 (deadline next Tuesday), but if we introduce an API in 3.x we should have a plan for how to continue it in 4.x. I don't feel strongly about the above, tending towards "whatever gets the job done" at the moment. We need to make a call one way or another in the next 24h on this, and I'm happy to follow Dan's lead since he's put the most energy into this.

[chillu]

Dan has sent a pull request for controllerpolicy: silverstripe/silverstripe-controllerpolicy#21

[sminnee]

In addition, we should say something like "Please use HTTPCacheControl::singleton() methods to more safely customise your cache-control headers."

[tractorcow]

How about

trigger_error(
	"Cache-Control header has already been set. "
	. "Please use HTTPCacheControl API to set caching options instead.",
	E_USER_WARNING
);

[tractorcow]

I'm going to squash on merge; There are too many commits here to be worthwhile keeping if that's ok.

[sminnee]

Woohoo!

[tractorcow]

Merged!

[madmatt]

Looks like it might be a bit late now heh, but earlier in this thread there were requests from @chillu to keep some of the commit messages separate so that they were clearer about their intention to future developers.

Oh well - great to see it merged regardless!