Make CSRF links cacheable

+++ b/core/lib/Drupal/Core/Access/RouteProcessorCsrf.php
@@ -45,13 +45,38 @@ public function processOutbound($route_name, Route $route, array &$parameters, B
+          // Tokens are per user and per session.
+          '#cache' => ['contexts' => ['user']],

Where's the per-session part reflected in the cache context?

LOL. #6 is x-post with #5.

+++ b/core/lib/Drupal/Core/Access/RouteProcessorCsrf.php
@@ -45,13 +45,38 @@ public function processOutbound($route_name, Route $route, array &$parameters, B
+          '#cache' => ['contexts' => ['user']],

I think for now we are okay to use max-age=0, else this will be cached per user and we get bugs.

RTBC, looks great. Needs beta eval, doc updates for the new cache context, change record updates, etc. as usual ...

Do we need a change record update for the introduction of the cache hierarchy?

The improvement to RouteProcessorCsrf is fantastic: yay for placeholders!

But, the issue summary says there's no API change and no disruption, but sadly that's not true, because PHP does not implement contravariance for parameter types (PHP--). Therefore, the changing of the processOutbound() signature for OutboundRouteProcessorInterface and OutboundPathProcessorInterface will result in every contrib module that implements one of those breaking fatally until they change their signature to match.

Options:

1. Decide that that's ok, on the assumption that there's not that many contrib implementations of these interfaces, and that fixing the error is just a matter of fixing the signature.
2. Change this patch to be backwards compatible by leaving the interface signature alone, and letting specific implementations that need BubbleableMetadata do an instanceof check on the $cacheable_metadata parameter that they get. So, for example, RouteProcessorCsrf could do that check, and if true, could render a placeholder, but if not true, could set max-age to 0.

I see benefits to both options (#1 results in simpler code due to needing fewer if statements, but #2 is less disruptive to already ported contrib modules), so setting to "needs review" for feedback from others.

I'm open to being convinced otherwise, but after thinking about it some more, currently I'd prefer #21.2, because the instanceof check could be for AttachmentsInterface. So, every outbound processor gets a CacheableMetadata object, whose purpose is clear (tell us how this processor affects cacheability), and the very few that want to optimize their caching by using a placeholder can do so in an if statement that checks whether the $cacheable_metadata object they were given supports attachments (i.e., implements AttachmentsInterface).

outbound route/path processors are extremely rare

Do you have data to back that up?

#22:

I am torn between "Changing the API again is really bad" and "This just got in, there can't be that many users, yet."

I also think it could be a major "WTF!":

The interface says "CacheableMetadata", but you "should" pass in BubbleableMetadata, but no-one except some docs tell you.

And we have to do this for the whole D8 cycle.

My main concern is:

When we just ship with CacheableMetadata and then we have a contrib module, which uses the functionality of opting in via AttachmentsInterface, but then there is another contrib module, which displays links, but just uses CacheableMetadata, then those two clash.

So I think we should bite the bullet and do the API change.

I don't see what the WTF would be. The only caller of processOutbound() that creates the object to pass in is UrlGenerator. So, really, only if you're swapping out UrlGenerator do you need to make a decision of what to pass to processOutbound(). And if we want to, we can make UrlGenerator's protected methods processPath() and processRoute() typehint their last parameter to GeneratedUrl, which I think will provide adequate guidance to people subclassing or replacing UrlGenerator.

I think we can also add docs to OutboundRouteProcessorInterface and OutboundPathProcessorInterface that for best cache optimization, callers are encouraged to pass in a $cacheable_metadata object that also implements AttachmentsInterface, and that processors that could benefit from using placeholders or other attachments should do so within an if ($cacheable_metadata instanceof AttachmentsInterface) statement.

When we just ship with CacheableMetadata and then we have a contrib module, which uses the functionality of opting in via AttachmentsInterface, but then there is another contrib module, which displays links, but just uses CacheableMetadata, then those two clash.

So for example, suppose a contrib module does swap out UrlGenerator and ends up invoking processOutbound() with a $cacheable_metadata object that doesn't implement AttachmentsInterface. Nothing functionally breaks. All that happens is rendered content (e.g., blocks, pages, node views, menus) that contain links to CSRF-protected routes are not cached when they otherwise could have been. Similar kind of progressive degradation would happen for contrib processors. That's not really a clash though, is it?

I'm trying to apply the reasoning from https://www.drupal.org/contribute/core/beta-changes#bc to this issue, and that says that BC is needed unless:

• a BC layer is not feasible,
• providing a BC layer would introduce significant technical debt, or
• the API is sufficiently new that it is unlikely to be used anywhere.

I don't think the BC I'm proposing is either infeasible or would introduce significant technical debt. The $cacheable_metadata parameter was added to processOutbound() on May 9th, so I think there is a possible argument that 7 weeks is still sufficiently new, but it's not true that it's not used anywhere yet. For example, Secure Login added it to its signature on May 14th.

#24 - anecdotally I only ever come across outbound path processors in custom code - usually where there are extremely specific requirements and for whatever reason people didn't want to use aliases.

Note we've just changed the API for inbound path processors in #2509300: Path alias UI allows node/1 and /node/1 as system path then fatals, and it's even rarer to have an outbound path processor without an accompanying inbound path processor. While the nature of the change here is different, I don't think there will be much real term impact in someone having to change some code that they otherwise wouldn't have if we break BC.

Great, sounds like we're ok with the signature change. In that case, should we change it all the way to GeneratedUrl? That way, if in the future we decide that GeneratedUrl should implement some other capability too, that we want outbound processors to be able to impact, we can add it with no future signature change needed?

Also:

+++ b/core/lib/Drupal/Core/Access/RouteProcessorCsrf.php
@@ -45,13 +45,43 @@ public function processOutbound($route_name, Route $route, array &$parameters, B
+          // Tokens are per user and per session.
+          '#cache' => [
+            'contexts' => [
+              'user',
+              'session',
+            ],

Where do they vary per user? And also, is it possible for different users to have the same session?

I also wondered about the second question in #29. Shouldn't do any harm but looks unnecessary/confusing.

Couple of proper questions, couple of small nits.

1. +++ b/core/lib/Drupal/Core/Access/RouteProcessorCsrf.php
   @@ -45,13 +45,42 @@ public function processOutbound($route_name, Route $route, array &$parameters, C
   +          // Tokens are per session.
   +          '#cache' => [
   +            'contexts' => [
   +              'session',
   +            ],
   +          ],
   +        ];
   

   If we have a #lazy_builder, why does the placeholder need to be cached by session? Aren't we adding this just so that it's not only cacheable by session?

2. +++ b/core/lib/Drupal/Core/Cache/Context/SessionCacheContext.php
   @@ -0,0 +1,31 @@
   + * Defines the SessionCacheContext service, for "per session" caching.
   

   In which case, do we need this at all? And then, do we even need the parameter change here to make CSRF links cacheable? I'd expect the #lazy_builder/placeholder to be it. The parameter change looks fine to me for the js use case or similar things adding js tied to links, just not at all clear why it's necessary here - makes the patch a lot larger for what appears to be a 20 line change.

3. +++ b/core/lib/Drupal/Core/PathProcessor/OutboundPathProcessorInterface.php
   @@ -25,12 +25,12 @@
   +  public function processOutbound($path, &$options = array(), Request $request = NULL, BubbleableMetadata $bubbleeable_metadata = NULL);
   

   bubbleeable

4. +++ b/core/lib/Drupal/Core/Render/BubbleableMetadata.php
   @@ -74,6 +74,27 @@ public static function createFromRenderArray(array $build) {
   +   * Creates a bubbleable metadata object from a depended object.
   

   object dependency?

#36: It is a judgment call, but I think it is okay to cache the placeholders per session, but maybe we should use a max-age=86400 to have them be expired some time?

We could definitely remove that, too.

But I think it might even make sense to have the e.g. logout link be ESI'd in per user (and cached in Varnish) in the future.

#37: X-POST, yes that makes sense too and would bubble up correctly, so could also be cached by ESI, so best out of both worlds ...

I also forget we don't have set 'keys' so no need for max-age.

=> Nvm, on #38, RTBC + 1

Committed/pushed to 8.0.x, thanks!

Too bad, this broke other stuff, see #2630920: _csrf_token is broken due to cacheability metadata integration, results in rendered links without valid CSRF tokens