Add UploadSourceMaterializer with policy-driven transforms by crazytonyli · Pull Request #25620 · wordpress-mobile/WordPress-iOS

crazytonyli · 2026-06-08T11:11:27Z

Description

The main changes is introducing UploadSourceMaterializer which turns an UploadSource into an uploadable temp file plus MediaCreateParams, which is then uploaded to the site via wordpress-rs.

dangermattic · 2026-06-08T11:12:30Z

	1 Warning
⚠️	This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

wpmobilebot · 2026-06-08T11:26:10Z

🤖 Build Failure Analysis

This build has failures. Claude has analyzed them - check the build annotations for details.

wpmobilebot · 2026-06-08T23:32:28Z

📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.

	App Name	WordPress
	Configuration	Release-Alpha
	Build Number	`33084`
	Version	`PR #25620`
	Bundle ID	`org.wordpress.alpha`
	Commit	`18466f5`
	Installation URL	3lnoh3qkfu9io

Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

wpmobilebot · 2026-06-08T23:32:35Z

📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.

	App Name	Jetpack
	Configuration	Release-Alpha
	Build Number	`33084`
	Version	`PR #25620`
	Bundle ID	`com.jetpack.alpha`
	Commit	`18466f5`
	Installation URL	7tltjlrcvs9d0

Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

The flag is the single Remove Location setting: it strips GPS EXIF from images and selects the identifying-metadata filter for re-exported video, so the name should not imply it is image-only.

Turns an upload source into an uploadable file: image normalization, video re-export, geolocation strip, and remote-URL download, all gated by the injected MediaUploadPolicy. Images are validated, type-sniffed, converted, resized, and GPS-stripped in a single ImageIO pass that preserves the EXIF orientation tag whenever pixels are not resampled. GIF and SVG never re-encode: one shared raw-passthrough rule covers the photo library, file importer, and remote sources. Includes the filename allocator, remote downloader, materializer error type, and their unit tests.

ProcessInfo.systemUptime measures time since device boot, so the sweep cutoff spared staging directories orphaned by an earlier run in the same boot session. Capture a per-run reference date on the first sweep instead, which the app schedules shortly after launch, before any upload is staged.

URLSession's async download hands back a temp file the caller owns and must delete. A non-2xx status or a failed move threw without removing it, leaking the file in the system temp directory on every failed remote pick.

jkmassel · 2026-07-08T22:05:57Z

+        let location: URL
+        let response: URLResponse
+        do {
+            (location, response) = try await URLSession.shared.download(from: url, delegate: delegate)


This should probably use an ephemeral session – we can get really weird behaviour by using shared (like sending login cookies with requests when we don't want to)

sending login cookies with requests when we don't want to

Do you mean sending WP.com cookies to other sites? I don't think URLSession would do that? It would send WP.com cookies to WP.com URLs, which seems harmless.

Nonetheless I have switched to use ephemeral session in a25f413

jkmassel · 2026-07-08T22:09:14Z

+            )
+            throw MaterializerError.remoteDownloadFailed(underlyingError: statusError)
+        }
+


Potential fun bug – some servers will return an HTML 200 response for an image URL (for instance, to avoid hot linking). It may be worth reading the magic bytes of the top of the file to validate that it's actually an image?

In practice, this downloading code only runs on UploadSource.RemoteURL, which is produced by "Stock Photos" from https://www.pexels.com/ (endpoint: https://public-api.wordpress.com/rest/v1/meta/external-media/pexels?search=cat). It's probably not likely that they'd return a 200 html page when the url is xxx.jpeg?

If they do, the image processing code is likely to catch the issue because the content can be decoded as an image. However, we do have code that skip image processing (like for gif). Still, I think we can trust the "Stock Photos" provider to return a sensible html response if an image url does not return the requested image?

jkmassel · 2026-07-08T22:12:51Z

+        contentType: UTType,
+        stem: String
+    ) throws -> MaterializedUpload {
+        let data = try Data(contentsOf: sourceURL)


IIRC, we pass paths to the Rust layer for upload, so do we really need to load the entire image into memory?

jkmassel · 2026-07-08T22:13:02Z

+                parentDir: parentDir
+            )
+        } else if contentType.conforms(to: .image) {
+            let data = try Data(contentsOf: localFile)


Same question about whether we need to load this into memory

We need to process the image so that the uploaded image matches user settings, like removing GPS, compression rate, etc (see MediaUploadPolicy).

jkmassel · 2026-07-08T22:55:23Z

+    }
+
+    // MARK: - end-to-end
+


Suggested change

@Test("the basename for a very long source name is writable to disk")

func longNameBasenameIsWritable() throws {

let dir = URL(fileURLWithPath: NSTemporaryDirectory())

.appendingPathComponent(UUID().uuidString, isDirectory: true)

try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true)

defer { try? FileManager.default.removeItem(at: dir) }

let stem = allocator.stem(

preferred: String(repeating: "a", count: 300),

fallbackPrefix: "Photo",

date: Date()

)

let dest = dir.appendingPathComponent(allocator.basename(stem: stem, ext: "jpg"))

try Data("x".utf8).write(to: dest) // throws ENAMETOOLONG today → test fails

}

Claude suggested that there's no limit to the filename path component size and we should clamp it.

I got the same comment in my local review, but decided to ignore it considering it's not likely to have a 300 characters long file name. But it's probably pretty trivia to address the concern and the one suggestedName: "../escaped" below.

Addressed in 18466f5

jkmassel · 2026-07-08T23:11:30Z

+            )
+        }
+    }
+


Claude flagged an issue with path traversal:

Suggested change

@Test("remote dispatch keeps output inside parentDir for a traversing suggestedName")

func remoteDispatchContainsTraversingName() async throws {

let parentDir = try makeTempDir()

let sourceFile = parentDir.appendingPathComponent("download.tmp")

try encodeImage(makeSolidColorImage(size: CGSize(width: 10, height: 10), color: .red), as: .jpeg)

.write(to: sourceFile)

let result = try await makeMaterializer(policy())

.dispatchRemoteDownload(

localFile: sourceFile,

contentType: .jpeg,

suggestedName: "../escaped",

caption: nil,

parentDir: parentDir

)

let parent = parentDir.standardizedFileURL.path

// The finalized file must stay INSIDE parentDir.

#expect(

result.tempFileURL.standardizedFileURL.path.hasPrefix(parent + "/"),

"materialized file escaped parentDir: \(result.tempFileURL.standardizedFileURL.path)"

)

// The documented cleanup target must be parentDir itself, never its parent —

// deleting it must not wipe sibling uploads in the staging root.

#expect(result.stagingDirectory.standardizedFileURL.path == parent)

}

The materialized temp-file name drives the uploaded filename, so an over-long source name could overflow NAME_MAX (ENAMETOOLONG) and a name carrying path separators could escape the staging directory via appendingPathComponent. Move both guards into basename, the single point every source path funnels through, so the .remoteURL and .imagePlayground paths that skip stem are covered too: sanitize separators and NUL, then cap the stem to a byte budget on a grapheme boundary.

crazytonyli mentioned this pull request Jun 8, 2026

Add MediaUploader actor with broadcast queue lifecycle #25621

Draft

crazytonyli force-pushed the task/media-v2-upload-foundation branch from 1b5f4d8 to 26d3fb8 Compare June 8, 2026 23:18

crazytonyli force-pushed the task/media-v2-upload-materializer branch from f85702c to fde43a0 Compare June 8, 2026 23:18

crazytonyli force-pushed the task/media-v2-upload-foundation branch from 26d3fb8 to a4390f8 Compare June 11, 2026 00:32

crazytonyli force-pushed the task/media-v2-upload-materializer branch 2 times, most recently from 323dddf to 56a1ac7 Compare June 11, 2026 21:30

crazytonyli force-pushed the task/media-v2-upload-foundation branch from a4390f8 to 4e19015 Compare June 11, 2026 21:30

crazytonyli force-pushed the task/media-v2-upload-materializer branch from 56a1ac7 to 7e76794 Compare June 12, 2026 00:50

crazytonyli force-pushed the task/media-v2-upload-foundation branch 2 times, most recently from 486c1bf to de0286a Compare June 16, 2026 03:38

crazytonyli force-pushed the task/media-v2-upload-materializer branch 4 times, most recently from 610e467 to 0c14222 Compare June 19, 2026 00:20

Base automatically changed from task/media-v2-upload-foundation to trunk July 7, 2026 21:56

crazytonyli added 2 commits July 8, 2026 09:59

Rename MediaUploadPolicy.stripImageLocation to stripGPSLocation

fbd1fe2

The flag is the single Remove Location setting: it strips GPS EXIF from images and selects the identifying-metadata filter for re-exported video, so the name should not imply it is image-only.

crazytonyli force-pushed the task/media-v2-upload-materializer branch from 0c14222 to f548d7a Compare July 7, 2026 22:09

crazytonyli requested a review from jkmassel July 7, 2026 22:20

crazytonyli added the Media label Jul 7, 2026

crazytonyli added this to the 27.1 milestone Jul 7, 2026

crazytonyli marked this pull request as ready for review July 7, 2026 22:21

crazytonyli added 2 commits July 8, 2026 11:20

Release the remote download temp file on failure paths

82bc166

URLSession's async download hands back a temp file the caller owns and must delete. A non-2xx status or a failed move threw without removing it, leaking the file in the system temp directory on every failed remote pick.

jkmassel requested changes Jul 8, 2026

View reviewed changes

Reuse an ephemeral URLSession for remote downloads

a25f413

crazytonyli requested a review from jkmassel July 9, 2026 01:51

+@Test("the basename for a very long source name is writable to disk")
+func longNameBasenameIsWritable() throws {
+    let dir = URL(fileURLWithPath: NSTemporaryDirectory())
+        .appendingPathComponent(UUID().uuidString, isDirectory: true)
+    try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true)
+    defer { try? FileManager.default.removeItem(at: dir) }
+    let stem = allocator.stem(
+        preferred: String(repeating: "a", count: 300),
+        fallbackPrefix: "Photo",
+        date: Date()
+    )
+    let dest = dir.appendingPathComponent(allocator.basename(stem: stem, ext: "jpg"))
+    try Data("x".utf8).write(to: dest) // throws ENAMETOOLONG today → test fails
+}

+    @Test("remote dispatch keeps output inside parentDir for a traversing suggestedName")
+    func remoteDispatchContainsTraversingName() async throws {
+        let parentDir = try makeTempDir()
+        let sourceFile = parentDir.appendingPathComponent("download.tmp")
+        try encodeImage(makeSolidColorImage(size: CGSize(width: 10, height: 10), color: .red), as: .jpeg)
+            .write(to: sourceFile)
+        let result = try await makeMaterializer(policy())
+            .dispatchRemoteDownload(
+                localFile: sourceFile,
+                contentType: .jpeg,
+                suggestedName: "../escaped",
+                caption: nil,
+                parentDir: parentDir
+            )
+        let parent = parentDir.standardizedFileURL.path
+        // The finalized file must stay INSIDE parentDir.
+        #expect(
+            result.tempFileURL.standardizedFileURL.path.hasPrefix(parent + "/"),
+            "materialized file escaped parentDir: \(result.tempFileURL.standardizedFileURL.path)"
+        )
+        // The documented cleanup target must be parentDir itself, never its parent —
+        // deleting it must not wipe sibling uploads in the staging root.
+        #expect(result.stagingDirectory.standardizedFileURL.path == parent)
+    }

Uh oh!

Conversation

crazytonyli commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

dangermattic commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Jun 8, 2026

🤖 Build Failure Analysis

Uh oh!

wpmobilebot commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

crazytonyli commented Jun 8, 2026 •

edited

Loading

dangermattic commented Jun 8, 2026 •

edited

Loading

wpmobilebot commented Jun 8, 2026 •

edited

Loading

wpmobilebot commented Jun 8, 2026 •

edited

Loading