Add $entity->toUrl() and $entity->toLink() methods to allowed methods list in Twig sandbox policy

Seems reasonable, here's the patch.

Maybe while we're hear we should do EntityInterface::toLink() as well? Both aren't deprecated as are ::link() and ::url().

Added patch which includes ::toLink.

Yes could you create a change record for this @Chi?

Regarding change record, i believe there is a minor space issue:

{{ node.toLink ('Add new comment' | t, 'canonical', {fragment:'comment-form'}) }}

Should be

{{ node.toLink ('Add new comment'|t, 'canonical', {fragment:'comment-form'}) }}

This is a minor CS issue (there is no space in twig when using filters between |).

Hi, I agree with this one, normally do it on my sites anyway.

But we need a test here, otherwise we could easily lose it without knowing.

You an add to the existing \Drupal\Tests\Core\Template\TwigSandboxTest::testEntitySafeMethods

Added tests. No interdiff because the only change is in the test_only file.

+++ b/core/tests/Drupal/Tests/Core/Template/TwigSandboxTest.php
@@ -139,6 +139,20 @@ public function testEntitySafeMethods() {
+    $this->assertEquals($result, 'testing', 'Sandbox policy allows get() to be called.');
...
+    $this->assertEquals($result, 'testing', 'Sandbox policy allows get() to be called.');

Note: the expected result should actually be the first parameter ...

That is a very valid point, thanks @dawehner. I had simply copied and edited the existing tests. I have made the correction for my new tests, but left the existing tests as is because that is not part of the scope of this issue (but I am happy to make the change, if required, or create a new issue to address this).

Yeah let's not touch existing tests

1. Do we want entity.toUrl() + entity.toLink(), and not entity.url() + entity.link()? The latter seems a bit more logical?
2. I'd like to see test coverage for non-default link relation types.

Oh and

3. Test coverage for the case of the current user not being allowed to access the generated entity URL — what's the behavior in that case?

@Wim Leers,

1. re #19.1 I don't think so, those are deprecated methods and I'd rather not confuse people who are looking things up problems to follow a name change of an existing name on that object and make D9 changes more difficult for removing the BC layer.
2. re #19.2 What's an example of this?
3. re #19.3 The behavior would be the same as calling the method in PHP and passing the result to a variable in Twig. I think it may be worth a follow-up if something different from the current functionality of that method call is being tested.

1. Oh, I forgot about those. That'd indeed be confusing. Too bad that has to stand in the way of a better TX though.
2. $entity->toUrl('edit-form')
3. Okay, it may be possible today, but you can pass anything to Twig in a variable. That doesn't mean we need to consider API additions to Twig carefully. Many more people will create links like this once it's supported, so we need to have clear tests + documentation on how this is A) expected to behave, B) expected to be used.

   Addressing this a follow-up is IMO dangerous — because once the API is added, it can't be changed anymore.

1. Yes, it would be nicer TX... throwing an alternative out there(hopefully not a wrench), put a getUrl()/getLink() and it wouldn't need whitelisting and slightly better TX {{ entity.getUrl() }}, or is that just putting more cruft?
2. Cool, didn't know that's a thing:) agreed should get a test.
3. I take your point in being cautious about exposing API's for general use/larger audience. In D7 you could expose node view/edit/delete link's in Views yet it was possible to avoid access checks for performance. (which would mean the links would be visible, but going to them would yield a 403 without access). I'd expect the links/urls to render regardless of access and if I needed access checks that I'd check it before a call to {{ entity.toUrl('edit-form') }} , I bet this is not the case in D8 but yeah I guess I should test that here too.

Also, maybe we should double check the output for Exceptions like EntityMalformedException and UndefinedLinkTemplateException to ensure the templates don't explode in horrible ways?

Thanks for the review @Wim Leers, sorry didn't say that in #23 but much appreciated!

2. 👍 :) That's why we have peer reviews!
3. Aha, see your expectation is that it would render regardless of access! I'd have expected the opposite. Even if only for one reason: many entities have path aliases, and path aliases typically include the title, and therefore may result in unintentional information disclosure.

   This is why it's important to discuss this here first, agree on how it should work, and have tests to prove it works this way. There's a security aspect to it too!

[…] to ensure the templates don't explode in horrible ways

Yep, you're right — if I do {{ entity.toUrl('non-existent-link-relation-type') }} then it will indeed fail. We need to have tests to ensure a good TX in this case.

Yes, it would be nicer TX... throwing an alternative out there(hopefully not a wrench), put a getUrl()/getLink() and it wouldn't need whitelisting and slightly better TX {{ entity.getUrl() }}, or is that just putting more cruft?

Mh, so this would 3 methods on the entity objects all doing mostly the same. I guess this would treat DX against TX. Maybe we could provide something like { entity | url } or so?

Regarding point 3, its an old discussion, but in that case we should fix Entity.toLink. I remember its not the first time we have this discussion.

@Wim Leers In which file should I put the test coverage discussed in #19-2?

I'd like to see test coverage for non-default link relation types.

As we're only mocking the method, does this just mean adding a test that tries {{ entity.toUrl('edit') }} instead of {{ entity.toUrl }} and checking that the argument is passed to the mock?

#30 worked perfectly for me.

+++ b/core/tests/Drupal/Tests/Core/Template/TwigSandboxTest.php
@@ -139,6 +139,26 @@ public function testEntitySafeMethods() {
+    $entity = $this->getMock('Drupal\Core\Entity\EntityInterface');
+    $entity->expects($this->atLeastOnce())
+      ->method('toLink')
+      ->withConsecutive([], [NULL, 'edit'])
+      ->willReturnOnConsecutiveCalls('default-link', 'edit-link');
+    $result = $this->twig->render('{{ entity.toLink }}', ['entity' => $entity]);
+    $this->assertEquals('default-link', $result,  'Sandbox policy allows get() to be called.');
+    $result = $this->twig->render('{{ entity.toLink(null, "edit") }}', ['entity' => $entity]);
+    $this->assertEquals('edit-link', $result,  'Sandbox policy allows get() to be called.');

Do you mind actually returning something which is a link? Right now it is just some sort of random string :)

@dawehner do you mean a Drupal\Core\Link or something that looks like a link, e.g. '/this/is/a/link' or 'http://drupal.com'?

@Jo Fitzgerald Yeah something which is more of a realistic link. A link should be something like <a href="/node/123">This is a node</a>

I just tried this; If we want to return actual Link or Url objects here, we will need

- Drupal's Twig extension to convert them into strings. Fortunately it looks like all its service dependencies can be empty mocks.
- The unrouted URL assembler, to actually build the URL.
- A mocked request stack (which the assembler uses to get its base path).

It's probably not impossible (I got as far as point one), but there seems to be some some non-trivial mocking involved.

(Note that simply adding Drupal's twig extension in ::setUp also causes some other side effects, such as failing the ::testExtendedClass test.)

(Cross-post)

Return links (and URLs) as requested by @dawehner in #33.

Also removed redundant, erroneous messages.

Thanks for the tip about the method whitelist. I'd love to see toUrl added by default, as proposed here.

Question, will this patch be added to the Core anytime soon? Since every new update of Drupal I have to re-apply the patch over and over again.

#42: Good question. This has gone under the radar for a long time.

Looks like #19.2's reason for Needs tests was addressed in #30. Except it's just returning different values for consecutive calls, so it's not really testing this. But I think this is probably good enough nonetheless: testing this better requires setting up a lot more test infrastructure, and we don't have that elsewhere in \Drupal\Tests\Core\Template\TwigSandboxTest::testEntitySafeMethods() either.

Fixing CS violations and RTBC'ing. Let's see if front-end framework manager agrees this is sufficient.

This will still need a change record though.

+1 to add this from a theme system subsystem maintainer FWIW;)

Removing tag for CR because it's here: https://www.drupal.org/node/2908090

#44: YAY! (And oops 😊)

Updated CR to say "Drupal 8.8".

Thank you for this, really appreciate the effort for pushing this to the core. Looking forward to it 😊

+++ b/core/tests/Drupal/Tests/Core/Template/TwigSandboxTest.php
@@ -138,6 +138,26 @@ public function testEntitySafeMethods() {
+    $entity->expects($this->atLeastOnce())
+      ->method('toLink')
+      ->withConsecutive([], [NULL, 'edit'])
+      ->willReturnOnConsecutiveCalls('<a href="/node/123">This is a node</a>', '<a href="/">Home</a>');
+    $result = $this->twig->render('{{ entity.toLink }}', ['entity' => $entity]);
+    $this->assertEquals(htmlentities('<a href="/node/123">This is a node</a>'), $result);
+    $result = $this->twig->render('{{ entity.toLink(null, "edit") }}', ['entity' => $entity]);
+    $this->assertEquals(htmlentities('<a href="/">Home</a>'), $result);

In #26.3 I mentioned security aspects, but I missed one so far.

There's a security risk in doing this, because:

  public function toLink($text = NULL, $rel = 'canonical', array $options = []) {
    if (!isset($text)) {
      $text = $this->label();
    }
    $url = $this->toUrl($rel);
    $options += $url->getOptions();
    $url->setOptions($options);
    return new Link($text, $url);
  }

and

  public function __construct($text, Url $url) {
    $this->text = $text;
    $this->url = $url;
  }

and last but not least, fortunately in \Drupal\Core\Utility\LinkGenerator::generate():

    if (!($variables['text'] instanceof MarkupInterface)) {
      $variables['text'] = Html::escape($variables['text']);
    }

… but due to the mocking of toLink() (which I also called out in #43, we're not verifying that <script>alert('xss');</script> as the first toLink() parameter is being processed appropriately.

You could argue that it'd be stupid for a front-end developer to pass in a script tag, but there's no guarantee that they're not passing in user input.

Is there any progress of having this patch push to the core?

ugh what a twisty rabbit hole this sent me down... And I just wanted to dump an node link in a twig template :(

First the easy stuff. Ugh mockery mocks... But I think Wim's comments in #47aren't a big deal and mocks here are probably fine. We don't have to assert that TwigExtension renders Link's and Url's safely. We should already be confident of that based on TwigExtension's tests because even if we can't call toUrl and toLink directly from twig, passing links and urls to twig files is common practice. And in fact TwigExtensionTest has some basic assertions around GeneratedLink and Url so I _think_ we're ok.

The rest is a little ranty, because I started digging into the Url and Link classes... Sorry.

Confirming my understanding of how Link and Url would render lead me down the real rabbit hole though, passing through such issues as #2491981: There are too many ways to generate URLs and links and #2529560: Expand support for link objects. I made the naive assumption that Drupal would render Links and Url's in twig the same way. Probably throwing them into render array's and then churning that through the renderer.

What I found was Links do have RenderableInterface, TwigExtensions picks that up, and then through some twisty turny render array logic we get a rendered link out of LinkGenerator. Pretty much exactly like expected. Link also has a much simpler toString method that skips all the logic in Element\Link but still gets the escaping in LinkGenerator so that's safe too. Why the difference, who knows but the render array works so we're fine.

Where I was really surprised was with Url. It _has_ a render array method but its named different and ends up not actually implementing RenderableInterface so TwigExtension falls through to calling Url::toString. Since its not the interface method I wonder if its even used or just cruft of some bygone idea(follow up, it is only used by shortcut.module in core).

Anyways, the real WTF is like Link, toRenderArray and toString methods behave different. If we where getting the render array it would be wrapped in an access callback and toUrl() would follow Wim's expectation and apply an access check. But calling toString like we currently do means we get Joel's expectation. Truely we still have too many ways to many ways to generate links and URLs... or more accurately don't have a consistent way :(

As payment for that long rant, here's an updated patch addressing #50. I just converted them to prophecy because with all the new assertions it makes the test shorter with less repetition and a heck of a lot easier to follow. Also I'm not a fan of Mocks. :-D.

I also added an assertion that link handles both passing null and a string correctly since it was trivial and there's a couple phpcs fixes and a couple fixes to the older assertions shoved in there too. The argument order was reversed and gave incorrect failure messages. Sorry to the kittens but it was getting hacked up anyways. :-/

oh... moving to 9.1.x for targeting since that's where it would currently land.

I gave this patch a test and indeed, it lets us use {{ node.toUrl }} in a template quite nicely. (Considering that all entities implement toUrl(), this is a wonderful, generic way to print out any entity's URL in a template, and it solves #3184316: Add entity_url Twig function for easy generation of entity URLs, which I am closing as a duplicate.)

Unfortunately, if you try to make that an absolute URL -- for which I can absolutely imagine some legitimate use cases, as @webchick alluded to in #3184316-7: Add entity_url Twig function for easy generation of entity URLs -- it fails with an exception:

Twig\Sandbox\SecurityError: Calling "setAbsolute" method on a "Drupal\Core\Url" object is not allowed. in Drupal\Core\Template\TwigSandboxPolicy->checkMethodAllowed()

This happens whether you do {{ node.toUrl.setAbsolute }} or {% set absolute_url = node.toUrl().setAbsolute() %}. (Obviously the former would be far more preferable from a themer perspective.)

So I think we almost certainly want to fix that, or it will be a pretty big WTF for themers who want to print out absolute URLs. Therefore, I'm kicking this back and tagging for the various administrative changes it needs before it can be committed.

Re-titling, since we have retired the terms "whitelist" and "blacklist" in core.

I'd like to preserve my RTBC privileges, so I won't be the one to implement this...but one way we could fix #55 is to allow TwigSandboxPolicy::$allowed_methods to also contain fully-qualified method names, like Drupal\Core\Url::setAbsolute, and modify checkMethodAllowed() to search for those as well.

+1 for setAbsolute, and also setOption - provided it doesn't derail the scope too much, those are definitely needed to make this useful

If that requires a fairly major pivot, lets do those in a followup

I re-rolled this for the 9.2.x branch.

I also implemented setAbsolute and added the ability to add fully-qualified names... which is probably how it always should have worked, so I added a deprecation notice for not using FQNs.

I did not implement setOption because:

1. I don't know if the arguments would need to be sanitized or not and would rather explore that in a follow up issue
2. You can work around it with something like this: {{ entity.toUrl }}?myQuery=param

I left all the toLink changes in the patch, but I'm not convinced that toLink is needed/wanted.
The htmlentities() method in the tests has me worried because I'm afraid that it's going to render as escaped HTML rather than an actual hyperlink and that that is not the expected behavior. I'm not a render/twig expert though, so maybe it's fine.

+++ b/core/tests/Drupal/Tests/Core/Template/TwigSandboxTest.php
@@ -67,11 +67,12 @@ public function getTwigEntityDangerousMethods() {
+      ['{{ entity.setAbsolute }}'],

Whoops, this was not meant to be in the patch. However, this does remind me of a point I forgot to make in my last comment...

It's really difficult to test the FQN thing since I don't believe that the Twig system runs the security policy code if the method doesn't exist and since EntityInterface and Url don't share the setAbsolute method, the above does not return an error as expected.

There's a lot of good things going on here but the scope seems to be expanding quite a bit which doesn't feel right. We've got a entity sandbox changes, url sandbox changes, and now deprecation to sandbox behaviors. They're all kinda bundled up in the same place but its harder to discuss the impact of the changes. Think of the kittens.

That said, I like the fully qualified method change. There is still some security concerns from Wim that I'm not sure are fixed but this does feel like it buttons up some things that didn't feel right.

I agree with @neclimdul and suggest that we dial the scope back a bit here. I propose:

1. We add the fully-qualified change here, but only for Url::setAbsolute() and ::setOption().
2. We then open a follow-up to convert the other ones to fully-qualified, and possibly also deprecate the more permissive get, has, etc. prefixes.

Kicking back to "needs work" to readjust the scope.

Reroll for 9.2.x.

+++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
@@ -55,17 +57,29 @@ public function __construct() {
-      'id',
-      'label',
-      'bundle',
...
+      EntityInterface::class . '::id',
+      EntityInterface::class . '::label',
+      EntityInterface::class . '::bundle',

Is this possibly breaking? I think you're right that was the intention but if I had my own object I was throwing into twig and calling .id on because it just worked then it would break right?

Big +1 to this. It would make many things much cleaner and better.

There was already a CR for this, and I just gave it some updates, so removing that tag.

Haven't yet read the full issue history, nor reviewed the code.

Gave this a proper summary, removing that tag, too...

opened #3239105: Harden TwigSandbox methods with the hardening component of the current patch to discuss the impacts and solve some issues with the current patch.

Reroll for 10.1

Just run into this again after an internal discussion where we debated Twig vs PHP:

{{ url('entity.node.canonical', { 'node': node.id }) }}

or

$variables['node_url'] = $node->toUrl()->setAbsolute()->toString();

I couldn't believe there wasn't a simpler way, and then I found this issue again.

I would also note that the url() Twig function already assumes all URLs will be absolute:

    $options['absolute'] = TRUE;
    $generated_url = $this->urlGenerator->generateFromRoute($name, $parameters, $options, TRUE);

+++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
@@ -56,15 +58,29 @@ public function __construct() {
-      'id',
-      'label',
-      'bundle',
-      'get',
-      '__toString',
-      'toString',
+      EntityInterface::class . '::id',
+      EntityInterface::class . '::label',
+      EntityInterface::class . '::bundle',
+      EntityInterface::class . '::toLink',
+      EntityInterface::class . '::toUrl',

I think we could sidestep this and the BC and deprecation issues if we just assume that a method name (without ::) also means "any class"?