From 61402f62a21b435ec7f8155eb47b032f7ff8d3e9 Mon Sep 17 00:00:00 2001 From: chihlasm Date: Thu, 9 Apr 2026 04:19:28 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20address=20Task=206=20quality=20review=20?= =?UTF-8?q?=E2=80=94=20rename=20helper,=20restore=20403=20for=20intra-acco?= =?UTF-8?q?unt,=20add=20docs=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename _get_tree_or_403 → _get_tree_or_404 in maintenance_schedules.py (function now raises 404, old name was misleading) - Restore HTTP 403 for intra-account permission failures in update_tree: same-account users who can see a tree but can't edit it got 404 (wrong); only cross-account lookups should return 404 to avoid confirming existence - Apply same 403/404 distinction to update_tree_visibility - Add test: get_documentation must return 404 for cross-user session access - Add comment documenting owner-only design for documentation endpoints Co-Authored-By: Claude Sonnet 4.6 --- backend/app/api/endpoints/ai_sessions.py | 4 ++- .../api/endpoints/maintenance_schedules.py | 10 +++---- backend/app/api/endpoints/trees.py | 16 +++++++++++ backend/tests/test_tenant_isolation_p0.py | 28 +++++++++++++++++++ 4 files changed, 52 insertions(+), 6 deletions(-) diff --git a/backend/app/api/endpoints/ai_sessions.py b/backend/app/api/endpoints/ai_sessions.py index 6acce5b1..425e6421 100644 --- a/backend/app/api/endpoints/ai_sessions.py +++ b/backend/app/api/endpoints/ai_sessions.py @@ -921,7 +921,9 @@ async def get_documentation( db: Annotated[AsyncSession, Depends(get_db)], ): """Get auto-generated documentation for a session.""" - # Verify session ownership — return 404 (not 403) to avoid confirming existence. + # Verify session ownership — owner only. Documentation endpoints require direct + # ownership; escalated_to_id / picked_up_by handlers use get_session (read-only). + # This is consistent with stream_documentation which has the same owner-only check. result = await db.execute( select(AISession).where( AISession.id == session_id, diff --git a/backend/app/api/endpoints/maintenance_schedules.py b/backend/app/api/endpoints/maintenance_schedules.py index 9fd979d0..506da0e3 100644 --- a/backend/app/api/endpoints/maintenance_schedules.py +++ b/backend/app/api/endpoints/maintenance_schedules.py @@ -29,8 +29,8 @@ def _compute_next_run(cron_expression: str, tz_name: str) -> datetime: return cron.get_next(datetime).astimezone(timezone.utc) -async def _get_tree_or_403(tree_id: UUID, current_user: User, db: AsyncSession) -> "Tree": - """Fetch tree and verify the current user's team owns it.""" +async def _get_tree_or_404(tree_id: UUID, current_user: User, db: AsyncSession) -> "Tree": + """Fetch tree and verify the current user's team owns it. Raises 404 if not found or access denied.""" result = await db.execute(select(Tree).where(Tree.id == tree_id)) tree = result.scalar_one_or_none() if not tree: @@ -51,7 +51,7 @@ async def create_schedule( ): """Create a cron schedule for a maintenance flow. One per flow.""" # Verify user's team owns the tree - tree = await _get_tree_or_403(data.tree_id, current_user, db) + tree = await _get_tree_or_404(data.tree_id, current_user, db) if tree.tree_type != "maintenance": raise HTTPException(status_code=400, detail="Schedules are only supported for maintenance flows") @@ -94,7 +94,7 @@ async def get_schedule_for_tree( ): """Get the schedule for a specific maintenance flow.""" # Verify user's team owns the tree before returning schedule data - await _get_tree_or_403(tree_id, current_user, db) + await _get_tree_or_404(tree_id, current_user, db) result = await db.execute( select(MaintenanceSchedule).where(MaintenanceSchedule.tree_id == tree_id) @@ -122,7 +122,7 @@ async def update_schedule( raise HTTPException(status_code=404, detail="Schedule not found") # Verify user's team owns the tree this schedule belongs to - await _get_tree_or_403(schedule.tree_id, current_user, db) + await _get_tree_or_404(schedule.tree_id, current_user, db) update_fields = data.model_fields_set was_active = schedule.is_active diff --git a/backend/app/api/endpoints/trees.py b/backend/app/api/endpoints/trees.py index 2ea68c48..75cdaf70 100644 --- a/backend/app/api/endpoints/trees.py +++ b/backend/app/api/endpoints/trees.py @@ -610,6 +610,14 @@ async def update_tree( ) if not can_edit_tree(current_user, tree): + # If the user can see this tree (same account, team visibility), give a 403 with + # a clear message — returning 404 here would be confusing since GET returns 200. + # For truly inaccessible trees (cross-account), return 404 to avoid confirming existence. + if can_access_tree(current_user, tree): + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="You do not have permission to edit this flow" + ) raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, detail="Tree not found" @@ -1144,6 +1152,14 @@ async def update_tree_visibility( ) if not can_edit_tree(current_user, tree): + # If the user can see this tree (same account, team visibility), give a 403 with + # a clear message — returning 404 here would be confusing since GET returns 200. + # For truly inaccessible trees (cross-account), return 404 to avoid confirming existence. + if can_access_tree(current_user, tree): + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="You do not have permission to edit this flow" + ) raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, detail="Tree not found" diff --git a/backend/tests/test_tenant_isolation_p0.py b/backend/tests/test_tenant_isolation_p0.py index 5ed77d34..c7519f59 100644 --- a/backend/tests/test_tenant_isolation_p0.py +++ b/backend/tests/test_tenant_isolation_p0.py @@ -548,3 +548,31 @@ async def test_maintenance_schedule_returns_404_for_other_team( assert resp.status_code == 404, ( f"Expected 404 for cross-team maintenance schedule access, got {resp.status_code}: {resp.text}" ) + + +@pytest.mark.asyncio +async def test_get_documentation_returns_404_for_other_user_session( + client: AsyncClient, test_db: AsyncSession +): + """GET /ai-sessions/{id}/documentation must return 404 (not 403) for cross-user access.""" + from app.models.ai_session import AISession + + acct_a, user_a, pass_a = await _create_account_and_user(test_db, "doc-a") + acct_b, user_b, pass_b = await _create_account_and_user(test_db, "doc-b") + + session_b = AISession( + user_id=user_b.id, + account_id=acct_b.id, + problem_summary="B's confidential session", + problem_domain="networking", + status="resolved", + ) + test_db.add(session_b) + await test_db.commit() + + headers_a = await _login(client, user_a.email, pass_a) + resp = await client.get( + f"/api/v1/ai-sessions/{session_b.id}/documentation", + headers=headers_a, + ) + assert resp.status_code == 404, f"Expected 404, got {resp.status_code}: {resp.text}"