Skip to content

Harden site REST proxy against SSRF via redirects and origin drift - #4256

Closed
wojtekn wants to merge 1 commit into
trunkfrom
fix-rest-proxy-ssrf
Closed

Harden site REST proxy against SSRF via redirects and origin drift#4256
wojtekn wants to merge 1 commit into
trunkfrom
fix-rest-proxy-ssrf

Conversation

@wojtekn

@wojtekn wojtekn commented Jul 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Related issues

How AI was used in this PR

Claude Code investigated the CodeQL alert, traced the taint flow from the /sites/:id/rest endpoint into the shared REST proxy, implemented the hardening, and added unit tests. All changes were reviewed by the author.

Proposed Changes

The shared "proxy a REST request to a running local site" helper takes a caller-supplied request. It's the transport behind @wordpress/api-fetch/core-data, reachable both over IPC (desktop) and over HTTP via POST /sites/:id/rest on the local CLI server (studio ui / agent surface) — so its input is not fully trusted. It already rejected paths/URLs that escape the selected site's REST root, but two gaps remained that let a request be redirected off that origin, carrying the site's auth cookie and REST nonce with it:

  • The proxied fetch used redirect: 'follow', so a 3xx response from the site could silently send the authenticated request to an arbitrary host, with the response fed back to the caller.
  • The final fetch relied on the URL object built during validation, leaving no guarantee at the point of use that host/protocol were still server-controlled.

This PR adds a final SSRF barrier: the request target is rebuilt from the server-controlled loopback base, carrying over only the already-validated path/query/fragment, and redirects are no longer followed. There is no behavior change for legitimate same-origin REST requests.

Testing Instructions

  • npm test -- apps/studio/src/lib/tests/wordpress-rest-api.test.ts — passes (5/5), including new cases asserting the request always targets the loopback origin and that redirects are not followed.
  • npm run typecheck — passes.

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?
@wojtekn @claude
Harden site REST proxy against SSRF via redirects and origin drift
028a33f 
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@wpmobilebot

wpmobilebot commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

📊 Performance Test Results

Comparing 028a33f vs trunk

app-size

Metric trunk 028a33f Diff Change
App Size (Mac) 1355.58 MB 1355.57 MB 0.01 MB ⚪ 0.0%

site-editor

Metric trunk 028a33f Diff Change
load 1073 ms 1067 ms 6 ms ⚪ 0.0%

site-startup

Metric trunk 028a33f Diff Change
siteCreation 6968 ms 7021 ms +53 ms 🔴 0.8%
siteStartup 2348 ms 2352 ms +4 ms ⚪ 0.0%

Results are median values from multiple test runs.

Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff)
@wojtekn
wojtekn requested review from a team and youknowriad July 20, 2026 11:50
@youknowriad

youknowriad commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Exercise the app's REST-backed views (e.g. pages/posts lists) against a running local site and confirm they load normally.

How is this currently used in Studio?
@wojtekn

wojtekn commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

@youknowriad I removed this part - it isn't used in the Desktop UI directly, but seems to be used in the browser-based studio ui to request the WordPress API.

However, it looks like the only consumer is WordPressDataProvider, and WordPressDataProvider is not rendered, imported, or referenced anywhere. Is it a Studio Desk leftover?
@youknowriad

youknowriad commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

However, it looks like the only consumer is WordPressDataProvider, and WordPressDataProvider is not rendered, imported, or referenced anywhere. Is it a Studio Desk leftover?

Yes, Desk definitely used this but I don't think I personally ever used this in agentic UI, so probably a left over. I think there's value in it though, if we want to offer some tailored flows (like showing the design system of a site) in the UI but since there doesn't seem to be a use-case, we could get rid of it.
@wojtekn

wojtekn commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for sharing more context and for the good catch. I will close this one and will open another PR for the cleanup.
@wojtekn wojtekn mentioned this pull request Jul 21, 2026
Merged
1 task
@wojtekn

wojtekn commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Closing in favor of #4279
@wojtekn wojtekn closed this Jul 21, 2026
wojtekn added a commit that referenced this pull request Jul 22, 2026
@wojtekn @claude
Remove dead WordPressDataProvider and site REST proxy scaffolding (#4279
99ad204 
)

## Related issues

- Related to #4256 (see note below)

## How AI was used in this PR

Claude Code verified each symbol was unused independently (`grep -rn`
across `apps` + `packages`, checking for dynamic imports and string
refs) before deleting anything, traced the full `fetchSiteRest`
dependency chain, confirmed the sibling `readLocalMedia` path was still
live and left it untouched, and ran lint/typecheck/tests. All changes
were reviewed by the author.

## Proposed Changes

The `@wordpress/core-data` data layer (`WordPressDataProvider`) was
built for Studio Desktop but is no longer wired into any UI, as Studio
Desktop was removed. Because it was the only caller of the
`fetchSiteRest` transport, that entire chain was dead: the connector
method and its `local`/`ipc`/`hosted` implementations, the desktop IPC
plumbing, the `POST /sites/:id/rest` endpoint on the local CLI server,
and the shared REST proxy in `@studio/common`.

## Testing Instructions

- `npm run typecheck` — passes across all workspaces.
- `npm test -- apps/studio/src/tests/ipc-handlers.test.ts
apps/ui/src/data/core
apps/ui/src/ui-classic/components/session-view/conversation` — 32/32
pass, including the `readLocalMediaFile` handler test confirming the
retained media path is intact.
- A repo-wide grep for the removed symbols (`fetchSiteRest`,
`WordPressDataProvider`, `SiteRestRequest`, etc.) returns no matches.

## Pre-merge Checklist

- [x] Have you checked for TypeScript, React or other console errors?

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants

@wojtekn @wpmobilebot @youknowriad