Make wpApiRestUrl, xmlRpcUrl, and app-password creds single-writer#22947
Draft
jkmassel wants to merge 11 commits into
Draft
Make wpApiRestUrl, xmlRpcUrl, and app-password creds single-writer#22947jkmassel wants to merge 11 commits into
jkmassel wants to merge 11 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.
jkmassel
changed the title
![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()) |
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
6885e4c to
67118a8
Compare
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 — notcarried by the general site-sync responses — yet every full-row
insertOrUpdateSiteUPDATEwrote them. When the in-memory
SiteModelcame from a source that doesn't carry a field(
/me/sites,/sites/<id>, the RN bridge, cookie-nonce auth), the full-row write stampedits stale/
nullover a good value. On Atomic sites the recoveredwpApiRestUrlwas wiped onevery app foreground; the same mechanism zeroes the application-password credential columns.
xmlRpcUrlis the same shape of bug but milder — the WP.com sync reliably carries it, so only apartial writer that doesn't (the WPAPI fetch) can null it. See #3 below.
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
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 ~80-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.:libs:fluxc:lintDebugnoise (mostlyDoNotMockDataClass) is unrelated and untouched.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.