From 5df127b9b186d9e2074d5a62f83033d7028ac0ff Mon Sep 17 00:00:00 2001 From: Andrey Shiryaev <7615137+shoom1@users.noreply.github.com> Date: Wed, 15 Jul 2026 21:04:27 -0400 Subject: [PATCH] fix(settings): guard /settings dialog to project-scoped keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Policy follow-up to #105: settings that persist at user level must not be editable via /settings — a dialog edit to a non-allowlisted key would land in ~/.{app}/settings.json and silently apply across all projects of the app. _build_ui_items() now excludes (with a per-key warning) any key whose target field is not in PROJECT_SETTABLE_KEYS, regardless of what a domain app returns from get_ui_setting_keys(). The synthetic "model" key passes via the _UI_KEY_TARGET_FIELDS alias (set_model() writes default_model, which is allowlisted). Dangling keys for removed fields (e.g. airesearcher's log_activity) now warn instead of being silently skipped. Programmatic update_setting()/save_settings() with user-scoped keys stays legitimate — the split-save from #105 remains their writer; the dialog just can't produce them. Claude-Session: https://claude.ai/code/session_01SwkKXQpy3JLEqrPkQA8QRv --- src/agentic_cli/cli/app.py | 21 +++++++ tests/cli/test_settings_ui_scope.py | 93 +++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+) create mode 100644 tests/cli/test_settings_ui_scope.py diff --git a/src/agentic_cli/cli/app.py b/src/agentic_cli/cli/app.py index 6fb83c2..7cf270b 100644 --- a/src/agentic_cli/cli/app.py +++ b/src/agentic_cli/cli/app.py @@ -24,6 +24,7 @@ from agentic_cli.cli.workflow_controller import WorkflowController from agentic_cli.config import BaseSettings from agentic_cli.logging import Loggers, configure_logging +from agentic_cli.settings_persistence import PROJECT_SETTABLE_KEYS if TYPE_CHECKING: from agentic_cli.settings_persistence import SettingsSaveResult @@ -33,6 +34,12 @@ logger = Loggers.cli() +# Synthetic dialog keys → the settings field their setter actually writes. +# "model" is not a field; update_setting() routes it via set_model() to +# default_model. A synthetic key without an entry here is treated as writing +# a field of the same name. +_UI_KEY_TARGET_FIELDS = {"model": "default_model"} + # === Slash Command Completer === @@ -196,6 +203,11 @@ def get_ui_setting_keys(self) -> list[str]: Override to customize which settings appear in the UI. Default: model, thinking_effort + Only project-scoped settings may appear: a key whose target field is + not in PROJECT_SETTABLE_KEYS is excluded by _build_ui_items (with a + warning), because a /settings edit to it would persist in the user + ~/.{app}/settings.json and apply across all projects of the app. + Returns: List of field names that should appear in the settings UI """ @@ -216,6 +228,15 @@ def _build_ui_items(self) -> list[Any]: items: list[tuple[int, Any]] = [] for key in self.get_ui_setting_keys(): + # Project-scope guard: the dialog may only expose settings the + # project file can persist (PROJECT_SETTABLE_KEYS). A user-scoped + # key would save to the user ~/.{app}/settings.json and leak the + # change across all projects of the app. + target_field = _UI_KEY_TARGET_FIELDS.get(key, key) + if target_field not in PROJECT_SETTABLE_KEYS: + logger.warning("user_scoped_setting_excluded_from_ui", key=key) + continue + # Handle special 'model' field with dynamic options if key == "model": available_models = list(self._settings.get_available_models()) diff --git a/tests/cli/test_settings_ui_scope.py b/tests/cli/test_settings_ui_scope.py new file mode 100644 index 0000000..60bfb92 --- /dev/null +++ b/tests/cli/test_settings_ui_scope.py @@ -0,0 +1,93 @@ +"""Guard: the /settings dialog exposes only project-scoped (allowlisted) settings. + +User-scoped keys (not in PROJECT_SETTABLE_KEYS) must never render in the +dialog, regardless of what a domain app returns from get_ui_setting_keys() — +otherwise a /settings edit would persist to the user ~/.{app}/settings.json +and apply across all projects. +""" + +from __future__ import annotations + +import pytest +import structlog + +from agentic_cli.cli.app import BaseCLIApp + +EXCLUDED_EVENT = "user_scoped_setting_excluded_from_ui" + + +def _make_app(settings, keys: list[str]) -> BaseCLIApp: + class _App(BaseCLIApp): + def get_ui_setting_keys(self) -> list[str]: + return keys + + app = _App.__new__(_App) + app._settings = settings + return app + + +@pytest.fixture +def isolated_settings(tmp_path, monkeypatch): + """Hermetic BaseSettings: temp cwd/HOME so no real config files load.""" + home = tmp_path / "home" + home.mkdir() + monkeypatch.chdir(tmp_path) + monkeypatch.setenv("HOME", str(home)) + from agentic_cli.config import BaseSettings + + # A key so get_available_models() is non-empty and "model" can render + return BaseSettings(google_api_key="test-key") + + +def _item_keys(items) -> list[str]: + return [item.key for item in items] + + +class TestSettingsUiScopeGuard: + def test_user_scoped_key_is_excluded_with_warning(self, isolated_settings): + """A non-allowlisted field never renders; the exclusion is logged.""" + app = _make_app(isolated_settings, ["thinking_effort", "raw_llm_logging"]) + + with structlog.testing.capture_logs() as logs: + items = app._build_ui_items() + + assert _item_keys(items) == ["thinking_effort"] + assert any( + e.get("event") == EXCLUDED_EVENT and e.get("key") == "raw_llm_logging" + for e in logs + ) + + def test_allowlisted_keys_render_without_warning(self, isolated_settings): + app = _make_app(isolated_settings, ["thinking_effort", "verbose_thinking"]) + + with structlog.testing.capture_logs() as logs: + items = app._build_ui_items() + + assert set(_item_keys(items)) == {"thinking_effort", "verbose_thinking"} + assert not any(e.get("event") == EXCLUDED_EVENT for e in logs) + + def test_model_synthetic_key_passes_guard(self, isolated_settings): + """"model" is not a field but writes default_model (allowlisted).""" + app = _make_app(isolated_settings, ["model"]) + + with structlog.testing.capture_logs() as logs: + items = app._build_ui_items() + + assert _item_keys(items) == ["model"] + assert not any(e.get("event") == EXCLUDED_EVENT for e in logs) + + def test_dangling_nonexistent_key_is_excluded_with_warning( + self, isolated_settings + ): + """A key for a removed field (e.g. airesearcher's log_activity) now + warns instead of being silently skipped.""" + app = _make_app(isolated_settings, ["log_activity"]) + + with structlog.testing.capture_logs() as logs: + items = app._build_ui_items() + + assert items == [] + assert any( + e.get("event") == EXCLUDED_EVENT and e.get("key") == "log_activity" + for e in logs + )