Skip to content

Skip the sandbox __toString check on arguments whose PHP parameter type cannot implicitly coerce to string#4823

Merged
fabpot merged 1 commit into
twigphp:3.xfrom
fabpot:sandbox-skip-tostring-walk-by-type
Jun 2, 2026
Merged

Skip the sandbox __toString check on arguments whose PHP parameter type cannot implicitly coerce to string#4823
fabpot merged 1 commit into
twigphp:3.xfrom
fabpot:sandbox-skip-tostring-walk-by-type

Conversation

@fabpot

@fabpot fabpot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

The sandbox visitor currently wraps every argument of every Twig callable with `CheckToStringNode.

As an optimization, we are now only wrapping when needed (based on the callable type hints). This is a conservative approach (untyped, mixed, string, array, iterable, object, Stringable, Traversable, self/static/parent and unknown class names all keep wrapping).

Here is a concrete before/after for template {{ demo(a, b) }} under the sandbox, with the following signature on the PHP side demo(int $a, string $b):

Before:

    yield $this->sandbox->ensureToStringAllowed(
       $this->env->getFunction('demo')->getCallable()(
           $this->sandbox->ensureToStringAllowed(($context["a"] ?? null), 1, $this->source),
           $this->sandbox->ensureToStringAllowed(($context["b"] ?? null), 1, $this->source),
       ),
       1, $this->source,
   );

After

   yield $this->sandbox->ensureToStringAllowed(
       $this->env->getFunction('demo')->getCallable()(
           ($context["a"] ?? null),                                                            // int: bare, skipped
           $this->sandbox->ensureToStringAllowed(($context["b"] ?? null), 1, $this->source),   // string: still wrapped
       ),
       1, $this->source,
   );

@fabpot fabpot changed the title Sandbox skip tostring walk by type Skip the sandbox __toString check on arguments whose PHP parameter type cannot implicitly coerce to string Jun 1, 2026
@fabpot fabpot force-pushed the sandbox-skip-tostring-walk-by-type branch 2 times, most recently from 9d29273 to 8458a85 Compare June 1, 2026 07:56
stof
stof reviewed Jun 1, 2026
View reviewed changes
Comment thread src/Util/CallableParameters.php Outdated
Comment thread src/Util/CallableParameters.php Outdated
@fabpot fabpot force-pushed the sandbox-skip-tostring-walk-by-type branch from 8458a85 to ad1ac44 Compare June 1, 2026 11:14
stof
stof reviewed Jun 1, 2026
View reviewed changes

@stof stof left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For final classes implementing Stringable, would it make sense to try checking the policy at compile-time and mark it safe if we know that the policy allows casting them to string ?
This might help with the kind of logic currently done by Drupal to optimize the sandbox: #4820 (comment)

Comment thread src/Util/CallableParameters.php Outdated
Comment thread src/Util/CallableParameters.php Outdated
@fabpot

fabpot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor Author

For final classes implementing Stringable, would it make sense to try checking the policy at compile-time and mark it safe if we know that the policy allows casting them to string ? This might help with the kind of logic currently done by Drupal to optimize the sandbox: #4820 (comment)

This is not possible as SecurityPolicy can be swapped, so we cannot make that decision at compile-time. The fact that security policies can be swapped is annoying for sure: 99a10384ff0

@fabpot fabpot force-pushed the sandbox-skip-tostring-walk-by-type branch from 3172595 to 6d5ef30 Compare June 2, 2026 11:58
@fabpot fabpot merged commit 2a2f058 into twigphp:3.x Jun 2, 2026
7 checks passed
@fabpot fabpot deleted the sandbox-skip-tostring-walk-by-type branch June 2, 2026 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@stof stof stof approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@fabpot @stof