From 0a6947c2d2b98ea72290c60434879149fecd40dc Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Thu, 25 Jun 2026 02:15:47 +0000 Subject: [PATCH] fix(#4754): namespace dismissed approvals by session (gate finding) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex CORE: the dismissed-set was keyed by bare approval_id, but gateway/run sources preserve externally-supplied IDs without session namespacing — so two sessions carrying the same approval_id collide and dismissing in session A hides session B's still-pending approval. Fix: _approvalDismissKey(sid, approvalId) builds a session-namespaced composite key, threaded through _isApprovalDismissed, _markApprovalDismissed, _unmarkApprovalDismissed + all 5 call sites (showApprovalCard, dismissApprovalCard, respondApproval, no-pending poll cleanup). Updated 4 stale assertions to the namespaced signatures + added a functional node-driver cross-session-collision regression (proven non-vacuous). Co-authored-by: Rod Boev --- static/messages.js | 40 +++++--- ...test_issue4754_approval_dismiss_persist.py | 91 +++++++++++++++++-- 2 files changed, 108 insertions(+), 23 deletions(-) diff --git a/static/messages.js b/static/messages.js index cdee9cb87..61df0522b 100644 --- a/static/messages.js +++ b/static/messages.js @@ -5305,27 +5305,39 @@ let _approvalPendingBySession = new Map(); const _DISMISSED_APPROVALS_KEY = 'hermes_dismissed_approvals'; +// Dismissed approvals are namespaced by session so that two sessions carrying +// the SAME approval_id (e.g. a gateway/run source that reuses externally +// supplied IDs across sessions) can't have a dismissal in one session hide the +// other's still-pending approval. Stored value is "\u0000". +function _approvalDismissKey(sid, approvalId) { + if (!approvalId) return ''; + return String(sid || '') + '\u0000' + String(approvalId); +} + function _getDismissedApprovals() { try { return JSON.parse(localStorage.getItem(_DISMISSED_APPROVALS_KEY) || '[]'); } catch (_) { return []; } } -function _isApprovalDismissed(approvalId) { - if (!approvalId) return false; - return _getDismissedApprovals().includes(approvalId); +function _isApprovalDismissed(sid, approvalId) { + const key = _approvalDismissKey(sid, approvalId); + if (!key) return false; + return _getDismissedApprovals().includes(key); } -function _markApprovalDismissed(approvalId) { - if (!approvalId) return; - const set = _getDismissedApprovals().filter(id => id !== approvalId); - set.push(approvalId); +function _markApprovalDismissed(sid, approvalId) { + const key = _approvalDismissKey(sid, approvalId); + if (!key) return; + const set = _getDismissedApprovals().filter(k => k !== key); + set.push(key); try { localStorage.setItem(_DISMISSED_APPROVALS_KEY, JSON.stringify(set.slice(-100))); } catch (_) {} } -function _unmarkApprovalDismissed(approvalId) { - if (!approvalId) return; - const set = _getDismissedApprovals().filter(id => id !== approvalId); +function _unmarkApprovalDismissed(sid, approvalId) { + const key = _approvalDismissKey(sid, approvalId); + if (!key) return; + const set = _getDismissedApprovals().filter(k => k !== key); try { localStorage.setItem(_DISMISSED_APPROVALS_KEY, JSON.stringify(set)); } catch (_) {} } @@ -5387,7 +5399,7 @@ function showApprovalForSession(sid, pending, pendingCount) { function showApprovalCard(pending, pendingCount) { const sid = _rememberApprovalPending(pending, pendingCount); if (!_approvalPromptBelongsToActiveSession(sid)) return; - if (pending && pending.approval_id && _isApprovalDismissed(pending.approval_id)) return; + if (pending && pending.approval_id && _isApprovalDismissed(sid, pending.approval_id)) return; const keys = pending.pattern_keys || (pending.pattern_key ? [pending.pattern_key] : []); const desc = (pending.description || "") + (keys.length ? " [" + keys.join(", ") + "]" : ""); const cmd = pending.command || ""; @@ -5432,8 +5444,8 @@ function showApprovalCard(pending, pendingCount) { } function dismissApprovalCard() { - if (_approvalCurrentId) _markApprovalDismissed(_approvalCurrentId); const sid = _approvalSessionId; + if (_approvalCurrentId) _markApprovalDismissed(sid, _approvalCurrentId); hideApprovalCard(true); if (sid) _clearApprovalPendingForSession(sid); } @@ -5501,7 +5513,7 @@ async function respondApproval(choice) { const sid = _approvalSessionId || (S.session && S.session.session_id); if (!sid) return; const approvalId = _approvalCurrentId; - _unmarkApprovalDismissed(approvalId); + _unmarkApprovalDismissed(sid, approvalId); // Disable all buttons immediately to prevent double-submit ["approvalBtnOnce","approvalBtnSession","approvalBtnAlways","approvalBtnDeny"].forEach(id => { const b = $(id); @@ -5558,7 +5570,7 @@ function _startApprovalFallbackPoll(sid) { const _resolvedEntry = _approvalPendingBySession.get(sid); _clearApprovalPendingForSession(sid); const _resolvedId = _resolvedEntry && _resolvedEntry.pending && _resolvedEntry.pending.approval_id; - if (_resolvedId) _unmarkApprovalDismissed(_resolvedId); + if (_resolvedId) _unmarkApprovalDismissed(sid, _resolvedId); _hideApprovalCardIfOwner(sid); if (!S.busy) { stopApprovalPollingForSession(sid); diff --git a/tests/test_issue4754_approval_dismiss_persist.py b/tests/test_issue4754_approval_dismiss_persist.py index de45665c1..2a97f3794 100644 --- a/tests/test_issue4754_approval_dismiss_persist.py +++ b/tests/test_issue4754_approval_dismiss_persist.py @@ -58,8 +58,9 @@ def test_guard_in_show_approval_card(): # Guard must appear inside showApprovalCard, after _rememberApprovalPending func_start = compact.find("functionshowApprovalCard(") assert func_start != -1 - # Locate the guard after the function start - guard = "_isApprovalDismissed(pending.approval_id)" + # Locate the guard after the function start (dismissals are namespaced by + # session, so the guard passes sid + approval_id). + guard = "_isApprovalDismissed(sid,pending.approval_id)" guard_idx = compact.find(guard, func_start) assert guard_idx != -1, "guard _isApprovalDismissed must appear in showApprovalCard" # _rememberApprovalPending must appear before the guard @@ -72,7 +73,7 @@ def test_guard_in_show_approval_card(): def test_guard_returns_early(): # The guard must be a return statement compact = _compact(MESSAGES_JS) - assert "if(pending&&pending.approval_id&&_isApprovalDismissed(pending.approval_id))return;" in compact + assert "if(pending&&pending.approval_id&&_isApprovalDismissed(sid,pending.approval_id))return;" in compact # --------------------------------------------------------------------------- @@ -89,7 +90,7 @@ def test_dismiss_approval_card_marks_dismissed(): assert func_start != -1 body_end = compact.find("}", func_start) body = compact[func_start:body_end + 1] - assert "_markApprovalDismissed(_approvalCurrentId)" in body + assert "_markApprovalDismissed(sid,_approvalCurrentId)" in body def test_dismiss_approval_card_hides_card(): @@ -138,8 +139,8 @@ def test_respond_approval_unmarks_dismissed(): func_start = compact.find("asyncfunctionrespondApproval(") assert func_start != -1 # Find the closing brace of the function (scan for matching }) - assert "_unmarkApprovalDismissed(approvalId)" in compact[func_start:], \ - "_unmarkApprovalDismissed(approvalId) must be called inside respondApproval" + assert "_unmarkApprovalDismissed(sid,approvalId)" in compact[func_start:], \ + "_unmarkApprovalDismissed(sid,approvalId) must be called inside respondApproval" def test_respond_approval_unmarks_before_clear(): @@ -148,7 +149,7 @@ def test_respond_approval_unmarks_before_clear(): compact = _compact(MESSAGES_JS) func_start = compact.find("asyncfunctionrespondApproval(") assert func_start != -1 - unmark_idx = compact.find("_unmarkApprovalDismissed(approvalId)", func_start) + unmark_idx = compact.find("_unmarkApprovalDismissed(sid,approvalId)", func_start) clear_idx = compact.find("_approvalCurrentId=null;", func_start) assert unmark_idx != -1 assert clear_idx != -1 @@ -170,10 +171,10 @@ def test_no_pending_branch_unmarks_dismissed(): # Must use session-scoped lookup, not the global _approvalCurrentId assert "_approvalPendingBySession.get(sid)" in nearby, \ "no-pending poll branch must read session-scoped pending via _approvalPendingBySession.get(sid)" - assert "_unmarkApprovalDismissed(_resolvedId)" in nearby, \ + assert "_unmarkApprovalDismissed(sid,_resolvedId)" in nearby, \ "no-pending poll branch must unmark the session-scoped resolved ID" # Must NOT use the global _approvalCurrentId to unmark (that caused the cross-session bug) - assert "_unmarkApprovalDismissed(_approvalCurrentId)" not in nearby, \ + assert "_unmarkApprovalDismissed(sid,_approvalCurrentId)" not in nearby, \ "no-pending poll branch must not unmark via the global _approvalCurrentId" @@ -248,3 +249,75 @@ def test_dismiss_button_near_collapse_button(): def test_approval_dismiss_css_defined(): assert ".approval-dismiss" in STYLE_CSS + + +# --------------------------------------------------------------------------- +# Functional: session-namespaced dismissal (the cross-session collision fix) +# --------------------------------------------------------------------------- + +import json +import shutil +import subprocess + +NODE = shutil.which("node") + + +def _extract_fn(src: str, name: str) -> str: + start = src.index(f"function {name}(") + brace = src.index("{", start) + depth = 0 + for i in range(brace, len(src)): + if src[i] == "{": + depth += 1 + elif src[i] == "}": + depth -= 1 + if depth == 0: + return src[start:i + 1] + raise AssertionError(f"{name} body not closed") + + +def test_same_approval_id_in_two_sessions_does_not_collide(): + """The bug Codex found: dismissing approval_id 'X' in session A must NOT hide + a still-pending approval_id 'X' in session B (gateway/run sources can reuse + externally-supplied IDs across sessions). Drives the real helper functions in + node with a mocked localStorage.""" + if NODE is None: + import pytest + pytest.skip("node not available") + helpers = "\n".join( + _extract_fn(MESSAGES_JS, n) + for n in ("_approvalDismissKey", "_getDismissedApprovals", + "_isApprovalDismissed", "_markApprovalDismissed", "_unmarkApprovalDismissed") + ) + script = ( + "const _store = {};\n" + "const localStorage = {\n" + " getItem: k => (k in _store ? _store[k] : null),\n" + " setItem: (k, v) => { _store[k] = String(v); },\n" + "};\n" + "const _DISMISSED_APPROVALS_KEY = 'hermes_dismissed_approvals';\n" + + helpers + + "\n" + "// Dismiss approval 'X' in session A.\n" + "_markApprovalDismissed('sessionA', 'X');\n" + "const out = {\n" + " a_dismissed: _isApprovalDismissed('sessionA', 'X'),\n" + " b_not_dismissed: _isApprovalDismissed('sessionB', 'X'),\n" + " diff_id_same_session_not_dismissed: _isApprovalDismissed('sessionA', 'Y'),\n" + "};\n" + "// Responding in session B (unmark) must not clear session A's dismissal.\n" + "_unmarkApprovalDismissed('sessionB', 'X');\n" + "out.a_still_dismissed_after_b_unmark = _isApprovalDismissed('sessionA', 'X');\n" + "process.stdout.write(JSON.stringify(out));\n" + ) + result = subprocess.run([NODE, "-e", script], check=True, capture_output=True, text=True, timeout=15) + out = json.loads(result.stdout) + assert out["a_dismissed"] is True, "session A's own dismissal must register" + assert out["b_not_dismissed"] is False, ( + "CROSS-SESSION COLLISION: dismissing approval_id 'X' in session A must NOT " + "hide the same approval_id 'X' in session B" + ) + assert out["diff_id_same_session_not_dismissed"] is False + assert out["a_still_dismissed_after_b_unmark"] is True, ( + "un-dismissing 'X' in session B must not clear session A's dismissal of 'X'" + )