From 7fd52cb1bbfbc2ec93245144b83f3ff6e40ea78b Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Wed, 8 Jun 2022 20:17:36 +0200 Subject: [PATCH 1/4] Add sessioncookie with SameSite=None; secure for tool_runner path That should fix https://github.com/galaxyproject/galaxy/issues/11066 / https://github.com/galaxyproject/galaxy/issues/11374. --- lib/galaxy/webapps/base/webapp.py | 41 ++++++++++++++++++++++++++++--- 1 file changed, 38 insertions(+), 3 deletions(-) diff --git a/lib/galaxy/webapps/base/webapp.py b/lib/galaxy/webapps/base/webapp.py index 306f25db402..e25a017316c 100644 --- a/lib/galaxy/webapps/base/webapp.py +++ b/lib/galaxy/webapps/base/webapp.py @@ -9,8 +9,14 @@ import socket import string import time from http.cookies import CookieError -from typing import Any, Dict -from urllib.parse import urlparse +from typing import ( + Any, + Dict, +) +from urllib.parse import ( + urljoin, + urlparse, +) import mako.lookup import mako.runtime @@ -63,6 +69,8 @@ UCSC_SERVERS = ( 'hgw8.soe.ucsc.edu', ) +TOOL_RUNNER_SESSION_COOKIE = "galaxytoolrunnersession" + class WebApplication(base.WebApplication): """ @@ -379,14 +387,41 @@ class GalaxyWebTransaction(base.DefaultWebTransaction, context.ProvidesHistoryCo if name in self.response.cookies: return self.response.cookies[name].value else: + if name not in self.request.cookies and TOOL_RUNNER_SESSION_COOKIE in self.request.cookies: + # TOOL_RUNNER_SESSION_COOKIE value is the encoded galaxysession cookie. + # We decode it here and pretend it's the galaxysession + tool_runner_path = urljoin(self.app.config.galaxy_url_prefix, "tool_runner") + if self.request.path.startswith(tool_runner_path): + return self.security.decode_guid(self.request.cookies[TOOL_RUNNER_SESSION_COOKIE].value) return self.request.cookies[name].value except Exception: return None - def set_cookie(self, value, name='galaxysession', path='/', age=90, version='1'): + def set_cookie(self, value, name="galaxysession", path="/", age=90, version="1"): + self._set_cookie(value, name=name, path=path, age=age, version=version) + if name == "galaxysession": + # Set an extra sessioncookie that will only be sent and be accepted on the tool_runner path. + # Use the id_secret to encode the sessioncookie, so if a malicious site + # obtains the sessioncookie they can only run tools. + self._set_cookie( + value, + name=TOOL_RUNNER_SESSION_COOKIE, + path=urljoin(path, "tool_runner"), + age=age, + version=version, + encode_value=True, + ) + tool_runner_cookie = self.response.cookies[TOOL_RUNNER_SESSION_COOKIE] + tool_runner_cookie["path"] = urljoin(path, "tool_runner") + tool_runner_cookie["SameSite"] = "None" + tool_runner_cookie["secure"] = True + + def _set_cookie(self, value, name="galaxysession", path="/", age=90, version="1", encode_value=False): """Convenience method for setting a session cookie""" # The galaxysession cookie value must be a high entropy 128 bit random number encrypted # using a server secret key. Any other value is invalid and could pose security issues. + if encode_value: + value = self.security.encode_guid(value) self.response.cookies[name] = unicodify(value) self.response.cookies[name]['path'] = path self.response.cookies[name]['max-age'] = 3600 * 24 * age # 90 days From eec196b31723ca0acfaf0df7c85c8b37a86bb866 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Thu, 9 Jun 2022 11:12:00 +0200 Subject: [PATCH 2/4] Api test to verify tool runner session handling --- lib/galaxy_test/api/test_authenticate.py | 27 ++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/lib/galaxy_test/api/test_authenticate.py b/lib/galaxy_test/api/test_authenticate.py index 613fd80640d..479d22ea150 100644 --- a/lib/galaxy_test/api/test_authenticate.py +++ b/lib/galaxy_test/api/test_authenticate.py @@ -1,4 +1,5 @@ import base64 +from urllib.parse import urljoin from requests import get @@ -27,3 +28,29 @@ class AuthenticationApiTestCase(ApiTestCase): random_api_url = self._api_url("users", use_key=False) random_api_response = get(random_api_url, params=dict(key=auth_dict["api_key"])) self._assert_status_code_is(random_api_response, 200) + + def test_tool_runner_session_cookie_handling(self): + response = get(self.url) + tool_runner_session_cookie = response.cookies["galaxytoolrunnersession"] + galaxy_session_cookie = response.cookies["galaxysession"] + assert tool_runner_session_cookie != galaxy_session_cookie + root_response = get(self.url, cookies={"galaxytoolrunnersession": tool_runner_session_cookie}) + root_response.raise_for_status() + # Browser will only send cookie to /tool_runner path, but let's make sure it isn't accepted. + # Galaxy responds with a new session and sessioncookie in that case. + # (We might want to redirect to the login page instead if require_login is set?) + assert root_response.cookies["galaxysession"] != galaxy_session_cookie + tool_runner_response = get( + urljoin(self.url, "tool_runner?tool_id=test_data_source"), + cookies={"galaxytoolrunnersession": tool_runner_session_cookie}, + ) + tool_runner_response.raise_for_status() + # Verify that we're not returning the sessioncookie + assert "galaxysession" not in tool_runner_response.cookies + # Make sure history for original session received job + current_history_json_response = get( + urljoin(self.url, "history/current_history_json"), cookies={"galaxysession": galaxy_session_cookie} + ) + current_history_json_response.raise_for_status() + current_history = current_history_json_response.json() + assert current_history["contents_active"]["active"] == 1 From 55ecc382e7ec88f3cb6ef4001cd2ac0110524313 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Thu, 9 Jun 2022 12:18:42 +0200 Subject: [PATCH 3/4] Add selenium test for ucsc table browser data source --- lib/galaxy/selenium/navigates_galaxy.py | 6 ++++ lib/galaxy/selenium/navigation.yml | 9 +++--- .../selenium/test_data_source_tools.py | 29 +++++++++++++++++++ test/functional/tools/samples_tool_conf.xml | 1 + test/functional/tools/ucsc_tablebrowser.xml | 1 + 5 files changed, 42 insertions(+), 4 deletions(-) create mode 100644 lib/galaxy_test/selenium/test_data_source_tools.py create mode 120000 test/functional/tools/ucsc_tablebrowser.xml diff --git a/lib/galaxy/selenium/navigates_galaxy.py b/lib/galaxy/selenium/navigates_galaxy.py index a4e2a1c4bb9..e01bca9fbe0 100644 --- a/lib/galaxy/selenium/navigates_galaxy.py +++ b/lib/galaxy/selenium/navigates_galaxy.py @@ -1245,6 +1245,12 @@ class NavigatesGalaxy(HasDriver): self.driver.execute_script("arguments[0].scrollIntoView(true);", tool_element) tool_link.wait_for_and_click() + def datasource_tool_open(self, tool_id): + tool_link = self.components.tool_panel.data_source_tool_link(tool_id=tool_id) + tool_element = tool_link.wait_for_present() + self.driver.execute_script("arguments[0].scrollIntoView(true);", tool_element) + tool_link.wait_for_and_click() + def tool_parameter_div(self, expanded_parameter_id): return self.components.tool_form.parameter_div(parameter=expanded_parameter_id).wait_for_clickable() diff --git a/lib/galaxy/selenium/navigation.yml b/lib/galaxy/selenium/navigation.yml index 0777d812704..cbe62a4dd9d 100644 --- a/lib/galaxy/selenium/navigation.yml +++ b/lib/galaxy/selenium/navigation.yml @@ -229,7 +229,7 @@ history_panel: refresh_button: '.history-refresh-button' name: '.title .name' name_beta: '.history-title span:last-child' - name_edit_input: + name_edit_input: selector: 'name input' type: data-description contents: '#current-history-panel .history-content' @@ -274,7 +274,7 @@ history_panel: selector: '//button[contains(span, "Return to legacy history panel")]' collection_menu_button: '.collection-menu' - collection_menu_edit_attributes: + collection_menu_edit_attributes: type: xpath selector: '//button[@title="Edit attributes"]' new_history_button: '.history-new-button' @@ -316,10 +316,10 @@ edit_dataset_attributes: edit_collection_attributes: selectors: - database_genome_tab: + database_genome_tab: type: xpath selector: '//a[contains(text(), "Database/Build")]' - database_value: + database_value: type: xpath selector: '//span[contains(text(), "${dbkey}")]' save_btn: '.save-collection-edit' @@ -331,6 +331,7 @@ tool_panel: selectors: tool_link: 'a[href$$="tool_runner?tool_id=${tool_id}"]' outer_tool_link: '.toolTitle a[href$$="tool_runner?tool_id=${tool_id}"]' + data_source_tool_link: 'a[href$$="tool_runner/data_source_redirect?tool_id=${tool_id}"]' search: '.search-query' workflow_names: '#internal-workflows .toolTitle' views_button: '.tool-panel-dropdown' diff --git a/lib/galaxy_test/selenium/test_data_source_tools.py b/lib/galaxy_test/selenium/test_data_source_tools.py new file mode 100644 index 00000000000..f537c6e20ae --- /dev/null +++ b/lib/galaxy_test/selenium/test_data_source_tools.py @@ -0,0 +1,29 @@ +from galaxy_test.base.populators import skip_if_site_down +from .framework import ( + managed_history, + selenium_test, + SeleniumTestCase, + UsesHistoryItemAssertions, +) + + +class DataSourceTestCase(SeleniumTestCase, UsesHistoryItemAssertions): + + ensure_registered = True + + @selenium_test + @managed_history + @skip_if_site_down("https://genome.ucsc.edu/cgi-bin/hgTables") + def test_ucsc_table_direct1_data_source(self): + self.home() + self.datasource_tool_open("ucsc_table_direct1") + self.screenshot("ucsc_table_browser_first_page") + checkbox = self.wait_for_selector("#checkboxGalaxy") + assert checkbox.get_attribute("checked") == "true" + submit_button = self.wait_for_selector("#hgta_doTopSubmit") + submit_button.click() + self.screenshot("ucsc_table_browser_second_page") + self.wait_for_selector("#hgta_doGalaxyQuery").click() + self.history_panel_wait_for_hid_ok(1) + # Make sure we're still logged in (xref https://github.com/galaxyproject/galaxy/issues/11374) + self.components.masthead.logged_in_only.wait_for_visible() diff --git a/test/functional/tools/samples_tool_conf.xml b/test/functional/tools/samples_tool_conf.xml index cb29384d5c4..8eeb4238f5e 100644 --- a/test/functional/tools/samples_tool_conf.xml +++ b/test/functional/tools/samples_tool_conf.xml @@ -8,6 +8,7 @@ + diff --git a/test/functional/tools/ucsc_tablebrowser.xml b/test/functional/tools/ucsc_tablebrowser.xml new file mode 120000 index 00000000000..ef273b19283 --- /dev/null +++ b/test/functional/tools/ucsc_tablebrowser.xml @@ -0,0 +1 @@ +../../../lib/galaxy/tools/bundled/data_source/ucsc_tablebrowser.xml \ No newline at end of file From 6679571bdd1e07dd1a8d660adf8988d428cefad6 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Mon, 13 Jun 2022 15:49:37 +0200 Subject: [PATCH 4/4] Improve tool_runner path construction That'll work without galaxy_url_prefix and seems a little more robust. --- lib/galaxy/webapps/base/webapp.py | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/lib/galaxy/webapps/base/webapp.py b/lib/galaxy/webapps/base/webapp.py index e25a017316c..4835a5cd892 100644 --- a/lib/galaxy/webapps/base/webapp.py +++ b/lib/galaxy/webapps/base/webapp.py @@ -13,10 +13,7 @@ from typing import ( Any, Dict, ) -from urllib.parse import ( - urljoin, - urlparse, -) +from urllib.parse import urlparse import mako.lookup import mako.runtime @@ -390,7 +387,7 @@ class GalaxyWebTransaction(base.DefaultWebTransaction, context.ProvidesHistoryCo if name not in self.request.cookies and TOOL_RUNNER_SESSION_COOKIE in self.request.cookies: # TOOL_RUNNER_SESSION_COOKIE value is the encoded galaxysession cookie. # We decode it here and pretend it's the galaxysession - tool_runner_path = urljoin(self.app.config.galaxy_url_prefix, "tool_runner") + tool_runner_path = url_for(controller="tool_runner") if self.request.path.startswith(tool_runner_path): return self.security.decode_guid(self.request.cookies[TOOL_RUNNER_SESSION_COOKIE].value) return self.request.cookies[name].value @@ -406,13 +403,12 @@ class GalaxyWebTransaction(base.DefaultWebTransaction, context.ProvidesHistoryCo self._set_cookie( value, name=TOOL_RUNNER_SESSION_COOKIE, - path=urljoin(path, "tool_runner"), + path=url_for(controller="tool_runner"), age=age, version=version, encode_value=True, ) tool_runner_cookie = self.response.cookies[TOOL_RUNNER_SESSION_COOKIE] - tool_runner_cookie["path"] = urljoin(path, "tool_runner") tool_runner_cookie["SameSite"] = "None" tool_runner_cookie["secure"] = True