From 5b0069cb906446b0360b546c0cc18be435f6f4c7 Mon Sep 17 00:00:00 2001 From: Maxime Beauchemin Date: Fri, 16 Sep 2016 09:41:39 -0700 Subject: [PATCH 1/2] [security] prevent XSS on FAB list views --- caravel/models.py | 34 +++++++++---------- .../appbuilder/general/widgets/list.html | 2 +- caravel/views.py | 6 ++-- 3 files changed, 21 insertions(+), 21 deletions(-) diff --git a/caravel/models.py b/caravel/models.py index 105de89495b6..41ba4120e822 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 '{}'.format(self.changed_on) + return Markup( + '{}'.format(self.changed_on)) @renders('changed_on') def modified(self): s = humanize.naturaltime(datetime.now() - self.changed_on) - return '{}'.format(s) + return Markup('{}'.format(s)) @property def icons(self): @@ -182,7 +183,7 @@ def datasource_link(self): if self.table: return self.table.link elif self.druid_datasource: - return self.druid_datasource.link + return self.link @property def datasource_edit_url(self): @@ -252,8 +253,8 @@ def edit_url(self): @property def slice_link(self): url = self.slice_url - return '{obj.slice_name}'.format( - url=url, obj=self) + name = escape(self.slice_name) + return Markup('{name}'.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 '{obj.dashboard_title}'.format(obj=self) + title = escape(self.dashboard_title) + return Markup( + '{title}'.format(**locals())) @property def json_data(self): @@ -684,7 +687,9 @@ def description_markeddown(self): @property def link(self): - return '{self.table_name}'.format(**locals()) + table_name = escape(self.table_name) + return Markup( + '{table_name}'.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 '{obj.table_name}'.format(obj=self) - @property def metrics_combo(self): return sorted( @@ -1228,9 +1229,8 @@ def perm(self): @property def link(self): - return ( - '' - '{self.datasource_name}').format(**locals()) + name = escape(self.datasource_name) + return Markup('{name}').format(**locals()) @property def full_name(self): @@ -1244,8 +1244,8 @@ def __repr__(self): @renders('datasource_name') def datasource_link(self): url = "/caravel/explore/{obj.type}/{obj.id}/".format(obj=self) - return '{obj.datasource_name}'.format( - url=url, obj=self) + name = escape(self.datasource_name) + return Markup('{name}'.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 294aa5b4c755..797787cdca63 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 %} {% endfor %} diff --git a/caravel/views.py b/caravel/views.py index cdcec757fc78..f768c9148f10 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"), From e6bf1e818032208d38d6d1043f670e2e53c100ad Mon Sep 17 00:00:00 2001 From: Maxime Beauchemin Date: Fri, 16 Sep 2016 11:55:35 -0700 Subject: [PATCH 2/2] addressing comments --- caravel/assets/javascripts/modules/caravel.js | 5 +++-- caravel/models.py | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/caravel/assets/javascripts/modules/caravel.js b/caravel/assets/javascripts/modules/caravel.js index c95b9fca8497..2e673aef9398 100644 --- a/caravel/assets/javascripts/modules/caravel.js +++ b/caravel/assets/javascripts/modules/caravel.js @@ -127,8 +127,9 @@ const px = function () { cachedSelector = $('#is_cached'); if (data !== undefined && data.is_cached) { cachedSelector - .attr('title', - 'Served from data cached at ' + data.cached_dttm + '. Click [Query] to force-refresh') + .attr( + 'title', + `Served from data cached at ${data.cached_dttm}. Click [Query] to force-refresh`) .show() .tooltip('fixTitle'); } else { diff --git a/caravel/models.py b/caravel/models.py index 41ba4120e822..bf6f4e5fa0b1 100644 --- a/caravel/models.py +++ b/caravel/models.py @@ -108,7 +108,7 @@ def changed_on_(self): @renders('changed_on') def modified(self): s = humanize.naturaltime(datetime.now() - self.changed_on) - return Markup('{}'.format(s)) + return Markup('{}'.format(s)) @property def icons(self): @@ -183,7 +183,7 @@ def datasource_link(self): if self.table: return self.table.link elif self.druid_datasource: - return self.link + return self.druid_datasource.link @property def datasource_edit_url(self):