Hi @NiharikaKohli, I looked at the master branch of https://gerrit.wikimedia.org/r/p/mediawiki/extensions/PageAssessments.git, and looks like there isn't much there yet. Or maybe this is just a really small extension? We'll probably need to do the review this week or early next week if we're going to complete it this quarter, so let us know if it's currently ready, or if there's a commit in gerrit we should review?

Also, can you point to any extra documentation we can look at to get an overview of what the extension does, and if you have a development version of the extension installed anywhere that we can look at?

In T120220#1878417, @csteipp wrote:

Hi @NiharikaKohli, I looked at the master branch of https://gerrit.wikimedia.org/r/p/mediawiki/extensions/PageAssessments.git, and looks like there isn't much there yet. Or maybe this is just a really small extension? We'll probably need to do the review this week or early next week if we're going to complete it this quarter, so let us know if it's currently ready, or if there's a commit in gerrit we should review?

Hi Chris, thanks for looking into this. It's a small enough extension but there are two major commits yet to be merged in. I'm hopeful to have it complete by end of this week, or if not, Monday. I'll ping you to let you know when they're ready for review.

Also, can you point to any extra documentation we can look at to get an overview of what the extension does, and if you have a development version of the extension installed anywhere that we can look at?

There isn't any documentation yet. In brief, this extension takes article assessment data for wikiprojects as input via a parser function and stores it in a table. There's also a logging table to log every update.

Hi @csteipp, I have a patch in Gerrit: https://gerrit.wikimedia.org/r/259522 that's up for security reviews.

Description: This extension takes article assessment data for wikiprojects as input via a parser function and stores it in ExtensionData until parsing is complete because there can be multiple assessment functions on the same page. Then it fetches them and inserts, deletes or updates the database records according to the new data.
There's also a logging table to log every update.

Thanks!

Hi @NiharikaKohli,

I reviewed the extension with gerrit 259522 today. Two major blockers for deploying this:

• PageAssessmentsBody::getAllProjects - sql injection in the way you're constructing the query with the page title. The page title Foo" OR 1="1 is completely valid.

• The way that you're handling logging in a dedicated database table is concerning. As it is, someone can put something private/defamatory in one of the function parameters, and that is written directly into the log. If the revision is later suppressed, although the value will be deleted in page_assessments, it will stay in page_assessments_log. Since we typically replicate tables like this to toollabs, that would leave us with data in the database that we can't suppress. You have several options to address this:
  • Log to a filesystem log instead of DB table. It looks like you aren't displaying that information anywhere in the extension, so if the data is just for debugging, then I'd recommend just logging it. Logs are rotated after 90 days, so you're automatically compliant with our privacy policy.
  • Use the normal mediawiki logging functions (where we have deletion/suppression built in)
  • If you really need to have the logs in a db table and you can't use mediawiki's logging, you can build in your own deletion/suppression support, but that will be both a lot of work and something you'll need to work with the Security Team pretty closely on to make sure we get right.

@csteipp: Thanks for the review!

@NiharikaKohli: My suggestion is to drop the logging entirely for now since it isn't strictly necessary for an MVP. We can always add logging later after we figure out how to handle suppression.

This task is a bit difficult to follow. I see Chris' note about two major security blockers. Were those addressed?

In T120220#1962460, @MZMcBride wrote:

This task is a bit difficult to follow. I see Chris' note about two major security blockers. Were those addressed?

Yes. https://gerrit.wikimedia.org/r/#/c/259522/13..16/PageAssessmentsBody.php

@NiharikaKohli: In the future, it's a good idea to list all the bugs that are affected by a gerrit commit in the commit summary. You can do so by just separating them with commas: "Bug: XXXXX, XXXXX, XXXX"

In T120220#1963158, @kaldari wrote:

@NiharikaKohli: In the future, it's a good idea to list all the bugs that are affected by a gerrit commit in the commit summary. You can do so by just separating them with commas: "Bug: XXXXX, XXXXX, XXXX"

Err, don't do that. Use individual Bug headers for each bug, so Bug: XXXXX\nBug: XXXXX, etc. :)