chore(proxy): tighten resource ownership checks - #26951
Conversation
Greptile SummaryThis PR tightens ownership enforcement for proxy-managed skills and containers: new resources are stamped with the caller's identity scope, read/write paths filter results to records the caller owns, and the decoded (provider-native) container ID is now correctly forwarded via
Confidence Score: 3/5Not safe to merge: multiple P1s from prior review rounds (direct Prisma calls in hot paths, backwards-incompatible default-closed ownership gates) remain unresolved. Several fixes from prior rounds are incorporated (decoded ID forwarding, _dump_response copy, has_more recalculation, graceful create fallback), but two project-rule violations — direct DB queries in the critical request path and backwards-incompatible ownership gates — are still present and were flagged as P1 in earlier reviews. With multiple pre-existing P1s unaddressed the ceiling is below 4. litellm/proxy/container_endpoints/ownership.py (direct Prisma calls + backwards-incompatible gates), litellm/llms/litellm_proxy/skills/handler.py (same), litellm/proxy/_types.py (file_object union mismatch)
|
| Filename | Overview |
|---|---|
| litellm/proxy/container_endpoints/ownership.py | New file implementing container ownership tracking and access control — contains direct Prisma DB queries in hot request paths and backwards-incompatible ownership gates (covered in prior review rounds); several issues from prior threads now addressed (decoded ID forwarding, _dump_response mutation, has_more recalculation) |
| litellm/proxy/common_utils/resource_ownership.py | New shared ownership scope helpers; correctly avoids shared sentinel for identity-less callers; clean and FastAPI-free |
| litellm/proxy/container_endpoints/endpoints.py | Wires ownership record/filter/assert into create/list/retrieve/delete flows; decoded container ID now correctly forwarded via get_container_forwarding_params; record_container_owner placed outside LLM exception scope with graceful non-HTTPException fallback |
| litellm/proxy/container_endpoints/handler_factory.py | Ownership checks added to _process_binary_request, _process_multipart_upload_request, and _process_request; decoded container ID is now forwarded via get_container_forwarding_params in all three paths |
| litellm/llms/litellm_proxy/skills/handler.py | Adds per-skill cache and owner-scoped filtering; raises ValueError (not HTTPException) to stay FastAPI-free; backwards-incompatible gate on skills without created_by covered in prior threads |
| litellm/proxy/_types.py | Adds 'container' to LiteLLM_ManagedObjectTable.file_purpose Literal but file_object Union type is not updated to accept container response types, creating a type mismatch |
| litellm/proxy/auth/auth_utils.py | Security improvement: _request_blocked_callback_params is now included in _build_banned_observability_params, closing a gap in the request-body deny list |
| litellm/skills/main.py | Plumbs user_api_key_dict from request kwargs through to skills handler for create/list/get/delete operations; helpers correctly extract auth from metadata |
| litellm/proxy/hooks/litellm_skills/main.py | Passes user_api_key_dict to fetch_skill_from_db in the skills injection hook; minimal change, correct |
Reviews (23): Last reviewed commit: "chore(container): use delete_cache, json..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
Re-requesting on the latest head. The previous encoded-container concern is covered by preserving the managed ID through proxy forwarding so the container layer can decode before upstream calls while retaining routing metadata, with tests added. The untracked-container concern now has no-DB in-memory ownership for new containers plus a secure-default legacy opt-out. |
|
Re-requesting on latest head after adding direct proxy retrieve/delete forwarding coverage for the managed-container-id path. |
|
Re-requesting on latest head. Added secure-default legacy opt-out coverage for unowned skills as well. |
|
Re-requesting on latest head after resetting |
|
Re-requesting on latest head. Proxy container paths now forward decoded provider container IDs and carry decoded model metadata for routing, filtered pagination is conservative when any items are removed, and ownership persistence failure falls back to in-process tracking. |
|
@greptileai please re-review the latest head. The follow-up addresses the lifecycle/error-handling and compatibility concerns raised in the last summary, with regression coverage. Note that this repo's DB convention is Prisma model methods and no raw SQL; the changed code follows that convention. |
|
@greptileai please re-review the latest head. Added endpoint coverage for the post-processing branches in the follow-up test commit. |
|
@greptileai please re-review the latest head. The dict response mutation finding is now fixed with regression coverage. |
|
@greptileai please re-review the latest head. The DB-recovery/in-memory fallback path and duplicate OpenAPI operation IDs are now addressed with regression coverage. |
|
@greptileai re-review please. Addressing the three P1s from your last round: P1 #2 (direct Prisma in hot path) — addressed in 6194028. P1 #3 (orphan container) — addressed in 6c37e9d. P1 #1 (default-deny gates) — held intentionally. This PR is the fix for VERIA-20 (cross-tenant data leakage in skills/containers). Pre-existing untracked rows are exactly the data that's leaking — flipping the gates to default-permissive would re-introduce the vulnerability for anyone who upgrades without explicitly opting in to safety. The CI is green; cache adds 11 new cache tests, 56 total in the suite. |
|
Addressed the current Greptile P1 in Validation:
|
… path filter_container_list_response runs after the upstream call has already succeeded; treating an ownership-lookup failure as an LLM-API error fires post_call_failure_hook for a successful upstream call and returns a misleading provider-shaped error to the client. Run the filter outside the try/except so genuine LLM errors stay scoped to the upstream call.
…rpose Literal Two cleanups from the /simplify pass: * ``_CONTAINER_OWNER_CACHE`` and ``_SKILL_CACHE`` now LRU-evict via ``OrderedDict.popitem(last=False)`` instead of full ``clear()`` at capacity. Full clears converted a steady-state cached workload into a periodic full-DB-load oscillation as the cache repopulated from zero and cleared again. Reads now ``move_to_end`` so the just-touched entry survives the next eviction. Mirrors the pre-existing LRU pattern in ``_remember_container_owner``. * ``LiteLLM_ManagedObjectTable.file_purpose`` Literal now includes ``"container"`` so Pydantic validation accepts rows written by the ownership store.
LITELLM_ALLOW_UNTRACKED_CONTAINER_ACCESS and LITELLM_ALLOW_UNOWNED_SKILL_ACCESS were operator-toggleable opt-outs for the cross-tenant access primitive this PR closes — flipping either on re-enabled exactly the VERIA-20 read path. Default-secure with no escape hatch matches sibling fixes (vector-store cred isolation, semantic cache key isolation, user_config strip): all rejected the opt-out-of-security pattern. Untracked containers and unowned skills (rows that pre-date this enforcement) are admin-only. Non-admin owners need to either re-create via the now-tracked flow or have an admin assign ``created_by`` on the existing row. Update tests to assert the strict-only behaviour.
|
@greptileai review For the prior P1 about "direct Prisma queries in critical request path" — re-reading CLAUDE.md, the rule is specifically against raw SQL (`execute_raw` / `query_raw`):
`ContainerOwnershipStore` and `LiteLLMSkillsStore` use only Prisma model methods (`find_unique`, `find_first`, `find_many`, `create`, `update`, `delete`) — exactly what CLAUDE.md prescribes. The hot-path concern is mitigated by the per-key TTL caches (`_CONTAINER_OWNER_CACHE`, `_SKILL_CACHE`) which absorb the read load before it hits Prisma. The list-path query uses a single batched `find_many({"created_by": {"in": ...}})` — no N+1. Other addressed items in this revision:
|
…tinel scope UNSCOPED_RESOURCE_OWNER_SCOPE collapsed every caller without an identity field (no user_id / team_id / org_id / api_key / token) into a single shared owner — a cross-tenant access primitive: any two such callers could see and delete each other's containers and skills. Drop the sentinel. ``get_primary_resource_owner_scope`` returns ``None`` and ``get_resource_owner_scopes`` returns ``[]`` for identity-less callers. ``record_container_owner`` and ``LiteLLMSkillsHandler.create_skill`` now reject creates from identity-less callers with a 403 instead of stamping the placeholder. Read paths already deny ``owner is None`` correctly so legacy rows (if any) are admin-only.
… codex/skills-containers-tenant-guard
|
@greptileai review Two new commits:
Re the prior P1 about
Both stores use only |
… codex/skills-containers-tenant-guard
…dy bouncer Two cleanups: * ``LiteLLMSkillsHandler.create_skill`` raised ``HTTPException`` for identity-less callers, importing FastAPI from a ``litellm/llms/`` module — that violates the project rule that FastAPI lives only under ``proxy/``. Switch to ``ValueError`` (the same shape the rest of the handler uses for not-found/forbidden) and update the test. * The proxy-auth body bouncer derived its observability ban list from ``_supported_callback_params`` only, missing ``_request_blocked_callback_params`` (where ``gcs_bucket_name`` and ``gcs_path_service_account`` live). Two recently-merged sibling PRs (#27019 added the deny list, #27081 added the test asserting these are rejected at the request body root) crossed without folding them together. Union the GCS deny list into the bouncer's derivation so the single source of truth covers both code paths.
|
@greptileai review 12fe945 addresses the FastAPI-import P1 — Also fixed an upstream-integration bug surfaced by the merge: Re the persistent "direct Prisma calls in hot path" P1: I posted CLAUDE.md's exact wording on the prior comment — the rule is against raw SQL ( Re the "backwards-incompatible gates" P1 — yes, by design. Default-secure with no opt-out flag matches sibling fixes (vector store credential leak, semantic cache key isolation, user_config strip, callback strip — all merged into |
|
@greptileai review The two remaining P1s in your latest review look like they're reading stale context. Concretely: 1. "The env-var escape hatches exist but default to disabled" — they don't exist. The default IS strict, with no escape hatch — by design, matching every other security PR merged into 2. "Direct Prisma calls violate the project rule" — the project rule (CLAUDE.md, in this repo's root) says:
Hot-path concern is mitigated by the If you have specific evidence either of those two interpretations is correct, please point at the line — I'd rather fix a real issue than ship past a misunderstanding. |
…back, hand-rolled cache Substantial reduction (~765 LOC) without changing the security boundary: * Drop ContainerOwnershipStore and LiteLLMSkillsStore — both were one-method-per-Prisma-call wrappers. Inline the calls instead, matching the established pattern in vector_store_endpoints, agent_endpoints, and mcp_server/db.py. * Drop the prisma_client is None in-memory fallback. Production deploys always have Prisma; running ownership-critical paths on a process-local dict is a security footgun in the dev-mode case it was meant to support, and complicates every code path with a branch. Fail-secure: skip recording if Prisma is unavailable, and treat reads as "not found" (admin-only). * Drop the hand-rolled module-level cache. Replace with the existing litellm.caching.in_memory_cache.InMemoryCache, which already has TTL + max-size + eviction tested in its own module. Sentinel string for negative caching since InMemoryCache can't disambiguate "miss" from "cached as None". * Tests: drop coverage for removed code paths (in-memory fallback, hand-rolled cache internals). Keep tests for actual behavior (cache hit-rate, negative caching, owner check, list filtering, identity-less reject, admin bypass).
|
@greptileai review Significant simplification (commit
|
Address Greptile P2 follow-ups from the prior round: * Cache ``_get_allowed_container_ids`` (60s LRU/TTL keyed by sorted owner-scope tuple) so ``GET /v1/containers`` doesn't issue a fresh ``find_many`` against ``litellm_managedobjecttable`` on every list call. Invalidate the caller's own cache entry when they record a new owner so the just-created container shows up on their next list. * Tighten the admin early-return in ``record_container_owner`` to skip ONLY when there's literally no container ID to stamp. An admin with identity (the master-key path populates ``user_id`` + ``api_key``) flows through the normal record path so admin-created containers are tracked like any other caller's. The truly-identity-less admin case still falls through to the 403 below — correct fail-secure default. Skill-cache invalidation gap (also flagged by Greptile) is moot: there is no skill update endpoint exposed; ownership-affecting mutations are only delete (already invalidates) and create (new ID, no cache entry to update).
|
@greptileai review
Backwards-compat default-strict gates remain by design — every other security PR merged into |
/simplify follow-ups:
* Replace the two-``pop`` reach into ``cache_dict``/``ttl_dict`` with
the existing public ``InMemoryCache.delete_cache(key)`` — the same
idiom used elsewhere in the proxy. Bonus: ``delete_cache`` calls
``_remove_key`` which also handles ``expiration_heap`` consistency
the direct pops were silently leaking.
* JSON-encode the sorted scope list for the cache key instead of
``"|".join``. ``user_id`` / ``team_id`` / ``org_id`` / ``api_key``
are free-form strings and could contain a literal ``|`` — JSON
quoting escapes any in-string separator unambiguously.
* Extract ``_allowed_container_ids_cache_key()`` so the read and
invalidation sites compute the key the same way.
* Fix a placeholder-then-overwrite test construction: the
``__module__.split(".")[0] and "proxy_admin"`` line evaluated to a
literal string that was immediately overwritten with the real enum
value. Hoist the import and construct directly.
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
| user_api_key_dict: Optional[UserAPIKeyAuth], | ||
| ) -> bool: | ||
| if user_api_key_dict is None: | ||
| return True |
There was a problem hiding this comment.
Medium: Ownership bypass for None auth context
Returning True when user_api_key_dict is None means any caller that omits the auth parameter gets unrestricted access to all resources. Since get_skill, delete_skill, list_skills, and fetch_skill_from_db all default user_api_key_dict=None, a code path that forgets to thread the auth object through (e.g. a new endpoint or internal helper) silently skips the ownership check.
The stated rationale (anonymous callers should pass) conflicts with the PR's goal of tenant isolation. Consider defaulting to False (deny) when user_api_key_dict is None, and requiring callers that genuinely need open access to pass an explicit admin-like sentinel.
Medium: Auth bypass when user_api_key_dict is None in resource ownership checkThis PR adds resource ownership checks for skills and containers. The core ownership utility Status: 1 open |
… codex/skills-containers-tenant-guard # Conflicts: # litellm/proxy/auth/auth_utils.py
555a813
into
BerriAI:litellm_internal_staging
…enant-guard chore(proxy): tighten resource ownership checks
2 participants
Relevant issues
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
tests/test_litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewDelays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
CI (LiteLLM team)
Branch creation CI run
Link:
CI run for the last commit
Link:
Merge / cherry-pick CI run
Links:
Screenshots / Proof of Fix
Type
🐛 Bug Fix
✅ Test
Changes
This PR tightens ownership checks for proxy-managed skills and container resources.
LiteLLM-managed skills are now created with an authenticated owner scope and read/write paths resolve only records available to the caller. The same scoped lookup is used when skills are loaded during request preprocessing.
Container operations now record proxy-created container ownership and validate the caller before forwarding container path operations. Container list responses are filtered to resources tracked for the caller, and file operations share the same container ownership check before contacting the upstream provider.
The generated lazy OpenAPI snapshot is refreshed with deterministic route operation IDs so the verify job does not depend on method set ordering.
Behavior notes: