[Hardening] Escape HTML in recap emails; stop echoing raw LLM errors from /ask #109

Closed
opened 2026-07-14 19:50:10 +00:00 by claude-bot · 2 comments
Contributor

Context

Two output-hygiene gaps in the backend.

(a) Recap email HTML injection. send_recap_email (webapp/backend/app/tasks/reminder_tasks.py:234) builds an HTML body by direct f-string interpolation with no escaping (:306-314):

  • data['session_title'] — GM-controlled — into the <h2> (:307) and also into the Subject: header (:303)
  • data['campaign_name'] — GM-controlled — into a <p> (:308)
  • data['summary'] — LLM-generated from player speech — into the body (:311)

The result is attached as MIMEText(html_body, "html") (:319). A session title like <script>...</script> or any HTML in the summary is delivered as live markup to every recipient's mail client — stored-content HTML injection.

(b) Raw LLM error echoed to clients. The /ask bot endpoint returns detail=f"LLM error: {exc}" (webapp/backend/app/routers/bot.py:964). Exception text from the LLM client can leak internal endpoint URLs/config, and it violates the project-wide error contract (human-readable detail, no internals).

Fix / Spec

  1. In send_recap_email, wrap every interpolated value with html.escape(...): session_title, campaign_name, and summary (escape before the .replace(chr(10), '<br>') newline conversion so the inserted <br> tags survive).
  2. Also sanitize the subject line: strip \r/\n from session_title before building Subject: (header-injection guard).
  3. At routers/bot.py:964, replace the detail with a generic message (e.g. "LLM request failed") and log.exception(...) / log.error(...) the real exception server-side.

Acceptance criteria

  • Unit test: a session title containing <script>alert(1)</script> produces an email body where it appears escaped (&lt;script&gt;...), and the summary is likewise escaped while newlines still become <br>.
  • Unit test or endpoint test: an LLM failure on /ask returns a generic detail with no exception text, and the exception is logged.
  • No other behavior change to email sending.

References

  • webapp/backend/app/tasks/reminder_tasks.py:234 (send_recap_email), :303 (subject), :306-314 (html_body), :319 (MIMEText html)
  • webapp/backend/app/routers/bot.py:964 (LLM error: {exc})
  • Error-format contract: root CLAUDE.md ("Error response format")

Filed from the July 2026 full-project review.

## Context Two output-hygiene gaps in the backend. **(a) Recap email HTML injection.** `send_recap_email` (`webapp/backend/app/tasks/reminder_tasks.py:234`) builds an HTML body by direct f-string interpolation with no escaping (`:306-314`): - `data['session_title']` — GM-controlled — into the `<h2>` (`:307`) and also into the `Subject:` header (`:303`) - `data['campaign_name']` — GM-controlled — into a `<p>` (`:308`) - `data['summary']` — LLM-generated from player speech — into the body (`:311`) The result is attached as `MIMEText(html_body, "html")` (`:319`). A session title like `<script>...</script>` or any HTML in the summary is delivered as live markup to every recipient's mail client — stored-content HTML injection. **(b) Raw LLM error echoed to clients.** The `/ask` bot endpoint returns `detail=f"LLM error: {exc}"` (`webapp/backend/app/routers/bot.py:964`). Exception text from the LLM client can leak internal endpoint URLs/config, and it violates the project-wide error contract (human-readable `detail`, no internals). ## Fix / Spec 1. In `send_recap_email`, wrap every interpolated value with `html.escape(...)`: `session_title`, `campaign_name`, and `summary` (escape **before** the `.replace(chr(10), '<br>')` newline conversion so the inserted `<br>` tags survive). 2. Also sanitize the subject line: strip `\r`/`\n` from `session_title` before building `Subject:` (header-injection guard). 3. At `routers/bot.py:964`, replace the detail with a generic message (e.g. `"LLM request failed"`) and `log.exception(...)` / `log.error(...)` the real exception server-side. ## Acceptance criteria - Unit test: a session title containing `<script>alert(1)</script>` produces an email body where it appears escaped (`&lt;script&gt;...`), and the summary is likewise escaped while newlines still become `<br>`. - Unit test or endpoint test: an LLM failure on `/ask` returns a generic `detail` with no exception text, and the exception is logged. - No other behavior change to email sending. ## References - `webapp/backend/app/tasks/reminder_tasks.py:234` (`send_recap_email`), `:303` (subject), `:306-314` (html_body), `:319` (MIMEText html) - `webapp/backend/app/routers/bot.py:964` (`LLM error: {exc}`) - Error-format contract: root `CLAUDE.md` ("Error response format") _Filed from the July 2026 full-project review._
Author
Contributor

Picking this up as part of a v3.3.0 push. Landing on branch hardening/backend together with #88, #90, #97, and #106 (grouped by component to keep the diffs reviewable).

Picking this up as part of a v3.3.0 push. Landing on branch `hardening/backend` together with #88, #90, #97, and #106 (grouped by component to keep the diffs reviewable).
Author
Contributor

Fixed on main (commit 54437d4, merged via 1c9c19f). send_recap_email now html.escape()s session_title, campaign_name, and summary before the chr(10)<br> conversion (so the inserted <br> survive), and strips CR/LF from the title before the Subject: header. /ask (routers/bot.py) returns a generic "LLM request failed" detail and logger.exception(...)s the real error server-side. Tests: <script>alert(1)</script> arrives escaped in the body, summary escaped with newlines still becoming <br>, header injection neutralized into subject text, and /ask returns the generic detail with the endpoint URL absent from the response but present in the log. Backend suite green (358 passed), ruff clean.

Separately filed #129 for an incidental latent bug found here (Celery tasks using the pooled AsyncSessionLocal instead of task_session() — out of scope for this issue, deferred to v3.4.0).

Fixed on `main` (commit `54437d4`, merged via `1c9c19f`). `send_recap_email` now `html.escape()`s `session_title`, `campaign_name`, and `summary` **before** the `chr(10)`→`<br>` conversion (so the inserted `<br>` survive), and strips CR/LF from the title before the `Subject:` header. `/ask` (`routers/bot.py`) returns a generic `"LLM request failed"` detail and `logger.exception(...)`s the real error server-side. Tests: `<script>alert(1)</script>` arrives escaped in the body, summary escaped with newlines still becoming `<br>`, header injection neutralized into subject text, and `/ask` returns the generic detail with the endpoint URL absent from the response but present in the log. Backend suite green (358 passed), ruff clean. Separately filed #129 for an incidental latent bug found here (Celery tasks using the pooled `AsyncSessionLocal` instead of `task_session()` — out of scope for this issue, deferred to v3.4.0).
Sign in to join this conversation.
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
rbrooks/Quest-Board#109
No description provided.