From 3725d4415e9e253dceba608538d20916d507bcfa Mon Sep 17 00:00:00 2001 From: Knacky Date: Mon, 8 Jun 2026 18:23:39 +0200 Subject: [PATCH] chore: code-review cleanups (NITs + filename defense-in-depth test) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - NIT-1: remove dead _technique_names() and _technique_ids() helpers (no callers) - NIT-2: rename engagement → _engagement in render_engagement_csv signature - NIT-4: remove duplicate inline User import in test_export_csv_escapes_special_characters - NIT-5: add comment on _CSV_FORMULA_TRIGGERS explaining \t and \r inclusion - REUSE: replace custom _html_escape with stdlib html.escape (quote=True default) - Remove now-unnecessary type: ignore comments on weasyprint (stubs resolve cleanly) - Add test_export_filename_never_contains_quote_or_crlf defense-in-depth test Co-Authored-By: Claude Sonnet 4.6 --- backend/app/services/export.py | 26 ++++++------------------- backend/tests/test_export_engagement.py | 2 -- backend/tests/test_export_render.py | 13 +++++++++++++ 3 files changed, 19 insertions(+), 22 deletions(-) diff --git a/backend/app/services/export.py b/backend/app/services/export.py index d4f4b22..368f72b 100644 --- a/backend/app/services/export.py +++ b/backend/app/services/export.py @@ -6,6 +6,7 @@ import io import re import unicodedata from datetime import date +from html import escape as _html_escape from typing import TYPE_CHECKING if TYPE_CHECKING: @@ -21,14 +22,6 @@ def _export_filename(engagement: Engagement, ext: str) -> str: return f"engagement-{engagement.id}-{slug}-{today}.{ext}" -def _technique_names(techniques: list[dict]) -> str: - return " | ".join(t.get("name", t.get("id", "")) for t in (techniques or [])) - - -def _technique_ids(techniques: list[dict]) -> str: - return " | ".join(t.get("id", "") for t in (techniques or [])) - - def _tactic_names(tactic_ids: list[str]) -> str: from backend.app.serializers import _enrich_tactics @@ -154,6 +147,8 @@ _CSV_HEADERS = [ "updated_at", ] +# \t and \r included: Excel auto-trims leading whitespace, so a tab/CR prefix still +# reaches the formula parser in some sheet versions. _CSV_FORMULA_TRIGGERS = ("=", "+", "-", "@", "\t", "\r") @@ -172,7 +167,7 @@ def _csv_safe(value: object) -> object: def render_engagement_csv( - engagement: Engagement, simulations: list[Simulation] + _engagement: Engagement, simulations: list[Simulation] ) -> str: buf = io.StringIO() writer = csv.writer(buf) @@ -223,15 +218,6 @@ pre { background: #f5f5f5; padding: 8px; font-size: 11px; white-space: pre-wrap; """ -def _html_escape(text: str) -> str: - return ( - text.replace("&", "&") - .replace("<", "<") - .replace(">", ">") - .replace('"', """) - ) - - def _render_engagement_html( engagement: Engagement, simulations: list[Simulation] ) -> str: @@ -312,7 +298,7 @@ def _render_engagement_html( def render_engagement_pdf( engagement: Engagement, simulations: list[Simulation] ) -> bytes: - from weasyprint import HTML # type: ignore[import-untyped] + from weasyprint import HTML html = _render_engagement_html(engagement, simulations) - return HTML(string=html).write_pdf() # type: ignore[no-any-return] + return HTML(string=html).write_pdf() diff --git a/backend/tests/test_export_engagement.py b/backend/tests/test_export_engagement.py index d264543..915e896 100644 --- a/backend/tests/test_export_engagement.py +++ b/backend/tests/test_export_engagement.py @@ -136,8 +136,6 @@ def test_export_csv_escapes_special_characters( eng = _make_engagement(client, admin_token) with app.app_context(): - from backend.app.models import User - admin = User.query.filter_by(username="admin1").first() sim = Simulation( engagement_id=eng["id"], diff --git a/backend/tests/test_export_render.py b/backend/tests/test_export_render.py index faf3e06..6fe752c 100644 --- a/backend/tests/test_export_render.py +++ b/backend/tests/test_export_render.py @@ -247,3 +247,16 @@ def test_render_engagement_csv_does_not_alter_safe_strings(app) -> None: cells = _parse_csv_data_row(result) assert cells[1] == "Mimikatz LSASS Dump", "safe name must not be modified" assert cells[6] == "whoami /all", "safe commands must not be modified" + + +def test_export_filename_never_contains_quote_or_crlf() -> None: + """Defense-in-depth: even with malicious engagement names, the filename + used in Content-Disposition must never contain header-injection chars.""" + from types import SimpleNamespace + + from backend.app.services.export import _export_filename + evil = SimpleNamespace(id=42, name='evil"name\r\nX-Injected: yes') + fname = _export_filename(evil, "md") + assert '"' not in fname + assert '\r' not in fname + assert '\n' not in fname