Repository navigation
fix(users): require the caller to hold the target's privileges on update and delete - #442
Open
rudycelekli wants to merge 1 commit into
Open
rudycelekli wants to merge 1 commit into
rudycelekli wants to merge 1 commit into
Conversation
…ate and delete PatchUser and DeleteUser only checked users.edit / users.delete, so a custom role holding just those privileges could reset the password of, block or delete a more privileged account. CreateUser already refuses to grant a role whose privileges the caller lacks; apply the same rule to the target of an update or delete. Acting on one's own record is unchanged. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
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
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.
Problem
PatchUserandDeleteUserinbackend/pkg/server/services/users.goonly check forusers.edit/users.delete. They never check that the caller holds every privilege of the target user's role, althoughCreateUserdoes. A custom role that hasusers.editorusers.deletebut fewer other privileges can therefore reset the password of, block, or delete an administrator.This needs a custom role to be reachable (only the default Admin role holds those privileges today), so it is defence-in-depth rather than an exploitable default.
Change
Add
canManageUser, which applies the same privilege-subset ruleCreateUseruses to the target's role and skips the check for the caller's own record. It runs in both handlers after the target is loaded and returns 403NotPermitted.Tests
New
TestUsers_PatchAndDeleteUser_RefuseATargetWithPrivilegesTheCallerLackscovers patch and delete against an administrator (refused, row untouched) and against a user whose privileges the caller holds (allowed).go test ./pkg/server/servicespasses with the change.users.go.🤖 Generated with Claude Code