SECURITY: Move badFile lookup to Linker

CVE-2023-36674 Bug: T335612 Change-Id: I849d02f1d3dc9995353b7a9995601d214053dca3
wikimedia · Jun 30, 2023 · 50401b2 · 50401b2
1 parent 4690431
commit 50401b2
Show file tree

Hide file tree

Showing 4 changed files with 60 additions and 26 deletions.
diff --git a/includes/linker/Linker.php b/includes/linker/Linker.php
@@ -472,7 +472,10 @@ public static function makeImageLink( Parser $parser, LinkTarget $title,
 			$thumb = false;
 		}
 
-		if ( !$thumb || ( !$enableLegacyMediaDOM && $thumb->isError() ) ) {
+		$isBadFile = $file && $thumb &&
+			$parser->getBadFileLookup()->isBadFile( $title->getDBkey(), $parser->getTitle() );
+
+		if ( !$thumb || ( !$enableLegacyMediaDOM && $thumb->isError() ) || $isBadFile ) {
 			$rdfaType = 'mw:Error ' . $rdfaType;
 			$currentExists = $file && $file->exists();
 			if ( $enableLegacyMediaDOM ) {
@@ -680,6 +683,7 @@ public static function makeThumbLink2(
 		$thumb = false;
 		$noscale = false;
 		$manualthumb = false;
+		$manual_title = '';
 		$rdfaType = 'mw:File/Thumb';
 
 		if ( !$exists ) {
@@ -765,6 +769,12 @@ public static function makeThumbLink2(
 				. "<div class=\"thumbinner\" style=\"width:{$outerWidth}px;\">";
 		}
 
+		$isBadFile = $exists && $thumb && $parser &&
+			$parser->getBadFileLookup()->isBadFile(
+				$manualthumb ? $manual_title : $title->getDBkey(),
+				$parser->getTitle()
+			);
+
 		if ( !$exists ) {
 			$rdfaType = 'mw:Error ' . $rdfaType;
 			$label = '';
@@ -775,19 +785,27 @@ public static function makeThumbLink2(
 				$title, $label, '', '', '', (bool)$time, $handlerParams, false
 			);
 			$zoomIcon = '';
-		} elseif ( !$thumb || ( !$enableLegacyMediaDOM && $thumb->isError() ) ) {
+		} elseif ( !$thumb || ( !$enableLegacyMediaDOM && $thumb->isError() ) || $isBadFile ) {
 			$rdfaType = 'mw:Error ' . $rdfaType;
 			if ( $enableLegacyMediaDOM ) {
-				$s .= wfMessage( 'thumbnail_error', '' )->escaped();
+				if ( !$thumb ) {
+					$s .= wfMessage( 'thumbnail_error', '' )->escaped();
+				} else {
+					$s .= self::makeBrokenImageLinkObj(
+						$title, '', '', '', '', (bool)$time, $handlerParams, true
+					);
+				}
 			} else {
 				if ( $thumb && $thumb->isError() ) {
 					Assert::invariant(
 						$thumb instanceof MediaTransformError,
 						'Unknown MediaTransformOutput: ' . get_class( $thumb )
 					);
 					$label = $thumb->toText();
-				} else {
+				} elseif ( !$thumb ) {
 					$label = wfMessage( 'thumbnail_error', '' )->text();
+				} else {
+					$label = '';
 				}
 				$s .= self::makeBrokenImageLinkObj(
 					$title, $label, '', '', '', (bool)$time, $handlerParams, true

diff --git a/includes/parser/Parser.php b/includes/parser/Parser.php
@@ -2674,25 +2674,23 @@ private function handleInternalLinks2( &$s ) {
 				}
 
 				if ( $ns === NS_FILE ) {
-					if ( !$this->badFileLookup->isBadFile( $nt->getDBkey(), $this->getTitle() ) ) {
-						if ( $wasblank ) {
-							# if no parameters were passed, $text
-							# becomes something like "File:Foo.png",
-							# which we don't want to pass on to the
-							# image generator
-							$text = '';
-						} else {
-							# recursively parse links inside the image caption
-							# actually, this will parse them in any other parameters, too,
-							# but it might be hard to fix that, and it doesn't matter ATM
-							$text = $this->handleExternalLinks( $text );
-							$holders->merge( $this->handleInternalLinks2( $text ) );
-						}
-						# cloak any absolute URLs inside the image markup, so handleExternalLinks() won't touch them
-						$s .= $prefix . $this->armorLinks(
-							$this->makeImage( $nt, $text, $holders ) ) . $trail;
-						continue;
+					if ( $wasblank ) {
+						# if no parameters were passed, $text
+						# becomes something like "File:Foo.png",
+						# which we don't want to pass on to the
+						# image generator
+						$text = '';
+					} else {
+						# recursively parse links inside the image caption
+						# actually, this will parse them in any other parameters, too,
+						# but it might be hard to fix that, and it doesn't matter ATM
+						$text = $this->handleExternalLinks( $text );
+						$holders->merge( $this->handleInternalLinks2( $text ) );
 					}
+					# cloak any absolute URLs inside the image markup, so handleExternalLinks() won't touch them
+					$s .= $prefix . $this->armorLinks(
+						$this->makeImage( $nt, $text, $holders ) ) . $trail;
+					continue;
 				} elseif ( $ns === NS_CATEGORY ) {
 					/**
 					 * Strip the whitespace Category links produce, see T2087

diff --git a/tests/parser/legacyMedia.txt b/tests/parser/legacyMedia.txt
@@ -218,6 +218,16 @@ Bar foo
 Bar foo</p>
 !! end
 
+!! test
+Bad images - manualthumb
+!! config
+wgParserEnableLegacyMediaDOM=true
+!! wikitext
+[[File:Foobar.jpg|thumb=Bad.jpg|Uh oh]]
+!! html/php
+<div class="thumb tright"><div class="thumbinner" style="width:322px;"><a href="/wiki/File:Foobar.jpg" title="File:Foobar.jpg">File:Foobar.jpg</a>  <div class="thumbcaption">Uh oh</div></div></div>
+!! end
+
 !! test
 Bad images - in gallery
 !! config

diff --git a/tests/parser/media.txt b/tests/parser/media.txt
@@ -196,21 +196,19 @@ wgParserEnableLegacyMediaDOM=false
 <figure class="mw-default-size" typeof="mw:File/Thumb"><a href="./File:Foobar.jpg" class="mw-file-description"><img resource="./File:Foobar.jpg" src="http://example.com/images/thumb/3/3a/Foobar.jpg/180px-Foobar.jpg" decoding="async" data-file-width="1941" data-file-height="220" data-file-type="bitmap" height="20" width="180" srcset="http://example.com/images/thumb/3/3a/Foobar.jpg/270px-Foobar.jpg 1.5x, http://example.com/images/thumb/3/3a/Foobar.jpg/360px-Foobar.jpg 2x" class="mw-file-element"/></a><figcaption>one <i>two</i> <span typeof="mw:Entity">|</span> three</figcaption></figure>
 !! end
 
-## FIXME: Legacy output doesn't match Parsoid
 !! test
 Bad images - basic functionality
 !! config
 wgParserEnableLegacyMediaDOM=false
 !! wikitext
 [[File:Bad.jpg]]
 !! html/php
-<p><a href="/wiki/File:Bad.jpg" title="File:Bad.jpg">File:Bad.jpg</a>
+<p><span class="mw-default-size" typeof="mw:Error mw:File"><a href="/wiki/File:Bad.jpg" title="File:Bad.jpg"><span class="mw-file-element mw-broken-media" data-width="320">File:Bad.jpg</span></a></span>
 </p>
 !! html/parsoid
 <p><span class="mw-default-size" typeof="mw:Error mw:File" data-mw='{"errors":[{"key":"apierror-badfile","message":"This image is on the bad file list."}]}'><a href="./Special:FilePath/Bad.jpg"><span class="mw-file-element mw-broken-media" resource="./File:Bad.jpg">File:Bad.jpg</span></a></span></p>
 !! end
 
-## FIXME: Legacy output doesn't match Parsoid
 !! test
 Bad images - T18039: text after bad image disappears
 !! config
@@ -221,7 +219,7 @@ Foo bar
 Bar foo
 !! html/php
 <p>Foo bar
-<a href="/wiki/File:Bad.jpg" title="File:Bad.jpg">File:Bad.jpg</a>
+<span class="mw-default-size" typeof="mw:Error mw:File"><a href="/wiki/File:Bad.jpg" title="File:Bad.jpg"><span class="mw-file-element mw-broken-media" data-width="320">File:Bad.jpg</span></a></span>
 Bar foo
 </p>
 !! html/parsoid
@@ -230,6 +228,16 @@ Bar foo
 Bar foo</p>
 !! end
 
+!! test
+Bad images - manualthumb
+!! config
+wgParserEnableLegacyMediaDOM=false
+!! wikitext
+[[File:Foobar.jpg|thumb=Bad.jpg|Uh oh]]
+!! html/php
+<figure typeof="mw:Error mw:File/Thumb"><a href="/wiki/File:Foobar.jpg" title="File:Foobar.jpg"><span class="mw-file-element mw-broken-media" data-width="180">File:Foobar.jpg</span></a><figcaption>Uh oh</figcaption></figure>
+!! end
+
 !! test
 Bad images - in gallery
 !! config