Merge pull request #4830 from open-sausages/pulls/3/fix-querystring-stage

API Disable unauthenticated get parameter access to site stage mode

control/VersionedRequestFilter.php

@@ -1,4 +1,5 @@
 <?php
+
 /**
  * Initialises the versioned stage when a request is made.
  *
@@ -8,7 +9,37 @@
 class VersionedRequestFilter implements RequestFilter {
 
 	public function preRequest(SS_HTTPRequest $request, Session $session, DataModel $model) {
-		Versioned::choose_site_stage($session);
+		// Bootstrap session so that Session::get() accesses the right instance
+		$dummyController = new Controller();
+		$dummyController->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. '.
+					'<a href="%s">Click here to go back to the published site.</a>'
+				),
+				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;
 	}
 

model/Versioned.php

@@ -1084,6 +1084,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.
 	 *
@@ -1095,14 +1115,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
+		$preexistingMode = Session::get('readingMode');
-			? $session->inst_get('readingMode')
-			: Session::get('readingMode');
 
 		// Determine the reading mode
 		if(isset($_GET['stage'])) {
@@ -1124,11 +1140,7 @@ class Versioned extends DataExtension implements TemplateGlobalProvider {
 		if(($preexistingMode && $preexistingMode !== $mode)
 			|| (!$preexistingMode && $mode !== self::DEFAULT_MODE)
 		) {
-			if($session) {
+			Session::set('readingMode', $mode);
-				$session->inst_set('readingMode', $mode);
-			} else {
-				Session::set('readingMode', $mode);
-			}
 		}
 
 		if(!headers_sent() && !Director::is_cli()) {

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)) {
+		if(!is_array($messageSet)) {
-				$messageSet = array('default' => $messageSet);
+			$messageSet = array('default' => $messageSet);
-			}
+		}
 
-			$member = Member::currentUser();
+		$member = Member::currentUser();
 
-			// Work out the right message to show
+		// Work out the right message to show
-			if($member && $member->exists()) {
+		if($member && $member->exists()) {
-				$response = ($controller) ? $controller->getResponse() : new SS_HTTPResponse();
+			$response = ($controller) ? $controller->getResponse() : new SS_HTTPResponse();
-				$response->setStatusCode(403);
+			$response->setStatusCode(403);
 
-				//If 'alreadyLoggedIn' is not specified in the array, then use the default
+			//If 'alreadyLoggedIn' is not specified in the array, then use the default
-				//which should have been specified in the lines above
+			//which should have been specified in the lines above
-				if(isset($messageSet['alreadyLoggedIn'])) {
+			if(isset($messageSet['alreadyLoggedIn'])) {
-					$message = $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;
 			} else {
 				$message = $messageSet['default'];
 			}
 
-			Session::set("Security.Message.message", $message);
+			// Somewhat hackish way to render a login form with an error message.
-			Session::set("Security.Message.type", 'warning');
+			$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(
+			return $response;
-				Config::inst()->get('Security', 'login_url')
+		} else {
-				. "?BackURL=" . urlencode($_SERVER['REQUEST_URI'])
+			$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() {

tests/model/VersionedTest.php

@@ -599,6 +599,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);
@@ -630,10 +632,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');
@@ -647,6 +650,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
 	 *