From 65239cd54ddde77a3b846f35ff05e0062256f352 Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Mon, 20 May 2019 15:42:44 +1200 Subject: [PATCH] [CVE-2019-12149] Fixed potential SQL injection vulnerability in RestfulServer --- code/RestfulServer.php | 18 ++- tests/unit/RestfulServerTest.php | 211 ++++++++++++++++++------------- tests/unit/RestfulServerTest.yml | 4 +- 3 files changed, 138 insertions(+), 95 deletions(-) diff --git a/code/RestfulServer.php b/code/RestfulServer.php index bc48df1..f7189ce 100644 --- a/code/RestfulServer.php +++ b/code/RestfulServer.php @@ -177,16 +177,22 @@ class RestfulServer extends Controller */ protected function getHandler($className, $id, $relationName) { - $sort = ''; + $sort = array('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 = array( + $sortQuery => $this->request->getVar('dir') === 'DESC' ? 'DESC' : 'ASC', + ); + } } $limit = array( - 'start' => $this->request->getVar('start'), - 'limit' => $this->request->getVar('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 ee6dfba..a9f49fb 100644 --- a/tests/unit/RestfulServerTest.php +++ b/tests/unit/RestfulServerTest.php @@ -1,6 +1,6 @@ objFromFixture('RestfulServerTest_Comment', 'comment1'); $page1 = $this->objFromFixture('RestfulServerTest_Page', 'page1'); - + // normal GET should succeed with $api_access enabled $url = "/api/v1/RestfulServerTest_Comment/" . $comment1->ID; $response = Director::test($url, null, null, 'GET'); $this->assertEquals($response->getStatusCode(), 200); - + $_SERVER['PHP_AUTH_USER'] = 'user@test.com'; $_SERVER['PHP_AUTH_PW'] = 'user'; - + // even with logged in user a GET with $api_access disabled should fail $url = "/api/v1/RestfulServerTest_Page/" . $page1->ID; $response = Director::test($url, null, null, 'GET'); $this->assertEquals($response->getStatusCode(), 401); - + unset($_SERVER['PHP_AUTH_USER']); unset($_SERVER['PHP_AUTH_PW']); } - + public function testApiAccessBoolean() { $comment1 = $this->objFromFixture('RestfulServerTest_Comment', 'comment1'); - + $url = "/api/v1/RestfulServerTest_Comment/" . $comment1->ID; $response = Director::test($url, null, null, 'GET'); $this->assertContains('', $response->getBody()); @@ -52,7 +52,7 @@ class RestfulServerTest extends SapphireTest $this->assertContains('getBody()); $this->assertContains('getBody()); } - + public function testAuthenticatedGET() { $thing1 = $this->objFromFixture('RestfulServerTest_SecretThing', 'thing1'); @@ -62,25 +62,25 @@ class RestfulServerTest extends SapphireTest $url = "/api/v1/RestfulServerTest_SecretThing/" . $thing1->ID; $response = Director::test($url, null, null, 'GET'); $this->assertEquals($response->getStatusCode(), 401); - + $_SERVER['PHP_AUTH_USER'] = 'user@test.com'; $_SERVER['PHP_AUTH_PW'] = 'user'; - + $url = "/api/v1/RestfulServerTest_Comment/" . $comment1->ID; $response = Director::test($url, null, null, 'GET'); $this->assertEquals($response->getStatusCode(), 200); - + unset($_SERVER['PHP_AUTH_USER']); unset($_SERVER['PHP_AUTH_PW']); } - + public function testAuthenticatedPUT() { $comment1 = $this->objFromFixture('RestfulServerTest_Comment', 'comment1'); - + $url = "/api/v1/RestfulServerTest_Comment/" . $comment1->ID; $data = array('Comment' => 'created'); - + $response = Director::test($url, $data, null, 'PUT'); $this->assertEquals($response->getStatusCode(), 401); // Permission failure @@ -92,21 +92,21 @@ class RestfulServerTest extends SapphireTest unset($_SERVER['PHP_AUTH_USER']); unset($_SERVER['PHP_AUTH_PW']); } - + public function testGETRelationshipsXML() { $author1 = $this->objFromFixture('RestfulServerTest_Author', 'author1'); $rating1 = $this->objFromFixture('RestfulServerTest_AuthorRating', 'rating1'); $rating2 = $this->objFromFixture('RestfulServerTest_AuthorRating', 'rating2'); - + // @todo should be set up by fixtures, doesn't work for some reason... $author1->Ratings()->add($rating1); $author1->Ratings()->add($rating2); - + $url = "/api/v1/RestfulServerTest_Author/" . $author1->ID; $response = Director::test($url, null, null, 'GET'); $this->assertEquals($response->getStatusCode(), 200); - + $responseArr = Convert::xml2array($response->getBody()); $ratingsArr = $responseArr['Ratings']['RestfulServerTest_AuthorRating']; $this->assertEquals(count($ratingsArr), 2); @@ -117,20 +117,20 @@ class RestfulServerTest extends SapphireTest $this->assertContains($rating1->ID, $ratingIDs); $this->assertContains($rating2->ID, $ratingIDs); } - + public function testGETManyManyRelationshipsXML() { // author4 has related authors author2 and author3 $author2 = $this->objFromFixture('RestfulServerTest_Author', 'author2'); $author3 = $this->objFromFixture('RestfulServerTest_Author', 'author3'); $author4 = $this->objFromFixture('RestfulServerTest_Author', 'author4'); - + $url = "/api/v1/RestfulServerTest_Author/" . $author4->ID . '/RelatedAuthors'; $response = Director::test($url, null, null, 'GET'); $this->assertEquals(200, $response->getStatusCode()); $arr = Convert::xml2array($response->getBody()); $authorsArr = $arr['RestfulServerTest_Author']; - + $this->assertEquals(count($authorsArr), 2); $ratingIDs = array( (int)$authorsArr[0]['ID'], @@ -143,10 +143,10 @@ class RestfulServerTest extends SapphireTest public function testPUTWithFormEncoded() { $comment1 = $this->objFromFixture('RestfulServerTest_Comment', 'comment1'); - + $_SERVER['PHP_AUTH_USER'] = 'editor@test.com'; $_SERVER['PHP_AUTH_PW'] = 'editor'; - + $url = "/api/v1/RestfulServerTest_Comment/" . $comment1->ID; $body = 'Name=Updated Comment&Comment=updated'; $headers = array( @@ -159,18 +159,18 @@ class RestfulServerTest extends SapphireTest $this->assertEquals($responseArr['ID'], $comment1->ID); $this->assertEquals($responseArr['Comment'], 'updated'); $this->assertEquals($responseArr['Name'], 'Updated Comment'); - + unset($_SERVER['PHP_AUTH_USER']); unset($_SERVER['PHP_AUTH_PW']); } - + public function testPOSTWithFormEncoded() { $comment1 = $this->objFromFixture('RestfulServerTest_Comment', 'comment1'); - + $_SERVER['PHP_AUTH_USER'] = 'editor@test.com'; $_SERVER['PHP_AUTH_PW'] = 'editor'; - + $url = "/api/v1/RestfulServerTest_Comment"; $body = 'Name=New Comment&Comment=created'; $headers = array( @@ -221,7 +221,7 @@ class RestfulServerTest extends SapphireTest $obj = Convert::json2obj($response->getBody()); $this->assertEquals($obj->ID, $comment1->ID); $this->assertEquals($obj->Comment, 'updated'); - + // by extension $url = sprintf("/api/v1/RestfulServerTest_Comment/%d.json", $comment1->ID); $body = '{"Comment":"updated"}'; @@ -234,18 +234,18 @@ class RestfulServerTest extends SapphireTest $obj = Convert::json2obj($response->getBody()); $this->assertEquals($obj->ID, $comment1->ID); $this->assertEquals($obj->Comment, 'updated'); - + unset($_SERVER['PHP_AUTH_USER']); unset($_SERVER['PHP_AUTH_PW']); } - + public function testPUTwithXML() { $comment1 = $this->objFromFixture('RestfulServerTest_Comment', 'comment1'); - + $_SERVER['PHP_AUTH_USER'] = 'editor@test.com'; $_SERVER['PHP_AUTH_PW'] = 'editor'; - + // by mimetype $url = "/api/v1/RestfulServerTest_Comment/" . $comment1->ID; $body = 'updated'; @@ -254,7 +254,7 @@ class RestfulServerTest extends SapphireTest $obj = Convert::xml2array($response->getBody()); $this->assertEquals($obj['ID'], $comment1->ID); $this->assertEquals($obj['Comment'], 'updated'); - + // by extension $url = sprintf("/api/v1/RestfulServerTest_Comment/%d.xml", $comment1->ID); $body = 'updated'; @@ -267,17 +267,17 @@ class RestfulServerTest extends SapphireTest $obj = Convert::xml2array($response->getBody()); $this->assertEquals($obj['ID'], $comment1->ID); $this->assertEquals($obj['Comment'], 'updated'); - + unset($_SERVER['PHP_AUTH_USER']); unset($_SERVER['PHP_AUTH_PW']); } - + public function testHTTPAcceptAndContentType() { $comment1 = $this->objFromFixture('RestfulServerTest_Comment', 'comment1'); - + $url = "/api/v1/RestfulServerTest_Comment/" . $comment1->ID; - + $headers = array('Accept' => 'application/json'); $response = Director::test($url, null, null, 'GET', null, $headers); $this->assertEquals($response->getStatusCode(), 200); // Success @@ -285,68 +285,68 @@ class RestfulServerTest extends SapphireTest $this->assertEquals($obj->ID, $comment1->ID); $this->assertEquals($response->getHeader('Content-Type'), 'application/json'); } - + public function testNotFound() { $_SERVER['PHP_AUTH_USER'] = 'user@test.com'; $_SERVER['PHP_AUTH_PW'] = 'user'; - + $url = "/api/v1/RestfulServerTest_Comment/99"; $response = Director::test($url, null, null, 'GET'); $this->assertEquals($response->getStatusCode(), 404); - + unset($_SERVER['PHP_AUTH_USER']); unset($_SERVER['PHP_AUTH_PW']); } - + public function testMethodNotAllowed() { $comment1 = $this->objFromFixture('RestfulServerTest_Comment', 'comment1'); - + $url = "/api/v1/RestfulServerTest_Comment/" . $comment1->ID; $response = Director::test($url, null, null, 'UNKNOWNHTTPMETHOD'); $this->assertEquals($response->getStatusCode(), 405); } - + public function testConflictOnExistingResourceWhenUsingPost() { $rating1 = $this->objFromFixture('RestfulServerTest_AuthorRating', 'rating1'); - + $url = "/api/v1/RestfulServerTest_AuthorRating/" . $rating1->ID; $response = Director::test($url, null, null, 'POST'); $this->assertEquals($response->getStatusCode(), 409); } - + public function testUnsupportedMediaType() { $_SERVER['PHP_AUTH_USER'] = 'user@test.com'; $_SERVER['PHP_AUTH_PW'] = 'user'; - + $url = "/api/v1/RestfulServerTest_Comment"; $data = "Comment||\/||updated"; // weird format $headers = array('Content-Type' => 'text/weirdformat'); $response = Director::test($url, null, null, 'POST', $data, $headers); $this->assertEquals($response->getStatusCode(), 415); - + unset($_SERVER['PHP_AUTH_USER']); unset($_SERVER['PHP_AUTH_PW']); } - + public function testXMLValueFormatting() { $rating1 = $this->objFromFixture('RestfulServerTest_AuthorRating', 'rating1'); - + $url = "/api/v1/RestfulServerTest_AuthorRating/" . $rating1->ID; $response = Director::test($url, null, null, 'GET'); $this->assertContains('' . $rating1->ID . '', $response->getBody()); $this->assertContains('' . $rating1->Rating . '', $response->getBody()); } - + public function testApiAccessFieldRestrictions() { $author1 = $this->objFromFixture('RestfulServerTest_Author', 'author1'); $rating1 = $this->objFromFixture('RestfulServerTest_AuthorRating', 'rating1'); - + $url = "/api/v1/RestfulServerTest_AuthorRating/" . $rating1->ID; $response = Director::test($url, null, null, 'GET'); $this->assertContains('', $response->getBody()); @@ -354,7 +354,7 @@ class RestfulServerTest extends SapphireTest $this->assertContains('getBody()); $this->assertNotContains('', $response->getBody()); $this->assertNotContains('', $response->getBody()); - + $url = "/api/v1/RestfulServerTest_AuthorRating/" . $rating1->ID . '?add_fields=SecretField,SecretRelation'; $response = Director::test($url, null, null, 'GET'); $this->assertNotContains('', $response->getBody(), @@ -363,7 +363,7 @@ class RestfulServerTest extends SapphireTest $this->assertNotContains('', $response->getBody(), '"add_fields" URL parameter filters out disallowed relations from $api_access' ); - + $url = "/api/v1/RestfulServerTest_AuthorRating/" . $rating1->ID . '?fields=SecretField,SecretRelation'; $response = Director::test($url, null, null, 'GET'); $this->assertNotContains('', $response->getBody(), @@ -372,7 +372,7 @@ class RestfulServerTest extends SapphireTest $this->assertNotContains('', $response->getBody(), '"fields" URL parameter filters out disallowed relations from $api_access' ); - + $url = "/api/v1/RestfulServerTest_Author/" . $author1->ID . '/Ratings'; $response = Director::test($url, null, null, 'GET'); $this->assertContains('', $response->getBody(), @@ -382,38 +382,38 @@ class RestfulServerTest extends SapphireTest 'Relation viewer on has-many filters out disallowed fields from $api_access' ); } - + public function testApiAccessRelationRestrictionsInline() { $author1 = $this->objFromFixture('RestfulServerTest_Author', 'author1'); - + $url = "/api/v1/RestfulServerTest_Author/" . $author1->ID; $response = Director::test($url, null, null, 'GET'); $this->assertNotContains('getBody(), 'Restricts many-many with api_access=false'); $this->assertNotContains('getBody(), 'Restricts has-many with api_access=false'); } - + public function testApiAccessRelationRestrictionsOnEndpoint() { $author1 = $this->objFromFixture('RestfulServerTest_Author', 'author1'); - + $url = "/api/v1/RestfulServerTest_Author/" . $author1->ID . "/ProfilePage"; $response = Director::test($url, null, null, 'GET'); $this->assertEquals(404, $response->getStatusCode(), 'Restricts has-one with api_access=false'); - + $url = "/api/v1/RestfulServerTest_Author/" . $author1->ID . "/RelatedPages"; $response = Director::test($url, null, null, 'GET'); $this->assertEquals(404, $response->getStatusCode(), 'Restricts many-many with api_access=false'); - + $url = "/api/v1/RestfulServerTest_Author/" . $author1->ID . "/PublishedPages"; $response = Director::test($url, null, null, 'GET'); $this->assertEquals(404, $response->getStatusCode(), 'Restricts has-many with api_access=false'); } - + public function testApiAccessWithPUT() { $rating1 = $this->objFromFixture('RestfulServerTest_AuthorRating', 'rating1'); - + $url = "/api/v1/RestfulServerTest_AuthorRating/" . $rating1->ID; $data = array( 'Rating' => '42', @@ -448,7 +448,7 @@ class RestfulServerTest extends SapphireTest '{"FirstName":"User","Email":"user@test.com"}]}', "Correct JSON formatting on a dataobjectset with field filter"); } - + public function testApiAccessWithPOST() { $url = "/api/v1/RestfulServerTest_AuthorRating"; @@ -492,6 +492,43 @@ class RestfulServerTest extends SapphireTest unset($_SERVER['PHP_AUTH_USER']); unset($_SERVER['PHP_AUTH_PW']); } + + /** @group wip */ + public function testGetWithSortDescending() + { + $url = '/api/v1/RestfulServerTest_Author?sort=FirstName&dir=DESC&fields=FirstName'; + $response = Director::test($url); + $results = Convert::xml2array($response->getBody()); + $this->assertSame('Author 4', $results['RestfulServerTest_Author'][0]['FirstName']); + $this->assertSame('Author 3', $results['RestfulServerTest_Author'][1]['FirstName']); + $this->assertSame('Author 2', $results['RestfulServerTest_Author'][2]['FirstName']); + $this->assertSame('Author 1', $results['RestfulServerTest_Author'][3]['FirstName']); + } + + /** @group wip */ + public function testGetWithSortAscending() + { + $url = '/api/v1/RestfulServerTest_Author?sort=FirstName&dir=ASC&fields=FirstName'; + $response = Director::test($url); + $results = Convert::xml2array($response->getBody()); + $this->assertSame('Author 1', $results['RestfulServerTest_Author'][0]['FirstName']); + $this->assertSame('Author 2', $results['RestfulServerTest_Author'][1]['FirstName']); + $this->assertSame('Author 3', $results['RestfulServerTest_Author'][2]['FirstName']); + $this->assertSame('Author 4', $results['RestfulServerTest_Author'][3]['FirstName']); + } + + /** @group wip */ + public function testGetSortsByIdWhenInvalidSortColumnIsProvided() + { + $url = '/api/v1/RestfulServerTest_Author?sort=Surname&dir=DESC&fields=FirstName'; + $response = Director::test($url); + $results = Convert::xml2array($response->getBody()); + $this->assertSame('Author 1', $results['RestfulServerTest_Author'][0]['FirstName']); + $this->assertSame('Author 2', $results['RestfulServerTest_Author'][1]['FirstName']); + $this->assertSame('Author 3', $results['RestfulServerTest_Author'][2]['FirstName']); + $this->assertSame('Author 4', $results['RestfulServerTest_Author'][3]['FirstName']); + } + } /** @@ -502,17 +539,17 @@ class RestfulServerTest extends SapphireTest class RestfulServerTest_Comment extends DataObject implements PermissionProvider,TestOnly { public static $api_access = true; - + public static $db = array( "Name" => "Varchar(255)", "Comment" => "Text" ); - + public static $has_one = array( 'Page' => 'RestfulServerTest_Page', 'Author' => 'RestfulServerTest_Author', ); - + public function providePermissions() { return array( @@ -521,22 +558,22 @@ class RestfulServerTest_Comment extends DataObject implements PermissionProvider 'DELETE_Comment' => 'Delete Comment Objects', ); } - + public function canView($member = null) { return true; } - + public function canEdit($member = null) { return Permission::checkMember($member, 'EDIT_Comment'); } - + public function canDelete($member = null) { return Permission::checkMember($member, 'DELETE_Comment'); } - + public function canCreate($member = null) { return Permission::checkMember($member, 'CREATE_Comment'); @@ -546,16 +583,16 @@ class RestfulServerTest_Comment extends DataObject implements PermissionProvider class RestfulServerTest_SecretThing extends DataObject implements TestOnly,PermissionProvider { public static $api_access = true; - + public static $db = array( "Name" => "Varchar(255)", ); - + public function canView($member = null) { return Permission::checkMember($member, 'VIEW_SecretThing'); } - + public function providePermissions() { return array( @@ -567,20 +604,20 @@ class RestfulServerTest_SecretThing extends DataObject implements TestOnly,Permi class RestfulServerTest_Page extends DataObject implements TestOnly { public static $api_access = false; - + public static $db = array( 'Title' => 'Text', 'Content' => 'HTMLText', ); - + public static $has_one = array( 'Author' => 'RestfulServerTest_Author', ); - + public static $has_many = array( 'TestComments' => 'RestfulServerTest_Comment' ); - + public static $belongs_many_many = array( 'RelatedAuthors' => 'RestfulServerTest_Author', ); @@ -589,21 +626,21 @@ class RestfulServerTest_Page extends DataObject implements TestOnly class RestfulServerTest_Author extends DataObject implements TestOnly { public static $api_access = true; - + public static $db = array( - 'Name' => 'Text', + 'FirstName' => 'Text', ); - + public static $many_many = array( 'RelatedPages' => 'RestfulServerTest_Page', 'RelatedAuthors' => 'RestfulServerTest_Author', ); - + public static $has_many = array( 'PublishedPages' => 'RestfulServerTest_Page', 'Ratings' => 'RestfulServerTest_AuthorRating', ); - + public function canView($member = null) { return true; @@ -622,28 +659,28 @@ class RestfulServerTest_AuthorRating extends DataObject implements TestOnly 'Rating' ) ); - + public static $db = array( 'Rating' => 'Int', 'SecretField' => 'Text', 'WriteProtectedField' => 'Text', ); - + public static $has_one = array( 'Author' => 'RestfulServerTest_Author', 'SecretRelation' => 'RestfulServerTest_Author', ); - + public function canView($member = null) { return true; } - + public function canEdit($member = null) { return true; } - + public function canCreate($member = null) { return true; diff --git a/tests/unit/RestfulServerTest.yml b/tests/unit/RestfulServerTest.yml index bb12d62..98ee014 100644 --- a/tests/unit/RestfulServerTest.yml +++ b/tests/unit/RestfulServerTest.yml @@ -46,7 +46,7 @@ RestfulServerTest_Author: author2: FirstName: Author 2 author3: - Firstname: Author 3 + FirstName: Author 3 author4: FirstName: Author 4 RelatedAuthors: =>RestfulServerTest_Author.author2,=>RestfulServerTest_Author.author3 @@ -63,4 +63,4 @@ RestfulServerTest_AuthorRating: SecretRelation: =>RestfulServerTest_Author.author1 RestfulServerTest_SecretThing: thing1: - Name: Unspeakable \ No newline at end of file + Name: Unspeakable