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

Sanitize server response to UI code, using cgi.escape #1140

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bbaja42
Copy link
Contributor

@bbaja42 bbaja42 commented Mar 29, 2017

Jinja is already doing most of sanitization, but since % raw syntax is used at few places, this sanitization still helps with preventing XSS attack.

Helps with solving #937
`

Read object, escape all dangerous values and return as json
'''
#First generate json, using nipap encoding library
# We can't sanitize passed value since html_sanitize works on primitive values

Choose a reason for hiding this comment

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

line too long (82 > 79 characters)

'''
Read object, escape all dangerous values and return as json
'''
#First generate json, using nipap encoding library

Choose a reason for hiding this comment

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

block comment should start with '# '

value = cgi.escape(value, quote=True)
return value

def html_sanitize_json(value):

Choose a reason for hiding this comment

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

expected 2 blank lines, found 1


def html_sanitize(value):
if isinstance(value, dict):
value = {html_sanitize(k):html_sanitize(v) for k, v in value.iteritems()}

Choose a reason for hiding this comment

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

missing whitespace after ':'
line too long (81 > 79 characters)

@@ -13,6 +13,31 @@

log = logging.getLogger(__name__)

import cgi

def html_sanitize(value):

Choose a reason for hiding this comment

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

expected 2 blank lines, found 1

@@ -13,6 +13,31 @@

log = logging.getLogger(__name__)

import cgi

Choose a reason for hiding this comment

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

module level import not at top of file

def html_sanitize(value):
if isinstance(value, dict):
value = {html_sanitize(k): html_sanitize(v) for
k, v in value.iteritems()}

Choose a reason for hiding this comment

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

continuation line under-indented for visual indent

@bbaja42
Copy link
Contributor Author

bbaja42 commented Mar 30, 2017

To give example of existing XSS, and to verify this solves it:
Go to http://nipap-demo.spritelink.net/, edit some prefix, and set following description

<script> alert("This is XSS :) " ) </script>

go back to main page, http://nipap-demo.spritelink.net/; there should be popup with XSS words.

after this fix, this will not be possible, I think :)

Jinja is already doing most of sanitization, but since % raw syntax is used at few places, this sanitization still helps with preventing XSS attack.
`
@bbaja42
Copy link
Contributor Author

bbaja42 commented Apr 22, 2017

Hey guys, any thoughts on this change?

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.

2 participants