-
-
Notifications
You must be signed in to change notification settings - Fork 18
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
sanitize submission result #1330
Conversation
c9b0546
to
5c2d7bf
Compare
Codecov Report
@@ Coverage Diff @@
## develop #1330 +/- ##
===========================================
+ Coverage 82.18% 83.78% +1.59%
===========================================
Files 89 89
Lines 4154 4156 +2
===========================================
+ Hits 3414 3482 +68
+ Misses 740 674 -66
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need a test where the actual stripping is tested? For example, for the script tag, we want the tag + contents gone and not only the tag itself.
Note: we get deprecation warnings because of this but it should be fixed in the next version of |
1dfbd95
to
03effe1
Compare
I've added tests for and fixed the stripping of the content of tags in d13c4ee. It's quite hard to test this in the tests of the renderers though, since we actually want to escape most of the HTML in the JSON (e.g. in the diff table), which means the content will still be there (escaped, but otherwise unchanged). |
03effe1
to
d13c4ee
Compare
This reverts commit d13c4ee.
"generated": "<script>alert('Your mother was a hamster and your father smelt of elderberries.')</script>" | ||
"accepted": false, | ||
"description": "test whether the user can XSS", | ||
"expected": "<script>alert('Your mother was a hamster and your father smelt of elderberries.')</script><script>alert('test')</script><script>alert('test')</script><script>alert('test')</script><script>alert('test')</script><script>alert('test')</script><script>alert('test')</script><script>alert('test')</script><script>alert('test')</script><script>alert('test')</script><script>alert('test')</script><script>alert('test')</script><script>alert('test')</script><script>alert('test')</script><script>alert('test')</script><script>alert('test')</script><script>alert('test')</script><script>alert('test')</script><script>alert('test')</script><script>alert('test')</script><script>alert('test')</script><script>alert('test')</script>", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This pull request sanitizes submission results (i.e. we prevent additional
<script>
in judge output from being executed).TODO:
Fixes #1139.