diff --git a/CHANGELOG.md b/CHANGELOG.md index c8874614..2c94a1f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,8 @@ - **Deep-link straight to a profile with `?profile=`.** Opening the WebUI with a `?profile=NAME` query parameter now switches to that profile on boot. The name is validated against the existing profile allowlist (a nonexistent profile boots normally, path-traversal / cross-profile attempts are rejected), the switch goes through the same access-enforced `switchToProfile`, and boot fails safe on anything invalid. Thanks @rodboev. (#5730, #5682) +- **"Approve once" no longer silently sticks for the whole session.** On local (non-gateway) deployments, choosing **Approve once** on a tool approval prompt was wrongly persisting as a session-wide approval, so the next matching guarded tool call skipped its approval card. "Once" now applies to only the single call that prompted it, and the next matching call re-prompts as intended; "Approve for session" and "Always allow" are unchanged. Thanks @rudidev08. (#6035, #6017) + - **Conversation actions menu is now keyboard- and screen-reader-accessible.** The per-conversation actions popup (the ⋯ menu in the sidebar) now exposes proper ARIA (`aria-expanded`/`aria-controls` on the trigger, `role=menu`/`menuitem` on the items), moves focus to the first action when opened, supports Arrow/Home/End navigation and Escape-to-close, and restores focus to the originating trigger on close. The existing click/touch behavior is unchanged. Thanks @sheldon-im. (#6019) - **Model picker no longer duplicates or drops proxy/catalog models.** When the same model was reachable via both a custom proxy and a first-party catalog (e.g. `grok-4.5`), the picker could show it twice; the dedup fix collapses true duplicates to one selectable row while keeping genuinely-distinct models that merely share a name suffix — including colon-suffixed proxy models like `…:free` / `…:thinking` / `…:latest` — independently selectable. The currently-selected model always survives dedup, and model restore no longer substitutes a suffix-sharing sibling for a missing model. Thanks @rodboev. (#5997, #5989) diff --git a/api/routes.py b/api/routes.py index 9d4685c1..511af492 100644 --- a/api/routes.py +++ b/api/routes.py @@ -23344,7 +23344,7 @@ def _resolve_approval_legacy(sid: str, approval_id: str, choice: str) -> bool: # Collect keys from both _pending and _gateway_queues keys_from_pending = pending.get("pattern_keys") or [pending.get("pattern_key", "")] if pending else [] all_keys = [k for k in keys_from_pending if k] + [k for k in gateway_keys if k] - if choice in ("once", "session"): + if choice == "session": for k in all_keys: approve_session(sid, k) elif choice == "always": @@ -23352,6 +23352,11 @@ def _resolve_approval_legacy(sid: str, approval_id: str, choice: str) -> bool: approve_session(sid, k) approve_permanent(k) save_permanent_allowlist(_permanent_approved) + # choice == "once": no persistence — approval lasts this single call only. + # resolve_gateway_approval() below unblocks the parked agent thread for + # every choice, so "once" still lets the current tool run; we just must not + # call approve_session() here, or the next matching guarded call would find + # the pattern already session-approved and skip its approval card (#6017). # Unblock the agent thread waiting in the gateway approval queue. # This is the primary signal when streaming is active — the agent # thread is parked in entry.event.wait() and needs to be woken up. diff --git a/tests/test_issue6017_approve_once_no_persist.py b/tests/test_issue6017_approve_once_no_persist.py new file mode 100644 index 00000000..a6d5ec83 --- /dev/null +++ b/tests/test_issue6017_approve_once_no_persist.py @@ -0,0 +1,100 @@ +"""Regression test for #6017: "Approve once" must not persist as a session +approval. + +Bug +--- +In the local (non-gateway) approval path, ``_resolve_approval_legacy`` in +``api/routes.py`` grouped ``"once"`` together with ``"session"`` and called +``approve_session()`` for both. ``approve_session()`` records the pattern in the +session-wide allowlist that the guard later consults, so a single "Approve +once" click silently approved every later matching tool call for the whole +session -- a destructive tool could then run with no approval card. + +Fix +--- +Only ``"session"``/``"always"`` persist. ``"once"`` releases the currently +parked call via ``resolve_gateway_approval()`` (which unblocks the waiting agent +thread for every choice) and writes nothing to ``_session_approved``. + +State layer: ``tools.approval._session_approved`` -- the session-scoped approval +allowlist that ``is_approved()`` consults. These tests drive the real +``_resolve_approval_legacy`` against a seeded gateway approval entry and assert +the persistence outcome directly via ``is_approved()``. +""" +from __future__ import annotations + +import importlib +import uuid + +from tests.conftest import requires_agent_modules + +pytestmark = requires_agent_modules + + +def _seed_gateway_entry(ta, sid, pattern_key): + """Park one guarded command in the gateway queue for ``sid``. + + Mirrors the local in-process agent path: a guarded command blocks on an + ``_ApprovalEntry`` in ``_gateway_queues`` waiting for the user's choice. + """ + entry = ta._ApprovalEntry({ + "command": "rm -rf /tmp/6017", + "pattern_key": pattern_key, + "pattern_keys": [pattern_key], + "description": "recursive delete", + }) + with ta._lock: + ta._pending.pop(sid, None) + ta._gateway_queues.setdefault(sid, []).append(entry) + return entry + + +def _cleanup(ta, sid): + with ta._lock: + ta._gateway_queues.pop(sid, None) + ta._session_approved.pop(sid, None) + ta._pending.pop(sid, None) + + +def test_approve_once_releases_call_but_does_not_persist(): + """'once' unblocks the current call yet leaves nothing session-approved, so + the next matching guarded call still re-prompts.""" + routes = importlib.import_module("api.routes") + ta = importlib.import_module("tools.approval") + + sid = f"issue6017-once-{uuid.uuid4().hex[:8]}" + key = f"pattern-{uuid.uuid4().hex[:8]}" + entry = _seed_gateway_entry(ta, sid, key) + try: + assert not ta.is_approved(sid, key), "precondition: pattern not yet approved" + assert not entry.event.is_set(), "precondition: parked call not yet released" + + routes._resolve_approval_legacy(sid, "", "once") + + assert entry.event.is_set(), "'once' must release the currently parked call" + assert ta.is_approved(sid, key) is False, ( + "'once' must NOT persist a session approval -- the next matching " + "guarded call must re-prompt (#6017)" + ) + finally: + _cleanup(ta, sid) + + +def test_approve_session_persists_session_approval(): + """Regression guard: 'session' still persists, so a later matching call in + the same session is not re-prompted.""" + routes = importlib.import_module("api.routes") + ta = importlib.import_module("tools.approval") + + sid = f"issue6017-session-{uuid.uuid4().hex[:8]}" + key = f"pattern-{uuid.uuid4().hex[:8]}" + entry = _seed_gateway_entry(ta, sid, key) + try: + routes._resolve_approval_legacy(sid, "", "session") + + assert entry.event.is_set(), "'session' must release the currently parked call" + assert ta.is_approved(sid, key) is True, ( + "'session' must persist so later matching calls are not re-prompted" + ) + finally: + _cleanup(ta, sid)