Make non-existent messages be html safe regardless of output format

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)</script' )->text() would
have outputted <script>alert(1)</script>. Now it outputs
⧼script&gt;alert(1)&lt;/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

diff --git a/includes/Message.php b/includes/Message.php
index d0325d7..2c979de 100644 (file)
--- 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 '&lt;' . htmlspecialchars( $this->key ) . '&gt;';
+                       // 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 2e059d7..be9ccaf 100644 (file)
--- a/tests/parser/parserTests.txt
+++ b/tests/parser/parserTests.txt
@@ -11013,7 +11013,7 @@ int keyword - non-existing message
 !! wikitext
 {{int:var}}
 !! html
-<p>&lt;var&gt;
+<p>⧼var⧽
 </p>
 !! end
 

diff --git a/tests/phpunit/includes/MessageTest.php b/tests/phpunit/includes/MessageTest.php
index c4f3fb1..4c689ab 100644 (file)
--- 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( '<i-dont-exist-evar>', wfMessage( 'i-dont-exist-evar' )->text() );
-               $this->assertEquals( '<i<dont>exist-evar>', wfMessage( 'i<dont>exist-evar' )->text() );
-               $this->assertEquals( '<i-dont-exist-evar>', wfMessage( 'i-dont-exist-evar' )->plain() );
-               $this->assertEquals( '<i<dont>exist-evar>', wfMessage( 'i<dont>exist-evar' )->plain() );
-               $this->assertEquals( '&lt;i-dont-exist-evar&gt;', wfMessage( 'i-dont-exist-evar' )->escaped() );
+               $this->assertEquals( '⧼i-dont-exist-evar⧽', wfMessage( 'i-dont-exist-evar' )->text() );
+               $this->assertEquals( '⧼i&lt;dont&gt;exist-evar⧽', wfMessage( 'i<dont>exist-evar' )->text() );
+               $this->assertEquals( '⧼i-dont-exist-evar⧽', wfMessage( 'i-dont-exist-evar' )->plain() );
+               $this->assertEquals( '⧼i&lt;dont&gt;exist-evar⧽', wfMessage( 'i<dont>exist-evar' )->plain() );
+               $this->assertEquals( '⧼i-dont-exist-evar⧽', wfMessage( 'i-dont-exist-evar' )->escaped() );
                $this->assertEquals(
-                       '&lt;i&lt;dont&gt;exist-evar&gt;',
+                       '⧼i&lt;dont&gt;exist-evar⧽',
                        wfMessage( 'i<dont>exist-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', '&lt;i-dont-exist-evar&gt;', 'escaped' ],
+                       [ 'i-dont-exist-evar', '⧼i-dont-exist-evar⧽' ],
+                       [ 'i-dont-exist-evar', '⧼i-dont-exist-evar⧽', 'escaped' ],
+                       [ 'script>alert(1)</script', '⧼script&gt;alert(1)&lt;/script⧽', 'escaped' ],
+                       [ 'script>alert(1)</script', '⧼script&gt;alert(1)&lt;/script⧽' ],
                ];
        }
 

diff --git a/tests/phpunit/includes/StatusTest.php b/tests/phpunit/includes/StatusTest.php
index 782fab0..474a481 100644 (file)
--- a/tests/phpunit/includes/StatusTest.php
+++ b/tests/phpunit/includes/StatusTest.php
@@ -376,9 +376,9 @@ class StatusTest extends MediaWikiLangTestCase {
                $status->warning( 'fooBar!' );
                $testCases['1StringWarning'] = [
                        $status,
-                       "<fooBar!>",
+                       "⧼fooBar!⧽",
                        "(wrap-short: (fooBar!))",
-                       "<p>&lt;fooBar!&gt;\n</p>",
+                       "<p>⧼fooBar!⧽\n</p>",
                        "<p>(wrap-short: (fooBar!))\n</p>",
                ];
 
@@ -387,9 +387,9 @@ class StatusTest extends MediaWikiLangTestCase {
                $status->warning( 'fooBar2!' );
                $testCases['2StringWarnings'] = [
                        $status,
-                       "* <fooBar!>\n* <fooBar2!>\n",
+                       "* ⧼fooBar!⧽\n* ⧼fooBar2!⧽\n",
                        "(wrap-long: * (fooBar!)\n* (fooBar2!)\n)",
-                       "<ul><li> &lt;fooBar!&gt;</li>\n<li> &lt;fooBar2!&gt;</li></ul>\n",
+                       "<ul><li> ⧼fooBar!⧽</li>\n<li> ⧼fooBar2!⧽</li></ul>\n",
                        "<p>(wrap-long: * (fooBar!)\n</p>\n<ul><li> (fooBar2!)</li></ul>\n<p>)\n</p>",
                ];
 
@@ -397,9 +397,9 @@ class StatusTest extends MediaWikiLangTestCase {
                $status->warning( new Message( 'fooBar!', [ 'foo', 'bar' ] ) );
                $testCases['1MessageWarning'] = [
                        $status,
-                       "<fooBar!>",
+                       "⧼fooBar!⧽",
                        "(wrap-short: (fooBar!: foo, bar))",
-                       "<p>&lt;fooBar!&gt;\n</p>",
+                       "<p>⧼fooBar!⧽\n</p>",
                        "<p>(wrap-short: (fooBar!: foo, bar))\n</p>",
                ];
 
@@ -408,9 +408,9 @@ class StatusTest extends MediaWikiLangTestCase {
                $status->warning( new Message( 'fooBar2!' ) );
                $testCases['2MessageWarnings'] = [
                        $status,
-                       "* <fooBar!>\n* <fooBar2!>\n",
+                       "* ⧼fooBar!⧽\n* ⧼fooBar2!⧽\n",
                        "(wrap-long: * (fooBar!: foo, bar)\n* (fooBar2!)\n)",
-                       "<ul><li> &lt;fooBar!&gt;</li>\n<li> &lt;fooBar2!&gt;</li></ul>\n",
+                       "<ul><li> ⧼fooBar!⧽</li>\n<li> ⧼fooBar2!⧽</li></ul>\n",
                        "<p>(wrap-long: * (fooBar!: foo, bar)\n</p>\n<ul><li> (fooBar2!)</li></ul>\n<p>)\n</p>",
                ];