Only open http and https addresses externally by juanmaguitar · Pull Request #121 · WordPress/contributor-toolkit

juanmaguitar · 2026-08-05T16:40:05Z

Closes #116.

url:open passed whatever it received to shell.openExternal, which hands an address to whatever application the OS has registered for its scheme — a wider action than "show this page in the browser". A file: address opens an arbitrary local path in its associated application (on Windows that can mean running it rather than viewing it), and any other registered scheme is reachable the same way.

Every caller passes an http/https address today, so nothing misuses it. It matters as the second half of a chain: the renderer displays content the app does not author, and the URL the app auto-opens on server start is parsed out of the Playground server's stdout. This guard is the step that keeps any future influence over that string from becoming an action on the contributor's machine.

What changed

src/external-url.js (new) — ALLOWED_URL_SCHEMES = ['http:', 'https:'], plus isAllowedExternalUrl() and openExternalUrl(). The scheme is read off a parsed new URL() rather than the raw string, so casing and leading whitespace (FILE:, file:) normalize before the comparison instead of being a way around it; an address Node can't parse is refused rather than handed to the OS to interpret. What gets opened is the parser's own href, not the caller's string. That second half matters as much as the allow-list: the URL parser strips tabs and newlines from anywhere in the input, including the middle of the scheme, so ht\ntp://example.com/x validates as http: — and forwarding the raw text would hand the OS an address nothing had checked. For every caller here the two forms differ only by the trailing slash the parser adds to a bare origin.
src/main.js — the handler delegates to that module, passing shell.openExternal and an onRefused that logs through the existing logEvent (scope url). A refusal is visible in the log file people attach to bug reports, not silent, so a future caller that trips the guard is diagnosable. The logged address is truncated, since by hypothesis it is the influenced input.

The logic lives in its own module, per the repo convention (src/bind-loopback.js), so both sides of the guard are testable without an Electron process.

No user-facing behaviour changes: every existing caller passes http/https.

Testing

test/external-url.test.cjs — 11 tests, using a stub in place of shell.openExternal so "did this reach the OS?" is an assertion rather than something the test takes on trust:

The four address shapes the app actually passes (Trac, the feedback form, the site on an ephemeral loopback port, wp-admin) still open, and reach the stub in their normalized form.
A control-character case (ht\ntp://example.com/x) pins the normalization: it opens http://example.com/x, not the string carrying the newline. This test fails against the first commit of this PR, which forwarded the raw input.
file:///etc/passwd and file:///C:/Windows/System32/cmd.exe never reach the stub. The Windows one is the case that matters most: the OS association for a .exe is "run it".
javascript:, data:, mailto:, and OS-registered third-party schemes (ms-msdt:, vscode:) are refused too.
Junk input ('', whitespace, null, undefined, a number, an object, an array, 'not a url') is refused rather than thrown.
A refused address cannot forge a log line: control characters are escaped rather than passed through, so an address carrying a newline can't close its entry and open another one in the app's own timestamp-and-scope format. Truncation runs after escaping, since escaping is what decides the final length.
One test pins the allow-list itself, so widening it has to be a deliberate change that shows up in a diff.

Suite: 158 passing, 0 failing, on node --test and again on Electron's bundled Node via npm run test:electron. npm run lint is clean across the repo.

To verify by hand

Nothing in the UI can reach the refusal path by design, so the manual pass is about confirming the allowed side is unaffected. On a signed build from this branch:

Start a site — it should still auto-open in the browser as before.
Click the site URL and the wp-admin link in the site header — both open.
Open a site with the database tool and click through to adminer — opens.
Generate a patch and click the core.trac.wordpress.org link in the "Next steps" text — opens.
Click the feedback form link — opens.
Optional, to see the guard fire: from the renderer devtools console, run window.api.openExternal('file:///etc/passwd'). It should resolve false, nothing should open, and the log file (Help → the log path) should contain a url line reading refused to open file:///etc/passwd — only http:, https: are allowed.

Also updates .github/instructions/code-review.instructions.md: it named this handler as its known-open calibration example, which stops being true here. Replaced with what the fix teaches, so the next review looks for both halves rather than re-reporting something already closed.

Rebased on trunk after #122/#125/#127; the review standard's calibration example moved with it to .github/instructions/.

🤖 Generated with Claude Code

github-actions · 2026-08-05T17:27:42Z

Findings: 0 (🔴 0 · 🟡 0 · 🔵 0) — no [fix here] or [follow-up] findings across the five dimensions.

This PR replaces the direct shell.openExternal(url) call in url:open with openExternalUrl() from the new src/external-url.js, which is exactly the fix the review rules' worked example describes: it allow-lists http:/https: by parsed scheme (not string matching) and passes the OS the parser's own href rather than the caller's raw string, closing the control-character split between what's validated and what's opened. .github/ai-review-rules.md is updated in the same PR to describe this as resolved, which matches the shipped code.

Verified:

No other shell.openExternal call site exists (src/main.js:671 is the only one); preload.js's openExternal bridge is unchanged and still routes through the single url:open handler.
normalizeExternalUrl reads parsed.protocol off a new URL() result (catches parse failures, non-string/empty input) rather than inspecting the raw string, and returns parsed.href — the two halves the rules call out.
Refusals are logged via the existing logEvent, truncated to 120 chars, so a malicious/attacker-influenced address doesn't blow up the log file.
New test/external-url.test.cjs covers the app's real call sites (Trac, feedback form, loopback site/admin URLs), the control-character bypass, file:/javascript:/data:/other-scheme refusal, and junk input — exercising both the fix and the regression it prevents.
No new spawns, listeners, dependencies, or IPC surface; contextBridge exposure is unchanged.

Style / process notes

None.

The `url:open` handler passed whatever it received to `shell.openExternal`, which hands an address to whatever application the OS registered for its scheme. That is a wider action than "show this page in the browser": a `file:` address opens an arbitrary local path in its associated application — on Windows that can mean running it rather than viewing it — and any other registered scheme is reachable the same way. Every caller passes an http/https address today, so nothing misuses it. It matters as the second half of a chain: the renderer displays content the app does not author, and the URL it auto-opens on server start is parsed out of the Playground server's stdout. This guard is the step that keeps any future influence over that string from becoming an action on the contributor's machine. The check lives in its own module so both sides of it can be tested without an Electron process — `shell.openExternal` in the app, a recording stub in the tests. A refusal is logged rather than dropped, so a future caller that trips the guard shows up in the log file instead of just doing nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Checking one string and opening another is not a check. The URL parser strips tabs and newlines from anywhere in the input, including the middle of the scheme, so 'ht\ntp://example.com/x' validates as http: while `openExternal` would receive an address carrying a newline for the OS parser to resolve by its own rules. Forwarding `parsed.href` closes the gap between the two: the address that was checked is the address that gets opened. For every caller in the app the two forms are identical but for the trailing slash the parser adds to a bare origin. The rules file listed this handler as its known-open calibration example, which stops being true here. Replaced with what the fix teaches — the allow-list and the normalization are two halves, and the second is the one that is easy to miss in the next handler that takes a URL or a path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The refused address goes into the log verbatim, and electron-log passes newlines through unchanged, so an address carrying one could close its line and open another in the app's own timestamp-and-scope format. A log that can be made to describe events that never happened is worse than no log — and being the diagnosable trace for a caller that trips the guard is the whole reason the refusal is logged at all. Control characters are now escaped rather than dropped, so the line still says what the caller actually sent, and truncation runs afterwards, since escaping is what decides the final length. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Restricts external URL opening to normalized HTTP/HTTPS addresses, closing issue #116.

Changes:

Adds URL validation, normalization, and safe refusal logging.
Routes url:open through the new guard.
Adds comprehensive security regression tests.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
`src/external-url.js`	Implements the URL security boundary.
`src/main.js`	Applies the guard to IPC requests.
`test/external-url.test.cjs`	Tests allowed, refused, malformed, and forged inputs.
`.github/instructions/code-review.instructions.md`	Updates review guidance for URL validation.

Closes #129. The house pattern is to lift a handler's decision-making into a small module and test that module directly. Every test in `test/` talks to the module; none talks to the handler. So the connection between the two is untested: delete the `openExternalUrl` call from the `url:open` handler, call `shell.openExternal` in its place, and the whole suite stays green while the app loses the guard #121 added. For a module that formats a string that is a small gap. For one that exists to refuse something, the connection is the entire point. ## What changed **`test/ipc-wiring.test.cjs`** (new) — loads `src/main.js` in an ordinary `node --test` process with `require('electron')` replaced by a recording stub, captures the callbacks main.js registers with `ipcMain.handle`, and invokes them with the delegate module stubbed. It runs the real handler body, so it fails on a deleted call, a renamed export, or a bypass added beside it — not just on missing text. Nothing in the startup path runs: the stub's `app.whenReady()` returns a promise that never settles, so requiring the file registers handlers and does nothing else. No Electron process, and the suite stays a plain `node --test`. The harness lives in the test file rather than a helper, because `node --test` discovers every `.cjs` under `test/` and would report a helper as a file of its own — and the suite is deliberately run without a path (see the comment in `scripts/run-tests-electron.cjs`). **`src/main.js`** — the `electron-store` import moves from module load into `getStore()`. It has to: an ESM `import()` is invisible to the `Module._load` hook, so at require time it pulled in the real `electron` behind the stub's back and rejected. Deferring it also removes a startup unhandled rejection — a promise nobody was awaiting yet, at the one moment the app has no way to report it. Concurrent callers still share a single import, and a failure still surfaces to the handler that asked. No behaviour change: no handler can run before a window exists. ## The coverage guard The last test requires every channel main.js registers to be classified, each with a reason: - **wired** — asserted by a test above. - **no delegation** — electron-store reads and writes, a dialog, a path check. There is no module call to delete. - **unwired invariants** — `npm:kill`, `playground:start/stop`, `playground-web:start/stop`. These call no module; they hold the spawn and kill invariants inline instead (the child env built by hand rather than by `buildChildEnv`, `shell: false`, `detached` on POSIX, `child.kill()` where the npm paths use `killChildTree`). Deleting `detached:` from `playground:start` still leaves this suite green. Recorded rather than folded into "no delegation", because fixing it is a decision about main.js — see follow-ups. - **not reachable** — `site:status` and `wordpress:setup` do delegate, but behind a store read and a network clone respectively. `ipcMain.on` channels are recorded too, so a future one-way handler cannot arrive invisible to the guard. A new handler fails the suite until someone classifies it, and a renamed one fails it from the other side. That is what makes this a fix to the pattern rather than seven one-off tests. ## Testing 14 tests: `url:open` → `external-url` (both with a spy and end-to-end through the real module — `file:` refused and logged, `https:` opened as the normalized href), `git:worktree-dirty` / `git:discard-changes` / `git:update-trunk` / `sites:add` → `trunk-update`, the three patch channels → `ensureAutocrlf` and `git-update.normalizeEol` against a real throwaway repository, `npm:install` / `npm:run-script` → `npm-runner` (the environment `buildChildEnv` returned is the one that reaches `spawn`; a failing close consults `shouldRetryWithRelaxedEngines`, and a `true` actually starts a second attempt), and the `before-quit` sweep → `kill-tree`. Each assertion was mutation-checked — the acceptance criterion in the issue is "a handler that stops using its module fails the suite", so each mutation was applied alone and reverted: | mutation | result | | --- | --- | | `url:open` calls `shell.openExternal` directly | 2 tests fail | | `ensureAutocrlf` deleted from `sites:add` | 1 fails | | `ensureAutocrlf` deleted from the patch path | 2 fail | | `child.kill()` instead of `killChildTree` on quit | 1 fails | | child env hand-rolled instead of `buildChildEnv` | 2 fail | | `normalizeEol` dropped from both sides of the diff | 1 fails | | a new unclassified `ipcMain.handle` | guard fails | | a new `ipcMain.on` channel | guard fails | Suite: **172 passing, 0 failing**, on `node --test` and again on Electron's bundled Node via `npm run test:electron`. `npm run lint` clean across the repo. ## The second commit The first commit passed locally and broke `test/electron-node-version.test.cjs` on both platforms in CI. Worth writing down, because the trap is invisible on any machine that has already run the app. `node_modules/electron/index.js` resolves the binary path at module scope and spawns Electron's installer when it is missing. Requiring the package is therefore not inert on a cold checkout: it starts a download into the same `dist` directory that `electron-node-version.test.cjs` spawns the binary out of, and `node --test` runs files concurrently. That test got a half-written framework — `segment '__TEXT' load command content extends beyond end of file` on macOS, a second "Downloading Electron binary..." on Windows. Locally the binary is already there, the require is a file read, and nothing happens. Two paths reached it, both from the new file: - Stubs are built by merging over the real module, so stubbing `src/logging.js` means requiring it, and it requires `electron`. That require ran before the `Module._load` hook was installed. It now happens inside it, and the hook also covers `electron/*` subpaths and anything resolving into the package. - `sites:add` was allowed to run past its delegation into `getStore()`, whose `import('electron-store')` does `import {app} from 'electron'` through the ESM loader — where `Module._load` does not apply and no hook can help. The test now stops at the delegation, which is all it was ever asserting, the same way the patch-channel test does. The 14th test fails if the real package is loaded at all, since neither path is visible on a machine where the binary is present. Verified in both directions by removing `node_modules/electron/path.txt`: before the fix the suite prints "Downloading Electron binary..." and that test fails; after it, neither happens. No manual pass needed: nothing here ships in the app except the lazy store import, which no handler can observe. ## Review Ran the review in `.github/instructions/code-review.instructions.md`, judgement pass dispatched to a subagent with the diff and the standard. **1 `[fix here]` · 3 `[follow-up]`.** Fixed here: - 🔵 tests — the npm tests reach the real `runNpmWithEngineRetry`, which calls `ensureNodeShimDir()` and leaves `$TMPDIR/electron-node-shims-<pid>/` behind on every run, on both runtimes. It is module-local and cannot be stubbed; an `after()` hook now sweeps it. Also taken, though reported as follow-ups, because they were two lines each: - 🔵 tests — the guard only saw `ipcMain.handle`; `on` was a silent no-op in the stub. Now recorded and asserted. - 🟡 tests — "no delegation" was documented as "deleting a line from it could not disarm a guard", which was not true of the five spawn/kill handlers. Split into the `UNWIRED_INVARIANTS` list described above, with what each one holds inline. Left as follow-ups: - 🟡 tests — actually wiring those five handlers, either by giving them the modules the npm handlers use or by asserting their spawn options directly. It is a change to main.js, not to this test file, so it does not belong in this PR. - 🔵 security — `sites:delete` (`src/main.js:658`) does `fse.remove()` on a renderer-supplied path with no check that it is one of the registered sites. Pre-existing and unrelated to this change; filed as #133. Style notes, neither wrong today: `assert.equal(spawned.options.env, env)` asserts object identity, which is deliberate (a `deepEqual` would pass for a handler that rebuilt an identical object by hand) but would fail a future `env: {...buildChildEnv(), X: 1}`. And the `'child_process'` stub key now also registers under `'node:child_process'`, so switching to the prefixed spelling in main.js cannot silently unhook it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Closes #133. Deleting a site removed whatever directory the request named. `sites:delete` took a `sitePath` straight from the renderer and called `fse.remove` on it — recursively, best-effort, error swallowed — without first checking that the path was one the app registered. The `sites` array in the store is the app's own record of what it created or adopted, so it is the boundary: a delete request naming anything outside it is refused and logged, and no directory is removed. This is the second half of the same chain as #121 (opening external addresses) — the window displays content the app does not author, and removing a directory is the least recoverable action reachable from it. ## What changed - **`src/site-registry.js`** (new) — an Electron-free module mirroring `external-url.js`: - `isRegisteredSite(sitePath, sites)` — pure exact-match check (same convention `sites:add`/`sites:delete` already use). - `describeRefusedSite(sitePath)` — safe log formatter: escapes control chars/newlines and truncates, so a crafted path can't forge a second entry in the log contributors attach to bug reports. - `deleteRegisteredSite(sitePath, { sites, forget, remove, onRefused })` — the handler body with injected effects; refuses (logs, no store mutation, no removal) when the path isn't registered. - **`src/main.js`** — `sites:delete` delegates to that wrapper. Refusals log via `logEvent('sites', 'refused to delete … — not a registered site')`, mirroring the `url:open` wiring. Removal stays best-effort. - **`test/site-registry.test.cjs`** (new) — 8 tests: registered/unregistered/junk paths, exact-match (parent and child of a registered site are rejected), truncation, and log-forging. ## Notes - The handler now returns `false` on refusal instead of always `true`. The renderer (`onDelete`) ignores the result and just refreshes, so the UI is unaffected — and this path is unreachable from the UI today. - Scoped to `sites:delete`, where the outcome is genuinely unrecoverable (a recursive `fse.remove`). The other site-path handlers use their path as a git worktree or a process cwd, with far smaller blast radius, so they are deliberately left as-is; the guard is a reusable module if one of them ever needs it. ## Done when - [x] A delete request naming a path the app does not have in its registry is refused, and the refusal shows up in the log. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

juanmaguitar and others added 3 commits August 6, 2026 10:24

juanmaguitar requested a balanced review from Copilot August 6, 2026 08:24

juanmaguitar force-pushed the juanmaguitar/issue-116-restrict-which-url branch from d3d56b3 to aadd1ec Compare August 6, 2026 08:24

Copilot started reviewing on behalf of juanmaguitar August 6, 2026 08:25 View session

Copilot AI reviewed Aug 6, 2026

View reviewed changes

juanmaguitar mentioned this pull request Aug 6, 2026

DO NOT MERGE — probe: does Copilot apply the review standard? #128

Closed

juanmaguitar merged commit b7277d3 into trunk Aug 6, 2026
4 checks passed

juanmaguitar deleted the juanmaguitar/issue-116-restrict-which-url branch August 6, 2026 08:34

This was referenced Aug 6, 2026

Test that the IPC handlers use the modules they delegate to #131

Merged

Exclude coding-agent directories from generated patches #36

Merged

juanmaguitar mentioned this pull request Aug 6, 2026

Refuse to delete a path that is not a registered site #138

Merged

1 task

Navigation Menu

Uh oh!

Only open http and https addresses externally - #121

Only open http and https addresses externally#121
juanmaguitar merged 3 commits into
trunkfrom
juanmaguitar/issue-116-restrict-which-url

juanmaguitar commented Aug 5, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 5, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

juanmaguitar commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed

Testing

To verify by hand

Uh oh!

github-actions Bot commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

juanmaguitar commented Aug 5, 2026 •

edited

Loading

github-actions Bot commented Aug 5, 2026 •

edited

Loading