From 0c8289e5db2b328d9868fabb4fe3cce5d46a43f1 Mon Sep 17 00:00:00 2001
From: Erik Bernhardson <ebernhardson@wikimedia.org>
Date: Thu, 5 Dec 2019 16:31:03 -0800
Subject: [PATCH] Return HtmlArmor for Search ResultSet snippets

Passing around strings that are expected to be safe html and are
known to be based on user input is a fairly unsafe operation. Make
it harder to do the wrong thing by requiring HtmlArmor to be returned
from the ResultSet snippets. This does not address the snippets on
individual result objects as the api surface is larger and requires
more bc handling.

Change-Id: I76231d6fc53c4982eb4cd174d2e6a75eb2740497
---
 includes/api/ApiQuerySearch.php             |  4 +--
 includes/search/ISearchResultSet.php        |  4 +--
 includes/search/SearchResultSet.php         |  7 ++---
 includes/widget/search/DidYouMeanWidget.php | 29 ++++++++++++++++++---
 4 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/includes/api/ApiQuerySearch.php b/includes/api/ApiQuerySearch.php
index 9228d9ace91..e76788959da 100644
--- a/includes/api/ApiQuerySearch.php
+++ b/includes/api/ApiQuerySearch.php
@@ -133,13 +133,13 @@ class ApiQuerySearch extends ApiQueryGeneratorBase {
 				$apiResult->addValue( [ 'query', 'searchinfo' ],
 					'suggestion', $matches->getSuggestionQuery() );
 				$apiResult->addValue( [ 'query', 'searchinfo' ],
-					'suggestionsnippet', $matches->getSuggestionSnippet() );
+					'suggestionsnippet', HtmlArmor::getHtml( $matches->getSuggestionSnippet() ) );
 			}
 			if ( isset( $searchInfo['rewrittenquery'] ) && $matches->hasRewrittenQuery() ) {
 				$apiResult->addValue( [ 'query', 'searchinfo' ],
 					'rewrittenquery', $matches->getQueryAfterRewrite() );
 				$apiResult->addValue( [ 'query', 'searchinfo' ],
-					'rewrittenquerysnippet', $matches->getQueryAfterRewriteSnippet() );
+					'rewrittenquerysnippet', HtmlArmor::getHtml( $matches->getQueryAfterRewriteSnippet() ) );
 			}
 		}
 
diff --git a/includes/search/ISearchResultSet.php b/includes/search/ISearchResultSet.php
index 5faa4451cdd..79337b04d93 100644
--- a/includes/search/ISearchResultSet.php
+++ b/includes/search/ISearchResultSet.php
@@ -54,7 +54,7 @@ interface ISearchResultSet extends \Countable, \IteratorAggregate {
 	public function getQueryAfterRewrite();
 
 	/**
-	 * @return string|null Same as self::getQueryAfterRewrite(), but in HTML
+	 * @return HtmlArmor|string|null Same as self::getQueryAfterRewrite(), but in HTML
 	 *  and with changes highlighted. Null when the query was not rewritten.
 	 */
 	public function getQueryAfterRewriteSnippet();
@@ -73,7 +73,7 @@ interface ISearchResultSet extends \Countable, \IteratorAggregate {
 	public function getSuggestionQuery();
 
 	/**
-	 * @return string HTML highlighted suggested query, '' if none
+	 * @return HtmlArmor|string HTML highlighted suggested query, '' if none
 	 */
 	public function getSuggestionSnippet();
 
diff --git a/includes/search/SearchResultSet.php b/includes/search/SearchResultSet.php
index d759db9853b..692e55105cf 100644
--- a/includes/search/SearchResultSet.php
+++ b/includes/search/SearchResultSet.php
@@ -107,8 +107,9 @@ class SearchResultSet extends BaseSearchResultSet {
 	}
 
 	/**
-	 * @return string|null Same as self::getQueryAfterRewrite(), but in HTML
-	 *  and with changes highlighted. Null when the query was not rewritten.
+	 * @return HtmlArmor|string|null Same as self::getQueryAfterRewrite(), but
+	 *  with changes highlighted if HtmlArmor is returned. Null when the query
+	 *  was not rewritten.
 	 */
 	public function getQueryAfterRewriteSnippet() {
 		return null;
@@ -132,7 +133,7 @@ class SearchResultSet extends BaseSearchResultSet {
 	}
 
 	/**
-	 * @return string HTML highlighted suggested query, '' if none
+	 * @return HtmlArmor|string HTML highlighted suggested query, '' if none
 	 */
 	public function getSuggestionSnippet() {
 		return '';
diff --git a/includes/widget/search/DidYouMeanWidget.php b/includes/widget/search/DidYouMeanWidget.php
index a8f57e2b629..414b96d632f 100644
--- a/includes/widget/search/DidYouMeanWidget.php
+++ b/includes/widget/search/DidYouMeanWidget.php
@@ -2,7 +2,6 @@
 
 namespace MediaWiki\Widget\Search;
 
-use HtmlArmor;
 use ISearchResultSet;
 use SpecialSearch;
 
@@ -55,9 +54,21 @@ class DidYouMeanWidget {
 
 		$linkRenderer = $this->specialSearch->getLinkRenderer();
 		$snippet = $resultSet->getQueryAfterRewriteSnippet();
+		if ( $snippet === '' || $snippet === null ) {
+			// This should never happen. But if it did happen we would render
+			// links as `Special:Search` which is even more useless. Since this
+			// was only documented but not enforced previously emit a
+			// deprecation warning and in the future we can simply fail on bad
+			// inputs
+			wfDeprecated(
+				get_class( $resultSet ) . '::getQueryAfterRewriteSnippet returning empty snippet',
+				'1.34'
+			);
+			$snippet = $resultSet->getQueryAfterRewrite();
+		}
 		$rewritten = $linkRenderer->makeKnownLink(
 			$this->specialSearch->getPageTitle(),
-			$snippet ? new HtmlArmor( $snippet ) : null,
+			$snippet,
 			[ 'id' => 'mw-search-DYM-rewritten' ],
 			$stParams
 		);
@@ -92,9 +103,21 @@ class DidYouMeanWidget {
 		$stParams = array_merge( $params, $this->specialSearch->powerSearchOptions() );
 
 		$snippet = $resultSet->getSuggestionSnippet();
+		if ( $snippet === '' || $snippet === null ) {
+			// This should never happen. But if it did happen we would render
+			// links as `Special:Search` which is even more useless. Since this
+			// was only documented but not enforced previously emit a
+			// deprecation warning and in the future we can simply fail on bad
+			// inputs
+			wfDeprecated(
+				get_class( $resultSet ) . '::getSuggestionSnippet returning empty snippet',
+				'1.34'
+			);
+			$snippet = $resultSet->getSuggestionSnippet();
+		}
 		$suggest = $this->specialSearch->getLinkRenderer()->makeKnownLink(
 			$this->specialSearch->getPageTitle(),
-			$snippet ? new HtmlArmor( $snippet ) : null,
+			$snippet,
 			[ 'id' => 'mw-search-DYM-suggestion' ],
 			$stParams
 		);