Bug 186291 – EWS for security bugs

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Daniel Bates]

We should support EWS processing of security bug patches.

[Aakash Jain]

The current plan is to re-architect EWS in https://bugs.webkit.org/show_bug.cgi?id=179506, and then use the new architecture for security EWS.

[Daniel Bates]

Created attachment 341938 [details]
Work-in-progress - First pass

[Daniel Bates]

Created attachment 341939 [details]
Work-in-progress - First pass

[Radar WebKit Bug Importer]

<rdar://problem/40829658>

[Daniel Bates]

Created attachment 342058 [details]
Work-in-progress - Second pass

Need to ensure webkitpy handles fetching bugs and attachments from restricted bugs gracefully when it does not have a privileged Bugzilla account.

[Daniel Bates]

Here is the high level idea: have the status server (http://webit-queues.webkit.org) host patches for security sensitive bugs. More specifically, teach webkit-patch to upload to the status server the contents and metadata of each Bugzilla patch posted to a security sensitive bug. The feeder EWS (responsible for polling Bugzilla for unreviewed patches) will do the same. The EWS queue machines prefer to fetch a patch from Bugzilla. We now teach them to fall back to fetching the patch from
the status server if they failed to fetch it from Bugzilla due to an authorization issue.

This bug represents a first pass at supporting this feature. For now EWS reporting will be limited to compile time failures and any results that are uploaded to the status server. For now, layout test archives for failed tests will not be available (*). We will add such support in a subsequent bug.

(*) Currently these archives are posted directly to Bugzilla by the EWS bots and EWS bot use (or should be using)
An unprivileged Bugzilla account.

[Alexey Proskuryakov]

Thank you for the overview! I have a coiple questions to clarify the design:

1. What makes the EWS feeder trusted enough to have unrestricted access to security bugs? I think that it's better than EWS bots which run unvetted code, but it seems open to privilege escalation.

2. Do the patches need to be protected when stored on the status server?

[Daniel Bates]

(In reply to Alexey Proskuryakov from comment #7)
> Thank you for the overview! I have a coiple questions to clarify the design:
> 
> 1. What makes the EWS feeder trusted enough to have unrestricted access to
> security bugs? 

We will need to audit the feeder EWS code and ensure it can be trusted. Its code is small and should be straightforward to audit. Notice that the feeder EWS only submits attachment ids to the status server and with the proposed patch it will now submit metadata and the patch content as well. That is, the feeder never applies a patch, builds or tests a patch.

> I think that it's better than EWS bots which run unvetted
> code, but it seems open to privilege escalation.
> 

Would it be sufficient to audit the feeder EWS code to alleviate such fears?

> 2. Do the patches need to be protected when stored on the status server?

I do not see the need to protect them because these patches must be visible to all EWS bots and the EWS bots are untrusted. (If an EWS bot is compromised then an attacker would have the patch). So, I do not see much benefit to protect such patches on the status server.

[Alexey Proskuryakov]

> Would it be sufficient to audit the feeder EWS code to alleviate such fears?

Not the privilege escalation part. An attacker that gained access to any committer account will now be able to access all security bugs by landing a change to the feeder.

> So, I do not see much benefit to protect such patches on the status server.

While this is logically accurate, making access much easier could be problematic. Perhaps a wider discussion would be useful (webkit-dev?)

[Daniel Bates]

(In reply to Alexey Proskuryakov from comment #9)
> > Would it be sufficient to audit the feeder EWS code to alleviate such fears?
> 
> Not the privilege escalation part. An attacker that gained access to any
> committer account will now be able to access all security bugs by landing a
> change to the feeder.
>

This threat is not unique to the feeder EWS source. The same threat exists with respect to any source in the tree, including the bugs.webkit.org source.

> > So, I do not see much benefit to protect such patches on the status server.
> 
> While this is logically accurate, making access much easier could be
> problematic. Perhaps a wider discussion would be useful (webkit-dev?)

I will look to bring this up on WebKit-dev.

[Daniel Bates]

(In reply to Daniel Bates from comment #10)
> The same threat exists with respect to any source in the tree, including the bugs.webkit.org source.

What I meant to say is:

The same threat (to gain access to all security bugs) exists if an attacker modifies the bugs.webkit.org source in the tree. (An attacker may not even need to modify the tree to get access to all security bugs if the compromised account’s credentials are the same for the associated Bugzilla account and the Bugzilla account can see all security bugs). There are more mischievous things an attacker can do if they can commit to the repo, including introducing new security bugs.

[Aakash Jain]

> An attacker that gained access to any committer account will now be able to access all security bugs by landing a change to the feeder.
Instead of creating an account with unrestricted access to security bugs, maybe we can explore having feeder queue use an account with limited access to security bugs (e.g.: read-only access to bugs with r? flag and modified within last few hours/days). So that if the account get compromised, attacker doesn't get unrestricted access to all security bugs, but only r? bugs (currently in review, that would anyways land soon).


> While this is logically accurate, making access much easier could be problematic.
I agree with this concern. Even if it is vulnerable to an attack by expert hacker, we should still make it harder. We can use password/token as we discussed.

[Daniel Bates]

(In reply to Aakash Jain from comment #12)
> > An attacker that gained access to any committer account will now be able to access all security bugs by landing a change to the feeder.
> Instead of creating an account with unrestricted access to security bugs,
> maybe we can explore having feeder queue use an account with limited access
> to security bugs (e.g.: read-only access to bugs with r? flag and modified
> within last few hours/days). So that if the account get compromised,
> attacker doesn't get unrestricted access to all security bugs, but only r?
> bugs (currently in review, that would anyways land soon).
> 
> 

I do not recall what version of Bugzilla we are using. From reading the Bugzilla 5.0 documentation, <https://bugzilla.readthedocs.io/en/5.0/administering/groups.html>, Bugzilla does not seem to support the granular account permissions that you are looking for. We may be able to achieve a similar effect by using a Bugzilla hook that either submits the security sensitive patch to the status server directly OR adds the Bugzilla account used by the feeder EWS to the CC list of the security sensitive bug (so that the feeder EWS can see the bug and its attachments assuming the bug allows CC list members to view it). If we do the latter then the feeder EWS would remove itself from the CC list after it submits the attachment identifier, attachment metadata, and attachment contents to the status server.

Additional remarks:

One downside of the former approach is that we need to duplicate functionality in the Bugzilla hook to send the attachment details to the status server (as we still need this logic in webkitpy if we want to support “webkit-patch upload —no-review” - uploads a patch without marking for review and submits it to the EWS - for security sensitive patches.

One downside of the latter approach is that the special account the Bugzilla hook adds to the CC list is never validated (to see that it is controlled by us). We will need to ensure that this special account has a strong password and the account name is either not an email address (e.g. security-EWS; is this possible?) or is an email address with a domain that we (Apple or The WebKit OpenSource Project controls (e.g. WebKit.org). Otherwise, an attacker could re-create the account and run a modified feeder EWS and this activity may not be immediately noticeable.

> > While this is logically accurate, making access much easier could be problematic.
> I agree with this concern. Even if it is vulnerable to an attack by expert
> hacker, we should still make it harder. We can use password/token as we
> discussed.

Using a token(s) sounds reasonable to prevent a crime of opportunity by a “regular joe” and limit access to the security sensitive data on the status server to the EWS queue machines.

[Daniel Bates]

Created attachment 342741 [details]
[Patch] Bugzilla extension

A Bugzilla extension to CC the feeder EWS queue on bugs with unreviewed patches, including bugs that would normally be inaccessible to the feeder EWS (e.g. security bugs).

[Daniel Bates]

Created attachment 342744 [details]
[Patch] Work-in-progress - webkitpy

[Daniel Bates]

Created attachment 342765 [details]
[Patch] Part 1 - webkitpy and QueueStatusServer

[EWS Watchlist]

Attachment 342765 [details] did not pass style-queue:


ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:32:  [UploadAttachment.get] Instance of 'UploadAttachment' has no 'response' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:35:  [UploadAttachment.post] Instance of 'UploadAttachment' has no '_int_from_request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:36:  [UploadAttachment.post] Instance of 'UploadAttachment' has no 'request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:37:  [UploadAttachment.post] Instance of 'UploadAttachment' has no 'request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:39:  [UploadAttachment.post] Instance of 'UploadAttachment' has no 'response' member  [pylint/E1101] [5]
ERROR: Tools/ChangeLog:3:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: security bug  [changelog/unwantedsecurityterms] [3]
ERROR: Tools/QueueStatusServer/handlers/releasepatch.py:72:  [ReleasePatch.post] No value passed for parameter 'attachment_id' in function call  [pylint/E1120] [5]
ERROR: Tools/QueueStatusServer/model/attachmentdata.py:33:  [AttachmentData.add_attachment_data] Class 'AttachmentData' has no 'get_or_insert' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/model/attachmentdata.py:37:  [AttachmentData.lookup_if_exists] Class 'AttachmentData' has no 'get_by_key_name' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/config/authorization.py:27:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: Tools/QueueStatusServer/config/authorization.py:30:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: Tools/QueueStatusServer/config/authorization.py:34:  at least two spaces before inline comment  [pep8/E261] [5]
ERROR: Tools/QueueStatusServer/config/authorization.py:39:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: Tools/QueueStatusServer/config/authorization.py:48:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: Tools/QueueStatusServer/config/authorization.py:59:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachmentcontents.py:30:  [FetchAttachmentContents.get] Instance of 'FetchAttachmentContents' has no 'request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachmentcontents.py:33:  [FetchAttachmentContents.get] Instance of 'FetchAttachmentContents' has no 'error' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachmentcontents.py:37:  [FetchAttachmentContents.get] Instance of 'FetchAttachmentContents' has no 'error' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachmentcontents.py:39:  [FetchAttachmentContents.get] Instance of 'FetchAttachmentContents' has no 'response' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:30:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:33:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'error' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:37:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'error' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:39:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'response' member  [pylint/E1101] [5]
Total errors found: 23 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Created attachment 342766 [details]
[Patch] Part 2 - Bugzilla extension

[EWS Watchlist]

Attachment 342766 [details] did not pass style-queue:


ERROR: Websites/bugs.webkit.org/ChangeLog:3:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: security bug, security bug  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Created attachment 342767 [details]
[Patch] Part 1 - webkitpy and QueueStatusServer

[EWS Watchlist]

Attachment 342767 [details] did not pass style-queue:


ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:32:  [UploadAttachment.get] Instance of 'UploadAttachment' has no 'response' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:35:  [UploadAttachment.post] Instance of 'UploadAttachment' has no '_int_from_request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:36:  [UploadAttachment.post] Instance of 'UploadAttachment' has no 'request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:37:  [UploadAttachment.post] Instance of 'UploadAttachment' has no 'request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:39:  [UploadAttachment.post] Instance of 'UploadAttachment' has no 'response' member  [pylint/E1101] [5]
ERROR: Tools/ChangeLog:3:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: security bug  [changelog/unwantedsecurityterms] [3]
ERROR: Tools/QueueStatusServer/model/attachmentdata.py:33:  [AttachmentData.add_attachment_data] Class 'AttachmentData' has no 'get_or_insert' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/model/attachmentdata.py:37:  [AttachmentData.lookup_if_exists] Class 'AttachmentData' has no 'get_by_key_name' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachmentcontents.py:30:  [FetchAttachmentContents.get] Instance of 'FetchAttachmentContents' has no 'request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachmentcontents.py:33:  [FetchAttachmentContents.get] Instance of 'FetchAttachmentContents' has no 'error' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachmentcontents.py:37:  [FetchAttachmentContents.get] Instance of 'FetchAttachmentContents' has no 'error' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachmentcontents.py:39:  [FetchAttachmentContents.get] Instance of 'FetchAttachmentContents' has no 'response' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:30:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:33:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'error' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:37:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'error' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:39:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'response' member  [pylint/E1101] [5]
Total errors found: 16 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Igalia-pontevedra EWS]

Comment on attachment 342766 [details]
[Patch] Part 2 - Bugzilla extension

Attachment 342766 [details] did not pass gtk-wk2-ews (gtk-wk2):
Output: http://webkit-queues.webkit.org/results/8189722

New failing tests:
http/tests/misc/char-encoding-bocu-1-blacklisted.html

[Igalia-pontevedra EWS]

Created attachment 342787 [details]
Archive of layout-test-results from ltilve-gtk-wk2-ews for gtk-wk2

The attached test failures were seen while running run-webkit-tests on the gtk-wk2-ews.
Bot: ltilve-gtk-wk2-ews  Port: gtk-wk2  Platform: Linux-4.16.0-0.bpo.1-amd64-x86_64-with-debian-9.4

[Daniel Bates]

Created attachment 342789 [details]
[Patch] Part 2 - Bugzilla extension

Simplified the Bugzilla extension and added per-function remarks in the ChangeLog.

[EWS Watchlist]

Attachment 342789 [details] did not pass style-queue:


ERROR: Websites/bugs.webkit.org/ChangeLog:3:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: security bug, security bug  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Created attachment 342819 [details]
[Patch] Part 1 - webkitpy and QueueStatusServer

Updated StatusServer.fetch_attachment() to handle a missing, invalid or revoked API key. Standardized on "API key" instead of "client id".

[EWS Watchlist]

Attachment 342819 [details] did not pass style-queue:


ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:32:  [UploadAttachment.get] Instance of 'UploadAttachment' has no 'response' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:35:  [UploadAttachment.post] Instance of 'UploadAttachment' has no '_int_from_request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:36:  [UploadAttachment.post] Instance of 'UploadAttachment' has no 'request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:37:  [UploadAttachment.post] Instance of 'UploadAttachment' has no 'request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:39:  [UploadAttachment.post] Instance of 'UploadAttachment' has no 'response' member  [pylint/E1101] [5]
ERROR: Tools/ChangeLog:3:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: security bug  [changelog/unwantedsecurityterms] [3]
ERROR: Tools/QueueStatusServer/model/attachmentdata.py:33:  [AttachmentData.add_attachment_data] Class 'AttachmentData' has no 'get_or_insert' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/model/attachmentdata.py:37:  [AttachmentData.lookup_if_exists] Class 'AttachmentData' has no 'get_by_key_name' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachmentcontents.py:30:  [FetchAttachmentContents.get] Instance of 'FetchAttachmentContents' has no 'request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachmentcontents.py:33:  [FetchAttachmentContents.get] Instance of 'FetchAttachmentContents' has no 'error' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachmentcontents.py:37:  [FetchAttachmentContents.get] Instance of 'FetchAttachmentContents' has no 'error' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachmentcontents.py:39:  [FetchAttachmentContents.get] Instance of 'FetchAttachmentContents' has no 'response' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:30:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:33:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'error' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:37:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'error' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:39:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'response' member  [pylint/E1101] [5]
Total errors found: 16 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

(In reply to Build Bot from comment #27)
> Attachment 342819 [details] did not pass style-queue:
> 
> 
> ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:32: 
> [UploadAttachment.get] Instance of 'UploadAttachment' has no 'response'
> member  [pylint/E1101] [5]
> ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:35: 
> [UploadAttachment.post] Instance of 'UploadAttachment' has no
> '_int_from_request' member  [pylint/E1101] [5]
> ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:36: 
> [UploadAttachment.post] Instance of 'UploadAttachment' has no 'request'
> member  [pylint/E1101] [5]
> ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:37: 
> [UploadAttachment.post] Instance of 'UploadAttachment' has no 'request'
> member  [pylint/E1101] [5]
> ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:39: 
> [UploadAttachment.post] Instance of 'UploadAttachment' has no 'response'
> member  [pylint/E1101] [5]
> ERROR: Tools/ChangeLog:3:  Please consider whether the use of
> security-sensitive phrasing could help someone exploit WebKit: security bug 
> [changelog/unwantedsecurityterms] [3]
> ERROR: Tools/QueueStatusServer/model/attachmentdata.py:33: 
> [AttachmentData.add_attachment_data] Class 'AttachmentData' has no
> 'get_or_insert' member  [pylint/E1101] [5]
> ERROR: Tools/QueueStatusServer/model/attachmentdata.py:37: 
> [AttachmentData.lookup_if_exists] Class 'AttachmentData' has no
> 'get_by_key_name' member  [pylint/E1101] [5]
> ERROR: Tools/QueueStatusServer/handlers/fetchattachmentcontents.py:30: 
> [FetchAttachmentContents.get] Instance of 'FetchAttachmentContents' has no
> 'request' member  [pylint/E1101] [5]
> ERROR: Tools/QueueStatusServer/handlers/fetchattachmentcontents.py:33: 
> [FetchAttachmentContents.get] Instance of 'FetchAttachmentContents' has no
> 'error' member  [pylint/E1101] [5]
> ERROR: Tools/QueueStatusServer/handlers/fetchattachmentcontents.py:37: 
> [FetchAttachmentContents.get] Instance of 'FetchAttachmentContents' has no
> 'error' member  [pylint/E1101] [5]
> ERROR: Tools/QueueStatusServer/handlers/fetchattachmentcontents.py:39: 
> [FetchAttachmentContents.get] Instance of 'FetchAttachmentContents' has no
> 'response' member  [pylint/E1101] [5]
> ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:30: 
> [FetchAttachment.get] Instance of 'FetchAttachment' has no 'request' member 
> [pylint/E1101] [5]
> ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:33: 
> [FetchAttachment.get] Instance of 'FetchAttachment' has no 'error' member 
> [pylint/E1101] [5]
> ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:37: 
> [FetchAttachment.get] Instance of 'FetchAttachment' has no 'error' member 
> [pylint/E1101] [5]
> ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:39: 
> [FetchAttachment.get] Instance of 'FetchAttachment' has no 'response' member
> [pylint/E1101] [5]
> Total errors found: 16 in 29 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

check-webkit-style does not seem to be able to see definitions in other Python modules.

[Daniel Bates]

(In reply to Build Bot from comment #27)

> ERROR: Tools/ChangeLog:3:  Please consider whether the use of
> security-sensitive phrasing could help someone exploit WebKit: security bug 
> [changelog/unwantedsecurityterms] [3]

Will keep ChangeLog as-is. I am not revealing anything sensitive in the ChangeLog by using the phrase "security bug" in the bug title.

[Aakash Jain]

Comment on attachment 342819 [details]
[Patch] Part 1 - webkitpy and QueueStatusServer

View in context: https://bugs.webkit.org/attachment.cgi?id=342819&action=review

> Tools/QueueStatusServer/handlers/fetchattachmentcontents.py:28
> +class FetchAttachmentContents(webapp.RequestHandler):

This code looks duplicated from fetchattachment.py
Do we need two separate classes? 
Probably we can just have one class having two methods for attachment metadata and data, or just one method with an parameter to select whether to return data or metadata.

[Aakash Jain]

Comment on attachment 342819 [details]
[Patch] Part 1 - webkitpy and QueueStatusServer

View in context: https://bugs.webkit.org/attachment.cgi?id=342819&action=review

> Tools/ChangeLog:26
> +        (_path_to_authorized_api_keys_file):

Can you please add the description of the changes in the ChangeLog here? That would make review easier.

> Tools/Scripts/webkitpy/common/net/bugzilla/bug.py:77
> +        return bool(group and group == 'Security-Sensitive')

I believe this can be simplified to:

return self.group() == 'Security-Sensitive'

[Daniel Bates]

(In reply to Aakash Jain from comment #30)
> Comment on attachment 342819 [details]
> [Patch] Part 1 - webkitpy and QueueStatusServer
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=342819&action=review
> 
> > Tools/QueueStatusServer/handlers/fetchattachmentcontents.py:28
> > +class FetchAttachmentContents(webapp.RequestHandler):
> 
> This code looks duplicated from fetchattachment.py
> Do we need two separate classes? 

No, we do not need separate classes. Will remove class FetchAttachmentContents and modify the URL route /attachment to take two parameters an action ("metadata", "data") and an attachment id (in that order).

[Daniel Bates]

(In reply to Aakash Jain from comment #31)
> Comment on attachment 342819 [details]
> [Patch] Part 1 - webkitpy and QueueStatusServer
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=342819&action=review
> 
> > Tools/ChangeLog:26
> > +        (_path_to_authorized_api_keys_file):
> 
> Can you please add the description of the changes in the ChangeLog here?
> That would make review easier.
> 

What would you like me to elaborate on? I mean, the purpose of the function is to the return the path to the authorized API keys file.

> > Tools/Scripts/webkitpy/common/net/bugzilla/bug.py:77
> > +        return bool(group and group == 'Security-Sensitive')
> 
> I believe this can be simplified to:
> 
> return self.group() == 'Security-Sensitive'

Will use the empty string to indicate that a bug does not have a group (since you cannot create such a named group), have Bug.group() always return a string, and then will take your suggestion.

[Aakash Jain]

Comment on attachment 342819 [details]
[Patch] Part 1 - webkitpy and QueueStatusServer

View in context: https://bugs.webkit.org/attachment.cgi?id=342819&action=review

>>> Tools/ChangeLog:26
>>> +        (_path_to_authorized_api_keys_file):
>> 
>> Can you please add the description of the changes in the ChangeLog here? That would make review easier.
> 
> What would you like me to elaborate on? I mean, the purpose of the function is to the return the path to the authorized API keys file.

I meant description of the changes for all the files/functions (not just this function).

[Daniel Bates]

Created attachment 342836 [details]
[Patch] Part 1 - webkitpy and QueueStatusServer

Updated patch based on feedback from Aakash Jain. I added per-function/class remarks in the ChangeLog where I felt such remarks would add value.

[EWS Watchlist]

Attachment 342836 [details] did not pass style-queue:


ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:32:  [UploadAttachment.get] Instance of 'UploadAttachment' has no 'response' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:35:  [UploadAttachment.post] Instance of 'UploadAttachment' has no '_int_from_request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:36:  [UploadAttachment.post] Instance of 'UploadAttachment' has no 'request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:37:  [UploadAttachment.post] Instance of 'UploadAttachment' has no 'request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:39:  [UploadAttachment.post] Instance of 'UploadAttachment' has no 'response' member  [pylint/E1101] [5]
ERROR: Tools/ChangeLog:3:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: security bug, security bug, security bug, security bug, security bug  [changelog/unwantedsecurityterms] [3]
ERROR: Tools/QueueStatusServer/model/attachmentdata.py:33:  [AttachmentData.add_attachment_data] Class 'AttachmentData' has no 'get_or_insert' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/model/attachmentdata.py:37:  [AttachmentData.lookup_if_exists] Class 'AttachmentData' has no 'get_by_key_name' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:31:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'error' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:33:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:36:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'error' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:40:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'error' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:43:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'response' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:45:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'response' member  [pylint/E1101] [5]
Total errors found: 14 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 342836 [details]
[Patch] Part 1 - webkitpy and QueueStatusServer

Attachment 342836 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8202013

New failing tests:
http/tests/security/contentSecurityPolicy/video-redirect-blocked.html

[EWS Watchlist]

Created attachment 342844 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Daniel Bates]

(In reply to Alexey Proskuryakov from comment #9)
> While this is logically accurate, making access much easier could be
> problematic. Perhaps a wider discussion would be useful (webkit-dev?)

Emailed webkit-dev: <https://lists.webkit.org/pipermail/webkit-dev/2018-June/030037.html>

[Aakash Jain]

Comment on attachment 342836 [details]
[Patch] Part 1 - webkitpy and QueueStatusServer

View in context: https://bugs.webkit.org/attachment.cgi?id=342836&action=review

> Tools/QueueStatusServer/config/authorization.py:29
> +    return os.path.abspath(os.path.join(os.path.dirname(__file__), "authorized_api_keys.txt"))

Is it looking for the file in QueueStatusServer directory or QueueStatusServer/config directory?

> Tools/QueueStatusServer/config/authorization.py:46
> +    authorized_api_keys_path = _path_to_authorized_api_keys_file()

do we need this authorized_api_keys_path variable. it is used only once in the line below. we can directly call the function there.

> Tools/QueueStatusServer/config/authorization.py:58
> +    token68_encoded_value = parts[1]

probably we should simply name it 'key' instead of token68_encoded_value, to make it more readable.

> Tools/QueueStatusServer/config/authorization.py:59
> +    if scheme.lower() == "apikey":

we should probably inverse this if statement and do early return. e.g.:

if scheme.lower() != "apikey":
    return ""

> Tools/QueueStatusServer/config/authorization.py:70
> +        api_key = request.get("api-key")

any particular reason to use 'api-key' instead of 'apikey'?

> Tools/QueueStatusServer/handlers/releasepatch.py:45
> +    def check_complete(attachment_id):

This method name is not very descriptive, doesn't indicate what is complete. Can make this name more readable, e.g.: is_ews_processing_complete()

> Tools/QueueStatusServer/model/attachmentdata.py:36
> +    def lookup_if_exists(cls, attachment_id):

the name of this method can be improved. e.g.: get_value_by_key() , lookup seems confusing, this is actually fetching the data.

> Tools/QueueStatusServer/model/attachmentdata.py:41
> +        attachment_data = cls.lookup_if_exists(attachment_id)

don't need to use the helper function, can directly do:
attachment_data = cls.get_by_key_name(str(attachment_id))

> Tools/Scripts/webkitpy/common/net/bugzilla/attachment.py:127
> +        temp = dict(self._attachment_dictionary)

can rename temp to temp_attachment_dictionary here to be more descriptive.

> Tools/Scripts/webkitpy/common/net/bugzilla/attachment.py:134
> +        temp = json.loads(json_string)

Ditto about variable name temp.

> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:453
> +            _log.warning("You don't have permission to view this bug.")

Can we print the bug_id in the log message?

> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:493
> +        if bug_dictionary:

should inverse this if statement to do early-return.

if not bug_dictionary:
    return None
return Bug(bug_dictionary, self)

> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:545
> +            raise Bugzilla.AccessError(attachment_id, error_code, str(self._parse_bug_title_from_attachment_page(page)))

Who handles this exception? Is this logged somewhere?

> Tools/Scripts/webkitpy/common/net/statusserver.py:67
> +            new_headers.append((AUTHORIZATION_HEADER_NAME, 'APIKey ' + api_key))

why 'APIKey' instead of 'apikey', on server-side we are checking for apikey (although it would work fine since .lower() is used on server-side, it would be better to be consistent).

[Aakash Jain]

Comment on attachment 342836 [details]
[Patch] Part 1 - webkitpy and QueueStatusServer

View in context: https://bugs.webkit.org/attachment.cgi?id=342836&action=review

> Tools/Scripts/webkitpy/common/net/statusserver.py:132
> +        self._browser['attachment_id'] = unicode(attachment_id)

shouldn't we use str here instead of unicode to be consistent.

[Daniel Bates]

Created attachment 342858 [details]
[Patch] Part 1 - webkitpy and QueueStatusServer

[EWS Watchlist]

Attachment 342858 [details] did not pass style-queue:


ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:32:  [UploadAttachment.get] Instance of 'UploadAttachment' has no 'response' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:35:  [UploadAttachment.post] Instance of 'UploadAttachment' has no '_int_from_request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:36:  [UploadAttachment.post] Instance of 'UploadAttachment' has no 'request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:37:  [UploadAttachment.post] Instance of 'UploadAttachment' has no 'request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:39:  [UploadAttachment.post] Instance of 'UploadAttachment' has no 'response' member  [pylint/E1101] [5]
ERROR: Tools/ChangeLog:3:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: security bug, security bug, security bug, security bug, security bug  [changelog/unwantedsecurityterms] [3]
ERROR: Tools/QueueStatusServer/model/attachmentdata.py:33:  [AttachmentData.add_attachment_data] Class 'AttachmentData' has no 'get_or_insert' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/model/attachmentdata.py:37:  [AttachmentData.lookup_if_exists] Class 'AttachmentData' has no 'get_by_key_name' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:31:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'error' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:33:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:36:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'error' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:40:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'error' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:43:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'response' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:45:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'response' member  [pylint/E1101] [5]
Total errors found: 14 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Comment on attachment 342836 [details]
[Patch] Part 1 - webkitpy and QueueStatusServer

View in context: https://bugs.webkit.org/attachment.cgi?id=342836&action=review

I addressed all these remarks in the latest iteration of the patch (attachment #342858 [details]).

>> Tools/QueueStatusServer/config/authorization.py:29
>> +    return os.path.abspath(os.path.join(os.path.dirname(__file__), "authorized_api_keys.txt"))
> 
> Is it looking for the file in QueueStatusServer directory or QueueStatusServer/config directory?

The latter because __file__ will be /path/to/config/authorization.py.

>> Tools/QueueStatusServer/config/authorization.py:46
>> +    authorized_api_keys_path = _path_to_authorized_api_keys_file()
> 
> do we need this authorized_api_keys_path variable. it is used only once in the line below. we can directly call the function there.

Will inline the value of this variable into the line below.

>> Tools/QueueStatusServer/config/authorization.py:58
>> +    token68_encoded_value = parts[1]
> 
> probably we should simply name it 'key' instead of token68_encoded_value, to make it more readable.

I named this variable to indicate the encoding of its value, token68. See <https://tools.ietf.org/html/rfc7235> for more detail. In particular, see <https://tools.ietf.org/html/rfc7235#section-2.1>.

>> Tools/QueueStatusServer/config/authorization.py:59
>> +    if scheme.lower() == "apikey":
> 
> we should probably inverse this if statement and do early return. e.g.:
> 
> if scheme.lower() != "apikey":
>     return ""

This would be checking for failure. It is good programming practice to check for success instead of failure and let the failure case be part of the "else" code path.

>> Tools/QueueStatusServer/config/authorization.py:70
>> +        api_key = request.get("api-key")
> 
> any particular reason to use 'api-key' instead of 'apikey'?

As per our conversation, will leave as-is.

>> Tools/QueueStatusServer/handlers/releasepatch.py:45
>> +    def check_complete(attachment_id):
> 
> This method name is not very descriptive, doesn't indicate what is complete. Can make this name more readable, e.g.: is_ews_processing_complete()

I renamed this function to check_processed_by_all_queues. I am open to naming suggestions.

>> Tools/QueueStatusServer/model/attachmentdata.py:36
>> +    def lookup_if_exists(cls, attachment_id):
> 
> the name of this method can be improved. e.g.: get_value_by_key() , lookup seems confusing, this is actually fetching the data.

As per our conversation, will keep as-is. I was just following the naming convention we use for getter functions throughout the code.

>> Tools/QueueStatusServer/model/attachmentdata.py:41
>> +        attachment_data = cls.lookup_if_exists(attachment_id)
> 
> don't need to use the helper function, can directly do:
> attachment_data = cls.get_by_key_name(str(attachment_id))

Will keep as-is since we use lookup_if_exists() in fetchattachment.py.

>> Tools/Scripts/webkitpy/common/net/bugzilla/attachment.py:127
>> +        temp = dict(self._attachment_dictionary)
> 
> can rename temp to temp_attachment_dictionary here to be more descriptive.

As per our conversation, will keep as-is since "temp" is more canonical and the right-hand side makes it clear this is a copy of the attachment dictionary.

>> Tools/Scripts/webkitpy/common/net/bugzilla/attachment.py:134
>> +        temp = json.loads(json_string)
> 
> Ditto about variable name temp.

Will keep as-is for the same reason as above.

>> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:453
>> +            _log.warning("You don't have permission to view this bug.")
> 
> Can we print the bug_id in the log message?

I prefer not to include the bug ID in the warning message here and leave it to the calling code, Bugzilla._fetch_bug(), to print out this information when it prints out the full URL it is fetching to avoid duplication of information and to keep the format of this message consistent with the formatting of other logging messages that are emitted form parsing functions (e.g. Bugzilla._parse_bug_id_from_attachment_page())

>> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:493
>> +        if bug_dictionary:
> 
> should inverse this if statement to do early-return.
> 
> if not bug_dictionary:
>     return None
> return Bug(bug_dictionary, self)

Will keep as-is.

>> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:545
>> +            raise Bugzilla.AccessError(attachment_id, error_code, str(self._parse_bug_title_from_attachment_page(page)))
> 
> Who handles this exception? Is this logged somewhere?

Yes, this is handled. One place it is handled is in file earlywarningsystem.py.

>> Tools/Scripts/webkitpy/common/net/statusserver.py:67
>> +            new_headers.append((AUTHORIZATION_HEADER_NAME, 'APIKey ' + api_key))
> 
> why 'APIKey' instead of 'apikey', on server-side we are checking for apikey (although it would work fine since .lower() is used on server-side, it would be better to be consistent).

Will rename to apikey.

[Daniel Bates]

(In reply to Aakash Jain from comment #41)
> Comment on attachment 342836 [details]
> [Patch] Part 1 - webkitpy and QueueStatusServer
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=342836&action=review
> 
> > Tools/Scripts/webkitpy/common/net/statusserver.py:132
> > +        self._browser['attachment_id'] = unicode(attachment_id)
> 
> shouldn't we use str here instead of unicode to be consistent.

I was just mimic'ing the same code in _post_work_item_to_ews().

[Daniel Bates]

Created attachment 342892 [details]
[Patch] Part 1 - webkitpy and QueueStatusServer

Minor cosmetic changes. Have FetchAttachment.get() convert the attachment id string to an integer to make such handling consistent with the handling logic in Patch.get() and StatusBubble.get(). Simplify the added function ReleasePatch._check_processed_by_all_queues() by making use of existing convenience functions.

[EWS Watchlist]

Attachment 342892 [details] did not pass style-queue:


ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:32:  [UploadAttachment.get] Instance of 'UploadAttachment' has no 'response' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:35:  [UploadAttachment.post] Instance of 'UploadAttachment' has no '_int_from_request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:36:  [UploadAttachment.post] Instance of 'UploadAttachment' has no 'request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:37:  [UploadAttachment.post] Instance of 'UploadAttachment' has no 'request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/uploadattachment.py:39:  [UploadAttachment.post] Instance of 'UploadAttachment' has no 'response' member  [pylint/E1101] [5]
ERROR: Tools/ChangeLog:3:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: security bug, security bug, security bug, security bug, security bug  [changelog/unwantedsecurityterms] [3]
ERROR: Tools/QueueStatusServer/model/attachmentdata.py:33:  [AttachmentData.add_attachment_data] Class 'AttachmentData' has no 'get_or_insert' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/model/attachmentdata.py:37:  [AttachmentData.lookup_if_exists] Class 'AttachmentData' has no 'get_by_key_name' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:31:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'error' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:33:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:36:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'error' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:40:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'error' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:43:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'response' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/fetchattachment.py:45:  [FetchAttachment.get] Instance of 'FetchAttachment' has no 'response' member  [pylint/E1101] [5]
Total errors found: 14 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Created attachment 342907 [details]
[Patch] Part 1 - webkitpy and QueueStatusServer

This patch depends on the patch for bug #186748. In particular, this patch depends on the file attachment_unittest.py, which is added in the patch for bug #186748.

[Lucas Forschler]

Comment on attachment 342907 [details]
[Patch] Part 1 - webkitpy and QueueStatusServer

View in context: https://bugs.webkit.org/attachment.cgi?id=342907&action=review

r=me after fixing identified concerns.

> Tools/Scripts/webkitpy/common/net/bugzilla/attachment.py:143
> +        return Attachment(temp, None)

Can you used a named argument form here for the None value?

> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:470
> +            bug['group'] = self._string_contents(group)

I'm not sure if we will ever get a bug with multiple groups. But, if we did, we might need to handle this smart. 
What does soup.find('group') return if a bug is in multiple groups?
Let's assert if we find more than one group.

> Tools/Scripts/webkitpy/common/net/statusserver.py:54
> +    def set_host(self, host, use_https=False):

is there any reason not to use_https=True by default?

> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:160
> +            # status server.

Can we add a link to the Bugzilla id for future work?

> Tools/Scripts/webkitpy/tool/main.py:50
> +        make_option("--status-host-uses-https", action="store_true", default=False, dest="status_host_uses_https", help="Use HTTPS when querying the status host."),

let's set default=True for this one.

> Tools/Scripts/webkitpy/tool/main.py:104
> +            pass

Let's log something here rather than pass.

[Dean Johnson]

Comment on attachment 342907 [details]
[Patch] Part 1 - webkitpy and QueueStatusServer

View in context: https://bugs.webkit.org/attachment.cgi?id=342907&action=review

Minor comments on simplifying loops, and other small nits. One more major concern on ensuring `feed` is getting security bugs to process, but otherwise the code looks good to me. Unofficial r=me w/ clarification on the feed function with respect to security bugs.

> Tools/QueueStatusServer/config/authorization.py:33
> +    api_keys = set()

Nit: These 5 lines might be more succinctly written like so (if all the bots use at least python2.7): 
api_keys = {line for line in file if line and not line.startswith('#')}
return frozenset(api_keys)

> Tools/QueueStatusServer/config/authorization.py:42
> +def authorized_api_keys():

Nit: Do we have a memoize decorator somewhere in webkitpy we can use?

> Tools/QueueStatusServer/config/authorization.py:52
> +_API_KEY_SCHEME = "apikey"

Nit: Can we move this definition to the top of the file near `_cached_authorized_api_keys`?

> Tools/QueueStatusServer/handlers/releasepatch.py:44
> +    def _check_processed_by_all_queues(attachment):

For your consideration, you could write this more succinctly like so:
return all(attachment.position_in_queue(queue) is None for queue in Queue.all_ews())

>> Tools/Scripts/webkitpy/common/net/bugzilla/attachment.py:143
>> +        return Attachment(temp, None)
> 
> Can you used a named argument form here for the None value?

Would it be more accurate to instead return cls(temp, None)?

> Tools/Scripts/webkitpy/common/net/statusserver.py:56
> +        if use_https:

Nit: This may read better written like so: `http_protocol = 'https' if use_https else 'http'`

> Tools/Scripts/webkitpy/common/net/statusserver.py:222
> +            return urllib2.urlopen(request, timeout=300).read()

Is there a reason to not convert this to use NetworkTransaction while you're here?

> Tools/Scripts/webkitpy/tool/bot/feeders.py:90
> +        security_ids_needing_review = frozenset(self._tool.bugs.queries.fetch_attachment_ids_from_review_queue(current_time - timedelta(7), only_security_bugs=True))

How do these bugs get into `new_ids` for processing? Does fetch_attachment_ids_from_revoew_queue add the bugs to self._ids_sent_to_server?

[Igalia-pontevedra EWS]

Comment on attachment 342789 [details]
[Patch] Part 2 - Bugzilla extension

Attachment 342789 [details] did not pass gtk-wk2-ews (gtk-wk2):
Output: http://webkit-queues.webkit.org/results/8240191

New failing tests:
http/tests/misc/canvas-pattern-from-incremental-image.html

[Igalia-pontevedra EWS]

Created attachment 343010 [details]
Archive of layout-test-results from ltilve-gtk-wk2-ews for gtk-wk2

The attached test failures were seen while running run-webkit-tests on the gtk-wk2-ews.
Bot: ltilve-gtk-wk2-ews  Port: gtk-wk2  Platform: Linux-4.16.0-0.bpo.1-amd64-x86_64-with-debian-9.4

[Daniel Bates]

(In reply to Lucas Forschler from comment #49)
> > Tools/Scripts/webkitpy/common/net/bugzilla/attachment.py:143
> > +        return Attachment(temp, None)
> 
> Can you used a named argument form here for the None value?
> 

Will update code to use a named argument before landing.

> > Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:470
> > +            bug['group'] = self._string_contents(group)
> 
> I'm not sure if we will ever get a bug with multiple groups. But, if we did,
> we might need to handle this smart. 
> What does soup.find('group') return if a bug is in multiple groups?
> Let's assert if we find more than one group.
> 

As it turns out it is straightforward to add support for multiple groups. Will change this code to read:

bug['groups'] = frozenset([self._string_contents(group) for group in soup.findAll('group')])

And will change the added code in bug.py to read:

[[
    def groups(self):
        return self.bug_dictionary.get('groups', frozenset())

    def is_security_sensitive(self):
        return 'Security-Sensitive' in self.groups()
]]

> > Tools/Scripts/webkitpy/common/net/statusserver.py:54
> > +    def set_host(self, host, use_https=False):
> 
> is there any reason not to use_https=True by default?
> 

Will make this the default and add a remark in the ChangeLog.

> > Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:160
> > +            # status server.
> 
> Can we add a link to the Bugzilla id for future work?
> 
> > Tools/Scripts/webkitpy/tool/main.py:50
> > +        make_option("--status-host-uses-https", action="store_true", default=False, dest="status_host_uses_https", help="Use HTTPS when querying the status host."),
> 
> let's set default=True for this one.
> 

Will make HTTPS the default.

> > Tools/Scripts/webkitpy/tool/main.py:104
> > +            pass
> 
> Let's log something here rather than pass.

Will import module logging, define _log, and replace "pass" with the following code:

_log.warning('Could not read API key from Git: {}'.format(str(e)))

[Daniel Bates]

(In reply to Dean Johnson from comment #50)
> > Tools/QueueStatusServer/config/authorization.py:33
> > +    api_keys = set()
> 
> Nit: These 5 lines might be more succinctly written like so (if all the bots
> use at least python2.7): 
> api_keys = {line for line in file if line and not line.startswith('#')}
> return frozenset(api_keys)
> 

This does not produce the same results if the line contains whitespace:

>>> import StringIO
>>> file = StringIO.StringIO("""
...              
... # A line that begins with a comment.
...     # A line with leading whitespace then a comment.
... # A line with a comment then trailing whitespace.     
...     # A line with leading whitespace and a comment with trailing whitespace.    
...     api_key_with_leading_whitespace
... api_key_with_trailing_whitespace    
...     api_key_with_leading_and_trailing_whitespace    
... """)
>>> api_keys = {line for line in file if line and not line.startswith('#')}
>>> frozenset(api_keys)
frozenset(['    api_key_with_leading_whitespace\n', '             \n', 'api_key_with_trailing_whitespace    \n', '    # A line with leading whitespace then a comment.\n', '\n', '    # A line with leading whitespace and a comment with trailing whitespace.    \n', '    api_key_with_leading_and_trailing_whitespace    \n'])

Modifying your suggestion to read:

api_keys = {line.strip() for line in file if line.strip() and not line.strip().startswith('#')}
return frozenset(api_keys)

Then I get what I want at the cost of computing strip() three times. Is there a way to avoid this. Otherwise, I will leave the current code as-is.

> > Tools/QueueStatusServer/config/authorization.py:42
> > +def authorized_api_keys():
> 
> Nit: Do we have a memoize decorator somewhere in webkitpy we can use?

Notice that this code lives in QueueStatusServer and does not make use of webkitpy to simplify appengine deployment. We can copy over memoized.py from webkitpy though I am unclear how beneficial it is to do so given that I count only two places in QueueStatusServer that cache results: this function and Attachment._queue_positions(). And the latter has a FIXME to consider storing the result in memcache.

> 
> > Tools/QueueStatusServer/config/authorization.py:52
> > +_API_KEY_SCHEME = "apikey"
> 
> Nit: Can we move this definition to the top of the file near
> `_cached_authorized_api_keys`?
> 

Will move to the top of the file under _cached_authorized_api_keys. I will also rename it to _AUTHORIZATION_SCHEME to coincide with "auth-scheme" used in <https://tools.ietf.org/html/rfc7235>.

> > Tools/QueueStatusServer/handlers/releasepatch.py:44
> > +    def _check_processed_by_all_queues(attachment):
> 
> For your consideration, you could write this more succinctly like so:
> return all(attachment.position_in_queue(queue) is None for queue in
> Queue.all_ews())
> 

Will take your suggestion replacement.

> >> Tools/Scripts/webkitpy/common/net/bugzilla/attachment.py:143
> >> +        return Attachment(temp, None)
> > 
> > Can you used a named argument form here for the None value?
> 
> Would it be more accurate to instead return cls(temp, None)?
> 

Will change to use cls().

> > Tools/Scripts/webkitpy/common/net/statusserver.py:56
> > +        if use_https:
> 
> Nit: This may read better written like so: `http_protocol = 'https' if
> use_https else 'http'`
> 

I prefer the name I used because "http_protocol" may not be equal to "http". Will update the code to read:

protocol_to_use = 'https' if use_https else 'http'

> > Tools/Scripts/webkitpy/common/net/statusserver.py:222
> > +            return urllib2.urlopen(request, timeout=300).read()
> 
> Is there a reason to not convert this to use NetworkTransaction while you're
> here?
> 

I would prefer to do such a change in a separate bug/patch as this patch is sufficiently large as-is and such a change is not strictly needed to implement support for EWS processing of security bug patches.

> > Tools/Scripts/webkitpy/tool/bot/feeders.py:90
> > +        security_ids_needing_review = frozenset(self._tool.bugs.queries.fetch_attachment_ids_from_review_queue(current_time - timedelta(7), only_security_bugs=True))
> 
> How do these bugs get into `new_ids` for processing? Does
> fetch_attachment_ids_from_revoew_queue add the bugs to
> self._ids_sent_to_server?

Notice that ids_needing_review is a superset: it includes all ids in security_ids_needing_review. Therefore, new_ids contains both non-security and security attachment ids.

[Daniel Bates]

Committed r232979: <https://trac.webkit.org/changeset/232979>

[Daniel Bates]

Committed r232980: <https://trac.webkit.org/changeset/232980>

[Daniel Bates]

(In reply to Lucas Forschler from comment #49)
> > Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:160
> > +            # status server.
> 
> Can we add a link to the Bugzilla id for future work?
> 

I did not mean to ignore this. Filed bug #186817 and updated FIXME comment in <https://trac.webkit.org/changeset/232982>.

[Daniel Bates]

Fixed inadvertently removed line in <https://trac.webkit.org/changeset/233061>.

[Daniel Bates]

Substituted "review" for "r" as the name of the Bugzilla flag to look at to determine if we should CC the feeder EWS on the associated bug and committed that in <https://trac.webkit.org/changeset/233109>.