bugfix: Improve support for special characters in schema and table na…

…mes (#7297) * Bugfix to SQL Lab to support tables and schemas with characters that require quoting * Remove debugging prints * Add uri encoding to secondary tables call * Quote schema names for presto * Quote selected_schema on Snowflake, MySQL and Hive * Remove redundant parens * Add python unit tests * Add js unit test * Fix flake8 linting error
apache · May 8, 2019 · 959c35d · 959c35d
1 parent a3f0912
commit 959c35d
Show file tree

Hide file tree

Showing 7 changed files with 73 additions and 19 deletions.
diff --git a/superset/assets/spec/javascripts/components/TableSelector_spec.jsx b/superset/assets/spec/javascripts/components/TableSelector_spec.jsx
@@ -85,6 +85,7 @@ describe('TableSelector', () => {
         .getTableNamesBySubStr('')
         .then((data) => {
           expect(data).toEqual({ options: [] });
+          return Promise.resolve();
         }));
 
     it('should handle table name', () => {
@@ -104,6 +105,23 @@ describe('TableSelector', () => {
         .then((data) => {
           expect(fetchMock.calls(GET_TABLE_NAMES_GLOB)).toHaveLength(1);
           expect(data).toEqual(mockTableOptions);
+          return Promise.resolve();
+        });
+    });
+
+    it('should escape schema and table names', () => {
+      const GET_TABLE_GLOB = 'glob:*/superset/tables/1/*/*';
+      const mockTableOptions = { options: [table] };
+      wrapper.setProps({ schema: 'slashed/schema' });
+      fetchMock.get(GET_TABLE_GLOB, mockTableOptions, { overwriteRoutes: true });
+
+      return wrapper
+        .instance()
+        .getTableNamesBySubStr('slashed/table')
+        .then(() => {
+          expect(fetchMock.lastUrl(GET_TABLE_GLOB))
+            .toContain('/slashed%252Fschema/slashed%252Ftable');
+          return Promise.resolve();
         });
     });
   });
@@ -125,6 +143,7 @@ describe('TableSelector', () => {
         .fetchTables(true, 'birth_names')
         .then(() => {
           expect(wrapper.state().tableOptions).toHaveLength(3);
+          return Promise.resolve();
         });
     });
 
@@ -138,6 +157,7 @@ describe('TableSelector', () => {
           expect(wrapper.state().tableOptions).toEqual([]);
           expect(wrapper.state().tableOptions).toHaveLength(0);
           expect(mockedProps.handleError.callCount).toBe(1);
+          return Promise.resolve();
         });
     });
   });

diff --git a/superset/assets/src/SqlLab/actions/sqlLab.js b/superset/assets/src/SqlLab/actions/sqlLab.js
@@ -285,7 +285,8 @@ export function addTable(query, tableName, schemaName) {
       }),
     );
 
-    SupersetClient.get({ endpoint: `/superset/table/${query.dbId}/${tableName}/${schemaName}/` })
+    SupersetClient.get({ endpoint: encodeURI(`/superset/table/${query.dbId}/` +
+            `${encodeURIComponent(tableName)}/${encodeURIComponent(schemaName)}/`) })
       .then(({ json }) => {
         const dataPreviewQuery = {
           id: shortid.generate(),
@@ -322,7 +323,8 @@ export function addTable(query, tableName, schemaName) {
       );
 
     SupersetClient.get({
-      endpoint: `/superset/extra_table_metadata/${query.dbId}/${tableName}/${schemaName}/`,
+      endpoint: encodeURI(`/superset/extra_table_metadata/${query.dbId}/` +
+          `${encodeURIComponent(tableName)}/${encodeURIComponent(schemaName)}/`),
     })
       .then(({ json }) =>
         dispatch(mergeTable({ ...table, ...json, isExtraMetadataLoading: false })),

diff --git a/superset/assets/src/components/TableSelector.jsx b/superset/assets/src/components/TableSelector.jsx
@@ -90,7 +90,7 @@ export default class TableSelector extends React.PureComponent {
   onChange() {
     this.props.onChange({
       dbId: this.state.dbId,
-      shema: this.state.schema,
+      schema: this.state.schema,
       tableName: this.state.tableName,
     });
   }
@@ -101,9 +101,8 @@ export default class TableSelector extends React.PureComponent {
       return Promise.resolve({ options });
     }
     return SupersetClient.get({
-      endpoint: (
-        `/superset/tables/${this.props.dbId}/` +
-        `${this.props.schema}/${input}`),
+      endpoint: encodeURI(`/superset/tables/${this.props.dbId}/` +
+        `${encodeURIComponent(this.props.schema)}/${encodeURIComponent(input)}`),
     }).then(({ json }) => ({ options: this.addOptionIfMissing(json.options, tableName) }));
   }
   dbMutator(data) {
@@ -123,7 +122,8 @@ export default class TableSelector extends React.PureComponent {
     const { dbId, schema } = this.props;
     if (dbId && schema) {
       this.setState(() => ({ tableLoading: true, tableOptions: [] }));
-      const endpoint = `/superset/tables/${dbId}/${schema}/${substr}/${forceRefresh}/`;
+      const endpoint = encodeURI(`/superset/tables/${dbId}/` +
+          `${encodeURIComponent(schema)}/${encodeURIComponent(substr)}/${forceRefresh}/`);
        return SupersetClient.get({ endpoint })
         .then(({ json }) => {
           const filterOptions = createFilterOptions({ options: json.options });

diff --git a/superset/db_engine_specs.py b/superset/db_engine_specs.py
@@ -37,6 +37,7 @@
 import textwrap
 import time
 from typing import List, Tuple
+from urllib import parse
 
 from flask import g
 from flask_babel import lazy_gettext as _
@@ -577,6 +578,7 @@ def adjust_database_uri(cls, uri, selected_schema=None):
         if '/' in uri.database:
             database = uri.database.split('/')[0]
         if selected_schema:
+            selected_schema = parse.quote(selected_schema, safe='')
             uri.database = database + '/' + selected_schema
         return uri
 
@@ -757,7 +759,7 @@ def convert_dttm(cls, target_type, dttm):
     @classmethod
     def adjust_database_uri(cls, uri, selected_schema=None):
         if selected_schema:
-            uri.database = selected_schema
+            uri.database = parse.quote(selected_schema, safe='')
         return uri
 
     @classmethod
@@ -1081,6 +1083,7 @@ def select_star(cls, my_db, table_name: str, engine: Engine, schema: str = None,
     def adjust_database_uri(cls, uri, selected_schema=None):
         database = uri.database
         if selected_schema and database:
+            selected_schema = parse.quote(selected_schema, safe='')
             if '/' in database:
                 database = database.split('/')[0] + '/' + selected_schema
             else:
@@ -1484,7 +1487,7 @@ def convert_dttm(cls, target_type, dttm):
     @classmethod
     def adjust_database_uri(cls, uri, selected_schema=None):
         if selected_schema:
-            uri.database = selected_schema
+            uri.database = parse.quote(selected_schema, safe='')
         return uri
 
     @classmethod

diff --git a/superset/utils/core.py b/superset/utils/core.py
@@ -33,6 +33,7 @@
 import sys
 from time import struct_time
 from typing import List, Optional, Tuple
+from urllib.parse import unquote_plus
 import uuid
 import zlib
 
@@ -141,8 +142,18 @@ def wrapper(f):
         return wrapper
 
 
-def js_string_to_python(item: str) -> Optional[str]:
-    return None if item in ('null', 'undefined') else item
+def parse_js_uri_path_item(item: Optional[str], unquote: bool = True,
+                           eval_undefined: bool = False) -> Optional[str]:
+    """Parse a uri path item made with js.
+
+    :param item: a uri path component
+    :param unquote: Perform unquoting of string using urllib.parse.unquote_plus()
+    :param eval_undefined: When set to True and item is either 'null'  or 'undefined',
+    assume item is undefined and return None.
+    :return: Either None, the original item or unquoted item
+    """
+    item = None if eval_undefined and item in ('null', 'undefined') else item
+    return unquote_plus(item) if unquote and item else item
 
 
 def string_to_num(s: str):

diff --git a/superset/views/core.py b/superset/views/core.py
@@ -1566,8 +1566,8 @@ def tables(self, db_id, schema, substr, force_refresh='false'):
         """Endpoint to fetch the list of tables for given database"""
         db_id = int(db_id)
         force_refresh = force_refresh.lower() == 'true'
-        schema = utils.js_string_to_python(schema)
-        substr = utils.js_string_to_python(substr)
+        schema = utils.parse_js_uri_path_item(schema, eval_undefined=True)
+        substr = utils.parse_js_uri_path_item(substr, eval_undefined=True)
         database = db.session.query(models.Database).filter_by(id=db_id).one()
 
         if schema:
@@ -2350,11 +2350,11 @@ def sqllab_viz(self):
         }))
 
     @has_access
-    @expose('/table/<database_id>/<path:table_name>/<schema>/')
+    @expose('/table/<database_id>/<table_name>/<schema>/')
     @log_this
     def table(self, database_id, table_name, schema):
-        schema = utils.js_string_to_python(schema)
-        table_name = parse.unquote_plus(table_name)
+        schema = utils.parse_js_uri_path_item(schema, eval_undefined=True)
+        table_name = utils.parse_js_uri_path_item(table_name)
         mydb = db.session.query(models.Database).filter_by(id=database_id).one()
         payload_columns = []
         indexes = []
@@ -2410,11 +2410,11 @@ def table(self, database_id, table_name, schema):
         return json_success(json.dumps(tbl))
 
     @has_access
-    @expose('/extra_table_metadata/<database_id>/<path:table_name>/<schema>/')
+    @expose('/extra_table_metadata/<database_id>/<table_name>/<schema>/')
     @log_this
     def extra_table_metadata(self, database_id, table_name, schema):
-        schema = utils.js_string_to_python(schema)
-        table_name = parse.unquote_plus(table_name)
+        schema = utils.parse_js_uri_path_item(schema, eval_undefined=True)
+        table_name = utils.parse_js_uri_path_item(table_name)
         mydb = db.session.query(models.Database).filter_by(id=database_id).one()
         payload = mydb.db_engine_spec.extra_table_metadata(
             mydb, table_name, schema)
@@ -2427,6 +2427,8 @@ def extra_table_metadata(self, database_id, table_name, schema):
     def select_star(self, database_id, table_name, schema=None):
         mydb = db.session.query(
             models.Database).filter_by(id=database_id).first()
+        schema = utils.parse_js_uri_path_item(schema, eval_undefined=True)
+        table_name = utils.parse_js_uri_path_item(table_name)
         return json_success(
             mydb.select_star(
                 table_name,

diff --git a/tests/utils_tests.py b/tests/utils_tests.py
@@ -35,6 +35,7 @@
     merge_extra_filters,
     merge_request_params,
     parse_human_timedelta,
+    parse_js_uri_path_item,
     validate_json,
     zlib_compress,
     zlib_decompress_to_string,
@@ -756,3 +757,18 @@ def test_convert_legacy_filters_into_adhoc_present_and_nonempty(self):
         }
         convert_legacy_filters_into_adhoc(form_data)
         self.assertEquals(form_data, expected)
+
+    def test_parse_js_uri_path_items_eval_undefined(self):
+        self.assertIsNone(parse_js_uri_path_item('undefined', eval_undefined=True))
+        self.assertIsNone(parse_js_uri_path_item('null', eval_undefined=True))
+        self.assertEqual('undefined', parse_js_uri_path_item('undefined'))
+        self.assertEqual('null', parse_js_uri_path_item('null'))
+
+    def test_parse_js_uri_path_items_unquote(self):
+        self.assertEqual('slashed/name', parse_js_uri_path_item('slashed%2fname'))
+        self.assertEqual('slashed%2fname', parse_js_uri_path_item('slashed%2fname',
+                                                                  unquote=False))
+
+    def test_parse_js_uri_path_items_item_optional(self):
+        self.assertIsNone(parse_js_uri_path_item(None))
+        self.assertIsNotNone(parse_js_uri_path_item('item'))