Skip to content

Commit

Permalink
Better handling of invalid URLs, @karlcow review. #847
Browse files Browse the repository at this point in the history
  • Loading branch information
Hallvord R. M. Steen committed Dec 10, 2015
1 parent b2f51e9 commit 9b4121c
Showing 1 changed file with 17 additions and 6 deletions.
23 changes: 17 additions & 6 deletions webcompat/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@
# file, You can obtain one at http://mozilla.org/MPL/2.0/.

import json
import requests
import re
import urllib

import requests

from flask.ext.github import GitHubError
from flask import flash
from flask import g
Expand Down Expand Up @@ -278,12 +280,21 @@ def cssfixme():

url = request.args.get('url')
escaped_csscode = ''
req = None
if url:
req = requests.get(url)
# Security precautions: we want to make sure we escape HTML tags.
# (The code should end up in TEXTAREA so the main risk is actually
# some attacker sneaking in a </TEXTAREA> tag..)
escaped_csscode = req.text.replace('<', '&lt;')
try:
req = requests.get(url)
except requests.exceptions.MissingSchema, e:
# Somebody gave us an URL not prefixed by http(s)://
req = requests.get('http://{0}'.format(url))
except Exception,e:

This comment has been minimized.

Copy link
@karlcow

karlcow Dec 11, 2015

Member

syntax Exception, e

pass # If we can't load this URL, the TEXTAREA loads empty. No big deal.
if req:
# Security precautions: we want to make sure we escape HTML tags.
# (The code should end up in TEXTAREA so the main risk is actually
# some attacker sneaking in a </TEXTAREA> tag..)
rx_textarea = re.compile('</textarea>', re.IGNORECASE)
escaped_csscode = rx_textarea.sub('&lt;/textarea>', req.text)

This comment has been minimized.

Copy link
@karlcow

karlcow Dec 11, 2015

Member

Not enough. This will still create XSS vulnerability with

</textarea   ><script>alert('attacked')</script>

This comment has been minimized.

Copy link
@karlcow

karlcow Dec 11, 2015

Member

</textarea should be enough.

This comment has been minimized.

Copy link
@miketaylr

miketaylr Dec 11, 2015

Member

It's really early, so forgive the dumb question -- but why aren't we escaping all instances of <>&?

This comment has been minimized.

Copy link
@karlcow

karlcow Dec 11, 2015

Member

@miketaylr if you look at the line 286. It's what was there. Not sure why @hallvors decided to move away from it.

This comment has been minimized.

Copy link
@hallvors

hallvors Dec 11, 2015

Contributor

The problem is that TEXTAREA will render those entities in the content - literally, so escaping everything will mangle any CSS code that contains <>&. Hence div > p would show up as div &gt; p in the generated output.
AFAIK the only risk here is input with a TEXTAREA end tag followed by malicious stuff.

This comment has been minimized.

Copy link
@karlcow

karlcow Dec 11, 2015

Member

yep indeed… also content: '> ';

return render_template('cssfixme.html', escaped_csscode=escaped_csscode)


Expand Down

0 comments on commit 9b4121c

Please sign in to comment.