From 3b69f92d70f4ec9a2f1624238d92c2af7a792f2d Mon Sep 17 00:00:00 2001 From: Aditya Toshniwal Date: Fri, 14 Jun 2019 09:16:55 +0100 Subject: [PATCH] Ensure strings are properly encoded in the Query History. Fixes #4349 --- docs/en_US/release_notes_4_9.rst | 4 +- .../xss_checks_panels_and_query_tool_test.py | 56 +++++++++++++++++++ .../history/query_history_details.js | 8 +-- .../history/query_history_entries.js | 3 +- .../javascript/history/query_history_spec.js | 4 +- 5 files changed, 65 insertions(+), 10 deletions(-) diff --git a/docs/en_US/release_notes_4_9.rst b/docs/en_US/release_notes_4_9.rst index 07b69d37a..0c0efd4a4 100644 --- a/docs/en_US/release_notes_4_9.rst +++ b/docs/en_US/release_notes_4_9.rst @@ -26,5 +26,5 @@ Bug fixes | `Bug #4320 `_ - Fix issue where SSH tunnel connection using password is failing, it's regression of Master Password. | `Bug #4329 `_ - Fix an initialisation error when two functions with parameters are debugged in parallel. | `Bug #4343 `_ - Fix issue where property dialog of column should open properly for EPAS v12. -| `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. \ No newline at end of file +| `Bug #4349 `_ - Ensure strings are properly encoded in the Query History. +| `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. \ 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 8175bf9f3..d93d2b5db 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 @@ -63,6 +63,7 @@ class CheckForXssFeatureTest(BaseFeatureTest): # Query tool self.page.open_query_tool() self._check_xss_in_query_tool() + self._check_xss_in_query_tool_history() self.page.close_query_tool() # Explain module @@ -177,6 +178,61 @@ class CheckForXssFeatureTest(BaseFeatureTest): "Query tool (SlickGrid)" ) + def _check_xss_in_query_tool_history(self): + print( + "\n\tChecking the query tool history for the XSS", + file=sys.stderr, end="" + ) + + self.page.fill_codemirror_area_with( + "select '" + ) + self.page.find_by_id("btn-flash").click() + + self.page.click_tab('Query History') + + # Check for history entry + history_ele = self.page.find_by_css_selector( + ".query-history div.query-group:first-child" + " .list-item.selected .query" + ) + + source_code = history_ele.get_attribute('innerHTML') + + self._check_escaped_characters( + source_code, + '<script>alert(1)</script>', + "Query tool (History Entry)" + ) + + # Check for history details message + history_ele = self.page.find_by_css_selector( + ".query-detail .content-value" + ) + + source_code = history_ele.get_attribute('innerHTML') + + self._check_escaped_characters( + source_code, + '<script>alert(1)</script>', + "Query tool (History Details-Message)" + ) + + # Check for history details error message + history_ele = self.page.find_by_css_selector( + ".query-detail .history-error-text" + ) + + source_code = history_ele.get_attribute('innerHTML') + + self._check_escaped_characters( + source_code, + '<script>alert(1)</script>', + "Query tool (History Details-Error)" + ) + + self.page.click_tab('Query Editor') + def _check_xss_in_explain_module(self): print( "\n\tChecking the Graphical Explain plan for the XSS ...", diff --git a/web/pgadmin/static/js/sqleditor/history/query_history_details.js b/web/pgadmin/static/js/sqleditor/history/query_history_details.js index 4286d2ca5..9e6758dea 100644 --- a/web/pgadmin/static/js/sqleditor/history/query_history_details.js +++ b/web/pgadmin/static/js/sqleditor/history/query_history_details.js @@ -117,7 +117,7 @@ export default class QueryHistoryDetails { updateMessageContent() { this.$message_content .empty() - .append(`
${this.entry.message}
`); + .append(`
${_.escape(this.entry.message)}
`); } updateErrorMessage() { @@ -125,10 +125,8 @@ export default class QueryHistoryDetails { this.$errMsgBlock.removeClass('d-none'); this.$errMsgBlock.empty().append( `
- Error Message ${this.parseErrorMessage( - this.entry.message - )} -
` + Error Message${_.escape(this.parseErrorMessage(this.entry.message))} + ` ); } else { this.$errMsgBlock.addClass('d-none'); diff --git a/web/pgadmin/static/js/sqleditor/history/query_history_entries.js b/web/pgadmin/static/js/sqleditor/history/query_history_entries.js index 54a069cb9..51c7847e4 100644 --- a/web/pgadmin/static/js/sqleditor/history/query_history_entries.js +++ b/web/pgadmin/static/js/sqleditor/history/query_history_entries.js @@ -1,5 +1,6 @@ import moment from 'moment'; import $ from 'jquery'; +import _ from 'underscore'; const ARROWUP = 38; const ARROWDOWN = 40; @@ -68,7 +69,7 @@ export class QueryHistoryItem { this.$el = $( `
  • -
    ${this.entry.query}
    +
    ${_.escape(this.entry.query)}
    ${this.formatDate(this.entry.start_time)}
    diff --git a/web/regression/javascript/history/query_history_spec.js b/web/regression/javascript/history/query_history_spec.js index 976026d8c..d4782f2d6 100644 --- a/web/regression/javascript/history/query_history_spec.js +++ b/web/regression/javascript/history/query_history_spec.js @@ -260,7 +260,7 @@ describe('QueryHistory', () => { }); it('displays the error message on top of the details pane', () => { - expect(queryDetail.text()).toContain('Error Message message from second sql query'); + expect(queryDetail.text()).toContain('Error Messagemessage from second sql query'); }); }); }); @@ -322,7 +322,7 @@ describe('QueryHistory', () => { }); it('displays fourth query SQL in the right pane', () => { - expect(queryDetail.text()).toContain('Error Message unexpected error from fourth sql message'); + expect(queryDetail.text()).toContain('Error Messageunexpected error from fourth sql message'); }); }); });