twitter-server: escape user input that is rendered in HTML, and make …

…bin/travisci publish finagle-toggle

Problem:

TwitterServer has many methods that generate HTML using unescaped user-defined
variables. For security reasons, it is best to escape these variables (see the
OWASP's XSS Prevention Cheat Sheet). Also, bin/travisci does not publish
finagle-toggle, so changes to finagle-toggle are not picked up.

Solution:

Escape string variables that represent user input using `xml.Utility.escape()`.
Add `./sbt ++$SCALA_VERSION finagle-toggle/publishLocal` to `bin/travisci`.

RB_ID=848579

CHANGES

@@ -7,6 +7,12 @@ Note that ``RB_ID=#`` correspond to associated messages in commits.
 1.x
 -----
 
+Bug Fixes
+~~~~~~~~~
+
+   * twitter-server: escape user input that is rendered in HTML, and make
+     bin/travisci publish finagle-toggle. ``RB_ID=848579``
+
 New Features
 ~~~~~~~~~~~~
 

bin/travisci

@@ -38,6 +38,7 @@ if [ "$TWITTER_SERVER_BRANCH" != "master" ]; then
   # finagle-thrift, finagle-zipkin
   cd $TWITTER_SERVER_TMP_DIR/finagle
   ./sbt ++$SCALA_VERSION finagle-thrift/publishLocal
+  ./sbt ++$SCALA_VERSION finagle-toggle/publishLocal
   ./sbt ++$SCALA_VERSION finagle-zipkin-core/publishLocal
   ./sbt ++$SCALA_VERSION finagle-zipkin/publishLocal
   # clean up

src/main/scala/com/twitter/server/handler/ClientRegistryHandler.scala

@@ -5,6 +5,7 @@ import com.twitter.finagle.client.{ClientRegistry, EndpointRegistry}
 import com.twitter.finagle.http.{Request, Response}
 import com.twitter.finagle.util.StackRegistry
 import com.twitter.io.Buf
+import com.twitter.server.util.HtmlUtils.escapeHtml
 import com.twitter.server.util.HttpUtils.{parse, new404, newResponse}
 import com.twitter.server.util.MetricSource
 import com.twitter.server.view.{EndpointRegistryView, StackRegistryView}
@@ -32,22 +33,22 @@ private object ClientRegistryHandler {
 
   /** Renders `profiles` in an html template. */
   def render(title: String, profiles: Seq[ClientProfile]): String =
-    s"""<h4 class="header text-center">${title}</h4>
+    s"""<h4 class="header text-center">${escapeHtml(title)}</h4>
         <hr/>
         <div id="clients" class="row">
         ${
           (for (ClientProfile(name, addr, scope, sr, unavailable) <- profiles) yield {
             s"""<div class="col-md-3">
                   <div class="client">
-                    <h4 class="name"><a href="/admin/clients/$name">${name}</a></h4>
-                    <p class="dest text-muted">$addr</p>
+                    <h4 class="name"><a href="/admin/clients/$name">${escapeHtml(name)}</a></h4>
+                    <p class="dest text-muted">${escapeHtml(addr)}</p>
                     ${
                       if (unavailable == 0) "" else {
                         s"""<a href="/admin/metrics#$scope/loadbalancer/available"
                             data-toggle="tooltip" data-placement="top"
                             class="conn-trouble btn-xs btn-default">
                             <span class="glyphicon glyphicon-exclamation-sign"
-                            aria-hidden="true"></span> $unavailable unavailable endpoint(s)</a>"""
+                            aria-hidden="true"></span> ${escapeHtml(unavailable.toString)} unavailable endpoint(s)</a>"""
                       }
                     }
                     <hr/>

src/main/scala/com/twitter/server/handler/EventRecordingHandler.scala

@@ -5,6 +5,7 @@ import com.twitter.finagle.http.{Request, Response}
 import com.twitter.io.Buf
 import com.twitter.util.Future
 import com.twitter.util.events.Sink
+import com.twitter.server.util.HtmlUtils.escapeHtml
 import com.twitter.server.util.HttpUtils.newResponse
 import java.util.logging.Logger
 
@@ -33,7 +34,7 @@ private[server] class EventRecordingHandler(
 
     newResponse(
       contentType = "text/html;charset=UTF-8",
-      content = Buf.Utf8(reply))
+      content = Buf.Utf8(escapeHtml(reply)))
   }
 
   private[handler] def updateRecording(uri: String): String = {

src/main/scala/com/twitter/server/handler/HistogramQueryHandler.scala

@@ -4,6 +4,7 @@ import com.twitter.finagle.http.{Request, Response}
 import com.twitter.finagle.Service
 import com.twitter.finagle.stats.{BucketAndCount, HistogramDetail, WithHistogramDetails}
 import com.twitter.io.Buf
+import com.twitter.server.util.HtmlUtils.escapeHtml
 import com.twitter.server.util.HttpUtils.{newResponse, parse}
 import com.twitter.server.util.JsonConverter
 import com.twitter.util.Future
@@ -55,7 +56,7 @@ object HistogramQueryHandler {
       val statsTable = {
         def entry(name: String): String = {
           s"""<tr>
-                <td>$name:</td>
+                <td>${escapeHtml(name)}:</td>
                 <td id=$name></td>
               </tr>"""
         }
@@ -120,7 +121,7 @@ object HistogramQueryHandler {
         <div class="col-md-4 snuggle-right">
           <ul id="metrics" class="list-unstyled">
             ${ (for (key <- keys.sorted) yield {
-                  s"""<li id="${key.replace("/", "-")}"><a id="special-$key">$key</a></li>"""
+                  s"""<li id="${key.replace("/", "-")}"><a id="special-$key">${escapeHtml(key)}</a></li>"""
                 }).mkString("\n") }
           </ul>
         </div>

src/main/scala/com/twitter/server/handler/IndexHandler.scala

@@ -3,6 +3,7 @@ package com.twitter.server.handler
 import com.twitter.finagle.Service
 import com.twitter.finagle.http.{HttpMuxer, Request, Response}
 import com.twitter.io.Buf
+import com.twitter.server.util.HtmlUtils.escapeHtml
 import com.twitter.server.util.HttpUtils.{expectsHtml, newOk, newResponse}
 import com.twitter.util.Future
 
@@ -17,7 +18,7 @@ class IndexHandler(
   extends Service[Request, Response] {
   def apply(req: Request): Future[Response] = {
     val paths = patterns.filter(_.startsWith(prefix))
-    val links = paths map { p => s"<a href='$p'>$p</a>" }
+    val links = paths map { p => s"<a href='$p'>${escapeHtml(p)}</a>" }
     if (!expectsHtml(req)) newOk(paths.mkString("\n"))
     else newResponse(
       contentType = "text/html;charset=UTF-8",

src/main/scala/com/twitter/server/handler/LintHandler.scala

@@ -4,6 +4,7 @@ import com.twitter.finagle.Service
 import com.twitter.finagle.http.{Response, Request}
 import com.twitter.io.Buf
 import com.twitter.server.handler.LintHandler.LintView
+import com.twitter.server.util.HtmlUtils.escapeHtml
 import com.twitter.server.util.HttpUtils._
 import com.twitter.server.util.JsonConverter
 import com.twitter.util.Future
@@ -113,9 +114,6 @@ private object LintHandler {
   });
 </script>"""
 
-    private def escapeHtml(s: String): String =
-      xml.Utility.escape(s)
-
     def summary: String = {
       val numIssues = nots.foldLeft(0) { case (total, (_, issues)) =>
         total + issues.size
@@ -132,19 +130,19 @@ private object LintHandler {
         <tbody>
           <tr>
             <td><strong>Number of rules run</strong></td>
-            <td>${rules.size}</td>
+            <td>${escapeHtml(rules.size.toString)}</td>
           </tr>
           <tr>
             <td><strong>Number of rules ok</strong></td>
-            <td>${oks.size}</td>
+            <td>${escapeHtml(oks.size.toString)}</td>
           </tr>
           <tr>
             <td><strong>Number of rules failed</strong></td>
-            <td>${nots.size}</td>
+            <td>${escapeHtml(nots.size.toString)}</td>
           </tr>
           <tr>
             <td><strong>Number of issues found</strong></td>
-            <td>$numIssues</td>
+            <td>${escapeHtml(numIssues.toString)}</td>
           </tr>
         </tbody>
       </table>
@@ -174,7 +172,7 @@ private object LintHandler {
     def failedRows: String = {
       val data = nots.map { case (rule, issues) =>
         issues.map { issue =>
-          failedRow(rule, issue)
+          escapeHtml(failedRow(rule, issue))
         }.mkString("")
       }.mkString("")
 

src/main/scala/com/twitter/server/handler/LoggingHandler.scala

@@ -4,6 +4,7 @@ import com.twitter.finagle.Service
 import com.twitter.finagle.http.{Request, Response}
 import com.twitter.io.Buf
 import com.twitter.logging.{Level, Logger}
+import com.twitter.server.util.HtmlUtils.escapeHtml
 import com.twitter.server.util.HttpUtils.{expectsHtml, newResponse, parse}
 import com.twitter.util.Future
 import java.net.URLEncoder
@@ -27,14 +28,14 @@ private object LoggingHandler {
   def renderText(loggers: Seq[Logger], updateMsg: String): String = {
     val out = loggers.toSeq.map { logger =>
       val loggerName = if (logger.name == "") "root" else logger.name
-      s"$loggerName ${getLevel(logger)}"
+      escapeHtml(s"$loggerName ${getLevel(logger)}")
     }.mkString("\n")
-    if (updateMsg.isEmpty) s"$out" else s"$updateMsg\n$out"
+    if (updateMsg.isEmpty) s"$out" else s"${escapeHtml(updateMsg)}\n$out"
   }
 
   def renderHtml(loggers: Seq[Logger], levels: Seq[Level], updateMsg: String): String =
     s"""<table class="table table-striped table-condensed">
-        <caption>$updateMsg</caption>
+        <caption>${escapeHtml(updateMsg)}</caption>
         <thead>
           <tr>
             <th>com.twitter.logging.Logger</th>
@@ -45,7 +46,7 @@ private object LoggingHandler {
           (for (logger <- loggers) yield {
             val loggerName = if (logger.name == "") "root" else logger.name
             s"""<tr>
-                <td><h5>$loggerName</h5></td>
+                <td><h5>${escapeHtml(loggerName)}</h5></td>
                 <td><div class="btn-group" role="group">
                   ${
                      (for (level <- levels) yield {
@@ -93,10 +94,10 @@ class LoggingHandler extends Service[Request, Response] {
           logger <- Logger.iterator.find(_.name == name)
         } yield {
           logger.setLevel(level)
-          s"""Changed ${if (logger.name == "") "root" else logger.name} to Level.$level"""
+          escapeHtml(s"""Changed ${if (logger.name == "") "root" else logger.name} to Level.$level""")
         }
 
-        updated.getOrElse(s"Unable to change $name to Level.$level!")
+        escapeHtml(updated.getOrElse(s"Unable to change $name to Level.$level!"))
 
       case _ => ""
     }

src/main/scala/com/twitter/server/handler/MetricQueryHandler.scala

@@ -3,6 +3,7 @@ package com.twitter.server.handler
 import com.twitter.finagle.Service
 import com.twitter.finagle.http.{Request, Response}
 import com.twitter.io.Buf
+import com.twitter.server.util.HtmlUtils.escapeHtml
 import com.twitter.server.util.HttpUtils.{newResponse, parse}
 import com.twitter.server.util.{JsonConverter, MetricSource}
 import com.twitter.util.Future
@@ -16,7 +17,7 @@ private object MetricQueryHandler {
           <div class="col-md-4 snuggle-right">
             <ul id="metrics" class="list-unstyled">
               ${ (for (key <- keys.toSeq.sorted) yield {
-                    s"""<li id="${key.replace("/", "-")}">$key</li>"""
+                    s"""<li id="${key.replace("/", "-")}">${escapeHtml(key)}</li>"""
                   }).mkString("\n") }
             </ul>
           </div>

src/main/scala/com/twitter/server/handler/ServerRegistryHandler.scala

@@ -5,6 +5,7 @@ import com.twitter.finagle.http.{Request, Response}
 import com.twitter.finagle.server.ServerRegistry
 import com.twitter.finagle.util.StackRegistry
 import com.twitter.io.Buf
+import com.twitter.server.util.HtmlUtils.escapeHtml
 import com.twitter.server.util.HttpUtils.{new404, newResponse, parse}
 import com.twitter.server.util.MetricSource
 import com.twitter.server.view.StackRegistryView
@@ -20,7 +21,7 @@ private object ServerRegistryHandler {
              (for {
                  (scope, entry) <- servers
                } yield {
-                 s"""<li><a href="#${entry.name}-entry" data-toggle="tab">$scope</a></li>"""
+                 s"""<li><a href="#${entry.name}-entry" data-toggle="tab">${escapeHtml(scope)}</a></li>"""
              }).mkString("\n")
            }
         </ul>

src/main/scala/com/twitter/server/util/HtmlUtils.scala

@@ -0,0 +1,8 @@
+package com.twitter.server.util
+
+private[server] object HtmlUtils {
+
+  private[server] def escapeHtml(s: String) : String =
+    scala.xml.Utility.escape(s)
+
+}

src/main/scala/com/twitter/server/view/IndexView.scala

@@ -4,6 +4,7 @@ import com.twitter.concurrent.AsyncStream
 import com.twitter.finagle.{Service, SimpleFilter}
 import com.twitter.finagle.http.{Request, Response}
 import com.twitter.io.{Reader, Buf}
+import com.twitter.server.util.HtmlUtils.escapeHtml
 import com.twitter.server.util.HttpUtils.{expectsHtml, newResponse}
 import com.twitter.util.Future
 
@@ -35,7 +36,7 @@ object IndexView {
           sb ++= s"""
             <a href="${href}">
               <li id="${id}" class="selectable $selected">
-                ${id}
+                ${escapeHtml(id)}
               </li>
             </a>
             """
@@ -55,7 +56,7 @@ object IndexView {
             <li class="subnav $active">
               <div class="subnav-title selectable">
                 <span class="glyphicon glyphicon-expand $collapse"></span>
-                <span>${id}</span>
+                <span>${escapeHtml(id)}</span>
               </div>
               <ul>${renderNav(links)}</ul>
             </li>"""

src/main/scala/com/twitter/server/view/StackRegistryView.scala

@@ -1,6 +1,7 @@
 package com.twitter.server.view
 
 import com.twitter.finagle.util.StackRegistry
+import com.twitter.server.util.HtmlUtils.escapeHtml
 
 private[server] object StackRegistryView {
   /**
@@ -18,23 +19,23 @@ private[server] object StackRegistryView {
     def renderParams(params: Seq[(String, String)]): String =
       (for ((field, value) <- params) yield {
         s"""<tr>
-              <td>${field}</td>
-              <td>${value}</td>
+              <td>${escapeHtml(field)}</td>
+              <td>${escapeHtml(value)}</td>
             </tr>"""
       }).mkString("\n")
 
     val modules = entry.modules
 
     s"""<h2>
-          ${entry.name}
-          <small>${entry.addr}</small>
+          ${escapeHtml(entry.name)}
+          <small>${escapeHtml(entry.addr)}</small>
         </h2>
         ${
            (for (scope <- statScope) yield {
             s"""<a href="/admin/metrics#${scope}/requests"
                 class="btn btn-default">
               <span class="glyphicon glyphicon-stats"></span>
-                Watch metrics for ${entry.name}
+                Watch metrics for ${escapeHtml(entry.name)}
               </a>"""
            }).getOrElse("")
          }
@@ -47,7 +48,7 @@ private[server] object StackRegistryView {
                 <ul class="nav nav-tabs">
                   ${
                      (for (StackRegistry.Module(role, _, _) <- modules) yield {
-                        s"""<li><a href="#${role}-module" data-toggle="tab">${role}</a></li>"""
+                        s"""<li><a href="#${role}-module" data-toggle="tab">${escapeHtml(role)}</a></li>"""
                       }).mkString("\n")
                    }
                 </ul>
@@ -59,8 +60,8 @@ private[server] object StackRegistryView {
                     s"""<div class="tab-pane" id="${role}-module">
                           <div style="display:inline-block; min-width:50%">
                             <div class="panel panel-default">
-                              <div class="panel-heading">${role}</div>
-                              <div class="panel-body"><p>${desc}</p></div>
+                              <div class="panel-heading">${escapeHtml(role)}</div>
+                              <div class="panel-body"><p>${escapeHtml(desc)}</p></div>
                               ${
                                 if (params.isEmpty) "" else {
                                    s"""<table class="table table-condensed table-bordered">

src/main/scala/com/twitter/server/view/ThreadsView.scala

@@ -1,6 +1,8 @@
 package com.twitter.server.view
 
 import com.twitter.server.handler.ThreadsHandler.{StackTrace, ThreadInfo}
+import com.twitter.server.util.HtmlUtils.escapeHtml
+import com.twitter.server.view.ThreadsView._
 
 private object ThreadsView {
 
@@ -58,11 +60,6 @@ private[server] class ThreadsView(
     all: Seq[ThreadInfo],
     deadlockedIds: Seq[Long])
 {
-  import com.twitter.server.view.ThreadsView._
-
-  private def escapeHtml(s: String): String =
-    xml.Utility.escape(s)
-
   private def summary: String = {
     val filtered = all.filter { info =>
       deadlockedIds.contains(info.thread.getId)
@@ -71,7 +68,7 @@ private[server] class ThreadsView(
     val deadlockLinks = if (filtered.isEmpty) "none" else {
       filtered.map { info =>
         val id = info.thread.getId
-        s"""<a href='#threadId-$id'>$id</a>"""
+        s"""<a href='#threadId-$id'>${escapeHtml(id.toString)}</a>"""
       }.mkString(", ")
     }
 
@@ -119,7 +116,7 @@ private[server] class ThreadsView(
     val daemonText =
       if (thread.isDaemon) "Yes" else "<strong><span class=\"text-danger\">No</span></strong>"
     val threadState =
-      s"${if (t.isIdle) "Idle" else "Active"} <small>(${thread.getState})</small>"
+      s"${if (t.isIdle) "Idle" else "Active"} <small>(${escapeHtml(thread.getState.toString)})</small>"
     val domId = s"threadId-${thread.getId}"
     val stackDomId = s"threadId-stack-${thread.getId}"
 
@@ -130,11 +127,11 @@ private[server] class ThreadsView(
           <span class="glyphicon glyphicon-expand"></span> <span class="text-muted"><small>Stack</small></span>
         </a>
       </td>
-      <td>${thread.getId}</td>
-      <td>${escapeHtml(thread.getName)}</td>
-      <td>$threadState</td>
-      <td>$daemonText</td>
-      <td>${thread.getPriority}</td>
+      <td>${thread.getId.toString}</td>
+      <td>${thread.getName}</td>
+      <td>${threadState}</td>
+      <td>${daemonText}</td>
+      <td>${thread.getPriority.toString}</td>
     </tr>
     <tr id="$stackDomId" class="$rowClassStyle $rowClassStackIdle $StackTraceRowClass">
       <td colspan="6">