Sorry, we don't support your browser.  Install a modern browser
This post is closed.

Resolve $field->replace() recursively#321

Let’s say I have:

Title: Hello {​{​ site.getCustomValue }}

In my template, I have:

echo $page->title()->replace();

If my getCustomValue() method returns a regular value, like "World", I get:

Hello World

…but if it returns another replace pattern, like "{​{​ site.title }} World", I get:

Hello {​{​ site.title }} World

I expect the resulting pattern to also be interpolated by the replace() method so I get:

Hello My Site World

I know I could do this:

echo $page->title()->replace()->replace();

…but that’s silly. If there’s yet another nested pattern, it’ll break again, and I can’t know how nested the user could need it to be.

A much more fitting solution would be to execute replace() as long as the value changes, with a check to avoid infinite loops.

3 months ago
1

This could become a security vulnerability because suddenly potentially untrusted content is parsed as a query.

Assuming getCustomValue() is a custom site method, what speaks against returning $site->title() . ' world' from it in this example? Or returning a string that is passed through Str::template() already?

3 months ago

Yes, it would be a vulnerability to evaluate PHP. But I don’t think Kirby does that in its Str::template() method? It simply has a regex that matches {​{​. If you put $site->title() in the content, the regex won’t match and you should just get the PHP code as plain text?

3 months ago

Str::template() calls the methods that are used in the queries, so it’s basically the same risk as evaluating PHP code.

3 months ago

You said that parsing the input would be problematic. Executing methods is part of the current functionality as well. What new risks does the recursion introduce? In order to execute something, you’d need to have access to the content folder and change the query. If someone untrusted has access to that - you have a bigger problem.

But anyway, Kirby classes could have a static queryBlocklist array, for example, that Str::template() checks in order to prevent destructive actions.

3 months ago

Let’s take the example from your first post: How can Kirby ensure that the $site->getCustomValue() method only ever returns a string that comes from a fully trusted source?

And before we discuss this to death: Have you considered the other options I suggested for your use case?

3 months ago

You haven’t suggested any other options?

Custom methods on the Site object can only be added with plugins. And the plugins on the site are explicitly installed by the owner/developer of the site. If you don’t trust a plugin, you sholdn’t be installing it in the first place. If you do trust it, then Kirby should trust it as well.

3 months ago

I have nothing to add to my previous comments. Please see my first comment for suggestions.

3 months ago

Oh, now that I read it a couple more times, I see that I’ve misunderstood. I thought your suggestions were an argument for the security vulnerability part in the beginning of your comment. I’m sorry!

I do something similar to what you suggested. In my case, $site->getCustomValue() can return $page->title() based on a condition, but the issue is that the title of the page could have patterns in it as well. The resulting string may also have patterns and so on.

I guess having a Str::template() call in my custom function makes the most sense. But my main concern is that I’ll be calling it even in the other 90% of cases where it’s not needed. The other thing is that the user can chain up several strings that return more templates, and it’s harder to know for certain how long that chain can be. So my idea was to unwrap {​{​ and }} until there’s no longer anything left to unwrap.

3 months ago

For better performance, your custom method could check if the string contains {​{​ and only call Str::template() if that’s the case.

To be honest, I feel like this feature would be a too large security risk for a core feature. You are right that custom site methods can be trusted as they were installed by the developer. But the deeper the query recursion goes, the more complex it gets to grasp. At some point it gets impossible to rule out that there is a risk in the specific use case.

Simple example: Let’s assume you have a query that looks like this:

Title: {​{​ page.fieldA }} {​{​ page.fieldB }} {​{​ page.fieldC }}

If an attacker finds a way to set the following values:

FieldA: {​{​
FieldB: site.aHarmfulMethod
FieldC: }}

Each one of these fields looks fine on its own (so for example a validator that filters queries wouldn’t find this attack). But together these fields suddenly become a query.

Of course this example is very theoretical. Murphy’s law tells me that something like this can happen in real life though. Like with any complex system, even skilled and careful devs cannot fully grasp the risks, which is really bad for good security.

Another argument: Evaluating queries until no more queries can be found can cause infinite loops. And even if the loop is not infinite, the performance will still suffer from a recursive check.

So all in all it’s much much safer to use Str::template() manually wherever queries are expected and wherever it can be ensured that the input string comes from a trusted source (e.g. editors with Panel access, not random people using frontend forms).

I hope I could make clear why I think that this feature shouldn’t be added to the Kirby core. I fully understand that it would make some use cases like yours simpler, but the risk for vulnerabilities, performance regressions and bugs is just too high.

If you still want this feature in your use case, you can use the new callback option that was added to Str::template() in Kirby 3.5.7. This can be used to recursively apply query templates to each return value.

With the different existing solutions in mind, I think we should close this feature ticket because of the mentioned disadvantages of a core implementation.

3 months ago

Untested example code:

$data = [
    'kirby' => kirby(),
    'site'  => kirby()->site(),
    'page'  => kirby()->page()
];

$callback = function (string $result) use (&$callback, $data) {
    return Str::template($result, $data, compact('callback'));
};

echo $callback($page->title());

Again, be careful. The risks I mentioned above still apply.

3 months ago
1

I agree with you, it’s unnecessary to have this in core. Thanks for the help!

3 months ago
Changed the status to
Archived
3 months ago