Merge pull request #2665 from pulzarraider/tostring_escape

Fix possible XSS vulnerability

CHANGELOG.md

@@ -1,6 +1,9 @@
 CHANGELOG
 =========
 
+### 2015-01-05
+ * [BC BREAK] #2665 - text from Admin's toString method is escaped for html output before adding in flash message to prevent possible XSS vulnerability.
+
 ### 2014-08-08
  * added new form type ``sonata_type_model_autocomplete``
  * changed ``collection`` form type to ``sonata_type_native_collection``

Controller/CRUDController.php

@@ -289,7 +289,7 @@ class CRUDController extends Controller
                     'sonata_flash_success',
                     $this->admin->trans(
                         'flash_delete_success',
-                        array('%name%' => $this->admin->toString($object)),
+                        array('%name%' => $this->escapeHtml($this->admin->toString($object))),
                         'SonataAdminBundle'
                     )
                 );
@@ -305,7 +305,7 @@ class CRUDController extends Controller
                     'sonata_flash_error',
                     $this->admin->trans(
                         'flash_delete_error',
-                        array('%name%' => $this->admin->toString($object)),
+                        array('%name%' => $this->escapeHtml($this->admin->toString($object))),
                         'SonataAdminBundle'
                     )
                 );
@@ -375,7 +375,7 @@ class CRUDController extends Controller
                         'sonata_flash_success',
                         $this->admin->trans(
                             'flash_edit_success',
-                            array('%name%' => $this->admin->toString($object)),
+                            array('%name%' => $this->escapeHtml($this->admin->toString($object))),
                             'SonataAdminBundle'
                         )
                     );
@@ -397,7 +397,7 @@ class CRUDController extends Controller
                         'sonata_flash_error',
                         $this->admin->trans(
                             'flash_edit_error',
-                            array('%name%' => $this->admin->toString($object)),
+                            array('%name%' => $this->escapeHtml($this->admin->toString($object))),
                             'SonataAdminBundle'
                         )
                     );
@@ -619,7 +619,7 @@ class CRUDController extends Controller
                         'sonata_flash_success',
                         $this->admin->trans(
                             'flash_create_success',
-                            array('%name%' => $this->admin->toString($object)),
+                            array('%name%' => $this->escapeHtml($this->admin->toString($object))),
                             'SonataAdminBundle'
                         )
                     );
@@ -641,7 +641,7 @@ class CRUDController extends Controller
                         'sonata_flash_error',
                         $this->admin->trans(
                             'flash_create_error',
-                            array('%name%' => $this->admin->toString($object)),
+                            array('%name%' => $this->escapeHtml($this->admin->toString($object))),
                             'SonataAdminBundle'
                         )
                     );
@@ -1090,6 +1090,18 @@ class CRUDController extends Controller
         }
     }
 
+    /**
+     * Escape string for html output
+     *
+     * @param string $s
+     *
+     * @return string
+     */
+    protected function escapeHtml($s)
+    {
+        return htmlspecialchars($s, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8');
+    }
+
     /**
      * Get CSRF token
      *

Tests/Controller/CRUDControllerTest.php

@@ -960,7 +960,10 @@ class CRUDControllerTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals(array(), $this->session->getFlashBag()->all());
     }
 
-    public function testDeleteActionSuccess1()
+    /**
+     * @dataProvider getToStringValues
+     */
+    public function testDeleteActionSuccess1($expectedToStringValue, $toStringValue)
     {
         $object = new \stdClass();
 
@@ -971,9 +974,9 @@ class CRUDControllerTest extends \PHPUnit_Framework_TestCase
         $this->admin->expects($this->once())
             ->method('toString')
             ->with($this->equalTo($object))
-            ->will($this->returnValue('test'));
+            ->will($this->returnValue($toStringValue));
 
-        $this->expectTranslate('flash_delete_success', array('%name%' => 'test'));
+        $this->expectTranslate('flash_delete_success', array('%name%' => $expectedToStringValue), 'SonataAdminBundle');
 
         $this->admin->expects($this->once())
             ->method('isGranted')
@@ -991,7 +994,10 @@ class CRUDControllerTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals('list', $response->getTargetUrl());
     }
 
-    public function testDeleteActionSuccess2()
+    /**
+     * @dataProvider getToStringValues
+     */
+    public function testDeleteActionSuccess2($expectedToStringValue, $toStringValue)
     {
         $object = new \stdClass();
 
@@ -1004,7 +1010,12 @@ class CRUDControllerTest extends \PHPUnit_Framework_TestCase
             ->with($this->equalTo('DELETE'))
             ->will($this->returnValue(true));
 
-        $this->expectTranslate('flash_delete_success');
+        $this->admin->expects($this->once())
+            ->method('toString')
+            ->with($this->equalTo($object))
+            ->will($this->returnValue($toStringValue));
+
+        $this->expectTranslate('flash_delete_success', array('%name%' => $expectedToStringValue), 'SonataAdminBundle');
 
         $this->request->setMethod('POST');
         $this->request->request->set('_method', 'DELETE');
@@ -1018,7 +1029,10 @@ class CRUDControllerTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals('list', $response->getTargetUrl());
     }
 
-    public function testDeleteActionSuccessNoCsrfTokenProvider()
+    /**
+     * @dataProvider getToStringValues
+     */
+    public function testDeleteActionSuccessNoCsrfTokenProvider($expectedToStringValue, $toStringValue)
     {
         $this->csrfProvider = null;
 
@@ -1033,7 +1047,12 @@ class CRUDControllerTest extends \PHPUnit_Framework_TestCase
             ->with($this->equalTo('DELETE'))
             ->will($this->returnValue(true));
 
-        $this->expectTranslate('flash_delete_success');
+        $this->admin->expects($this->once())
+            ->method('toString')
+            ->with($this->equalTo($object))
+            ->will($this->returnValue($toStringValue));
+
+        $this->expectTranslate('flash_delete_success', array('%name%' => $expectedToStringValue), 'SonataAdminBundle');
 
         $this->request->setMethod('POST');
         $this->request->request->set('_method', 'DELETE');
@@ -1075,7 +1094,10 @@ class CRUDControllerTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals('SonataAdminBundle:CRUD:delete.html.twig', $this->template);
     }
 
-    public function testDeleteActionError()
+    /**
+     * @dataProvider getToStringValues
+     */
+    public function testDeleteActionError($expectedToStringValue, $toStringValue)
     {
         $object = new \stdClass();
 
@@ -1088,7 +1110,12 @@ class CRUDControllerTest extends \PHPUnit_Framework_TestCase
             ->with($this->equalTo('DELETE'))
             ->will($this->returnValue(true));
 
-        $this->expectTranslate('flash_delete_error');
+        $this->admin->expects($this->once())
+            ->method('toString')
+            ->with($this->equalTo($object))
+            ->will($this->returnValue($toStringValue));
+
+        $this->expectTranslate('flash_delete_error', array('%name%' => $expectedToStringValue), 'SonataAdminBundle');
 
         $this->assertLoggerLogsModelManagerException($this->admin, 'delete');
 
@@ -1194,7 +1221,10 @@ class CRUDControllerTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals('SonataAdminBundle:CRUD:edit.html.twig', $this->template);
     }
 
-    public function testEditActionSuccess()
+    /**
+     * @dataProvider getToStringValues
+     */
+    public function testEditActionSuccess($expectedToStringValue, $toStringValue)
     {
         $object = new \stdClass();
 
@@ -1223,7 +1253,12 @@ class CRUDControllerTest extends \PHPUnit_Framework_TestCase
             ->method('isValid')
             ->will($this->returnValue(true));
 
-        $this->expectTranslate('flash_edit_success');
+        $this->admin->expects($this->once())
+            ->method('toString')
+            ->with($this->equalTo($object))
+            ->will($this->returnValue($toStringValue));
+
+        $this->expectTranslate('flash_edit_success', array('%name%' => $expectedToStringValue), 'SonataAdminBundle');
 
         $this->request->setMethod('POST');
 
@@ -1234,7 +1269,10 @@ class CRUDControllerTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals('stdClass_edit', $response->getTargetUrl());
     }
 
-    public function testEditActionError()
+    /**
+     * @dataProvider getToStringValues
+     */
+    public function testEditActionError($expectedToStringValue, $toStringValue)
     {
         $object = new \stdClass();
 
@@ -1259,7 +1297,12 @@ class CRUDControllerTest extends \PHPUnit_Framework_TestCase
             ->method('isValid')
             ->will($this->returnValue(false));
 
-        $this->expectTranslate('flash_edit_error');
+        $this->admin->expects($this->once())
+            ->method('toString')
+            ->with($this->equalTo($object))
+            ->will($this->returnValue($toStringValue));
+
+        $this->expectTranslate('flash_edit_error', array('%name%' => $expectedToStringValue), 'SonataAdminBundle');
 
         $this->request->setMethod('POST');
 
@@ -1480,7 +1523,10 @@ class CRUDControllerTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals('SonataAdminBundle:CRUD:edit.html.twig', $this->template);
     }
 
-    public function testCreateActionSuccess()
+    /**
+     * @dataProvider getToStringValues
+     */
+    public function testCreateActionSuccess($expectedToStringValue, $toStringValue)
     {
         $object = new \stdClass();
 
@@ -1518,7 +1564,12 @@ class CRUDControllerTest extends \PHPUnit_Framework_TestCase
             ->method('isValid')
             ->will($this->returnValue(true));
 
-        $this->expectTranslate('flash_create_success');
+        $this->admin->expects($this->once())
+            ->method('toString')
+            ->with($this->equalTo($object))
+            ->will($this->returnValue($toStringValue));
+
+        $this->expectTranslate('flash_create_success', array('%name%' => $expectedToStringValue), 'SonataAdminBundle');
 
         $this->request->setMethod('POST');
 
@@ -1569,7 +1620,10 @@ class CRUDControllerTest extends \PHPUnit_Framework_TestCase
         $this->controller->createAction();
     }
 
-    public function testCreateActionError()
+    /**
+     * @dataProvider getToStringValues
+     */
+    public function testCreateActionError($expectedToStringValue, $toStringValue)
     {
         $this->admin->expects($this->once())
             ->method('isGranted')
@@ -1594,7 +1648,12 @@ class CRUDControllerTest extends \PHPUnit_Framework_TestCase
             ->method('isValid')
             ->will($this->returnValue(false));
 
-        $this->expectTranslate('flash_create_error');
+        $this->admin->expects($this->once())
+            ->method('toString')
+            ->with($this->equalTo($object))
+            ->will($this->returnValue($toStringValue));
+
+        $this->expectTranslate('flash_create_error', array('%name%' => $expectedToStringValue), 'SonataAdminBundle');
 
         $this->request->setMethod('POST');
 
@@ -2900,19 +2959,21 @@ class CRUDControllerTest extends \PHPUnit_Framework_TestCase
         return $this->csrfProvider;
     }
 
-    private function expectTranslate()
+    public function getToStringValues()
     {
-        $args = func_get_args();
-
-        // creates equalTo of all arguments passed to this function
-        $phpunit = $this; // PHP 5.3 compatibility
-        $argsCheck = array_map(function ($item) use ($phpunit) {
-            return $phpunit->equalTo($item);
-        }, func_get_args());
+        return array(
+            array('', ''),
+            array('Foo', 'Foo'),
+            array('&lt;a href=&quot;http://foo&quot;&gt;Bar&lt;/a&gt;', '<a href="http://foo">Bar</a>'),
+            array('&lt;&gt;&amp;&quot;&#039;abcdefghijklmnopqrstuvwxyz*-+.,?_()[]\/', '<>&"\'abcdefghijklmnopqrstuvwxyz*-+.,?_()[]\/'),
+        );
+    }
 
-        $mock = $this->admin->expects($this->once())->method('trans');
-        // passes all arguments to the 'with' of the $admin->trans method
-        $mock = call_user_func_array(array($mock, 'with'), $argsCheck);
-        $mock->will($this->returnValue($args[0]));
+    private function expectTranslate($id, array $parameters = array(), $domain = null, $locale = null)
+    {
+        $this->admin->expects($this->once())
+            ->method('trans')
+            ->with($this->equalTo($id), $this->equalTo($parameters), $this->equalTo($domain), $this->equalTo($locale))
+            ->will($this->returnValue($id));
     }
 }