Change 529168 had a related patch set uploaded (by SBassett; owner: SBassett):
[mediawiki/extensions/PageNotice@master] Adding phan-taint-check support via extra field

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

Hey @TTO and @Victar -

I just had a quick look at master - no need to security-protect this task. Some thoughts:

1. I added support in composer.json for phan-taint-check in this patch set. Feel free to merge if you'd like. While it's not a requirement to run phan-taint-check in CI, it's proven to be a fairly valuable, automated security tool.
2. The quibble, phan and secheck dockers are happy.
3. Dependencies seem fine within package.json and composer.json.
4. I didn't complete any exhaustive performance checks. The extension is pretty simple though and I'm not seeing anything that should obviously impact performance from a security standpoint. Of course I'd defer to the Performance-Team as the subject matter experts here. And possibly recommend running fresnel against the entire extension.
5. As an optional hardening measure, the values of $name and $ns could probably be further validated when building $header, $nsheader, $footer and $nsfooter. But $ns gets type-forced other places and msg() should be fairly robust at handling arbitrary values.
6. A possible defacement attack might be if someone were able to alter pages under the MediaWiki messages namespace. They could then expose malicious content to a large number of users. These are well-protected on most wikis; I just wanted to mention this here for completeness' sake.

The Security-Team has our security reviews scrum weekly on Tuesdays, so I'll leave this task open until at least then (August 13th) in case @Reedy or anybody else wanted to weigh in.

Change 529168 merged by jenkins-bot:
[mediawiki/extensions/PageNotice@master] Adding phan-taint-check support via extra field

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

Re number 6, there are already many avenues to deface the wiki if you have edit access to the MediaWiki: namespace. Editing MediaWiki:Sitenotice would have a similar effect as creating a page notice. I don't think the fact that this extension adds another avenue is of any special concern.

In T229718#5403914, @sbassett wrote:

The Security-Team has our security reviews scrum weekly on Tuesdays, so I'll leave this task open until at least then (August 13th) in case @Reedy or anybody else wanted to weigh in.

Any other comments?

Thanks. To be 100% clear, does that mean that this extension has successfully passed its security review?

Hey @TTO -

The Security-Team is trying to get away from providing a "pass" or "thumbs up" for code during security reviews, as it assumes a level of accountability on our part which we cannot sustain. So we are adopting the more standard system of risk classification and risk ownership for our security reviews. This entails us performing a risk analysis during the review process and then assigning and communicating a level of risk to the requesters/owners of the code. The levels of risk we're using within our analyses are:

These would have different implications around the likelihood of an extension getting deployed, etc. but obviously a low (or no) risk as the outcome of a security review would be considered fairly safe.