fix(ios): gate the media upload server on the site root too - #624
fix(ios): gate the media upload server on the site root too#624jkmassel wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
[Type] Bug
XCFramework BuildThis PR's XCFramework is available for testing. Add the following to your .package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/624")Built from dadd64b |
`startUploadServer` checked only `authHeader.isEmpty`, though the comment directly above it said the uploader "needs a site root and an auth header". Android has checked both since it landed. An iOS host that configured an auth header but no `siteApiRoot` therefore started a server whose every request failed at the URLSession layer, instead of falling back to the WebView upload path the way Android does. `siteApiRoot` is a `URL` here where Android types it as a `String`, so `isEmpty()` has no direct equivalent — "addressable" is spelled as scheme and host both being present. Put the check in `MediaServerCredentials` rather than inline. `EditorViewController` is `#if canImport(UIKit)`, so it does not exist on the macOS host and nothing in it is reachable from the test suite — which is how the two platforms diverged here unnoticed. Outside the gate, the predicate gets five tests, including both arms of the site-root check.
72b6329 to
dadd64b
Compare
dcalhoun
left a comment
dcalhoun
left a comment
There was a problem hiding this comment.
Changes look sound. I tested uploading an image through the native proxy.
dcalhoun
left a comment
dcalhoun
left a comment
There was a problem hiding this comment.
Capturing a few findings by Claude that I overlooked earlier.
| guard MediaServerCredentials.areUsable( | ||
| siteApiRoot: configuration.siteApiRoot, | ||
| authHeader: configuration.authHeader | ||
| ) else { |
There was a problem hiding this comment.
Finding from Claude:
This widens the gate from !authHeader.isEmpty to also require an addressable siteApiRoot — but a delegate that handles uploads itself never touches DefaultMediaUploader. MediaUploadServer.processAndUpload returns .uploaded(result) straight from uploadDelegate without resolving context.defaultUploader, so that host now loses its own upload path over a site root it never uses.
Intended? If not, the site-root arm belongs where defaultUploader is actually built.
|
|
||
| @Test("rejects a site root with no scheme") | ||
| func rejectsSchemelessSiteRoot() { | ||
| let relative = URL(string: "example.com/wp-json/")! |
There was a problem hiding this comment.
Finding from Claude:
URL(string: "example.com/wp-json/") has no host either (scheme=nil, host=nil), so this passes on the host arm alone — and rejectsEmptySiteRoot already covers that shape. Nothing isolates scheme != nil; drop it from areUsable and all five tests stay green.
A network-path reference separates them (scheme=nil, host="example.com"):
| let relative = URL(string: "example.com/wp-json/")! | |
| let relative = URL(string: "//example.com/wp-json/")! |
| #expect(!MediaServerCredentials.areUsable(siteApiRoot: fileURL, authHeader: "Bearer t")) | ||
| } | ||
|
|
||
| @Test("rejects an empty site root, the default when a host configures none") |
There was a problem hiding this comment.
Finding from Claude:
siteApiRoot has no default — it's required on EditorConfiguration.init, on EditorConfigurationBuilder.init, and on Android's builder. A host can't configure none; "/" is a value someone passes deliberately.
| @Test("rejects an empty site root, the default when a host configures none") | |
| @Test("rejects a site root with no scheme or host") |
3 participants
Stacked on #594. First of ten PRs splitting #621; each is a single commit on the one before.
What?
iOS starts the native media upload server without checking that
siteApiRootis usable. Android has checked it since it landed.Why?
startUploadServerguards onauthHeader.isEmptyalone, though the comment directly above it says the uploader "needs a site root and an auth header". An iOS host that configured an auth header but nositeApiRootstarted a server whose every request failed at the URLSession layer, instead of falling back to the WebView upload path the way Android does.The two platforms diverged here unnoticed because the check lives in
EditorViewController, which is#if canImport(UIKit)— it does not exist on the macOS host, so nothing in it is reachable from the test suite.How?
areUsable(siteApiRoot:authHeader:)owns the check, outside the UIKit gate so it is testable.startUploadServercalls it.siteApiRootis aURLhere where Android types it as aString, soisEmpty()has no direct equivalent — "addressable" is spelled as scheme and host both being present.Testing Instructions
Five tests pin the predicate, including both arms of the site-root check.
swift test— host suite greenxcodebuild(the host build compilesEditorViewControlleras empty, so the gated change needs a simulator build)