This will tentatively be scheduled for review during the next quarter (2024-01-01 through 2024-03-31).

Hello @KStoller-WMF - As I prepare for this review, could you kindly provide your project timeline? In particular, I'm interested in whether you have any imminent plans to release a substantial amount of code or introduce new features. This information will help me decide whether to wait for those updates and include them in my review.

Alternatively, if there are no such plans, I'll start the review from the latest stable branch commit.

Please share any relevant timelines, deadlines, or additional details.

Thank you.

In T349568#9473357, @mmartorana wrote:

Hello @KStoller-WMF - As I prepare for this review, could you kindly provide your project timeline? In particular, I'm interested in whether you have any imminent plans to release a substantial amount of code or introduce new features. This information will help me decide whether to wait for those updates and include them in my review.

Alternatively, if there are no such plans, I'll start the review from the latest stable branch commit.

Please share any relevant timelines, deadlines, or additional details.

Thank you.

@mmartorana Thanks! You can review our current timeline / milestones here. In other words, we will add quite a bit more code before release. End of February is likely the earliest we will be ready for a release to a beta wiki, end of March we hope to release to 2 or 3 pilot wikis.

@Urbanecm_WMF - are we ready for an initial pass at Application Security Review?
https://gitlab.wikimedia.org/repos/growth/community-configuration - Should they start here?

Hi @mmartorana!

My name is Martin Urbanec and I work as a Software Engineer within the Growth team. Unfortunately, we're somehow behind the originally anticipated schedule. However, we now have CommunityConfiguration working in some way within mediawiki/extensions/CommunityConfiguration on Gerrit (since it is meant as a platform for other developers, it is not directly usable, but we have an example available in GitLab). Documentation of the extension is available at https://www.mediawiki.org/wiki/Extension:CommunityConfiguration and https://www.mediawiki.org/wiki/Extension:CommunityConfiguration/Developer_setup.

So far, not everything that we want to be a part of CommunityConfiguration is fully ready yet, but it would make testing and reviewing much easier if we could have the extension available at the beta cluster. Unfortunately, I'm not fully sure what exactly we need to have "checked off" by the Security team for that to happen. As far as I understand it, Writing an extension for deployment indicates an Application Security Review needs to be completed for a beta deployment to happen. Is that understanding correct? If so, what is needed from the Growth-Team to make an Application Security Review possible? Is there something else we need to run by the Security team before moving ahead to Beta?

Looking forward to hearing from you,
Martin Urbanec, Growth team

FTR: We've moved this task to our sprint board to ensure it stays on our radar. For the same reason, I'm also adding myself as the Other Assignee.

Feel free to re-triage as needed, I just wanted to note that from the Growth team perspective this is high priority because we hope to release this new Extension to Beta by the end of March 2024.

Noting that I discussed Growth's timeline with @mmartorana, and that he'll aim to start this review as soon as possible and aim to complete this prior to March 23rd.

In T349568#9602791, @Urbanecm_WMF wrote:

Hi @mmartorana!

My name is Martin Urbanec and I work as a Software Engineer within the Growth team. Unfortunately, we're somehow behind the originally anticipated schedule. However, we now have CommunityConfiguration working in some way within mediawiki/extensions/CommunityConfiguration on Gerrit (since it is meant as a platform for other developers, it is not directly usable, but we have an example available in GitLab). Documentation of the extension is available at https://www.mediawiki.org/wiki/Extension:CommunityConfiguration and https://www.mediawiki.org/wiki/Extension:CommunityConfiguration/Developer_setup.

So far, not everything that we want to be a part of CommunityConfiguration is fully ready yet, but it would make testing and reviewing much easier if we could have the extension available at the beta cluster. Unfortunately, I'm not fully sure what exactly we need to have "checked off" by the Security team for that to happen. As far as I understand it, Writing an extension for deployment indicates an Application Security Review needs to be completed for a beta deployment to happen. Is that understanding correct? If so, what is needed from the Growth-Team to make an Application Security Review possible? Is there something else we need to run by the Security team before moving ahead to Beta?

Looking forward to hearing from you,
Martin Urbanec, Growth team

Hello @Urbanecm_WMF - Appreciate the update. As told to @KStoller-WMF, there aren't any stringent requirements from the security team for release at this time. However, it's always highly recommended to get it reviewed from us as we not to own the associated risks.

Currently, we don't require any immediate action from your end. I'll reach out if necessary. Thanks again.

Security Review Summary - T349568 - 2024-03-22
Last commit reviewed: cb8c5d5

Summary

In general, the code being reviewed appears to be robust in terms of security. There are no vulnerable dependencies, and no apparent vulnerabilities are present. However, it may be beneficial to provide further clarification on two medium-severity findings identified in the SAST section. Addressing these findings and updating the outdated dependencies could enhance clarity and ensure that the overall risk score remains low.

Vulnerable Packages - Production
Risk: none

snyk returned no results. low risk
osv-scanner returned no results. low risk
sast-scan returned no results. low risk
osv-detector returned no results. low risk
local-php-security-checker returned no results. low risk
npm audit returned no results. low risk

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

npm outdated returned no results. low risk

Static Analysis Findings
Risk: medium

Snyk found a potential problem with information disclosure in src/Api/ApiEdit.php, line 53. I'm curious if the logger here could accidentally reveal sensitive data. medium risk

Semgrep has detected a potential template injection vulnerability in src/Specials/templates/Features.mustache at line 6. If user manipulation is possible for the variables <a href="{{ href }}"> {{ title }}</a>, the vulnerability may be exploited. medium risk

bearer returned a warning of Information Leakage in JavaScript Logger Message: File: resources/ext.communityConfiguration.Editor/init.js:68
( err ) => console.error( err ) low risk

horusec returned some informational issues. (file attached) low risk
sast-scan returned no results. low risk

General code health score
Risk:.

1. The Wikimedia code health check tool returned a weighted risk score of 27.50 low risk.

PROJECT: CommunityConfiguration@master
REPOSITORY: https://gerrit.wikimedia.org/r/mediawiki/extensions/CommunityConfiguration

+-----------+----------+----------+------+---------------+---------------+--------------+-------------+------------+--------------+-----------+---------------+
| 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 |        0 |        5 |    0 |             5 |            10 |            4 |           7 |         10 |            2 |         0 |         27.50 |
+-----------+----------+----------+------+---------------+---------------+--------------+-------------+------------+--------------+-----------+---------------+

General Security Issues
Risk:.
gitleaks and git-secrets returned no results. low risk

Supplemental Materials
Please see attached:

1. horusec.txt3 KBDownload

Thank you for review, @mmartorana!

@Sgs, @Urbanecm_WMF, & @DMburugu - should we follow up regarding the "medium risk" Static Analysis Findings?

Static Analysis Findings
Risk: medium risk
Snyk found a potential problem with information disclosure in src/Api/ApiEdit.php, line 53. I'm curious if the logger here could accidentally reveal sensitive data. medium risk

Semgrep has detected a potential template injection vulnerability in src/Specials/templates/Features.mustache at line 6. If user manipulation is possible for the variables <a href="{{ href }}"> {{ title }}</a>, the vulnerability may be exploited. medium risk

Thank you for the review, @mmartorana!

In T349568#9654663, @mmartorana wrote:

Security Review Summary - T349568 - 2024-03-22
Static Analysis Findings
Risk: medium

Snyk found a potential problem with information disclosure in src/Api/ApiEdit.php, line 53. I'm curious if the logger here could accidentally reveal sensitive data. medium risk

I do not see what possible problem is being flagged here. The logger at the line you specified looks like this:

// NOTE: Assuming the list of supported keys in getAllowedParams() are correct, this
// branch should be never triggered.
$this->logger->error(
	__METHOD__ . ' failed to construct the {provider} provider',
	[ 'provider' => $params['provider'], 'exception' => $e ],
);

This logger call is here to ensure the Growth team is quickly informed whenever the CommunityConfiguration extension has a bug (considering under normal conditions, the list of providers defined in ApiEdit::getAllowedParams() and the situations in which newProvider() throws should be in sync). This means the code branch is unreachable under normal conditions. However, even when it does get called, the only user-supplied information it logs is the provider's name ($params['provider']). The provider name:

1. is publicly available within the configuration of the client repository (within the Git repo) and as such, it is not private information (there is a short set of strings the user is able to enter),
2. is already logged internally via the mediawiki_api_request stream in EventStreams, and queryable in the data lake (which has the same data retention as logstash),
3. if the operation was successful, the same information would be visible publicly (as the ApiEdit endpoint results in an on-wiki edit)

The other information that gets logged is the traceback, which is a standard part of our error logging.

Considering one information is user-provided and the other one is a standard part of error logging, I am unsure what is the risky part that is identified here. In my opinion, there is no info leakage possible (considering none of ApiEdit operates with privileged information). Do we need to change something about this approach? Can this be re-assessed by the Security team, please?

Semgrep has detected a potential template injection vulnerability in src/Specials/templates/Features.mustache at line 6. If user manipulation is possible for the variables <a href="{{ href }}"> {{ title }}</a>, the vulnerability may be exploited. medium risk

The href and title variables come from extension.json files of the client extension (example: CommunityConfigurationExample), or from a PHP global (in the form of a configuration variable). If the attacker has the ability to manipulate with extension.json, its loading logic or PHP globals, that already gives them nearly-complete control over the MediaWiki instance involved. From CommunityConfiguration perspective, values provided by extension.json or PHP globals should be safe to render in the way used here.
Within CommunityConfiguration, those values are loaded in https://github.com/wikimedia/mediawiki-extensions-CommunityConfiguration/blob/master/src/Specials/DashboardEditorCapability.php, and one can see the values indeed do come from internal sources.

Unfortunately, I do not understand what is the medium risk here. Is there something wrong with the approach we have chosen here? Do we need to change something? If not, can the risk rating be reassessed, please?

If there is anything else we need to do/provide, please do not hesitate to let me know.

Thanks for the clarification, @mmartorana, that makes sense.