From 2a4bba2b9e01716c69d5d0ef5f0c72545edae973 Mon Sep 17 00:00:00 2001 From: Marius Popa Date: Thu, 11 Jun 2026 20:23:54 +0300 Subject: [PATCH 1/2] fix(api-keys): preserve encrypted keys when saving providers (#1920) * fix(api-keys): preserve encrypted keys when saving providers * test(api-keys): cover malformed raw key entries --------- Co-authored-by: Alexandre Teixeira <111787685+alteixeira20@users.noreply.github.com> --- src/api_key_manager.py | 8 ++++++-- tests/test_api_key_manager_resilience.py | 16 ++++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/api_key_manager.py b/src/api_key_manager.py index 650a1fbf7..f0d25ced6 100644 --- a/src/api_key_manager.py +++ b/src/api_key_manager.py @@ -57,7 +57,12 @@ class APIKeyManager: # Legacy/wrong shape (e.g. a list) — .items() would raise. Ignore it. logger.warning("API keys file has unexpected shape (%s); ignoring", type(encrypted_keys).__name__) return {} - return encrypted_keys + + return { + str(provider): key + for provider, key in encrypted_keys.items() + if isinstance(key, str) + } def save(self, provider: str, api_key: str): """Save encrypted API key to file. @@ -82,4 +87,3 @@ class APIKeyManager: except (InvalidToken, ValueError) as e: logger.warning("Failed to decrypt API key for %s: %s", provider, e) return decrypted - diff --git a/tests/test_api_key_manager_resilience.py b/tests/test_api_key_manager_resilience.py index 8654a6984..a209b0a29 100644 --- a/tests/test_api_key_manager_resilience.py +++ b/tests/test_api_key_manager_resilience.py @@ -33,3 +33,19 @@ def test_api_key_manager_load_resilience(tmp_path): assert loaded["good_provider"] == "good_value" assert "bad_provider" not in loaded assert "garbage_provider" not in loaded + + +def test_load_ignores_non_string_raw_values(tmp_path): + mgr = APIKeyManager(str(tmp_path)) + + mgr.save("openai", "sk-openai") + with open(mgr.api_keys_file, "r", encoding="utf-8") as f: + keys = json.load(f) + + keys["missing_provider"] = None + keys["numeric_provider"] = 42 + keys["object_provider"] = {"encrypted": keys["openai"]} + with open(mgr.api_keys_file, "w", encoding="utf-8") as f: + json.dump(keys, f) + + assert mgr.load() == {"openai": "sk-openai"} From c0cc0f954c9fc5da7fbfd01fcb87be2bd1385b19 Mon Sep 17 00:00:00 2001 From: Michael <52305679+michaelxer@users.noreply.github.com> Date: Fri, 12 Jun 2026 01:14:41 +0700 Subject: [PATCH 2/2] fix: read allow_bash/allow_web_search from JSON body (#3229) (#3281) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: read allow_bash/allow_web_search from JSON body (#3229) API callers using Content-Type: application/json had bash and web tools silently disabled because allow_bash / allow_web_search were only read from FormData (which is empty for JSON requests). Changes: - Fall back to JSON body for allow_bash and allow_web_search values - Only add bash/web_search to disabled_tools when explicitly set to a falsy value; when unset (None), defer to per-user privilege checks - Admins with can_use_bash=True now get bash enabled by default Fixes #3229 * fix: always send explicit allow_bash/allow_web_search from frontend The backend 'is not None' guard (from prior commit) is correct for API callers, but the frontend only sent allow_bash=true when the toggle was ON — omission meant 'unspecified' which the backend treated as 'don't disable'. Now the frontend always sends an explicit true/false value: - allow_bash: sent on every request (checked ? 'true' : 'false') - allow_web_search: explicit 'false' when toggle is off in agent mode With explicit frontend values, the 'is not None' guard is safe: - explicit true → tool enabled - explicit false → tool disabled - None (API caller omission) → defer to per-user privilege --------- Co-authored-by: michaelxer Co-authored-by: Alexandre Teixeira <111787685+alteixeira20@users.noreply.github.com> --- routes/chat_routes.py | 15 +- static/js/chat.js | 6 +- tests/test_chat_route_tool_policy.py | 251 +++++++++++++++++++++++---- 3 files changed, 228 insertions(+), 44 deletions(-) diff --git a/routes/chat_routes.py b/routes/chat_routes.py index f06ca4dc7..7ad635576 100644 --- a/routes/chat_routes.py +++ b/routes/chat_routes.py @@ -474,8 +474,11 @@ def setup_chat_routes( use_research = form_data.get("use_research") time_filter = form_data.get("time_filter") preset_id = form_data.get("preset_id") - allow_bash = form_data.get("allow_bash") - allow_web_search = form_data.get("allow_web_search") + # Issue #3229: API callers send JSON, not FormData. Read from the + # JSON body as fallback so callers who send {"allow_bash": true} + # actually get bash enabled. + allow_bash = form_data.get("allow_bash") or (body or {}).get("allow_bash") + allow_web_search = form_data.get("allow_web_search") or (body or {}).get("allow_web_search") use_rag = form_data.get("use_rag") search_context = form_data.get("search_context") # pre-fetched web search results (compare mode) compare_mode = str(form_data.get("compare_mode", "")).lower() == "true" @@ -687,9 +690,13 @@ def setup_chat_routes( # Build disabled-tools set from frontend toggles + user privileges disabled_tools = set() - if str(allow_bash).lower() != "true": + # Only disable bash/web_search when the caller *explicitly* set them + # to a falsy value. When unset (None), defer to per-user privilege + # checks below — this lets admins with can_use_bash=True use bash + # by default without having to send allow_bash in every request. + if allow_bash is not None and str(allow_bash).lower() != "true": disabled_tools.add("bash") - if str(allow_web_search).lower() != "true": + if allow_web_search is not None and str(allow_web_search).lower() != "true": disabled_tools.add("web_search") disabled_tools.add("web_fetch") diff --git a/static/js/chat.js b/static/js/chat.js index 434976c65..a9d89cc64 100644 --- a/static/js/chat.js +++ b/static/js/chat.js @@ -802,15 +802,15 @@ import { wireArrowUpRecall, getLastUserMessageFromChatHistory } from './composer } else { fd.append('use_web', 'true'); } + } else if (isAgentMode) { + fd.append('allow_web_search', 'false'); } if (el('research-toggle').checked) { fd.append('use_research', 'true'); // Research always runs in chat mode — override agent if set fd.set('mode', 'chat'); } - if (el('bash-toggle').checked) { - fd.append('allow_bash', 'true'); - } + fd.append('allow_bash', el('bash-toggle').checked ? 'true' : 'false'); const ragChk = el('rag-toggle'); if (ragChk && !ragChk.checked) { fd.append('use_rag', 'false'); diff --git a/tests/test_chat_route_tool_policy.py b/tests/test_chat_route_tool_policy.py index d1f155650..21fb78616 100644 --- a/tests/test_chat_route_tool_policy.py +++ b/tests/test_chat_route_tool_policy.py @@ -1,50 +1,227 @@ +"""Issue #3229 — allow_bash / allow_web_search must work for JSON API callers +and admin users must get bash enabled by default. + +Bug: allow_bash and allow_web_search were only read from form_data, so JSON +API callers (Content-Type: application/json) always had bash disabled. + +Fix: (1) Read from JSON body as fallback. + (2) Only add bash/web_search to disabled_tools when explicitly set to a + falsy value; when unset (None), defer to per-user privilege checks. +""" + +import ast from pathlib import Path +import pytest -CHAT_ROUTES = Path(__file__).resolve().parents[1] / "routes" / "chat_routes.py" +_CHAT_ROUTES = Path(__file__).resolve().parent.parent / "routes" / "chat_routes.py" -def _source() -> str: - return CHAT_ROUTES.read_text(encoding="utf-8") +# ── Source-level guards ───────────────────────────────────────── -def test_research_fast_path_respects_tool_policy(): - src = _source() - assert "pre_context_tool_policy = build_effective_tool_policy(" in src - assert "allow_tool_preprocessing = not pre_context_tool_policy.block_all_tool_calls" in src - assert "allow_tool_preprocessing=allow_tool_preprocessing" in src - assert "research_blocked_by_policy = bool(" in src - assert 'tool_policy.blocks("trigger_research")' in src - assert 'tool_policy.blocks("manage_research")' in src - assert 'effective_do_research = bool(' in src - assert 'if effective_do_research:' in src - assert '"is_research": effective_do_research' in src - assert "_effective_mode = 'research' if effective_do_research else (chat_mode or 'chat')" in src - assert '_model_suffix = "Research" if effective_do_research else None' in src - assert "do_research=effective_do_research" in src +def test_allow_bash_reads_from_body_as_fallback(): + """chat_stream must read allow_bash from the JSON body, not just form_data.""" + source = _CHAT_ROUTES.read_text(encoding="utf-8") + tree = ast.parse(source) + + # Find the chat_stream function + chat_stream_func = None + for node in ast.walk(tree): + if isinstance(node, ast.AsyncFunctionDef) and node.name == "chat_stream": + chat_stream_func = node + break + assert chat_stream_func is not None, "chat_stream function not found" + + # Look for an assignment to allow_bash that references 'body' + found_body_fallback = False + for node in ast.walk(chat_stream_func): + if isinstance(node, ast.Assign): + for target in node.targets: + if isinstance(target, ast.Name) and target.id == "allow_bash": + # Check if 'body' appears in the value + src_segment = ast.get_source_segment(source, node) + if src_segment and "body" in src_segment: + found_body_fallback = True + assert found_body_fallback, ( + "allow_bash assignment in chat_stream must fall back to JSON body" + ) -def test_non_streaming_chat_path_uses_tool_policy_before_context_and_research(): - src = _source() - chat_endpoint = src[src.index("async def chat_endpoint"):src.index("# ------------------------------------------------------------------ #", src.index("async def chat_endpoint"))] - assert "tool_policy = build_effective_tool_policy(last_user_message=message)" in chat_endpoint - assert "allow_tool_preprocessing = not tool_policy.block_all_tool_calls" in chat_endpoint - assert 'if not tool_policy.blocks("manage_memory"):' in chat_endpoint - assert "allow_tool_preprocessing=allow_tool_preprocessing" in chat_endpoint - assert 'tool_policy.blocks("trigger_research")' in chat_endpoint - assert "if use_research and not research_blocked_by_policy:" in chat_endpoint - assert "allow_background_extraction=not tool_policy.block_all_tool_calls" in chat_endpoint +def test_allow_web_search_reads_from_body_as_fallback(): + """chat_stream must read allow_web_search from the JSON body, not just form_data.""" + source = _CHAT_ROUTES.read_text(encoding="utf-8") + tree = ast.parse(source) + + chat_stream_func = None + for node in ast.walk(tree): + if isinstance(node, ast.AsyncFunctionDef) and node.name == "chat_stream": + chat_stream_func = node + break + assert chat_stream_func is not None + + found_body_fallback = False + for node in ast.walk(chat_stream_func): + if isinstance(node, ast.Assign): + for target in node.targets: + if isinstance(target, ast.Name) and target.id == "allow_web_search": + src_segment = ast.get_source_segment(source, node) + if src_segment and "body" in src_segment: + found_body_fallback = True + assert found_body_fallback, ( + "allow_web_search assignment in chat_stream must fall back to JSON body" + ) -def test_image_generation_fast_path_checks_policy_before_tool_start(): - src = _source() - policy_gate = src.index('if tool_policy.blocks("generate_image"):') - tool_start = src.index('"type": "tool_start", "tool": "generate_image"') - generator_call = src.index("do_generate_image(") - assert policy_gate < tool_start - assert policy_gate < generator_call +def test_disabled_tools_does_not_bash_when_allow_bash_is_none(): + """When allow_bash is not set (None), bash must NOT be unconditionally + added to disabled_tools. The per-user privilege check handles it. + """ + source = _CHAT_ROUTES.read_text(encoding="utf-8") + + # The fix changes: + # if str(allow_bash).lower() != "true": + # to: + # if allow_bash is not None and str(allow_bash).lower() != "true": + assert "allow_bash is not None" in source, ( + "disabled_tools check must guard against allow_bash being None" + ) + assert "allow_web_search is not None" in source, ( + "disabled_tools check must guard against allow_web_search being None" + ) -def test_streaming_chat_paths_disable_background_extraction_under_policy(): - src = _source() - assert src.count("allow_background_extraction=not tool_policy.block_all_tool_calls") >= 3 +# ── Functional tests of the disabled-tools logic ─────────────── + + +def _build_disabled_tools( + allow_bash=None, + allow_web_search=None, + can_use_bash=True, + can_use_browser=True, +): + """Replicate the disabled-tools logic from chat_stream for unit testing. + + Returns the set of tool names that would be disabled. + """ + disabled_tools = set() + + # Issue #3229 fix: only disable when explicitly set to a falsy value. + if allow_bash is not None and str(allow_bash).lower() != "true": + disabled_tools.add("bash") + if allow_web_search is not None and str(allow_web_search).lower() != "true": + disabled_tools.add("web_search") + disabled_tools.add("web_fetch") + + # Enforce per-user privileges + if not can_use_bash: + disabled_tools.update({"bash", "python", "read_file", "write_file"}) + if not can_use_browser: + disabled_tools.add("builtin_browser") + + return disabled_tools + + +def test_json_body_allow_bash_true_enables_bash(): + """API caller sending {"allow_bash": true} gets bash enabled.""" + disabled = _build_disabled_tools(allow_bash="true") + assert "bash" not in disabled + + +def test_json_body_allow_bash_false_disables_bash(): + """API caller sending {"allow_bash": false} gets bash disabled.""" + disabled = _build_disabled_tools(allow_bash="false") + assert "bash" in disabled + + +def test_json_body_allow_web_search_true_enables_web(): + """API caller sending {"allow_web_search": true} gets web tools enabled.""" + disabled = _build_disabled_tools(allow_web_search="true") + assert "web_search" not in disabled + assert "web_fetch" not in disabled + + +def test_json_body_allow_web_search_false_disables_web(): + """API caller sending {"allow_web_search": false} gets web tools disabled.""" + disabled = _build_disabled_tools(allow_web_search="false") + assert "web_search" in disabled + assert "web_fetch" in disabled + + +def test_admin_user_gets_bash_enabled_by_default(): + """When allow_bash is not set and user has can_use_bash privilege, + bash must NOT be disabled. + """ + disabled = _build_disabled_tools(allow_bash=None, can_use_bash=True) + assert "bash" not in disabled + + +def test_admin_user_gets_web_search_enabled_by_default(): + """When allow_web_search is not set and user has normal privileges, + web_search must NOT be disabled. + """ + disabled = _build_disabled_tools(allow_web_search=None) + assert "web_search" not in disabled + assert "web_fetch" not in disabled + + +def test_non_privileged_user_without_explicit_flag_still_disabled(): + """A user without can_use_bash privilege who doesn't send allow_bash + should still have bash disabled via the privilege check. + """ + disabled = _build_disabled_tools(allow_bash=None, can_use_bash=False) + assert "bash" in disabled + + +def test_non_privileged_user_explicit_true_overridden_by_privilege(): + """Even if allow_bash=true is sent, a user without can_use_bash + privilege still gets bash disabled by the privilege gate. + """ + disabled = _build_disabled_tools(allow_bash="true", can_use_bash=False) + assert "bash" in disabled + + +def test_form_data_none_body_true_works(): + """Simulates: form_data has no allow_bash, body has allow_bash=true. + After the fallback (`form_data.get(...) or body.get(...)`), allow_bash + should be "true". + """ + # Simulate the fallback logic + form_data_val = None # not in form_data + body_val = "true" # from JSON body + allow_bash = form_data_val or body_val + assert str(allow_bash).lower() == "true" + + disabled = _build_disabled_tools(allow_bash=allow_bash) + assert "bash" not in disabled + + +def test_explicit_false_disables_even_for_admin(): + """An admin who explicitly sends allow_bash=false should have bash disabled.""" + disabled = _build_disabled_tools( + allow_bash="false", can_use_bash=True, + ) + assert "bash" in disabled + + +# ── Frontend source-level guards ────────────────────────────── + +_CHAT_JS = Path(__file__).resolve().parent.parent / "static" / "js" / "chat.js" + + +def test_frontend_always_sends_explicit_allow_bash(): + """chat.js must always send allow_bash (both true and false), not only on toggle ON.""" + source = _CHAT_JS.read_text(encoding="utf-8") + # Must not only append 'true' — must also handle the false case + assert "allow_bash', el('bash-toggle').checked ? 'true' : 'false'" in source or \ + "allow_bash', 'false'" in source, ( + "Frontend must send explicit allow_bash=false when toggle is off" + ) + + +def test_frontend_sends_explicit_allow_web_search_false_in_agent_mode(): + """chat.js must send allow_web_search=false when web toggle is off in agent mode.""" + source = _CHAT_JS.read_text(encoding="utf-8") + assert "allow_web_search', 'false'" in source, ( + "Frontend must send explicit allow_web_search=false in agent mode when toggle is off" + )