From 6e8ebbd375f825acde7383244bf83d1c9c1c15e5 Mon Sep 17 00:00:00 2001 From: Aditya Toshniwal Date: Thu, 20 Jun 2019 12:21:37 +0100 Subject: [PATCH] Allow editing of data where a primary key column includes a % sign in the value. Fixes #4036 Fix an XSS issue seen in View/Edit data mode if a column name includes HTML. Fixes #4367 --- docs/en_US/release_notes_4_9.rst | 4 +- .../xss_checks_panels_and_query_tool_test.py | 28 ++++++++++- web/pgadmin/tools/sqleditor/__init__.py | 7 ++- web/pgadmin/tools/sqleditor/command.py | 46 ++++++++++++++----- .../tools/sqleditor/static/js/sqleditor.js | 4 +- .../sqleditor/sql/default/insert.sql | 2 +- .../sqleditor/sql/default/update.sql | 2 +- web/pgadmin/utils/driver/psycopg2/cursor.py | 3 ++ web/regression/feature_utils/pgadmin_page.py | 22 +++++++++ .../python_test_utils/test_utils.py | 17 +++++-- 10 files changed, 112 insertions(+), 23 deletions(-) diff --git a/docs/en_US/release_notes_4_9.rst b/docs/en_US/release_notes_4_9.rst index 3ca0b530a..273f58b73 100644 --- a/docs/en_US/release_notes_4_9.rst +++ b/docs/en_US/release_notes_4_9.rst @@ -16,6 +16,7 @@ Bug fixes ********* | `Bug #3994 `_ - Fix issue where the dependencies tab for inherited tables/foreign keys shows partial text. +| `Bug #4036 `_ - Allow editing of data where a primary key column includes a % sign in the value. | `Bug #4171 `_ - Fix issue where reverse engineered SQL was failing for foreign tables, if it had "=" in the options. | `Bug #4195 `_ - Fix keyboard navigation in "inner" tabsets such as the Query Tool and Debugger. | `Bug #4228 `_ - Ensure the correct label is used in panel headers when viewing filtered rows. @@ -35,4 +36,5 @@ Bug fixes | `Bug #4350 `_ - Ensure we include the CSRF token when uploading files. | `Bug #4357 `_ - Fix connection restoration issue when pgAdmin server is restarted and the page is refreshed. | `Bug #4360 `_ - Ensure the debugger control buttons are only enabled once initialisation is complete. -| `Bug #4365 `_ - Fix help links for backup globals and backup server. \ No newline at end of file +| `Bug #4365 `_ - Fix help links for backup globals and backup server. +| `Bug #4367 `_ - Fix an XSS issue seen in View/Edit data mode if a column name includes HTML. \ No newline at end of file diff --git a/web/pgadmin/feature_tests/xss_checks_panels_and_query_tool_test.py b/web/pgadmin/feature_tests/xss_checks_panels_and_query_tool_test.py index d93d2b5db..c415b2b93 100644 --- a/web/pgadmin/feature_tests/xss_checks_panels_and_query_tool_test.py +++ b/web/pgadmin/feature_tests/xss_checks_panels_and_query_tool_test.py @@ -36,7 +36,8 @@ class CheckForXssFeatureTest(BaseFeatureTest): def before(self): test_utils.create_table( - self.server, self.test_db, self.test_table_name + self.server, self.test_db, self.test_table_name, + ['"" char'] ) # This is needed to test dependents tab (eg: BackGrid) test_utils.create_constraint( @@ -66,6 +67,11 @@ class CheckForXssFeatureTest(BaseFeatureTest): self._check_xss_in_query_tool_history() self.page.close_query_tool() + # Query tool view/edit data + self.page.open_view_data(self.test_table_name) + self._check_xss_view_data() + self.page.close_data_grid() + # Explain module self.page.open_query_tool() self._check_xss_in_explain_module() @@ -233,6 +239,26 @@ class CheckForXssFeatureTest(BaseFeatureTest): self.page.click_tab('Query Editor') + def _check_xss_view_data(self): + print( + "\n\tChecking the SlickGrid cell for the XSS", + file=sys.stderr, end="" + ) + + self.page.find_by_css_selector(".slick-header-column") + cells = self.driver.\ + find_elements_by_css_selector(".slick-header-column") + + # remove first element as it is row number. + # currently 4th col + source_code = cells[4].get_attribute('innerHTML') + + self._check_escaped_characters( + source_code, + '<script>alert(1)</script>', + "View Data (SlickGrid)" + ) + def _check_xss_in_explain_module(self): print( "\n\tChecking the Graphical Explain plan for the XSS ...", diff --git a/web/pgadmin/tools/sqleditor/__init__.py b/web/pgadmin/tools/sqleditor/__init__.py index 16f7133f7..ac923a550 100644 --- a/web/pgadmin/tools/sqleditor/__init__.py +++ b/web/pgadmin/tools/sqleditor/__init__.py @@ -11,6 +11,7 @@ import os import pickle import sys +import re import simplejson as json from flask import Response, url_for, render_template, session, request, \ @@ -478,7 +479,7 @@ def poll(trans_id): if not st: return internal_server_error(types) - for col_info in columns.values(): + for col_name, col_info in columns.items(): for col_type in types: if col_type['oid'] == col_info['type_code']: typname = col_type['typname'] @@ -487,6 +488,10 @@ def poll(trans_id): col_info['type_name'] = typname + # Using characters %, (, ) in the argument names is not + # supported in psycopg2 + col_info['pgadmin_alias'] = \ + re.sub("[%()]+", "|", col_name) session_obj['columns_info'] = columns # status of async_fetchmany_2darray is True and result is none # means nothing to fetch diff --git a/web/pgadmin/tools/sqleditor/command.py b/web/pgadmin/tools/sqleditor/command.py index d4b0700f9..28b7cc449 100644 --- a/web/pgadmin/tools/sqleditor/command.py +++ b/web/pgadmin/tools/sqleditor/command.py @@ -677,6 +677,11 @@ class TableCommand(GridCommand): list_of_sql = {} _rowid = None + pgadmin_alias = { + col_name: col_info['pgadmin_alias'] + for col_name, col_info in columns_info + .items() + } if conn.connected(): # Start the transaction @@ -745,6 +750,7 @@ class TableCommand(GridCommand): sql = render_template( "/".join([self.sql_path, 'insert.sql']), data_to_be_saved=column_data, + pgadmin_alias=pgadmin_alias, primary_keys=None, object_name=self.object_name, nsp_name=self.nsp_name, @@ -774,11 +780,17 @@ class TableCommand(GridCommand): list_of_sql[of_type] = [] for each_row in changed_data[of_type]: data = changed_data[of_type][each_row]['data'] - pk = changed_data[of_type][each_row]['primary_keys'] + pk_escaped = { + pk: pk_val.replace('%', '%%') + for pk, pk_val in + changed_data[of_type][each_row]['primary_keys'] + .items() + } sql = render_template( "/".join([self.sql_path, 'update.sql']), data_to_be_saved=data, - primary_keys=pk, + pgadmin_alias=pgadmin_alias, + primary_keys=pk_escaped, object_name=self.object_name, nsp_name=self.nsp_name, data_type=column_type @@ -831,17 +843,14 @@ class TableCommand(GridCommand): for opr, sqls in list_of_sql.items(): for item in sqls: if item['sql']: + item['data'] = { + pgadmin_alias[k] if k in pgadmin_alias else k: v + for k, v in item['data'].items() + } + row_added = None - # Fetch oids/primary keys - if 'select_sql' in item and item['select_sql']: - status, res = conn.execute_dict( - item['sql'], item['data']) - else: - status, res = conn.execute_void( - item['sql'], item['data']) - - if not status: + def failure_handle(): conn.execute_void('ROLLBACK;') # If we roll backed every thing then update the # message for each sql query. @@ -861,6 +870,21 @@ class TableCommand(GridCommand): return status, res, query_res, _rowid + try: + # Fetch oids/primary keys + if 'select_sql' in item and item['select_sql']: + status, res = conn.execute_dict( + item['sql'], item['data']) + else: + status, res = conn.execute_void( + item['sql'], item['data']) + except Exception as _: + failure_handle() + raise + + if not status: + return failure_handle() + # Select added row from the table if 'select_sql' in item: status, sel_res = conn.execute_dict( diff --git a/web/pgadmin/tools/sqleditor/static/js/sqleditor.js b/web/pgadmin/tools/sqleditor/static/js/sqleditor.js index 6af098b4b..e2d69f78a 100644 --- a/web/pgadmin/tools/sqleditor/static/js/sqleditor.js +++ b/web/pgadmin/tools/sqleditor/static/js/sqleditor.js @@ -783,7 +783,7 @@ define('tools.querytool', [ pos: c.pos, field: c.name, name: c.label, - display_name: c.display_name, + display_name: _.escape(c.display_name), column_type: c.column_type, column_type_internal: c.column_type_internal, not_null: c.not_null, @@ -794,7 +794,7 @@ define('tools.querytool', [ // Get the columns width based on longer string among data type or // column name. var column_type = c.column_type.trim(); - var label = c.name.length > column_type.length ? c.name : column_type; + var label = c.name.length > column_type.length ? _.escape(c.display_name) : column_type; if (_.isUndefined(column_size[table_name][c.name])) { options['width'] = SqlEditorUtils.calculateColumnWidth(label); diff --git a/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/insert.sql b/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/insert.sql index 7e3f8a073..b9fedeaa2 100644 --- a/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/insert.sql +++ b/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/insert.sql @@ -4,7 +4,7 @@ INSERT INTO {{ conn|qtIdent(nsp_name, object_name) }} ( {% if not loop.first %}, {% endif %}{{ conn|qtIdent(col) }}{% endfor %} ) VALUES ( {% for col in data_to_be_saved %} -{% if not loop.first %}, {% endif %}%({{ col }})s::{{ data_type[col] }}{% endfor %} +{% if not loop.first %}, {% endif %}%({{ pgadmin_alias[col] }})s::{{ data_type[col] }}{% endfor %} ) {% if pk_names and not has_oids %} returning {{pk_names}}{% endif %} {% if has_oids %} returning oid{% endif %}; diff --git a/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/update.sql b/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/update.sql index c9dfddfd9..8c011619f 100644 --- a/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/update.sql +++ b/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/update.sql @@ -1,7 +1,7 @@ {# Update the row with primary keys (specified in primary_keys) #} UPDATE {{ conn|qtIdent(nsp_name, object_name) }} SET {% for col in data_to_be_saved %} -{% if not loop.first %}, {% endif %}{{ conn|qtIdent(col) }} = %({{ col }})s::{{ data_type[col] }}{% endfor %} +{% if not loop.first %}, {% endif %}{{ conn|qtIdent(col) }} = %({{ pgadmin_alias[col] }})s::{{ data_type[col] }}{% endfor %} WHERE {% for pk in primary_keys %} {% if not loop.first %} AND {% endif %}{{ conn|qtIdent(pk) }} = {{ primary_keys[pk]|qtLiteral }}{% endfor %}; diff --git a/web/pgadmin/utils/driver/psycopg2/cursor.py b/web/pgadmin/utils/driver/psycopg2/cursor.py index 1e9f545e1..7bf2879e9 100644 --- a/web/pgadmin/utils/driver/psycopg2/cursor.py +++ b/web/pgadmin/utils/driver/psycopg2/cursor.py @@ -196,6 +196,9 @@ class DictCursor(_cursor): Execute function """ self._odt_desc = None + if params is not None and len(params) == 0: + params = None + return _cursor.execute(self, query, params) def executemany(self, query, params=None): diff --git a/web/regression/feature_utils/pgadmin_page.py b/web/regression/feature_utils/pgadmin_page.py index d633f95dc..33cc8fc4c 100644 --- a/web/regression/feature_utils/pgadmin_page.py +++ b/web/regression/feature_utils/pgadmin_page.py @@ -97,6 +97,28 @@ class PgadminPage: self.find_by_partial_link_text("Query Tool").click() + def open_view_data(self, table_name): + self.driver.find_element_by_link_text("Object").click() + ActionChains( + self.driver + ).move_to_element( + self.driver.find_element_by_link_text("View/Edit Data") + ).perform() + self.find_by_partial_link_text("All Rows").click() + time.sleep(1) + # wait until datagrid frame is loaded. + + self.click_tab(table_name) + + WebDriverWait(self.driver, self.timeout).until( + EC.visibility_of_element_located( + (By.CSS_SELECTOR, 'iframe') + ), "Timed out waiting for div element to appear" + ) + self.driver.switch_to.frame( + self.driver.find_element_by_tag_name('iframe') + ) + def enable_menu_item(self, menu_item, wait_time): start_time = time.time() # wait until menu becomes enabled. diff --git a/web/regression/python_test_utils/test_utils.py b/web/regression/python_test_utils/test_utils.py index 99cdb6dbe..96a009bdf 100644 --- a/web/regression/python_test_utils/test_utils.py +++ b/web/regression/python_test_utils/test_utils.py @@ -144,7 +144,7 @@ def create_database(server, db_name, encoding=None): traceback.print_exc(file=sys.stderr) -def create_table(server, db_name, table_name): +def create_table(server, db_name, table_name, extra_columns=[]): """ This function create the table in given database name :param server: server details @@ -166,18 +166,25 @@ def create_table(server, db_name, table_name): ) old_isolation_level = connection.isolation_level connection.set_isolation_level(0) + + extra_columns_sql = ", " + ", ".join(extra_columns) \ + if len(extra_columns) > 0 else '' + pg_cursor = connection.cursor() pg_cursor.execute( '''CREATE TABLE "%s" (some_column VARCHAR, value NUMERIC, - details VARCHAR)''' % table_name) + details VARCHAR%s)''' % (table_name, extra_columns_sql)) pg_cursor.execute( - '''INSERT INTO "%s" VALUES ('Some-Name', 6, 'some info')''' + '''INSERT INTO "%s"(some_column, value, details) + VALUES ('Some-Name', 6, 'some info')''' % table_name) pg_cursor.execute( - '''INSERT INTO "%s" VALUES ('Some-Other-Name', 22, + '''INSERT INTO "%s"(some_column, value, details) + VALUES ('Some-Other-Name', 22, 'some other info')''' % table_name) pg_cursor.execute( - '''INSERT INTO "%s" VALUES ('Yet-Another-Name', 14, + '''INSERT INTO "%s"(some_column, value, details) + VALUES ('Yet-Another-Name', 14, 'cool info')''' % table_name) connection.set_isolation_level(old_isolation_level)