[!!!][BUGFIX] Severe data-loss on workspaces publishing action 21/45321/6
authorOliver Hader <oliver@typo3.org>
Wed, 16 Dec 2015 19:12:42 +0000 (20:12 +0100)
committerOliver Hader <oliver.hader@typo3.org>
Thu, 17 Dec 2015 22:21:15 +0000 (23:21 +0100)
If pages records in a given scenario are published this causes
a severe data-loss for the whole TYPO3 installation since all
records are deleted. Actually they are marked as deleted, but
that's not less problematic.

The scenario for this in a draft workspace is having reordered
sub-pages (move-placeholder) and a parent-page that is marked
for deletion. On publishing the parent-page, the delete process
iterates over all pages on the root-level due to some essential
missing checks and an implicit type-cast from null to interger
zero (0) on the pages.pid value.

The accordant places are validated now. In addition to that the
possibility to delete everything implicitly from the root-page
is disabled to prevent other programmatic flaws like this.

Resolves: #72273
Releases: master, 6.2
Change-Id: I175f220cc0939124e34713fff07685ba902ad385
Reviewed-on: https://review.typo3.org/45321
Reviewed-by: Alexander Opitz <opitz.alexander@googlemail.com>
Tested-by: Alexander Opitz <opitz.alexander@googlemail.com>
Reviewed-by: Oliver Hader <oliver.hader@typo3.org>
Tested-by: Oliver Hader <oliver.hader@typo3.org>
typo3/sysext/core/Classes/DataHandling/DataHandler.php
typo3/sysext/core/Tests/Unit/DataHandling/DataHandlerTest.php
typo3/sysext/version/Classes/Hook/DataHandlerHook.php
typo3/sysext/workspaces/Tests/Functional/DataHandling/Regular/Publish/DataSet/createPlaceholdersAndDeleteDraftParentPage.csv
typo3/sysext/workspaces/Tests/Functional/DataHandling/Regular/PublishAll/DataSet/createPlaceholdersAndDeleteDraftParentPage.csv

index 504b60a..bd10a23 100644 (file)
@@ -4890,7 +4890,9 @@ class DataHandler
                     $versionState = VersionState::cast($verRec['t3ver_state']);
                     if ($versionState->equals(VersionState::MOVE_POINTER)) {
                         $versionMovePlaceholder = BackendUtility::getMovePlaceholder($table, $uid, 'uid', $verRec['t3ver_wsid']);
-                        $this->deleteEl($table, $versionMovePlaceholder['uid'], true, $forceHardDelete);
+                        if (!empty($versionMovePlaceholder)) {
+                            $this->deleteEl($table, $versionMovePlaceholder['uid'], true, $forceHardDelete);
+                        }
                     }
                 }
             }
@@ -5073,6 +5075,11 @@ class DataHandler
      */
     public function deletePages($uid, $force = false, $forceHardDelete = false)
     {
+        $uid = (int)$uid;
+        if ($uid === 0) {
+               $this->newlog2('Deleting all pages starting from the root-page is disabled.', 'pages', 0, 0, 2);
+               return;
+        }
         // Getting list of pages to delete:
         if ($force) {
             // Returns the branch WITHOUT permission checks (0 secures that)
@@ -5146,9 +5153,11 @@ class DataHandler
     protected function copyMovedRecordToNewLocation($table, $uid)
     {
         if ($this->BE_USER->workspace > 0) {
-            // Check move placeholder at workspace
+            $originalRecord = BackendUtility::getRecord($table, $uid);
             $movePlaceholder = BackendUtility::getMovePlaceholder($table, $uid);
-            if ($movePlaceholder !== false) {
+            // Check whether target page to copied to is different to current page
+            // Cloning on the same page is superfluous and does not help at all
+            if (!empty($originalRecord) && !empty($movePlaceholder) && (int)$originalRecord['pid'] !== (int)$movePlaceholder['pid']) {
                 // If move placeholder exists, copy to new location
                 // This will create a New placeholder on the new location
                 // and a version for this new placeholder
index ab9b60b..e675289 100644 (file)
@@ -17,6 +17,7 @@ namespace TYPO3\CMS\Core\Tests\Unit\DataHandler;
 use TYPO3\CMS\Core\Authentication\BackendUserAuthentication;
 use TYPO3\CMS\Core\Database\DatabaseConnection;
 use TYPO3\CMS\Core\DataHandling\DataHandler;
+use TYPO3\CMS\Core\Tests\AccessibleObjectInterface;
 use TYPO3\CMS\Core\Tests\Unit\DataHandling\Fixtures\AllowAccessHookFixture;
 use TYPO3\CMS\Core\Tests\Unit\DataHandling\Fixtures\InvalidHookFixture;
 use TYPO3\CMS\Core\Utility\GeneralUtility;
@@ -32,7 +33,7 @@ class DataHandlerTest extends \TYPO3\CMS\Core\Tests\UnitTestCase
     protected $singletonInstances = array();
 
     /**
-     * @var DataHandler|\PHPUnit_Framework_MockObject_MockObject|\TYPO3\CMS\Core\Tests\AccessibleObjectInterface
+     * @var DataHandler|\PHPUnit_Framework_MockObject_MockObject|AccessibleObjectInterface
      */
     protected $subject;
 
@@ -667,7 +668,7 @@ class DataHandlerTest extends \TYPO3\CMS\Core\Tests\UnitTestCase
     {
         $table = 'phpunit_dummy';
 
-        /** @var DataHandler|\PHPUnit_Framework_MockObject_MockObject|\TYPO3\CMS\Core\Tests\AccessibleObjectInterface $subject */
+        /** @var DataHandler|\PHPUnit_Framework_MockObject_MockObject|AccessibleObjectInterface $subject */
         $subject = $this->getAccessibleMock(
             DataHandler::class,
             array('dummy')
@@ -718,6 +719,24 @@ class DataHandlerTest extends \TYPO3\CMS\Core\Tests\UnitTestCase
     /**
      * @test
      */
+    public function deletePagesOnRootLevelIsDenied()
+    {
+        /** @var DataHandler|\PHPUnit_Framework_MockObject_MockObject|AccessibleObjectInterface $dataHandlerMock */
+        $dataHandlerMock = $this->getMock(DataHandler::class, ['canDeletePage', 'newlog2']);
+        $dataHandlerMock
+            ->expects($this->never())
+            ->method('canDeletePage');
+        $dataHandlerMock
+            ->expects($this->once())
+            ->method('newlog2')
+            ->with('Deleting all pages starting from the root-page is disabled.', 'pages', 0, 0, 2);
+
+        $dataHandlerMock->deletePages(0);
+    }
+
+    /**
+     * @test
+     */
     public function deleteRecord_procBasedOnFieldTypeRespectsEnableCascadingDelete()
     {
         $table = $this->getUniqueId('foo_');
@@ -735,7 +754,7 @@ class DataHandlerTest extends \TYPO3\CMS\Core\Tests\UnitTestCase
             '1' => array('table' => $this->getUniqueId('bar_'), 'id' => 67)
         );
 
-        /** @var DataHandler|\PHPUnit_Framework_MockObject_MockObject|\TYPO3\CMS\Core\Tests\AccessibleObjectInterface $mockDataHandler */
+        /** @var DataHandler|\PHPUnit_Framework_MockObject_MockObject|AccessibleObjectInterface $mockDataHandler */
         $mockDataHandler = $this->getAccessibleMock(DataHandler::class, array('getInlineFieldType', 'deleteAction', 'createRelationHandlerInstance'), array(), '', false);
         $mockDataHandler->expects($this->once())->method('getInlineFieldType')->will($this->returnValue('field'));
         $mockDataHandler->expects($this->once())->method('createRelationHandlerInstance')->will($this->returnValue($mockRelationHandler));
index af6aff8..80f6d5a 100644 (file)
@@ -204,8 +204,10 @@ class DataHandlerHook
                         $tcemainObj->deleteEl($table, $id);
                         // Delete move-placeholder if current version record is a move-to-pointer
                         if ($recordVersionState->equals(VersionState::MOVE_POINTER)) {
-                            $movePlaceholder = BackendUtility::getMovePlaceholder($table, $liveRec['uid'], 'uid');
-                            $tcemainObj->deleteEl($table, $movePlaceholder['uid']);
+                            $movePlaceholder = BackendUtility::getMovePlaceholder($table, $liveRec['uid'], 'uid', $record['t3ver_wsid']);
+                            if (!empty($movePlaceholder)) {
+                                $tcemainObj->deleteEl($table, $movePlaceholder['uid']);
+                            }
                         }
                     } else {
                         // If live record was placeholder (new/deleted), rather clear
index e1b0e1c..3e558ee 100644 (file)
@@ -1,15 +1,14 @@
 pages
 ,uid,pid,sorting,deleted,t3ver_wsid,t3ver_state,t3ver_stage,t3ver_oid,t3ver_move_id,title
-,1,0,1000000000,1,0,0,0,0,0,FunctionalTest
+,1,0,256,0,0,0,0,0,0,FunctionalTest
 ,88,1,1000000000,1,0,0,0,0,0,DataHandlerTest
 ,89,88,1000000000,1,0,0,0,0,0,Relations
 ,90,88,1000000000,1,0,0,0,0,0,Target
 ,91,-1,1000000000,1,1,4,0,89,0,Relations
+,92,88,1000000000,1,1,3,0,0,89,"[MOVE-TO PLACEHOLDER for #89, WS#1]"
 ,93,88,1000000000,1,1,1,0,0,0,"Testing #1"
 ,94,-1,1000000000,1,1,-1,0,93,0,"Testing #1"
 ,95,-1,1000000000,1,0,0,0,88,0,DataHandlerTest
-,96,88,1000000000,1,1,1,0,0,0,"Relations (copy 1)"
-,97,-1,1000000000,1,1,-1,0,96,0,"Relations (copy 1)"
 tt_content
 ,uid,pid,sorting,deleted,sys_language_uid,l18n_parent,t3ver_wsid,t3ver_state,t3ver_stage,t3ver_oid,t3ver_move_id,header
 ,296,88,1000000000,1,0,0,0,0,0,0,0,"Regular Element #0"
index e1b0e1c..3e558ee 100644 (file)
@@ -1,15 +1,14 @@
 pages
 ,uid,pid,sorting,deleted,t3ver_wsid,t3ver_state,t3ver_stage,t3ver_oid,t3ver_move_id,title
-,1,0,1000000000,1,0,0,0,0,0,FunctionalTest
+,1,0,256,0,0,0,0,0,0,FunctionalTest
 ,88,1,1000000000,1,0,0,0,0,0,DataHandlerTest
 ,89,88,1000000000,1,0,0,0,0,0,Relations
 ,90,88,1000000000,1,0,0,0,0,0,Target
 ,91,-1,1000000000,1,1,4,0,89,0,Relations
+,92,88,1000000000,1,1,3,0,0,89,"[MOVE-TO PLACEHOLDER for #89, WS#1]"
 ,93,88,1000000000,1,1,1,0,0,0,"Testing #1"
 ,94,-1,1000000000,1,1,-1,0,93,0,"Testing #1"
 ,95,-1,1000000000,1,0,0,0,88,0,DataHandlerTest
-,96,88,1000000000,1,1,1,0,0,0,"Relations (copy 1)"
-,97,-1,1000000000,1,1,-1,0,96,0,"Relations (copy 1)"
 tt_content
 ,uid,pid,sorting,deleted,sys_language_uid,l18n_parent,t3ver_wsid,t3ver_state,t3ver_stage,t3ver_oid,t3ver_move_id,header
 ,296,88,1000000000,1,0,0,0,0,0,0,0,"Regular Element #0"