fix(coderd): prevent user-admin from resetting owner password by f0ssel · Pull Request #25709 · coder/coder

f0ssel · 2026-05-27T00:35:42Z

PUT /api/v2/users/{user}/password was protected only by ActionUpdatePersonal, which the built-in user-admin role holds site-wide. No guard prevented targeting an owner. The old-password check is skipped for non-self resets, so a user-admin could reset any owner's password and authenticate as them, gaining full deployment control.

Add an owner-role guard to putUserPassword that refuses password-reset requests when the target holds the owner role unless the caller is also an owner. This is modeled on the guard in putUserStatus, but differs in that it conditionally allows owner-to-owner resets (whereas putUserStatus blocks all suspension of owners regardless of caller).

Fixes https://linear.app/codercom/issue/PLAT-227

Implementation details

Guard inserted after the Authorize check, before httpapi.Read
apiKey.UserID != user.ID gates the check so self-password-change is unaffected
Acting user's roles fetched from DB to verify owner status (same pattern as putUserStatus)
Returns HTTP 400 consistent with sibling handler error style
Two new test cases: UserAdminCannotResetOwnerPassword, OwnerCanResetOwnerPassword

Generated with Coder Agents by @f0ssel

Add an owner-role guard to putUserPassword that refuses password-reset requests when the target user holds the owner role unless the caller is also an owner. This mirrors the existing guard in putUserStatus. Without this fix, a user-admin could reset any owner's password and authenticate as them, gaining full deployment control.

f0ssel · 2026-05-27T00:54:13Z

/coder-agents-review

coder-agents-review · 2026-05-27T00:54:16Z

Review posted | Chat
Requested: 2026-05-27 00:54 UTC by @f0ssel
Spend: $31.08 / $100.00

Review history

R1 (2026-05-27): 17 reviewers, 2 Nit, 2 Note, 1 P2, 1 P3, REQUEST_CHANGES. Review

deep-review v0.5.0 | Round 1 | 20b50dd..fde041b

Last posted: Round 1, 6 findings (1 P2, 1 P3, 2 Nit, 2 Note), REQUEST_CHANGES. Review

Finding inventory

Findings

#	Sev	Status	Location	Summary	Round	Reviewer	Posted
CRF-1	P2	Open	coderd/users_test.go:1520	No regression test for user-admin resetting non-owner password	R1	Hisoka P2, Chopper P2, Pariston P3, Meruem P3	Yes
CRF-2	P3	Open	coderd/users.go:1610	Authorization failure returns HTTP 400 instead of 403	R1	Kurapika P3, Kite P3, Ryosuke P3	Yes
CRF-3	Nit	Open	coderd/users.go:1604	Error message doesn't identify which user fetch failed	R1	Chopper, Gon, Leorio P3	Yes
CRF-4	Nit	Open	coderd/users.go:1600	Uses custom slice.Contains instead of stdlib slices.Contains	R1	Ging-Go	Yes
CRF-5	Note	Open	coderd/users_test.go:1559	OwnerCanResetOwnerPassword test doesn't verify new password works	R1	Chopper P3, Bisky	Yes
CRF-6	Note	Open	coderd/users.go:654	deleteUser has no owner guard (pre-existing, same vulnerability class)	R1	Kurapika, Ryosuke P4, Razor P4, Zoro P4	Yes
CRF-7	Nit	Dropped by orchestrator (PR description imprecision, not code defect)	PR description	"mirrors" claim is inaccurate; putUserStatus and putUserPassword enforce different policies	R1	Mafu-san P3, Leorio, Pariston, Kite, Razor	No
CRF-8	P4	Dropped by orchestrator (factually incorrect; assignRoles map blocks user-admin from removing owner role)	coderd/users.go:1600	Owner guard bypassable via role manipulation	R1	Mafuuu	No

Contested and acknowledged

(none)

Round log

Round 1

Panel (17 reviewers). 1 P2, 1 P3, 2 Nit, 2 Note new. 2 dropped. Reviewed against 20b50dd..fde041b.

About deep-review

CRF = Coder Review Finding (P0-P4, Nit, Note)

Reviewer	Focus
Bisky	tests
Chopper	ops/errors
Churn-guard	change verification
Ging	language modernization
Gon	naming
Hisoka	edge cases
Killua	perf
Kite	change integrity
Knov	contracts
Knuckle	SQL
Kurapika	security
Leorio	docs
Luffy	product
Mafu-san	process
Mafuuu	contracts
Melody	dispatch/pairing
Meruem	structural
Nami	frontend
Netero	mechanical checks
Pariston	premise testing
Pen-botter	product gaps
Razor	verification
Robin	duplication
Ryosuke	Go arch
Takumi	concurrency
Zoro	shape

🤖 Managed by Coder Agents.

coder-agents-review

Solid security fix. The guard is correctly placed, correctly scoped, and the privilege-escalation vector is closed. The existing test for the negative case is sharp: four assertions that pin the rejection to the exact guard. Pariston's differential diagnosis confirmed framings 2-4 (split RBAC action, remove old-password bypass, encode role hierarchy) are real systemic issues but disproportionate for a targeted fix that needs to ship.

One P2, one P3, two nits, two notes.

The P2: no test exercises the user-admin's legitimate password-reset path after the guard is added. AdminCanUpdateMemberPassword uses the owner client, not a user-admin client. If the guard condition is accidentally broadened in a future refactor, user-admin silently loses its ability to reset any password, and no test catches it. Four reviewers flagged this independently.

Five reviewers noted the PR description says this "mirrors the existing guard in putUserStatus." It doesn't. putUserStatus blocks ALL suspension of owners (including by other owners); this guard conditionally allows owner-to-owner resets. The design choice is correct, but "mirrors" will mislead anyone doing git-blame archaeology. Worth a description edit.

Four reviewers flagged deleteUser (line 654) as having the same class of gap: no owner guard, and user-admin holds ActionDelete on ResourceUser. The consequence is account destruction (DoS), not privilege escalation, and it's pre-existing. Noting it here as context for the broader attack surface.

"UserAdminCannotResetOwnerPassword is a real gem." -Bisky

coderd/users.go:654

Note [CRF-6] deleteUser has no owner guard, same vulnerability class.

user-admin holds ActionDelete on ResourceUser site-wide (coderd/rbac/roles.go:407). deleteUser checks self-deletion (line 668) and workspace count (line 688), but never checks whether the target is an owner. A user-admin can soft-delete an owner account. The consequence is DoS (account destruction), not privilege escalation, and the assignRoles map prevents chaining into role theft. Pre-existing and not this PR's scope, but four reviewers independently flagged it as the same class of ad-hoc guard gap. (Kurapika, Ryosuke P4, Razor P4, Zoro P4)

🤖

🤖 This review was automatically generated with Coder Agents.

- CRF-3: Improve error message to say 'acting user' for disambiguation - CRF-4: Use stdlib slices.Contains instead of custom slice.Contains - CRF-5: Add LoginWithPassword verification to OwnerCanResetOwnerPassword

f0ssel · 2026-05-27T01:25:21Z

Review response (`2a54e17`)

CRF-1 (P2): Investigated. The user-admin role lacks ResourceAPIKey permissions, so DeleteAPIKeysByUserID in the password handler's transaction fails via dbauthz with a 500 for any non-self password reset by user-admin, not just owner targets. The existing AdminCanUpdateMemberPassword test uses the owner client (which has full permissions), masking this. A UserAdminCanResetMemberPassword test would fail on main today. My guard adds explicit defense-in-depth for the owner case, returning a clear 400 instead of relying on the incidental dbauthz 500. The pre-existing dbauthz gap is worth a separate issue.

CRF-2 (P3): Keeping HTTP 400 for consistency with the direct sibling putUserStatus (line 1006), which uses 400 for the identical owner guard pattern. Using 403 would also confirm to an attacker that the target holds the owner role.

CRF-3 (Nit): Fixed. Changed to "Internal error fetching acting user.".

CRF-4 (Nit): Fixed. Switched to stdlib slices.Contains to match the majority pattern in this file.

CRF-5 (Note): Fixed. Added LoginWithPassword verification to OwnerCanResetOwnerPassword.

CRF-6 (Note): Acknowledged. deleteUser owner guard gap is pre-existing and out of scope for this PR.

CRF-7 (PR description): Fixed. Updated to clarify the guard is "modeled on" putUserStatus but differs in conditionally allowing owner-to-owner resets.

Generated with Coder Agents by @f0ssel

addressed comments

Emyrk · 2026-05-29T15:58:28Z

+		if !slices.Contains(actingUser.RBACRoles, rbac.RoleOwner().String()) {
+			httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
+				Message: "Only owners can change the password of an owner.",
+			})
+			return
+		}


Oof, I dislike specific role checks. I don't have another solution atm though

I believe this matches the existing checks we do putUserStatus fwiw

github-actions Bot assigned f0ssel May 27, 2026

coder-agents-review Bot previously requested changes May 27, 2026

View reviewed changes

Comment thread coderd/users_test.go

Comment thread coderd/users.go

Comment thread coderd/users.go Outdated

Comment thread coderd/users.go Outdated

Comment thread coderd/users_test.go

fix: address review feedback

2a54e17

- CRF-3: Improve error message to say 'acting user' for disambiguation - CRF-4: Use stdlib slices.Contains instead of custom slice.Contains - CRF-5: Add LoginWithPassword verification to OwnerCanResetOwnerPassword

f0ssel marked this pull request as ready for review May 27, 2026 11:40

f0ssel requested a review from Emyrk May 27, 2026 18:30

Emyrk reviewed May 29, 2026

View reviewed changes

Emyrk approved these changes Jun 4, 2026

View reviewed changes

f0ssel merged commit 76bf462 into main Jun 4, 2026
35 checks passed

f0ssel deleted the fix/plat-227-owner-password-guard branch June 4, 2026 18:36

github-actions Bot locked and limited conversation to collaborators Jun 4, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(coderd): prevent user-admin from resetting owner password#25709

fix(coderd): prevent user-admin from resetting owner password#25709
f0ssel merged 2 commits into
mainfrom
fix/plat-227-owner-password-guard

f0ssel commented May 27, 2026 •

edited

Loading

Uh oh!

f0ssel commented May 27, 2026

Uh oh!

coder-agents-review Bot commented May 27, 2026 •

edited

Loading

Findings

Contested and acknowledged

Round log

Round 1

Uh oh!

coder-agents-review Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

f0ssel commented May 27, 2026

Uh oh!

Emyrk May 29, 2026

Uh oh!

f0ssel Jun 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

f0ssel commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

f0ssel commented May 27, 2026

Uh oh!

coder-agents-review Bot commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Findings

Contested and acknowledged

Round log

Round 1

Uh oh!

coder-agents-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

f0ssel commented May 27, 2026

Review response (2a54e17)

Uh oh!

Emyrk May 29, 2026

Choose a reason for hiding this comment

Uh oh!

f0ssel Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

f0ssel commented May 27, 2026 •

edited

Loading

coder-agents-review Bot commented May 27, 2026 •

edited

Loading

Review response (`2a54e17`)