#4558 Ensure @<username> doesn't over-reach when sending notifications
Merged 4 years ago by pingou. Opened 4 years ago by pingou.

file modified
+2 -2
@@ -34,6 +34,7 @@ 

  import pagure.lib.query

  import pagure.lib.tasks_services

  from pagure.config import config as pagure_config

+ from pagure.pfmarkdown import MENTION_RE

  

  

  _log = logging.getLogger(__name__)
@@ -233,8 +234,7 @@ 

      """ Check the comment to see if an user is mentioned in it and if

      so add this user to the list of people to notify.

      """

-     mentio_re = r"@(\w+)"

-     for username in re.findall(mentio_re, comment):

+     for username in re.findall(MENTION_RE, comment):

          user = pagure.lib.query.search_user(flask.g.session, username=username)

          if user:

              emails.add(user.default_email)

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

  # MENTION_RE regex). Note that it is a zero-length match - it does

  # not capture or consume any of the string - and it does not appear

  # as a group for the match object.

- MENTION_RE = r"(?<!\w)@(\w+)"

+ MENTION_RE = r"(?<![\w\-\"\'\`\$\!\*\+#%&/=^{}|~])@(\w+)"

  # Each line below correspond to a line of the regex:

  #  1) Don't start matching in the middle of a word

  #  2) See if there is a `forks/` at the start

file modified
+3 -3
@@ -1633,7 +1633,7 @@ 

              pagure.cli.admin.do_block_user(args)

  

          output = output.getvalue()

-         self.assertEqual("No users are currently blocked\n", output)

+         self.assertIn("No users are currently blocked\n", output)

  

      @patch("pagure.cli.admin._ask_confirmation", MagicMock(return_value=True))

      def test_list_blocked_user_with_date_and_data(self):
@@ -1653,7 +1653,7 @@ 

              pagure.cli.admin.do_block_user(args)

  

          output = output.getvalue()

-         self.assertEqual(

+         self.assertIn(

              "Users blocked:\n"

              " pingou                -  2050-12-31T00:00:00\n",

              output,
@@ -1667,7 +1667,7 @@ 

              pagure.cli.admin.do_block_user(args)

  

          output = output.getvalue()

-         self.assertEqual("No users are currently blocked\n", output)

+         self.assertIn("No users are currently blocked\n", output)

  

  

  class PagureAdminDeleteProjectTests(tests.Modeltests):

@@ -15,6 +15,7 @@ 

  import os

  

  import mock

+ import munch

  import six

  

  sys.path.insert(
@@ -141,6 +142,120 @@ 

          self.assertEqual(kwargs["user_from"], self.user1.fullname)

  

      @mock.patch("pagure.lib.notify.send_email")

+     def test_user_notified_new_comment(self, fakemail):

+         """Check that users that are @mentionned are notified."""

+         self.comment1.comment = "Hey @foo. Let's do it!"

+         g = munch.Munch()

+         g.session = self.session

+         with mock.patch("flask.g", g):

+             pagure.lib.notify.notify_new_comment(self.comment1)

+ 

+         (_, args, kwargs) = fakemail.mock_calls[0]

+ 

+         # Mail should be sent to both users

+         self.assertIn(

+             args[2],

+             ["bar@pingou.com,foo@bar.com", "foo@bar.com,bar@pingou.com"],

+         )

+ 

+         # Mail ID should be comment #1's mail ID...

+         self.assertEqual(kwargs["mail_id"], self.comment1.mail_id)

+ 

+         # In reply to issue #1's mail ID.

+         self.assertEqual(kwargs["in_reply_to"], self.issue1.mail_id)

+ 

+         # Project name should be...project (full) name.

+         self.assertEqual(kwargs["project_name"], self.project1.fullname)

+ 

+         # Mail should be from user1 (who wrote the comment).

+         self.assertEqual(kwargs["user_from"], self.user1.fullname)

+ 

+     @mock.patch("pagure.lib.notify.send_email")

+     def test_user_notified_new_comment_start_row(self, fakemail):

+         """Check that users that are @mentionned are notified."""

+         self.comment1.comment = "@foo, okidoki"

+         g = munch.Munch()

+         g.session = self.session

+         with mock.patch("flask.g", g):

+             pagure.lib.notify.notify_new_comment(self.comment1)

+ 

+         (_, args, kwargs) = fakemail.mock_calls[0]

+ 

+         # Mail should be sent to both users

+         self.assertIn(

+             args[2],

+             ["bar@pingou.com,foo@bar.com", "foo@bar.com,bar@pingou.com"],

+         )

+ 

+         # Mail ID should be comment #1's mail ID...

+         self.assertEqual(kwargs["mail_id"], self.comment1.mail_id)

+ 

+         # In reply to issue #1's mail ID.

+         self.assertEqual(kwargs["in_reply_to"], self.issue1.mail_id)

+ 

+         # Project name should be...project (full) name.

+         self.assertEqual(kwargs["project_name"], self.project1.fullname)

+ 

+         # Mail should be from user1 (who wrote the comment).

+         self.assertEqual(kwargs["user_from"], self.user1.fullname)

+ 

+     @mock.patch("pagure.lib.notify.send_email")

+     def test_user_notified_new_comment_with_email(self, fakemail):

+         """Ensures that @mention doesn't over-reach."""

+         self.comment1.comment = "So apparently bar@foo.com exists"

+         g = munch.Munch()

+         g.fas_user = tests.FakeUser(username="pingou")

+         g.authenticated = True

+         g.session = self.session

+         with mock.patch("flask.g", g):

+             pagure.lib.notify.notify_new_comment(self.comment1)

+ 

+         (_, args, kwargs) = fakemail.mock_calls[0]

+ 

+         # Mail should be sent to both users

+         self.assertEqual(args[2], "bar@pingou.com")

+ 

+         # Mail ID should be comment #1's mail ID...

+         self.assertEqual(kwargs["mail_id"], self.comment1.mail_id)

+ 

+         # In reply to issue #1's mail ID.

+         self.assertEqual(kwargs["in_reply_to"], self.issue1.mail_id)

+ 

+         # Project name should be...project (full) name.

+         self.assertEqual(kwargs["project_name"], self.project1.fullname)

+ 

+         # Mail should be from user1 (who wrote the comment).

+         self.assertEqual(kwargs["user_from"], self.user1.fullname)

+ 

+     @mock.patch("pagure.lib.notify.send_email")

+     def test_user_notified_new_comment_with_email_with_number(self, fakemail):

+         """Ensures that @mention doesn't over-reach."""

+         self.comment1.comment = "So apparently bar123@foo.com exists"

+         g = munch.Munch()

+         g.fas_user = tests.FakeUser(username="pingou")

+         g.authenticated = True

+         g.session = self.session

+         with mock.patch("flask.g", g):

+             pagure.lib.notify.notify_new_comment(self.comment1)

+ 

+         (_, args, kwargs) = fakemail.mock_calls[0]

+ 

+         # Mail should be sent to both users

+         self.assertEqual(args[2], "bar@pingou.com")

+ 

+         # Mail ID should be comment #1's mail ID...

+         self.assertEqual(kwargs["mail_id"], self.comment1.mail_id)

+ 

+         # In reply to issue #1's mail ID.

+         self.assertEqual(kwargs["in_reply_to"], self.issue1.mail_id)

+ 

+         # Project name should be...project (full) name.

+         self.assertEqual(kwargs["project_name"], self.project1.fullname)

+ 

+         # Mail should be from user1 (who wrote the comment).

+         self.assertEqual(kwargs["user_from"], self.user1.fullname)

+ 

+     @mock.patch("pagure.lib.notify.send_email")

      def test_notify_new_issue_namespaced(

          self, fakemail

      ):  # pylint: disable=invalid-name

If someone comments in a ticket or a PR with a text that contains an
email address, for example: foo@bar.com and the domain corresponds to an
existing username, we do not want to notify that user.
(Imagine if an gmail user gets created! :D)

Signed-off-by: Pierre-Yves Chibon pingou@pingoured.fr

rebased onto 55a1d5bd29bdc73e0d00a7703a5349c7c3503cba

4 years ago

rebased onto d77470ec36bfabadad893ac3bb0e77c8d9420140

4 years ago

rebased onto b16a15ba3ad3909d6dd12de3fd67383d94a17b33

4 years ago

rebased onto f42c03404b8d1ce09682b58f5eb16a044111dc5f

4 years ago

rebased onto e9aa55d907d677b7467f1b39d6629d084ba01294

4 years ago

pretty please pagure-ci rebuild

4 years ago

An email can have a number on their last character and \w will not capture it. pingou123@gmail.com will mention to gmail ?

rebased onto 4eb117fc175583ed18ac4d5ffe82615b3b36d782

4 years ago

New test added for bar123@foo.com :)

rebased onto fff28fb1358ea032a02de65988a37e56f86368c1

4 years ago
r"(?<![\w\-\"\'\`\$\!\*\+#%&/=^{}|~])@(\w+)"

This matches RFC5322 but could be too much, I never saw an email address with % or { on it's local part

Let's be safe rather than sorry :)

rebased onto 3ff5842

4 years ago

1 new commit added

  • Fix tests for new arrow version
4 years ago

Pull-Request has been merged by pingou

4 years ago