From dd637bdc842d7e34449de5b606b827b48f6323c9 Mon Sep 17 00:00:00 2001 From: Kyle Pope Date: Tue, 17 Mar 2026 04:55:47 +0800 Subject: [PATCH] Fix QA findings from performance, pentest, and code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Perf-1: Eliminate duplicate permission query on task update. get_effective_task_permission now returns (effective, project_level) tuple so the SEC-P02 allowlist check reuses the project-level permission from the first call instead of querying again. Perf-2: Memoize member permission lookup in ProjectDetail. Replace 3 inline acceptedMembers.find() calls with useMemo-derived myPermission and canEditTasks. S-06: Pass members/currentUserId/ownerId/canAssign to mobile TaskDetailPanel (was missing — AssignmentPicker never appeared on mobile). S-08: Add missing selectinload(TaskComment.user) to subtask comments chain in _task_load_options. Subtask comment author_name was always null. W-01: useDeltaPoll stores queryKeyToInvalidate in a ref to prevent infinite re-render if caller passes inline array literal. Co-Authored-By: Claude Opus 4.6 (1M context) --- backend/app/routers/projects.py | 5 ++--- backend/app/services/project_sharing.py | 18 +++++++++--------- .../src/components/projects/ProjectDetail.tsx | 13 +++++++++---- frontend/src/hooks/useDeltaPoll.ts | 8 ++++++-- 4 files changed, 26 insertions(+), 18 deletions(-) diff --git a/backend/app/routers/projects.py b/backend/app/routers/projects.py index 2f85ce0..32cd0e7 100644 --- a/backend/app/routers/projects.py +++ b/backend/app/routers/projects.py @@ -54,7 +54,7 @@ def _task_load_options(): """All load options needed for task responses.""" return [ selectinload(ProjectTask.comments).selectinload(TaskComment.user), - selectinload(ProjectTask.subtasks).selectinload(ProjectTask.comments), + selectinload(ProjectTask.subtasks).selectinload(ProjectTask.comments).selectinload(TaskComment.user), selectinload(ProjectTask.subtasks).selectinload(ProjectTask.subtasks), selectinload(ProjectTask.assignments).selectinload(ProjectTaskAssignment.user), selectinload(ProjectTask.subtasks).selectinload(ProjectTask.assignments).selectinload(ProjectTaskAssignment.user), @@ -375,7 +375,7 @@ async def update_project_task( current_user: User = Depends(get_current_user) ): """Update a project task. Permission checked at project and task level.""" - perm = await get_effective_task_permission(db, current_user.id, task_id, project_id) + perm, project_perm = await get_effective_task_permission(db, current_user.id, task_id, project_id) if perm is None: raise HTTPException(status_code=404, detail="Project not found") if perm == "read_only": @@ -395,7 +395,6 @@ async def update_project_task( update_data = task_update.model_dump(exclude_unset=True) # SEC-P02: Assignees (non-owner, non-project-member with create_modify) restricted to content fields - project_perm = await get_project_permission(db, project_id, current_user.id) if project_perm not in ("owner", "create_modify"): # This user's create_modify comes from task assignment — enforce allowlist disallowed = set(update_data.keys()) - ASSIGNEE_ALLOWED_FIELDS diff --git a/backend/app/services/project_sharing.py b/backend/app/services/project_sharing.py index e56602d..6f6360d 100644 --- a/backend/app/services/project_sharing.py +++ b/backend/app/services/project_sharing.py @@ -110,19 +110,19 @@ async def validate_project_connections( async def get_effective_task_permission( db: AsyncSession, user_id: int, task_id: int, project_id: int -) -> str | None: +) -> tuple[str | None, str | None]: """ - Returns effective permission for a specific task: + Returns (effective_permission, project_level_permission) for a specific task. 1. Get project-level permission (owner/create_modify/read_only) 2. If user is assigned to THIS task → max(project_perm, create_modify) 3. If task has parent and user assigned to PARENT → same as above - 4. Return effective permission + 4. Return (effective, project_level) """ project_perm = await get_project_permission(db, project_id, user_id) if project_perm is None: - return None + return None, None if project_perm == "owner": - return "owner" + return "owner", "owner" # Check direct assignment on this task task_result = await db.execute( @@ -130,7 +130,7 @@ async def get_effective_task_permission( ) task_row = task_result.one_or_none() if not task_row: - return project_perm + return project_perm, project_perm parent_task_id = task_row[0] @@ -148,10 +148,10 @@ async def get_effective_task_permission( if assignment_result.scalar_one_or_none() is not None: # Assignment grants at least create_modify if PERMISSION_RANK.get(project_perm, 0) >= PERMISSION_RANK["create_modify"]: - return project_perm - return "create_modify" + return project_perm, project_perm + return "create_modify", project_perm - return project_perm + return project_perm, project_perm async def ensure_auto_membership( diff --git a/frontend/src/components/projects/ProjectDetail.tsx b/frontend/src/components/projects/ProjectDetail.tsx index 5908930..235a182 100644 --- a/frontend/src/components/projects/ProjectDetail.tsx +++ b/frontend/src/components/projects/ProjectDetail.tsx @@ -165,7 +165,12 @@ export default function ProjectDetail() { }, enabled: !!id, }); - const acceptedMembers = members.filter((m) => m.status === 'accepted'); + const acceptedMembers = useMemo(() => members.filter((m) => m.status === 'accepted'), [members]); + const myPermission = useMemo( + () => acceptedMembers.find((m) => m.user_id === currentUserId)?.permission ?? null, + [acceptedMembers, currentUserId] + ); + const canEditTasks = isOwner || myPermission === 'create_modify'; const toggleTaskMutation = useMutation({ mutationFn: async ({ taskId, status }: { taskId: number; status: string }) => { @@ -425,7 +430,7 @@ export default function ProjectDetail() { {/* Permission badge for non-owners */} {isShared && ( - {acceptedMembers.find((m) => m.user_id === currentUserId)?.permission === 'create_modify' ? 'Editor' : 'Viewer'} + {myPermission === 'create_modify' ? 'Editor' : 'Viewer'} )} {canManageProject && ( @@ -602,7 +607,7 @@ export default function ProjectDetail() { )} - {(isOwner || acceptedMembers.find((m) => m.user_id === currentUserId)?.permission === 'create_modify') && ( + {canEditTasks && (