Make wpApiRestUrl, xmlRpcUrl, and app-password creds single-writer - #22947
Merged
Conversation
Collaborator
Generated by 🚫 Danger |
Contributor
|
|
Contributor
|
|
Contributor
🤖 Build Failure AnalysisThis build has failures. Claude has analyzed them - check the build annotations for details. |
jkmassel
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
jkmassel
force-pushed
the
jkmassel/sitesqlutils-partial-updates
branch
from
June 9, 2026 22:18
6885e4c to
67118a8
Compare
jkmassel
force-pushed
the
jkmassel/sitesqlutils-partial-updates
branch
from
June 10, 2026 21:56
67118a8 to
88e8833
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #22947 +/- ##
==========================================
+ Coverage 37.22% 37.28% +0.06%
==========================================
Files 2330 2330
Lines 125667 125701 +34
Branches 17115 17122 +7
==========================================
+ Hits 46777 46872 +95
+ Misses 75104 75032 -72
- Partials 3786 3797 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jkmassel
marked this pull request as ready for review
June 10, 2026 23:08
On Atomic / Jetpack-WPCom-REST sites a recovered wpApiRestUrl was wiped to NULL on every app foreground: any full-row insertOrUpdateSite UPDATE built from a partial in-memory SiteModel (FETCH_SITE/FETCH_SITES, RN, cookie-nonce) wrote every column, clobbering the healed value. Exclude WP_API_REST_URL from the generic UpdateAllExceptId mapper so updateWpApiRestUrl is the sole writer on an existing row, and route every legitimate writer through the targeted helpers. Fixes #22905.
The same full-row insertOrUpdateSite UPDATE that clobbered wpApiRestUrl also zeroed the application-password credential columns: a credential-less inbound SiteModel (e.g. a /me/sites sync) overwrote the encrypted username/password and their IVs with empty values. Exclude API_REST_USERNAME, API_REST_PASSWORD and their IV columns from the UpdateAllExceptId mapper (as a set — the IVs are required to decrypt), and route the credential writers through targeted single-column writers. Extends the #22905 fix to the credential columns.
xmlRpcUrl is set either out of band (XML-RPC rediscovery) or by the WP.com REST sync (meta.links.xmlrpc). A full-row insertOrUpdateSite UPDATE built from a partial SiteModel that doesn't carry it — e.g. the WPAPI fetch, which builds a model with a null xmlRpcUrl — wrote that null over a good value. Preserve XMLRPC_URL on absence in the generic update path: keep the stored value when the inbound model has none, persist it when the model carries one (the WP.com sync, including a changed endpoint after a domain migration). Add a targeted updateXmlRpcUrl writer for the single-column rediscovery heal; the recovery flow that calls it lands separately. Extends the #22905 fix to XMLRPC_URL.
attemptXmlRpcRediscovery dispatched newUpdateSiteAction to change one field, which (with XMLRPC_URL excluded from the full-row mapper) dropped the value and rewrote ~80 unchanged columns. Add SiteStore.persistXmlRpcUrl and route rediscovery through it — one targeted write, mirroring the wpApiRestUrl heal. Drops the now-unused dispatcher from the slice.
createOrUpdateSites (the XML-RPC app-password login store path) does a full-row write that excludes the credential columns, with no targeted writer to follow up — so re-logging into an already-stored self-hosted site silently dropped the credentials. Add SiteSqlUtils.insertOrUpdateSiteReturningId, which returns the local id of the row it wrote; insertOrUpdateSite becomes a thin rows-affected wrapper over it (behavior unchanged). createOrUpdateSites uses the returned id to persist credentials + wpApiRestUrl via the targeted writers on the exact row.
- The cookie-nonce and React Native 404 handlers reset wpApiRestUrl in memory to force rediscovery on retry, then persisted via insertOrUpdateSite/persistSiteSafely — now a no-op for the excluded column. Drop the dead DB write (keep the in-memory reset), which also orphaned ReactNativeStore.persistSiteSafely and its injected persist function; remove those. - updateSite / createOrUpdateSites copied credentials + wpApiRestUrl from the DB onto the inbound model before the write; the mapper exclusion already preserves those columns, so drop the moot copy (editor-prefs copy stays). - Rework ReactNativeStoreWPAPITest to mock SiteSqlUtils and assert on updateWpApiRestUrl (the discover path persists there now).
After the single-writer migration the full-row write skips the credential and WP_API_REST_URL columns, so the in-memory mutations and the self-insertOrUpdateSite in updateApplicationPassword, removeApplicationPassword, and clearApplicationPasswordColumns persisted nothing — the targeted writers do all the work. Drop the dead code (keeping the new-site insert in updateApplicationPassword) and remove the now-unused insertOrUpdateSite stubs from the tests.
decryptAPIRestCredentials bailed only when the ciphertext columns were empty; a row with ciphertext but a blank IV would reach decrypt(ciphertext, "") and throw on read. Also short-circuit when either IV is empty, treating the malformed row as having no credentials.
updateApplicationPasswordCredentials and its URL-keyed variant were verified only against a mocked SiteSqlUtils, so their ContentValues mapping never ran. A .first/.second swap, a wrong column, or a dropped IV would ship uncaught — and the new blank-IV decrypt guard would mask it as 'no credentials' rather than surfacing it. EncryptionUtils can't run under Robolectric (AndroidKeyStore), so add a stubbed-EncryptionUtils SiteSqlUtils and assert the column mapping via the raw storedSite() read, plus a decrypt round-trip and the ORIGIN_WPAPI URL-scoping (writes the matching row, leaves a same-url non-WPAPI row untouched).
UPDATE_APPLICATION_PASSWORD, the WPAPI app-password fetch, and app-password XML-RPC login in createOrUpdateSites each duplicated the read-plain / null-guard / write-credentials-then-wpApiRestUrl logic, with subtle divergences (rowsAffected reassigned vs not; the URL write nested under the credentials check in one path, independent in another). Extract persistAppPasswordColumns(site, persistCredentials, persistWpApiRestUrl); each caller passes its targeted writer pair (local id or URL-keyed). wpApiRestUrl stays gated behind credentials on purpose: a credential-less /me/sites model can be a WP.com simple site whose getWpApiRestUrl() synthesizes a public-api proxy URL, and gating keeps that synthetic value out of the DB. removeApplicationPassword now sums both clear-writes instead of returning only the wpApiRestUrl count.
updateApplicationPasswordCredentials and clearApplicationPasswordCredentials hand-wrote the same four-column ContentValues, so the column set lived in two places — a future column change has to touch both or clear leaves a stale column. Route both through one private writeApplicationPasswordCredentialColumns. ApplicationPasswordViewModelSliceTest: the xmlRpc-rediscovery-success assertion was pre-satisfied by @before seeding siteTest.xmlRpcUrl to the same value; reset it to null first so the assertion proves rediscovery assigned it.
jkmassel
force-pushed
the
jkmassel/sitesqlutils-partial-updates
branch
from
June 13, 2026 15:26
88e8833 to
fb89734
Compare
jkmassel
enabled auto-merge (squash)
June 13, 2026 15:26
jkmassel
added a commit
that referenced
this pull request
Jun 13, 2026
…riter #22947 made the app-password columns single-writer — excluded from the generic full-row update, written only by updateApplicationPasswordCredentials — so a fresh getSiteByLocalId after a mint reliably carries the credentials and a concurrent site write can't clobber them (#22905). The pipeline no longer needs to thread them as a value: remove ProvisionedCredentials, AuthResult.credentials, and the detectCapabilities overlay. ensureAuth returns a bare SiteAuthState; detectCapabilities and recoverXmlRpcIfNeeded read the credentials off the fresh read.
11 tasks
nbradbury
added a commit
that referenced
this pull request
Sep 2, 2026
…ce (#22944) * Unify editor capability detection behind a single per-site detector Introduces EditorCapabilityDetector — one app-scoped owner of editor REST capability state, exposed as a per-site StateFlow — so the connectivity banner and editor preloader share a single, deduplicated probe instead of each re-deriving the state and racing the same async preconditions. Folds in the authenticated direct-host probe fallback so private Atomic sites detect correctly on trunk. Part of #22942. * Add release note for editor capability detection rework * Remove CredentialsChangedNotifier event bus, superseded by the detector #22926's first-login hardening signalled credential establishment through a process-global CredentialsChangedNotifier that the banner collected against a re-read of getSelectedSite() — the staleness race called out in #22942. EditorCapabilityDetector.refresh(storedSite), called from the mint path on the exact mutated SiteModel, replaces that coordination, so drop the notifier and its wiring in ApplicationPasswordLoginHelper. * Fold provisioning + detection into one single-flight SiteProvisioningSource Promotes the EditorCapabilityDetector into SiteProvisioningSource: one per-site, single-flight pipeline that ensures application-password credentials, recovers the REST root, and detects editor capabilities — each stage awaited before the next. Because the capability probe is now structurally downstream of credential provisioning, it can never run before the mint, so the first-login race is gone by construction rather than mitigated. The application-password card, connectivity banner, and editor preloader all render slices of the one SiteReadiness state. The mint/validate mechanics move out of ApplicationPasswordViewModelSlice (now a renderer) into the source's ensureAuth stage; the per-site single-flight subsumes the card's old 409-safety guard. The duplicate wpApiRestUrl heal collapses into the source's recoverRestUrl stage. * Read fresh / write targeted per stage; run XML-RPC recovery in parallel Removes the threaded, mutated SiteModel from the provisioning pipeline: each stage now reads the site fresh by local id and writes back only the column it changed (persistApiRootUrl / a new persistXmlRpcUrl), so the parallel branches can't clobber one another and there's no stale-model write (#22905). With writes targeted, XML-RPC endpoint recovery moves out of the application-password card into the pipeline as a parallel branch off ensureAuth -- independent of the REST capability probe, since it needs only the credentials. The card becomes a pure renderer that reads the recovered xmlRpcUrl fresh. Adds SiteSqlUtils.updateXmlRpcUrl and a SiteXmlRpcUrlRecoverer mirroring SiteApiRestUrlRecoverer. * Satisfy detekt and checkstyle - @Suppress("ReturnCount") on ensureAuth — its five returns are each a distinct auth outcome; a single-return rewrite would read worse. - Drop a stray blank line before a brace left from removing a test region. * Rename recover stages to recoverRestUrlIfNeeded / recoverXmlRpcIfNeeded The IfNeeded suffix makes the short-circuit (skip when the field is already present / not applicable) clear at the call site. * Don't gate capability detection behind a mint for WP.com Simple sites WP.com Simple sites are proxy-served and OAuth-authed; the application-password mint returns NotSupported for them, so the pipeline was returning Unprovisionable and never reaching capability detection (which works fine through the proxy) -- a regression vs. the old ungated probe. ensureAuth now short-circuits them to a new SiteAuthState.NotApplicable (treated like Provisioned), and recoverRestUrlIfNeeded skips them too. * Probe the direct host for Jetpack capability detection, not the proxy The route-support probe only sent Atomic sites to the direct host; Jetpack WPCom-REST sites fell through to the WP.com proxy. Since the proxy and the direct host advertise different route lists (the #22879 premise), Jetpack sites got the wrong answer. Broaden the predicate to isUsingWpComRestApi && !isWPComSimpleSite (Atomic + Jetpack); the proxy is only for minting the application password. * Carry minted credentials to the capability probe as a value On a first provision of an Atomic site, the My Site screen showed a false "Unable to connect to your site" banner. `ensureAuth` minted an application password, but `detectCapabilities` re-read the `SiteModel` fresh and a concurrent whole-row site write (`insertOrUpdateSite` uses `UpdateAllExceptId`) had clobbered the just-encrypted credential columns before that read (#22905). With no credentials present, the authenticated direct-host probe was skipped, the unauthenticated discovery failed against the private host, and the probe reported the site unreachable. `ensureAuth` now returns the credentials it obtained in an internal `AuthResult`, and `detectCapabilities` overlays that immutable value onto its own coroutine-local `SiteModel` copy. The stages still read fresh and write only their own column — no `SiteModel` is shared or mutated across the parallel detect/`recoverXmlRpc` branches, so there's no race. Verified on-device against a private Atomic site: the authenticated direct-host probe runs and the banner is gone. * Contain unexpected throws in SiteXmlRpcUrlRecoverer discoverAndVerifyXmlRpcUrl caught only DiscoveryException, so a RuntimeException from the discovery/verify path would escape the async, cancel the whole provisioning coroutine, and reach appScope (no SupervisorJob/handler). Catch CancellationException + generic Exception like the sibling SiteApiRestUrlRecoverer, so any failure degrades to the XML-RPC-disabled card and retries next run. * Drop credential forwarding now that app-password columns are single-writer #22947 made the app-password columns single-writer — excluded from the generic full-row update, written only by updateApplicationPasswordCredentials — so a fresh getSiteByLocalId after a mint reliably carries the credentials and a concurrent site write can't clobber them (#22905). The pipeline no longer needs to thread them as a value: remove ProvisionedCredentials, AuthResult.credentials, and the detectCapabilities overlay. ensureAuth returns a bare SiteAuthState; detectCapabilities and recoverXmlRpcIfNeeded read the credentials off the fresh read. * Contain unexpected throws in the SiteProvisioningSource pipeline SiteApiRestUrlRecoverer and SiteXmlRpcUrlRecoverer already contain their own throws, but launchPipeline runs the whole pipeline inside appScope.launch with no guard -- so any other escaping throw (a SQLiteException from a stage's WellSql write, a failure in detectCapabilities) still reaches appScope. appScope is a plain Job (no SupervisorJob/handler), so it cancels the scope, takes down every other app-scoped coroutine, and leaves the readiness flow stuck on Probing. Wrap the launch body like the recoverers: rethrow CancellationException so invalidate/clear stay clean, contain anything else as Unreachable so the flow settles and the next run retries. Adds a regression test that fails without the guard. * Heal revoked app passwords in the pipeline; reauth only on failure On a persistent 401, WPMainActivity and MediaBrowserActivity navigated straight to interactive re-auth, even for WP.com-connected sites whose application password can be re-minted headlessly -- and once a site latched Ready nothing re-validated its credentials, so a server-side revocation went unnoticed until pull-to-refresh. Route invalid-auth through the provisioning pipeline. SiteProvisioningSource listens to the raw wordpress-rs 401 (WpAppNotifierHandler) and re-runs ensureAuth: a WP.com-connected site re-mints silently; one that can't be settles Unprovisionable. Only that terminal failure escalates to interactive re-auth -- via a new app-scoped ApplicationPasswordReauthNotifier the two activities now observe instead of the raw 401. So a successful re-mint no longer flashes the re-auth screen, and bearer-only WP.com Simple sites no longer mis-trigger an application-password prompt. Churn guards: skip sites already Unprovisionable and WP.com Simple sites; the no-op-while-active invalidate keeps the heal's own validate-401 from looping. A 401-triggered run is flagged so only it, never a routine onResume run, can escalate to re-auth. Also documents why invalidate no-ops during an in-flight run. * Use getSiteByLocalId in the preloader; drop dead isAwaitingApplicationPassword Two review cleanups, no behavior change: - GutenbergEditorPreloader re-read the provisioned site with siteStore.sites.firstOrNull { it.id == siteId } -- a full-table read plus a per-row credential decrypt to fetch one row by id. Use getSiteByLocalId(siteId), the single-row lookup the rest of SiteProvisioningSource already uses. - EditorSettingsRepository.isAwaitingApplicationPassword is dead: its only caller (the connectivity banner's pending-auth suppression) was removed when detection moved behind the pipeline. Delete it. (SiteStore.persistXmlRpcUrl is also orphaned by this stack but lives in fluxc code this PR doesn't touch -- left for a fluxc follow-up.) * Harden SiteProvisioningSource and key the 401 heal to the exact site - launchPipeline now contains the post-runPipeline tail. `maybeRequestReauth` reads the DB (`getSiteByLocalId`), and an escaping throw there would cancel the non-supervisor `appScope` and wedge provisioning for every site — wrap it so only `CancellationException` propagates. - `detectCapabilities` returns a private `PipelineResult(readiness, latch)`; the per-site dedup gate latches only on a live probe, so a `Ready` served from stale cache re-probes on the next run instead of sticking for the process lifetime. - A 401 that arrives while a run is in flight is deferred (`healForInvalidAuth`) instead of letting `invalidate` no-op and an unrelated run consume the re-auth flag — which could drop a revoked credential's interactive re-auth escalation. - `WpAppNotifierHandler.NotifierListener` hands listeners the `SiteModel`, not just the URL — URL is not unique (the constraint is `SITE_ID+URL`), so the heal now targets the exact row that 401'd. Drops the per-401 full site-table load+decrypt. * Give the Jetpack install step its own client so its 401 stays local The install step talked to the site through the shared, globally-wired `getWpApiClient`, so a 401 there fired the app-wide `WpAppNotifierHandler` — which `SiteProvisioningSource` now also observes, racing the in-flight connection (a concurrent validate / wipe / re-mint can clear the `apiRest*` columns mid-install, and `requireRestCredentials` then throws). `initWpApiClient` now builds a dedicated `WpApiClient` with a connection-local notifier (mirroring `initJetpackConnectionClient`), and `JetpackInstaller` threads the callback through. `JetpackRestConnectionViewModel` drops its `WpAppNotifierHandler.NotifierListener` and handles the install 401 via a local `onInstallAuthFailed` callback. `SiteProvisioningSource` is now the single app-wide invalid-auth authority. * Fix a 401 relaunch loop, a lost connectivity banner, and a swallowed re-auth Three defects in the new provisioning pipeline, found reviewing #22944. 1. The pipeline's own capability detection can raise the 401 that feeds back into healForInvalidAuth. getWpComApiClient installs the same invalid-auth notifier as the application-password client, so a rejected WP.com bearer token on an Atomic site 401s the theme fetch every run while the app password validates fine. Nothing broke the cycle: the run settles Ready or Unreachable, so neither the Unprovisionable guard nor the "already escalated" check fires, and the deferred heal relaunches forever. ensureAuth now reports whether it actually replaced the credentials, and PipelineResult carries whether auth was confirmed. A heal that confirmed a working password without replacing it cannot be the fix for a 401, so the site is recorded in healFutile and its 401s are dropped until an explicit retry. A heal that re-minted, or one that never reached the site, is not marked futile — so genuine revocation still heals. 2. The validator maps every ambiguous failure (DNS, timeout, refused, 5xx) to NetworkUnavailable so it never wipes credentials on a guess, and ensureAuth turned that into Provisioning. Since the banner renders only Unreachable and the card hides on Provisioning, a site that was simply down showed nothing at all — the exact case "Unable to connect to your site" exists for. ensureAuth now distinguishes device-offline from site-unreachable via SiteAuthState.SiteUnreachable, which maps to SiteReadiness.Unreachable. 3. A routine run never arms reauthOnFailure, so when a 401 arrived mid-run and that run settled Unprovisionable, the deferred handler returned "already escalated" when nothing had escalated. It now escalates instead of relaunching (a relaunch would only re-fail the mint), through a single escalateReauth funnel that is idempotent per Unprovisionable episode. Each fix has a test that fails when the fix is reverted; the loop test exhausts the test JVM without the guard. Also renames the transient-validation test, which was passing on a defaulted mock rather than a stated offline precondition. * Model the auth stage and heal evidence as their own types Follow-up cleanup on the provisioning pipeline; no behaviour change. SiteAuthState had five variants but only two — Provisioning and Unprovisionable — were ever wrapped in NeedsAuth and observed by a consumer. The other three were ensureAuth control-flow signals that happened to live in a public sealed interface, which forced the application-password card into two dead `when` arms. ensureAuth now returns a private AuthStage (Proceed / SiteUnreachable / Stop), so runPipeline is a three-way when with no else and SiteAuthState means exactly what the card renders. PipelineResult carried credentialsChanged and authConfirmed, two booleans encoding a three-state fact with one combination that was unrepresentable in practice but perfectly constructible. They collapse into a HealEvidence enum (Inconclusive / ConfirmedUnchanged / Replaced) derived entirely in ensureAuth, so the futile-heal test is a single equality and runPipeline no longer sets authConfirmed on the side. This also drops a dead credentialsChanged pass-through that was always false on the NeedsAuth branch. No test file needed changing: nothing referenced the three removed variants. The loop test only passes when healFutile is populated, so it exercises the new evidence wiring end to end — verified by emitting Replaced on the Valid path, which reproduces the 401 loop and hangs the suite. One unreachable semantic shift: a WP.com Simple site used to satisfy the futile condition and now reports Inconclusive, which is the honest encoding since no application password applies. Simple sites return early from onRequestedWithInvalidAuthentication, so they can never be a heal. * Extract the 401 heal predicate to satisfy detekt's ReturnCount The healFutile guard added a third early return to onRequestedWithInvalidAuthentication, one past detekt's limit of 2. The three skip conditions collapse cleanly into a conjunction, so extract them as canHeal() rather than suppressing the rule — the per-condition rationale reads better as KDoc on the predicate than as inline comments between guards. Also fixes a stale [maybeRequestReauth] KDoc link left by the rename to settleHealState. No behaviour change: canHeal reads the states map before the Simple-site check instead of after, which is a side-effect-free lookup. * Move the release note to the active section The note was filed under 26.9, which shipped on 1 July. Trunk's top section is still labelled 27.0 even though versionName is 27.2 and both 27.0 and 27.1 have released — that top section is the accumulating bucket everything lands in, so append there rather than adding a new 27.2 heading. Left under 26.9 the note would never have reached a release. * Tell the user their site is private instead of "not supported" Logging in to a private WordPress.com site reported "The provided site does not support Application Password authentication." The site supports them fine — the Privacy gate sits in front of WordPress and answers the anonymous discovery request with 403 private_site, so discovery never reaches the REST API. Two things were wrong. The reason was thrown away. The ViewModel signalled failure by sending an empty discoveryURL, and the fragment read that empty string as "unsupported" and overwrote the specific message it had just been given. Every discovery failure — DNS, timeout, TLS, 403, malformed URL — reported the same wrong cause. Fixed structurally rather than by patching the message: discoveryURL now only ever carries a URL to navigate to, and failure travels through errorMessage alone, so no future failure path can clobber it either. setError() went with its only caller. The reason wasn't named. FailureFetchAndParseApiRoot carries a WpError with the error code, message and status, so DiscoveryResult.Failed now carries a FailureReason and the UI can explain a private site rather than guess. Verified against a live private Atomic site rather than assumed: the library surfaces the code as WpErrorCode.CustomException("private_site") with status 403. Non-private failures now surface the library's own message, which is still better than the previous blanket claim. Leaves application_password_not_supported_error unreferenced. It is tagged a8c-src-lib="module:login", so removing it means every values-*/strings.xml plus whatever the string sync does — worth its own change. * Bound the 401 heal by budget, not by evidence type Six review findings, four of them in the heal containment added earlier on this branch. The heal could disable itself on success. validate() talks through a notifier-wired client, so a revoked credential's 401 fires during the very heal that is fixing it. That scheduled a redundant second run, which re-validated the fresh password, recorded "confirmed unchanged", and permanently suppressed healing for the site. The deferred heal now stands down when the run it waited on already re-minted. Keying suppression on evidence type also missed the opposite case: a host that mints happily but never accepts the result produced Replaced every time, which never suppressed anything and re-minted on every 401 — accumulating application passwords on the user's account. healFutile is replaced by a spend counter: confirming an unchanged password exhausts the budget outright, a re-mint costs one, and reaching Ready without needing a heal restores it. invalidate() cleared that suppression below its in-flight guard, so it never ran for the sites that needed it — MySiteViewModel.refresh builds the application-password card first, and buildCard starts a run synchronously on Main.immediate. The clears move above the guard; only the relaunch has to respect an in-flight mint. escalateReauth read the DB and started an Activity while holding the class monitor, which the main thread takes on every stateFor / invalidate. The deferred handler now decides under the lock and acts outside it. Removing the fragment's blanket error overwrite took with it the only producer of application_password_not_supported_error — a site that advertises no application-passwords endpoint then showed raw untranslated exception text. That case gets its own FailureReason and the string back. The Jetpack install client lost buildUrl's empty-string guard when it moved off getWpApiClient, so an empty wpApiRestUrl reached ParsedUrl.parse instead of falling back. Tests use a validator stub that fires the notifier the way the real one does; plain thenReturn stubs hid the feedback loop that caused the first bug. * Cover the application-password card click handlers Three tests asserting each card emits the right SiteNavigationAction when tapped: the create and re-authentication cards open auto-authentication, the XML-RPC disabled card opens the bottom sheet. The slice's rendering was covered already; its click wiring was not. * Retire the bearer-token framing after trunk stopped it notifying trunk c45e08e made the WP.com bearer client a no-op notifier, on the same reasoning the heal containment was written for: application-password reauthentication can't fix a bearer-token 401. That removes one of the two self-feeding 401 sources, so the comments no longer cite it. The containment still applies: getApplicationPasswordClient — the client validation uses — and the self-hosted capability probe both still report to the handler, which is what makes a heal schedule a redundant re-run of itself. * Tell the user their site is private instead of showing nothing Revoke a private site's application passwords and pull to refresh, and the app said nothing at all. The pipeline was right — validate, wipe, mint, settle Unprovisionable — but building the re-authentication card needs an authorization URL, discovery can't get one through the Privacy gate, and the Failed branch posted null. The user is left with broken credentials and no explanation. The two discovery-failure branches (create card and re-auth banner) were identical, so they share one handler. It names the one cause this branch can recognise; everything else still hides pending #22884. Its own string rather than the login screen's: there the user is signing in, here they are already signed in and the site can't be reconnected. * Centre the private-site card and drop its invisible icon The message sat off-centre and wrapped onto two lines because the layout constrains the text to start after the icon, and the card was passing ic_notice_white_24dp — a white icon on a white card. Nothing was drawn, but the space was still reserved. SingleActionCard gains a centerText flag: the image is hidden and the text spans the whole card. Reclaiming that space also fits the message on one line. The other four call sites take the default and are unaffected. Both branches of each layout decision are written out because view holders are recycled — a centred card must not leave the next one centred. bind() split into bindText / bindImage to stay under detekt's LongMethod limit. * Drop card-click tests that cover unchanged behaviour The three tests asserting each card emits the right SiteNavigationAction covered wiring this PR never touches: neither OpenApplicationPasswordAutoAuthentication nor OpenXmlRpcDisabledBottomSheet appears as a changed line in the slice's diff, and they replaced nothing — the pre-PR test file had no click assertions at all. Good tests, wrong PR. This one is already flagged oversized, so they can land separately against the code they actually cover. Audited the rest and kept it: every other added test either belongs to a new class, replaces one removed in the same rewrite, or covers behaviour this PR changed. * Run XML-RPC discovery on IO, and stop losing backgrounded re-auth prompts Three review findings. SiteXmlRpcUrlRecoverer ran verifyOrDiscoverXMLRPCEndpoint — a blocking chain of HTTP calls that can take tens of seconds against a dead host — on BG_THREAD, which is Dispatchers.Default. That is the same CPU-sized pool APPLICATION_SCOPE runs every site's pipeline on, so a few slow self-hosted sites could park most of it. The code this replaced used IO_THREAD; restore that. escalateReauth marked the once-per-episode flag before knowing whether anyone took the prompt. Activities register their listener in onResume, so a heal that settles while the app is backgrounded notified an empty map and lost the prompt for good: later 401s are blocked by the Unprovisionable state, and routine runs settle with wasHeal = false. notifyReauthRequired now reports whether a live listener took it, and an undelivered prompt is held and retried by the next run — in practice the onResume run, once an activity has registered. The pipeline's catch-all mapped any throw to Unreachable without the offline check that ensureAuth and detectCapabilities both make, stacking the site banner on the global no-connection one. It now makes the same distinction. The first attempt at the second fix only stopped burning the flag, which left nothing to retry — the test caught it. * Remove the blank line the dropped tests left before the class brace checkstyle's RegexpMultilineCheck runs over Kotlin too, and deleting the last test in the file left its trailing blank line sitting before the closing brace. * Hide the site banner while the device is offline Reported on the PR: with "Unable to connect to your site" showing, turning the device offline leaves it stacked on the global "no connection" bar. Losing the network doesn't re-run the pipeline, so the readiness the banner collects stays Unreachable and nothing re-evaluates. The old code had the same hole — its suppressForOffline was computed once inside fetchCapabilities — but this PR added a second route into Unreachable (site unreachable at the auth stage), so the state is easier to be sitting in when the network drops. The banner now combines readiness with connectivity through a MediatorLiveData. ConnectionStatusLiveData only emits on transitions and swallows its initial value, so it serves as a change trigger and the decision reads isNetworkAvailable() for the live answer. Starting offline already behaved correctly — ensureAuth settles Provisioning, not Unreachable. This is only about losing the network while the banner is up. * Stop a failed column write from discarding a successful probe Reported on the PR. The two recovery stages end in an unguarded WellSql update. Only the discovery half of XML-RPC recovery had a try/catch, so a throwing write — SQLiteException, database locked — escaped the async, cancelled the sibling through the enclosing coroutineScope, and settled the whole site Unreachable. The connectivity banner appeared and Ready never latched even though capability detection had already succeeded. The outer catch in launchPipeline anticipated a DB throw but contains it too coarsely: by then the successful result is gone. Both stages are best effort, so contain them individually and let the capability probe alone decide readiness. The report named the XML-RPC branch; persistApiRootUrl has the same hole, and worse placement — it runs before detectCapabilities in the capability branch, so a throw there kills the probe before it starts. Both are covered. --------- Co-authored-by: Nick Bradbury <nick.bradbury@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #22905. The REST API root (
wpApiRestUrl) is discovered/healed out of band — nevercarried by the general site-sync responses — yet every full-row
insertOrUpdateSiteUPDATErewrote it. When the in-memory
SiteModelcame from a source that doesn't carry it (/me/sites,/sites/<id>, the RN bridge, cookie-nonce auth), the write stamped a stalenullover the goodvalue, so on Atomic sites the recovered URL was wiped on every app foreground.
Two adjacent columns share that write path and are folded in here. The application-password
credentials weren't actually leaking — the same copy-forward shielded them (it was gated on
them) — but it had to be torn out for the
wpApiRestUrlfix, so they take the same exclusion as aTOCTOU-free replacement.
xmlRpcUrlis a third case: the WP.com sync reliably carries it, sorather than exclude it we preserve it on absence — only a partial writer that omits it (the WPAPI
fetch) can null it. See #3.
Root cause
SiteSqlUtils.insertOrUpdateSiteupdates viaWellSql.update().put(model, UpdateAllExceptId(...)),which writes every column except
_id. Per-handler preservation existed for some columnsbut was gated on encrypted AP creds being present — which Atomic sites don't have — so it
didn't fire for them, and it was TOCTOU-fragile regardless.
Fix
Two mechanisms, matched to how each column is sourced:
WP_API_REST_URLand the app-password credential columns.These are never carried by a site-sync response, so the full-row UPDATE skips them entirely (via a new
UpdateAllExceptIdskip-list) and a targeted single-column writer is their sole writer on anexisting row. INSERT is untouched. Every caller that legitimately set one is rerouted to the targeted
writer; redundant writes are removed.
XMLRPC_URL. The WP.com sync does carry it, so the full-row UPDATE stillwrites it (that's how a changed endpoint lands); it just keeps the stored value when the inbound model
carries none, so a partial writer can't null it.
Column-by-column:
1.
WP_API_REST_URLupdateWpApiRestUrl(the heal writer from Populate missingwpApiRestUrlduring editor preload #22903) becomes the sole writer;clearWpApiRestUrlfor sign-out.
updateApplicationPassword,CookieNonceAuthenticator,ReactNativeStore, andfetchSiteWPAPIFromApplicationPassword(URL-keyed, since the freshlyfetched site has no local id).
2. Application-password credentials
Hardening, not a clobber fix — the gated copy-forward already shielded these (see Root cause), so
they weren't leaking; the exclusion is a robust, TOCTOU-free replacement once that copy-forward goes.
API_REST_USERNAME,API_REST_PASSWORDand their two IV columns, excluded as a set — theIVs are required to decrypt the ciphertext, so persisting the values without them breaks reads.
updateApplicationPasswordCredentials(encrypts) /clearApplicationPasswordCredentials,wired into
updateApplicationPassword,removeApplicationPassword(full sign-out — also clearswpApiRestUrl),clearApplicationPasswordColumns(rotation — intentionally preserveswpApiRestUrl), and the WPAPI app-password fetch.createOrUpdateSitespersists creds via the targeted writer too:insertOrUpdateSitegained aninsertOrUpdateSiteReturningIdvariant so the handler can target the exact written row. Thisfixes app-password login of an existing XML-RPC site, where the exclusion would otherwise
drop the creds.
3.
XMLRPC_URL— preserved, not excludedUnlike the other two, the WP.com REST sync reliably returns
meta.links.xmlrpc(verified against/me/sites), so excluding the column would silently drop a changed endpoint (e.g. a domain migration)on an existing row. Instead,
insertOrUpdateSitepreserves it on absence — copying the stored valueforward only when the inbound model has none — so the WPAPI fetch (which builds a model with a
nullxmlRpcUrl) can't clobber a stored/rediscovered value.updateXmlRpcUrl/SiteStore.persistXmlRpcUrlremain for the single-column rediscovery heal — onetargeted write instead of an ~100-column full-row
updateSite.attemptXmlRpcRediscovery(the My Site app-password card) persists the rediscovered endpoint throughthat writer.
Cleanup enabled by the migration
wpApiRestUrlwrites inCookieNonceAuthenticator/ReactNativeStore(and RN's now-orphaned
persistSiteSafely/sitePersistanceFunction) — they reset the columnin memory to drive rediscovery and never needed to persist.
wpApiRestUrlcopy-forward blocks inupdateSite/createOrUpdateSites.updateApplicationPassword/removeApplicationPassword/clearApplicationPasswordColumnsto their targeted writers — the in-memory mutations and self-
insertOrUpdateSitethey used todo persisted nothing once the columns were excluded.
Defensive hardening (unrelated to the clobber)
decryptAPIRestCredentialsnow also short-circuits when an IV column is blank (not just when theciphertext is empty), so a malformed ciphertext-without-IV row reads as "no credentials" instead
of throwing on
decrypt(ciphertext, ""). Cheap, and on the same read path.Out of scope / follow-ups
updateXmlRpcUrl(provided here) and its ownrework of
attemptXmlRpcRediscovery; both reconcile against this.Testing instructions
Automated
./gradlew :libs:fluxc:testDebugUnitTestStatsUtilsTestflake unrelated to this change).SiteSqlUtilsTestcovers each column family — a stale full-row update preserves the protectedcolumn while still updating the others, and for
xmlRpcUrlthat a carried value still overwrites(the migration case) — plus the targeted writers and the blank-IV decrypt guard.
./gradlew :WordPress:testJetpackDebugUnitTest --tests "*ApplicationPasswordViewModelSliceTest"persistXmlRpcUrlwrite.Manual — Atomic site (the original repro)
*.jurassic.ninja) and open it sowpApiRestUrlisdiscovered/healed.
WP_API_REST_URLsurvives — no longer reset toNULLacross launches.Manual — self-hosted application-password site
xmlRpcUrlis repopulated and survives a relaunch.wpApiRestUrlare cleared.Appendix: site-type × column handling
How each site type's three out-of-band columns are persisted, and what this PR changes (Δ).
Mechanisms: 🔒 excluded — the full-row sync
UPDATEskips the column; only a dedicated targeted writer persists it (never carried by a sync, so a full-row write could only zero it). 🛡️ preserve-on-absence — the full-rowUPDATEwrites the column when the inbound model carries a value, but keeps the stored value when it doesn't. Targeted writers:updateWpApiRestUrl/updateXmlRpcUrl/updateApplicationPasswordCredentials(+clear*and URL-keyed*ForWPAPISitevariants).wpApiRestUrlxmlRpcUrlisWPCom && !atomic)public-api.wordpress.com/wp/v2/sites/<id>on read, so the stored column is moot (mapper writes the synthetic value on INSERT; 🔒 only stops UPDATEs rewriting it). No change.meta.links.xmlrpc; 🛡️ written on sync, but unused (Simple sites don't speak XML-RPC). No meaningful change.isUsingWpComRestApi, non-simple)updateWpApiRestUrl.meta.links.xmlrpc; 🛡️ written on sync (incl. a migrated endpoint)./me/sitessyncs; now 🔒 excluded → preserved viaupdateApplicationPasswordCredentials.ORIGIN_WPAPI)updateWpApiRestUrlForWPAPISite(fresh model has no local id yet).fetchWPAPISiteleaves it null. Δ 🛡️ preserve-on-absence keeps a rediscovered value — a re-fetch would otherwise clobber it; healed viapersistXmlRpcUrl(rediscovery card, gated!isUsingWpComRestApi).updateApplicationPasswordCredentialsForWPAPISite.ORIGIN_XMLRPC)apiRootUrl) when present. 🔒 excluded → written on the stamped row byupdateWpApiRestUrl.insertOrUpdateSiteReturningId+updateApplicationPasswordCredentials(the existing-site half of the #22905 fix; the full-row write alone dropped them).Notes
wpApiRestUrland creds on Atomic / Jetpack-via-REST / self-hosted sites. WP.com Simple was never affected (synthetic getter), andxmlRpcUrlwas never the real problem (the WP.com sync reliably returnsmeta.links.xmlrpc).xmlRpcUrlwas de-escalated from excluded to preserve-on-absence during review: excluding it would have silently dropped a migrated endpoint for WP.com sites, and the only real non-WP.com clobber (theORIGIN_WPAPIre-fetch) is covered by preserve-on-absence.wpApiRestUrl; credential rotation clears creds but preserveswpApiRestUrl(reused across rotations).