#5239 Warn users about the presence of bi-directionnal unicode characters
Merged 2 years ago by pingou. Opened 2 years ago by pingou.

file modified
+14
@@ -1931,6 +1931,20 @@ 

  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

  -------------------

file modified
+14
@@ -643,3 +643,17 @@ 

      "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),

+     ]

+ )

file modified
+1 -1
@@ -48,7 +48,7 @@ 

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

file modified
+23
@@ -6216,3 +6216,26 @@ 

              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

@@ -132,7 +132,8 @@ 

  

  

        </h4>

-     <div class="ml-auto d-flex">

+     <div class="ml-auto d-flex flex-column">

+       <div class="d-flex justify-content-end">

          {% 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 @@ 

                </button>

              </form>

            {% endif %}

-           <div class="dropdown float-right ml-1">

+           <div class="dropdown inline ml-1">

              <span class="dropdown dropdown-btn">

                <a href="#" id="merge_dropdown_btn"

                    class="btn btn-outline-secondary btn-sm disabled dropdown-toggle" data-toggle="dropdown">
@@ -236,7 +237,19 @@ 

              </button>

            </form>

         {% endif %}

+       </div>

+       {% if warning_characters %}

+       <div>

+           <span class="badge badge-danger font-size-09">warning</span>

+         <small>

+           Special characters such as: <a

+               href="https://en.wikipedia.org/wiki/Bidirectional_text">Bidirectional

+               characters</a>, found in this PR.

+         </small>

+       </div>

+       {% endif %}

      </div>

+ 

    </div>

  

  

file modified
+5
@@ -320,6 +320,10 @@ 

      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 @@ 

          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,

      )

  

  

file modified
+5 -1
@@ -1150,6 +1150,7 @@ 

      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 @@ 

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

  

@@ -0,0 +1,132 @@ 

+ # -*- coding: utf-8 -*-

+ 

+ """

+  (c) 2021 - Copyright Red Hat Inc

+ 

+  Authors:

+    Pierre-Yves Chibon <pingou@pingoured.fr>

+ 

+ """

+ 

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

no initial comment

rebased onto 8bacd4d

2 years ago

Reviewed with @pingou changes make sense to me

fedora-rpms-py3 is passing, it fails on fedora-pip-py3 but this is unrelated to this PR, so I'm going to merge this :)

Thanks for the review @jrichardson !

Pull-Request has been merged by pingou

2 years ago