Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better XSS protection #1056

Merged
merged 3 commits into from
Apr 16, 2013
Merged

Conversation

rjmackay
Copy link
Contributor

@rjmackay rjmackay commented Apr 9, 2013

  • Add HTMLPurifier library (LGPL)
  • Add helper functions to html helper
  • Set default encoding header to UTF-8
  • Make sure the doctype is the same everywhere (admin/members/frontend)
  • Remove use of strip_tags() and htmlspecialchars()
  • Replace vanilla htmlentities with html::escape() - make sure no one forgets the UTF-8
  • Remove _csv_text() fn - no longer used and was using strip_tags()

Feedback appreciated.

Particularly around the range of allowed tags, current: "a[href|title],p,img[src|alt],br,b,u,strong,em,i" and iframes from trusted sources: youtube, vimeo, soundcloud

Other iframes we should allow? Other tags we should allow?

Also on names of helper fns:
html::escape()
html::clean()
html::strip_tags()

* Add HTMLPurifier library (LGPL)
* Add helper functions to html helper
* Set default encoding header to UTF-8
* Make sure the doctype is the same everywhere (admin/members/frontend)
* Remove use of strip_tags() and htmlspecialchars()
* Replace vanilla htmlentities with html::escape() - make sure no one forgets the UTF-8
* Remove _csv_text() fn - no longer used and was using strip_tags()
@rjmackay
Copy link
Contributor Author

rjmackay commented Apr 9, 2013

Related to #511 and #536
Updated to make allowed tags and iframe urls configurable - however still need to make sure the defaults are good.

@rjmackay rjmackay mentioned this pull request Apr 9, 2013
@rjmackay
Copy link
Contributor Author

rjmackay commented Apr 9, 2013

thinking about this a little more still need to add a note in the UI with a list of allowed tags..

ping @kamaulynder can you review this?

@heatherleson
Copy link
Contributor

Maybe share this with our security working group?

@rjmackay
Copy link
Contributor Author

Updated to add UI about what is allowed
Screen Shot 2013-04-16 at 10 20 53 AM

@kamaulynder
Copy link
Contributor

For any other tags or iframes, these can be added when needed. Otherwise, it looks good.

@rjmackay rjmackay merged commit 6d7cc9d into ushahidi:develop Apr 16, 2013
@rjmackay rjmackay deleted the html-purifier-xss-protect branch April 16, 2013 21:44
rjmackay added a commit to rjmackay/Ushahidi_Web that referenced this pull request Apr 23, 2013
rjmackay added a commit that referenced this pull request Apr 23, 2013
This avoids errors if upgraders don't update config.php

Conflicts:

	application/hooks/2_settings.php
rjmackay added a commit to rjmackay/Ushahidi_Web that referenced this pull request Apr 23, 2013
This avoids errors if upgraders don't update config.php
rjmackay added a commit to rjmackay/Ushahidi_Web that referenced this pull request Apr 23, 2013
Conflicts:

	themes/default/views/reports/list.php
rjmackay added a commit to rjmackay/Ushahidi_Web that referenced this pull request Apr 23, 2013
This avoids errors if upgraders don't update config.php
rjmackay added a commit to rjmackay/Ushahidi_Web that referenced this pull request Apr 30, 2013
Conflicts:

	themes/default/views/reports/list.php
rjmackay added a commit to rjmackay/Ushahidi_Web that referenced this pull request Apr 30, 2013
This avoids errors if upgraders don't update config.php
rjmackay added a commit that referenced this pull request May 1, 2013
This avoids errors if upgraders don't update config.php

Conflicts:

	application/i18n
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants