Repository navigation
fix(security): members can only grant permissions they hold - #6624
Merged
Merged
Conversation
mikeh-lago
force-pushed
the
security/gl-004-role-permission-subset
branch
from
October 9, 2026 23:08
a095fc5 to
2db9506
Compare
mikeh-lago
marked this pull request as ready for review
October 10, 2026 14:04
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a47e254048
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
groyoh
approved these changes
Oct 12, 2026
Creating a role, or adding permissions to one, now requires the acting member to hold every permission being granted. Admins are not affected. Renaming a role or removing permissions from it stays allowed.
… does not hold A non-admin member with members:update or members:create could assign or invite any non-admin role, including roles carrying permissions they do not hold, and assign such a role to themselves. - Add Membership#can_grant_roles?: admins can grant any role, other members only roles whose permissions they hold. - Check it in Memberships::UpdateService and Invites::UpdateService for added roles, and in Invites::CreateService for the invite roles. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DQ5fPejwJTr99LvF5zYRbw
Add a grantable field on Role, computed with Membership#can_grant_roles?, so the front end can disable roles the member is not allowed to assign. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DQ5fPejwJTr99LvF5zYRbw
A custom role can no longer be created or renamed to admin, finance or manager (case-insensitive), so it cannot be mistaken for a predefined role. Existing roles stay valid until their name changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DQ5fPejwJTr99LvF5zYRbw
can_grant_roles? compared permissions only, so a non-admin holding every permission saw the predefined Admin role as grantable while the services reject that assignment. Admin roles are now grantable by admins only. Specs use a named subject and declare fixtures outside examples. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DQ5fPejwJTr99LvF5zYRbw
- Membership#can_grant_permissions? holds the grant rule once; role services and can_grant_roles? use it. - Roles::UpdateService refuses any update of a role the member could not grant, not only added permissions. - Reserved names use a format validation, and normalize_name collapses Unicode spaces and format characters. - Spec for a member update keeping a stronger role already held. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DQ5fPejwJTr99LvF5zYRbw
Reload the shared membership between examples and give the grantable contexts their own user, since main now creates a default user. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DQ5fPejwJTr99LvF5zYRbw
mikeh-lago
force-pushed
the
security/gl-004-role-permission-subset
branch
from
October 12, 2026 20:20
99ac41b to
ced67d8
Compare
Saving an existing role no longer rewrites its name, so a legacy name with a Unicode space stays updatable. Normalization also removes default ignorable code points (e.g. U+034F) before the reserved name check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DQ5fPejwJTr99LvF5zYRbw
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
2 participants
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.
Context
A member allowed to manage roles or members could grant permissions they don't hold themselves: create or update a role with extra permissions, then assign it to themselves or someone else through a membership update or an invite.
A custom role could also be named like a predefined role (Admin, Finance, Manager), which made it look like the real one in the UI.
Changes
Membership#can_grant_roles?: admins can grant any role; other members only roles whose permissions they all hold.Roles::CreateService/Roles::UpdateService: refuse to add a permission the acting member doesn't hold (cannot_grant_permissions).Memberships::UpdateService,Invites::CreateService,Invites::UpdateService: refuse to assign a role the acting member can't grant.Role.grantableGraphQL field, so the front end can disable roles the current member can't grant.Role: namesadmin,financeandmanagerare reserved (case-insensitive) on create and rename. Existing roles stay valid until renamed.Tests
Model, service and GraphQL specs for each path, including a non-admin member trying to grant a permission they don't hold, and reserved names in any case.