test: restore active-document and owner-isolation sensitivity

tests/test_agent_loop.py

@@ -65,6 +65,12 @@ def test_what_im_writing_targets_the_visible_document():
         language="richtext",
     )
 
+    assert _turn_targets_active_document(
+        {"domains": set()},
+        "can you fact check what Im writing",
+        active_document,
+    )
+
 
 def test_unrelated_fact_check_does_not_target_a_stale_visible_document():
     active_document = MagicMock(

tests/test_chat_document_owner_context.py

@@ -1,119 +0,0 @@
-"""The chat stream must never pass another owner's document to the model."""
-
-import json
-from types import SimpleNamespace
-
-import pytest
-from fastapi import FastAPI
-from fastapi.testclient import TestClient
-
-from core.auth import AuthManager
-from routes.auth_routes import SESSION_COOKIE
-from tests.helpers.database import isolated_session_database
-
-
-@pytest.fixture
-def document_chat(tmp_path, monkeypatch):
-    from routes import chat_routes, chat_helpers, session_routes
-    from src.agent_tools import document_tools
-    from src.auth_helpers import effective_user
-
-    monkeypatch.setenv("AUTH_ENABLED", "true")
-    auth = AuthManager(str(tmp_path / "auth.json"))
-    for owner in ("alice", "bob"):
-        assert auth.create_user(owner, "disposable-test-password")
-    captured = []
-
-    async def context(sess, request, *args, **kwargs):
-        # Preprocessing and external model I/O are outside the owner lookup.
-        # The actual route, session-owner gate and document ORM queries run.
-        return chat_helpers.ChatContext(
-            preface=[], rag_sources=[], web_sources=[], used_memories=[],
-            messages=[{"role": "user", "content": "Summarize my writing"}],
-            context_length=4096, was_compacted=False, user=effective_user(request), uprefs={},
-            preset=chat_helpers.PresetInfo(0.2, 128, None, None),
-            preprocessed=chat_helpers.PreprocessedMessage("", "", "", [], []),
-        )
-
-    async def model(endpoint, model, messages, **kwargs):
-        doc = kwargs.get("active_document")
-        captured.append(None if doc is None else (doc.id, doc.owner, doc.current_content))
-        text = doc.current_content if doc is not None else "no document context"
-        yield f'data: {json.dumps({"delta": text})}\n\n'
-        yield 'data: [DONE]\n\n'
-
-    monkeypatch.setattr(chat_routes, "build_chat_context", context)
-    monkeypatch.setattr(chat_routes, "stream_agent_loop", model)
-    monkeypatch.setattr(chat_routes, "save_assistant_response", lambda *a, **kw: None)
-    monkeypatch.setattr(chat_routes, "run_post_response_tasks", lambda *a, **kw: None)
-    monkeypatch.setattr(document_tools, "_active_document_id", None)
-    with isolated_session_database(tmp_path) as (manager, database):
-        factory = database.SessionLocal
-        for module in (chat_routes, chat_helpers, session_routes):
-            monkeypatch.setattr(module, "SessionLocal", factory)
-        # Existing route tests may re-import session_routes after chat imported
-        # its owner check. Bind that real function's database too, without
-        # replacing the authorization logic or reloading production modules.
-        monkeypatch.setitem(chat_routes._verify_session_owner.__globals__, "SessionLocal", factory)
-        with factory() as db:
-            db.add_all([
-                database.Session(id="alice-chat", name="Alice", owner="alice",
-                                 endpoint_url="http://model.invalid/v1", model="fixture-model"),
-                database.Session(id="bob-chat", name="Bob", owner="bob",
-                                 endpoint_url="http://model.invalid/v1", model="fixture-model"),
-                database.ModelEndpoint(id="fixture-endpoint", name="Fixture",
-                                       base_url="http://model.invalid/v1", owner="alice"),
-            ])
-            db.commit()
-        app = FastAPI()
-        app.state.auth_manager = auth
-
-        @app.middleware("http")
-        async def fixture_session(request, call_next):
-            request.state.current_user = auth.get_username_for_token(request.cookies.get(SESSION_COOKIE))
-            return await call_next(request)
-
-        app.include_router(chat_routes.setup_chat_routes(
-            manager, SimpleNamespace(), SimpleNamespace(), SimpleNamespace(),
-            SimpleNamespace(), SimpleNamespace(),
-        ))
-        token = auth.create_session("alice", "disposable-test-password")
-        assert token
-        with TestClient(app) as client:
-            client.cookies.set(SESSION_COOKIE, token)
-            yield SimpleNamespace(client=client, factory=factory, database=database,
-                                  captured=captured, document_tools=document_tools)
-
-
-@pytest.mark.parametrize("source", ["explicit", "session", "email-session", "memory"])
-@pytest.mark.parametrize("document_owner", ["alice", "bob"])
-def test_chat_document_context_is_owner_scoped(document_chat, source, document_owner):
-    case = document_chat
-    form = {"message": "Summarize my writing", "session": "alice-chat", "mode": "agent",
-            "compare_mode": "true", "no_memory": "true", "use_rag": "false"}
-    with case.factory() as db:
-        for owner in ("alice", "bob"):
-            # A stale session link is not evidence that the caller owns a doc.
-            linked = "alice-chat" if source in {"session", "email-session"} and owner == document_owner else None
-            db.add(case.database.Document(id=f"{owner}-doc", owner=owner, session_id=linked,
-                                         title=f"{owner} draft",
-                                         language="email" if source == "email-session" else "markdown",
-                                         current_content=f"{owner}-private-context", is_active=True))
-        db.commit()
-    if source == "explicit":
-        form.update(active_doc_id=f"{document_owner}-doc", active_doc_state="visible")
-    elif source == "memory":
-        case.document_tools.set_active_document(f"{document_owner}-doc")
-    response = case.client.post("/api/chat_stream", data=form)
-    assert response.status_code == 200, response.text
-    if document_owner == "alice":
-        assert case.captured == [("alice-doc", "alice", "alice-private-context")]
-        assert "alice-private-context" in response.text
-    else:
-        assert case.captured == [None]
-    assert "bob-private-context" not in response.text
-    with case.factory() as db:
-        assert db.get(case.database.Document, "bob-doc").owner == "bob"
-        assert db.get(case.database.Document, "bob-doc").session_id == (
-            "alice-chat" if source in {"session", "email-session"} and document_owner == "bob" else None
-        )

tests/test_skills_manager_owner_isolation.py

@@ -1,24 +1,8 @@
-"""Independent validation test for the claim that
+"""Skills operations must preserve ownership across same-slug skills.
-`SkillsManager.update_skill` mutates the first skill on disk matching
-`name` regardless of the caller's owner, and that `owner` is in its
-`scalar_keys` whitelist allowing cross-user ownership reassignment.
 
-This test sets up two user-owned skills on disk with the SAME slug
+An unscoped update may match only ownerless skills. With Alice's and Bob's
-(`login-flow`) — Alice's and Bob's — and then calls `update_skill` with
+skills sharing a slug, it must return False and leave both files unchanged.
-NO `owner` argument. If the bug is real, exactly one of the two files
+Unexpected implementation errors must fail the regression test.
-will be mutated (whichever `_iter_skill_files` yields first) and the
-caller will have effectively re-stamped the file as owned by the value
-in `updates["owner"]` ("attacker"). If the manager method is safe (or
-the slug uniqueness invariant makes the bug moot), the call should
-either:
-  * raise (it requires an `owner` argument), OR
-  * be a no-op (no other side effect on Bob's file), OR
-  * the file that gets modified should still belong to its original
-    owner (no ownership reassignment).
-
-We assert the safer behaviors; the test FAILS only when update_skill
-silently mutates a file owned by a different user AND overwrites the
-`owner` field with an attacker's value.
 """
 
 import os
@@ -27,9 +11,6 @@ import textwrap
 from pathlib import Path
 from unittest.mock import MagicMock
 
-import pytest
-
-
 # ── module-load stubbing (matches other tests in this repo) ──────────
 # Stub heavy deps so importing the skills manager doesn't pull DB / FastAPI.
 for _mod in ("sqlalchemy", "sqlalchemy.orm", "sqlalchemy.ext", "sqlalchemy.ext.declarative"):
@@ -100,37 +81,34 @@ def test_update_skill_does_not_mutate_foreign_owned_skill(tmp_path):
     sm = SkillsManager(str(tmp_path))
 
     # Snapshot before.
-    before_alice = alice_path.read_text(encoding="utf-8")
+    before_alice = alice_path.read_bytes()
-    before_bob = bob_path.read_text(encoding="utf-8")
+    before_bob = bob_path.read_bytes()
 
     # Try to reassign + mutate. The caller does NOT supply an owner
     # arg, mirroring the in-process callers in tool_implementations.py
     # (lines 716, 740, 753) which call sm.update_skill(name, updates).
-    try:
+    result = sm.update_skill(
-        result = sm.update_skill(
+        "login-flow",
-            "login-flow",
+        {"owner": "attacker", "description": "pwned"},
-            {"owner": "attacker", "description": "pwned"},
+    )
-        )
-    except TypeError as e:
-        # If the method were fixed to require an owner arg, this is
-        # the desired (safe) behavior — the call refused.
-        pytest.skip(
-            f"update_skill raised TypeError (refused unsafe call): {e}"
-        )
-        return
 
     # After: read what each file now contains.
-    after_alice = alice_path.read_text(encoding="utf-8")
+    after_alice = alice_path.read_bytes()
-    after_bob = bob_path.read_text(encoding="utf-8")
+    after_bob = bob_path.read_bytes()
+
+    assert after_alice == before_alice
+    assert after_bob == before_bob
+    assert b"owner: alice" in after_alice
+    assert b"owner: bob" in after_bob
 
     # Invariant 1: a file that was owned by `alice` (resp. `bob`) MUST
     # NOT end up owned by `attacker` after the call. If it does, that's
     # the cross-user ownership reassignment bug.
-    assert "owner: attacker" not in after_alice, (
+    assert b"owner: attacker" not in after_alice, (
         "BUG: Alice's file was silently re-owned as 'attacker' by "
         "update_skill (cross-user ownership reassignment)."
     )
-    assert "owner: attacker" not in after_bob, (
+    assert b"owner: attacker" not in after_bob, (
         "BUG: Bob's file was silently re-owned as 'attacker' by "
         "update_skill (cross-user ownership reassignment)."
     )
@@ -138,23 +116,19 @@ def test_update_skill_does_not_mutate_foreign_owned_skill(tmp_path):
     # Invariant 2: a file that was owned by `alice` and contained
     # description "alice original" must not be silently mutated into
     # "pwned" by a caller that did not supply an owner.
-    if "alice original" in before_alice:
+    if b"alice original" in before_alice:
-        assert "alice original" in after_alice, (
+        assert b"alice original" in after_alice, (
             "BUG: Alice's skill description was overwritten by a call "
             "to update_skill that did not scope to her owner."
         )
 
-    if "bob original" in before_bob:
+    if b"bob original" in before_bob:
-        assert "bob original" in after_bob, (
+        assert b"bob original" in after_bob, (
             "BUG: Bob's skill description was overwritten by a call "
             "to update_skill that did not scope to his owner."
         )
 
-    # The return value should not lie about success — if the manager
+    assert result is False, "an unscoped update must not report a foreign-owner mutation"
-    # touched nothing because both files were foreign-owned, the safer
-    # behavior is to return False, not True. (A return of True is the
-    # buggy path; we don't assert False, we just don't assert True.)
-    _ = result  # not asserted; documented behavior is not the point.
 
 
 def test_update_skill_scalar_keys_exclude_owner():