Skip to content

fix(android): scope the editor globals and keep site pages out of the editor frame - #730

Merged
dcalhoun merged 20 commits into
trunkfrom
fix/android-scope-config-injection
Sep 30, 2026
Merged

dcalhoun merged 20 commits into
trunkfrom
fix/android-scope-config-injection

Conversation

@dcalhoun

@dcalhoun dcalhoun commented Sep 25, 2026 •

Copy link
Copy Markdown
Member

What?

Mitigate exposing editor globals to non-editor pages.

Why?

The editor globals contain configuration that should only be accessible to the editor HTML page and its scripts.

How?

  • Remove logic allowing REST API endpoints to load directly in the WebView, fetch of these URLs works without this
  • Narrow the asset path detection to matching schemes
  • Disable editor globals and upload server for non-editor URLs

Testing Instructions

Tip

If inspecting emulators with Chrome becomes difficult due to lack of responsiveness, using a physical device may be easier.

Manual testing on a device against wp-env, in both configurations, using chrome://inspect to reach the editor WebView's console:

  1. Production path — with GUTENBERG_EDITOR_URL unset in android/local.properties, open a post. window.GBKit is defined.
  2. Various editor functionality works: edits, upload an image, etc.
  3. In the console, each of these opens in the OS browser instead of replacing the editor:
    • window.top.location.href = location.origin + '/wp-json/wp/v2/posts'
    • window.top.location.href = location.origin + '/?rest_route=/wp/v2/posts&rest_route=' (on trunk, the themed front page)
    • On an https site (not wp-env), window.top.location.href = 'http://' + location.host + '/wp-json/wp/v2/posts'
  4. Add a temporary android/app/src/main/assets/probe.html, rebuild, and navigate via window.top.location.href = location.origin + '/assets/probe.html'. It loads, and in its console window.GBKit is undefined (on trunk, the object).
  5. Dev server — set GUTENBERG_EDITOR_URL=http://10.0.2.2:5173/, run make dev-server, open a post. window.GBKit is defined.
  6. With the dev server still configured, navigate via window.top.location.href = 'http://10.0.2.2:8888/' — the wp-env site on another port of the same host. It opens in the OS browser rather than being taken for the dev server.

Accessibility Testing Instructions

N/A, no user-facing changes.

Screenshots or screencast

N/A, no user-facing changes.


AI-generated details

Problem

onPageStarted received the URL of the page that had begun loading and discarded it, so window.GBKit — the site credential and the local upload server's port and token — was injected into whatever loaded in the main frame. Separately, the navigation policy admitted the REST API into the main frame, recognizing it by substring: any path containing /wp-json/ or any query containing rest_route=.

Impact

Since #181 the Android editor loads from the site's own origin, so the pages those substrings admit are ordinary pages that WordPress serves with the site's theme, plugins and third-party scripts — rendered inside the editor's frame, and handed the credential in a readable global. /blog/wp-json/a-post and /a-page/?utm_campaign=rest_route=x both qualify. iOS is unaffected: it blocks all main-frame navigation outside the editor.

Both behaviors reproduce on trunk; the Robolectric cases added here fail against it.

Mechanism

  • onEditorPageStarted takes the loaded URL and advertises the globals, and starts the upload server, only for the editor document: the URL loadEditor recorded (feat(android): show why the editor document failed to load #740's editorUri), compared by scheme, host and path. Readiness still resets for any page, since navigating away leaves the editor unusable either way.
  • Hosts compare case-insensitively, since Chromium lowercases the host it reports; a dev server URL written with capitals, like a Mac's mDNS name, otherwise never matched.
  • The main frame no longer admits the REST API, site or WordPress.com. The editor reaches it by fetch, which never passes through shouldOverrideUrlLoading, so the allowlist only ever admitted navigations — and matching WordPress's URL parsing proved open-ended: a duplicate or empty rest_route, or http on an https site, still let a page replace the editor.
  • Navigation to the editor's assets matches only the scheme the editor loads with; on an https site, the same path over http reaches the site over the network. The host app's other bundled pages, and other paths on the dev server, load but don't match the recorded editor URL, so they receive no globals.
  • The new cases live in GutenbergViewNavigationTest rather than GutenbergViewTest, which Detekt flags as LargeClass once they are added.

Testing

Unit and lint, both green, and each commit passes on its own:

make test-android-library-unit
make lint-android

Manual testing on a device against wp-env, in both configurations, using chrome://inspect to reach the editor WebView's console:

  1. Production path — with GUTENBERG_EDITOR_URL unset in android/local.properties, open a post. window.GBKit is defined.
  2. Edit, save, upload an image, and open the inserter's Patterns tab. All work; the API is reached by fetch, and pattern previews by blob: frames, neither of which the policy gates.
  3. In the console, each of these opens in the OS browser instead of replacing the editor:
    • window.top.location.href = location.origin + '/wp-json/wp/v2/posts'
    • window.top.location.href = location.origin + '/?rest_route=/wp/v2/posts&rest_route=' (on trunk, the themed front page)
    • On an https site, window.top.location.href = 'http://' + location.host + '/wp-json/wp/v2/posts'
  4. Add a temporary android/app/src/main/assets/probe.html, rebuild, and navigate to location.origin + '/assets/probe.html'. It loads, and in its console window.GBKit is undefined (on trunk, the object).
  5. Dev server — set GUTENBERG_EDITOR_URL=http://10.0.2.2:5173/, run make serve-dev, open a post. window.GBKit is defined.
  6. With the dev server still configured, navigate to http://10.0.2.2:8888/ — the wp-env site on another port of the same host. It opens in the OS browser rather than being taken for the dev server.
  7. Navigate to location.origin + '/index.html', which Vite serves as the same HTML at another path. It loads, window.GBKit is undefined, and the page reports "GBKit global not available after timeout".
  8. make test-android-library-e2e on an emulator.

Not verified on a device: asset paths over the other scheme, covered by a unit case.

The localStorage copy of the globals is removed separately in #613.

@github-actions github-actions Bot added the [Type] Bug An existing feature does not function as intended label Sep 25, 2026
@wpmobilebot

wpmobilebot commented Sep 25, 2026 •

Copy link
Copy Markdown

XCFramework Build

This PR's XCFramework is available for testing. Add the following to your Package.swift:

.package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/730")

Built from 83346fb

@dcalhoun dcalhoun changed the title fix(android): scope the editor globals and tighten the REST allowlist fix(android): scope the editor globals and keep site pages out of the editor frame Sep 25, 2026
dcalhoun and others added 11 commits September 25, 2026 16:37
`onPageStarted` received the loaded URL and discarded it, so any page
reaching the main frame was handed `window.GBKit` — the site credential
and the upload server's port and token. `shouldOverrideUrlLoading` admits
several site URLs into that frame, and since #181 the editor shares an
origin with the site, so those pages are served by the site's own theme
and plugins.

Check the destination before advertising the globals or starting the
upload server, matching the dev server by authority so a local site on
another port of the same host is not mistaken for the editor. Readiness
still resets for any page, since navigating away from the editor leaves
it unusable either way.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016zAv5Tqtqr1bcYWVDJHGAh
…ring

The navigation policy admitted any site URL whose path contained
`/wp-json/` or whose query contained `rest_route=`. Both are satisfied by
ordinary pages — `/blog/wp-json/a-post`, or any URL carrying
`?utm_campaign=rest_route=x` — which WordPress serves with the site's
theme and plugins, inside the editor's own frame.

Compare against the configured `siteApiRoot` instead: a path under its
path root, or `rest_route` as an actual query parameter. Reading the root
also settles the cases the characters cannot, so the same path is the API
on a subdirectory install and a page on a root install.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016zAv5Tqtqr1bcYWVDJHGAh
Without pretty permalinks the API root is `/index.php?rest_route=/`, and `/index.php` also serves ordinary pages, so matching its path admitted them into the editor frame.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CSbnGRWvgz7WNn6cGFRKxs
A root without a trailing slash, such as `/wp-json`, otherwise prefixes page slugs like `/wp-json-tutorial/`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CSbnGRWvgz7WNn6cGFRKxs
WordPress skips an empty route, including `0`, and renders the requested page with the site's theme instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CSbnGRWvgz7WNn6cGFRKxs
Checking only the last evaluated script would pass if another script ran after an injection.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CSbnGRWvgz7WNn6cGFRKxs
The asset loader serves one scheme, so the same path over the other reaches the site over the network, yet it was admitted and handed the editor globals. One helper now backs both checks so they can't drift apart.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CSbnGRWvgz7WNn6cGFRKxs
The asset loader also serves the host app's other bundled pages, some of which load third-party scripts, and those received the editor globals.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CSbnGRWvgz7WNn6cGFRKxs
WordPress lets the parameter override the route a `/wp-json/` path sets, so `/wp-json/?rest_route=` serves the themed front page, yet it passed the path check.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CSbnGRWvgz7WNn6cGFRKxs
The editor reaches the REST API by fetch, which never passes through `shouldOverrideUrlLoading`, so the allowlist only admitted navigations. Those let site pages whose URLs WordPress reads differently from Android, and http API URLs on https sites, replace the editor.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CSbnGRWvgz7WNn6cGFRKxs
@dcalhoun
dcalhoun force-pushed the fix/android-scope-config-injection branch from 1a1a592 to 39894ad Compare September 25, 2026 20:37
dcalhoun and others added 4 commits September 25, 2026 18:16
Since the REST allowlist was removed, the navigation policy admits no site pages, but loads it never sees still can.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CSbnGRWvgz7WNn6cGFRKxs
Nothing pinned the editor-only check above the server start, so reordering them would have passed every test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CSbnGRWvgz7WNn6cGFRKxs
… client

The client reads it, so assigning it alongside the asset authority avoids relying on no navigation running in between.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CSbnGRWvgz7WNn6cGFRKxs
A local http site's asset loader serves https too, so the other scheme doesn't always reach the site over the network.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CSbnGRWvgz7WNn6cGFRKxs
@dcalhoun
dcalhoun marked this pull request as ready for review September 28, 2026 12:56
@dcalhoun
dcalhoun requested a review from nbradbury September 28, 2026 12:56
@nbradbury

nbradbury commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

@dcalhoun As always, Claude found some possible issues, but I'm unsure any are meaningful.

Severity Location Issue Impact
Medium GutenbergView.kt:737 setGlobalJavaScriptVariables still writes the whole GBKit object (auth header, upload port and token) to localStorage, which is scoped to the origin the editor shares with the site. Once the editor has loaded, a themed site page that later reaches the WebView can run JSON.parse(localStorage.getItem('GBKit')).authHeader; the new tests only check lastEvaluatedJavascript, so they pass while this stays open until #613 lands.
Medium GutenbergView.kt:693 When a non-editor page loads, isEditorLoaded/didFireEditorLoaded are reset without notifying the host or changing the UI phase. A POST form replaces the ready editor with no onEditorUnavailable callback and the ready phase still showing, so the host's next save gets EditorNotReadyException with no warning and the user has no way back to the editor.
Medium GutenbergView.kt:720 isDevServerUrl compares Chromium's lowercased uri.authority with the authority from GUTENBERG_EDITOR_URL exactly as written, so a mixed-case host never matches. With GUTENBERG_EDITOR_URL=http://MyMac.local:5173/, isEditorUrl returns false and GBKit is never injected, so the editor fails with "GBKit global not available after timeout", which worked before this PR.
Low GutenbergView.kt:719 In dev-server mode, isEditorUrl accepts any URL on the dev server's authority, with any scheme and any path, while the production branch requires the exact index path over the asset scheme. Navigating to any other page Vite serves, such as http://10.0.2.2:5173/some-other.html, injects the credential and upload token and starts the upload server, which is exactly what the production branch now rejects.
Low GutenbergView.kt:715 isEditorUrl works out again which URL loadEditor chose instead of storing the URI actually passed to loadUrl and comparing against that. The editor-URL logic now exists in four places (loadEditor, isEditorUrl, and two test helpers), so adding a query param or changing the index path silently stops globals from being injected unless all four are updated.
Low GutenbergViewTest.kt:599 The reload readiness test still calls onPageStarted(webView, null, null), which now takes the non-editor branch, so no test covers GBKit being re-injected after reloadEditor(). A regression that drops globals when the editor reloads leaves the reloaded editor without GBKit, and every existing test still passes.
Low GutenbergViewNavigationTest.kt:43 When a developer has GUTENBERG_EDITOR_URL set, the withholding tests go through the dev-server branch, and the local-http wp-env editor URL has no injection coverage at all. Breaking the ASSET_PATH_INDEX or scheme check in isAssetUrl still passes locally with GUTENBERG_EDITOR_URL set, and a bug that denies globals to http://10.0.2.2:8888/assets/index.html would not be caught.
Low GutenbergViewNavigationTest.kt:60 configuredSiteView() builds a new GutenbergView and WebView for every URL inside the forEach loops and never destroys them. The two REST API tests build 8 full views, each running loadEditor, WebView init and cookie clearing, where one view per test would do, which makes them slower and leaks Robolectric WebViews and network callbacks.

8 findings — 3 Medium, 5 Low.

@nbradbury nbradbury left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The testing steps passed so I'm good to :shipit: once conflicts are resolved

dcalhoun and others added 5 commits September 30, 2026 13:42
…nfig-injection

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CSbnGRWvgz7WNn6cGFRKxs
Chromium lowercases the host it reports, so a dev server URL written with capitals, like a Mac's mDNS name, never matched and the editor received no globals.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CSbnGRWvgz7WNn6cGFRKxs
Re-deriving the editor URL duplicated loadEditor's choice and let any page on the dev server's host receive the globals. Comparing against the recorded URL keeps the two in step and requires the exact document.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CSbnGRWvgz7WNn6cGFRKxs
Neither path had a positive case, so dropping the globals from either would have passed every test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CSbnGRWvgz7WNn6cGFRKxs
Each URL built its own editor view, loading the editor eight times where two views suffice.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CSbnGRWvgz7WNn6cGFRKxs
@dcalhoun

Copy link
Copy Markdown
Member Author

Thanks, @nbradbury.

Fixed in this PR:

  • Mixed-case dev server host never matches: 9c5d871 compares hosts case-insensitively.
  • Dev server accepts any path, and isEditorUrl re-derives the editor URL: 6cff4cc matches the editor against the URL loadEditor recorded (from feat(android): show why the editor document failed to load #740), so there's one source of truth and the dev server must match the exact document.
  • No re-injection test after reloadEditor(), and no local-http injection coverage: a211496 adds both.
  • A view per URL in the loop tests: 83346fb builds one view per test.

Out of scope here:

@dcalhoun
dcalhoun merged commit 5c0f736 into trunk Sep 30, 2026
26 checks passed
@dcalhoun
dcalhoun deleted the fix/android-scope-config-injection branch September 30, 2026 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Bug An existing feature does not function as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants