Merge pull request #225 from SaplingLearn/security/125-search-materials-user-scope

fix(security): scope search_course_materials by user_id (#125)

Author: Jose-Gael-Cruz-Lopez

backend/agents/tools/chat_context.py

@@ -145,9 +145,17 @@ async def search_course_materials(
     course_id: str | None,
     query: str,
     limit: int = 5,
+    *,
+    user_id: str,
 ) -> list[CourseMaterial]:
-    """Return the top `limit` documents for `course_id`, ranked by
-    keyword overlap with `query`.
+    """Return the top `limit` documents owned by `user_id` in `course_id`,
+    ranked by keyword overlap with `query`.
+
+    #125: documents are user-scoped *within* a shared course, so the query
+    MUST filter on user_id as well as course_id — otherwise another enrolled
+    student's private summary/concept_notes get decrypted into this user's LLM
+    context. user_id is keyword-only and required so no caller can silently
+    omit the scope.
 
     Drops rows that have neither a summary nor concept notes — there's
     nothing to ground on, and including them would waste a tool-result
@@ -167,7 +175,10 @@ def _fetch() -> list[dict[str, Any]]:
             return (
                 table("documents").select(
                     "id,file_name,summary,concept_notes",
-                    filters={"course_id": f"eq.{course_id}"},
+                    filters={
+                        "course_id": f"eq.{course_id}",
+                        "user_id": f"eq.{user_id}",
+                    },
                     order="created_at.desc",
                 )
                 or []
@@ -238,11 +249,13 @@ async def search_course_materials_tool(
 ) -> list[CourseMaterial]:
     """Pydantic AI tool wrapper.
 
-    The LLM supplies `query` (and optionally `limit`); `course_id` is
-    pulled from `ctx.deps` so the model can't aim a search at another
-    course's materials.
+    The LLM supplies `query` (and optionally `limit`); `course_id` and
+    `user_id` are pulled from `ctx.deps` so the model can't aim a search at
+    another course's — or another user's — materials.
     """
-    return await search_course_materials(ctx.deps.course_id, query, limit)
+    return await search_course_materials(
+        ctx.deps.course_id, query, limit, user_id=ctx.deps.user_id
+    )
 
 
 # read_session_history

backend/tests/test_chat_context_tools.py

@@ -41,12 +41,62 @@ def _run(coro):
 # ── search_course_materials ───────────────────────────────────────────────
 
 
+class TestSearchCourseMaterialsUserScope:
+    """#125: documents are user-scoped within a shared course. The query must
+    filter on user_id, or another enrolled student's private summary/concept
+    notes get decrypted into this user's LLM context."""
+
+    def test_does_not_return_other_users_documents(self):
+        doc_mine = {
+            "id": "doc_mine",
+            "file_name": "my_notes.pdf",
+            "summary": "my private summary about recursion",
+            "concept_notes": [],
+        }
+        doc_other = {
+            "id": "doc_other",
+            "file_name": "their_notes.pdf",
+            "summary": "another student's private summary about recursion",
+            "concept_notes": [],
+        }
+
+        def _row_scoped_select(*_args, **kwargs):
+            # Faithful row-scoped store: both docs sit in the same course, owned
+            # by different users. A query scoped to me returns only mine; an
+            # UNSCOPED query (pre-fix — no user_id filter) leaks the whole class.
+            f = kwargs.get("filters", {})
+            uid = f.get("user_id")
+            if uid == "eq.user_mine":
+                return [doc_mine]
+            if "user_id" not in f:
+                return [doc_mine, doc_other]
+            return []
+
+        with patch("agents.tools.chat_context.table") as t, patch(
+            "agents.tools.chat_context.decrypt_if_present", side_effect=lambda v: v
+        ):
+            t.return_value.select.side_effect = _row_scoped_select
+            result = _run(
+                search_course_materials("course_cs101", "recursion", user_id="user_mine")
+            )
+
+        ids = {m.document_id for m in result}
+        assert "doc_mine" in ids
+        # Pre-fix this leaked the other student's doc into the result (and thus
+        # the LLM context); the user_id scope keeps it out.
+        assert "doc_other" not in ids
+        # And the mechanism that keeps it out: the DB query is scoped to the
+        # caller's user_id, not course_id alone.
+        filters = t.return_value.select.call_args.kwargs["filters"]
+        assert filters.get("user_id") == "eq.user_mine"
+
+
 class TestSearchCourseMaterials:
     def test_returns_empty_when_course_id_is_none(self):
         # No table call should happen at all — cross-course search is a
         # data-leak risk we explicitly avoid.
         with patch("agents.tools.chat_context.table") as t:
-            result = _run(search_course_materials(None, "recursion"))
+            result = _run(search_course_materials(None, "recursion", user_id="user_andres"))
 
         assert result == []
         t.assert_not_called()
@@ -82,7 +132,9 @@ def test_scores_by_keyword_overlap_and_caps_at_limit(self):
         ):
             t.return_value.select.return_value = rows
             result = _run(
-                search_course_materials("course_cs101", "recursion base case", limit=2)
+                search_course_materials(
+                    "course_cs101", "recursion base case", limit=2, user_id="user_andres"
+                )
             )
 
         assert len(result) == 2
@@ -113,7 +165,9 @@ def test_drops_empty_entries(self):
             "agents.tools.chat_context.decrypt_if_present", side_effect=lambda v: v
         ):
             t.return_value.select.return_value = rows
-            result = _run(search_course_materials("course_cs101", "pointer"))
+            result = _run(
+                search_course_materials("course_cs101", "pointer", user_id="user_andres")
+            )
 
         assert [m.document_id for m in result] == ["doc_good"]
 
@@ -145,7 +199,9 @@ def fake_decrypt_json(value):
             "agents.tools.chat_context.decrypt_json", side_effect=fake_decrypt_json
         ) as dec_json:
             t.return_value.select.return_value = rows
-            result = _run(search_course_materials("course_cs101", "foo"))
+            result = _run(
+                search_course_materials("course_cs101", "foo", user_id="user_andres")
+            )
 
         # Both decrypt helpers were invoked at the boundary.
         assert dec_str.called, "decrypt_if_present must run on summary"
@@ -309,8 +365,10 @@ def test_search_tool_passes_course_id_from_deps(self):
             inner.return_value = []
             _run(search_course_materials_tool(self._ctx(), "recursion", limit=3))
 
-        # course_id pulled from deps, not from the LLM.
-        inner.assert_awaited_once_with("course_cs101", "recursion", 3)
+        # course_id AND user_id pulled from deps, not from the LLM (#125).
+        inner.assert_awaited_once_with(
+            "course_cs101", "recursion", 3, user_id="user_andres"
+        )
 
     def test_history_tool_passes_session_id_from_deps(self):
         with patch(