Skip to content

Commit

Permalink
New feature #15693: Allow simple user to update script with XSS enable (
Browse files Browse the repository at this point in the history 
#1372)
  • Loading branch information
@Shnoulle @lacrioque
Shnoulle authored and lacrioque committed Jan 28, 2020
1 parent 73f405e commit ae8a6cb
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 6 deletions.
6 changes: 6 additions & 0 deletions application/config/config-defaults.php
View file
Expand Up @@ -237,6 +237,12 @@
// allow these users to be able to use Javascript etc. .
$config['filterxsshtml'] = true;

// disablescriptwithxss
// Allow update of script in question
// true : Default : follow XSS rules
// false : allowed for all
$config['disablescriptwithxss'] = true;

// usercontrolSameGroupPolicy
// If this option is set to true, then limesurvey operators will only 'see'
// users that belong to at least one of their groups
Expand Down
3 changes: 3 additions & 0 deletions application/core/LSWebUser.php
View file
Expand Up @@ -133,6 +133,9 @@ public function isXssFiltered()
// Permission::model exist only after 172 DB version
return Yii::app()->getConfig('filterxsshtml');
}
if (!Yii::app()->getConfig('disablescriptwithxss')) {
return true;
}
if (Yii::app()->getConfig('filterxsshtml')) {
return !\Permission::model()->hasGlobalPermission('superadmin', 'read');
}
Expand Down
30 changes: 24 additions & 6 deletions application/views/admin/globalsettings/_security.php
View file
Expand Up @@ -11,7 +11,7 @@
<?php $this->widget('yiiwheels.widgets.switch.WhSwitch', array(
'name' => 'surveyPreview_require_Auth',
'id'=>'surveyPreview_require_Auth',
'value' => getGlobalSetting('surveyPreview_require_Auth'),
'value' => Yii::app()->getConfig('surveyPreview_require_Auth'),
'onLabel'=>gT('On'),
'offLabel' => gT('Off')));
?>
Expand All @@ -24,24 +24,42 @@
<?php $this->widget('yiiwheels.widgets.switch.WhSwitch', array(
'name' => 'filterxsshtml',
'id'=>'filterxsshtml',
'value' => getGlobalSetting('filterxsshtml'),
'value' => Yii::app()->getConfig('filterxsshtml'),
'onLabel'=>gT('On'),
'offLabel' => gT('Off')
));
?>
</div>
<div class="help-block">
<span class='text-success'><?php eT("Note: XSS filtering is always disabled for the superadministrator."); ?></span>
</div>
</div>

<div class="form-group">
<label class=" control-label" for='disablescriptwithxss'><?php eT("Disable question script for XSS restricted user:"); ?></label>
<div class="">
<span class='hint'><?php eT("Note: XSS filtering is always disabled for the superadministrator."); ?></span>
<?php $this->widget('yiiwheels.widgets.switch.WhSwitch', array(
'name' => 'filterxsshtml',
'id'=>'filterxsshtml',
'value' => Yii::app()->getConfig('disablescriptwithxss'),
'onLabel'=>gT('On'),
'offLabel' => gT('Off')
));
?>
</div>
<div class="help-block">
<span class='text-warning'><?php eT("If you disable this option : user with XSS restriction still can add script. This allow user to add cross-site scripting javascript system."); ?></span>

This comment has been minimized.

Sign in to view

Copy link
@c-schmitz

c-schmitz Feb 11, 2020

Contributor

@Shnoulle I am sorry but I have no clue what you want to say here. Can you please give me an explanation in French and I will try to correct the string?

This comment has been minimized.

Sign in to view

Copy link
@c-schmitz

c-schmitz Feb 12, 2020

Contributor

@Shnoulle Pretty please?

This comment has been minimized.

Sign in to view

Copy link
@Shnoulle

Shnoulle Feb 12, 2020

Author Collaborator

If you enable XSS, but disable Disable question script for XSS restricted user: (set to OFF) : then if simple user can not add <script>alert('XSS');<script> inside question text : they can add alert('XSS'); in question script.

Something like this in French

Si vous désactivez cette option: les utilisateur avec restriction XSS porront ajouter des scripts java-script.
Cela permet aux utilisateurs d'ajouter un système de type cross-site scripting.

This comment has been minimized.

Sign in to view

Copy link
@c-schmitz

c-schmitz Feb 12, 2020 •
edited

Contributor

The option says 'Disable question script for XSS restricted user:' On/Off

So when I disable this option it disables the disabled? This is way too complicated. I still don't understand it.

Can we change the text of the switch and the explanation to something that is easier to understand?

Like 'Allow question script editing for users if XSS filter is enabled' Yes/No ?
If the switch text is like that what would the explanation look like?

This comment has been minimized.

Sign in to view

Copy link
@Shnoulle

Shnoulle Feb 12, 2020

Author Collaborator

The option are disablescriptwithxss with true : Default : follow XSS rules

Then we need
Allow question script editing for users if XSS filter is enabled :

  • Yes : set disablescriptwithxss to false
  • No : set disablescriptwithxss to true

Else : i make 4 pull request for this simple feature .... the 1st one (and complete) are here : #1358
</div>
</div>


<div class="form-group">
<label class=" control-label" for='usercontrolSameGroupPolicy'><?php eT("Group member can only see own group:"); ?></label>
<div class="">
<?php $this->widget('yiiwheels.widgets.switch.WhSwitch', array(
'name' => 'usercontrolSameGroupPolicy',
'id'=>'usercontrolSameGroupPolicy',
'value' => getGlobalSetting('usercontrolSameGroupPolicy'),
'value' => Yii::app()->getConfig('usercontrolSameGroupPolicy'),
'onLabel'=>gT('On'),
'offLabel' => gT('Off')));
?>
Expand All @@ -57,7 +75,7 @@
<div class="">
<?php $this->widget('yiiwheels.widgets.buttongroup.WhButtonGroup', array(
'name' => 'x_frame_options',
'value'=> getGlobalSetting('x_frame_options'),
'value'=> Yii::app()->getConfig('x_frame_options'),
'selectOptions'=>array(
"allow"=>gT("Allow",'unescaped'),
"sameorigin"=>gT("Same origin",'unescaped')
Expand All @@ -75,7 +93,7 @@
<div class="">
<?php $this->widget('yiiwheels.widgets.buttongroup.WhButtonGroup', array(
'name' => 'force_ssl',
'value'=> getGlobalSetting('force_ssl'),
'value'=> Yii::app()->getConfig('force_ssl'),
'selectOptions'=>array(
"on"=>gT("On",'unescaped'),
"off"=>gT("Off",'unescaped')
Expand Down

8 comments on commit ae8a6cb

@olleharstedt
Copy link
Contributor

Choose a reason for hiding this comment

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

This feature has no acceptance tests and should be reverted.

@olleharstedt
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was it merged?

@Shnoulle
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@olleharstedt sorry, but … #1364

And … more and more complex to add new feature on LS ? Maybe time to fork and stop contribution …

@Shnoulle
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This fix https://bugs.limesurvey.org/view.php?id=13175

@olleharstedt
Copy link
Contributor

Choose a reason for hiding this comment

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

And … more and more complex to add new feature on LS ? Maybe time to fork and stop contribution …

If our new workflow doesn't work, we should reevaluate it. But we have to try it first. If we continue like before, we will have the same instability as before. We should test the new workflow with scenarios, acceptance tests, regular releases. Then we'll crunch the numbers and see if the stability went up. Maybe we have to reach a compromise if contributors hate it or come up with something new. I hope this makes sense to you. We need to discuss internally (and on IRC, perhaps) what "regular releases" means. I gave you some details on Zulip. I definitely want to get away from large releases with tons of regressions, and also from "feature freeze". We can talk more about it later.

@olleharstedt
Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe LS GmbH should do some of the work of a PR, depending on how requested the new feature is.

@Shnoulle
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mean : The 2st pull request done for this feature are #1358

And here : about acceptance test : i don't want to do a Admin Gui edition acceptance test for this single option.
Give me a working admin gui question edition acceptance test … before.

I can do with import, but how to check JS is here or not … must find a way.

And currently : this part of feature are not totally made : need XSS for admin and Question script management.

@olleharstedt
Copy link
Contributor

Choose a reason for hiding this comment

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

Give me a working admin gui question edition acceptance test … before.

This is a reasonable request, I think. Contributors should have access to example tests they can copy and paste from, to make it easy to add new tests.

And currently : this part of feature are not totally made : need XSS for admin and Question script management.

Right.

Please sign in to comment.