Skip to content

Commit

Permalink
Fixed issue #15696: Multiple email addresses in token are not working
Browse files Browse the repository at this point in the history
  • Loading branch information
lacrioque committed Jan 8, 2020
1 parent 2eb78e6 commit 1eee6cf
Show file tree
Hide file tree
Showing 3 changed files with 184 additions and 16 deletions.
122 changes: 122 additions & 0 deletions application/core/LSYii_XssValidator.php
View file
@@ -0,0 +1,122 @@
<?php if (!defined('BASEPATH')) {
exit('No direct script access allowed');
}
/*
* LimeSurvey
* Copyright (C) 2007-2020 The LimeSurvey Project Team / Carsten Schmitz
* All rights reserved.
* License: GNU/GPL License v2 or later, see LICENSE.php
* LimeSurvey is free software. This version may have been modified pursuant
* to the GNU General Public License, and as distributed it includes or
* is derivative of works licensed under the GNU General Public License or
* other free or open source software licenses.
* See COPYRIGHT.php for copyright notices and details.
*
*/

class LSYii_XssValidator extends CValidator
{

/**
* Trim input
* @var boolean
*/
public $trim = true;

/**
* Remove newlines
* @var boolean
*/
public $rmnewlines = true;

/**
* allow HTML
* @var boolean
*/
public $allowHTML = true;

/**
* Allow all css (unused if no HTML allowed)
* @var boolean
*/
public $allowCSS = true;


/**
* Remove any script or dangerous HTML
*
* @param string $value
* @return string
*/
public function filterHTML($value)
{
$filter = new CHtmlPurifier();
$filter->options = array(
'AutoFormat.RemoveEmpty'=>false,
'Core.NormalizeNewlines'=>false,
'CSS.AllowTricky'=>$this->allowCSS, // Allow display:none; (and other)
'HTML.SafeObject'=>true, // To allow including youtube
'Output.FlashCompat'=>true,
'Attr.EnableID'=>true, // Allow to set id
'Attr.AllowedFrameTargets'=>array('_blank', '_self'),
'URI.AllowedSchemes'=>array(
'http' => true,
'https' => true,
'mailto' => true,
'ftp' => true,
'nntp' => true,
'news' => true,
)
);
// To allow script BUT purify : HTML.Trusted=true (plugin idea for admin or without XSS filtering ?)

/** Start to get complete filtered value with url decode {QCODE} (bug #09300). This allow only question number in url, seems OK with XSS protection **/
$sFiltered = preg_replace('#%7B([a-zA-Z0-9\.]*)%7D#', '{$1}', $filter->purify($value));
Yii::import('application.helpers.expressions.em_core_helper', true); // Already imported in em_manager_helper.php ?
$oExpressionManager = new ExpressionManager;
/** We get 2 array : one filtered, other unfiltered **/
$aValues = $oExpressionManager->asSplitStringOnExpressions($value); // Return array of array : 0=>the string,1=>string length,2=>string type (STRING or EXPRESSION)
$aFilteredValues = $oExpressionManager->asSplitStringOnExpressions($sFiltered); // Same but for the filtered string
$bCountIsOk = count($aValues) == count($aFilteredValues);
/** Construction of new string with unfiltered EM and filtered HTML **/
$sNewValue = "";
foreach ($aValues as $key=>$aValue) {
if ($aValue[2] == "STRING") {
$sNewValue .= $bCountIsOk ? $aFilteredValues[$key][0] : $filter->purify($aValue[0]); // If EM is broken : can throw invalid $key
} else {
$sExpression = trim($aValue[0], '{}');
$sNewValue .= "{";
$aParsedExpressions = $oExpressionManager->Tokenize($sExpression, true);
foreach ($aParsedExpressions as $aParsedExpression) {
if ($aParsedExpression[2] == 'DQ_STRING') {
$sNewValue .= "\"".(string) $filter->purify($aParsedExpression[0])."\""; // This disallow complex HTML construction with XSS
} elseif ($aParsedExpression[2] == 'SQ_STRING') {
$sNewValue .= "'".(string) $filter->purify($aParsedExpression[0])."'";
} else {
$sNewValue .= $aParsedExpression[0];
}
}
$sNewValue .= "}";
}
}
gc_collect_cycles(); // To counter a high memory usage of HTML-Purifier
return $sNewValue;
}

protected function validateAttribute($object, $attribute)
{
if($this->rmnewlines) {
$object->$attribute = preg_replace("/(\r?\n\r?)/","", $object->$attribute);
}

if ($this->allowHTML) {
$object->$attribute = $this->filterHTML($object->$attribute);
} else {
$object->$attribute = htmlentities(strip_tags($object->$attribute));
}

if($this->trim) {
$object->$attribute = trim($object->$attribute);
}
}
}
3 changes: 2 additions & 1 deletion application/models/TokenDynamic.php
View file
Expand Up @@ -100,7 +100,8 @@ public function primaryKey()
public function rules()
{
return array(
array('token', 'unique', 'allowEmpty'=>true), // 'caseSensitive'=>false only for mySql
array('firstname, lastname', 'LSYii_XssValidator', 'allowHTML' => false),

This comment has been minimized.

Sign in to view

Copy link
@Shnoulle

Shnoulle Jan 9, 2020

Collaborator

Not related to email issue …

This comment has been minimized.

Sign in to view

Copy link
@Shnoulle

Shnoulle Jan 9, 2020

Collaborator

And why not allow html : allowing HTML in fistname and lastname can allow some workrounbd : PLEASE allwo HTML or give a really good reason to disallow.
array('token', 'unique', 'allowEmpty'=>true),
array('remindercount', 'numerical', 'integerOnly'=>true, 'allowEmpty'=>true),
array('email', 'filter', 'filter'=>'trim'),
array('email', 'LSYii_EmailIDNAValidator', 'allowEmpty'=>true, 'allowMultiple'=>true, 'except'=>'allowinvalidemail'),
Expand Down
75 changes: 60 additions & 15 deletions application/views/admin/token/tokenform.php
View file
Expand Up @@ -194,32 +194,47 @@
<label class=" control-label" for='email'>
<?php eT("Email:"); ?>
</label>
<div class="">
<?=TbHtml::emailField('email', $email, [

This comment has been minimized.

Sign in to view

Copy link
@Shnoulle

Shnoulle Jan 9, 2020

Collaborator

email field allow multiple : see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/email

This is really the best solution …
<div class="input-group">
<?php
// Official IETF email pattern
$emailPattern = "[a-zA-Z0-9!#\\&$%'*+=?^`{}|~_-](\.?[a-zA-Z0-9\\!#$&%'*+=?^`{}|~_-]){0,}@[a-zA-Z0-9]+\.(?!-)([a-zA-Z0-9]?((-?[a-zA-Z0-9]+)+\.(?!-))){0,}[a-zA-Z0-9]{2,8}";

This comment has been minimized.

Sign in to view

Copy link
@Shnoulle

Shnoulle Jan 9, 2020

Collaborator

We already check multiple emauil in thgis validator
https://github.com/LimeSurvey/LimeSurvey/blob/master/application/core/LSYii_EmailIDNAValidator.php

Are you sure you use same system ?

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/email#multiple still the best solution
echo TbHtml::textField('email', $email, [
'class' => 'form-control',
'size' => '50',
'maxlength' => '320'
'maxlength' => '320',
'pattern' => "^".$emailPattern."(;".$emailPattern.")*$"
]);?>
<span class="input-group-addon" id="selector--emailValidationIcon">
<i class="fa fa-check hidden"></i>
</span>
</div>
</div>
</div>

<!-- Email Status -->
<div class="form-group">
<!-- Email Status -->
<div class="form-group">
<label class=" control-label" for='emailstatus'>
<?php eT("Email status:"); ?>
</label>
<div class="">
<div class="input-group">
<?=TbHtml::textField('emailstatus', $emailstatus, [
'class' => 'form-control',
'size' => '50',
'maxlength' => '320',
'placeholder' => 'OK'
'placeholder' => 'OK',
'readonly' => true,
'data-toggle' => "tooltip",
"title" => gT("This field is to record the delivery state. Normally the system will handle this automatically")

This comment has been minimized.

Sign in to view

Copy link
@Shnoulle

Shnoulle Jan 9, 2020 •
edited

Collaborator

NO ! PLEASE : a lot of user update manually … emailstatus

  1. Bad bounce (holiday alert for example)
  2. User send an email asking to be removed to mailing but sill want access.

etc …
]);?>
<span class="input-group-addon">
<button id="selector--emailStatusIconButton" class="btn btn-xs">
<i class="fa fa-lock"></i>
</button>
</span>
</div>
</div>
</div>

<!-- Invitation sent, Reminder sent -->
<div class="form-group">
<!-- Invitation sent, Reminder sent -->
<div class="form-group">
<!-- Invitation sent -->
<label class=" control-label" for='sent'>
<?php eT("Invitation sent?"); ?>
Expand Down Expand Up @@ -288,11 +303,11 @@
</div>
</div>
</div>
</div>
<input class='form-control hidden YesNoDateHidden' type='text' size='20' id='sent' name='sent' value="<?php if (isset($sent)) {echo $sent; } else {echo " N "; }?>" />
</div>
</div>
<div class="form-group">
<input class='form-control hidden YesNoDateHidden' type='text' size='20' id='sent' name='sent' value="<?php if (isset($sent)) {echo $sent; } else {echo " N "; }?>" />
</div>
</div>
<div class="form-group">
<!-- Reminder sent -->
<label class=" control-label" for='remindersent'>
<?php eT("Reminder sent?"); ?>
Expand Down Expand Up @@ -457,4 +472,34 @@
App()->getClientScript()->registerScript('TokenformViewBSSwitcher', "
LS.renderBootstrapSwitch();
", LSYii_ClientScript::POS_POSTSCRIPT);

App()->getClientScript()->registerScript('TokenformViewEmailValidate', "

This comment has been minimized.

Sign in to view

Copy link
@Shnoulle

Shnoulle Jan 9, 2020

Collaborator

Validation different here than all other email validation …
https://github.com/LimeSurvey/LimeSurvey/blob/master/application/core/LSYii_EmailIDNAValidator.php
var validateEmailField = function(){
if($('#email').val().length > 0) {
$('#selector--emailValidationIcon').find('i.fa').removeClass('hidden');
if(/^(".$emailPattern.")(;".$emailPattern.")*$/.test($('#email').val())) {
$('#email').parent('div').removeClass('has-error');
$('#selector--emailValidationIcon').find('i.fa').removeClass('fa-times').addClass('fa-check');
} else {
$('#email').parent('div').addClass('has-error');
$('#selector--emailValidationIcon').find('i.fa').removeClass('fa-check').addClass('fa-times');
}
} else {
$('#selector--emailValidationIcon').find('i.fa').addClass('hidden');
}
};
validateEmailField();
$('#email').on('keyup', validateEmailField);
$('#email').on('blur', validateEmailField);
$('#selector--emailStatusIconButton').on('click', function(e){
e.preventDefault();
if($(this).find('i.fa').hasClass('fa-lock')) {
$(this).find('i.fa').removeClass('fa-lock').addClass('fa-unlock');
$('#emailstatus').prop('readonly', false);
} else {
$(this).find('i.fa').removeClass('fa-unlock').addClass('fa-lock');
$('#emailstatus').prop('readonly', true);
}
});
", LSYii_ClientScript::POS_POSTSCRIPT);
?>

6 comments on commit 1eee6cf

@Shnoulle
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why adding LSYii_XssValidator.php copying partially LSYii_Validators .

My opinion : remove LSYii_Validators and add more cleaner validator but only in develop !

@Shnoulle
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This must be reverted :

  1. No reason to disable html from username, maybe reason, , but done without discussion and mantis issue (who know, maybe some workaround can use HTML)
  2. No reason to disable edition of emailstatus : admin user NEED to be allowed to edit emailstatus
  3. bad solution for multuple email : unsure it was compatible with existing PHP solution, and HTML5 have really better solution

@olleharstedt
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe revert and discuss further on Mantis?

@Shnoulle
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can revert ?

Or make a pull request with revert + fix ?

@olleharstedt
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Shnoulle OK, please revert and then make a new PR.

@Shnoulle
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1368 revert + fix (revert email input to text input)

Please sign in to comment.