Recover apiRootUrl on app password callback cache miss

[adalpari]

Description

The in-memory ApiRootUrlCache is populated during the initial site
discovery step and read again when the application-password auth
callback comes back. If the app process is killed between those two
moments (or discovery never ran for that exact URL), the cache is empty
on the callback and storeApplicationPasswordCredentials fails with a
spurious BadData error, blocking login.

This PR makes that path recoverable:

• In ApplicationPasswordLoginHelper, when apiRootUrl is missing but
  siteUrl is present on the callback, re-run wpLoginClient.apiDiscovery
  as a fallback, repopulate the cache, and continue the login flow.
• In ApiRootUrlCache, normalize the malformed-URL fallback through the
  same stripToDomain helper used by the happy path so put and get
  always derive the same key for the same input (previously a malformed
  write and a later read could end up under different keys).

Testing instructions

This is fixing and edge case, so it's not easily reproduceable.

1. Perform a normal application-password login

• Verify login still completes successfully

[dangermattic]

	1 Warning
⚠️	PR is not assigned to a milestone.

Generated by 🚫 Danger

[wpmobilebot]

📲 You can test the changes from this Pull Request in WordPress Android by scanning the QR code below to install the corresponding build.

	App Name	WordPress Android
	Build Type	Debug
	Version	pr22771-fd92266
	Build Number	1488
	Application ID	org.wordpress.android.prealpha
	Commit	fd92266
	Installation URL	6n8v4ar6litcg

Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

[wpmobilebot]

📲 You can test the changes from this Pull Request in Jetpack Android by scanning the QR code below to install the corresponding build.

	App Name	Jetpack Android
	Build Type	Debug
	Version	pr22771-fd92266
	Build Number	1488
	Application ID	com.jetpack.android.prealpha
	Commit	fd92266
	Installation URL	6qs6nk5mefa20

Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

[codecov]

Codecov Report

❌ Patch coverage is 80.95238% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 37.42%. Comparing base (2eec23e) to head (fd92266).
⚠️ Report is 3 commits behind head on trunk.

Files with missing lines	Patch %	Lines
...i/accounts/login/ApplicationPasswordLoginHelper.kt	75.75%	4 Missing and 4 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            trunk   #22771   +/-   ##
=======================================
  Coverage   37.41%   37.42%           
=======================================
  Files        2321     2321           
  Lines      123775   123796   +21     
  Branches    16804    16809    +5     
=======================================
+ Hits        46312    46332   +20     
+ Misses      73747    73745    -2     
- Partials     3716     3719    +3     

☔ View full report in Codecov by Sentry.
📢 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.

[adalpari]

@claude

[claude]

Claude finished @adalpari's task in 1m 46s —— View job

---

Code Review

• Read changed files
• Analyze code quality, security, performance, and testing
• Post review feedback

---

Overview

The fix addresses a real bug — the in-memory ApiRootUrlCache being cleared between discovery and the auth callback (process kill). The two-part approach (cache-key consistency fix + recovery path) is solid. A few things worth discussing below.

---

ApiRootUrlCache.kt

Bug fix is correct. The old catch (IllegalArgumentException) block was returning urlOrDomain.trim() raw, without stripping the scheme/www/port. This meant a malformed URL like http://bad site.com stored via put (which trims to http://bad site.com) would never be found by a subsequent get (which also trims, but now consistently calls stripToDomain). The refactor to extract stripToDomain and use it in both the no-host branch and the catch branch is a clean fix.

Missing test coverage for the fixed branch. The catch (IllegalArgumentException) code path — the actual bug being fixed — has no test. Consider adding:

@Test
fun `put and get are consistent for malformed URL`() {
    whenever(uriUtilsWrapper.parse(any())).thenThrow(IllegalArgumentException("malformed"))

    val malformedUrl = "http://bad site.com/path"
    apiRootUrlCache.put(malformedUrl, "https://bad site.com/wp-json/")

    assertNotNull(apiRootUrlCache.get(malformedUrl))
}

Without this, the exact regression being fixed has no automated guard.

---

ApplicationPasswordLoginHelper.kt

Recovery logic (recoverApiRootUrl) is structurally sound. The pattern — attempt fallback discovery, log result, return null on failure — is consistent with the rest of the file.

Missing test: recovery success path. Codecov is flagging 11 uncovered lines in this file, and the core recovery scenario (null apiRootUrl + successful fallback discovery → Success result) has no test. This is the happy path of the new feature. For example:

@Test
fun `storeApplicationPasswordCredentialsFrom recovers missing apiRootUrl via fallback discovery`() = runTest {
    val autoDiscoveryAttemptSuccess = AutoDiscoveryAttemptSuccess(...)
    val apiDiscoveryResult = ApiDiscoveryResult.Success(autoDiscoveryAttemptSuccess)
    whenever(wpLoginClient.apiDiscovery(any())).thenReturn(apiDiscoveryResult)
    whenever(discoverSuccessWrapper.getApiRootUrl(any())).thenReturn(TEST_API_ROOT_URL)
    whenever(siteStore.sites).thenReturn(listOf(SiteModel().apply { url = TEST_URL }))

    val loginWithoutApiRoot = UriLogin(TEST_URL, TEST_USER, TEST_PASSWORD, null)
    val result = applicationPasswordLoginHelper.storeApplicationPasswordCredentialsFrom(loginWithoutApiRoot)

    assertIs<StoreCredentialsResult.Success>(result)
    verify(wpLoginClient).apiDiscovery(any()) // fallback ran exactly once
}

Missing test: no double-discovery when cache is populated. The PR description mentions "discovery is not run a second time" as part of the happy-path regression test. There's no unit test verifying wpLoginClient.apiDiscovery is NOT called when apiRootUrl is already present:

verify(wpLoginClient, times(0)).apiDiscovery(any())

Existing test behavior silently changed. storeApplicationPasswordCredentialsFrom with empty api root url returns BadData (line 150) now exercises the recovery path because siteUrl = TEST_URL is non-null. Recovery is attempted (unconfigured mock returns null) and gracefully falls through to BadData. The test still passes and the outcome is still correct, but the test description no longer accurately reflects what's happening. It would be cleaner to either:

• rename it to "...when apiRootUrl is null and fallback discovery also fails, returns BadData", or
• configure the mock to explicitly fail (whenever(wpLoginClient.apiDiscovery(any())).thenReturn(ApiDiscoveryResult.FailureFindApiRoot))

No observability for the recovery. The recovery logs a debug message, but there's no analytics event. Given this is a silent failure mode that blocked login, emitting a tracking event (e.g., a new Stat.APPLICATION_PASSWORD_API_ROOT_URL_RECOVERED) would let you monitor in production how frequently this path is actually hit, and whether the fallback is working. This is a suggestion, not a blocker.

Minor: UrlUtils.normalizeUrl(siteUrl).orEmpty() at line 164 — the orEmpty() is correct and necessary, but it means a URL that normalizeUrl rejects won't be cached. That's acceptable because apiRootUrl is still returned to the caller; only the cache put is skipped. No issue, just worth a comment.

---

Summary

The core logic is correct and the approach is appropriate for the problem. The main gap is test coverage — specifically the recovery success path and a test for the IllegalArgumentException branch in ApiRootUrlCache. The observability suggestion is optional but recommended for a production login fix.

[nbradbury]

Looks good!