From b3d1c6a9699dec107eed7c165a5315cab31e1c5a Mon Sep 17 00:00:00 2001 From: Arlo Breault Date: Wed, 28 Jun 2023 18:22:29 -0400 Subject: [PATCH 5/5] SECURITY: Move badFile lookup to Linker Bug: T335612 Change-Id: I849d02f1d3dc9995353b7a9995601d214053dca3 --- includes/Linker.php | 25 +++++++++++++++--- includes/parser/Parser.php | 34 ++++++++++++------------- tests/parser/legacyMediaParserTests.txt | 8 ++++++ tests/parser/mediaParserTests.txt | 16 +++++++++--- 4 files changed, 57 insertions(+), 26 deletions(-) diff --git a/includes/Linker.php b/includes/Linker.php index caf48a22c4f..021e3cb3d28 100644 --- a/includes/Linker.php +++ b/includes/Linker.php @@ -450,7 +450,10 @@ class Linker { $thumb = false; } - if ( !$thumb || ( !$enableLegacyMediaDOM && $thumb->isError() ) ) { + $isBadFile = $file && $thumb && $parser && + $parser->getBadFileLookup()->isBadFile( $title->getDBkey(), $parser->getTitle() ); + + if ( !$thumb || ( !$enableLegacyMediaDOM && $thumb->isError() ) || $isBadFile ) { $rdfaType = 'mw:Error ' . $rdfaType; $currentExists = $file && $file->exists(); if ( $enableLegacyMediaDOM ) { @@ -731,6 +734,12 @@ class Linker { . "
"; } + $isBadFile = $exists && $thumb && $parser && + $parser->getBadFileLookup()->isBadFile( + $manualthumb ? $manual_title : $title->getDBkey(), + $parser->getTitle() + ); + if ( !$exists ) { $rdfaType = 'mw:Error ' . $rdfaType; $label = ''; @@ -738,10 +747,16 @@ class Linker { $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( @@ -749,8 +764,10 @@ class Linker { '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 index 2af48e1b519..940d7c9abc4 100644 --- a/includes/parser/Parser.php +++ b/includes/parser/Parser.php @@ -2696,25 +2696,23 @@ class Parser { } 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/legacyMediaParserTests.txt b/tests/parser/legacyMediaParserTests.txt index 423f6ee9d98..a6a4965240b 100644 --- a/tests/parser/legacyMediaParserTests.txt +++ b/tests/parser/legacyMediaParserTests.txt @@ -174,6 +174,14 @@ Bar foo Bar foo

!! end +!! test +Bad images - manualthumb +!! wikitext +[[File:Foobar.jpg|thumb=Bad.jpg|Uh oh]] +!! html/php +
File:Foobar.jpg
Uh oh
+!! end + !! test Bad images - in gallery !! wikitext diff --git a/tests/parser/mediaParserTests.txt b/tests/parser/mediaParserTests.txt index ecd12cbece4..3fb85931d71 100644 --- a/tests/parser/mediaParserTests.txt +++ b/tests/parser/mediaParserTests.txt @@ -172,7 +172,6 @@ wgParserEnableLegacyMediaDOM=false
one two | three
!! end -## FIXME: Legacy output doesn't match Parsoid !! test Bad images - basic functionality !! config @@ -180,13 +179,12 @@ wgParserEnableLegacyMediaDOM=false !! wikitext [[File:Bad.jpg]] !! html/php -

File:Bad.jpg +

File:Bad.jpg

!! html/parsoid

File:Bad.jpg

!! end -## FIXME: Legacy output doesn't match Parsoid !! test Bad images - T18039: text after bad image disappears !! config @@ -197,7 +195,7 @@ Foo bar Bar foo !! html/php

Foo bar -File:Bad.jpg +File:Bad.jpg Bar foo

!! html/parsoid @@ -206,6 +204,16 @@ Bar foo Bar foo

!! end +!! test +Bad images - manualthumb +!! config +wgParserEnableLegacyMediaDOM=false +!! wikitext +[[File:Foobar.jpg|thumb=Bad.jpg|Uh oh]] +!! html/php +
File:Foobar.jpg
Uh oh
+!! end + !! test Bad images - in gallery !! config -- 2.41.0