Files
odysseus/tests/test_vault_password_not_in_argv.py
T
Tal.Yuan fc1351d0f8 refactor(tools): split tool_implementations.py into src/tools/ package (#4423)
* 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 dropped c504214 ("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>
2026-06-26 15:40:04 +01:00

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() == {}