Problem/Motivation

It was discovered in #2800737: Add TrashManager::undo to revert to previous revision that placing a link with a CSRF token in a drupal_set_message causes the link to not render correctly. The link has a token, but it's incorrect. While testing it was found that in a controller placing a CSRF link and a drupal_set_message with a CSRF displayed two different tokens, one that works, one that doesn't.

Proposed resolution

The issues looks similar to the one in forms with #2630920: _csrf_token is broken due to cacheability metadata integration, results in rendered links without valid CSRF tokens, so I guess the resolution would be similar?

Remaining tasks

User interface changes

API changes

Data model changes

Comments

timmillwood created an issue. See original summary.

wim leers’s picture

Issue tags: +Needs tests

A test to reproduce this would make it much easier to work on this.

amateescu’s picture

Status: Active » Needs review
Issue tags: -Needs tests
StatusFileSize
new1.77 KB

Here's a test :)

wim leers’s picture

This reminds me of #2382667: #post_render_callback's that result from other #post_render_calback are not processed btw: recursion in placeholders (that issue I linked is from before placeholders were called placeholders).

#2478483: Introduce placeholders (#lazy_builder) to replace #post_render_cache then updated that test and moved it to \Drupal\Tests\Core\Render\RendererPlaceholdersTest::testRecursivePlaceholder().

That test is obviously passing, so it probably isn't this. Though it feels related.

Status: Needs review » Needs work

The last submitted patch, 3: 2804327-test-only.patch, failed testing.

timmillwood’s picture

dawehner’s picture

So yeah, the problem is that we loose the attachments when rendering the links.
One thing we could do is

  1. Expand FormattableMarkup with cacheable metadata
  2. Make drupal_set_message() capable of storing that
  3. Leverage those attachments when rendering the messages
dawehner’s picture

@alexpott, @fabianx and @dawehner discussed that and well, drupal_set_message() supports render arrays, so we can add the required information on there:


$url = Url::fromRoute('meh')->toString(TRUE);

$build = [
  '#markup' => t('Meh @url', $url->getGeneratedUrl()),
];

CacheableMetadata::createFromObject($url)->applyTo($build);

just for this specific redirect.

berdir’s picture

Status: Needs work » Needs review
StatusFileSize
new2.3 KB

The IMHO easier workaround is to use renderPlain() as I suggested. It works fine, the test was just broken and the user *actually* didn't have access to run cron ;)

I think we should change the placeholder to be more obvious, like 'render-placeholder:' . $hash, so you see what is actually going on.

Note that using Link in a translatable message is wrong anyway, the correct approach is to use run cron. But that won't change anything, the problem is that \Drupal\Core\Render\MetadataBubblingUrlGenerator::generateFromRoute() always passes TRUE. We would need to have a way to call the inner, non-bubbling url generator directly.

I also propose this as a duplicate of #2630920: _csrf_token is broken due to cacheability metadata integration, results in rendered links without valid CSRF tokens. This is exactly the same problem.

timmillwood’s picture

Issue tags: +Dublin2016

Do we want to close this as the patch in #9 being the way to render CSRF links in drupal_set_message?

Do we want to commit the test to make sure it always works?

Do we want to build a better solution?

wim leers’s picture

Status: Needs review » Closed (duplicate)