Skip to content

Commit

Permalink
Fix possible XSS vulnerability
Browse files Browse the repository at this point in the history 
Fix possible XSS vulnerability in Admin's toString method used in flash messages.
  • Loading branch information
@pulzarraider
pulzarraider committed Jan 5, 2015
1 parent 3ad4f9f commit e5bd3b5
Show file tree
Hide file tree
Showing 3 changed files with 111 additions and 35 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
@@ -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``
Expand Down
24 changes: 18 additions & 6 deletions Controller/CRUDController.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ public function deleteAction($id)
'sonata_flash_success',
$this->admin->trans(
'flash_delete_success',
array('%name%' => $this->admin->toString($object)),
array('%name%' => $this->escapeHtml($this->admin->toString($object))),
'SonataAdminBundle'
)
);
Expand All @@ -305,7 +305,7 @@ public function deleteAction($id)
'sonata_flash_error',
$this->admin->trans(
'flash_delete_error',
array('%name%' => $this->admin->toString($object)),
array('%name%' => $this->escapeHtml($this->admin->toString($object))),
'SonataAdminBundle'
)
);
Expand Down Expand Up @@ -375,7 +375,7 @@ public function editAction($id = null)
'sonata_flash_success',
$this->admin->trans(
'flash_edit_success',
array('%name%' => $this->admin->toString($object)),
array('%name%' => $this->escapeHtml($this->admin->toString($object))),
'SonataAdminBundle'
)
);
Expand All @@ -397,7 +397,7 @@ public function editAction($id = null)
'sonata_flash_error',
$this->admin->trans(
'flash_edit_error',
array('%name%' => $this->admin->toString($object)),
array('%name%' => $this->escapeHtml($this->admin->toString($object))),
'SonataAdminBundle'
)
);
Expand Down Expand Up @@ -619,7 +619,7 @@ public function createAction()
'sonata_flash_success',
$this->admin->trans(
'flash_create_success',
array('%name%' => $this->admin->toString($object)),
array('%name%' => $this->escapeHtml($this->admin->toString($object))),
'SonataAdminBundle'
)
);
Expand All @@ -641,7 +641,7 @@ public function createAction()
'sonata_flash_error',
$this->admin->trans(
'flash_create_error',
array('%name%' => $this->admin->toString($object)),
array('%name%' => $this->escapeHtml($this->admin->toString($object))),
'SonataAdminBundle'
)
);
Expand Down Expand Up @@ -1090,6 +1090,18 @@ protected function validateCsrfToken($intention)
}
}

/**
* 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
*
Expand Down
119 changes: 90 additions & 29 deletions Tests/Controller/CRUDControllerTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -960,7 +960,10 @@ public function testDeleteActionAjaxError()
$this->assertEquals(array(), $this->session->getFlashBag()->all());
}

public function testDeleteActionSuccess1()
/**
* @dataProvider getToStringValues
*/
public function testDeleteActionSuccess1($expectedToStringValue, $toStringValue)
{
$object = new \stdClass();

Expand All @@ -971,9 +974,9 @@ public function testDeleteActionSuccess1()
$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')
Expand All @@ -991,7 +994,10 @@ public function testDeleteActionSuccess1()
$this->assertEquals('list', $response->getTargetUrl());
}

public function testDeleteActionSuccess2()
/**
* @dataProvider getToStringValues
*/
public function testDeleteActionSuccess2($expectedToStringValue, $toStringValue)
{
$object = new \stdClass();

Expand All @@ -1004,7 +1010,12 @@ public function testDeleteActionSuccess2()
->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');
Expand All @@ -1018,7 +1029,10 @@ public function testDeleteActionSuccess2()
$this->assertEquals('list', $response->getTargetUrl());
}

public function testDeleteActionSuccessNoCsrfTokenProvider()
/**
* @dataProvider getToStringValues
*/
public function testDeleteActionSuccessNoCsrfTokenProvider($expectedToStringValue, $toStringValue)
{
$this->csrfProvider = null;

Expand All @@ -1033,7 +1047,12 @@ public function testDeleteActionSuccessNoCsrfTokenProvider()
->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');
Expand Down Expand Up @@ -1075,7 +1094,10 @@ public function testDeleteActionWrongRequestMethod()
$this->assertEquals('SonataAdminBundle:CRUD:delete.html.twig', $this->template);
}

public function testDeleteActionError()
/**
* @dataProvider getToStringValues
*/
public function testDeleteActionError($expectedToStringValue, $toStringValue)
{
$object = new \stdClass();

Expand All @@ -1088,7 +1110,12 @@ public function testDeleteActionError()
->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');

Expand Down Expand Up @@ -1194,7 +1221,10 @@ public function testEditAction()
$this->assertEquals('SonataAdminBundle:CRUD:edit.html.twig', $this->template);
}

public function testEditActionSuccess()
/**
* @dataProvider getToStringValues
*/
public function testEditActionSuccess($expectedToStringValue, $toStringValue)
{
$object = new \stdClass();

Expand Down Expand Up @@ -1223,7 +1253,12 @@ public function testEditActionSuccess()
->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');

Expand All @@ -1234,7 +1269,10 @@ public function testEditActionSuccess()
$this->assertEquals('stdClass_edit', $response->getTargetUrl());
}

public function testEditActionError()
/**
* @dataProvider getToStringValues
*/
public function testEditActionError($expectedToStringValue, $toStringValue)
{
$object = new \stdClass();

Expand All @@ -1259,7 +1297,12 @@ public function testEditActionError()
->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');

Expand Down Expand Up @@ -1480,7 +1523,10 @@ public function testCreateAction()
$this->assertEquals('SonataAdminBundle:CRUD:edit.html.twig', $this->template);
}

public function testCreateActionSuccess()
/**
* @dataProvider getToStringValues
*/
public function testCreateActionSuccess($expectedToStringValue, $toStringValue)
{
$object = new \stdClass();

Expand Down Expand Up @@ -1518,7 +1564,12 @@ public function testCreateActionSuccess()
->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');

Expand Down Expand Up @@ -1569,7 +1620,10 @@ public function testCreateActionAccessDenied2()
$this->controller->createAction();
}

public function testCreateActionError()
/**
* @dataProvider getToStringValues
*/
public function testCreateActionError($expectedToStringValue, $toStringValue)
{
$this->admin->expects($this->once())
->method('isGranted')
Expand All @@ -1594,7 +1648,12 @@ public function testCreateActionError()
->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');

Expand Down Expand Up @@ -2904,19 +2963,21 @@ public function getCsrfProvider()
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));
}
}

0 comments on commit e5bd3b5

Please sign in to comment.