From 4079f12ae6f1534702e4cfe10e6ccce47aeaf64b Mon Sep 17 00:00:00 2001 From: Alexandre Teixeira <111787685+alteixeira20@users.noreply.github.com> Date: Wed, 28 Oct 2026 00:31:42 +0000 Subject: [PATCH] fix(editor): resolve preview authorization and CI regressions --- routes/chat_helpers.py | 10 ++++ src/clean_agent_preview.py | 3 ++ tests/test_caldav_redirect_hardening.py | 13 ++++- tests/test_clean_agent_preview.py | 50 ++++++++++++++++++++ tests/test_containment_enforcement.py | 36 ++++++++++++-- tests/test_utility_reasoning_effort.py | 63 +++++++++++++++++++++++-- 6 files changed, 166 insertions(+), 9 deletions(-) diff --git a/routes/chat_helpers.py b/routes/chat_helpers.py index a0c23ae0b..0d815309e 100644 --- a/routes/chat_helpers.py +++ b/routes/chat_helpers.py @@ -569,6 +569,16 @@ async def auto_name_session(session_manager, sess): if not first_msg: return + from src.owner_identity import is_request_sentinel_owner + owner = getattr(sess, "owner", None) + invalid_owner = ( + not isinstance(owner, str) or not owner.strip() + or owner != owner.strip() or is_request_sentinel_owner(owner) + ) + if invalid_owner: + session_manager.update_session_name(sess.id, fallback_session_title(first_msg)) + return + endpoint_url = str(getattr(sess, "endpoint_url", "") or "") model_name = str(getattr(sess, "model", "") or "") diff --git a/src/clean_agent_preview.py b/src/clean_agent_preview.py index eed0c4cb1..a9899f570 100644 --- a/src/clean_agent_preview.py +++ b/src/clean_agent_preview.py @@ -3881,6 +3881,9 @@ def evaluate_preview_call(name, args, user_text='', *, allow_execute_code=False, or bare in BROKERED_JOB_TOOLS or (contract_required and bare in {'edit_image', 'generate_image'}) or (contract_required and bare == 'create_document' and standalone_code_request(user_text)) + # The editor command is conservatively classified as a write; + # document.info is a read even when the user requested no edit. + or (bare == 'run_editor_command' and args.get('op') == 'document.info') ) ) if family not in authorized and not contextual_revision and not contract_scoped_mutation and not fixture_delete and not offered_private_action: diff --git a/tests/test_caldav_redirect_hardening.py b/tests/test_caldav_redirect_hardening.py index 0d3ce91b7..cd82974aa 100644 --- a/tests/test_caldav_redirect_hardening.py +++ b/tests/test_caldav_redirect_hardening.py @@ -77,8 +77,19 @@ def test_dav_client_does_not_follow_redirect_to_internal_host(): pass # The request must actually have left the building — otherwise an early # error would make "sink not hit" pass vacuously. - assert public_methods == ["PROPFIND"], "the PROPFIND must reach the public server first" + # Some caldav releases retry the original URL with GET to probe auth + # after the redirect exception. Neither request may reach the sink. + assert public_methods in (["PROPFIND"], ["PROPFIND", "GET"]), public_methods assert sink_hits == [], "redirect toward an internal host must not be followed" + + # Positive control: demonstrate that the sink would see the redirect + # if the session's redirect protection were removed. + client.session.max_redirects = 30 + try: + client.request(public_url, "PROPFIND", "") + except Exception: + pass # The sink's response is intentionally not a valid DAV body. + assert sink_hits == ["/leak"], "the test must detect a followed redirect" finally: internal.shutdown() public.shutdown() diff --git a/tests/test_clean_agent_preview.py b/tests/test_clean_agent_preview.py index 122a05b02..c24f8e6f3 100644 --- a/tests/test_clean_agent_preview.py +++ b/tests/test_clean_agent_preview.py @@ -3551,6 +3551,56 @@ def test_every_compactly_offered_preview_tool_has_valid_policy_permitted_call(): assert decision.allowed, f'{name}: {decision.reason}' +def test_photo_editor_tools_require_explicit_turn_family_authorization(): + # Positive: permitted photo editor workflow with turn authorization + for tool, args, prompt in ( + ('run_editor_command', {'op': 'document.info'}, 'inspect the document'), + ('run_editor_command', {'op': 'adjustments.apply', 'params': {'type': 'brightness-contrast'}}, 'make it brighter'), + ('ai_edit_image', {'op': 'remove-bg'}, 'remove background'), + ): + allowed = evaluate_preview_call( + tool, args, prompt, + turn_authorized_families={'photo_editor'}, + ) + assert allowed.allowed, f"Expected {tool} to be allowed with photo_editor turn authority: {allowed.reason}" + + # Negative: unauthorized preview (unauthorized family, empty families, or cross-family) fails closed + for unauth_families in (frozenset(), {'notes'}, {'email'}, {'calendar'}, {'search_browser'}): + denied_info = evaluate_preview_call( + 'run_editor_command', {'op': 'document.info'}, 'inspect the document', + turn_authorized_families=unauth_families, + ) + assert not denied_info.allowed + assert denied_info.reason == 'write_family_not_authorized' + + denied_edit = evaluate_preview_call( + 'run_editor_command', {'op': 'adjustments.apply', 'params': {'type': 'brightness-contrast'}}, + 'make it brighter', + turn_authorized_families=unauth_families, + ) + assert not denied_edit.allowed + assert denied_edit.reason == 'write_family_not_authorized' + + denied_ai = evaluate_preview_call( + 'ai_edit_image', {'op': 'remove-bg'}, 'remove background', + turn_authorized_families=unauth_families, + ) + assert not denied_ai.allowed + assert denied_ai.reason == 'write_family_not_authorized' + + for prompt in ('inspect the document', "don't edit the image"): + denied = evaluate_preview_call( + 'run_editor_command', {'op': 'adjustments.apply', 'params': {'type': 'brightness-contrast'}}, + prompt, turn_authorized_families={'photo_editor'}, + ) + assert not denied.allowed + assert denied.reason == 'write_family_not_authorized' + + denied = evaluate_preview_call('manage_notes', {'action': 'add', 'content': 'x'}, + 'make it brighter', turn_authorized_families={'photo_editor'}) + assert not denied.allowed + + def test_full_compact_inventory_contains_pipeline_for_exact_session_contract(): from src.turn_contract import resolve_full_inventory_contract diff --git a/tests/test_containment_enforcement.py b/tests/test_containment_enforcement.py index d4d2d5a34..998241a9b 100644 --- a/tests/test_containment_enforcement.py +++ b/tests/test_containment_enforcement.py @@ -207,12 +207,42 @@ async def test_default_namespace_preserves_loopback_sidecars(namespaces): assert "network" not in result["containment"]["enforced"] +def _assert_sandboxed_shell_output(result, expected): + # Some distributions' login profiles invoke systemd-cat via im-config. + # The sandbox intentionally has no host journal socket; retain its exact + # diagnostic in the result and reject every other stderr message. + assert result["exit_code"] == 0, result + assert result["containment"]["mechanism"] == "bubblewrap", result + assert result["containment"]["contained"] is True, result + assert result["containment"]["executed"] is True, result + assert result["teardown"]["dead"] is True, result + stdout, separator, stderr = result["output"].partition("\nSTDERR: ") + assert stdout == expected, result + if separator: + assert stderr and all( + line == "Failed to create stream fd: No such file or directory" + for line in stderr.splitlines() + ), result + + +def test_shell_output_check_rejects_unexpected_diagnostics_and_failed_launch(): + result = { + "exit_code": 0, "containment": {"mechanism": "bubblewrap", "contained": True, "executed": True}, + "teardown": {"dead": True}, "output": "closed\nSTDERR: unexpected error", + } + with pytest.raises(AssertionError): + _assert_sandboxed_shell_output(result, "closed") + result["output"] = "closed" + result["containment"]["executed"] = False + with pytest.raises(AssertionError): + _assert_sandboxed_shell_output(result, "closed") + + async def test_namespace_handshake_closes_model_stdin(namespaces): result = await subprocess_tools.BashTool().execute( "if read value; then echo unexpected; else echo closed; fi", {}, ) - assert result["output"] == "closed", result - assert result["teardown"]["dead"] is True + _assert_sandboxed_shell_output(result, "closed") @pytest.mark.parametrize("legacy_name", [True, False]) @@ -240,7 +270,7 @@ async def test_execution_environment_cannot_replace_probed_bwrap(namespaces, mon assert "containment unavailable" in result["error"] assert outside.read_text() == "escaped" else: - assert result["output"] == "contained", result + _assert_sandboxed_shell_output(result, "contained") assert not outside.exists() diff --git a/tests/test_utility_reasoning_effort.py b/tests/test_utility_reasoning_effort.py index 1ed2b115b..abba49793 100644 --- a/tests/test_utility_reasoning_effort.py +++ b/tests/test_utility_reasoning_effort.py @@ -311,14 +311,67 @@ class _Manager: pass -def test_auto_title_inherits_chat_effort_when_utility_is_blank(util, monkeypatch): +@pytest.fixture +def title_registered_endpoint(tmp_path, monkeypatch): + import core.database as database + import src.database as compat_database + import src.endpoint_resolver as resolver + from tests.helpers.database import disposable_database + + monkeypatch.setenv("AUTH_ENABLED", "true") + monkeypatch.setattr("src.settings.load_settings", lambda: {}) + monkeypatch.setattr(resolver, "resolve_url", lambda url: url) + with disposable_database(tmp_path) as factory: + monkeypatch.setattr(compat_database, "SessionLocal", factory) + monkeypatch.setattr(resolver, "SessionLocal", factory) + with factory() as db: + db.add(database.ModelEndpoint(id="chat", name="chat", owner="alice", + base_url=CHAT[0], is_enabled=True)) + db.add(database.ModelEndpoint(id="shared", name="shared", owner=None, + base_url="https://shared.example/v1", is_enabled=True)) + db.commit() + yield + + +def test_auto_title_inherits_chat_effort_when_utility_is_blank(util, monkeypatch, title_registered_endpoint): + from routes import chat_helpers + from src.endpoint_resolver import build_chat_url + + sent = _capture_llm(monkeypatch) + chat_url = build_chat_url(CHAT[0]) + asyncio.run(chat_helpers.auto_name_session(_Manager(), _title_session(url=chat_url))) + assert sent == [(chat_url, CHAT[1], "low")] + + +@pytest.mark.parametrize("owner,url", [ + ("alice", "https://unregistered.example/v1"), + ("bob", CHAT[0]), + (None, "https://shared.example/v1"), + ("", "https://shared.example/v1"), + (" ", "https://shared.example/v1"), + (" alice ", "https://shared.example/v1"), + ("system", "https://shared.example/v1"), + (42, "https://shared.example/v1"), +]) +def test_auto_title_rejects_unauthorized_chat_endpoint_when_utility_is_blank( + util, monkeypatch, title_registered_endpoint, owner, url, +): from routes import chat_helpers sent = _capture_llm(monkeypatch) - # Blank utility/task resolves to the session's own route. - monkeypatch.setattr(task_endpoint, "resolve_task_endpoint", lambda *a, **k: (CHAT[0], CHAT[1], {})) - asyncio.run(chat_helpers.auto_name_session(_Manager(), _title_session())) - assert sent == [(CHAT[0], CHAT[1], "low")] + asyncio.run(chat_helpers.auto_name_session(_Manager(), _title_session(owner=owner, url=url))) + assert sent == [] + + +def test_auto_title_missing_owner_fails_closed_in_single_user_mode(util, monkeypatch, title_registered_endpoint): + from routes import chat_helpers + + monkeypatch.setenv("AUTH_ENABLED", "false") + sent = _capture_llm(monkeypatch) + asyncio.run(chat_helpers.auto_name_session( + _Manager(), _title_session(owner=None, url="https://shared.example/v1"), + )) + assert sent == [] def test_auto_title_setting_beats_chat_effort(util, monkeypatch):