Adding our sec review team

@RhinosF1 What is your threat model? My understanding is that anyone who can modify wg variables via LocalSettings.php has full control over the MediaWiki instance anyway (he/she can execute arbitrary code on the server, and inject arbitrary HTML), so no validation is needed in the extension. Now, I haven't been active in the MediaWiki community for a while, so my understanding might be outdated - if yes, please correct me :)
I would of course accept patches to e.g. check that the variables are proper integers (which is nice for catching errors), but so far I don't see this as a security problem.

@Tbleher: You should always validate variables are correct and as expected. It's simply good practice anywhere.

Given the required access anyway, it's a very low risk issue but improper validation is still an issue for me.

Can you also read the comment on our task by the reviewer that talks about submitting comments being broken anyway?

@RhinosF1: I've removed the security tag, since from my point of view, this is not a security issue. You are very welcome to submit patches to improve validation, though.
Regarding wpUnicodeCheck: It's quite possible that the extension doesn't work on current versions of MediaWiki. I use it in production (see http://spiele.j-crew.de/wiki/SpieleWiki:Spielwiese), and it works there, but the MediaWiki version there is very ancient. I currently don't have time to update the code and test it with newer versions of MediaWiki. Do you want to take over maintenance of this extension? That would be very welcome :)

I'm not a php developer. Could we consider archiving / marking the extension as unstable.

@Reedy, @sbaset: could you make public?

@sbassett: apologies for the spelling but ^

I've had a go at bringing this up to date. There's plenty of other stuff that could also be fixed, but the above patch at least gets it working again.

In T270767#6711694, @Samwilson wrote:

I've had a go at bringing this up to date. There's plenty of other stuff that could also be fixed, but the above patch at least gets it working again.

Would you mind writing a short do somewhere?

I noticed the comment / rows validation doesn't seem to be in that patch explicitly but it uses a builder now so not sure if that does anything.

Are you also willing to take on the maintenance as given you're a security reviewer with us that would give me a lot of comfort if you were happy?

In T270767#6711536, @RhinosF1 wrote:

@Reedy, @sbassett: could you make public?

Done. And I'd agree that this is likely lower-risk, given that various wg config variables should be considered fairly secure in most cases. If an attacker has write access to your LS.php (or related files) there's likely far worse things they can do than perpetrate a stored XSS in what does not appear to be a widely-used extension.

I noticed the comment / rows validation doesn't seem to be in that patch explicitly but it uses a builder now so not sure if that does anything.

Html::element should appropriately sanitize the relevant variables, as opposed to something like Html::rawElement which would not.

Also, linking the related patch since the bot couldn't when this task was protected: https://gerrit.wikimedia.org/r/651934

Thanks for the explanation @sbassett!

@Samwilson, @R4536th: Do you have any other concerns I'm missing now in regards to the extension? Is there a plan for future maintenance?

I'm happy with Scott's explanation and Sam's patch that this is probably resolved for the scope of this specific issue though.

I don't really have any plans to maintain this, but am happy to help out. I've made a patch to improve the code style: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Commentbox/+/654567

Can't think of any other obvious issues with the extension.