Skip to content

Add tenant isolation for Crew storage (Redis, Manager, Handlers) #549

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
phenobarbital merged 11 commits into from
Feb 4, 2026
Merged

Add tenant isolation for Crew storage (Redis, Manager, Handlers) #549

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d6ac176
Add tenant isolation for crew storage
phenobarbital Feb 4, 2026
16db083
Initial plan
Copilot Feb 4, 2026
a5a9e28
Update parrot/manager/manager.py
phenobarbital Feb 4, 2026
d36c40f
Initial plan
Copilot Feb 4, 2026
93b04dd
Initial plan
Copilot Feb 4, 2026
39f2e46
Update parrot/manager/manager.py
phenobarbital Feb 4, 2026
c2c69df
Update parrot/manager/manager.py
phenobarbital Feb 4, 2026
f0c6be8
Update parrot/handlers/crew/execution_handler.py
phenobarbital Feb 4, 2026
31767ae
Merge pull request #554 from phenobarbital/copilot/sub-pr-549-another…
phenobarbital Feb 4, 2026
9990efc
Merge pull request #552 from phenobarbital/copilot/sub-pr-549
phenobarbital Feb 4, 2026
0d6ac1b
Merge pull request #553 from phenobarbital/copilot/sub-pr-549-again
phenobarbital Feb 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion parrot/handlers/crew/execution_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,12 +557,17 @@ async def execute_crew(self, data: Dict[str, Any]):
query = data.get('query')
if not query:
return self.error(response={"message": "query is required"}, status=400)
tenant = data.get('tenant') or "global"
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

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

The tenant parameter from the request body is not validated or sanitized before use. A malicious actor could inject special characters or patterns that could compromise tenant isolation or cause issues with Redis key construction. This is particularly concerning for an execution endpoint where tenant determines which crew is executed. Implement strict tenant validation to prevent injection attacks and ensure proper tenant isolation.

Copilot uses AI. Check for mistakes.

if not self.bot_manager:
return self.error(response={"message": "BotManager not available"}, status=500)

# Load Crew
crew, crew_def = await self.bot_manager.get_crew(crew_id, as_new=True)
crew, crew_def = await self.bot_manager.get_crew(
crew_id,
as_new=True,
tenant=tenant
)
if not crew:
return self.error(response={"message": f"Crew '{crew_id}' not found"}, status=404)

Expand Down Expand Up @@ -590,6 +595,7 @@ async def execute_crew(self, data: Dict[str, Any]):

# Store crew name in metadata for future persistence
job.metadata['crew_name'] = crew_def.name
job.metadata['tenant'] = crew_def.tenant

# Cache the running crew
self._active_crews[job_id] = crew
Expand Down
19 changes: 13 additions & 6 deletions parrot/handlers/crew/handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@ async def upload(self):
{
"message": "Crew uploaded and created successfully",
"crew_id": crew_def.crew_id,
"tenant": crew_def.tenant,
"name": crew_def.name,
"execution_mode": crew_def.execution_mode.value, # pylint: disable=E1101 #noqa
"agents": [agent.agent_id for agent in crew_def.agents],
Expand Down Expand Up @@ -285,6 +286,7 @@ async def put(self):
# Parse request body
data = await self.request.json()
crew_def = CrewDefinition(**data)
tenant = crew_def.tenant

# Validate bot manager availability
if not self.bot_manager:
Expand All @@ -296,7 +298,7 @@ async def put(self):
)
# if crew_id is provided, then is an update
if url_crew_id:
existing_crew = await self.bot_manager.get_crew(url_crew_id)
existing_crew = await self.bot_manager.get_crew(url_crew_id, tenant=tenant)
if not existing_crew:
return self.error(
response={
Expand All @@ -311,7 +313,7 @@ async def put(self):
crew_def.updated_at = None # Will be set on save

# Remove old crew
await self.bot_manager.remove_crew(url_crew_id)
await self.bot_manager.remove_crew(url_crew_id, tenant=tenant)

self.logger.info(f"Updating crew '{url_crew_id}'")

Expand All @@ -335,6 +337,7 @@ async def put(self):
{
"message": f"Crew {action} successfully",
"crew_id": crew_def.crew_id,
"tenant": crew_def.tenant,
"name": crew_def.name,
"execution_mode": crew_def.execution_mode.value, # pylint: disable=E1101
"agents": [agent.agent_id for agent in crew_def.agents],
Expand Down Expand Up @@ -380,6 +383,7 @@ async def get(self):
match_params = self.match_parameters(self.request)
crew_id = match_params.get('id') or qs.get('crew_id')
crew_name = qs.get('name')
tenant = qs.get('tenant') or "global"
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

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

The tenant parameter defaults to "global" when not provided in the query string, but this hardcoded default could be problematic in a truly multi-tenant system. If a client forgets to pass the tenant parameter, they'll inadvertently access the global tenant's data instead of receiving an error. Consider whether tenant should be a required parameter for non-global use cases, or at least log a warning when the default is used, to help catch integration bugs early.

Suggested change
tenant = qs.get('tenant') or "global"
tenant = qs.get('tenant')
if not tenant:
self.logger.warning(
"Tenant parameter missing in CrewHandler.get request; "
"defaulting to 'global'."
)
tenant = "global"

Copilot uses AI. Check for mistakes.

if not self.bot_manager:
return self.error(
Expand All @@ -390,7 +394,7 @@ async def get(self):
# Get specific crew
if crew_name or crew_id:
identifier = crew_name or crew_id
crew_data = await self.bot_manager.get_crew(identifier)
crew_data = await self.bot_manager.get_crew(identifier, tenant=tenant)

if not crew_data:
return self.error(
Expand All @@ -403,6 +407,7 @@ async def get(self):
crew, crew_def = crew_data
return self.json_response({
"crew_id": crew_def.crew_id,
"tenant": crew_def.tenant,
"name": crew_def.name,
"description": crew_def.description,
"execution_mode": crew_def.execution_mode.value,
Expand All @@ -421,12 +426,13 @@ async def get(self):
await self.bot_manager.sync_crews()

# List all crews
crews = self.bot_manager.list_crews()
crews = self.bot_manager.list_crews(tenant=tenant)
crew_list = []

crew_list.extend(
{
"crew_id": crew_def.crew_id,
"tenant": crew_def.tenant,
"name": crew_def.name,
"description": crew_def.description,
"execution_mode": crew_def.execution_mode.value,
Expand Down Expand Up @@ -467,6 +473,7 @@ async def delete(self):
qs = self.get_arguments(self.request)
crew_id = match_params.get('id') or qs.get('crew_id')
crew_name = qs.get('name')
tenant = qs.get('tenant') or "global"
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

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

The tenant parameter defaults to "global" when not provided, which mirrors the same issue in the GET handler. In a multi-tenant system, silently defaulting to "global" could lead to accidental deletion of global tenant crews when the client intended to delete from a specific tenant but forgot the parameter. Consider making tenant a required parameter or at least validating/logging when the default is used.

Copilot uses AI. Check for mistakes.
Copy link
Owner Author

@phenobarbital phenobarbital Feb 4, 2026

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback

Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

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

Similar to the GET handler, the tenant parameter from the query string is not validated or sanitized. This creates a security risk where malicious tenant values could be used to construct invalid Redis keys or potentially access data from other tenants if the key construction is vulnerable to injection. Implement tenant validation to ensure only safe, expected tenant identifiers are accepted.

Copilot uses AI. Check for mistakes.

if not crew_name and not crew_id:
return self.error(
Expand All @@ -483,15 +490,15 @@ async def delete(self):
identifier = crew_name or crew_id

# Check if exists first
crew_data = await self.bot_manager.get_crew(identifier)
crew_data = await self.bot_manager.get_crew(identifier, tenant=tenant)
if not crew_data:
return self.error(
response={"message": f"Crew '{identifier}' not found"},
status=404
)

# Remove crew
await self.bot_manager.remove_crew(identifier)
await self.bot_manager.remove_crew(identifier, tenant=tenant)

return self.json_response({
"message": f"Crew '{identifier}' deleted successfully"
Expand Down
4 changes: 4 additions & 0 deletions parrot/handlers/crew/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,10 @@ class CrewDefinition(BaseModel):
default_factory=lambda: str(uuid.uuid4()),
description="Unique identifier for the crew"
)
tenant: str = Field(
default="global",
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

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

When a legacy crew (without tenant field) is loaded from Redis and deserialized, it will automatically get tenant="global" from the Field default. However, if this crew is then modified and re-saved, it could be saved under a new tenant-aware key structure. This could lead to data duplication where the same crew exists under both legacy keys (crew:name) and new tenant-aware keys (crew:global:name). Consider implementing a migration strategy or adding logic to clean up legacy keys when crews are re-saved.

Suggested change
default="global",
...,

Copilot uses AI. Check for mistakes.
description="Tenant identifier for crew isolation"
)
name: str = Field(description="Name of the crew")
description: Optional[str] = Field(
default=None,
Expand Down
Loading
Loading