tonyair/silverstripe-comments
1
0
Fork 0
mirror of https://github.com/silverstripe/silverstripe-comments synced 2024-10-22 11:05:49 +02:00
Code Issues Projects Releases Wiki Activity

Merge pull request #99 from tractorcow/pulls/better-xss-protection

Browse Source 
Better XSS Protection via hashed token
This commit is contained in:
Christopher Pitt 2015-03-30 15:30:21 +13:00
commit 974b4554fb
6 changed files with 534 additions and 296 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
.travis.yml
View File

@ -6,13 +6,13 @@ php:
env:
- DB=MYSQL CORE_RELEASE=3.1
- DB=MYSQL CORE_RELEASE=master
- DB=MYSQL CORE_RELEASE=3
- DB=PGSQL CORE_RELEASE=3.1
matrix:
include:
- php: 5.4
env: DB=MYSQL CORE_RELEASE=master
env: DB=MYSQL CORE_RELEASE=3
before_script:
- git clone git://github.com/silverstripe-labs/silverstripe-travis-support.git ~/travis-support
@ -20,4 +20,4 @@ before_script:
- cd ~/builds/ss
script:
- phpunit comments/tests/
- vendor/bin/phpunit comments/tests/

74
code/controllers/CommentingController.php
View File

@ -130,80 +130,70 @@ class CommentingController extends Controller {
* Deletes a given {@link Comment} via the URL.
*/
public function delete() {
if(!$this->checkSecurityToken($this->request)) {
return $this->httpError(400);
}
$comment = $this->getComment();
if(!$comment) return $this->httpError(404);
if(!$comment->canDelete()) return $this->httpError(403);
if(!$comment->getSecurityToken()->checkRequest($this->request)) return $this->httpError(400);
if(($comment = $this->getComment()) && $comment->canDelete()) {
$comment->delete();
return ($this->request->isAjax()) ? true : $this->redirectBack();
}
return $this->httpError(404);
return $this->request->isAjax()
? true
: $this->redirectBack();
}
/**
* Marks a given {@link Comment} as spam. Removes the comment from display
*/
public function spam() {
if(!$this->checkSecurityToken($this->request)) {
return $this->httpError(400);
}
$comment = $this->getComment();
if(!$comment) return $this->httpError(404);
if(!$comment->canEdit()) return $this->httpError(403);
if(!$comment->getSecurityToken()->checkRequest($this->request)) return $this->httpError(400);
if(($comment = $this->getComment()) && $comment->canEdit()) {
$comment->IsSpam = true;
$comment->Moderated = true;
$comment->write();
return ($this->request->isAjax()) ? $comment->renderWith('CommentsInterface_singlecomment') : $this->redirectBack();
}
return $this->httpError(404);
return $this->request->isAjax()
? $comment->renderWith('CommentsInterface_singlecomment')
: $this->redirectBack();
}
/**
* Marks a given {@link Comment} as ham (not spam).
*/
public function ham() {
if(!$this->checkSecurityToken($this->request)) {
return $this->httpError(400);
}
$comment = $this->getComment();
if(!$comment) return $this->httpError(404);
if(!$comment->canEdit()) return $this->httpError(403);
if(!$comment->getSecurityToken()->checkRequest($this->request)) return $this->httpError(400);
if(($comment = $this->getComment()) && $comment->canEdit()) {
$comment->IsSpam = false;
$comment->Moderated = true;
$comment->write();
return ($this->request->isAjax()) ? $comment->renderWith('CommentsInterface_singlecomment') : $this->redirectBack();
}
return $this->httpError(404);
return $this->request->isAjax()
? $comment->renderWith('CommentsInterface_singlecomment')
: $this->redirectBack();
}
/**
* Marks a given {@link Comment} as approved.
*/
public function approve() {
if(!$this->checkSecurityToken($this->request)) {
return $this->httpError(400);
}
$comment = $this->getComment();
if(!$comment) return $this->httpError(404);
if(!$comment->canEdit()) return $this->httpError(403);
if(!$comment->getSecurityToken()->checkRequest($this->request)) return $this->httpError(400);
if(($comment = $this->getComment()) && $comment->canEdit()) {
$comment->IsSpam = false;
$comment->Moderated = true;
$comment->write();
return ($this->request->isAjax()) ? $comment->renderWith('CommentsInterface_singlecomment') : $this->redirectBack();
}
return $this->httpError(404);
return $this->request->isAjax()
? $comment->renderWith('CommentsInterface_singlecomment')
: $this->redirectBack();
}
/**
@ -226,20 +216,6 @@ class CommentingController extends Controller {
return false;
}
/**
* Checks the security token given with the URL to prevent CSRF attacks
* against administrators allowing users to hijack comment moderation.
*
* @param SS_HTTPRequest
*
* @return boolean
*/
public function checkSecurityToken($req) {
$token = SecurityToken::inst();
return $token->checkRequest($req);
}
/**
* Post a comment form
*

239
code/dataobjects/Comment.php
View File

@ -3,6 +3,17 @@
/**
* Represents a single comment object.
*
* @property string $Name
* @property string $Comment
* @property string $Email
* @property string $URL
* @property string $BaseClass
* @property boolean $Moderated
* @property boolean $IsSpam True if the comment is known as spam
* @property integer $ParentID ID of the parent page / dataobject
* @property boolean $AllowHtml If true, treat $Comment as HTML instead of plain text
* @property string $SecretToken Secret admin token required to provide moderation links between sessions
* @method Member Author()
* @package comments
*/
class Comment extends DataObject {
@ -16,7 +27,8 @@ class Comment extends DataObject {
"Moderated" => "Boolean",
"IsSpam" => "Boolean",
"ParentID" => "Int",
'AllowHtml' => "Boolean"
'AllowHtml' => "Boolean",
"SecretToken" => "Varchar(255)",
);
private static $has_one = array(
@ -36,7 +48,12 @@ class Comment extends DataObject {
private static $casting = array(
'AuthorName' => 'Varchar',
'RSSName' => 'Varchar'
'RSSName' => 'Varchar',
'DeleteLink' => 'Varchar',
'SpamLink' => 'Varchar',
'HamLink' => 'Varchar',
'ApproveLink' => 'Varchar',
'Permalink' => 'Varchar',
);
private static $searchable_fields = array(
@ -65,6 +82,13 @@ class Comment extends DataObject {
}
}
/**
* @return Comment_SecurityToken
*/
public function getSecurityToken() {
return Injector::inst()->createWithArgs('Comment_SecurityToken', array($this));
}
/**
* Migrates the old {@link PageComment} objects to {@link Comment}
*/
@ -283,55 +307,70 @@ class Comment extends DataObject {
}
/**
* Generate a secure admin-action link authorised for the specified member
*
* @param string $action An action on CommentingController to link to
* @param Member $member The member authorised to invoke this action
* @return string
*/
public function DeleteLink() {
if($this->canDelete()) {
$token = SecurityToken::inst();
protected function actionLink($action, $member = null) {
if(!$member) $member = Member::currentUser();
if(!$member) return false;
return DBField::create_field("Varchar", Director::absoluteURL($token->addToUrl(sprintf(
"CommentingController/delete/%s", (int) $this->ID
))));
}
$url = Controller::join_links(
Director::baseURL(),
"CommentingController",
$action,
$this->ID
);
// Limit access for this user
$token = $this->getSecurityToken();
return $token->addToUrl($url, $member);
}
/**
* Link to delete this comment
*
* @param Member $member
* @return string
*/
public function SpamLink() {
if($this->canEdit() && !$this->IsSpam) {
$token = SecurityToken::inst();
return DBField::create_field("Varchar", Director::absoluteURL($token->addToUrl(sprintf(
"CommentingController/spam/%s", (int) $this->ID
))));
}
public function DeleteLink($member = null) {
if(!$this->canDelete($member)) return false;
return $this->actionLink('delete', $member);
}
/**
* Link to mark as spam
*
* @param Member $member
* @return string
*/
public function HamLink() {
if($this->canEdit() && $this->IsSpam) {
$token = SecurityToken::inst();
return DBField::create_field("Varchar", Director::absoluteURL($token->addToUrl(sprintf(
"CommentingController/ham/%s", (int) $this->ID
))));
}
public function SpamLink($member = null) {
if(!$this->canEdit($member) || $this->IsSpam) return false;
return $this->actionLink('spam', $member);
}
/**
* Link to mark as not-spam (ham)
*
* @param Member $member
* @return string
*/
public function ApproveLink() {
if($this->canEdit() && !$this->Moderated) {
$token = SecurityToken::inst();
return DBField::create_field("Varchar", Director::absoluteURL($token->addToUrl(sprintf(
"CommentingController/approve/%s", (int) $this->ID
))));
public function HamLink($member = null) {
if(!$this->canEdit($member) || !$this->IsSpam) return false;
return $this->actionLink('ham', $member);
}
/**
* Link to approve this comment
*
* @param Member $member
* @return string
*/
public function ApproveLink($member = null) {
if(!$this->canEdit($member) || $this->Moderated) return false;
return $this->actionLink('approve', $member);
}
/**
@ -367,9 +406,8 @@ class Comment extends DataObject {
*/
public function getCMSFields() {
$fields = parent::getCMSFields();
$parent = $this->getParent()->ID;
$hidden = array('ParentID', 'AuthorID', 'BaseClass', 'AllowHtml');
$hidden = array('ParentID', 'AuthorID', 'BaseClass', 'AllowHtml', 'SecretToken');
foreach($hidden as $private) {
$fields->removeByName($private);
@ -419,3 +457,136 @@ class Comment extends DataObject {
return $gravatar;
}
}
/**
* Provides the ability to generate cryptographically secure tokens for comment moderation
*/
class Comment_SecurityToken {
private $secret = null;
/**
* @param Comment $comment Comment to generate this token for
*/
public function __construct($comment) {
if(!$comment->SecretToken) {
$comment->SecretToken = $this->generate();
$comment->write();
}
$this->secret = $comment->SecretToken;
}
/**
* Generate the token for the given salt and current secret
*
* @param string $salt
* @return string
*/
protected function getToken($salt) {
return function_exists('hash_pbkdf2')
? hash_pbkdf2('sha256', $this->secret, $salt, 1000, 30)
: $this->hash_pbkdf2('sha256', $this->secret, $salt, 100, 30);
}
/**
* Get the member-specific salt.
*
* The reason for making the salt specific to a user is that it cannot be "passed in" via a querystring,
* requiring the same user to be present at both the link generation and the controller action.
*
* @param string $salt Single use salt
* @param type $member Member object
* @return string Generated salt specific to this member
*/
protected function memberSalt($salt, $member) {
// Fallback to salting with ID in case the member has not one set
return $salt . ($member->Salt ?: $member->ID);
}
/**
* @param string $url Comment action URL
* @param Member $member Member to restrict access to this action to
* @return string
*/
public function addToUrl($url, $member) {
$salt = $this->generate(15); // New random salt; Will be passed into url
// Generate salt specific to this member
$memberSalt = $this->memberSalt($salt, $member);
$token = $this->getToken($memberSalt);
return Controller::join_links(
$url,
sprintf(
'?t=%s&s=%s',
urlencode($token),
urlencode($salt)
)
);
}
/**
* @param SS_HTTPRequest $request
* @return boolean
*/
public function checkRequest($request) {
$member = Member::currentUser();
if(!$member) return false;
$salt = $request->getVar('s');
$memberSalt = $this->memberSalt($salt, $member);
$token = $this->getToken($memberSalt);
// Ensure tokens match
return $token === $request->getVar('t');
}
/**
* Generates new random key
*
* @param integer $length
* @return string
*/
protected function generate($length = null) {
$generator = new RandomGenerator();
$result = $generator->randomToken('sha256');
if($length !== null) return substr ($result, 0, $length);
return $result;
}
/*-----------------------------------------------------------
* PBKDF2 Implementation (described in RFC 2898) from php.net
*-----------------------------------------------------------
* @param string a hash algorithm
* @param string p password
* @param string s salt
* @param int c iteration count (use 1000 or higher)
* @param int kl derived key length
* @param int st start position of result
*
* @return string derived key
*/
private function hash_pbkdf2 ($a, $p, $s, $c, $kl, $st=0) {
$kb = $st+$kl; // Key blocks to compute
$dk = ''; // Derived key
// Create key
for ($block=1; $block<=$kb; $block++) {
// Initial hash for this block
$ib = $h = hash_hmac($a, $s . pack('N', $block), $p, true);
// Perform block iterations
for ($i=1; $i<$c; $i++) {
// XOR each iterate
$ib ^= ($h = hash_hmac($a, $h, $p, true));
}
$dk .= $ib; // Append iterated block
}
// Return derived key of correct length
return substr($dk, $st, $kl);
}
}

3
composer.json
View File

@ -14,6 +14,9 @@
"suggest": {
"ezyang/htmlpurifier": "4.*"
},
"require-dev": {
"phpunit/PHPUnit": "~3.7@stable"
},
"extra": {
"branch-alias": {
"dev-master": "1.2.x-dev"

156
tests/CommentsTest.php
View File

@ -90,82 +90,168 @@ class CommentsTest extends FunctionalTest {
}
public function testDeleteComment() {
// Test anonymous user
if($member = Member::currentUser()) $member->logOut();
$comment = $this->objFromFixture('Comment', 'firstComA');
$this->assertNull($comment->DeleteLink(), 'No permission to see delete link');
$commentID = $comment->ID;
$this->assertFalse($comment->DeleteLink(), 'No permission to see delete link');
$delete = $this->get('CommentingController/delete/'.$comment->ID);
$check = DataObject::get_by_id('Comment', $comment->ID);
$this->assertEquals(403, $delete->getStatusCode());
$check = DataObject::get_by_id('Comment', $commentID);
$this->assertTrue($check && $check->exists());
$firstPage = $this->objFromFixture('CommentableItem', 'first');
$this->autoFollowRedirection = false;
$this->logInAs('commentadmin');
// Test non-authenticated user
$this->logInAs('visitor');
$this->assertFalse($comment->DeleteLink(), 'No permission to see delete link');
$firstComment = $this->objFromFixture('Comment', 'firstComA');
$firstCommentID = $firstComment->ID;
Director::test($firstPage->RelativeLink(), null, $this->session());
$delete = $this->get('CommentingController/delete/'.$firstComment->ID);
$check = DataObject::get_by_id('Comment', $firstCommentID);
// Test authenticated user
$this->logInAs('commentadmin');
$comment = $this->objFromFixture('Comment', 'firstComA');
$commentID = $comment->ID;
$adminComment1Link = $comment->DeleteLink();
$this->assertContains('CommentingController/delete/'.$commentID.'?t=', $adminComment1Link);
// Test that this link can't be shared / XSS exploited
$this->logInAs('commentadmin2');
$delete = $this->get($adminComment1Link);
$this->assertEquals(400, $delete->getStatusCode());
$check = DataObject::get_by_id('Comment', $commentID);
$this->assertTrue($check && $check->exists());
// Test that this other admin can delete the comment with their own link
$adminComment2Link = $comment->DeleteLink();
$this->assertNotEquals($adminComment2Link, $adminComment1Link);
$this->autoFollowRedirection = false;
$delete = $this->get($adminComment2Link);
$this->assertEquals(302, $delete->getStatusCode());
$check = DataObject::get_by_id('Comment', $commentID);
$this->assertFalse($check && $check->exists());
}
public function testSpamComment() {
// Test anonymous user
if($member = Member::currentUser()) $member->logOut();
$comment = $this->objFromFixture('Comment', 'firstComA');
$this->assertNull($comment->SpamLink(), 'No permission to see mark as spam link');
$commentID = $comment->ID;
$this->assertFalse($comment->SpamLink(), 'No permission to see mark as spam link');
$spam = $this->get('CommentingController/spam/'.$comment->ID);
$this->assertEquals(403, $spam->getStatusCode());
$check = DataObject::get_by_id('Comment', $commentID);
$this->assertEquals(0, $check->IsSpam, 'No permission to mark as spam');
// Test non-authenticated user
$this->logInAs('visitor');
$this->assertFalse($comment->SpamLink(), 'No permission to see mark as spam link');
// Test authenticated user
$this->logInAs('commentadmin');
$comment = $this->objFromFixture('Comment', 'firstComA');
$commentID = $comment->ID;
$adminComment1Link = $comment->SpamLink();
$this->assertContains('CommentingController/spam/'.$commentID.'?t=', $adminComment1Link);
// Test that this link can't be shared / XSS exploited
$this->logInAs('commentadmin2');
$spam = $this->get($adminComment1Link);
$this->assertEquals(400, $spam->getStatusCode());
$check = DataObject::get_by_id('Comment', $comment->ID);
$this->assertEquals(0, $check->IsSpam, 'No permission to mark as spam');
// Test that this other admin can spam the comment with their own link
$adminComment2Link = $comment->SpamLink();
$this->assertNotEquals($adminComment2Link, $adminComment1Link);
$this->autoFollowRedirection = false;
$this->logInAs('commentadmin');
$this->assertContains('CommentingController/spam/'. $comment->ID, $comment->SpamLink()->getValue());
$spam = $this->get('CommentingController/spam/'.$comment->ID);
$check = DataObject::get_by_id('Comment', $comment->ID);
$spam = $this->get($adminComment2Link);
$this->assertEquals(302, $spam->getStatusCode());
$check = DataObject::get_by_id('Comment', $commentID);
$this->assertEquals(1, $check->IsSpam);
$this->assertNull($check->SpamLink());
// Cannot re-spam spammed comment
$this->assertFalse($check->SpamLink());
}
public function testHamComment() {
// Test anonymous user
if($member = Member::currentUser()) $member->logOut();
$comment = $this->objFromFixture('Comment', 'secondComC');
$this->assertNull($comment->HamLink(), 'No permission to see mark as ham link');
$commentID = $comment->ID;
$this->assertFalse($comment->HamLink(), 'No permission to see mark as ham link');
$ham = $this->get('CommentingController/ham/'.$comment->ID);
$this->assertEquals(403, $ham->getStatusCode());
$check = DataObject::get_by_id('Comment', $commentID);
$this->assertEquals(1, $check->IsSpam, 'No permission to mark as ham');
// Test non-authenticated user
$this->logInAs('visitor');
$this->assertFalse($comment->HamLink(), 'No permission to see mark as ham link');
// Test authenticated user
$this->logInAs('commentadmin');
$comment = $this->objFromFixture('Comment', 'secondComC');
$commentID = $comment->ID;
$adminComment1Link = $comment->HamLink();
$this->assertContains('CommentingController/ham/'.$commentID.'?t=', $adminComment1Link);
// Test that this link can't be shared / XSS exploited
$this->logInAs('commentadmin2');
$ham = $this->get($adminComment1Link);
$this->assertEquals(400, $ham->getStatusCode());
$check = DataObject::get_by_id('Comment', $comment->ID);
$this->assertEquals(1, $check->IsSpam, 'No permission to mark as ham');
// Test that this other admin can ham the comment with their own link
$adminComment2Link = $comment->HamLink();
$this->assertNotEquals($adminComment2Link, $adminComment1Link);
$this->autoFollowRedirection = false;
$this->logInAs('commentadmin');
$this->assertContains('CommentingController/ham/'. $comment->ID, $comment->HamLink()->getValue());
$ham = $this->get('CommentingController/ham/'.$comment->ID);
$check = DataObject::get_by_id('Comment', $comment->ID);
$ham = $this->get($adminComment2Link);
$this->assertEquals(302, $ham->getStatusCode());
$check = DataObject::get_by_id('Comment', $commentID);
$this->assertEquals(0, $check->IsSpam);
$this->assertNull($check->HamLink());
// Cannot re-ham hammed comment
$this->assertFalse($check->HamLink());
}
public function testApproveComment() {
// Test anonymous user
if($member = Member::currentUser()) $member->logOut();
$comment = $this->objFromFixture('Comment', 'secondComB');
$this->assertNull($comment->ApproveLink(), 'No permission to see mark as approved link');
$ham = $this->get('CommentingController/approve/'.$comment->ID);
$commentID = $comment->ID;
$this->assertFalse($comment->ApproveLink(), 'No permission to see approve link');
$approve = $this->get('CommentingController/approve/'.$comment->ID);
$this->assertEquals(403, $approve->getStatusCode());
$check = DataObject::get_by_id('Comment', $commentID);
$this->assertEquals(0, $check->Moderated, 'No permission to approve');
$check = DataObject::get_by_id('Comment', $comment->ID);
$this->assertEquals(0, $check->Moderated, 'No permission to mark as approved');
// Test non-authenticated user
$this->logInAs('visitor');
$this->assertFalse($comment->ApproveLink(), 'No permission to see approve link');
$this->autoFollowRedirection = false;
// Test authenticated user
$this->logInAs('commentadmin');
$comment = $this->objFromFixture('Comment', 'secondComB');
$commentID = $comment->ID;
$adminComment1Link = $comment->ApproveLink();
$this->assertContains('CommentingController/approve/'.$commentID.'?t=', $adminComment1Link);
$this->assertContains('CommentingController/approve/'. $comment->ID, $comment->ApproveLink()->getValue());
$ham = $this->get('CommentingController/approve/'.$comment->ID);
// Test that this link can't be shared / XSS exploited
$this->logInAs('commentadmin2');
$approve = $this->get($adminComment1Link);
$this->assertEquals(400, $approve->getStatusCode());
$check = DataObject::get_by_id('Comment', $comment->ID);
$this->assertEquals(0, $check->Moderated, 'No permission to approve');
// Test that this other admin can approve the comment with their own link
$adminComment2Link = $comment->ApproveLink();
$this->assertNotEquals($adminComment2Link, $adminComment1Link);
$this->autoFollowRedirection = false;
$approve = $this->get($adminComment2Link);
$this->assertEquals(302, $approve->getStatusCode());
$check = DataObject::get_by_id('Comment', $commentID);
$this->assertEquals(1, $check->Moderated);
$this->assertNull($check->ApproveLink());
// Cannot re-approve approved comment
$this->assertFalse($check->ApproveLink());
}
public function testCommenterURLWrite() {

4
tests/CommentsTest.yml
View File

@ -1,13 +1,15 @@
Member:
commentadmin:
FirstName: admin
commentadmin2:
FirstName: admin2
visitor:
FirstName: visitor
Group:
commentadmins:
Title: Admin
Members: =>Member.commentadmin
Members: =>Member.commentadmin, =>Member.commentadmin2
Permission:
admin: