search

collectiveaccess / pawtucket2

Pawtucket Improved
GNU General Public License v3.0
85 stars 76 forks source link

Strip tags to sanitize input in User profile #42

Closed gautiermichelin closed 2 years ago

gautiermichelin commented 2 years ago

Strip tags to sanitize input in User profile and in Tags & Comments. I had a mail from etat.ge.ch audit firm that reports these as XSS injection risk in their automated exam.

collectiveaccess commented 2 years ago

Striptags() is not enough. HTMLPurifier must be used. I'll add that shortly.

gautiermichelin commented 2 years ago

Hi,

JavaScript is already removed (tested) but simple tags , ... are kept without attributes.

Gautier

collectiveaccess commented 2 years ago

What is the problem then?

gautiermichelin commented 2 years ago

Sorry, easier to understand :

“Those tags are still displayed everywhere (dropdown user menu, bottom bar in Providence for user name, within the tags or comments…) ; and not tested, but tags may be cutted with a comma within some tags :-(“

Le ven. 17 juin 2022 à 14:19, Gautier Michelin @.***> a écrit :

Hi Seth or CA team,

Those tags are still displayed everywhere (dropdown user menu, bottom bar in Providence for user name, within the tags are commands…), and not tested, but tags may be cutted with a comma within some tags :-(

For me, it’s more a question than etat.ge.ch their tool will continue to report false XSS injection risks (you’ll risk having such demand from other users too) why not simple strip_tags ?

Best,

Gautier

Le ven. 17 juin 2022 à 14:13, CollectiveAccess @.***> a écrit :

What is the problem then?

— Reply to this email directly, view it on GitHub https://github.com/collectiveaccess/pawtucket2/pull/42#issuecomment-1158811133, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACM2SM6JYTMXLBHZGT36ELVPRTXRANCNFSM5ZBY3R2Q . You are receiving this because you authored the thread.Message ID: @.***>

-- Gautier MICHELIN

-- Gautier MICHELIN

collectiveaccess commented 2 years ago

Is this an XSS risk? Or just a formatting issue?

collectiveaccess commented 2 years ago

I'm going to merge this, as regardless there's no reason for HTML tags to be in there.

gautiermichelin commented 2 years ago

Hi @collectiveaccess

I'm just answering your question.

For me, formatting isn't needed in these. As you can add tags here, it's reported as a risk with certains (at least one) automated XSS report tool. Let's remove this false positive.

Thanks for having merged this PR, best to all team member

gautiermichelin commented 2 years ago

Hi Seth or CA team,

Those tags are still displayed everywhere (dropdown user menu, bottom bar in Providence for user name, within the tags are commands…), and not tested, but tags may be cutted with a comma within some tags :-(

For me, it’s more a question than etat.ge.ch their tool will continue to report false XSS injection risks (you’ll risk having such demand from other users too) why not simple strip_tags ?

Best,

Gautier

Le ven. 17 juin 2022 à 14:13, CollectiveAccess @.***> a écrit :

What is the problem then?

— Reply to this email directly, view it on GitHub https://github.com/collectiveaccess/pawtucket2/pull/42#issuecomment-1158811133, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACM2SM6JYTMXLBHZGT36ELVPRTXRANCNFSM5ZBY3R2Q . You are receiving this because you authored the thread.Message ID: @.***>

-- Gautier MICHELIN

collectiveaccess commented 2 years ago

Ok sure let's do that then.

Can you make a new PR for both master and develop?

thanks