From 38e154af0aae89a36f4d3906612ea4bbbf726177 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Mon, 7 Dec 2015 17:25:27 +1300 Subject: [PATCH] API Disable get parameter access to site stage mode BUG Fix missing and undocumented response from Security::permissionFailure() --- control/VersionedRequestFilter.php | 33 +++++++- model/Versioned.php | 34 ++++++--- security/Security.php | 116 ++++++++++++++--------------- tests/model/VersionedTest.php | 16 +++- 4 files changed, 127 insertions(+), 72 deletions(-) diff --git a/control/VersionedRequestFilter.php b/control/VersionedRequestFilter.php index 7072a18c2..e7d6a3863 100644 --- a/control/VersionedRequestFilter.php +++ b/control/VersionedRequestFilter.php @@ -1,4 +1,5 @@ setSession($session); + $dummyController->setRequest($request); + $dummyController->pushCurrent(); + + // Block non-authenticated users from setting the stage mode + if(!Versioned::can_choose_site_stage($request)) { + $permissionMessage = sprintf( + _t( + "ContentController.DRAFT_SITE_ACCESS_RESTRICTION", + 'You must log in with your CMS password in order to view the draft or archived content. '. + 'Click here to go back to the published site.' + ), + Controller::join_links(Director::baseURL(), $request->getURL(), "?stage=Live") + ); + + // Force output since RequestFilter::preRequest doesn't support response overriding + $response = Security::permissionFailure($dummyController, $permissionMessage); + $session->inst_save(); + $dummyController->popCurrent(); + // Prevent output in testing + if(class_exists('SapphireTest', false) && SapphireTest::is_running_test()) { + return false; + } + $response->output(); + die; + } + + Versioned::choose_site_stage(); + $dummyController->popCurrent(); return true; } diff --git a/model/Versioned.php b/model/Versioned.php index e089d4feb..8b568ca56 100644 --- a/model/Versioned.php +++ b/model/Versioned.php @@ -991,6 +991,26 @@ class Versioned extends DataExtension implements TemplateGlobalProvider { //-----------------------------------------------------------------------------------------------// + /** + * Determine if the current user is able to set the given site stage / archive + * + * @param SS_HTTPRequest $request + * @return bool + */ + public static function can_choose_site_stage($request) { + // Request is allowed if stage isn't being modified + if((!$request->getVar('stage') || $request->getVar('stage') === static::get_live_stage()) + && !$request->getVar('archiveDate') + ) { + return true; + } + + // Check permissions with member ID in session. + $member = Member::currentUser(); + $permissions = Config::inst()->get(get_called_class(), 'non_live_permissions'); + return $member && Permission::checkMember($member, $permissions); + } + /** * Choose the stage the site is currently on. * @@ -1002,14 +1022,10 @@ class Versioned extends DataExtension implements TemplateGlobalProvider { * * If neither of these are set, it checks the session, otherwise the stage * is set to 'Live'. - * - * @param Session $session Optional session within which to store the resulting stage */ - public static function choose_site_stage($session = null) { + public static function choose_site_stage() { // Check any pre-existing session mode - $preexistingMode = $session - ? $session->inst_get('readingMode') - : Session::get('readingMode'); + $preexistingMode = Session::get('readingMode'); // Determine the reading mode if(isset($_GET['stage'])) { @@ -1031,11 +1047,7 @@ class Versioned extends DataExtension implements TemplateGlobalProvider { if(($preexistingMode && $preexistingMode !== $mode) || (!$preexistingMode && $mode !== self::DEFAULT_MODE) ) { - if($session) { - $session->inst_set('readingMode', $mode); - } else { - Session::set('readingMode', $mode); - } + Session::set('readingMode', $mode); } if(!headers_sent() && !Director::is_cli()) { diff --git a/security/Security.php b/security/Security.php index a0e0db854..54e1ffe45 100644 --- a/security/Security.php +++ b/security/Security.php @@ -208,6 +208,7 @@ class Security extends Controller implements TemplateGlobalProvider { * * The alreadyLoggedIn value can contain a '%s' placeholder that will be replaced with a link * to log in. + * @return SS_HTTPResponse */ public static function permissionFailure($controller = null, $messageSet = null) { self::set_ignore_disallowed_actions(true); @@ -226,79 +227,78 @@ class Security extends Controller implements TemplateGlobalProvider { } } return $response; - } else { - // Prepare the messageSet provided - if(!$messageSet) { - if($configMessageSet = static::config()->get('default_message_set')) { - $messageSet = $configMessageSet; - } else { - $messageSet = array( - 'default' => _t( - 'Security.NOTEPAGESECURED', - "That page is secured. Enter your credentials below and we will send " - . "you right along." - ), - 'alreadyLoggedIn' => _t( - 'Security.ALREADYLOGGEDIN', - "You don't have access to this page. If you have another account that " - . "can access that page, you can log in again below.", + } - "%s will be replaced with a link to log in." - ) - ); - } + // Prepare the messageSet provided + if(!$messageSet) { + if($configMessageSet = static::config()->get('default_message_set')) { + $messageSet = $configMessageSet; + } else { + $messageSet = array( + 'default' => _t( + 'Security.NOTEPAGESECURED', + "That page is secured. Enter your credentials below and we will send " + . "you right along." + ), + 'alreadyLoggedIn' => _t( + 'Security.ALREADYLOGGEDIN', + "You don't have access to this page. If you have another account that " + . "can access that page, you can log in again below.", + + "%s will be replaced with a link to log in." + ) + ); } + } - if(!is_array($messageSet)) { - $messageSet = array('default' => $messageSet); - } + if(!is_array($messageSet)) { + $messageSet = array('default' => $messageSet); + } - $member = Member::currentUser(); + $member = Member::currentUser(); - // Work out the right message to show - if($member && $member->exists()) { - $response = ($controller) ? $controller->getResponse() : new SS_HTTPResponse(); - $response->setStatusCode(403); + // Work out the right message to show + if($member && $member->exists()) { + $response = ($controller) ? $controller->getResponse() : new SS_HTTPResponse(); + $response->setStatusCode(403); - //If 'alreadyLoggedIn' is not specified in the array, then use the default - //which should have been specified in the lines above - if(isset($messageSet['alreadyLoggedIn'])) { - $message = $messageSet['alreadyLoggedIn']; - } else { - $message = $messageSet['default']; - } - - // Somewhat hackish way to render a login form with an error message. - $me = new Security(); - $form = $me->LoginForm(); - $form->sessionMessage($message, 'warning'); - Session::set('MemberLoginForm.force_message',1); - $formText = $me->login(); - - $response->setBody($formText); - - $controller->extend('permissionDenied', $member); - - return $response; + //If 'alreadyLoggedIn' is not specified in the array, then use the default + //which should have been specified in the lines above + if(isset($messageSet['alreadyLoggedIn'])) { + $message = $messageSet['alreadyLoggedIn']; } else { $message = $messageSet['default']; } - Session::set("Security.Message.message", $message); - Session::set("Security.Message.type", 'warning'); + // Somewhat hackish way to render a login form with an error message. + $me = new Security(); + $form = $me->LoginForm(); + $form->sessionMessage($message, 'warning'); + Session::set('MemberLoginForm.force_message',1); + $formText = $me->login(); - Session::set("BackURL", $_SERVER['REQUEST_URI']); + $response->setBody($formText); - // TODO AccessLogEntry needs an extension to handle permission denied errors - // Audit logging hook $controller->extend('permissionDenied', $member); - $controller->redirect( - Config::inst()->get('Security', 'login_url') - . "?BackURL=" . urlencode($_SERVER['REQUEST_URI']) - ); + return $response; + } else { + $message = $messageSet['default']; } - return; + + Session::set("Security.Message.message", $message); + Session::set("Security.Message.type", 'warning'); + + Session::set("BackURL", $_SERVER['REQUEST_URI']); + + // TODO AccessLogEntry needs an extension to handle permission denied errors + // Audit logging hook + $controller->extend('permissionDenied', $member); + + return $controller->redirect( + Config::inst()->get('Security', 'login_url') + . "?BackURL=" . urlencode($_SERVER['REQUEST_URI']) + ); } public function init() { diff --git a/tests/model/VersionedTest.php b/tests/model/VersionedTest.php index 989b71be0..28364f5f1 100644 --- a/tests/model/VersionedTest.php +++ b/tests/model/VersionedTest.php @@ -597,6 +597,8 @@ class VersionedTest extends SapphireTest { */ public function testReadingPersistent() { $session = Injector::inst()->create('Session', array()); + $adminID = $this->logInWithPermission('ADMIN'); + $session->inst_set('loggedInAs', $adminID); // Set to stage Director::test('/?stage=Stage', null, $session); @@ -628,10 +630,11 @@ class VersionedTest extends SapphireTest { // Test that session doesn't redundantly store the default stage if it doesn't need to $session2 = Injector::inst()->create('Session', array()); + $session2->inst_set('loggedInAs', $adminID); Director::test('/', null, $session2); - $this->assertEmpty($session2->inst_changedData()); + $this->assertArrayNotHasKey('readingMode', $session2->inst_changedData()); Director::test('/?stage=Live', null, $session2); - $this->assertEmpty($session2->inst_changedData()); + $this->assertArrayNotHasKey('readingMode', $session2->inst_changedData()); // Test choose_site_stage Session::set('readingMode', 'Stage.Stage'); @@ -645,6 +648,15 @@ class VersionedTest extends SapphireTest { $this->assertEquals('Stage.Live', Versioned::get_reading_mode()); } + /** + * Test that stage parameter is blocked by non-administrative users + */ + public function testReadingModeSecurity() { + $this->setExpectedException('SS_HTTPResponse_Exception', 'Invalid request'); + $session = Injector::inst()->create('Session', array()); + $result = Director::test('/?stage=Stage', null, $session); + } + /** * Ensures that the latest version of a record is the expected value *