fix(child-env): openviking drops the profile overlay's bot tokens; unprovable manifests don't block spawns
Phase 2 review follow-ups: - OpenViking overlaid the bound profile's whole .env after the scrub, so that profile's bot, dashboard and relay tokens reached the server again. The overlaid env is now scrubbed a second time for Tier 1. Provider keys still pass, and they still come from the bound profile. - The strict per-home manifest read raised on any unreadable entry under <home>/plugins, which failed every child spawn for that profile. It is now strict only where the manifest is known to be a platform's: bundled or plugins/platforms/. Dunder and dot directories, unsearchable plugin directories and unreadable flat plugins/* manifests are skipped with a warning, because they can't load as plugins either. - The per-home cache is keyed on hermes_home_key(), so a symlinked alias no longer gets a second entry. - The modal snapshot test swapped hermes_cli.config for a stub, which the policy import can no longer use. HERMES_HOME already points the real module at the test home, so the stub is gone. - The env_passthrough docstring now describes declared names rather than the dropped prefix rule.
This commit is contained in:
+23
-7
@@ -4005,8 +4005,14 @@ def _platform_manifest_paths(home: Optional[Path] = None, source: str = "all"):
|
||||
if not root.is_dir():
|
||||
continue
|
||||
for child in root.iterdir():
|
||||
manifest_path = next(
|
||||
(p for p in (child / "plugin.yaml", child / "plugin.yml") if child.is_dir() and p.exists()), None)
|
||||
if child.name.startswith((".", "__")):
|
||||
continue # __pycache__, .git, editor state: never a plugin
|
||||
try:
|
||||
manifest_path = next(
|
||||
(p for p in (child / "plugin.yaml", child / "plugin.yml") if child.is_dir() and p.exists()), None)
|
||||
except OSError as exc: # an unsearchable dir cannot be loaded as a plugin either
|
||||
logger.warning("Skipping unreadable plugin directory %s: %s", child, exc)
|
||||
continue
|
||||
if manifest_path is not None:
|
||||
yield child.name, manifest_path, require_kind
|
||||
|
||||
@@ -4014,21 +4020,31 @@ def _platform_manifest_paths(home: Optional[Path] = None, source: str = "all"):
|
||||
def platform_manifest_stamp(home: Optional[Path] = None, source: str = "user") -> tuple:
|
||||
"""``(path, mtime_ns)`` of every platform plugin manifest file: changes whenever one is added,
|
||||
removed or edited in place, so a cache keyed on it never serves a stale declaration."""
|
||||
return tuple((str(path), path.stat().st_mtime_ns) for _dir, path, _kind in _platform_manifest_paths(home, source))
|
||||
stamp = []
|
||||
for _dir, path, _kind in _platform_manifest_paths(home, source):
|
||||
try:
|
||||
stamp.append((str(path), path.stat().st_mtime_ns))
|
||||
except OSError:
|
||||
stamp.append((str(path), None)) # the strict read reports it
|
||||
return tuple(stamp)
|
||||
|
||||
|
||||
def _platform_plugin_manifests(home: Optional[Path] = None, source: str = "all", *, strict: bool = False):
|
||||
"""Yield ``(dir_name, manifest_dict)`` for every platform plugin manifest (see
|
||||
:func:`_platform_manifest_paths`). ``strict`` raises when a manifest cannot be read instead of
|
||||
skipping it: the child-env scrub must not lose a declared secret to an I/O error. A manifest
|
||||
that does not parse declares nothing (its adapter cannot load either) and is skipped."""
|
||||
skipping it: the child-env scrub must not lose a declared secret to an I/O error. Only a
|
||||
manifest known to be a platform's counts (the bundled and ``plugins/platforms/`` dirs); a
|
||||
flat ``plugins/*`` manifest proves it is one only by its content, so an unreadable one is
|
||||
skipped with a warning. A manifest that does not parse declares nothing (its adapter cannot
|
||||
load either) and is skipped."""
|
||||
for dir_name, manifest_path, require_kind in _platform_manifest_paths(home, source):
|
||||
try:
|
||||
with open(manifest_path, "r", encoding="utf-8-sig") as f:
|
||||
manifest = fast_safe_load(f) or {}
|
||||
except OSError:
|
||||
if strict:
|
||||
except OSError as exc:
|
||||
if strict and not require_kind:
|
||||
raise
|
||||
logger.warning("Skipping unreadable plugin manifest %s: %s", manifest_path, exc)
|
||||
continue
|
||||
except Exception:
|
||||
continue
|
||||
|
||||
@@ -983,8 +983,11 @@ def _start_local_openviking_server(endpoint: str) -> tuple[str, str]:
|
||||
# (never the launch profile's: under multiplex the process env belongs to whoever started
|
||||
# the gateway, and with no bound profile the builder refuses); bot, gateway and relay
|
||||
# tokens never do. HOME stays the user's: ov.conf defaults to ~/.openviking.
|
||||
from tools.environments.local import served_profile_child_env
|
||||
child_env = served_profile_child_env(inherit_credentials=True)
|
||||
from tools.environments.local import hermes_subprocess_env, served_profile_child_env
|
||||
# The profile overlay re-adds everything in its .env, bot tokens included; the second pass
|
||||
# drops Tier 1 again while keeping the provider keys.
|
||||
child_env = hermes_subprocess_env(
|
||||
inherit_credentials=True, base_env=served_profile_child_env(inherit_credentials=True))
|
||||
child_env["HOME"] = child_env["HERMES_REAL_HOME"]
|
||||
child_env.pop("PYTHONPATH", None)
|
||||
with log_path.open("ab") as log_file:
|
||||
|
||||
@@ -191,6 +191,19 @@ def test_unreadable_platform_manifest_fails_closed(tmp_path):
|
||||
platform_manifest_secret_envs(tmp_path, strict=True)
|
||||
finally:
|
||||
manifest.chmod(0o644)
|
||||
# Not provably a platform's, so no reason to fail: an unsearchable plugin dir (a root-owned
|
||||
# __pycache__ or plugin in a volume) or an unreadable flat plugins/* manifest.
|
||||
locked = [tmp_path / "plugins" / "__pycache__", tmp_path / "plugins" / "memx"]
|
||||
for d in locked:
|
||||
d.mkdir()
|
||||
(locked[1] / "plugin.yaml").write_text("kind: memory\n", encoding="utf-8")
|
||||
locked[0].chmod(0)
|
||||
(locked[1] / "plugin.yaml").chmod(0)
|
||||
try:
|
||||
assert platform_manifest_secret_envs(tmp_path, strict=True) == {"CHATX_SIGNING_SECRET"}
|
||||
finally:
|
||||
locked[0].chmod(0o755)
|
||||
(locked[1] / "plugin.yaml").chmod(0o644)
|
||||
|
||||
|
||||
def test_inheriting_child_gets_provider_keys_but_never_adapter_secrets(child_env, monkeypatch):
|
||||
|
||||
@@ -64,9 +64,6 @@ def _install_modal_test_modules(
|
||||
sys.modules["hermes_cli"] = hermes_cli
|
||||
hermes_home = tmp_path / "hermes-home"
|
||||
os.environ["HERMES_HOME"] = str(hermes_home)
|
||||
sys.modules["hermes_cli.config"] = types.SimpleNamespace(
|
||||
get_hermes_home=lambda: hermes_home,
|
||||
)
|
||||
|
||||
tools_package = types.ModuleType("tools")
|
||||
tools_package.__path__ = [str(TOOLS_DIR)] # type: ignore[attr-defined]
|
||||
|
||||
@@ -87,6 +87,25 @@ def test_openviking_server_keeps_provider_keys_but_never_tier1_secrets(child_env
|
||||
_PROVIDER: "fake-openai_api_key", **own}
|
||||
|
||||
|
||||
def test_openviking_server_gets_the_bound_profiles_provider_keys_not_its_bot_tokens(child_env, monkeypatch):
|
||||
# A routed profile's own .env is overlaid for its provider keys; its bot and dashboard
|
||||
# secrets must not ride along, and the launch profile's keys must not either.
|
||||
from hermes_constants import reset_hermes_home_override, set_hermes_home_override
|
||||
_plant(monkeypatch)
|
||||
routed = child_env / "profiles" / "b"
|
||||
routed.mkdir(parents=True)
|
||||
(routed / ".env").write_text(
|
||||
"TELEGRAM_BOT_TOKEN=b-bot\nHERMES_DASHBOARD_BASIC_AUTH_PASSWORD=b-dash\nOPENAI_API_KEY=b-openai\n",
|
||||
encoding="utf-8")
|
||||
token = set_hermes_home_override(routed)
|
||||
try:
|
||||
seen = _openviking_server_seen(
|
||||
child_env, monkeypatch, ["TELEGRAM_BOT_TOKEN", "HERMES_DASHBOARD_BASIC_AUTH_PASSWORD", _PROVIDER])
|
||||
finally:
|
||||
reset_hermes_home_override(token)
|
||||
assert seen == {"TELEGRAM_BOT_TOKEN": None, "HERMES_DASHBOARD_BASIC_AUTH_PASSWORD": None, _PROVIDER: "b-openai"}
|
||||
|
||||
|
||||
@pytest.mark.platforms("posix") # the stand-in binaries are shebang scripts
|
||||
@pytest.mark.parametrize("site", ["lsp_server", "lsp_go_install", "lsp_npm_install", "raft_bridge", "buzz_cli"])
|
||||
def test_third_party_children_never_see_hermes_credentials(child_env, monkeypatch, site):
|
||||
|
||||
@@ -117,8 +117,8 @@ def _load_config_passthrough() -> frozenset[str]:
|
||||
|
||||
def is_env_passthrough(var_name: str) -> bool:
|
||||
"""True if *var_name* was registered by a skill or listed in config and is not a
|
||||
Hermes-managed credential NOW. Ownership changes after acceptance (a plugin adapter
|
||||
registering later claims ``<PREFIX>_*_SECRET``), so the refusal applied at registration
|
||||
Hermes-managed credential NOW. Ownership changes after acceptance (a platform plugin
|
||||
registered later declares the name in its ``required_env`` or manifest), so the refusal applied at registration
|
||||
is re-applied here, where every child builder consumes the allowlist."""
|
||||
return ((var_name in _get_allowed() or var_name in _load_config_passthrough())
|
||||
and not _is_hermes_provider_credential(var_name))
|
||||
|
||||
@@ -128,13 +128,13 @@ def _home_adapter_secret_env() -> frozenset:
|
||||
mtime (an in-place edit invalidates it); an unreadable manifest raises instead of silently
|
||||
dropping the declaration."""
|
||||
from hermes_cli.config import platform_manifest_secret_envs, platform_manifest_stamp
|
||||
from hermes_constants import get_hermes_home
|
||||
from hermes_constants import get_hermes_home, hermes_home_key
|
||||
home = get_hermes_home()
|
||||
stamp = platform_manifest_stamp(home)
|
||||
cached = _HOME_ADAPTER_SECRET_CACHE.get(str(home))
|
||||
key, stamp = hermes_home_key(home), platform_manifest_stamp(home)
|
||||
cached = _HOME_ADAPTER_SECRET_CACHE.get(key)
|
||||
if cached is None or cached[0] != stamp:
|
||||
cached = (stamp, platform_manifest_secret_envs(home, strict=True) - _ADAPTER_SECRET_ENV)
|
||||
_HOME_ADAPTER_SECRET_CACHE[str(home)] = cached
|
||||
_HOME_ADAPTER_SECRET_CACHE[key] = cached
|
||||
return cached[1]
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user