From 184658eb32f6c5561cd3789716bd98c1e9f0ba04 Mon Sep 17 00:00:00 2001 From: Brian Wolff Date: Wed, 29 Jun 2016 18:09:18 -0400 Subject: [PATCH] Make non-existent messages be html safe regardless of output format MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit If you have a non-existent message in the output, chances are its user-controlled. If the message has the ->plain() or ->text() format, the output used to be not safe for html. Obviously people should not be using those format types where html is being outputted, but sometimes that happens. I think we should prioritize always being safe over the fallback content not potentially being double escaped. Additionally switch the enclosing brackets to be fancy unicode characters, to sidestep the escaping issue on the enclosing brackets. So previously, wfMessage( 'script>alert(1)text() would have outputted . Now it outputs ⧼script>alert(1)</script⧽. No sane message key will include < or >, so this would really only come up if the user can control the message key name. This goes somewhat against T68199. Change-Id: Ic8a60892b8e847e6021494c10968814aac391731 --- includes/Message.php | 11 +++++++---- tests/parser/parserTests.txt | 2 +- tests/phpunit/includes/MessageTest.php | 18 ++++++++++-------- tests/phpunit/includes/StatusTest.php | 16 ++++++++-------- 4 files changed, 26 insertions(+), 21 deletions(-) diff --git a/includes/Message.php b/includes/Message.php index d0325d79b8..2c979dedd0 100644 --- a/includes/Message.php +++ b/includes/Message.php @@ -802,10 +802,13 @@ class Message implements MessageSpecifier, Serializable { $string = $this->fetchMessage(); if ( $string === false ) { - if ( $this->format === 'plain' || $this->format === 'text' ) { - return '<' . $this->key . '>'; - } - return '<' . htmlspecialchars( $this->key ) . '>'; + // Err on the side of safety, ensure that the output + // is always html safe in the event the message key is + // missing, since in that case its highly likely the + // message key is user-controlled. + // '⧼' is used instead of '<' to side-step any + // double-escaping issues. + return '⧼' . htmlspecialchars( $this->key ) . '⧽'; } # Replace $* with a list of parameters for &uselang=qqx. diff --git a/tests/parser/parserTests.txt b/tests/parser/parserTests.txt index 2e059d7423..be9ccaf297 100644 --- a/tests/parser/parserTests.txt +++ b/tests/parser/parserTests.txt @@ -11013,7 +11013,7 @@ int keyword - non-existing message !! wikitext {{int:var}} !! html -

<var> +

⧼var⧽

!! end diff --git a/tests/phpunit/includes/MessageTest.php b/tests/phpunit/includes/MessageTest.php index c4f3fb1497..4c689abb04 100644 --- a/tests/phpunit/includes/MessageTest.php +++ b/tests/phpunit/includes/MessageTest.php @@ -223,13 +223,13 @@ class MessageTest extends MediaWikiLangTestCase { */ public function testToStringKey() { $this->assertEquals( 'Main Page', wfMessage( 'mainpage' )->text() ); - $this->assertEquals( '', wfMessage( 'i-dont-exist-evar' )->text() ); - $this->assertEquals( 'exist-evar>', wfMessage( 'iexist-evar' )->text() ); - $this->assertEquals( '', wfMessage( 'i-dont-exist-evar' )->plain() ); - $this->assertEquals( 'exist-evar>', wfMessage( 'iexist-evar' )->plain() ); - $this->assertEquals( '<i-dont-exist-evar>', wfMessage( 'i-dont-exist-evar' )->escaped() ); + $this->assertEquals( '⧼i-dont-exist-evar⧽', wfMessage( 'i-dont-exist-evar' )->text() ); + $this->assertEquals( '⧼i<dont>exist-evar⧽', wfMessage( 'iexist-evar' )->text() ); + $this->assertEquals( '⧼i-dont-exist-evar⧽', wfMessage( 'i-dont-exist-evar' )->plain() ); + $this->assertEquals( '⧼i<dont>exist-evar⧽', wfMessage( 'iexist-evar' )->plain() ); + $this->assertEquals( '⧼i-dont-exist-evar⧽', wfMessage( 'i-dont-exist-evar' )->escaped() ); $this->assertEquals( - '<i<dont>exist-evar>', + '⧼i<dont>exist-evar⧽', wfMessage( 'iexist-evar' )->escaped() ); } @@ -237,8 +237,10 @@ class MessageTest extends MediaWikiLangTestCase { public static function provideToString() { return [ [ 'mainpage', 'Main Page' ], - [ 'i-dont-exist-evar', '' ], - [ 'i-dont-exist-evar', '<i-dont-exist-evar>', 'escaped' ], + [ 'i-dont-exist-evar', '⧼i-dont-exist-evar⧽' ], + [ 'i-dont-exist-evar', '⧼i-dont-exist-evar⧽', 'escaped' ], + [ 'script>alert(1)alert(1)warning( 'fooBar!' ); $testCases['1StringWarning'] = [ $status, - "", + "⧼fooBar!⧽", "(wrap-short: (fooBar!))", - "

<fooBar!>\n

", + "

⧼fooBar!⧽\n

", "

(wrap-short: (fooBar!))\n

", ]; @@ -387,9 +387,9 @@ class StatusTest extends MediaWikiLangTestCase { $status->warning( 'fooBar2!' ); $testCases['2StringWarnings'] = [ $status, - "* \n* \n", + "* ⧼fooBar!⧽\n* ⧼fooBar2!⧽\n", "(wrap-long: * (fooBar!)\n* (fooBar2!)\n)", - "
  • <fooBar!>
    • \n
  • <fooBar2!>
\n", + "
  • ⧼fooBar!⧽
    • \n
  • ⧼fooBar2!⧽
\n", "

(wrap-long: * (fooBar!)\n

\n
  • (fooBar2!)
\n

)\n

", ]; @@ -397,9 +397,9 @@ class StatusTest extends MediaWikiLangTestCase { $status->warning( new Message( 'fooBar!', [ 'foo', 'bar' ] ) ); $testCases['1MessageWarning'] = [ $status, - "", + "⧼fooBar!⧽", "(wrap-short: (fooBar!: foo, bar))", - "

<fooBar!>\n

", + "

⧼fooBar!⧽\n

", "

(wrap-short: (fooBar!: foo, bar))\n

", ]; @@ -408,9 +408,9 @@ class StatusTest extends MediaWikiLangTestCase { $status->warning( new Message( 'fooBar2!' ) ); $testCases['2MessageWarnings'] = [ $status, - "* \n* \n", + "* ⧼fooBar!⧽\n* ⧼fooBar2!⧽\n", "(wrap-long: * (fooBar!: foo, bar)\n* (fooBar2!)\n)", - "
  • <fooBar!>
    • \n
  • <fooBar2!>
\n", + "
  • ⧼fooBar!⧽
    • \n
  • ⧼fooBar2!⧽
\n", "

(wrap-long: * (fooBar!: foo, bar)\n

\n
  • (fooBar2!)
\n

)\n

", ]; -- 2.20.1