Skip to content

Commit

Permalink
Master ls base controller check params (#1479)
Browse files Browse the repository at this point in the history 
Fixed issue #16469: Any admin user can see any question (without read right on survey)
Fixed issue #16467: Reflected XSS vulnerabilities
Dev: add a function to validate int parameters
Dev: throw 400/403 or 404 error

Dev: remove some uneeded redirect and filter

Dev: Move functionnality to an helper function to call
Dev : getValidateSurveyId return a validated sid related with qid and gid

Dev: rename to getValidatedSurveyId
Dev: getQuestionObject must be reviewed : $sureyId must be a mandatory param

Dev: more details in function doc and fix partially phpDoc

Dev: getValidatedSurveyId get the final sid by param
Dev: review test :
  • Loading branch information
@Shnoulle
Shnoulle committed Aug 6, 2020
1 parent ee73610 commit 17f0a0d
Show file tree
Hide file tree
Showing 3 changed files with 104 additions and 22 deletions.
61 changes: 60 additions & 1 deletion application/controllers/LSBaseController.php
View file
Expand Up @@ -55,6 +55,66 @@ protected function _init()
Yii::setPathOfAlias('lsadminmodules', Yii::app()->getConfig('lsadminmodulesrootdir'));
}

/**
* Validate params used for sid, gid and qid. Validate if qid is included in gid is included in sid.
* Validate if final survey exist
* Currently : used by QuestionEditorController
* @param integer|mixed $sid : sid param to be controlled. If invalid (not an integer) : throw a 404
* @param integer|mixed $gid : gid param to be controlled. If invalid (not an integer) : throw a 404
* @param integer|mixed $qid : qid param to be controlled. If invalid (not an integer) : throw a 404
* @Throw CHttpException
* @return false|integer the existing sid related to current params.
*/
protected function getValidatedSurveyId($sid = null, $gid = null, $qid = null)
{
$oQuestion = null;
if ($qid) {
$oQuestion = Question::model()->findByPk($qid);
if(!$oQuestion) {
throw new CHttpException(404);
}
}
$oGroup = null;
if ($gid) {
$oGroup = QuestionGroup::model()->findByPk($gid);
if(!$oGroup) {
throw new CHttpException(404);
}
if ($oQuestion && $gid != $oQuestion->gid) {
// Try to hack : 400
throw new CHttpException(400);
}
}

$surveyId = false;
if(is_null($sid)) {
/* @todo : rempove this to move to params of function */
$sid = App()->request->getParam('sid', App()->request->getParam('surveyid'));
}
if ($sid) {
$oSurvey = Survey::model()->findByPk($sid);
if (!$oSurvey) {
throw new CHttpException(404);
}
if ($oQuestion && $sid != $oQuestion->sid) {
// Try to hack : 400
throw new CHttpException(400);
}
if ($oGroup && $sid != $oGroup->sid) {
// Try to hack : 400
throw new CHttpException(400);
}
$surveyId = $sid;
} else {
if (!empty($oQuestion)) {
$surveyId = $oQuestion->sid;
} elseif (!empty($oGroup)) {
$surveyId = $oGroup->sid;
}
}
return $surveyId;
}

/**
* This part comes from _renderWrappedTemplate (not the best way to refactoring, but a temporary solution)
*
Expand Down Expand Up @@ -136,7 +196,6 @@ public function run($action)
}
}
}

parent::run($action);
}

Expand Down
63 changes: 43 additions & 20 deletions application/controllers/QuestionEditorController.php
View file
Expand Up @@ -3,8 +3,6 @@

class QuestionEditorController extends LSBaseController
{


public function accessRules()
{
return array(
Expand Down Expand Up @@ -59,21 +57,14 @@ protected function beforeRender($view)
*/
public function actionView($surveyid, $gid = null, $qid = null, $landOnSideMenuTab = 'structure')
{

$aData = array();
$iSurveyID = (int) $surveyid;

if (!Permission::model()->hasSurveyPermission($iSurveyID, 'surveycontent', 'read')) {
Yii::app()->user->setFlash('error', gT("Access denied. You have no permission to view this survey"));
$this->redirect(Yii::app()->request->urlReferrer);
/* Minimal security */
$surveyid = $this->getValidatedSurveyId($surveyid, $gid, $qid);
if (!Permission::model()->hasSurveyPermission($surveyid, 'surveycontent', 'read')) {
throw new CHttpException(403);
}

$iSurveyID = $surveyid;
$oSurvey = Survey::model()->findByPk($iSurveyID);

if ($oSurvey === null) {
throw new CHttpException(500, "Survey not found $iSurveyID");
}

$gid = $gid ?? $oSurvey->groups[0]->gid;
$oQuestion = $this->getQuestionObject($qid, null, $gid);
App()->getClientScript()->registerPackage('questioneditor');
Expand Down Expand Up @@ -243,6 +234,9 @@ public function actionView($surveyid, $gid = null, $qid = null, $landOnSideMenuT
*/
public function actionGetPossibleLanguages($iSurveyId)
{
if (!Permission::model()->hasSurveyPermission($iSurveyId, 'surveycontent', 'read')) {
throw new CHttpException(403);
}
$iSurveyId = (int) $iSurveyId;
$aLanguages = Survey::model()->findByPk($iSurveyId)->allLanguages;
$this->renderJSON($aLanguages);
Expand All @@ -258,10 +252,9 @@ public function actionGetPossibleLanguages($iSurveyId)
*/
public function actionSaveQuestionData($sid)
{
$iSurveyId = (int) $sid;
$iSurveyId = $sid;
if (!Permission::model()->hasSurveyPermission($iSurveyId, 'surveycontent', 'update')) {
Yii::app()->user->setFlash('error', gT("Access denied"));
$this->redirect(Yii::app()->request->urlReferrer);
throw new CHttpException(403);
}

$questionData = App()->request->getPost('questionData', []);
Expand Down Expand Up @@ -422,6 +415,11 @@ public function actionReloadQuestionData(
$gid = null,
$question_template = 'core'
) {
/* Minimal security */
$surveyid = $this->getValidatedSurveyId(null, $gid, $iQuestionId);
if (!Permission::model()->hasSurveyPermission($surveyid, 'surveycontent', 'read')) {
throw new CHttpException(403);
}
$iQuestionId = (int) $iQuestionId;
$oQuestion = $this->getQuestionObject($iQuestionId, $type, $gid);

Expand Down Expand Up @@ -478,8 +476,11 @@ public function actionGetGeneralOptions(
$returnArray = false, //todo see were this ajaxrequest is done and take out the parameter there and here
$question_template = 'core'
) {
$surveyid = $this->getValidatedSurveyId(null, $gid, $iQuestionId);
if (!Permission::model()->hasSurveyPermission($surveyid, 'surveycontent', 'read')) {
throw new CHttpException(403);
}
$aGeneralOptionsArray = $this->getGeneralOptions($iQuestionId, $sQuestionType, $gid, $question_template);

$this->renderJSON($aGeneralOptionsArray);
}

Expand All @@ -502,6 +503,10 @@ public function actionGetAdvancedOptions(
$returnArray = false, //todo see were this ajaxrequest is done and take out the parameter there and here
$question_template = 'core'
) {
$surveyid = $this->getValidatedSurveyId(null, null, $iQuestionId);
if (!Permission::model()->hasSurveyPermission($surveyid, 'surveycontent', 'read')) {
throw new CHttpException(403);
}
//here we get a Question object (also if question is new --> QuestionCreate)
$oQuestion = $this->getQuestionObject($iQuestionId, $sQuestionType);
$aAdvancedOptionsArray = $this->getAdvancedOptions($iQuestionId, $sQuestionType, $question_template);
Expand All @@ -528,6 +533,10 @@ public function actionGetAdvancedOptions(
*/
public function actionGetQuestionData($iQuestionId = null, $gid = null, $type = null)
{
$surveyid = $this->getValidatedSurveyId(null, $gid, $iQuestionId);
if (!Permission::model()->hasSurveyPermission($surveyid, 'surveycontent', 'read')) {
throw new CHttpException(403);
}
$iQuestionId = (int) $iQuestionId;
$oQuestion = $this->getQuestionObject($iQuestionId, $type, $gid);

Expand Down Expand Up @@ -568,6 +577,10 @@ function ($lngString) use (&$aLanguages, $aAllLanguages) {
*/
public function actionGetQuestionPermissions($iQuestionId = null)
{
$surveyid = $this->getValidatedSurveyId(null, null, $iQuestionId);
if (!Permission::model()->hasSurveyPermission($surveyid, 'surveycontent', 'read')) {
throw new CHttpException(403);
}
$iQuestionId = (int) $iQuestionId;
$oQuestion = $this->getQuestionObject($iQuestionId);

Expand Down Expand Up @@ -617,6 +630,10 @@ public function actionGetQuestionTypeInformation($sQuestionType)
*/
public function actionGetQuestionTopbar($qid = null)
{
$surveyid = $this->getValidatedSurveyId(null, null, $qid);
if (!Permission::model()->hasSurveyPermission($surveyid, 'surveycontent', 'read')) {
throw new CHttpException(403);
}
$oQuestion = $this->getQuestionObject($qid);
$sid = $oQuestion->sid;
$gid = $oQuestion->gid;
Expand Down Expand Up @@ -674,8 +691,10 @@ public function actionGetQuestionTopbar($qid = null)
*/
private function getQuestionObject($iQuestionId = null, $sQuestionType = null, $gid = null)
{
//todo: this should be done in the action directly
$iSurveyId = App()->request->getParam('sid') ?? App()->request->getParam('surveyid');
$iSurveyId = $this->getValidatedSurveyId(null, $gid, $iQuestionId);
if (!Permission::model()->hasSurveyPermission($iSurveyId, 'surveycontent', 'read')) {
throw new CHttpException(403);
}
$oQuestion = Question::model()->find('sid = :sid AND qid = :qid', [':sid' => $iSurveyId, ':qid' => $iQuestionId]);

if ($oQuestion == null) {
Expand Down Expand Up @@ -710,6 +729,10 @@ public function getGeneralOptions(
$gid = null,
$question_template = 'core'
) {
$surveyid = $this->getValidatedSurveyId(null, $gid, $iQuestionId);
if (!Permission::model()->hasSurveyPermission($surveyid, 'surveycontent', 'read')) {
throw new CHttpException(403);
}
$oQuestion = $this->getQuestionObject($iQuestionId, $sQuestionType, $gid);
return $oQuestion
->getDataSetObject()
Expand Down
2 changes: 1 addition & 1 deletion tests/functional/backend/AdminViewsTest.php
View file
Expand Up @@ -108,7 +108,7 @@ public function testAdminSurveyViews($name, $view)
$view['route'] = ReplaceFields($view['route'], ['{QID}'=> $question->qid,'{GID}'=> $question->gid,'{SID}'=> self::$testSurvey->primaryKey]);

}
$view['route'] = ReplaceFields($view['route'], ['{SID}'=> self::$testSurvey->primaryKey]);
$view['route'] = ReplaceFields($view['route'], ['{QID}'=> 0,'{GID}'=> 0,'{SID}'=> self::$testSurvey->primaryKey]);
$this->findViewTag($name, $view);
}

Expand Down

0 comments on commit 17f0a0d

Please sign in to comment.