Page MenuHomePhabricator

Security review for GrowthExperiments extension
Closed, ResolvedPublic

Description

Project Information

  • Name of tool/project: GrowthExperiments
  • Project home page:
  • Name of team requesting review: Growth
  • Primary contact: @Catrope
  • Target date for deployment: no earlier than Nov 13
  • Link to code repository / patchset: mediawiki/extensions/GrowthExperiments.git and this patch (which contains the bulk of the code, and will be merged soon if it hasn't been already)

Description of the tool/project

Extension to group together experiments and instrumentation done by the Growth team to understand what new editors do, and what does and doesn't work to retain them.
Initially, this will just contain an optional survey that's shown to new users right after they create an account. The results will be stored as a userjs- preference.

Description of how the tool will be used at WMF

Surveying new users on Korean and Czech WIkipedias to find out what motivates them to create an account

Dependencies

None

Has this project been reviewed before?

No, other than patch reviews in Gerrit by @Catrope

Working test environment

wfLoadExtension( 'GrowthExperiments' );
$wgWelcomeSurveyEnabled = true;

Post-deployment

The Growth team will remain responsible for this extension, with @Catrope and @SBisson being the primary contacts. The extension will only be deployed to cswiki and kowiki, and possibly to one more wiki later, but we don't expect that we'll want to deploy this to more than a handful of wikis.

Event Timeline

Catrope created this task.Oct 23 2018, 8:58 PM
Restricted Application added subscribers: Urbanecm, revi, Aklapper. · View Herald TranscriptOct 23 2018, 8:58 PM
Catrope moved this task from Inbox to External on the Growth-Team board.

Target date for deployment: week of October 29th

Uh? Is there a deployment task under Wikimedia-extension-review-queue as per https://www.mediawiki.org/wiki/Review_queue (plus docs, etc)?

sbassett claimed this task.Oct 24 2018, 3:56 PM
sbassett triaged this task as Normal priority.Oct 24 2018, 4:49 PM

Hey @Catrope - I should be able to look at this soon and have a review by the 29th for you. I'll focus on the patch (469211) and let you know if I have any questions. Thanks.

Hey @Catrope - I should be able to look at this soon and have a review by the 29th for you. I'll focus on the patch (469211) and let you know if I have any questions. Thanks.

Thanks!

Target date for deployment: week of October 29th

Uh? Is there a deployment task under Wikimedia-extension-review-queue as per https://www.mediawiki.org/wiki/Review_queue (plus docs, etc)?

Not yet, creating those now. I wanted to get the security review task up first, because that's the part I have the least control over.

@Catrope- still hope to have this completed by tomorrow or this Weds (apologies - recent security incident fallout). One initial step I was reminded of by @Bawolff that is quasi-security-related - have the new privacy and data retention policy statements (for surveys, etc.) been approved by legal yet?

@Catrope- still hope to have this completed by tomorrow or this Weds (apologies - recent security incident fallout). One initial step I was reminded of by @Bawolff that is quasi-security-related - have the new privacy and data retention policy statements (for surveys, etc.) been approved by legal yet?

Ping @MMiller_WMF, he's the one working on that.

Also, I apologize for this code being more in flux than I thought it would be. When I filed this task I thought it was pretty much ready, but it's changed a lot since then.

Catrope updated the task description. (Show Details)Oct 29 2018, 10:20 PM
MMiller_WMF added a comment.EditedOct 29 2018, 10:26 PM

Thanks for helping with this, @sbassett. We are finalizing a "survey privacy statement" with Legal now. This should be complete this week, and we plan to translate it into Czech and Korean so that it can be linked from the feature on those wikis.

There is also the Privacy Policy itself, which is currently partially translated into Czech and Korean. We also are considering fully translating those, but still working on the plans, timelines, and details for that.

Does this answer your question? Are those the policy statements to which you are referring?

@MMiller_WMF Ok, sounds good. I believe a local Privacy Policy like this would override the standard Terms of Use, Cookie statement and Data retention guidelines, which should be fine. Thanks.

Apologies, but this is going to be delayed a bit due to other ongoing issues. Still hopeful I can turn it around sometime soon.

Catrope updated the task description. (Show Details)Nov 2 2018, 6:45 PM

Apologies, but this is going to be delayed a bit due to other ongoing issues. Still hopeful I can turn it around sometime soon.

No worries; for unrelated reasons, we will not be able to deploy this earlier than the week of Nov 12 anyway.

Apologies, but this is going to be delayed a bit due to other ongoing issues. Still hopeful I can turn it around sometime soon.

No worries; for unrelated reasons, we will not be able to deploy this earlier than the week of Nov 12 anyway.

We would really like to have the extension in betalabs and prod (dark launch) a good week before that date though.

fwiw, I'm not seeing anything egregious within r/469211. I've got the extension enabled in mw-vagrant and am going to pen-test a bit more. Should have full results early next week.

This will be completed today. I spent most of yesterday reviewing it and just have a couple more items I wanted to check. So far nothing major to report.

Security Review Summary - November 2018
Overall, this extension looks pretty good. No PHP or JS package/module vulnerabilities as reported by npm audit and security-checker. No DoS vectors or http leaks. And it looks like many of the items elicited here and here are fine or a non-issue for this extension. I did find a few minor things noted below:

Final Links/URLs
I'd like to confirm the final URLs for the Privacy Policy, Tutorial and Help Desk links once those are ready to go.

Odd Config Issue
When I enable both:

$wgWelcomeSurveyEnabled = true;
$wgWelcomeSurveyExperiments = true;

I get some nasty PHP warnings/errors. I imagine these $wg vars may never be enabled at the same time, but perhaps these errors could be handled in a cleaner way?

Possible XSS Issue
Per https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/core/+/refs/heads/master/includes/Title.php#2069, it seems like getLinkUrl() on lines 190 and 219 of includes/Specials/SpecialWelcomeSurvey.php should be sanitized, probably via htmlspecialchars(..., ENT_QUOTES).

Finally, if their are any new patch sets for this prior to launch, if you could reference on this task for further review, that would be great.

Hi @sbassett, thanks for getting this done quickly.

Odd Config Issue
When I enable both:

$wgWelcomeSurveyEnabled = true;
$wgWelcomeSurveyExperiments = true;

I get some nasty PHP warnings/errors. I imagine these $wg vars may never be enabled at the same time, but perhaps these errors could be handled in a cleaner way?

$wgWelcomeSurveyExperiments, now renamed $wgWelcomeSurveyExperimentalGroups, is not a boolean but an object structure containing the definition of the experimental groups. This is documented in extension.json. We generally don't do defensive programming for config variables being assigned invalid values. Unless of course there's a specific risk involved.

Possible XSS Issue
Per https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/core/+/refs/heads/master/includes/Title.php#2069, it seems like getLinkUrl() on lines 190 and 219 of includes/Specials/SpecialWelcomeSurvey.php should be sanitized, probably via htmlspecialchars(..., ENT_QUOTES).

The return value from getLinkUrl() is being used in Html::linkButton() and Html::element(). Both end up calling Sanitizer::encodeAttribute(), which does htmlspecialchars( ..., ENT_QUOTES );

Finally, if their are any new patch sets for this prior to launch, if you could reference on this task for further review, that would be great.

There's a CSS-only patch coming up. And probably several fine-turning patches between betalabs and prod deployments since more people will be able to test the feature. Does it work if I add you as a reviewer in gerrit on those?

@SBisson -

Thanks for the update and info - still learning my way around some of these components. This all sounds fine. And yes, if you wouldn't mind adding me as a reviewer when any additional patches are submitted to gerrit prior to launch, that'd be great.

Thanks for the update and info - still learning my way around some of these components.

Me too... after many years.

And yes, if you wouldn't mind adding me as a reviewer when any additional patches are submitted to gerrit prior to launch, that'd be great.

I'll do that.

@Catrope, looks like we're ready to put the extension in betalabs!

sbassett closed this task as Resolved.Nov 13 2018, 4:03 PM