Skip to content

Commit

Permalink
API CHANGE Fixed various controllers to enforce CSRF protection throu…
Browse files Browse the repository at this point in the history 
…gh Form_SecurityToken on GET actions that are not routed through Form->httpSubmission(): AssetAdmin, CMSBatchActionHandler, CMSMain, CommentTableField, LeftAndMain, MemberTableField, PageComment, PageComment_Controller

git-svn-id: svn://svn.silverstripe.com/silverstripe/open/modules/cms/branches/2.4@113282 467b73ca-7a2a-4603-9d3b-597d59a354a9
  • Loading branch information
@chillu
chillu authored and Sam Minnee committed Feb 2, 2011
1 parent 4bc9a5a commit bc3df65
Show file tree
Hide file tree
Showing 20 changed files with 285 additions and 74 deletions.
17 changes: 13 additions & 4 deletions code/AssetAdmin.php
View file
Expand Up @@ -558,7 +558,10 @@ public function getsubtree() {
/**
* Add a new folder and return its details suitable for ajax.
*/
public function addfolder() {
public function addfolder($request) {
// Protect against CSRF on destructive action
if(!SecurityToken::inst()->checkRequest($request)) return $this->httpError(400);

$parent = ($_REQUEST['ParentID'] && is_numeric($_REQUEST['ParentID'])) ? (int)$_REQUEST['ParentID'] : 0;
$name = (isset($_REQUEST['Name'])) ? basename($_REQUEST['Name']) : _t('AssetAdmin.NEWFOLDER',"NewFolder");

Expand Down Expand Up @@ -626,7 +629,7 @@ function DeleteItemsForm() {
/**
* Delete a folder
*/
public function deletefolder() {
public function deletefolder($data, $ofmr) {
$ids = split(' *, *', $_REQUEST['csvIDs']);

if(!$ids) return false;
Expand Down Expand Up @@ -655,7 +658,10 @@ public function deletefolder() {
return $script;
}

public function removefile(){
public function removefile($request){
// Protect against CSRF on destructive action
if(!SecurityToken::inst()->checkRequest($request)) return $this->httpError(400);

if($fileID = $this->urlParams['ID']) {
$file = DataObject::get_by_id('File', $fileID);
// Delete the temp verions of this file in assets/_resampled
Expand Down Expand Up @@ -700,7 +706,10 @@ public function save($urlParams, $form) {
* Removes all unused thumbnails from the file store
* and returns the status of the process to the user.
*/
public function deleteunusedthumbnails() {
public function deleteunusedthumbnails($request) {
// Protect against CSRF on destructive action
if(!SecurityToken::inst()->checkRequest($request)) return $this->httpError(400);

$count = 0;
$thumbnails = $this->getUnusedThumbnails();

Expand Down
3 changes: 3 additions & 0 deletions code/CMSBatchActionHandler.php
View file
Expand Up @@ -53,6 +53,9 @@ function handleAction($request) {
Director::redirectBack();
return;
}

// Protect against CSRF on destructive action
if(!SecurityToken::inst()->checkRequest($request)) return $this->httpError(400);

$actions = Object::get_static($this->class, 'batch_actions');
$actionClass = $actions[$request->param('BatchAction')];
Expand Down
43 changes: 32 additions & 11 deletions code/CMSMain.php
View file
Expand Up @@ -532,7 +532,10 @@ function RootForm() {
// Data saving handlers


public function addpage() {
public function addpage($data, $form) {
// Protect against CSRF on destructive action
if(!SecurityToken::inst()->checkRequest($this->request)) return $this->httpError(400);

$className = isset($_REQUEST['PageType']) ? $_REQUEST['PageType'] : "Page";
$parent = isset($_REQUEST['ParentID']) ? $_REQUEST['ParentID'] : 0;
$suffix = isset($_REQUEST['Suffix']) ? "-" . $_REQUEST['Suffix'] : null;
Expand All @@ -543,6 +546,7 @@ public function addpage() {
}

if(is_numeric($parent)) $parentObj = DataObject::get_by_id("SiteTree", $parent);
else $parentObj = null;
if(!$parentObj || !$parentObj->ID) $parent = 0;

if($parentObj){
Expand Down Expand Up @@ -803,7 +807,7 @@ function versions() {
/**
* Roll a page back to a previous version
*/
function rollback() {
function rollback($data, $form) {
if(isset($_REQUEST['Version']) && (bool)$_REQUEST['Version']) {
$this->extend('onBeforeRollback', $_REQUEST['ID']);
$record = $this->performRollback($_REQUEST['ID'], $_REQUEST['Version']);
Expand All @@ -814,7 +818,7 @@ function rollback() {
}
}

function unpublish() {
function unpublish($data, $form) {
$SQL_id = Convert::raw2sql($_REQUEST['ID']);

$page = DataObject::get_by_id("SiteTree", $SQL_id);
Expand Down Expand Up @@ -1147,7 +1151,10 @@ function DeleteItemsForm() {
return $form;
}

function buildbrokenlinks() {
function buildbrokenlinks($request) {
// Protect against CSRF on destructive action
if(!SecurityToken::inst()->checkRequest($request)) return $this->httpError(400);

if($this->urlParams['ID']) {
$newPageSet[] = DataObject::get_by_id("Page", $this->urlParams['ID']);
} else {
Expand Down Expand Up @@ -1231,13 +1238,16 @@ function getpagecount() {
echo '<p>' . _t('CMSMain.TOTALPAGES',"Total pages: ") . "$count</p>";
}

function publishall() {
function publishall($request) {
ini_set("memory_limit", -1);
ini_set('max_execution_time', 0);

$response = "";

if(isset($this->requestParams['confirm'])) {
// Protect against CSRF on destructive action
if(!SecurityToken::inst()->checkRequest($request)) return $this->httpError(400);

$start = 0;
$pages = DataObject::get("SiteTree", "", "", "", "$start,30");
$count = 0;
Expand All @@ -1263,14 +1273,16 @@ function publishall() {
$response .= sprintf(_t('CMSMain.PUBPAGES',"Done: Published %d pages"), $count);

} else {
$token = SecurityToken::inst();
$response .= '<h1>' . _t('CMSMain.PUBALLFUN','"Publish All" functionality') . '</h1>
<p>' . _t('CMSMain.PUBALLFUN2', 'Pressing this button will do the equivalent of going to every page and pressing "publish". It\'s
intended to be used after there have been massive edits of the content, such as when the site was
first built.') . '</p>
<form method="post" action="publishall">
<input type="submit" name="confirm" value="'
. _t('CMSMain.PUBALLCONFIRM',"Please publish every page in the site, copying content stage to live",PR_LOW,'Confirmation button') .'" />
</form>';
. _t('CMSMain.PUBALLCONFIRM',"Please publish every page in the site, copying content stage to live",PR_LOW,'Confirmation button') .'" />'
. $token->getFormField()->FieldHolder() .
'</form>';
}

return $response;
Expand All @@ -1279,7 +1291,7 @@ function publishall() {
/**
* Restore a completely deleted page from the SiteTree_versions table.
*/
function restore() {
function restore($data, $form) {
if(($id = $_REQUEST['ID']) && is_numeric($id)) {
$restoredPage = Versioned::get_latest_version("SiteTree", $id);
if($restoredPage) {
Expand All @@ -1299,7 +1311,10 @@ function restore() {
}
}

function duplicate() {
function duplicate($request) {
// Protect against CSRF on destructive action
if(!SecurityToken::inst()->checkRequest($request)) return $this->httpError(400);

if(($id = $this->urlParams['ID']) && is_numeric($id)) {
$page = DataObject::get_by_id("SiteTree", $id);
if($page && (!$page->canEdit() || !$page->canCreate())) {
Expand All @@ -1320,7 +1335,10 @@ function duplicate() {
}
}

function duplicatewithchildren() {
function duplicatewithchildren($request) {
// Protect against CSRF on destructive action
if(!SecurityToken::inst()->checkRequest($request)) return $this->httpError(400);

if(($id = $this->urlParams['ID']) && is_numeric($id)) {
$page = DataObject::get_by_id("SiteTree", $id);
if($page && (!$page->canEdit() || !$page->canCreate())) {
Expand All @@ -1340,7 +1358,10 @@ function duplicatewithchildren() {
/**
* Create a new translation from an existing item, switch to this language and reload the tree.
*/
function createtranslation () {
function createtranslation($request) {
// Protect against CSRF on destructive action
if(!SecurityToken::inst()->checkRequest($request)) return $this->httpError(400);

$langCode = Convert::raw2sql($_REQUEST['newlang']);
$originalLangID = (int)$_REQUEST['ID'];

Expand Down
50 changes: 50 additions & 0 deletions code/CommentTableField.php
View file
Expand Up @@ -85,6 +85,10 @@ function SearchForm() {

return $fieldContainer->FieldHolder();
}

function handleItem($request) {
return new CommentTableField_ItemRequest($this, $request->param('ID'));
}
}

/**
Expand All @@ -93,6 +97,7 @@ function SearchForm() {
* @subpackage comments
*/
class CommentTableField_Item extends ComplexTableField_Item {

function HasSpamButton() {
return $this->parent()->HasSpamButton();
}
Expand All @@ -104,6 +109,51 @@ function HasApproveButton() {
function HasHamButton() {
return $this->parent()->HasHamButton();
}

/**
* @return String
*/
function SpamLink() {
return Controller::join_links($this->Link(), "pagecommentaction", 'reportspam', $this->ID);
}

/**
* @return String
*/
function HamLink() {
return Controller::join_links($this->Link(), "pagecommentaction", 'reportham', $this->ID);
}

/**
* @return String
*/
function ApproveLink() {
return Controller::join_links($this->Link(), "pagecommentaction", 'approve', $this->ID);
}
}

/**
* @package cms
* @subpackage comments
*/
class CommentTableField_ItemRequest extends ComplexTableField_ItemRequest {

static $url_handlers = array(
'pagecommentaction/$Action/$ID' => 'handlePageCommentAction',
);

/**
* @param SS_HTTPRequest $request
* @return SS_HTTPResponse
*/
function handlePageCommentAction($request) {
$action = $request->param('Action');
$whitelist = array('approve', 'reportspam', 'reportham');
if(!in_array($action, $whitelist)) $this->httpError(403);

$c = new PageComment_Controller($request);
$c->init();
return $c->$action($request);
}
}
?>
15 changes: 12 additions & 3 deletions code/LeftAndMain.php
View file
Expand Up @@ -869,7 +869,10 @@ static function ForceReload(){
/**
* Ajax handler for updating the parent of a tree node
*/
public function ajaxupdateparent() {
public function ajaxupdateparent($request) {
// Protect against CSRF on destructive action
if(!SecurityToken::inst()->checkRequest($request)) return $this->httpError(400);

$id = $_REQUEST['ID'];
$parentID = $_REQUEST['ParentID'];
if($parentID == 'root'){
Expand Down Expand Up @@ -918,7 +921,10 @@ public function ajaxupdateparent() {
* $_GET[ID]: An array of node ids in the correct order
* $_GET[MovedNodeID]: The node that actually got moved
*/
public function ajaxupdatesort() {
public function ajaxupdatesort($request) {
// Protect against CSRF on destructive action
if(!SecurityToken::inst()->checkRequest($request)) return $this->httpError(400);

$className = $this->stat('tree_class');
$counter = 0;
$js = '';
Expand Down Expand Up @@ -965,7 +971,10 @@ public function CanOrganiseSitetree() {
/**
* Delete a number of items
*/
public function deleteitems() {
public function deleteitems($request) {
// Protect against CSRF on destructive action
if(!SecurityToken::inst()->checkRequest($request)) return $this->httpError(400);

$ids = split(' *, *', $_REQUEST['csvIDs']);

$script = "st = \$('sitetree'); \n";
Expand Down
19 changes: 16 additions & 3 deletions code/MemberTableField.php
View file
Expand Up @@ -131,7 +131,7 @@ function sourceID() {
}

function AddLink() {
return $this->Link() . '/add';
return Controller::join_links($this->Link(), 'add');
}

function SearchForm() {
Expand Down Expand Up @@ -159,6 +159,10 @@ function SearchForm() {
* Add existing member to group rather than creating a new member
*/
function addtogroup() {
// Protect against CSRF on destructive action
$token = $this->getForm()->getSecurityToken();
if(!$token->checkRequest($this->controller->getRequest())) return $this->httpError(400);

$data = $_REQUEST;
$groupID = (isset($data['ctf']['ID'])) ? $data['ctf']['ID'] : null;

Expand Down Expand Up @@ -229,6 +233,11 @@ function addtogroup() {
* Remove member from group rather than from the database
*/
function delete() {
// Protect against CSRF on destructive action
$token = $this->getForm()->getSecurityToken();
// TODO Not sure how this is called, using $_REQUEST to be on the safe side
if(!$token->check($_REQUEST['SecurityID'])) return $this->httpError(400);

$groupID = Convert::raw2sql($_REQUEST['ctf']['ID']);
$memberID = Convert::raw2sql($_REQUEST['ctf']['childID']);
if(is_numeric($groupID) && is_numeric($memberID)) {
Expand Down Expand Up @@ -477,7 +486,7 @@ class MemberTableField_Item extends ComplexTableField_Item {

function Actions() {
$actions = parent::Actions();

foreach($actions as $action) {
if($action->Name == 'delete') {
if($this->parent->getGroup()) {
Expand Down Expand Up @@ -510,7 +519,11 @@ class MemberTableField_ItemRequest extends ComplexTableField_ItemRequest {
/**
* Deleting an item from a member table field should just remove that member from the group
*/
function delete() {
function delete($request) {
// Protect against CSRF on destructive action
$token = $this->ctf->getForm()->getSecurityToken();
if(!$token->checkRequest($request)) return $this->httpError('400');

if($this->ctf->Can('delete') !== true) {
return false;
}
Expand Down
2 changes: 1 addition & 1 deletion code/SecurityAdmin.php
View file
Expand Up @@ -327,7 +327,7 @@ public function SiteTreeAsUL() {

public function addgroup($request) {
// Protect against CSRF on destructive action
if(!Form::get_security_token()->checkRequest($request)) return $this->httpError(400);
if(!SecurityToken::inst()->checkRequest($request)) return $this->httpError(400);

if(!singleton($this->stat('tree_class'))->canCreate()) return Security::permissionFailure($this);

Expand Down

0 comments on commit bc3df65

Please sign in to comment.