Make wpApiRestUrl, xmlRpcUrl, and app-password creds single-writer#22947
Draft
jkmassel wants to merge 6 commits into
Draft
Make wpApiRestUrl, xmlRpcUrl, and app-password creds single-writer#22947jkmassel wants to merge 6 commits into
jkmassel wants to merge 6 commits into
Conversation
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.
Collaborator
Generated by 🚫 Danger |
Contributor
|
|
Contributor
|
|
Contributor
🤖 Build Failure AnalysisThis build has failures. Claude has analyzed them - check the build annotations for details. |
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 the same kind of out-of-band-discovered column as wpApiRestUrl: a full-row insertOrUpdateSite UPDATE from a partial SiteModel (e.g. a /sites sync whose response omits meta.links) writes a stale/null over a good value. Exclude XMLRPC_URL from the UpdateAllExceptId mapper and add a targeted updateXmlRpcUrl writer as its sole writer on an existing row. The xmlRpcUrl recovery flow that calls it lands separately. Extends the #22905 fix to XMLRPC_URL.
jkmassel
changed the title
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.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| eq(xmlRpcUrl), any(), any() | ||
| ) | ||
| ).thenReturn(SitesModel(listOf(SiteModel()))) | ||
| whenever(siteStore.persistXmlRpcUrl(any(), any())).thenReturn(mock()) |
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).
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
4 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. Several
SiteModelcolumns are discovered or healed out of band — not carried by the general site-sync responses — yet every full-rowinsertOrUpdateSiteUPDATE wrote them. When the in-memorySiteModelcame from a source that doesn't carry a field (/me/sites,/sites/<id>, the RN bridge, cookie-nonce auth), the full-row write stamped its stale/nullover a good value. On Atomic sites the recoveredwpApiRestUrlwas wiped on every app foreground; the same mechanism zeroes the application-password credential columns andxmlRpcUrl.Root cause
SiteSqlUtils.insertOrUpdateSiteupdates viaWellSql.update().put(model, UpdateAllExceptId(...)), which writes every column except_id. Per-handler preservation existed for some columns but 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
Make targeted single-column writers authoritative and exclude these columns from the generic mapper.
UpdateAllExceptIdnow accepts additional columns to skip;insertOrUpdateSite's UPDATE excludes them. INSERT is untouched, so new sites still persist everything.Three column families, each with a targeted writer as the sole writer on an existing row:
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 freshly fetched site has no local id).2. Application-password credentials
API_REST_USERNAME,API_REST_PASSWORDand their two IV columns, excluded as a set — the IVs are required to decrypt the ciphertext, so protecting the values without them would break reads.updateApplicationPasswordCredentials(encrypts) /clearApplicationPasswordCredentials. Wired intoupdateApplicationPassword,removeApplicationPassword(full sign-out — also clearswpApiRestUrl),clearApplicationPasswordColumns(credential rotation — intentionally preserveswpApiRestUrl), and the WPAPI app-password fetch.3.
XMLRPC_URLupdateXmlRpcUrlas the sole writer. Excluded from the mapper here so the clobber is fixed when this lands; the recovery flow that calls it lands in Unify site provisioning + capability detection into one per-site source #22944 (blocked behind this PR).Out of scope / follow-ups
wpApiRestUrlpreservation blocks inupdateSite/createOrUpdateSitesare left in place — harmless, optional cleanup.updateXmlRpcUrl(provided here) and keeps theSiteXmlRpcUrlRecovererthat consumes it.:libs:fluxc:lintDebugnoise (mostlyDoNotMockDataClass) is unrelated and untouched. Detekt is clean.Testing instructions
Automated
./gradlew :libs:fluxc:testDebugUnitTest --tests "*SiteSqlUtilsTest" --tests "*SiteStoreTest"SiteSqlUtilsTestincludes a clobber-prevention guard per family (a stale full-row update preserves the column while still updating other columns), plus the targeted writers.Manual — Atomic site (the original repro)
*.jurassic.ninja) and open it sowpApiRestUrlis discovered/healed.NULLtheWP_API_REST_URLcolumn for that site row.WP_API_REST_URLis healed and then survives — no longer reset toNULL.Manual — self-hosted application-password site
wpApiRestUrlare cleared.