Skip to content

fix(cli): escape role and tool-call names in HTML session export - #61348

Closed
briandevans wants to merge 2 commits into
NousResearch:mainfrom
briandevans:fix/cli-html-export-escape-roles
Closed

fix(cli): escape role and tool-call names in HTML session export#61348
briandevans wants to merge 2 commits into
NousResearch:mainfrom
briandevans:fix/cli-html-export-escape-roles

Conversation

@briandevans

Copy link
Copy Markdown
Contributor

What does this PR do?

Closes an HTML/JS-injection gap in the standalone HTML session export (hermes_cli/session_export_html.py). The exporter already routes most interpolated fields through the module's own _escape_htmlargs, content, reasoning, title, model, system_prompt — but three sibling sites on the same builder were written raw:

  • the message role interpolated into the class="message message-{role} active" attribute, and
  • the same role in the role badge, and
  • the tool-call function name ({fn_name}) in the tool-call header.

fn_name comes from tc["function"]["name"], i.e. a tool-call name — for MCP/custom tools this is externally influenced. A session containing a tool call named <script>alert(document.domain)</script> (or a crafted role with an attribute breakout like "><script>…) injects live HTML/JS into the exported standalone .html, which executes when the user opens the export in a browser. This is exactly the escaping-consistency defect the rest of the file already guards against; only these sites were missed.

The fix reuses the file's own _escape_html for parity — role is escaped once into safe_role (it feeds both an attribute and text) and fn_name is escaped inline.

Related Issue

Fixes #

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • 🔒 Security fix

Changes Made

  • hermes_cli/session_export_html.py: escape role (class attribute + role badge, via a single safe_role = _escape_html(role)) and the tool-call fn_name in _generate_messages_html.
  • tests/hermes_cli/test_session_export_html_escape.py: new regression test asserting a <script> tool-call name and a markup role are emitted escaped, never raw.

How to Test

  1. uv run --with pytest --with pytest-asyncio python3 -m pytest tests/hermes_cli/test_session_export_html_escape.py -v
  2. Fail-before / pass-after: reverting only the production hunk makes both new assertions fail (raw <script> / <img …> leaks into the output); with the fix they pass (only the &lt;script&gt; / &lt;img …&gt; escaped form appears).
  3. Full session-export area is green: pytest tests/hermes_cli/test_session_export.py tests/hermes_cli/test_session_export_md.py tests/hermes_cli/test_sessions_export_md_cli.py tests/hermes_cli/test_session_export_html_escape.py -q → 35 passed.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15 (Python 3.11)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A

The standalone HTML session export escapes most interpolated fields via
_escape_html (args, content, reasoning, title, model, system_prompt), but a
few sibling sites were missed: the message role (used both in the class
attribute and the role badge) and the tool-call function name were written
raw.

A session whose tool call is named '<script>...</script>' (tool/MCP names
are externally influenced) or whose role contains markup injects live
HTML/JS into the exported artifact, which executes when the user opens the
export in a browser. Route both fields through the file's own _escape_html
for parity with the already-escaped siblings.
Copilot AI review requested due to automatic review settings July 9, 2026 07:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Hardens the standalone HTML session export against HTML/JS injection by ensuring role and tool-call names are escaped when rendered into exported markup, and adds regression coverage to prevent future escaping regressions.

Changes:

  • Escape message role when rendered into the message header and message wrapper class.
  • Escape tool-call function name when rendered into the tool-call header.
  • Add regression tests covering tool-call-name and role injection attempts.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
hermes_cli/session_export_html.py Escapes role and tool-call fn_name in _generate_messages_html to prevent injection in exported HTML.
tests/hermes_cli/test_session_export_html_escape.py Adds regression tests asserting tool-call names and roles are emitted escaped in HTML exports.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread hermes_cli/session_export_html.py Outdated
Comment on lines +685 to +688
# Build message HTML. Escape the role once: it feeds both an attribute
# (class) and text, and for tool/MCP messages it is externally influenced.
safe_role = _escape_html(role)
msg_class = f"message message-{safe_role} active"
Addresses Copilot review on NousResearch#61348: the HTML-escaped role, while safe from
injection (quotes are escaped), still contains whitespace when a crafted role
is supplied, which splits the class attribute into several unintended CSS
classes. Keep the escaped role for the display badge, and reduce the raw role
to a single safe CSS token (alnum/-/_) for the class name. Real roles
(user/assistant/system/tool) are unchanged, so the existing .message-<role>
rules still match.
@briandevans

Copy link
Copy Markdown
Contributor Author

@copilot Addressed in commit 3a7b6e8: the message role now feeds two sinks on their own terms — the display badge keeps the HTML-escaped role, while the class attribute uses a single normalized CSS token (alnum/-/_ only). A crafted role can no longer split into multiple unintended classes, and real roles (user/assistant/system/tool) are unchanged so the existing .message-<role> rules still match. Added a regression assertion that the class stays a single well-formed token.

@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists labels Jul 9, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #61343 (the stored-XSS report) and the earlier competing fix PR #61345. #61345 escapes the tool-call name and adds a restrictive CSP; this PR escapes the tool-call name and the message role (class attribute + role badge) but no CSP. They are complementary -- the union (escape role + fn_name + CSP) is the complete fix. Not a duplicate; flagging the cluster for a maintainer to reconcile.

@egilewski

Copy link
Copy Markdown
Contributor

looks mergeable

I reviewed the standalone HTML session export escaping fix against current GitHub main: the meaningful patch replayed cleanly onto 411d59976410bc6eabcd4d59b6f688de3e879f05, the exporter now escapes untrusted role and tool-call function-name display text while reducing role-derived CSS classes to a single safe token, existing real role classes such as assistant still match, and the focused session-export tests passed on the replay tree.

Security evidence:

  • trust boundary: persisted session message fields, including externally influenced tool/MCP function names and message roles, are rendered into a standalone HTML export that a user opens in a browser.
  • source/sink/invariant: role and tool_calls[].function.name must be rendered as data; display text must be HTML-escaped, the CSS class suffix must stay a single safe token, and existing real role classes must keep matching.
  • current-main reproduction: an import-pinned current-main probe showed raw <script>alert(1)</script> and raw <img src=x onerror=alert(document.domain)> reaching generated HTML, including the raw role inside the message class.
  • PR-head or patch-replay validation: import-pinned probes against PR head and a current-main replay showed no raw script or role markup, the escaped forms present, and one message-... class token; the current-main merge-tree also succeeded.
  • positive/negative cases: the new tests cover a script-shaped tool-call name, markup-shaped role, and preserved assistant CSS class; the focused session-export suite passed as 36 passed on the current-main replay.
  • residual bypass search: sibling message sinks for content, tool arguments, reasoning, system prompt, title, and model remain escaped, and I did not find another role/tool-name path that bypasses this fix.
  • reviewer validation: CodeRabbit completed with no findings in the clean-pass flow.

Because I validated the meaningful patch replayed onto current GitHub main, that replay supports the changed behavior but does not by itself prove the submitted branch will keep merging cleanly if main moves again.

Signed: GPT-5.5-xhigh in Codex

@alt-glitch alt-glitch added the sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data label Jul 9, 2026
teknium1 pushed a commit that referenced this pull request Jul 10, 2026
Addresses Copilot review on #61348: the HTML-escaped role, while safe from
injection (quotes are escaped), still contains whitespace when a crafted role
is supplied, which splits the class attribute into several unintended CSS
classes. Keep the escaped role for the display badge, and reduce the raw role
to a single safe CSS token (alnum/-/_) for the class name. Real roles
(user/assistant/system/tool) are unchanged, so the existing .message-<role>
rules still match.
@teknium1

Copy link
Copy Markdown
Contributor

Merged via PR #61769 — your commit was cherry-picked onto current main with your authorship preserved in git log (rebase merge, commit c49d51b). Thanks @briandevans — and for the record: you were the EARLIEST submitter of the tool-call-name escape (this PR predates the merged #61345 by ~10 hours). The fn_name portion had already landed via #61345, so the salvage carried your surviving remainder: the role-badge escaping and the CSS-class token sanitization (a sink the other PRs missed entirely, since tool/MCP roles can be externally influenced), plus your test file. Good, thorough work.

@teknium1 teknium1 closed this Jul 10, 2026
santhreal pushed a commit to santhreal/hermes-agent that referenced this pull request Jul 13, 2026
Addresses Copilot review on NousResearch#61348: the HTML-escaped role, while safe from
injection (quotes are escaped), still contains whitespace when a crafted role
is supplied, which splits the class attribute into several unintended CSS
classes. Keep the escaped role for the display badge, and reduce the raw role
to a single safe CSS token (alnum/-/_) for the class name. Real roles
(user/assistant/system/tool) are unchanged, so the existing .message-<role>
rules still match.
justemu pushed a commit to justemu/hermes-agent that referenced this pull request Jul 18, 2026
Addresses Copilot review on NousResearch#61348: the HTML-escaped role, while safe from
injection (quotes are escaped), still contains whitespace when a crafted role
is supplied, which splits the class attribute into several unintended CSS
classes. Keep the escaped role for the display badge, and reduce the raw role
to a single safe CSS token (alnum/-/_) for the class name. Real roles
(user/assistant/system/tool) are unchanged, so the existing .message-<role>
rules still match.
Gravezzz pushed a commit to Gravezzz/hermes-agent that referenced this pull request Jul 21, 2026
Addresses Copilot review on NousResearch#61348: the HTML-escaped role, while safe from
injection (quotes are escaped), still contains whitespace when a crafted role
is supplied, which splits the class attribute into several unintended CSS
classes. Keep the escaped role for the display badge, and reduce the raw role
to a single safe CSS token (alnum/-/_) for the class name. Real roles
(user/assistant/system/tool) are unchanged, so the existing .message-<role>
rules still match.
leewenjie pushed a commit to leewenjie/hermes-agent that referenced this pull request Aug 7, 2026
Addresses Copilot review on NousResearch#61348: the HTML-escaped role, while safe from
injection (quotes are escaped), still contains whitespace when a crafted role
is supplied, which splits the class attribute into several unintended CSS
classes. Keep the escaped role for the display badge, and reduce the raw role
to a single safe CSS token (alnum/-/_) for the class name. Real roles
(user/assistant/system/tool) are unchanged, so the existing .message-<role>
rules still match.
randlee pushed a commit to randlee/hermes-agent that referenced this pull request Aug 11, 2026
Addresses Copilot review on NousResearch#61348: the HTML-escaped role, while safe from
injection (quotes are escaped), still contains whitespace when a crafted role
is supplied, which splits the class attribute into several unintended CSS
classes. Keep the escaped role for the display badge, and reduce the raw role
to a single safe CSS token (alnum/-/_) for the class name. Real roles
(user/assistant/system/tool) are unchanged, so the existing .message-<role>
rules still match.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants