OUT-3661 | Limited access IUs shouldn't see tasks that are associated with clients/companies beyond their access list#1208
OUT-3661 | Limited access IUs shouldn't see tasks that are associated with clients/companies beyond their access list#1208arpandhakal wants to merge 1 commit intomainfrom
Conversation
…ies from limited-access IUs Migrates Tasks.associations from jsonb[] to jsonb to enable Prisma's array_contains operator, then filters tasks by association companyId (not just assignee) for limited-access IUs across API, cascade SQL, and realtime. Also fixes a pre-existing useFilter dedup bug that caused disjoint-promoted subtasks to render twice. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR closes a security gap where limited-access IUs could see tasks associated with out-of-scope clients/companies, even when the task's assignee was an IU or the task was unassigned. To enable clean SQL filtering,
Confidence Score: 3/5The security logic is consistent across API, realtime, and in-memory paths, but the unguarded parse in The unguarded
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Task request — limited-access IU] --> B{getAccessFilterForTasks}
B --> C[AND]
C --> D["OR: internalUserId≠null OR unassigned OR companyId IN accessList"]
C --> E["getAccessibleAssociationsFilter: associations=[] OR array_contains companyId in accessList"]
D & E --> F[SQL query filtered task rows]
F --> G{filterTasksByClientAccess in-memory pass}
G --> H{task.companyId in accessList?}
H -- No companyId or in list --> I{any association companyId out of scope?}
I -- No --> J[Task visible]
I -- Yes --> K[Task blocked]
H -- Out of scope --> K
F --> L[Realtime path]
L --> M{hasInaccessibleAssociation?}
M -- Yes --> N[Block INSERT/UPDATE]
M -- No --> O[Apply to store]
Reviews (1): Last reviewed commit: "fix(OUT-3661): hide tasks associated wit..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return false | ||
| } | ||
| // If task is associated with a client/company, every association's company must be in access list | ||
| const associations = AssociationsSchema.parse(task.associations) || [] |
There was a problem hiding this comment.
AssociationsSchema.parse can throw for all tasks
AssociationsSchema.parse(task.associations) will raise a ZodError if any row's associations column is not a valid association array (e.g. a bare JSON object {} or a string). Previously, IU-assigned tasks and fully-unassigned tasks returned early before this branch was reached, so only client/company tasks were ever parsed. Now every task in the batch is parsed. If a single malformed row exists — whether from a backfill bug, a direct-DB write, or pre-schema-max(1) data — the entire filterTasksByClientAccess call throws, rendering the task list completely inaccessible for all callers on that path. Use AssociationsSchema.safeParse with a fallback to avoid cascading failures.
| ALTER TABLE "Tasks" | ||
| ALTER COLUMN "associations" DROP DEFAULT, | ||
| ALTER COLUMN "associations" TYPE JSONB | ||
| USING coalesce(to_jsonb("associations"), '[]'::jsonb), | ||
| ALTER COLUMN "associations" SET DEFAULT '[]'::jsonb, | ||
| ALTER COLUMN "associations" SET NOT NULL; |
There was a problem hiding this comment.
equals-based association filters silently break if any task has multi-element associations
The hasSome → equals conversion relies entirely on the application-level AssociationsSchema.max(1) constraint. There is no CHECK constraint in the DB, so any row written before max(1) was enforced, or via a direct DB write, would have more than one element. For such a row equals: [{clientId, companyId}] never matches, causing that task to silently disappear from the CU/CRM read path. Consider adding a max(1) DB check constraint as part of this migration to make the invariant enforceable at the storage layer.
| ALTER TABLE "Tasks" | |
| ALTER COLUMN "associations" DROP DEFAULT, | |
| ALTER COLUMN "associations" TYPE JSONB | |
| USING coalesce(to_jsonb("associations"), '[]'::jsonb), | |
| ALTER COLUMN "associations" SET DEFAULT '[]'::jsonb, | |
| ALTER COLUMN "associations" SET NOT NULL; | |
| ALTER TABLE "Tasks" | |
| ALTER COLUMN "associations" DROP DEFAULT, | |
| ALTER COLUMN "associations" TYPE JSONB | |
| USING coalesce(to_jsonb("associations"), '[]'::jsonb), | |
| ALTER COLUMN "associations" SET DEFAULT '[]'::jsonb, | |
| ALTER COLUMN "associations" SET NOT NULL; | |
| -- Enforce the application-level max(1) invariant at the DB layer so that | |
| -- `equals`-based Prisma filters remain correct even after direct DB writes. | |
| ALTER TABLE "Tasks" | |
| ADD CONSTRAINT "Tasks_associations_max_one_element" | |
| CHECK (jsonb_array_length("associations") <= 1); |
| const isReassignedOutOfLimitedIUScope = (() => { | ||
| if (this.userRole !== AssigneeType.internalUser) return false | ||
| const iu = InternalUsersSchema.parse(this.user) | ||
| if (!iu.isClientAccessLimited) return false | ||
| const companyAccessList = iu.companyAccessList || [] | ||
| // Out of scope via association: applies even when assignee is an IU or unassigned | ||
| if (this.hasInaccessibleAssociation(updatedTask.associations, companyAccessList)) { | ||
| return true | ||
| } | ||
| if (updatedTask.internalUserId || !updatedTask.assigneeId) { | ||
| return false | ||
| } | ||
| return iu.isClientAccessLimited && !iu.companyAccessList?.includes(updatedTask.companyId || '__') | ||
| return !companyAccessList.includes(updatedTask.companyId || '__') | ||
| })() |
There was a problem hiding this comment.
isReassignedOutOfLimitedIUScope evaluates the updated state only — missing explicit documentation of the downstream guard
The early-return at line 332 (hasInaccessibleAssociation(updatedTask.associations, ...)) can fire even when the task was never in the board. The caller guards against this on line 343 (tasks.find((task) => task.id === updatedTask.id)), so there is no data corruption. A clarifying comment here would make the correctness argument self-evident without requiring readers to trace through to the guard.
1 participant
Summary
Closes OUT-3661.
Limited-access IUs were filtered only by task assignee company. Tasks merely associated with an out-of-scope client/company still leaked through. This PR closes that gap across the API, cascade SQL, and realtime paths.
To make the SQL filter clean and avoid expensive Copilot SDK enumeration of all clients/companies,
Tasks.associationsis migrated fromjsonb[]tojsonb(still holding an array). That unlocks Prisma'sarray_contains(@>containment), which lets us match associations bycompanyIddirectly usingcompanyAccessList.Schema + migration
Tasks.associationscolumn:jsonb[]→jsonbUSING coalesce(to_jsonb(associations), '[]'::jsonb)— Postgres rewrites every row duringALTER. No separate app-side backfill. JS shape ([{clientId?, companyId}]) is preserved across the migration.Core fix (
tasksShared.service.ts)filterTasksByClientAccessnow also rejects tasks whoseassociationsreference a company outsidecompanyAccessList— this is the actual user-visible fix for the read path.getAccessFilterForTasksANDs the newgetAccessibleAssociationsFilterfor limited IUs, so cascade workflow updates and subtask counts stay in sync with the read filter.getDisjointTasksFilter(limited IU branch) treats a parent as inaccessible when its associations are out of scope. Combined into oneparent: { OR: [...] }to avoid Prisma double-joining the parent relation.getAccessibleAssociationsFilterusesarray_contains(Postgres@>) for partial-match —{companyId: cid}matches both stored{companyId}and{clientId, companyId}shapes without needing to enumerate clientIds.hasSomefilters (which relied on jsonb exact-element equality) converted toOR: [{equals: ...}, ...]. Behavior is identical formax=1arrays — semantics-preserving.validateTaskShareupdated for the newJson(single) shape with anArray.isArrayguard.hasSome→equalsconversions in adjacent filesMechanical, semantics-preserving:
src/app/api/tasks/tasks.service.ts—getIncompleteTasksForCompany,resetAllSharedTaskssrc/app/api/comments/comment.service.ts— three CU/CRM filter sitessrc/app/api/webhook/webhook.service.ts—handleCompanyUnassignmenttasksSharedandcommentservicesRealtime sync (
src/lib/realtime.ts)Realtime guardrails for limited IUs only checked
task.companyId. Now also checkassociationsso a task withcompanyId = nullbut an out-of-scope association can no longer slip into the store via realtime.hasInaccessibleAssociationmirrors the API filter.isSubtaskAccessible,handleRealtimeTaskInsertguardrail,isReassignedOutOfLimitedIUScope,isReassignedIntoLimitedIUScope.Disjoint duplicate fix (
src/hooks/useFilter.tsx)Pre-existing latent bug: limited-IU disjoint-promoted subtasks rendered twice in the board after AllTasksFetcher Suspense resolved (the "second card appears with a delay" symptom). The standalone-subtasks dedup checked only
parentId. Now also checks the subtask's ownid, so a task that's already infilteredParentTasks(via disjoint promotion) isn't re-added as a standalone.Type tightening
src/app/api/tasks/subtasks.service.ts:JsonValue[]→JsonValuesrc/app/api/tasks/public/public.service.ts:Prisma.JsonValue[]→Prisma.JsonValuePre-existing oddities preserved (NOT introduced or worsened by this PR — worth a follow-up ticket)
getIncompleteTasksForCompanyfilters for{clientId: null, companyId}but stored objects don't storenullclientIds (the key is either present with a UUID or absent) — this branch likely never matches.resetAllSharedTasksfilters for{clientId: assigneeId}(no companyId) butcompanyIdis required byAssociationSchema— that half of the OR likely never matches.Both behaviors carried over identically.
Test plan
equalspreserves the priorhasSomestrict-match semantics).{clientId, companyId}association) and tasks associated company-wide ({companyId}association).🤖 Generated with Claude Code