Skip to content

Commit c3f35fb

Browse files
adalpariclaude
andauthored
Fix false "XML-RPC Disabled" card on rate-limited self-hosted sites (#23120)
App-password self-hosted sites could wrongly show the "XML-RPC Disabled" My Site card even when XML-RPC is enabled, because a transient HTTP 429 (rate-limit) was treated as "XML-RPC unavailable" in two places: 1. Discovery succeeds at login but the authenticated XML-RPC fetch 429s: fetchSitesXmlRpcFromApplicationPassword fell back to WPAPI and dropped the already-verified endpoint. Now it persists the verified endpoint on the WPAPI-stored site via the new URL-keyed updateXmlRpcUrlForWPAPISite. 2. Discovery itself 429s: DiscoveryXMLRPCClient.listMethods swallowed the 429 as an empty response, so discovery reported MISSING_XMLRPC_METHOD / NO_SITE_ERROR, indistinguishable from a genuinely disabled site. (Note: Volley's ClientError for 429 extends ServerError, so the 429 must be handled inside the ServerError branch.) A new DiscoveryError.RATE_LIMITED is now thrown and propagated through verifyXMLRPCUrl / checkXMLRPCEndpointValidity. The card slice no longer shows the warning optimistically: attemptXmlRpcRediscovery surfaces the card only on a definitive-negative discovery result (NO_SITE_ERROR, MISSING_XMLRPC_METHOD, XMLRPC_BLOCKED, XMLRPC_FORBIDDEN); transient/unknown failures keep it hidden and re-check on the next refresh. It also bails on the first 429 instead of firing a burst of discovery requests that re-trigger the rate limit. Genuine detection is preserved: a truly disabled site (clean 404/405/403, no 429) still surfaces the warning. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 8abbdf1 commit c3f35fb

8 files changed

Lines changed: 201 additions & 28 deletions

File tree

WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt

Lines changed: 40 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,10 @@ class ApplicationPasswordViewModelSlice @Inject constructor(
147147
// Only true self-hosted sites need the XML-RPC fallback path — Atomic and Jetpack-WPCom-REST
148148
// sites talk REST end-to-end and don't need XML-RPC.
149149
if (!site.isUsingWpComRestApi && site.xmlRpcUrl.isNullOrEmpty()) {
150-
buildXmlRpcDisabledCard(site)
150+
// A missing xmlRpcUrl doesn't mean XML-RPC is disabled — the login fetch may have fallen back to
151+
// WPAPI on a transient error (e.g. a 429 rate-limit). Keep the card hidden and let rediscovery
152+
// decide; it only surfaces the warning on a definitive negative.
153+
uiModelMutable.postValue(null)
151154
attemptXmlRpcRediscovery(site)
152155
} else {
153156
uiModelMutable.postValue(null)
@@ -235,39 +238,57 @@ class ApplicationPasswordViewModelSlice @Inject constructor(
235238
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
236239
internal fun attemptXmlRpcRediscovery(site: SiteModel) {
237240
scope.launch {
238-
try {
239-
val xmlRpcEndpoint = withContext(ioDispatcher) {
241+
val xmlRpcEndpoint = try {
242+
withContext(ioDispatcher) {
240243
selfHostedEndpointFinder
241244
.verifyOrDiscoverXMLRPCEndpoint(site.url)
242245
}
243-
244-
// Verify with an authenticated call
245-
val result = withContext(ioDispatcher) {
246-
siteXMLRPCClient.fetchSites(
247-
xmlRpcEndpoint,
248-
site.apiRestUsernamePlain,
249-
site.apiRestPasswordPlain
246+
} catch (e: SelfHostedEndpointFinder.DiscoveryException) {
247+
// Only surface the "XML-RPC Disabled" warning on a definitive negative. A transient failure
248+
// (e.g. a 429 rate-limit) leaves the state unknown, so keep the card hidden and re-check on the
249+
// next refresh rather than wrongly claiming XML-RPC is off.
250+
if (e.discoveryError.indicatesXmlRpcUnavailable()) {
251+
buildXmlRpcDisabledCard(site)
252+
} else {
253+
uiModelMutable.postValue(null)
254+
appLogWrapper.d(
255+
AppLog.T.MAIN,
256+
"A_P: XML-RPC rediscovery inconclusive for ${site.url} " +
257+
"(${e.discoveryError}) - hiding card"
250258
)
251259
}
252-
if (result.isError) {
253-
return@launch
254-
}
260+
return@launch
261+
}
255262

263+
// Discovery verified a working xmlrpc.php, so XML-RPC is enabled. Confirm the credentials with an
264+
// authenticated call before persisting the endpoint, but keep the card hidden either way.
265+
val result = withContext(ioDispatcher) {
266+
siteXMLRPCClient.fetchSites(
267+
xmlRpcEndpoint,
268+
site.apiRestUsernamePlain,
269+
site.apiRestPasswordPlain
270+
)
271+
}
272+
if (!result.isError) {
256273
site.xmlRpcUrl = xmlRpcEndpoint
257274
// Persist only the rediscovered column — mirrors healApiRestUrlIfMissing. A full-row
258275
// updateSite would rewrite ~80 columns from this in-memory model for a one-field change
259276
// (risking clobbering other out-of-band values), so write just xmlRpcUrl.
260277
siteStore.persistXmlRpcUrl(site.id, xmlRpcEndpoint)
261-
buildCard(site)
262-
} catch (
263-
@Suppress("SwallowedException")
264-
e: SelfHostedEndpointFinder.DiscoveryException
265-
) {
266-
// XML-RPC rediscovery failed; card remains visible
267278
}
279+
uiModelMutable.postValue(null)
268280
}
269281
}
270282

283+
// A missing/unusable XML-RPC endpoint is only a genuine "disabled" signal when discovery reaches a
284+
// definitive conclusion. Transient errors (RATE_LIMITED, GENERIC_ERROR) and unrelated conditions
285+
// (auth/SSL/invalid URL) must not surface the warning, to avoid false positives on throttled sites.
286+
private fun SelfHostedEndpointFinder.DiscoveryError.indicatesXmlRpcUnavailable(): Boolean =
287+
this == SelfHostedEndpointFinder.DiscoveryError.NO_SITE_ERROR ||
288+
this == SelfHostedEndpointFinder.DiscoveryError.MISSING_XMLRPC_METHOD ||
289+
this == SelfHostedEndpointFinder.DiscoveryError.XMLRPC_BLOCKED ||
290+
this == SelfHostedEndpointFinder.DiscoveryError.XMLRPC_FORBIDDEN
291+
271292
private fun onClick(site: SiteModel, alternativeUrl: String) {
272293
_onNavigation.postValue(
273294
Event(

WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSliceTest.kt

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import org.mockito.kotlin.verify
1919
import org.mockito.kotlin.whenever
2020
import org.wordpress.android.BaseUnitTest
2121
import org.mockito.kotlin.mock
22+
import org.wordpress.android.R
2223
import org.wordpress.android.fluxc.model.SiteModel
2324
import org.wordpress.android.fluxc.model.SitesModel
2425
import org.wordpress.android.fluxc.network.BaseRequest.BaseNetworkError
@@ -424,14 +425,16 @@ class ApplicationPasswordViewModelSliceTest : BaseUnitTest() {
424425
}
425426

426427
@Test
427-
fun `given xmlRpc rediscovery fails, then do not persist`() =
428+
fun `given xmlRpc rediscovery fails with a definitive negative, then show the disabled card`() =
428429
runTest {
429430
siteTest.xmlRpcUrl = null
430431
whenever(
431432
selfHostedEndpointFinder
432433
.verifyOrDiscoverXMLRPCEndpoint(TEST_URL)
433434
).thenThrow(
434-
mock<SelfHostedEndpointFinder.DiscoveryException>()
435+
SelfHostedEndpointFinder.DiscoveryException(
436+
SelfHostedEndpointFinder.DiscoveryError.NO_SITE_ERROR, TEST_URL
437+
)
435438
)
436439

437440
applicationPasswordViewModelSlice
@@ -441,5 +444,33 @@ class ApplicationPasswordViewModelSliceTest : BaseUnitTest() {
441444
.verifyOrDiscoverXMLRPCEndpoint(TEST_URL)
442445
verify(siteStore, never()).persistXmlRpcUrl(any(), any())
443446
assert(siteTest.xmlRpcUrl.isNullOrEmpty())
447+
val card = applicationPasswordCard
448+
assertNotNull(card)
449+
assert(card is MySiteCardAndItem.Item.SingleActionCard)
450+
assert(
451+
(card as MySiteCardAndItem.Item.SingleActionCard).textResource ==
452+
R.string.xmlrpc_disabled_card_text
453+
)
454+
}
455+
456+
@Test
457+
fun `given xmlRpc rediscovery fails transiently with rate limiting, then keep the card hidden`() =
458+
runTest {
459+
siteTest.xmlRpcUrl = null
460+
whenever(
461+
selfHostedEndpointFinder
462+
.verifyOrDiscoverXMLRPCEndpoint(TEST_URL)
463+
).thenThrow(
464+
SelfHostedEndpointFinder.DiscoveryException(
465+
SelfHostedEndpointFinder.DiscoveryError.RATE_LIMITED, TEST_URL
466+
)
467+
)
468+
469+
applicationPasswordViewModelSlice
470+
.attemptXmlRpcRediscovery(siteTest)
471+
472+
verify(siteStore, never()).persistXmlRpcUrl(any(), any())
473+
assert(siteTest.xmlRpcUrl.isNullOrEmpty())
474+
assertNull(applicationPasswordCard)
444475
}
445476
}

libs/fluxc/src/main/java/org/wordpress/android/fluxc/network/discovery/DiscoveryXMLRPCClient.java

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,11 +116,19 @@ public Object[] listMethods(@NonNull String url) throws DiscoveryException {
116116
// In the event of an SSL handshake error we should stop attempting discovery
117117
throw new DiscoveryException(DiscoveryError.ERRONEOUS_SSL_CERTIFICATE, url);
118118
} else if (e.getCause() instanceof ServerError) {
119+
// Note: Volley's ClientError (used for 4xx) extends ServerError, so 4xx responses land here too.
119120
NetworkResponse networkResponse = ((ServerError) e.getCause()).networkResponse;
120121
if (networkResponse == null) {
121122
return null;
122123
}
123124

125+
if (networkResponse.statusCode == 429) {
126+
// A 429 (rate-limited) is transient. Falling through to the null return below would be read
127+
// as "no XML-RPC methods found" and misclassified as MISSING_XMLRPC_METHOD/NO_SITE_ERROR,
128+
// so surface a distinct transient error instead.
129+
throw new DiscoveryException(DiscoveryError.RATE_LIMITED, url);
130+
}
131+
124132
if (networkResponse.statusCode == 405 && !new String(networkResponse.data).contains(
125133
"XML-RPC server accepts POST requests only.")) {
126134
// XML-RPC is blocked by the server (POST request returns a 405 "Method Not Allowed" error)

libs/fluxc/src/main/java/org/wordpress/android/fluxc/network/discovery/SelfHostedEndpointFinder.java

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,9 @@ public enum DiscoveryError implements OnChangedError {
4545
WORDPRESS_COM_SITE,
4646
XMLRPC_BLOCKED,
4747
XMLRPC_FORBIDDEN,
48+
// Transient: the server rate-limited us (HTTP 429). Kept distinct from the "missing/blocked" errors so
49+
// callers don't misread a temporary throttle as XML-RPC being disabled.
50+
RATE_LIMITED,
4851
GENERIC_ERROR
4952
}
5053

@@ -54,7 +57,7 @@ public static class DiscoveryException extends Exception {
5457
@NonNull public final DiscoveryError discoveryError;
5558
@Nullable public final String failedUrl;
5659

57-
DiscoveryException(@NonNull DiscoveryError failureType, @Nullable String failedUrl) {
60+
public DiscoveryException(@NonNull DiscoveryError failureType, @Nullable String failedUrl) {
5861
this.discoveryError = failureType;
5962
this.failedUrl = failedUrl;
6063
}
@@ -183,7 +186,8 @@ private String verifyXMLRPCUrl(@NonNull final String siteUrl) throws DiscoveryEx
183186
if (e.discoveryError == DiscoveryError.ERRONEOUS_SSL_CERTIFICATE
184187
|| e.discoveryError == DiscoveryError.HTTP_AUTH_REQUIRED
185188
|| e.discoveryError == DiscoveryError.MISSING_XMLRPC_METHOD
186-
|| e.discoveryError == DiscoveryError.XMLRPC_BLOCKED) {
189+
|| e.discoveryError == DiscoveryError.XMLRPC_BLOCKED
190+
|| e.discoveryError == DiscoveryError.RATE_LIMITED) {
187191
throw e;
188192
}
189193
// Otherwise. swallow the error since we are just verifying various URLs
@@ -353,6 +357,10 @@ private boolean checkXMLRPCEndpointValidity(@NonNull String url) throws Discover
353357
throw new DiscoveryException(DiscoveryError.XMLRPC_BLOCKED, url);
354358
} else if (e.discoveryError.equals(DiscoveryError.MISSING_XMLRPC_METHOD)) {
355359
throw new DiscoveryException(DiscoveryError.MISSING_XMLRPC_METHOD, url);
360+
} else if (e.discoveryError.equals(DiscoveryError.RATE_LIMITED)) {
361+
// Surface the transient throttle instead of swallowing it and returning false (which callers
362+
// would treat as "endpoint found but methods missing").
363+
throw new DiscoveryException(DiscoveryError.RATE_LIMITED, url);
356364
}
357365
} catch (IllegalArgumentException e) {
358366
// The XML-RPC client returns this error in case of redirect to an invalid URL.

libs/fluxc/src/main/java/org/wordpress/android/fluxc/persistence/SiteSqlUtils.kt

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -404,6 +404,17 @@ class SiteSqlUtils
404404
return updateApplicationPasswordCredentials(localId, usernamePlain, passwordPlain)
405405
}
406406

407+
/**
408+
* URL-keyed variant of [updateXmlRpcUrl] for ORIGIN_WPAPI sites fetched without a local id. Used when an
409+
* application-password site falls back to the WPAPI fetch even though XML-RPC discovery already verified a
410+
* working xmlrpc.php endpoint, so the verified endpoint is recorded rather than lost. See
411+
* [updateWpApiRestUrlForWPAPISite].
412+
*/
413+
fun updateXmlRpcUrlForWPAPISite(siteUrl: String, xmlRpcUrl: String): Int {
414+
val localId = wpApiSiteLocalIdByUrl(siteUrl) ?: return 0
415+
return updateXmlRpcUrl(localId, xmlRpcUrl)
416+
}
417+
407418
private fun wpApiSiteLocalIdByUrl(siteUrl: String): Int? =
408419
WellSql.select(SiteModel::class.java)
409420
.where().beginGroup()

libs/fluxc/src/main/java/org/wordpress/android/fluxc/store/SiteStore.kt

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1559,13 +1559,17 @@ open class SiteStore @Inject constructor(
15591559
" (${sites.error?.message})," +
15601560
" falling back to WPAPI"
15611561
)
1562-
// Use apiRootUrl as the base URL for
1563-
// WPAPI discovery since payload.url
1564-
// is the xmlrpc.php endpoint.
1562+
// Use apiRootUrl as the base URL for WPAPI discovery since payload.url is the xmlrpc.php
1563+
// endpoint. This action is only dispatched after verifyOrDiscoverXMLRPCEndpoint already
1564+
// confirmed payload.url is a working xmlrpc.php endpoint, so this fetch failure is transient
1565+
// (e.g. a 429 rate-limit) or auth-only — XML-RPC itself is enabled. Pass the verified
1566+
// endpoint through so it's recorded on the WPAPI-stored site and the "XML-RPC Disabled" card
1567+
// isn't shown for a site that actually supports XML-RPC.
15651568
fetchSiteWPAPIFromApplicationPassword(
15661569
payload.copy(
15671570
url = payload.apiRootUrl
1568-
)
1571+
),
1572+
verifiedXmlRpcUrl = payload.url,
15691573
)
15701574
} else {
15711575
updateSites(sites)
@@ -1596,7 +1600,8 @@ open class SiteStore @Inject constructor(
15961600

15971601
@Suppress("TooGenericExceptionCaught")
15981602
internal suspend fun fetchSiteWPAPIFromApplicationPassword(
1599-
payload: RefreshSitesXMLRPCApplicationPasswordCredentialsPayload
1603+
payload: RefreshSitesXMLRPCApplicationPasswordCredentialsPayload,
1604+
verifiedXmlRpcUrl: String? = null,
16001605
): OnSiteChanged {
16011606
return coroutineEngine.withDefaultContext(
16021607
T.API,
@@ -1628,6 +1633,13 @@ open class SiteStore @Inject constructor(
16281633
},
16291634
{ siteSqlUtils.updateWpApiRestUrlForWPAPISite(siteModel.url, it) }
16301635
)
1636+
// When XML-RPC discovery already verified an endpoint but the XML-RPC fetch fell back to
1637+
// WPAPI, record the verified endpoint (keyed by siteModel.url, since the fresh site has no
1638+
// local id) so XML-RPC-dependent features stay available and the "XML-RPC Disabled" card
1639+
// isn't shown.
1640+
if (!verifiedXmlRpcUrl.isNullOrEmpty()) {
1641+
siteSqlUtils.updateXmlRpcUrlForWPAPISite(siteModel.url, verifiedXmlRpcUrl)
1642+
}
16311643
}
16321644
result
16331645
} catch (e: Exception) {

libs/fluxc/src/test/java/org/wordpress/android/fluxc/persistence/SiteSqlUtilsTest.kt

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,41 @@ class SiteSqlUtilsTest {
135135
.isEqualTo("https://selfhosted.test/wp-json/")
136136
}
137137

138+
@Test
139+
fun `updateXmlRpcUrlForWPAPISite updates the matching WPAPI site by url`() {
140+
WellSql.insert(SiteModel().apply {
141+
url = "https://selfhosted.test"
142+
origin = SiteModel.ORIGIN_WPAPI
143+
xmlRpcUrl = null
144+
}).execute()
145+
146+
val rows = siteSqlUtils.updateXmlRpcUrlForWPAPISite(
147+
siteUrl = "https://selfhosted.test",
148+
xmlRpcUrl = "https://selfhosted.test/xmlrpc.php"
149+
)
150+
151+
assertThat(rows).isEqualTo(1)
152+
assertThat(siteSqlUtils.getSites().single().xmlRpcUrl)
153+
.isEqualTo("https://selfhosted.test/xmlrpc.php")
154+
}
155+
156+
@Test
157+
fun `updateXmlRpcUrlForWPAPISite leaves a non-WPAPI site with the same url untouched`() {
158+
WellSql.insert(SiteModel().apply {
159+
url = "https://shared.test"
160+
origin = SiteModel.ORIGIN_WPCOM_REST
161+
xmlRpcUrl = null
162+
}).execute()
163+
164+
val rows = siteSqlUtils.updateXmlRpcUrlForWPAPISite(
165+
siteUrl = "https://shared.test",
166+
xmlRpcUrl = "https://shared.test/xmlrpc.php"
167+
)
168+
169+
assertThat(rows).isEqualTo(0)
170+
assertThat(siteSqlUtils.getSites().single().xmlRpcUrl).isNull()
171+
}
172+
138173
@Test
139174
fun `updateWpApiRestUrlForWPAPISite leaves a non-WPAPI site with the same url untouched`() {
140175
WellSql.insert(SiteModel().apply {

libs/fluxc/src/test/java/org/wordpress/android/fluxc/store/SiteStoreTest.kt

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -592,6 +592,53 @@ class SiteStoreTest {
592592
)
593593
}
594594

595+
@Test
596+
fun `fetchSitesXmlRpc AP fallback persists verified xmlrpc endpoint on WPAPI site`() =
597+
test {
598+
org.mockito.Mockito.mockStatic(
599+
org.wordpress.android.util.AppLog::class.java
600+
).use {
601+
// Given: XML-RPC discovery already verified this endpoint (that's the only reason this action
602+
// is dispatched), but the authenticated XML-RPC fetch fails transiently (e.g. a 429 rate-limit).
603+
val xmlRpcEndpoint = "https://example.com/xmlrpc.php"
604+
val payload =
605+
SiteStore.RefreshSitesXMLRPCApplicationPasswordCredentialsPayload(
606+
username = "appUser",
607+
password = "appPass",
608+
url = xmlRpcEndpoint,
609+
apiRootUrl = "https://example.com/wp-json/",
610+
)
611+
val erroredSites = SitesModel().apply { error = BaseNetworkError(PARSE_ERROR) }
612+
whenever(
613+
siteXMLRPCClient.fetchSitesFromApplicationPassword(
614+
payload.url,
615+
payload.apiRootUrl,
616+
payload.username,
617+
payload.password
618+
)
619+
).thenReturn(erroredSites)
620+
val fetchedSite = SiteModel().apply {
621+
name = "Test Site"
622+
origin = SiteModel.ORIGIN_WPAPI
623+
url = "https://example.com"
624+
}
625+
whenever(siteWPAPIClient.fetchWPAPISite(any<SiteStore.FetchWPAPISitePayload>()))
626+
.thenReturn(fetchedSite)
627+
whenever(siteSqlUtils.insertOrUpdateSite(fetchedSite)).thenReturn(1)
628+
629+
// When
630+
val result = siteStore.fetchSitesXmlRpcFromApplicationPassword(payload)
631+
632+
// Then: the WPAPI fallback stored the site, and the verified xmlrpc endpoint is recorded so the
633+
// "XML-RPC Disabled" card isn't shown for a site that actually supports XML-RPC.
634+
assertThat(result.isError).isFalse()
635+
verify(siteSqlUtils).updateXmlRpcUrlForWPAPISite(
636+
"https://example.com",
637+
xmlRpcEndpoint
638+
)
639+
}
640+
}
641+
595642
@Test
596643
fun `updateApplicationPassword persists credentials and wpApiRestUrl via targeted writers`() {
597644
val existing = SiteModel().apply {

0 commit comments

Comments
 (0)