goose icon indicating copy to clipboard operation
goose copied to clipboard

[MCP-UI] Proxy and Better Message Handling

Open aharvard opened this issue 1 month ago • 7 comments

Summary

This PR upgrades @mcp-ui/client to better handle messages from MCP-UI iframes and also decouples the MCP-UI iframe CSP from Goose's top-level CSP set for Electron.

Our top-level CSP previously blocked external fonts, stylesheets, scripts, and media. We now use an MCP-UI Proxy with a permissive CSP inside a sandboxed iframe for security. This creates a parent-child iframe architecture in Goose.

Demo

Now, Goose can load external fonts inside MCP-UI iframes and MCP-UI resources can send request-render-data to get the theme value from goose (as shown here in mcp-ui docs)

https://github.com/user-attachments/assets/9f9d53f2-642e-484e-a1c3-6874bad0ef51

Now, Goose can load external video and audio media files (unmute video below for sound).

https://github.com/user-attachments/assets/659aa3e4-1ab0-4bdd-a12c-88e1fbcb3bd5

Type of Change

  • [x] Feature
  • [x] Bug fix
  • [x] Refactor / Code quality
  • [ ] Performance improvement
  • [ ] Documentation
  • [ ] Tests
  • [ ] Security fix
  • [ ] Build / Release
  • [ ] Other (specify below)

Testing

Manual testing with aharavrd mcp as shown in videos above.

Related Issues

No issue or discussions to link.

[!NOTE] This work enables MCP-UI interoperability between Goose and ChatGPT by addressing two blockers:

  1. Sandboxed iframe with permissive CSP - Implements ChatGPT's approach of using a 3rd party sandboxed origin with a lax CSP, enabling capabilities like remote script execution. This follows MCP-UI's documented pattern (used by Postman).

  2. Render data support - Adds full render data implementation, which is currently missing in Goose.

This unblocks @idosal and @liady from targeting Goose for cross-platform MCP-UI development with the Apps SDK.

aharvard avatar Oct 30 '25 21:10 aharvard

.bundle

aharvard avatar Oct 31 '25 13:10 aharvard

@aharvard looks good! Glad this also uses the SDK functionality instead of custom Goose code. Good proxy implementation 👍. I think we should probably make this code reusable from the SDK somehow at some point.

liady avatar Oct 31 '25 19:10 liady

Thanks for looking @liady!

I think we should probably make this code reusable from the SDK somehow at some point.

^ yeah, you read my mind!

aharvard avatar Oct 31 '25 19:10 aharvard

Hey @michaelneale, @alexhancock, @jamadeo, @zanesq — I think this PR is stable enough for a final review know. Made some changes to main.ts that spins up the proxy server and want to make sure that looks good.

aharvard avatar Nov 03 '25 17:11 aharvard

I think looks good - do we want to have pnpm file checked in (not sure what pros/cons are - always liked pnpm but probably need thoughts from @zanesq when around)

michaelneale avatar Nov 06 '25 22:11 michaelneale

I think looks good - do we want to have pnpm file checked in (not sure what pros/cons are - always liked pnpm but probably need thoughts from @zanesq when around)

Whoops! No need for pnpm (altho that is our Block standard now) I accidentally committed it out of habit. Just need the package-lock.json and will make sure that's correct before we merge. Will report back.

aharvard avatar Nov 06 '25 23:11 aharvard

@michaelneale, thanks for catching the accidentally committed pnpm-lock, I've remove it. The package-lock looks good to me.

aharvard avatar Nov 07 '25 00:11 aharvard

@aharvard I asked copilot for a security review and it came back with the following. Can you take a look?

GitHub Copilot Chat Assistant — quick security review of the changes in this PR.

Short answer: yes — there are several security risks and hardening opportunities in the new MCP‑UI proxy and related changes. Below are the main concerns (priority order), why they matter, and concrete mitigations you can apply.

High / Important
- IPC handler is unrestricted
  - What I saw: ipcMain.handle('get-mcp-ui-proxy-url', () => { ... }) returns the proxy URL to any renderer caller.
  - Risk: any code running in any renderer (including untrusted/compromised web content) can call this and learn the proxy URL and then trigger the proxy page to be loaded. This lowers the barrier for malicious use.
  - Mitigation: restrict the handler to trusted callers — inspect the event (ipcMain handler receives an event param) and validate event.senderFrame/frameId or event.sender.getURL() to ensure only your trusted renderer contexts can call it; or only register the handler per-window (register on BrowserWindow.webContents) instead of globally.

- Header‑token based auth + broad header injection surface
  - What I saw: a random token is generated in main and webRequest.onBeforeSendHeaders injects that token header for requests to localhost:port across sessions / all webContents.
  - Risk: any renderer (including malicious frames/scripts) that can initiate requests to the proxy host/port will have the token automatically injected by Electron, enabling access to the proxy. This effectively lets any in‑app content that can make requests to localhost use the proxy. If a renderer is compromised, that compromise can access the proxy. Also custom clients (curl) could query the proxy if they obtain the token.
  - Mitigation:
    - Narrow the injection scope — only add the header for specific webContents / partitions / origins (check contents.getURL() or webContentsId) rather than every session. Do not add for arbitrary webContents.
    - Consider binding token injection to a specific partition or to a whitelist of webContents origins.
    - Consider additional verification server‑side (e.g., mutual TLS, or require an ephemeral nonce per webContents) if threat model requires it.

- IPC returns port but not token — port disclosure risk
  - What I saw: get-mcp-ui-proxy-url returns http://localhost:port/mcp-ui-proxy.html (port visible).
  - Risk: port alone is not sufficient without token, but exposing port makes it easier to target the service and attempt token discovery or exploitation.
  - Mitigation: apply the IPC restrictions above; do not broadly expose the URL to untrusted renderers. Consider returning a URL that is only usable in the trusted renderer or a signed URL/nonce if needed.

Medium
- Permissive CSP in mcp-ui-proxy.html
  - What I saw: meta CSP is very permissive (script-src *, 'unsafe-inline', 'unsafe-eval', blob:, style-src * 'unsafe-inline', connect-src *, frame-src *, etc.).
  - Risk: if that page is ever able to include remote scripts or is modified, it becomes an easy avenue for executing arbitrary remote code in an app context. The proxy page is top-level for embedded MCP-UIs; loose CSP reduces isolation.
  - Mitigation: tighten CSP to the minimum required. Prefer 'self' and specific allowed origins, avoid 'unsafe-inline' and 'unsafe-eval' unless strictly required. Limit frame-src to specific origins or only allow same-origin embedding when possible.

- postMessage usage uses '*' or overly broad origins
  - What I saw: postMessage(event.data, '*') used in several places; parent->inner sometimes uses urlOrigin (good) but inner->parent uses '*' or forwards with '*' in some branches.
  - Risk: forwarding messages with '*' can leak messages to unexpected recipients or allow confused deputy style attacks.
  - Mitigation: use explicit origin values when posting (use known origin strings) and validate the origin of incoming messages before relaying. Add a message format and origin checks.

- Express static serving and directory exposure
  - What I saw: express.static(staticPath) serves the whole static dir; code logs readdirSync(staticPath) contents.
  - Risk: if static contains files that should not be exposed, they will be served. Logging contents may expose filenames in logs.
  - Mitigation: serve only the specific files/routes you need (e.g., app.get('/mcp-ui-proxy.html', ...) and serve subpaths with explicit allowlist). Remove or reduce directory content logging; avoid logging sensitive paths.

Low / Additional improvements
- ALLOWED_HOSTNAMES excludes IPv6 loopback
  - Add ::1 to ALLOWED_HOSTNAMES (or use localhost/loopback check that handles IPv6).

- Relying on Origin/Referer for security
  - What I saw: validateOrigin checks origin/referer headers and in production accepts missing or file://.
  - Risk: headers can be spoofed by non-browser clients. Token mitigates but combine defenses: keep token, scope injection, and optionally bind token to a particular webContents/session.
  - Mitigation: don't treat origin header as sole auth; use it as an additional check only.

- Potential token leakage via redirects or forwarded requests
  - If you later add proxying behavior (fetching remote content server-side), be careful not to forward the proxy token to upstream servers. Current code doesn't proxy requests, so low risk now.

- webRequest header injection nuances
  - Ensure you normalize header name casing and avoid clobbering legitimate headers. Use a header name unlikely to collide (currently x-mcp-ui-proxy-token is good).

- srcdoc/sandbox behavior
  - For raw HTML mode you set sandbox attribute from payload; ensure the sandbox never allows 'allow-same-origin' unless you explicitly want it. If you allow same-origin, html in srcdoc can access cookies/localStorage of the proxy page.

Concrete short list of fixes I recommend first
1. In ipcMain.handle, validate the caller (use the first argument event and check event.sender.getURL() or event.senderFrame) and only return the URL to trusted windows.
2. Limit webRequest header injection to a whitelist of webContents (e.g., only the main window and explicit windows you create), not every session/webContents created.
3. Tighten CSP in mcp-ui-proxy.html and avoid 'unsafe-inline'/'unsafe-eval' where possible.
4. Replace postMessage('*') with explicit allowed origins and validate event.origin in message handlers before relaying.
5. Add ::1 to ALLOWED_HOSTNAMES and consider a stricter check for loopback addresses.
6. Serve only intended files (explicit routes) instead of blanket express.static, and remove file listing logs.

zanesq avatar Nov 10 '25 17:11 zanesq

Thanks @zanesq! Me & Goose+Claude4.5 took y'all's feedback and jammed on it. Here's our update!


Summary of Security Improvements

We've addressed 4 of the 6 recommended security fixes, with 2 items intentionally skipped due to architectural constraints:

✅ Implemented (4 items)

1. IPC Handler Origin Validation

  • Restricted get-mcp-ui-proxy-url IPC handler to only respond to trusted renderer processes
  • Validates caller origin using event.sender.getURL()
  • Rejects requests from untrusted sources with logging

2. WebContents Whitelisting for Header Injection

  • Implemented a whitelist (trustedWebContentsIds) to control which webContents receive the proxy auth token
  • Only webContents with validated type ('window'/'webview') and trusted origin (file:// or dev server) are whitelisted
  • Automatic cleanup when webContents are destroyed
  • Prevents compromised iframes/popups from accessing the proxy

5. IPv6 Loopback Support

  • Added ::1 to ALLOWED_HOSTNAMES for IPv6 loopback compatibility
  • Ensures proxy works on IPv6-enabled and IPv6-only systems

6. Explicit File Serving

  • Replaced blanket express.static() with explicit route for mcp-ui-proxy.html
  • Removed directory listing logs that could leak file information
  • Added 404 handler for unauthorized path access
  • Follows principle of least privilege

⏭️ Skipped (2 items)

3. CSP Tightening

  • Why skipped: The mcp-ui-proxy.html creates nested srcdoc iframes that inherit the parent CSP. These iframes host arbitrary MCP-UI content that requires permissive policies to function. The existing multi-layered security (token auth, origin validation, webContents whitelisting, iframe sandboxing) provides adequate protection.

4. postMessage Origin Restrictions

  • Why skipped: The message relay requires '*' target origins for file:// contexts (where origins are 'null') and srcdoc iframes (which lack meaningful origins). Attempted restrictions broke the relay logic. The existing security layers provide adequate protection, and source validation is already performed.

Security Posture

The implemented changes significantly reduce the attack surface by:

  • Preventing unauthorized renderers from discovering or accessing the proxy
  • Limiting token injection to explicitly trusted webContents
  • Reducing information disclosure through explicit file serving
  • Improving IPv6 compatibility

The two skipped items represent cases where architectural requirements (supporting arbitrary MCP-UI content in sandboxed iframes) necessitate more permissive policies, but the defense-in-depth approach (multiple security layers) maintains adequate protection.

aharvard avatar Nov 10 '25 21:11 aharvard

@michaelneale can you take another look? (still show you as requesting changes) ty!

aharvard avatar Nov 10 '25 21:11 aharvard

I chatted with @DOsinga about moving the server into the Rust backend, and @alexhancock will perform the maneuver. Thanks, guys!

aharvard avatar Nov 13 '25 21:11 aharvard

Thanks @alexhancock for moving the proxy server from Electron to the backend in https://github.com/block/goose/pull/5749

@DOsinga and @michaelneale — can we get another look? If everything is in order, I'd like to merge!

aharvard avatar Nov 17 '25 20:11 aharvard

would that live mcp sampling test have anytihng to do with this - or just bad luck?

michaelneale avatar Nov 18 '25 00:11 michaelneale

would that live mcp sampling test have anytihng to do with this - or just bad luck?

FYI gpt-5-mini sampling test kept failing. I got it to pass on the second manual re-run.

All checks passed.

Ready to merge?

aharvard avatar Nov 18 '25 16:11 aharvard

Ran copilot security review and this came up as potentially important. Wondering if worth addressing @alexhancock or @DOsinga

**Secret in query string (critical)**
You place the secret in the proxy URL query (…/mcp-ui-proxy?secret=...), then load that page and—if it embeds an external target iframe—the external target can receive the proxy page URL as the referrer (document.referrer / Referer header). That means the secret can leak to third-party sites when the proxy iframe loads external URLs. This is a high-risk leak.
Recommendation: never put long-lived secrets in the URL. Use one of:
Deliver short-lived session token via a Set-Cookie (HTTP-only) from the server and read it via State on the proxy endpoint.
POST the secret to the proxy page (i.e. open a small HTML page that receives the secret via POST and stores it in JS memory), or have the server render the proxy page without the secret in the URL and provide the secret to the iframe via a safer channel (cookie, server session).
Use a one-time ephemeral token generated server-side for that UI session, not your main secret key.
At minimum, set Referrer-Policy to no-referrer on the proxy HTML and send Referrer-Policy response header so the browser won't send the full URL as the referer — but this is still weaker than avoiding secrets in URLs.

**Solution**
Remove secret from the query string. Provide the secret via:
a short-lived token (server-generated) embedded in the proxy page or a cookie, or
by having Electron call the proxy page and then postMessage the secret into it after load (so it’s never in the URL).

zanesq avatar Nov 18 '25 19:11 zanesq

Copilot also mentioned this, created a PR for this change, @aharvard is testing it out

Add a Referrer-Policy header/meta tag (e.g. no-referrer) on the proxy HTML to help prevent accidental leak if secret is ever in the URL.

zanesq avatar Nov 18 '25 19:11 zanesq