From b78e2561ab7e0545509d7d7a5906f4f2d7ccdb82 Mon Sep 17 00:00:00 2001 From: andrecs <12188364+andrecsilva@users.noreply.github.com> Date: Thu, 16 Jan 2025 13:59:05 -0300 Subject: [PATCH 1/5] Initial version of SonarSecureCookie --- src/core_codemods/__init__.py | 2 + src/core_codemods/secure_flask_cookie.py | 42 ++++++++++++------ src/core_codemods/sonar/api.py | 40 +++++++++++------ .../sonar/sonar_secure_cookie.py | 43 +++++++++++++++++++ 4 files changed, 101 insertions(+), 26 deletions(-) create mode 100644 src/core_codemods/sonar/sonar_secure_cookie.py diff --git a/src/core_codemods/__init__.py b/src/core_codemods/__init__.py index fdc71435..0ae2c15c 100644 --- a/src/core_codemods/__init__.py +++ b/src/core_codemods/__init__.py @@ -89,6 +89,7 @@ SonarRemoveAssertionInPytestRaises, ) from .sonar.sonar_sandbox_process_creation import SonarSandboxProcessCreation +from .sonar.sonar_secure_cookie import SonarSecureCookie from .sonar.sonar_secure_random import SonarSecureRandom from .sonar.sonar_sql_parameterization import SonarSQLParameterization from .sonar.sonar_tempfile_mktemp import SonarTempfileMktemp @@ -204,6 +205,7 @@ SonarInvertedBooleanCheck, SonarTimezoneAwareDatetime, SonarSandboxProcessCreation, + SonarSecureCookie, ], ) diff --git a/src/core_codemods/secure_flask_cookie.py b/src/core_codemods/secure_flask_cookie.py index 41f5445f..8e72d45a 100644 --- a/src/core_codemods/secure_flask_cookie.py +++ b/src/core_codemods/secure_flask_cookie.py @@ -1,9 +1,25 @@ -from core_codemods.api import Metadata, Reference, ReviewGuidance, SimpleCodemod +from codemodder.codemods.libcst_transformer import ( + LibcstResultTransformer, + LibcstTransformerPipeline, +) +from codemodder.codemods.semgrep import SemgrepRuleDetector +from core_codemods.api import Metadata, Reference, ReviewGuidance +from core_codemods.api.core_codemod import CoreCodemod from core_codemods.secure_cookie_mixin import SecureCookieMixin -class SecureFlaskCookie(SimpleCodemod, SecureCookieMixin): - metadata = Metadata( +class SecureCookieTransformer(LibcstResultTransformer, SecureCookieMixin): + change_description = "Flask response `set_cookie` call should be called with `secure=True`, `httponly=True`, and `samesite='Lax'`." + + def on_result_found(self, original_node, updated_node): + new_args = self.replace_args( + original_node, self._choose_new_args(original_node) + ) + return self.update_arg_target(updated_node, new_args) + + +SecureFlaskCookie = CoreCodemod( + metadata=Metadata( name="secure-flask-cookie", summary="Use Safe Parameters in `flask` Response `set_cookie` Call", review_guidance=ReviewGuidance.MERGE_AFTER_CURSORY_REVIEW, @@ -14,11 +30,14 @@ class SecureFlaskCookie(SimpleCodemod, SecureCookieMixin): Reference( url="https://owasp.org/www-community/controls/SecureCookieAttribute" ), + Reference(url="https://cwe.mitre.org/data/definitions/1004"), + Reference(url="https://cwe.mitre.org/data/definitions/311"), + Reference(url="https://cwe.mitre.org/data/definitions/315"), Reference(url="https://cwe.mitre.org/data/definitions/614"), ], - ) - change_description = "Flask response `set_cookie` call should be called with `secure=True`, `httponly=True`, and `samesite='Lax'`." - detector_pattern = """ + ), + detector=SemgrepRuleDetector( + """ rules: - id: secure-flask-cookie mode: taint @@ -39,10 +58,7 @@ class SecureFlaskCookie(SimpleCodemod, SecureCookieMixin): - pattern: $SINK.set_cookie(...) - pattern-not: $SINK.set_cookie(..., secure=True, ..., httponly=True, ..., samesite="Lax", ...) - pattern-not: $SINK.set_cookie(..., secure=True, ..., httponly=True, ..., samesite="Strict", ...) - """ - - def on_result_found(self, original_node, updated_node): - new_args = self.replace_args( - original_node, self._choose_new_args(original_node) - ) - return self.update_arg_target(updated_node, new_args) + """ + ), + transformer=LibcstTransformerPipeline(SecureCookieTransformer), +) diff --git a/src/core_codemods/sonar/api.py b/src/core_codemods/sonar/api.py index 4833e675..5688770d 100644 --- a/src/core_codemods/sonar/api.py +++ b/src/core_codemods/sonar/api.py @@ -15,39 +15,53 @@ def origin(self): return "sonar" @classmethod - def from_core_codemod( + def from_core_codemod_with_multiple_rules( cls, name: str, other: CoreCodemod, - rule_id: str, - rule_name: str, + rules: list[ToolRule], transformer: BaseTransformerPipeline | None = None, ): - rule_url = sonar_url_from_id(rule_id) return SonarCodemod( metadata=Metadata( name=name, summary=other.summary, review_guidance=other._metadata.review_guidance, references=( - other.references + [Reference(url=rule_url, description=rule_name)] + other.references + + [Reference(url=tr.url or "", description=tr.name) for tr in rules] ), description=other.description, tool=ToolMetadata( name="Sonar", - rules=[ - ToolRule( - id=rule_id, - name=rule_name, - url=rule_url, - ) - ], + rules=rules, ), ), transformer=transformer if transformer else other.transformer, detector=SonarDetector(), default_extensions=other.default_extensions, - requested_rules=[rule_id], + requested_rules=[tr.id for tr in rules], + ) + + @classmethod + def from_core_codemod( + cls, + name: str, + other: CoreCodemod, + rule_id: str, + rule_name: str, + transformer: BaseTransformerPipeline | None = None, + ): + rule_url = sonar_url_from_id(rule_id) + rules = [ + ToolRule( + id=rule_id, + name=rule_name, + url=rule_url, + ), + ] + return cls.from_core_codemod_with_multiple_rules( + name, other, rules, transformer ) diff --git a/src/core_codemods/sonar/sonar_secure_cookie.py b/src/core_codemods/sonar/sonar_secure_cookie.py new file mode 100644 index 00000000..5d3d6b9d --- /dev/null +++ b/src/core_codemods/sonar/sonar_secure_cookie.py @@ -0,0 +1,43 @@ +from codemodder.codemods.base_codemod import ToolRule +from codemodder.codemods.libcst_transformer import ( + LibcstResultTransformer, + LibcstTransformerPipeline, +) +from core_codemods.secure_cookie_mixin import SecureCookieMixin +from core_codemods.secure_flask_cookie import SecureFlaskCookie +from core_codemods.sonar.api import SonarCodemod + +rules = [ + ToolRule( + id="python:S3330", + name='Creating cookies without the "HttpOnly" flag is security-sensitive', + url="https://rules.sonarsource.com/python/RSPEC-3330/", + ), + ToolRule( + id="python:S2092", + name='Creating cookies without the "secure" flag is security-sensitive', + url="ahttps://rules.sonarsource.com/python/RSPEC-2092/", + ), +] + + +class SonarSecureCookieTransformer(LibcstResultTransformer, SecureCookieMixin): + change_description = "Flask response `set_cookie` call should be called with `secure=True`, `httponly=True`, and `samesite='Lax'`." + + def leave_Call(self, original_node, updated_node): + # Try to match the func + if self.node_is_selected(original_node.func): + self.report_change(original_node) + new_args = self.replace_args( + original_node, self._choose_new_args(original_node) + ) + return self.update_arg_target(updated_node, new_args) + return updated_node + + +SonarSecureCookie = SonarCodemod.from_core_codemod_with_multiple_rules( + name="secure-cookie", + other=SecureFlaskCookie, + rules=rules, + transformer=LibcstTransformerPipeline(SonarSecureCookieTransformer), +) From 1354343f1a68e05b36903eb02bd8ac7afc0b6009 Mon Sep 17 00:00:00 2001 From: andrecs <12188364+andrecsilva@users.noreply.github.com> Date: Thu, 16 Jan 2025 13:06:45 -0300 Subject: [PATCH 2/5] Added unit test --- .../sonar/test_sonar_secure_cookie.py | 87 +++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 tests/codemods/sonar/test_sonar_secure_cookie.py diff --git a/tests/codemods/sonar/test_sonar_secure_cookie.py b/tests/codemods/sonar/test_sonar_secure_cookie.py new file mode 100644 index 00000000..922fbc00 --- /dev/null +++ b/tests/codemods/sonar/test_sonar_secure_cookie.py @@ -0,0 +1,87 @@ +import json + +from codemodder.codemods.test import BaseSASTCodemodTest +from core_codemods.sonar.sonar_secure_cookie import SonarSecureCookie + + +class TestSonarSecureCookie(BaseSASTCodemodTest): + codemod = SonarSecureCookie + tool = "sonar" + + def test_name(self): + assert self.codemod.name == "secure-cookie" + + def test_simple(self, tmpdir): + input_code = """ + import flask + + response = flask.make_response() + var = "hello" + response.set_cookie("name", "value") + + response2 = flask.Response() + var = "hello" + response2.set_cookie("name", "value") + """ + expected = """ + import flask + + response = flask.make_response() + var = "hello" + response.set_cookie("name", "value", secure=True, httponly=True, samesite='Lax') + + response2 = flask.Response() + var = "hello" + response2.set_cookie("name", "value", secure=True, httponly=True, samesite='Lax') + """ + issues = { + "hotspots": [ + { + "component": "code.py", + "status": "TO_REVIEW", + "textRange": { + "startLine": 6, + "endLine": 6, + "startOffset": 0, + "endOffset": 19, + }, + "ruleKey": "python:S2092", + }, + { + "component": "code.py", + "status": "TO_REVIEW", + "textRange": { + "startLine": 10, + "endLine": 10, + "startOffset": 0, + "endOffset": 20, + }, + "ruleKey": "python:S2092", + }, + { + "component": "code.py", + "status": "TO_REVIEW", + "textRange": { + "startLine": 6, + "endLine": 6, + "startOffset": 0, + "endOffset": 19, + }, + "ruleKey": "python:S3330", + }, + { + "component": "code.py", + "status": "TO_REVIEW", + "textRange": { + "startLine": 10, + "endLine": 10, + "startOffset": 0, + "endOffset": 20, + }, + "ruleKey": "python:S3330", + }, + ], + } + self.run_and_assert( + tmpdir, input_code, expected, results=json.dumps(issues), num_changes=2 + ) From 2528368db2e995851c4e5959ec77577a17bd771e Mon Sep 17 00:00:00 2001 From: andrecs <12188364+andrecsilva@users.noreply.github.com> Date: Thu, 16 Jan 2025 13:58:14 -0300 Subject: [PATCH 3/5] Added integration test --- .../sonar/test_sonar_secure_cookie.py | 19 +++++++++ integration_tests/test_secure_flask_cookie.py | 4 +- .../codemods/test/integration_utils.py | 26 +++++++----- .../sonar/sonar_secure_cookie.py | 2 +- tests/samples/sonar_hotspots.json | 40 +++++++++++++++++++ 5 files changed, 79 insertions(+), 12 deletions(-) create mode 100644 integration_tests/sonar/test_sonar_secure_cookie.py diff --git a/integration_tests/sonar/test_sonar_secure_cookie.py b/integration_tests/sonar/test_sonar_secure_cookie.py new file mode 100644 index 00000000..409592ae --- /dev/null +++ b/integration_tests/sonar/test_sonar_secure_cookie.py @@ -0,0 +1,19 @@ +from codemodder.codemods.test import SonarIntegrationTest +from core_codemods.sonar.sonar_secure_cookie import ( + SonarSecureCookie, + SonarSecureCookieTransformer, +) + + +class TestSonarSecureCookie(SonarIntegrationTest): + codemod = SonarSecureCookie + code_path = "tests/samples/secure_cookie.py" + replacement_lines = [ + ( + 8, + """ resp.set_cookie('custom_cookie', 'value', secure=True, httponly=True, samesite='Lax')\n""", + ), + ] + expected_diff = "--- \n+++ \n@@ -5,5 +5,5 @@\n @app.route('/')\n def index():\n resp = make_response('Custom Cookie Set')\n- resp.set_cookie('custom_cookie', 'value')\n+ resp.set_cookie('custom_cookie', 'value', secure=True, httponly=True, samesite='Lax')\n return resp\n" + expected_line_change = "8" + change_description = SonarSecureCookieTransformer.change_description diff --git a/integration_tests/test_secure_flask_cookie.py b/integration_tests/test_secure_flask_cookie.py index b49c155d..d9752f84 100644 --- a/integration_tests/test_secure_flask_cookie.py +++ b/integration_tests/test_secure_flask_cookie.py @@ -1,5 +1,5 @@ from codemodder.codemods.test import BaseIntegrationTest -from core_codemods.secure_flask_cookie import SecureFlaskCookie +from core_codemods.secure_flask_cookie import SecureCookieTransformer, SecureFlaskCookie class TestSecureFlaskCookie(BaseIntegrationTest): @@ -23,4 +23,4 @@ def index(): ] expected_diff = "--- \n+++ \n@@ -5,5 +5,5 @@\n @app.route('/')\n def index():\n resp = make_response('Custom Cookie Set')\n- resp.set_cookie('custom_cookie', 'value')\n+ resp.set_cookie('custom_cookie', 'value', secure=True, httponly=True, samesite='Lax')\n return resp\n" expected_line_change = "8" - change_description = SecureFlaskCookie.change_description + change_description = SecureCookieTransformer.change_description diff --git a/src/codemodder/codemods/test/integration_utils.py b/src/codemodder/codemods/test/integration_utils.py index 262dc871..484b2bb8 100644 --- a/src/codemodder/codemods/test/integration_utils.py +++ b/src/codemodder/codemods/test/integration_utils.py @@ -124,8 +124,12 @@ def _assert_results_fields(self, results, output_path): # TODO: if/when we add description for each url for reference in result["references"][ - # Last reference for Sonar has a different description - : (-1 if self.sonar_issues_json or self.sonar_hotspots_json else None) + # Last references for Sonar has a different description + : ( + -len(self.codemod.requested_rules) + if self.sonar_issues_json or self.sonar_hotspots_json + else None + ) ]: assert reference["url"] == reference["description"] @@ -288,21 +292,25 @@ def check_sonar_issues(cls): (cls.sonar_issues_json, cls.sonar_hotspots_json) ) - assert ( - cls.codemod.requested_rules[-1] in sonar_results + assert any( + map(lambda x: x in sonar_results, cls.codemod.requested_rules) ), f"Make sure to add a sonar issue/hotspot for {cls.codemod.rule_id} in {cls.sonar_issues_json} or {cls.sonar_hotspots_json}" results_for_codemod = sonar_results[cls.codemod.requested_rules[-1]] file_path = pathlib.Path(cls.code_filename) assert ( file_path in results_for_codemod - ), f"Make sure to add a sonar issue/hotspot for file `{cls.code_filename}` under rule `{cls.codemod.rule_id}`in {cls.sonar_issues_json} or {cls.sonar_hotspots_json}" + ), f"Make sure to add a sonar issue/hotspot for file `{cls.code_filename}` under one of the rules `{cls.codemod.requested_rules}`in {cls.sonar_issues_json} or {cls.sonar_hotspots_json}" def _assert_sonar_fields(self, result): assert self.codemod_instance._metadata.tool is not None - assert ( - result["references"][-1]["description"] - == self.codemod_instance._metadata.tool.rules[0].name - ) + rules = self.codemod_instance._metadata.tool.rules + for i in range(len(rules)): + assert ( + result["references"][len(result["references"]) - len(rules) + i][ + "description" + ] + == self.codemod_instance._metadata.tool.rules[i].name + ) assert result["detectionTool"]["name"] == "Sonar" diff --git a/src/core_codemods/sonar/sonar_secure_cookie.py b/src/core_codemods/sonar/sonar_secure_cookie.py index 5d3d6b9d..2301bef7 100644 --- a/src/core_codemods/sonar/sonar_secure_cookie.py +++ b/src/core_codemods/sonar/sonar_secure_cookie.py @@ -16,7 +16,7 @@ ToolRule( id="python:S2092", name='Creating cookies without the "secure" flag is security-sensitive', - url="ahttps://rules.sonarsource.com/python/RSPEC-2092/", + url="https://rules.sonarsource.com/python/RSPEC-2092/", ), ] diff --git a/tests/samples/sonar_hotspots.json b/tests/samples/sonar_hotspots.json index ac52ee57..897220e9 100644 --- a/tests/samples/sonar_hotspots.json +++ b/tests/samples/sonar_hotspots.json @@ -5,6 +5,46 @@ "total": 4 }, "hotspots": [ + { + "key": "AZRvB_g13jBxJiUZnPHJ", + "component": "pixee_codemodder-python:secure_cookie.py", + "project": "pixee_codemodder-python", + "securityCategory": "insecure-conf", + "vulnerabilityProbability": "LOW", + "status": "TO_REVIEW", + "line": 8, + "message": "Make sure creating this cookie without the \"secure\" flag is safe.", + "creationDate": "2025-01-16T13:11:02+0100", + "updateDate": "2025-01-16T13:12:34+0100", + "textRange": { + "startLine": 8, + "endLine": 8, + "startOffset": 4, + "endOffset": 19 + }, + "flows": [], + "ruleKey": "python:S2092" + }, + { + "key": "AZRvB_g13jBxJiUZnPHI", + "component": "pixee_codemodder-python:secure_cookie.py", + "project": "pixee_codemodder-python", + "securityCategory": "others", + "vulnerabilityProbability": "LOW", + "status": "TO_REVIEW", + "line": 8, + "message": "Make sure creating this cookie without the \"HttpOnly\" flag is safe.", + "creationDate": "2025-01-16T13:11:02+0100", + "updateDate": "2025-01-16T13:12:34+0100", + "textRange": { + "startLine": 8, + "endLine": 8, + "startOffset": 4, + "endOffset": 19 + }, + "flows": [], + "ruleKey": "python:S3330" + }, { "key": "AY6fXn2rzaaymEtIucTd", "component": "pixee_codemodder-python:secure_random.py", From 8b1d826fff1f4707fc0c4955cf504bf41680cbb2 Mon Sep 17 00:00:00 2001 From: andrecs <12188364+andrecsilva@users.noreply.github.com> Date: Thu, 16 Jan 2025 14:09:38 -0300 Subject: [PATCH 4/5] Fixed missing file for integration test --- tests/samples/secure_cookie.py | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 tests/samples/secure_cookie.py diff --git a/tests/samples/secure_cookie.py b/tests/samples/secure_cookie.py new file mode 100644 index 00000000..e66bc986 --- /dev/null +++ b/tests/samples/secure_cookie.py @@ -0,0 +1,9 @@ +from flask import Flask, session, make_response + +app = Flask(__name__) + +@app.route('/') +def index(): + resp = make_response('Custom Cookie Set') + resp.set_cookie('custom_cookie', 'value') + return resp From 2b86454325d55eb8f6ea783a3256afff5e232887 Mon Sep 17 00:00:00 2001 From: andrecs <12188364+andrecsilva@users.noreply.github.com> Date: Fri, 17 Jan 2025 07:26:19 -0300 Subject: [PATCH 5/5] Fixed tests and refactoring --- src/codemodder/codemods/test/integration_utils.py | 2 +- src/codemodder/scripts/generate_docs.py | 6 ++++++ src/core_codemods/sonar/sonar_secure_cookie.py | 1 - tests/test_sonar_results.py | 4 ++-- 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/codemodder/codemods/test/integration_utils.py b/src/codemodder/codemods/test/integration_utils.py index 484b2bb8..ae191227 100644 --- a/src/codemodder/codemods/test/integration_utils.py +++ b/src/codemodder/codemods/test/integration_utils.py @@ -293,7 +293,7 @@ def check_sonar_issues(cls): ) assert any( - map(lambda x: x in sonar_results, cls.codemod.requested_rules) + [x in sonar_results for x in cls.codemod.requested_rules] ), f"Make sure to add a sonar issue/hotspot for {cls.codemod.rule_id} in {cls.sonar_issues_json} or {cls.sonar_hotspots_json}" results_for_codemod = sonar_results[cls.codemod.requested_rules[-1]] file_path = pathlib.Path(cls.code_filename) diff --git a/src/codemodder/scripts/generate_docs.py b/src/codemodder/scripts/generate_docs.py index 42e03503..63a0095c 100644 --- a/src/codemodder/scripts/generate_docs.py +++ b/src/codemodder/scripts/generate_docs.py @@ -328,6 +328,12 @@ class DocMetadata: need_sarif="Yes (Sonar)", ) for name in SONAR_CODEMOD_NAMES +} | { + "secure-cookie": DocMetadata( + importance="Medium", + guidance_explained="Our change provides the most secure way to create cookies in Flask. However, it's possible you have configured your Flask application configurations to use secure cookies. In these cases, using the default parameters for `set_cookie` is safe.", + need_sarif="Yes (Sonar)", + ), } SEMGREP_CODEMOD_NAMES = [ diff --git a/src/core_codemods/sonar/sonar_secure_cookie.py b/src/core_codemods/sonar/sonar_secure_cookie.py index 2301bef7..5f4cd1a9 100644 --- a/src/core_codemods/sonar/sonar_secure_cookie.py +++ b/src/core_codemods/sonar/sonar_secure_cookie.py @@ -25,7 +25,6 @@ class SonarSecureCookieTransformer(LibcstResultTransformer, SecureCookieMixin): change_description = "Flask response `set_cookie` call should be called with `secure=True`, `httponly=True`, and `samesite='Lax'`." def leave_Call(self, original_node, updated_node): - # Try to match the func if self.node_is_selected(original_node.func): self.report_change(original_node) new_args = self.replace_args( diff --git a/tests/test_sonar_results.py b/tests/test_sonar_results.py index cb59e5a5..54227d4d 100644 --- a/tests/test_sonar_results.py +++ b/tests/test_sonar_results.py @@ -43,7 +43,7 @@ def test_parse_issues_json(): def test_parse_hotspots_json(): results = SonarResultSet.from_json(SAMPLE_DIR / "sonar_hotspots.json") - assert len(results) == 2 + assert len(results) == 4 def test_combined_json(tmpdir): @@ -54,7 +54,7 @@ def test_combined_json(tmpdir): ) results = SonarResultSet.from_json(Path(tmpdir).joinpath("combined.json")) - assert len(results) == 36 + assert len(results) == 38 def test_empty_issues(tmpdir, caplog):