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
6885e4c to
67118a8
Compare
jkmassel
force-pushed
the
jkmassel/sitesqlutils-partial-updates
branch
from
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:
|
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
88e8833 to
fb89734
Compare
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.
8 tasks
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).