On a preliminary analysis, this code path does not seem triggerable via ProofreadPage, the concerned line in includes/language/Language.php:3026 is ( in mediawiki/core)

if ( $number === (string)NAN ) {
	return $this->msg( 'formatnum-nan' )->text();
}

$number is passed by includes/Pagination/PageNumber.php which explicitly checks for the numeric-ness of the input before passing it the offending function, formatNumNoSeparators( $number ); which makes sure that $number will not be NaN is that specific flow.

     public function getFormattedPageNumber( Language $language ): string {
		if ( !is_numeric( $this->number ) ) {
			return $this->number;
		}

		$number = (int)$this->number;
		switch ( $this->displayMode ) {
			case self::DISPLAY_NORMAL:
				return $language->formatNumNoSeparators( $number );
			case self::DISPLAY_FOLIO:
				return $language->formatNumNoSeparators( $number ) .
					$this->formatRectoVerso();
			....
		}
	}

Given that the phan security check plugin recently released a new version this might be a regression in the taint analysis ? (cc @Jdforrester-WMF who released the new version)

Also cc @Tpt @Samwilson who also work on the same codebase and might be able to give more insight.

I'd note that sometimes phan-taint-check just isn't "smart enough" to understand certain taint flows, which could be the case here. Unless this is a confirmed regression from a previous phan/phan-taint-check release. If we're confident in this being a false positive, we could easily add // @phan-suppress-next-line SecurityCheck-XSS ... before the appropriate line in ProofreadPage.php or wherever.

which explicitly checks for the numeric-ness

NaN is numeric in php, so that check doesn't stop this condition from ocurring.

Regardless, i think there is an argument, that if a function outputs something that can sometimes be unsafe html, and otherwise it outputs something safe but not escaped, it makes sense to always escape, even if always unnecessary at runtime, as it makes the code easier to check for security issues at a glance.

What @Bawolff said, ideally every method should return values with a consistent level of escaping. Otherwise it's not just taint-check, but also humans who could get confused. Escaping the return value of formatNum seems a good idea even if the code tries to avoid the potentially-unsafe paths.

(Also, the new version of taint-check is not used anywhere yet, because we need a new version of mediawiki-phan-config first, and then individual repos should be updated to use this new version)

In T345693#9146950, @Bawolff wrote:

which explicitly checks for the numeric-ness

NaN is numeric in php, so that check doesn't stop this condition from ocurring.

---

Regardless, i think there is an argument, that if a function outputs something that can sometimes be unsafe html, and otherwise it outputs something safe but not escaped, it makes sense to always escape, even if always unnecessary at runtime, as it makes the code easier to check for security issues at a glance.

I wasn't able to reproduce this (XSS via NaN), but I agree with this in principle, I'm adding a patch to santize those specific code paths

In T345693#9146964, @Daimona wrote:

(Also, the new version of taint-check is not used anywhere yet, because we need a new version of mediawiki-phan-config first, and then individual repos should be updated to use this new version)

I'm extremely unsure and wanted to flag this in case it was indeed a issue with the taint-check (since while testing locally, I did see a bump in mediawiki/phan-taint-check-plugin), if this is expected, maybe we can deploy the patch above to mitigate the issue? :)

In T345693#9147474, @Soda wrote:

I wasn't able to reproduce this (XSS via NaN), but I agree with this in principle, I'm adding a patch to santize those specific code paths

0001-Escape-formatNumNoSeperator-number-output.patch2 KBDownload

CR+1, if we need entity support and can't just call htmlspecialchars directly. I think this could be deployed during this Monday's security deployment window.

Actually, one minor change, can we prefix the subject in the patch with SECURITY:?

In T345693#9152890, @sbassett wrote:

Actually, one minor change, can we prefix the subject in the patch with SECURITY:?

Sure, attached the patch

In T345693#9154297, @Soda wrote:

In T345693#9152890, @sbassett wrote:

Actually, one minor change, can we prefix the subject in the patch with SECURITY:?

Sure, attached the patch

0001-SECURITY-Escape-formatNumNoSeperator-number-output.patch2 KBDownload

CR+2, I'll try to get this deployed today.

The patch from T345693#9154297 has been deployed. Now tracking at T276237 and T340874.

Change 961262 had a related patch set uploaded (by Mstyles; author: Sohom Datta):

[mediawiki/extensions/ProofreadPage@master] SECURITY: Escape formatNumNoSeperator( $number ) output

https://gerrit.wikimedia.org/r/961262

Change 961262 merged by jenkins-bot:

[mediawiki/extensions/ProofreadPage@master] SECURITY: Escape formatNumNoSeperator( $number ) output

https://gerrit.wikimedia.org/r/961262