From a2dde4136310f316c33d99e292b194dd05170bb1 Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Tue, 16 Jul 2019 09:12:07 +0300 Subject: [PATCH 1/8] avoid catching errors on text widgets' load(), as they don't have a visualization and therefore do not return any promise --- .../app/pages/dashboards/public-dashboard-page.js | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/client/app/pages/dashboards/public-dashboard-page.js b/client/app/pages/dashboards/public-dashboard-page.js index aaaaf17a57..45b7c990c5 100644 --- a/client/app/pages/dashboards/public-dashboard-page.js +++ b/client/app/pages/dashboards/public-dashboard-page.js @@ -39,12 +39,15 @@ const PublicDashboardPage = { this.dashboard = new Dashboard(data); this.dashboard.widgets = Dashboard.prepareDashboardWidgets(this.dashboard.widgets); this.dashboard.widgets.forEach((widget) => { - widget.load(!!refreshRate).catch((error) => { - const isSafe = widget.getQuery() ? widget.getQuery().is_safe : true; - if (!isSafe) { - error.errorMessage = 'This query contains potentially unsafe parameters and cannot be executed on a publicly shared dashboard.'; - } - }); + const promise = widget.load(!!refreshRate); + if (promise) { + promise.catch((error) => { + const isSafe = widget.getQuery() ? widget.getQuery().is_safe : true; + if (!isSafe) { + error.errorMessage = 'This query contains potentially unsafe parameters and cannot be executed on a publicly shared dashboard.'; + } + }); + } }); this.filters = []; // TODO: implement (@/services/dashboard.js:collectDashboardFilters) this.filtersOnChange = (allFilters) => { From 645bf3d996d34dd2587453cdbfb042837fb601c3 Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Tue, 16 Jul 2019 09:14:13 +0300 Subject: [PATCH 2/8] throw error when failing to load widgets on public dashboards - in case something needs to be done with it at a later time, and it's the right thing to do anyway --- client/app/pages/dashboards/public-dashboard-page.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/client/app/pages/dashboards/public-dashboard-page.js b/client/app/pages/dashboards/public-dashboard-page.js index 45b7c990c5..5b6045729d 100644 --- a/client/app/pages/dashboards/public-dashboard-page.js +++ b/client/app/pages/dashboards/public-dashboard-page.js @@ -46,6 +46,8 @@ const PublicDashboardPage = { if (!isSafe) { error.errorMessage = 'This query contains potentially unsafe parameters and cannot be executed on a publicly shared dashboard.'; } + + throw error; }); } }); From 52acdb37088640838ac92669cba02cca258a4a18 Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Tue, 16 Jul 2019 10:15:45 +0300 Subject: [PATCH 3/8] use Promise.resolve instead of checking for undefined --- .../pages/dashboards/public-dashboard-page.js | 19 ++++++++----------- client/app/services/widget.js | 2 +- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/client/app/pages/dashboards/public-dashboard-page.js b/client/app/pages/dashboards/public-dashboard-page.js index 5b6045729d..90866dfa9f 100644 --- a/client/app/pages/dashboards/public-dashboard-page.js +++ b/client/app/pages/dashboards/public-dashboard-page.js @@ -39,17 +39,14 @@ const PublicDashboardPage = { this.dashboard = new Dashboard(data); this.dashboard.widgets = Dashboard.prepareDashboardWidgets(this.dashboard.widgets); this.dashboard.widgets.forEach((widget) => { - const promise = widget.load(!!refreshRate); - if (promise) { - promise.catch((error) => { - const isSafe = widget.getQuery() ? widget.getQuery().is_safe : true; - if (!isSafe) { - error.errorMessage = 'This query contains potentially unsafe parameters and cannot be executed on a publicly shared dashboard.'; - } - - throw error; - }); - } + widget.load(!!refreshRate).catch((error) => { + const isSafe = widget.getQuery() ? widget.getQuery().is_safe : true; + if (!isSafe) { + error.errorMessage = 'This query contains potentially unsafe parameters and cannot be executed on a publicly shared dashboard.'; + } + + throw error; + }); }); this.filters = []; // TODO: implement (@/services/dashboard.js:collectDashboardFilters) this.filtersOnChange = (allFilters) => { diff --git a/client/app/services/widget.js b/client/app/services/widget.js index a682879c62..8816c7762f 100644 --- a/client/app/services/widget.js +++ b/client/app/services/widget.js @@ -114,7 +114,7 @@ function WidgetFactory($http, $location, Query) { load(force, maxAge) { if (!this.visualization) { - return undefined; + return Promise.resolve(); } // Both `this.data` and `this.queryResult` are query result objects; From e58787c6b86e838e6f525daab5b3ff77572daedf Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Tue, 16 Jul 2019 10:24:51 +0300 Subject: [PATCH 4/8] call serialize_query_result instead of directly calling to_dict --- redash/handlers/query_results.py | 4 ++-- redash/serializers/__init__.py | 2 +- redash/serializers/query_result.py | 4 ++++ 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/redash/handlers/query_results.py b/redash/handlers/query_results.py index 815a7a154d..a557abc069 100644 --- a/redash/handlers/query_results.py +++ b/redash/handlers/query_results.py @@ -12,7 +12,7 @@ from redash.tasks.queries import enqueue_query from redash.utils import (collect_parameters_from_request, gen_query_hash, json_dumps, utcnow, to_filename) from redash.models.parameterized_query import ParameterizedQuery, InvalidParameterError, dropdown_values -from redash.serializers import serialize_query_result_to_csv, serialize_query_result_to_xlsx +from redash.serializers import serialize_query_result, serialize_query_result_to_csv, serialize_query_result_to_xlsx def error_response(message): @@ -42,7 +42,7 @@ def run_query(query, parameters, data_source, query_id, max_age=0): query_result = models.QueryResult.get_latest(data_source, query.text, max_age) if query_result: - return {'query_result': query_result.to_dict()} + return {'query_result': serialize_query_result(query_result)} else: job = enqueue_query(query.text, data_source, current_user.id, current_user.is_api_user(), metadata={ "Username": repr(current_user) if current_user.is_api_user() else current_user.email, diff --git a/redash/serializers/__init__.py b/redash/serializers/__init__.py index ba51ad0eb1..9223aaaac4 100644 --- a/redash/serializers/__init__.py +++ b/redash/serializers/__init__.py @@ -12,7 +12,7 @@ from redash.utils import json_loads from redash.models.parameterized_query import ParameterizedQuery -from .query_result import serialize_query_result_to_csv, serialize_query_result_to_xlsx +from .query_result import serialize_query_result, serialize_query_result_to_csv, serialize_query_result_to_xlsx def public_widget(widget): diff --git a/redash/serializers/query_result.py b/redash/serializers/query_result.py index 3a6dd7e123..5a960eb698 100644 --- a/redash/serializers/query_result.py +++ b/redash/serializers/query_result.py @@ -57,6 +57,10 @@ def _get_column_lists(columns): return fieldnames, special_columns +def serialize_query_result(query_result): + return query_result.to_dict() + + def serialize_query_result_to_csv(query_result): s = cStringIO.StringIO() From 7f749c179f6bf00150369d2a7a58ccd022788a1a Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Tue, 16 Jul 2019 11:01:37 +0300 Subject: [PATCH 5/8] filter unneeded query result fields for unauthenticated users --- redash/handlers/query_results.py | 2 +- redash/serializers/query_result.py | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/redash/handlers/query_results.py b/redash/handlers/query_results.py index a557abc069..28f8485b8b 100644 --- a/redash/handlers/query_results.py +++ b/redash/handlers/query_results.py @@ -42,7 +42,7 @@ def run_query(query, parameters, data_source, query_id, max_age=0): query_result = models.QueryResult.get_latest(data_source, query.text, max_age) if query_result: - return {'query_result': serialize_query_result(query_result)} + return {'query_result': serialize_query_result(query_result, current_user.is_api_user())} else: job = enqueue_query(query.text, data_source, current_user.id, current_user.is_api_user(), metadata={ "Username": repr(current_user) if current_user.is_api_user() else current_user.email, diff --git a/redash/serializers/query_result.py b/redash/serializers/query_result.py index 5a960eb698..2425254441 100644 --- a/redash/serializers/query_result.py +++ b/redash/serializers/query_result.py @@ -57,8 +57,12 @@ def _get_column_lists(columns): return fieldnames, special_columns -def serialize_query_result(query_result): - return query_result.to_dict() +def serialize_query_result(query_result, is_api_user): + if is_api_user: + publicly_needed_keys = ['data', 'retrieved_at'] + return { key: query_result.to_dict()[key] for key in publicly_needed_keys } + else: + return query_result.to_dict() def serialize_query_result_to_csv(query_result): From ffc49483ea1af8ae51aafd27778a63b240172cc2 Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Tue, 16 Jul 2019 11:14:13 +0300 Subject: [PATCH 6/8] test for serialization filtering --- tests/serializers/test_query_results.py | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/tests/serializers/test_query_results.py b/tests/serializers/test_query_results.py index ff77d20646..3f9e7d0381 100644 --- a/tests/serializers/test_query_results.py +++ b/tests/serializers/test_query_results.py @@ -6,7 +6,7 @@ from redash import models from redash.utils import utcnow, json_dumps -from redash.serializers import serialize_query_result_to_csv +from redash.serializers import serialize_query_result, serialize_query_result_to_csv data = { @@ -24,6 +24,19 @@ ] } +class QueryResultSerializationTest(BaseTestCase): + def test_serializes_all_keys_for_authenticated_users(self): + query_result = self.factory.create_query_result(data=json_dumps({})) + serialized = serialize_query_result(query_result, False) + self.assertSetEqual(set(query_result.to_dict().keys()), + set(serialized.keys())) + + def test_doesnt_serialize_sensitive_keys_for_unauthenticated_users(self): + query_result = self.factory.create_query_result(data=json_dumps({})) + serialized = serialize_query_result(query_result, True) + self.assertSetEqual(set(['data', 'retrieved_at']), + set(serialized.keys())) + class CsvSerializationTest(BaseTestCase): def get_csv_content(self): query_result = self.factory.create_query_result(data=json_dumps(data)) From df2691a4ed06305b3ec56b92abb24f3df97162b7 Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Tue, 16 Jul 2019 11:18:46 +0300 Subject: [PATCH 7/8] lint --- redash/serializers/query_result.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/redash/serializers/query_result.py b/redash/serializers/query_result.py index 2425254441..0a7a9a0bdc 100644 --- a/redash/serializers/query_result.py +++ b/redash/serializers/query_result.py @@ -60,7 +60,7 @@ def _get_column_lists(columns): def serialize_query_result(query_result, is_api_user): if is_api_user: publicly_needed_keys = ['data', 'retrieved_at'] - return { key: query_result.to_dict()[key] for key in publicly_needed_keys } + return {key: query_result.to_dict()[key] for key in publicly_needed_keys} else: return query_result.to_dict() From 371ecd2c500e2545dbe720c07924718ee96d9fb6 Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Tue, 16 Jul 2019 11:45:15 +0300 Subject: [PATCH 8/8] use project instead of list comprehension --- redash/serializers/query_result.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/redash/serializers/query_result.py b/redash/serializers/query_result.py index 0a7a9a0bdc..32eef9789f 100644 --- a/redash/serializers/query_result.py +++ b/redash/serializers/query_result.py @@ -1,7 +1,7 @@ import cStringIO import csv import xlsxwriter -from funcy import rpartial +from funcy import rpartial, project from dateutil.parser import isoparse as parse_date from redash.utils import json_loads, UnicodeWriter from redash.query_runner import (TYPE_BOOLEAN, TYPE_DATE, TYPE_DATETIME) @@ -60,7 +60,7 @@ def _get_column_lists(columns): def serialize_query_result(query_result, is_api_user): if is_api_user: publicly_needed_keys = ['data', 'retrieved_at'] - return {key: query_result.to_dict()[key] for key in publicly_needed_keys} + return project(query_result.to_dict(), publicly_needed_keys) else: return query_result.to_dict()