Attachment 343131 Details for Bug 186831 – Patch

[patch] Patch

bug-186831-20180619212122.patch (text/plain), 5.33 KB, created by Daniel Bates on 2018-06-19 21:21:22 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Daniel Bates

Created: 2018-06-19 21:21:22 PDT

Size: 5.33 KB

patch

obsolete

>Subversion Revision: 232980
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 10eb66e3b0e73269315832cff5e6d140aab119b6..c2e44830e56ee13089927e86d21217d246b989eb 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,5 +1,31 @@
> 2018-06-19  Daniel Bates  <dabates@apple.com>
> 
>+        EWS should not try to post comments or upload result archives to security-sensitive
>+        bugs unless it has access
>+        https://bugs.webkit.org/show_bug.cgi?id=186831
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Following r232979 security-sensitive patches are uploaded to the status server so
>+        that they can be retrieved and processed by EWS bots without the need for Bugzilla
>+        security bug access. Although the EWS machinery is robust against unexpected exceptions,
>+        including exceptions raised when interacting with Bugzilla bugs/attachments with
>+        insufficient credentials, we should not depend on such defenses as they cause webkit-
>+        patch to log a message for the "unexpected" exception. We should reserve such logging
>+        for truly unexpected exceptions that indicate a programming mistake that we need to fix.
>+
>+        * Scripts/webkitpy/tool/commands/earlywarningsystem.py:
>+        (AbstractEarlyWarningSystem._post_reject_message_on_bug): Bail out early if we cannot
>+        access the bug.
>+        * Scripts/webkitpy/tool/commands/queues.py:
>+        (PatchProcessingQueue._can_access_bug): Added.
>+        (PatchProcessingQueue._upload_results_archive_for_patch): Only add an attachment if we
>+        can access the bug.
>+        (CommitQueue.process_work_item): Only post a rejection comment (i.e. call CommitterValidatorreject_patch_from_commit_queue())
>+        if we can access the bug.
>+
>+2018-06-19  Daniel Bates  <dabates@apple.com>
>+
>         Implement EWS real-time patch status updates and tail logging
>         https://bugs.webkit.org/show_bug.cgi?id=186823
> 
>diff --git a/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py b/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py
>index 7a2007368e40f21f290df1b8c9b6b8f7b61853df..cd2d8fbc833f5cb7f804c63983ccfb60dbc2726a 100644
>--- a/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py
>+++ b/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py
>@@ -89,6 +89,8 @@ class AbstractEarlyWarningSystem(AbstractReviewQueue, EarlyWarningSystemTaskDele
>             message += "\n\n%s" % extra_message_text
>         # FIXME: We might want to add some text about rejecting from the commit-queue in
>         # the case where patch.commit_queue() isn't already set to '-'.
>+        if not self._can_access_bug(patch.bug_id()):
>+            return
>         if self.watchers:
>             tool.bugs.add_cc_to_bug(patch.bug_id(), self.watchers)
>         tool.bugs.set_flag_on_attachment(patch.id(), "commit-queue", "-", message)
>diff --git a/Tools/Scripts/webkitpy/tool/commands/queues.py b/Tools/Scripts/webkitpy/tool/commands/queues.py
>index 80d8c8455d9cc681d9ee8c21fb80da8d74d3e71b..510457cbbfd966657eb1bdf8e8c6346586470385 100644
>--- a/Tools/Scripts/webkitpy/tool/commands/queues.py
>+++ b/Tools/Scripts/webkitpy/tool/commands/queues.py
>@@ -304,6 +304,13 @@ class PatchProcessingQueue(AbstractPatchQueue):
> 
>         self._create_port()
> 
>+    # FIXME: Bugzilla member functions should perform this check as they can do it as part of the same
>+    # network request. This would also avoid bugs where the access of the Bugzilla bug changes between
>+    # the time-of-check (calling this function) and time-of-use (when we ask Bugzilla to perform the
>+    # actual operation).
>+    def _can_access_bug(self, bug_id):
>+        return bool(self._tool.bugs.fetch_bug(bug_id))
>+
>     def _upload_results_archive_for_patch(self, patch, results_archive_zip):
>         if not self._port:
>             self._create_port()
>@@ -321,7 +328,8 @@ class PatchProcessingQueue(AbstractPatchQueue):
>         # FIXME: We could easily list the test failures from the archive here,
>         # currently callers do that separately.
>         comment_text += BotInfo(self._tool, self._port.name()).summary_text()
>-        self._tool.bugs.add_attachment_to_bug(patch.bug_id(), results_archive_file, description, filename="layout-test-results.zip", comment_text=comment_text)
>+        if self._can_access_bug(patch.bug_id()):
>+            self._tool.bugs.add_attachment_to_bug(patch.bug_id(), results_archive_file, description, filename="layout-test-results.zip", comment_text=comment_text)
> 
> 
> class CommitQueue(PatchProcessingQueue, StepSequenceErrorHandler, CommitQueueTaskDelegate):
>@@ -355,8 +363,9 @@ class CommitQueue(PatchProcessingQueue, StepSequenceErrorHandler, CommitQueueTas
>             self._did_error(patch, "%s did not process patch. Reason: %s" % (self.name, error.failure_message))
>             return False
>         except ScriptError as e:
>-            validator = CommitterValidator(self._tool)
>-            validator.reject_patch_from_commit_queue(patch.id(), self._error_message_for_bug(task, patch, e))
>+            if self._can_access_bug(patch.bug_id()):
>+                validator = CommitterValidator(self._tool)
>+                validator.reject_patch_from_commit_queue(patch.id(), self._error_message_for_bug(task, patch, e))
>             results_archive = task.results_archive_from_patch_test_run(patch)
>             if results_archive:
>                 self._upload_results_archive_for_patch(patch, results_archive)

Actions: View | Formatted Diff | Diff

Attachments on bug 186831: 343131