mirror of
https://github.com/pewdiepie-archdaemon/odysseus.git
synced 2026-06-27 23:25:22 -04:00
fc1351d0f8
* test(tools): add shim protection test for tool_implementations split Covers all 48 top-level functions (33 do_* + 15 _helpers) extracted from the original module. Guards the upcoming split: the shim must re-export every symbol so existing 'from src.tool_implementations import X' imports keep working. Passes on baseline (pre-split). * refactor(tools): add src/tools/ package with shared _common Slice 1 Task 2 (#4082/#4071). Adds the package skeleton and moves the shared _parse_tool_args helper into src/tools/_common.py. Domain modules will import from here. tool_implementations.py is untouched at this step. * refactor(tools): extract system domain into src/tools/system.py Slice 1 (#4082/#4071), Task 3: move the system-domain tool functions (do_manage_skills/_skill_dump/do_manage_tasks/do_manage_endpoints/ do_manage_mcp/do_manage_webhooks/do_manage_tokens/do_manage_settings/ do_api_call/do_app_api) and the app_api blocklist constants out of tool_implementations.py into a new src/tools/system.py module. tool_implementations.py re-imports all of them so it stays a working backward-compatible facade (shim test stays green). - do_manage_mcp resolves get_mcp_manager via a function-local import from tool_implementations so the test that patches src.tool_implementations.get_mcp_manager still applies post-move. - do_app_api imports _internal_headers and _INTERNAL_BASE (still in tool_implementations) function-locally to avoid a circular import. - Repoint test_context_budget introspection assertion to the moved code's new home in src/tools/system.py. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(tools): extract cookbook domain into src/tools/cookbook.py Moves the model-serving (cookbook) tool domain out of tool_implementations.py into src/tools/cookbook.py as part of slice 1 (#4082/#4071): - 13 do_* tools: download/serve/list/stop/tail/search/adopt/cached models, list downloads/cancel, list cookbook servers, serve presets - 9 private helpers: _cookbook_servers, _resolve_cookbook_host, _cookbook_env_for_host, _infer_serve_{port,host}, _ensure_served_endpoint, _cookbook_register_task, _cookbook_apply_retry_suggestion, _scan_running_model_processes, _cookbook_kill_session - _MODEL_PROCESS_PATTERNS constant (used only by _scan_running_model_processes) tool_implementations.py stays a backward-compatible facade via a re-import from src.tools.cookbook; src/tools/__init__ re-exports the same symbols. _internal_headers and _INTERNAL_BASE stay in tool_implementations.py (shared by system.py's do_app_api and many cookbook funcs). Each cookbook function that needs them does a function-local import to avoid a top-level circular dependency, matching the system-domain split. Verified: compileall clean; shim test green; cookbook-touching suite (652 passed, 1 skipped); full suite 3587 passed, 2 failed (pre-existing test_api_chat_security, unrelated). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(tools): extract search domain into src/tools/search.py * refactor(tools): extract notes domain into src/tools/notes.py * refactor(tools): extract calendar domain into src/tools/calendar.py Repoints tests/test_caldav_bidirectional_sync.py source-introspection to src/tools/calendar.py (do_manage_calendar moved there). * refactor(tools): extract image domain into src/tools/image.py * refactor(tools): extract research domain into src/tools/research.py * refactor(tools): extract contacts domain into src/tools/contacts.py * refactor(tools): extract vault domain into src/tools/vault.py Repoints tests/test_vault_password_not_in_argv.py source-introspection to src/tools/vault.py (the vault do_* helpers moved there). * refactor(tools): collapse tool_implementations to clean re-export shim Move shared _INTERNAL_BASE/_internal_headers to src/tools/_common.py and drop the duplicate _parse_tool_args (already in _common). tool_implementations.py is now a pure re-export facade (+ 3 pre-existing email-context helpers, out of scope). Domain files' function-local imports of these names still resolve via the facade re-export. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(tools): port upstream cookbook workflow changes to split module Rebase onto dev droppedc504214("Cookbook model workflow fixes") edits to do_serve_model / do_tail_serve_output: the extraction commit moved the pre-edit bodies into src/tools/cookbook.py and git auto-accepted the deletion from tool_implementations.py, losing dev's changes. Restore them in their post-split home: - do_serve_model: add where/log_path/next_tools and the expanded "Next required check" output message - do_tail_serve_output: empty-output fallback message replacing "(empty pane)" (do_manage_settings web_fetch alias edit was already applied to src/tools/system.py during the system-extract conflict resolution.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(tools): break admin_tools circular import in split facade After rebasing onto dev (#3629 moved the admin manage_* tools into src/agent_tools/admin_tools), the facade re-exported them via a top-level `from src.agent_tools.admin_tools import ...`. But src.agent_tools.__init__ imports this facade at top level, so the eager import re-entered the partially-initialized agent_tools package and broke collection. Re-export the admin symbols (do_manage_endpoints/mcp/webhooks/tokens/ settings, _MCP_DENIED_COMMANDS, _validate_mcp_command) lazily through module __getattr__ instead, and drop them from src/tools/__init__ (they no longer live in the src.tools package). system.py now holds only the skills/tasks/api bridges; admin tools live solely in admin_tools.py, matching upstream. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(tools): re-export dropped helpers through the split shim Address review finding from #4423: the compatibility facade claimed to preserve every original top-level symbol but omitted three helpers the old src.tool_implementations exposed. Re-export them and pin them in the shim protection test: - _string_arg, _validate_cookbook_ssh_target <- src/tools/cookbook.py - _mcp_allowed_commands <- src/agent_tools/admin_tools.py (lazily via __getattr__, to keep the agent_tools.__init__ <-> facade import acyclic after the #3629 admin-tools migration) All three added to tests/test_tool_implementations_shim.py _EXPECTED so the test contract now matches its "every original top-level function" comment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(tools): self-verify shim re-exports every domain do_* The hand-maintained _EXPECTED list in the shim protection test can drift silently when a new tool is added to a domain module but not re-exported by the facade — exactly the omission a reviewer flagged post-split. Add an auto-discovering test that enumerates every do_* from the domain modules (incl. admin_tools) and asserts reachability through the shim, so a forgotten re-export fails the build automatically. Uses hasattr (not dir(ti)) because the admin symbols are re-exported lazily via module __getattr__ and don't appear in dir(ti). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(tools): self-verify every in-repo facade import resolves RaresKeY's P3 on the shim test was a claim-vs-reality gap: the docstring said it protected "every from src.tool_implementations import X" but the hand-maintained _EXPECTED list omitted three underscore helpers, so the claim wasn't enforced. Re-exporting the three (cf1f5e3) fixed the known gap; this closes the structural one. Add test_every_facade_import_in_repo_resolves: ast-enumerate every `from src.tool_implementations import X` site in src/ and tests/ and assert hasattr(ti, X) for each. A forgotten re-export that anything in the repo imports now fails the build automatically — including underscore helpers, which the do_* discovery test does not cover. Together with test_shim_reexports_every_domain_do_function, the shim contract is now self-verifying. Demote _EXPECTED in the docstring to the curated historical/downstream surface (the three helpers have no in-repo consumer, so they stay manual by necessity) instead of "ground truth". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(tools): dedupe _parse_tool_args + align shim guard with route consumers Addresses two P3s from review (RaresKeY, 2026-06-26): 1. maintainability — _common carried a full copy of _parse_tool_args alongside the canonical src.tool_utils one; future parser fixes could diverge. The two bodies were byte-identical in logic, so _common now re-exports from tool_utils (a leaf module, no circular-import risk). The single-source test is extended to assert _common._parse_tool_args and tool_implementations._parse_tool_args are the same object as tool_utils._parse_tool_args. 2. test — the shim guard's import-site scan only walked src/ and tests/, missing routes/chat_routes.py's clear_active_email/set_active_email imports, and _EXPECTED omitted the active-email facade helpers. The scan now walks every first-party Python dir (pruning venvs/caches/data in-place), and set/get/clear_active_email are added to _EXPECTED (get_active_email has no in-repo importer, so the scan alone can't see it). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: yuandonghao <yuandonghao@cohl.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
118 lines
4.4 KiB
Python
118 lines
4.4 KiB
Python
"""Pin the vault master-password handling so it never regresses into argv.
|
|
|
|
`routes.vault_routes._run_bw` launches the Bitwarden CLI with
|
|
``asyncio.create_subprocess_exec(bw_path, *args)`` — every element of ``args``
|
|
becomes a process argument, which is world-readable through ``ps`` /
|
|
``/proc/<pid>/cmdline``. The master password therefore must be handed to ``bw``
|
|
out-of-band (stdin or ``--passwordenv BW_PASSWORD``), and never as a positional
|
|
argv element.
|
|
|
|
The /unlock route previously did ``_run_bw(["unlock", req.master_password,
|
|
"--raw"])`` — leaking the Bitwarden master password (which decrypts the whole
|
|
vault) to any local user for the lifetime of the unlock subprocess.
|
|
"""
|
|
|
|
import os
|
|
import json
|
|
import re
|
|
import sys
|
|
import types
|
|
from unittest.mock import MagicMock
|
|
|
|
import pytest
|
|
|
|
sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
|
|
|
|
# Importing routes.vault_routes pulls in core.middleware → core/__init__ →
|
|
# session_manager, which explodes under the conftest stubs. Stub the heavy
|
|
# imports the module needs so we can reach the self-contained _run_bw helper.
|
|
if "core.database" not in sys.modules:
|
|
_db = types.ModuleType("core.database")
|
|
for _n in ("SessionLocal", "ChatMessage", "Session", "Document"):
|
|
setattr(_db, _n, MagicMock())
|
|
sys.modules["core.database"] = _db
|
|
if "core.middleware" not in sys.modules:
|
|
_mw = types.ModuleType("core.middleware")
|
|
_mw.require_admin = MagicMock()
|
|
sys.modules["core.middleware"] = _mw
|
|
if "core.platform_compat" not in sys.modules:
|
|
_pc = types.ModuleType("core.platform_compat")
|
|
_pc.IS_WINDOWS = False
|
|
_pc.safe_chmod = MagicMock()
|
|
_pc.which_tool = MagicMock(return_value="bw")
|
|
sys.modules["core.platform_compat"] = _pc
|
|
|
|
import routes.vault_routes as vr # noqa: E402
|
|
|
|
|
|
class _FakeProc:
|
|
def __init__(self, stdout=b"session-key", stderr=b"", rc=0):
|
|
self._out, self._err, self.returncode = stdout, stderr, rc
|
|
|
|
async def communicate(self, input=None):
|
|
return self._out, self._err
|
|
|
|
|
|
def _patch_exec(monkeypatch):
|
|
"""Capture the argv + env handed to create_subprocess_exec."""
|
|
captured = {}
|
|
|
|
async def _fake_exec(*argv, env=None, **kwargs):
|
|
captured["argv"] = list(argv)
|
|
captured["env"] = env or {}
|
|
return _FakeProc()
|
|
|
|
monkeypatch.setattr(vr, "_find_bw", lambda: "bw")
|
|
monkeypatch.setattr(vr.asyncio, "create_subprocess_exec", _fake_exec)
|
|
return captured
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_run_bw_passwordenv_does_not_put_password_in_argv(monkeypatch):
|
|
captured = _patch_exec(monkeypatch)
|
|
secret = "correct horse battery staple"
|
|
await vr._run_bw(["unlock", "--passwordenv", "BW_PASSWORD", "--raw"],
|
|
bw_password=secret)
|
|
# The secret must reach bw through the environment...
|
|
assert captured["env"].get("BW_PASSWORD") == secret
|
|
# ...and must NOT appear anywhere in the argv (which `ps` exposes).
|
|
assert secret not in captured["argv"]
|
|
assert all(secret not in str(a) for a in captured["argv"])
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_run_bw_without_password_does_not_set_env(monkeypatch):
|
|
captured = _patch_exec(monkeypatch)
|
|
await vr._run_bw(["lock"])
|
|
assert "BW_PASSWORD" not in captured["env"]
|
|
|
|
|
|
def test_unlock_handler_feeds_password_on_stdin_not_argv():
|
|
"""Source-level guard: the /unlock route must feed the master password via
|
|
stdin, never as a bare positional argv element."""
|
|
src = vr.__file__
|
|
with open(src, encoding="utf-8") as fh:
|
|
text = fh.read()
|
|
# The old, vulnerable call shape must be gone.
|
|
assert 'req.master_password, "--raw"' not in text
|
|
assert "[\"unlock\", req.master_password" not in text
|
|
# And the safer stdin shape must be present.
|
|
assert "[\"unlock\", \"--raw\"]" in text
|
|
assert re.search(r'input_text\s*=\s*req\.master_password\s*\+\s*"\\n"', text)
|
|
|
|
|
|
def test_tool_vault_unlock_feeds_password_on_stdin_not_argv():
|
|
text = open("src/tools/vault.py", encoding="utf-8").read()
|
|
|
|
assert '["unlock", master_password, "--raw"]' not in text
|
|
assert '_run_bw(["unlock", master_password' not in text
|
|
assert re.search(r'input_text\s*=\s*master_password\s*\+\s*"\\n"', text)
|
|
|
|
|
|
def test_load_config_ignores_non_object_json(tmp_path, monkeypatch):
|
|
vault_file = tmp_path / "vault.json"
|
|
vault_file.write_text(json.dumps(["not", "a", "config", "object"]), encoding="utf-8")
|
|
monkeypatch.setattr(vr, "VAULT_FILE", vault_file)
|
|
|
|
assert vr._load_config() == {}
|