From 8bacd4da4fa6de578b818aa7a4b36bbeaaa243d7 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 01 2021 10:34:40 +0000 Subject: [PATCH 1/3] Warn users when a PR contains some characters Unicode bi-directional characters can be present but unseen and thus missed during the review. With this PR, we create a list of characters that we want to warn the users about if present in a PR. Since that list is configurable, it can be extended as needed/desired. This relates to the CVE: CVE-2021-42574 and CVE-2021-42694 More can be found about it at: https://access.redhat.com/security/vulnerabilities/RHSB-2021-007 Signed-off-by: Pierre-Yves Chibon --- diff --git a/doc/configuration.rst b/doc/configuration.rst index 44bec54..8f74ebc 100644 --- a/doc/configuration.rst +++ b/doc/configuration.rst @@ -1931,6 +1931,20 @@ used. Defaults to: ``None`` (which results in the default branch being ``master``). +PR_WARN_CHARACTERS +~~~~~~~~~~~~~~~~~~ + +List of characters that triggers a warning to the users when met in a commit of +a pull-request (each commit being made checked). + +Defaults to: +:: + + set([ + chr(0x202a), chr(0x202b), chr(0x202c), chr(0x202d), chr(0x202e), + chr(0x2066), chr(0x2067), chr(0x2068), chr(0x2069) + ]) + RepoSpanner Options ------------------- diff --git a/pagure/default_config.py b/pagure/default_config.py index ac6c812..4aa1051 100644 --- a/pagure/default_config.py +++ b/pagure/default_config.py @@ -643,3 +643,17 @@ CSP_HEADERS = ( "base-uri 'self';" "img-src 'self' https:;" ) + +PR_WARN_CHARACTERS = set( + [ + chr(0x202A), + chr(0x202B), + chr(0x202C), + chr(0x202D), + chr(0x202E), + chr(0x2066), + chr(0x2067), + chr(0x2068), + chr(0x2069), + ] +) diff --git a/pagure/lib/query.py b/pagure/lib/query.py index 76bc763..6047b78 100644 --- a/pagure/lib/query.py +++ b/pagure/lib/query.py @@ -6216,3 +6216,26 @@ def update_ticket_board_status( user=user, notification=True, ) + + +def find_warning_characters(repo_obj, commits): + """Return whether the given list of commits of the specified repository + object contains forbidden characters or not. + """ + warn_characters = pagure_config["PR_WARN_CHARACTERS"] + + for commit in commits: + if commit.parents: + diff = repo_obj.diff(commit.parents[0], commit) + else: + # First commit in the repo + diff = commit.tree.diff_to_tree(swap=True) + + for patch in diff: + for hunk in patch.hunks: + for line in hunk.lines: + subset = [c for c in line.content if c in warn_characters] + if subset: + return True + + return False diff --git a/pagure/templates/repo_pull_request.html b/pagure/templates/repo_pull_request.html index 8e53c2d..9fcd1e5 100644 --- a/pagure/templates/repo_pull_request.html +++ b/pagure/templates/repo_pull_request.html @@ -132,7 +132,8 @@ -
+
+
{% if pull_request.status == 'Open' %} {% if g.authenticated and (g.fas_user.username == pull_request.user.username or g.repo_committer) %} @@ -161,7 +162,7 @@ {% endif %} -
+
@@ -236,7 +237,19 @@ {% endif %} +
+ {% if warning_characters %} + + {% endif %}
+
diff --git a/pagure/ui/fork.py b/pagure/ui/fork.py index e4613d2..0ba221f 100644 --- a/pagure/ui/fork.py +++ b/pagure/ui/fork.py @@ -320,6 +320,10 @@ def request_pull(repo, requestid, username=None, namespace=None): if diff: diff.find_similar() + warning_characters = pagure.lib.query.find_warning_characters( + repo_obj, diff_commits + ) + form = pagure.forms.MergePRForm() trigger_ci_pr_form = pagure.forms.TriggerCIPRForm() @@ -369,6 +373,7 @@ def request_pull(repo, requestid, username=None, namespace=None): trigger_ci=trigger_ci, trigger_ci_pr_form=trigger_ci_pr_form, flag_statuses_labels=json.dumps(pagure_config["FLAG_STATUSES_LABELS"]), + warning_characters=warning_characters, ) From de6434c2f0134b5ee8789f647cf762b4ed9dc9be Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 01 2021 10:34:40 +0000 Subject: [PATCH 2/3] Test the behavior of the PR page with bidi characters This PR adds unit-tests checking the behavior of the PR page with it finds bidi characters. Signed-off-by: Pierre-Yves Chibon --- diff --git a/tests/__init__.py b/tests/__init__.py index 17f1e8e..28d33c1 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -1150,6 +1150,7 @@ def add_pull_request_git_repo( branch_from="feature", user="pingou", allow_rebase=False, + append_content=None, ): """Set up the git repo and create the corresponding PullRequest object. @@ -1198,8 +1199,11 @@ def add_pull_request_git_repo( remote.fetch() # Edit the sources file again + content = "foo\n bar\nbaz\n boose" + if append_content: + content = content + append_content with open(os.path.join(new_gitrepo, "sources"), "w") as stream: - stream.write("foo\n bar\nbaz\n boose") + stream.write(content) clone_repo.index.add("sources") clone_repo.index.write() diff --git a/tests/test_pagure_flask_ui_pr_bidi.py b/tests/test_pagure_flask_ui_pr_bidi.py new file mode 100644 index 0000000..6ebdc91 --- /dev/null +++ b/tests/test_pagure_flask_ui_pr_bidi.py @@ -0,0 +1,132 @@ +# -*- coding: utf-8 -*- + +""" + (c) 2021 - Copyright Red Hat Inc + + Authors: + Pierre-Yves Chibon + +""" + +from __future__ import unicode_literals, absolute_import + +import unittest +import sys +import os + +import pygit2 +from mock import patch, MagicMock + +sys.path.insert( + 0, os.path.join(os.path.dirname(os.path.abspath(__file__)), "..") +) + +import pagure.lib.query +import tests + + +class PagureFlaskPrBiditests(tests.Modeltests): + """ Tests PR in pagure when the PR has bi-directional characters """ + + maxDiff = None + + @patch("pagure.lib.notify.send_email", MagicMock(return_value=True)) + @patch("pagure.lib.notify.fedmsg_publish", MagicMock(return_value=True)) + def setUp(self): + """ Set up the environnment, ran before every tests. """ + super(PagureFlaskPrBiditests, self).setUp() + + tests.create_projects(self.session) + tests.create_projects_git(os.path.join(self.path, "repos"), bare=True) + + # Create foo's fork of pingou's test project + item = pagure.lib.model.Project( + user_id=2, # foo + name="test", + description="test project #1", + hook_token="aaabbb", + is_fork=True, + parent_id=1, + ) + self.session.add(item) + self.session.commit() + # Create the fork's git repo + repo_path = os.path.join(self.path, "repos", item.path) + pygit2.init_repository(repo_path, bare=True) + + def set_up_git_repo( + self, repo, fork, branch_from="feature", append_content=None + ): + """Set up the git repo and create the corresponding PullRequest + object. + """ + + req = tests.add_pull_request_git_repo( + self.path, + self.session, + repo, + fork, + branch_from, + append_content=append_content, + ) + + self.assertEqual(req.id, 1) + self.assertEqual(req.title, "PR from the %s branch" % branch_from) + + tests.clean_pull_requests_path() + + def test_accessing_pr_no_bidi(self): + """ Test accessing the PR which has no bidi characters. """ + project = pagure.lib.query.get_authorized_project(self.session, "test") + fork = pagure.lib.query.get_authorized_project( + self.session, "test", user="foo" + ) + self.set_up_git_repo(repo=project, fork=fork) + + # Ensure things got setup straight + project = pagure.lib.query.get_authorized_project(self.session, "test") + self.assertEqual(len(project.requests), 1) + + # wait for the worker to process the task + path = os.path.join( + self.path, "repos", "test.git", "refs", "pull", "1", "head" + ) + self.assertTrue(os.path.exists(path)) + + # View the pull-request -- no bidi characters found + output = self.app.get("/test/pull-request/1") + self.assertEqual(output.status_code, 200) + self.assertNotIn( + "Special characters such as:", output.get_data(as_text=True) + ) + + def test_accessing_pr_bidi(self): + """ Test accessing the PR which has no bidi characters. """ + project = pagure.lib.query.get_authorized_project(self.session, "test") + fork = pagure.lib.query.get_authorized_project( + self.session, "test", user="foo" + ) + self.set_up_git_repo( + repo=project, fork=fork, append_content="ahah %s" % chr(0x2067) + ) + + # Ensure things got setup straight + project = pagure.lib.query.get_authorized_project(self.session, "test") + self.assertEqual(len(project.requests), 1) + + # wait for the worker to process the task + path = os.path.join( + self.path, "repos", "test.git", "refs", "pull", "1", "head" + ) + self.assertTrue(os.path.exists(path)) + + # View the pull-request -- bidi characters found + output = self.app.get("/test/pull-request/1") + self.assertEqual(output.status_code, 200) + self.assertIn( + "Special characters such as:", output.get_data(as_text=True) + ) + + +if __name__ == "__main__": + unittest.main(verbosity=2) From 0c035db55721c5bb0d9fd3604b9cd8ef29454eb2 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 01 2021 10:34:40 +0000 Subject: [PATCH 3/3] Rework the string formatting It seems to misbehave with python 3.9 or something changed its behavior. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/model_base.py b/pagure/lib/model_base.py index 170d615..38417b2 100644 --- a/pagure/lib/model_base.py +++ b/pagure/lib/model_base.py @@ -48,7 +48,7 @@ def create_session(db_url=None, debug=False, pool_recycle=3600): global SESSIONMAKER if SESSIONMAKER is None or ( - db_url and db_url != ("%s" % SESSIONMAKER.kw["bind"].engine.url) + db_url and db_url != ("{}".format(SESSIONMAKER.kw["bind"].engine.url)) ): if db_url is None: raise ValueError("First call to create_session needs db_url")