Skip to content

Set OpenStation windows free into real OS windows (Electron adapter) - #542

Open
AllTerrainDeveloper wants to merge 10 commits into
trunkfrom
experimental-feature-electron
Open

Set OpenStation windows free into real OS windows (Electron adapter)#542
AllTerrainDeveloper wants to merge 10 commits into
trunkfrom
experimental-feature-electron

Conversation

@AllTerrainDeveloper

@AllTerrainDeveloper AllTerrainDeveloper commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator
Electron.mov

Any OpenStation window can now leave the desk and become a real window of your operating system — its own dock entry, its own Alt-Tab slot, sitting among your native apps.

Pick ⋯ → Send to your Mac (or Windows PC / Linux desktop — the label follows the host). Closing the native window brings it back. It works from the app and from a plain browser tab.

Send to your Mac in the window's ⋯ menu

What this adds

A desktop host, shipped as an extension. Everything Electron-specific lives in extensions/openstation-electron-adapter/ — a separate WordPress plugin plus the Electron app it talks to. Deactivate it and OpenStation is exactly the browser experience it was.

Both window types travel. An iframe window opens on the chromeless page it was already showing — same session, same scroll, same admin JS. A native window (Files, Games, a plugin canvas) has no URL of its own, so it opens in solo mode: the whole shell, painting exactly one window, no desk around it. Either way it is the same window, not a lookalike.

The browser gets native windows too. The app runs a loopback agent and hands its coordinates to the site, so a Chrome tab can ask your machine to open a window. Start the app after the page loaded and one ⋯ click picks it up — no refresh.

New windows go to the desktop. Launch a game from a freed Games hub and the game opens as its own OS window; the hub keeps showing the hub. Same for any window.open() in the app, except off-site URLs and "Open in browser tab", which go to the browser as asked.

Windows keep their names. The OS title bar reads Trash, Posts, Inkfall — not the browser tab title.

Core gains two generic capabilities

Neither mentions Electron, and both stand on their own:

wp.os.registerWindowAction() Rows in every window's ⋯ menu — the one title-bar surface with no extension point. label / icon / isVisible may be functions of the window, re-read per open, so one row can express a toggle. Paired with HOOKS.WINDOW_MENU_OPENED and a menu that repaints while open, a plugin can answer with something async and still land its row under the pointer.
?openstation_solo=<window-id> Boot the shell painting exactly one window — no dock, taskbar, wallpaper, desk or admin bar, and no session restore. A rendering mode, not an access grant. Useful for embeds and kiosks, not just this.

Core footprint: 857 lines across 16 files. Nothing in window-manager/.

Try it

  1. Symlink and activate the adapter — this step is the feature; without it the app connects and no ⋯ row appears:
    cd path/to/wp-content/plugins
    ln -sfn desktop-mode/extensions/openstation-electron-adapter openstation-electron-adapter
    wp plugin activate openstation-electron-adapter
  2. Run the app:
    cd extensions/openstation-electron-adapter
    npm install && npm start
  3. Enter your site address (http://localhost:8889), sign in, and choose Open here or Use my browser — both connect.
  4. Open any window → ⋯ → Send to your Mac. It appears on your desktop; the row becomes Bring back into OpenStation.
  5. Close the native window → it docks back into the shell.

Worth trying specifically:

  • A native window — Trash or Games — for the solo path.
  • From a browser tab at /openstation/, with the app running.
  • Games → play a game from a freed Games window: the game opens as its own OS window.
  • Start the app after loading the browser tab, then click ⋯ — the row appears without a refresh.

Security notes

The local agent is bound to 127.0.0.1, requires a per-installation bearer token (which forces a CORS preflight, so a hostile page cannot fire a blind request), accepts exactly one origin — the paired site — and re-checks every URL against that site before opening a window. The token reaches the page only while a host is actually live.

Tests

155 extension tests (agent gates, connection pacing, both freed-window registries, URL rules, store) and 35 core tests (action registry, menu painting, solo mode, the adapter's REST surface). Full suites green: 4060 vitest, 2123 PHPUnit, phpcs, typecheck, lint, build.

Status

Experimental. wp.os.electron, the REST routes and the solo flag may still move. The capability-probe model — a global whose absence means "browser, behave normally" — will not, and neither will the rule that core stays free of Electron.

Docs: docs/desktop-host.md · extensions/openstation-electron-adapter/README.md

🤖 Generated with Claude Code

Open WordPress Playground Preview

AllTerrainDeveloper and others added 8 commits August 10, 2026 02:15
OpenStation stays a web application. This adds an optional layer on top:
a small Electron app that loads the same site and gives it the one thing
a browser tab cannot — real operating system windows.

Every window's ⋯ menu grows a row: "Send to your Mac" / "…Windows PC" /
"…Linux desktop". Picking it takes the window out of the OpenStation
desk and opens it as a genuine window on the user's desktop — its own
dock entry, its own Alt-Tab slot. Closing that native window brings it
back. The row toggles rather than duplicating, because a window is
either here or there, never both.

Core mentions Electron nowhere. Everything Electron-specific lives in
extensions/openstation-electron-adapter/ — a separate WordPress plugin
plus the app it talks to. Deactivate it and OpenStation is byte-for-byte
the browser experience it was.

Two generic capabilities land in core to make it possible, each useful
on its own:

  - wp.os.registerWindowAction() — a registry for rows in every window's
    ⋯ menu. That menu was the one title-bar surface with no extension
    point. label / icon / isVisible may each be functions of the window,
    re-read on every open, which is what lets a single row express a
    state-dependent toggle.

  - ?openstation_solo=<id> — boot the whole shell painting exactly one
    window: no dock, taskbar, wallpaper, desk, or session restore. It
    exists because a native window has no URL of its own; it is a render
    callback, so the only way for it to *be* the same window elsewhere
    is to bring the framework along. Nothing about it is Electron-
    specific — an embed, a kiosk, or a PWA shortcut can use it too.

The adapter detects its host through a single injected global. Presence
is the probe: synchronous, no network, cannot go stale, and absent in
every browser. Freed windows load the chromeless URL their iframe was
showing, or solo mode for native windows — a decision the adapter makes,
never the app, which only ever takes a URL.

The app reports liveness to the site on a deliberately slow pulse: the
server picks the interval (filterable, so a constrained host widens it
without a new app build), idle skips beats, failure backs off, and the
whole server-side record is one user-meta row.

Anything that would surface a freed window inside the shell — a dock
click, the switcher, a plugin's openWindow() — raises the native window
instead. Without that rule the user ends up with two Posts windows that
know nothing about each other.

The app is TypeScript throughout, with its own ESLint config that keeps
the browser half and the Electron half structurally apart (src/** may
not import electron or Node built-ins; preloads may not expose
ipcRenderer). Decisions worth testing were kept out of the Electron-
coupled wiring: 101 tests cover pacing, the connection state machine,
both freed-window registries, URL rules and the store without launching
Electron. 35 more cover the core registry, the menu paint pass, solo
mode and the adapter's REST surface.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two bugs on the desktop app's first-run screen.

**Connect did nothing, forever.** The renderer was compiled by the app's
tsconfig alongside the main process and the preloads, which emit
CommonJS — correct for Electron, fatal in a page with nodeIntegration
off. The built file opened with `Object.defineProperty(exports, …)`,
`exports` was not defined, the script threw on its first statement, and
the form's submit listener was never attached. Clicking Connect ran the
browser's default action, which the page's own CSP blocks, so nothing
happened and nothing said why. No type error, no lint error, no failing
test.

`app/src/renderer/**` is now excluded from that tsconfig, bundled to an
IIFE by Vite, and typechecked by the root (browser) tsconfig with the
rest of the front-end code. ESM was the other candidate fix and is not
available: `type="module"` over `file://` is blocked by CORS.

`tests/connect-bundle.test.ts` asserts the built artefact carries
neither a CommonJS prologue nor ESM syntax, and `npm run verify` now
builds before it tests so that assertion runs against fresh output.

**The mark was a placeholder gradient.** It is now the real OpenStation
icon, taken from `.wordpress-org/icon.svg` so the app and the plugin
listing cannot drift apart, and shipped alongside as a 256px PNG for the
window icon on Windows and Linux (macOS takes its icon from the bundle,
and is passed no key at all — Electron warns on `icon: undefined`).

Two robustness fixes found while confirming the first: the connect
window is now destroyed on the next tick rather than mid-handler, so the
IPC reply is actually delivered instead of leaving the caller's `await`
pending; and a shell window whose main frame fails to load returns to
the connect screen carrying the reason, rather than stranding the user
on a blank window with no way back.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ymlink

**The app introduced itself as "openstation-electron-adapter".** That is
the right name for an npm package describing an extension, and the wrong
name for the thing in the macOS menu bar and dock — the user did not
install an adapter, they installed OpenStation. Unpackaged Electron
falls back to `package.json`'s `name`, so `productName` is set and
`app.setName()` runs at module scope, before `app.getPath( 'userData' )`
derives a directory from it. The About panel is set too; it does not
read the name on its own.

One consequence: the state file moves to an `OpenStation` folder, so a
site address entered under the old name is forgotten once.

**The ⋯ menu row was missing because the adapter was never installed.**
It lives inside the OpenStation plugin directory and WordPress does not
look for plugins in nested folders, so — like every other extension in
this repo — it needs a symlink into `wp-content/plugins/`. Nothing was
broken: the app connected, the desktop loaded, and the one script that
registers the row was simply never enqueued.

That failure is silent and looks exactly like a bug in the feature, so
the README and docs now lead with the symlink, name it as the step the
whole feature depends on, and say to check it first when "Send to your
Mac" is missing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…hat ate it

Two real bugs, both found by driving the running app over the DevTools
protocol rather than by reading the code again.

**The adapter never registered anything.** `wp_register_script` was
called with a plain `$in_footer = true`, while the `openstation` handle
it depends on is registered `strategy => defer`. A declared dependency
orders the *tags*, not the *execution*: a classic script runs the moment
the parser reaches it, which is before any deferred script. So the
adapter ran first, found no `window.wp.os`, logged one line, and gave
up. The app connected, the desktop loaded, and the ⋯ menu row was
simply absent.

Fixed by matching the shell's strategy — the only spelling that
actually honours the dependency. `src/index.ts` additionally waits for
`wp.os` instead of giving up on the first look, so a future change to
how either script is enqueued cannot silently switch the feature off
again. A PHPUnit test now asserts the adapter's strategy equals the
strategy of the handle it depends on.

**Freed windows rescued themselves into a second desktop.** The
chromeless bridge treats a top-level chromeless page as an accident — a
stale bookmark, a bad redirect — and strips the flag to reload as
classic admin, because such a page has no admin bar and therefore no
way out. A freed native window is the one legitimate top-level
chromeless page, so three seconds after opening it stripped its own
flag, bounced through the portal, and painted an entire second
OpenStation desktop inside a window meant to hold one screen.

The escape hatch now asks whether anything *claims* the page rather
than only whether it is framed: `window.openStationChromelessHost`
opts out, the freed-window preload sets it, and it is a JS global
rather than a query flag precisely so it survives in-window navigation.
Core still knows nothing about Electron — an embed or a kiosk can claim
a page the same way.

Also: solo mode hides the WordPress admin bar. It is the desk's chrome,
not the window's, and its "Switch to Classic Admin" link would navigate
a freed window out of itself.

Verified end to end in the running app: the row reads "Send to your
Mac"; clicking it opens a real OS window on the chromeless page for an
iframe window and on solo mode for a native one; the row toggles to
"Bring back into OpenStation"; closing the native window docks it back
and restores the in-shell copy.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…re to go

Three things the desktop host was missing, all found by driving it.

**Real title bars, with the desktop name.** Freed windows used macOS's
`hiddenInset` style, which left floating traffic lights over the
content and no name anywhere the OS could show one — an anonymous
rectangle in Mission Control. They now wear the real title bar. The two
window kinds need opposite handling to make that read correctly: a
native window pins its OpenStation name, because solo mode renders
inside `index.php` and Trash would otherwise rename itself "Dashboard";
an iframe window follows the page but drops WordPress's
` ‹ Site — WordPress` suffix, which is a tab's business, not a
window's. The adapter's own title bar is hidden there — one title bar,
not two saying the same word.

**The browser can now use the desktop.** "Send to your Mac" worked only
inside the app, which is backwards: the app is the thing that can give
you native windows, and the browser is where most people work. The app
now also runs a loopback agent, hands its coordinates to the site on
each handshake, and the site prints them back into the admin page. Four
gates, because "HTTP server on your machine" deserves them: bound to
127.0.0.1, a bearer token (which also forces a CORS preflight, so a
hostile page cannot fire a blind request), one allowed origin, and URLs
re-checked against the paired site before any window opens. The agent
URL is validated as loopback on the way IN rather than trusted on the
way out, and the token is emitted only while a host is actually live.

The result is shaped as the same `DesktopHostBridge` the preload
provides, so `boot()` cannot tell the two apart — one implementation of
the here-or-there rules rather than two that drift. It has no push
channel, so it polls while anything is freed and stops the moment the
last window comes back; an idle tab makes no requests.

After login the app asks where to work. Both answers connect: "Use my
browser" opens the site in the default browser and keeps this process
running as the agent, which is what that tab will call.

**A second window opened inside a freed one had nowhere to go.**
Launching a game from a freed Games window put the game in the same
solo shell, where the CSS stretches every window to fill the viewport —
so it covered the hub, with no dock and no window controls to get back.
Two windows in the DOM, one visible, no error anywhere. Now anything
opening a window there opens a new native window instead, as does
`window.open()` anywhere in the app, except for off-site URLs and
`desktop_mode_classic=1` — the ⋯ menu asking for the browser by name.

That exposed a core gap: solo mode resolves a window by id, and a
game's window is minted at launch rather than registered, so
`?openstation_solo=os-game-<id>` now launches the game. And when an id
resolves to nothing, solo mode paints nothing rather than substituting
the current page — the substitute was itself a window the host had
never seen, which the forwarder then forwarded, turning one request
into two windows.

Verified end to end: freed Games hub → launch Inkfall → one new OS
window, playable, titled "Inkfall"; hub still showing the hub.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two follow-ups from using it.

**A game launched from a freed Games window flashed inside it before
opening on the desktop.** The forwarder was closing the local copy
*after* the host accepted it, so the window painted for a frame or two
first. It now never paints: solo mode emits an inline rule hiding every
window that is not the one it was booted for, with the id baked in.

That has to be CSS, and it has to be inline. JavaScript can only act
once the window exists, which is a frame too late; a static stylesheet
cannot express it, because the selector depends on which window this
is. `visibility` rather than `display`, so a hidden-but-laid-out window
still has a size — canvas windows need one to initialise without
dividing by zero on their way to being closed.

**Starting the app no longer needs a browser refresh.** The tab probed
for the local agent once, at load, so launching the app afterwards did
nothing until you reloaded. It now retries on tab focus and on ⋯ menu
open — both user actions, so nothing polls — and re-fetches the pairing
from `GET /host` rather than reusing the one baked into the page, since
the agent's port is ephemeral and a restarted app listens somewhere the
page has never heard of.

For the row to land under the pointer rather than on the next click,
core gained two small generic pieces: `HOOKS.WINDOW_MENU_OPENED`, fired
after a menu paints, and an open menu that repaints itself when the
action registry changes. Together they let any plugin answer "what
should this menu contain?" with something asynchronous — a probe, a
permission check — and still have its row appear immediately.

Verified: a page loaded with no app running and no pairing on the
server, the app started afterwards on a port that page had never seen,
one ⋯ click, and "Send to your Mac" appeared — then freed and docked a
window over that connection.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The app menu already read "About OpenStation" / "Quit OpenStation" —
macOS builds those from `app.getName()` — while the bold title beside
them said **Electron**, and the Dock showed Electron's atom.

Both come from the running **application bundle**, not from
`app.setName()`: `CFBundleName` in its `Info.plist`, and the bundle's
own icon. In development there is no OpenStation bundle, because
`electron .` runs `node_modules/electron/dist/Electron.app`. A packaged
build never had this problem — its bundle *is* OpenStation.app — so
this is a development affordance plus the packaging config that makes
the shipped app right.

- **Icons.** `build/icon.icns` and a 1024px `build/icon.png`, generated
  from `.wordpress-org/icon.svg` — the same artwork the plugin ships to
  WordPress.org — and committed, so packaging never needs a rasteriser
  installed. electron-builder derives the Windows `.ico` and the Linux
  sizes from the PNG. The *runtime* icon (the Dock, and window icons off
  macOS) is a second copy under `app/src/renderer/`, because
  electron-builder treats `build/` as packaging resources and leaves it
  out of the bundle — the earlier path would have resolved to nothing in
  a packaged app.
- **`app.dock.setIcon()`** for the running process, wrapped so a
  cosmetic failure can never stop the app starting.
- **`scripts/brand-dev-bundle.mjs`** renames the local Electron bundle
  and swaps its icon, on `npm start` and `postinstall`. Writing into
  node_modules is the one thing that can work here, and it is contained:
  idempotent, macOS-only, never fatal, and `npm install` regenerates the
  file it edits. Skip it and you get an app called Electron — the status
  quo, not a break.

Verified: macOS now reports `LSDisplayName="OpenStation"` for the
running process, which is the string the menu bar draws.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`npm audit` reported 21 (1 critical, 19 high). Most were fixable, and
the ones that mattered most were not the loudest.

**Electron 33 → 43.** This is the only dependency that ships, so its
advisories are the only ones describing code a user actually executes —
a dozen of them, including use-after-frees and an ASAR integrity
bypass. Everything in the audit's "critical" and most of its "high"
count came from the packager instead, which is the reverse of the
attention they deserve.

**electron-builder 25 → 26**, which clears the critical `node-tar`
finding and the `app-builder-lib` / `builder-util` / `node-gyp` tree
beneath it. Packaging-only, never shipped.

**`minimatch` pinned through `overrides`**, because
`@wordpress/eslint-plugin` holds an old `@typescript-eslint` that holds
a vulnerable one. Nothing here feeds user input to a glob, but a
one-line override beats waiting upstream.

Two remain, both the same root cause: esbuild's dev-server advisory,
reached through vite 5. This package never runs a dev server — `vite
build` only — so the vulnerable surface is not reachable, and clearing
it means vite 7+, which would diverge from the plugin's own pinned
vite 5. The README now records that reasoning, and the distinction the
audit itself does not draw: what ships, and what runs on a developer's
machine.

A ten-major Electron jump is not something to assume, so the whole flow
was re-run on 43: log in, free a window, launch a game from a freed
Games hub into its own OS window, and the app still identifies itself
to macOS as OpenStation.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@AllTerrainDeveloper

Copy link
Copy Markdown
Collaborator Author

@claude can you review this?

@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown

Claude finished @AllTerrainDeveloper's task in 8m 43s —— View job


Review

  • Read repo guidelines (AGENTS.md / CLAUDE.md)
  • Gather context: read diff vs origin/trunk
  • Review core footprint changes (window-actions registry, menus.ts, solo-window.php, chromeless-bridge.php)
  • Review extension security surface (agent-bridge.ts, host.php, agent.ts, connection.ts, preload scripts, main.ts)
  • Review docs updates for consistency
  • Post final review

Solid, well-documented PR — the core footprint is genuinely small (window-actions registry, WINDOW_MENU_OPENED, solo mode) and follows the extension-not-core boundary described in the PR body. One real security bug in the extension, plus a smaller doc/dead-code mismatch in core.

🔴 Security: agent token can be exfiltrated via ordinary same-window navigation

extensions/openstation-electron-adapter/app/src/main.ts guards new-window opens (setWindowOpenHandlerisSameSiteUrl(), both for the shell window at L256 and freed windows at L484), but registers no will-navigate/will-redirect handler on either webContents. A plain same-tab navigation of the shell BrowserWindow — a stock wp-admin link without target, a plugin admin page doing location.href = …, a meta-refresh — is not intercepted at all.

app/src/preload/shell.ts exposes window.openStationDesktopHost via contextBridge; that global stays reachable after such a navigation regardless of the new document's origin. Its handshake() forwards straight to ipcMain.handle(CHANNELS.INVOKE_HANDSHAKE, …) in main.ts (~L645), which calls Connection.handshake() (app/src/lib/connection.ts L129-163) with no validation of args.restUrl against the paired siteUrl — unlike routeNewWindow() and FreeWindows.isAllowedUrl(), which both run isSameSiteUrl() on the URLs they handle.

Connection.request() (connection.ts L263-286) then does fetch(${restUrl}/${namespace}/host/handshake, { body: JSON.stringify({ ...describe() }) }), and describe() (main.ts L731-742) includes agentUrl and agentToken — the loopback bearer secret.

Net effect: get the shell window to same-tab-navigate anywhere (no XSS on the real site required, just an un-targeted link), and the attacker page can call openStationDesktopHost.handshake({ restUrl: 'https://attacker.example', nonce: 'x' }); the main process POSTs the real agent token to that attacker origin. This directly contradicts the PR's claim that "the token reaches the page only while a host is actually live" — it can now reach an arbitrary origin. Confirmed tests/connection.test.ts never asserts restUrl is checked against a configured site, so the gap isn't exercised anywhere.

Impact is bounded — the token is only useful against the victim's own loopback agent, which still enforces its own single-allowed-origin + CORS-preflight check, so this isn't full remote takeover — but it's a real secret leak of the pairing token to an attacker-chosen server, worth fixing before this graduates past "Experimental":

  1. Add will-navigate/will-redirect handlers on both the shell and freed-window webContents, mirroring the isSameSiteUrl() check setWindowOpenHandler already applies.
  2. Validate args.restUrl in Connection.handshake() against the currently-paired siteUrl (or the URL the shell window was actually loaded from) before ever calling describe()/sending the token.

Fix this →

🟡 Core: owner-based live unregistration for window actions is documented but unreachable

src/window-actions/registry.ts implements unregisterWindowActionsByOwner() and docs/javascript-reference.md documents owner as "Script handle, for live unregistration on deactivation" (matching the registerWindowAction JSDoc in src/desktop.ts). But unlike title-bar buttons, commands, or settings tabs — each of which has a src/*/server-sync.ts module that receives the plugin-deactivation payload diff and calls the corresponding unregister*ByOwner() — there is no src/window-actions/server-sync.ts, no PHP-side openstation_register_window_action_script(), and no payload key for window actions at all (confirmed via grep across src/menu-refresh-apply.ts and includes/core/payload.php). That matches AGENTS.md's note that window actions, like palettes, are "JS-registered-only" — but the doc table and JSDoc both promise deactivation behavior that nothing in this PR actually wires up. unregisterWindowActionsByOwner() is currently dead code outside its own unit test.

Not a bug (nothing crashes), but worth either: (a) wiring a real server-sync.ts if window actions get a PHP registration path in a follow-up, or (b) softening the owner field's docs to "reserved for future live-unregistration" so plugin authors don't rely on behavior that doesn't exist yet.

Nitpicks

  • extensions/openstation-electron-adapter/src/agent-bridge.ts's token comparison and app/src/lib/agent.ts's bearer check use plain !== string comparison rather than a constant-time compare — low real-world risk given the CORS/origin gating in front of it, but cheap to harden with crypto.timingSafeEqual if the extension wants defense in depth.
  • Everything else checked out well: loopback-only binding, CORS-preflight-forcing bearer token, single-origin allowlist, and outbound URL re-validation (isSameSiteUrl() for routeNewWindow/FreeWindows) are all implemented as claimed and are well covered by tests/site-url.test.ts and tests/agent.test.ts.

AllTerrainDeveloper and others added 2 commits August 10, 2026 12:03
Review of #542 found a way to get the local agent's bearer token to an
attacker-chosen server. The hole was slightly wider than reported, and
so is the fix.

**A preload survives navigation.** `setWindowOpenHandler` guarded
`window.open()` and nothing else, so an un-`target`ed link, a
`location.href =`, a meta refresh or a 302 walked the shell window to
any origin it liked — and the new document still held
`window.openStationDesktopHost`, because a preload's `contextBridge`
exports belong to the *window*, not to the document that happened to be
in it. `handshake()` was reachable from there, and
`Connection.handshake()` sent `describe()` — agent URL and bearer token
included — to whatever `restUrl` it was handed.

Both ends are closed:

- **`will-navigate` / `will-redirect` on the shell window and every
  freed window**, held to the same rule popups already followed. The
  decision itself is `navigationVerdict()` in `lib/site-url.ts` rather
  than inline in `main.ts`, because everything in this app worth
  testing lives in `lib/` and a rule enforced only inside Electron is a
  rule nobody can test.
- **`Connection.handshake()` checks `restUrl` against the paired site**
  before `describe()` is called at all, so the token cannot leave even
  if a window somehow did. A refused handshake also clears the previous
  good root and stops the timer — a page that navigated away does not
  get to keep the heartbeat it inherited.

**The first navigation chain is deliberately unguarded.** A site
answering `example.com` with a redirect to `www.example.com` is
ordinary canonicalization, and refusing it would not degrade the
feature, it would break the connection outright. So the shell's first
chain runs free and whatever it settles on becomes the site; everything
after is held to that. This also fixes a footgun that was already
there: such a site could never free a window, because
`FreeWindows.isAllowedUrl` made the same host assumption with nothing
to establish it. The cost is that a site signing in through an external
identity provider sends that hop to the browser, which the docs now say.

Two more of the same family, found while in there:

- **The `certificate-error` handler compared `url.startsWith( site )`**,
  and `https://example.com` is a prefix of
  `https://example.com.attacker.example`. That offered the user a
  "Continue anyway" button for a lookalike domain's bad certificate.
  Host-to-host now.
- **The agent's bearer check is `crypto.timingSafeEqual`.** Worth more
  than defence in depth: the `Origin` gate in front of it is a header,
  and a header is something any program on the machine can simply
  write, so on this machine the token is the only real gate — and one
  that answers a byte at a time is not much of one.

The review also flagged a token comparison in `src/agent-bridge.ts`.
There isn't one; that module only sends `Authorization: Bearer …`.

+14 tests: the REST-root check against a wrong host, a lookalike
suffix, a userinfo-prefixed host, a scheme downgrade, a non-http
scheme and a wrong port; that a refused handshake does not keep an
earlier one beating; the navigation verdict for each class of
destination; and that a token which is merely a prefix of the real one
is as uninformative as a wild guess.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`WindowActionDef.owner` was documented as "script handle, for live
unregistration on deactivation", the JSDoc said the same, and
`unregisterWindowActionsByOwner()` was there to do it. Nothing called
it. Review of #542 caught the gap: window actions had no server-side
opt-in, so there was no payload key to diff, so there was no moment at
which a departing plugin could be noticed. The docs described behaviour
no code performed, and plugin authors would have tagged `owner` and
believed it.

This is the fix shape AGENTS.md prescribes for exactly this class of
gap, cloned from title-bar buttons rather than invented:

- **`openstation_register_window_action_script( $handle )`** in
  `includes/window-actions.php`, with the registry, the flush hook and
  the payload builder that pattern carries.
- **`serverWindowActionScripts`** in the menu payload and the shell
  config, so it rides the same live-refresh path the chromeless bridge
  already emits on plugin install/activate.
- **`src/window-actions/server-sync.ts`**, wired into
  `createApplyPayload()` and into boot. Activation loads the opted-in
  script; deactivation sweeps every action carrying the departing
  handle.

A row appears in the next ⋯ menu that opens — and in a menu that is
*already* open, because the registry's subscribe fan-out repaints it,
which is the same mechanism that lets a plugin answer
`WINDOW_MENU_OPENED` asynchronously. An action with no `owner` still
survives past deactivation until the next reload: the same graceful
backwards-compat commands and title-bar buttons offer, and the reason a
plugin written before today keeps working.

The other option was to soften the docs to "reserved for future use".
That would have been honest, and it would also have left the ⋯ menu the
one extension point whose rows outlive the plugin that put them there.

Docs say what the bargain is: `owner` alone is inert, and the PHP call
is what gives it something to match against. `docs/examples/window-action.md`
grew the pairing, since a plugin author reading only the JS side is
exactly who this misled.

+12 tests: the PHP registration, its payload resolution, the
unresolvable-handle diagnostic, the action, and that the handle
actually reaches `openstation_build_menu_payload()`; on the JS side,
injection, idempotence, the owner-tagged sweep with an untagged
survivor, subscriber notification so an open menu repaints, and the
live-refresh contract in `menu-refresh-apply`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@AllTerrainDeveloper

Copy link
Copy Markdown
Collaborator Author

@claude can you review once more?

@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown

Claude finished @AllTerrainDeveloper's task in 1m 51s —— View job


Review (round 2)

  • Check what changed since the previous review
  • Re-review main.ts / connection.ts navigation + handshake validation
  • Re-check the owner-based live-unregistration doc/dead-code nitpick
  • Sweep rest of diff for anything new
  • Post updated review

Both issues from the previous pass are fixed, and both fixes go further than the minimum ask.

🔴 (Resolved) Token exfiltration via same-tab navigation

Fixed in 2f9e2f2, closing it on both ends rather than picking one:

  • guardNavigation() (app/src/main.ts:467-495) now hooks will-navigate and will-redirect on both the shell window (main.ts:272) and every freed window (main.ts:564), routing through the new pure navigationVerdict() in app/src/lib/site-url.ts (allow same-site, send other http(s) to the OS browser, block anything else). The shell's first navigation chain is deliberately left unguarded via the settling flag (main.ts:271-281) so ordinary canonicalization redirects (example.comwww.example.com) don't break first connect — settling flips false on the first did-navigate, so this is a one-shot window, not a standing bypass.
  • Connection.handshake() (app/src/lib/connection.ts:158-179) now rejects any restUrl that fails isSameSiteUrl() against the paired site before describe() (and its bearer token) is ever built, and clears the previous good restUrl + stops the timer on rejection so a navigated-away window can't keep riding an inherited heartbeat.
  • Two related hardenings found in the same pass: certificate-error now compares host-to-host via isSameSiteUrl() instead of url.startsWith(site) (was vulnerable to example.com.attacker.example), and the agent's bearer check (app/src/lib/agent.ts:66-74) now uses crypto.timingSafeEqual with a length check first.
  • 14 new tests across site-url.test.ts, connection.test.ts, agent.test.ts cover the wrong-host/lookalike-suffix/userinfo-prefix/scheme-downgrade/wrong-port cases and the "rejected handshake doesn't keep an earlier root alive" behavior.

This matches the reported gap and closes it correctly — confirmed by reading the diff, not just the commit message.

🟡 (Resolved) owner-based live unregistration

Fixed in c616d03, cloning the exact pattern AGENTS.md prescribes (matching title-bar-buttons):

  • openstation_register_window_action_script( $handle ) in includes/window-actions.php, a serverWindowActionScripts payload key, and src/window-actions/server-sync.ts (createWindowActionRegistrySync) wired into createApplyPayload() and boot.
  • server-sync.ts mirrors title-bar-buttons/server-sync.ts exactly: tracks loadedHandles, unregisters via unregisterWindowActionsByOwner() for handles that drop out of the incoming payload, loads newly-arrived scripts otherwise. unregisterWindowActionsByOwner() now has a real caller.
  • Docs (docs/hooks-reference.md, docs/javascript-reference.md, docs/examples/window-action.md) updated to describe the actual opt-in, not just the JS-side promise.

Nothing new flagged

Skimmed the rest of the diff (docs, desktop-host.md additions, README) for consistency with the code changes — all in sync with the new navigation/handshake behavior (the "first hop runs free" caveat is now documented for external-IdP sign-in flows). No new issues spotted.

CI was still running at review time (CI workflow in progress; PR Preview Build passed) — worth confirming green before merge, but nothing in this pass suggests it won't be.

@mmtr

mmtr commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Reviewed the code and ran through the manual testing, both the solo path in a browser and the full app with the adapter activated.

Findings below, split into what came up while testing and what came up reading.

Found while testing

Solo mode leaks the desktop icon rail

solo.css hides .os-icons, which is the Classic grid. The Spatial layout paints into .os-files-layer (src/desktop-files/layer.ts:238) and that selector is not in the list. The docblock at src/desktop-icons.ts:210 says it plainly: "the Spatial layout paints <os-tile> placements in the files layer, and on a stock install that second one is what the user is actually looking at".

It is masked whenever a window paints, because body.os-solo .os-window fills the viewport at inset 0 with no radius. I hit it with ?openstation_solo=nope, which paints nothing. It is also a flash on every solo boot before the window lands.

Worth fixing rather than shrugging at, because the file's own reasoning for display: none over visibility is "so nothing keeps a layout box, an animation frame, or a pointer target". The files layer keeps all three, including drag and drop targets. One line:

body.os-solo .os-files-layer,
Screenshot 2026-08-10 at 13 18 31

The shell window shows the raw WordPress tab title

Freed windows get the treatment: createFreedWindow prevents page-title-updated and runs screenNameFrom(), so the OS title bar reads "Posts". openShellWindow() has no such handler, so the main window reads Plugins ‹ WordPress Develop — WordPress.

The PR says "Windows keep their names", and the main window is the one you look at most.

Screenshot 2026-08-10 at 13 27 24

macOS Dock tooltip says "Electron"

scripts/brand-dev-bundle.mjs rewrites CFBundleName and CFBundleDisplayName and swaps the icon. The icon lands, the name does not: LaunchServices caches the bundle's display name, so the plist edit alone does not move the Dock tooltip. Either refresh lsregister from the script or soften the claim in its docblock, since right now it says it fixes the Dock name and in practice it does not.

Screenshot 2026-08-10 at 13 27 09

Pink outline on the dock and the side rail, app only

Happens in the app, not on the same site in Chrome, so it is not site state. I could not track it down. --os-dock-border defaults to near white at 10%, the only shipped desktop theme sets it to white at 8%, and neither this PR's solo.css nor the adapter's solo-host.css applies to the shell page. The small pink indicator dots are separate and correct, those are --os-dock-item-outline.

Any idea what would differ under Electron here?

Screenshot 2026-08-10 at 13 26 31

From reading the code

The ⋯ menu subscription outlives its window

src/window/menus.ts:87 subscribes to the registry so the menu repaints while open, and only closeActionsMenu() unsubscribes. destroy() at src/window/index.ts:3384 tears down _boundOnDocumentPointerDown but not this one, so a programmatic win.close() with the menu open leaves a live listener holding the window and a detached panel. Click driven closes hide it, because the pointerdown capture handler closes the menu first. One line next to the existing cleanup.

Off-site auth redirects

After settling, navigationVerdict sends any off-site navigation to the browser, so a site using Jetpack or WordPress.com SSO, or an off-site 2FA step, bounces the user out mid login and strands the app on the login screen. Worth a note in docs/desktop-host.md at least.

The related edge: if that first chain does land off-site, store.set( 'siteUrl', landed ) makes that origin the agent's allowedOrigin and the navigation guard's allowlist. Narrowing the settle to the same registrable domain would close it without breaking the example.com to www.example.com case the comment is protecting.

CI does not run the adapter's JS suite

ci.yml has per-extension jobs for popup-siege and feed-buddy, including git diff --exit-code -- extensions/desktop-mode-feed-buddy/assets/js to catch a stale committed bundle. The adapter adds none, so its 155 tests, its typecheck, its lint, and the check that assets/js/electron-adapter.js still matches src/ all run only locally. The PHP side is fine, tests/phpunit/tests/electronAdapterHost.php loads the extension directly and rides the core suite.

No .distignore

bin/package-extensions.sh zips git ls-files -co minus .distignore, and only popup-siege ships one. Without it the adapter's plugin zip carries app/, tests/, scripts/, the vite, vitest and eslint configs, build/icon.icns and a 12,961 line lockfile. WordPress only needs the plugin file, includes/, assets/ and the README.

wp.os.electron skips registerNamespace()

src/index.ts does os.electron = api directly, which bypasses the RESERVED_NAMESPACE_KEYS collision check in src/api/facade.ts:757. The file docblock says the adapter "uses nothing but OpenStation's public API", and registerNamespace is that API. It also means a second plugin claiming electron silently wins.

Smaller

  • Raw fetch in src/agent-bridge.ts. The loopback calls are defensible since they are not site requests, but fetchPairing() hits the site REST root and should go through wp.os.fetch with { silent: true, source: … }, or carry the documented eslint-disable with a reason. feed-buddy's eslintrc has the rule, the adapter's does not.
  • Generic solo mode has a silent failure that the forwarder only fixes under a host. An embed or kiosk, which the docs invite, opens a second window and gets an invisible unreachable one with no error. Worth a caveat in docs/desktop-host.md, or a console warning from core when a second window opens in solo mode.
  • The agent token never rotates. Store.agentToken() generates once and persists, and it is mirrored into user meta and printed into every admin page. The capability is bounded to opening a same site window, so impact is low, but the port already changes per launch and rotating the token with it is close to free.
  • LocalAgent.readBody counts raw.length in UTF-16 code units rather than bytes, so MAX_BODY is up to three times looser than documented for multi-byte input. It also writes a response after req.destroy().
  • routeNewWindow matches desktop_mode_classic=1 as a substring of the whole URL, so a path or fragment containing it routes to the browser. searchParams would be exact.
  • certificate-error offers "Continue anyway" for any localhost URL, not only the configured site, which is narrower than the reasoning in its own docblock.
  • src/window-actions/server-sync.ts never clears loadedUrls on deactivation, so deactivate then reactivate leaves the script cached and the owner swept actions unregistered until a reload. This is inherited verbatim from title-bar-buttons/server-sync.ts, so a family wide follow up rather than something to hold this PR for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants