Page MenuHomePhabricator

Security Readiness Review For GlobalWatchlist extension
Closed, ResolvedPublic

Description

Project Information

Description of the tool/project: This extension creates a "global" watchlist display at Special:GlobalWatchlist, configurable at Special:GlobalWatchlistSettings and displayed via javascript. The watchlist is retrieved via the Watchlist API and is "global" in the sense that it can interact with multiple sites, but also works with a just a single site

Description of how the tool will be used at WMF: To be deployed on Meta as part of the grant

Dependencies MediaWiki core (1.35+), as well as CentralAuth and a wiki running WikibaseRepo (both optional, see below)

Has this project been reviewed before? No

Working test environment
Install MediaWiki core and this extension
Foreign wikis are handled the same as local ones, via mw.ForeignApi. To test this functionality, install CentralAuth and add a connected wiki to the list of sites to query
To test the special handling of wikibase wikis, set wgGlobalWatchlistWikibaseSite to that site's address and add it to the list of wikis to display (using Special:GlobalWatchlistSettings)

Post-deployment Contact @DannyS712

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Per https://www.mediawiki.org/wiki/Security/SOP/Security_Readiness_Reviews, I've submitted this task now, but it is not yet ready for review. Once the blockers at T260465: Submit extension for security review are fully resolved, I will update the description to link to the patchset for review, and add the target date for deployment as 30 days later. Filing now so that this is on the security team's radar, and in case there is anything else I missed that I can address now.
Additionally, I'll note that there are two different displays that can be used at Special:GlobalWatchlist; one uses jQuery and the other uses Vue. By default, the non-Vue version is used, as the new Vue version is still under development and, along with some missing functionality, doesn't look as nice. wgGlobalWatchlistUseVue can be set to true to use the Vue version. They use the same "backend" handling though.

Okay, this should be ready for review. I put in the target deployment date more than 30 days from now. It aligns with the grant requirement of allowing time for post-deployment support. Please let me know if any more information is needed.

sbassett triaged this task as Medium priority.Aug 20 2020, 4:12 PM

@DannyS712 - the Security-Team has some questions that are a bit outside of the standard security review process, but are somewhat tangential and partially-relevant in helping us schedule this review:

  1. Can you briefly describe the intended code stewardship plan if this extension is eventually deployed to wikimedia production?
  2. Is there currently any sponsoring wmf engineering team or collective of wmf staff to help shepherd this extension into wikimedia production and assist with code stewardship if it proves to be successful on the beta cluster?
  3. Given the above items and the remaining work at T260862, is September 21st still the target date for a beta deployment? Or was that originally the target date for wikimedia production deployment? If there are any updates to the timeline, please feel free to update the task description.

Thanks.

@DannyS712 - the Security-Team has some questions that are a bit outside of the standard security review process, but are somewhat tangential and partially-relevant in helping us schedule this review:

  1. Can you briefly describe the intended code stewardship plan if this extension is eventually deployed to wikimedia production?

Since my grant includes some post-deployment support, I plan to be the code steward myself at first

  1. Is there currently any sponsoring wmf engineering team or collective of wmf staff to help shepherd this extension into wikimedia production and assist with code stewardship if it proves to be successful on the beta cluster?

I am still waiting to be assigned a technical advisor, but for now @Legoktm is helping in their volunteer capacity

  1. Given the above items and the remaining work at T260862, is September 21st still the target date for a beta deployment? Or was that originally the target date for wikimedia production deployment? If there are any updates to the timeline, please feel free to update the task description.

Thanks.

That was originally the target date for production deployment, but then I realized that the performance review cannot take place until after this is done and its deployed to the beta cluster. I hope to deploy to the beta cluster as soon as this review is resolved

@sbassett is this still "waiting" for something else from me?

@sbassett is this still "waiting" for something else from me?

No, I'll move it back to Back Orders. I'm hopeful we can get this one scheduled for this quarter.

@Reedy do you have any updates on the security review? The target date for deployment has already passed.

Jcross changed the task status from Open to Stalled.Oct 7 2020, 4:05 PM
Jcross added a subscriber: Jcross.

@Niharika this is currently stalled and we will have a more complete update soon.

@Jcross Hello, may I ask what is this stalled on?

@Urbanecm - based upon the comments starting at T260860#6523535, at the very least we'd need to see a solution for T264833 before any security review commenced, since that blocks deployment of the extension. Have we also confirmed a Foundation-based steward of the code? Is Community-Tech or Anti-Harassment filling that capacity, @Niharika?

@Urbanecm - based upon the comments starting at T260860#6523535, at the very least we'd need to see a solution for T264833 before any security review commenced, since that blocks deployment of the extension. Have we also confirmed a Foundation-based steward of the code? Is Community-Tech or Anti-Harassment filling that capacity, @Niharika?

Thanks. I don't understand why a WMF steward is necessary through - many of the inter-wiki code (code that aggregates info from more than one wiki) doesn't have a WMF code steward and/or a WMF maintainer, CentralAuth being the most notable example. As opposed to CentralAuth, GlobalWatchlist isn't a critical extension, if it starts to be an issue (performance, security or similar), we can disable it without much hassle. Also, note Danny has a WMF grant to work on this extension, so he isn't exactly a volunteer here :-).

@Urbanecm - based upon the comments starting at T260860#6523535, at the very least we'd need to see a solution for T264833 before any security review commenced, since that blocks deployment of the extension. Have we also confirmed a Foundation-based steward of the code? Is Community-Tech or Anti-Harassment filling that capacity, @Niharika?

The performance review is usually done after the security review, and the only reason its possible to address T264833 now is that I noticed the creation of T264817 independent from any work on the GlobalWatchlist extension. I'm planning to address T264833 this week, but am I to understand that the security review hasn't been started yet?

Thanks. I don't understand why a WMF steward is necessary though

It isn't necessary per se, but is one of the inputs which helps our team (and others) determine a sense of both priority and probability of production deployment, as discussed within the security readiness review SOP.

many of the inter-wiki code (code that aggregates info from more than one wiki) doesn't have a WMF code steward and/or a WMF maintainer, CentralAuth being the most notable example.

Perhaps, though there is an implicit ownership of many critical Wikimedia extensions and services by Foundation teams, where at minimum, they could or would be tapped to support such an extension or service, especially under emergency circumstances. I'd note that half of the top 10 human contributors to CentralAuth are WMF/WMDE employees.

As opposed to CentralAuth, GlobalWatchlist isn't a critical extension, if it starts to be an issue (performance, security or similar), we can disable it without much hassle.

Sure, but this is hardly a desirable outcome.

Also, note Danny has a WMF grant to work on this extension, so he isn't exactly a volunteer here :-).

I would personally agree on this point, but a WMF grant doesn't necessarily imply that code stewardship concerns or paths to production deployment have been satisfactorily addressed to trigger other processes, unless such issues have been specifically described within the terms of a WMF grant.

The performance review is usually done after the security review

I do not believe that this is a correct assumption. It certainly is not documented anywhere of which I'm aware.

and the only reason its possible to address T264833 now is that I noticed the creation of T264817 independent from any work on the GlobalWatchlist extension.

This was noticed by the Security-Team as well.

I'm planning to address T264833 this week, but am I to understand that the security review hasn't been started yet?

The task was stalled on October 7th due to both previous prioritization conflicts and the discovery of T260860#6523535. Once T264833 is resolved (as an alleged critical blocker to deployment), this task will be moved to the In Progress column.

@Urbanecm - based upon the comments starting at T260860#6523535, at the very least we'd need to see a solution for T264833 before any security review commenced, since that blocks deployment of the extension. Have we also confirmed a Foundation-based steward of the code? Is Community-Tech or Anti-Harassment filling that capacity, @Niharika?

Hi @sbassett. We do not have WMF team as steward. However, @MusikAnimal, me and some others are willing to help out in our volunteer time. There is a well-known demand for the GlobalWatchlist extension - both from the community and staff. In an ideal world teams would have some operating capacity to support the incredible work our volunteers do. However given the existing workload our teams operate under, it is highly unlikely that any team would want to pick up the extra work of shepherding this through.

The performance review is usually done after the security review

I do not believe that this is a correct assumption. It certainly is not documented anywhere of which I'm aware.

Per https://www.mediawiki.org/wiki/Wikimedia_Performance_Team/Performance_Review, the prerequesite for a performance review is deployment to the beta cluster. Deployment to the beta cluster is dependent on the security review, thus until this security review is completed the performance review cannot begin. This is also the order used at https://www.mediawiki.org/wiki/Writing_an_extension_for_deployment#Preparing_for_deployment

See also T260860#6453192

Marking this as stalled for my own tracking of performance reviews that are currently actionable for us. Please move it back to "open" once the extension is running on Beta and we can test it there. Thanks!


I'm planning to address T264833 this week, but am I to understand that the security review hasn't been started yet?

The task was stalled on October 7th due to both previous prioritization conflicts and the discovery of T260860#6523535. Once T264833 is resolved (as an alleged critical blocker to deployment), this task will be moved to the In Progress column.

T264833 blocks deployment to production, but since performance reviews normally do not begin until after deployment to the beta cluster, my understanding is that T264833 is not a blocker to deployment to the beta cluster, while this security review is


Also, note Danny has a WMF grant to work on this extension, so he isn't exactly a volunteer here :-).

I would personally agree on this point, but a WMF grant doesn't necessarily imply that code stewardship concerns or paths to production deployment have been satisfactorily addressed to trigger other processes, unless such issues have been specifically described within the terms of a WMF grant.

The terms given at https://meta.wikimedia.org/wiki/Grants_talk:Project/DannyS712/Create_a_global_watchlist_extension#Round_1_2020_decision specifically describe (emphasis added)

Extension must go through proper security and performance review, with expectation that review may result in a "no go"

Hi @sbassett. We do not have WMF team as steward. However, @MusikAnimal, me and some others are willing to help out in our volunteer time. There is a well-known demand for the GlobalWatchlist extension - both from the community and staff. In an ideal world teams would have some operating capacity to support the incredible work our volunteers do. However given the existing workload our teams operate under, it is highly unlikely that any team would want to pick up the extra work of shepherding this through.

Ok, thanks for confirming.

Per https://www.mediawiki.org/wiki/Wikimedia_Performance_Team/Performance_Review, the prerequesite for a performance review is deployment to the beta cluster. Deployment to the beta cluster is dependent on the security review, thus until this security review is completed the performance review cannot begin. This is also the order used at https://www.mediawiki.org/wiki/Writing_an_extension_for_deployment#Preparing_for_deployment

I'd have to ask, if these are to be read as policies, why the performance review was started in T260860 prior to beta deployment? It seems that perhaps not everyone is of the same understanding as to what blocks what on the path to production deployment, which likely needs some revisiting. I'm also not sure I'd read the Preparing for Deployment documentation as requiring a security review prior to beta deployment, especially when many items within this list appear out of order (e.g. why, under any circumstance, would work be expended upon security and performance reviews prior to a design review and a product owner signaling acceptance?) These are, of course, not issues which you have personally created, but issues nonetheless.

T264833 blocks deployment to production, but since performance reviews normally do not begin until after deployment to the beta cluster, my understanding is that T264833 is not a blocker to deployment to the beta cluster, while this security review is

As the owners of the security readiness review process, the Security-Team requests that any code be in a stable state as near to production-deployable as possible. Since T264833 would be a blocker for production deployment, we would like the issue to be addressed prior to the commencement of any review. That the performance review is being performed out of order according to existing documentation is less an issue than the discovery of a production deployment blocker. Since the Security-Team cannot accommodate multiple security review requests of the same code as it mutates on its journey to production deployment, these guidelines are necessary to ensure the most effective and sanest security readiness review process possible.

For the record performance reviews are non-blocking, we just want to see something run somewhere before we review it. Beta is the de facto option, but we're not picky about where people want to run something currently in development (eg. on a WMCS machine, a fully self-contained Docker image that takes a single command to run, etc.). The criteria is that it should zero setup or a single command for us to try something. We also don't care about any particular order in regards to other reviews.

We encourage people to consult the performance team early in their development phase, which can be done without having to follow the perf review template. We do prefer to get a chance to review things before they go to production, as we frequently catch easily preventable issues early that can have site-wide effects if deployed unchecked. Nobody enjoys an emergency rollback of their publicly announced feature, so it's really in the teams' interest to have the review done before something hits production.

We also won't get mad if we review something that ultimately doesn't get the green light for production for a variety of reasons (security review being a possible one). Performance reviews are as much, if not more, about training and sharing knowledge as they are about catching performance issues.

The performance review is usually done after the security review

I do not believe that this is a correct assumption. It certainly is not documented anywhere of which I'm aware.

https://www.mediawiki.org/wiki/Writing_an_extension_for_deployment#Preparing_for_deployment says 2.1: Security review; 2.3 Performance review.

I'm not sure that the list on that wiki page should be a numbered/ordered one.

All - it appears the ordering stems from an edit about 4 years ago, quite a bit prior to several changes which were made to our security readiness review service, currently governed by this SOP. I'm going to try out an edit for the Preparing for deployment section which changes the list to a <ul>, removes the security review beta deployment "requirement" and attempts to provide a few additional, helpful notes.

Update: edit here. I would of course welcome feedback on this if anything seems amiss.

Hello! Is there a rough estimate of when we think the security review might be completed? This grant project is a bit behind schedule (which is fine :). I'm more curious than anything. Thank you!

HI @MusikAnimal - we've had a few bumps in the road recently and we do plan to complete it this quarter. We'll be in touch with any questions or concerns and we apologize for the delay.

HI @MusikAnimal - we've had a few bumps in the road recently and we do plan to complete it this quarter. We'll be in touch with any questions or concerns and we apologize for the delay.

Do you know when during the quarter? The grant was originally scheduled to be done at the end of November

@DannyS712 I've just spoken with @Reedy and at this point "by the end of the quarter" is what we are in a position to commit to. We appreciate that the grant was scheduled for the end of November, but I'm afraid we are still limited by pandemic resourcing, such as it is.

Reedy moved this task from In Progress to Our Part Is Done on the secscrum board.

So first off, I'm sorry this has taken as long as it has. As mentioned, resources are limited, and other things have taken priority.

I'm still not overly happy about the lack of official WMF steward (specifically a team).

Being a grant doesn't mean something is automatically eligible to be deployed (even if it's wanted by the community; they want numerous things deploying that are never going to happen). The Grants Team/Department doesn't necessarily have control/influence to make other WMF teams do things either, nor should they be making that decision or constraint on a grant - it's possible someone writes good code, but it could technically never be deployable, for various reasons. Stretch goal of "get deployed on Wikimedia Wikis"? Sure.

A few people have spoken up willing to help in their volunteer time, but that doesn't really solve the problem properly for the longer term. We can't make/force volunteers do anything. And just because "we've always (or previously) done it that way" doesn't make it a good reason to continue doing so. Numerous of the unmaintained, but actually used extensions are technical debt, and do on occasion cause problems for WMF production.

We've seen it before where people have been paid (grants, employment or whatever) who have no interest in maintaining their stuff after they're not being paid any more. Which makes some sense, but also goes against what may have been said before.

Now I get off my soapbox a little. I think there are enough people who have offered to steward this (on the WMF side), and with their teams/positions, probably can find some time to work on it officially if it's urgent enough (and necessary). Or the extension gets (temporarily, or even potentially permenantly) turned off if the need arises. This obviously still carries some risk, but having at least 3 or 4 other people (from what I've seen) "happy to help", it does help mitigate that.

There's nothing particularly of obvious concern for me security wise. I imagine issues in terms of performance/scalability may be more likely, but Gilles has picked up on a few things (that have seemingly been addressed).

It's unclear if $wgGlobalWatchlistUseVue is destined to be turned on intially, or later. Which isn't so much of an issue at the moment, as other Vue using code is still WIP AIUI (T257734 and T257579 for example). I guess at some point when Vue becomes more default in MW, that will go away, and the number of code paths/different JS files will be reduced.

Similar for $wgGlobalWatchlistSiteLimit; definitely a potential bottleneck waiting to happen if set too high (at some later date). But the default seems sane enough for the time being.

It's unclear if $wgGlobalWatchlistUseVue is destined to be turned on intially, or later. Which isn't so much of an issue at the moment, as other Vue using code is still WIP AIUI (T257734 and T257579 for example). I guess at some point when Vue becomes more default in MW, that will go away, and the number of code paths/different JS files will be reduced.

Hopefully later once the Vue version looks better

Similar for $wgGlobalWatchlistSiteLimit; definitely a potential bottleneck waiting to happen if set too high (at some later date). But the default seems sane enough for the time being.

Any patch to change will probably be discussed with the performance team too


Thanks for the review!