From 5f828149c37ee78bc6c84e98909b8cbc85fa404f Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Thu, 29 Aug 2013 13:59:45 +1200 Subject: [PATCH] BUG Fixed instances of loosely defined SQL predicates not qualified by table name Fixed duplicate SQL escaping on SiteTree::get_by_link --- code/controllers/CMSMain.php | 5 ++++- code/controllers/CMSPageAddController.php | 2 +- code/controllers/ContentController.php | 14 +++++++++----- code/controllers/ModelAsController.php | 16 ++++++++-------- code/model/ErrorPage.php | 4 ++-- code/model/SiteTree.php | 19 ++++++++++++++----- tasks/SiteTreeMaintenanceTask.php | 2 +- tests/model/SiteTreeTest.php | 4 ++-- 8 files changed, 41 insertions(+), 25 deletions(-) diff --git a/code/controllers/CMSMain.php b/code/controllers/CMSMain.php index 86cedb15..ede6d08c 100644 --- a/code/controllers/CMSMain.php +++ b/code/controllers/CMSMain.php @@ -814,7 +814,10 @@ class CMSMain extends LeftAndMain implements CurrentPageIdentifier, PermissionPr // Fall back to homepage record if(!$id) { $homepageSegment = RootURLController::get_homepage_link(); - $homepageRecord = DataObject::get_one('SiteTree', sprintf('"URLSegment" = \'%s\'', $homepageSegment)); + $homepageRecord = DataObject::get_one('SiteTree', sprintf( + '"SiteTree"."URLSegment" = \'%s\'', + Convert::raw2sql($homepageSegment) + )); if($homepageRecord) $id = $homepageRecord->ID; } diff --git a/code/controllers/CMSPageAddController.php b/code/controllers/CMSPageAddController.php index 06f5ae48..c1881a85 100644 --- a/code/controllers/CMSPageAddController.php +++ b/code/controllers/CMSPageAddController.php @@ -123,7 +123,7 @@ class CMSPageAddController extends CMSPageEditController { $suffix = isset($data['Suffix']) ? "-" . $data['Suffix'] : null; if(!$parentID && isset($data['Parent'])) { - $page = SiteTree:: get_by_link(Convert::raw2sql($data['Parent'])); + $page = SiteTree::get_by_link($data['Parent']); if($page) $parentID = $page->ID; } diff --git a/code/controllers/ContentController.php b/code/controllers/ContentController.php index f2b7a45d..9bc553ea 100644 --- a/code/controllers/ContentController.php +++ b/code/controllers/ContentController.php @@ -163,9 +163,10 @@ class ContentController extends Controller { // See ModelAdController->getNestedController() for similar logic if(class_exists('Translatable')) Translatable::disable_locale_filter(); // look for a page with this URLSegment - $child = $this->model->SiteTree->where(sprintf ( - "\"ParentID\" = %s AND \"URLSegment\" = '%s'", $this->ID, Convert::raw2sql(rawurlencode($action)) - ))->First(); + $child = $this->model->SiteTree->filter(array( + 'ParentID' => $this->ID, + 'URLSegment' => rawurlencode($action) + ))->first(); if(class_exists('Translatable')) Translatable::enable_locale_filter(); // if we can't find a page with this URLSegment try to find one that used to have @@ -258,7 +259,10 @@ class ContentController extends Controller { */ public function getMenu($level = 1) { if($level == 1) { - $result = DataObject::get("SiteTree", "\"ShowInMenus\" = 1 AND \"ParentID\" = 0"); + $result = SiteTree::get()->filter(array( + "ShowInMenus" => 1, + "ParentID" => 0 + )); } else { $parent = $this->data(); @@ -399,7 +403,7 @@ HTML; $this->httpError(410); } // The manifest should be built by now, so it's safe to publish the 404 page - $fourohfour = Versioned::get_one_by_stage('ErrorPage', 'Stage', '"ErrorCode" = 404'); + $fourohfour = Versioned::get_one_by_stage('ErrorPage', 'Stage', '"ErrorPage"."ErrorCode" = 404'); if($fourohfour) { $fourohfour->write(); $fourohfour->publish("Stage", "Live"); diff --git a/code/controllers/ModelAsController.php b/code/controllers/ModelAsController.php index 90fa2081..2563f35d 100644 --- a/code/controllers/ModelAsController.php +++ b/code/controllers/ModelAsController.php @@ -93,9 +93,9 @@ class ModelAsController extends Controller implements NestedController { $sitetree = DataObject::get_one( 'SiteTree', sprintf( - '"URLSegment" = \'%s\' %s', + '"SiteTree"."URLSegment" = \'%s\' %s', Convert::raw2sql(rawurlencode($URLSegment)), - (SiteTree::config()->nested_urls ? 'AND "ParentID" = 0' : null) + (SiteTree::config()->nested_urls ? 'AND "SiteTree"."ParentID" = 0' : null) ) ); if(class_exists('Translatable')) Translatable::enable_locale_filter(); @@ -146,16 +146,15 @@ class ModelAsController extends Controller implements NestedController { * @return SiteTree */ static public function find_old_page($URLSegment,$parentID = 0, $ignoreNestedURLs = false) { - $URLSegment = Convert::raw2sql(rawurlencode($URLSegment)); $useParentIDFilter = SiteTree::config()->nested_urls && $parentID; // First look for a non-nested page that has a unique URLSegment and can be redirected to. if(SiteTree::config()->nested_urls) { - $pages = DataObject::get( - 'SiteTree', - "\"URLSegment\" = '$URLSegment'" . ($useParentIDFilter ? ' AND "ParentID" = ' . (int)$parentID : '') - ); + $pages = SiteTree::get()->filter("URLSegment", rawurlencode($URLSegment)); + if($useParentIDFilter) { + $pages = $pages->filter("ParentID", (int)$parentID); + } if($pages && $pages->Count() == 1 && ($page = $pages->First())) { $parent = $page->ParentID ? $page->Parent() : $page; @@ -164,10 +163,11 @@ class ModelAsController extends Controller implements NestedController { } // Get an old version of a page that has been renamed. + $URLSegmentSQL = Convert::raw2sql(rawurlencode($URLSegment)); $query = new SQLQuery ( '"RecordID"', '"SiteTree_versions"', - "\"URLSegment\" = '$URLSegment' AND \"WasPublished\" = 1" . ($useParentIDFilter ? ' AND "ParentID" = ' . (int)$parentID : ''), + "\"URLSegment\" = '$URLSegmentSQL' AND \"WasPublished\" = 1" . ($useParentIDFilter ? ' AND "ParentID" = ' . (int)$parentID : ''), '"LastEdited" DESC', null, null, diff --git a/code/model/ErrorPage.php b/code/model/ErrorPage.php index ad6827e3..ac029c7a 100644 --- a/code/model/ErrorPage.php +++ b/code/model/ErrorPage.php @@ -50,7 +50,7 @@ class ErrorPage extends Page { */ public static function response_for($statusCode) { // first attempt to dynamically generate the error page - if($errorPage = DataObject::get_one('ErrorPage', "\"ErrorCode\" = $statusCode")) { + if($errorPage = DataObject::get_one('ErrorPage', "\"ErrorPage\".\"ErrorCode\" = $statusCode")) { Requirements::clear(); Requirements::clear_combined_files(); @@ -93,7 +93,7 @@ class ErrorPage extends Page { $code = $defaultData['ErrorCode']; $page = DataObject::get_one( 'ErrorPage', - sprintf("\"ErrorCode\" = '%s'", $code) + sprintf("\"ErrorPage\".\"ErrorCode\" = '%s'", $code) ); $pageExists = ($page && $page->exists()); $pagePath = self::get_filepath_for_errorcode($code); diff --git a/code/model/SiteTree.php b/code/model/SiteTree.php index cd6443a6..a9776176 100644 --- a/code/model/SiteTree.php +++ b/code/model/SiteTree.php @@ -310,11 +310,18 @@ class SiteTree extends DataObject implements PermissionProvider,i18nEntityProvid // Grab the initial root level page to traverse down from. $URLSegment = array_shift($parts); $sitetree = DataObject::get_one ( - 'SiteTree', "\"URLSegment\" = '$URLSegment'" . (self::config()->nested_urls ? ' AND "ParentID" = 0' : ''), $cache + 'SiteTree', + "\"SiteTree\".\"URLSegment\" = '$URLSegment'" . ( + self::config()->nested_urls ? ' AND "SiteTree"."ParentID" = 0' : '' + ), + $cache ); /// Fall back on a unique URLSegment for b/c. - if(!$sitetree && self::config()->nested_urls && $page = DataObject::get('SiteTree', "\"URLSegment\" = '$URLSegment'")->First()) { + if(!$sitetree + && self::config()->nested_urls + && $page = DataObject::get_one('SiteTree', "\"SiteTree\".\"URLSegment\" = '$URLSegment'", $cache) + ) { return $page; } @@ -335,7 +342,9 @@ class SiteTree extends DataObject implements PermissionProvider,i18nEntityProvid // Traverse down the remaining URL segments and grab the relevant SiteTree objects. foreach($parts as $segment) { $next = DataObject::get_one ( - 'SiteTree', "\"URLSegment\" = '$segment' AND \"ParentID\" = $sitetree->ID", $cache + 'SiteTree', + "\"SiteTree\".\"URLSegment\" = '$segment' AND \"SiteTree\".\"ParentID\" = $sitetree->ID", + $cache ); if(!$next) { @@ -405,7 +414,7 @@ class SiteTree extends DataObject implements PermissionProvider,i18nEntityProvid if ( !($page = DataObject::get_by_id('SiteTree', $arguments['id'])) // Get the current page by ID. && !($page = Versioned::get_latest_version('SiteTree', $arguments['id'])) // Attempt link to old version. - && !($page = DataObject::get_one('ErrorPage', '"ErrorCode" = \'404\'')) // Link to 404 page directly. + && !($page = DataObject::get_one('ErrorPage', '"ErrorPage"."ErrorCode" = \'404\'')) // Link to 404 page. ) { return; // There were no suitable matches at all. } @@ -1603,7 +1612,7 @@ class SiteTree extends DataObject implements PermissionProvider,i18nEntityProvid $existingPage = DataObject::get_one( 'SiteTree', - "\"URLSegment\" = '$this->URLSegment' $IDFilter $parentFilter" + "\"SiteTree\".\"URLSegment\" = '$this->URLSegment' $IDFilter $parentFilter" ); return !($existingPage); diff --git a/tasks/SiteTreeMaintenanceTask.php b/tasks/SiteTreeMaintenanceTask.php index 0c84fd54..66dbd949 100644 --- a/tasks/SiteTreeMaintenanceTask.php +++ b/tasks/SiteTreeMaintenanceTask.php @@ -10,7 +10,7 @@ class SiteTreeMaintenanceTask extends Controller { public function makelinksunique() { $badURLs = "'" . implode("', '", DB::query("SELECT URLSegment, count(*) FROM SiteTree GROUP BY URLSegment HAVING count(*) > 1")->column()) . "'"; - $pages = DataObject::get("SiteTree", "\"URLSegment\" IN ($badURLs)"); + $pages = DataObject::get("SiteTree", "\"SiteTree\".\"URLSegment\" IN ($badURLs)"); foreach($pages as $page) { echo "
  • $page->Title: "; diff --git a/tests/model/SiteTreeTest.php b/tests/model/SiteTreeTest.php index a1722bc4..46e0afc4 100644 --- a/tests/model/SiteTreeTest.php +++ b/tests/model/SiteTreeTest.php @@ -136,7 +136,7 @@ class SiteTreeTest extends SapphireTest { $oldMode = Versioned::get_reading_mode(); Versioned::reading_stage('Live'); - $checkSiteTree = DataObject::get_one("SiteTree", "\"URLSegment\" = 'get-one-test-page'"); + $checkSiteTree = DataObject::get_one("SiteTree", "\"SiteTree\".\"URLSegment\" = 'get-one-test-page'"); $this->assertEquals("V1", $checkSiteTree->Title); Versioned::set_reading_mode($oldMode); @@ -426,7 +426,7 @@ class SiteTreeTest extends SapphireTest { public function testReadArchiveDate() { $date = '2009-07-02 14:05:07'; Versioned::reading_archived_date($date); - DataObject::get('SiteTree', "\"ParentID\" = 0"); + DataObject::get('SiteTree', "\"SiteTree\".\"ParentID\" = 0"); Versioned::reading_archived_date(null); $this->assertEquals( Versioned::get_reading_mode(),