Unmerged review follow-ups from #2206: registry-tool authorization and API-key IDOR fixes - #2281
Draft
hari-kuriakose wants to merge 9 commits into
Draft
Unmerged review follow-ups from #2206: registry-tool authorization and API-key IDOR fixes#2281hari-kuriakose wants to merge 9 commits into
hari-kuriakose wants to merge 9 commits into
Conversation
…m 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/<pk>/`, 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/<api_id>/` 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/<pipeline_id>/` 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) <noreply@anthropic.com>
The new `DELETE registry/<pk>/` 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) <noreply@anthropic.com>
Regression tests for the two gates on `DELETE registry/<pk>/`.
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) <noreply@anthropic.com>
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014PWpGFA4Z5qktUj5DW2oiK
Standardized review of bfde081 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014PWpGFA4Z5qktUj5DW2oiK
Second review round on 4666d4b. 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 `<str:api_id>` / `<str:pipeline_id>`, 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014PWpGFA4Z5qktUj5DW2oiK
Third review round flagged the helper tests as source-text assertions: they matched `"ValidationError" in <except line>`, 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014PWpGFA4Z5qktUj5DW2oiK
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 3b06040. 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014PWpGFA4Z5qktUj5DW2oiK
`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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014PWpGFA4Z5qktUj5DW2oiK
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Registry-tool authorization fixes, API-key IDOR fixes, and the tests covering them.
Authorization and IDOR
cae909946bfde0813a4666d4b14bad1ded674666d4b14is the one to read first: the path-parameter route was guarded while the remainingroutes were not, so the check read as present while staying open.
Tests and supporting refactor
facf9c7023b060403900165fcae9e255db8a3b0604039replaces an assertion on source text with one that exercises the code path — agrep-based test passes whether or not the catch fires.
Scope
14 files, +1528 / −72:
backend/prompt_studio/prompt_studio_registry_v2/— views, urls, exceptions, and a 490-lineguard test
backend/permissions/permission.py,backend/prompt_studio/permission.pybackend/prompt_studio/tool_usage.py(new)backend/tests_common/(new shared test helper)Before merging
The branch is based on
mainas of88eed95b7, which is now 36 commits behind. It needs arebase or merge from
mainand a test run against currentmain. The authorization changes inparticular want a look against present-day routing, since the surrounding routes have moved.
Draft until that has been done.
🤖 Generated with Claude Code
https://claude.ai/code/session_01X5SYUSp5fAwsejGVn1F57N