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

@@ -16,6 +16,7 @@ Bug fixes
 *********
 
 | `Bug #3994 <https://redmine.postgresql.org/issues/3994>`_ - Fix issue where the dependencies tab for inherited tables/foreign keys shows partial text.
+| `Bug #4036 <https://redmine.postgresql.org/issues/4036>`_ - Allow editing of data where a primary key column includes a % sign in the value.
 | `Bug #4171 <https://redmine.postgresql.org/issues/4171>`_ - Fix issue where reverse engineered SQL was failing for foreign tables, if it had "=" in the options.
 | `Bug #4195 <https://redmine.postgresql.org/issues/4195>`_ - Fix keyboard navigation in "inner" tabsets such as the Query Tool and Debugger.
 | `Bug #4228 <https://redmine.postgresql.org/issues/4228>`_ - Ensure the correct label is used in panel headers when viewing filtered rows.
@@ -35,4 +36,5 @@ Bug fixes
 | `Bug #4350 <https://redmine.postgresql.org/issues/4350>`_ - Ensure we include the CSRF token when uploading files.
 | `Bug #4357 <https://redmine.postgresql.org/issues/4357>`_ - Fix connection restoration issue when pgAdmin server is restarted and the page is refreshed.
 | `Bug #4360 <https://redmine.postgresql.org/issues/4360>`_ - Ensure the debugger control buttons are only enabled once initialisation is complete.
-| `Bug #4365 <https://redmine.postgresql.org/issues/4365>`_ - Fix help links for backup globals and backup server.
+| `Bug #4365 <https://redmine.postgresql.org/issues/4365>`_ - Fix help links for backup globals and backup server.
+| `Bug #4367 <https://redmine.postgresql.org/issues/4367>`_ - Fix an XSS issue seen in View/Edit data mode if a column name includes HTML.

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,
+            ['"<script>alert(1)</script>" 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,
+            '&lt;script&gt;alert(1)&lt;/script&gt;',
+            "View Data (SlickGrid)"
+        )
+
     def _check_xss_in_explain_module(self):
         print(
             "\n\tChecking the Graphical Explain plan for the XSS ...",

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

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
+                        def failure_handle():
-                        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:
                             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(

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);

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 %};

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 %};

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):

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.

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)