From dbb8e18644bb11647c65cea91f4c1978497864f0 Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Mon, 20 May 2019 11:04:52 +1200 Subject: [PATCH] [CVE-2019-12149] Fixed potential SQL injection vulnerability in RestfulServer --- src/RestfulServer.php | 36 +++++++++------- tests/unit/RestfulServerTest.php | 45 +++++++++++++++++++- tests/unit/RestfulServerTest.yml | 4 +- tests/unit/Stubs/RestfulServerTestAuthor.php | 2 +- 4 files changed, 68 insertions(+), 19 deletions(-) diff --git a/src/RestfulServer.php b/src/RestfulServer.php index 38da66f..360f041 100644 --- a/src/RestfulServer.php +++ b/src/RestfulServer.php @@ -2,20 +2,20 @@ namespace SilverStripe\RestfulServer; -use SilverStripe\ORM\ArrayList; -use SilverStripe\Core\Config\Config; +use SilverStripe\CMS\Model\SiteTree; use SilverStripe\Control\Controller; -use SilverStripe\ORM\DataList; -use SilverStripe\ORM\DataObject; use SilverStripe\Control\Director; use SilverStripe\Control\HTTPRequest; +use SilverStripe\Core\Config\Config; +use SilverStripe\Core\Injector\Injector; +use SilverStripe\ORM\ArrayList; +use SilverStripe\ORM\DataList; +use SilverStripe\ORM\DataObject; use SilverStripe\ORM\SS_List; use SilverStripe\ORM\ValidationException; use SilverStripe\ORM\ValidationResult; use SilverStripe\Security\Member; use SilverStripe\Security\Security; -use SilverStripe\CMS\Model\SiteTree; -use SilverStripe\Core\Injector\Injector; /** * Generic RESTful server, which handles webservice access to arbitrary DataObjects. @@ -205,23 +205,29 @@ class RestfulServer extends Controller * @todo Access checking * * @param string $className - * @param Int $id + * @param int $id * @param string $relation * @return string The serialized representation of the requested object(s) - usually XML or JSON. */ protected function getHandler($className, $id, $relationName) { - $sort = ''; + $sort = ['ID' => 'ASC']; - if ($this->request->getVar('sort')) { - $dir = $this->request->getVar('dir'); - $sort = array($this->request->getVar('sort') => ($dir ? $dir : 'ASC')); + if ($sortQuery = $this->request->getVar('sort')) { + /** @var DataObject $singleton */ + $singleton = singleton($className); + // Only apply a sort filter if it is a valid field on the DataObject + if ($singleton && $singleton->hasDatabaseField($sortQuery)) { + $sort = [ + $sortQuery => $this->request->getVar('dir') === 'DESC' ? 'DESC' : 'ASC', + ]; + } } - $limit = array( - 'start' => $this->request->getVar('start'), - 'limit' => $this->request->getVar('limit') - ); + $limit = [ + 'start' => (int) $this->request->getVar('start'), + 'limit' => (int) $this->request->getVar('limit'), + ]; $params = $this->request->getVars(); diff --git a/tests/unit/RestfulServerTest.php b/tests/unit/RestfulServerTest.php index c188f35..0ad7684 100644 --- a/tests/unit/RestfulServerTest.php +++ b/tests/unit/RestfulServerTest.php @@ -49,7 +49,7 @@ class RestfulServerTest extends SapphireTest { parent::setUp(); Director::config()->set('alternate_base_url', $this->baseURI); - Security::setCurrentUser(null); + $this->logOut(); } public function testApiAccess() @@ -613,6 +613,49 @@ class RestfulServerTest extends SapphireTest ); } + public function testGetWithSortDescending() + { + $urlSafeClassname = $this->urlSafeClassname(RestfulServerTestAuthor::class); + $url = "{$this->baseURI}/api/v1/{$urlSafeClassname}?sort=FirstName&dir=DESC&fields=FirstName"; + + $response = Director::test($url); + $results = Convert::xml2array($response->getBody()); + + $this->assertSame('Author 4', $results[$urlSafeClassname][0]['FirstName']); + $this->assertSame('Author 3', $results[$urlSafeClassname][1]['FirstName']); + $this->assertSame('Author 2', $results[$urlSafeClassname][2]['FirstName']); + $this->assertSame('Author 1', $results[$urlSafeClassname][3]['FirstName']); + } + + public function testGetWithSortAscending() + { + $urlSafeClassname = $this->urlSafeClassname(RestfulServerTestAuthor::class); + $url = "{$this->baseURI}/api/v1/{$urlSafeClassname}?sort=FirstName&dir=ASC&fields=FirstName"; + + $response = Director::test($url); + $results = Convert::xml2array($response->getBody()); + + $this->assertSame('Author 1', $results[$urlSafeClassname][0]['FirstName']); + $this->assertSame('Author 2', $results[$urlSafeClassname][1]['FirstName']); + $this->assertSame('Author 3', $results[$urlSafeClassname][2]['FirstName']); + $this->assertSame('Author 4', $results[$urlSafeClassname][3]['FirstName']); + } + + public function testGetSortsByIdWhenInvalidSortColumnIsProvided() + { + $urlSafeClassname = $this->urlSafeClassname(RestfulServerTestAuthor::class); + $url = "{$this->baseURI}/api/v1/{$urlSafeClassname}?sort=Surname&dir=DESC&fields=FirstName"; + + $response = Director::test($url); + + $results = Convert::xml2array($response->getBody()); + + $this->assertSame('Author 1', $results[$urlSafeClassname][0]['FirstName']); + $this->assertSame('Author 2', $results[$urlSafeClassname][1]['FirstName']); + $this->assertSame('Author 3', $results[$urlSafeClassname][2]['FirstName']); + $this->assertSame('Author 4', $results[$urlSafeClassname][3]['FirstName']); + } + public function testApiAccessWithPOST() { $urlSafeClassname = $this->urlSafeClassname(RestfulServerTestAuthorRating::class); diff --git a/tests/unit/RestfulServerTest.yml b/tests/unit/RestfulServerTest.yml index 63dd21e..6bdc16a 100644 --- a/tests/unit/RestfulServerTest.yml +++ b/tests/unit/RestfulServerTest.yml @@ -46,10 +46,10 @@ SilverStripe\RestfulServer\Tests\Stubs\RestfulServerTestAuthor: author2: FirstName: Author 2 author3: - Firstname: Author 3 + FirstName: Author 3 author4: FirstName: Author 4 - RelatedAuthors: + RelatedAuthors: - =>SilverStripe\RestfulServer\Tests\Stubs\RestfulServerTestAuthor.author2 - =>SilverStripe\RestfulServer\Tests\Stubs\RestfulServerTestAuthor.author3 SilverStripe\RestfulServer\Tests\Stubs\RestfulServerTestAuthorRating: diff --git a/tests/unit/Stubs/RestfulServerTestAuthor.php b/tests/unit/Stubs/RestfulServerTestAuthor.php index 0145f03..faeff8b 100644 --- a/tests/unit/Stubs/RestfulServerTestAuthor.php +++ b/tests/unit/Stubs/RestfulServerTestAuthor.php @@ -12,7 +12,7 @@ class RestfulServerTestAuthor extends DataObject implements TestOnly private static $table_name = 'RestfulServerTestAuthor'; private static $db = array( - 'Name' => 'Text', + 'FirstName' => 'Text', ); private static $many_many = array(