URL generation does not bubble cache contexts

.g. Yes, need separate token for admin forms and admin links, but could have just one token for content forms.

Or even better we could decide to use token groups, e.g.:

_csrf_token: TRUE
_csrf_token_group: admin

If you don't send the $value argument to drupal_get_token(), you already get the same token for every occurrence on the page.

So I think what you're really saying is that we'd restrict $value to a handful of constants? That seems completely reasonable. Some of the worst hacks in cacheable_csrf are due to the value argument, especially for links.

OMG AWESOME!

I'll help with this patch once you get it going — definitely by providing reviews, but if necessary also by pushing it further. I'd love to see this in :)

This will make all blocks containing forms cacheable, but also Views exposed filters!

I've read through this a few times now and it looks great!

If we use cookies/local storage, then this feels like exactly the same security model that cacheable_csrf has.

I think it'd be good to get someone less involved in 8.x caching, but with a big interest in security to look at the client-side tokens, just in case we missed something. I especially say that as the main author of cacheable_csrf, but it has had review from several other people by now already so feel pretty good about it personally.

If we use #post_render_cache then there's no change in the security model compared to 7.x - assuming we support token groups (for admin vs. non-admin etc.). We'd also need to do that for the fallback confirm forms anyway.

#1798296: Integrate CSRF link token directly into routing system was the original issue. We discussed keeping the route flag but swapping out the implementation of the token to something cacheable/replaceable on the issue too.

Great plan.

For the record, the implementation in Authcache does not even bother collecting all the tokens into one request. This made the ESI implementation straight forward. The only problem on real sites I've encountered with this approach is the commerce add-to-cart form (which generates as many different forms as there are buttons on the page). But this would be easily solvable with the proposed csrf group method.

There is still the problem with cached forms on cacheable pages , but it looks like this is worked on in #2242749: Port Form API security fix SA-CORE-2014-002 to Drupal 8.

This is a rough start, but we would need something like this. Has a few benefits first off;

- Outbound route processor is greatly simplified - it just needs to insert a placeholder
- The CsrfAccessCheck would also not need to do the path dance. It would just use the _csrf_token requirement value from the route. As long as we are ok with using this limited set of values. I think it would be fine, per-path is maybe a step too far, more idealistic from a security point of view but not everything else. So security feedback would be good. Some compromise here.

damiankloip++ :)

At that point, wouldn't it be a lot easier to just start using the Origin header instead of token-based validation?

Origin is approved by the secteam, we discussed that internally several times. The only problem with this is browser support, but I guess it's not a problem anymore for Drupal 8?

(Relying on the Origin header could actually be *more* secure than token-based validation because third-party plugins seem to be doing a better job at preventing the Origin header from being spoofed than at enforcing basic cross-domain boundaries.)

Since we're only doing PHP replacement here there's no significant change to the security model, seems OK to proceed.

I do think we should discuss the Origin header before looking at JavaScript replacement, since that's more complex to implement.

Agreed. Sounds like a sensible way forward.

Since this is really the only way to fix #2335661: Outbound path & route processors must specify cacheability metadata properly, bumping to critical.

In order to reduce risk and speed up this issue, I'd rather only attack menu-link CSRF tokens here and do forms in another issue. This is because some forms may have user dependent #default_value and caching them will cause problems (e.g. #2307555: Comments intermittently misattributed over in Authcache was one not so obvious instance of that problem. The same fragile code still exists in D8 AFAIK). We cannot expect #4 to magically happen by just offloading CSRF form token generation to Ajax or #post_render_cache.

Would anyone be willing to Assign this issue to themselves. I'm a Base system component maintainer and doing triage on Critical items.

I can do this. I did the first half. Might as well carry on I guess :)

New patch with a CsrfController instead. Will talk to Fabianx about the render cache side of things. Should have something today.

@alexpott, @effulgentsia, @xjm, and I discussed this and were unclear as to how this cache poisoning bug would be more than a very rare edgecase, and it's not a security bug because sessions that receive the invalid link would still be denied access.

The only case the cache poisoning would occur is if a link with a CSRF token were added to a menu.
This is really unlikely given that CRSF dependend routes are dynamic.
Examples:

• Theme install/uninstall links: admin/theme/install?theme=bar
• Comment approve links: /comment/299792/approve

If someone puts those dynamic links into menu links, they have other problems. CSRF links appears on admin listing forms, not on usual menu links.

It does sound from #19 and #20 that we want a separate issue for forms, and I think that separate issue would actually be critical?

The only case the cache poisoning would occur is if a link with a CSRF token were added to a menu. This is really unlikely [...]

Note #144538: User logout is vulnerable to CSRF. Also I think this is not only about menu, but also render cache (?)

Regarding forms: This is IMHO not feasible unless most core/contrib forms are crafted in a way that they are cacheable. This is not the case at the moment. E.g. the comment form puts the username in a '#type' => 'value' element, the contact form has profile data in #default_value. Therefore in this cases it is not enough to just dynamically generate the CSRF token. Rather it is necessary to strip personalized data from the form (and perhaps restore it in the browser via Ajax).

What we might want to do is adding cacheability metadata to forms as a first step (or even form fields?).

If we make the csrf token support for forms cacheable, then it'd make it much easier to create a cacheable form at the API level (i.e. without requiring cacheable_csrf module). I think it's very unlikely we'll be able to enable HTML caching of all forms by default though for the reasons znerol states.

This is blocking #1965074: Add cache wrapper to the UrlGenerator and #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees, adding tag.

needs the remaining tasks identified in the issue summary

and to see if the summary can be more of a summary: be more concise

Related reading: http://www.fastly.com/blog/Caching-the-Uncacheable-CSRF-security/

.

reroll

oops

There is a patch, so "needs review".

Nothing special, just found couple of minor nits while reading up on this.

Ah 0 bytes in the last patch :(

While looking at the test failings in RouteProcessorCsrfTest I can see :-

a documentation bug on the interface.
a couple of invalid assertNull's
and some testing that is too tightly coupled to the current implementation, which will need fixing for this issue to pass.

In short a bundle of side issues that now have a dedicated issue all of their own.

#2430167: Broken tests for \Drupal\Core\RouteProcessor\OutboundRouteProcessorInterface::processOutbound

This is me acting on #44

RouteProcessorCsrfTest has been refactored in light of the fact that RouteProcessorCsrf no longer depends on CsrfToken.

[ One failing test should now pass. ]

Retitling - this is any link.

I don't think this blocks the related issues - we already cache some links which may or may not be CSRF enabled.

Also we discussed this on the maintainer call and are thinking about downgrading to major, but this was a very borderline decision and I need to type the reasoning up properly - so leaving critical unless that's typed up.

Thought about this overnight, there is a security issue if we don't do this I think.

1. User A visits a page, the HTML is cached with their CSRF token.

2. User B visits the page, the User A's CSRF token is in the HTML. Clicking the link will fail.

3. However, User B then takes the link with User A's token, and creates a CSRF attack using that.

4. User A visits the page with the CSRF attack, and the link is valid for them.

CSRF token is based on session, so it'll work as long as the user has the same session ID, which could be a long time.

Discussed with the branch maintainers, and they were convinced by this being a security bug, and therefore critical, per #48.

Recategorizing as a bug and retitling to describe the bug rather than the proposed solution.

updating issue summary #1805054-138: Cache localized, access filtered, URL resolved, and rendered menu trees said this is not blocking that anymore. (still related though)

taking a look at this

Looking at the approach so far, I'm not seeing how it would be secure against replay attacks. maybe we need to move to a post-render approach?

per discussion with Wim Leers, postponing on #2335661: Outbound path & route processors must specify cacheability metadata

We can't implement this using post-render without more generally solving the cachability of links.

The patch as is seemed only about 30% complete, and had issues like defining the same constant in multiple places and yet not using it. e.g. I started down this path for a while:

+++ b/core/tests/Drupal/Tests/Core/Access/RouteProcessorCsrfTest.php
@@ -7,6 +7,7 @@
 
 namespace Drupal\Tests\Core\Access;
 
+use Drupal\Core\Access\CsrfAccessCheck;
 use Drupal\Tests\UnitTestCase;
 use Drupal\Core\Access\RouteProcessorCsrf;
 use Symfony\Component\Routing\Route;
@@ -44,7 +45,7 @@ public function testProcessOutboundNoRequirement() {
    * Tests the processOutbound() method with a _csrf_token route requirement.
    */
   public function testProcessOutbound() {
-    $route = new Route('/test-path', array(), array('_csrf_token' => 'TRUE'));
+    $route = new Route('/test-path', array(), array(CsrfAccessCheck::ROUTE_REQUIREMENT_KEY => 'TRUE'));
     $parameters = array();
 

Yes. Sounds reasonable. The approach so far is just the token placeholder side. Not the actual rendering. Fabian has some good ideas about that :)

Yes. Sounds reasonable. The approach so far is just the token placeholder side. Not the actual rendering. Fabian has some good ideas about that :)

If Damian is also convinced, then I'm pretty sure this is going to be awesome :)

I'm not convinced that the approach in this issue is correct at all.

Please update the issue summary to clarify if you have a new approach.

+1 to pwolanin's skepticism: this needs a much clearer explanation.

Just the tag fairy, carry on.

Thank you fabian!

Quick issue summary update to note what this issue's postponed on.

So #2335661: Outbound path & route processors must specify cacheability metadata is in.

As far as I understand we would be safe, in case all those CSRF cache contexts are bubbled up. For menu links this is the case after #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees
but in custom code, you could still render a URL to a random route, but the probability that wrong entries are stored in cache is less likely.

If I'm not mistaken, this is no longer blocked, since #2335661: Outbound path & route processors must specify cacheability metadata got in.

Removing assigned field to encourage also other people to look at it.

So #2335661: Outbound path & route processors must specify cacheability metadata at least when combined with #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees should fix the security issue.

Then we have the (major) issue that the presence of a CSRF token in any rendered HTML will disable caching. In core this isn't a big deal, but it means for example flag module will disable caching for all users with access to flag links on entities or similar.

On the patch, I'd expect this to look more like #2478443: Set the 'is-active' class for anonymous users in a Response Filter instead of a #post_render_cache callback. We know there are use-cases for CSRF tokens to be replaced via either PHP or JS, and the HTML parsing would also be very similar.

On the patch, I'd expect this to look more like #2478443: Set the 'is-active' class for anonymous users in a Response Filter instead of a #post_render_cache callback We know there are use-cases for CSRF tokens to be replaced via either PHP or JS, and the HTML parsing would also be very similar.

Hah, that is exactly what @effulgentsia and I concluded yesterday too.

But!

So #2335661: Outbound path & route processors must specify cacheability metadata at least when combined with #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees should fix the security issue.

That will fix the security issues for links with CSRF tokens if they are part of menus, NOT the more general problem of any link.

Then we have the (major) issue that the presence of a CSRF token in any rendered HTML will disable caching. In core this isn't a big deal, but it means for example flag module will disable caching for all users with access to flag links on entities or similar.

Aha, I see. The idea that @effulgentsia and I had was to have that response filter just search the HTML for CSRF token placeholders, and replace all of them, always.

But even then, we are left with the problem of other outbound path & route processing having cacheability metadata that isn't bubbled when URLs/links are added to a render array in the typical $this->t('Please see <a href="@url">the docs</a>", ['@url' => \Drupal::url()]) way. For example:

1. language path processing: a link that points to "/fr/node/1" is render cached, and is reused when the current language is English. Solvable by adding the "URL language" cache context to renderer.config.required_cache_contexts.
2. URL alias path processing: if we link to /node/1 and the alias is /foo at time A, and is render cached, yet at time B it is changed to /bar, then we don't invalidate it, hence the cached link (/foo) will result in a 404. This is a general problem we've never really solved (see #2480077: Using the URL alias UI to change aliases doesn't do necessary invalidations: path aliases don't have cache tags), and which you can kind of solve by using the Global Redirect. So I don't think we need to solve that specific problem here, but the general problem (no cacheability metadata) remains
3. more generally speaking, other outbound path/route processors may need cacheability metadata to be bubbled.

But that's the thing here: we can mitigate the problems/risks of the 99% cases (CSRF tokens via response filter + outbound language path processor by adding an additional required cache context) very easily, but doing so does not solve the general problem. One could argue that writing additional path/route processors is so very rare that it's okay for that to be kinda painful to get to work correctly in combination with render caching, but at the same time that is icky. On the other hand, solving it properly requires quiet widespread changes, which makes me lean towards solving the 99% here and then in a non-critical follow-up work on making it *better*.

Thoughts?

Yes, using always placeholders and the response filter to replace them should work.

IIRC even Ajax responses go via the response filter, so as long as it does not get somehow encoded, it should work ...

Indeed.

But I just thought of a case where it would break: if an e-mail containing links with CSRF tokens is being sent. Those e-mails aren't responses, so would never get processed by a response filter, so … damn :(

The key problem here is that we render URLs using the same context-unaware API for vastly different use cases: when sending e-mails or when sending HTML responses…

The key problem here is that we render URLs using the same context-unaware API for vastly different use cases: when sending e-mails or when sending HTML responses…

Could we introduce some basic replacement logic subsystem, which you can either use it when you render mails (maybe have a dedicated renderapi method to do so), or when you
render things via responses?

But URL rendering cannot specify cache contexts. Not without #2450993: Rendered Cache Metadata created during the main controller request gets lost at least, which I'm not sure is actually going to happen.

But I just thought of a case where it would break: if an e-mail containing links with CSRF tokens is being sent. Those e-mails aren't responses, so would never get processed by a response filter, so … damn :(

There's no use-case for this though.

If you send a CSRF-protected link via e-mail, there is only a small chance that it will be opened in the same session as the one that was used to generate it. Also if the token doesn't get replaced, the worst that happens is you get a 403 - same as if your session has expired/you opened the link in a different browser/on a different device. So I don't think we have to handle that case at all.

If you have an e-mail that links to an action, it'll have to be a link to a confirm form, or it'll be a special one-off link like password reset. Either way what we do here doesn't affect that.

Of course you're right.

I'll get a patch going for this.

What are the next steps on this issue? Is this issue somehow actionable in any form?

Yeah, sorry for the silence on my part. I said in #80 I was going to work on it, but then got stuck. Attempting my rough outline.

The part I got stuck on was the bit where the CSRF outbound route processor needs to track somewhere somehow which the placeholders are that it uses, because the response subscriber will need to know those placeholders. And I don't see how we can do that.

I think we kind of need #2450993: Rendered Cache Metadata created during the main controller request gets lost for this to be resolved, and fortunately @Fabianx & @Crell came up with a good plan for that at DrupalCon Los Angeles 1.5 week ago.

Assigning to @Fabianx for feedback.

@Fabianx, can you provide a very short explanation of how we can resolve this using #2450993, and link to another comment with more details?

+1 to #2450993: Rendered Cache Metadata created during the main controller request gets lost being very valuable to fix regardless of this issue. As to whether or not it should be critical, I'm not sure yet. I'm curious if this patch passes tests, and if so, whether something like it would be enough to then make the rest of this issue and that one both be not critical.

Ok, that's not looking promising. How about this?

Assume that was a copy/pasta error.

#90 is green, but needs new tests to prove it fixes the bug. @Fabianx: unassigning you in case someone else wants to work on those tests or otherwise comment on #90, but re-assign to yourself if you want to.

Raising priority to critical, because so far, I'm still unconvinced about #2450993: Rendered Cache Metadata created during the main controller request gets lost remaining critical, and if that ends up getting downgraded, this specific bug is still critical.

Now, #90 is definitely ugly, and could result in false positives (e.g., token parameters for something other than CSRF tokens resulting in not caching things that could be), but I'm curious what people think about this as an interim solution, and then removing it once there's a better fix, such as #2450993: Rendered Cache Metadata created during the main controller request gets lost, whether that happens before 8.0.0 or after. Despite this still needing tests, leaving at "Needs review" to gather that feedback.

What about changing csrf-tokens to be prefixed with ?token=csrf-[hash] for now, that is an internal detail anyway, but it would reduce false-positives to 0.

Its odd in the beginning that we don't use token-csrf, given that it would semantically describe what its used for.

While working on this, I ended up creating #2512132: Make CSRF links cacheable as a separate issue, because it only addresses the "CSRF links break cacheability" part, not the security part, which is why this particular issue is critical.

With #94 splitting off cacheability, we can get better focus in this issue. Also updated the IS. So far, we've been mixing the cacheability discussion with the security discussion. So let's restate what the problems are:

1. If some code decides to show a URL with a CSRF token, but doesn't collect the cacheability metadata, i.e. calls either of \Drupal::url(), \Drupal::l(), Url::toString(), UrlGenerator::generateFromRoute(), etc. without setting the optional $collect_cacheability_metadata parameter to TRUE, then we don't get \Drupal\Core\Access\RouteProcessorCsrf's max-age = 0, and hence we end up caching the individual fragment of the page that this URL ends up in across users.
2. Worse, even if you are opting in to collecting cacheability metadata, but you're doing this as part of your controller, i.e. if you're doing early rendering, then … we still lose that metadata, and we still end up caching across users.

Problem 2 is the part that #2450993: Rendered Cache Metadata created during the main controller request gets lost fixes. Yay!

But that still leaves us with point 1, which is a quite nasty problem as well. The reason that parameter is optional is simple: we didn't want to break BC for URL generation.

As far as I can tell, we have only two options for solving problem 1:

1. Break BC for URL generation, and remove those optional parameters, set them to TRUE always.
2. Have the URL generation system call out to the renderer, and if a render context is currently active, update the render context directly. I.e. something like RendererInterface::updateRenderContext(BubbleableMetadata). This violates quite a lot of principles, because it'll mean tying coupling the URL generation system with the render system.

P.S.: if the many ways to generate URLs make you sad, see #2491981: There are too many ways to generate URLs and links.

Also completely redid the IS, since it was mostly discussing cacheability, which is now handled by #2512132: Make CSRF links cacheable. Also migrated the relevant parts of the IS from here to there.

Break BC for URL generation, and remove those optional parameters, set them to TRUE always.

Yeah mh, that sounds like a gigantic patch and tons of broken contrib for well, a case, which is in the majority not broken ... as the link just depends things which are part of the cache context anyway (remember, most URLs don't have CSRF checks, i mean you could pretty much limit those to entity operations and similar, places which are kinda controlled).

In case we really want to do that, I would vote for changing GeneratedUr to implement Psr\Http\Message\UriInterface
which then potentially makes us compatible again with over code out there.

Given that I fear that option 2) is kinda the more pratical one.

Just had a call about this (#95) with effulgentsia.

He had an idea to make proposal 2 not disruptive at all, which we discussed and together ended up on this proposal:

• Keep \Drupal\Core\Routing\UrlGenerator, unchanged, but make it a private service called e.g. url_generator.uncacheable
• Add \Drupal\Core\Render\UrlGenerator, make it the new url_generator service
• (This is similar to what we do with config.storage & config.storage.active.)
• (This also happens to be exactly what Fabian and Crell concluded at #2450993-24: Rendered Cache Metadata created during the main controller request gets lost, point 6. :))
• (Also: this is a great validation of the architecture: we have a flawed URL generation API, and the fix is filthy, but we can fix it by just swapping the service out for another implementation that thinly layers on top of the existing implementation, but also is coupled to the renderer. Isolated crappiness. Hurray! :))
• Don't add new methods to Renderer, just use the same technique that ThemeManager uses to allow theme preprocess functions to specify $variables['#attached']: call Renderer::render() with an in-place generated render array that contains the cacheability metadata. That'll put it in the renderer's context for us :)
• And in fact, the latter will also automatically cause exceptions (once #2450993: Rendered Cache Metadata created during the main controller request gets lost lands), complaining that no render context is active. And as of #2450993, that will only ever happen during cron runs generating e-mails or drush scripts. So in the exception, we can point those users to url_generator.uncacheable — which is also more optimal for those use cases, given they never care about caching anyway.

Did I capture that well, effulgentsia?

Nice!

Unassigning in case somebody wants to take this on, I thought I was going to be able to start working on this today, but that won't happen. I'll probably start working on it on Monday or Tuesday.

Given #97 was +1'd by both Fabian & dawehner, I think we have enough consensus to pursue this solution. Updated the IS.

Isolated crappiness++

Working on this now.

This implements #97.

Let's see what breaks.

1. +++ b/core/core.services.yml
   @@ -682,9 +682,15 @@ services:
   +  url_generator.uncacheable:
        class: Drupal\Core\Routing\UrlGenerator
        arguments: ['@router.route_provider', '@path_processor_manager', '@route_processor_manager', '@config.factory', '@request_stack']
   +    public: false
   

   Do we never want to allow to call it? Just curious

2. +++ b/core/lib/Drupal/Core/Render/UrlGenerator.php
   @@ -0,0 +1,121 @@
   +   */
   +  public function generate($name, $parameters = array(), $absolute = FALSE) {
   +    $options['absolute'] = $absolute;
   +    $generated_url = $this->generateFromRoute($name, $parameters, $options, TRUE);
   +    $this->bubble($generated_url);
   +    return $generated_url->getGeneratedUrl();
   +  }
   

   Doesn't bubbles this then two times, once in generateFromRoute and once in generate()?

(Working on fixing the failures and addressing #106.)

Note that the failures in #104 reflect the cache contexts that are missing in HEAD, where our tests have the wrong expectations.

The first step is to make this patch green. Then this patch will be blocked on #2450993: Rendered Cache Metadata created during the main controller request gets lost. Once that lands, we'll need to re-test this patch, and more wrong test expectations (i.e. missing cache contexts) will be revealed.

In other words: by fixing the root cause, we're finding where our expectations have been wrong, and where we've overlooked the cacheability of links. This all yields quite interesting information (as you will see in my next comment, stay tuned!).

While working on this, I found that a form's action attribute also uses a URL that is dynamically generated using <current>, hence making that per-route, and therefore effectively any form that doesn't really care about the current page ends up being cached for every page (well, route). See #2504115-2: AJAX forms should submit to $form['#action'] instead of <current>.

I'm curious whether the form #action should be then interested using some form of placeholder?

#109: that's exactly what I proposed there, see #2504115-2: AJAX forms should submit to $form['#action'] instead of <current> :)

This is turning out to be yet another treasure trove of hidden cacheability bugs!

CommentRssTest: new url.site cache context

The RSS feed is generated by views, which uses the render system. The feed contains a link to the site, which depends on url.site.

Updated the test expectations.

PageCacheTagsIntegrationTest: new route cache context

Because the route.* routes have been optimized to just route now that thanks to this patch something is adding the route cache context.
That something is the UserLoginBlock, which is calling getDestinationArray(), which calls generateFromRoute('<current>', …), which explains it.

Updated the test expectations.

ShortcutCacheTagsTest + ToolbarCacheContextsTest: new route cache context

shortcut_preprocess_page() is calling $query += \Drupal::destination()->getAsArray();, which calls generateFromRoute('<current>', …), which explains it. Except that that code should never have been executed, since it actually calls that regardless of whether it will be used. By simply moving that call into the place where it is actually being used.

Views with attached feeds (FrontPageTest): new url.site cache context

url.site from \Drupal\views\Plugin\views\display\Feed::attachTo() which builds an URL.

Updated the test expectations.

Pagers (FrontPageTest, PagerTest, ExposedFormTest): ne w route cache context

route from template_preprocess_pager(), which calls \Drupal::url('<current>'). I thought about changing <current> to <none>, because this really only needs a query string, not the underlying route. But <none> is only designed for fragment URLs, so, no luck.
But, it's easy enough to change it to ($options['query']) ? '?' . UrlHelper::buildQuery($options['query']) : '' — which hence makes the route cache context disappear. Which is a good thing; for example, otherwise any non-page View reused on many pages would need to be re-rendered for every single route it appears on.

Sorting (FieldWebTest, GlossaryTest): new route cache context.

Entirely analogous to pagers. Wanted to fix this analogously, but this one is not just a URL, but a link. So I can't use the same work-around. Instead, thsi should probably fix #2305013: Add a helper method to the Url class to render just the query string and fragment.

So, still expect the failures in FieldWebTest, GlossaryTest. If I didn't make mistakes, that should bring us down to 6 failures.

route from template_preprocess_pager(), which calls \Drupal::url(''). I thought about changing to , because this really only needs a query string, not the underlying route. But is only designed for fragment URLs, so, no luck.
But, it's easy enough to change it to ($options['query']) ? '?' . UrlHelper::buildQuery($options['query']) : '' — which hence makes the route cache context disappear. Which is a good thing; for example, otherwise any non-page View reused on many pages would need to be re-rendered for every single route it appears on.

As said on IRC, this would be just wrong. You need to render all existing query parameters as well, to have something working.

As said on IRC, this would be just wrong. You need to render all existing query parameters as well, to have something working.

That's actually already working:

    $options = array(
      'query' => pager_query_add_page($parameters, $element, $pager_max - 1),
    );

That pager_query_add_page() calls adds to the existing query args.

BUT you're absolutely right that the corresponding cache context (url.query_args) is missing. So that was yet another hidden cacheability bug! Fixed.

This interdiff beautifully shows how problematic the very legacy implementation of the Pager component really is.

Such noob.

Reminder: There's a @todo in UserLoginForm to enable caching for that once this landed.

Also, #2375695: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed will conflict with this on the page cache integration test.

@Wim:

Working a bit on this, feel free to claim it back any time.

Not yet, sorry

I'm trying to understand some of the test failure ... for now this is a reroll in order to get it working locally.

That was doable.

+++ b/core/modules/config/src/Tests/ConfigEntityListTest.php
@@ -38,7 +38,7 @@ protected function setUp() {
+  function ptestList() {

@@ -151,7 +151,7 @@ function testList() {
+  function ptestListUI() {

@@ -254,6 +254,7 @@ public function testPager() {
+    debug($this->getUrl());

@@ -267,6 +268,7 @@ public function testPager() {
+    debug($this->getUrl());

Mmh

Oh well, the actual patch does not have that any longer :)

On this for reals

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -2355,16 +2355,34 @@ protected function clickLink($label, $index = 0) {
+   *   A path from the internal browser content.T

Just a small nit but there is one extra T

1. +++ b/core/includes/pager.inc
   @@ -218,7 +218,7 @@ function template_preprocess_pager(&$variables) {
   -    $items['first']['href'] = \Drupal::url('<current>', [], $options);
   +    $items['first']['href'] = ($options['query']) ? '?' . UrlHelper::buildQuery($options['query']) : '';
   

   We clearly need to discuss whether changing this is right always. Are there usecases to embed Drupal HTML into other sites for example?

   We could also replace the implementation of ?

2. +++ b/core/includes/pager.inc
   @@ -311,6 +311,12 @@ function pager_query_add_page(array $query, $element, $index) {
   +  drupal_render($build);
   

   Let's not use the global function

3. +++ b/core/lib/Drupal/Core/Render/UrlGenerator.php
   @@ -0,0 +1,121 @@
   + * The cacheable URL generator; decorates the uncacheable URL generator.
   

   Well, its nat that the URL generator is cacheable, its rather a CacheableMetadataAwareUrlGenerator, right? We used to have done A LOT of work on the cacheable URL generator, which did some actual caching. This horrible that this was blocked

4. +++ b/core/lib/Drupal/Core/Render/UrlGenerator.php
   @@ -0,0 +1,121 @@
   +    $options['absolute'] = $absolute;
   +    $generated_url = $this->generateFromRoute($name, $parameters, $options, TRUE);
   +    $this->bubble($generated_url);
   +    return $generated_url->getGeneratedUrl();
   ...
   +    $generated_url = $this->urlGenerator->generateFromRoute($name, $parameters, $options, TRUE);
   +    $this->bubble($generated_url);
   +    return $collect_cacheability_metadata ? $generated_url : $generated_url->getGeneratedUrl();
   

   I still don't get why we need to bubble twice

#128

1. We clearly need to discuss whether changing this is right always. Are there usecases to embed Drupal HTML into other sites for example?

   Yes, but since #2514136: Add default clickjacking defense to core you'll have to do extra work for that already anyway, so yes, overriding the default implementation would do the trick.

2. Sure, let's use \Drupal::service('renderer')->render()
3. Better nuance, yes. Let's do that.
4. Indeed, we should NOT do:
   

   $this->bubble($generated_url);
   

   but instead:

   if (!$collect_cacheability_metadata) {
     $this->bubble($generated_url);
   }
   

+++ b/core/includes/pager.inc
@@ -218,7 +218,7 @@ function template_preprocess_pager(&$variables) {
-    $items['first']['href'] = \Drupal::url('<current>', [], $options);
+    $items['first']['href'] = ($options['query']) ? '?' . UrlHelper::buildQuery($options['query']) : '';

What I think we should do is to use and fix url generator to also take query strings into account, not just fragment

What I think we should do is to use and fix url generator to also take query strings into account, not just fragment

+1, but that can be a major follow-up I think.

This just fixes some test failures, reviews above still to be addressed.

.

More test fixes

#135.2: that's what I said in #129.4 already, so yes :)

Nice progress!

+++ b/core/modules/views/src/Plugin/views/pager/SqlBase.php
@@ -382,14 +382,8 @@ public function isCacheable() {
+    // This needs to play well with any other exposed item on the page.
+    return ['url.query_args'];

+++ b/core/modules/views/src/Plugin/views/style/Table.php
@@ -444,8 +444,8 @@ public function getCacheContexts() {
+        // This needs to play well with any other exposed item on the page.
+        $contexts[] = 'url.query_args';

The rendered link needs to play well with any other query parameter used on the page, like pager and exposed filter.

1. +++ b/core/modules/language/src/Tests/LanguageUrlRewritingTest.php
   @@ -59,7 +59,7 @@ protected function setUp() {
   -  function testUrlRewritingEdgeCases() {
   +  function atestUrlRewritingEdgeCases() {
   

   Cheater!

2. +++ b/core/modules/views_ui/src/Tests/PreviewTest.php
   @@ -263,7 +263,8 @@ protected function clickPreviewLinkAJAX($url, $row_count) {
   +    $output = $this->drupalPost($url, 'application/vnd.drupal-ajax', $post);
   +    $result = Json::decode($output);
   

   Let's skip this change ...

3. +++ b/core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTest.php
   @@ -23,6 +23,7 @@
   + * @coversDefaultClass \Drupal\Core\Routing\UrlGenerator
     * @group Routing
   

   +1!!!

#2450993: Rendered Cache Metadata created during the main controller request gets lost just landed, which is why plach re-tested in #140. This is now completely unblocked :)

Just reupload the patch and let's see what fails now.

Let's get rid of a few failures

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -954,7 +954,11 @@ protected function initKernel(Request $request) {
+    // Make sure we use the uncacheable URL generator in tests, since most of
+    // the URL generations will happen outside of a render context.
+    $container->set('url_generator', $container->get('url_generator.uncacheable'));
+    return $container;

Looks like this didn't actually help resolve the ~1000 Drupal\simpletest\WebTestBase->buildUrl('user/login', Array) -style exceptions.

I wonder why.

Just a reroll for now

Fixed some failures

#2491981: There are too many ways to generate URLs and links is really relevant to this: we are wondering whether it should become a prerequisite. Stay tuned.

Fixed PHP unit and addressed part of #128.

This patch introduces a way to detect whether we are in a render context, which allows the URL generator to bubble cacheable metadata only when it's the case. This should fix quite some failures.

Addressed the remaining points of the review above and fixed more failures.

There is no need to remove that.
If I collect cacheable metadata myself I probably have reasons for doing so.

Yep, thanks, I had reverted that part unintentionally: #157 already re-included that.

That would be nice, but is unfortunately the wrong fix.

I suspected that, but I wanted to see what would happen. I'm going to figure out what's going on for the last failures tomorrow with @Wim.

The attached patch should fix the last failure, aside from those in https://qa.drupal.org/pifr/test/1088008.

I suspected that, but I wanted to see what would happen. I'm going to figure out what's going on for the last failures tomorrow with @Wim.

+1

+++ b/core/includes/pager.inc
@@ -176,6 +176,7 @@ function template_preprocess_pager(&$variables) {
+  $route = !empty($variables['pager']['#route']) ? $variables['pager']['#route'] : '<none>';

s/#route/#route_name/, because "route" means "route name + parameters".

It is great that its green

1. +++ b/core/includes/pager.inc
   @@ -176,6 +176,7 @@ function template_preprocess_pager(&$variables) {
      $quantity = $variables['pager']['#quantity'];
   +  $route = !empty($variables['pager']['#route']) ? $variables['pager']['#route'] : '<none>';
      global $pager_page_array, $pager_total;
   

   As wim was saying, let's use #route_name. ... Do we have any place to document that? Maybe on \Drupal\Core\Render\Element\Pager would actually make sense.

2. +++ b/core/includes/pager.inc
   @@ -311,6 +312,12 @@ function pager_query_add_page(array $query, $element, $index) {
   +
   +  // This is is based on the entire current query string. We need to ensure
   +  // cacheability is affected accordingly.
   +  $build = ['#cache' => ['contexts' => ['url.query_args']]];
   +  \Drupal::service('renderer')->render($build);
   +
   

   I think we should rather put $variables['#cache']['contexts'][] = 'url.query_args'; into template_preprocess_page() and template_preprocess_mini_pager().

3. +++ b/core/includes/pager.inc
   @@ -311,6 +312,12 @@ function pager_query_add_page(array $query, $element, $index) {
   diff --git a/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php
   
   diff --git a/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php
   index e3ec321..c1e5a21 100644
   
   index e3ec321..c1e5a21 100644
   --- a/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php
   
   --- a/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php
   +++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php
   
   +++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php
   +++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php
   @@ -15,8 +15,8 @@
   
   @@ -15,8 +15,8 @@
    use Symfony\Component\HttpFoundation\Request;
    use Symfony\Component\HttpFoundation\Response;
    use Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent;
   -use Symfony\Component\HttpKernel\HttpKernelInterface;
    use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface;
   +use Symfony\Component\HttpKernel\HttpKernelInterface;
    
    /**
     * Exception subscriber for handling core default HTML error pages.
   

   Seems entirely out of scope ...

4. +++ b/core/lib/Drupal/Core/Render/CacheableMetadataAwareUrlGenerator.php
   @@ -0,0 +1,127 @@
   +/**
   + * The cacheable URL generator; decorates the uncacheable URL generator.
   + */
   +class CacheableMetadataAwareUrlGenerator implements UrlGeneratorInterface {
   

   See earlier comment, we should explain more and its not that the URL generator is cacheable.

5. +++ b/core/lib/Drupal/Core/Render/CacheableMetadataAwareUrlGenerator.php
   @@ -0,0 +1,127 @@
   +    $generated_url = $this->generateFromRoute($name, $parameters, $options, TRUE);
   +    $this->bubble($generated_url);
   ...
   +    $generated_url = $this->urlGenerator->generateFromRoute($name, $parameters, $options, TRUE);
   +    if (!$collect_cacheability_metadata) {
   +      $this->bubble($generated_url);
   +    }
   +    return $collect_cacheability_metadata ? $generated_url : $generated_url->getGeneratedUrl();
   

   I still think its wrong to bubble twice

6. +++ b/core/lib/Drupal/Core/Render/RendererInterface.php
   @@ -322,6 +322,14 @@ public function renderPlain(&$elements);
   +   * Checks whether a render context is active.
   +   *
   +   * @return bool
   +   *   TRUE if the renderer has a render context active, FALSE otherwise.
   +   */
   

   It should at least explain that this is rather @internal

7. +++ b/core/modules/comment/src/Tests/CommentPagerTest.php
   @@ -381,7 +381,12 @@ protected function clickLinkWithXPath($xpath, $arguments = array(), $index = 0)
   +      $path = $urls[$index]['href'];
   +      // Support relative links: links that don't have a path component (and
   +      // hence only a fragment or querystring), use the path component of the
   +      // current page.
   +      $path = (isset(parse_url($path)['path'])) ? $path : parse_url($this->getUrl())['path'] . $path;
   +      $url_target = $this->getAbsoluteUrl($path);
   

   With the change to getAbsoluteUrl, I think this entire part is not needed.

8. +++ b/core/modules/simpletest/tests/src/Unit/WebTestBaseTest.php
   index 87730ff..bd78ec6 100644
   --- a/core/modules/system/src/Tests/Pager/PagerTest.php
   
   --- a/core/modules/system/src/Tests/Pager/PagerTest.php
   +++ b/core/modules/system/src/Tests/Pager/PagerTest.php
   
   +++ b/core/modules/system/src/Tests/Pager/PagerTest.php
   @@ -65,7 +65,7 @@ function testActiveClass() {
   -    $this->drupalGet($GLOBALS['base_root'] . $elements[0]['href'], array('external' => TRUE));
   +    $this->drupalGet($GLOBALS['base_root'] . parse_url($this->getUrl())['path'] . $elements[0]['href'], array('external' => TRUE));
   
   @@ -77,18 +77,21 @@ protected function testPagerQueryParametersAndCacheContext() {
   -    $this->drupalGet($GLOBALS['base_root'] . $elements[0]['href'], array('external' => TRUE));
   +    $this->drupalGet($GLOBALS['base_root'] . parse_url($this->getUrl())['path'] . $elements[0]['href'], array('external' => TRUE));
   ...
   -    $this->drupalGet($GLOBALS['base_root'] . $elements[0]['href'], array('external' => TRUE));
   +    $this->drupalGet($GLOBALS['base_root'] . parse_url($this->getUrl())['path'] . $elements[0]['href'], array('external' => TRUE));
        $this->assertText(t('Pager calls: 2'), 'Second link call to pager shows 2 calls.');
   

   Can't we reuse getAbsoluteUrl?

9. +++ b/core/modules/system/system.module
   @@ -658,7 +658,7 @@ function system_js_settings_alter(&$settings, AttachedAssetsInterface $assets) {
   -  Url::fromRoute('<front>', [], array('script' => &$scriptPath, 'prefix' => &$pathPrefix))->toString();
   +  Url::fromRoute('<front>', [], array('script' => &$scriptPath, 'prefix' => &$pathPrefix))->toString(TRUE);
   

   Opened up a follow up for this piece of shit: #2527848: Try to get rid of Url::fromRoute in system_js_settings_alter()

Do we have any place to document that? Maybe on \Drupal\Core\Render\Element\Pager would actually make sense.

+1

I still think its wrong to bubble twice

The code you cited specifically prevents double bubbling.

@internal

+1

RE: follow-up-to-remove-that-piece-of-shit: +1000

Re-titling.

The comment-related test failures were caused by a sub-request being fired: @dawehner suggested to have a separate "global" render context for each request, which is what this patch is implementing. Let's see how many failures remain after reverting the wrong fix mentioned in #160. Reviews still to be addressed.

Yep, so a render context per request; to allow subrequests to have entirely separate render contexts. Makes total sense. Before #2450993: Rendered Cache Metadata created during the main controller request gets lost + this patch, that bug was simply being masked. It's been there all along.
(If a service contains state, like the Renderer does, then it needs to ensure it is different per request.)

The interdiff looks great! Basically only nits. Curious how many failures we'll have left after this :)

1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -22,6 +23,23 @@
   +   * @var \Drupal\Core\Render\RenderContext[]
   

   It's not an array, but an \SplObjectStorage.

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -22,6 +23,23 @@
   +  protected static $contextCollection;
   

   The patch looks strange because you moved this to the top.
   Can we move it to where protected static $context; used to be defined? That'd make the patch much more legible.

   Besides, we always list the injected services first, then the other protected class properties.

3. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -268,10 +289,10 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   -        static::$context->update($elements);
   +        $context->update($elements);
   

   Nice :)

4. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -546,23 +568,51 @@ public function hasRenderContext() {
   +    catch (\UnexpectedValueException $e) {
   +      $context = NULL;
   +    }
   

   When can this happen? I don't understand this bit.

5. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -546,23 +568,51 @@ public function hasRenderContext() {
   +  /**
   +   * Sets the current render context.
   +   *
   +   * @return $this
   +   */
   +  protected function setCurrentRenderContext(RenderContext $context = NULL) {
   

   Incomplete docblock.

   Also, why is NULL an acceptable variable? The docblock should say so.

6. +++ b/core/modules/views/tests/src/Unit/Controller/ViewAjaxControllerTest.php
   @@ -7,13 +7,15 @@
   -use Drupal\Tests\UnitTestCase;
   +  use Drupal\Core\Render\RenderContext;
   +  use Drupal\Tests\UnitTestCase;
    use Drupal\views\Ajax\ViewAjaxResponse;
    use Drupal\views\Controller\ViewAjaxController;
    use Symfony\Component\HttpFoundation\Request;
    use Symfony\Component\DependencyInjection\ContainerBuilder;
   +  use Symfony\Component\HttpFoundation\RequestStack;
    
   -/**
   +  /**
   

   Nit: a bunch of indentation problems. PHPStorm does this sometimes :(

This should fix a few more failures by adding the UncacheableUrl class and using to fix REST entity canonical URIs.

Reviews not addressed yet, sorry, focusing on test failures for now.

Edit: Aw, I forgot some crap in PageCacheTest, I'll fix it in the next patch.

More fixes

Hopefully more

MOAR fixes

Added a follow up to improve the DX: #2528598: Opt out of cacheablity protection on route level

And a few more

Some cleanup in the meantime.