[dev] [Marfuen] fix/custom-role-rbac-bugs#2831
Merged
Merged
Conversation
…r invites, gate portal Remove the APP_IMPLYING_RESOURCES fallback that let custom roles bypass the App Access toggle. Replace hardcoded role string checks in the member invite flow with RBAC permission checks (member:create/update), and add privilege escalation prevention for non-admin callers. Add portal:read / compliance-obligation check to the portal so unapproved roles are redirected. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…e checks Replace role string matching (isAdmin/isAuditor) with actual RBAC permission resolution — resolves the caller's member actions from both built-in and custom roles via BUILT_IN_ROLE_PERMISSIONS + DB lookup. Uses member:delete as the signal for full control (can assign any role) vs restricted (can only assign employee/contractor/custom roles). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove redundant validateAssignableRoles — the @RequirePermission guard on the controller already checks member:create. If the admin gave a custom role Members: Write, that role can invite. No second layer of role-string checks needed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/people/people-invite.service.ts">
<violation number="1" location="apps/api/src/people/people-invite.service.ts:447">
P1: Auditors can no longer invite other auditors. The new generic privilege check treats `auditor` as a privileged role (it's a built-in, non-restricted role), so any caller without `member:delete` — including auditors who only have `member:create,read` — is blocked from assigning it. This contradicts the permissions definition comment ("Can invite other auditors") and the PR description ("auditors can only invite auditors").
Consider adding a same-role exception: callers should be allowed to assign roles they themselves hold, or add explicit logic for the auditor self-invite case.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
… permission level Resolve the caller's member actions from RBAC (built-in + custom roles). Write-level access (all CRUD) can assign any role. Partial access (e.g. auditor with create+read only) can only assign restricted roles (employee/contractor) and custom roles — cannot assign privileged built-in roles. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace hardcoded isAdminOrOwner/isAuditor role string checks with Write-level member permission check (all CRUD actions). Mirrors the backend validation logic. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract the repeated typeof/JSON.parse pattern for OrganizationRole fields into typed helpers in the auth package. Replaces verbose defensive checks with one-liner calls that return typed objects. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/auth/src/permissions.ts">
<violation number="1" location="packages/auth/src/permissions.ts:269">
P1: `JSON.parse` can throw on malformed input. Since these helpers parse user-defined DB data and are otherwise written defensively, wrap the parse in a try-catch to return the safe fallback (`null` / `{}`) instead of crashing the caller.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic
Add try/catch to JSON parse helpers, extract generic parseJsonField, add isRestrictedRole() to eliminate verbose readonly casts, and make portal-access checks consistent. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Committed to the repo so all Claude agents working in this codebase will have it available and are required to run it after writing code. Checks for verbose patterns, inconsistent idioms, missing error handling, and readability issues. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
@cubic-dev-ai review this |
Contributor
@Marfuen I have started the AI code review. It will take a few minutes to complete. |
The hook now fires for all TS files in apps/ and packages/ and reminds agents to run the cleanup skill before committing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Marfuen
approved these changes
May 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is an automated pull request to merge fix/custom-role-rbac-bugs into dev.
It was created by the [Auto Pull Request] action.
Summary by cubic
Tightened RBAC for app/portal access and the member invite flow to prevent privilege escalation and align UI/API behavior with permissions. Users now need explicit
app:readto use the app, portal access is gated byportalor compliance, and invites use permission guards with server-side checks on assignable roles.Bug Fixes
app:read; removed implicit resource fallback.hasPortalAccess(checksportalor compliance via@trycompai/auth+@db); enforced on the org portal page.@RequirePermission(member:create); server resolves callermemberactions (built-in + custom) and enforces assignment—write-level (CRUD) can assign any role; others onlyemployee/contractorand custom.canInviteUsers/canManageMembersusemember:create/member:update; People page computesallowedBuiltInRoleswith the same write-level rule and passes to the invite modal.Refactors
@trycompai/auth: addedparseRolePermissions/parseRoleObligations(safe JSON parsing) andisRestrictedRole; exported helpers andRolePermissionsand used across API/portal for consistent role parsing and portal-access checks..claude/skills/cleanupand updated the PostToolUse hook to remind running the cleanup skill and typecheck for all TS files inapps/andpackages/.Written for commit 6ab7095. Summary will update on new commits.