This might be possible for next quarter (January 2022 to March 2022).

In T295065#7483021, @sbassett wrote:

This might be possible for next quarter (January 2022 to March 2022).

There's no rush but equally no reason to delay if room :)

In T295065#7483110, @RhinosF1 wrote:

In T295065#7483021, @sbassett wrote:

This might be possible for next quarter (January 2022 to March 2022).

There's no rush but equally no reason to delay if room :)

Likely not for this quarter, sorry.

Oh ye definitely not expecting that. Was just meaning that by as soon as I mean whenever suits security team. If you get time next quater, that's more than great!

Note that AFAIK, there's no long-time volunteer or WMF staff member who explicitly committed to both implement all the changes requested in the various reviews and take on long-term maintenance of the extension.

I'm not sure it's a good idea to proceed with deployment under those circumstances (and by extension, with security review).

CC'ing @Octfx as extension maintainer.

Hi @Urbanecm,

Before proceeding with the review, we would like to shed some light on a couple of points:

1. Is there a WMF sponsoring team at the moment for the production deployment of this extension? If so, where is the confirmation and documentation of their level of commitment.

2. If there isn't a WMF sponsoring team, which technical volunteers have committed to assisting with the production deployment and ongoing maintenance of the extension?

3. What other processes and requests have been planned if the extension passes security review? A performance test? A beta deployment with some kind of evaluation? There doesn't appear to be anything else listed under the parent task at https://phabricator.wikimedia.org/T294885.

Hello @mmartorana,

Thanks for the questions, but I think you're asking the wrong individual :-). I'm not involved in this apart from saying T295065#7483954 here (and quickly reviewing the code myself, cf. T294885#7502536).

I think your questions might be answered better by @Octfx, who is one of the maintainers of the extension in question. I also think that before starting any kind of formal review, question asked at T294885#7502394 (and at few other places too) should likely be answered by @Octfx (or other extension maintainer).

Hope this helps.

Thanks @Urbanecm.

I will then ask @Octfx and @RhinosF1 to answer my questions, please.

In T295065#7697906, @mmartorana wrote:

Hi @Urbanecm,

Before proceeding with the review, we would like to shed some light on a couple of points:

1. Is there a WMF sponsoring team at the moment for the production deployment of this extension? If so, where is the confirmation and documentation of their level of commitment.

Not that I'm aware of.

2. If there isn't a WMF sponsoring team, which technical volunteers have committed to assisting with the production deployment and ongoing maintenance of the extension?

I can provide some level of support given Miraheze use it myself. I imagine @Octfx would. I'm fairly responsive and monitor the train anyway.

3. What other processes and requests have been planned if the extension passes security review? A performance test? A beta deployment with some kind of evaluation? There doesn't appear to be anything else listed under the parent task at https://phabricator.wikimedia.org/T294885.

I can't really imagine a performance impact but they'll need to have a look. It would definitely be on beta first.

Hi @RhinosF1, thanks a lot for your feedback!

I will procede with the review and I will get back to you.

Security Review Summary - T295065 - 2022-03-31
Last commit reviewed: 08d39f11c9abdf22d10e6a0138d4ea3d6817cc62

Summary

Overall, the extension looks good, with a risk rating of: low. There are some potential findings that are easy to fix. I would address them using the proposed remediations.

Vulnerable Packages - Production
none

Vulnerable Packages - Development
none

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

Static Analysis Findings

• phan returned no results. low risk
• phan-taint-check returned no results. low risk
• snyk returned no results. low risk
• semgrep which was run with the following rule sets:
  1. p/owasp-top-ten: This policy returned no results. low risk
  2. p/r2c-ci: This policy returned no results. low risk
  3. p/jwt This policy returned no results. low risk
  4. p/xss: This policy returned no results. low risk
  5. p/security-audit : This policy returned no results. low risk
  6. p/ci : This policy returned no results. low risk
  7. p/secrets : This policy returned no results. low risk
  8. p/supply-chain : This policy returned no results. low risk
  9. p/insecure-transport : This policy returned no results. low risk

The following semgrep policies all generated results, which I've included as supplemental files at the end of this review:

1. r/php
2. p/phpcs-security-audit

The general summary of the above policy results are:

Several unsafe unserialize() callings with user input in the pattern. It is not advised to unserialize random data as this can lead to arbitrary code execution. There is also a TODO comment in include/WikiSEO.php:

256 ┆ // TODO: Remove this sometime in the future
257 ┆ // Values were serialized between v2.3.1 and v2.4.1
258 ┆ $valueUnserialized = unserialize( $value, [ 'allowed_classes' => false ] );

It is advised to remove the unserialize calls since you are now on v2.6.5. I'd rate this as low risk.

• mw_php_sec_sniff returned some validation issues. low risk. In includes/Hooks/InfoAction.php there are some ->plain() , ->text() and Html::rawElement calls that require further discussion:

A. These calls could be changed to use escaped() since they don't and shouldn't have html in them:

80 ┆ ( new Message( 'wiki-seo-pageinfo-header-description' ) )->plain()`
84 ┆ ( new Message( 'wiki-seo-pageinfo-header-content' ) )->plain()`

B. These calls, could contain html, so it is advised to declare those as HtmlRawMessages in extension.json:

112 ┆ ( new Message( sprintf( 'wiki-seo-param-%s', $param ) ) )->plain()`
119 ┆ ( new Message( sprintf( 'wiki-seo-param-%s', $param ) ) )->text()`

Supplemental Materials
Please see attached:

1. 2022-wikiseo-semgrep-r-php-phpcs-security-audit.txt1 KBDownload

@mmartorana: Thanks for the review.

Most of these look fairly easy to fix. Are they all okay to push to gerrit?

Hi @RhinosF1, as per WMF's risk management framework, low risk (which this review has overall) is automatically accepted by the WMF.

Also, since this isn't deployed to Wikimedia production, and the review task is public, pushing to gerrit is totally fine.

Let me know if you have other concerns.

Unfortunately, CI fails on https://gerrit.wikimedia.org/r/c/mediawiki/extensions/WikiSEO/+/777740

CI fails due to one test checking if wiki.png is output, if no logo ist set. But it seems that the default logo name has changed to change-your-logo.svg in recent versions?

In T295065#7834249, @Octfx wrote:

CI fails due to one test checking if wiki.png is output, if no logo ist set. But it seems that the default logo name has changed to change-your-logo.svg in recent versions?

I updated the test in my patch and requested your review.

I haven't replaced the html::rawElement calls in includes/Hooks/InfoAction.php in case they are needed but I'm not sure they are from a quick glance

I hadn't found a way to create img and a elements without using Html::rawElement.
But I think as they don't contain user submitted content they should be safe.

In T295065#7834259, @Octfx wrote:

I hadn't found a way to create img and a elements without using Html::rawElement.
But I think as they don't contain user submitted content they should be safe.

Thanks for clarifying. If no user submitted content is included, they are definitely safe.

In T295065#7834259, @Octfx wrote:

I hadn't found a way to create img and a elements without using Html::rawElement.
But I think as they don't contain user submitted content they should be safe.

We always flag Html::rawElement during security reviews since its content parameter obviously doesn't escape anything. But for the img tag in question, there's obviously no content. And expandAttributes should do a good job at sanitizing anything potentially dangerous in the string used to construct various attributes. And for the a tag rawElement, $title->prefixedText should be pretty safe.