fix(android): load the editor same-origin on sites with a non-standard port - #599
Open
dcalhoun wants to merge 3 commits into
Open
fix(android): load the editor same-origin on sites with a non-standard port#599dcalhoun wants to merge 3 commits into
dcalhoun wants to merge 3 commits into
Conversation
…d port The editor document was served from the site's host without its port, so on a site with a non-standard port it ran on a different origin than the site and every REST request became cross-origin — the opposite of what the surrounding code intends. An origin is scheme + host + port, so `http://10.0.2.2:8888` yielded an editor at `http://10.0.2.2` (implicit port 80). Against wp-env this blocked the entire editor bootstrap: settings, taxonomies, global styles, block patterns, blocks, media, pages, and the theme's webfont. Blocks that fetch from the REST API to render — Latest Posts, Categories List, Terms List — fail. Sites on default ports are unaffected, since their URLs carry no port. The bug is also unreachable while `GUTENBERG_EDITOR_URL` is set, because the WebView then loads the dev server rather than the bundled assets, so it surfaces only in bundled builds. Derive the asset origin from the site URL's authority (host *and* port) rather than its host. `WebViewAssetLoader` matches on authority, so a port-bearing value still resolves assets. The comparisons in `shouldInterceptRequest` and `shouldOverrideUrlLoading` move to `authority` in the same change, since a port-bearing value would otherwise stop matching and break interception. `cachedAssetHosts` stays host-based: it is a caller-supplied allowlist rather than an origin comparison. `DEFAULT_ASSET_DOMAIN` keeps its name as a public top-level const, and is already a valid authority. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015iGCfxbytmGeVPkUAPW1NC
XCFramework BuildThis PR's XCFramework is available for testing. Add the following to your .package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/599")Built from 57f373a |
Contributor
|
@dcalhoun I asked Claude for a review, and it seems like at least the first issue needs to be addressed. |
Taking `Uri.authority` verbatim regressed site URLs that carry an explicit default port. Chromium canonicalizes a URL before it reaches the WebViewClient, dropping a default port and any userinfo, so a site stored as `https://example.com:443` yields an asset authority of `example.com:443` while every request arrives as `example.com`. `WebViewAssetLoader.PathMatcher` compares authorities exactly, so nothing matches: the bundled `index.html` is never served, the WebView issues a real network request, and it lands on the site's 404 page. The editor does not load at all — worse than the cross-origin bug this branch fixes, and a case trunk handled correctly via `Uri.host`. Derive the origin authority from the host plus the port, keeping the port only when it is not the scheme's default. Rebuilding from `host` also drops userinfo, matching canonicalization there too. `Uri` does not split a bracketed IPv6 literal into host/port — `host` returns `[` — so those fall back to the authority as written rather than being rebuilt. Reaching this in practice is unlikely: a self-hosted site's URL comes from the server's own `home_url` rather than user input, so it carries a default port only when WordPress has one persisted in its `home` option. The failure is total when it does happen, and normalizing costs little. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015iGCfxbytmGeVPkUAPW1NC
…rity
The "Allow WordPress REST API" branch compared `url.host` against the API root
with only its scheme stripped, leaving the path attached — `example.com/wp-json/`
— so it could never equal a bare host. Every caller passes a full API root with
a path (WordPress-Android's `site.wpApiRestUrl ?: "${site.url}/wp-json/"`, the
demo app's builders), making the branch unreachable.
An in-editor navigation to a REST URL therefore fell through to the external
browser instead of loading in the WebView.
Compare authorities instead, reusing `originAuthority` so this matches what
Chromium sends and keeps working for an API root that carries a port.
Pre-existing, and unrelated to the same-origin change on this branch, but the
same host-vs-authority bug class and adjacent code.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015iGCfxbytmGeVPkUAPW1NC
Member
Author
|
@nbradbury I addressed the two findings from your PDF. Ready for your re-review. |
Member
Author
|
@nbradbury to clarify, I fixed the issues as described by your PDF. I did not replicate the described problem, but addressed them a defense in depth. Did you replicate the reported issue on a site? Particularly first finding. |
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
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.
What?
The editor document was served from the site's host without its port, so on a site with a non-standard port it ran on a different origin than the site and every REST request became cross-origin — the opposite of what the surrounding code intends.
Why?
An origin is scheme + host + port, so a site at
http://10.0.2.2:8888produced an editor athttp://10.0.2.2(implicit port 80).Users on a self-hosted site with a non-standard port — behind a reverse proxy, on alternate-port hosting, or an internal deployment — get an editor that cannot read site data. Blocks that fetch from the REST API to render, such as Latest Posts, Categories List, and Terms List, fail. Requests only succeed at all if the site adds server-side CORS configuration that a same-origin editor never needs. Even then, response headers outside the CORS safelist stay unreadable, because core's
rest_send_cors_headers()only exposesX-WP-Total,X-WP-TotalPages, andLink.Sites on default ports (
:80/:443) are unaffected, since their URLs carry no port. That covers WordPress.com and most self-hosted sites, which is why this went unnoticed.It is also invisible in day-to-day development: setting
GUTENBERG_EDITOR_URLmakes the WebView load the dev server instead of the bundled assets, so the affected value is computed and discarded. The bug only surfaces in bundled builds — the configuration that ships.Against wp-env, this blocked the entire editor bootstrap: settings, taxonomies, global styles, block patterns, blocks, media, pages, and the theme's webfont.
How?
Derive the asset origin from the site URL's authority (host and port) rather than its host.
WebViewAssetLoadermatches on authority, so a port-bearing value still resolves assets correctly.The comparisons in
shouldInterceptRequestandshouldOverrideUrlLoadingmove toauthorityin the same change — a port-bearing value would otherwise stop matching and break asset interception entirely.Deliberately left alone:
cachedAssetHostsis a caller-supplied host allowlist, not an origin comparison. WordPress-Android passes bare hosts (s0.wp.com, the site host), so converting it would silently break those callers.LOCAL_HOSTSis likewise a host allowlist.DEFAULT_ASSET_DOMAINkeeps its name as a public top-levelconst, and is already a valid authority.Not a breaking change for consumers: the renamed field is
private, and the publicEditorConfigurationAPI is untouched.Testing Instructions
The bug is only reachable in a bundled build, so
GUTENBERG_EDITOR_URLmust be unset.make wp-env-android— installs the URL-remap mu-plugin and starts wp-env with the site URL remapped tohttp://10.0.2.2:8888for the emulator.GUTENBERG_EDITOR_URLinandroid/local.propertiesso the app uses the bundled build.Expected: each block renders its content. Before this fix they fail, because the requests backing them are blocked cross-origin.
Also confirm the editor bootstrap itself is clean —
adb logcatshould show noblocked by CORS policyerrors, and the consolesourceshould behttp://10.0.2.2:8888/assets/index.html(with the port), matching the site origin.To see the failure for comparison, check out
trunkand repeat from step 3: the same blocks fail and logcat fills with CORS errors againstorigin 'http://10.0.2.2'.Regression check on the unaffected path: open a WordPress.com or default-port self-hosted site and confirm the editor still loads normally. Host and authority are identical there, so behavior should be unchanged.
Accessibility Testing Instructions
N/A — no user interface changes.