Skip to content

fix: verify tenant/database ownership in data-plane collection auth - #7480

Open
PiedPiper911 wants to merge 1 commit into
chroma-core:mainfrom
PiedPiper911:fix/cross-tenant-collection-resolution-7462
Open

fix: verify tenant/database ownership in data-plane collection auth#7480
PiedPiper911 wants to merge 1 commit into
chroma-core:mainfrom
PiedPiper911:fix/cross-tenant-collection-resolution-7462

Conversation

@PiedPiper911

Copy link
Copy Markdown

Problem

Chroma's REST API scopes every collection operation under /api/v2/tenants/{tenant}/databases/{database}/collections/{collection_id}/..., but the data-plane endpoints (add, get, query, count, update records, update_collection, fork_count, indexing_status) resolve the target collection purely by UUID via get_cached_collection — never verifying that the UUID actually belongs to the {tenant}/{database} in the URL path.

Any caller who knows another tenant's collection UUID can read, write, and modify that collection's data by issuing the request under their own tenant/database prefix. This defeats Chroma's tenant/database isolation model.

The sibling endpoints (GET .../collections/by-id/{id}, DELETE .../collections/{id}) correctly 404 for cross-tenant references, proving this is an omission rather than intentional design.

Solution

Add a tenant/database ownership verification in authenticate_and_authorize_collection (the shared auth helper used by all data-plane endpoints). After resolving the collection via get_cached_collection, verify that collection.tenant and collection.database match the values from the request path (available in AuthzResource). Return 404 for mismatches — matching the behavior of the correctly-scoped sibling endpoints.

Changes

  • rust/frontend/src/server.rs: Added tenant and database ownership checks in authenticate_and_authorize_collection after collection resolution.

Testing

The fix ensures that for any collection C owned by (T1, D1), a data-plane request authenticated as (T2, D2) where (T1,D1) != (T2,D2) returns 404 instead of silently succeeding. This matches the existing behavior of GET by-id and DELETE.

Notes for Reviewer

  • The check uses resource.tenant and resource.database from AuthzResource, which are populated from the URL path parameters at every call site.
  • The check is placed after get_cached_collection (which populates the cache) but before the auth delegation, so it protects all 13+ data-plane endpoints that go through this helper.
  • The cache-key concern raised in the issue (keyed by collection_id alone) is a separate optimization; this fix ensures correctness regardless of cache state since the ownership check runs on every request.

Fixes #7462

…ross-tenant access

The data-plane endpoints (add, get, query, count, update, etc.) resolve
collections purely by UUID without verifying that the collection belongs
to the tenant/database in the URL path. This allows any caller who knows
a collection UUID to bypass tenant isolation.

Add an ownership check in authenticate_and_authorize_collection that
verifies the resolved collection's tenant and database match the request
path, returning 404 for cross-tenant references — matching the behavior
of the correctly-scoped sibling endpoints (GET by-id, DELETE).

Fixes chroma-core#7462

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@github-actions

Copy link
Copy Markdown

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

@PiedPiper911

Copy link
Copy Markdown
Author

Friendly ping - this PR adds tenant/database ownership verification in data-plane collection operations to prevent cross-tenant access. Happy to add tests or adjust the implementation if needed. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cross-tenant/cross-database collection read, write, and metadata tampering via UUID-only collection resolution (tenant/database URL path ignored)

1 participant