security: explicit verify=True + remove manual Markup() in settings render (M1, M3)#54
Conversation
…ender (M1, M3) M1: pass verify=True explicitly to every httpx.Client / AsyncClient constructor (cleanup, health, vad, webui dashboard probe). httpx defaults to verify=True today, but stating it at every site prevents future regression — anyone disabling TLS verification has to type it. M3: replace the manual Markup() / escape() YAML highlighter in webui/routes.py with a token list rendered by Jinja under autoescape. The previous code was correct (every interpolation passed through escape()) but carried a #nosec B704 and was a refactor-foot-gun. Now there is no Markup() construction in the routes module at all; the template iterates tokens and Jinja's HTML autoescape does the work. Adjusted the FakeAsyncClient in test_cleanup_circuit.py to accept the new verify kwarg. Full suite: 322 passed. Smoke test of /settings confirms YAML renders with yaml-key / yaml-punctuation spans intact and no unescaped output. Tracks M1 + M3 in issue #51. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Pre-merge checks · ✅ 2 · ⚠ 0 · ❌ 0 · ⏭ 1
|
WalkthroughAdds explicit verify=True to all httpx client call sites as defence in depth and removes manual Markup() HTML construction from the settings YAML highlighter in favour of pure data tokens rendered by Jinja with autoescape. Includes a minor test fix and incidental formatting tidies from ruff. Changes
🎯 Effort: 2 (Simple) · ⏱ ~12 minutes Generated by Sebastion AI · docs |
There was a problem hiding this comment.
🔒 Sebastion AI security review
1 finding on this PR. 🔸 1 medium
Sources: 1 LLM
Inline comments are posted on changed lines below.
Other findings (in scanned files but outside this diff)
- MEDIUM
template-injection-xss· CWE-79 —dictate/webui/templates/settings.html:16— The variableload_erroris rendered with Jinja2's default{{ }}which auto-escapes in most configurations, but if auto-escaping is disabled or the value contains trusted HTML markup, an attacker-controlled config error message could inject arbitrary HTML/JS. Verify Jinja2 Environment hasautoescape=True.
Fix: Ensure the Jinja2 Environment is created withautoescape=Trueor use{{ load_error|e }}explicitly.
Audited by Sebastion AI · docs · install on more repos
Prompt for all review comments with AI agents
In dictate/webui/templates/settings.html:
- Line 16: In
dictate/webui/templates/settings.htmlat line16, the variableload_erroris rendered with Jinja2's default{{ }}which auto-escapes in most configurations, but if auto-escaping is disabled or the value contains trusted HTML markup, an attacker-controlled config error message could inject arbitrary HTML/JS. Verify Jinja2 Environment hasautoescape=True. Fix: Ensure the Jinja2 Environment is created withautoescape=Trueor use{{ load_error|e }}explicitly. Context: ruletemplate-injection-xssand CWECWE-79.
lewiswigmore
left a comment
There was a problem hiding this comment.
Approved. Closes M1 + M3. verify=True made explicit at every httpx callsite is a cheap regression guard. Removing the manual Markup() in favour of token-list + Jinja autoescape eliminates the nosec and a refactor foot-gun. Output unchanged (yaml-key + yaml-punctuation spans still render). 322 tests pass.
Summary
Closes M1 and M3 from the security review (issue #51). Pure defense-in-depth — no exploit chain is unblocked, but two foot-guns are eliminated.
M1 — explicit
verify=TrueEvery
httpx.Client/AsyncClientconstructor now passesverify=Trueexplicitly:dictate/cleanup.py(×3)dictate/health.pydictate/vad.pydictate/webui/routes.pyhttpx already defaults to
verify=True, so behaviour is unchanged. The value is making the verification policy visible at every call site so a futureverify=Falseslip becomes obvious in review rather than silent.M3 — remove manual Markup() in YAML highlighter
webui/routes.py:_highlight_yamlpreviously built HTML by f-string concat throughmarkupsafe.escape(), wrapping the result inMarkup()and carrying a# nosec B704. Correct, but fragile to future edits.This change:
_tokenize_yamland returnslist[list[tuple[str, str]]]— pure data, no HTML.settings.htmlto render those tokens via Jinja with autoescape on ({{ text }}).markupsafeimport entirely. NoMarkup()construction left in the file.Net effect: same rendered output (yaml-key + yaml-punctuation spans), zero raw-HTML construction in Python, no
nosecto maintain.Tests
tests/test_cleanup_circuit.py— updatedFakeAsyncClient.__init__to accept the newverifykwarg.test_settings_returns_html_and_resolved_pathcovers the new template path.GET /settings: returns 200, contains both<span class="yaml-key">and<span class="yaml-punctuation">spans, no leftover Jinja markers.pytest: 322 passed.ruff check+ruff format --check: clean (some pre-existing format drift inroutes.pywas tidied incidentally byruff format).What's left from issue #51
config/*.yaml(touches every config callsite, larger refactor).