Skip to content
Snippets Groups Projects

Resolve #3239105 "Harden twigsandbox methods"

Open Resolve #3239105 "Harden twigsandbox methods"
issue/drupal-3239105:3239105-harden-twigsandbox-methods into 11.x
2 unresolved threads
Open James Gilliland requested to merge issue/drupal-3239105:3239105-harden-twigsandbox-methods into 11.x
2 unresolved threads

Closes #3239105

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
111 118 * The list of allowed methods on all objects.
112 119 */
113 120 public static function getMethodsAllowedOnAllObjects(): array {
  • Comment on lines 107 to 113
    Derek Wright
    Derek Wright @dww
    Developer

    Possible can of worms, maybe with BC implications, but this method no longer does what this comment or its name says. :sweat_smile: It's more like getAllowedMethods() or something, and some of the methods are "allowed on all objects" but many are not...

    Do we keep this method name and comment as-is, leave it to only return the truly global methods (::get, ::__toString and ::toString), and then refactor to have another static method for the interface-specific additions? Or do we rename this method, update the docs, and let it handle both sets?

  • A
    Alex Pott @alexpott
    Maintainer

    I would add a new method and make this one work the way the name implies it should.

  • Please register or sign in to reply
  • Derek Wright requested changes

    requested changes

  • Alex Pott @alexpott started a thread on the diff
    • core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
    119 'get',
    120 '__toString',
    121 'toString',
    122 ]);
    121 $allowed_methods = [];
    122 // Align the array, so we can check using isset() during checks.
    123 foreach (
    124 Settings::get('twig_sandbox_allowed_methods', [
    125 // Only allow idempotent methods.
    126 EntityInterface::class . '::id',
    127 EntityInterface::class . '::label',
    128 EntityInterface::class . '::bundle',
    129 LayoutDefinition::class . '::id',
    130 '::get',
    131 '::__toString',
    132 '::toString',
    • Comment on lines +130 to +132
      A
      Alex Pott @alexpott
      Maintainer

      Why do we have to add :: to the front here? Just for the explode later? This doesn't seem necessary and results in unnecessary return value and setting value changing. I think the setting could be:

           Settings::get('twig_sandbox_allowed_methods', [
        // Only allow idempotent methods.
        EntityInterface::class . '::id',
        EntityInterface::class . '::label',
        EntityInterface::class . '::bundle',
        LayoutDefinition::class . '::id',
        'get',
        '__toString',
        'toString',
     ]

      i.e we add support for class::method naming but we leave the all object methods alone. Also all the preprocessing of the array here could move to the TwigSandboxPolicy constructor or another method so we don't change the intention / return value of this method.

    • Derek Wright
      Derek Wright @dww
      Developer

      I'm a little confused by this pair of comments. If we have a single setting that contains both fully-qualified class::method pairs, and generic/global method names, how can we leave getMethodsAllowedOnAllObjects() alone? Will it start filtering out anything from the setting that contains :: and we do all that processing directly in another method called by the constructor?

      Maybe it's cleaner all around to add a new method and a new setting, something like Settings::get('twig_sandbox_allowed_class_methods')? Then this class maintains 4 lists (allowed_classes, allowed_methods, allowed_prefixes, allowed_class_methods), not just the 1st 3 of those, and when we're checking if something is allowed, we do the obvious thing? This MR will move id, label and bundle from allowed_methods to allowed_class_methods. New MRs (e.g. for [#2907810]) will add to allowed_class_methods by default, unless we come up with other method names that should truly be allowed on all classes.

    • Derek Wright
      Derek Wright @dww
      Developer

      p.s. Adding a new setting means we don't have to add a deprecation to change the behavior of the old one. :grinning:

    • Please register or sign in to reply
  • Pierre Rudloff added 850 commits

    added 850 commits

    Compare with previous version

    Toggle commit list
    • Please register or sign in to reply