Merge pull request #2945 from sadielbartholomew/web-security-autoesca…

…ping review: security patch to prevent code injection
cylc · Feb 15, 2019 · fa1fec8 · fa1fec8
2 parents 6dea703 + 1ba9521
commit fa1fec8
Show file tree

Hide file tree

Showing 6 changed files with 65 additions and 56 deletions.
diff --git a/lib/cylc/cylc-review/template/cycles.html b/lib/cylc/cylc-review/template/cycles.html
@@ -38,8 +38,8 @@
                 <div class="input-group">
                   <label for="order">Sort Order</label>
                   <select name="order" title="Sort Order" class="form-control">
-                  {% for k, v in [("time_desc", "new-&gt;old"),
-                                  ("time_asc", "old-&gt;new")] -%}
+                  {% for k, v in [("time_desc", "new-&gt;old" | safe ),
+                                  ("time_asc", "old-&gt;new" | safe )] -%}
                     <option
                     {% if order and order == k -%}selected="selected"{% endif -%}
                     value="{{k}}">{{v}}</option>
@@ -140,8 +140,8 @@
 {% for entry in entries -%}
   {% set cycle_in_url = entry.cycle|replace('+', '%2B') -%}
   {% set task_jobs_url = (
-      script ~ "/taskjobs/" ~ user ~ "?&amp;suite=" ~
-      suite|replace('+', '%2F') ~ "&amp;cycles=" ~ cycle_in_url
+      (script ~ "/taskjobs/" ~ user ~ "?&amp;suite=" ~
+       suite|replace('+', '%2F') ~ "&amp;cycles=" ~ cycle_in_url) | safe
   ) -%}
     <tr class="entry">
 
@@ -162,21 +162,22 @@
   {# task and jobs states -#}
   {% for state, icon, label, title_, unit1, unit2, url_arg in [
       ("active", "play", "info", "active", "task", "tasks",
-       "&amp;task_status=" ~
-       task_status_groups["active"]|join("&amp;task_status=")),
+       ("&amp;task_status=" ~
+        task_status_groups["active"]|join("&amp;task_status=")) | safe ),
       ("job_active", "play-circle", "info", "active", "job", "jobs",
-       "&amp;job_status=submitted,running"),
+       "&amp;job_status=submitted,running" | safe ),
       ("success", "ok", "success", "succeeded", "task", "tasks",
-       "&amp;task_status=" ~
-       task_status_groups["success"]|join("&amp;task_status=")),
+       ("&amp;task_status=" ~
+        task_status_groups["success"]|join("&amp;task_status=")) | safe ),
       ("job_success", "ok-circle", "success", "succeeded", "job", "jobs",
-       "&amp;job_status=succeeded"),
+       "&amp;job_status=succeeded" | safe ),
       ("fail", "remove", "danger", "failed", "task", "tasks",
-       "&amp;task_status=" ~
-       task_status_groups["fail"]|join("&amp;task_status=")),
+       ("&amp;task_status=" ~
+        task_status_groups["fail"]|join("&amp;task_status=")) | safe ),
       ("job_fail", "remove-circle", "danger", "failed", "job", "jobs",
-       "&amp;job_status=submission-failed,failed"),
+       "&amp;job_status=submission-failed,failed" | safe ),
   ] -%}
+
     {% set n_state = entry.n_states[state] -%}
     {% set unit = unit1 -%}
     {% if n_state -%}
@@ -206,8 +207,9 @@
     <td>
   {% if entry.has_log_job_tar_gz -%}
     {% set download = (
-        script ~ "/view/" ~ user ~ "?&amp;suite=" ~ suite|replace('+', '%2F') ~
-        "&amp;path=log/job-" ~ cycle_in_url ~ ".tar.gz&amp;mode=download"
+        (script ~ "/view/" ~ user ~ "?&amp;suite=" ~
+         suite|replace('+', '%2F') ~ "&amp;path=log/job-" ~ cycle_in_url ~
+         ".tar.gz&amp;mode=download") | safe
     ) -%}
       <a href="{{download}}"
       download="{{user}}-{{suite|replace("/", "%2F")}}-log-job-{{entry.cycle}}.tar.gz">

diff --git a/lib/cylc/cylc-review/template/job-entry.html b/lib/cylc/cylc-review/template/job-entry.html
@@ -5,10 +5,12 @@
 {% endif -%}
 {% set cycle_str = entry.cycle|replace('+', '%2B') -%}
 {% set taskjobs_link = (
-    script ~ "/taskjobs/" ~ user ~ "?&amp;suite=" ~ suite|replace('+', '%2F') ~ no_fuzzy_time_str
+    (script ~ "/taskjobs/" ~ user ~ "?&amp;suite=" ~
+     suite|replace('+', '%2F') ~ no_fuzzy_time_str) | safe
 ) -%}
 {% set view_link = (
-    script ~ "/view/" ~ user ~ "?&amp;suite=" ~ suite|replace('+', '%2F') ~ no_fuzzy_time_str
+    (script ~ "/view/" ~ user ~ "?&amp;suite=" ~ suite|replace('+', '%2F') ~
+     no_fuzzy_time_str) | safe
 ) -%}
 <tr class="entry"><!-- entry row -->
   <td>
@@ -17,22 +19,22 @@
       {% set icon = "ok" %}
       {% set label_class = "label-success" %}
       {% set url_arg = (
-        "&amp;task_status=" ~
-        task_status_groups["success"]|join("&amp;task_status=")
+        ("&amp;task_status=" ~
+         task_status_groups["success"]|join("&amp;task_status=")) | safe
       ) -%}
     {% elif entry.task_status in ["failed", "submission failed"] -%}
       {% set icon = "remove" %}
       {% set label_class = "label-danger" %}
       {% set url_arg = (
-        "&amp;task_status=" ~
-        task_status_groups["fail"]|join("&amp;task_status=")
+        ("&amp;task_status=" ~
+         task_status_groups["fail"]|join("&amp;task_status=")) | safe
       ) -%}
     {% else -%}
       {% set icon = "play" %}
       {% set label_class = "label-info" %}
       {% set url_arg = (
-        "&amp;task_status=" ~
-        task_status_groups["active"]|join("&amp;task_status=")
+        ("&amp;task_status=" ~
+         task_status_groups["active"]|join("&amp;task_status=")) | safe
       ) -%}
     {% endif -%}
     <small>
@@ -47,8 +49,8 @@
     <!-- entry: submit_status, run_status -->
     <small>
     {% set link = (
-        script ~ "/taskjobs/" ~ user ~ "?&amp;suite=" ~ suite|replace('+', '%2F') ~
-        no_fuzzy_time_str
+        (script ~ "/taskjobs/" ~ user ~ "?&amp;suite=" ~
+         suite|replace('+', '%2F') ~ no_fuzzy_time_str) | safe
     ) -%}
     {% if entry.run_status == 0 -%}
       <a href="{{taskjobs_link}}&amp;job_status=succeeded"
@@ -161,7 +163,7 @@
         <ul class="list-inline">
           {% for key, log in entry.logs|dictsort if not log.seq_key -%}
             {% if key in ["job.out", "job.err"] -%}
-              {% set key_str = "<strong>" ~ key ~ "</strong>" -%}
+              {% set key_str = ("<strong>" ~ key ~ "</strong>") | safe -%}
             {% else -%}
               {% set key_str = key -%}
             {% endif -%}

diff --git a/lib/cylc/cylc-review/template/suites.html b/lib/cylc/cylc-review/template/suites.html
@@ -96,8 +96,8 @@
                   <label for="order">Sort Order</label>
                   <select name="order" title="Sort Order" class="form-control">
                   {% for k, v in [
-                      ("time_desc", "new-&gt;old"),
-                      ("time_asc", "old-&gt;new"),
+                      ("time_desc", "new-&gt;old" | safe ),
+                      ("time_asc", "old-&gt;new" | safe ),
                       ("name_asc", "a-z suite name"),
                       ("name_desc", "z-a suite name"),
                   ] -%}

diff --git a/lib/cylc/cylc-review/template/taskjobs.html b/lib/cylc/cylc-review/template/taskjobs.html
@@ -24,8 +24,8 @@
         <fieldset class="container-fluid">
           <div class="row">
 {% for key, name, value in [
-    ("cycles",
-     "Cycles (before, after or patterns): &lt;CYCLE | &gt;CYCLE | GLOB ...", cycles),
+    ("cycles", "Cycles (before, after or patterns):" ~
+     "&lt;CYCLE | &gt;CYCLE | GLOB ..." | safe , cycles),
     ("tasks", "Task Name Globs", tasks),
 ] -%}
             <div class="form-group col-sm-12 col-md-6">
@@ -107,28 +107,28 @@
               <select id="order" name="order" title="Sort Order"
               class="form-control">
 {% for k, v in [
-    ("time_desc", "new-&gt;old"),
-    ("time_asc", "old-&gt;new"),
-    ("cycle_desc_name_asc", "new-&gt;old cycle, a-z task name"),
-    ("cycle_desc_name_desc", "new-&gt;old cycle, z-a task name"),
-    ("cycle_asc_name_asc", "old-&gt;new cycle, a-z task name"),
-    ("cycle_asc_name_desc", "old-&gt;new cycle, z-a task name"),
-    ("name_asc_cycle_desc", "a-z task name, new-&gt;old cycle"),
-    ("name_desc_cycle_desc", "z-a task name, new-&gt;old cycle"),
-    ("name_asc_cycle_asc", "a-z task name, old-&gt;new cycle"),
-    ("name_desc_cycle_asc", "z-a task name, old-&gt;new cycle"),
-    ("time_submit_desc", "submit time, new-&gt;old"),
-    ("time_submit_asc", "submit time, old-&gt;new"),
-    ("time_run_desc", "run start time, new-&gt;old"),
-    ("time_run_asc", "run start time, old-&gt;new"),
-    ("time_run_exit_desc", "run exit time, new-&gt;old"),
-    ("time_run_exit_asc", "run exit time, old-&gt;new"),
-    ("duration_queue_desc", "queue duration, long-&gt;short"),
-    ("duration_queue_asc", "queue duration, short-&gt;long"),
-    ("duration_run_desc", "run duration, long-&gt;short"),
-    ("duration_run_asc", "run duration, short-&gt;long"),
-    ("duration_queue_run_desc", "queue+run duration, long-&gt;short"),
-    ("duration_queue_run_asc", "queue+run duration, short-&gt;long"),
+    ("time_desc", "new-&gt;old" | safe ),
+    ("time_asc", "old-&gt;new" | safe ),
+    ("cycle_desc_name_asc", "new-&gt;old cycle, a-z task name" | safe ),
+    ("cycle_desc_name_desc", "new-&gt;old cycle, z-a task name" | safe ),
+    ("cycle_asc_name_asc", "old-&gt;new cycle, a-z task name" | safe ),
+    ("cycle_asc_name_desc", "old-&gt;new cycle, z-a task name" | safe ),
+    ("name_asc_cycle_desc", "a-z task name, new-&gt;old cycle" | safe ),
+    ("name_desc_cycle_desc", "z-a task name, new-&gt;old cycle" | safe ),
+    ("name_asc_cycle_asc", "a-z task name, old-&gt;new cycle" | safe ),
+    ("name_desc_cycle_asc", "z-a task name, old-&gt;new cycle" | safe ),
+    ("time_submit_desc", "submit time, new-&gt;old" | safe ),
+    ("time_submit_asc", "submit time, old-&gt;new" | safe ),
+    ("time_run_desc", "run start time, new-&gt;old" | safe ),
+    ("time_run_asc", "run start time, old-&gt;new" | safe ),
+    ("time_run_exit_desc", "run exit time, new-&gt;old" | safe ),
+    ("time_run_exit_asc", "run exit time, old-&gt;new" | safe ),
+    ("duration_queue_desc", "queue duration, long-&gt;short" | safe ),
+    ("duration_queue_asc", "queue duration, short-&gt;long" | safe ),
+    ("duration_run_desc", "run duration, long-&gt;short" | safe ),
+    ("duration_run_asc", "run duration, short-&gt;long" | safe ),
+    ("duration_queue_run_desc", "queue+run duration, long-&gt;short" | safe ),
+    ("duration_queue_run_asc", "queue+run duration, short-&gt;long" | safe ),
 ] -%}
                 <option
 {% if order and order == k -%}

diff --git a/lib/cylc/cylc-review/template/view.html b/lib/cylc/cylc-review/template/view.html
@@ -71,9 +71,9 @@ <h1>{{path}}</h1>
     'WARNING', '<span class="nocode text-warning">WARNING</span>') -%}
 {% endif -%}
 {% if mode == "tags" -%}
-{{line}}
+{{line|safe}}
 {% else -%}
-{{line|urlise}}
+{{ (line|urlise) | safe }}
 {% endif -%}
 {% endfor -%}
 </pre>

diff --git a/lib/cylc/review.py b/lib/cylc/review.py
@@ -78,8 +78,13 @@ def __init__(self, *args, **kwargs):
         if self.host_name and "." in self.host_name:
             self.host_name = self.host_name.split(".", 1)[0]
         self.cylc_version = CYLC_VERSION
-        template_env = jinja2.Environment(loader=jinja2.FileSystemLoader(
-            get_util_home("lib", "cylc", "cylc-review", "template")))
+        # Autoescape markup to prevent code injection from user inputs.
+        template_env = jinja2.Environment(
+            loader=jinja2.FileSystemLoader(
+                get_util_home("lib", "cylc", "cylc-review", "template")),
+            autoescape=jinja2.select_autoescape(
+                enabled_extensions=('html', 'xml'), default_for_string=True),
+        )
         template_env.filters['urlise'] = self.url2hyperlink
         self.template_env = template_env