From bd88dd6807048bb09989754257bdaff762687f42 Mon Sep 17 00:00:00 2001 From: Alex Date: Wed, 1 Jul 2026 20:34:27 +0200 Subject: [PATCH] feat: scope artefacts on api --- application/api/user/artifacts/authz.py | 85 +++++++++--- application/api/user/artifacts/routes.py | 54 +++++--- application/api/user/tools/routes.py | 6 +- tests/agents/test_workflow_input_documents.py | 6 +- tests/api/user/test_artifacts_routes.py | 130 +++++++++++++----- 5 files changed, 204 insertions(+), 77 deletions(-) diff --git a/application/api/user/artifacts/authz.py b/application/api/user/artifacts/authz.py index d9441f4a..09d34d80 100644 --- a/application/api/user/artifacts/authz.py +++ b/application/api/user/artifacts/authz.py @@ -2,11 +2,13 @@ from __future__ import annotations +from dataclasses import dataclass from typing import Optional from flask import request from application.storage.db.repositories.agents import AgentsRepository +from application.storage.db.repositories.artifacts import ArtifactsRepository from application.storage.db.repositories.conversations import ConversationsRepository from application.storage.db.repositories.shared_conversations import ( SharedConversationsRepository, @@ -15,19 +17,46 @@ from application.storage.db.repositories.workflow_runs import WorkflowRunsReposi from application.storage.db.session import db_readonly -def resolve_authenticated_user() -> Optional[str]: - """Resolve the caller to a user id via decoded JWT ``sub`` or api_key→owner.""" +@dataclass(frozen=True) +class Principal: + """The resolved caller of an artifact route. + + A decoded JWT is a full *owner* session (``agent_id`` is ``None``). An + ``api_key`` is a low-trust, publicly-distributed *agent* credential -- it is + embedded client-side in the widget and accepted from the query string -- so + it resolves to the owning ``user_id`` but stays **agent-scoped**: it may only + reach artifacts from that agent's own conversations, never the owner's whole + corpus, and may not perform mutating operations. This mirrors the scoping the + MCP resource layer already enforces (see ``artifact_resource_service``). + """ + + user_id: Optional[str] = None + agent_id: Optional[str] = None + + @property + def is_agent_scoped(self) -> bool: + """True for an api_key (agent) principal that must be confined to its agent.""" + return self.agent_id is not None + + +def resolve_principal() -> Principal: + """Resolve the caller to a :class:`Principal`. + + Priority: a decoded JWT (owner session) first, then an ``api_key`` query/form + param (agent-scoped). An unresolvable caller yields an anonymous principal + (both fields ``None``) that only a valid ``share_token`` can authorize. + """ decoded_token = getattr(request, "decoded_token", None) if decoded_token: - return decoded_token.get("sub") + return Principal(user_id=decoded_token.get("sub")) api_key = request.args.get("api_key") or request.form.get("api_key") if api_key: with db_readonly() as conn: agent = AgentsRepository(conn).find_by_key(api_key) - if agent: - return agent.get("user_id") - return None + if agent and agent.get("user_id") and agent.get("id"): + return Principal(user_id=str(agent["user_id"]), agent_id=str(agent["id"])) + return Principal() def user_can_access_conversation( @@ -49,46 +78,62 @@ def user_can_access_conversation( return False -def authorize_artifact(conn, artifact: dict, user_id: Optional[str]) -> bool: - """Authorize an artifact by resolving its parent; missing parent fails closed.""" +def authorize_artifact(conn, artifact: dict, principal: Principal) -> bool: + """Authorize a READ of ``artifact`` for ``principal``; missing parent fails closed. + + A low-trust agent api_key is confined to its own agent's conversations (owner + match + agent scope) and never inherits share-link access. A JWT owner, + ``shared_with`` collaborator, or share-token holder is authorized by resolving + the artifact's parent (conversation or workflow run). + """ + if principal.is_agent_scoped: + # An agent key is not the owner's session: require both that the artifact + # belongs to the owner AND that its parent conversation is this agent's, + # so a public widget key cannot enumerate the owner's whole corpus. + if str(artifact.get("user_id")) != str(principal.user_id): + return False + return ArtifactsRepository(conn).artifact_in_agent_scope( + str(artifact.get("id")), str(principal.agent_id) + ) + conversation_id = artifact.get("conversation_id") workflow_run_id = artifact.get("workflow_run_id") share_token = request.args.get("share_token") if conversation_id is not None: return user_can_access_conversation( - conn, str(conversation_id), user_id, share_token + conn, str(conversation_id), principal.user_id, share_token ) if workflow_run_id is not None: - if not user_id: + if not principal.user_id: return False run = WorkflowRunsRepository(conn).get(str(workflow_run_id)) - return run is not None and run.get("user_id") == user_id + return run is not None and run.get("user_id") == principal.user_id # No parent row reachable -> deny (e.g. deleted conversation/run). return False -def authorize_artifact_write(conn, artifact: dict, user_id: Optional[str]) -> bool: - """Authorize a *mutating* artifact operation (e.g. restore). +def authorize_artifact_write(conn, artifact: dict, principal: Principal) -> bool: + """Authorize a *mutating* artifact operation (e.g. delete / restore). Stricter than :func:`authorize_artifact`: a write requires an authenticated - owner of the parent. Share links and ``shared_with`` collaborators inherit - read/download access only, so an anonymous (``user_id`` is ``None``) or - share-token-only caller is denied -- they can read an artifact but never - mutate it. + *owner* of the parent. Low-trust agent api_keys, share links, and + ``shared_with`` collaborators inherit read/download access only, so an + agent-scoped, anonymous, or share-token-only caller is denied -- they can read + an artifact but never mutate it. """ - if not user_id: + if principal.is_agent_scoped or not principal.user_id: return False conversation_id = artifact.get("conversation_id") workflow_run_id = artifact.get("workflow_run_id") if conversation_id is not None: return ( - ConversationsRepository(conn).get_owned(str(conversation_id), user_id) + ConversationsRepository(conn).get_owned(str(conversation_id), principal.user_id) is not None ) if workflow_run_id is not None: run = WorkflowRunsRepository(conn).get(str(workflow_run_id)) - return run is not None and run.get("user_id") == user_id + return run is not None and run.get("user_id") == principal.user_id # No parent row reachable -> deny (e.g. deleted conversation/run). return False diff --git a/application/api/user/artifacts/routes.py b/application/api/user/artifacts/routes.py index daf2d693..b1a7b217 100644 --- a/application/api/user/artifacts/routes.py +++ b/application/api/user/artifacts/routes.py @@ -20,7 +20,7 @@ from application.api import api from application.api.user.artifacts.authz import ( authorize_artifact, authorize_artifact_write, - resolve_authenticated_user, + resolve_principal, user_can_access_conversation, ) from application.core.settings import settings @@ -98,7 +98,8 @@ def _iso(value): class ListArtifacts(Resource): @api.doc(description="List artifacts for a conversation, workflow run, or the caller") def get(self): - user_id = resolve_authenticated_user() + principal = resolve_principal() + user_id = principal.user_id conversation_id = request.args.get("conversation_id") workflow_run_id = request.args.get("workflow_run_id") share_token = request.args.get("share_token") @@ -121,32 +122,45 @@ class ListArtifacts(Resource): try: with db_readonly() as conn: - if conversation_id: + repo = ArtifactsRepository(conn) + if principal.is_agent_scoped: + # A low-trust agent api_key only ever sees its OWN agent's + # artifacts (never the owner's whole corpus): scope to the + # agent, then optionally narrow to the requested conversation. + # Workflow runs are not agent-scoped, so reject that filter. + if workflow_run_id: + return make_response( + jsonify({"success": False, "message": "Forbidden"}), 403 + ) + rows = repo.list_artifacts_for_agent(principal.agent_id, user_id) + if conversation_id: + rows = [ + r + for r in rows + if str(r.get("conversation_id")) == str(conversation_id) + ] + elif conversation_id: if not user_can_access_conversation( conn, conversation_id, user_id, share_token ): return make_response( jsonify({"success": False, "message": "Forbidden"}), 403 ) - rows = ArtifactsRepository(conn).list_artifacts( - conversation_id=conversation_id - ) + rows = repo.list_artifacts(conversation_id=conversation_id) elif workflow_run_id: run = WorkflowRunsRepository(conn).get(workflow_run_id) if run is None or run.get("user_id") != user_id: return make_response( jsonify({"success": False, "message": "Forbidden"}), 403 ) - rows = ArtifactsRepository(conn).list_artifacts( - workflow_run_id=workflow_run_id - ) + rows = repo.list_artifacts(workflow_run_id=workflow_run_id) else: if not user_id: return make_response( jsonify({"success": False, "message": "Authentication required"}), 401, ) - rows = ArtifactsRepository(conn).list_artifacts(user_id=user_id) + rows = repo.list_artifacts(user_id=user_id) return make_response( jsonify( @@ -167,7 +181,7 @@ class GetArtifact(Resource): return make_response( jsonify({"success": False, "message": "Artifact not found"}), 404 ) - user_id = resolve_authenticated_user() + principal = resolve_principal() try: with db_readonly() as conn: repo = ArtifactsRepository(conn) @@ -176,7 +190,7 @@ class GetArtifact(Resource): return make_response( jsonify({"success": False, "message": "Artifact not found"}), 404 ) - if not authorize_artifact(conn, artifact, user_id): + if not authorize_artifact(conn, artifact, principal): return make_response( jsonify({"success": False, "message": "Forbidden"}), 403 ) @@ -197,7 +211,7 @@ class GetArtifact(Resource): return make_response( jsonify({"success": False, "message": "Artifact not found"}), 404 ) - user_id = resolve_authenticated_user() + principal = resolve_principal() try: with db_session() as conn: repo = ArtifactsRepository(conn) @@ -208,7 +222,7 @@ class GetArtifact(Resource): ) # Delete is a WRITE: only the parent owner may delete; share # links / read-only collaborators are denied (read access only). - if not authorize_artifact_write(conn, artifact, user_id): + if not authorize_artifact_write(conn, artifact, principal): return make_response( jsonify({"success": False, "message": "Forbidden"}), 403 ) @@ -247,7 +261,7 @@ class GetArtifactVersion(Resource): return make_response( jsonify({"success": False, "message": "Artifact not found"}), 404 ) - user_id = resolve_authenticated_user() + principal = resolve_principal() try: with db_readonly() as conn: repo = ArtifactsRepository(conn) @@ -256,7 +270,7 @@ class GetArtifactVersion(Resource): return make_response( jsonify({"success": False, "message": "Artifact not found"}), 404 ) - if not authorize_artifact(conn, artifact, user_id): + if not authorize_artifact(conn, artifact, principal): return make_response( jsonify({"success": False, "message": "Forbidden"}), 403 ) @@ -286,7 +300,7 @@ class DownloadArtifact(Resource): return make_response( jsonify({"success": False, "message": "Artifact not found"}), 404 ) - user_id = resolve_authenticated_user() + principal = resolve_principal() version_arg = request.args.get("version") try: with db_readonly() as conn: @@ -296,7 +310,7 @@ class DownloadArtifact(Resource): return make_response( jsonify({"success": False, "message": "Artifact not found"}), 404 ) - if not authorize_artifact(conn, artifact, user_id): + if not authorize_artifact(conn, artifact, principal): return make_response( jsonify({"success": False, "message": "Forbidden"}), 403 ) @@ -388,7 +402,7 @@ class RestoreArtifact(Resource): return make_response( jsonify({"success": False, "message": "Artifact not found"}), 404 ) - user_id = resolve_authenticated_user() + principal = resolve_principal() data = request.get_json(silent=True) or {} target_version = data.get("version") if target_version is None: @@ -413,7 +427,7 @@ class RestoreArtifact(Resource): # Restore is a WRITE (it appends a new current version); share # links / shared_with collaborators inherit read access only, so # gate on the stricter owner-required write check. - if not authorize_artifact_write(conn, artifact, user_id): + if not authorize_artifact_write(conn, artifact, principal): return make_response( jsonify({"success": False, "message": "Forbidden"}), 403 ) diff --git a/application/api/user/tools/routes.py b/application/api/user/tools/routes.py index 8c42c487..4c1f3f92 100644 --- a/application/api/user/tools/routes.py +++ b/application/api/user/tools/routes.py @@ -16,7 +16,7 @@ from application.agents.default_tools import ( from application.agents.tools.spec_parser import parse_spec from application.agents.tools.tool_manager import ToolManager from application.api import api -from application.api.user.artifacts.authz import authorize_artifact +from application.api.user.artifacts.authz import Principal, authorize_artifact from application.api.user.team_sharing import effective_write_owner, visible_with_access from application.core.url_validation import SSRFError, validate_url from application.security.encryption import decrypt_credentials, encrypt_credentials @@ -954,7 +954,9 @@ class GetArtifact(Resource): if looks_like_uuid(artifact_id): artifacts_repo = ArtifactsRepository(conn) artifact_doc = artifacts_repo.get_artifact(artifact_id) - if artifact_doc and authorize_artifact(conn, artifact_doc, user_id): + if artifact_doc and authorize_artifact( + conn, artifact_doc, Principal(user_id=user_id) + ): current = artifacts_repo.get_version( artifact_id, artifact_doc.get("current_version") ) diff --git a/tests/agents/test_workflow_input_documents.py b/tests/agents/test_workflow_input_documents.py index cb14d872..ebb5ea72 100644 --- a/tests/agents/test_workflow_input_documents.py +++ b/tests/agents/test_workflow_input_documents.py @@ -227,14 +227,14 @@ def test_shared_agent_run_and_artifacts_owned_by_caller(pg_engine, tmp_path, mon # share token, so it needs a request context. from flask import Flask - from application.api.user.artifacts.authz import authorize_artifact + from application.api.user.artifacts.authz import Principal, authorize_artifact app = Flask(__name__) with app.test_request_context(): with pg_engine.connect() as conn: artifact = ArtifactsRepository(conn).get_artifact(refs[0]["artifact_id"]) - assert authorize_artifact(conn, artifact, RUNNER) is True - assert authorize_artifact(conn, artifact, OWNER) is False + assert authorize_artifact(conn, artifact, Principal(user_id=RUNNER)) is True + assert authorize_artifact(conn, artifact, Principal(user_id=OWNER)) is False def test_code_state_excludes_chat_history(pg_engine, tmp_path, monkeypatch): diff --git a/tests/api/user/test_artifacts_routes.py b/tests/api/user/test_artifacts_routes.py index 22638dcb..5ba5df26 100644 --- a/tests/api/user/test_artifacts_routes.py +++ b/tests/api/user/test_artifacts_routes.py @@ -10,6 +10,8 @@ import pytest from flask import request from sqlalchemy import text +from application.api.user.artifacts import authz +from application.storage.db.repositories.agents import AgentsRepository from application.storage.db.repositories.artifacts import ArtifactsRepository from application.storage.db.repositories.conversations import ConversationsRepository from application.storage.db.repositories.shared_conversations import ( @@ -53,6 +55,31 @@ def _make_workflow(conn, user_id=OWNER): return str(res.fetchone()[0]) +def _make_agent(conn, user_id=OWNER, key="secret-key"): + return AgentsRepository(conn).create(user_id, "widget-agent", "active", key=key) + + +def _make_agent_conversation(conn, agent_id, user_id=OWNER): + return ConversationsRepository(conn).create( + user_id, name="conv", agent_id=str(agent_id) + ) + + +def _wire_api_key(monkeypatch, conn): + """Point ``resolve_principal``'s own readonly conn at the test conn. + + ``resolve_principal`` opens ``db_readonly`` inside authz to resolve the + api_key via the real ``find_by_key``, so it must see the seeded agent row. + """ + from contextlib import contextmanager as _cm + + @_cm + def _use_conn(): + yield conn + + monkeypatch.setattr(authz, "db_readonly", _use_conn) + + def _make_artifact(conn, **kwargs): return ArtifactsRepository(conn).create_artifact( kwargs.pop("user_id", OWNER), @@ -646,33 +673,20 @@ class TestMalformedArtifactId: # --------------------------------------------------------------------------- -# api_key -> agent-owner principal resolution +# api_key -> agent-scoped principal (low-trust, widget-embedded credential) # --------------------------------------------------------------------------- @pytest.mark.unit class TestApiKeyPrincipal: - def test_api_key_owner_can_get_owned_artifact( + def test_api_key_reads_artifact_in_its_agent_scope( self, _patch_db, flask_app, monkeypatch ): - from application.api.user.artifacts import authz from application.api.user.artifacts.routes import GetArtifact - from application.storage.db.repositories.agents import AgentsRepository - conv = _make_conversation(_patch_db, user_id=OWNER) + agent = _make_agent(_patch_db) + conv = _make_agent_conversation(_patch_db, agent["id"]) art = _make_artifact(_patch_db, conversation_id=str(conv["id"])) + _wire_api_key(monkeypatch, _patch_db) - # The api_key path opens its own readonly conn inside authz; point it at - # the test conn and resolve the key to the artifact's owning agent. - @contextmanager - def _use_conn(): - yield _patch_db - - monkeypatch.setattr(authz, "db_readonly", _use_conn) - monkeypatch.setattr( - AgentsRepository, "find_by_key", - lambda self, key: {"user_id": OWNER} if key == "secret-key" else None, - ) - - # No JWT -> principal must be resolved from the api_key query param. resp = _call( flask_app, GetArtifact, art["id"], token=None, query={"api_key": "secret-key"}, @@ -680,25 +694,77 @@ class TestApiKeyPrincipal: assert resp.status_code == 200 assert resp.json["artifact"]["id"] == str(art["id"]) + def test_api_key_cannot_read_owner_artifact_outside_agent_scope( + self, _patch_db, flask_app, monkeypatch + ): + # Regression (critical IDOR): a public widget key resolves to the owner + # but must NOT reach an artifact from the owner's OTHER (non-agent) + # conversation -- it is confined to its own agent's conversations. + from application.api.user.artifacts.routes import GetArtifact + + _make_agent(_patch_db) # the widget agent (key=secret-key) + other_conv = _make_conversation(_patch_db, user_id=OWNER) # no agent_id + art = _make_artifact(_patch_db, conversation_id=str(other_conv["id"])) + _wire_api_key(monkeypatch, _patch_db) + + resp = _call( + flask_app, GetArtifact, art["id"], token=None, + query={"api_key": "secret-key"}, + ) + assert resp.status_code == 403 + + def test_api_key_list_is_scoped_to_agent_not_whole_corpus( + self, _patch_db, flask_app, monkeypatch + ): + # Regression (critical IDOR): the no-filter list must return only the + # agent's own conversations' artifacts, never the owner's whole corpus. + from application.api.user.artifacts.routes import ListArtifacts + + agent = _make_agent(_patch_db) + agent_conv = _make_agent_conversation(_patch_db, agent["id"]) + in_scope = _make_artifact(_patch_db, conversation_id=str(agent_conv["id"])) + other_conv = _make_conversation(_patch_db, user_id=OWNER) + out_of_scope = _make_artifact(_patch_db, conversation_id=str(other_conv["id"])) + _wire_api_key(monkeypatch, _patch_db) + + resp = _call( + flask_app, ListArtifacts, token=None, query={"api_key": "secret-key"}, + ) + assert resp.status_code == 200 + ids = {a["id"] for a in resp.json["artifacts"]} + assert str(in_scope["id"]) in ids + assert str(out_of_scope["id"]) not in ids + + def test_api_key_cannot_delete_even_in_scope( + self, _patch_db, flask_app, monkeypatch + ): + # Mutations require a JWT owner; a low-trust agent key may never delete, + # even an artifact inside its own agent scope. + from application.api.user.artifacts.routes import GetArtifact + + agent = _make_agent(_patch_db) + conv = _make_agent_conversation(_patch_db, agent["id"]) + art = _make_artifact(_patch_db, conversation_id=str(conv["id"])) + _wire_api_key(monkeypatch, _patch_db) + + resp = _call( + flask_app, GetArtifact, art["id"], token=None, + query={"api_key": "secret-key"}, method="delete", + ) + assert resp.status_code == 403 + assert ArtifactsRepository(_patch_db).get_artifact(art["id"]) is not None + def test_api_key_resolving_to_stranger_denied( self, _patch_db, flask_app, monkeypatch ): - from application.api.user.artifacts import authz + # A key owned by a different user cannot reach this owner's artifact. from application.api.user.artifacts.routes import GetArtifact - from application.storage.db.repositories.agents import AgentsRepository - conv = _make_conversation(_patch_db, user_id=OWNER) - art = _make_artifact(_patch_db, conversation_id=str(conv["id"])) - - @contextmanager - def _use_conn(): - yield _patch_db - - monkeypatch.setattr(authz, "db_readonly", _use_conn) - monkeypatch.setattr( - AgentsRepository, "find_by_key", - lambda self, key: {"user_id": STRANGER}, - ) + owner_conv = _make_conversation(_patch_db, user_id=OWNER) + art = _make_artifact(_patch_db, conversation_id=str(owner_conv["id"])) + stranger_agent = _make_agent(_patch_db, user_id=STRANGER, key="stranger-key") + _make_agent_conversation(_patch_db, stranger_agent["id"], user_id=STRANGER) + _wire_api_key(monkeypatch, _patch_db) resp = _call( flask_app, GetArtifact, art["id"], token=None,