Hi @CKoerner_WMF - we just wanted to touch base as this is noted as a November target deployment date. With all of the holidays this month and a somewhat reduced capacity, we are looking to complete this towards the end of the month. We hope that works for your timeline?

@Jcross, understandable. Our timeline is flexible.

Adding a note here that I've spoken to our developer and he is ready for a security review. As mentioned above, the code can be found at the following repo (let me know if you need access).

https://github.com/wpcomvip/wikimedia-blog-wikimedia-org/tree/develop/plugins/mw-oauth-client

You can test this feature on our development instance at https://blog-wikimedia-org-develop.go-vip.net/wp-login.php

Use the "Login with MediaWiki" button to test

Hi @CKoerner_WMF - we're revamping our scheduling process a bit to provide more clarity and transparency around our workload management and prioritization process. You'll see that we've placed you into queue for this quarter: https://phabricator.wikimedia.org/tag/secscrum/ and @sbassett or @Reedy will be in touch with any questions or concerns. Thanks for your patience as we work through this, and please feel free to reach out at any time!

In T266510#6720231, @CKoerner_WMF wrote:

Adding a note here that I've spoken to our developer and he is ready for a security review. As mentioned above, the code can be found at the following repo (let me know if you need access).

https://github.com/wpcomvip/wikimedia-blog-wikimedia-org/tree/develop/plugins/mw-oauth-client

Couple of requests....

Is this the canonical repo for developing this plugin? It's hard to tell from https://github.com/wpcomvip/wikimedia-blog-wikimedia-org/commits/develop/plugins/mw-oauth-client, but it looks like the code isn't being imported from elsewhere, the development has been done on this repo straight here? Or at least, not in a public repo.

If so, we need a composer.json and a composer.lock file commiting (just installed.json isn't enough). We need to be able to audit what code is there and what versions of vendored code is being brought in. One to confirm it's not being modified (unlikely, but it's not impossible), and two to be able to do updates and upgrades in future; both for features (PHP 8.0 support) and bugs (security and otherwise). Especially if people that aren't the original developer do that.

If it is being developed in another repo, can we get access to that?

While we do use numerous of these libraries already in MediaWiki/Wikimedia Deployment, I don't need to "review" them, but I need to be able to verify what is there. Especially as some of it is auth-related code, bugs can happen, and fixes will be released. We might need to apply some of them "quickly".

Non security related: Why is the license "Proprietary"? What does that actually mean? Is this going to change? Can it change?

It seems if Wikimedia is paying for this to be developed (which is seemingly what has happened?), we really should be open sourcing it for others to use; I imagine others might want to use it.

Hi @Reedy! Let me try my best.

Is this the canonical repo for developing this plugin?

Yes that where the work is happening. It is not in the public repo for Diff (https://github.com/wikimedia/diff-blog) but upon deployment will be (to be transparent and so other can benefit from it).

If so, we need a composer.json and a composer.lock file commiting

Ok, cool. I can ask our developer to add that.

Non security related: Why is the license "Proprietary"? What does that actually mean? Is this going to change? Can it change?

That appears to be a default and mistake. I'll ask the developer to clarity. Everything should be open-source and the license information should properly reflect that.

Brad has made the changes regarding licensing (and is also now subscribed to this task). :) 👋

https://github.com/wpcomvip/wikimedia-blog-wikimedia-org/tree/feature/mediawiki-sso/plugins/mw-oauth-client

In T266510#6773680, @CKoerner_WMF wrote:

Brad has made the changes regarding licensing (and is also now subscribed to this task). :) 👋

https://github.com/wpcomvip/wikimedia-blog-wikimedia-org/tree/feature/mediawiki-sso/plugins/mw-oauth-client

Thanks, but to be awkward... I *think* you need to specify the actual version of the GPL to be compliant too

* @license     http://opensource.org/licenses/gpl-license.php GNU Public License
* @copyright   Wikimedia Foundation

As per http://opensource.org/licenses/gpl-license.php, it needs to be labelled as version 2 or 3, and potentially "or later"

https://www.gnu.org/licenses/gpl-howto.en.html

I have updated feature/mediawiki-sso with appropriate license blurbs in each source file following the same convention as MediaWiki core. COPYING file has also been added to the root and composer.json _updated_ to GPL2.0 in line with MediaWiki core.

license lines have been removed from class-level docblocks as per PHPDoc recommendations: https://docs.phpdoc.org/3.0/guide/references/phpdoc/tags/license.html

@Reedy et al, are Brad's license updates satisfactory? Anything else needed from our end?

@CKoerner_WMF Looks like we are all set for now and @Reedy expects to finish next week. Have a great weekend :)

Security Review Summary - T266510 - 2021-03-01
Last commit reviewed: a1194b2

Overall, the code looks reasonable, and is using a popular/well used OAuth library (league/oauth2-client). I note the caveat that I am not a WordPress developer, and am therefore not familiar with various WP-isms, and the ways they do some of the stuff. I've tried to avoid leaving comments on most things that fall under that banner. I would however currently rate the overall risk as: medium.

As per T249039#6309061, medium requires management level acceptance of the risk.

The code is definitely a lot better than the previous plugin referenced in the aformentioned task at T257335. And the addition of the composer.json/composer.lock file help with the auditability of the 3rd party libraries brought in via the plugin itself.

My only real concerns are the fact that this is authentication related code, and what the longer term maintenance and support goals of the code are. As mentioned below (though, currently aren't really relevant from a security pov), there are already "outdated" php/composer libraries in the folder, and at some point updates such as those will need to be brought in, both for general code maintenance but potentially also for security purposes. Then there are also WordPress core changes. Both of which may result in potentially non insignificant changes needed to the plugin. If this was MediaWiki (and more so had a WMF technical team attached to it), this would be a less of a concern, as that would be their responsibility and part of their more day to day work.

The newer versions of the league/oauth2-client library mostly allow PHP 8.0 support, which I'm sure will be needed down the road. A bump isn't necessary currently however.

paquettg/php-html-parser

I'm curious why an old version of paquettg/php-html-parser (and dependancies) are being used? As per https://github.com/paquettg/php-html-parser/blob/f5c2dd9/SECURITY.md

We only support the most recent version with security fixes.

So, the version included will not be "supported". From what I can see, the latest release, 3.1.1 was released after the "intial commit" to the plugin folder, but 3.1.0 was a month or so before; and certainly 3.0.0/3.0.1 were out before too.

Having said all that, I cannot see where paquettg/php-html-parser is even used in the Plugin code itself. No use of the PHPHtmlParser namespace, only within the vendor/paquettg folder.

If it was previously used, but is no longer, please remove it from composer.json, run composer update to update composer.lock and the vendor folder, and commit/push that. That then remove those concerns, and reduces any potential attack/vulnerability surface as we're not including code we don't use.

Non security

I would suggest longer term that the plugin is probably moved to its own git repository (using git filter branch/similar) for development/maintenance purposes, rather than being embedded inside what is effectively a deployment repo as the primary/canonical. If it is intended for reuse, that would be more helpful for 3rd parties. And while I'm speculating, it seems it might have other uses within the Wikimedia movement in the near future wrt T275454.

Completely unrelated to security, it wouldn't be the worst thing if it was eventually registered in https://wordpress.org/plugins/ too, proably after it's been put into its own repo :).

Couple of minor license related copy paste issues in https://github.com/wpcomvip/wikimedia-blog-wikimedia-org/blob/feature/mediawiki-sso/plugins/mw-oauth-client/COPYING too.

• MediaWiki OAuth Client uses the following Creative Commons icons to illustrate links to the CC licenses - no it doesn't :)
• MediaWiki OAuth Client contributors, including those listed in the CREDITS file, hold the copyright to this work. - there's no CREDITS file.

The file doesn't need to be in wikitext. It's probably easiest to just replace most of the current contents with https://www.gnu.org/licenses/old-licenses/gpl-2.0.txt

There's a bit of other cleanup probably useful around the code. Unnecessary use statements, unused class/member variables etc.

Also, while the WP related i18n/l10n wrapper functions like __() are used... everything is still in English :). And in some cases, stuff is hard coded into English in help text like in render_help_tab() for example, not using the i18n/l10n wrapper functions.

For the stuff that is already ready to be localised, I guess it would be helpful for Brad to chip in with whatever else might be needed to make that doable, and maybe we can see about getting the plugin into translatewiki so we can get it translated.

Related to forking to a seperate repo etc... There's then benefits of things like some amount of extra CI... And maybe tests? Obviously it's harder code to test with the workflow of this sort of thing. But linting, static analysis etc. These add to the risk, and are stuff that would be non negiotiable in WMF production. I've no idea what the WPVIP CI jobs actually do; the teamcity urls don't load.

Relatedly; https://oauth2-client.thephpleague.com/providers/implementing/ - "Make It Official" says "Before new provider clients will be accepted, they must have 100% unit test code coverage" etc. Maybe something we might want to consider at some point.

I don't know how much of that is out of/in scope of the original contract :). But I do know that we general value things being internationalised/localised, because not everyone speaks/reads English.

I have left a few comments on github too about a couple of bits and pieces, mostly on https://github.com/wpcomvip/wikimedia-blog-wikimedia-org/commit/58499e20e91e9807a224975d711f04d3d4c8dc51

There is some odd code (as I see it), but hard to know what is common/expected/normal in WP land.

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

guzzlehttp/guzzle        7.0.1    7.2.0     Guzzle is a PHP HTTP client library
guzzlehttp/promises      v1.3.1   1.4.0     Guzzle promises library
guzzlehttp/psr7          1.6.1    1.7.0     PSR-7 message implementation that also provides common utility methods
league/oauth2-client     2.5.0    2.6.0     OAuth 2.0 Client Library
paquettg/php-html-parser 2.2.1    3.1.1     An HTML DOM parser. It allows you to manipulate HTML. Find tags on an HTML page with selectors just like jQuery.
paquettg/string-encode   1.0.1    2.1.1     Facilitating the process of altering string encoding in PHP.
paragonie/random_compat  v9.99.99 v9.99.100 PHP 5.x polyfill for random_bytes() and random_int() from PHP 7

Thanks Reedy. It's not often we have code reviewed by a skilled third party and it's certainly an invaluable process. I appreciate your time.

Please see below for my comments/changes based on what would certainly be within the scope of the original contract.

Composer Packages
paquettg/php-html-parser has been removed from the composer package. This is not used in the plugin and is (rather embarrassingly) simply left over from a previous project. This plugin is based on a generic framework I have built for WP Plugins and it looks like some errant files were still included in the skeleton.

GitHub Comments
I have actioned, and responded to, all comments in GitHub. There was one comment related to a WP quirk however the rest were completely valid and have been rectified.

i18n
I will work to ensure that any English text hard-coded in the plugin is wrapped in i18n functions, however creating and providing translations is beyond the scope of this project. i18n functions are written in the code as a matter of convention (and to please the linter) rather than a specific goal within the scoped contract.

Git Repository
We are using the Diff repository for purposes of testing and development. WPVIP's infrastructure does not seem to like submodules and so we have bundled the code directly in the repo during the development phase. I have no concerns about moving this to its own repo upon completion of the project and do second that recommendation as it is certainly more manageable in the long term.

General comments...

Linting/CI

• Linting follows the standard WP coding style guidelines. It is not typical for a WordPress plugin of this size to include linting rules directly in the plugin itself but can be done.
• CI/Testing is beyond the scope of the agreed project. Unit testing should be a standard convention however it is not something that is well adopted in the WordPress space.

Both of these would be out of scope of the original agreement at this stage, however I would be happy to advise on how best to achieve the above in the realm of a WP plugin once the original scope is completed.

In T266510#6873347, @Bradmorris82 wrote:

Thanks Reedy. It's not often we have code reviewed by a skilled third party and it's certainly an invaluable process. I appreciate your time.

Please see below for my comments/changes based on what would certainly be within the scope of the original contract.

Composer Packages
paquettg/php-html-parser has been removed from the composer package. This is not used in the plugin and is (rather embarrassingly) simply left over from a previous project. This plugin is based on a generic framework I have built for WP Plugins and it looks like some errant files were still included in the skeleton.

Thanks!

In T266510#6873347, @Bradmorris82 wrote:

GitHub Comments
I have actioned, and responded to, all comments in GitHub. There was one comment related to a WP quirk however the rest were completely valid and have been rectified.

One more left; seems I to forgot to leave it originally

In T266510#6873347, @Bradmorris82 wrote:

i18n
I will work to ensure that any English text hard-coded in the plugin is wrapped in i18n functions, however creating and providing translations is beyond the scope of this project. i18n functions are written in the code as a matter of convention (and to please the linter) rather than a specific goal within the scoped contract.

Yeah, I wasn't saying you need to create or provide translations (well, beyond the en strings of course). We're very good at that part :).

If you can just make sure it's in a good position to translate, that would be appreciated. I guess mostly using the correct functions probably helps do most of that, but there are a couple of things that probably still need addressing (as you confirm).

For reference, generally in MW we use key value pairs in json files like https://github.com/wikimedia/mediawiki/blob/master/languages/i18n/en.json; the key is used in the code, and then for translations, the same basic file, just with the value being in another language.

I know our friends over at translatewiki.net can support various platforms and formats, so we might just need a bit of a nudge in the right direction to finish that off later. Certainly not a blocker to deployment, but something that I know the Wikimedia movement will see value in longer term.

In T266510#6873347, @Bradmorris82 wrote:

Git Repository
We are using the Diff repository for purposes of testing and development. WPVIP's infrastructure does not seem to like submodules and so we have bundled the code directly in the repo during the development phase. I have no concerns about moving this to its own repo upon completion of the project and do second that recommendation as it is certainly more manageable in the long term.

There's certainly a few ways of doing that, I guess. Potentially using composer itself, or even "just" using a git export/tarball extracted into the repo is probably better long term, so we're then copying into the working copy.

Little out of scope. Stuff I can probably help advise further on later on too.

In T266510#6873347, @Bradmorris82 wrote:

Linting/CI

• Linting follows the standard WP coding style guidelines. It is not typical for a WordPress plugin of this size to include linting rules directly in the plugin itself but can be done.
• CI/Testing is beyond the scope of the agreed project. Unit testing should be a standard convention however it is not something that is well adopted in the WordPress space.

Both of these would be out of scope of the original agreement at this stage, however I would be happy to advise on how best to achieve the above in the realm of a WP plugin once the original scope is completed.

I'm not saying we need to make our own up, or even necessarily use the MediaWiki ones; it doesn't make much sense, and it's generally best to work with the ecosystem, not fight against it. Things like yoda conditionals irk me, but I know (from seeing it before), that WP actively encourage it, so hence I didn't highlight it at all.

Like I say, I have no visibilty into the CI that is being run (beyond obviously being no tests in the specific plugin). Well, other than the CI has marked it ok. So somewhat of a black box. It could just be a return true; for all I know :). Any idea if it's supposed to be accessible by random people? It just times out for me.

Oh, and as a minor aside...

WPVIP is definitely running on PHP 7+ isn't it?

If so, paragonie/random_compat isn't needed (as per the description PHP 5.x polyfill for random_bytes() and random_int() from PHP 7), so it's mostly filler (as you can see if you look in vendor). Add the below to the composer.json, and update/commit/push again.

	"replace": {
		"paragonie/random_compat": "9.99.99"
	}

Translating
WordPress uses Gettext/compiled POMO files for translation and so I've added a sample EN_us catalog in the /lang/ folder following WP convention. That should be everything a WP-competent translator needs. Will push later on tonight/tomorrow.

Git Repo
WP VIP does offer the ability to build an image of the site using an external CI service, but it does seem the "VIP" way is to update your plugins locally (i.e. copy and paste files) and then push the changes up. Thats how the current diff site is working as far as I can tell and I'd probably just advise keep going with that. We'll move the plugin into its own repository once dev is complete and just manually copy the files upstream into Diff's repo. @CKoerner_WMF may have more insight into that process

CI
I haven't been able to load the CI service either. I'm certainly not an authority figure on the VIP platform but I believe their 'CI' simply runs PHPCS and a few other extremely basic checks. They do have workarounds to allow you do integration & testing through external services and have it deployed. The 'CI' you see on GitHub does seem to be a very loose use of the idea of 'CI' and is mainly just linting against the WordPress code sniffer standards

I have removed random_compat from composer

As per T249039#6309061, medium requires management level acceptance of the risk.

I've spoken to my manager, Mayur, and he's ready to accept the risk to move this forward. What's the appropriate way for him to sign off on the risk associated with this plugin? Is there a form or some process we need to fulfill?

Spoke to Chris who briefed me on this and happy to provide management-level acceptance of the risk.