Don't use the `|safe` filter in code, it's risky (#9180)

Most things already use the `Markup` class to correctly escape problem
areas, this commit just fixes the last instances so that we can assert
that `|safe` is never used.

.pre-commit-config.yaml

@@ -246,6 +246,13 @@ metastore_browser/templates/.*\\.html$|.*\\.jinja2"
         entry: "pydevd.*settrace\\("
         pass_filenames: true
         files: \.py$
+      - id: dont-use-safe-filter
+        language: pygrep
+        name: Don't use safe in templates
+        description: the Safe filter is error-prone, use Markup() in code instead
+        entry: "\\|\\s*safe"
+        files: \.html$
+        pass_filenames: true
       - id: language-matters
         language: pygrep
         name: Check for language that we do not accept as community

airflow/contrib/plugins/metastore_browser/main.py

@@ -23,7 +23,7 @@ from datetime import datetime
 from typing import List
 
 import pandas as pd
-from flask import Blueprint, request
+from flask import Blueprint, Markup, request
 from flask_appbuilder import BaseView, expose
 
 from airflow.plugins_manager import AirflowPlugin
@@ -76,7 +76,7 @@ class MetastoreBrowserView(BaseView):
             escape=False,
             na_rep='',)
         return self.render_template(
-            "metastore_browser/dbs.html", table=table)
+            "metastore_browser/dbs.html", table=Markup(table))
 
     @expose('/table/')
     def table(self):

airflow/contrib/plugins/metastore_browser/templates/metastore_browser/dbs.html

@@ -23,5 +23,5 @@
     <h4>
         <span>Hive Databases</span>
     </h4>
-    {{ table|safe }}
+    {{ table }}
 {% endblock %}

airflow/www/static/css/main.css

@@ -291,6 +291,10 @@ label[for="timezone-other"],
   display: block;
 }
 
+.dag-doc {
+  margin-bottom: 15px;
+}
+
 /* stylelint-disable declaration-block-single-line-max-declarations */
 .hll { background-color: #ffc; }
 .c { color: #408080; font-style: italic; } /* Comment */

airflow/www/templates/airflow/chart.html

@@ -45,7 +45,7 @@
         <input name="_csrf_token" type="hidden" value="{{ csrf_token() }}">
     </form>
 </div>
-<div style="clear: both;">{{ chart |safe }}</div>
+<div style="clear: both;">{{ chart }}</div>
 <hr/>
 {% endblock %}
 

airflow/www/templates/airflow/code.html

@@ -38,6 +38,6 @@
     {% endif %}
 
     {% if code_html %}
-        {{ code_html|safe }}
+        {{ code_html }}
     {% endif %}
 {% endblock %}

airflow/www/templates/airflow/config.html

@@ -36,7 +36,7 @@
     {% endif %}
 
     {% if code_html %}
-        {{ code_html|safe }}
+        {{ code_html }}
     {% endif %}
 
     <hr>

airflow/www/templates/airflow/dag.html

@@ -427,8 +427,8 @@ function updateQueryStringParameter(uri, key, value) {
       });
       subdag_id = sd;
       execution_date = d;
-      $('#task_id').html(t);
-      $('#execution_date').html(d);
+      $('#task_id').text(t);
+      $('#execution_date').text(d);
       $('#myModal').modal({});
       $("#myModal").css("margin-top","0px");
       $('#extra_links').prev('hr').hide();
@@ -527,7 +527,7 @@ function updateQueryStringParameter(uri, key, value) {
     function call_modal_dag(dag) {
       id = dag && dag.id;
       execution_date = dag && dag.execution_date;
-      $('#dag_id').html(dag_id);
+      $('#dag_id').text(dag_id);
       $('#dagModal').modal({});
       $("#dagModal").css("margin-top","0px");
     }

airflow/www/templates/airflow/dag_code.html

@@ -27,7 +27,7 @@
     <div class="active">
       <a onclick="toggleWrap()">Toggle wrap</a>
     </div>
-    {{ html_code|safe }}
+    {{ html_code }}
 {% endblock %}
 
 {% block tail %}

airflow/www/templates/airflow/duration_chart.html

@@ -48,8 +48,8 @@
         <input name="_csrf_token" type="hidden" value="{{ csrf_token() }}">
     </form>
 </div>
-<div id="dur_chart" style="clear: both;">{{ chart |safe }}</div>
-<div id="cum_dur_chart" style="clear: both;">{{ cum_chart | safe}}</div>
+<div id="dur_chart" style="clear: both;">{{ chart }}</div>
+<div id="cum_dur_chart" style="clear: both;">{{ cum_chart }}</div>
 <hr/>
 {% endblock %}
 

airflow/www/templates/airflow/gantt.html

@@ -34,7 +34,7 @@
     Base date: {{ form.base_date(class_="form-control") }}
     Number of runs: {{ form.num_runs(class_="form-control") }}
     Run:<input type="hidden" value="{{ dag.dag_id }}" name="dag_id">
-    {{ form.execution_date(class_="form-control") | safe }}
+    {{ form.execution_date(class_="form-control") }}
     <input type="submit" value="Go" class="btn btn-default" action="" method="get">
     <input type="hidden" name="root" value="{{ root if root else '' }}">
     <input name="_csrf_token" type="hidden" value="{{ csrf_token() }}">
@@ -57,7 +57,7 @@
     var dag_id = '{{ dag.dag_id }}';
     var task_id = '';
     var execution_date = '';
-    data = {{ data |tojson|safe }};
+    data = {{ data |tojson }};
     var gantt = d3.gantt()
       .taskTypes(data.taskNames)
       .height(data.height)

airflow/www/templates/airflow/graph.html

@@ -29,7 +29,7 @@
 {% block content %}
 {{ super() }}
 {% if doc_md %}
-<div class="rich_doc" style="margin-bottom: 15px;">{{ doc_md|safe }}</div>
+{{ doc_md }}
 {% endif %}
 <div class="form-inline">
   <form method="get" style="float:left;">
@@ -37,9 +37,9 @@
     Base date: {{ form.base_date(class_="form-control") }}
     Number of runs: {{ form.num_runs(class_="form-control") }}
     Run:
-    {{ form.execution_date(class_="form-control") | safe }}
+    {{ form.execution_date(class_="form-control") }}
     Layout:
-    {{ form.arrange(class_="form-control") | safe }}
+    {{ form.arrange(class_="form-control") }}
     <input type="hidden" name="root" value="{{ root }}">
     <input type="hidden" value="{{ dag.dag_id }}" name="dag_id">
     <input name="_csrf_token" type="hidden" value="{{ csrf_token() }}">
@@ -107,14 +107,14 @@
     var initialStrokeWidth = '3px';
     var highlightStrokeWidth = '5px';
 
-    var nodes = {{ nodes|tojson|safe }};
-    var edges = {{ edges|tojson|safe }};
+    var nodes = {{ nodes|tojson }};
+    var edges = {{ edges|tojson }};
     var execution_date = "{{ execution_date }}";
     var arrange = "{{ arrange }}";
 
     // Below variables are being used in dag.js
-    var tasks = {{ tasks|tojson|safe }};
-    var task_instances = {{ task_instances|tojson|safe }};
+    var tasks = {{ tasks|tojson }};
+    var task_instances = {{ task_instances|tojson }};
     var getTaskInstanceURL = "{{ url_for('Airflow.task_instances') }}" +
       "?dag_id=" + encodeURIComponent(dag_id) + "&execution_date=" +
       encodeURIComponent(execution_date);

airflow/www/templates/airflow/model_list.html

@@ -79,7 +79,7 @@
                     {% if formatter and formatter(item) %}
                         <td>{{ formatter(item) }}</td>
                     {% elif item[value] != None %}
-                        <td>{{ item[value]|safe }}</td>
+                        <td>{{ item[value] }}</td>
                     {% else %}
                         <td></td>
                     {% endif %}

airflow/www/templates/airflow/task.html

@@ -39,14 +39,11 @@
                 </tr>
             {% endfor %}
         </table>
-        {% if html_code is defined %}
-            {{ html_code|safe }}
-        {% endif %}
     </div>
     <div>
         {% for attr, value in special_attrs_rendered.items() %}
             <h5>Attribute: {{ attr }}</h5>
-            {{ value|safe }}
+            {{ value }}
         {% endfor %}
         <h5>Task Instance Attributes</h5>
         <table class="table table-striped table-bordered">
@@ -74,8 +71,5 @@
                 </tr>
             {% endfor %}
         </table>
-        {% if html_code is defined %}
-            {{ html_code|safe }}
-        {% endif %}
     </div>
 {% endblock %}

airflow/www/templates/airflow/task_instance.html

@@ -31,7 +31,7 @@
               {{ task_id }}
               <input type="hidden" value="{{ task_id }}" name="task_id">
             </span>
-        {{ form.execution_date(class_="form-control") | safe }}
+        {{ form.execution_date(class_="form-control") }}
 
     </div>
 </form>

airflow/www/templates/airflow/ti_code.html

@@ -25,6 +25,6 @@
     <h4>{{ title }}</h4>
     {% for k, v in html_dict.items() %}
         <h5>{{ k }}</h5>
-        {{ v|safe }}
+        {{ v }}
     {% endfor %}
 {% endblock %}

airflow/www/utils.py

@@ -290,24 +290,25 @@ def pygment_html_render(s, lexer=lexers.TextLexer):
 def render(obj, lexer):
     out = ""
     if isinstance(obj, str):
-        out += pygment_html_render(obj, lexer)
+        out = Markup(pygment_html_render(obj, lexer))
     elif isinstance(obj, (tuple, list)):
         for i, s in enumerate(obj):
-            out += "<div>List item #{}</div>".format(i)
-            out += "<div>" + pygment_html_render(s, lexer) + "</div>"
+            out += Markup("<div>List item #{}</div>").format(i)
+            out += Markup("<div>" + pygment_html_render(s, lexer) + "</div>")
     elif isinstance(obj, dict):
         for k, v in obj.items():
-            out += '<div>Dict item "{}"</div>'.format(k)
-            out += "<div>" + pygment_html_render(v, lexer) + "</div>"
+            out += Markup('<div>Dict item "{}"</div>').format(k)
+            out += Markup("<div>" + pygment_html_render(v, lexer) + "</div>")
     return out
 
 
-def wrapped_markdown(s):
-    return (
-        '<div class="rich_doc">' + markdown.markdown(s) + "</div>"
-        if s is not None
-        else None
-    )
+def wrapped_markdown(s, css_class=None):
+    if s is None:
+        return None
+
+    return Markup(
+        '<div class="rich_doc {css_class}" >' + markdown.markdown(s) + "</div>"
+    ).format(css_class=css_class)
 
 
 def get_attr_renderer():

airflow/www/views.py

@@ -32,7 +32,6 @@ from typing import Dict, List, Optional, Tuple
 from urllib.parse import quote, unquote
 
 import lazy_object_proxy
-import markdown
 import nvd3
 import sqlalchemy as sqla
 from flask import (
@@ -555,15 +554,15 @@ class Airflow(AirflowBaseView):
             dag_id = request.args.get('dag_id')
             dag_orm = DagModel.get_dagmodel(dag_id, session=session)
             code = DagCode.get_code_by_fileloc(dag_orm.fileloc)
-            html_code = highlight(
-                code, lexers.PythonLexer(), HtmlFormatter(linenos=True))
+            html_code = Markup(highlight(
+                code, lexers.PythonLexer(), HtmlFormatter(linenos=True)))
 
         except Exception as e:
             all_errors += (
                 "Exception encountered during " +
                 "dag_id retrieval/dag retrieval fallback/code highlighting:\n\n{}\n".format(e)
             )
-            html_code = '<p>Failed to load file.</p><p>Details: {}</p>'.format(
+            html_code = Markup('<p>Failed to load file.</p><p>Details: {}</p>').format(
                 escape(all_errors))
 
         return self.render_template(
@@ -631,11 +630,9 @@ class Airflow(AirflowBaseView):
         for template_field in task.template_fields:
             content = getattr(task, template_field)
             if template_field in wwwutils.get_attr_renderer():
-                html_dict[template_field] = \
-                    wwwutils.get_attr_renderer()[template_field](content)
+                html_dict[template_field] = wwwutils.get_attr_renderer()[template_field](content)
             else:
-                html_dict[template_field] = (
-                    "<pre><code>" + str(content) + "</pre></code>")
+                html_dict[template_field] = Markup("<pre><code>{}</pre></code>").format(str(content))
 
         return self.render_template(
             'airflow/ti_code.html',
@@ -1602,8 +1599,7 @@ class Airflow(AirflowBaseView):
         if not tasks:
             flash("No tasks found", "error")
         session.commit()
-        doc_md = markdown.markdown(dag.doc_md) \
-            if hasattr(dag, 'doc_md') and dag.doc_md else ''
+        doc_md = wwwutils.wrapped_markdown(getattr(dag, 'doc_md', None), css_class='dag-doc')
 
         external_logs = conf.get('elasticsearch', 'frontend')
         return self.render_template(
@@ -1733,8 +1729,8 @@ class Airflow(AirflowBaseView):
             demo_mode=conf.getboolean('webserver', 'demo_mode'),
             root=root,
             form=form,
-            chart=chart.htmlcontent,
-            cum_chart=cum_chart.htmlcontent
+            chart=Markup(chart.htmlcontent),
+            cum_chart=Markup(cum_chart.htmlcontent),
         )
 
     @expose('/tries')
@@ -1798,7 +1794,7 @@ class Airflow(AirflowBaseView):
             demo_mode=conf.getboolean('webserver', 'demo_mode'),
             root=root,
             form=form,
-            chart=chart.htmlcontent,
+            chart=Markup(chart.htmlcontent),
             tab_title='Tries',
         )