[GitHub] [airflow] ashb commented on a change in pull request #9180: Don't use the `|safe` filter in code, it's risky

[GitBox <gi...@apache.org>]

ashb commented on a change in pull request #9180:
URL: https://github.com/apache/airflow/pull/9180#discussion_r436734448



##########
File path: airflow/www/templates/airflow/dag.html
##########
@@ -427,8 +427,8 @@ <h4 class="modal-title" id="dagModalLabel">
       });
       subdag_id = sd;
       execution_date = d;
-      $('#task_id').html(t);
-      $('#execution_date').html(d);
+      $('#task_id').text(t);
+      $('#execution_date').text(d);

Review comment:
       These were validated/protected against having anything outside a-zA-Z0-9-_ but better safe than sorry

##########
File path: airflow/www/templates/airflow/gantt.html
##########
@@ -57,7 +57,7 @@
     var dag_id = '{{ dag.dag_id }}';
     var task_id = '';
     var execution_date = '';
-    data = {{ data |tojson|safe }};
+    data = {{ data |tojson }};

Review comment:
       tojson already escapes.
   
   https://flask.palletsprojects.com/en/1.1.x/api/?highlight=tojson#module-flask.json
   
   > The htmlsafe_dumps() function of this json module is also available as a filter called |tojson in Jinja2. Note that in versions of Flask prior to Flask 0.10, you must disable escaping with |safe if you intend to use |tojson output inside script tags. In Flask 0.10 and above, this happens automatically (but it’s harmless to include |safe anyway).

##########
File path: airflow/www/templates/airflow/task.html
##########
@@ -39,14 +39,11 @@ <h5>Dependencies Blocking Task From Getting Scheduled</h5>
                 </tr>
             {% endfor %}
         </table>
-        {% if html_code is defined %}
-            {{ html_code|safe }}
-        {% endif %}

Review comment:
       This was never defined inside the `/task` handler, the only place this template is called from.
   
   (And if it was the content would have been duplicated!

##########
File path: .pre-commit-config.yaml
##########
@@ -246,6 +246,12 @@ 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"
+        pass_filenames: true

Review comment:
       Good idea.

##########
File path: .pre-commit-config.yaml
##########
@@ -246,6 +246,12 @@ 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"
+        pass_filenames: true

Review comment:
       https://github.com/apache/airflow/pull/9180/commits/d22084c186b6d50fc60ec907a2bf7d91f4351d16




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org