Permalink
Browse files

Escape HTML and JavaScript in the ui

Tests are for validation of the changes
  • Loading branch information...
1 parent e6eea02 commit 24106aa13ded590c0d9ab747d8bf1a149414244a @d2r d2r committed Feb 25, 2013
Showing with 64 additions and 8 deletions.
  1. +12 −8 src/clj/backtype/storm/ui/core.clj
  2. +52 −0 test/clj/backtype/storm/ui_test.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)
@@ -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>'
+;

0 comments on commit 24106aa

Please sign in to comment.