We recognize that this is last minute and will be thankful for your help. Note that the codebase will only be ready for review next week, but we are making this request now to get the ball rolling. We have left [TODO] on purpose.

This can actually be deployed after you have time for the review. We hope that this won’t be too much work as the code base will be quite small. Thank you so much for looking into this!

The code base is now ready for review, and I've added the scc output to the task description. Please let us know if you need anything else. Thanks!

Security Review Summary - T350900 - 2023-11-22
Last commit reviewed: be04cea

Summary

From a security perspective, the WikimediaCampaignEvents extension is robust and free from known security vulnerabilities due to its up-to-date dependencies. The codebase is generally clean; however, it does involve some usage, although discouraged, of the service locator pattern. Although this approach might not be optimal for testing and maintaining code, there are situations where it can be beneficial. I'm curious to understand the reasoning behind its choice.
The overall risk rating is: low.

Vulnerable Packages - Production
npm audit returned no results. low risk
snyk returned no results. low risk
auditjs returned no results. low risk
osv-scanner and osv-detector returned no results. low risk

Risk: none

Outdated Packages
As reported via npm outdated:
none

As reported via composer outdated:
(no explicit vulnerabilities reported, simply noting for completeness' sake.)

Static Analysis Findings
semgrep returned no results. low risk
snyk code returned no results. low risk
horusec returned a couple of false-positives related to secret leaks. low risk
sast-scan returned a couple of false-positives related to secret leaks. low risk

General Security Issues
gitleaks returned no results. low risk
whispers returned no results. low risk

General code health score

1. The Wikimedia code health check tool returned a weighted risk score of 31.10:

+-----------+----------+----------+------+---------------+---------------+--------------+-------------+------------+--------------+-----------+---------------+
| Vuln Pkgs | Pkg Mgmt | Test Cov | SAST | Non-auto Cmts | Uniq Contribs | Contrib Conc | Lang Guides | Staff Supp | Task Backlog | Code Stew | Weighted Risk |
+===========+==========+==========+======+===============+===============+==============+=============+============+==============+===========+===============+
|         0 |        4 |        3 |    0 |            10 |            10 |            4 |           7 |         10 |            0 |         0 |         31.10 |
+-----------+----------+----------+------+---------------+---------------+--------------+-------------+------------+--------------+-----------+---------------+

Thanks for the review!

In T350900#9423738, @mmartorana wrote:

The codebase is generally clean; however, it does involve some usage, although discouraged, of the service locator pattern. Although this approach might not be optimal for testing and maintaining code, there are situations where it can be beneficial. I'm curious to understand the reasoning behind its choice.

Where did you see those? I'm unable to find any static service access in the code base.

In T350900#9423899, @Daimona wrote:

Thanks for the review!

In T350900#9423738, @mmartorana wrote:

The codebase is generally clean; however, it does involve some usage, although discouraged, of the service locator pattern. Although this approach might not be optimal for testing and maintaining code, there are situations where it can be beneficial. I'm curious to understand the reasoning behind its choice.

Where did you see those? I'm unable to find any static service access in the code base.

I am talking about WikimediaCampaignEventsServices class/file where I can see a service locator pattern usage. I was just curious about this approach.

In T350900#9423938, @mmartorana wrote:

In T350900#9423899, @Daimona wrote:

Thanks for the review!

In T350900#9423738, @mmartorana wrote:

The codebase is generally clean; however, it does involve some usage, although discouraged, of the service locator pattern. Although this approach might not be optimal for testing and maintaining code, there are situations where it can be beneficial. I'm curious to understand the reasoning behind its choice.

Where did you see those? I'm unable to find any static service access in the code base.

I am talking about WikimediaCampaignEventsServices class/file where I can see a service locator pattern usage. I was just curious about this approach.

Ah, sure! That class is a service locator for extension-specific services. It can be used in place of the central service locator (MediaWikiServices) to obtain instances of the extension's services. Its main purposes are to avoid things like MediaWikiServices::getInstance()->get( 'NameOfTheExtensionService' ) where the name of the service can be misspelled, as well as provide information on the return type. In practice, though, DI is always preferred, and so this locator should only used either when DI is not possible (e.g., old-style static hook handlers), or unnecessary (e.g., tests). HTH!

Hi @mmartorana
Are you done with the security review?
Let us know!