From 623a3b81ca71d585f2e7c2a7f0372b6382be3a89 Mon Sep 17 00:00:00 2001 From: Martin Cech Date: Wed, 27 Mar 2019 14:04:33 -0400 Subject: [PATCH 1/4] check for validity of user-given changesets --- .../tool_shed/controllers/repository.py | 24 +++++++++++++------ 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/lib/galaxy/webapps/tool_shed/controllers/repository.py b/lib/galaxy/webapps/tool_shed/controllers/repository.py index 9a9ca34a3e4..7398ff30f9b 100644 --- a/lib/galaxy/webapps/tool_shed/controllers/repository.py +++ b/lib/galaxy/webapps/tool_shed/controllers/repository.py @@ -1721,6 +1721,7 @@ class RepositoryController(BaseUIController, ratings_util.ItemRatings): user_id = kwd.get('user_id', None) repository_id = kwd.get('repository_id', None) changeset_revision = kwd.get('changeset_revision', None) + self.validate_changeset_revision(trans, changeset_revision, repository_id) return trans.fill_template('/webapps/tool_shed/index.mako', repository_metadata=repository_metadata, can_administer_repositories=can_administer_repositories, @@ -2167,6 +2168,7 @@ class RepositoryController(BaseUIController, ratings_util.ItemRatings): status = kwd.get('status', 'done') repository = repository_util.get_repository_in_tool_shed(trans.app, repository_id) changeset_revision = kwd.get('changeset_revision', repository.tip(trans.app)) + self.validate_changeset_revision(trans, changeset_revision, repository_id) repository_metadata = metadata_util.get_repository_metadata_by_changeset_revision(trans.app, repository_id, changeset_revision) if repository_metadata: repository_metadata_id = trans.security.encode_id(repository_metadata.id), @@ -2801,13 +2803,7 @@ class RepositoryController(BaseUIController, ratings_util.ItemRatings): repo = hg_util.get_repo_for_repository(trans.app, repository=repository) avg_rating, num_ratings = self.get_ave_item_rating_data(trans.sa_session, repository, webapp_model=trans.model) changeset_revision = kwd.get('changeset_revision', repository.tip(trans.app)) - if not hg_util.get_changectx_for_changeset(repo, changeset_revision): - message = 'Invalid changeset revision' - return trans.response.send_redirect(web.url_for(controller='repository', - action='index', - repository_id=id, - message=message, - status='error')) + self.validate_changeset_revision(trans, changeset_revision, id) repository.share_url = repository_util.generate_sharable_link_for_repository_in_tool_shed(repository, changeset_revision=changeset_revision) repository.clone_url = common_util.generate_clone_url_for_repository_in_tool_shed(trans.user, repository) display_reviews = kwd.get('display_reviews', False) @@ -2902,6 +2898,7 @@ class RepositoryController(BaseUIController, ratings_util.ItemRatings): tool_lineage = [] tool = None guid = None + self.validate_changeset_revision(trans, changeset_revision, repository_id) revision_label = hg_util.get_revision_label(trans.app, repository, changeset_revision, include_date=False) repository_metadata = metadata_util.get_repository_metadata_by_changeset_revision(trans.app, repository_id, changeset_revision) if repository_metadata: @@ -2989,3 +2986,16 @@ class RepositoryController(BaseUIController, ratings_util.ItemRatings): metadata=metadata, message=message, status=status) + + def validate_changeset_revision(self, trans, changeset_revision, repository_id): + """In case changeset revision is invalid send them to the repository page""" + if changeset_revision: + repository = repository_util.get_repository_in_tool_shed(trans.app, repository_id) + repo = hg_util.get_repo_for_repository(trans.app, repository=repository) + if not hg_util.get_changectx_for_changeset(repo, changeset_revision): + message = 'Invalid changeset revision' + return trans.response.send_redirect(web.url_for(controller='repository', + action='index', + repository_id=repository_id, + message=message, + status='error')) From 76af203de8452407ef1f2d52c88e7dd672210ce4 Mon Sep 17 00:00:00 2001 From: Nicola Soranzo Date: Fri, 29 Mar 2019 03:02:51 +0000 Subject: [PATCH 2/4] Encode content before writing to temp file --- test/shed_functional/base/twilltestcase.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/test/shed_functional/base/twilltestcase.py b/test/shed_functional/base/twilltestcase.py index 7080af5df4a..bf66965d225 100644 --- a/test/shed_functional/base/twilltestcase.py +++ b/test/shed_functional/base/twilltestcase.py @@ -29,7 +29,7 @@ import galaxy.model.tool_shed_install as galaxy_model import galaxy.util import galaxy.webapps.tool_shed.util.hgweb_config from base.testcase import FunctionalTestCase # noqa: I100,I201,I202 -from galaxy.util import unicodify # noqa: I201 +from galaxy.util import smart_str, unicodify # noqa: I201 from galaxy.web import security # noqa: I201 from tool_shed.util import hg_util, xml_util from tool_shed.util.encoding_util import tool_shed_encode @@ -465,11 +465,9 @@ class ShedTwillTestCase(FunctionalTestCase): return self.wait_for(lambda: self.get_running_datasets(), **kwds) def write_temp_file(self, content, suffix='.html'): - fd, fname = tempfile.mkstemp(suffix=suffix, prefix='twilltestcase-') - f = os.fdopen(fd, "w") - f.write(content) - f.close() - return fname + with tempfile.NamedTemporaryFile(suffix=suffix, prefix='twilltestcase-', delete=False) as fh: + fh.write(smart_str(content)) + return fh.name def add_repository_review_component(self, **kwd): params = { From 6230d93f6acef2d2728c22bbb8302d380ce7b2d9 Mon Sep 17 00:00:00 2001 From: Nicola Soranzo Date: Fri, 29 Mar 2019 13:18:33 +0000 Subject: [PATCH 3/4] Use bleach to clean msg which will not strip safe HTML tags and attributes, which we use. Also, restrict the possible values of `status` instead of trying to escape it. --- templates/message.mako | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/templates/message.mako b/templates/message.mako index a47b02c4741..02efb55ed3b 100644 --- a/templates/message.mako +++ b/templates/message.mako @@ -1,4 +1,6 @@ <%! + import bleach + def inherit(context): if context.get('use_panels'): if context.get('webapp'): @@ -56,7 +58,14 @@ ## Render a message <%def name="render_msg( msg, status='done' )"> -
${_(msg)}
-
+ <% + if status == "done": + status = "success" + elif status == "error": + status = "danger" + if status not in ("danger", "info", "success", "warning"): + status = "info" + %> +
${_(bleach.clean(msg))}
From a07c99d5901d7bad1939dfc9ab8cf0b4ab123409 Mon Sep 17 00:00:00 2001 From: Martin Cech Date: Fri, 29 Mar 2019 16:40:35 -0400 Subject: [PATCH 4/4] prevent XSS on old style-mako and expand to the messageLarge method --- templates/message.mako | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/templates/message.mako b/templates/message.mako index 02efb55ed3b..8c980e21f05 100644 --- a/templates/message.mako +++ b/templates/message.mako @@ -53,19 +53,23 @@ ## Render large message. <%def name="render_large_message( message, status )"> -
${_(message)}
+ <% + if status not in ("done", "info", "error", "warning"): + status = "infomessagelarge" + else: + status = status + "messagelarge" + %> +
${_(bleach.clean(message))}
## Render a message <%def name="render_msg( msg, status='done' )"> <% - if status == "done": - status = "success" - elif status == "error": - status = "danger" - if status not in ("danger", "info", "success", "warning"): - status = "info" + if status not in ("done", "info", "error", "warning"): + status = "infomessage" + else: + status = status + "message" %> -
${_(bleach.clean(msg))}
+
${_(bleach.clean(msg))}