Page MenuHomePhabricator

Security Review Request for WikimediaApiPortalOAuth Extension
Closed, ResolvedPublic


Project Information

Description of the tool/project:

We are developing a publicly accessible API portal. The work is described by the API Gateway documentation plan.

As part of this project we will be launching a new wiki on which we plan to use the WikimediaApiPortalOAuth extension.

The WikimediaApiPortalOAuth extension is designed to connect the API Portal with the OAuth server on Meta-Wiki. It provides a user interface to create and manage OAuth 2.0 clients.

To create and manage OAuth 2.0 clients, use the Special:AppManagement page. The extension supports creating two types of OAuth 2.0 clients:

  • owner-only clients
  • clients with authorization code, client credentials, and refresh token grant types

Has this project been reviewed before?

Working test environment
Local env setup is described in the docs sections on Installation, Configuration and Usage

The extension is also available on the Beta API Portal Wiki

@apaskulin can facilitate account creation if need.

Platform Engineering will own the extension.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
sbassett changed the task status from Open to Stalled.Jun 17 2020, 4:00 PM
sbassett moved this task from Incoming to Back Orders on the secscrum board.
sbassett added a subscriber: sbassett.

Backlogging and stalling for Security-Team for now.

@sbassett The OAuth Extension final patch is ready. Is it appropriate for the review to happen on the patch or should we proceed to merging first?

@sbassett The OAuth Extension final patch is ready. Is it appropriate for the review to happen on the patch or should we proceed to merging first?

I would suggest fixing it up and merging it first.

There's some changes that don't need to block a review, some are just nit picks... But some changes do change how/where messages are used, which might change escaping, parameters etc. Shouldn't be much work to get it somewhere near

Obviously we don't want to give translators extra work, but I don't think translation has been enabled on the repo yet, so some churn probably isn't an issue at this point, as some message changes have been detailed on the CR by Alex and I.

@WDoranWMF - As @Reedy implied above, we'd like the code to be as close to production-deployable as possible before we expend cycles on a formal security review. The patch doesn't have to be merged IMO, but it should be ready to have the Submit button clicked and then ride the train.

sbassett triaged this task as Medium priority.Jul 8 2020, 3:20 PM

@sbassett Great, thank you. We're working on ensuring it is in that state now. I will update when we have completed all steps up to merge.

Jcross raised the priority of this task from Medium to High.EditedAug 10 2020, 4:50 PM
Jcross added a subscriber: Jcross.

Changing priority per @Naike 's request, noting that this does not move the ticket forward for the Security team. It is my understanding that we are still waiting for steps to be completed, as mentioned in previous comments.

Just as brief status update:

Based on advice from Security on T261358 we have made changes to REST framework based on T261696 and the WikimediaApiPortalOauth Extension on 597553.

Remaining for us to is to complete review on T261696 and 597553.

Hey @WDoranWMF Is the extension in a stable reviewable state?

WDoranWMF changed the task status from Stalled to Open.Oct 5 2020, 4:41 PM
WDoranWMF updated the task description. (Show Details)
WDoranWMF added a subscriber: apaskulin.

@Jcross Sorry for delay, meant to do this last week. Have now updated the tickets and made it conform to the correct Security structure.

Let me know if I have elided or missed anything. Thanks!

WDoranWMF lowered the priority of this task from High to Medium.Oct 5 2020, 4:49 PM

Hey @WDoranWMF - this looks good and we hope to get it assigned shortly.

Jcross moved this task from Back Orders to In Progress on the secscrum board.

Change 639925 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/WikimediaApiPortalOAuth@master] Disable User JS on Special:AppManagement

Nit picks/Minor issues, not security (just generic CR so not blocking of this) were dumped into T267493: WikimediaApiPortalOAuth nit picks/minor CR

Security issue:

  • The Special Page allows execution of arbitary User JS.

Therefore, the Special Page should call $this->getOutput()->disallowUserJs(); to disable user JS. Especially as we're passing/outputting wgWikimediaApiPortalUserEmail/userEmail... User JS could be a fun way to pick that up (and use for whatever means), along with oauth tokens/input etc.

I know editing of pages on the wiki is locked down... But better safe than sorry. Incase that does change in future!

Patch going up before I submit this comment. Risk is low, and easily mitigated; but patch should be merged (and backported as appropriate) before wider enabling of the extension in WMF production

  • CORS breaks the extension on Beta (in FF 82.0.2). Presumably prod will have the same/similar issue (though, potentially at a later date)

Screenshot 2020-11-08 at 17.45.59.png (342×2 px, 180 KB)

Thanks, @Reedy! I really appreciate the feedback on T267493.

Change 639925 merged by jenkins-bot:
[mediawiki/extensions/WikimediaApiPortalOAuth@master] Disable User JS on Special:AppManagement

I've filed T267608: Better error handling for remote rest requests as a more generic way for dealing with both the CORS "errors" and other errors generally as we've seen on related tasks (like the perf one and spawned related tasks too).

Good to go otherwise.