From 9a6a234645549fd2658320adb81cd30df6a4d722 Mon Sep 17 00:00:00 2001 From: andrecs <12188364+andrecsilva@users.noreply.github.com> Date: Wed, 17 Jan 2024 08:00:17 -0300 Subject: [PATCH] Docs and tests suggestions --- ...n_flask-send-file-path-parameterization.md | 4 +- .../flask_send_file_path_parameterization.py | 2 +- ...t_flask_send_file_path_parameterization.py | 39 +++++++++++++++---- 3 files changed, 34 insertions(+), 11 deletions(-) diff --git a/src/core_codemods/docs/pixee_python_flask-send-file-path-parameterization.md b/src/core_codemods/docs/pixee_python_flask-send-file-path-parameterization.md index 9600de36a..2042114b4 100644 --- a/src/core_codemods/docs/pixee_python_flask-send-file-path-parameterization.md +++ b/src/core_codemods/docs/pixee_python_flask-send-file-path-parameterization.md @@ -1,5 +1,5 @@ -The `send_file` function from Flask is susceptible to a path injection attack if its input is not proper validated. -In a path injection attack, the malicious agent can craft a path containing special paths like `./` or `../` to resolve an unintended file. This allows the agent to overwrite, delete or read arbitrary files. +The `Flask` `send_file` function from Flask is susceptible to a path traversal attack if its input is not properly validated. +In a path traversal attack, the malicious agent can craft a path containing special paths like `./` or `../` to resolve a file outside of the expected directory path. This potentially allows the agent to overwrite, delete or read arbitrary files. In the case of `flask.send_file`, the result is that a malicious user could potentially download sensitive files that exist on the filesystem where the application is being hosted. Flask offers a native solution with the `flask.send_from_directory` function that validates the given path. Our changes look something like this: diff --git a/src/core_codemods/flask_send_file_path_parameterization.py b/src/core_codemods/flask_send_file_path_parameterization.py index 196fe240d..835c9d600 100644 --- a/src/core_codemods/flask_send_file_path_parameterization.py +++ b/src/core_codemods/flask_send_file_path_parameterization.py @@ -9,7 +9,7 @@ class FlaskSendFilePathParameterization(BaseCodemod, NameAndAncestorResolutionMixin): NAME = "flask-send-file-path-parameterization" - SUMMARY = "Replaces `send_file` with `send_from_directory`" + SUMMARY = "Replace unsafe usage of `flask.send_file`" DESCRIPTION = SUMMARY REVIEW_GUIDANCE = ReviewGuidance.MERGE_WITHOUT_REVIEW REFERENCES = [ diff --git a/tests/codemods/test_flask_send_file_path_parameterization.py b/tests/codemods/test_flask_send_file_path_parameterization.py index 75bc045e0..c588c06b5 100644 --- a/tests/codemods/test_flask_send_file_path_parameterization.py +++ b/tests/codemods/test_flask_send_file_path_parameterization.py @@ -2,7 +2,6 @@ FlaskSendFilePathParameterization, ) from tests.codemods.base_codemod_test import BaseCodemodTest -from textwrap import dedent class TestFlaskSendFilePathParameterization(BaseCodemodTest): @@ -32,7 +31,31 @@ def download_file(name): def download_file(name): return flask.send_from_directory((p := Path(f'path/to/{name}.txt')).parent, p.name) """ - self.run_and_assert(tmpdir, dedent(input_code), dedent(expected)) + self.run_and_assert(tmpdir, input_code, expected) + assert len(self.file_context.codemod_changes) == 1 + + def test_direct_simple_string(self, tmpdir): + input_code = """\ + from flask import Flask, send_file + + app = Flask(__name__) + + @app.route("/uploads/") + def download_file(name): + return send_file('path/to/file.txt') + """ + expected = """\ + from flask import Flask + import flask + from pathlib import Path + + app = Flask(__name__) + + @app.route("/uploads/") + def download_file(name): + return flask.send_from_directory((p := Path('path/to/file.txt')).parent, p.name) + """ + self.run_and_assert(tmpdir, input_code, expected) assert len(self.file_context.codemod_changes) == 1 def test_direct_string_convert_arguments(self, tmpdir): @@ -56,7 +79,7 @@ def download_file(name): def download_file(name): return flask.send_from_directory((p := Path(f'path/to/{name}.txt')).parent, p.name, mimetype = None, as_attachment = False, download_name = True) """ - self.run_and_assert(tmpdir, dedent(input_code), dedent(expected)) + self.run_and_assert(tmpdir, input_code, expected) assert len(self.file_context.codemod_changes) == 1 def test_direct_path(self, tmpdir): @@ -81,7 +104,7 @@ def download_file(name): def download_file(name): return flask.send_from_directory((p := Path(f'path/to/{name}.txt')).parent, p.name) """ - self.run_and_assert(tmpdir, dedent(input_code), dedent(expected)) + self.run_and_assert(tmpdir, input_code, expected) assert len(self.file_context.codemod_changes) == 1 def test_indirect_path(self, tmpdir): @@ -108,7 +131,7 @@ def download_file(name): path = Path(f'path/to/{name}.txt') return flask.send_from_directory(path.parent, path.name) """ - self.run_and_assert(tmpdir, dedent(input_code), dedent(expected)) + self.run_and_assert(tmpdir, input_code, expected) assert len(self.file_context.codemod_changes) == 1 def test_indirect_path_alias(self, tmpdir): @@ -135,7 +158,7 @@ def download_file(name): path = Path(f'path/to/{name}.txt') return flask.send_from_directory(path.parent, path.name) """ - self.run_and_assert(tmpdir, dedent(input_code), dedent(expected)) + self.run_and_assert(tmpdir, input_code, expected) assert len(self.file_context.codemod_changes) == 1 def test_indirect_string(self, tmpdir): @@ -161,7 +184,7 @@ def download_file(name): path = f'path/to/{name}.txt' return flask.send_from_directory((p := Path(path)).parent, p.name) """ - self.run_and_assert(tmpdir, dedent(input_code), dedent(expected)) + self.run_and_assert(tmpdir, input_code, expected) assert len(self.file_context.codemod_changes) == 1 def test_unknown_type(self, tmpdir): @@ -174,5 +197,5 @@ def test_unknown_type(self, tmpdir): def download_file(name): return send_file(name) """ - self.run_and_assert(tmpdir, dedent(input_code), dedent(input_code)) + self.run_and_assert(tmpdir, input_code, input_code) assert len(self.file_context.codemod_changes) == 0