Skip to content

feat(auth): include email in password-reset URL for password-manager pairing#92

Merged
NotThatKindOfDrLiz merged 1 commit into
mainfrom
feat/reset-email-param
Apr 25, 2026
Merged

feat(auth): include email in password-reset URL for password-manager pairing#92
NotThatKindOfDrLiz merged 1 commit into
mainfrom
feat/reset-email-param

Conversation

@rabble
Copy link
Copy Markdown
Member

@rabble rabble commented Apr 17, 2026

Summary

  • Adds &email=<url-encoded> to the password-reset URL emitted in both the dev logger and SendGrid/SMTP email paths (api/src/email_service.rs).
  • Token remains the sole authenticator for POST /api/auth/reset-password; the email is only a hint so mobile password managers can pair the new password with the existing saved credential instead of saving a duplicate entry.

Why

iCloud Keychain (iOS) and Google Password Manager (Android) will only update an existing saved credential when they see the same account identifier alongside the new password. Without the email on the reset URL, the mobile app (divine-mobile) has to create a new credential — producing duplicate entries for the same Divine account. Users then hit autofill with the stale entry, fail to sign in, and contact support.

Cross-repo dependency: divinevideo/divine-mobile#3156 — client-side fix is ready to ship once this lands.

Security note

Reset tokens are short-lived and are still the only thing that authorizes the reset. Putting the account email in the URL during that window is a conscious trade against the UX cost of duplicate password-manager entries. If there's a standing policy against any PII in URLs for reset flows, flag it and we'll switch to Option B in issue #3156 (a GET /api/auth/reset-password/inspect?token=XYZ endpoint that returns {email} without state change).

What is unchanged

Test plan

  • cargo build --lib on the api crate passes.
  • cargo clippy --lib is clean.
  • Library unit tests that don't require a live Postgres all pass (50/50). 3 failing tests in api::http::auth::tests::test_*_path_components are pre-existing — they fail on master without a local Postgres (VersionMissing(9) during sqlx::migrate!) and are unrelated to this change.
  • Manual verification in dev: trigger password reset and confirm the logged URL contains &email=<encoded> with +, @, . correctly escaped.
  • Mobile QA with Reset-password flow: plumb email into route so Keychain updates existing entries instead of duplicating divine-mobile#3156: reset password end-to-end on iOS and Android, confirm password manager updates the existing entry rather than creating a new one.

🤖 Generated with Claude Code

…pairing

Mobile clients (iCloud Keychain on iOS, Google Password Manager on Android)
can only update an existing saved credential when they see the account email
alongside the new password. Without the email in the reset URL, the app must
save a new entry, producing duplicate password-manager records for the same
Divine account.

Adds an `&email=<url-encoded>` param to the reset URL emitted by both the
dev console logger and the SendGrid/SMTP provider. Token remains the sole
authenticator for POST /api/auth/reset-password; the email is purely a hint
for client-side autofill pairing.

Refs divinevideo/divine-mobile#3156

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@realmeylisdev
Copy link
Copy Markdown

Paired-PR review from the mobile side (divinevideo/divine-mobile#3156). Contract verification, plus a few non-blocking quality notes.

Contract match with mobile: ✅

Traced the full round-trip for a tricky input test+alias@example.com:

  1. Backend emits (this PR): &email=test%2Balias%40example.com via urlencoding::encode — percent-encodes all except A-Za-z0-9-_.~, spaces → %20.
  2. Flutter deep-link listener's Uri.queryParameters['email'] decodes %2B+, %40@test+alias@example.com.
  3. Listener re-encodes for the internal GoRoute URL; router decodes again; screen displays the original.

Matches exactly what the mobile widget test asserts. No compatibility gap.

Non-blocking quality notes

  1. DRY — URL format is now duplicated in DevEmailSender::send_password_reset_email (email_service.rs:127-132) and SendGridEmailSender::send_password_reset_email (:399-409). Low-effort fix:

    fn build_reset_url(base_url: &str, token: &str, email: &str) -> String {
        format!("{}/reset-password?token={}&email={}", base_url, token, urlencoding::encode(email))
    }

    Reduces drift risk if the URL format ever changes again (add &locale=, rename email, etc.).

  2. No unit test for the URL format. PR body cites cargo build / cargo clippy passing. Not strictly needed for a 12-line template change, but if #1 is applied, a one-liner like

    #[test]
    fn reset_url_encodes_email_specials() {
        assert_eq!(
            build_reset_url("https://login.divine.video", "T", "test+alias@ex.com"),
            "https://login.divine.video/reset-password?token=T&email=test%2Balias%40ex.com",
        );
    }

    guards the mobile ↔ backend contract against both sides drifting in the future. High-value if cheap.

  3. reset_token interpolated unencoded. format!("...?token={}&email={}", ..., reset_token, ...) — the token is raw. Today's keycast tokens look URL-safe (alphanumeric), so this is fine. Defensive wrap with urlencoding::encode(reset_token) costs nothing and insulates against a future token-format change (e.g. base64 +//). Nit.

One question

"Account-claim and admin-reset flows untouched."

If there's an admin-triggered password reset that also surfaces a URL to the end-user's email, it has the same duplicate-in-Keychain problem this PR fixes. If the flow exists, worth the same 1-line treatment; if not, maybe note in the PR body so future readers don't have to re-check.

Security framing

The "PII-in-URL trade vs UX cost of duplicate Keychain entries" framing in the PR body is the right call. Reset tokens are short-lived and self-authenticating. Mobile side ships with a graceful fallback (null-guard on widget.email), so old reset emails in users' inboxes without &email= keep working after this lands.

Unblock

CI is green; mergeable_state: blocked is REVIEW_REQUIRED only. If there's a security/legal gate on email-in-URL for reset flows, they're the one to tag — otherwise any keycast maintainer's approval unblocks.

@realmeylisdev
Copy link
Copy Markdown

Paired-PR update from the mobile side so the deploy decision isn't blocked on info.

Mobile side is now pinned to this contract

Two regression tests on divinevideo/divine-mobile#3243 assert the exact byte shape this PR must produce, and they share a single @visibleForTesting helper so the mobile side can't drift silently:

  • mobile/test/router/reset_password_redirect_test.dart — router redirect preserves ?token=T&email=a%2Bb%40x.com end-to-end.
  • mobile/test/services/password_reset_listener_test.dart — native deep-link listener re-encodes test+alias@example.com to test%2Balias%40example.com.

Expanded contract verification

Extends my Apr 22 round-trip check beyond test+alias:

Input urlencoding::encode output Dart Uri.queryParameters decode Round-trip
a+b@example.com a%2Bb%40example.com a+b@example.com
user.name+tag@example.co.uk user.name%2Btag%40example.co.uk recovered
user%tag@example.com user%25tag%40example.com recovered
user@例え.jp (unicode IDN) UTF-8 bytes percent-encoded recovered
"" (empty) "" mobile null-guard treats as absent

Contract holds across the full input class.

Deploy sequencing — both orders are safe

Mobile has a null-guard on absent email, so old reset emails already in users' inboxes keep working regardless of order:

Order Old emails (no email) New emails (token+email)
Mobile ships first Pre-PR behavior preserved N/A yet
Keycast ships first N/A Old mobile clients fall back to pre-fix behavior (no crash; duplicate Keychain entries as before, same as today).
Both live Works Works — autofill updates the existing credential.

No gating either way — whichever is ready first can ship; the full benefit lands when both are live.

Remaining gap (unchanged from Apr 22)

The only open items are the two asks from my prior review:

  1. Extract build_reset_url to DRY the two senders (api/src/email_service.rs ~:127 and ~:399).

  2. Add a one-line assert_eq! test on it:

    #[test]
    fn reset_url_encodes_email_specials() {
        assert_eq!(
            build_reset_url("https://login.divine.video", "T", "test+alias@example.com"),
            "https://login.divine.video/reset-password?token=T&email=test%2Balias%40example.com",
        );
    }

~8 lines total. Pins the backend side of the contract so a future encoder swap or lost email arg fails CI instead of silently regressing #3156.

Copy link
Copy Markdown
Member

@NotThatKindOfDrLiz NotThatKindOfDrLiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the actual diff and the paired mobile discussion. I do not see a blocker in this change itself: it is narrowly scoped to adding a URL-encoded email hint to the emitted password-reset link, while the reset token remains the sole authenticator on the backend. The mobile-side contract review also makes the deploy sequencing risk clear and acceptable.

Two non-blocking follow-ups are now tracked:

  • #138 centralize and test password reset URL construction
  • #139 make auth/reset page referrer policy explicit for URL-carried email hints

Approving.

@NotThatKindOfDrLiz NotThatKindOfDrLiz merged commit 50b7b16 into main Apr 25, 2026
4 checks passed
NotThatKindOfDrLiz pushed a commit to divinevideo/divine-mobile that referenced this pull request Apr 26, 2026
…3243)

* fix(auth): plumb email into reset-password route for Keychain update

ResetPasswordScreen had no AutofillHints.username field co-located with
the new-password field inside its AutofillGroup, so iOS Keychain / Google
Password Manager filed reset-saves as new credential entries instead of
updating the existing one — producing duplicate password-manager entries
per account that users then autofilled, failed login, and contacted
support about.

Plumbs an optional email through the reset deep link:
  login.divine.video/reset-password?token=T&email=<url-encoded>

- PasswordResetListener extracts the email param and forwards it to the
  internal route; the log line now emits only uri.path, not the full URI
  (prevents a new PII leak once the URL carries email).
- Router passes email from query params into ResetPasswordScreen.
- ResetPasswordScreen accepts an optional email; when present, renders a
  read-only DivineAuthTextField with AutofillHints.username above the
  new-password field inside the existing AutofillGroup. Graceful fallback
  when email is absent (old reset emails still in users' inboxes).
- Wraps content in Expanded + SingleChildScrollView so the extra field
  doesn't overflow on small viewports.

Paired with divinevideo/keycast#92 which emits the email in the URL.
Client ships safely without backend thanks to the null-guard fallback;
end-to-end verification (Keychain update vs duplicate) requires both
deployed. Device test guide: tasks/pr3156_device_test_guide.md.

Closes #3156

* fix(auth): preserve email through top-level reset-password redirect

The top-level /reset-password GoRoute redirect rewrote the deep link
to the nested /welcome/login-options/reset-password path but forwarded
only the token — dropping the email query param. Native deep links via
PasswordResetListener already forward email, but that listener is
native-only (app_links), so on the Flutter web build at login.divine.video
the redirect was the only entry point and the AutofillHints.username
field never rendered there. Web users still got duplicate password-
manager entries, which is the exact bug #3156 is meant to fix.

Mirror the StringBuffer + Uri.encodeQueryComponent logic from
PasswordResetListener so both entry points produce identical output
for identical input. Backward-compat preserved when email is absent
(old reset emails still in users' inboxes).

Drive-by: `?? ''` on token avoids the prior `?token=null` string
interpolation when the query param is missing.

Adds a pure-function regression test at
mobile/test/router/reset_password_redirect_test.dart following the
pattern in login_flow_redirect_test.dart. Covers: token+email
preserved, email absent (backward compat), email empty string,
special-char round-trip, and missing-token safety.

* refactor(router): share reset-password redirect logic with its test

The router-level test was a line-for-line mirror of the inline redirect
callback — if one drifted, the other could still pass silently. Extract
the rewrite as a @VisibleForTesting top-level function and have both
the GoRoute and the test call it directly, so there is exactly one
source of truth.

Behavior unchanged; all 99 router tests green.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants