From b62d7e3e8eaa80e201af3141fb4fe26c39e1ff79 Mon Sep 17 00:00:00 2001
From: Maxime Beauchemin <maximebeauchemin@gmail.com>
Date: Fri, 16 Sep 2016 16:25:42 -0700
Subject: [PATCH] [security] prevent XSS on FAB list views (#1125)

* [security] prevent XSS on FAB list views

* addressing comments
---
 caravel/models.py                             | 32 +++++++++----------
 .../appbuilder/general/widgets/list.html      |  2 +-
 caravel/views.py                              |  6 ++--
 3 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/caravel/models.py b/caravel/models.py
index e61468f5a452c..1ca7483a3fcf5 100644
--- a/caravel/models.py
+++ b/caravel/models.py
@@ -22,7 +22,7 @@
 import sqlparse
 from dateutil.parser import parse
 
-from flask import request, g
+from flask import escape, g, Markup, request
 from flask_appbuilder import Model
 from flask_appbuilder.models.mixins import AuditMixin
 from flask_appbuilder.models.decorators import renders
@@ -102,12 +102,13 @@ def changed_by_(self):
 
     @renders('changed_on')
     def changed_on_(self):
-        return '<span class="no-wrap">{}</span>'.format(self.changed_on)
+        return Markup(
+            '<span class="no-wrap">{}</span>'.format(self.changed_on))
 
     @renders('changed_on')
     def modified(self):
         s = humanize.naturaltime(datetime.now() - self.changed_on)
-        return '<span class="no-wrap">{}</nobr>'.format(s)
+        return Markup('<span class="no-wrap">{}</span>'.format(s))
 
     @property
     def icons(self):
@@ -252,8 +253,8 @@ def edit_url(self):
     @property
     def slice_link(self):
         url = self.slice_url
-        return '<a href="{url}">{obj.slice_name}</a>'.format(
-            url=url, obj=self)
+        name = escape(self.slice_name)
+        return Markup('<a href="{url}">{name}</a>'.format(**locals()))
 
     def get_viz(self, url_params_multidict=None):
         """Creates :py:class:viz.BaseViz object from the url_params_multidict.
@@ -349,7 +350,9 @@ def sqla_metadata(self):
         return metadata.reflect()
 
     def dashboard_link(self):
-        return '<a href="{obj.url}">{obj.dashboard_title}</a>'.format(obj=self)
+        title = escape(self.dashboard_title)
+        return Markup(
+            '<a href="{self.url}">{title}</a>'.format(**locals()))
 
     @property
     def json_data(self):
@@ -684,7 +687,9 @@ def description_markeddown(self):
 
     @property
     def link(self):
-        return '<a href="{self.url}">{self.table_name}</a>'.format(**locals())
+        table_name = escape(self.table_name)
+        return Markup(
+            '<a href="{self.url}">{table_name}</a>'.format(**locals()))
 
     @property
     def perm(self):
@@ -728,10 +733,6 @@ def html(self):
     def name(self):
         return self.table_name
 
-    @renders('table_name')
-    def table_link(self):
-        return '<a href="{obj.explore_url}">{obj.table_name}</a>'.format(obj=self)
-
     @property
     def metrics_combo(self):
         return sorted(
@@ -1231,9 +1232,8 @@ def perm(self):
 
     @property
     def link(self):
-        return (
-            '<a href="{self.url}">'
-            '{self.datasource_name}</a>').format(**locals())
+        name = escape(self.datasource_name)
+        return Markup('<a href="{self.url}">{name}</a>').format(**locals())
 
     @property
     def full_name(self):
@@ -1247,8 +1247,8 @@ def __repr__(self):
     @renders('datasource_name')
     def datasource_link(self):
         url = "/caravel/explore/{obj.type}/{obj.id}/".format(obj=self)
-        return '<a href="{url}">{obj.datasource_name}</a>'.format(
-            url=url, obj=self)
+        name = escape(self.datasource_name)
+        return Markup('<a href="{url}">{name}</a>'.format(**locals()))
 
     def get_metric_obj(self, metric_name):
         return [
diff --git a/caravel/templates/appbuilder/general/widgets/list.html b/caravel/templates/appbuilder/general/widgets/list.html
index 294aa5b4c7559..797787cdca632 100644
--- a/caravel/templates/appbuilder/general/widgets/list.html
+++ b/caravel/templates/appbuilder/general/widgets/list.html
@@ -66,7 +66,7 @@
                           id="{{ '{}__{}'.format(pk, value) }}"
                           data-checkbox-api-prefix="/caravel/checkbox/{{ modelview_name }}/{{ pk }}/{{ value }}/">
                     {% else %}
-                      {{ item[value]|safe }}
+                      {{ item[value] }}
                     {% endif %}
                     </td>
                 {% endfor %}
diff --git a/caravel/views.py b/caravel/views.py
index a3f510580db79..0f0e69070ab78 100755
--- a/caravel/views.py
+++ b/caravel/views.py
@@ -535,10 +535,10 @@ class DatabaseTablesAsync(DatabaseView):
 class TableModelView(CaravelModelView, DeleteMixin):  # noqa
     datamodel = SQLAInterface(models.SqlaTable)
     list_columns = [
-        'table_link', 'database', 'is_featured',
+        'link', 'database', 'is_featured',
         'changed_by_', 'changed_on_']
     order_columns = [
-        'table_link', 'database', 'is_featured', 'changed_on_']
+        'link', 'database', 'is_featured', 'changed_on_']
     add_columns = ['table_name', 'database', 'schema']
     edit_columns = [
         'table_name', 'sql', 'is_featured', 'database', 'schema',
@@ -563,7 +563,7 @@ class TableModelView(CaravelModelView, DeleteMixin):  # noqa
     }
     base_filters = [['id', TableSlice, lambda: []]]
     label_columns = {
-        'table_link': _("Table"),
+        'link': _("Table"),
         'changed_by_': _("Changed By"),
         'database': _("Database"),
         'changed_on_': _("Last Changed"),