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)?

In T207798#4692173, @sbassett wrote:

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!

In T207798#4691566, @Aklapper wrote:

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?

In T207798#4704381, @sbassett wrote:

@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.

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.

In T207798#4714305, @sbassett wrote:

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.

In T207798#4716863, @Catrope wrote:

In T207798#4714305, @sbassett wrote:

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.

In T207798#4726572, @sbassett wrote:

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.

In T207798#4728590, @sbassett wrote:

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!