Page MenuHomePhabricator

Security review for TheWikipediaLibrary extension
Closed, ResolvedPublic

Description

Project Information

Description of the tool/project

Notifies a user when they qualify for the Wikipedia Library. The notification is sent upon the user's first edit when their global edit count is at least 500 and their account age is at least 6 months. The extension uses a hidden global preference to remember whether the user has already received a notification, so they get notified only once, not for every edit after they qualify.

Description of how the tool will be used at WMF

To educate users about the Wikipedia Library project and resources available through it, once they qualify

Dependencies

  • CentralAuth (for getting the user's global edit count and the registration time of their global account)
  • GlobalPreference (for remembering which users have already been notified)

Has this project been reviewed before?

Individual patches were reviewed by myself and Stephane Bisson, but nobody outside the team has reviewed the extension

Working test environment

wfLoadExtension( 'TheWikipediaLibrary' );
// For easier testing you might consider:
$wgTwlEditCount = 2; // send notification after two edits
$wgTwlRegistrationDays = 1; // require accounts to be one day old

Post-deployment

Growth team / @Catrope

Event Timeline

Not a security thing, but you should get it setup using phpcs etc, as I notice until just recently it was still using array() not []

Objects don't need & in function definitions, they're always passed by reference

Not a security thing, but you should get it setup using phpcs etc, as I notice until just recently it was still using array() not []

I've added phpcs in this change: https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/TheWikipediaLibrary/+/470524

Objects don't need & in function definitions, they're always passed by reference

They're passed by reference (using ampersands) by the hook caller though, so don't they also need ampersands in the hook callbacks to avoid warnings?

sbassett subscribed.

I'll start on this. @Catrope - just to confirm, everything there is for this extension is already at: https://github.com/wikimedia/mediawiki-extensions-TheWikipediaLibrary (aside from Flow, which it integrates with)? No pending or near-future commits atm? Thanks.

I'll start on this. @Catrope - just to confirm, everything there is for this extension is already at: https://github.com/wikimedia/mediawiki-extensions-TheWikipediaLibrary (aside from Flow, which it integrates with)? No pending or near-future commits atm? Thanks.

There is one minor patch that isn't merged yet (and I need to rebase it): https://gerrit.wikimedia.org/r/c/mediawiki/extensions/TheWikipediaLibrary/+/371516 . But that shouldn't affect your review. No other work is planned.

It also doesn't integrate with Flow, but with Echo (and it depends on CentralAuth and GlobalPreferences).

sbassett claimed this task.
sbassett added a subscriber: mmarble.

Hello @Catrope -

Sorry for the crazy delay on this, but this extension looks fine to me. Some phpcs issues came up during my review, but they aren't security-related, so I was going to let you review those in CI, etc. Let me know if you have any other questions, etc. Thanks.

@Catrope - just to follow up on this, while the extension does look fine, there are some interesting issues around wikipedialibrary.wmflabs.org. Nothing show-stopping at the moment, but if you'd like some further information on this, feel free to contact me or security-team@.