There is no code to review or RFC at this time and we are unable to move forward until they exist. At this time we can and will move forward with the minishlink/web-push review: https://phabricator.wikimedia.org/T246714

@sbassett, I don't think I'd describe putting the same code in Echo vs. a new extension as an "entirely new approach." In any case, it's been my experience that many teams (including this one) appreciate having a tech review task opened early as a placeholder to assist with planning. If it's not useful to you, feel free to close it.

In T246712#6130339, @Mholloway wrote:

@sbassett, I don't think I'd describe putting the same code in Echo vs. a new extension as an "entirely new approach." In any case, it's been my experience that many teams (including this one) appreciate having a tech review task opened early as a placeholder to assist with planning. If it's not useful to you, feel free to close it.

I think our team is fine with advanced notice and the filing of a security review task. It's just that when the project is still within an earlier phase (outstanding RFC, etc.) and a far-off deployment date which may be likely to change, it's not quite as useful to us. For our security reviews, we really need the code to be in as close to a production-ready state as possible. Apologies if this isn't immediately clear within our SOP. I'm going to move this to our back orders column for now until the project and code are a bit further along, at which point we'll want to agree upon what commit shas we should review (basically a freezing point for the code for this review).

@Mholloway - I assume the "Relevant files/directories" for Echo mentioned within the task description are all on master? That branch doesn't appear very volatile for the time being, so I assume it's stable enough for a review.

Hi @sbassett, there's one patch I've just uploaded to Gerrit which will update the push-related code in Echo: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Echo/+/619570. Yes, the rest is on master.

Additionally, we met as a team with Gilles today to discuss the performance review feedback. Pending further discussion tomorrow, I anticipate that the architecture will remain the same as it is now, but some refinements will be added on the service side.

In T246712#6377970, @Mholloway wrote:

Hi @sbassett, there's one patch I've just uploaded to Gerrit which will update the push-related code in Echo: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Echo/+/619570. Yes, the rest is on master.

Ok, thanks. Looks like https://gerrit.wikimedia.org/r/619570 is maybe more experimental at this stage? At least that's what I'm getting from your -2 and comments.

Additionally, we met as a team with Gilles today to discuss the performance review feedback. Pending further discussion tomorrow, I anticipate that the architecture will remain the same as it is now, but some refinements will be added on the service side.

Sounds good.

Is there a new target deployment date for this? I've been looking at the pieces of code which are most complete and could probably have a review completed by the middle of next week if that works.

@sbassett, Very sorry for the delayed response. We were discussing internally whether the work related to T259148 (involving both Echo and the service) was a blocker for initial launch, and therefore for security readiness review. We just reached consensus that it isn't a blocker for either.

With that, I don't expect any further changes to the push-related code in Echo, and the only additional patches that will be coming in on the service will be adding metrics, so I don't anticipate that those will introduce any security concerns.

Also, since we've run into a couple of late-breaking delays in the past couple of weeks, we decided to push the target public launch date back to September 21.

In T246712#6396582, @Mholloway wrote:

Also, since we've run into a couple of late-breaking delays in the past couple of weeks, we decided to push the target public launch date back to September 21.

Ok, sounds good. I should be able to have the entire review completed by early September as long as the code bases do not become more volatile. Also is https://gerrit.wikimedia.org/r/619570 close to being merged?

Thanks, @sbassett, that sounds good. https://gerrit.wikimedia.org/r/619570 is ready to land from my perspective, and I've asked my teammates to make it a review priority.

Hi @sbassett, I wanted to check in with you on the current status of this review. There is one substantial patch for the push service which would be best to land before launching, but which we could wait on until post-launch refinements if necessary: https://gerrit.wikimedia.org/r/c/mediawiki/services/push-notifications/+/622376. (It looks large, but the number of lines of code affected is somewhat inflated by what amount to formatting changes.) I know we discussed your looking at the Echo extension first, so I just wanted to see if we still have at least a day or two to get this reviewed and merged before you move on to the push service. Thanks again.

@Mholloway - if you could get c622376 reviewed and merged this week, I could focus on the entire node service next week and hopefully complete the review by the end of next week. It would be best for me to review the code as close to what it will look like in production as possible, so having that code merged seems like a good idea.

Thanks, @sbassett! c622376 is now merged.

Security Review Summary - T246712 - 2020-09-04
Last commits reviewed:

• Echo: ce3e271f7734a742ffda0bf76c24a8e33bdba40e
• push-notifications: 6a9c691f94aaf35c2c90c5063b541b64b018edda

Summary

Overall, the relevant pieces incorporated into Echo look good - nothing particularly egregious that I found. I'd currently give that a risk rating of low.

The push-notifications service code looks good as well, with some issues detailed below. Given some of the vulnerabilities within various production packages, this will have an overall risk rating of medium.

Vulnerable Packages - Production

Outdated Packages
As reported via npm outdated:
(no explicit vulnerabilities reported, simply noting for completeness' sake.)
Risk: none

TLS

• I assume the push-notifications service will require TLS per the trend in T235411. Risk: low.

Security Headers

• Given that push-notifications will be a private, internal, RESTful service accessed only by Wikimedia production infrastructure, security headers aren't quite as much of a concern here. But it likely wouldn't hurt to tighten the defaults. media-src, img-src and style-src could all be set to 'none', I believe. I'd also recommend adding base-uri: 'self' to the content-security-policy header, and possibly removing x-webkit-csp and x-content-security-policy since those headers are deprecated. It also wouldn't be a bad idea to enable hsts if my assumption above that this service will be forced over TLS is correct. And finally setting access-control-allow-origin to whatever the internal hostname of the service ends up being. Risk: low.

Static Analysis Findings

• njsscan found an unvalidated regular expression on line 32 within test/utils/assert.js. This isn't much of a concern, given that 1) these are tests 2) the way it's used within features/app/spec.ts and features/info/info.ts is fine, it might be a good idea to sanitize expectedRegexString based upon expected values within the context of these tests, if possible (though regular expression sanitization can be difficult or even impossible at times). Risk: low.

Miscellanous Issues/Points of Discussion/Nits

• The README.md seems a copy of https://github.com/wikimedia/service-template-node/blob/master/README.md, which is fine, though I don't believe the routes are necessarily the same or that push-notification's (/v1/message/fcm, /v1/message/apns) have been documented. Maybe just a note to check spec.yaml? Risk: none

@sbassett, thanks again for the review. I believe that all comments have been addressed; the only remaining issue is an npm package with a low-severity vulnerability that is being pulled in by service-runner, which is probably out of scope for us here. Can we call this task resolved?