Skip to content

fix: normalize the trailing slash on an inserted site API namespace - #595

Open
dcalhoun wants to merge 15 commits into
fix/register-core-media-upload-middlewarefrom
fix/normalize-site-api-namespace-slash
Open

fix: normalize the trailing slash on an inserted site API namespace#595
dcalhoun wants to merge 15 commits into
fix/register-core-media-upload-middlewarefrom
fix/normalize-site-api-namespace-slash

Conversation

@dcalhoun

@dcalhoun dcalhoun commented Aug 21, 2026

Copy link
Copy Markdown
Member

What?

Normalizes the trailing slash when apiPathModifierMiddleware inserts the site API namespace into a request path.

Why?

A namespace configured without a trailing slash runs into the following path segment, producing a malformed URL: /wp/v2/sites/123 + posts becomes /wp/v2/sites/123posts, and the request 404s.

Both forms are supported input. WordPressRESTURL (iOS) and RestUrlBuilder (Android) normalize them identically for native-issued requests, with tests pinning the unslashed case. This middleware was the one consumer not applying that rule.

No in-repo host currently emits the unslashed form, but setSiteApiNamespace is public API, so a host app can pass either.

How?

Strips trailing slashes from the namespace and appends exactly one before inserting it, matching RestUrlBuilder's trimEnd('/') + "/".

Testing Instructions

Covered by automated tests. Verified that the new test fails without the fix, and that it is the only failure.

Accessibility Testing Instructions

Not applicable — no user interface changes.

dcalhoun and others added 11 commits August 20, 2026 15:26
@dcalhoun @claude
refactor: rename local media upload middleware
8c2404e 
The local `mediaUploadMiddleware` shadowed the same-named export from
`@wordpress/api-fetch`, so the file read as though core's post-process
retry behavior was registered when only the draft post ID stripping was.

Rename it to `stripDraftPostIdMiddleware` to describe what it does. No
behavior change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
@dcalhoun @claude
fix: register core's media upload post-process retry middleware
25a172f 
When `wp_generate_attachment_metadata()` fails server-side (commonly a PHP
memory_limit or max_execution_time fatal on large images), WordPress
returns a 5xx carrying an `x-wp-upload-attachment-id` header. Core's
`mediaUploadMiddleware` recovers from this by retrying
`POST /wp/v2/media/<id>/post-process` up to five times, then deleting the
orphaned attachment if every attempt fails.

That middleware was never registered, so these uploads surfaced as failures
that left an orphaned gray attachment behind and duplicated the attachment
on retry.

`apiFetch.use` unshifts, so registration order is the reverse of execution
order. Core's middleware is registered before the native one so that it
runs after it, and below auth, namespacing, and the root URL so the
`post-process` requests it issues through `next` stay authenticated and
correctly addressed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
@dcalhoun @claude
fix: correct the site API namespace guard
620e8e8 
`new RegExp('(' + [].join('|') + ')')` is `/()/`, which matches every
string, so `alreadyHasSiteNamespace` was unconditionally true whenever a
site configured no namespace — as self-hosted sites do.

That was load-bearing rather than merely benign: it suppressed a rewrite
that would otherwise interpolate `siteApiNamespace[0]` — `undefined` for an
empty namespace — into every path, producing `/wp/v2/undefinedposts`. Gate
the rewrite on a configured namespace so the guard no longer has to, and
escape the namespaces so each is matched literally.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
@dcalhoun @claude
fix: let native media uploads reach the post-process retry
098f29b 
The native upload path circumvented core's post-process retry entirely, so
a metadata fatal on a delegate-handled upload stayed a permanent failure
with an orphaned attachment left behind. Two things blocked it:

- `relayResponse` rebuilt the response with a hardcoded Content-Type,
  dropping `x-wp-upload-attachment-id` — the header core's middleware needs
  to identify the attachment to retry. Relay it (via an allowlist, since
  the body is re-sent with a recomputed length) and expose it through CORS,
  without which the WebView cannot read it cross-origin regardless.
- `nativeMediaUploadMiddleware` always parsed the body, so it never yielded
  the `Response` that core's middleware inspects. Honor `parse: false` by
  resolving or rejecting with the `Response` itself.

`MediaUploadResponse` gains a `headers` property on both platforms,
defaulted so existing host callers are unaffected.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
@dcalhoun @claude
docs: record when the media upload retry can recover
7831679 
The retry depends on reading `x-wp-upload-attachment-id`, which is only
possible same-origin or where the server exposes it via CORS. Record which
combinations recover, and why there is no client-side fallback, so the iOS
direct-upload case is not mistaken for a bug in this registration.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
@dcalhoun @claude
test: add a wp-env media upload failure simulator
5a8d7d7 
Reproduces the server-side image processing fatal the post-process retry
recovers from, so the middleware can be exercised locally without a large
image or a resource-starved host.

Adapted from the approach in WordPress/gutenberg#17858, with the random
failure rate replaced by an explicit mode (`recover`/`always`/`off`) so both
the recovery and the exhaust-and-delete paths are reproducible. The mode is
an option rather than per-request state, since the upload and each retry are
separate requests and a native-server upload carries no browser cookie.

The plugin sets the 500 itself: a real fatal under FPM surfaces as a 500,
but the Playground runtime returns 200, which the retry would ignore.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
@dcalhoun @claude
test: add a make target for the media failure simulation mode
f1d205f 
Reading or flipping the mode meant a curl invocation with an inline
credentials lookup, which is easy to get wrong mid-debug and silently
no-ops if it fails — leaving a passing upload that looks like the retry
never fired.

Wrap it in `make wp-env-media-failure [MODE=off|recover|always]`, matching
the existing VAR=value convention and the thin-target-plus-bin-script
pattern. Each precondition reports the fix that applies to it: missing
credentials, a rejected 401 (stale after a Playground restart), an
unreachable server, and an unregistered endpoint.

Also document the orphaned-server and 401 cases in troubleshooting; both
came up repeatedly while testing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
@dcalhoun @claude
fix(wp-env): allow the Android bundled-build editor origin
96d4576 
A bundled Android build serves the editor from the site's host without a
port (`http://10.0.2.2`), which was not in the allowlist, so its REST
requests were rejected before reaching WordPress.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
@dcalhoun @claude
docs: correct when a direct upload can recover on Android
1a68e38 
Testing a bundled Android build against wp-env disproved the claim that
Android direct uploads are same-origin and therefore recover.
`GutenbergView` derives the asset domain from the site's host, and `host`
drops the port — so the editor at `http://10.0.2.2` is cross-origin with a
site at `http://10.0.2.2:8888`, and the attachment ID header stays
unreadable.

The claim holds only when the site runs on the scheme's default port, as
production sites do. Say that, rather than implying every Android site
recovers.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
@dcalhoun @claude
test: never fail a delete in the media failure simulator
a588074 
`always` mode fatalled on every sub-size pass, and core's `force=true`
delete path runs sub-size handling too — so the editor's orphan cleanup
fatalled as well, leaving the orphan behind. The retry logic was correct;
the simulator refused the cleanup it had correctly requested.

Exempt deletes, including the `POST` + `X-Http-Method-Override: DELETE`
form api-fetch sends, so the bare request method alone is not enough to
identify one.

Also document that a simulated fatal aborts the request before WordPress
adds CORS headers, so these responses surface as CORS errors rather than
readable 500s.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
@dcalhoun @claude
fix: relay the media orphan cleanup through the native upload server
02a0a17 
When every post-process retry fails, core's middleware deletes the orphaned
attachment. A cross-origin editor cannot make that request: api-fetch
tunnels DELETE as a POST carrying `X-HTTP-Method-Override`, and core's
`rest_allowed_cors_headers` omits that header, so the browser blocks it at
preflight and the orphan survives.

Route media deletions through the loopback upload server instead, which
sets its own CORS policy and already permits DELETE. The middleware
intercepts before api-fetch's `httpV1` adds the override header, so what
reaches the native server is a plain DELETE.

Both servers gain a single narrow route — `DELETE /media/<id>` with a
numeric ID — rather than a general proxy, matching the existing
`POST /upload`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
@github-actions github-actions Bot added the [Type] Bug An existing feature does not function as intended label Aug 21, 2026
@wpmobilebot

wpmobilebot commented Aug 21, 2026 •
edited
Loading

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/595")

Built from 499a649

@dcalhoun @claude
fix: let a delegate handle its own media deletions
f591f1f 
`handleDelete` went straight to the default uploader, unlike `handleUpload`
which offers the work to the delegate first. A host whose `uploadFile`
uploads to its own media service holds an ID only it can resolve, so
deleting through the default uploader would address the wrong site.

Add `deleteFile(attachmentId:)` to `MediaUploadDelegate` on both platforms,
defaulted to nil so existing hosts are unaffected, and try it before falling
back. Rename the handler to `handleMediaDelete`, since it deletes an
attachment rather than an upload and no longer mirrors `handleUpload`'s
signature.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
@dcalhoun
dcalhoun force-pushed the fix/normalize-site-api-namespace-slash branch from e5c18e4 to dc79b90 Compare August 21, 2026 13:18
dcalhoun and others added 2 commits August 21, 2026 09:22
@dcalhoun @claude
docs: trim implementation detail from the wp-env media failure guide
9732e1e 
Drop the two notes explaining how the simulator produces its 500 and why a
fatal response reads as a CORS error — implementation detail that belongs in
the plugin, not the guide. Drop the orphaned-server and stale-credential
troubleshooting entries; those are environment problems to address on their
own. Also drop a comment restating what the adjacent condition already says,
and reword the make target's help text.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
@dcalhoun @claude
refactor: extract the native media upload helper
ebb67a5 
`nativeMediaUploadMiddleware` mixed dispatch with the whole upload
implementation, so adding the deletion path left the two handled
asymmetrically — one extracted, one inline.

Extract `nativeMediaUpload` alongside `nativeMediaDelete`, both returning
null when a request is not theirs, leaving the middleware as a short
dispatcher. No behavior change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
@dcalhoun @claude
fix: normalize the trailing slash on an inserted site API namespace
499a649 
`apiPathModifierMiddleware` inserted `siteApiNamespace[0]` verbatim, so a
namespace configured without a trailing slash ran into the following
segment: `/wp/v2/sites/123` + `posts` produced `/wp/v2/sites/123posts`.

Both forms are supported input — `WordPressRESTURL` and `RestUrlBuilder`
normalize them identically for native-issued requests, with tests pinning
the unslashed case. This middleware was the one consumer not applying that
rule.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
@dcalhoun
dcalhoun force-pushed the fix/normalize-site-api-namespace-slash branch from dc79b90 to 499a649 Compare August 21, 2026 13:23
@dcalhoun
dcalhoun marked this pull request as ready for review August 21, 2026 20:06
@dcalhoun
dcalhoun requested a review from crazytonyli August 21, 2026 20:15
@dcalhoun dcalhoun mentioned this pull request Sep 4, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@crazytonyli crazytonyli crazytonyli approved these changes

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dcalhoun @wpmobilebot @crazytonyli