From 747d4f440244ff37ff03a06d2cfa82b2034d2439 Mon Sep 17 00:00:00 2001 From: Simon Gow Date: Thu, 20 Sep 2018 14:53:35 +1200 Subject: [PATCH 1/2] Prevent storing formdata to cookies. - XSS an be stored in a cookie and potentially abused in other ways, so to prevent this we need to use session instead. This requires the user to have a session with silverstripe, but this is better than saving potentially malicious content in cookies. (Also some cookies have limited length). @see https://github.com/silverstripe/silverstripe-comments/issues/263 --- src/Forms/CommentForm.php | 50 ++++++++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 17 deletions(-) diff --git a/src/Forms/CommentForm.php b/src/Forms/CommentForm.php index edee7de..364a7fc 100644 --- a/src/Forms/CommentForm.php +++ b/src/Forms/CommentForm.php @@ -14,7 +14,6 @@ use SilverStripe\Forms\RequiredFields; use SilverStripe\Forms\TextareaField; use SilverStripe\Forms\TextField; use SilverStripe\Security\Member; -use SilverStripe\Control\Cookie; use SilverStripe\Core\Convert; use SilverStripe\Security\Security; use SilverStripe\Comments\Model\Comment; @@ -132,21 +131,28 @@ class CommentForm extends Form // Set it so the user gets redirected back down to the form upon form fail $this->setRedirectToFormOnValidationError(true); - // load any data from the cookies - if ($data = Cookie::get('CommentsForm_UserData')) { - $data = Convert::json2array($data); + // load any data from the session + $data = $this->getSessionData(); + if (is_array($data)) { - $this->loadDataFrom(array( - 'Name' => isset($data['Name']) ? $data['Name'] : '', - 'URL' => isset($data['URL']) ? $data['URL'] : '', - 'Email' => isset($data['Email']) ? $data['Email'] : '' - )); + // load user data from previous form request back into form. + if (array_key_exists('UserData', $data)) { + $formData = Convert::json2array($data['UserData']); - // allow previous value to fill if comment not stored in cookie (i.e. validation error) - $prevComment = Cookie::get('CommentsForm_Comment'); + $this->loadDataFrom(array( + 'Name' => isset($formData['Name']) ? $formData['Name'] : '', + 'URL' => isset($formData['URL']) ? $formData['URL'] : '', + 'Email' => isset($formData['Email']) ? $formData['Email'] : '' + )); + } - if ($prevComment && $prevComment != '') { - $this->loadDataFrom(array('Comment' => $prevComment)); + // allow previous value to fill if comment + if (array_key_exists('Comment', $data)) { + $prevComment = $data['Comment']; + + if ($prevComment && $prevComment != '') { + $this->loadDataFrom(array('Comment' => $prevComment)); + } } } } @@ -184,8 +190,10 @@ class CommentForm extends Form } // cache users data - Cookie::set('CommentsForm_UserData', Convert::raw2json($data)); - Cookie::set('CommentsForm_Comment', $data['Comment']); + $form->setSessionData(array( + 'UserData' => Convert::raw2json($data), + 'Comment' => $data['Comment'] + )); // extend hook to allow extensions. Also see onAfterPostComment $this->controller->extend('onBeforePostComment', $form); @@ -246,8 +254,16 @@ class CommentForm extends Form $this->getRequest()->getSession()->set('CommentsModerated', 1); } - // clear the users comment since it passed validation - Cookie::set('CommentsForm_Comment', false); + // clear the users comment since the comment was successful. + if($comment->exists()){ + // Remove the comment data as it's been saved already. + unset($data['Comment']); + } + + // cache users data (name, email, etc to prepopulate on other forms). + $form->setSessionData(array( + 'UserData' => Convert::raw2json($data), + )); // Find parent link if (!empty($data['ReturnURL'])) { From 94950ee79cb996852521ac325f2fe75dfc7de0c5 Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Mon, 24 Sep 2018 17:57:31 +0200 Subject: [PATCH 2/2] Tidy up phpcs violations, use short array syntax, optimise class imports --- src/Forms/CommentForm.php | 56 +++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/src/Forms/CommentForm.php b/src/Forms/CommentForm.php index 364a7fc..9cae509 100644 --- a/src/Forms/CommentForm.php +++ b/src/Forms/CommentForm.php @@ -2,7 +2,11 @@ namespace SilverStripe\Comments\Forms; +use SilverStripe\Comments\Controllers\CommentingController; +use SilverStripe\Comments\Model\Comment; +use SilverStripe\Control\Controller; use SilverStripe\Control\HTTPResponse; +use SilverStripe\Core\Convert; use SilverStripe\Forms\CompositeField; use SilverStripe\Forms\EmailField; use SilverStripe\Forms\FieldList; @@ -14,12 +18,7 @@ use SilverStripe\Forms\RequiredFields; use SilverStripe\Forms\TextareaField; use SilverStripe\Forms\TextField; use SilverStripe\Security\Member; -use SilverStripe\Core\Convert; use SilverStripe\Security\Security; -use SilverStripe\Comments\Model\Comment; -use SilverStripe\Control\Controller; -use SilverStripe\Comments\Controllers\CommentingController; -use SilverStripe\Core\Config\Config; class CommentForm extends Form { @@ -121,11 +120,11 @@ class CommentForm extends Form // we do not want to read a new URL when the form has already been submitted // which in here, it hasn't been. - $this->loadDataFrom(array( + $this->loadDataFrom([ 'ParentID' => $record->ID, 'ReturnURL' => $controller->getRequest()->getURL(), 'ParentClassName' => $controller->getParentClass() - )); + ]); } // Set it so the user gets redirected back down to the form upon form fail @@ -133,26 +132,27 @@ class CommentForm extends Form // load any data from the session $data = $this->getSessionData(); - if (is_array($data)) { + if (!is_array($data)) { + return; + } - // load user data from previous form request back into form. - if (array_key_exists('UserData', $data)) { - $formData = Convert::json2array($data['UserData']); + // load user data from previous form request back into form. + if (array_key_exists('UserData', $data)) { + $formData = Convert::json2array($data['UserData']); - $this->loadDataFrom(array( - 'Name' => isset($formData['Name']) ? $formData['Name'] : '', - 'URL' => isset($formData['URL']) ? $formData['URL'] : '', - 'Email' => isset($formData['Email']) ? $formData['Email'] : '' - )); - } + $this->loadDataFrom([ + 'Name' => isset($formData['Name']) ? $formData['Name'] : '', + 'URL' => isset($formData['URL']) ? $formData['URL'] : '', + 'Email' => isset($formData['Email']) ? $formData['Email'] : '' + ]); + } - // allow previous value to fill if comment - if (array_key_exists('Comment', $data)) { - $prevComment = $data['Comment']; + // allow previous value to fill if comment + if (array_key_exists('Comment', $data)) { + $prevComment = $data['Comment']; - if ($prevComment && $prevComment != '') { - $this->loadDataFrom(array('Comment' => $prevComment)); - } + if ($prevComment && $prevComment != '') { + $this->loadDataFrom(['Comment' => $prevComment]); } } } @@ -190,10 +190,10 @@ class CommentForm extends Form } // cache users data - $form->setSessionData(array( + $form->setSessionData([ 'UserData' => Convert::raw2json($data), 'Comment' => $data['Comment'] - )); + ]); // extend hook to allow extensions. Also see onAfterPostComment $this->controller->extend('onBeforePostComment', $form); @@ -255,15 +255,15 @@ class CommentForm extends Form } // clear the users comment since the comment was successful. - if($comment->exists()){ + if ($comment->exists()) { // Remove the comment data as it's been saved already. unset($data['Comment']); } // cache users data (name, email, etc to prepopulate on other forms). - $form->setSessionData(array( + $form->setSessionData([ 'UserData' => Convert::raw2json($data), - )); + ]); // Find parent link if (!empty($data['ReturnURL'])) {