View Issue Details

This bug affects 1 person(s).
 8
IDProjectCategoryView StatusLast Update
16853Bug reportsSurvey participants (Tokens)public2022-10-26 10:04
ReporterDenisChenu Assigned Toc_schmitz  
PriorityhighSeverityminor 
Status resolvedResolutionfixed 
Product Version3.25.0 
Fixed in Version3.25.6 
Summary16853: Inconsistent filter behaviour when create token
Description

There are a complete inconsistent behavior on token filter in firstname, lastname and attribute

Sometime : allow tag, sometimes not

Steps To Reproduce

As super admin

  1. Via CPDB : tag is allowed in CPDB, when assign user to survey : no filter (or model rules)
    1. Create user with : Last name: <em>em</em> Via CPDB <strong>strong</strong> for firstname
    2. Add participant to survey
    3. Check participant list
  2. Via register
    1. Launch survey
    2. Add participant to survey Last name: <em>em</em> Via register <strong>strong</strong> for firstname
    3. Check participant list
  3. Via import
    1. Import tokens_641657.csv
    2. Check participant list

And now : edit any of this participant : all tag was deleted ....

Additional Information

The issue :

  1. Usage of flattenTex in GUI : https://github.com/LimeSurvey/LimeSurvey/blob/0d467f9db35e4279614a001e2142a8f91f97b94e/application/controllers/admin/tokens.php#L544
  2. Usage of LSYii_Validators in model

My proposed solution : use only model->rules.

Without XSS system : always clean up HTML with HtmlPurifier or a complete flat way.

If complete flat : need a javascript validation or an HTML5 validation : no filter without inform.

LSA attached to see

TagsNo tags attached.
Bug heat8
Complete LimeSurvey version number (& build)5.0.0
I will donate to the project if issue is resolvedNo
Browsernot relevant ?
Database type & versionnot relevant?
Server OS (if known)not relevant ?
Webserver software & version (if known)not relevant ?
PHP Versionnot relevant ?

Relationships

parent of 17840 closedgabrieljenik Participant attribute with html LS 5.2.9 

Users monitoring this issue

User List There are no users monitoring this issue.

Activities

DenisChenu

DenisChenu

2020-11-23 12:22

developer  

tokens_641657.csv (102 bytes)   
firstname,lastname,email
Via import <strong>strong</strong>,Last name: <em>em</em>,import@example.org
tokens_641657.csv (102 bytes)   
DenisChenu

DenisChenu

2020-11-23 14:46

developer   ~60738

About removing all tags : https://gfycat.com/fr/slimyvalidjavalina

DenisChenu

DenisChenu

2020-11-24 19:17

developer   ~60751

I think i prefer disallow tag and XSS for all user.

DenisChenu

DenisChenu

2021-01-13 17:47

developer   ~61535

Fix committed to 3.x-LTS branch: http://bugs.limesurvey.org/plugin.php?page=Source/view&amp;id=30847

DenisChenu

DenisChenu

2021-01-14 19:11

developer   ~61550

Fix committed to master branch: http://bugs.limesurvey.org/plugin.php?page=Source/view&amp;id=30849

gabrieljenik

gabrieljenik

2021-01-15 19:05

manager   ~61564

Last edited: 2021-01-15 19:05

Added a PR https://github.com/LimeSurvey/LimeSurvey/pull/1721/files
Seems something was wrong
Not sure I made it right. I just removed some 500 errors

DenisChenu

DenisChenu

2021-01-16 10:47

developer   ~61567

Arg … i cherry-pick … i check again master

lime_release_bot

lime_release_bot

2021-01-18 11:05

administrator   ~61578

Fixed in Release 3.25.8+210118

DenisChenu

DenisChenu

2021-01-21 13:36

developer   ~61624

Fix committed to master branch: http://bugs.limesurvey.org/plugin.php?page=Source/view&amp;id=30907

c_schmitz

c_schmitz

2022-09-21 14:45

administrator   ~71868

Last edited: 2022-09-21 14:49

Why would a superadmin not be allowed to use tags in token atributes? Can we please fix that so it is allowed, at least for the custom attributes.

DenisChenu

DenisChenu

2022-09-21 15:38

developer   ~71869

We have a discussion about this … maybe on zoho ?

Because it was , before the update : disallowed totally.
An alternative solution can be use LSYii_Validators for attributes : then simple admin and token user can use tag.
See : https://github.com/LimeSurvey/LimeSurvey/pull/1665/files#diff-a2eee4799865587f00bee528655e3af7d11394aed268da7d38cd294eb47e2ae8R377

The alternate commit …

DenisChenu

DenisChenu

2022-09-21 15:39

developer   ~71870

PS : see https://bugs.limesurvey.org/view.php?id=16884
We need a scenario here …

c_schmitz

c_schmitz

2022-09-21 16:03

administrator   ~71871

The scenario would only be for superadmins, nobody else.
Can you please implement that?

DenisChenu

DenisChenu

2022-09-21 16:04

developer   ~71872

Why superadmin ?
XSS_filter for admin
striptags for register
No ?

DenisChenu

DenisChenu

2022-09-21 16:05

developer   ~71873

Any admin can use email send to add bad value. We must avoid it for registering only.
Right ?

DenisChenu

DenisChenu

2022-09-21 16:05

developer   ~71874

PS : since this commit i use bbcode when i need tag in attribute s⋅…

DenisChenu

DenisChenu

2022-09-21 17:02

developer   ~71876

master : https://github.com/LimeSurvey/LimeSurvey/pull/2627
LTS : https://github.com/LimeSurvey/LimeSurvey/pull/2626

gabrieljenik

gabrieljenik

2022-10-05 17:19

manager   ~72125

Got lost here.
How https://github.com/LimeSurvey/LimeSurvey/pull/2627 is supposed to be tested?
Can you ellaborate a bit on the test case?

I guess html entities should be encoded on the DB, right?

DenisChenu

DenisChenu

2022-10-06 10:46

developer   ~72141

I guess html entities should be encoded on the DB, right?

No…

1st issue : if you import CSV : no filter was done (you can save <script>alert("XSS")</script>, but when you edit you loose all HTML tag. When user register he can add HTML tag, but when edit via GUI : deleted.
The 1st commit disallow HTML tag in all situation : follow Admin GUI

But Carsten reopen issue because need to allow tag for admin user
The now

  • Registering : disallow ALL tags (participant)
  • Simple admin : via GUI/CPDB, remote or import (or plugins): allow tag but no XSS
  • Super admin : via GUI/CPDB, remote or import (or plugins) : allow tag and XSS
gabrieljenik

gabrieljenik

2022-10-12 14:57

manager   ~72217

Simple admin : via GUI/CPDB, remote or import (or plugins): allow tag but no XSS

What does it mean to allow tag but no XSS ?
Can you provide an example?
I thought we were testing if tags are allowed.
Sometimes we don't allow tags as they can allow XSS.
Right?

DenisChenu

DenisChenu

2022-10-12 15:01

developer   ~72218

What does it mean to allow tag but no XSS ?

tag : HTML tag, i don't understand what is your problem here ?

Tag : <strong>strong</strong>
XSS : &lt;script>alert('XSS')&lt;/script>

I thought we were testing if tags are allowed.

Yes, but must disallow XSS for simple admin.

Sometimes we don't allow tags as they can allow XSS.

We Always ALLOW html tag for question text or help …

gabrieljenik

gabrieljenik

2022-10-12 15:19

manager   ~72219

Sometimes we don't allow tags as they can allow XSS.

We Always ALLOW html tag for question text or help …

https://manual.limesurvey.org/Global_settings/en#Security
I had the thought that regular users can't add HTML if Filter HTML for XSS is ON.
Maybe got confused.

DenisChenu

DenisChenu

2022-10-12 15:27

developer   ~72220

I had the thought that regular users can't add HTML if Filter HTML for XSS is ON.

But yes : HTML tag allowed … for simple user in token attributes.

Tag != XSS

I really don't understand here …

gabrieljenik

gabrieljenik

2022-10-12 16:12

manager   ~72221

You are right sorry. <script> tags are filtered! My bad.

DenisChenu

DenisChenu

2022-10-12 16:14

developer   ~72222

;)

Maybe more clear like this :

  • Registering : disallow ALL HTML (participant)
  • Simple admin : via GUI/CPDB, remote or import (or plugins): allow HTML but activate XSS protection
  • Super admin : via GUI/CPDB, remote or import (or plugins) : allow HTML without XSS protection
DenisChenu

DenisChenu

2022-10-12 16:25

developer   ~72224

Simple admin : via GUI: don't allow tag, don't allow <script> tag

Why disallow HTML for admin ?

It's already merged in 3.X …

And see : https://github.com/LimeSurvey/LimeSurvey/pull/2627#pullrequestreview-1121944581 for master …

DenisChenu

DenisChenu

2022-10-12 16:25

developer   ~72225

And https://github.com/LimeSurvey/LimeSurvey/pull/2627/files#r981147912

gabrieljenik

gabrieljenik

2022-10-12 16:30

manager   ~72226

Why disallow HTML for admin ?

My mistake

gabrieljenik

gabrieljenik

2022-10-21 14:51

manager   ~72376

Simple admin : via GUI/CPDB, remote or import (or plugins): allow HTML but activate XSS protection

Should this also apply for participants added directly to the survey (not through CPDB?) ?
If that's the case, it didn't work

gabrieljenik

gabrieljenik

2022-10-24 22:52

manager   ~72394

Also, when participants are transferred from the CPDB to the survey, the tags are filtered.

DenisChenu

DenisChenu

2022-10-25 12:04

developer   ~72408

Should this also apply for participants added directly to the survey (not through CPDB?) ?

I check with remote_co,ntrol : it work without any issue

Also, when participants are transferred from the CPDB to the survey, the tags are filtered.

No : it's OK …
It's only on attribute_X, not on firstname or lastname.

See screencast …

DenisChenu

DenisChenu

2022-10-25 12:05

developer   ~72409

Peek 25-10-2022 12-04.gif (1,853,596 bytes)
gabrieljenik

gabrieljenik

2022-10-25 14:45

manager   ~72413

This is the test case run:

  • Non superadmin
  • Enter a participant in CPDB which contains tags (Ex: ) in the first name
  • Copy the participant to a survey
  • The tag in the participant was removed.
  • Same participant, different First Name in survey vs CPDB.

It's only on attribute_X, not on firstname or lastname.

OH, ok. I was testing on the first name and last name as that was on the reproduction steps.
Maybe then I found another issue?

DenisChenu

DenisChenu

2022-10-25 17:55

developer   ~72416

OH, ok. I was testing on the first name and last name as that was on the reproduction steps.
Maybe then I found another issue?

No : original issue was about whole : fitsname/lastname/attrute_X was not filtered when register, but only when edit via GUI.
Allowing on register create security issue. Allowing XSS create too security issue.
Since all HTML tag are totally filtered when edition : i use the same behaviour every wherer
The original issue was fixed a long time ago with : https://github.com/LimeSurvey/LimeSurvey/commit/e3c517f609d4ddc09acb90f2808762eb96d9428e removing all tag in all consition.

But @c_schmitz reopen the issue to allow superadmin to add tag on attributes : then it's the new PR  …
see https://bugs.limesurvey.org/view.php?id=16853#c71868

gabrieljenik

gabrieljenik

2022-10-25 18:04

manager   ~72417

OK, I was trying to collaborate with the testing, but not sure I can add a lot more here.

Just to clarify, after my testing the participant on the CPDB and the participant on the survey had different first_name and last name.
CPDB: With allowed tags
Survey: No tags at all.

That was not expected (at least not by me :) ).

DenisChenu

DenisChenu

2022-10-25 18:05

developer   ~72418

CPDB: With allowed tags
Survey: No tags at all.

Clearly another issue …

Must chek too if CPDB allow XSS for simple admin.

But : clearly : another issue to report …

c_schmitz

c_schmitz

2022-10-26 10:04

administrator   ~72427

Both PRs were merged now.

Related Changesets

LimeSurvey: 3.x-LTS c12c9d46

2021-01-13 17:47:27

DenisChenu


Committer: GitHub Details Diff
Fixed issue 16853: Inconsistent filter behaviour when create token (#1666)

Dev: remove flattenText in controller
Dev: add some rules, leave LSYii_Validators
Dev: use filter_var / FILTER_SANITIZE_STRING for attribute
Affected Issues
16853
mod - application/controllers/admin/tokens.php Diff File
mod - application/models/Token.php Diff File
mod - application/models/TokenDynamic.php Diff File

LimeSurvey: master af64ca0c

2021-01-14 19:11:21

DenisChenu

Details Diff
Fixed issue 16853: Inconsistent filter behaviour when create token
Fixed issue [security] #16884: LimeSurvey registration emails can be abused (thanks to winfried)
Dev: remove flattenText in controller
Dev: add some rules, leave LSYii_Validators
Dev: use filter_var / FILTER_SANITIZE_STRING for attribute
# Conflicts:
# application/controllers/admin/tokens.php
Affected Issues
16853
mod - application/controllers/admin/tokens.php Diff File
mod - application/models/Token.php Diff File
mod - application/models/TokenDynamic.php Diff File

LimeSurvey: master e3c517f6

2021-01-14 19:14:17

DenisChenu

Details Diff
Fixed issue 16853: Inconsistent filter behaviour when create token
Fixed issue [security] #16884: LimeSurvey registration emails can be abused (thanks to winfried)
Dev: remove flattenText in controller
Dev: add some rules, leave LSYii_Validators
Dev: use filter_var / FILTER_SANITIZE_STRING for attribute
# Conflicts:
# application/controllers/admin/tokens.php

# Conflicts:
# application/controllers/admin/tokens.php
Affected Issues
16853
mod - application/controllers/admin/tokens.php Diff File
mod - application/models/Token.php Diff File
mod - application/models/TokenDynamic.php Diff File

Issue History

Date Modified Username Field Change
2020-11-23 12:22 DenisChenu New Issue
2020-11-23 12:22 DenisChenu File Added: survey_archive_641657.lsa
2020-11-23 12:22 DenisChenu File Added: tokens_641657.csv
2020-11-23 12:22 DenisChenu File Added: Capture d’écran du 2020-11-23 11-58-33.png
2020-11-23 12:22 DenisChenu File Added: Capture d’écran du 2020-11-23 11-58-46.png
2020-11-23 12:22 DenisChenu File Added: Capture d’écran du 2020-11-23 12-22-17.png
2020-11-23 14:40 ollehar Product Version => 3.25.0
2020-11-23 14:46 DenisChenu Note Added: 60738
2020-11-24 18:11 DenisChenu Assigned To => DenisChenu
2020-11-24 18:11 DenisChenu Status new => assigned
2020-11-24 19:17 DenisChenu Assigned To DenisChenu => cdorin
2020-11-24 19:17 DenisChenu Status assigned => ready for testing
2020-11-24 19:17 DenisChenu Note Added: 60751
2021-01-13 17:47 DenisChenu Changeset attached => LimeSurvey 3.x-LTS c12c9d46
2021-01-13 17:47 DenisChenu Note Added: 61535
2021-01-13 17:47 DenisChenu Assigned To cdorin => DenisChenu
2021-01-13 17:47 DenisChenu Resolution open => fixed
2021-01-14 19:11 DenisChenu Changeset attached => LimeSurvey master af64ca0c
2021-01-14 19:11 DenisChenu Note Added: 61550
2021-01-14 19:15 DenisChenu Status ready for testing => resolved
2021-01-14 19:15 DenisChenu Fixed in Version => 3.25.6
2021-01-15 19:05 gabrieljenik Note Added: 61564
2021-01-15 19:05 gabrieljenik Note Edited: 61564
2021-01-16 10:47 DenisChenu Note Added: 61567
2021-01-18 11:05 lime_release_bot Note Added: 61578
2021-01-18 11:05 lime_release_bot Status resolved => closed
2021-01-21 13:36 DenisChenu Changeset attached => LimeSurvey master e3c517f6
2021-01-21 13:36 DenisChenu Note Added: 61624
2022-01-19 09:20 DenisChenu Relationship added parent of 17840
2022-09-21 14:45 c_schmitz Note Added: 71868
2022-09-21 14:45 c_schmitz Bug heat 6 => 8
2022-09-21 14:46 c_schmitz Status closed => feedback
2022-09-21 14:46 c_schmitz Resolution fixed => reopened
2022-09-21 14:49 c_schmitz Note Edited: 71868
2022-09-21 14:49 c_schmitz Note Edited: 71868
2022-09-21 15:38 DenisChenu Note Added: 71869
2022-09-21 15:39 DenisChenu Note Added: 71870
2022-09-21 16:03 c_schmitz Note Added: 71871
2022-09-21 16:04 DenisChenu Note Added: 71872
2022-09-21 16:05 c_schmitz Priority none => high
2022-09-21 16:05 DenisChenu Note Added: 71873
2022-09-21 16:05 DenisChenu Note Added: 71874
2022-09-21 17:02 DenisChenu Assigned To DenisChenu => c_schmitz
2022-09-21 17:02 DenisChenu Note Added: 71876
2022-09-21 17:02 DenisChenu Status feedback => assigned
2022-09-21 17:02 DenisChenu Status assigned => ready for code review
2022-10-05 17:19 gabrieljenik Note Added: 72125
2022-10-06 10:46 DenisChenu Note Added: 72141
2022-10-12 14:57 gabrieljenik Note Added: 72217
2022-10-12 15:01 DenisChenu Note Added: 72218
2022-10-12 15:19 gabrieljenik Note Added: 72219
2022-10-12 15:27 DenisChenu Note Added: 72220
2022-10-12 16:12 gabrieljenik Note Added: 72221
2022-10-12 16:14 DenisChenu Note Added: 72222
2022-10-12 16:25 DenisChenu Note Added: 72224
2022-10-12 16:25 DenisChenu Note Added: 72225
2022-10-12 16:26 DenisChenu Assigned To c_schmitz => gabrieljenik
2022-10-12 16:26 DenisChenu Status ready for code review => ready for testing
2022-10-12 16:30 gabrieljenik Note Added: 72226
2022-10-21 14:51 gabrieljenik Note Added: 72376
2022-10-24 22:52 gabrieljenik Note Added: 72394
2022-10-24 22:52 gabrieljenik Assigned To gabrieljenik => DenisChenu
2022-10-24 22:52 gabrieljenik Status ready for testing => assigned
2022-10-25 12:04 DenisChenu Note Added: 72408
2022-10-25 12:04 DenisChenu File Added: Capture d’écran du 2022-10-25 10-24-29.png
2022-10-25 12:04 DenisChenu Assigned To DenisChenu => gabrieljenik
2022-10-25 12:04 DenisChenu Status assigned => in testing
2022-10-25 12:05 DenisChenu File Deleted: Capture d’écran du 2022-10-25 10-24-29.png
2022-10-25 12:05 DenisChenu Note Added: 72409
2022-10-25 12:05 DenisChenu File Added: Peek 25-10-2022 12-04.gif
2022-10-25 14:45 gabrieljenik Note Added: 72413
2022-10-25 17:55 DenisChenu Note Added: 72416
2022-10-25 18:04 gabrieljenik Note Added: 72417
2022-10-25 18:04 gabrieljenik Assigned To gabrieljenik => DenisChenu
2022-10-25 18:05 DenisChenu Note Added: 72418
2022-10-25 18:10 DenisChenu Status in testing => ready for merge
2022-10-25 18:10 DenisChenu Complete LimeSurvey version number (& build) 3.25.0 git => 5.0.0
2022-10-26 08:36 DenisChenu Assigned To DenisChenu =>
2022-10-26 10:04 c_schmitz Assigned To => c_schmitz
2022-10-26 10:04 c_schmitz Status ready for merge => resolved
2022-10-26 10:04 c_schmitz Resolution reopened => fixed
2022-10-26 10:04 c_schmitz Note Added: 72427