Uh oh!
There was an error while loading. Please reload this page.
Fix registration sessions not persisting; add auth policy settings - #667
Merged
Conversation
…-registry loading/param-key guards
Registration auto-login never wrote the persistent remember-me cookie, so
the session rode the plain browser-session cookie that mobile browsers drop
when they evict a tab. Users who registered on a phone were silently logged
out overnight even though their 60-day session token was still valid —
nothing referenced it. Magic-link login had the same gap.
Session persistence is now one site-wide policy with a per-user opt-out:
- `remember_me_enabled` (default true) is the master switch; off hides the
checkbox everywhere AND blocks the cookie inside
`maybe_write_remember_me_cookie/3`, so no caller or forged param can
persist a session.
- `remember_me_default` (default true) decides whether the checkbox starts
checked. Users untick it for a session-only login.
- All four flows with a UI (password login, registration, magic-link
completion, QR handoff) render the same pre-checked box; the two
registration LiveViews track it across `phx-change` re-renders so
unticking sticks through the trigger-action POST.
- Flows with nothing to tick (magic-link login, OAuth) follow
`remember_me_default` via `Auth.remember_me_params/0` instead of the
hardcoded `true` they carried before.
Post-auth destinations become configurable through one resolver,
`Routes.post_auth_path/1`: `after_login_path` (default "/") and
`after_registration_path` (empty = fall back to after-login). An explicit
`return_to` — query param or gate-stashed session key — always wins.
`log_in_user/3` now honors a `"return_to"` in its params, which the OAuth
callback had always passed and silently lost. Both settings are validated as
local paths on save and re-guarded on read.
`require_email_confirmation` (default true, the historical behavior) gates
enforcement only; confirmation emails still send. All six gates honor it: the
two conn plugs and the four on_mount hooks.
/users/confirm no longer strands the users parked there. Gates record where
the user was headed, and the page moves them along both on mount (covering a
`confirmed_at` flipped straight in the database, then a refresh) and live off
the existing `{:user_confirmed, _}` broadcast, so clicking the emailed link
in another tab advances the parked one. It subscribes before re-reading the
user so a confirmation landing mid-mount cannot be missed, and the email-link
LiveView resolves the same destination so the two tabs cannot diverge.
Security and robustness fixes found along the way:
- `Routes.local_path?/1`, the only redirect guard in the codebase, accepted
ASCII control characters. Browsers strip tab/CR/LF while parsing, so
`/\t/evil.example` arrived at window.location as `//evil.example`.
`Phoenix.Controller.redirect/2` rejects those itself, but LiveView's
`validate_local_url!` does not — and the new auto-advance is a LiveView
redirect.
- `Settings.get_setting/1` had no error handling and the cached variants had
`rescue` only, yet an unreachable database raises on an unowned checkout
and exits on a dead pool. A transient database problem therefore crashed
its callers, the login redirect resolver among them. Both clauses added,
log-then-default, mirrored in the boot-safe URL wrappers.
- LayoutWrapper's standalone fallback (no host `layout:` configured) nested a
second full document inside the LiveView and passed `inner_content: nil`
into a template rendering `{@inner_content}`, so auth pages rendered chrome
with an empty body and duplicate ids. Flash now belongs to the LiveView
tree alone; the root layout's copy double-rendered every message and froze
at its dead-render value.The "Sign in with QR code" button read "Sign in with password" in Russian,
Estonian and French. Gettext had fuzzy-matched all three against a similar
password-login string, and fuzzy entries render live, so the button on the
login page actively told users the wrong thing.
Corrected it in all seven catalogues and dropped the fuzzy flags so a later
merge cannot resurrect the guess. The five other QR strings — the admin
toggle, the page title, and the three sign-in outcome messages — were empty
and falling back to English; they are translated now too.
Also moves the Login & Registration reference into
`dev_docs/guides/2026-07-28-login-and-registration.md`, matching the split
upstream just made, and leaves AGENTS.md with a summary section.
Note: ~250-485 fuzzy entries per language still render auto-matched text, and
spot checks show real mistranslations of the same kind ("Archived" as
"Активно"/active, "Accept" as "Tout accepter"/accept all, "Choose" as
"Fermer"/close). That is a separate sweep.Three external reviewers went over the login and registration surface. Their findings, verified against the code and fixed here. Session persistence, the same failure the original fix was written to close: - Changing a password deletes every token for the user, including the one inside the live remember-me cookie, and the dashboard form that re-logs them in carries no checkbox. The user kept a cookie pointing at a deleted token — fine until the browser dropped its session cookie, then a silent sign-out and a cookie that kept failing for 60 days. The re-login now carries the choice the user already made, and a login without remember-me actively clears any cookie rather than leaving a stale one behind. - `remember_me_enabled` was enforced only when writing. Turning it off left every already-issued cookie restoring sessions until it expired, which is the opposite of what turning it off means; the switch is honored on read now. Email confirmation: - Magic-link registration never confirmed the account. `confirmed_at` was put into attrs, but the registration changeset casts a fixed allowlist that omits it, so the value was dropped — and with confirmation required by default, a user who had just proved inbox control was parked at /users/confirm by a flow that sends no confirmation email. There was no link to click. - The four role-based conn plugs checked the role but never the email, and the shipped :phoenix_kit_admin_only pipeline runs require_admin with no preceding authentication plug, so a host controller route behind it enforced confirmation nowhere. They share one gate now. - Organization invitations were unreachable with confirmation turned off: the auto-accept runs only from confirm_user/1, and the manual banner was itself gated on confirmed_at. Both paths honor the setting. Enumeration and abuse, all on public endpoints: - Password reset was rate-limited inside the delivery call, so only addresses that resolved to a user were ever throttled. Past the threshold a registered address got "Too many password reset requests" while an unknown one still got the generic notice, which turned deliberately vague copy into a precise account-existence oracle. Limiting happens before the lookup now, and all outcomes answer identically. - Confirmation resend had no limit at all. Each request for an existing unconfirmed account inserted a token and sent mail synchronously: an unauthenticated targeted-mail-flood vector and a timing oracle. - The magic-link registration request said "This email is already registered", which neither login nor magic-link login does. Redirects: - `after_login_path` accepted `/users/log-out`, a real GET route, so every successful login would sign the user straight back out — including the admin who would have to undo it. Every path that bounces an authenticated visitor is now refused, at save time and at read time, since update_setting/2 skips the changeset. - `return_to` was dropped by every alternate sign-in route. It now threads through the links between login, register, magic-link, QR and OAuth, and through the magic-link email itself. - `after_registration_path` was stashed before the credentials were checked, so a rejected handoff left it in the session and the user's next ordinary login landed on the registration page. Smaller ones: an expired or raced registration token crashed the completion LiveView with a CaseClauseError; the public registration form could write `custom_fields`, the one attacker-reachable cast field, so both forms now pass only what they own; abandoned OAuth attempts left their return_to and referral code in the session for the next login to inherit; and a parked unconfirmed user subscribed to the site-wide admin users feed, receiving every other user's struct — it listens on its own confirmation topic now. Test isolation: rate limiting keys on the peer from get_peer_data/1, not conn.remote_ip, and the test adapter reports the same peer for every conn — so these tests shared one login bucket and got bounced under some seed orderings. Each test now gets its own peer.
ddon pushed a commit
that referenced
this pull request
Jul 28, 2026
Post-merge review of #667 (registration session persistence + auth policy settings). Findings in dev_docs/pull_requests/2026/667-auth-session-persistence-policy-settings/CLAUDE_REVIEW.md. - Password reset charged its per-email rate-limit bucket twice per public request: the forgot-password LiveView now throttles before the user lookup, but the limiter inside deliver_user_reset_password_instructions/2 was left in place and hits the same key. A registered user's SECOND reset request in a 5-minute window was denied and silently sent nothing while the page still showed the success notice. deliver_*/3 gains a :rate_limit option (default true, so the admin "send reset link" action stays protected) and the LiveView opts out. - log_in_user/3 guarded its return_to with local_path?/1 only, so ?return_to=/users/log-out — a real GET route — signed the user back out the instant they signed in. The settings are refused for exactly this reason; the URL param, which is less trusted, was not. Both it and post_auth_path/1's candidates now apply the same local + not-a-sign-in-page rule, and log_in_user/3 resolves through the single post_auth_path/1 resolver. - @auth_paths and auth_page?/1 were copy-pasted into Routes (read side) and Setting.SettingsForm (save side). Deduped onto a public Routes.auth_page?/1 so a new auth route can't be guarded on read and forgotten on write. - Session.maybe_store_after_registration_path/1 didn't re-guard the stored value the way Routes.after_login_path/0 does. Settings persist from changeset.params, not changes, so the changeset's trim never reaches storage and " /welcome" was silently ignored on read. - AGENTS.md said require_email_confirmation is honored at six sites and listed seven, omitting the four role plugs wired through confirmation_gate/2. It's eleven. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ddon pushed a commit
that referenced
this pull request
Jul 28, 2026
Independent re-check of release 1.7.217 (d7008e0) against each review's findings: every claimed fix confirmed present in the released code with file:line evidence, gates re-run (mix quality.ci exit 0, format clean, touched unit suites + test.js green). Records the CHANGELOG gap: the #667 auth fixes ship in 1.7.217 with no changelog entry.
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.
The incident
A user registered on their phone, used the site all evening, and was signed out by morning. Nothing server-side explains it: the session token was valid for 60 days and still in the table.
Registration auto-login was the one flow that never wrote the persistent remember-me cookie, so the session rode the plain browser-session cookie — which mobile browsers drop when they evict the tab. The token was fine; nothing referenced it. Magic-link login had the same gap.
What changed
Session persistence is now a site-wide policy with a per-user opt-out. A real "Keep me logged in" checkbox, pre-checked, on all four flows that have a UI (password login, registration, magic-link completion, QR handoff). Two settings back it:
remember_me_enabled(master switch — off hides the checkbox and blocks the cookie at the writer, so no caller or forged param can persist a session) andremember_me_default(whether it starts checked). Flows with nothing to tick — magic-link login, OAuth — follow the default instead of the hardcodedtruethey carried before.Post-auth destinations are configurable.
after_login_pathandafter_registration_path, resolved through one helper. An explicitreturn_toalways wins, and it now threads through every alternate sign-in route rather than dying the moment a user switches method.require_email_confirmation(default true, unchanged behaviour) gates enforcement only — emails always send. All six gates honour it./users/confirmno longer strands people. It advances on mount when already confirmed (covering aconfirmed_atflipped straight in the database, then a refresh) and live off the existing broadcast, so clicking the emailed link in another tab moves the parked one along.Bugs found along the way
Beyond the incident, mostly surfaced by three external reviewers:
Routes.local_path?/1— the only redirect guard in the codebase — accepted ASCII control characters. Browsers strip tab/CR/LF while parsing, so/\t/evil.examplereachedwindow.locationas//evil.example.Phoenix.Controller.redirect/2blocks these; LiveView's validator does not, and the new auto-advance is a LiveView redirect.confirmed_atwas dropped by the changeset's cast allowlist, so a user who had just proved inbox control was parked at a page whose flow sends no email.after_login_pathaccepted/users/log-out, a real GET route: every login would sign the user straight back out, admin included.:phoenix_kit_admin_onlypipeline has no preceding auth plug.Settings.get_setting/1had no error handling and the cached variants onlyrescue, but an unreachable database raises on an unowned checkout and exits on a dead pool — so a transient DB blip crashed callers, the login redirect resolver among them.Notes
main: summary inAGENTS.md, full reference indev_docs/guides/2026-07-28-login-and-registration.md.Still open, deliberately out of scope: ~250–485 fuzzy gettext entries per language render auto-matched text, and spot checks show the same class of error elsewhere — Russian shows "Archived" as Активно (active), French shows "Accept" as Tout accepter and "Choose" as Fermer. That wants its own sweep.
Verification
2216 tests, 0 failures;
mix precommit(compile as errors, format, credo --strict, dialyzer) clean. 121 new tests, including a regression for each finding above.