619580 - Slugify tags ("<script>alert('xss')</script>" should be invalid)

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Developer Pages, defect)

Description

[krupa raj[:krupa]]

steps to reproduce:
1. Go to the /edit page for any add-on
2. Click on "edit" for Basic Information
3. Add <script>alert('xss')</script> as a tag
4. Save changes
5. Load https://addons.allizom.org/z/en-US/developers/addons

expected behavior:
That tag should not be accepted or at least, get slugified. 

actual behavior:
All developers associated with that add-on will not be able to access their dev-tools dashboard.

traceback details:

 File "/usr/lib/python2.6/site-packages/jinja2/environment.py", line 891, in render
   return self.environment.handle_exception(exc_info, True)

 File "/data/amo_python/www/preview/zamboni/apps/devhub/templates/devhub/addons/dashboard.html", line 1, in top-level template code
   {% extends "devhub/base.html" %}

 File "/data/amo_python/www/preview/zamboni/apps/devhub/templates/devhub/base.html", line 1, in top-level template code
   {% extends "base.html" %}

 File "/data/amo_python/www/preview/zamboni/templates/base.html", line 89, in top-level template code
   {% block main_content %}

 File "/data/amo_python/www/preview/zamboni/templates/base.html", line 94, in block "main_content"
   {% block content %}{% endblock %}

 File "/data/amo_python/www/preview/zamboni/apps/devhub/templates/devhub/addons/dashboard.html", line 71, in block "content"
   {{ item|safe }}

 File "/usr/lib/python2.6/site-packages/jinja2/filters.py", line 623, in do_mark_safe
   return Markup(value)

 File "/usr/lib/python2.6/site-packages/markupsafe/__init__.py", line 71, in __new__
   return unicode.__new__(cls, base)

 File "/data/amo_python/www/preview/zamboni/apps/devhub/models.py", line 214, in __unicode__
   return self.to_string()

 File "/data/amo_python/www/preview/zamboni/apps/devhub/models.py", line 203, in to_string
   tag = u'<a href="%s">%s</a>' % (arg.get_url_path(),

 File "/data/amo_python/www/preview/zamboni/apps/tags/models.py", line 34, in get_url_path
   return reverse('tags.detail', args=[self.tag_text])

 File "/data/amo_python/www/preview/zamboni/apps/amo/urlresolvers.py", line 58, in reverse
   url = django_reverse(viewname, urlconf, args, kwargs, prefix, current_app)

 File "/data/amo_python/www/preview/zamboni/vendor/src/django/django/core/urlresolvers.py", line 390, in reverse
   *args, **kwargs)))

 File "/data/amo_python/www/preview/zamboni/vendor/src/django/django/core/urlresolvers.py", line 336, in reverse
   "arguments '%s' not found." % (lookup_view_s, args, kwargs))

NoReverseMatch: Reverse for 'tags.detail' with arguments '(u"<script>alert('xss')</script>",)' and keyword arguments '{}' not found.

Comment 1

[Jeff Balogh (:jbalogh)]

This is not an xss; the page doesn't even load.  This is an input validation issue.

Comment 2

[Gregory Koberger (:gkoberger)]

We should come up with a list of allowed characters for tags-- slugifying is too strict (at the very least, we need spaces), however we're probably too liberal in what we allow.

Comment 3

[Wil Clouser [:clouserw]]

We should allow the same thing we allow collection slugs (which includes unicode).  I don't know if there is a constant for it like SLUG_OK though.

Comment 4

[Andy McKay]

Slugify looks good to me. Tags shouldn't really have spaces imho. 

Presumably will need to write a migration to clean up and conflate tags too.

Comment 5

[Andy McKay]

Migration to clean up and conflate is a new ticket:

https://bugzilla.mozilla.org/show_bug.cgi?id=621811

Comment 6

[Andy McKay]

Tags slugified. Spaces within a tag preserved. Spaces on either end stripped. Tags lowercased.

https://github.com/jbalogh/zamboni/commit/9287ad449d62bf918466298f8c5e6af0c9003dc3

Comment 7

[krupa raj[:krupa]]

verified @ https://addons.allizom.org/z/en-US/developers/addon/amazonassist-test-addon3011/edit

Comment 8

[Yvan Boily [:ygjb][:yvan]]

Adding keywords to bugs for metrics, no action required.  Sorry about bugmail spam.