diff --git a/src/scribe/mcp/tools/processes.py b/src/scribe/mcp/tools/processes.py index b4d831c..1246406 100644 --- a/src/scribe/mcp/tools/processes.py +++ b/src/scribe/mcp/tools/processes.py @@ -190,10 +190,12 @@ async def update_process(process_id: int, title: str = "", body: str = "", fields["tags"] = tags # As the owner — update_note is owner-scoped and the write is authorised above. updated = await notes_svc.update_note(note.user_id, process_id, **fields) - # Written as the OWNER, matching the update above: an editor-shared process - # keeps its owner's associations rather than sprouting a second set. + # The CALLER, not the owner. `update_note` above is owner-scoped because + # the service demands it; `set_record_systems` is not — it runs its own + # share-aware check and links only Systems the acting user can read, so + # passing the owner would bypass the check and borrow their reach (#47). if system_ids is not None: - await systems_svc.set_record_systems(note.user_id, process_id, system_ids) + await systems_svc.set_record_systems(uid, process_id, system_ids) if updated is None: raise ValueError(f"process {process_id} not found") out = updated.to_dict() diff --git a/src/scribe/mcp/tools/snippets.py b/src/scribe/mcp/tools/snippets.py index e4ad0ad..5824d6b 100644 --- a/src/scribe/mcp/tools/snippets.py +++ b/src/scribe/mcp/tools/snippets.py @@ -483,7 +483,10 @@ async def update_snippet( if note is None: raise ValueError(f"snippet {snippet_id} not found") if system_ids is not None: - await systems_svc.set_record_systems(note.user_id, snippet_id, system_ids) + # The CALLER, not the owner (#4249). `update_snippet` above already + # raised PermissionError if this user may not write, so the tagging + # runs with the actor's own System visibility rather than the owner's. + await systems_svc.set_record_systems(uid, snippet_id, system_ids) data = snippets_svc.snippet_to_dict(note) data.update(await access_svc.describe_provenance(uid, note)) await systems_tools.attach_systems( diff --git a/src/scribe/routes/lessons.py b/src/scribe/routes/lessons.py index cd384d1..e7e19d6 100644 --- a/src/scribe/routes/lessons.py +++ b/src/scribe/routes/lessons.py @@ -240,9 +240,13 @@ async def update_lesson_route(lesson_id: int): if updated is None: return not_found("Lesson") if data.get("system_ids") is not None: - await systems_svc.set_record_systems( - owner_uid, lesson_id, data["system_ids"] - ) + # The CALLER, not owner_uid (#4249). `set_record_systems` runs its own + # `can_write_note` and links only Systems the acting user can read; + # handing it the owner makes that check trivially pass and filters by + # the owner's visibility instead. The caller's write permission is + # already established above, so this neither loosens nor tightens who + # may edit — it decides WHOSE reach the tagging uses (#47). + await systems_svc.set_record_systems(uid, lesson_id, data["system_ids"]) out = lessons_svc.lesson_to_dict(updated) out["systems"] = [ s.to_dict() diff --git a/src/scribe/routes/snippets.py b/src/scribe/routes/snippets.py index 5f9978f..ff28699 100644 --- a/src/scribe/routes/snippets.py +++ b/src/scribe/routes/snippets.py @@ -208,7 +208,8 @@ async def update_snippet_route(snippet_id: int): if updated is None: return not_found("Snippet") if data.get("system_ids") is not None: - await systems_svc.set_record_systems(owner_uid, snippet_id, data["system_ids"]) + # The CALLER, not owner_uid — see routes/lessons.py for the why (#4249). + await systems_svc.set_record_systems(uid, snippet_id, data["system_ids"]) out = snippets_svc.snippet_to_dict(updated) out["systems"] = [ s.to_dict() for s in await systems_svc.list_record_systems(owner_uid, snippet_id) diff --git a/tests/test_system_tagging_door_parity.py b/tests/test_system_tagging_door_parity.py index 8bb4fdd..80e9966 100644 --- a/tests/test_system_tagging_door_parity.py +++ b/tests/test_system_tagging_door_parity.py @@ -178,6 +178,60 @@ def test_the_registry_names_functions_that_exist(): assert not missing, f"registry names functions that no longer exist: {missing}" +def test_every_tagging_write_acts_as_the_caller(): + """`set_record_systems` is handed the ACTING user, never the record's owner. + + THE ARGUMENT. `set_record_systems` is not a dumb setter — it runs its own + `can_write_note` and then links only Systems the given user can READ. + Handing it `note.user_id` therefore does two things at once, both quiet: + the access check becomes trivially true (an owner can always write their + own record), and the System filter uses the owner's visibility instead of + the actor's. On a single-user install those are invisible. With a share + they are an editor acting with the owner's reach — the shape #47 exists to + prevent, and the same reasoning routes/notes.py already applies to + `set_supersedes`. + + It is NOT a permission loosening either way: every call site establishes + the caller's write access first (a route's `can_write_note`, or a service + that raises PermissionError). What this decides is WHOSE reach the tagging + runs with, which is exactly the kind of difference that survives review + because each call site reads fine on its own. + + Four of twenty sites passed the owner before #4249 — two routes, two MCP + tools, one of them written earlier in the very session that unified them. + That is the tell that this needs a guard rather than care. + """ + offenders = [] + for path in ROOT.rglob("*.py"): + src = path.read_text() + if "set_record_systems" not in src: + continue + tree = ast.parse(src) + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + fn = node.func + name = fn.attr if isinstance(fn, ast.Attribute) else getattr(fn, "id", None) + if name != "set_record_systems" or not node.args: + continue + first = node.args[0] + # The acting user arrives as a bare local — `uid` in the tools and + # routes, `user_id` in the service's own signature. An attribute + # access (`note.user_id`, `target.user_id`) is a record's owner, + # and `owner_uid` is the same thing already unpacked. + ok = isinstance(first, ast.Name) and first.id in {"uid", "user_id"} + if not ok: + shown = ast.unparse(first) if hasattr(ast, "unparse") else "?" + rel = path.relative_to(ROOT.parent.parent) + offenders.append(f"{rel}:{node.lineno} passes {shown!r}") + assert not offenders, ( + "set_record_systems must be called with the acting user, not the " + "record's owner — passing the owner bypasses its own access check and " + "borrows the owner's System visibility (#47, #4249):\n " + + "\n ".join(offenders) + ) + + def test_milestones_are_absent_on_purpose(): """The one 'gap' that is not a gap, pinned so it is not re-opened.