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" + )