Hi there @Pchelolo - we were able to take a look at this in our clinic meeting today and have noted that you are aiming for an April 30th deployment date. While we don't currently see any issues with that date, please keep in mind that our ability to review in a timely fashion is subject to change (due to world pandemic chaos, and team members being affected, etc). Please let us know of any questions or concerns and we will be in touch as we move forward.

Are we still on target for this review?

@CCicalese_WMF - I'm hopeful to have a report by the end of this week. I've glanced at the code a couple of times and the extension appears well-written and uncontroversial, though I definitely need to perform a more rigorous analysis.

@sbassett Checking on the status of the security review report. Thanks.

@CCicalese_WMF @Pchelolo - sorry for the delay on this. Overall this looks fine from a basic security perspective.

Security Review Summary - T248483 - 2020-05-05
Last commit reviewed: dbd207998c55af1b7918690a0199e0d29e4e72a5

Vulnerable Packages
As reported by snyk test:

1. lodash@4.17.15, Prototype Pollution (medium risk)
   • Introduced by grunt@1.1.0 > grunt-legacy-log@2.0.0 > lodash@4.17.15 and 2 other path(s). See also: https://snyk.io/vuln/SNYK-JS-LODASH-567746

Policy Compliance

1. Without digging very deeply, I assume this extension's interactions with a third-party API has gone through review by WMF-Legal and is in compliance with the current Wikimedia privacy policy.

Other General Security Issues

1. While it's fairly common for MediaWiki maintenance scripts (e.g. ModerateExistingFiles.php) to include files with dynamically-created paths via variables like $IP and RUN_MAINTENANCE_IF_MAIN, these get picked up by various static analysis tools as potential means of remote code injection. While such vulnerabilities should be difficult to exploit in the case of MediaWiki maintenance scripts, fortifying such includes with checks for valid/expected paths may be a worthwhile effort in defensive coding. Though I'd rate this as a very low risk and note that it has been a point of contention in the past, e.g. some discussion on this review: T242134.

Thank you, @sbassett! I have requested a response to the policy compliance question. @Pchelolo is looking into the other general security question.

Change 594948 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/extensions/MediaModeration@master] maintenance: add security check for $basePath, updated mpn packages

https://gerrit.wikimedia.org/r/594948

@Pchelolo @sbassett

I've upload a patch that adds additional security verification on $IP.
As to lodash, the vulnerability was found Apr, 28, it appears on the latest version of lodash (4.17.15) and no official patch has been released yet.

So there's no way impossible to fix the issue just by increasing the version in package.json.
There's a patch that, probably, fixes the issue: https://snyk-patches.s3.amazonaws.com/npm/lodash/20200430/lodash_0_0_20200430_6baae67d501e4c45021280876d42efe351e77551.patch

We could apply it as a postinstall step. I'm not sure how safe is this though.

Another thing is that lodash and npm is used by 'grunt' and 'grunt-banana-checker' which are CLI tools and are going to be run in dev mode only.

So, I need and advice on what is the best option here.

@sbassett Just to confirm, legal has reviewed the use of the API here and this is approved.

I'm going to stall this for now until we get the thumbs up from WMF-Legal, then it can be resolved.

Should this be closed then?

Change 594948 merged by jenkins-bot:
[mediawiki/extensions/MediaModeration@master] maintenance: add security check for $basePath, updated mpn packages

https://gerrit.wikimedia.org/r/594948