From 20d92a28f50884703e835a4497af0695b7d77c7f Mon Sep 17 00:00:00 2001 From: Hari John Kuriakose Date: Sat, 25 Jul 2026 17:29:14 +0530 Subject: [PATCH 01/14] [FEAT] Allow unpublishing an exported tool; derive API-key target from the URL Two API ergonomics fixes. 1. No way to delete an exported registry tool The registry was read-only over the API - `prompt_studio_registry_v2/urls.py` mapped only `{"get": "list"}`. The sole way to remove an entry was to delete the Prompt Studio project, which cascades to it. That works, but it is implicit and undocumented, and it is a blunt instrument: there was no way to unpublish a tool while keeping the project. Adds `DELETE registry//`, guarded by the same in-use check `prompt-studio delete` performs - a tool still attached to a workflow is refused with 409 rather than silently breaking those workflows. The guard filters `ToolInstance` on `tool_id=instance.pk`, matching the existing check in `prompt_studio_core_v2/views.py`, where an exported tool's `tool_id` is its `prompt_registry_id`. `get_queryset` previously returned `None` when no query-param filters were present, which would break `get_object()` on a detail route. It now returns the full queryset when addressing a single row by PK. Keyed off the URL kwarg rather than `self.detail`, which DRF only populates for router-generated views and leaves as `None` under a manual `as_view()` - the wiring used here. 2. API-key creation wanted an identifier already present in the URL `POST keys/api//` took `api_id` as a path segment but also expected `api` in the body - the same value spelled twice. Omitting the body field failed validation. `POST keys/pipeline//` had the identical shape. POST routed to the default `ModelViewSet.create`, which never sees the URL kwargs, so the body had to repeat them. `create` now derives the target from the path when present. It uses `setdefault`, so an explicit body value still wins, and falls through to the default implementation for the body-only routes (`keys/api/`, `keys/pipeline/`) - both remain working. Note: making a no-RAG profile (`chunk_size=0`) omit the vector DB and embedding model is deliberately NOT included here; it is not an ergonomics-sized change. See the PR description. Co-Authored-By: Claude Opus 4.8 (1M context) --- backend/api_v2/api_key_views.py | 29 ++++++++++++++- .../prompt_studio_registry_v2/exceptions.py | 8 +++++ .../prompt_studio_registry_v2/urls.py | 5 +++ .../prompt_studio_registry_v2/views.py | 36 +++++++++++++++++++ 4 files changed, 77 insertions(+), 1 deletion(-) diff --git a/backend/api_v2/api_key_views.py b/backend/api_v2/api_key_views.py index 9f6b8532e0..275491e5fa 100644 --- a/backend/api_v2/api_key_views.py +++ b/backend/api_v2/api_key_views.py @@ -3,7 +3,7 @@ from permissions.permission import IsOwnerOrSharedUser, IsParentDeploymentOwner from pipeline_v2.exceptions import PipelineNotFound from pipeline_v2.pipeline_processor import PipelineProcessor -from rest_framework import serializers, viewsets +from rest_framework import serializers, status, viewsets from rest_framework.decorators import action from rest_framework.request import Request from rest_framework.response import Response @@ -33,6 +33,33 @@ def get_serializer_class(self) -> serializers.Serializer: return APIKeyListSerializer return APIKeySerializer + def create(self, request: Request, *args: Any, **kwargs: Any) -> Response: + """Create an API key, deriving the target from the URL. + + `POST keys/api//` and `POST keys/pipeline//` + already name the resource in the path, so callers should not have to + repeat it in the body. Fall back to whatever the body carries, keeping + the body-only routes (`keys/api/`, `keys/pipeline/`) working. + """ + api_id = kwargs.get("api_id") + pipeline_id = kwargs.get("pipeline_id") + + if api_id or pipeline_id: + request_data = request.data.copy() + if api_id: + request_data.setdefault("api", api_id) + if pipeline_id: + request_data.setdefault("pipeline", pipeline_id) + serializer = self.get_serializer(data=request_data) + serializer.is_valid(raise_exception=True) + self.perform_create(serializer) + headers = self.get_success_headers(serializer.data) + return Response( + serializer.data, status=status.HTTP_201_CREATED, headers=headers + ) + + return super().create(request, *args, **kwargs) + @action(detail=True, methods=["get"]) def api_keys( self, diff --git a/backend/prompt_studio/prompt_studio_registry_v2/exceptions.py b/backend/prompt_studio/prompt_studio_registry_v2/exceptions.py index cb0e3c71ba..5c01902a07 100644 --- a/backend/prompt_studio/prompt_studio_registry_v2/exceptions.py +++ b/backend/prompt_studio/prompt_studio_registry_v2/exceptions.py @@ -31,3 +31,11 @@ class InValidCustomToolError(APIException): "This prompt studio project cannot be exported. It probably " "has some empty or unexecuted prompts." ) + + +class RegistryToolInUseError(APIException): + status_code = 409 + default_detail = ( + "This exported tool is still used by one or more workflows. " + "Remove those usages before deleting it." + ) diff --git a/backend/prompt_studio/prompt_studio_registry_v2/urls.py b/backend/prompt_studio/prompt_studio_registry_v2/urls.py index b2c3032891..86f0760da8 100644 --- a/backend/prompt_studio/prompt_studio_registry_v2/urls.py +++ b/backend/prompt_studio/prompt_studio_registry_v2/urls.py @@ -9,6 +9,11 @@ PromptStudioRegistryView.as_view({"get": "list"}), name="prompt_studio_registry_list", ), + path( + "registry//", + PromptStudioRegistryView.as_view({"delete": "destroy"}), + name="prompt_studio_registry_detail", + ), ] # Optional: Apply format suffix patterns diff --git a/backend/prompt_studio/prompt_studio_registry_v2/views.py b/backend/prompt_studio/prompt_studio_registry_v2/views.py index 0b4e64e763..c9d6c56e10 100644 --- a/backend/prompt_studio/prompt_studio_registry_v2/views.py +++ b/backend/prompt_studio/prompt_studio_registry_v2/views.py @@ -1,8 +1,12 @@ import logging +from typing import Any from django.db.models import QuerySet from rest_framework import viewsets +from rest_framework.request import Request +from rest_framework.response import Response from rest_framework.versioning import URLPathVersioning +from tool_instance_v2.models import ToolInstance from utils.filtering import FilterHelper from prompt_studio.prompt_studio_registry_v2.constants import PromptStudioRegistryKeys @@ -10,6 +14,7 @@ PromptStudioRegistrySerializer, ) +from .exceptions import RegistryToolInUseError from .models import PromptStudioRegistry logger = logging.getLogger(__name__) @@ -24,6 +29,13 @@ class PromptStudioRegistryView(viewsets.ModelViewSet): serializer_class = PromptStudioRegistrySerializer def get_queryset(self) -> QuerySet | None: + # Detail routes address a single row by PK; the list filters below are + # query-param driven and would resolve to None, breaking get_object(). + # Keyed off the URL kwarg rather than `self.detail`, which DRF only + # populates for router-generated views (it is None under as_view()). + if self.kwargs.get("pk"): + return PromptStudioRegistry.objects.all() + filterArgs = FilterHelper.build_filter_args( self.request, PromptStudioRegistryKeys.PROMPT_REGISTRY_ID, @@ -34,3 +46,27 @@ def get_queryset(self) -> QuerySet | None: queryset = PromptStudioRegistry.objects.filter(**filterArgs) return queryset + + def destroy( + self, request: Request, *args: tuple[Any], **kwargs: dict[str, Any] + ) -> Response: + """Unpublish an exported tool without deleting its Prompt Studio project. + + Deleting the project cascades to its registry entry, but that is a blunt + instrument - it gives no way to unpublish a tool while keeping the + project. Guarded by the same in-use check `prompt-studio delete` + performs, so a tool still attached to a workflow is refused. + """ + instance: PromptStudioRegistry = self.get_object() + dependent_wfs = set( + ToolInstance.objects.filter(tool_id=instance.pk) + .values_list("workflow_id", flat=True) + .distinct() + ) + if dependent_wfs: + logger.info( + f"Cannot delete exported tool {instance.prompt_registry_id}, " + f"depended by workflows {dependent_wfs}" + ) + raise RegistryToolInUseError() + return super().destroy(request, *args, **kwargs) From cae9099465efa7ae009a0a08bc6d0c8f1d7ccade Mon Sep 17 00:00:00 2001 From: Hari John Kuriakose Date: Fri, 24 Jul 2026 16:00:50 +0530 Subject: [PATCH 02/14] [FIX] Restrict registry tool deletion to the project's owner The new `DELETE registry//` route resolved its object from `PromptStudioRegistry.objects.all()`, and the viewset carried no permission classes (`DEFAULT_PERMISSION_CLASSES` is empty). `OrganizationFilterBackend` runs inside `get_object()` via `filter_queryset`, so cross-org deletion was already impossible - but any member of the same organization could delete any other member's exported tool by PK. Adds `IsRegistryToolOwner`, gating only the `destroy` action. Ownership is inherited from the linked `CustomTool`, mirroring `IsParentToolOwner` (which does the same for `ProfileManager`), with a fallback to the row's own owner for unlinked legacy rows since `custom_tool` is nullable. Service accounts and org admins are admitted, matching the sibling permission classes. Read access is deliberately left broader - `list` visibility is still derived by `PromptStudioRegistry.objects.list_tools`, unchanged. Only the destructive route is restricted. Lives in `prompt_studio/permission.py` next to `PromptAcesssToUser` rather than in the view module, matching where the app's other permission classes live. Co-Authored-By: Claude Opus 4.8 (1M context) --- backend/prompt_studio/permission.py | 23 +++++++++++++++++++ .../prompt_studio_registry_v2/views.py | 8 +++++++ 2 files changed, 31 insertions(+) diff --git a/backend/prompt_studio/permission.py b/backend/prompt_studio/permission.py index 6f24418988..43eb9c75da 100644 --- a/backend/prompt_studio/permission.py +++ b/backend/prompt_studio/permission.py @@ -31,3 +31,26 @@ def has_object_permission(self, request: Request, view: APIView, obj: Any) -> bo if has_group_access(request.user, tool): return True return OrganizationMemberService.is_user_organization_admin(request.user) + + +class IsRegistryToolOwner(permissions.BasePermission): + """Is unpublishing an exported tool allowed to user. + + A ``PromptStudioRegistry`` row is not itself a membership resource, so + ownership is inherited from the linked ``CustomTool`` -- mirroring + ``IsParentToolOwner``, which does the same for ``ProfileManager``. Falls + back to the row's own owner for unlinked legacy rows (``custom_tool`` is + nullable). + + Read access is deliberately broader (see + ``PromptStudioRegistry.objects.list_tools``); deleting is restricted to + owners and org admins. + """ + + def has_object_permission(self, request: Request, view: APIView, obj: Any) -> bool: + if getattr(request.user, "is_service_account", False): + return True + owner_resource = obj.custom_tool or obj + if _is_resource_owner(request.user, owner_resource): + return True + return OrganizationMemberService.is_user_organization_admin(request.user) diff --git a/backend/prompt_studio/prompt_studio_registry_v2/views.py b/backend/prompt_studio/prompt_studio_registry_v2/views.py index c9d6c56e10..f406ec1b5a 100644 --- a/backend/prompt_studio/prompt_studio_registry_v2/views.py +++ b/backend/prompt_studio/prompt_studio_registry_v2/views.py @@ -9,6 +9,7 @@ from tool_instance_v2.models import ToolInstance from utils.filtering import FilterHelper +from prompt_studio.permission import IsRegistryToolOwner from prompt_studio.prompt_studio_registry_v2.constants import PromptStudioRegistryKeys from prompt_studio.prompt_studio_registry_v2.serializers import ( PromptStudioRegistrySerializer, @@ -28,6 +29,13 @@ class PromptStudioRegistryView(viewsets.ModelViewSet): versioning_class = URLPathVersioning serializer_class = PromptStudioRegistrySerializer + def get_permissions(self) -> list[Any]: + # `list` stays as it was - visibility is already derived by + # `list_tools`. Only the destructive detail route is gated. + if self.action == "destroy": + return [IsRegistryToolOwner()] + return super().get_permissions() + def get_queryset(self) -> QuerySet | None: # Detail routes address a single row by PK; the list filters below are # query-param driven and would resolve to None, breaking get_object(). From facf9c7021e668624483017eb1609142d1a63678 Mon Sep 17 00:00:00 2001 From: Hari John Kuriakose Date: Fri, 24 Jul 2026 21:15:08 +0530 Subject: [PATCH 03/14] [TEST] Pin the authorization and in-use guards on registry tool deletion Regression tests for the two gates on `DELETE registry//`. The authorization test is the important one: it fails if `IsRegistryToolOwner` is loosened. The viewset carries no `permission_classes` and `DEFAULT_PERMISSION_CLASSES` is empty, so without that gate any member of an organization could delete another member's exported tool by PK. `OrganizationFilterBackend` blocks cross-org access inside `get_object()`, but not intra-org - which is exactly the case asserted here. Exercises the real `has_object_permission` body against stubbed collaborators, since Django is not importable in a plain checkout. Mirrors `prompt_studio_core_v2/tests/test_build_index_payload.py`. Coverage: - the project owner may delete - another org member may NOT delete (the IDOR this guard closes) - org admins and service accounts may delete - ownership follows the parent `custom_tool`, not the registry row, so a stale export-time owner cannot outrank the project's current owner - unlinked legacy rows (`custom_tool` is nullable) fall back to their own owner rather than becoming undeletable or world-deletable - an in-use tool is refused and an unused one is not - `RegistryToolInUseError` is a 409, not a 500 like the neighbouring `ToolDeleteError`, since the condition is caller-correctable Verified by mutation: making the gate unconditionally permissive fails the non-owner, parent-ownership, and legacy-row assertions. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../tests/test_registry_tool_delete_guards.py | 214 ++++++++++++++++++ 1 file changed, 214 insertions(+) create mode 100644 backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py diff --git a/backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py b/backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py new file mode 100644 index 0000000000..cbaf1f0ae6 --- /dev/null +++ b/backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py @@ -0,0 +1,214 @@ +"""Regression tests for the two guards on ``DELETE registry//``. + +The route added in this PR is the first by-PK operation on +``PromptStudioRegistryView``, which previously exposed only ``list``. Two +things gate it, and both are load-bearing: + +1. **Authorization** (``IsRegistryToolOwner``). The viewset carries no + ``permission_classes`` and ``DEFAULT_PERMISSION_CLASSES`` is empty, so + without this every member of an organization could delete any other + member's exported tool by PK. ``OrganizationFilterBackend`` runs inside + ``get_object()`` and blocks *cross-org* access, but not intra-org. + +2. **In-use refusal** (409). An exported tool still attached to a workflow must + not be deletable, or those workflows break. + +``has_object_permission`` is pure logic over collaborators, so these tests stub +the Django-coupled boundary (``permissions.permission``, +``OrganizationMemberService``) and exercise the real method body. Django is not +importable in a plain checkout, so the class body is extracted from source -- +mirroring ``prompt_studio_core_v2/tests/test_build_index_payload.py``. A rename +fails these tests rather than silently skipping them. + +The in-use check is asserted against the same predicate the view applies +(a non-empty set of dependent workflow IDs raises), without standing up the ORM. +""" + +from __future__ import annotations + +import textwrap +from pathlib import Path +from typing import Any + +import pytest + +BACKEND_DIR = Path(__file__).resolve().parents[3] +PERMISSION_MODULE = BACKEND_DIR / "prompt_studio" / "permission.py" + +START_MARKER = "class IsRegistryToolOwner(permissions.BasePermission):" + + +class _User: + def __init__(self, name: str, is_service_account: bool = False) -> None: + self.name = name + self.is_service_account = is_service_account + + def __repr__(self) -> str: # pragma: no cover - debugging aid + return f"" + + +class _CustomTool: + """Stand-in for the parent Prompt Studio project.""" + + def __init__(self, owner: _User) -> None: + self.owner = owner + + +class _RegistryRow: + """Stand-in for a ``PromptStudioRegistry`` row. + + ``custom_tool`` is nullable, so it may be ``None`` for legacy rows exported + before the link existed; those fall back to the row's own owner. + """ + + def __init__(self, custom_tool: _CustomTool | None, owner: _User) -> None: + self.custom_tool = custom_tool + self.owner = owner + + +class _Request: + def __init__(self, user: _User) -> None: + self.user = user + + +def _build_permission(*, org_admins: set[str]) -> Any: + """Extract the real ``IsRegistryToolOwner`` against stubbed collaborators.""" + source = PERMISSION_MODULE.read_text() + if START_MARKER not in source: + pytest.fail( + f"Could not find {START_MARKER!r} in {PERMISSION_MODULE}. If it was " + "renamed, update this test rather than deleting it." + ) + body = textwrap.dedent(source[source.index(START_MARKER) :]) + + class _BasePermission: + pass + + class _Permissions: + BasePermission = _BasePermission + + def _is_resource_owner(user: _User, obj: Any) -> bool: + return getattr(obj, "owner", None) is user + + class _OrganizationMemberService: + @staticmethod + def is_user_organization_admin(user: _User) -> bool: + return user.name in org_admins + + namespace: dict[str, Any] = { + "permissions": _Permissions, + "_is_resource_owner": _is_resource_owner, + "OrganizationMemberService": _OrganizationMemberService, + "Request": object, + "APIView": object, + "Any": Any, + } + exec(compile(body, str(PERMISSION_MODULE), "exec"), namespace) + return namespace["IsRegistryToolOwner"]() + + +OWNER = _User("owner") +STRANGER = _User("stranger") +ADMIN = _User("admin") +SERVICE = _User("service", is_service_account=True) + + +def _linked_row(owner: _User = OWNER) -> _RegistryRow: + """A normal row whose parent project is owned by ``owner``.""" + return _RegistryRow(custom_tool=_CustomTool(owner=owner), owner=_User("unused")) + + +class TestRegistryToolDeleteAuthorization: + def test_project_owner_may_delete(self) -> None: + permission = _build_permission(org_admins=set()) + assert ( + permission.has_object_permission(_Request(OWNER), None, _linked_row()) is True + ) + + def test_other_org_member_may_not_delete(self) -> None: + """The IDOR this guard exists to close. + + Org filtering already blocks cross-org access; this covers a member of + the *same* org who does not own the project. + """ + permission = _build_permission(org_admins=set()) + + allowed = permission.has_object_permission( + _Request(STRANGER), None, _linked_row() + ) + + assert allowed is False, ( + "A non-owner in the same organization must not be able to delete " + "another member's exported tool" + ) + + def test_org_admin_may_delete(self) -> None: + permission = _build_permission(org_admins={"admin"}) + assert ( + permission.has_object_permission(_Request(ADMIN), None, _linked_row()) is True + ) + + def test_service_account_may_delete(self) -> None: + permission = _build_permission(org_admins=set()) + assert ( + permission.has_object_permission(_Request(SERVICE), None, _linked_row()) + is True + ) + + def test_ownership_follows_the_parent_project_not_the_row(self) -> None: + """Ownership is inherited from ``custom_tool``, mirroring IsParentToolOwner. + + The row's own ``owner`` must be ignored while a parent exists, otherwise + a stale export-time owner could outrank the project's current owner. + """ + row = _RegistryRow(custom_tool=_CustomTool(owner=OWNER), owner=STRANGER) + permission = _build_permission(org_admins=set()) + + assert permission.has_object_permission(_Request(STRANGER), None, row) is False + assert permission.has_object_permission(_Request(OWNER), None, row) is True + + def test_unlinked_legacy_row_falls_back_to_its_own_owner(self) -> None: + """``custom_tool`` is nullable; those rows must stay deletable by their owner.""" + row = _RegistryRow(custom_tool=None, owner=OWNER) + permission = _build_permission(org_admins=set()) + + assert permission.has_object_permission(_Request(OWNER), None, row) is True + assert permission.has_object_permission(_Request(STRANGER), None, row) is False + + +class TestRegistryToolInUseRefusal: + """The 409 guard: a tool attached to a workflow must not be deleted. + + Mirrors the predicate in ``PromptStudioRegistryView.destroy`` -- a non-empty + set of dependent workflow IDs refuses the delete. + """ + + @staticmethod + def _refuses(dependent_workflow_ids: set[str]) -> bool: + return bool(dependent_workflow_ids) + + def test_tool_used_by_a_workflow_is_refused(self) -> None: + assert self._refuses({"workflow-1"}) is True + + def test_unused_tool_is_deletable(self) -> None: + assert self._refuses(set()) is False + + def test_in_use_error_is_a_409(self) -> None: + """Deleting an in-use tool is a conflict, not a server error. + + The neighbouring ``ToolDeleteError`` is a 500; this must not be modelled + on it, since the condition is caller-correctable. + """ + source = ( + BACKEND_DIR / "prompt_studio" / "prompt_studio_registry_v2" / "exceptions.py" + ).read_text() + + assert "class RegistryToolInUseError" in source, ( + "RegistryToolInUseError is missing; the in-use guard has no way to " + "signal a conflict" + ) + body = source[source.index("class RegistryToolInUseError") :] + assert "status_code = 409" in body.split("class ")[1], ( + "RegistryToolInUseError must be a 409 so callers can distinguish a " + "correctable conflict from a server fault" + ) From bfde0813a5e1f610c376857d22bcaa2b765ceecf Mon Sep 17 00:00:00 2001 From: Hari John Kuriakose Date: Fri, 31 Jul 2026 16:05:07 +0530 Subject: [PATCH 04/14] [FIX] Close the API-key create IDOR; make the registry 409 actionable Addresses the review on #2206. `create` performed no ownership check on the target deployment. DRF resolves `IsParentDeploymentOwner` for it, but `create` is collection-level -- DRF never calls `get_object()`, so `has_object_permission` never ran and any authenticated org member could mint a live key for a deployment they do not own. The view now object-checks the path target itself. (Pre-existing; the body-based path had the same gap. Closed here because this is the method where the fix belongs.) `IsParentDeploymentOwner` had to change to accept the parent directly: neither `APIDeployment` nor `Pipeline` declares an `api` or `pipeline` field (`APIKey.api` points *at* the deployment, `related_name="api_keys"`), so the bare `obj.api` in the reviewer's suggested snippet raises `AttributeError` -> 500 on every key creation. The lookups are now `getattr` guarded; a test pins the regression. The path target is also made authoritative rather than a `setdefault`: a body naming the *other* target is a contradiction and is refused with a 400 instead of producing a key for whichever one wins. That also disposes of two edges flagged in review -- a JSON array body now 400s rather than `AttributeError`-ing into a 500, and `{"api": ""}` no longer defeats the derivation into a confusing 400. The registry 409 now names the blocking deployment types, mirroring `prompt_studio_core_v2/views.py:249`, so the refusal is actionable rather than just telling the caller "no". Tests: the in-use guard was asserted against a restated `bool(ids)` predicate, which passed regardless of what the view did. It now drives the real method bodies extracted from `views.py`; verified by mutation (neutering the raise, dropping a deployment-type branch, or breaking the workflow query all fail the suite). Django settings are unavailable in the unit tier, so the source-extraction technique already used in this package is retained; route binding and the live permission cycle need a database and remain integration-tier. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014PWpGFA4Z5qktUj5DW2oiK --- backend/api_v2/api_key_views.py | 62 +++-- .../tests/test_api_key_create_target.py | 229 ++++++++++++++++++ backend/permissions/permission.py | 12 +- .../tests/test_registry_tool_delete_guards.py | 191 ++++++++++++++- .../prompt_studio_registry_v2/views.py | 95 +++++++- 5 files changed, 557 insertions(+), 32 deletions(-) create mode 100644 backend/api_v2/tests/test_api_key_create_target.py diff --git a/backend/api_v2/api_key_views.py b/backend/api_v2/api_key_views.py index 275491e5fa..d4266c2f92 100644 --- a/backend/api_v2/api_key_views.py +++ b/backend/api_v2/api_key_views.py @@ -38,27 +38,57 @@ def create(self, request: Request, *args: Any, **kwargs: Any) -> Response: `POST keys/api//` and `POST keys/pipeline//` already name the resource in the path, so callers should not have to - repeat it in the body. Fall back to whatever the body carries, keeping - the body-only routes (`keys/api/`, `keys/pipeline/`) working. + repeat it in the body. The body-only routes (`keys/api/`, + `keys/pipeline/`) fall through to the default implementation. + + The path target is authoritative: a body naming the *other* target is + a contradiction, not an override, and is refused rather than silently + creating a key for whichever one wins. Ownership of the target is + checked here because `create` is collection-level -- DRF resolves + `IsParentDeploymentOwner` for it but never calls `get_object()`, so + `has_object_permission` would otherwise never run and any org member + could mint a live key for a deployment they do not own. """ api_id = kwargs.get("api_id") pipeline_id = kwargs.get("pipeline_id") - if api_id or pipeline_id: - request_data = request.data.copy() - if api_id: - request_data.setdefault("api", api_id) - if pipeline_id: - request_data.setdefault("pipeline", pipeline_id) - serializer = self.get_serializer(data=request_data) - serializer.is_valid(raise_exception=True) - self.perform_create(serializer) - headers = self.get_success_headers(serializer.data) - return Response( - serializer.data, status=status.HTTP_201_CREATED, headers=headers - ) + if not (api_id or pipeline_id): + return super().create(request, *args, **kwargs) + + # A JSON array (or scalar) body has no `.copy()` returning a mapping; + # reject it as a 400 rather than letting `AttributeError` become a 500. + if not isinstance(request.data, dict): + raise serializers.ValidationError("Request body must be a JSON object.") + request_data = request.data.copy() + + if api_id: + if request_data.get("pipeline"): + raise serializers.ValidationError( + "This endpoint creates a key for the API deployment named " + "in the URL; remove `pipeline` from the body." + ) + api = DeploymentHelper.get_api_by_id(api_id=api_id) + if not api: + raise APINotFound() + self.check_object_permissions(request, api) + request_data["api"] = api_id + else: + if request_data.get("api"): + raise serializers.ValidationError( + "This endpoint creates a key for the pipeline named in the " + "URL; remove `api` from the body." + ) + pipeline = PipelineProcessor.get_active_pipeline(pipeline_id=pipeline_id) + if not pipeline: + raise PipelineNotFound() + self.check_object_permissions(request, pipeline) + request_data["pipeline"] = pipeline_id - return super().create(request, *args, **kwargs) + serializer = self.get_serializer(data=request_data) + serializer.is_valid(raise_exception=True) + self.perform_create(serializer) + headers = self.get_success_headers(serializer.data) + return Response(serializer.data, status=status.HTTP_201_CREATED, headers=headers) @action(detail=True, methods=["get"]) def api_keys( diff --git a/backend/api_v2/tests/test_api_key_create_target.py b/backend/api_v2/tests/test_api_key_create_target.py new file mode 100644 index 0000000000..4792b14f33 --- /dev/null +++ b/backend/api_v2/tests/test_api_key_create_target.py @@ -0,0 +1,229 @@ +"""Guards on ``APIKeyViewSet.create`` for the path-derived target. + +``POST keys/api//`` and ``POST keys/pipeline//`` name the +target in the URL. Deriving it there removed the need to repeat it in the body, +but ``create`` is a *collection*-level action: DRF resolves +``IsParentDeploymentOwner`` for it and then never calls ``get_object()``, so +``has_object_permission`` never ran. Any authenticated org member could mint a +live key for a deployment they do not own. The view now performs the object +check itself. + +Two things are pinned here, both cheap to break: + +1. **``IsParentDeploymentOwner`` accepts the parent itself.** The view hands it + an ``APIDeployment``/``Pipeline``, neither of which declares an ``api`` or + ``pipeline`` field. A plain ``obj.api`` raises ``AttributeError`` -> 500 on + every key creation; the lookups must be ``getattr`` guarded. + +2. **The path target is authoritative.** A body naming the *other* target is a + contradiction and must be refused, not silently resolved to whichever wins. + +These are unit tests over the real method bodies -- Django settings are not +configured in the unit tier, so collaborators are stubbed and the source is +extracted, mirroring +``prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py``. The +end-to-end request cycle needs a database and lives in the integration tier. +""" + +from __future__ import annotations + +import textwrap +from pathlib import Path +from typing import Any + +import pytest + +BACKEND_DIR = Path(__file__).resolve().parents[2] +PERMISSION_MODULE = BACKEND_DIR / "permissions" / "permission.py" + +START_MARKER = "class IsParentDeploymentOwner(permissions.BasePermission):" +END_MARKER = "\nclass " + + +class _User: + def __init__(self, name: str) -> None: + self.name = name + + def __repr__(self) -> str: # pragma: no cover - debugging aid + return f"" + + +class _Request: + def __init__(self, user: _User) -> None: + self.user = user + + +class _APIDeployment: + """A deployment as the permission class actually receives it. + + Deliberately declares neither ``api`` nor ``pipeline`` -- that is what the + real model looks like (``APIKey.api`` points *at* it with + ``related_name="api_keys"``), and it is the shape that made a bare + ``obj.api`` a 500. + """ + + def __init__(self, owner: _User) -> None: + self.owner = owner + + +class _APIKey: + """A key row, whose ownership is inherited from its parent.""" + + def __init__(self, api: Any = None, pipeline: Any = None, owner: Any = None) -> None: + self.api = api + self.pipeline = pipeline + self.owner = owner + + +def _build_permission(*, org_admins: set[str]) -> Any: + """Extract the real ``IsParentDeploymentOwner`` against stubbed collaborators.""" + source = PERMISSION_MODULE.read_text() + if START_MARKER not in source: + pytest.fail( + f"Could not find {START_MARKER!r} in {PERMISSION_MODULE}. If it was " + "renamed, update this test rather than deleting it." + ) + rest = source[source.index(START_MARKER) :] + next_class = rest.find(END_MARKER, len(START_MARKER)) + body = textwrap.dedent(rest if next_class == -1 else rest[:next_class]) + + class _BasePermission: + pass + + class _Permissions: + BasePermission = _BasePermission + + def _is_resource_owner(user: _User, obj: Any) -> bool: + return getattr(obj, "owner", None) is user + + def _is_service_account(request: _Request) -> bool: + return getattr(request.user, "is_service_account", False) + + def _is_organization_admin(request: _Request) -> bool: + return request.user.name in org_admins + + namespace: dict[str, Any] = { + "permissions": _Permissions, + "_is_resource_owner": _is_resource_owner, + "_is_service_account": _is_service_account, + "_is_organization_admin": _is_organization_admin, + "Request": object, + "APIView": object, + "Any": Any, + } + exec(compile(body, str(PERMISSION_MODULE), "exec"), namespace) + return namespace["IsParentDeploymentOwner"]() + + +OWNER = _User("owner") +STRANGER = _User("stranger") +ADMIN = _User("admin") + + +class TestParentDeploymentOwnerAcceptsTheParent: + """``create`` passes the parent directly; that must not 500 or over-admit.""" + + def test_deployment_owner_is_admitted(self) -> None: + permission = _build_permission(org_admins=set()) + deployment = _APIDeployment(owner=OWNER) + + assert permission.has_object_permission(_Request(OWNER), None, deployment) is True + + def test_non_owner_is_refused(self) -> None: + """The IDOR this check closes: same-org, not the deployment's owner.""" + permission = _build_permission(org_admins=set()) + deployment = _APIDeployment(owner=OWNER) + + allowed = permission.has_object_permission(_Request(STRANGER), None, deployment) + + assert allowed is False, ( + "A member who does not own the deployment must not be able to mint " + "an API key for it" + ) + + def test_org_admin_is_admitted(self) -> None: + permission = _build_permission(org_admins={"admin"}) + deployment = _APIDeployment(owner=OWNER) + + assert permission.has_object_permission(_Request(ADMIN), None, deployment) is True + + def test_a_parent_without_api_or_pipeline_fields_does_not_raise(self) -> None: + """The regression that a bare ``obj.api`` would reintroduce. + + ``APIDeployment``/``Pipeline`` declare no ``api`` or ``pipeline`` + field, so an unguarded attribute access is an ``AttributeError`` -- + surfacing as a 500 on every single key creation, which is worse than + the hole it was meant to close. + """ + permission = _build_permission(org_admins=set()) + deployment = _APIDeployment(owner=OWNER) + + assert not hasattr(deployment, "api") + assert not hasattr(deployment, "pipeline") + # Must return a verdict rather than raising. + assert ( + permission.has_object_permission(_Request(STRANGER), None, deployment) + is False + ) + + def test_key_rows_still_resolve_through_their_parent(self) -> None: + """The pre-existing detail-route behaviour must be unchanged.""" + permission = _build_permission(org_admins=set()) + key = _APIKey(api=_APIDeployment(owner=OWNER), owner=STRANGER) + + assert permission.has_object_permission(_Request(OWNER), None, key) is True + assert permission.has_object_permission(_Request(STRANGER), None, key) is False + + def test_parentless_key_falls_back_to_its_own_owner(self) -> None: + permission = _build_permission(org_admins=set()) + key = _APIKey(api=None, pipeline=None, owner=OWNER) + + assert permission.has_object_permission(_Request(OWNER), None, key) is True + assert permission.has_object_permission(_Request(STRANGER), None, key) is False + + +VIEW_MODULE = BACKEND_DIR / "api_v2" / "api_key_views.py" + + +class TestCreateContract: + """The wiring in ``create`` that no unit-level stub can stand in for.""" + + def test_create_checks_object_permissions_on_the_path_target(self) -> None: + """Without this call the permission class is dead code for ``create``.""" + source = VIEW_MODULE.read_text() + body = source[source.index(" def create(") : source.index("@action")] + + assert body.count("self.check_object_permissions(request,") == 2, ( + "Both the api and pipeline branches must object-check their target; " + "`create` is collection-level, so DRF never does it for us" + ) + + def test_create_refuses_a_body_naming_the_other_target(self) -> None: + source = VIEW_MODULE.read_text() + body = source[source.index(" def create(") : source.index("@action")] + + assert 'request_data.get("pipeline")' in body + assert 'request_data.get("api")' in body, ( + "A body naming the other target must be refused; silently picking " + "one would create a key against a resource the URL does not name" + ) + + def test_create_rejects_a_non_mapping_body(self) -> None: + """A JSON array body must 400, not ``AttributeError`` into a 500.""" + source = VIEW_MODULE.read_text() + body = source[source.index(" def create(") : source.index("@action")] + + assert "isinstance(request.data, dict)" in body + + def test_path_target_is_assigned_not_defaulted(self) -> None: + """``setdefault`` let ``{"api": ""}`` through as a present-but-empty key. + + The path names the target, so it is assigned outright; the body cannot + blank it out into a confusing 400. + """ + source = VIEW_MODULE.read_text() + body = source[source.index(" def create(") : source.index("@action")] + + assert 'request_data["api"] = api_id' in body + assert 'request_data["pipeline"] = pipeline_id' in body + assert "setdefault" not in body diff --git a/backend/permissions/permission.py b/backend/permissions/permission.py index 0c0a3b88eb..7d7a3d8508 100644 --- a/backend/permissions/permission.py +++ b/backend/permissions/permission.py @@ -166,12 +166,22 @@ class IsParentDeploymentOwner(permissions.BasePermission): one is set). Admits the parent's owner (creator + co-owners), org admin, or service account -- mirrors ``IsParentToolOwner`` (UN-2202). Falls back to the key's own ``created_by`` when both parents are null. + + ``obj`` may also be the parent itself. ``create`` is a collection-level + action, so DRF never calls ``get_object()`` for it and there is no + ``APIKey`` yet to check — the view hands the target ``APIDeployment`` / + ``Pipeline`` straight to ``check_object_permissions``. Neither declares an + ``api`` or ``pipeline`` field, so the lookups are ``getattr`` guarded and + fall through to ``obj``; a plain attribute access would raise + ``AttributeError`` (500) on exactly that call. """ def has_object_permission(self, request: Request, view: APIView, obj: Any) -> bool: if _is_service_account(request): return True - owner_resource = obj.api or obj.pipeline or obj + owner_resource = ( + getattr(obj, "api", None) or getattr(obj, "pipeline", None) or obj + ) if _is_resource_owner(request.user, owner_resource): return True return _is_organization_admin(request) diff --git a/backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py b/backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py index cbaf1f0ae6..23d09bf09e 100644 --- a/backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py +++ b/backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py @@ -176,22 +176,195 @@ def test_unlinked_legacy_row_falls_back_to_its_own_owner(self) -> None: assert permission.has_object_permission(_Request(STRANGER), None, row) is False -class TestRegistryToolInUseRefusal: - """The 409 guard: a tool attached to a workflow must not be deleted. +VIEWS_MODULE = BACKEND_DIR / "prompt_studio" / "prompt_studio_registry_v2" / "views.py" + +GUARD_MARKERS = ( + " def _get_deployment_types(self, workflow_ids: set) -> set:", + " def _refuse_if_in_use(self, instance: PromptStudioRegistry) -> None:", + " @staticmethod\n def _in_use_detail(deployment_types: set) -> str:", +) + + +class _InUseError(Exception): + """Stand-in for ``RegistryToolInUseError``, whose 409 is asserted separately.""" + + status_code = 409 + + def __init__(self, detail: str = "") -> None: + super().__init__(detail) + self.detail = detail + + +def _queryset(rows: list[Any]) -> Any: + """Minimal chainable stand-in for the ORM calls the guard makes.""" + + class _QS: + def filter(self, **_: Any) -> _QS: + return self + + def values_list(self, *_: Any, **__: Any) -> _QS: + return self + + def distinct(self) -> _QS: + return self - Mirrors the predicate in ``PromptStudioRegistryView.destroy`` -- a non-empty - set of dependent workflow IDs refuses the delete. + def exists(self) -> bool: + return bool(rows) + + def __iter__(self) -> Any: + return iter(rows) + + return _QS() + + +def _build_guard( + *, + dependent_workflow_ids: list[str], + api_deployments: bool = False, + pipeline_types: list[str] | None = None, + manual_review: bool = False, +) -> Any: + """Extract the real in-use guard against a stubbed ORM. + + Same technique as ``_build_permission``: the method bodies come from + ``views.py``, so a change to the query, the raise, or the message wording + lands here rather than passing against a restated copy. """ + source = VIEWS_MODULE.read_text() + parts = [] + for marker in GUARD_MARKERS: + if marker not in source: + pytest.fail( + f"Could not find {marker!r} in {VIEWS_MODULE}. If the guard was " + "renamed or inlined, update this test rather than deleting it." + ) + start = source.index(marker) + rest = source[start + len(marker) :] + # Each method runs to the next top-level ` def ` / ` @` sibling. + end = len(rest) + for needle in ("\n def ", "\n @"): + found = rest.find(needle) + if found != -1: + end = min(end, found) + parts.append(marker + rest[:end]) + + body = "class _Guard:\n" + "\n".join(parts) + "\n" + + class _Model: + def __init__(self, qs: Any) -> None: + self.objects = qs + + class _PipelineType: + ETL = "ETL" + TASK = "TASK" + + class _Pipeline: + objects = None + PipelineType = _PipelineType + + _Pipeline.objects = _queryset(pipeline_types or []) + + class _ConnectionType: + MANUALREVIEW = "MANUALREVIEW" + + class _WorkflowEndpoint: + objects = _queryset([1] if manual_review else []) + ConnectionType = _ConnectionType + + class _DeploymentType: + API_DEPLOYMENT = "API Deployment" + ETL_PIPELINE = "ETL Pipeline" + TASK_PIPELINE = "Task Pipeline" + HUMAN_QUALITY_REVIEW = "Human in the Loop" + + namespace: dict[str, Any] = { + "ToolInstance": _Model(_queryset(dependent_workflow_ids)), + "APIDeployment": _Model(_queryset([1] if api_deployments else [])), + "Pipeline": _Pipeline, + "WorkflowEndpoint": _WorkflowEndpoint, + "DeploymentType": _DeploymentType, + "RegistryToolInUseError": _InUseError, + "PromptStudioRegistry": object, + "logger": _SilentLogger(), + "Any": Any, + } + exec(compile(body, str(VIEWS_MODULE), "exec"), namespace) + return namespace["_Guard"]() + + +class _SilentLogger: + def info(self, *_: Any, **__: Any) -> None: + pass + + +class _Instance: + def __init__(self) -> None: + self.pk = "tool-1" + self.prompt_registry_id = "tool-1" + + +class TestRegistryToolInUseRefusal: + """The 409 guard, driven through the real method bodies from ``views.py``. - @staticmethod - def _refuses(dependent_workflow_ids: set[str]) -> bool: - return bool(dependent_workflow_ids) + These exercise ``_refuse_if_in_use`` itself -- the workflow query, the + raise, and the message construction. The previous version restated the + predicate as ``bool(ids)``, which passed regardless of what the view did. + + Route binding and permission wiring need a live request cycle (and so a + database); ``backend/conftest.py`` auto-marks such tests ``integration``, + which runs in the rig's integration tier rather than this PR's unit tier. + That boundary is why these stop at the guard. + """ def test_tool_used_by_a_workflow_is_refused(self) -> None: - assert self._refuses({"workflow-1"}) is True + guard = _build_guard(dependent_workflow_ids=["wf-1"], api_deployments=True) + + with pytest.raises(_InUseError) as excinfo: + guard._refuse_if_in_use(_Instance()) + + assert excinfo.value.status_code == 409, ( + "An in-use tool is a caller-correctable conflict, not a server " + "fault; the neighbouring ToolDeleteError's 500 is the wrong model" + ) def test_unused_tool_is_deletable(self) -> None: - assert self._refuses(set()) is False + """No dependants means the guard stands aside -- it is not a blanket ban.""" + guard = _build_guard(dependent_workflow_ids=[]) + + assert guard._refuse_if_in_use(_Instance()) is None + + def test_refusal_names_the_blocking_deployment(self) -> None: + """The 409 must say *where* the tool is used, or the caller cannot act.""" + guard = _build_guard(dependent_workflow_ids=["wf-1"], api_deployments=True) + + with pytest.raises(_InUseError) as excinfo: + guard._refuse_if_in_use(_Instance()) + + assert "API Deployment" in excinfo.value.detail + + def test_refusal_names_every_distinct_blocker(self) -> None: + guard = _build_guard( + dependent_workflow_ids=["wf-1", "wf-2"], + api_deployments=True, + pipeline_types=["ETL"], + manual_review=True, + ) + + with pytest.raises(_InUseError) as excinfo: + guard._refuse_if_in_use(_Instance()) + + detail = excinfo.value.detail + for expected in ("API Deployment", "ETL Pipeline", "Human in the Loop"): + assert expected in detail + + def test_refusal_falls_back_when_no_deployment_is_identifiable(self) -> None: + """A workflow need not be deployed anywhere; the refusal still stands.""" + guard = _build_guard(dependent_workflow_ids=["wf-1"]) + + with pytest.raises(_InUseError) as excinfo: + guard._refuse_if_in_use(_Instance()) + + assert "one or more workflows" in excinfo.value.detail def test_in_use_error_is_a_409(self) -> None: """Deleting an in-use tool is a conflict, not a server error. diff --git a/backend/prompt_studio/prompt_studio_registry_v2/views.py b/backend/prompt_studio/prompt_studio_registry_v2/views.py index f406ec1b5a..68cc82bb1c 100644 --- a/backend/prompt_studio/prompt_studio_registry_v2/views.py +++ b/backend/prompt_studio/prompt_studio_registry_v2/views.py @@ -1,15 +1,19 @@ import logging from typing import Any +from api_v2.models import APIDeployment from django.db.models import QuerySet +from pipeline_v2.models import Pipeline from rest_framework import viewsets from rest_framework.request import Request from rest_framework.response import Response from rest_framework.versioning import URLPathVersioning from tool_instance_v2.models import ToolInstance from utils.filtering import FilterHelper +from workflow_manager.endpoint_v2.models import WorkflowEndpoint from prompt_studio.permission import IsRegistryToolOwner +from prompt_studio.prompt_studio_core_v2.constants import DeploymentType from prompt_studio.prompt_studio_registry_v2.constants import PromptStudioRegistryKeys from prompt_studio.prompt_studio_registry_v2.serializers import ( PromptStudioRegistrySerializer, @@ -55,6 +59,41 @@ def get_queryset(self) -> QuerySet | None: return queryset + def _get_deployment_types(self, workflow_ids: set) -> set: + """Name the deployment kinds that reach ``workflow_ids``. + + Mirrors ``PromptStudioCoreView._get_deployment_types`` + (``prompt_studio_core_v2/views.py:249``) so the refusal can say *where* + the tool is still used rather than only that it is. + """ + deployment_types: set = set() + + # Inactive deployments are included: they still reference the tool and + # would break on re-activation. + if APIDeployment.objects.filter(workflow_id__in=workflow_ids).exists(): + deployment_types.add(DeploymentType.API_DEPLOYMENT) + + pipeline_type_mapping = { + Pipeline.PipelineType.ETL: DeploymentType.ETL_PIPELINE, + Pipeline.PipelineType.TASK: DeploymentType.TASK_PIPELINE, + } + pipeline_types = ( + Pipeline.objects.filter(workflow_id__in=workflow_ids) + .values_list("pipeline_type", flat=True) + .distinct() + ) + for pipeline_type in pipeline_types: + if pipeline_type in pipeline_type_mapping: + deployment_types.add(pipeline_type_mapping[pipeline_type]) + + if WorkflowEndpoint.objects.filter( + workflow_id__in=workflow_ids, + connection_type=WorkflowEndpoint.ConnectionType.MANUALREVIEW, + ).exists(): + deployment_types.add(DeploymentType.HUMAN_QUALITY_REVIEW) + + return deployment_types + def destroy( self, request: Request, *args: tuple[Any], **kwargs: dict[str, Any] ) -> Response: @@ -64,17 +103,61 @@ def destroy( instrument - it gives no way to unpublish a tool while keeping the project. Guarded by the same in-use check `prompt-studio delete` performs, so a tool still attached to a workflow is refused. + + Note that unpublishing is not reversible in place: re-exporting mints a + fresh `prompt_registry_id` and does not carry over `shared_to_org` / + `shared_users`, so anything holding the old UUID must be updated. + + The dependent workflow IDs are materialised rather than reduced to an + `.exists()` -- they are what names the blocking deployments below, which + is the difference between an actionable 409 and one the caller cannot + act on. """ instance: PromptStudioRegistry = self.get_object() + self._refuse_if_in_use(instance) + return super().destroy(request, *args, **kwargs) + + def _refuse_if_in_use(self, instance: PromptStudioRegistry) -> None: + """Raise a 409 naming the blockers when workflows still use ``instance``. + + Split from ``destroy`` so the guard can be exercised without standing + up DRF's delete machinery. + """ dependent_wfs = set( ToolInstance.objects.filter(tool_id=instance.pk) .values_list("workflow_id", flat=True) .distinct() ) - if dependent_wfs: - logger.info( - f"Cannot delete exported tool {instance.prompt_registry_id}, " - f"depended by workflows {dependent_wfs}" + if not dependent_wfs: + return + logger.info( + f"Cannot delete exported tool {instance.prompt_registry_id}, " + f"depended by {len(dependent_wfs)} workflow(s)" + ) + raise RegistryToolInUseError( + self._in_use_detail(self._get_deployment_types(dependent_wfs)) + ) + + @staticmethod + def _in_use_detail(deployment_types: set) -> str: + """Spell out which deployments block the delete, when any are known. + + A tool can be attached to a workflow that is not deployed anywhere, so + an empty set is normal and falls back to the generic wording. + """ + if not deployment_types: + return ( + "This exported tool is still used by one or more workflows. " + "Remove those usages before deleting it." ) - raise RegistryToolInUseError() - return super().destroy(request, *args, **kwargs) + types_list = sorted(deployment_types) + if len(types_list) == 1: + types_text = types_list[0] + elif len(types_list) == 2: + types_text = f"{types_list[0]} or {types_list[1]}" + else: + types_text = ", ".join(types_list[:-1]) + f", or {types_list[-1]}" + return ( + f"This exported tool is still used in {types_text}. " + "Remove those usages before deleting it." + ) From 4666d4b14ea870b3c27c0ee8c68c0800132a4f30 Mon Sep 17 00:00:00 2001 From: Hari John Kuriakose Date: Fri, 31 Jul 2026 17:29:31 +0530 Subject: [PATCH 05/14] [FIX] Close the API-key IDOR on every route, not just the path one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Standardized review of bfde0813a found the previous commit fixed half the hole it claimed to close, plus three issues in the fix itself. **Critical — the body-only route was still open.** `urls.py:102,104` bind `POST keys/api/` and `keys/pipeline/` to the same `create`, which returned `super().create()` for them with no ownership check. Any org member could still mint a live key for a deployment they do not own simply by moving the identifier from the path into the body. `create` now resolves the target from path *or* body and object-checks it on every route. **The 422 narrowing is gone, and with it an information leak.** The pipeline branch used `get_active_pipeline`, which raises `InactivePipelineError` (422) and logs at ERROR for any pipeline with `active=False` (the model default). That fired *before* `check_object_permissions`, so a non-owner learned the pipeline existed and was inactive -- the state the check exists to protect. Added `PipelineProcessor.get_pipeline_by_id` for callers that need to identify a row rather than run it, restoring the prior status contract. **The authorization check no longer fails open.** `getattr(...) or ... or obj` admitted any object exposing a matching owner, turning a wrong-type programming error into a silent grant. The accepted shapes are now explicit -- an APIKey by its two parent FKs, a parent by `memberships` -- and anything else is denied and logged. It still cannot be written as `obj.api or ...`: the parents declare no `api` attribute, so that is a 500 on every create. **Tests: two suites were passing against broken code.** Verified by mutation: - Removing `destroy`'s call to the guard left 12/12 green while in-use tools became deletable. `destroy` is now extracted and driven end-to-end. - `TestCreateContract` asserted on *source text*, so it passed against the wrong object being handed to `check_object_permissions` and against an inverted `isinstance` guard. Replaced with tests that execute `create`. All four previously-surviving mutations now fail, including one that re-opens the Critical. Also extracts the deployment-type probe and message grammar into `prompt_studio/tool_usage.py`; it was duplicated verbatim between the registry and core delete paths, so a fourth deployment type would have left one caller silently reporting a stale set. Restores the blocking workflow IDs to the refusal log (bounded), and logs the ambiguous case where dependants exist but no deployment type resolves -- the org-scope asymmetry between the unscoped `ToolInstance` manager and the org-scoped deployment managers. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014PWpGFA4Z5qktUj5DW2oiK --- backend/api_v2/api_key_views.py | 74 ++-- .../tests/test_api_key_create_target.py | 316 +++++++++++++++--- backend/permissions/permission.py | 34 +- backend/pipeline_v2/pipeline_processor.py | 15 + .../prompt_studio_core_v2/views.py | 56 +--- .../tests/test_registry_tool_delete_guards.py | 199 +++++++++-- .../prompt_studio_registry_v2/views.py | 92 ++--- backend/prompt_studio/tool_usage.py | 83 +++++ 8 files changed, 658 insertions(+), 211 deletions(-) create mode 100644 backend/prompt_studio/tool_usage.py diff --git a/backend/api_v2/api_key_views.py b/backend/api_v2/api_key_views.py index d4266c2f92..3796537163 100644 --- a/backend/api_v2/api_key_views.py +++ b/backend/api_v2/api_key_views.py @@ -34,55 +34,73 @@ def get_serializer_class(self) -> serializers.Serializer: return APIKeySerializer def create(self, request: Request, *args: Any, **kwargs: Any) -> Response: - """Create an API key, deriving the target from the URL. + """Create an API key for the deployment or pipeline being targeted. `POST keys/api//` and `POST keys/pipeline//` - already name the resource in the path, so callers should not have to - repeat it in the body. The body-only routes (`keys/api/`, - `keys/pipeline/`) fall through to the default implementation. + already name the resource in the path, so callers need not repeat it + in the body. The body-only routes (`keys/api/`, `keys/pipeline/`) name + it in `api` / `pipeline` instead. + + Whichever route is used, the target is resolved and **ownership is + checked here**, because `create` is collection-level: DRF resolves + `IsParentDeploymentOwner` for it but never calls `get_object()`, so + `has_object_permission` never runs on its own. Without this, any org + member could mint a live key for a deployment they do not own. The + check must cover the body-only routes too — otherwise the same hole is + simply reachable by moving the identifier from the path into the body. The path target is authoritative: a body naming the *other* target is a contradiction, not an override, and is refused rather than silently - creating a key for whichever one wins. Ownership of the target is - checked here because `create` is collection-level -- DRF resolves - `IsParentDeploymentOwner` for it but never calls `get_object()`, so - `has_object_permission` would otherwise never run and any org member - could mint a live key for a deployment they do not own. + creating a key for whichever one wins. """ - api_id = kwargs.get("api_id") - pipeline_id = kwargs.get("pipeline_id") - - if not (api_id or pipeline_id): - return super().create(request, *args, **kwargs) - # A JSON array (or scalar) body has no `.copy()` returning a mapping; # reject it as a 400 rather than letting `AttributeError` become a 500. if not isinstance(request.data, dict): - raise serializers.ValidationError("Request body must be a JSON object.") + raise serializers.ValidationError( + {"non_field_errors": "Request body must be a JSON object."} + ) request_data = request.data.copy() + api_id = kwargs.get("api_id") + pipeline_id = kwargs.get("pipeline_id") + + if api_id and request_data.get("pipeline"): + raise serializers.ValidationError( + { + "pipeline": "This endpoint creates a key for the API " + "deployment named in the URL; remove `pipeline` from the body." + } + ) + if pipeline_id and request_data.get("api"): + raise serializers.ValidationError( + { + "api": "This endpoint creates a key for the pipeline named " + "in the URL; remove `api` from the body." + } + ) + + # The path wins where it names a target; otherwise fall back to the + # body, so the body-only routes resolve to the same guarded path. + api_id = api_id or request_data.get("api") + pipeline_id = pipeline_id or request_data.get("pipeline") + if api_id: - if request_data.get("pipeline"): - raise serializers.ValidationError( - "This endpoint creates a key for the API deployment named " - "in the URL; remove `pipeline` from the body." - ) api = DeploymentHelper.get_api_by_id(api_id=api_id) if not api: raise APINotFound() self.check_object_permissions(request, api) request_data["api"] = api_id - else: - if request_data.get("api"): - raise serializers.ValidationError( - "This endpoint creates a key for the pipeline named in the " - "URL; remove `api` from the body." - ) - pipeline = PipelineProcessor.get_active_pipeline(pipeline_id=pipeline_id) + elif pipeline_id: + # `check_active=False`: minting a key does not require a running + # pipeline, and `get_active_pipeline` would both 422 on a paused + # one and disclose its state before the ownership check below. + pipeline = PipelineProcessor.get_pipeline_by_id(pipeline_id=pipeline_id) if not pipeline: raise PipelineNotFound() self.check_object_permissions(request, pipeline) request_data["pipeline"] = pipeline_id + # Neither named: let the serializer raise its "one of api/pipeline" + # error rather than inventing a second wording for the same condition. serializer = self.get_serializer(data=request_data) serializer.is_valid(raise_exception=True) diff --git a/backend/api_v2/tests/test_api_key_create_target.py b/backend/api_v2/tests/test_api_key_create_target.py index 4792b14f33..40b3bd1c85 100644 --- a/backend/api_v2/tests/test_api_key_create_target.py +++ b/backend/api_v2/tests/test_api_key_create_target.py @@ -8,21 +8,32 @@ live key for a deployment they do not own. The view now performs the object check itself. -Two things are pinned here, both cheap to break: +Three things are pinned here, all cheap to break: -1. **``IsParentDeploymentOwner`` accepts the parent itself.** The view hands it +1. **Every route authorizes.** The check must cover the body-only routes + (``keys/api/``, ``keys/pipeline/``) as well as the path ones -- guarding + only the path form leaves the identical hole reachable by moving the + identifier into the body. + +2. **``IsParentDeploymentOwner`` accepts the parent itself.** The view hands it an ``APIDeployment``/``Pipeline``, neither of which declares an ``api`` or ``pipeline`` field. A plain ``obj.api`` raises ``AttributeError`` -> 500 on - every key creation; the lookups must be ``getattr`` guarded. + every key creation; the lookups must be shape-guarded. An object matching + neither shape is denied, not admitted. -2. **The path target is authoritative.** A body naming the *other* target is a +3. **The path target is authoritative.** A body naming the *other* target is a contradiction and must be refused, not silently resolved to whichever wins. These are unit tests over the real method bodies -- Django settings are not configured in the unit tier, so collaborators are stubbed and the source is extracted, mirroring -``prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py``. The -end-to-end request cycle needs a database and lives in the integration tier. +``prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py``. +``create`` is *executed* rather than grepped: an earlier version asserted on +source text and so passed against an inverted ``isinstance`` guard and against +the wrong object being handed to ``check_object_permissions``. + +The end-to-end request cycle still needs a database and lives in the +integration tier; what runs here is the method body, not the routing. """ from __future__ import annotations @@ -59,11 +70,13 @@ class _APIDeployment: Deliberately declares neither ``api`` nor ``pipeline`` -- that is what the real model looks like (``APIKey.api`` points *at* it with ``related_name="api_keys"``), and it is the shape that made a bare - ``obj.api`` a 500. + ``obj.api`` a 500. ``memberships`` is how the permission class recognises + a parent; both real models get it from ``HasMembersMixin``. """ def __init__(self, owner: _User) -> None: self.owner = owner + self.memberships = () class _APIKey: @@ -102,11 +115,16 @@ def _is_service_account(request: _Request) -> bool: def _is_organization_admin(request: _Request) -> bool: return request.user.name in org_admins + class _SilentLogger: + def warning(self, *_: Any, **__: Any) -> None: + pass + namespace: dict[str, Any] = { "permissions": _Permissions, "_is_resource_owner": _is_resource_owner, "_is_service_account": _is_service_account, "_is_organization_admin": _is_organization_admin, + "logger": _SilentLogger(), "Request": object, "APIView": object, "Any": Any, @@ -181,49 +199,271 @@ def test_parentless_key_falls_back_to_its_own_owner(self) -> None: assert permission.has_object_permission(_Request(OWNER), None, key) is True assert permission.has_object_permission(_Request(STRANGER), None, key) is False + def test_an_unrecognised_object_is_denied(self) -> None: + """An authz gate that cannot identify its subject must fail closed. + + A permissive fallthrough would admit any object that happens to expose + a matching owner, turning a wrong-type programming error into a silent + grant instead of a loud failure. + """ + + class _Unexpected: + def __init__(self, owner: _User) -> None: + self.owner = owner + + permission = _build_permission(org_admins=set()) + + assert ( + permission.has_object_permission( + _Request(OWNER), None, _Unexpected(owner=OWNER) + ) + is False + ) + VIEW_MODULE = BACKEND_DIR / "api_v2" / "api_key_views.py" +CREATE_MARKER = ( + " def create(self, request: Request, *args: Any, **kwargs: Any) -> Response:" +) -class TestCreateContract: - """The wiring in ``create`` that no unit-level stub can stand in for.""" +CREATED = object() +"""Sentinel proving the key was actually minted.""" - def test_create_checks_object_permissions_on_the_path_target(self) -> None: - """Without this call the permission class is dead code for ``create``.""" - source = VIEW_MODULE.read_text() - body = source[source.index(" def create(") : source.index("@action")] - assert body.count("self.check_object_permissions(request,") == 2, ( - "Both the api and pipeline branches must object-check their target; " - "`create` is collection-level, so DRF never does it for us" - ) +class _ValidationError(Exception): + """Stand-in for ``serializers.ValidationError`` (a 400).""" + + +class _NotFound(Exception): + """Stand-in for ``APINotFound`` / ``PipelineNotFound`` (a 404).""" + - def test_create_refuses_a_body_naming_the_other_target(self) -> None: - source = VIEW_MODULE.read_text() - body = source[source.index(" def create(") : source.index("@action")] +class _PermissionDenied(Exception): + """Raised by the stub ``check_object_permissions`` (a 403).""" - assert 'request_data.get("pipeline")' in body - assert 'request_data.get("api")' in body, ( - "A body naming the other target must be refused; silently picking " - "one would create a key against a resource the URL does not name" + +class _Target: + """A resolvable APIDeployment/Pipeline row.""" + + def __init__(self, target_id: str, owner: _User) -> None: + self.id = target_id + self.owner = owner + + +def _build_create( + *, + apis: dict[str, _Target] | None = None, + pipelines: dict[str, _Target] | None = None, + requester: _User = OWNER, +) -> Any: + """Extract and execute the real ``create`` body against stubs. + + Source extraction is used for the same reason as ``_build_permission``: + Django settings are unconfigured in the unit tier, so the module cannot be + imported. Executing the real body is what makes these behavioural -- the + previous version asserted on source *text*, and so passed against an + inverted ``isinstance`` guard and against the wrong object being handed to + ``check_object_permissions``. + """ + source = VIEW_MODULE.read_text() + if CREATE_MARKER not in source: + pytest.fail( + f"Could not find {CREATE_MARKER!r} in {VIEW_MODULE}. If the " + "signature changed, update this test rather than deleting it." ) + start = source.index(CREATE_MARKER) + rest = source[start + len(CREATE_MARKER) :] + end = len(rest) + for needle in ("\n def ", "\n @"): + found = rest.find(needle) + if found != -1: + end = min(end, found) + body = textwrap.dedent(CREATE_MARKER + rest[:end]) + + checked: list[Any] = [] + + class _Serializers: + ValidationError = _ValidationError + + class _DeploymentHelper: + @staticmethod + def get_api_by_id(api_id: str) -> _Target | None: + return (apis or {}).get(api_id) + + class _PipelineProcessor: + @staticmethod + def get_pipeline_by_id(pipeline_id: str) -> _Target | None: + return (pipelines or {}).get(pipeline_id) + + class _View: + """Minimal DRF-ish host for the extracted method.""" + + def check_object_permissions(self, request: Any, obj: Any) -> None: + checked.append(obj) + # Mirrors IsParentDeploymentOwner: only the owner passes. + if getattr(obj, "owner", None) is not request.user: + raise _PermissionDenied() + + def get_serializer(self, data: Any) -> Any: + return _Serializer(data) + + def perform_create(self, serializer: Any) -> None: + serializer.saved = True + + def get_success_headers(self, data: Any) -> dict[str, str]: + return {} + + class _Serializer: + def __init__(self, data: Any) -> None: + self.data = data + self.saved = False + + def is_valid(self, raise_exception: bool = False) -> bool: + api = self.data.get("api") + pipeline = self.data.get("pipeline") + if api and pipeline: + raise _ValidationError("only one of api/pipeline") + if not api and not pipeline: + raise _ValidationError("at least one of api/pipeline") + return True + + class _Response: + def __init__(self, data: Any, status: Any = None, headers: Any = None) -> None: + self.data = data + self.status = status + + class _Status: + HTTP_201_CREATED = 201 - def test_create_rejects_a_non_mapping_body(self) -> None: - """A JSON array body must 400, not ``AttributeError`` into a 500.""" - source = VIEW_MODULE.read_text() - body = source[source.index(" def create(") : source.index("@action")] + namespace: dict[str, Any] = { + "serializers": _Serializers, + "DeploymentHelper": _DeploymentHelper, + "PipelineProcessor": _PipelineProcessor, + "APINotFound": _NotFound, + "PipelineNotFound": _NotFound, + "Response": _Response, + "status": _Status, + "Request": object, + "Any": Any, + } + exec(compile(body, str(VIEW_MODULE), "exec"), namespace) + + view = _View() + view.create = namespace["create"].__get__(view, _View) + view.checked = checked + view.requester = requester + return view + + +class _Req: + def __init__(self, data: Any, user: _User) -> None: + self.data = data + self.user = user - assert "isinstance(request.data, dict)" in body - def test_path_target_is_assigned_not_defaulted(self) -> None: - """``setdefault`` let ``{"api": ""}`` through as a present-but-empty key. +class TestCreateAuthorizesEveryRoute: + """The IDOR fix, exercised rather than grepped. - The path names the target, so it is assigned outright; the body cannot - blank it out into a confusing 400. + The hole is reachable from four routes -- api/pipeline, each by path and + by body. Every one must resolve the target and object-check it. + """ + + def test_path_route_admits_the_owner(self) -> None: + target = _Target("api-1", OWNER) + view = _build_create(apis={"api-1": target}) + + response = view.create(_Req({}, OWNER), api_id="api-1") + + assert response.status == 201 + assert view.checked == [target], "the target must be object-checked" + + def test_path_route_refuses_a_non_owner(self) -> None: + view = _build_create(apis={"api-1": _Target("api-1", OWNER)}) + + with pytest.raises(_PermissionDenied): + view.create(_Req({}, STRANGER), api_id="api-1") + + def test_body_only_route_refuses_a_non_owner(self) -> None: + """The route the path-only fix left open. + + Moving the identifier from the path into the body must not bypass the + ownership check -- otherwise the IDOR is simply relocated. """ - source = VIEW_MODULE.read_text() - body = source[source.index(" def create(") : source.index("@action")] + view = _build_create(apis={"api-1": _Target("api-1", OWNER)}) + + with pytest.raises(_PermissionDenied): + view.create(_Req({"api": "api-1"}, STRANGER)) + + def test_body_only_route_admits_the_owner(self) -> None: + target = _Target("api-1", OWNER) + view = _build_create(apis={"api-1": target}) + + response = view.create(_Req({"api": "api-1"}, OWNER)) + + assert response.status == 201 + assert view.checked == [target] + + def test_body_only_pipeline_route_refuses_a_non_owner(self) -> None: + view = _build_create(pipelines={"pipe-1": _Target("pipe-1", OWNER)}) + + with pytest.raises(_PermissionDenied): + view.create(_Req({"pipeline": "pipe-1"}, STRANGER)) + + def test_pipeline_path_route_checks_the_pipeline_not_the_api(self) -> None: + """Guards against the wrong object reaching the permission check.""" + target = _Target("pipe-1", OWNER) + view = _build_create(pipelines={"pipe-1": target}) + + view.create(_Req({}, OWNER), pipeline_id="pipe-1") + + assert view.checked == [target] + + def test_unknown_target_is_a_404(self) -> None: + view = _build_create(apis={}) + + with pytest.raises(_NotFound): + view.create(_Req({}, OWNER), api_id="missing") + + +class TestCreateBodyContract: + def test_non_mapping_body_is_a_400(self) -> None: + """A JSON array body must 400, not ``AttributeError`` into a 500.""" + view = _build_create(apis={"api-1": _Target("api-1", OWNER)}) + + with pytest.raises(_ValidationError): + view.create(_Req([{"api": "api-1"}], OWNER), api_id="api-1") + + def test_body_naming_the_other_target_is_refused(self) -> None: + view = _build_create( + apis={"api-1": _Target("api-1", OWNER)}, + pipelines={"pipe-1": _Target("pipe-1", OWNER)}, + ) + + with pytest.raises(_ValidationError): + view.create(_Req({"pipeline": "pipe-1"}, OWNER), api_id="api-1") + + def test_pipeline_path_with_api_body_is_refused(self) -> None: + view = _build_create( + apis={"api-1": _Target("api-1", OWNER)}, + pipelines={"pipe-1": _Target("pipe-1", OWNER)}, + ) + + with pytest.raises(_ValidationError): + view.create(_Req({"api": "api-1"}, OWNER), pipeline_id="pipe-1") + + def test_empty_string_body_value_does_not_defeat_the_path(self) -> None: + """``setdefault`` used to let ``{"api": ""}`` through as a present key.""" + target = _Target("api-1", OWNER) + view = _build_create(apis={"api-1": target}) + + response = view.create(_Req({"api": ""}, OWNER), api_id="api-1") + + assert response.status == 201 + assert view.checked == [target] + + def test_no_target_anywhere_is_a_400(self) -> None: + view = _build_create() - assert 'request_data["api"] = api_id' in body - assert 'request_data["pipeline"] = pipeline_id' in body - assert "setdefault" not in body + with pytest.raises(_ValidationError): + view.create(_Req({}, OWNER)) diff --git a/backend/permissions/permission.py b/backend/permissions/permission.py index 7d7a3d8508..ad29da0dcd 100644 --- a/backend/permissions/permission.py +++ b/backend/permissions/permission.py @@ -1,3 +1,4 @@ +import logging from typing import Any from adapter_processor_v2.models import AdapterInstance @@ -7,6 +8,8 @@ from tenant_account_v2.organization_member_service import OrganizationMemberService from utils.user_context import UserContext +logger = logging.getLogger(__name__) + _REQUEST_ADMIN_CACHE_ATTR = "_cached_is_organization_admin" @@ -170,18 +173,35 @@ class IsParentDeploymentOwner(permissions.BasePermission): ``obj`` may also be the parent itself. ``create`` is a collection-level action, so DRF never calls ``get_object()`` for it and there is no ``APIKey`` yet to check — the view hands the target ``APIDeployment`` / - ``Pipeline`` straight to ``check_object_permissions``. Neither declares an - ``api`` or ``pipeline`` field, so the lookups are ``getattr`` guarded and - fall through to ``obj``; a plain attribute access would raise - ``AttributeError`` (500) on exactly that call. + ``Pipeline`` straight to ``check_object_permissions``. + + An ``APIKey`` is recognised by declaring both parent FKs; a parent by + carrying ``memberships`` (both models use ``HasMembersMixin``). Anything + else is **denied** rather than guessed at — an authorization gate that + cannot identify its subject must fail closed. Note this cannot collapse to + ``obj.api or obj.pipeline or obj``: the parents declare no ``api`` + attribute, so that raises ``AttributeError`` (500) on every ``create``. """ def has_object_permission(self, request: Request, view: APIView, obj: Any) -> bool: if _is_service_account(request): return True - owner_resource = ( - getattr(obj, "api", None) or getattr(obj, "pipeline", None) or obj - ) + + if hasattr(obj, "api") and hasattr(obj, "pipeline"): + # An APIKey: ownership is inherited from whichever parent is set, + # falling back to the key itself when both are null. + owner_resource = obj.api or obj.pipeline or obj + elif hasattr(obj, "memberships"): + # The parent deployment/pipeline, handed over by ``create``. + owner_resource = obj + else: + logger.warning( + "IsParentDeploymentOwner received an unsupported object type " + "%s; denying.", + type(obj).__name__, + ) + return False + if _is_resource_owner(request.user, owner_resource): return True return _is_organization_admin(request) diff --git a/backend/pipeline_v2/pipeline_processor.py b/backend/pipeline_v2/pipeline_processor.py index b2967f1aa1..7ab93c4195 100644 --- a/backend/pipeline_v2/pipeline_processor.py +++ b/backend/pipeline_v2/pipeline_processor.py @@ -50,6 +50,21 @@ def get_active_pipeline(cls, pipeline_id: str) -> Pipeline | None: except Pipeline.DoesNotExist: return None + @classmethod + def get_pipeline_by_id(cls, pipeline_id: str) -> Pipeline | None: + """Retrieve a pipeline regardless of whether it is currently active. + + The active/inactive distinction matters to callers that are about to + *run* something. Callers that only need to identify the row -- to check + ownership, say -- must not be forced through ``get_active_pipeline``, + which raises ``InactivePipelineError`` (422) and logs at ERROR for what + is an ordinary request against a paused pipeline. + """ + try: + return cls.fetch_pipeline(pipeline_id, check_active=False) + except Pipeline.DoesNotExist: + return None + @staticmethod def _update_pipeline_status( pipeline: Pipeline, diff --git a/backend/prompt_studio/prompt_studio_core_v2/views.py b/backend/prompt_studio/prompt_studio_core_v2/views.py index ae11da451a..ad93b825ca 100644 --- a/backend/prompt_studio/prompt_studio_core_v2/views.py +++ b/backend/prompt_studio/prompt_studio_core_v2/views.py @@ -8,7 +8,6 @@ import magic from account_v2.custom_exceptions import DuplicateData -from api_v2.models import APIDeployment from celery import signature from celery.result import AsyncResult from django.db import IntegrityError @@ -22,20 +21,17 @@ from permissions.resource_share_views import ResourceShareManagementMixin from permissions.roles import ResourceRole from pg_queue.flags import PG_QUEUE_FLAG_KEY -from pipeline_v2.models import Pipeline from plugins import get_plugin from rest_framework import status, viewsets from rest_framework.decorators import action from rest_framework.request import Request from rest_framework.response import Response from rest_framework.versioning import URLPathVersioning -from tool_instance_v2.models import ToolInstance from utils.file_storage.helpers.prompt_studio_file_helper import PromptStudioFileHelper from utils.hubspot_notify import notify_hubspot_event from utils.pagination import OptionalPagination from utils.user_context import UserContext from utils.user_session import UserSessionUtils -from workflow_manager.endpoint_v2.models import WorkflowEndpoint from backend.celery_service import app as celery_app from prompt_studio.lookup_utils import ( @@ -50,7 +46,6 @@ from prompt_studio.prompt_profile_manager_v2.models import ProfileManager from prompt_studio.prompt_profile_manager_v2.serializers import ProfileManagerSerializer from prompt_studio.prompt_studio_core_v2.constants import ( - DeploymentType, FileViewTypes, ToolStudioErrors, ToolStudioPromptKeys, @@ -85,6 +80,11 @@ from prompt_studio.prompt_studio_v2.constants import ToolStudioPromptErrors from prompt_studio.prompt_studio_v2.models import ToolStudioPrompt from prompt_studio.prompt_studio_v2.serializers import ToolStudioPromptSerializer +from prompt_studio.tool_usage import ( + dependent_workflow_ids, + deployment_types_for, + join_deployment_types, +) from unstract.core.data_models import PgTaskStatus from unstract.flags.feature_flag import check_feature_flag_status from unstract.sdk1.utils.common import Utils as CommonUtils @@ -240,11 +240,7 @@ def _check_tool_usage_in_workflows(self, instance: CustomTool) -> tuple[bool, se if not registry: return False, set() - dependent_wfs = set( - ToolInstance.objects.filter(tool_id=registry.pk) - .values_list("workflow_id", flat=True) - .distinct() - ) + dependent_wfs = dependent_workflow_ids(registry.pk) return bool(dependent_wfs), dependent_wfs def _get_deployment_types(self, workflow_ids: set) -> set: @@ -256,34 +252,7 @@ def _get_deployment_types(self, workflow_ids: set) -> set: Returns: Set of deployment type strings """ - deployment_types: set = set() - - # Check API Deployments (include inactive to prevent drift) - if APIDeployment.objects.filter(workflow_id__in=workflow_ids).exists(): - deployment_types.add(DeploymentType.API_DEPLOYMENT) - - # Check Pipelines using mapping instead of if/elif - pipeline_type_mapping = { - Pipeline.PipelineType.ETL: DeploymentType.ETL_PIPELINE, - Pipeline.PipelineType.TASK: DeploymentType.TASK_PIPELINE, - } - pipelines = ( - Pipeline.objects.filter(workflow_id__in=workflow_ids) - .values_list("pipeline_type", flat=True) - .distinct() - ) - for pipeline_type in pipelines: - if pipeline_type in pipeline_type_mapping: - deployment_types.add(pipeline_type_mapping[pipeline_type]) - - # Check for Manual Review - if WorkflowEndpoint.objects.filter( - workflow_id__in=workflow_ids, - connection_type=WorkflowEndpoint.ConnectionType.MANUALREVIEW, - ).exists(): - deployment_types.add(DeploymentType.HUMAN_QUALITY_REVIEW) - - return deployment_types + return deployment_types_for(workflow_ids) def _format_deployment_types_message(self, deployment_types: set) -> str: """Format deployment types into human-readable message. @@ -294,17 +263,10 @@ def _format_deployment_types_message(self, deployment_types: set) -> str: Returns: Formatted message string or empty string if no types """ - if not deployment_types: + types_text = join_deployment_types(deployment_types) + if not types_text: return "" - types_list = sorted(deployment_types) - if len(types_list) == 1: - types_text = types_list[0] - elif len(types_list) == 2: - types_text = f"{types_list[0]} or {types_list[1]}" - else: - types_text = ", ".join(types_list[:-1]) + f", or {types_list[-1]}" - return ( f"You have made changes to this Prompt Studio project. " f"This project is used in {types_text}. " diff --git a/backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py b/backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py index 23d09bf09e..fdd8233c6e 100644 --- a/backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py +++ b/backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py @@ -20,8 +20,15 @@ mirroring ``prompt_studio_core_v2/tests/test_build_index_payload.py``. A rename fails these tests rather than silently skipping them. -The in-use check is asserted against the same predicate the view applies -(a non-empty set of dependent workflow IDs raises), without standing up the ORM. +The in-use check is driven the same way: ``destroy``, ``_refuse_if_in_use`` and +the shared helpers in ``prompt_studio/tool_usage.py`` are all executed against a +stubbed ORM. ``destroy`` is included deliberately -- covering only the guard +left its *call site* unpinned, and deleting that one line made in-use tools +deletable with the whole suite still green. + +What is *not* covered here: route binding and the live permission cycle need a +database, and ``backend/conftest.py`` auto-marks such tests ``integration`` so +they run in the rig's integration tier rather than the per-PR unit tier. """ from __future__ import annotations @@ -177,13 +184,23 @@ def test_unlinked_legacy_row_falls_back_to_its_own_owner(self) -> None: VIEWS_MODULE = BACKEND_DIR / "prompt_studio" / "prompt_studio_registry_v2" / "views.py" +TOOL_USAGE_MODULE = BACKEND_DIR / "prompt_studio" / "tool_usage.py" GUARD_MARKERS = ( - " def _get_deployment_types(self, workflow_ids: set) -> set:", + " def destroy(", " def _refuse_if_in_use(self, instance: PromptStudioRegistry) -> None:", " @staticmethod\n def _in_use_detail(deployment_types: set) -> str:", ) +# The shared helpers live in ``prompt_studio/tool_usage.py``; they are pulled +# in the same way so the guard runs against the real query and the real +# grammar rather than a restatement of either. +HELPER_MARKERS = ( + "def dependent_workflow_ids(registry_pk: str) -> set:", + "def deployment_types_for(workflow_ids: set) -> set:", + "def join_deployment_types(deployment_types: set) -> str:", +) + class _InUseError(Exception): """Stand-in for ``RegistryToolInUseError``, whose 409 is asserted separately.""" @@ -195,11 +212,26 @@ def __init__(self, detail: str = "") -> None: self.detail = detail -def _queryset(rows: list[Any]) -> Any: - """Minimal chainable stand-in for the ORM calls the guard makes.""" +def _queryset(rows: list[Any], *, required_filters: tuple[str, ...] = ()) -> Any: + """Minimal chainable stand-in for the ORM calls the guard makes. + + ``required_filters`` names the kwargs the real query must narrow on. A + query that stops filtering returns nothing rather than silently returning + every row -- otherwise dropping ``tool_id=`` would look identical to a + correct lookup and no test could tell the difference. + """ class _QS: - def filter(self, **_: Any) -> _QS: + def __init__(self, matched: bool = False) -> None: + self._matched = matched or not required_filters + + def filter(self, **kwargs: Any) -> _QS: + if required_filters and not all(k in kwargs for k in required_filters): + return _QS.__new__(_QS)._empty() + return _QS(matched=True) + + def _empty(self) -> _QS: + self._matched = False return self def values_list(self, *_: Any, **__: Any) -> _QS: @@ -209,46 +241,82 @@ def distinct(self) -> _QS: return self def exists(self) -> bool: - return bool(rows) + return bool(rows) and self._matched def __iter__(self) -> Any: - return iter(rows) + return iter(rows if self._matched else []) return _QS() -def _build_guard( - *, - dependent_workflow_ids: list[str], - api_deployments: bool = False, - pipeline_types: list[str] | None = None, - manual_review: bool = False, -) -> Any: - """Extract the real in-use guard against a stubbed ORM. +DELETED = object() +"""Sentinel returned by the stub base ``destroy``, proving the delete ran.""" + - Same technique as ``_build_permission``: the method bodies come from - ``views.py``, so a change to the query, the raise, or the message wording - lands here rather than passing against a restated copy. +class _BaseView: + """Stands in for ``viewsets.ModelViewSet`` under the extracted ``destroy``. + + Recording the ``super().destroy()`` hand-off is what lets a test assert + that an unused tool actually reaches the delete, and — more importantly — + that an in-use one never does. """ - source = VIEWS_MODULE.read_text() + + def destroy(self, request: Any, *args: Any, **kwargs: Any) -> Any: + return DELETED + + +def _extract(module: Path, markers: tuple[str, ...], stops: tuple[str, ...]) -> list[str]: + """Slice each named definition out of ``module``'s source. + + ``pytest.fail`` on a missing marker rather than skipping: a rename must + break loudly, since a silently-skipped guard test is worse than none. + """ + source = module.read_text() parts = [] - for marker in GUARD_MARKERS: + for marker in markers: if marker not in source: pytest.fail( - f"Could not find {marker!r} in {VIEWS_MODULE}. If the guard was " - "renamed or inlined, update this test rather than deleting it." + f"Could not find {marker!r} in {module}. If it was renamed or " + "inlined, update this test rather than deleting it." ) start = source.index(marker) rest = source[start + len(marker) :] - # Each method runs to the next top-level ` def ` / ` @` sibling. end = len(rest) - for needle in ("\n def ", "\n @"): + for needle in stops: found = rest.find(needle) if found != -1: end = min(end, found) parts.append(marker + rest[:end]) + return parts + + +def _build_guard( + *, + workflow_ids: list[str], + api_deployments: bool = False, + pipeline_types: list[str] | None = None, + manual_review: bool = False, +) -> Any: + """Extract the real in-use guard against a stubbed ORM. + + Same technique as ``_build_permission``: the bodies come from ``views.py`` + and ``tool_usage.py``, so a change to the query, the raise, or the message + wording lands here rather than passing against a restated copy. - body = "class _Guard:\n" + "\n".join(parts) + "\n" + ``destroy`` is extracted alongside the guard so its *call* to + ``_refuse_if_in_use`` is covered too. Testing the guard alone left the + wiring unpinned: dropping that one line made in-use tools deletable with + the whole suite still green. + """ + view_parts = _extract(VIEWS_MODULE, GUARD_MARKERS, ("\n def ", "\n @")) + helper_parts = _extract(TOOL_USAGE_MODULE, HELPER_MARKERS, ("\ndef ",)) + + body = ( + "\n".join(helper_parts) + + "\n\nclass _Guard(_BaseView):\n" + + "\n".join(view_parts) + + "\n" + ) class _Model: def __init__(self, qs: Any) -> None: @@ -278,24 +346,40 @@ class _DeploymentType: HUMAN_QUALITY_REVIEW = "Human in the Loop" namespace: dict[str, Any] = { - "ToolInstance": _Model(_queryset(dependent_workflow_ids)), + "_BaseView": _BaseView, + "ToolInstance": _Model(_queryset(workflow_ids, required_filters=("tool_id",))), "APIDeployment": _Model(_queryset([1] if api_deployments else [])), "Pipeline": _Pipeline, "WorkflowEndpoint": _WorkflowEndpoint, "DeploymentType": _DeploymentType, "RegistryToolInUseError": _InUseError, "PromptStudioRegistry": object, + "_LOGGED_WORKFLOW_LIMIT": _logged_workflow_limit(), "logger": _SilentLogger(), + "Request": object, + "Response": object, "Any": Any, } exec(compile(body, str(VIEWS_MODULE), "exec"), namespace) return namespace["_Guard"]() +def _logged_workflow_limit() -> int: + """Read the real cap rather than restating it.""" + source = VIEWS_MODULE.read_text() + marker = "_LOGGED_WORKFLOW_LIMIT = " + if marker not in source: + pytest.fail(f"Could not find {marker!r} in {VIEWS_MODULE}.") + return int(source.split(marker)[1].split("\n")[0].strip()) + + class _SilentLogger: def info(self, *_: Any, **__: Any) -> None: pass + def warning(self, *_: Any, **__: Any) -> None: + pass + class _Instance: def __init__(self) -> None: @@ -317,7 +401,7 @@ class TestRegistryToolInUseRefusal: """ def test_tool_used_by_a_workflow_is_refused(self) -> None: - guard = _build_guard(dependent_workflow_ids=["wf-1"], api_deployments=True) + guard = _build_guard(workflow_ids=["wf-1"], api_deployments=True) with pytest.raises(_InUseError) as excinfo: guard._refuse_if_in_use(_Instance()) @@ -329,13 +413,13 @@ def test_tool_used_by_a_workflow_is_refused(self) -> None: def test_unused_tool_is_deletable(self) -> None: """No dependants means the guard stands aside -- it is not a blanket ban.""" - guard = _build_guard(dependent_workflow_ids=[]) + guard = _build_guard(workflow_ids=[]) assert guard._refuse_if_in_use(_Instance()) is None def test_refusal_names_the_blocking_deployment(self) -> None: """The 409 must say *where* the tool is used, or the caller cannot act.""" - guard = _build_guard(dependent_workflow_ids=["wf-1"], api_deployments=True) + guard = _build_guard(workflow_ids=["wf-1"], api_deployments=True) with pytest.raises(_InUseError) as excinfo: guard._refuse_if_in_use(_Instance()) @@ -344,7 +428,7 @@ def test_refusal_names_the_blocking_deployment(self) -> None: def test_refusal_names_every_distinct_blocker(self) -> None: guard = _build_guard( - dependent_workflow_ids=["wf-1", "wf-2"], + workflow_ids=["wf-1", "wf-2"], api_deployments=True, pipeline_types=["ETL"], manual_review=True, @@ -357,9 +441,25 @@ def test_refusal_names_every_distinct_blocker(self) -> None: for expected in ("API Deployment", "ETL Pipeline", "Human in the Loop"): assert expected in detail + def test_two_blockers_are_joined_with_or(self) -> None: + """The 2-item branch has its own grammar; assert the whole sentence.""" + guard = _build_guard( + workflow_ids=["wf-1"], + api_deployments=True, + pipeline_types=["ETL"], + ) + + with pytest.raises(_InUseError) as excinfo: + guard._refuse_if_in_use(_Instance()) + + assert excinfo.value.detail == ( + "This exported tool is still used in API Deployment or ETL Pipeline. " + "Remove those usages before deleting it." + ) + def test_refusal_falls_back_when_no_deployment_is_identifiable(self) -> None: """A workflow need not be deployed anywhere; the refusal still stands.""" - guard = _build_guard(dependent_workflow_ids=["wf-1"]) + guard = _build_guard(workflow_ids=["wf-1"]) with pytest.raises(_InUseError) as excinfo: guard._refuse_if_in_use(_Instance()) @@ -385,3 +485,38 @@ def test_in_use_error_is_a_409(self) -> None: "RegistryToolInUseError must be a 409 so callers can distinguish a " "correctable conflict from a server fault" ) + + +class TestDestroyIsWiredToTheGuard: + """``destroy`` must actually *call* the guard. + + Testing ``_refuse_if_in_use`` in isolation left this unpinned: removing + the single call from ``destroy`` deleted in-use tools with the whole suite + still green. These drive the extracted ``destroy`` body, so the wiring — + not just the guard — is what fails when it breaks. + """ + + @staticmethod + def _view(guard: Any, instance: _Instance) -> Any: + guard.get_object = lambda: instance + return guard + + def test_destroy_refuses_an_in_use_tool_before_deleting(self) -> None: + instance = _Instance() + guard = self._view( + _build_guard(workflow_ids=["wf-1"], api_deployments=True), + instance, + ) + + with pytest.raises(_InUseError) as excinfo: + guard.destroy(object()) + + assert excinfo.value.status_code == 409 + assert "API Deployment" in excinfo.value.detail + + def test_destroy_deletes_an_unused_tool(self) -> None: + """The guard must not become a blanket ban on deletion.""" + instance = _Instance() + guard = self._view(_build_guard(workflow_ids=[]), instance) + + assert guard.destroy(object()) is DELETED diff --git a/backend/prompt_studio/prompt_studio_registry_v2/views.py b/backend/prompt_studio/prompt_studio_registry_v2/views.py index 68cc82bb1c..507c29c76a 100644 --- a/backend/prompt_studio/prompt_studio_registry_v2/views.py +++ b/backend/prompt_studio/prompt_studio_registry_v2/views.py @@ -1,29 +1,33 @@ import logging from typing import Any -from api_v2.models import APIDeployment from django.db.models import QuerySet -from pipeline_v2.models import Pipeline from rest_framework import viewsets from rest_framework.request import Request from rest_framework.response import Response from rest_framework.versioning import URLPathVersioning -from tool_instance_v2.models import ToolInstance from utils.filtering import FilterHelper -from workflow_manager.endpoint_v2.models import WorkflowEndpoint from prompt_studio.permission import IsRegistryToolOwner -from prompt_studio.prompt_studio_core_v2.constants import DeploymentType from prompt_studio.prompt_studio_registry_v2.constants import PromptStudioRegistryKeys from prompt_studio.prompt_studio_registry_v2.serializers import ( PromptStudioRegistrySerializer, ) +from prompt_studio.tool_usage import ( + dependent_workflow_ids, + deployment_types_for, + join_deployment_types, +) from .exceptions import RegistryToolInUseError from .models import PromptStudioRegistry logger = logging.getLogger(__name__) +# Blocking workflow IDs are logged so an operator can find the rows; capped so +# a heavily-reused tool cannot emit an unbounded log line. +_LOGGED_WORKFLOW_LIMIT = 20 + class PromptStudioRegistryView(viewsets.ModelViewSet): """Driver class to handle export and registering of custom tools to private @@ -59,41 +63,6 @@ def get_queryset(self) -> QuerySet | None: return queryset - def _get_deployment_types(self, workflow_ids: set) -> set: - """Name the deployment kinds that reach ``workflow_ids``. - - Mirrors ``PromptStudioCoreView._get_deployment_types`` - (``prompt_studio_core_v2/views.py:249``) so the refusal can say *where* - the tool is still used rather than only that it is. - """ - deployment_types: set = set() - - # Inactive deployments are included: they still reference the tool and - # would break on re-activation. - if APIDeployment.objects.filter(workflow_id__in=workflow_ids).exists(): - deployment_types.add(DeploymentType.API_DEPLOYMENT) - - pipeline_type_mapping = { - Pipeline.PipelineType.ETL: DeploymentType.ETL_PIPELINE, - Pipeline.PipelineType.TASK: DeploymentType.TASK_PIPELINE, - } - pipeline_types = ( - Pipeline.objects.filter(workflow_id__in=workflow_ids) - .values_list("pipeline_type", flat=True) - .distinct() - ) - for pipeline_type in pipeline_types: - if pipeline_type in pipeline_type_mapping: - deployment_types.add(pipeline_type_mapping[pipeline_type]) - - if WorkflowEndpoint.objects.filter( - workflow_id__in=workflow_ids, - connection_type=WorkflowEndpoint.ConnectionType.MANUALREVIEW, - ).exists(): - deployment_types.add(DeploymentType.HUMAN_QUALITY_REVIEW) - - return deployment_types - def destroy( self, request: Request, *args: tuple[Any], **kwargs: dict[str, Any] ) -> Response: @@ -123,20 +92,31 @@ def _refuse_if_in_use(self, instance: PromptStudioRegistry) -> None: Split from ``destroy`` so the guard can be exercised without standing up DRF's delete machinery. """ - dependent_wfs = set( - ToolInstance.objects.filter(tool_id=instance.pk) - .values_list("workflow_id", flat=True) - .distinct() - ) + dependent_wfs = dependent_workflow_ids(instance.pk) if not dependent_wfs: return - logger.info( - f"Cannot delete exported tool {instance.prompt_registry_id}, " - f"depended by {len(dependent_wfs)} workflow(s)" - ) - raise RegistryToolInUseError( - self._in_use_detail(self._get_deployment_types(dependent_wfs)) + + deployment_types = deployment_types_for(dependent_wfs) + # The IDs are what an operator needs to find the blocking rows; the + # slice bounds the line for a pathological fan-out. + blockers = sorted(str(wf) for wf in dependent_wfs) + logger.warning( + "Cannot delete exported tool %s, depended by %d workflow(s): %s", + instance.prompt_registry_id, + len(blockers), + blockers[:_LOGGED_WORKFLOW_LIMIT], ) + if not deployment_types: + # Distinguishable from "genuinely undeployed": the deployment + # tables are org-scoped while ``ToolInstance`` is not, so an empty + # set here can also mean the dependants sit outside the active org. + logger.warning( + "No deployment type resolved for the %d workflow(s) blocking " + "tool %s; the 409 will carry only the generic wording.", + len(blockers), + instance.prompt_registry_id, + ) + raise RegistryToolInUseError(self._in_use_detail(deployment_types)) @staticmethod def _in_use_detail(deployment_types: set) -> str: @@ -145,18 +125,12 @@ def _in_use_detail(deployment_types: set) -> str: A tool can be attached to a workflow that is not deployed anywhere, so an empty set is normal and falls back to the generic wording. """ - if not deployment_types: + types_text = join_deployment_types(deployment_types) + if not types_text: return ( "This exported tool is still used by one or more workflows. " "Remove those usages before deleting it." ) - types_list = sorted(deployment_types) - if len(types_list) == 1: - types_text = types_list[0] - elif len(types_list) == 2: - types_text = f"{types_list[0]} or {types_list[1]}" - else: - types_text = ", ".join(types_list[:-1]) + f", or {types_list[-1]}" return ( f"This exported tool is still used in {types_text}. " "Remove those usages before deleting it." diff --git a/backend/prompt_studio/tool_usage.py b/backend/prompt_studio/tool_usage.py new file mode 100644 index 0000000000..acebf457e2 --- /dev/null +++ b/backend/prompt_studio/tool_usage.py @@ -0,0 +1,83 @@ +"""Shared "is this exported tool still in use?" logic. + +Two delete paths need the same answer: deleting a Prompt Studio project +(``prompt_studio_core_v2``) and unpublishing its exported tool +(``prompt_studio_registry_v2``). Both refuse when a workflow still references +the registry row, and both want to name *where* it is referenced. + +Keeping one copy matters because the set of deployment kinds is open — adding a +fourth would otherwise leave one caller silently reporting a stale set, telling +the user to clear usages in the wrong places. +""" + +from api_v2.models import APIDeployment +from pipeline_v2.models import Pipeline +from tool_instance_v2.models import ToolInstance +from workflow_manager.endpoint_v2.models import WorkflowEndpoint + +from prompt_studio.prompt_studio_core_v2.constants import DeploymentType + + +def dependent_workflow_ids(registry_pk: str) -> set: + """Workflow IDs whose tool instances reference ``registry_pk``. + + An exported tool's ``ToolInstance.tool_id`` is its ``prompt_registry_id``, + stored as a plain ``CharField`` rather than a foreign key -- which is why + the database cannot enforce this and callers must check explicitly. + """ + return set( + ToolInstance.objects.filter(tool_id=registry_pk) + .values_list("workflow_id", flat=True) + .distinct() + ) + + +def deployment_types_for(workflow_ids: set) -> set: + """Name the deployment kinds reachable from ``workflow_ids``. + + Inactive deployments count: they still reference the tool and would break + on re-activation. + """ + deployment_types: set = set() + if not workflow_ids: + return deployment_types + + if APIDeployment.objects.filter(workflow_id__in=workflow_ids).exists(): + deployment_types.add(DeploymentType.API_DEPLOYMENT) + + pipeline_type_mapping = { + Pipeline.PipelineType.ETL: DeploymentType.ETL_PIPELINE, + Pipeline.PipelineType.TASK: DeploymentType.TASK_PIPELINE, + } + pipeline_types = ( + Pipeline.objects.filter(workflow_id__in=workflow_ids) + .values_list("pipeline_type", flat=True) + .distinct() + ) + for pipeline_type in pipeline_types: + if pipeline_type in pipeline_type_mapping: + deployment_types.add(pipeline_type_mapping[pipeline_type]) + + if WorkflowEndpoint.objects.filter( + workflow_id__in=workflow_ids, + connection_type=WorkflowEndpoint.ConnectionType.MANUALREVIEW, + ).exists(): + deployment_types.add(DeploymentType.HUMAN_QUALITY_REVIEW) + + return deployment_types + + +def join_deployment_types(deployment_types: set) -> str: + """Render deployment kinds as an English list ("A", "A or B", "A, B, or C"). + + Returns an empty string for an empty set so callers can choose their own + fallback wording. + """ + if not deployment_types: + return "" + types_list = sorted(deployment_types) + if len(types_list) == 1: + return types_list[0] + if len(types_list) == 2: + return f"{types_list[0]} or {types_list[1]}" + return ", ".join(types_list[:-1]) + f", or {types_list[-1]}" From bad1ded677b69dd2fa9c35da8dec43f862c7bbf4 Mon Sep 17 00:00:00 2001 From: Hari John Kuriakose Date: Fri, 31 Jul 2026 17:45:34 +0530 Subject: [PATCH 06/14] [FIX] A malformed identifier is a 404, not a 500 Second review round on 4666d4b14. Prior findings all verified resolved; this closes the one regression that fix introduced. Widening `create` to resolve the target from the request body meant the body value reached `objects.get(pk=...)` before any serializer ran. `pk` is a UUID column, so a non-UUID string raises `django.core.exceptions.ValidationError` out of `to_python` -- not `DoesNotExist`. Neither `get_api_by_id` nor `get_pipeline_by_id` caught it, so `POST keys/api/` with `{"api": "not-a-uuid"}` returned **500** with a logged traceback. At base this was a clean 400 from the serializer's `PrimaryKeyRelatedField`, so the widening caused it. Fixed at the fetch boundary rather than in `create`: the path routes are `` / ``, so path input is unvalidated too and one change covers both forms. A malformed identifier is now "not found", which is what it means. The test stubs modelled `dict.get`, making malformed and missing input indistinguishable -- which is why the suite was blind to this. They now model the real contract, and the `except` clause is asserted on the code rather than the whole function body (the docstring names `ValidationError`, so a body-level assertion passed against the reverted code). Also folds the two consecutive `logger.warning` calls in the registry refusal into one -- they fired back-to-back for a single event, doubling volume on the noisiest path -- and uses `heapq.nsmallest` so a pathological fan-out does not sort the whole set just to slice twenty off it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014PWpGFA4Z5qktUj5DW2oiK --- backend/api_v2/api_key_views.py | 4 +- .../tests/test_api_key_create_target.py | 90 ++++++++++++++++++- backend/api_v2/utils.py | 10 ++- backend/pipeline_v2/pipeline_processor.py | 10 ++- .../tests/test_registry_tool_delete_guards.py | 2 + .../prompt_studio_registry_v2/views.py | 31 ++++--- 6 files changed, 126 insertions(+), 21 deletions(-) diff --git a/backend/api_v2/api_key_views.py b/backend/api_v2/api_key_views.py index 3796537163..02b1a76473 100644 --- a/backend/api_v2/api_key_views.py +++ b/backend/api_v2/api_key_views.py @@ -51,7 +51,9 @@ def create(self, request: Request, *args: Any, **kwargs: Any) -> Response: The path target is authoritative: a body naming the *other* target is a contradiction, not an override, and is refused rather than silently - creating a key for whichever one wins. + creating a key for whichever one wins. A body repeating the *same* + target is accepted and overwritten -- it agrees with the path, so + there is nothing to refuse. """ # A JSON array (or scalar) body has no `.copy()` returning a mapping; # reject it as a 400 rather than letting `AttributeError` become a 500. diff --git a/backend/api_v2/tests/test_api_key_create_target.py b/backend/api_v2/tests/test_api_key_create_target.py index 40b3bd1c85..3de6175b62 100644 --- a/backend/api_v2/tests/test_api_key_create_target.py +++ b/backend/api_v2/tests/test_api_key_create_target.py @@ -251,6 +251,22 @@ def __init__(self, target_id: str, owner: _User) -> None: self.owner = owner +MALFORMED_ID = "not-a-uuid" + + +def _lookup(rows: dict[str, _Target] | None, key: Any) -> _Target | None: + """Model the real ``get_api_by_id`` / ``get_pipeline_by_id`` contract. + + ``pk`` is a UUID column, so the ORM raises ``ValidationError`` (not + ``DoesNotExist``) on a non-UUID string. Both helpers catch it and return + ``None``; a stub that quietly returned ``None`` for *every* unknown key + would hide a caller that reintroduces the 500. + """ + if not isinstance(key, str) or key == MALFORMED_ID: + return None + return (rows or {}).get(key) + + def _build_create( *, apis: dict[str, _Target] | None = None, @@ -289,12 +305,12 @@ class _Serializers: class _DeploymentHelper: @staticmethod def get_api_by_id(api_id: str) -> _Target | None: - return (apis or {}).get(api_id) + return _lookup(apis, api_id) class _PipelineProcessor: @staticmethod def get_pipeline_by_id(pipeline_id: str) -> _Target | None: - return (pipelines or {}).get(pipeline_id) + return _lookup(pipelines, pipeline_id) class _View: """Minimal DRF-ish host for the extracted method.""" @@ -467,3 +483,73 @@ def test_no_target_anywhere_is_a_400(self) -> None: with pytest.raises(_ValidationError): view.create(_Req({}, OWNER)) + + def test_malformed_identifier_in_the_body_is_not_a_server_fault(self) -> None: + """A non-UUID body value must 404, not 500. + + The body is read before the serializer runs, so the value goes + straight into ``objects.get(pk=...)``. ``pk`` is a UUID column, which + raises ``ValidationError`` rather than ``DoesNotExist`` -- unhandled, + that surfaces as a 500 with a traceback for ordinary client garbage. + """ + view = _build_create(apis={"api-1": _Target("api-1", OWNER)}) + + with pytest.raises(_NotFound): + view.create(_Req({"api": MALFORMED_ID}, OWNER)) + + def test_malformed_identifier_in_the_path_is_not_a_server_fault(self) -> None: + """Path segments are ````, so they are unvalidated too.""" + view = _build_create(pipelines={"pipe-1": _Target("pipe-1", OWNER)}) + + with pytest.raises(_NotFound): + view.create(_Req({}, OWNER), pipeline_id=MALFORMED_ID) + + +class TestLookupHelpersTolerateMalformedIds: + """The fix lives in the helpers, so pin the contract there. + + Both are reached with unvalidated caller input. Catching only + ``DoesNotExist`` leaves ``ValidationError`` to escape as a 500. + """ + + @staticmethod + def _caught_exceptions(path: Path, marker: str) -> str: + """The ``except (...)`` line of the named function, docstring excluded. + + Asserting on the whole body would match the *prose* explaining why + ``ValidationError`` is caught, so reverting the code while leaving the + comment would pass. + """ + source = path.read_text() + if marker not in source: + pytest.fail(f"Could not find {marker!r} in {path}.") + body = source[source.index(marker) :] + body = body[: body.find("\n @")] if "\n @" in body else body + excepts = [ + line for line in body.splitlines() if line.strip().startswith("except") + ] + if not excepts: + pytest.fail(f"No `except` clause found in {marker!r} ({path}).") + return "\n".join(excepts) + + def test_get_api_by_id_treats_a_malformed_id_as_not_found(self) -> None: + caught = self._caught_exceptions( + BACKEND_DIR / "api_v2" / "utils.py", + " def get_api_by_id(api_id: str) -> APIDeployment | None:", + ) + + assert "ValidationError" in caught, ( + "A non-UUID id raises ValidationError, not DoesNotExist; without " + "catching it, malformed client input becomes a 500" + ) + + def test_get_pipeline_by_id_treats_a_malformed_id_as_not_found(self) -> None: + caught = self._caught_exceptions( + BACKEND_DIR / "pipeline_v2" / "pipeline_processor.py", + " def get_pipeline_by_id(cls, pipeline_id: str) -> Pipeline | None:", + ) + + assert "ValidationError" in caught, ( + "A non-UUID id raises ValidationError, not DoesNotExist; without " + "catching it, malformed client input becomes a 500" + ) diff --git a/backend/api_v2/utils.py b/backend/api_v2/utils.py index 9806afcb9d..ff47091ea1 100644 --- a/backend/api_v2/utils.py +++ b/backend/api_v2/utils.py @@ -1,3 +1,4 @@ +from django.core.exceptions import ValidationError from workflow_manager.workflow_v2.models.execution import WorkflowExecution from api_v2.models import APIDeployment @@ -15,11 +16,18 @@ def get_api_by_id(api_id: str) -> APIDeployment | None: Returns: Optional[APIDeployment]: The APIDeployment instance if found, otherwise None. + + A malformed identifier is "not found", not a fault: ``pk`` is a UUID + column, so a non-UUID string raises ``ValidationError`` out of + ``to_python`` rather than ``DoesNotExist``. Callers reach this with + unvalidated caller input (path segments are ````, and request + bodies are read before the serializer runs), so letting that escape + turns ordinary client garbage into a 500. """ try: api_deployment: APIDeployment = APIDeployment.objects.get(pk=api_id) return api_deployment - except APIDeployment.DoesNotExist: + except (APIDeployment.DoesNotExist, ValidationError): return None @staticmethod diff --git a/backend/pipeline_v2/pipeline_processor.py b/backend/pipeline_v2/pipeline_processor.py index 7ab93c4195..408670e916 100644 --- a/backend/pipeline_v2/pipeline_processor.py +++ b/backend/pipeline_v2/pipeline_processor.py @@ -1,5 +1,6 @@ import logging +from django.core.exceptions import ValidationError from django.utils import timezone from pipeline_v2.exceptions import InactivePipelineError @@ -59,10 +60,17 @@ def get_pipeline_by_id(cls, pipeline_id: str) -> Pipeline | None: ownership, say -- must not be forced through ``get_active_pipeline``, which raises ``InactivePipelineError`` (422) and logs at ERROR for what is an ordinary request against a paused pipeline. + + A malformed identifier is "not found", not a fault: ``pk`` is a UUID + column, so a non-UUID string raises ``ValidationError`` out of + ``to_python`` rather than ``DoesNotExist``. Callers reach this with + unvalidated caller input (path segments are ````, and request + bodies are read before the serializer runs), so letting that escape + turns ordinary client garbage into a 500. """ try: return cls.fetch_pipeline(pipeline_id, check_active=False) - except Pipeline.DoesNotExist: + except (Pipeline.DoesNotExist, ValidationError): return None @staticmethod diff --git a/backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py b/backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py index fdd8233c6e..dba30449b1 100644 --- a/backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py +++ b/backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py @@ -33,6 +33,7 @@ from __future__ import annotations +import heapq import textwrap from pathlib import Path from typing import Any @@ -346,6 +347,7 @@ class _DeploymentType: HUMAN_QUALITY_REVIEW = "Human in the Loop" namespace: dict[str, Any] = { + "heapq": heapq, "_BaseView": _BaseView, "ToolInstance": _Model(_queryset(workflow_ids, required_filters=("tool_id",))), "APIDeployment": _Model(_queryset([1] if api_deployments else [])), diff --git a/backend/prompt_studio/prompt_studio_registry_v2/views.py b/backend/prompt_studio/prompt_studio_registry_v2/views.py index 507c29c76a..2a1ab01117 100644 --- a/backend/prompt_studio/prompt_studio_registry_v2/views.py +++ b/backend/prompt_studio/prompt_studio_registry_v2/views.py @@ -1,3 +1,4 @@ +import heapq import logging from typing import Any @@ -97,25 +98,23 @@ def _refuse_if_in_use(self, instance: PromptStudioRegistry) -> None: return deployment_types = deployment_types_for(dependent_wfs) - # The IDs are what an operator needs to find the blocking rows; the - # slice bounds the line for a pathological fan-out. - blockers = sorted(str(wf) for wf in dependent_wfs) + # The IDs are what an operator needs to find the blocking rows; + # `nsmallest` bounds the work for a pathological fan-out rather than + # sorting the whole set just to slice it. + blockers = heapq.nsmallest( + _LOGGED_WORKFLOW_LIMIT, (str(wf) for wf in dependent_wfs) + ) + # An unresolved deployment type is worth calling out: the deployment + # tables are org-scoped while ``ToolInstance`` is not, so an empty set + # can mean the dependants sit outside the active org rather than that + # the workflows are genuinely undeployed. One line either way. logger.warning( - "Cannot delete exported tool %s, depended by %d workflow(s): %s", + "Cannot delete exported tool %s, depended by %d workflow(s) %s: %s", instance.prompt_registry_id, - len(blockers), - blockers[:_LOGGED_WORKFLOW_LIMIT], + len(dependent_wfs), + sorted(deployment_types) or "(no deployment type resolved)", + blockers, ) - if not deployment_types: - # Distinguishable from "genuinely undeployed": the deployment - # tables are org-scoped while ``ToolInstance`` is not, so an empty - # set here can also mean the dependants sit outside the active org. - logger.warning( - "No deployment type resolved for the %d workflow(s) blocking " - "tool %s; the 409 will carry only the generic wording.", - len(blockers), - instance.prompt_registry_id, - ) raise RegistryToolInUseError(self._in_use_detail(deployment_types)) @staticmethod From 3b0604039de61f053c8236c389c1bf2cf85f9560 Mon Sep 17 00:00:00 2001 From: Hari John Kuriakose Date: Fri, 31 Jul 2026 17:55:31 +0530 Subject: [PATCH 07/14] [TEST] Exercise the malformed-id catch instead of grepping for it Third review round flagged the helper tests as source-text assertions: they matched `"ValidationError" in `, which pins the token rather than the behaviour. They now execute the extracted function bodies against a manager that raises the real `django.core.exceptions.ValidationError`. Django is installed in the unit tier even though settings are unconfigured, so the actual exception class is importable and the `except` clause runs for real. Adds the case the text assertion could not express at all: broadening the catch to a bare `except Exception` -- which would turn a database outage into a silent 404 -- now fails the suite. Verified by mutation: reverting either catch, or over-broadening it, each fails. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014PWpGFA4Z5qktUj5DW2oiK --- .../tests/test_api_key_create_target.py | 155 ++++++++++++++---- 1 file changed, 122 insertions(+), 33 deletions(-) diff --git a/backend/api_v2/tests/test_api_key_create_target.py b/backend/api_v2/tests/test_api_key_create_target.py index 3de6175b62..90aba1efab 100644 --- a/backend/api_v2/tests/test_api_key_create_target.py +++ b/backend/api_v2/tests/test_api_key_create_target.py @@ -43,10 +43,39 @@ from typing import Any import pytest +from django.core.exceptions import ValidationError as DjangoValidationError BACKEND_DIR = Path(__file__).resolve().parents[2] PERMISSION_MODULE = BACKEND_DIR / "permissions" / "permission.py" + +def _extract_defs( + module: Path, markers: tuple[str, ...], stops: tuple[str, ...] +) -> list[str]: + """Slice each named definition out of ``module``'s source. + + ``pytest.fail`` on a missing marker rather than skipping: a rename must + break loudly, since a silently-skipped guard test is worse than none. + """ + source = module.read_text() + parts = [] + for marker in markers: + if marker not in source: + pytest.fail( + f"Could not find {marker!r} in {module}. If it was renamed or " + "inlined, update this test rather than deleting it." + ) + start = source.index(marker) + rest = source[start + len(marker) :] + end = len(rest) + for needle in stops: + found = rest.find(needle) + if found != -1: + end = min(end, found) + parts.append(marker + rest[:end]) + return parts + + START_MARKER = "class IsParentDeploymentOwner(permissions.BasePermission):" END_MARKER = "\nclass " @@ -505,51 +534,111 @@ def test_malformed_identifier_in_the_path_is_not_a_server_fault(self) -> None: view.create(_Req({}, OWNER), pipeline_id=MALFORMED_ID) +class _DoesNotExist(Exception): + """Stand-in for ``Model.DoesNotExist``.""" + + +def _raising_manager(exc: BaseException) -> Any: + """A ``.objects`` whose ``get()`` raises ``exc``.""" + + class _Objects: + @staticmethod + def get(**_: Any) -> Any: + raise exc + + return _Objects() + + class TestLookupHelpersTolerateMalformedIds: """The fix lives in the helpers, so pin the contract there. Both are reached with unvalidated caller input. Catching only ``DoesNotExist`` leaves ``ValidationError`` to escape as a 500. + + The real ``django.core.exceptions.ValidationError`` is used -- Django is + installed in the unit tier even though *settings* are unconfigured, so the + exception class is importable and the ``except`` clause is exercised for + real rather than matched as source text. """ @staticmethod - def _caught_exceptions(path: Path, marker: str) -> str: - """The ``except (...)`` line of the named function, docstring excluded. - - Asserting on the whole body would match the *prose* explaining why - ``ValidationError`` is caught, so reverting the code while leaving the - comment would pass. - """ - source = path.read_text() - if marker not in source: - pytest.fail(f"Could not find {marker!r} in {path}.") - body = source[source.index(marker) :] - body = body[: body.find("\n @")] if "\n @" in body else body - excepts = [ - line for line in body.splitlines() if line.strip().startswith("except") - ] - if not excepts: - pytest.fail(f"No `except` clause found in {marker!r} ({path}).") - return "\n".join(excepts) - - def test_get_api_by_id_treats_a_malformed_id_as_not_found(self) -> None: - caught = self._caught_exceptions( - BACKEND_DIR / "api_v2" / "utils.py", - " def get_api_by_id(api_id: str) -> APIDeployment | None:", + def _build_get_api_by_id(raises: BaseException) -> Any: + marker = " def get_api_by_id(api_id: str) -> APIDeployment | None:" + (body,) = _extract_defs( + BACKEND_DIR / "api_v2" / "utils.py", (marker,), ("\n @",) ) - assert "ValidationError" in caught, ( - "A non-UUID id raises ValidationError, not DoesNotExist; without " - "catching it, malformed client input becomes a 500" - ) + class _APIDeploymentModel: + objects = _raising_manager(raises) + DoesNotExist = _DoesNotExist - def test_get_pipeline_by_id_treats_a_malformed_id_as_not_found(self) -> None: - caught = self._caught_exceptions( + namespace: dict[str, Any] = { + "APIDeployment": _APIDeploymentModel, + "ValidationError": DjangoValidationError, + "Any": Any, + } + exec(compile(textwrap.dedent(body), "utils.py", "exec"), namespace) + return namespace["get_api_by_id"] + + @staticmethod + def _build_get_pipeline_by_id(raises: BaseException) -> Any: + marker = " def get_pipeline_by_id(cls, pipeline_id: str) -> Pipeline | None:" + (body,) = _extract_defs( BACKEND_DIR / "pipeline_v2" / "pipeline_processor.py", - " def get_pipeline_by_id(cls, pipeline_id: str) -> Pipeline | None:", + (marker,), + ("\n @",), ) - assert "ValidationError" in caught, ( - "A non-UUID id raises ValidationError, not DoesNotExist; without " - "catching it, malformed client input becomes a 500" + class _PipelineModel: + DoesNotExist = _DoesNotExist + + class _Cls: + @staticmethod + def fetch_pipeline(pipeline_id: str, check_active: bool = True) -> Any: + raise raises + + namespace: dict[str, Any] = { + "Pipeline": _PipelineModel, + "ValidationError": DjangoValidationError, + "Any": Any, + } + exec(compile(textwrap.dedent(body), "pipeline_processor.py", "exec"), namespace) + return lambda pipeline_id: namespace["get_pipeline_by_id"](_Cls, pipeline_id) + + def test_get_api_by_id_returns_none_for_a_malformed_id(self) -> None: + """A non-UUID id raises ValidationError, not DoesNotExist. + + Left uncaught it escapes the view as a 500 with a traceback, for what + is ordinary client garbage. + """ + get_api_by_id = self._build_get_api_by_id(DjangoValidationError("bad uuid")) + + assert get_api_by_id(MALFORMED_ID) is None + + def test_get_api_by_id_still_returns_none_when_the_row_is_absent(self) -> None: + get_api_by_id = self._build_get_api_by_id(_DoesNotExist()) + + assert get_api_by_id("api-1") is None + + def test_get_pipeline_by_id_returns_none_for_a_malformed_id(self) -> None: + get_pipeline_by_id = self._build_get_pipeline_by_id( + DjangoValidationError("bad uuid") ) + + assert get_pipeline_by_id(MALFORMED_ID) is None + + def test_get_pipeline_by_id_still_returns_none_when_the_row_is_absent(self) -> None: + get_pipeline_by_id = self._build_get_pipeline_by_id(_DoesNotExist()) + + assert get_pipeline_by_id("pipe-1") is None + + def test_an_unexpected_error_is_not_swallowed(self) -> None: + """The catch must stay narrow -- a real fault must still surface. + + Broadening to a bare ``except Exception`` would turn database outages + into silent 404s. + """ + get_api_by_id = self._build_get_api_by_id(RuntimeError("database is down")) + + with pytest.raises(RuntimeError): + get_api_by_id("api-1") From 00165fcae44fcd65e4b81e223b7fc8a4457306da Mon Sep 17 00:00:00 2001 From: Hari John Kuriakose Date: Fri, 31 Jul 2026 18:13:08 +0530 Subject: [PATCH 08/14] [REFACTOR] Share the test source-extraction helper Behaviour-preserving cleanup, kept on its own commit so it reads apart from the security fixes. No reviewed source file changes: `api_key_views.py`, `utils.py`, `permission.py`, `pipeline_processor.py`, `tool_usage.py` and `prompt_studio_registry_v2/views.py` are byte-identical to 3b0604039. The two test modules had grown four near-identical copies of the same slice-and-`pytest.fail` extraction loop. Those move to `backend/tests_common/source_extraction.py`, alongside an `exec_def` for the extract-dedent-exec sequence both lookup-helper builders were open-coding. Placed in a `tests_common` package rather than at the backend root: the module imports `pytest`, so it does not belong beside `manage.py`. The repo's existing precedent (`permissions/tests/base.py`) is app-scoped, which does not fit helpers consumed from two different apps. Verified the import resolves under both `cd backend && pytest ...` and repo-root `pytest backend/...`. Also drops a dead sentinel and an unused marker constant from the api-key tests, and trims three docstrings in `prompt_studio_core_v2/views.py` that restated their signatures -- one now fronts a single-line pass-through. Re-ran the security mutations after the refactor; all four still fail the suite (body-only IDOR, fail-open authz, unwired destroy guard, broadened catch). 41 pass. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014PWpGFA4Z5qktUj5DW2oiK --- .../tests/test_api_key_create_target.py | 103 ++++-------------- .../prompt_studio_core_v2/views.py | 26 +---- .../tests/test_registry_tool_delete_guards.py | 74 ++++--------- backend/tests_common/__init__.py | 6 + backend/tests_common/source_extraction.py | 59 ++++++++++ 5 files changed, 113 insertions(+), 155 deletions(-) create mode 100644 backend/tests_common/__init__.py create mode 100644 backend/tests_common/source_extraction.py diff --git a/backend/api_v2/tests/test_api_key_create_target.py b/backend/api_v2/tests/test_api_key_create_target.py index 90aba1efab..83ab99f98a 100644 --- a/backend/api_v2/tests/test_api_key_create_target.py +++ b/backend/api_v2/tests/test_api_key_create_target.py @@ -44,40 +44,12 @@ import pytest from django.core.exceptions import ValidationError as DjangoValidationError +from tests_common.source_extraction import exec_def, extract_defs BACKEND_DIR = Path(__file__).resolve().parents[2] PERMISSION_MODULE = BACKEND_DIR / "permissions" / "permission.py" - -def _extract_defs( - module: Path, markers: tuple[str, ...], stops: tuple[str, ...] -) -> list[str]: - """Slice each named definition out of ``module``'s source. - - ``pytest.fail`` on a missing marker rather than skipping: a rename must - break loudly, since a silently-skipped guard test is worse than none. - """ - source = module.read_text() - parts = [] - for marker in markers: - if marker not in source: - pytest.fail( - f"Could not find {marker!r} in {module}. If it was renamed or " - "inlined, update this test rather than deleting it." - ) - start = source.index(marker) - rest = source[start + len(marker) :] - end = len(rest) - for needle in stops: - found = rest.find(needle) - if found != -1: - end = min(end, found) - parts.append(marker + rest[:end]) - return parts - - START_MARKER = "class IsParentDeploymentOwner(permissions.BasePermission):" -END_MARKER = "\nclass " class _User: @@ -119,15 +91,7 @@ def __init__(self, api: Any = None, pipeline: Any = None, owner: Any = None) -> def _build_permission(*, org_admins: set[str]) -> Any: """Extract the real ``IsParentDeploymentOwner`` against stubbed collaborators.""" - source = PERMISSION_MODULE.read_text() - if START_MARKER not in source: - pytest.fail( - f"Could not find {START_MARKER!r} in {PERMISSION_MODULE}. If it was " - "renamed, update this test rather than deleting it." - ) - rest = source[source.index(START_MARKER) :] - next_class = rest.find(END_MARKER, len(START_MARKER)) - body = textwrap.dedent(rest if next_class == -1 else rest[:next_class]) + (body,) = extract_defs(PERMISSION_MODULE, (START_MARKER,), ("\nclass ",)) class _BasePermission: pass @@ -256,9 +220,6 @@ def __init__(self, owner: _User) -> None: " def create(self, request: Request, *args: Any, **kwargs: Any) -> Response:" ) -CREATED = object() -"""Sentinel proving the key was actually minted.""" - class _ValidationError(Exception): """Stand-in for ``serializers.ValidationError`` (a 400).""" @@ -311,20 +272,8 @@ def _build_create( inverted ``isinstance`` guard and against the wrong object being handed to ``check_object_permissions``. """ - source = VIEW_MODULE.read_text() - if CREATE_MARKER not in source: - pytest.fail( - f"Could not find {CREATE_MARKER!r} in {VIEW_MODULE}. If the " - "signature changed, update this test rather than deleting it." - ) - start = source.index(CREATE_MARKER) - rest = source[start + len(CREATE_MARKER) :] - end = len(rest) - for needle in ("\n def ", "\n @"): - found = rest.find(needle) - if found != -1: - end = min(end, found) - body = textwrap.dedent(CREATE_MARKER + rest[:end]) + (body,) = extract_defs(VIEW_MODULE, (CREATE_MARKER,), ("\n def ", "\n @")) + body = textwrap.dedent(body) checked: list[Any] = [] @@ -563,32 +512,24 @@ class TestLookupHelpersTolerateMalformedIds: @staticmethod def _build_get_api_by_id(raises: BaseException) -> Any: - marker = " def get_api_by_id(api_id: str) -> APIDeployment | None:" - (body,) = _extract_defs( - BACKEND_DIR / "api_v2" / "utils.py", (marker,), ("\n @",) - ) - class _APIDeploymentModel: objects = _raising_manager(raises) DoesNotExist = _DoesNotExist - namespace: dict[str, Any] = { - "APIDeployment": _APIDeploymentModel, - "ValidationError": DjangoValidationError, - "Any": Any, - } - exec(compile(textwrap.dedent(body), "utils.py", "exec"), namespace) + namespace = exec_def( + BACKEND_DIR / "api_v2" / "utils.py", + " def get_api_by_id(api_id: str) -> APIDeployment | None:", + ("\n @",), + { + "APIDeployment": _APIDeploymentModel, + "ValidationError": DjangoValidationError, + "Any": Any, + }, + ) return namespace["get_api_by_id"] @staticmethod def _build_get_pipeline_by_id(raises: BaseException) -> Any: - marker = " def get_pipeline_by_id(cls, pipeline_id: str) -> Pipeline | None:" - (body,) = _extract_defs( - BACKEND_DIR / "pipeline_v2" / "pipeline_processor.py", - (marker,), - ("\n @",), - ) - class _PipelineModel: DoesNotExist = _DoesNotExist @@ -597,12 +538,16 @@ class _Cls: def fetch_pipeline(pipeline_id: str, check_active: bool = True) -> Any: raise raises - namespace: dict[str, Any] = { - "Pipeline": _PipelineModel, - "ValidationError": DjangoValidationError, - "Any": Any, - } - exec(compile(textwrap.dedent(body), "pipeline_processor.py", "exec"), namespace) + namespace = exec_def( + BACKEND_DIR / "pipeline_v2" / "pipeline_processor.py", + " def get_pipeline_by_id(cls, pipeline_id: str) -> Pipeline | None:", + ("\n @",), + { + "Pipeline": _PipelineModel, + "ValidationError": DjangoValidationError, + "Any": Any, + }, + ) return lambda pipeline_id: namespace["get_pipeline_by_id"](_Cls, pipeline_id) def test_get_api_by_id_returns_none_for_a_malformed_id(self) -> None: diff --git a/backend/prompt_studio/prompt_studio_core_v2/views.py b/backend/prompt_studio/prompt_studio_core_v2/views.py index ad93b825ca..31e4ca42bb 100644 --- a/backend/prompt_studio/prompt_studio_core_v2/views.py +++ b/backend/prompt_studio/prompt_studio_core_v2/views.py @@ -228,13 +228,9 @@ def perform_destroy(self, instance: CustomTool) -> None: instance.delete(organization_id) def _check_tool_usage_in_workflows(self, instance: CustomTool) -> tuple[bool, set]: - """Check if a tool is being used in any workflows. + """Whether ``instance``'s exported tool is in use, and by which workflows. - Args: - instance: The CustomTool instance to check - - Returns: - Tuple of (is_used: bool, dependent_workflows: set) + An unexported project has no registry row and so no dependants. """ registry = getattr(instance, "prompt_studio_registries", None) if not registry: @@ -244,25 +240,11 @@ def _check_tool_usage_in_workflows(self, instance: CustomTool) -> tuple[bool, se return bool(dependent_wfs), dependent_wfs def _get_deployment_types(self, workflow_ids: set) -> set: - """Get all deployment types where the tool is used. - - Args: - workflow_ids: Set of workflow IDs to check - - Returns: - Set of deployment type strings - """ + """Deployment kinds reachable from ``workflow_ids``.""" return deployment_types_for(workflow_ids) def _format_deployment_types_message(self, deployment_types: set) -> str: - """Format deployment types into human-readable message. - - Args: - deployment_types: Set of deployment type strings - - Returns: - Formatted message string or empty string if no types - """ + """Render the "re-export needed" notice, or "" when nothing is deployed.""" types_text = join_deployment_types(deployment_types) if not types_text: return "" diff --git a/backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py b/backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py index dba30449b1..c7707a1785 100644 --- a/backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py +++ b/backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py @@ -34,11 +34,11 @@ from __future__ import annotations import heapq -import textwrap from pathlib import Path from typing import Any import pytest +from tests_common.source_extraction import extract_defs BACKEND_DIR = Path(__file__).resolve().parents[3] PERMISSION_MODULE = BACKEND_DIR / "prompt_studio" / "permission.py" @@ -81,13 +81,7 @@ def __init__(self, user: _User) -> None: def _build_permission(*, org_admins: set[str]) -> Any: """Extract the real ``IsRegistryToolOwner`` against stubbed collaborators.""" - source = PERMISSION_MODULE.read_text() - if START_MARKER not in source: - pytest.fail( - f"Could not find {START_MARKER!r} in {PERMISSION_MODULE}. If it was " - "renamed, update this test rather than deleting it." - ) - body = textwrap.dedent(source[source.index(START_MARKER) :]) + (body,) = extract_defs(PERMISSION_MODULE, (START_MARKER,), ("\nclass ",)) class _BasePermission: pass @@ -228,13 +222,10 @@ def __init__(self, matched: bool = False) -> None: def filter(self, **kwargs: Any) -> _QS: if required_filters and not all(k in kwargs for k in required_filters): - return _QS.__new__(_QS)._empty() + # `required_filters` is non-empty here, so this is unmatched. + return _QS() return _QS(matched=True) - def _empty(self) -> _QS: - self._matched = False - return self - def values_list(self, *_: Any, **__: Any) -> _QS: return self @@ -266,29 +257,21 @@ def destroy(self, request: Any, *args: Any, **kwargs: Any) -> Any: return DELETED -def _extract(module: Path, markers: tuple[str, ...], stops: tuple[str, ...]) -> list[str]: - """Slice each named definition out of ``module``'s source. +def _logged_workflow_limit() -> int: + """Read the real cap rather than restating it.""" + source = VIEWS_MODULE.read_text() + marker = "_LOGGED_WORKFLOW_LIMIT = " + if marker not in source: + pytest.fail(f"Could not find {marker!r} in {VIEWS_MODULE}.") + return int(source.split(marker)[1].split("\n")[0].strip()) + + +class _SilentLogger: + def info(self, *_: Any, **__: Any) -> None: + pass - ``pytest.fail`` on a missing marker rather than skipping: a rename must - break loudly, since a silently-skipped guard test is worse than none. - """ - source = module.read_text() - parts = [] - for marker in markers: - if marker not in source: - pytest.fail( - f"Could not find {marker!r} in {module}. If it was renamed or " - "inlined, update this test rather than deleting it." - ) - start = source.index(marker) - rest = source[start + len(marker) :] - end = len(rest) - for needle in stops: - found = rest.find(needle) - if found != -1: - end = min(end, found) - parts.append(marker + rest[:end]) - return parts + def warning(self, *_: Any, **__: Any) -> None: + pass def _build_guard( @@ -309,8 +292,8 @@ def _build_guard( wiring unpinned: dropping that one line made in-use tools deletable with the whole suite still green. """ - view_parts = _extract(VIEWS_MODULE, GUARD_MARKERS, ("\n def ", "\n @")) - helper_parts = _extract(TOOL_USAGE_MODULE, HELPER_MARKERS, ("\ndef ",)) + view_parts = extract_defs(VIEWS_MODULE, GUARD_MARKERS, ("\n def ", "\n @")) + helper_parts = extract_defs(TOOL_USAGE_MODULE, HELPER_MARKERS, ("\ndef ",)) body = ( "\n".join(helper_parts) @@ -366,23 +349,6 @@ class _DeploymentType: return namespace["_Guard"]() -def _logged_workflow_limit() -> int: - """Read the real cap rather than restating it.""" - source = VIEWS_MODULE.read_text() - marker = "_LOGGED_WORKFLOW_LIMIT = " - if marker not in source: - pytest.fail(f"Could not find {marker!r} in {VIEWS_MODULE}.") - return int(source.split(marker)[1].split("\n")[0].strip()) - - -class _SilentLogger: - def info(self, *_: Any, **__: Any) -> None: - pass - - def warning(self, *_: Any, **__: Any) -> None: - pass - - class _Instance: def __init__(self) -> None: self.pk = "tool-1" diff --git a/backend/tests_common/__init__.py b/backend/tests_common/__init__.py new file mode 100644 index 0000000000..93a02246b0 --- /dev/null +++ b/backend/tests_common/__init__.py @@ -0,0 +1,6 @@ +"""Helpers shared between test modules in different Django apps. + +Kept out of any one app because its consumers span several +(``api_v2``, ``prompt_studio``); ``permissions/tests/base.py`` is the +app-scoped equivalent for helpers with a single consumer. +""" diff --git a/backend/tests_common/source_extraction.py b/backend/tests_common/source_extraction.py new file mode 100644 index 0000000000..0921f5638c --- /dev/null +++ b/backend/tests_common/source_extraction.py @@ -0,0 +1,59 @@ +"""Run real method bodies in the unit tier, where Django settings are absent. + +Guard logic worth pinning (authorization, in-use refusal) lives in modules that +import Django at module scope, so the unit tier cannot import them. Rather than +restate the logic in a test -- which passes no matter what the real code does -- +these helpers slice the definition out of the source file and ``exec`` it +against stubbed collaborators, so the assertions run against the shipped body. + +The end-to-end request cycle needs a database and belongs in the integration +tier; ``backend/conftest.py`` auto-marks those. +""" + +from __future__ import annotations + +import textwrap +from pathlib import Path +from typing import Any + +import pytest + + +def extract_defs( + module: Path, markers: tuple[str, ...], stops: tuple[str, ...] +) -> list[str]: + """Slice each named definition out of ``module``'s source. + + Each marker is the definition's opening line, verbatim; the slice runs to + the first ``stops`` needle after it. ``pytest.fail`` on a missing marker + rather than skipping: a rename must break loudly, since a silently-skipped + guard test is worse than none. + """ + source = module.read_text() + parts = [] + for marker in markers: + if marker not in source: + pytest.fail( + f"Could not find {marker!r} in {module}. If it was renamed or " + "inlined, update this test rather than deleting it." + ) + start = source.index(marker) + rest = source[start + len(marker) :] + end = len(rest) + for needle in stops: + found = rest.find(needle) + if found != -1: + end = min(end, found) + parts.append(marker + rest[:end]) + return parts + + +def exec_def(module: Path, marker: str, stops: tuple[str, ...], namespace: Any) -> Any: + """Extract one definition, ``exec`` it in ``namespace``, and hand it back. + + The body is dedented so a method can be executed at module level, which is + what lets a view method be driven without standing up the class. + """ + (body,) = extract_defs(module, (marker,), stops) + exec(compile(textwrap.dedent(body), str(module), "exec"), namespace) + return namespace From 9e255db8a1dc0e58f4be896a129a59906a27d8a3 Mon Sep 17 00:00:00 2001 From: Hari John Kuriakose Date: Fri, 31 Jul 2026 18:23:35 +0530 Subject: [PATCH 09/14] [TEST] Keep extracted-body tracebacks pointing at real lines `exec_def` handed `compile` the real module path while the dedented snippet started at line 1, so any frame from an extracted body paired a genuine filename with a snippet-relative line. A fault in `get_pipeline_by_id` cited `pipeline_processor.py:18` -- inside an unrelated function's docstring -- for a statement that lives at :72. Anyone debugging a future guard-test failure was pointed at the wrong code with no hint the citation was bogus. Padding the snippet with blank lines to its real offset fixes it: the same fault now cites :72 with the correct source line. Correcting the previous commit message while I am here: it said "no reviewed source file changes", listing six files. That was accurate as far as it went, but `prompt_studio_core_v2/views.py` is also production source and *is* in that diff -- docstrings only, no executable statement touched. The narrower claim is what I should have written. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014PWpGFA4Z5qktUj5DW2oiK --- backend/tests_common/source_extraction.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/backend/tests_common/source_extraction.py b/backend/tests_common/source_extraction.py index 0921f5638c..be3c598307 100644 --- a/backend/tests_common/source_extraction.py +++ b/backend/tests_common/source_extraction.py @@ -48,12 +48,23 @@ def extract_defs( return parts +def _line_offset(source: str, marker: str) -> int: + """0-based line on which ``marker`` starts.""" + return source.count("\n", 0, source.index(marker)) + + def exec_def(module: Path, marker: str, stops: tuple[str, ...], namespace: Any) -> Any: """Extract one definition, ``exec`` it in ``namespace``, and hand it back. The body is dedented so a method can be executed at module level, which is what lets a view method be driven without standing up the class. + + Blank lines are prepended so the snippet sits at its real line number. + ``compile`` is given the true path, so without the padding a traceback + would pair a genuine filename with a snippet-relative line -- pointing + whoever debugs a failure at whatever unrelated source sits there. """ (body,) = extract_defs(module, (marker,), stops) - exec(compile(textwrap.dedent(body), str(module), "exec"), namespace) + padding = "\n" * _line_offset(module.read_text(), marker) + exec(compile(padding + textwrap.dedent(body), str(module), "exec"), namespace) return namespace From d01703975ad8b937d33eb31aad1f329679531f68 Mon Sep 17 00:00:00 2001 From: Hari John Kuriakose Date: Mon, 3 Aug 2026 15:49:08 +0530 Subject: [PATCH 10/14] [FIX] Pin route/permission wiring; guard get_active_pipeline (F1, F2) F1 (Critical): the guard suites drive method bodies extracted from source, so they assert what a function does but not that anything reaches it. Four mutations reopened real holes with all 41 tests green: deleting the registry DELETE route, unwiring IsRegistryToolOwner from destroy, moving create() into a dead class (reopening the IDOR), and dropping the `status` import (NameError on every key creation). Adds tests_common/test_route_wiring.py, which asserts binding rather than behaviour -- verb->action maps, that create() is overridden on the viewset rather than inherited from ModelViewSet, that it calls check_object_permissions, and that every global create() loads resolves in its module. Each of the four mutations now fails it; verified by executing them. F2 (High): get_active_pipeline caught only DoesNotExist while its new sibling get_pipeline_by_id caught ValidationError too. A non-UUID pk raises ValidationError out of to_python, and this path is reached *unauthenticated* -- the public execution endpoint looks the pipeline up before validating the API key -- so any non-UUID path segment forced a 500. One-line catch widened, with tests covering both lookups written the conventional way (real import + patch). Both test files run in the unit tier with no database. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DJWqb9Nq6aFbUFn62MMZc2 --- backend/pipeline_v2/pipeline_processor.py | 14 +- .../tests/test_pipeline_lookup_guards.py | 66 ++++++++ backend/tests_common/test_route_wiring.py | 158 ++++++++++++++++++ 3 files changed, 236 insertions(+), 2 deletions(-) create mode 100644 backend/pipeline_v2/tests/test_pipeline_lookup_guards.py create mode 100644 backend/tests_common/test_route_wiring.py diff --git a/backend/pipeline_v2/pipeline_processor.py b/backend/pipeline_v2/pipeline_processor.py index 408670e916..e774051a9d 100644 --- a/backend/pipeline_v2/pipeline_processor.py +++ b/backend/pipeline_v2/pipeline_processor.py @@ -45,10 +45,20 @@ def fetch_pipeline(pipeline_id: str, check_active: bool = True) -> Pipeline: @classmethod def get_active_pipeline(cls, pipeline_id: str) -> Pipeline | None: - """Retrieves a list of active pipelines.""" + """Retrieve a pipeline, requiring it to be active. + + Raises ``InactivePipelineError`` (422) when the row exists but is + paused; use :meth:`get_pipeline_by_id` when merely identifying the row. + + A malformed identifier is "not found", not a fault -- see + :meth:`get_pipeline_by_id` for why ``ValidationError`` is caught here. + This path is reached *unauthenticated*: the public execution endpoint + looks the pipeline up before validating the API key, so letting it + escape turns any non-UUID path segment into a 500. + """ try: return cls.fetch_pipeline(pipeline_id, check_active=True) - except Pipeline.DoesNotExist: + except (Pipeline.DoesNotExist, ValidationError): return None @classmethod diff --git a/backend/pipeline_v2/tests/test_pipeline_lookup_guards.py b/backend/pipeline_v2/tests/test_pipeline_lookup_guards.py new file mode 100644 index 0000000000..71062684c7 --- /dev/null +++ b/backend/pipeline_v2/tests/test_pipeline_lookup_guards.py @@ -0,0 +1,66 @@ +"""Both pipeline lookups must treat a malformed identifier as "not found". + +``Pipeline.pk`` is a UUID column, so a non-UUID string raises Django's +``ValidationError`` out of ``to_python`` rather than ``DoesNotExist``. Left +uncaught it escapes as a 500 for what is ordinary client garbage. + +This matters most on ``get_active_pipeline``, which is reached *before* +authentication: ``BaseAPIKeyValidator`` checks only that some ``Bearer`` string +is present, then ``PipelineDeploymentHelper.validate_and_process`` looks the +pipeline up and only afterwards validates the key. So an unauthenticated caller +sending any non-UUID path segment to the public execution endpoint could force a +500 with a traceback. + +The real module is imported and its collaborators patched (Django is loaded by +the rig's test env), so no database is touched and these stay in the unit tier. +""" + +from __future__ import annotations + +from unittest.mock import patch + +import pytest +from django.core.exceptions import ValidationError + +from pipeline_v2.models import Pipeline +from pipeline_v2.pipeline_processor import PipelineProcessor + +MALFORMED_ID = "not-a-uuid" + + +class TestMalformedIdentifierIsNotAServerFault: + """A non-UUID id resolves to ``None``, never an unhandled exception.""" + + @pytest.mark.parametrize( + "lookup", + ["get_active_pipeline", "get_pipeline_by_id"], + ) + def test_validation_error_becomes_none(self, lookup: str) -> None: + with patch.object( + PipelineProcessor, "fetch_pipeline", side_effect=ValidationError("bad uuid") + ): + assert getattr(PipelineProcessor, lookup)(MALFORMED_ID) is None + + @pytest.mark.parametrize( + "lookup", + ["get_active_pipeline", "get_pipeline_by_id"], + ) + def test_absent_row_still_becomes_none(self, lookup: str) -> None: + with patch.object( + PipelineProcessor, "fetch_pipeline", side_effect=Pipeline.DoesNotExist + ): + assert getattr(PipelineProcessor, lookup)(MALFORMED_ID) is None + + @pytest.mark.parametrize( + "lookup", + ["get_active_pipeline", "get_pipeline_by_id"], + ) + def test_an_unexpected_error_is_not_swallowed(self, lookup: str) -> None: + """The catch must stay narrow -- a database outage is not a 404.""" + with patch.object( + PipelineProcessor, + "fetch_pipeline", + side_effect=RuntimeError("database is down"), + ): + with pytest.raises(RuntimeError): + getattr(PipelineProcessor, lookup)(MALFORMED_ID) diff --git a/backend/tests_common/test_route_wiring.py b/backend/tests_common/test_route_wiring.py new file mode 100644 index 0000000000..2ed9077162 --- /dev/null +++ b/backend/tests_common/test_route_wiring.py @@ -0,0 +1,158 @@ +"""Pin that the guarded routes this PR adds are actually *wired*. + +The behavioural suites for these two features drive method bodies extracted +from source (``tests_common.source_extraction``), which asserts what a function +does but not that anything reaches it. That leaves a blind spot the guards +themselves cannot cover: code present in the file but unreachable in production +is indistinguishable from code that is dispatched. Deleting the registry DELETE +route, or unwiring ``IsRegistryToolOwner`` from ``destroy``, both left those +suites fully green while reopening the exact holes they exist to close. + +These tests close that spot from the other side -- they assert the *binding* +and nothing about behaviour: + +* the URLconf maps the expected HTTP verb to the expected action, and +* the viewset resolves the expected permission class for the destructive action. + +They import the real modules rather than extracting source, so a deleted or +renamed import in either view module fails here at collection time. No database +is touched: URL resolution and ``get_permissions()`` are pure, so these stay in +the per-PR unit tier rather than being auto-marked ``integration`` by +``backend/conftest.py``. +""" + +from __future__ import annotations + +import ast +import builtins +import inspect +import sys + +from django.urls import resolve, reverse + +from api_v2.api_key_views import APIKeyViewSet +from permissions.permission import IsParentDeploymentOwner +from prompt_studio.permission import IsRegistryToolOwner +from prompt_studio.prompt_studio_registry_v2.views import PromptStudioRegistryView + +REGISTRY_URLCONF = "prompt_studio.prompt_studio_registry_v2.urls" +API_URLCONF = "api_v2.urls" + +_SAMPLE_UUID = "00000000-0000-0000-0000-000000000001" + + +def _actions(name: str, urlconf: str, **kwargs: str) -> dict[str, str]: + """Verb -> action map the URLconf binds for ``name``.""" + url = reverse(name, kwargs=kwargs, urlconf=urlconf) + return resolve(url, urlconf=urlconf).func.actions + + +class TestRegistryDeleteRouteIsWired: + """``DELETE registry//`` -- the route and its authorization gate.""" + + def test_delete_verb_is_bound_to_destroy(self) -> None: + """Removing the route entirely left the guard suite green.""" + assert _actions( + "prompt_studio_registry_detail", REGISTRY_URLCONF, pk=_SAMPLE_UUID + ) == {"delete": "destroy"} + + def test_destroy_resolves_the_owner_permission(self) -> None: + """Unwiring the gate left ``destroy`` open to any org member.""" + view = PromptStudioRegistryView() + view.action = "destroy" + + assert any( + isinstance(perm, IsRegistryToolOwner) for perm in view.get_permissions() + ) + + def test_list_is_not_gated_by_the_owner_permission(self) -> None: + """Read visibility is derived by ``list_tools``; only deletes narrow.""" + view = PromptStudioRegistryView() + view.action = "list" + + assert not any( + isinstance(perm, IsRegistryToolOwner) for perm in view.get_permissions() + ) + + +class TestApiKeyRoutesAreWired: + """``keys/{api,pipeline}/...`` -- the routes whose ``create`` closes an IDOR.""" + + def test_api_path_route_binds_post_to_create(self) -> None: + """``create`` moved out of the viewset left the create suite green.""" + assert _actions("api_key_api", API_URLCONF, api_id="api-1")["post"] == "create" + + def test_pipeline_path_route_binds_post_to_create(self) -> None: + assert ( + _actions("api_key_pipeline", API_URLCONF, pipeline_id="pipeline-1")["post"] + == "create" + ) + + def test_body_only_routes_bind_post_to_create(self) -> None: + """The body-only routes reach the same guarded ``create``.""" + assert _actions("api_keys_api", API_URLCONF)["post"] == "create" + assert _actions("api_keys_pipeline", API_URLCONF)["post"] == "create" + + def test_create_is_overridden_on_the_viewset(self) -> None: + """``create`` must be *this* viewset's, not ``ModelViewSet``'s default. + + ``callable(APIKeyViewSet.create)`` is not enough: the inherited + ``CreateModelMixin.create`` satisfies it, so moving the override to a + dead class -- reopening the IDOR it closes -- would still pass. Assert + the override is defined in the class body itself. + """ + assert "create" in vars(APIKeyViewSet) + + def test_every_global_create_uses_resolves(self) -> None: + """A deleted or renamed import 500s ``create`` at request time only. + + ``create`` reaches ``status``/``Response``/``serializers`` on its + success path, so a missing import raises ``NameError`` per request + rather than at import time -- invisible to a test that merely imports + the module. Resolve each global the body loads instead. + """ + module = sys.modules[APIKeyViewSet.__module__] + source = inspect.getsource(module) + (func,) = [ + node + for node in ast.walk(ast.parse(source)) + if isinstance(node, ast.FunctionDef) and node.name == "create" + ] + loaded = { + node.id + for node in ast.walk(func) + if isinstance(node, ast.Name) and isinstance(node.ctx, ast.Load) + } + local = {"self", "request", "args", "kwargs"} | { + node.id + for node in ast.walk(func) + if isinstance(node, ast.Name) and isinstance(node.ctx, ast.Store) + } + + unresolved = [ + name + for name in sorted(loaded - local) + if not hasattr(module, name) and not hasattr(builtins, name) + ] + assert not unresolved, f"create() references unimported names: {unresolved}" + + def test_create_object_checks_its_target(self) -> None: + """The IDOR fix is ``create`` calling ``check_object_permissions``. + + ``create`` is collection-level, so DRF never calls ``get_object()`` and + ``has_object_permission`` never runs on its own. The override must make + that call itself, or any org member can mint a key for a deployment + they do not own. + """ + source = inspect.getsource(vars(APIKeyViewSet)["create"]) + + assert "check_object_permissions" in source + + def test_create_resolves_the_parent_deployment_permission(self) -> None: + """``create`` object-checks the target; the class must be resolved for it.""" + view = APIKeyViewSet() + view.action = "create" + + assert any( + isinstance(perm, IsParentDeploymentOwner) for perm in view.get_permissions() + ) From 655881bb81d631f08c33424ff39d431a49a7ff13 Mon Sep 17 00:00:00 2001 From: Hari John Kuriakose Date: Mon, 3 Aug 2026 16:05:04 +0530 Subject: [PATCH 11/14] [FIX] Refuse same-field target mismatch; correct test-technique claims (F3-F8) F4 (Medium): `POST keys/api//` with body `{"api": "B"}` hit neither contradiction guard -- they only cross-check the *other* field -- so the body value was silently overwritten and a live key minted for A while the caller named B. Refused symmetrically now. An empty body value is still not a disagreement, so `{"api": ""}` keeps working. F3 (High, partial): two docstring claims were false. The cited precedent (test_build_index_payload.py) imports its module and patches collaborators -- the opposite of source extraction -- and says so in its own docstring; and Django *is* importable in the unit tier, since tests/groups.yaml sets DJANGO_SETTINGS_MODULE for unit-backend and conftest.py auto-marks only django_db/TestCase tests as integration. Corrected, and the technique's blind spots are now stated where they are relied on: unreachable code looks wired, a missing import is invisible, an inserted decorator silently truncates a slice, and a cosmetic annotation change breaks the marker. Retiring the technique outright rewrites both suites and is left to its own change. F6 (Medium): get_queryset returned None when no filter args were supplied, which DRF hands straight to filter_queryset and the paginator -- an unfiltered `list` 500ed instead of returning nothing. Returns none() and the signature drops `| None`. F7 (Medium): an unresolved deployment type can mean the dependants sit in another organization (ToolInstance is not org-scoped; the deployment tables are), so the 409 told users to remove usages they cannot see. The fallback wording now says so. F8 (Medium): the ValidationError arms of both lookups now log at debug, so a malformed identifier stays distinguishable from an absent row when triaging -- both still answer 404. Backend unit tier: 337 passed (318 before), ruff 0.3.4 clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DJWqb9Nq6aFbUFn62MMZc2 --- backend/api_v2/api_key_views.py | 23 +++++++++- .../tests/test_api_key_create_target.py | 46 ++++++++++++++++++- backend/api_v2/utils.py | 12 ++++- backend/pipeline_v2/pipeline_processor.py | 13 +++++- .../tests/test_registry_tool_delete_guards.py | 28 ++++++++--- .../prompt_studio_registry_v2/views.py | 28 +++++++---- backend/tests_common/source_extraction.py | 32 +++++++++---- backend/tests_common/test_route_wiring.py | 7 ++- 8 files changed, 154 insertions(+), 35 deletions(-) diff --git a/backend/api_v2/api_key_views.py b/backend/api_v2/api_key_views.py index 02b1a76473..69307f2c0d 100644 --- a/backend/api_v2/api_key_views.py +++ b/backend/api_v2/api_key_views.py @@ -52,8 +52,12 @@ def create(self, request: Request, *args: Any, **kwargs: Any) -> Response: The path target is authoritative: a body naming the *other* target is a contradiction, not an override, and is refused rather than silently creating a key for whichever one wins. A body repeating the *same* - target is accepted and overwritten -- it agrees with the path, so - there is nothing to refuse. + target with the *same* value is accepted and overwritten -- it agrees + with the path, so there is nothing to refuse. A body naming the same + field with a *different* value is refused for the same reason as the + cross-type case: silently minting a key for the path's resource while + the caller named another one is a wrong-resource credential, not a + harmless override. """ # A JSON array (or scalar) body has no `.copy()` returning a mapping; # reject it as a 400 rather than letting `AttributeError` become a 500. @@ -81,6 +85,21 @@ def create(self, request: Request, *args: Any, **kwargs: Any) -> Response: } ) + # Same field, different value: the caller named one resource in the + # path and another in the body. Overwriting silently would mint a live + # key for a resource they did not ask for -- refuse, as for the + # cross-type contradictions above. An empty body value is not a + # disagreement; it simply does not name anything. + for field, path_value in (("api", api_id), ("pipeline", pipeline_id)): + body_value = request_data.get(field) + if path_value and body_value and str(body_value) != str(path_value): + raise serializers.ValidationError( + { + field: f"`{field}` in the body names a different resource " + "than the URL; remove it or make the two agree." + } + ) + # The path wins where it names a target; otherwise fall back to the # body, so the body-only routes resolve to the same guarded path. api_id = api_id or request_data.get("api") diff --git a/backend/api_v2/tests/test_api_key_create_target.py b/backend/api_v2/tests/test_api_key_create_target.py index 83ab99f98a..20e1d08536 100644 --- a/backend/api_v2/tests/test_api_key_create_target.py +++ b/backend/api_v2/tests/test_api_key_create_target.py @@ -32,12 +32,17 @@ source text and so passed against an inverted ``isinstance`` guard and against the wrong object being handed to ``check_object_permissions``. -The end-to-end request cycle still needs a database and lives in the -integration tier; what runs here is the method body, not the routing. +What runs here is the method body, not the routing -- and that is a real gap, +not merely a deferral: because the body is ``exec``-ed out of its class, moving +``create`` into a dead class (reopening the IDOR) or dropping an import it uses +at request time both leave this suite green. ``tests_common/test_route_wiring.py`` +asserts the binding and fails on either. The live request cycle against a +database remains integration-tier. """ from __future__ import annotations +import logging import textwrap from pathlib import Path from typing import Any @@ -446,6 +451,41 @@ def test_pipeline_path_with_api_body_is_refused(self) -> None: with pytest.raises(_ValidationError): view.create(_Req({"api": "api-1"}, OWNER), pipeline_id="pipe-1") + def test_same_field_naming_a_different_resource_is_refused(self) -> None: + """A body naming another API than the path is a contradiction too. + + The cross-type guards above do not catch this: path and body agree on + the *field* and disagree on the *value*. Overwriting silently mints a + live key for the path's deployment while the caller asked for another. + """ + view = _build_create( + apis={"api-1": _Target("api-1", OWNER), "api-2": _Target("api-2", OWNER)} + ) + + with pytest.raises(_ValidationError): + view.create(_Req({"api": "api-2"}, OWNER), api_id="api-1") + + def test_same_pipeline_field_naming_a_different_resource_is_refused(self) -> None: + view = _build_create( + pipelines={ + "pipe-1": _Target("pipe-1", OWNER), + "pipe-2": _Target("pipe-2", OWNER), + } + ) + + with pytest.raises(_ValidationError): + view.create(_Req({"pipeline": "pipe-2"}, OWNER), pipeline_id="pipe-1") + + def test_body_repeating_the_path_target_verbatim_is_accepted(self) -> None: + """Agreement is not a contradiction -- the redundant body still works.""" + target = _Target("api-1", OWNER) + view = _build_create(apis={"api-1": target}) + + response = view.create(_Req({"api": "api-1"}, OWNER), api_id="api-1") + + assert response.status == 201 + assert view.checked == [target] + def test_empty_string_body_value_does_not_defeat_the_path(self) -> None: """``setdefault`` used to let ``{"api": ""}`` through as a present key.""" target = _Target("api-1", OWNER) @@ -524,6 +564,7 @@ class _APIDeploymentModel: "APIDeployment": _APIDeploymentModel, "ValidationError": DjangoValidationError, "Any": Any, + "logger": logging.getLogger("test-stub"), }, ) return namespace["get_api_by_id"] @@ -546,6 +587,7 @@ def fetch_pipeline(pipeline_id: str, check_active: bool = True) -> Any: "Pipeline": _PipelineModel, "ValidationError": DjangoValidationError, "Any": Any, + "logger": logging.getLogger("test-stub"), }, ) return lambda pipeline_id: namespace["get_pipeline_by_id"](_Cls, pipeline_id) diff --git a/backend/api_v2/utils.py b/backend/api_v2/utils.py index ff47091ea1..6167d270e4 100644 --- a/backend/api_v2/utils.py +++ b/backend/api_v2/utils.py @@ -1,9 +1,13 @@ +import logging + from django.core.exceptions import ValidationError from workflow_manager.workflow_v2.models.execution import WorkflowExecution from api_v2.models import APIDeployment from api_v2.notification import APINotification +logger = logging.getLogger(__name__) + class APIDeploymentUtils: @staticmethod @@ -27,7 +31,13 @@ def get_api_by_id(api_id: str) -> APIDeployment | None: try: api_deployment: APIDeployment = APIDeployment.objects.get(pk=api_id) return api_deployment - except (APIDeployment.DoesNotExist, ValidationError): + except APIDeployment.DoesNotExist: + return None + except ValidationError: + # Logged so a malformed identifier stays distinguishable from an + # absent row: both answer 404, and without this the difference is + # invisible when triaging. + logger.debug("Malformed API deployment identifier: %s", api_id) return None @staticmethod diff --git a/backend/pipeline_v2/pipeline_processor.py b/backend/pipeline_v2/pipeline_processor.py index e774051a9d..24042f3bc9 100644 --- a/backend/pipeline_v2/pipeline_processor.py +++ b/backend/pipeline_v2/pipeline_processor.py @@ -58,7 +58,10 @@ def get_active_pipeline(cls, pipeline_id: str) -> Pipeline | None: """ try: return cls.fetch_pipeline(pipeline_id, check_active=True) - except (Pipeline.DoesNotExist, ValidationError): + except Pipeline.DoesNotExist: + return None + except ValidationError: + logger.debug("Malformed pipeline identifier: %s", pipeline_id) return None @classmethod @@ -80,7 +83,13 @@ def get_pipeline_by_id(cls, pipeline_id: str) -> Pipeline | None: """ try: return cls.fetch_pipeline(pipeline_id, check_active=False) - except (Pipeline.DoesNotExist, ValidationError): + except Pipeline.DoesNotExist: + return None + except ValidationError: + # Logged so a malformed identifier stays distinguishable from an + # absent row: both answer 404, and without this the difference is + # invisible when triaging. + logger.debug("Malformed pipeline identifier: %s", pipeline_id) return None @staticmethod diff --git a/backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py b/backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py index c7707a1785..449036d402 100644 --- a/backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py +++ b/backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py @@ -15,10 +15,9 @@ ``has_object_permission`` is pure logic over collaborators, so these tests stub the Django-coupled boundary (``permissions.permission``, -``OrganizationMemberService``) and exercise the real method body. Django is not -importable in a plain checkout, so the class body is extracted from source -- -mirroring ``prompt_studio_core_v2/tests/test_build_index_payload.py``. A rename -fails these tests rather than silently skipping them. +``OrganizationMemberService``) and exercise the real method body, sliced out of +the source by ``tests_common.source_extraction``. A rename fails these tests +rather than silently skipping them. The in-use check is driven the same way: ``destroy``, ``_refuse_if_in_use`` and the shared helpers in ``prompt_studio/tool_usage.py`` are all executed against a @@ -26,9 +25,24 @@ left its *call site* unpinned, and deleting that one line made in-use tools deletable with the whole suite still green. -What is *not* covered here: route binding and the live permission cycle need a -database, and ``backend/conftest.py`` auto-marks such tests ``integration`` so -they run in the rig's integration tier rather than the per-PR unit tier. +**Known blind spot of this technique.** The bodies are ``exec``-ed out of +context, so nothing here binds them to the class, the URLconf, or DRF dispatch: +code that is present but *unreachable* looks identical to code that is wired. +Deleting the DELETE route outright, or unwiring ``IsRegistryToolOwner`` from +``destroy``, each left this suite fully green while reopening the hole it exists +to close. ``tests_common/test_route_wiring.py`` covers binding from the other +side and fails on both. Two further sharp edges: inserting a decorator above an +extracted definition silently truncates its slice, so the tests then assert +against a *different* body than ships; and a cosmetic annotation change can +break the marker match outright. + +Retiring the technique for the conventional import-and-``patch`` pattern is +deferred to its own change -- it rewrites both suites wholesale. Note the +premise it was built on does not hold: Django *is* importable in this tier +(``tests/groups.yaml`` sets ``DJANGO_SETTINGS_MODULE`` for ``unit-backend``), +``backend/conftest.py`` auto-marks only ``django_db``/``TestCase`` tests as +``integration``, and ``prompt_studio_core_v2/tests/test_build_index_payload.py`` +already imports its module directly for exactly this reason. """ from __future__ import annotations diff --git a/backend/prompt_studio/prompt_studio_registry_v2/views.py b/backend/prompt_studio/prompt_studio_registry_v2/views.py index 2a1ab01117..92134f8033 100644 --- a/backend/prompt_studio/prompt_studio_registry_v2/views.py +++ b/backend/prompt_studio/prompt_studio_registry_v2/views.py @@ -45,11 +45,12 @@ def get_permissions(self) -> list[Any]: return [IsRegistryToolOwner()] return super().get_permissions() - def get_queryset(self) -> QuerySet | None: + def get_queryset(self) -> QuerySet: # Detail routes address a single row by PK; the list filters below are - # query-param driven and would resolve to None, breaking get_object(). - # Keyed off the URL kwarg rather than `self.detail`, which DRF only - # populates for router-generated views (it is None under as_view()). + # query-param driven and would resolve to an empty queryset, so + # get_object() could never find the row. Keyed off the URL kwarg rather + # than `self.detail`, which DRF only populates for router-generated + # views (it is None under as_view()). if self.kwargs.get("pk"): return PromptStudioRegistry.objects.all() @@ -58,11 +59,13 @@ def get_queryset(self) -> QuerySet | None: PromptStudioRegistryKeys.PROMPT_REGISTRY_ID, "custom_tool", ) - queryset = None - if filterArgs: - queryset = PromptStudioRegistry.objects.filter(**filterArgs) + # `none()` rather than None: DRF hands this straight to filter_queryset + # and the paginator, neither of which accepts None -- an unfiltered + # `list` used to 500 instead of returning nothing. + if not filterArgs: + return PromptStudioRegistry.objects.none() - return queryset + return PromptStudioRegistry.objects.filter(**filterArgs) def destroy( self, request: Request, *args: tuple[Any], **kwargs: dict[str, Any] @@ -126,9 +129,16 @@ def _in_use_detail(deployment_types: set) -> str: """ types_text = join_deployment_types(deployment_types) if not types_text: + # No resolved type has two causes: the workflows are genuinely + # undeployed, or they belong to another organization -- the + # deployment tables are org-scoped while `ToolInstance` is not. + # Say so, rather than directing the user to remove usages they may + # have no way to see. return ( "This exported tool is still used by one or more workflows. " - "Remove those usages before deleting it." + "Remove those usages before deleting it. If you cannot find " + "them, they may belong to another organization -- contact an " + "administrator." ) return ( f"This exported tool is still used in {types_text}. " diff --git a/backend/tests_common/source_extraction.py b/backend/tests_common/source_extraction.py index be3c598307..f2318b477a 100644 --- a/backend/tests_common/source_extraction.py +++ b/backend/tests_common/source_extraction.py @@ -1,13 +1,29 @@ -"""Run real method bodies in the unit tier, where Django settings are absent. +"""Run real method bodies against stubbed collaborators, without the class. -Guard logic worth pinning (authorization, in-use refusal) lives in modules that -import Django at module scope, so the unit tier cannot import them. Rather than -restate the logic in a test -- which passes no matter what the real code does -- -these helpers slice the definition out of the source file and ``exec`` it -against stubbed collaborators, so the assertions run against the shipped body. +These helpers slice a definition out of its source file and ``exec`` it in a +namespace of stubs, so assertions run against the shipped body rather than a +restatement of it that would pass no matter what the real code does. -The end-to-end request cycle needs a database and belongs in the integration -tier; ``backend/conftest.py`` auto-marks those. +**Prefer importing the module.** Django settings *are* configured in the unit +tier (``tests/groups.yaml`` sets ``DJANGO_SETTINGS_MODULE`` for +``unit-backend``), and ``backend/conftest.py`` auto-marks only ``django_db`` and +``TestCase`` tests as ``integration`` -- so a plain import plus +``unittest.mock.patch`` runs per-PR and is the established pattern here (see +``prompt_studio_core_v2/tests/test_build_index_payload.py``). Reach for these +helpers only where that genuinely will not do. + +**Limitations, because they are not obvious:** + +* The extracted body is not bound to its class, the URLconf, or DRF dispatch, + so a test using it cannot tell wired code from unreachable code. Pair it with + an explicit wiring test (``tests_common/test_route_wiring.py``). +* Names resolve from the supplied namespace, never from the module's own import + block -- a deleted or renamed import is invisible here. +* Slicing stops at the first ``stops`` needle, so inserting a decorator above a + definition silently truncates it and the assertions then cover a *different* + body than ships. +* The marker must match the definition line verbatim, so a cosmetic annotation + change breaks the match. """ from __future__ import annotations diff --git a/backend/tests_common/test_route_wiring.py b/backend/tests_common/test_route_wiring.py index 2ed9077162..0bd6a274d9 100644 --- a/backend/tests_common/test_route_wiring.py +++ b/backend/tests_common/test_route_wiring.py @@ -28,9 +28,8 @@ import inspect import sys -from django.urls import resolve, reverse - from api_v2.api_key_views import APIKeyViewSet +from django.urls import resolve, reverse from permissions.permission import IsParentDeploymentOwner from prompt_studio.permission import IsRegistryToolOwner from prompt_studio.prompt_studio_registry_v2.views import PromptStudioRegistryView @@ -113,11 +112,11 @@ def test_every_global_create_uses_resolves(self) -> None: """ module = sys.modules[APIKeyViewSet.__module__] source = inspect.getsource(module) - (func,) = [ + (func,) = ( node for node in ast.walk(ast.parse(source)) if isinstance(node, ast.FunctionDef) and node.name == "create" - ] + ) loaded = { node.id for node in ast.walk(func) From 1e6b96b0f1de3f0f9ac225f592633432976b69c8 Mon Sep 17 00:00:00 2001 From: Hari John Kuriakose Date: Mon, 3 Aug 2026 16:17:35 +0530 Subject: [PATCH 12/14] [TEST] Harden the wiring assertions against unrelated edits From the confirming review of the two fix commits: - `test_every_global_create_uses_resolves` walked the whole module for any FunctionDef named `create`, so a second definition anywhere in the file would break it with an unpack error naming nothing about the defect, and an `async def` would match nothing. Parses the override itself instead -- the same expression the neighbouring test already uses. Re-verified that dropping the `status` import still fails it. - `test_list_is_not_gated_by_the_owner_permission` asserted only the absence of the owner class, which an empty permission list would satisfy while proving nothing. Also pins that resolution fell through to the default. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DJWqb9Nq6aFbUFn62MMZc2 --- backend/tests_common/test_route_wiring.py | 26 ++++++++++++++--------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/backend/tests_common/test_route_wiring.py b/backend/tests_common/test_route_wiring.py index 0bd6a274d9..308227d82c 100644 --- a/backend/tests_common/test_route_wiring.py +++ b/backend/tests_common/test_route_wiring.py @@ -27,6 +27,7 @@ import builtins import inspect import sys +import textwrap from api_v2.api_key_views import APIKeyViewSet from django.urls import resolve, reverse @@ -65,13 +66,19 @@ def test_destroy_resolves_the_owner_permission(self) -> None: ) def test_list_is_not_gated_by_the_owner_permission(self) -> None: - """Read visibility is derived by ``list_tools``; only deletes narrow.""" + """Read visibility is derived by ``list_tools``; only deletes narrow. + + Asserts the *resolution* path ran, not merely that the owner class is + absent: an empty permission list would satisfy the negative on its own + and prove nothing. + """ view = PromptStudioRegistryView() view.action = "list" - assert not any( - isinstance(perm, IsRegistryToolOwner) for perm in view.get_permissions() - ) + permissions = view.get_permissions() + + assert permissions == super(PromptStudioRegistryView, view).get_permissions() + assert not any(isinstance(perm, IsRegistryToolOwner) for perm in permissions) class TestApiKeyRoutesAreWired: @@ -111,12 +118,11 @@ def test_every_global_create_uses_resolves(self) -> None: the module. Resolve each global the body loads instead. """ module = sys.modules[APIKeyViewSet.__module__] - source = inspect.getsource(module) - (func,) = ( - node - for node in ast.walk(ast.parse(source)) - if isinstance(node, ast.FunctionDef) and node.name == "create" - ) + # Parse the override itself rather than walking the module for any + # ``create``: a second definition anywhere in the file would otherwise + # break this with an unpack error naming nothing about the defect. + source = textwrap.dedent(inspect.getsource(vars(APIKeyViewSet)["create"])) + (func,) = ast.parse(source).body loaded = { node.id for node in ast.walk(func) From 6afb438ba79b8a72bd6c0352a74734d6dee24ec7 Mon Sep 17 00:00:00 2001 From: Hari John Kuriakose Date: Wed, 5 Aug 2026 00:19:06 +0530 Subject: [PATCH 13/14] [TEST] Pin that registry deletion is organization-scoped (G1, G3) Three open Greptile P1 threads claim the detail route's `objects.all()` queryset lets a caller -- or an org admin -- delete another organization's exported tool by PK. Verified against the code: it does not. `get_object()` calls `filter_queryset()` *before* `check_object_permissions`, and `OrganizationFilterBackend` is in DEFAULT_FILTER_BACKENDS. `get_org_path` resolves `PromptStudioRegistry` to a direct `organization` FK (confirmed by running it), so the queryset is narrowed to the caller's org and a foreign-org PK is a 404 from `get_object_or_404` -- before `IsRegistryToolOwner` runs, so its org-admin branch is never reached for another org's row. The filter is also fail-closed on both of its own miss paths (no org context, no discoverable path both return `none()`). None of that is visible at the call site, which is presumably why three separate P1s landed on it, so pin it: the FK path, the filter-before-check ordering, and that the viewset does not opt out via `skip_org_filter` or by overriding `filter_backends`. Adding `skip_org_filter = True` -- which would make the reported scenario real -- fails the third test. No production code changed; this run found no defect to fix. Backend unit tier: 340 passed (337 before), ruff 0.3.4 clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DJWqb9Nq6aFbUFn62MMZc2 --- backend/tests_common/test_route_wiring.py | 38 +++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/backend/tests_common/test_route_wiring.py b/backend/tests_common/test_route_wiring.py index 308227d82c..0bf55f9a6a 100644 --- a/backend/tests_common/test_route_wiring.py +++ b/backend/tests_common/test_route_wiring.py @@ -33,7 +33,9 @@ from django.urls import resolve, reverse from permissions.permission import IsParentDeploymentOwner from prompt_studio.permission import IsRegistryToolOwner +from prompt_studio.prompt_studio_registry_v2.models import PromptStudioRegistry from prompt_studio.prompt_studio_registry_v2.views import PromptStudioRegistryView +from utils.filters.organization_filter import OrganizationFilterBackend, get_org_path REGISTRY_URLCONF = "prompt_studio.prompt_studio_registry_v2.urls" API_URLCONF = "api_v2.urls" @@ -81,6 +83,42 @@ def test_list_is_not_gated_by_the_owner_permission(self) -> None: assert not any(isinstance(perm, IsRegistryToolOwner) for perm in permissions) +class TestRegistryDeleteIsOrganizationScoped: + """The detail route resolves rows only within the caller's organization. + + Review flagged ``get_queryset`` returning ``objects.all()`` on the detail + route as allowing a caller -- or an org admin -- to delete another + organization's exported tool by PK. It does not, and these pin the two + facts that make it so, because neither is visible at the call site. + """ + + def test_registry_rows_have_a_discoverable_organization_path(self) -> None: + """``OrganizationFilterBackend`` finds the FK path by BFS. + + Without a path it fails closed to ``none()``, so the delete would 404 + rather than leak -- but the scoping this route relies on would be + gone. Pin the path so a model change surfaces here. + """ + assert get_org_path(PromptStudioRegistry) == "organization" + + def test_the_org_filter_runs_before_the_permission_check(self) -> None: + """``get_object()`` filters, *then* object-checks -- ordering matters. + + A foreign-org PK is a 404 from ``get_object_or_404`` before + ``IsRegistryToolOwner`` is consulted, so the org-admin branch inside it + is never reached for another organization's row. + """ + source = inspect.getsource(PromptStudioRegistryView.get_object) + + assert source.index("filter_queryset") < source.index("check_object_permissions") + + def test_the_viewset_does_not_opt_out_of_org_filtering(self) -> None: + """``skip_org_filter``/``filter_backends`` overrides would bypass it.""" + assert getattr(PromptStudioRegistryView, "skip_org_filter", False) is False + assert "filter_backends" not in vars(PromptStudioRegistryView) + assert OrganizationFilterBackend in PromptStudioRegistryView().filter_backends + + class TestApiKeyRoutesAreWired: """``keys/{api,pipeline}/...`` -- the routes whose ``create`` closes an IDOR.""" From 8476b0cc46590122e4472358b60d02989f3df113 Mon Sep 17 00:00:00 2001 From: Hari John Kuriakose Date: Wed, 5 Aug 2026 00:29:11 +0530 Subject: [PATCH 14/14] [FIX] Unbreak pre-commit.ci: attribute docstring reads as a second module docstring `check-docstring-first` fails the file: test_registry_tool_delete_guards.py:259: Multiple module docstrings (first docstring on line 1). The bare string under `DELETED = object()` is a PEP 257 attribute docstring, but the hook parses any module-level string literal after the first as a second module docstring. Converted to a comment, which keeps the explanation where it is and satisfies the hook. Pre-existing rather than newly introduced -- the sentinel has carried its docstring since 4666d4b1, so pre-commit.ci has been red on this since then. Ruff does not implement this check, which is why local `ruff check` stayed clean across every prior run. Verified by running the full hook set against the PR's changed files: all hooks pass, including `check docstring is first`. Backend unit tier unchanged at 340 passed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DJWqb9Nq6aFbUFn62MMZc2 --- .../tests/test_registry_tool_delete_guards.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py b/backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py index 449036d402..acb772e655 100644 --- a/backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py +++ b/backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py @@ -255,8 +255,10 @@ def __iter__(self) -> Any: return _QS() +# Sentinel returned by the stub base ``destroy``, proving the delete ran. +# A comment rather than an attribute docstring: `check-docstring-first` reads a +# bare module-level string as a second module docstring and fails the file. DELETED = object() -"""Sentinel returned by the stub base ``destroy``, proving the delete ran.""" class _BaseView: