Skip to content

security(tui): strip stale dangerous confirmations on session resume - #60394

Open
isheng-eqi wants to merge 2 commits into
NousResearch:mainfrom
isheng-eqi:clean/tui-stale-confirmation
Open

security(tui): strip stale dangerous confirmations on session resume#60394
isheng-eqi wants to merge 2 commits into
NousResearch:mainfrom
isheng-eqi:clean/tui-stale-confirmation

Conversation

@isheng-eqi

Copy link
Copy Markdown
Contributor

What does this PR do?

The TUI gateway has two session resume paths that strip dangling tool-call tails but do not expire stale dangerous-confirmation text — unlike the main gateway, which already does this (#60110, #60117).

When a user confirms a destructive action in a TUI session, that confirmation text persists. On resume, the model could re-interpret an unrelated message as a fresh confirmation and re-trigger the destructive action.

Related Issue

Fixes the TUI sibling path of #59607.

Type of Change

  • Bug fix
  • Security fix

Changes Made

  • tui_gateway/server.py: Import strip_stale_dangerous_confirmations, add call after sanitize_replay_history() in both TUI resume paths
  • tests/tui_gateway/test_stale_confirmation_resume.py: 4 tests covering stale/fresh confirmation, expiry window, and dangerous pattern detection

How to Test

  1. Start TUI, confirm a destructive action
  2. Kill the session, resume it after 60+ seconds
  3. Verify the confirmation text is stripped from the replayed history

The gateway already strips stale dangerous-confirmation text from
replayed history (NousResearch#60110, NousResearch#60117 — fix for NousResearch#59607). The TUI gateway
has two session resume paths that sanitize replay history but do not
strip stale dangerous confirmations, leaving the same vulnerability
unaddressed in TUI sessions.

Add strip_stale_dangerous_confirmations() after sanitize_replay_history()
in both TUI resume paths (resume_session and the WebUI resume path).

Refs: NousResearch#59607, NousResearch#60110, NousResearch#60117
@alt-glitch alt-glitch added type/bug Something isn't working comp/tui Terminal UI (ui-tui/ + tui_gateway/) sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state P2 Medium — degraded but workaround exists labels Jul 7, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for tracing the TUI sibling of the gateway expiry fix. The normal TUI cold-resume premise is verified on current main: tui_gateway/server.py:5774 and tui_gateway/server.py:5852 sanitize replay tails but do not call the stale-confirmation expiry helper.

Problems

  • The patch leaves lazy/watch resume unprotected. That branch loads raw history at tui_gateway/server.py:5699 and stores it in the deferred session record at tui_gateway/server.py:5709. After the watch session upgrades, _run_prompt_submit forwards that stored history as conversation_history at tui_gateway/server.py:8951 and tui_gateway/server.py:9094-9103.
  • The new tests directly exercise the helper, not session.resume. tests/tui_gateway/test_protocol.py:338-462 and tests/tui_gateway/test_protocol.py:582-652 provide existing resume harnesses that can verify the actual server wiring.

Suggested changes

  • Expire confirmations in the lazy branch too, before constructing its deferred record.
  • Add resume-level coverage for deferred, eager, and lazy modes, including stale and fresh confirmations.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/sessions Session lifecycle, resume, persistence, history labels Jul 15, 2026

@GottZ GottZ left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This was generated by AI during triage.

Summary

Two PRs address the TUI sibling of #59607. Both apply the same expiry helper to the eager and deferred cold-resume paths, but neither protects the lazy/watch resume path, and their tests cover the helper rather than the actual resume wiring.

Related pull requests

  • #60209 [closed] duplicate — (+64/-1) — superseded duplicate: The diff adds stale-confirmation expiry after replay sanitization in two cold-resume paths plus helper-level tests, but misses lazy/watch resume; this closed PR remains relevant as the contaminated-branch predecessor that was rebuilt as #60394.
  • #60394 related — (+64/-1) — keep open with a salvage path: The two cold-resume call sites are useful, but the diff leaves raw history unexpired in the lazy/watch path and does not test session.resume wiring. This is consistent with the automated keep-open review, with salvage requiring coverage of the lazy branch and resume-level tests for deferred, eager, and lazy modes.

Duplicates

#60209 and #60394 contain essentially the same change; #60394 is the clean-base replacement for the closed #60209.

Suggested consolidation

Keep #60394 open with a salvage path: preserve its eager and deferred cold-resume expiry calls, add expiry before the lazy/watch deferred record is constructed, and replace or supplement the helper-only tests with stale/fresh session.resume coverage across deferred, eager, and lazy modes. Treat #60209 as the superseded duplicate of #60394 and leave it closed.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    subgraph Dup60209 ["PRs duplicating each other"]
        P60209["PR #60209 (closed)"]
        P60394["PR #60394 (open)"]
    end
    class P60209 closed
    class P60394 open
    class P60394 target
    click P60209 "https://github.com/NousResearch/hermes-agent/pull/60209"
    click P60394 "https://github.com/NousResearch/hermes-agent/pull/60394"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 10 kB of PR diffs, 2 kB of issue/PR text, <1 kB of discussion (1 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/tui Terminal UI (ui-tui/ + tui_gateway/) P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants