Merge pull request #2908 from graphite-project/copilot/fix-issue-2794

Fix reflected XSS in composer mygraph view

Author: deniszh

webapp/graphite/composer/views.py

@@ -21,6 +21,7 @@
 from django.http import HttpResponse
 from django.conf import settings
 from django.core.exceptions import ObjectDoesNotExist
+from django.utils.html import escape
 
 
 def composer(request):
@@ -65,7 +66,7 @@ def mygraph(request):
                 newGraph.save()
             except Exception:
                 log.exception("Failed to create new MyGraph in /composer/mygraph/, graphName=%s" % graphName)
-                return HttpResponse("Failed to save graph %s" % graphName)
+                return HttpResponse("Failed to save graph %s" % escape(graphName))
 
         return HttpResponse("SAVED")
 
@@ -75,9 +76,9 @@ def mygraph(request):
             existingGraph.delete()
 
         except ObjectDoesNotExist:
-            return HttpResponse("No such graph '%s'" % graphName)
+            return HttpResponse("No such graph '%s'" % escape(graphName))
 
         return HttpResponse("DELETED")
 
     else:
-        return HttpResponse("Invalid operation '%s'" % action)
+        return HttpResponse("Invalid operation '%s'" % escape(action))

webapp/tests/test_xss.py

@@ -6,6 +6,8 @@
 except ImportError:  # Django < 1.10
     from django.urls import reverse
 
+from django.contrib.auth.models import User
+
 from .base import TestCase
 
 # Silence logging during tests
@@ -49,3 +51,25 @@ def test_find_xss_script_tag(self):
         for param in ('from', 'until'):
             response = self.client.get(url, {'query': 'test', param: xssStr})
             self.assertXSS(response, status_code=400, msg_prefix='XSS detected in %s: ' % param)
+
+
+class ComposerMyGraphXSSTest(TestCase):
+    def setUp(self):
+        self.user = User.objects.create_user('testxss', 'testxss@example.com', 'pass')
+        self.client.login(username='testxss', password='pass')
+
+    def test_mygraph_xss_action(self):
+        """Test that XSS in the action parameter is properly escaped (issue #2794)"""
+        url = reverse('composer_mygraph')
+        xssStr = '"><script>alert(1)</script>'
+
+        response = self.client.get(url, {'action': xssStr, 'graphName': 'test'})
+        self.assertXSS(response, msg_prefix='XSS detected in action: ')
+
+    def test_mygraph_xss_graphname(self):
+        """Test that XSS in the graphName parameter is properly escaped (issue #2794)"""
+        url = reverse('composer_mygraph')
+        xssStr = '"><script>alert(1)</script>'
+
+        response = self.client.get(url, {'action': 'delete', 'graphName': xssStr})
+        self.assertXSS(response, msg_prefix='XSS detected in graphName: ')