Escape HTML and JavaScript in the ui

Tests are for validation of the changes

src/clj/backtype/storm/ui/core.clj

@@ -16,6 +16,7 @@
             [compojure.handler :as handler]
             [ring.util.response :as resp]
             [backtype.storm [thrift :as thrift]])
+  (:import [org.apache.commons.lang StringEscapeUtils])
   (:gen-class))
 
 (def ^:dynamic *STORM-CONF* (read-storm-config))
@@ -85,7 +86,7 @@
 (defn topology-link
   ([id] (topology-link id id))
   ([id content]
-     (link-to (url-format "/topology/%s" id) content)))
+     (link-to (url-format "/topology/%s" id) (escape-html content))))
 
 (defn main-topology-summary-table [summs]
   ;; make the id clickable
@@ -94,7 +95,7 @@
    ["Name" "Id" "Status" "Uptime" "Num workers" "Num executors" "Num tasks"]
    (for [^TopologySummary t summs]
      [(topology-link (.get_id t) (.get_name t))
-      (.get_id t)
+      (escape-html (.get_id t))
       (.get_status t)
       (pretty-uptime-sec (.get_uptime_secs t))
       (.get_num_workers t)
@@ -301,8 +302,8 @@
   (let [executors (.get_executors summ)
         workers (set (for [^ExecutorSummary e executors] [(.get_host e) (.get_port e)]))]
     (table ["Name" "Id" "Status" "Uptime" "Num workers" "Num executors" "Num tasks"]
-           [[(.get_name summ)
-             (.get_id summ)
+           [[(escape-html (.get_name summ))
+             (escape-html (.get_id summ))
              (.get_status summ)
              (pretty-uptime-sec (.get_uptime_secs summ))
              (count workers)
@@ -376,7 +377,7 @@
       )))
 
 (defn component-link [storm-id id]
-  (link-to (url-format "/topology/%s/component/%s" storm-id id) id))
+  (link-to (url-format "/topology/%s/component/%s" storm-id id) (escape-html id)))
 
 (defn render-capacity [capacity]
   (let [capacity (nil-to-zero capacity)]
@@ -463,7 +464,10 @@
   [:input {:type "button"
            :value action
            (if enabled :enabled :disabled) ""
-           :onclick (str "confirmAction('" id "', '" name "', '" command "', " is-wait ", " default-wait ")")}])
+           :onclick (str "confirmAction('" 
+                         (StringEscapeUtils/escapeJavaScript id) "', '" 
+                         (StringEscapeUtils/escapeJavaScript name) "', '"
+                         command "', " is-wait ", " default-wait ")")}])
 
 (defn topology-page [id window include-sys?]
   (with-nimbus nimbus
@@ -609,7 +613,7 @@
     (sorted-table
      ["Component" "Stream" "Execute latency (ms)" "Executed" "Process latency (ms)" "Acked" "Failed"]
      (for [[^GlobalStreamId s stats] stream-summary]
-       [(.get_componentId s)
+       [(escape-html (.get_componentId s))
         (.get_streamId s)
         (float-str (:execute-latencies stats))
         (nil-to-zero (:executed stats))
@@ -712,7 +716,7 @@
       (concat
        [[:h2 "Component summary"]
         (table ["Id" "Topology" "Executors" "Tasks"]
-               [[component
+               [[(escape-html component)
                  (topology-link (.get_id summ) (.get_name summ))
                  (count summs)
                  (sum-tasks summs)

test/clj/backtype/storm/ui_test.clj

@@ -0,0 +1,52 @@
+(ns backtype.storm.ui-test
+  (:use [backtype.storm.ui core])
+  (:use [clojure test])
+  (:use [hiccup.core :only (html)])
+)
+
+(deftest test-javascript-escaped-in-action-buttons
+  (let [expected-s "confirmAction('XX\\\"XX\\'XX\\\\XX\\/XX\\nXX', 'XX\\\"XX\\'XX\\\\XX\\/XX\\nXX', 'activate', false, 0)"
+        malicious-js "XX\"XX'XX\\XX/XX
+XX"
+        result (topology-action-button malicious-js malicious-js
+                                "Activate" "activate" false 0 true)
+        onclick (:onclick (second result))]
+
+    (is (= expected-s onclick)
+        "Escapes quotes, slashes, back-slashes, and new-lines.")
+  )
+)
+
+(deftest test-topology-link-escapes-content-html
+  (let [topo-name "BOGUSTOPO"]
+    (is (= (str "<a href=\"/topology/" topo-name "\">&lt;BLINK&gt;foobar</a>")
+           (html (topology-link topo-name "<BLINK>foobar"))))
+  )
+)
+
+(deftest test-component-link-escapes-content-html
+  (let [topo-name "BOGUSTOPO"]
+    (is (= (str "<a href=\"/topology/" topo-name "/component/%3CBLINK%3Ecomp-id\">&lt;BLINK&gt;comp-id</a>")
+           (html (component-link topo-name "<BLINK>comp-id"))))
+  )
+)
+
+; main-topology-summary-table
+; submit topo name like "<BLINK>foobar"
+; Load / and visually confirm the 'id' column does not blink for the topo.
+
+; topology-summary-table
+; submit topo name like "<BLINK>foobar"
+; Load / and visually confirm the 'id' column does not blink for the topo or the name
+
+; topology-summary-table
+; submit topo name like "<BLINK>foobar"
+; Load / and visually confirm the 'id' column does not blink for the topo or the name
+
+; component-page
+; recompile a topology (such as the ExclamationTopology from storm-starter) and hardcode bolt/spout names with '<blink>' 
+; Load the bolt or spout component page and visually confirm the 'id' column does not blink the component name.
+
+; bolt-input-summary-table
+; recompile a topology (such as the ExclamationTopology from storm-starter) and hardcode bolt/spout names with '<blink>' 
+;