Fix XSS vulnerability via unsafe rendering of objects in link text

modules/admin/app/views/admin/common/propertyList.scala.html

@@ -7,13 +7,13 @@
         case s: String if s.isEmpty => {
         }
         case Some(s) => {
-            <li>@Html(highlighter.highlight(s.toString))</li>
+            <li>@highlighter.highlightText(s.toString)</li>
         }
         case s: String => {
-            <li>@Html(highlighter.highlight(s))</li>
+            <li>@highlighter.highlightText(s)</li>
         }
         case s => {
-            <li>@Html(highlighter.highlight(s.toString))</li>
+            <li>@highlighter.highlightText(s.toString)</li>
         }
     }
-}
+}

modules/admin/app/views/admin/helpers/linkTo.scala.html

@@ -1,3 +1,3 @@
-@(item: Model, content: Html = Html(""), attributes: Seq[(Symbol,String)] = Seq.empty, fragment: String = "")(implicit userOpt: Option[UserProfile], req: RequestHeader, messages: Messages, conf: AppConfig)
+@(item: Model, htmlContent: Html = Html(""), textContent: Option[String] = None, attributes: Seq[(Symbol,String)] = Seq.empty, fragment: String = "")(implicit userOpt: Option[UserProfile], req: RequestHeader, messages: Messages, conf: AppConfig)
 
-@views.html.admin.helpers.linkToWithFragment(item, fragment=fragment, content=content, attributes=attributes)
+@views.html.admin.helpers.linkToWithFragment(item, fragment=fragment, htmlContent=htmlContent, textContent=textContent, attributes=attributes)

modules/admin/app/views/admin/helpers/linkToWithBody.scala.html

@@ -1,3 +1,3 @@
-@(item: Model, attributes: Seq[(Symbol,String)] = Seq.empty, fragment: String = "")(content: Html)(implicit req: RequestHeader, messages: Messages)
+@(item: Model, attributes: Seq[(Symbol,String)] = Seq.empty, fragment: String = "")(htmlContent: Html)(implicit req: RequestHeader, messages: Messages)
 
-@views.html.admin.helpers.linkToWithFragment(item, fragment=fragment, content=content, attributes=attributes)
+@views.html.admin.helpers.linkToWithFragment(item, fragment=fragment, htmlContent=htmlContent, attributes=attributes)

modules/admin/app/views/admin/helpers/linkToWithFragment.scala.html

@@ -1,3 +1,5 @@
-@(item: Model, fragment: String, content: Html = Html(""), attributes: Seq[(Symbol,String)] = Seq.empty)(implicit req: RequestHeader, messages: Messages)
+@(item: Model, fragment: String, htmlContent: Html = Html(""), textContent: Option[String] = None, attributes: Seq[(Symbol,String)] = Seq.empty)(implicit req: RequestHeader, messages: Messages)
 
-<a href="@{views.admin.Helpers.linkTo(item) + fragment}" @play.api.templates.PlayMagic.toHtmlArgs(attributes.toMap) >@{if(content.body.trim.isEmpty) Html(item.toStringLang) else content}</a>
+<a href="@{views.admin.Helpers.linkTo(item) + fragment}" @play.api.templates.PlayMagic.toHtmlArgs(attributes.toMap) >
+    @{if(htmlContent.body.trim.isEmpty) textContent.getOrElse(item.toStringLang) else htmlContent}
+</a>

modules/admin/app/views/admin/link/itemLink.scala.html

@@ -1,6 +1,6 @@
 @(link: Link, item: Model, src: Option[Model], text: Option[String] = None)(implicit userOpt: Option[UserProfile], req: RequestHeader, conf: AppConfig, messages: Messages)
 
-@views.html.admin.helpers.linkTo(item, content = Html(text.getOrElse(item.toStringLang)))
+@views.html.admin.helpers.linkTo(item, textContent = Some(text.getOrElse(item.toStringLang)))
 <small>
     <span class="badge badge-dark">@Messages("link." + link.data.linkType.toString)</span>
 </small>

modules/admin/app/views/formHelpers/submitButton.scala.html

@@ -8,4 +8,4 @@
         }
         @extra
     </div>
-</div>
+</div>

modules/core/src/main/scala/services/search/SearchHit.scala

@@ -4,7 +4,9 @@ import scala.annotation.tailrec
 import SearchConstants._
 import models.EntityType
 import play.api.libs.json.JsValue
-import play.twirl.api.HtmlFormat
+import play.twirl.api.{Html,HtmlFormat}
+import org.jsoup.Jsoup
+import org.jsoup.safety.Whitelist
 
 /**
  * Class representing a search engine hit
@@ -42,18 +44,20 @@ case class SearchHit(
   private lazy val highlightFields: List[String] = filteredHighlights.values.flatten.toList
 
   /**
-   * Try and highlight some text according to the highlights.
+   * Try and highlight some text according to the highlights. The input text is
+    * assumed to already contain HTML.
+    *
    * @param text Some input text
-   * @return  The text and a boolean indicating if highlighting was successful
+   * @return  The text, with highlights inserted
    */
   def highlight(text: String): String = {
-    def canHighlightWith(raw: String, text: String): Either[String,String] = {
+    def canHighlightWith(rawHighlight: String, text: String): Either[String,String] = {
       // NB: We have to escape the input string using HtmlFormat because it's
       // specialised to avoid XSS. This assumes that the highlight material coming
       // back from Solr doesn't itself contain XSS attacks... what are the chances
       // this will come back to bite me?
-      val stripped = HtmlFormat.escape(stripTags(raw)).body
-      if (text.contains(stripped)) Right(text.replace(stripped, raw))
+      val stripped = HtmlFormat.escape(stripTags(rawHighlight)).body
+      if (text.contains(stripped)) Right(text.replace(stripped, rawHighlight))
       else Left(text)
     }
     @tailrec def tryHighlight(texts: Seq[String], input: String, ok: Boolean): String = {
@@ -68,4 +72,12 @@ case class SearchHit(
 
     tryHighlight(highlightFields, text, ok = false)
   }
+
+  /**
+    * Try and highlight some text which we know should not contain HTML.
+    *
+    * @param text Some input text
+    * @return The text, with highlights inserted
+    */
+  def highlightText(text: String): Html = Html(highlight(Jsoup.clean(text, Whitelist.none())))
 }

modules/core/src/main/scala/views/Highlighter.scala

@@ -1,9 +1,13 @@
 package views
 
+import play.twirl.api.Html
+
 object NoopHighlighter extends Highlighter {
   def highlight(text: String): String = text
+  def highlightText(text: String): Html = Html(text)
 }
 
 trait Highlighter {
   def highlight(text: String): String
+  def highlightText(text: String): Html
 }

modules/core/src/test/scala/services/search/SearchHitSpec.scala

@@ -61,5 +61,11 @@ class SearchHitSpec extends PlaySpecification {
       hl must contain("<em>Demandes</em>")
       hl must not contain("<em>lu-006007-lu-11-iv-3-286</em>")
     }
+
+    "prevent XSS attacks" in {
+      val hl = testHit.highlightText("some text<script>alert('hello');</script>")
+      hl.body must not contain "<script>"
+      hl.body must not contain "alert"
+    }
   }
 }

modules/portal/app/views/activity/eventSummary.scala.html

@@ -29,7 +29,7 @@ <h4 class="timeline-event-title">@views.html.helpers.linkTo(user)</h4>
             <dd>
                 <ul class="timeline-summary-subjects">
                     @for(s <- subjects.toSeq.sortBy(_.isA)) {
-                        <li>@views.html.helpers.linkTo(s, content = Html(s.toStringAbbr))</li>
+                        <li>@views.html.helpers.linkTo(s, textContent = Some(s.toStringAbbr))</li>
                     }
                 </ul>
             </dd>

modules/portal/app/views/common/accessPointLink.scala.html

@@ -6,7 +6,7 @@
  *@
 @accessPoint.target(item, links).map { case (link, other) =>
     <li class="resolved-access-point" id="@accessPoint.id" dir="@views.Helpers.textDirectionAttr(accessPoint.name)">
-        @views.html.helpers.linkTo(other, content = Html(accessPoint.name),
+        @views.html.helpers.linkTo(other, textContent = Some(accessPoint.name),
             attributes = link.data.description.filterNot(_.trim.isEmpty).toSeq.map('title -> _))
     </li>
 }.getOrElse {

modules/portal/app/views/common/accessPoints.scala.html

@@ -6,7 +6,7 @@
             @list.map { accessPoint =>
                 @accessPoint.target(item, links).map { case (link, other) =>
                     <li class="resolved-access-point" id="@accessPoint.id">
-                        @views.html.helpers.linkTo(other, Html(accessPoint.name))
+                        @views.html.helpers.linkTo(other, textContent = Some(accessPoint.name))
                         @if(link.data.dates.nonEmpty) {
                             <span class="small">@link.data.dateRange</span>
                         }

modules/portal/app/views/common/otherNameList.scala.html

@@ -4,7 +4,7 @@
     @if(ofn.nonEmpty) {
         <ul class="other-name-list">
             @ofn.map { on =>
-                <li>@Html(highlighter.highlight(on))</li>
+                <li>@highlighter.highlightText(on)</li>
             }
             @if(rest.nonEmpty) {
                 <li title="@rest.mkString(", ")">@Messages("truncated")</li>

modules/portal/app/views/concept/listItemBody.scala.html

@@ -5,7 +5,7 @@
         @item.vocabulary.map { vocab =>
             <li>
                 @views.html.helpers.link(views.Helpers.linkTo(vocab), Symbol("class") -> "alt") {
-                    @Html(highlighter.highlight(vocab.toStringLang))
+                    @highlighter.highlightText(vocab.toStringLang)
                 }
             </li>
         }

modules/portal/app/views/concept/searchItem.scala.html

@@ -3,7 +3,7 @@
 @views.html.common.searchItemOutline(item, watched) {
     @item.data.primaryDescription(descriptionId).map { desc =>
         @defining(if (item.descriptions.size > 1) desc.localId else None) { localDescId =>
-            @views.html.helpers.linkTo(item, content = Html(highlighter.highlight(item.toStringLang)),
+            @views.html.helpers.linkTo(item, htmlContent = highlighter.highlightText(item.toStringLang),
                     call = Some(controllers.portal.routes.Concepts.browse(item.id, localDescId)))
         }
     }

modules/portal/app/views/country/listItemMeta.scala.html

@@ -1,8 +1,7 @@
 @(item: Country, highlighter: views.Highlighter)(implicit messages: Messages)
 
 
-<li title="@Messages("country.identifier.description"): @item.data.identifier.toUpperCase(messages.lang.toLocale)">@Html(highlighter.highlight(item.data
-        .identifier.toUpperCase(messages.lang.toLocale)))</li>
+<li title="@Messages("country.identifier.description"): @item.data.identifier.toUpperCase(messages.lang.toLocale)">@highlighter.highlightText(item.data.identifier.toUpperCase(messages.lang.toLocale))</li>
 @item.latestEvent.map { event =>
     <li title="@common.eventTitle(event)">
         <time datetime="@event.data.timestamp">

modules/portal/app/views/country/searchItem.scala.html

@@ -4,8 +4,8 @@
     <img alt="Country Flag" width="48" height="48" src="https://ehri-portal.s3.eu-central-1.amazonaws.com/portal/images/Country/@{item.id.toLowerCase}.png"/>
 }
 
-@views.html.common.searchItemOutline(item, watched, views.html.helpers.linkTo(item, content = image)) {
-    @views.html.helpers.linkTo(item, content = Html(highlighter.highlight(item.toStringLang)))
+@views.html.common.searchItemOutline(item, watched, views.html.helpers.linkTo(item, htmlContent = image)) {
+    @views.html.helpers.linkTo(item, htmlContent = highlighter.highlightText(item.toStringLang))
 } {
     @listItemBody(item)
 }

modules/portal/app/views/documentaryUnit/infobar.scala.html

@@ -5,10 +5,10 @@
         <li>@views.html.helpers.linkTo(country)</li>
     }
     @item.holder.map { repo =>
-        <li>@views.html.helpers.linkTo(repo, Html(views.Helpers.ellipsize(repo.toStringLang, 100)))</li>
+        <li>@views.html.helpers.linkTo(repo, textContent = Some(views.Helpers.ellipsize(repo.toStringLang, 100)))</li>
     }
     @item.ancestors.reverse.map { ann =>
-        <li>@views.html.helpers.linkTo(ann, Html(views.Helpers.ellipsize(ann.toStringLang, 100)))</li>
+        <li>@views.html.helpers.linkTo(ann, textContent = Some(views.Helpers.ellipsize(ann.toStringLang, 100)))</li>
     }
 }
 

modules/portal/app/views/documentaryUnit/listItemBody.scala.html

@@ -7,7 +7,7 @@
             @item.holder.map { holder =>
               <li>
                   @views.html.helpers.link(views.Helpers.linkTo(holder), Symbol("class") -> "alt") {
-                      @Html(highlighter.highlight(holder.toStringLang))
+                      @highlighter.highlightText(holder.toStringLang)
                   }
               </li>
             }

modules/portal/app/views/documentaryUnit/searchItem.scala.html

@@ -5,7 +5,7 @@
         @parent.map { p =>
             @wrapParent(doc, p.parent)
             <li>
-              @views.html.helpers.linkTo(p, content = Html(highlighter.highlight(p.toStringLang)), Seq(Symbol("class") -> "search-item-parent-name"))
+              @views.html.helpers.linkTo(p, htmlContent = highlighter.highlightText(p.toStringLang), attributes = Seq(Symbol("class") -> "search-item-parent-name"))
             </li>
         }
     }
@@ -19,7 +19,7 @@
             </div>
             <h3 class="search-item-heading type-highlight @item.isA.toString" dir="@dir">
                 @defining(if (item.descriptions.size > 1) desc.localId else None) { localDescId =>
-                    @views.html.helpers.linkTo(item, content = Html(highlighter.highlight(desc.name)),
+                    @views.html.helpers.linkTo(item, htmlContent = highlighter.highlightText(desc.name),
                         call = Some(controllers.portal.routes.DocumentaryUnits.browse(item.id, localDescId)))
                 }
             </h3>

modules/portal/app/views/helpers/linkTo.scala.html

@@ -1,5 +1,5 @@
-@(item: Model, content: Html = Html(""), attributes: Seq[(Symbol,String)] = Seq.empty, call: Option[Call] = None)(implicit conf: AppConfig, req: RequestHeader, messages: Messages)
+@(item: Model, htmlContent: Html = Html(""), textContent: Option[String] = None, attributes: Seq[(Symbol,String)] = Seq.empty, call: Option[Call] = None)(implicit conf: AppConfig, req: RequestHeader, messages: Messages)
 
 @* This just delegates to the anchored link (which no fragment)
    because we can't use a default argument with a varargs method. *@
-@views.html.helpers.linkToWithFragment(item, fragment="", content=content, attributes=attributes, call = call)
+@views.html.helpers.linkToWithFragment(item, fragment="", htmlContent=htmlContent, textContent=textContent, attributes=attributes, call = call)

modules/portal/app/views/helpers/linkToWithFragment.scala.html

@@ -1,3 +1,5 @@
-@(item: Model, fragment: String, content: Html = Html(""), attributes: Seq[(Symbol,String)] = Seq.empty, call: Option[Call] = None)(implicit conf: AppConfig, req: RequestHeader, messages: Messages)
+@(item: Model, fragment: String, htmlContent: Html = Html(""), textContent: Option[String] = None, attributes: Seq[(Symbol,String)] = Seq.empty, call: Option[Call] = None)(implicit conf: AppConfig, req: RequestHeader, messages: Messages)
 
-<a @{if(conf.isEmbedMode) "target=\"_top\""} href="@call.getOrElse(views.Helpers.linkTo(item))@fragment" class="@{if(conf.isEmbedMode) "external "}@("type-highlight " + item.isA)" @play.api.templates.PlayMagic.toHtmlArgs(attributes.toMap) >@{if(content.body.trim.isEmpty) Html(item.toStringLang) else content}</a>
+<a @{if(conf.isEmbedMode) "target=\"_top\""} href="@call.getOrElse(views.Helpers.linkTo(item))@fragment" class="@{if(conf.isEmbedMode) "external "}@("type-highlight " + item.isA)" @play.api.templates.PlayMagic.toHtmlArgs(attributes.toMap) >
+    @{if(htmlContent.body.trim.isEmpty) textContent.getOrElse(item.toStringLang) else htmlContent}
+</a>

modules/portal/app/views/historicalAgent/listItemBody.scala.html

@@ -6,7 +6,7 @@
         @item.set.map { set =>
             <li>
                 @views.html.helpers.link(views.Helpers.linkTo(set), Symbol("class") -> "alt") {
-                    @Html(highlighter.highlight(set.toStringLang))
+                    @highlighter.highlightText(set.toStringLang)
                 }
             </li>
         }

modules/portal/app/views/historicalAgent/searchItem.scala.html

@@ -1,7 +1,7 @@
 @(item: HistoricalAgent, highlighter: views.Highlighter, watched: Boolean = false)(implicit userOpt: Option[UserProfile], req: RequestHeader, conf: AppConfig, messages: Messages, md: MarkdownRenderer, descriptionId: Option[String] = None)
 
 @views.html.common.searchItemOutline(item, watched) {
-    @views.html.helpers.linkTo(item, content = Html(highlighter.highlight(item.toStringLang)))
+    @views.html.helpers.linkTo(item, htmlContent = highlighter.highlightText(item.toStringLang))
 } {
     @listItemBody(item, highlighter = highlighter)
 }

modules/portal/app/views/repository/conciseAddress.scala.html

@@ -4,14 +4,14 @@
     @item.country.map { ct =>
         <li>
             @views.html.helpers.link(views.Helpers.linkTo(ct), Symbol("class") -> "alt") {
-                @Html(highlighter.highlight(ct.toStringLang))
+                @highlighter.highlightText(ct.toStringLang)
             }
         </li>
 
     }
 }
 @desc.addresses.headOption.map(_.concise).filterNot(_.isEmpty).map { conciseAddress =>
-    <li>@Html(highlighter.highlight(conciseAddress))</li>
+    <li>@highlighter.highlightText(conciseAddress)</li>
 }
 @item.latestEvent.map { event =>
     <li title="@common.eventTitle(event)">

modules/portal/app/views/repository/searchItem.scala.html

@@ -4,8 +4,8 @@
     <img alt="Institution Logo" src="@item.data.logoUrl.getOrElse(controllers.portal.routes.PortalAssets.versioned("img/institution-icon.png"))"/>
 }
 
-@views.html.common.searchItemOutline(item, watched, image = views.html.helpers.linkTo(item, content = image)) {
-  @views.html.helpers.linkTo(item, content = Html(highlighter.highlight(item.toStringLang)))
+@views.html.common.searchItemOutline(item, watched, image = views.html.helpers.linkTo(item, htmlContent = image)) {
+  @views.html.helpers.linkTo(item, htmlContent = highlighter.highlightText(item.toStringLang))
 } {
     @listItemBody(item, showCountry, highlighter = highlighter)
 }

modules/portal/app/views/userProfile/annotation.scala.html

@@ -9,7 +9,7 @@
             </div>
         }
         @annotation.target.map { src =>
-            @views.html.helpers.linkToWithFragment(src, "#" + annotation.id, content = Html(highlighter.highlight(src.toStringLang)))
+            @views.html.helpers.linkToWithFragment(src, "#" + annotation.id, htmlContent = highlighter.highlightText(src.toStringLang))
             @annotation.data.field.map { field =>
             - <strong>@Messages(views.Helpers.prefixFor(src.isA) + "." + field)</strong>
             }

modules/portal/app/views/virtualUnit/infobar.scala.html

@@ -1,7 +1,7 @@
 @(item: Model, path: Seq[Model], watched: Boolean)(implicit userOpt: Option[UserProfile], req: RequestHeader, conf: AppConfig, messages: Messages)
 
 @linkVc(item: Model, path: Seq[Model]) = {
-    <li>@views.html.helpers.linkTo(item, call = Some(views.Helpers.virtualUnitUrl(path, item.id)), content = Html(item.toStringAbbr))</li>
+    <li>@views.html.helpers.linkTo(item, call = Some(views.Helpers.virtualUnitUrl(path, item.id)), textContent = Some(item.toStringAbbr))</li>
 }
 
 @common.infobar {

modules/portal/app/views/virtualUnit/searchItem.scala.html

@@ -6,7 +6,7 @@
             @wrapParent(doc, path.dropRight(1))
             <li>
                 @views.html.helpers.link(views.Helpers.virtualUnitUrl(path, item.id), Symbol("class") -> s"search-item-parent-name type-highlight ${p.isA}") {
-                    @Html(highlighter.highlight(p.toStringLang))
+                    @highlighter.highlightText(p.toStringLang)
                 }
             </li>
         }
@@ -19,7 +19,7 @@
             @wrapParent2(doc, p.parent)
         <li>
             @views.html.helpers.link(views.Helpers.virtualUnitUrl(path, item.id), Symbol("class") -> s"search-item-parent-name type-highlight ${p.isA}") {
-              @Html(highlighter.highlight(p.toStringLang))
+              @highlighter.highlightText(p.toStringLang)
             }
         </li>
         }
@@ -31,7 +31,7 @@
         @views.html.common.watchButtonsSmall(item, watched)
     </div>
     <h3 class="search-item-heading type-highlight @item.isA.toString">
-        <a class="@item.isA" href="@views.Helpers.virtualUnitUrl(path, item.id)">@Html(highlighter.highlight(item.toStringLang))</a>
+        <a class="@item.isA" href="@views.Helpers.virtualUnitUrl(path, item.id)">@highlighter.highlightText(item.toStringLang)</a>
     </h3>
     <div class="search-item-body">
         @views.html.virtualUnit.ifVirtual(item) { v =>

test/integration/portal/AnnotationsSpec.scala

@@ -34,7 +34,6 @@ class AnnotationsSpec extends IntegrationTestRunner {
         .withUser(unprivilegedUser).withCsrf.call()
       status(doc) must equalTo(OK)
       contentAsString(doc) must not contain testAnnotationBody
-
     }
 
     "allow annotating fields with correct visibility" in new ITestApp {

test/integration/portal/UserProfilesSpec.scala

@@ -116,7 +116,7 @@ class UserProfilesSpec extends IntegrationTestRunner with FakeMultipartUpload {
       val script = "<script>alert(\"Hello...\");</script>"
       val testInstitution = "Nefarious"
       val data = Map(
-        UserProfileF.NAME -> Seq(testName),
+        UserProfileF.NAME -> Seq(testName + script),
         UserProfileF.INSTITUTION -> Seq(testInstitution + script)
       )
       val update = FakeRequest(profileRoutes.updateProfilePost())
@@ -150,7 +150,6 @@ class UserProfilesSpec extends IntegrationTestRunner with FakeMultipartUpload {
       contentAsString(result) must contain(message("errors.badFileType"))
     }
 
-
     "allow uploading image files as profile image" in new ITestApp {
       val file = getProfileImage
       val stamp = FileStorage.fingerprint(file)