Skip to content

[PM-40507] Change Password Well Known URLs Issues#8036

Open
gbubemismith wants to merge 5 commits into
mainfrom
vault/pm-40507-change-password-uri
Open

[PM-40507] Change Password Well Known URLs Issues#8036
gbubemismith wants to merge 5 commits into
mainfrom
vault/pm-40507-change-password-uri

Conversation

@gbubemismith

@gbubemismith gbubemismith commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-40507

📔 Objective

The extension's Change Password notification was hanging for 30–60 seconds before timing out and falling back to the raw cipher URI, despite the target domain still exposing a valid .well-known/change-password endpoint. The issue reproduced only on US production (not self-host or EU cloud) and behaved inconsistently, flipping between working and broken. This PR fixes the underlying DNS-resolution regression and stops transient lookup failures from being cached as permanent "no change-password URL" answers.

Root cause

The SSRF protection added in #7256 resolves DNS on every outbound probe via Dns.GetHostEntryAsync, which also performs a reverse (PTR) lookup the SSRF check never needs, and passes no cancellation token — so a slow resolve was never bounded by the 20-second HttpClient timeout. Two sequential probes against a stalling resolver produced the 30–60 second hang. Because DNS/PTR resolution is environment-specific, this only manifested on US production.

Two caching layers then amplified the failure into intermittent, "sticky" behavior: the controller cached the resulting transient null for 24 hours, and the Icons pipeline stamped a blanket 7-day edge (Fastly) Cache-Control header on every response — including failures — pinning a wrong answer per-PoP for a week.

What this fixes

  1. DNS (root cause)SsrfProtectionHandler now uses Dns.GetHostAddressesAsync (forward resolution only) and threads the cancellation token through, so a stalled resolve is bounded by the request timeout. This benefits every SSRF-protected client (Icons, Webhook, Datadog, Slack, Teams, Admin).
  2. Failure cachingChangePasswordUriService now returns a result that distinguishes a definitive answer (Found / NotSupported) from a transient LookupFailed, instead of collapsing both to null. The controller caches only definitive answers; transient failures are not cached.
  3. Edge cache — the change-password route no longer receives the 7-day blanket Cache-Control. The controller sets its own header per result: public, max-age=1h for definitive answers, no-store for transient failures.

📸 Screenshots

Edge-cache bug - Failed lookup is cached for 7 days

Before

image

After

image

DNS/PTR Stall

Before

image

After

image

@github-actions

github-actions Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the change-password well-known URL bugfix spanning the SSRF DNS-resolution fix, the ChangePasswordUriService result-type refactor, the controller caching/Cache-Control logic, and the Icons pipeline header change. The DNS root-cause fix (forward-only resolution via GetHostAddressesAsync with the cancellation token threaded through) is correct and benefits every SSRF-protected client. The transient-vs-definitive result distinction and per-result Cache-Control handling are sound, and the new controller/service tests plus the SSRF cancellation regression test cover the relevant paths.

Code Review Details

No findings above the confidence threshold.

Notes considered but not flagged:

  • catch (Exception) in GetChangePasswordUri treats an SsrfProtectionException as a transient LookupFailed (never cached, re-probed each request). This is an explicitly documented, deliberate design choice for a rare edge case, not a correctness bug.
  • The HttpRequestMessage/HttpResponseMessage/HttpClient disposal concerns raised earlier by automated tooling were already addressed via using statements.

Comment thread src/Icons/Services/ChangePasswordUriService.cs Fixed
Comment thread src/Icons/Services/ChangePasswordUriService.cs Fixed
Comment thread test/Icons.Test/Services/ChangePasswordUriServiceTests.cs Fixed
@codecov

codecov Bot commented Jul 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.37500% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.49%. Comparing base (ddcbd36) to head (3fb0660).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/Icons/Startup.cs 0.00% 9 Missing ⚠️
...c/Icons/Controllers/ChangePasswordUriController.cs 95.65% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8036      +/-   ##
==========================================
+ Coverage   62.44%   62.49%   +0.04%     
==========================================
  Files        2293     2294       +1     
  Lines       99829    99941     +112     
  Branches     9010     9017       +7     
==========================================
+ Hits        62337    62454     +117     
+ Misses      35319    35308      -11     
- Partials     2173     2179       +6     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@gbubemismith gbubemismith added ai-review-vnext Request a Claude code review using the vNext workflow t:bugfix Change Type - Bugfix labels Jul 22, 2026

@nick-livefront nick-livefront 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.

Nice find, and solution!

@jaasen-livefront jaasen-livefront left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good! Just a single failing test to fix.

@gbubemismith

Copy link
Copy Markdown
Contributor Author

Looks good! Just a single failing test to fix.

It's a flaky test. There's a WIP #8041 to address it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review-vnext Request a Claude code review using the vNext workflow t:bugfix Change Type - Bugfix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants