fix: drawer delete-impact modal HTML escapes user-controllable fields (T110.2)
The delete-impact modal is built via raw f-string concatenation from the ImpactReport — item.kind / item.description / report.notes ultimately embed user-controllable content (turn prose, scene timestamps). A turn with prose like "<script>alert(1)</script>" would reach the rendered HTML verbatim. Currently safe (the fields embedded today are bounded strings) but defense-in-depth — wrap with html.escape() so future description changes can't smuggle markup through. Test: tests/test_drawer_phase4.py::test_delete_impact_modal_escapes_user_controllable_strings.
This commit is contained in:
+15
-3
@@ -27,6 +27,7 @@ one so a later inverse edit can restore state (§6.4 final paragraph).
|
|||||||
|
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import html
|
||||||
import json
|
import json
|
||||||
import uuid
|
import uuid
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
@@ -1238,18 +1239,29 @@ async def delete_preview(
|
|||||||
# reusing the drawer template would require a fragment include just
|
# reusing the drawer template would require a fragment include just
|
||||||
# for this surface. Mirrors the rewind-preview style in
|
# for this surface. Mirrors the rewind-preview style in
|
||||||
# :func:`chat.web.turns.rewind_preview`.
|
# :func:`chat.web.turns.rewind_preview`.
|
||||||
|
#
|
||||||
|
# T110.2: ``item.kind``, ``item.description``, and the notes carry
|
||||||
|
# user-controllable content (turn prose, scene timestamps, etc.).
|
||||||
|
# Wrap them with :func:`html.escape` so a payload like
|
||||||
|
# ``<script>alert(1)</script>`` renders as inert text. ``chat_id``
|
||||||
|
# is matched against the projected ``chats`` table at request time
|
||||||
|
# (404 above) so it isn't free-form, but we escape it for symmetry.
|
||||||
items_html = "".join(
|
items_html = "".join(
|
||||||
f"<li><strong>{item.kind}</strong>: {item.description}</li>"
|
f"<li><strong>{html.escape(item.kind)}</strong>: "
|
||||||
|
f"{html.escape(item.description)}</li>"
|
||||||
for item in report.cascading
|
for item in report.cascading
|
||||||
)
|
)
|
||||||
notes_html = "".join(f"<li>{note}</li>" for note in report.notes)
|
notes_html = "".join(
|
||||||
|
f"<li>{html.escape(note)}</li>" for note in report.notes
|
||||||
|
)
|
||||||
body = (
|
body = (
|
||||||
"<div class='delete-impact-modal'>"
|
"<div class='delete-impact-modal'>"
|
||||||
f"<h3>Delete event {report.target_event_id}?</h3>"
|
f"<h3>Delete event {report.target_event_id}?</h3>"
|
||||||
f"<p>This will discard {len(report.cascading)} events. Cascade:</p>"
|
f"<p>This will discard {len(report.cascading)} events. Cascade:</p>"
|
||||||
f"<ul class='delete-impact-cascade'>{items_html or '<li>none</li>'}</ul>"
|
f"<ul class='delete-impact-cascade'>{items_html or '<li>none</li>'}</ul>"
|
||||||
f"<ul class='delete-impact-notes'>{notes_html}</ul>"
|
f"<ul class='delete-impact-notes'>{notes_html}</ul>"
|
||||||
f"<form hx-post='/chats/{chat_id}/drawer/turn/delete/{report.target_event_id}' "
|
f"<form hx-post='/chats/{html.escape(chat_id)}/drawer/turn/delete/"
|
||||||
|
f"{report.target_event_id}' "
|
||||||
"hx-target='#drawer' hx-swap='innerHTML'>"
|
"hx-target='#drawer' hx-swap='innerHTML'>"
|
||||||
"<button type='submit'>Confirm delete</button>"
|
"<button type='submit'>Confirm delete</button>"
|
||||||
"</form>"
|
"</form>"
|
||||||
|
|||||||
@@ -458,6 +458,42 @@ def test_t98_4_delete_invokes_rewind_and_drops_cascade(client, tmp_path):
|
|||||||
assert row is None, f"event {ev_id} should have been deleted"
|
assert row is None, f"event {ev_id} should have been deleted"
|
||||||
|
|
||||||
|
|
||||||
|
def test_delete_impact_modal_escapes_user_controllable_strings(client, tmp_path):
|
||||||
|
"""T110.2: defense-in-depth — fields embedded in the modal HTML come
|
||||||
|
from event payloads (turn prose, scene timestamps, etc.) which are
|
||||||
|
ultimately user-controllable. Wrap them with ``html.escape`` so a
|
||||||
|
payload like ``<script>alert(1)</script>`` renders as inert text and
|
||||||
|
doesn't leak through into the rendered modal as actual markup.
|
||||||
|
"""
|
||||||
|
db = tmp_path / "test.db"
|
||||||
|
_seed_chat(db)
|
||||||
|
|
||||||
|
# Seed a user_turn whose prose contains an HTML-script payload. The
|
||||||
|
# modal renders ``description = "turn N (you: <prose excerpt>)"`` so
|
||||||
|
# the prose flows verbatim into the cascade list <li>.
|
||||||
|
with open_db(db) as conn:
|
||||||
|
evil_id = append_and_apply(
|
||||||
|
conn,
|
||||||
|
kind="user_turn",
|
||||||
|
payload={
|
||||||
|
"chat_id": "chat_bot_a",
|
||||||
|
"prose": "<script>alert('xss')</script>",
|
||||||
|
"segments": [],
|
||||||
|
},
|
||||||
|
)
|
||||||
|
|
||||||
|
response = client.get(
|
||||||
|
f"/chats/chat_bot_a/drawer/turn/delete-preview/{evil_id}"
|
||||||
|
)
|
||||||
|
assert response.status_code == 200
|
||||||
|
body = response.text
|
||||||
|
|
||||||
|
# Raw <script> must NOT survive into the rendered HTML. The escaped
|
||||||
|
# form (<script>) is what we want to see instead.
|
||||||
|
assert "<script>alert" not in body
|
||||||
|
assert "<script>alert" in body
|
||||||
|
|
||||||
|
|
||||||
def test_delete_turn_with_event_id_zero_returns_400(client, tmp_path):
|
def test_delete_turn_with_event_id_zero_returns_400(client, tmp_path):
|
||||||
"""T110.1: ``event_id <= 0`` is an obvious client error and must NOT
|
"""T110.1: ``event_id <= 0`` is an obvious client error and must NOT
|
||||||
silently rewind the entire log via ``after_event_id = -1``. The route
|
silently rewind the entire log via ``after_event_id = -1``. The route
|
||||||
|
|||||||
Reference in New Issue
Block a user