Skip to content

Commit

Permalink
Fixed issue #12432: Reflected XSS in Notifications (thanks to mrbreaker)
Browse files Browse the repository at this point in the history 
Dev: using Throw error for invalid id
  • Loading branch information
@Shnoulle
Shnoulle committed May 31, 2017
1 parent 8a8ea2a commit 9e6f722
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 43 deletions.
43 changes: 26 additions & 17 deletions application/controllers/admin/NotificationController.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,17 @@ public function getNotificationAsJSON($notId)
{
$this->checkPermission();

if((string)(int)$notId!==(string)$notId) {
throw new CHttpException(403,gT("Invalid notification id"));
}
$not = Notification::model()->findByPk($notId);

if ($not)
{
if ($not) {
header('Content-type: application/json');
echo json_encode(array('result' => $not->getAttributes()));
}
else
{
echo json_encode(array('error' => 'Found no notification with id ' . $notId));
} else {
throw new CHttpException(404,printf(gT("Notification %s not found"),$notId));

This comment has been minimized.

Sign in to view

Copy link
@olleharstedt

olleharstedt May 31, 2017

Collaborator

Why not cast $notId to integer instead?

This comment has been minimized.

Sign in to view

Copy link
@Shnoulle

Shnoulle May 31, 2017

Author Collaborator

Because : if user send somethingNotInteger we must show an error and break. URL must send integer, or it's an hacked url ....

This comment has been minimized.

Sign in to view

Copy link
@olleharstedt

olleharstedt May 31, 2017

Collaborator

This is an Ajax call. The error will not show unless it's JSON.

This comment has been minimized.

Sign in to view

Copy link
@Shnoulle

Shnoulle May 31, 2017

Author Collaborator

Did you test ? Me : yes . if it's an ajax call : Yii send json (maybe related to dataType: 'json',). see 9e6f722#diff-981b94432ee932d7e6aee07a1fb7cadbR89 (oups I must remove the isread part)

This comment has been minimized.

Sign in to view

Copy link
@olleharstedt

olleharstedt May 31, 2017

Collaborator

OK, you extended the JS... But it still breaks the protocol. At worst it should be

echo json_encode(array('error' => $exceptionMessage));

This comment has been minimized.

Sign in to view

Copy link
@olleharstedt

olleharstedt May 31, 2017

Collaborator

Or: If you added success: function() { ... }, why not add error: function() {...} and send an error code? Maybe 404 is catch by this?

This comment has been minimized.

Sign in to view

Copy link
@Shnoulle

Shnoulle May 31, 2017

Author Collaborator

8-| testing this night i when send 404 or 403 i receive a json [error:"Invalid notice ID"], not today ....

You're right about have an error (in fact it's what i think to do, but when testing receive the [error:"Invalid notice ID"] but surely didn't do correctly in good order .... ).

For testing : i directly hack getNotificationAsJSON

This comment has been minimized.

Sign in to view

Copy link
@Shnoulle

Shnoulle May 31, 2017

Author Collaborator

error function must be done :/

This comment has been minimized.

Sign in to view

Copy link
@olleharstedt

olleharstedt Jun 6, 2017

Collaborator

Or just return json error => 'Failed' :)

This comment has been minimized.

Sign in to view

Copy link
@Shnoulle

Shnoulle Jun 6, 2017

Author Collaborator

I really don't like use a 200 if there are an error ...

And currently : if there are a 500 error for any other reason : it's broke too. Then : need a error systen. But do what you want

This comment has been minimized.

Sign in to view

Copy link
@olleharstedt

olleharstedt Jun 6, 2017

Collaborator

Good point.
//echo json_encode(array('error' => 'Found no notification with id ' . $notId));
}
}

Expand All @@ -50,15 +52,19 @@ public function getNotificationAsJSON($notId)
public function notificationRead($notId)
{
$this->checkPermission();

if((string)(int)$notId!==(string)$notId) {
throw new CHttpException(403,gT("Invalid notification id"));
}
try
{
$not = Notification::model()->findByPk($notId);
$result = $not->markAsRead();
header('Content-type: application/json');
echo json_encode(array('result' => $result));
}
catch (Exception $ex)
{
header('Content-type: application/json');
echo json_encode(array('error' => $ex->getMessage()));
}

Expand All @@ -85,13 +91,14 @@ public function actionGetMenuWidget($surveyId = null, $showLoader = false)
public function clearAllNotifications($surveyId = null)
{
Notification::model()->deleteAll(
'entity = \'user\' AND entity_id = ' . Yii::app()->user->id
'entity = :entity AND entity_id = :entity_id' ,
array(":entity"=>'user',":entity_id"=>Yii::app()->user->id)
);

if (is_int($surveyId))
{
if (is_int($surveyId)) {
Notification::model()->deleteAll(
'entity = \'survey\' AND entity_id = ' . $surveyId
'entity = :entity AND entity_id = :entity_id',
array(":entity"=>'survey',":entity_id"=>$surveyId)
);
}
}
Expand All @@ -103,9 +110,8 @@ public function clearAllNotifications($surveyId = null)
protected function checkPermission()
{
// Abort if user is not logged in
if(Yii::app()->user->isGuest)
{
die('No permission');
if(Yii::app()->user->isGuest) {
throw new CHttpException(401);
}
}

Expand All @@ -128,10 +134,13 @@ public static function getMenuWidget($surveyId = null, $showLoader = false)
$data = array();
$data['surveyId'] = $surveyId;
$data['showLoader'] = $showLoader;
$data['clearAllNotificationsUrl'] = Yii::app()->createUrl('admin/notification', array(
$params=array(
'sa' => 'clearAllNotifications',
'surveyId' => $surveyId
));
);
if($surveyId) {
$params['surveyId'] = $surveyId;
}
$data['clearAllNotificationsUrl'] = Yii::app()->createUrl('admin/notification', $params);
$data['updateUrl'] = Notification::getUpdateUrl($surveyId);
$data['nrOfNewNotifications'] = Notification::countNewNotifications($surveyId);
$data['nrOfNotifications'] = Notification::countNotifications($surveyId);
Expand Down
6 changes: 3 additions & 3 deletions application/core/Survey_Common_Action.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ private function _addPseudoParams($params)
}
$oQuestion=Question::model()->find("qid=:qid",array(":qid"=>$params['iQuestionId']));//Move this in model to use cache
if(!$oQuestion) {
throw new CHttpException(404,gT("Invalid question"));
throw new CHttpException(404,gT("Question not found"));
}
if(!isset($params['iGroupId'])) {
$params['iGroupId']=$params['gid']=$oQuestion->gid;
Expand All @@ -164,7 +164,7 @@ private function _addPseudoParams($params)
}
$oGroup=QuestionGroup::model()->find("gid=:gid",array(":gid"=>$params['iGroupId']));//Move this in model to use cache
if(!$oGroup) {
throw new CHttpException(404,gT("Invalid group"));
throw new CHttpException(404,gT("Group not found"));
}
if(!isset($params['iSurveyId'])) {
$params['iSurveyId']=$params['iSurveyID']=$params['surveyid']=$params['sid']=$oGroup->sid;
Expand All @@ -178,7 +178,7 @@ private function _addPseudoParams($params)
}
$oSurvey=Survey::model()->findByPk($params['iSurveyId']);
if(!$oSurvey) {
throw new CHttpException(404,gT("Invalid survey"));
throw new CHttpException(404,gT("Survey not found"));
}
// Minimal permission needed, extra permission must be tested in each controller
if (!Permission::model()->hasSurveyPermission($params['iSurveyId'], 'survey', 'read')) {
Expand Down
18 changes: 12 additions & 6 deletions application/models/Notification.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -225,8 +225,9 @@ public function search()
public function getAjaxUrl()
{
return Yii::app()->createUrl(
'admin/notification/sa/getNotificationAsJSON',
'admin/notification',
array(
'sa' => 'getNotificationAsJSON',
'notId' => $this->id
)
);
Expand All @@ -239,8 +240,9 @@ public function getAjaxUrl()
public function getReadUrl()
{
return Yii::app()->createUrl(
'admin/notification/sa/notificationRead',
'admin/notification',
array(
'sa' => 'notificationRead',
'notId' => $this->id
)
);
Expand All @@ -264,11 +266,15 @@ public function markAsRead()
* @return string
*/
public static function getUpdateUrl($surveyId = null) {
$params=array(
'sa' => 'actionGetMenuWidget',
);
if($surveyId) {
$params['surveyId'] = $surveyId;
}
return Yii::app()->createUrl(
'admin/notification/sa/actionGetMenuWidget',
array(
'surveyId' => $surveyId
)
'admin/notification',
$params
);
}

Expand Down
46 changes: 29 additions & 17 deletions scripts/admin/notifications.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,23 +84,35 @@ $(document).ready(function() {
$.ajax({
url: url,
method: 'GET',
}).done(function(response) {

var response = JSON.parse(response);
var not = response.result;

$('#admin-notification-modal .modal-title').html(not.title);
$('#admin-notification-modal .modal-body-text').html(not.message);
$('#admin-notification-modal .modal-content').addClass('panel-' + not.display_class);
$('#admin-notification-modal .notification-date').html(not.created.substr(0, 16));
$('#admin-notification-modal').modal();

// TODO: Will this work in message includes a link that is clicked?
$('#admin-notification-modal').unbind('hidden.bs.modal');
$('#admin-notification-modal').on('hidden.bs.modal', function(e) {
notificationIsRead(that);
$('#admin-notification-modal .modal-content').removeClass('panel-' + not.display_class);
});
dataType: 'json',
success : function(response) {
if(response.error) {
$('#admin-notification-modal .modal-title').html("Error");
$('#admin-notification-modal .modal-body-text').html(response.error);
$('#admin-notification-modal .modal-content').addClass('panel-danger');
//$('#admin-notification-modal .notification-date').html(not.created.substr(0, 16));
$('#admin-notification-modal').modal();
$('#admin-notification-modal').unbind('hidden.bs.modal');
$('#admin-notification-modal').on('hidden.bs.modal', function(e) {
notificationIsRead(that);
$('#admin-notification-modal .modal-content').removeClass('panel-danger');
});
return;
}
var not = response.result;
$('#admin-notification-modal .modal-title').html(not.title);
$('#admin-notification-modal .modal-body-text').html(not.message);
$('#admin-notification-modal .modal-content').removeClass('panel-danger').addClass('panel-' + not.display_class);
$('#admin-notification-modal .notification-date').html(not.created.substr(0, 16));
$('#admin-notification-modal').modal();

// TODO: Will this work in message includes a link that is clicked?
$('#admin-notification-modal').unbind('hidden.bs.modal');
$('#admin-notification-modal').on('hidden.bs.modal', function(e) {
notificationIsRead(that);
$('#admin-notification-modal .modal-content').removeClass('panel-' + not.display_class);
});
}
});
}

Expand Down

0 comments on commit 9e6f722

Please sign in to comment.