Skip to content

Password reset: Report an unconfigured application URL before the user lookup and always apply the minimum response time - #24152

Merged
AndyButland merged 3 commits into
v17/devfrom
v17/bugfix/password-reset-hardening
Oct 9, 2026
Merged

AndyButland merged 3 commits into
v17/devfrom
v17/bugfix/password-reset-hardening

Conversation

@iOvergaard

Copy link
Copy Markdown
Contributor

Prerequisites

  • I have added steps to test this contribution in the description below

Description

Two small fixes to the backoffice password reset endpoints, both found while reviewing #24042.

1. Report an unconfigured application URL before looking up the user

POST /security/forgot-password returns the same response whether or not the email belongs to a user, except for statuses that are determined before the lookup. ApplicationUrlNotConfigured is surfaced on that basis, but it was only produced by IForgotPasswordUriProvider.CreateForgotPasswordUriAsync(user), after the user had been found. So on a site where the application URL can't be resolved, a registered email got a different response from an unknown one.

IForgotPasswordUriProvider gets CanCreateForgotPasswordUriAsync() for failures that don't depend on the user, and UserService.SendResetPasswordEmailAsync checks it before the lookup, the same way #24042 handles PasswordResetUnavailable. ForgotPasswordUriProvider implements it as the application URL check. The default implementation succeeds, so existing custom providers behave as before.

2. Always apply the minimum response time

EnsureMinimumResponseTimeFilter (used by the request, verify and reset password endpoints) checked forceWait.Microseconds > 0. That is only the 0-999 µs component of the remaining time, not the total, so the delay was skipped whenever that component happened to be zero. It now compares the whole TimeSpan, and measures with TimeProvider instead of Stopwatch so the behaviour can be tested.

Testing

Automated:

  • UserServiceCrudTests.Reset_Password_Link_Unavailable_Status_Does_Not_Depend_On_Whether_User_Exists (registered and unknown email get the same ApplicationUrlNotConfigured result). Fails without the fix.
  • EnsureMinimumResponseTimeFilterTests (delays until the minimum has elapsed, including a remaining wait of exactly 1.5 s; no delay once it has already elapsed). The 1.5 s case fails without the fix.
  • UriProviderTests.CanCreateForgotPasswordUri_* for the provider's new check.

Manual (optional):

  1. Configure a site so the application URL can't be resolved (no Umbraco:CMS:WebRouting:UmbracoApplicationUrl and detection disabled), with SMTP configured.
  2. Request a password reset for a registered and for an unknown email. Both should now get the same response.

🤖 Generated with Claude Code

iOvergaard and others added 2 commits October 9, 2026 12:38
…reset user lookup

ApplicationUrlNotConfigured was only produced once the user had been found,
so the reset endpoint answered a registered email differently from an
unknown one on sites where the application URL could not be resolved.

Add IForgotPasswordUriProvider.CanCreateForgotPasswordUriAsync for failures
that don't depend on the user, and check it before the lookup. The default
implementation succeeds, so existing providers are unaffected.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…dpoints

The filter checked TimeSpan.Microseconds, which is only the 0-999 µs
component of the remaining wait, so the delay was skipped whenever that
component happened to be zero. Compare the whole TimeSpan instead, and
measure with TimeProvider so the behaviour can be tested deterministically.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 9, 2026 10:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@claude

claude Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Claude finished @iOvergaard's task in 1m 39s —— View job


PR Review

Target: origin/v17/dev · Based on commit: af81ab84c0f94562c54735ab130de85f368e7555

This PR fixes two problems in the password reset endpoints. First, an unconfigured application URL now gets the same response whether or not the email belongs to a user: the new IForgotPasswordUriProvider.CanCreateForgotPasswordUriAsync() is checked before the user lookup. Second, the minimum response time filter now compares the whole remaining TimeSpan instead of only its Microseconds part, which is 0 to 999 µs.

  • Modified public API: IForgotPasswordUriProvider gets a new CanCreateForgotPasswordUriAsync() member with a default implementation. ForgotPasswordUriProvider gets a public override of it.
  • Affected implementations (outside this PR): Custom IForgotPasswordUriProvider implementations. They keep the default, which always succeeds, so their behaviour doesn't change. They only get the same-response guarantee for user-independent failures if they override the new method.
  • Other changes:
    • On a site where the application URL can't be resolved, POST /security/forgot-password now returns ApplicationUrlNotConfigured for unknown emails too, not only for registered ones.
    • The request, verify and reset password endpoints now apply the minimum response time every time. Before, the delay was skipped when the 0 to 999 µs part of the remaining time happened to be zero.

Suggestions

  • src/Umbraco.Core/Security/IForgotPasswordUriProvider.cs:30: The TODO says V19. Root CLAUDE.md §6.3 asks for V{next-major}, which is V18 here. Nothing is being obsoleted, so V18 is the earliest the default could be dropped. Ignore this if you deliberately want to give custom providers an extra major.

Things I checked and found correct:

  • No breaking changes. The interface member has a default implementation. The new EnsureMinimumResponseTimeFilter constructor is on an internal type.
  • Filter. Task.Delay(forceWait, _timeProvider) and GetElapsedTime use the system clock in production, through the existing constructor that passes TimeProvider.System.
  • Filter tests. The 500 ms case leaves exactly 1.5 s to wait, whose microseconds part is zero. That makes it a real regression test for the old check.
  • UserService. The new check runs after PasswordResetUnavailable and before the user lookup. Returning early inside the scope without completing it matches the existing UserNotFound path.
  • Integration test. The singleton TestForgotPasswordUriProviderState can't leak between tests, because the host is rebuilt for each test.
  • Usings. The now-unused System.Diagnostics using is removed. No unused usings were added.

Approved with Suggestions for improvement

Good to go, but please carefully consider the importance of the suggestions.

Comment thread src/Umbraco.Core/Security/IForgotPasswordUriProvider.cs

@AndyButland AndyButland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good @iOvergaard. Good catches and fixes to tighten this up.

@sonarqubecloud

sonarqubecloud Bot commented Oct 9, 2026

Copy link
Copy Markdown

@AndyButland
AndyButland merged commit e09b43d into v17/dev Oct 9, 2026
29 of 30 checks passed
@AndyButland
AndyButland deleted the v17/bugfix/password-reset-hardening branch October 9, 2026 12:24
iOvergaard added a commit that referenced this pull request Oct 12, 2026
…r lookup and always apply the minimum response time (#24152)

Cherry-picked without the UserServiceCrudTests changes, which build on test helpers from #24042 (not in this release). The unit tests for both fixes are included.

* fix(api): report an unconfigured application URL before the password reset user lookup

ApplicationUrlNotConfigured was only produced once the user had been found,
so the reset endpoint answered a registered email differently from an
unknown one on sites where the application URL could not be resolved.

Add IForgotPasswordUriProvider.CanCreateForgotPasswordUriAsync for failures
that don't depend on the user, and check it before the lookup. The default
implementation succeeds, so existing providers are unaffected.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(api): always apply the minimum response time on password reset endpoints

The filter checked TimeSpan.Microseconds, which is only the 0-999 µs
component of the remaining wait, so the delay was skipped whenever that
component happened to be zero. Compare the whole TimeSpan instead, and
measure with TimeProvider so the behaviour can be tested deterministically.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Added docs for EnsureMinimumResponseTimeFilter.

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Andy Butland <abutland73@gmail.com>
iOvergaard added a commit that referenced this pull request Oct 12, 2026
…r lookup and always apply the minimum response time (#24152)

Cherry-picked without the UserServiceCrudTests changes, which build on test helpers from #24042 (not in this release). The unit tests for both fixes are included.

* fix(api): report an unconfigured application URL before the password reset user lookup

ApplicationUrlNotConfigured was only produced once the user had been found,
so the reset endpoint answered a registered email differently from an
unknown one on sites where the application URL could not be resolved.

Add IForgotPasswordUriProvider.CanCreateForgotPasswordUriAsync for failures
that don't depend on the user, and check it before the lookup. The default
implementation succeeds, so existing providers are unaffected.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(api): always apply the minimum response time on password reset endpoints

The filter checked TimeSpan.Microseconds, which is only the 0-999 µs
component of the remaining wait, so the delay was skipped whenever that
component happened to be zero. Compare the whole TimeSpan instead, and
measure with TimeProvider so the behaviour can be tested deterministically.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Added docs for EnsureMinimumResponseTimeFilter.

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Andy Butland <abutland73@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants