chore: code-review cleanups (NITs + filename defense-in-depth test)
- 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 <noreply@anthropic.com>
This commit is contained in:
@@ -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()
|
||||
|
||||
@@ -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"],
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user