Uh oh!
There was an error while loading. Please reload this page.
Fix four account-takeover paths found by a security review - #686
Merged
Conversation
Three related holes, all reachable from inside the admin area by an actor the system does not consider privileged enough for the effect they achieve. 1. The admin user form gated the password field on `Scope.can_access_admin_area?/1`, which is true for any holder of a single permission — despite the template comment saying "Admin/Owner only". Any role granted `users` (which the Admin role is auto-granted at boot) could therefore open the Owner's record and set a new password. Nothing else in the request path checked rank: not the router pipeline, not the on_mount hook, not the changeset. `Auth.can_manage_user_credentials?/2` now answers that question by ROLE and RANK, mirroring `can_delete_user?/2` and the impersonation authority, and gates the password field, the reset-mail button and the email field (owning the address a reset link goes to takes an account just as surely as setting the password). The events refuse independently of the template, since a LiveView event is a message the client composes. 2. Deactivating a user denied them access but never revoked their tokens: the row stayed valid for its full 60-day life, so any entry point resolving a token without the active-user filter still saw a live user. `update_user_status/2` now deletes the session tokens on true->false. 3. `MultiSession.root_user/1` and `root_authenticated?/1` resolved the root session token with a bare lookup, skipping the `ensure_active_user/1` filter that the plugs and `switch_to/2` apply. Combined with (2), a deactivated Owner/Admin holding a live cookie could POST to the impersonation endpoint — which sits in a pipeline with no auth gate — and mint a fresh session as any other active user.
`AuthPageWrapper.bg_style_tag/1` built a `<style>` element by string
concatenation from three settings values and emitted it through `raw/1`. A
`<style>` element's contents are raw character data, so HTML escaping does not
apply there and a value containing `</style>` closes the element and opens
whatever follows.
The background colour is a free-text field on /admin/settings/authorization,
whose permission key is `settings` — held by Manager as well as Admin, and
explicitly not Owner-only. The resulting script was served to every ANONYMOUS
visitor of the login, registration and password-reset pages, which makes
harvesting the credentials typed into the login form the obvious payload.
`PhoenixKit.Utils.CssValue` is an allowlist that fails to "": a colour may
contain only the characters colours and gradients need, so `;`, `{`, `}`, `<`,
`>`, quotes, backslash and `/` cannot appear at all, and `url(` / `expression(`
are refused by name. URLs must be a rooted path or an explicit http(s) URL with
no character that can terminate the `url('…')` token.
Applied on read (so values already in the table are neutralised), again in the
one function that assembles the stylesheet (so a caller passing the assign
directly cannot bypass it), and on write (so the operator is told the value was
rejected instead of silently keeping a broken page).…ertion `find_or_create_user/3` looked the callback's email up locally and, on a hit, confirmed that account and issued a session for it. Email string equality was the only check: no verification claim was read, no prior link was required, and the query was unscoped. Whoever gets an address attached to a provider account — or who registers a local account on an address someone will later sign in with — ends up in the other party's account. The claim was already in hand and discarded: `extract_oauth_data/1` captures `raw_info`. Resolution is now ordered by the proof each case carries. An existing (provider, provider_uid) link is decisive and consults no email. A pre-existing local account requires the provider to assert it verified the address — Google and OIDC `email_verified`, GitHub's per-address `verified`, Facebook `verified` — and is otherwise refused with `:provider_email_unverified` and a message telling the owner to sign in with their password and link the provider from settings. A new account is still created, but is only auto-confirmed on an assertion, so an unvouched address cannot be laundered into a confirmed local account. `oauth_require_verified_email` (default true) lets a deployment whose provider does not surface the claim lift the requirement deliberately.
The signed file-URL section recorded three read-side weaknesses. The write side is worse and was missing: POST /api/upload sits in the scope that pipes through [:browser, :phoenix_kit_auto_setup] with no auth plug, and UploadController falls back to params["user_uuid"] under a comment promising an admin check that was never written. Confirmed against a running app.
Found by an adversarial review of the previous commit. The claim lookup read
`raw_info["user"]` with a STRING key, but all three shipped strategies build
`raw_info` with ATOM keys — `%{token: ..., user: ...}` in ueberauth_google
(strategy/google.ex:122), ueberauth_github (:205) and ueberauth_facebook
(:139). The payload nested under `:user` is decoded JSON and does use string
keys, which is what made the mistake easy to miss.
So `raw_user/1` returned `%{}` on every real callback and
`provider_asserts_verified_email?/1` was constantly false. Two consequences,
neither of them the intended behaviour:
* a user with a password account could no longer sign in with Google, GitHub or
Facebook at all — the attach path refused every time;
* the obvious way out of that is `oauth_require_verified_email: false`, which
reopens exactly the takeover the gate exists to prevent. A broken gate that
pushes the operator toward disabling it is worse than no gate.
Every claim lookup now accepts both spellings via `fetch_claim/2`. The tests
were part of the problem — they built `raw_info` with a string key, so they
asserted a shape the strategies never produce; they now use the real atom-keyed
shape, with the string-keyed and top-level-OIDC variants kept as separate cases.
Also documents that Facebook's `verified` is an account-level flag rather than
a per-address one — the strongest signal that provider offers, and weaker than
Google's or GitHub's.…e more places Two adjacent holes of the shapes the previous commits closed, both raised by review of those commits. Deactivation was decided without reference to the actor: `update_user_status/2` consults `Roles.can_deactivate_user?/1`, which takes only the TARGET and asks one question — is this the last Owner. A role holding just `users` could therefore switch off an Admin, or a non-last Owner. That is not a takeover, but it is a denial of service against exactly the accounts meant to outrank it, and it is the same "permission admits you to the page, the action ignores rank" shape as the credential fix. The rank rule is now shared: `validate_admin_authority_over/2` backs both `can_manage_user_credentials?/2` and the new `can_manage_user_status?/2`, the event refuses, and the template explains the refusal instead of offering a button that will not work. Three more sites resolved a session token without `ensure_active_user/1`: the maintenance-mode plug (which decides who walks past the 503 — a token that outlived a deactivation got through), and the two `conn_scope/1` twins in the session and OAuth controllers. The last two are scope-for-redirect rather than access grants, but a deactivated account should not produce a non-anonymous scope anywhere, and leaving the filter off some resolutions is how the multi-session hole happened in the first place.
Two independent reviews of the previous commits found the same thing: the rank
check for activation was added to the admin EDIT FORM only, while
`update_user_status/2` is reached from three LiveViews. The user list
(`live/users/users.ex`) had a self-check and nothing else; the user detail page
had neither. Both pages are gated on the `users` permission, so a role holding
just that could still deactivate an Admin by clicking, or a non-last Owner with
a hand-built event — and since deactivation now revokes tokens, the target is
signed out instantly. The fix advertised in the previous commit was live on one
page out of three.
Leaving the rule in the callers is what made that possible, so it moves to the
one function they all go through: `update_user_status/3` takes `actor:` and
refuses with `:insufficient_permissions`. Omitting `:actor` is the explicit
system path (`Users.Referrals` expiring an account). All three pages now pass
the actor, and the menu items are hidden with the same predicate, matching how
`can_delete_user?/2` is already used a few lines away.
`toggle_user_confirmation/2` gets the same treatment. It is offered from the
same pages to the same holders, and `require_email_confirmation` is honoured at
eleven gates — so unconfirming an Admin locks them out of every protected page.
Same shape, same actor, same denial of service.
Two corrections that came out of the same review:
* `can_manage_user_status?/2` now refuses SELF. The admin form has always told
the operator "You cannot deactivate your own account for security reasons",
but only in markup — the server accepted it, and a LiveView event is a message
the client composes. Credentials keep the opposite rule: your own password is
yours to change.
* The unreachable `validate_admin_authority_over/2` catch-all is removed; both
public wrappers already answer `false` for a non-`%User{}`.
The tests now bind the rule to the function every caller goes through, rather
than to the predicate a caller may never ask.Review found the gate could not be satisfied for GitHub, and that the setting
documented as the operator's escape hatch did not exist.
`OAuthConfig` registered the GitHub strategy as `{Ueberauth.Strategy.Github, []}`,
so it ran with the strategy's default scope of `""`. Without `user:email` the
token cannot read `GET /user/emails`, the strategy stores the user WITHOUT an
`"emails"` key, and the verification lookup therefore had nothing to find. With
the requirement on by default, every GitHub sign-in to an existing account was
refused and every new GitHub account was created unconfirmed. The scope is now
requested — read-only access to the address list is also the minimum that
answers the question.
`oauth_require_verified_email` was read in exactly one place and declared
nowhere: not in `Settings.get_defaults/0`, not in the settings schema, not in
the admin UI. An operator hitting a provider that reports no verification could
only change it from iex. It is now a real setting with a default of `"true"` and
a checkbox on /admin/settings/authorization, next to the providers it governs,
with copy that states the consequence of turning it off.
Also corrected: the comment on `user_by_provider_identity/1` claimed
`(provider, provider_uid)` has no unique index. It does —
`phoenix_kit_oauth_providers_provider_uid_idx`, migration V16 — so the ordering
was justified by something untrue. The comment now records what that branch
really means: an existing link is treated as proof, which is right for a
provider whose uid is server-assigned and immutable, and is NOT right for one
whose `sub` a user can choose. It also cannot re-verify link rows written by the
older email-equality code; auditing those is an operator task for the release
notes.
Tests: the shape GitHub actually delivers with no email scope (a user payload
with no `"emails"` key at all), the setting being honoured in both positions,
and the setting being registered in the defaults and the schema.`is_active` was checked by the password controller only. Magic-link verification, QR-login completion and the OAuth callback all call `UserAuth.log_in_user/3` directly, and it did not check — so a deactivated account was issued a fresh session token. The fetch plugs then resolved it to nil, making the result a confusing dead session rather than access, but the correctness of that depended on every plug remembering the filter, which is the dependency this branch exists to remove. The check now sits on the funnel every entry point shares. Also from review: `update_user_profile/3` dropped `password` and `email` for an actor without credential authority, but not `username` — the second identifier `get_user_by_email_or_username_and_password/3` accepts, so rewriting it takes a sign-in route away from an account the actor may not manage. And two CssValue assertions were verifying nothing: `url(https://…)` is refused by the character allowlist alone (`:` and `/`), so both would still pass with the forbidden-construct pattern deleted. Added `url(x)` and `URL (x)`, which consist only of permitted characters and can therefore be rejected by nothing else.
timujinne
commented
Aug 7, 2026
timujinne
left a comment
ContributorAuthor
There was a problem hiding this comment.
Independent adversarial review — two reviewers, both REQUEST-CHANGES; findings addressed in 3 follow-up commits
This PR was reviewed twice, independently and in parallel, by two models given the same brief: defeat each fix, then look for the same shape elsewhere. Neither saw the other's output. Both returned REQUEST-CHANGES on this PR and APPROVE on the sibling phoenix_kit_emails#27.
Posting the merged result here for the record, since three of the findings were defects in the security fixes themselves rather than in the code they fixed.
HIGH — the activation rank check was live on one page out of three (both reviewers, independently)
The previous commit claimed activation was decided by rank. It was — in the admin edit form. Auth.update_user_status/2 is reached from three LiveViews:
| Entry point | Guard before this review |
|---|---|
lib/phoenix_kit_web/users/user_form.ex:426 | rank check ✅ |
lib/phoenix_kit_web/live/users/users.ex:422 → :605 → :658 | self-check only |
lib/phoenix_kit_web/live/users/user_details.ex:253 → :483 | nothing |
All three map to the users permission key (web/users/auth.ex:1829), so reachability is identical: any custom role granted users — not Admin, not Owner. The menu item rendered unconditionally and its wrapper hid only Owner targets, so Admin was one click away; Owner was one hand-built LiveView event away, since Roles.can_deactivate_user?/1 protects only the last Owner. Commit 2 of this PR made it worse by revoking tokens on deactivation, turning it into an immediate forced sign-out of the accounts that outrank the actor.
Fixed by moving the rule to the function all three go through: update_user_status/3 takes actor: and refuses with :insufficient_permissions; omitting it is the explicit system path (Users.Referrals). Menu items now hidden with the same predicate, matching the existing can_delete_user?/2 idiom.
toggle_user_confirmation/2 had the identical shape and is now gated the same way — require_email_confirmation is honoured at eleven gates, so unconfirming an Admin locks them out of every protected page.
HIGH — the OAuth verified-email gate could not be satisfied for GitHub, and its documented switch did not exist (second reviewer)
Two halves, both mine:
OAuthConfigregistered the strategy as{Ueberauth.Strategy.Github, []}, i.e. with the strategy's default scope of""(deps/ueberauth_github/…/github.ex:76). Withoutuser:emailthe token cannot readGET /user/emails; the strategy stores the user with no"emails"key, so the verification lookup had nothing to find. With the requirement on by default, every GitHub sign-in to an existing account was refused and every new GitHub account was created unconfirmed.oauth_require_verified_emailwas read in exactly one place and declared nowhere — not inSettings.get_defaults/0, not in the settings schema, not in the admin UI. The "deliberate, operator-visible decision" the docstring promised was reachable only from iex.
Fixed: the scope is requested; the setting is registered with a default of "true" and a checkbox on /admin/settings/authorization next to the providers it governs. Tests now pin the no-"emails"-key shape and both positions of the switch.
MEDIUM — a comment justified the code with something untrue
user_by_provider_identity/1 said (provider, provider_uid) has no unique index. It does — phoenix_kit_oauth_providers_provider_uid_idx, migration V16. Corrected, and the comment now records what that branch actually means: an existing link is treated as proof, which is right for a provider whose uid is server-assigned and immutable and not right for one whose sub a user can choose or recycle.
MEDIUM — deactivated accounts were still issued sessions by three of four entry points
is_active was checked by the password controller only; magic-link verification, QR-login completion and the OAuth callback call log_in_user/3 directly and it did not check. The fetch plugs filtered afterwards, so the effect was a dead session rather than access — but that correctness depended on every plug remembering, which is the dependency this branch exists to remove. The check now sits on the shared funnel.
LOW — fixed alongside
can_manage_user_status?/2refused nothing for self. The admin form has always said "You cannot deactivate your own account for security reasons" — in markup only. Now refused server-side; credentials keep the opposite rule, since your own password is yours to change.update_user_profile/3droppedpasswordandemailfor an actor without credential authority, but notusername— the second identifierget_user_by_email_or_username_and_password/3accepts.- Unreachable
validate_admin_authority_over/2catch-all removed.
Test quality — two tests were asserting nothing
Both reviewers checked whether the tests would fail with the fix reverted:
- Two
CssValueassertions (url(https://evil.example/x)) are refused by the character allowlist alone, so they would pass with@color_forbiddendeleted. Replaced withurl(x)/URL (x), which consist only of permitted characters and can be rejected by nothing else. - The OAuth tests never reproduced the shape GitHub actually delivers, and never exercised the setting. Both added.
- The status tests asserted the predicate in isolation while two pages bypassed it entirely — which is precisely why the HIGH survived. They now bind the rule to the function every caller goes through.
Confirmed correct by both reviewers
PhoenixKit.Utils.CssValue (one reviewer reimplemented both filters and fuzzed them; no breakout of the <style> element or the url('…') token was constructible, and %27 is a URL character rather than a terminator in CSS) · credential management on the edit form, with every caller of admin_update_user_password/3 and deliver_user_reset_password_instructions/3 enumerated · multi-session root resolution and the three added ensure_active_user/1 filters · role escalation already closed upstream by Roles.sync_user_roles/3 · no regressions in the existing suites.
Verification after the follow-ups
mix test test/integration/users test/phoenix_kit/utils — 528 tests + 16 doctests, 0 failures against a real PostgreSQL. mix compile --warnings-as-errors, mix format --check-formatted, mix credo --strict (10446 mods/funs) all clean.
For the release notes, not the code
- Link rows in
phoenix_kit_oauth_providerswritten before this change were created by the email-equality path. The new gate protects new attachments and cannot retroactively re-verify old ones — worth an audit query on any install where a provider was enabled before this release. - Deactivation revokes tokens but does not disconnect an already-mounted LiveView;
on_mountdoes not re-run until the next live_session transition or reload.
ddon pushed a commit
that referenced
this pull request
Aug 7, 2026
Post-merge review of #686 (four account-takeover paths). The five paths it closes are real and closed in the context rather than the template; five further defects surfaced, all fixed here. Full write-up in dev_docs/pull_requests/2026/686-fix-four-account-takeover-paths/CLAUDE_REVIEW.md. - Deletion never got the rank rule it is cited as the model for. `can_delete_user?/2` refused only an *Admin* target and never asked whether the actor held a staff role at all, deferring that to a page gate that admits any single-permission holder. An Owner holds only the `Owner` role, so an Admin — or a `users`-permission role — could delete any Owner who was not the last one. `validate_can_delete_user/2` now routes through the PR's own `validate_admin_authority_over/2`; self and last-Owner keep their messages. - A new OAuth account left unconfirmed is now sent the confirmation mail. Nothing on that path ever sent it, so an account the provider did not vouch for was signed in with "Successfully signed in!" and then bounced off every gate honouring `require_email_confirmation`, with an empty inbox. - The gettext catalogs were stale and the merge guessed four wrong translations. #686 added six strings and touched no catalog; all four refusal flashes fuzzy-matched onto "You don't have permission to manage permissions" — and fuzzy entries render. Re-extracted, translated all six in all seven locales, cleared the flags. Back to 2190/2190 everywhere. - The new per-row rank guard no longer queries per row. It asked up to four `EXISTS` questions per rendered user — ~200 round trips on a fifty-row page, on every sort, filter and PubSub re-render — though `list_users_paginated/1` already preloads `:roles`. The rule now reads that preload when present and the list hands it an actor loaded once. - The confirm/unconfirm menu entry is gated like its status twin, which was the only one hidden for an out-of-rank target. Also: postgrex 0.22.3 -> 0.22.4, clearing the `mix hex.audit` advisory, and the credo --strict failure #686 merged with. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ddon pushed a commit
that referenced
this pull request
Aug 9, 2026
This was referenced Aug 9, 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 freeto 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.
Found by a security review of the whole workspace on 2026-08-07 and verified against a running application. Four independent paths, each of which ends with an attacker holding someone else's account. Each commit is self-contained and carries its own reasoning.
1. Credential management on the admin user form ignored rank
/admin/users/edit/:idgated the password field onScope.can_access_admin_area?/1— true for any holder of a single permission — while the template comment above it said "Admin/Owner only". Nothing else in the path checked rank: not the pipeline, not the on_mount hook, not the changeset, not a caller one frame up. Any role grantedusers(which Admin is auto-granted at boot) could open the Owner's record and set a new password.Auth.can_manage_user_credentials?/2now answers by role and rank, mirroring the rulescan_delete_user?/2and the impersonation authority already enforce, so the three takeover surfaces agree. It gates the password field, the reset-mail button and the email field — owning the address a reset link is delivered to takes an account just as surely as setting the password does. The events refuse independently of the template.2. Deactivation denied access but did not revoke it
update_user_status/2flippedis_activeand broadcast; the token rows stayed valid for their full 60-day life. Every gate that reads a session appliesensure_active_user/1, so an operator sees the account bounce and believes it is off — while any entry point that resolves a token without that filter still sees a live user. Deactivation now deletes the session tokens.3. Multi-session resolved the root account without the active-user filter
root_user/1androot_authenticated?/1used a bare token lookup whereswitch_to/2usesensure_active_user/1. With (2), a deactivated Owner/Admin holding a live cookie couldPOST /users/session/impersonate/:uuid— a route in a pipeline with no auth gate — and mint a fresh session as any other active user.4. Stored XSS in the auth-page branding settings
AuthPageWrapper.bg_style_tag/1built a<style>element by string concatenation and emitted it withraw/1. A<style>body is raw character data, so HTML escaping does not apply and</style>in a value closes the element. The background colour is free text on/admin/settings/authorization, whose key issettings— held by Manager as well as Admin — and the result is served to every anonymous visitor of the login, registration and reset pages.New
PhoenixKit.Utils.CssValueis an allowlist that fails to"", applied on read (neutralising values already stored), in the one function that assembles the stylesheet, and on write (so the operator learns the value was rejected).5. OAuth attached an external identity on email string equality
find_or_create_user/3looked the callback address up locally and, on a hit, confirmed that account and issued a session. No verification claim was read — althoughextract_oauth_data/1already capturesraw_info— no prior link was required, and the query was unscoped.Resolution is now ordered by the proof each case carries: an existing
(provider, provider_uid)link is decisive and consults no email; a pre-existing local account requires the provider's own assertion (Google/OIDCemail_verified, GitHub per-addressverified, Facebookverified); a new account is created but auto-confirmed only on an assertion.oauth_require_verified_email(defaulttrue) lets a deployment lift the requirement deliberately rather than by accident.Also
AGENTS.md's storage TODO gains the unauthenticatedPOST /api/uploaddefect (no auth plug on the scope; owner taken fromparams["user_uuid"]under a comment promising a check that was never written). Implementation is being handled separately.Verification
mix test test/integration/users— 341 tests, 0 failures against a real PostgreSQL, including 19 new ones covering every case above.mix format --check-formatted,mix credo --strict(10435 mods/funs, no issues),mix compile --warnings-as-errors— all clean.Compatibility
Behaviour changes an operator will notice, all deliberate: