From ded478f82f4407ce012b8281ca9c3e24cfc51e62 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sun, 25 Oct 2026 03:57:57 -0700 Subject: [PATCH] fix(vault): judge a login backend's name without running plugin code The shared registrar read provider.name before the login-backend registry's static check, so a name property (or a __getattribute__ hook) ran plugin code there and its exception text reached the loader log. The login-backend row now runs agent.vault_backend_registry.static_name first: it inspects the class and instance dicts only, refuses attribute-access overrides and non-plain names, and logs a fixed refusal reason. --- agent/vault_backend_registry.py | 24 +++++++++--- hermes_cli/plugins_registrars.py | 14 +++++-- .../test_browser_vault_plugin_isolation.py | 37 +++++++++++++++++++ .../developer-guide/browser-vault-plugin.md | 2 +- 4 files changed, 68 insertions(+), 9 deletions(-) diff --git a/agent/vault_backend_registry.py b/agent/vault_backend_registry.py index 1942e55ed3..abeaeaf3d8 100644 --- a/agent/vault_backend_registry.py +++ b/agent/vault_backend_registry.py @@ -3,8 +3,9 @@ Registration (``ctx.register_login_backend``) is refused for: a name that is not a lowercase identifier, a handle prefix other than ``:``, a name or prefix in a built-in backend's namespace, a name another plugin already holds in this profile, a backend that wants Hermes' master-password -prompt (``needs_unlock``), and ``name``/``prefix`` computed by a descriptor (they could change after -validation). ``enabled_plugin_backends`` re-checks every stored key and wraps each backend in +prompt (``needs_unlock``), and ``name``/``prefix`` computed by a descriptor or an attribute-access hook +(they could change after validation, and reading them would run plugin code; :func:`static_name` judges +them before anything reads them). ``enabled_plugin_backends`` re-checks every stored key and wraps each backend in :class:`agent.vault_backends.plugin.PluginLoginBackend`, the only form the vault tool ever sees. """ @@ -41,17 +42,30 @@ def _namespace_error(name) -> str: return "" -def register_provider(provider, *, scope=None): - if not isinstance(provider, LoginBackend): +def static_name(provider) -> str: + """The backend's name, judged without running plugin code (``ValueError`` with a fixed message when refused). + + Registration reads ``provider.name``; a property, a ``__getattribute__`` hook or a ``__class__`` override + would run the plugin's code there, outside any guard, and its exception text would reach the loader log. + """ + cls = type(provider) + if not issubclass(cls, LoginBackend): raise ValueError("login backend must subclass LoginBackend") + if cls.__getattribute__ is not object.__getattribute__ or any("__class__" in vars(k) for k in cls.__mro__[:-1]): + raise ValueError("login backend must not override attribute access") name, prefix = inspect.getattr_static(provider, "name", None), inspect.getattr_static(provider, "prefix", None) - if not isinstance(name, str) or not isinstance(prefix, str): + if type(name) is not str or type(prefix) is not str: raise ValueError("login backend name and prefix must be plain string attributes") error = _namespace_error(name) or ("" if prefix == f"{name}:" else "login backend prefix must be ':'") if error: raise ValueError(error) if inspect.getattr_static(provider, "needs_unlock", False) is not False: raise ValueError("plugin login backends cannot use the Hermes master-password prompt (needs_unlock)") + return name + + +def register_provider(provider, *, scope=None): + name = static_name(provider) if name in _registry.merged(scope): raise ValueError(f"login backend name {name!r} is already registered") _registry.register(provider, scope=scope) diff --git a/hermes_cli/plugins_registrars.py b/hermes_cli/plugins_registrars.py index 0bb1bb5d99..7323e42a0e 100644 --- a/hermes_cli/plugins_registrars.py +++ b/hermes_cli/plugins_registrars.py @@ -6,7 +6,9 @@ the displaced entry. ``hermes_cli.plugins`` generates one ctx method per row. Ro registry module, base-class module:attr, label, docstring, options). ``normalize``: ``strip`` (default), ``lower`` (strip+lowercase) or ``None`` (raw name). ``admit``: ``module:function`` taking the plugin's manifest and returning why it may not register this kind ("" = it may), judged under the -manager's home before the registry is touched. Object-taking rows also need a +manager's home before the registry is touched. ``identity``: a registry function judging the provider's +name without running its code, before the shared body reads ``provider.name`` (``ValueError`` = refused, +logged with ``reject_message``). Object-taking rows also need a ``plugin_isolation.HOST_OBJECT_BASES`` entry so the provider works under ``plugins.isolation: host``. """ @@ -28,7 +30,7 @@ _SCOPED_PROVIDER_REGISTRARS: tuple[tuple[str, str, str, str, str, str, dict[str, "Backend instances must read active-profile configuration lazily. Accepted only from bundled " "plugins and catalog installs at tier verified or official.", {"normalize": None, "reject_message": "Plugin '%s' login backend rejected: %s", - "admit": "hermes_cli.plugins_cmd_catalog:login_backend_provenance_error"}), + "admit": "hermes_cli.plugins_cmd_catalog:login_backend_provenance_error", "identity": "static_name"}), ("register_image_gen_provider", "image_gen_provider", "agent.image_gen_registry", "agent.image_gen_provider:ImageGenProvider", "image_gen provider", "Register an :class:`agent.image_gen_provider.ImageGenProvider`; " @@ -95,7 +97,7 @@ def make_scoped_provider_registrar(method_name, kind, registry_mod, base_ref, la """Build one ``register__provider`` method from a ``_SCOPED_PROVIDER_REGISTRARS`` row.""" normalize_fn = _NAME_NORMALIZERS[options.get("normalize", "strip")] register_name = options.get("register") - admit = options.get("admit") + admit, identity = options.get("admit"), options.get("identity") def register(self, provider): if admit: @@ -105,6 +107,12 @@ def make_scoped_provider_registrar(method_name, kind, registry_mod, base_ref, la logger.warning(options["reject_message"], self.manifest.name, refusal) return None registry = importlib.import_module(registry_mod) + if identity: + try: + getattr(registry, identity)(provider) + except ValueError as exc: + logger.warning(options["reject_message"], self.manifest.name, exc) + return None return self._register_scoped_provider( provider, kind=kind, base_class=_resolve(base_ref), registry=registry, label=label, article=options.get("article", "a"), normalize=normalize_fn, diff --git a/tests/tools/test_browser_vault_plugin_isolation.py b/tests/tools/test_browser_vault_plugin_isolation.py index 59ad4bfed8..a736a075d9 100644 --- a/tests/tools/test_browser_vault_plugin_isolation.py +++ b/tests/tools/test_browser_vault_plugin_isolation.py @@ -465,3 +465,40 @@ def test_errors_reading_plugin_metadata_get_the_fixed_message(vault_home, page): assert json.loads(listed)["errors"] == [{"backend": "lazy", "error": "The lazy login backend failed."}] assert json.loads(filled)["error_type"] == "backend_unavailable" and page.secret_evals == [] + +_COMPUTED_NAME_BACKENDS = ''' +from agent.vault_backends.base import LoginBackend + +class Base(LoginBackend): + prefix, display_name = "computed:", "Computed" + def list_items(self): + return [] + def get_meta(self, handle): + return None + def resolve_password(self, handle): + return "unused" + +class PropertyName(Base): + @property + def name(self): + raise RuntimeError("manager said {leak}") + +class HookedName(Base): + name = "computed" + def __getattribute__(self, field): + if field == "name": + raise RuntimeError("manager said {leak}") + return super().__getattribute__(field) +''' + + +def test_registration_judges_the_name_without_running_plugin_code(vault_home, caplog): + home, _local, load = vault_home + _write_plugin(home, "computed_vault", _COMPUTED_NAME_BACKENDS.format(leak=LEAK), + " ctx.register_login_backend(PropertyName())\n ctx.register_login_backend(HookedName())\n") + caplog.set_level(logging.DEBUG) + load(["computed_vault"], {"computed": "true"}) + + assert _backend("computed") is None + assert LEAK not in caplog.text + assert caplog.text.count("login backend rejected") == 2 diff --git a/website/docs/developer-guide/browser-vault-plugin.md b/website/docs/developer-guide/browser-vault-plugin.md index e428c30c1c..eea47c78a8 100644 --- a/website/docs/developer-guide/browser-vault-plugin.md +++ b/website/docs/developer-guide/browser-vault-plugin.md @@ -83,7 +83,7 @@ hold however the plugin implements its class: | **Own namespace only** | Your handles are `:…`. Hermes decides ownership from the registered name, so it never calls your `owns()` and never passes you a `vault_…` (Hermes vault), `op:…`, `bw:…` or another plugin's handle. Items your `list_items()` returns with an id outside your namespace are dropped, and a `get_meta()` answer whose id is not the requested handle is ignored. | | **Handle in, value out** | `list_items()` gets no arguments; `get_meta()`, `resolve_password()`, `resolve_otp()` and `resolve_secret()` get your handle and nothing else. You never receive the page, the CDP/browser session, the task, other vault entries, or decrypted values from any other backend. | | **No master-password prompt** | Hermes' unlock prompt is reserved for built-in managers. A backend with `needs_unlock = True` is refused at registration, and a plugin backend is never offered the `unlock` action, so the user is never asked to type a master password into a dialog a plugin labelled. Authenticate through your manager's own agent, session or OS keychain. | -| **Reserved namespaces** | The names `local`, `onepassword`, `bitwarden`, `vault`, `op` and `bw`, and any prefix inside a built-in prefix (`vault_x:`), are refused. So is a name another plugin already holds in the profile: no backend, built-in or plugin, can be replaced or shadowed. `name` and `prefix` must be plain string attributes, not properties. | +| **Reserved namespaces** | The names `local`, `onepassword`, `bitwarden`, `vault`, `op` and `bw`, and any prefix inside a built-in prefix (`vault_x:`), are refused. So is a name another plugin already holds in the profile: no backend, built-in or plugin, can be replaced or shadowed. `name` and `prefix` must be plain string attributes, not properties, and the class may not override `__getattribute__` or `__class__`: Hermes judges the registration without running your code. | | **Off by default** | Only `vault..enabled: true` turns a backend on. | | **In-process only** | `ctx.register_login_backend` is refused under `plugins.isolation: host`, because `fill_context()` is a server-local context manager. | | **Same fill path** | Plugin items go through the same exact-origin pre-check, in-page origin re-assertion, supervisor CDP secret evaluation (never argv) and payment confirmation as built-in items. |