From 740e49061ffdc13e0c34d9c5e3ce97ee4ac4bb79 Mon Sep 17 00:00:00 2001
From: Sam Minnee <sam@silverstripe.com>
Date: Wed, 13 Oct 2010 01:24:15 +0000
Subject: [PATCH] API CHANGE Disallow methods/actions in
 RequestHandler->checkAccessAction() which are implemented on parent classes
 (e.g. ViewableData and Object), unless access is controlled through
 $allowed_actions. This limits information exposure from getters used in
 template contexts. (from r102003)

git-svn-id: svn://svn.silverstripe.com/silverstripe/open/modules/sapphire/trunk@112053 467b73ca-7a2a-4603-9d3b-597d59a354a9
---
 core/control/RequestHandler.php | 21 +++++++++++++++++----
 tests/RequestHandlingTest.php   |  5 +++++
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/core/control/RequestHandler.php b/core/control/RequestHandler.php
index f62119595..74633a386 100755
--- a/core/control/RequestHandler.php
+++ b/core/control/RequestHandler.php
@@ -233,6 +233,7 @@ class RequestHandler extends ViewableData {
 	 * It will interrogate {@link self::$allowed_actions} to determine this.
 	 */
 	function checkAccessAction($action) {
+		$actionOrigCasing = $action;
 		$action            = strtolower($action);
 		$allowedActions    = $this->allowedActions();
 
@@ -262,13 +263,25 @@ class RequestHandler extends ViewableData {
 		
 		// If we get here an the action is 'index', then it hasn't been specified, which means that
 		// it should be allowed.
-		if($action == 'index') return true;
+		if($action == 'index' || empty($action)) return true;
 		
 		if($allowedActions === null || !$this->uninherited('allowed_actions')) {
 			// If no allowed_actions are provided, then we should only let through actions that aren't handled by magic methods
-			// we test this by calling the unmagic method_exists and comparing it to the magic $this->hasMethod().  This will
-			// still let through actions that are handled by templates.
-			return method_exists($this, $action) || !$this->hasMethod($action);
+			// we test this by calling the unmagic method_exists. 
+			if(method_exists($this, $action)) {
+				// Disallow any methods which aren't defined on RequestHandler or subclasses
+				// (e.g. ViewableData->getSecurityID())
+				$r = new ReflectionClass(get_class($this));
+				if($r->hasMethod($actionOrigCasing)) {
+					$m = $r->getMethod($actionOrigCasing);
+					return ($m && is_subclass_of($m->class, 'RequestHandler'));
+				} else {
+					throw new Exception("method_exists() true but ReflectionClass can't find method - PHP is b0kred");
+				}
+			}	else	if(!$this->hasMethod($action)){
+				// Return true so that a template can handle this action
+				return true;
+			}
 		}
 		
 		return false;
diff --git a/tests/RequestHandlingTest.php b/tests/RequestHandlingTest.php
index fe7a56f0b..52aaec8c7 100755
--- a/tests/RequestHandlingTest.php
+++ b/tests/RequestHandlingTest.php
@@ -122,6 +122,11 @@ class RequestHandlingTest extends SapphireTest {
 		$this->assertEquals('This page does not exist.', $response->getBody());
 	}
 	
+	function testMethodsOnParentClassesOfRequestHandlerDeclined() {
+		$response = Director::test('testGoodBase1/getSecurityID');
+		$this->assertEquals(403, $response->getStatusCode());
+	}
+	
 }
 
 /**