Attachment #343131 for bug #186831

View Details Raw Unified Return to Bug 186831



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


Lines 89-94 class AbstractEarlyWarningSystem(AbstractReviewQueue, EarlyWarningSystemTaskDele a/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py_sec1
- a/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py +2 lines
89	message += "\n\n%s" % extra_message_text	89	message += "\n\n%s" % extra_message_text
90	# FIXME: We might want to add some text about rejecting from the commit-queue in	90	# FIXME: We might want to add some text about rejecting from the commit-queue in
91	# the case where patch.commit_queue() isn't already set to '-'.	91	# the case where patch.commit_queue() isn't already set to '-'.
		92	if not self._can_access_bug(patch.bug_id()):
		93	return
92	if self.watchers:	94	if self.watchers:
93	tool.bugs.add_cc_to_bug(patch.bug_id(), self.watchers)	95	tool.bugs.add_cc_to_bug(patch.bug_id(), self.watchers)
94	tool.bugs.set_flag_on_attachment(patch.id(), "commit-queue", "-", message)	96	tool.bugs.set_flag_on_attachment(patch.id(), "commit-queue", "-", message)

Lines 304-309 class PatchProcessingQueue(AbstractPatchQueue): a/Tools/Scripts/webkitpy/tool/commands/queues.py_sec1
- a/Tools/Scripts/webkitpy/tool/commands/queues.py -3 / +12 lines
304		304
305	self._create_port()	305	self._create_port()
306		306
		307	# FIXME: Bugzilla member functions should perform this check as they can do it as part of the same
		308	# network request. This would also avoid bugs where the access of the Bugzilla bug changes between
		309	# the time-of-check (calling this function) and time-of-use (when we ask Bugzilla to perform the
		310	# actual operation).
		311	def _can_access_bug(self, bug_id):
		312	return bool(self._tool.bugs.fetch_bug(bug_id))
		313
307	def _upload_results_archive_for_patch(self, patch, results_archive_zip):	314	def _upload_results_archive_for_patch(self, patch, results_archive_zip):
308	if not self._port:	315	if not self._port:
309	self._create_port()	316	self._create_port()
Lines 321-327 class PatchProcessingQueue(AbstractPatchQueue): a/Tools/Scripts/webkitpy/tool/commands/queues.py_sec2
321	# FIXME: We could easily list the test failures from the archive here,	328	# FIXME: We could easily list the test failures from the archive here,
322	# currently callers do that separately.	329	# currently callers do that separately.
323	comment_text += BotInfo(self._tool, self._port.name()).summary_text()	330	comment_text += BotInfo(self._tool, self._port.name()).summary_text()
324	self._tool.bugs.add_attachment_to_bug(patch.bug_id(), results_archive_file, description, filename="layout-test-results.zip", comment_text=comment_text)	331	if self._can_access_bug(patch.bug_id()):
		332	self._tool.bugs.add_attachment_to_bug(patch.bug_id(), results_archive_file, description, filename="layout-test-results.zip", comment_text=comment_text)
325		333
326		334
327	class CommitQueue(PatchProcessingQueue, StepSequenceErrorHandler, CommitQueueTaskDelegate):	335	class CommitQueue(PatchProcessingQueue, StepSequenceErrorHandler, CommitQueueTaskDelegate):
Lines 355-362 class CommitQueue(PatchProcessingQueue, StepSequenceErrorHandler, CommitQueueTas a/Tools/Scripts/webkitpy/tool/commands/queues.py_sec3
355	self._did_error(patch, "%s did not process patch. Reason: %s" % (self.name, error.failure_message))	363	self._did_error(patch, "%s did not process patch. Reason: %s" % (self.name, error.failure_message))
356	return False	364	return False
357	except ScriptError as e:	365	except ScriptError as e:
358	validator = CommitterValidator(self._tool)	366	if self._can_access_bug(patch.bug_id()):
359	validator.reject_patch_from_commit_queue(patch.id(), self._error_message_for_bug(task, patch, e))	367	validator = CommitterValidator(self._tool)
		368	validator.reject_patch_from_commit_queue(patch.id(), self._error_message_for_bug(task, patch, e))
360	results_archive = task.results_archive_from_patch_test_run(patch)	369	results_archive = task.results_archive_from_patch_test_run(patch)
361	if results_archive:	370	if results_archive:
362	self._upload_results_archive_for_patch(patch, results_archive)	371	self._upload_results_archive_for_patch(patch, results_archive)

Return to Bug 186831

Lines 1-5 a/Tools/ChangeLog_sec1
- a/Tools/ChangeLog +26 lines
1	2018-06-19 Daniel Bates <dabates@apple.com>	1	2018-06-19 Daniel Bates <dabates@apple.com>
2		2
		3	EWS should not try to post comments or upload result archives to security-sensitive
		4	bugs unless it has access
		5	https://bugs.webkit.org/show_bug.cgi?id=186831
		6
		7	Reviewed by NOBODY (OOPS!).
		8
		9	Following r232979 security-sensitive patches are uploaded to the status server so
		10	that they can be retrieved and processed by EWS bots without the need for Bugzilla
		11	security bug access. Although the EWS machinery is robust against unexpected exceptions,
		12	including exceptions raised when interacting with Bugzilla bugs/attachments with
		13	insufficient credentials, we should not depend on such defenses as they cause webkit-
		14	patch to log a message for the "unexpected" exception. We should reserve such logging
		15	for truly unexpected exceptions that indicate a programming mistake that we need to fix.
		16
		17	* Scripts/webkitpy/tool/commands/earlywarningsystem.py:
		18	(AbstractEarlyWarningSystem._post_reject_message_on_bug): Bail out early if we cannot
		19	access the bug.
		20	* Scripts/webkitpy/tool/commands/queues.py:
		21	(PatchProcessingQueue._can_access_bug): Added.
		22	(PatchProcessingQueue._upload_results_archive_for_patch): Only add an attachment if we
		23	can access the bug.
		24	(CommitQueue.process_work_item): Only post a rejection comment (i.e. call CommitterValidatorreject_patch_from_commit_queue())
		25	if we can access the bug.
		26
		27	2018-06-19 Daniel Bates <dabates@apple.com>
		28
3	Implement EWS real-time patch status updates and tail logging	29	Implement EWS real-time patch status updates and tail logging
4	https://bugs.webkit.org/show_bug.cgi?id=186823	30	https://bugs.webkit.org/show_bug.cgi?id=186823
5		31