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

Fix XSS security vulnerability #2072

Merged
merged 6 commits into from Jan 14, 2021
Merged

Fix XSS security vulnerability #2072

merged 6 commits into from Jan 14, 2021

Conversation

arzola
Copy link
Contributor

@arzola arzola commented Jan 12, 2021

Related issues #2066 and #404

This security fix clean and sanitize metadata book info metaboxes to prevent XSS attacks on fields that allows HTML input, this uses Htmlawed to filter and sanitize the input values.

How to test

Requirements:

  • Access to the latest Pressbooks admin interface

Steps:

  • Enter to the admin interface
  • Select any book and go to the book info page
  • Try to fill any wysiwyg and textareas with malicious XSS strings and open the book front page of that book to see the safe sanitized values printed
  • TIP you can use the following XSS Filter Evasion Cheat Sheet
  • You can try to test any input field metabox in the book info with the previous step, those values are already being properly escaped in the cover front page

Notes

Two tests were added

Functional test

Unit test

@arzola arzola changed the title Improved sanitization function Fixed XSS security vulnerability Jan 12, 2021
@codecov
Copy link

codecov bot commented Jan 12, 2021

Codecov Report

Merging #2072 (d637867) into dev (b266952) will increase coverage by 0.01%.
The diff coverage is 87.50%.

@@             Coverage Diff              @@
##                dev    #2072      +/-   ##
============================================
+ Coverage     67.73%   67.75%   +0.01%     
  Complexity     4907     4907              
============================================
  Files           128      128              
  Lines         21190    21198       +8     
============================================
+ Hits          14354    14362       +8     
  Misses         6836     6836              

@SteelWagstaff
Copy link
Member

@arzola will you add a reference to the issue addressed by this PR in the PR description as well as any information the reviewer/tester might need to review/test this PR? See https://github.com/pressbooks/pressbooks-lti-provider-1p3/pull/126 or #2070 for two previous examples.

@SteelWagstaff SteelWagstaff changed the title Fixed XSS security vulnerability Fix XSS security vulnerability Jan 13, 2021
@arzola
Copy link
Contributor Author

arzola commented Jan 13, 2021

@SteelWagstaff @richard015ar I added the description I think now it's ready for review, thanks 👍

@richard015ar richard015ar self-requested a review January 14, 2021 00:45
Copy link
Contributor

@richard015ar richard015ar left a comment

Choose a reason for hiding this comment

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

LGTM! thank you Oscar!

@richard015ar richard015ar merged commit 941a8c5 into dev Jan 14, 2021
@richard015ar richard015ar deleted the pb-404-xss-fix branch January 14, 2021 00:46
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.

None yet

3 participants