bug #3873 Fix premature loop exit in Security Policy lookup of allowed methods/properties (YSaxon)

This PR was squashed before being merged into the 2.x branch.

Discussion
----------

Fix premature loop exit in Security Policy lookup of allowed methods/properties

The current security policy logic exits too soon when checking permissions for allowed classes and their methods/properties, causing false negatives in situations involving classes related by inheritance.

Consider the following configuration:

```
'methods' => [
  'App\BasicCollection' => ['sortAlphabetically'],
  'App\AdvancedCollection'=> ['sortByTimestamp'],
],
```
where `AdvancedCollection` is a subclass of `BasicCollection`, and `mylist` is an instance of `AdvancedCollection`

If you try to call `{{ mylist.sortByTimestamp() }}`, the current code will first match `mylist` against `App\BasicCollection`. Since `sortByTimestamp` is not an allowed method for `App\BasicCollection`, the code will exit the loop and incorrectly deny access. It will never get to checking `App\AdvancedCollection`.

Note that reordering classes in the config can't solve this issue. If you flipped the order, then it would fail for `{{ mylist.sortAlphabetically() }}` instead.

This pull request fixes the issue by only exiting the loop early when both the class and method/property match.

Commits
-------

5e1838db Fix premature loop exit in Security Policy lookup of allowed methods/properties

src/Sandbox/SecurityPolicy.php

@@ -94,9 +94,8 @@ final class SecurityPolicy implements SecurityPolicyInterface
         $allowed = false;
         $method = strtr($method, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ', 'abcdefghijklmnopqrstuvwxyz');
         foreach ($this->allowedMethods as $class => $methods) {
-            if ($obj instanceof $class) {
-                $allowed = \in_array($method, $methods);
-
+            if ($obj instanceof $class && \in_array($method, $methods)) {
+                $allowed = true;
                 break;
             }
         }
@@ -111,9 +110,8 @@ final class SecurityPolicy implements SecurityPolicyInterface
     {
         $allowed = false;
         foreach ($this->allowedProperties as $class => $properties) {
-            if ($obj instanceof $class) {
-                $allowed = \in_array($property, \is_array($properties) ? $properties : [$properties]);
-
+            if ($obj instanceof $class && \in_array($property, \is_array($properties) ? $properties : [$properties])) {
+                $allowed = true;
                 break;
             }
         }

tests/Extension/SandboxTest.php

@@ -36,6 +36,7 @@ class SandboxTest extends TestCase
             'name' => 'Fabien',
             'obj' => new FooObject(),
             'arr' => ['obj' => new FooObject()],
+            'child_obj' => new ChildClass(),
         ];
 
         self::$templates = [
@@ -56,6 +57,8 @@ class SandboxTest extends TestCase
             '1_range_operator' => '{{ (1..2)[0] }}',
             '1_syntax_error_wrapper' => '{% sandbox %}{% include "1_syntax_error" %}{% endsandbox %}',
             '1_syntax_error' => '{% syntax error }}',
+            '1_childobj_parentmethod' => '{{ child_obj.ParentMethod() }}',
+            '1_childobj_childmethod' => '{{ child_obj.ChildMethod() }}',
         ];
     }
 
@@ -410,6 +413,42 @@ EOF
         $this->assertSame('foo, bar', $twig->load('index')->render([]));
     }
 
+    public function testMultipleClassMatchesViaInheritanceInAllowedMethods()
+    {
+        $twig_child_first = $this->getEnvironment(true, [], self::$templates, [], [], [
+            'Twig\Tests\Extension\ChildClass' => ['ChildMethod'],
+            'Twig\Tests\Extension\ParentClass' => ['ParentMethod'],
+        ]);
+        $twig_parent_first = $this->getEnvironment(true, [], self::$templates, [], [], [
+            'Twig\Tests\Extension\ParentClass' => ['ParentMethod'],
+            'Twig\Tests\Extension\ChildClass' => ['ChildMethod'],
+        ]);
+
+        try {
+            $twig_child_first->load('1_childobj_childmethod')->render(self::$params);
+        } catch (SecurityError $e) {
+            $this->fail('This test case is malfunctioning as even the child class method which comes first is not being allowed.');
+        }
+
+        try {
+            $twig_parent_first->load('1_childobj_parentmethod')->render(self::$params);
+        } catch (SecurityError $e) {
+            $this->fail('This test case is malfunctioning as even the parent class method which comes first is not being allowed.');
+        }
+
+        try {
+            $twig_parent_first->load('1_childobj_childmethod')->render(self::$params);
+        } catch (SecurityError $e) {
+            $this->fail('checkMethodAllowed is exiting prematurely after matching a parent class and not seeing a method allowed on a child class later in the list');
+    }
+
+        try {
+            $twig_child_first->load('1_childobj_parentmethod')->render(self::$params);
+        } catch (SecurityError $e) {
+            $this->fail('checkMethodAllowed is exiting prematurely after matching a child class and not seeing a method allowed on its parent class later in the list');
+        }
+    }
+
     protected function getEnvironment($sandboxed, $options, $templates, $tags = [], $filters = [], $methods = [], $properties = [], $functions = [])
     {
         $loader = new ArrayLoader($templates);
@@ -421,6 +460,19 @@ EOF
     }
 }
 
+class ParentClass
+{
+    public function ParentMethod()
+    {
+    }
+}
+class ChildClass extends ParentClass
+{
+    public function ChildMethod()
+    {
+    }
+}
+
 class FooObject
 {
     public static $called = ['__toString' => 0, 'foo' => 0, 'getFooBar' => 0];