Exempt all routes with a _csrf_token requirement from the page cache

What about a combination of a unit test and remove the _no_cache option from the admin cron route once that is committed for test coverage of this?

+++ b/core/lib/Drupal/Core/EventSubscriber/SetNoCacheRouteSubscriber.php
+++ b/core/lib/Drupal/Core/EventSubscriber/SetNoCacheRouteSubscriber.php
@@ -0,0 +1,29 @@

@@ -0,0 +1,29 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\Core\EventSubscriber\SetNoCacheRouteSubscriber.
+ */
+
+namespace Drupal\Core\EventSubscriber;
+
+use Drupal\Core\Routing\RouteSubscriberBase;
+use Symfony\Component\Routing\RouteCollection;
+
+/**
+ * Sets the 'no_cache' option for routes inferred to be uncacheable.
+ */
+class SetNoCacheRouteSubscriber extends RouteSubscriberBase {
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function alterRoutes(RouteCollection $collection) {
+    foreach ($collection->all() as $route) {
+      if ($route->hasRequirement('_csrf_token')) {
+        $route->setOption('no_cache', TRUE);
+      }
+    }
+  }
+

This needs an integration test ... its missing the entry in core.services.yml ... a unit test would have not triggered that.

I assume this needs Tests :-D

Adds a service to core.services.yml and a unit test.

This issue is a task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

Added Drupal 8 beta phase evaluation template.

So I understand that we wouldn't want a CSRF-protected route to end up in the page cache.

However:

- there's very limited use cases for rendering a CSRF-protected link to an anonymous user.
- if you do render a CSRF-protected link to an anonymous user, then that will start a session. Starting the session will disable page caching for the request rendering the link.
- when the user has the session, visiting the CSRF-protected route will also bypass page caching - since they have a session.

So as far as I can see, this already works. If for some reason it doesn't, I think we need a functional test that would fail without the patch, but this looks unnecessary to me.

We already had a test:

This was one of the blockers for the original "Enable page cache" issue, we since circumvented it by adding no_cache: true as route option, this was just to make this simpler / more automatic for CSRF routes ...

Assigning to Wim for feedback.

I'm also confused.

There is system.run_cron /admin/reports/status/run-cron? this for manually running cron via the UI - this has a permission check and a CSRF check. Anonymous users can never get to this.

Then there is system.cron: /cron/{key} which is for running cron via wget/curl and requires the cron key. This does not use CSRF protection and has to explicitly opt out of caching. Can be visited by anonymous users.

In terms of SmartCache this makes more sense, but I'm having trouble understanding why we need to set this on the route, and not just set max-age=0.

Apart from the lack of hit rate for csrf-protected routes, the main thing is that something happens (the operation that is being CSRF-protected like flagging) that needs to happen every time the route is hit.

Also my understanding of smartcache was that it would operate pre-routing, so how does having the information on the route help - wouldn't we need to run routing, to get the route, to check this property, to decide whether cache get or not - would be quicker just to cache get all the time and never set it no?

In related issue raised good points to exclude CSRF for anonymous users