From a9cb6d9764e56cb91922d5e6e9f81fbdb516541f Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Wed, 22 Jul 2026 14:21:09 -0400 Subject: [PATCH 01/29] feat: add gbkOptimizeMediaUploads feature flag Gates the upcoming native media upload processing for the experimental block editor, with a Debug-menu override for quick disabling. --- WordPress/Classes/Utility/BuildInformation/FeatureFlag.swift | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/WordPress/Classes/Utility/BuildInformation/FeatureFlag.swift b/WordPress/Classes/Utility/BuildInformation/FeatureFlag.swift index 0c909a9a5d74..9f50ce1dce9f 100644 --- a/WordPress/Classes/Utility/BuildInformation/FeatureFlag.swift +++ b/WordPress/Classes/Utility/BuildInformation/FeatureFlag.swift @@ -25,6 +25,7 @@ public enum FeatureFlag: Int, CaseIterable { case nativeBlockInserter case statsAds case mediaLibraryV2 + case gbkMediaUploadOptimization /// Returns a boolean indicating if the feature is enabled. /// @@ -80,6 +81,8 @@ public enum FeatureFlag: Int, CaseIterable { return BuildConfiguration.current == .debug case .mediaLibraryV2: return BuildConfiguration.current == .debug + case .gbkMediaUploadOptimization: + return true } } @@ -121,6 +124,7 @@ extension FeatureFlag { case .nativeBlockInserter: "Native Block Inserter" case .statsAds: "Stats Ads Tab" case .mediaLibraryV2: "Media Library v2" + case .gbkMediaUploadOptimization: "Optimize Experimental Block Editor Uploads" } } } From 6051a74e544c4161df4875033987ebb917be15b9 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Wed, 22 Jul 2026 14:21:21 -0400 Subject: [PATCH 02/29] build: use GutenbergKit pr-build/357 snapshot Points GutenbergKit at the XCFramework snapshot for wordpress-mobile/GutenbergKit#357, which adds the native media upload server and MediaUploadDelegate. Swap to a tagged release before merge. --- Modules/Package.resolved | 4 ++-- Modules/Package.swift | 2 +- Package.resolved | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Modules/Package.resolved b/Modules/Package.resolved index b36c195bea18..1d124601731b 100644 --- a/Modules/Package.resolved +++ b/Modules/Package.resolved @@ -131,8 +131,8 @@ "kind" : "remoteSourceControl", "location" : "https://github.com/wordpress-mobile/GutenbergKit", "state" : { - "revision" : "b6604e26792725b2e8125b3b715e1d4a298441b6", - "version" : "0.19.0" + "branch" : "pr-build/357", + "revision" : "72d422699607aec7b5e666db422787b5e2a6378d" } }, { diff --git a/Modules/Package.swift b/Modules/Package.swift index 4b1217724507..8feb7ee92788 100644 --- a/Modules/Package.swift +++ b/Modules/Package.swift @@ -62,7 +62,7 @@ let package = Package( revision: "b34794c9a3f32312e1593d4a3d120572afa0d010" ), .package(url: "https://github.com/zendesk/support_sdk_ios", from: "8.0.3"), - .package(url: "https://github.com/wordpress-mobile/GutenbergKit", from: "0.19.0"), + .package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/357"), .package( url: "https://github.com/automattic/wordpress-rs", exact: "0.6.0" diff --git a/Package.resolved b/Package.resolved index e4f561d43c78..76af08fe709c 100644 --- a/Package.resolved +++ b/Package.resolved @@ -131,8 +131,8 @@ "kind" : "remoteSourceControl", "location" : "https://github.com/wordpress-mobile/GutenbergKit", "state" : { - "revision" : "7180587f49d3c3bfdb34cc3e80b2a9d22a3cd93e", - "version" : "0.18.1" + "branch" : "pr-build/357", + "revision" : "72d422699607aec7b5e666db422787b5e2a6378d" } }, { From fb3f270087d93c5dda2e4c6dfa4b54d7a8b2c9c6 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Wed, 22 Jul 2026 14:21:36 -0400 Subject: [PATCH 03/29] feat: process GutenbergKit media uploads with app Media settings Implements GutenbergKit's MediaUploadDelegate so device media picked in the experimental block editor is processed natively before upload, honoring the app's Media settings: Optimize Images, Max Image Upload Size, Image Quality, Max Video Upload Size, and Remove Location From Media. Previously these uploads went directly from the WebView to the REST API with no processing. GBKMediaUploadProcessor mirrors the exporter option mapping used by MediaImportService and reuses MediaURLExporter, so the editor now matches the behavior of the legacy editor and My Site > Media. GIFs and non-media files pass through untouched, and non-web-safe image formats (e.g. HEIC) are converted to JPEG. Uploads still use GutenbergKit's default uploader, which relays the raw WordPress response to the editor. --- .../GBKMediaUploadProcessor.swift | 129 ++++++++++++++++++ .../PostGBKEditorViewController.swift | 7 + 2 files changed, 136 insertions(+) create mode 100644 WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift diff --git a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift new file mode 100644 index 000000000000..182fc633dfd5 --- /dev/null +++ b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift @@ -0,0 +1,129 @@ +import Foundation +import GutenbergKit +import UniformTypeIdentifiers +import WordPressData + +/// Processes media files picked in the GutenbergKit editor before upload, +/// applying the app's Media settings (image optimization, max upload size, +/// image quality, video resolution, and location stripping). +/// +/// Assigned to `GutenbergKit.EditorViewController.mediaUploadDelegate`, which +/// holds it weakly and invokes it off the main actor, so the type is `Sendable` +/// and snapshots the `Blog`-derived values it needs at initialization. +final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { + private let videoDurationLimit: TimeInterval? + private let allowableFileExtensions: Set + private let makeMediaSettings: @Sendable () -> MediaSettings + + /// Image types the WordPress REST API reliably accepts. Other image + /// formats (e.g. HEIC) are converted to JPEG during processing, mirroring + /// `ItemProviderMediaExporter`. + private static let webSafeImageTypes: Set = [.png, .jpeg, .gif, .svg] + + @MainActor + convenience init(blog: Blog) { + // HEIC isn't supported when uploading an image, so we filter it out, + // mirroring `MediaImportService`. + var allowedFileTypes = blog.allowedFileTypes + allowedFileTypes.remove("heic") + + self.init( + videoDurationLimit: blog.videoDurationLimit, + allowableFileExtensions: allowedFileTypes + ) + } + + init( + videoDurationLimit: TimeInterval?, + allowableFileExtensions: Set, + makeMediaSettings: @escaping @Sendable () -> MediaSettings = { MediaSettings() } + ) { + self.videoDurationLimit = videoDurationLimit + self.allowableFileExtensions = allowableFileExtensions + self.makeMediaSettings = makeMediaSettings + } + + // MARK: - MediaUploadDelegate + + func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { + let expected = try MediaURLExporter.expectedExport(with: url) + let settings = makeMediaSettings() + + switch expected { + case .gif: + // GIFs are uploaded unchanged; processing would only copy the file. + return .original + case .other: + // Non-media files are uploaded unchanged, but enforce the site's + // allowed file extensions, mirroring `MediaURLExporter.exportURL`. + if let fileExtension = url.typeIdentifierFileExtension, + !MediaImportService.defaultAllowableFileExtensions.contains(fileExtension), + !allowableFileExtensions.isEmpty, + !allowableFileExtensions.contains(fileExtension) + { + throw MediaURLExporter.URLExportError.unsupportedFileType + } + return .original + case .image: + // Skip processing when it would be a no-op: optimization and + // location stripping disabled, and the format is web-safe. + if !settings.imageOptimizationEnabled, + !settings.removeLocationSetting, + let type = url.typeIdentifier.flatMap(UTType.init), + Self.webSafeImageTypes.contains(type) + { + return .original + } + case .video: + // Always process video to apply the export preset, duration + // limit, and location stripping. + break + } + + let export = try await makeExporter(for: url, settings: settings).export() + + guard let mimeType = export.url.typeIdentifier.flatMap(UTType.init)?.preferredMIMEType else { + throw MediaURLExporter.URLExportError.unknownFileUTI + } + return .processed(export.url, mimeType: mimeType, filename: export.url.lastPathComponent) + } + + // MARK: - Exporter configuration + + /// Builds an exporter configured from the app's Media settings, mirroring + /// the option mapping in `MediaImportService`. + private func makeExporter(for url: URL, settings: MediaSettings) -> MediaURLExporter { + let exporter = MediaURLExporter(url: url) + // GutenbergKit deletes the processed file after uploading it, so the + // export is written to a temporary directory rather than the uploads + // directory tracked by `MediaFileManager`. + exporter.mediaDirectoryType = .temporary + + var imageOptions = MediaImageExporter.Options() + imageOptions.maximumImageSize = maximumImageSize(from: settings) + imageOptions.stripsGeoLocationIfNeeded = settings.removeLocationSetting + imageOptions.imageCompressionQuality = settings.imageQualityForUpload.doubleValue + if let type = url.typeIdentifier.flatMap(UTType.init), !Self.webSafeImageTypes.contains(type) { + imageOptions.exportImageType = UTType.jpeg.identifier + } + exporter.imageOptions = imageOptions + + var videoOptions = MediaVideoExporter.Options() + videoOptions.stripsGeoLocationIfNeeded = settings.removeLocationSetting + videoOptions.exportPreset = settings.maxVideoSizeSetting.videoPreset + videoOptions.durationLimit = videoDurationLimit + exporter.videoOptions = videoOptions + + var urlOptions = MediaURLExporter.Options() + urlOptions.allowableFileExtensions = allowableFileExtensions + urlOptions.stripsGeoLocationIfNeeded = settings.removeLocationSetting + exporter.urlOptions = urlOptions + + return exporter + } + + private func maximumImageSize(from settings: MediaSettings) -> CGFloat? { + let maxUploadSize = settings.imageSizeForUpload + return maxUploadSize < Int.max ? CGFloat(maxUploadSize) : nil + } +} diff --git a/WordPress/Classes/ViewRelated/NewGutenberg/PostGBKEditorViewController.swift b/WordPress/Classes/ViewRelated/NewGutenberg/PostGBKEditorViewController.swift index 46e498603e0d..4bab73af9442 100644 --- a/WordPress/Classes/ViewRelated/NewGutenberg/PostGBKEditorViewController.swift +++ b/WordPress/Classes/ViewRelated/NewGutenberg/PostGBKEditorViewController.swift @@ -16,6 +16,9 @@ class PostGBKEditorViewController: UIViewController, GutenbergKit.EditorViewCont private lazy var mediaPickerHelper = GutenbergMediaPickerHelper(context: self, blog: blog) + /// Retains the media upload processor, which the editor holds weakly. + private let mediaUploadProcessor: GBKMediaUploadProcessor + private var keyboardShowObserver: Any? private var keyboardHideObserver: Any? private var keyboardFrame = CGRect.zero @@ -55,10 +58,14 @@ class PostGBKEditorViewController: UIViewController, GutenbergKit.EditorViewCont dependencies: cachedDependencies, mediaPicker: MediaPickerController(blog: blog) ) + self.mediaUploadProcessor = GBKMediaUploadProcessor(blog: blog) super.init(nibName: nil, bundle: nil) self.editorViewController.delegate = self + if FeatureFlag.gbkMediaUploadOptimization.enabled { + self.editorViewController.mediaUploadDelegate = mediaUploadProcessor + } } required init?(coder aDecoder: NSCoder) { From 72c81d7874a66fd6fabffde75b622ac5574259a3 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Wed, 22 Jul 2026 14:21:42 -0400 Subject: [PATCH 04/29] test: cover GBKMediaUploadProcessor media processing Verifies resizing, GPS stripping, HEIC-to-JPEG conversion, GIF and no-op passthrough, disallowed file extensions, and the video duration limit using the existing media fixtures. --- .../Media/GBKMediaUploadProcessorTests.swift | 174 ++++++++++++++++++ 1 file changed, 174 insertions(+) create mode 100644 Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift diff --git a/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift b/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift new file mode 100644 index 000000000000..9d6c5a052f49 --- /dev/null +++ b/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift @@ -0,0 +1,174 @@ +import Foundation +import ImageIO +import Testing +import UniformTypeIdentifiers +import WordPressShared + +@testable import WordPress + +struct GBKMediaUploadProcessorTests { + + // MARK: - Images + + @Test func imageIsResizedWhenOptimizationEnabled() async throws { + let settings = makeSettings() + settings.imageOptimizationEnabled = true + settings.maxImageSizeSetting = 200 + let processor = makeProcessor(settings: settings) + let url = try fixtureURL("test-image-device-photo-gps.jpg") + + let result = try await processor.processFile(at: url, mimeType: "image/jpeg", filename: url.lastPathComponent) + + guard case .processed(let outputURL, let mimeType, let filename) = result else { + Issue.record("Expected a processed file") + return + } + defer { cleanUp(outputURL) } + let size = try imageSize(at: outputURL) + #expect(max(size.width, size.height) == 200) + #expect(mimeType == "image/jpeg") + #expect(filename.hasPrefix("test-image-device-photo-gps")) + } + + @Test func imageIsUntouchedWhenProcessingWouldBeNoOp() async throws { + let settings = makeSettings() + settings.imageOptimizationEnabled = false + settings.removeLocationSetting = false + let processor = makeProcessor(settings: settings) + let url = try fixtureURL("test-image-device-photo-gps.jpg") + + let result = try await processor.processFile(at: url, mimeType: "image/jpeg", filename: url.lastPathComponent) + + guard case .original = result else { + Issue.record("Expected the original file to pass through") + return + } + } + + @Test func gpsDataIsStrippedWhenRemoveLocationEnabled() async throws { + let settings = makeSettings() + settings.imageOptimizationEnabled = false + settings.removeLocationSetting = true + let processor = makeProcessor(settings: settings) + let url = try fixtureURL("test-image-device-photo-gps.jpg") + + let result = try await processor.processFile(at: url, mimeType: "image/jpeg", filename: url.lastPathComponent) + + guard case .processed(let outputURL, _, _) = result else { + Issue.record("Expected a processed file") + return + } + defer { cleanUp(outputURL) } + #expect(try imageProperties(at: url)[kCGImagePropertyGPSDictionary] != nil) + #expect(try imageProperties(at: outputURL)[kCGImagePropertyGPSDictionary] == nil) + } + + @Test func heicIsConvertedToJPEG() async throws { + let settings = makeSettings() + settings.imageOptimizationEnabled = false + settings.removeLocationSetting = false + let processor = makeProcessor(settings: settings) + let url = try fixtureURL("iphone-photo.heic") + + let result = try await processor.processFile(at: url, mimeType: "image/heic", filename: url.lastPathComponent) + + guard case .processed(let outputURL, let mimeType, let filename) = result else { + Issue.record("Expected a processed file") + return + } + defer { cleanUp(outputURL) } + #expect(mimeType == "image/jpeg") + #expect(filename.hasSuffix(".jpg") || filename.hasSuffix(".jpeg")) + } + + // MARK: - GIFs and other files + + @Test func gifPassesThroughUntouched() async throws { + let processor = makeProcessor(settings: makeSettings()) + let url = try fixtureURL("test-gif.gif") + + let result = try await processor.processFile(at: url, mimeType: "image/gif", filename: url.lastPathComponent) + + guard case .original = result else { + Issue.record("Expected the original file to pass through") + return + } + } + + @Test func disallowedFileExtensionThrows() async throws { + let processor = GBKMediaUploadProcessor( + videoDurationLimit: nil, + allowableFileExtensions: ["pdf"], + makeMediaSettings: makeSettingsFactory(makeSettings()) + ) + let url = FileManager.default.temporaryDirectory.appendingPathComponent("\(UUID().uuidString).txt") + try "plain text".write(to: url, atomically: true, encoding: .utf8) + defer { cleanUp(url) } + + await #expect(throws: MediaURLExporter.URLExportError.self) { + try await processor.processFile(at: url, mimeType: "text/plain", filename: url.lastPathComponent) + } + } + + // MARK: - Videos + + @Test func videoExceedingDurationLimitThrows() async throws { + let processor = GBKMediaUploadProcessor( + videoDurationLimit: 1, + allowableFileExtensions: [], + makeMediaSettings: makeSettingsFactory(makeSettings()) + ) + let url = try fixtureURL("test-video-device-gps.m4v") + + await #expect(throws: (any Error).self) { + try await processor.processFile(at: url, mimeType: "video/mp4", filename: url.lastPathComponent) + } + } + + // MARK: - Helpers + + private func makeProcessor(settings: MediaSettings) -> GBKMediaUploadProcessor { + GBKMediaUploadProcessor( + videoDurationLimit: nil, + allowableFileExtensions: [], + makeMediaSettings: makeSettingsFactory(settings) + ) + } + + private func makeSettings() -> MediaSettings { + MediaSettings(database: EphemeralKeyValueDatabase()) + } + + private func makeSettingsFactory(_ settings: MediaSettings) -> @Sendable () -> MediaSettings { + nonisolated(unsafe) let settings = settings + return { settings } + } + + private func fixtureURL(_ filename: String) throws -> URL { + let bundle = Bundle(for: BundleAnchor.self) + let name = (filename as NSString).deletingPathExtension + let ext = (filename as NSString).pathExtension + let url = try #require(bundle.url(forResource: name, withExtension: ext)) + return url + } + + private func imageProperties(at url: URL) throws -> [CFString: Any] { + let source = try #require(CGImageSourceCreateWithURL(url as CFURL, nil)) + let properties = try #require(CGImageSourceCopyPropertiesAtIndex(source, 0, nil) as? [CFString: Any]) + return properties + } + + private func imageSize(at url: URL) throws -> CGSize { + let properties = try imageProperties(at: url) + let width = try #require(properties[kCGImagePropertyPixelWidth] as? CGFloat) + let height = try #require(properties[kCGImagePropertyPixelHeight] as? CGFloat) + return CGSize(width: width, height: height) + } + + private func cleanUp(_ url: URL) { + try? FileManager.default.removeItem(at: url) + } +} + +/// Anchor for resolving the test bundle from Swift Testing suites. +private final class BundleAnchor {} From 327fce95fc74cc06703e2bfdf92846f899c1ce63 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Wed, 22 Jul 2026 15:21:13 -0400 Subject: [PATCH 05/29] feat: surface media upload optimization in Experimental Features Adds the gbkMediaUploadOptimization flag to the Experimental Features list and aligns display names with the "New Block Editor (NBE)" naming. --- WordPress/Classes/Utility/BuildInformation/FeatureFlag.swift | 2 +- .../Classes/Utility/BuildInformation/RemoteFeatureFlag.swift | 2 +- .../Me/App Settings/ExperimentalFeaturesDataProvider.swift | 1 + 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/WordPress/Classes/Utility/BuildInformation/FeatureFlag.swift b/WordPress/Classes/Utility/BuildInformation/FeatureFlag.swift index 9f50ce1dce9f..88fa7b16006c 100644 --- a/WordPress/Classes/Utility/BuildInformation/FeatureFlag.swift +++ b/WordPress/Classes/Utility/BuildInformation/FeatureFlag.swift @@ -124,7 +124,7 @@ extension FeatureFlag { case .nativeBlockInserter: "Native Block Inserter" case .statsAds: "Stats Ads Tab" case .mediaLibraryV2: "Media Library v2" - case .gbkMediaUploadOptimization: "Optimize Experimental Block Editor Uploads" + case .gbkMediaUploadOptimization: "NBE Media Upload Optimization" } } } diff --git a/WordPress/Classes/Utility/BuildInformation/RemoteFeatureFlag.swift b/WordPress/Classes/Utility/BuildInformation/RemoteFeatureFlag.swift index 7618900e2fca..0c5a7bbe03bf 100644 --- a/WordPress/Classes/Utility/BuildInformation/RemoteFeatureFlag.swift +++ b/WordPress/Classes/Utility/BuildInformation/RemoteFeatureFlag.swift @@ -203,7 +203,7 @@ public enum RemoteFeatureFlag: Int, CaseIterable { case .dotComWebLogin: return "Log in to WordPress.com from web browser" case .newGutenberg: - return "Experimental Block Editor" + return "New Block Editor (NBE)" case .newGutenbergPlugins: return "Experimental Block Editor Plugins" case .statsAds: diff --git a/WordPress/Classes/ViewRelated/Me/App Settings/ExperimentalFeaturesDataProvider.swift b/WordPress/Classes/ViewRelated/Me/App Settings/ExperimentalFeaturesDataProvider.swift index fb6990624a4c..ba1b005921b9 100644 --- a/WordPress/Classes/ViewRelated/Me/App Settings/ExperimentalFeaturesDataProvider.swift +++ b/WordPress/Classes/ViewRelated/Me/App Settings/ExperimentalFeaturesDataProvider.swift @@ -9,6 +9,7 @@ class ExperimentalFeaturesDataProvider: ExperimentalFeaturesViewModel.DataProvid FeatureFlag.intelligence, FeatureFlag.newStats, RemoteFeatureFlag.newGutenberg, + FeatureFlag.gbkMediaUploadOptimization, FeatureFlag.newSupport, ] From c6eaa2ee62888a6379a08d900ba50c9e66d0c8f1 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Thu, 23 Jul 2026 09:47:26 -0400 Subject: [PATCH 06/29] refactor: set media upload delegate unconditionally Removes the gbkMediaUploadOptimization gate ahead of reverting the flag. GutenbergKit degrades gracefully if the upload server cannot start, so a dedicated kill switch isn't needed. --- .../NewGutenberg/PostGBKEditorViewController.swift | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/WordPress/Classes/ViewRelated/NewGutenberg/PostGBKEditorViewController.swift b/WordPress/Classes/ViewRelated/NewGutenberg/PostGBKEditorViewController.swift index 4bab73af9442..4f227a83dcb6 100644 --- a/WordPress/Classes/ViewRelated/NewGutenberg/PostGBKEditorViewController.swift +++ b/WordPress/Classes/ViewRelated/NewGutenberg/PostGBKEditorViewController.swift @@ -63,9 +63,7 @@ class PostGBKEditorViewController: UIViewController, GutenbergKit.EditorViewCont super.init(nibName: nil, bundle: nil) self.editorViewController.delegate = self - if FeatureFlag.gbkMediaUploadOptimization.enabled { - self.editorViewController.mediaUploadDelegate = mediaUploadProcessor - } + self.editorViewController.mediaUploadDelegate = mediaUploadProcessor } required init?(coder aDecoder: NSCoder) { From bd08c35f8d7bd1735dab3e49c971bcbc7b76b261 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Thu, 23 Jul 2026 09:47:29 -0400 Subject: [PATCH 07/29] Revert "feat: surface media upload optimization in Experimental Features" This reverts commit f373395e0cc1bf63b1dc6885f6300cf337ee45ed. --- WordPress/Classes/Utility/BuildInformation/FeatureFlag.swift | 2 +- .../Classes/Utility/BuildInformation/RemoteFeatureFlag.swift | 2 +- .../Me/App Settings/ExperimentalFeaturesDataProvider.swift | 1 - 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/WordPress/Classes/Utility/BuildInformation/FeatureFlag.swift b/WordPress/Classes/Utility/BuildInformation/FeatureFlag.swift index 88fa7b16006c..9f50ce1dce9f 100644 --- a/WordPress/Classes/Utility/BuildInformation/FeatureFlag.swift +++ b/WordPress/Classes/Utility/BuildInformation/FeatureFlag.swift @@ -124,7 +124,7 @@ extension FeatureFlag { case .nativeBlockInserter: "Native Block Inserter" case .statsAds: "Stats Ads Tab" case .mediaLibraryV2: "Media Library v2" - case .gbkMediaUploadOptimization: "NBE Media Upload Optimization" + case .gbkMediaUploadOptimization: "Optimize Experimental Block Editor Uploads" } } } diff --git a/WordPress/Classes/Utility/BuildInformation/RemoteFeatureFlag.swift b/WordPress/Classes/Utility/BuildInformation/RemoteFeatureFlag.swift index 0c5a7bbe03bf..7618900e2fca 100644 --- a/WordPress/Classes/Utility/BuildInformation/RemoteFeatureFlag.swift +++ b/WordPress/Classes/Utility/BuildInformation/RemoteFeatureFlag.swift @@ -203,7 +203,7 @@ public enum RemoteFeatureFlag: Int, CaseIterable { case .dotComWebLogin: return "Log in to WordPress.com from web browser" case .newGutenberg: - return "New Block Editor (NBE)" + return "Experimental Block Editor" case .newGutenbergPlugins: return "Experimental Block Editor Plugins" case .statsAds: diff --git a/WordPress/Classes/ViewRelated/Me/App Settings/ExperimentalFeaturesDataProvider.swift b/WordPress/Classes/ViewRelated/Me/App Settings/ExperimentalFeaturesDataProvider.swift index ba1b005921b9..fb6990624a4c 100644 --- a/WordPress/Classes/ViewRelated/Me/App Settings/ExperimentalFeaturesDataProvider.swift +++ b/WordPress/Classes/ViewRelated/Me/App Settings/ExperimentalFeaturesDataProvider.swift @@ -9,7 +9,6 @@ class ExperimentalFeaturesDataProvider: ExperimentalFeaturesViewModel.DataProvid FeatureFlag.intelligence, FeatureFlag.newStats, RemoteFeatureFlag.newGutenberg, - FeatureFlag.gbkMediaUploadOptimization, FeatureFlag.newSupport, ] From 0bae62a18e54ccb6f8c7bf5911449e3d0035444e Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Thu, 23 Jul 2026 09:47:31 -0400 Subject: [PATCH 08/29] Revert "feat: add gbkOptimizeMediaUploads feature flag" This reverts commit ea1ad1f2920d85226c3cd6f80a99fa3e94d30fe9. --- WordPress/Classes/Utility/BuildInformation/FeatureFlag.swift | 4 ---- 1 file changed, 4 deletions(-) diff --git a/WordPress/Classes/Utility/BuildInformation/FeatureFlag.swift b/WordPress/Classes/Utility/BuildInformation/FeatureFlag.swift index 9f50ce1dce9f..0c909a9a5d74 100644 --- a/WordPress/Classes/Utility/BuildInformation/FeatureFlag.swift +++ b/WordPress/Classes/Utility/BuildInformation/FeatureFlag.swift @@ -25,7 +25,6 @@ public enum FeatureFlag: Int, CaseIterable { case nativeBlockInserter case statsAds case mediaLibraryV2 - case gbkMediaUploadOptimization /// Returns a boolean indicating if the feature is enabled. /// @@ -81,8 +80,6 @@ public enum FeatureFlag: Int, CaseIterable { return BuildConfiguration.current == .debug case .mediaLibraryV2: return BuildConfiguration.current == .debug - case .gbkMediaUploadOptimization: - return true } } @@ -124,7 +121,6 @@ extension FeatureFlag { case .nativeBlockInserter: "Native Block Inserter" case .statsAds: "Stats Ads Tab" case .mediaLibraryV2: "Media Library v2" - case .gbkMediaUploadOptimization: "Optimize Experimental Block Editor Uploads" } } } From 435046a84d126b0185dfa3382e09ad9476fa2a00 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Sat, 15 Aug 2026 22:17:50 -0400 Subject: [PATCH 09/29] build: track GutenbergKit trunk for native media uploads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moves the pin off the pr-build/357 snapshot now that wordpress-mobile/GutenbergKit#357 has merged. Trunk also carries the follow-up hardening in #561, which adds a defaulted handlesFile(ofType: named:) to MediaUploadDelegate, so GBKMediaUploadProcessor conforms unchanged. No tagged release includes #357 yet — v0.19.0 predates it. Swap to a tagged release before merge. --- Modules/Package.resolved | 4 ++-- Modules/Package.swift | 2 +- Package.resolved | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Modules/Package.resolved b/Modules/Package.resolved index 1d124601731b..73622f5c7f4b 100644 --- a/Modules/Package.resolved +++ b/Modules/Package.resolved @@ -131,8 +131,8 @@ "kind" : "remoteSourceControl", "location" : "https://github.com/wordpress-mobile/GutenbergKit", "state" : { - "branch" : "pr-build/357", - "revision" : "72d422699607aec7b5e666db422787b5e2a6378d" + "branch" : "trunk", + "revision" : "16aceb17716dbef9fb2289d316e27dc812a569f6" } }, { diff --git a/Modules/Package.swift b/Modules/Package.swift index 8feb7ee92788..a3fef2e8603a 100644 --- a/Modules/Package.swift +++ b/Modules/Package.swift @@ -62,7 +62,7 @@ let package = Package( revision: "b34794c9a3f32312e1593d4a3d120572afa0d010" ), .package(url: "https://github.com/zendesk/support_sdk_ios", from: "8.0.3"), - .package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/357"), + .package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "trunk"), .package( url: "https://github.com/automattic/wordpress-rs", exact: "0.6.0" diff --git a/Package.resolved b/Package.resolved index 76af08fe709c..e72ab17bc880 100644 --- a/Package.resolved +++ b/Package.resolved @@ -131,8 +131,8 @@ "kind" : "remoteSourceControl", "location" : "https://github.com/wordpress-mobile/GutenbergKit", "state" : { - "branch" : "pr-build/357", - "revision" : "72d422699607aec7b5e666db422787b5e2a6378d" + "branch" : "trunk", + "revision" : "16aceb17716dbef9fb2289d316e27dc812a569f6" } }, { From ebb91a281d50ad2d32f97cfaadbdb59d7e9452e9 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Mon, 17 Aug 2026 09:08:59 -0400 Subject: [PATCH 10/29] fix: pass SVG uploads through unprocessed SVG conforms to `UTType.image`, so `MediaURLExporter.expectedExport` classifies it as `.image` and it reaches the exporter. ImageIO cannot decode or encode SVG: `CGImageSourceCreateWithURL` returns a source with zero images, and both `CGImageSourceCreateThumbnailAtIndex` and `CGImageDestinationCreateWithURL` return nil. The export therefore fails instead of producing a file. Return the original file for SVG, as we already do for GIF, and drop `.svg` from `webSafeImageTypes`. That set decides whether an image needs converting to JPEG, so it should only hold raster formats ImageIO can actually read and write; SVG's membership there implied it could reach the exporter safely. Only sites whose plan allows SVG can upload one, so this is not reachable on plans where the picker greys the file out. Co-Authored-By: Claude Opus 5 (1M context) --- .../Media/GBKMediaUploadProcessorTests.swift | 25 +++++++++++++++++++ .../GBKMediaUploadProcessor.swift | 19 +++++++++++--- 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift b/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift index 9d6c5a052f49..55b3497c5479 100644 --- a/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift +++ b/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift @@ -95,6 +95,31 @@ struct GBKMediaUploadProcessorTests { } } + /// SVG conforms to `UTType.image`, so it reaches the image branch, but + /// ImageIO cannot decode or encode it. It must pass through untouched + /// rather than fail in the exporter. + @Test func svgPassesThroughUntouched() async throws { + let settings = makeSettings() + settings.imageOptimizationEnabled = true + settings.removeLocationSetting = true + let processor = makeProcessor(settings: settings) + let url = FileManager.default.temporaryDirectory.appendingPathComponent("\(UUID().uuidString).svg") + try #""# + .write(to: url, atomically: true, encoding: .utf8) + defer { cleanUp(url) } + + let result = try await processor.processFile( + at: url, + mimeType: "image/svg+xml", + filename: url.lastPathComponent + ) + + guard case .original = result else { + Issue.record("Expected the original file to pass through") + return + } + } + @Test func disallowedFileExtensionThrows() async throws { let processor = GBKMediaUploadProcessor( videoDurationLimit: nil, diff --git a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift index 182fc633dfd5..69a16423ec21 100644 --- a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift +++ b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift @@ -15,10 +15,14 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { private let allowableFileExtensions: Set private let makeMediaSettings: @Sendable () -> MediaSettings - /// Image types the WordPress REST API reliably accepts. Other image + /// Raster image types the WordPress REST API reliably accepts. Other image /// formats (e.g. HEIC) are converted to JPEG during processing, mirroring /// `ItemProviderMediaExporter`. - private static let webSafeImageTypes: Set = [.png, .jpeg, .gif, .svg] + /// + /// - Note: SVG is deliberately absent. It is web-safe, but it is a vector + /// format that ImageIO cannot decode or encode, so it never reaches the + /// exporter — `processFile` returns it unchanged (see below). + private static let webSafeImageTypes: Set = [.png, .jpeg, .gif] @MainActor convenience init(blog: Blog) { @@ -65,11 +69,20 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { } return .original case .image: + let type = url.typeIdentifier.flatMap(UTType.init) + + // SVG conforms to `UTType.image`, so it lands here, but ImageIO + // cannot decode or encode it: the export would fail rather than + // produce a file. Upload it unchanged, like a GIF. + if type == .svg { + return .original + } + // Skip processing when it would be a no-op: optimization and // location stripping disabled, and the format is web-safe. if !settings.imageOptimizationEnabled, !settings.removeLocationSetting, - let type = url.typeIdentifier.flatMap(UTType.init), + let type, Self.webSafeImageTypes.contains(type) { return .original From dd8c5bb5bee32d53592c814c995858aae42b355a Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Mon, 17 Aug 2026 09:18:38 -0400 Subject: [PATCH 11/29] fix: stop leaking a temporary directory per media upload `MediaDirectory.temporary` is a computed property returning `.temporary(id: UUID())`, so each export was written to a fresh `tmp//Media/`. GutenbergKit's cleanup removes only the file at `uploadURL`, never the two enclosing directories, so every processed image or video left an empty directory pair behind for the lifetime of the process. Write every export to one directory identified by a fixed UUID. `MediaFileManager.makeLocalMediaURL` increments filenames, so uploads sharing a source name do not collide. The ID is stable across launches so a directory orphaned by a crash is reused rather than accumulating. Co-Authored-By: Claude Opus 5 (1M context) --- .../Media/GBKMediaUploadProcessorTests.swift | 28 +++++++++++++++++++ .../GBKMediaUploadProcessor.swift | 15 +++++++++- 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift b/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift index 55b3497c5479..bbde2df27e16 100644 --- a/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift +++ b/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift @@ -81,6 +81,34 @@ struct GBKMediaUploadProcessorTests { #expect(filename.hasSuffix(".jpg") || filename.hasSuffix(".jpeg")) } + /// GutenbergKit removes the exported file but never its enclosing + /// directories, so exports must share one directory rather than each + /// creating its own. + @Test func exportsShareOneTemporaryDirectory() async throws { + let settings = makeSettings() + settings.imageOptimizationEnabled = true + settings.maxImageSizeSetting = 200 + let processor = makeProcessor(settings: settings) + let url = try fixtureURL("test-image-device-photo-gps.jpg") + + var directories: Set = [] + for _ in 0..<3 { + let result = try await processor.processFile( + at: url, + mimeType: "image/jpeg", + filename: url.lastPathComponent + ) + guard case .processed(let outputURL, _, _) = result else { + Issue.record("Expected a processed file") + return + } + defer { cleanUp(outputURL) } + directories.insert(outputURL.deletingLastPathComponent()) + } + + #expect(directories.count == 1) + } + // MARK: - GIFs and other files @Test func gifPassesThroughUntouched() async throws { diff --git a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift index 69a16423ec21..ff8a4767568e 100644 --- a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift +++ b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift @@ -24,6 +24,13 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { /// exporter — `processFile` returns it unchanged (see below). private static let webSafeImageTypes: Set = [.png, .jpeg, .gif] + /// A fixed ID for the temporary directory exports are written to, so every + /// upload reuses the same directory instead of creating a new one. + /// + /// Stable across launches: a directory orphaned by a crash is reused rather + /// than accumulating alongside a new one. + private static let exportDirectoryID = UUID(uuidString: "1D8A4E5C-1F3B-4E7A-9C2D-6B0F8A5E3C71")! + @MainActor convenience init(blog: Blog) { // HEIC isn't supported when uploading an image, so we filter it out, @@ -110,7 +117,13 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { // GutenbergKit deletes the processed file after uploading it, so the // export is written to a temporary directory rather than the uploads // directory tracked by `MediaFileManager`. - exporter.mediaDirectoryType = .temporary + // + // Reuse one directory for every export rather than `MediaDirectory + // .temporary`, which mints a new UUID per access. GutenbergKit removes + // only the exported file, never its enclosing directories, so a fresh + // UUID each time would leave an empty `tmp//Media/` behind for + // every upload. + exporter.mediaDirectoryType = .temporary(id: Self.exportDirectoryID) var imageOptions = MediaImageExporter.Options() imageOptions.maximumImageSize = maximumImageSize(from: settings) From c140b4111e18777129140d71a868b45976ed06e0 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Mon, 17 Aug 2026 09:19:01 -0400 Subject: [PATCH 12/29] docs: clarify when the image export is skipped The comment claimed the guard skips processing "when it would be a no-op", which is broader than what the condition tests. With image optimization off, `imageSizeForUpload` returns `Int.max` (no downscale) but `imageQualityForUpload` returns `.high`, so an image that falls through is still re-encoded at 0.9 quality. Describe what the guard actually checks and note that the quality mapping matches `MediaImportService`. No behavior change. Co-Authored-By: Claude Opus 5 (1M context) --- .../NewGutenberg/GBKMediaUploadProcessor.swift | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift index ff8a4767568e..92c8ed4d2c08 100644 --- a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift +++ b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift @@ -85,8 +85,15 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { return .original } - // Skip processing when it would be a no-op: optimization and - // location stripping disabled, and the format is web-safe. + // Skip the export when nothing would change the file: no + // downscaling, no location stripping, and no format conversion. + // + // This is narrower than "processing changes nothing". With + // optimization off, `imageQualityForUpload` is still `.high`, so a + // web-safe image that reaches the exporter is re-encoded at that + // quality even though `imageSizeForUpload` leaves its dimensions + // alone. That mirrors `MediaImportService`, which maps the same + // settings the same way. if !settings.imageOptimizationEnabled, !settings.removeLocationSetting, let type, From f75b3285f086951d957a5e9bc9e105d554348beb Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Mon, 17 Aug 2026 09:20:55 -0400 Subject: [PATCH 13/29] refactor: set exportImageType only for image exports `makeExporter` is shared by the image and video paths, and it derived `exportImageType` from the source URL's type alone. A video's UTI is not in `webSafeImageTypes`, so every video export was configured to write JPEG. `MediaURLExporter.exportVideo` ignores `imageOptions`, so this had no effect, but it stated something untrue about the export. Pass the classification `processFile` already computed and set `exportImageType` only for an image. No behavior change. Co-Authored-By: Claude Opus 5 (1M context) --- .../NewGutenberg/GBKMediaUploadProcessor.swift | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift index 92c8ed4d2c08..3fface018092 100644 --- a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift +++ b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift @@ -107,7 +107,7 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { break } - let export = try await makeExporter(for: url, settings: settings).export() + let export = try await makeExporter(for: url, expected: expected, settings: settings).export() guard let mimeType = export.url.typeIdentifier.flatMap(UTType.init)?.preferredMIMEType else { throw MediaURLExporter.URLExportError.unknownFileUTI @@ -119,7 +119,11 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { /// Builds an exporter configured from the app's Media settings, mirroring /// the option mapping in `MediaImportService`. - private func makeExporter(for url: URL, settings: MediaSettings) -> MediaURLExporter { + private func makeExporter( + for url: URL, + expected: MediaURLExporter.URLExportExpectation, + settings: MediaSettings + ) -> MediaURLExporter { let exporter = MediaURLExporter(url: url) // GutenbergKit deletes the processed file after uploading it, so the // export is written to a temporary directory rather than the uploads @@ -136,7 +140,14 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { imageOptions.maximumImageSize = maximumImageSize(from: settings) imageOptions.stripsGeoLocationIfNeeded = settings.removeLocationSetting imageOptions.imageCompressionQuality = settings.imageQualityForUpload.doubleValue - if let type = url.typeIdentifier.flatMap(UTType.init), !Self.webSafeImageTypes.contains(type) { + // Only meaningful for an image export: `exportImageType` is the + // destination type `MediaImageExporter` writes, and it also determines + // the output file extension. `exportVideo` ignores `imageOptions`, so + // setting it for a video would be misleading rather than harmless. + if case .image = expected, + let type = url.typeIdentifier.flatMap(UTType.init), + !Self.webSafeImageTypes.contains(type) + { imageOptions.exportImageType = UTType.jpeg.identifier } exporter.imageOptions = imageOptions From 32f978ac7793b30a3c7cfdd86b5a4c12e95bc310 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Mon, 17 Aug 2026 10:28:32 -0400 Subject: [PATCH 14/29] test: make the shared export directory test meaningful `exportsShareOneTemporaryDirectory` cleaned up each export inside the loop body, so `defer` fired at the end of every iteration and deleted the file before the next export ran. `incrementalFilename` only increments while a file exists, so all three exports resolved to the same path and the directory set was trivially of size one. The test passed whether or not exports shared a directory. Collect the output URLs and clean them up after the loop so all three files coexist, and assert they are three distinct paths in one directory. Verified by reverting dd8c5bb5be locally: the test now fails with three directories instead of one. Co-Authored-By: Claude Opus 5 (1M context) --- .../Media/GBKMediaUploadProcessorTests.swift | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift b/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift index bbde2df27e16..193c5c7081e4 100644 --- a/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift +++ b/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift @@ -91,7 +91,12 @@ struct GBKMediaUploadProcessorTests { let processor = makeProcessor(settings: settings) let url = try fixtureURL("test-image-device-photo-gps.jpg") - var directories: Set = [] + // Keep every export on disk until the end: deleting each one before the + // next would free its filename, so all three would collide on the same + // path and the directory count would match even if exports did not + // share a directory. + var outputURLs: [URL] = [] + defer { outputURLs.forEach(cleanUp) } for _ in 0..<3 { let result = try await processor.processFile( at: url, @@ -102,11 +107,11 @@ struct GBKMediaUploadProcessorTests { Issue.record("Expected a processed file") return } - defer { cleanUp(outputURL) } - directories.insert(outputURL.deletingLastPathComponent()) + outputURLs.append(outputURL) } - #expect(directories.count == 1) + #expect(Set(outputURLs).count == 3) + #expect(Set(outputURLs.map { $0.deletingLastPathComponent() }).count == 1) } // MARK: - GIFs and other files From 0c823752a448b97a2b8c2bc2797d7def5bcf133a Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Mon, 17 Aug 2026 15:59:15 -0400 Subject: [PATCH 15/29] build: Update to GutenbergKit v0.20.0-alpha.0 --- Modules/Package.resolved | 6 +++--- Modules/Package.swift | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Modules/Package.resolved b/Modules/Package.resolved index 73622f5c7f4b..6a70c94bb00e 100644 --- a/Modules/Package.resolved +++ b/Modules/Package.resolved @@ -1,5 +1,5 @@ { - "originHash" : "97462f04f7472535df3293d3f7601aaff8a4684769091394d4b1ca5bde9f8ed1", + "originHash" : "61e4f4fbfd341181ca9b893646e997a1b3b76afe110c2500eb4b500401ef1e34", "pins" : [ { "identity" : "alamofire", @@ -131,8 +131,8 @@ "kind" : "remoteSourceControl", "location" : "https://github.com/wordpress-mobile/GutenbergKit", "state" : { - "branch" : "trunk", - "revision" : "16aceb17716dbef9fb2289d316e27dc812a569f6" + "revision" : "297fcb55e3694c4f325468386972ac89f414cc70", + "version" : "0.20.0-alpha.0" } }, { diff --git a/Modules/Package.swift b/Modules/Package.swift index a3fef2e8603a..ec15ba8a087f 100644 --- a/Modules/Package.swift +++ b/Modules/Package.swift @@ -62,7 +62,7 @@ let package = Package( revision: "b34794c9a3f32312e1593d4a3d120572afa0d010" ), .package(url: "https://github.com/zendesk/support_sdk_ios", from: "8.0.3"), - .package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "trunk"), + .package(url: "https://github.com/wordpress-mobile/GutenbergKit", from: "0.20.0-alpha.0"), .package( url: "https://github.com/automattic/wordpress-rs", exact: "0.6.0" From 54c444cf34a78e48f2b84a11b35b92c71e01c072 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Mon, 17 Aug 2026 20:36:00 -0400 Subject: [PATCH 16/29] build: sync root Package.resolved to GutenbergKit v0.20.0-alpha.0 The root cross-platform package still pinned GutenbergKit to the trunk branch, left over from tracking trunk for native media uploads. Modules moved to the 0.20.0-alpha.0 tag, so re-resolve the root graph to match. Co-Authored-By: Claude Opus 5 (1M context) --- Package.resolved | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Package.resolved b/Package.resolved index e72ab17bc880..bba5f23b413e 100644 --- a/Package.resolved +++ b/Package.resolved @@ -1,5 +1,5 @@ { - "originHash" : "e5b9b888f12b9e2adfe5e293101989f58ec52d16d70ef0002c28093d8c2ed39f", + "originHash" : "1bbbb11e671a32eda51b479a3e29fd18839bb400ff80395fba2ed7a8553e9baa", "pins" : [ { "identity" : "alamofire", @@ -131,8 +131,8 @@ "kind" : "remoteSourceControl", "location" : "https://github.com/wordpress-mobile/GutenbergKit", "state" : { - "branch" : "trunk", - "revision" : "16aceb17716dbef9fb2289d316e27dc812a569f6" + "revision" : "297fcb55e3694c4f325468386972ac89f414cc70", + "version" : "0.20.0-alpha.0" } }, { From 9d71b6901021e7df076217fbdef801c758d74a7d Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Tue, 18 Aug 2026 08:24:43 -0400 Subject: [PATCH 17/29] fix: harden media upload processing against type and cleanup gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three related defects in GBKMediaUploadProcessor, all in the path between classifying an upload and handing the export back to GutenbergKit. Uploads without a file extension failed outright. `expectedExport` resolves the type from the path extension alone, and GutenbergKit names its temp file after the multipart `filename`, which the editor does not guarantee carries an extension — its native inserter derives one from a URL path segment. Such a file resolves to `public.data`, which conforms to no media type, so the export was rejected and the editor saw a 500 for a file that previously uploaded fine. Fall back to the reported MIME type, which the delegate already receives and ignored. Exports also had to stop going through `MediaURLExporter`, which re-derives the type from the path extension in `exportURL` and would reject the file again after it was classified. Use the concrete exporter per branch instead: `MediaImageExporter` reads the type from the file's contents via `CGImageSourceGetType`, so an extensionless image exports correctly. Replace the fixed export directory with one per export. Destination names come from `URL.incrementalFilename()`, a check-then-act `fileExists` loop with no locking, and uploads are processed concurrently, so a shared directory let two exports of the same source name resolve to the same path and clobber each other. Nothing sweeps that directory either — GutenbergKit removes only the file it is handed, and `MediaFileManager` cleans just the uploads directory — so a failure after the export wrote a file abandoned a full-size payload for the lifetime of the container. Clean up on the failure path. Derive the reported MIME type from `exportImageType` rather than re-reading the output path, so it comes from the value the export was configured with. --- .../Media/GBKMediaUploadProcessorTests.swift | 144 ++++++++++-- .../GBKMediaUploadProcessor.swift | 210 +++++++++++++----- 2 files changed, 273 insertions(+), 81 deletions(-) diff --git a/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift b/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift index 193c5c7081e4..01e5ba82b333 100644 --- a/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift +++ b/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift @@ -81,37 +81,64 @@ struct GBKMediaUploadProcessorTests { #expect(filename.hasSuffix(".jpg") || filename.hasSuffix(".jpeg")) } - /// GutenbergKit removes the exported file but never its enclosing - /// directories, so exports must share one directory rather than each - /// creating its own. - @Test func exportsShareOneTemporaryDirectory() async throws { + /// Destination names come from a check-then-act `fileExists` loop, and + /// GutenbergKit processes uploads concurrently, so exports of the same + /// source must not share a directory to race in. + @Test func concurrentExportsOfTheSameFileDoNotCollide() async throws { let settings = makeSettings() settings.imageOptimizationEnabled = true settings.maxImageSizeSetting = 200 let processor = makeProcessor(settings: settings) let url = try fixtureURL("test-image-device-photo-gps.jpg") - // Keep every export on disk until the end: deleting each one before the - // next would free its filename, so all three would collide on the same - // path and the directory count would match even if exports did not - // share a directory. - var outputURLs: [URL] = [] - defer { outputURLs.forEach(cleanUp) } - for _ in 0..<3 { - let result = try await processor.processFile( - at: url, - mimeType: "image/jpeg", - filename: url.lastPathComponent - ) - guard case .processed(let outputURL, _, _) = result else { - Issue.record("Expected a processed file") - return + let outputURLs = try await withThrowingTaskGroup(of: URL.self) { group in + for _ in 0..<8 { + group.addTask { + let result = try await processor.processFile( + at: url, + mimeType: "image/jpeg", + filename: url.lastPathComponent + ) + guard case .processed(let outputURL, _, _) = result else { + throw ProcessingError.expectedProcessedFile + } + return outputURL + } } - outputURLs.append(outputURL) + return try await group.reduce(into: [URL]()) { $0.append($1) } + } + defer { outputURLs.forEach(cleanUp) } + + // Every export is its own file, and every one of them survived the + // others finishing rather than being overwritten or swept away. + #expect(Set(outputURLs).count == outputURLs.count) + for outputURL in outputURLs { + #expect(FileManager.default.fileExists(atPath: outputURL.path)) + #expect(max(try imageSize(at: outputURL).width, try imageSize(at: outputURL).height) == 200) + } + } + + /// A failed export must not leave its temporary directory behind: nothing + /// else sweeps it, so an abandoned export would outlive the app session. + /// + /// The video exporter throws after `makeLocalMediaURL` has already created + /// the directory, which is exactly what an implementation without the + /// failure-path cleanup would leak. + @Test func failedExportLeavesNoDirectoryBehind() async throws { + let directory = MediaDirectory.temporary(id: UUID()) + let processor = GBKMediaUploadProcessor( + videoDurationLimit: 1, + allowableFileExtensions: [], + makeMediaSettings: makeSettingsFactory(makeSettings()), + makeExportDirectory: { directory } + ) + let url = try fixtureURL("test-video-device-gps.m4v") + + await #expect(throws: (any Error).self) { + try await processor.processFile(at: url, mimeType: "video/mp4", filename: url.lastPathComponent) } - #expect(Set(outputURLs).count == 3) - #expect(Set(outputURLs.map { $0.deletingLastPathComponent() }).count == 1) + #expect(!FileManager.default.fileExists(atPath: directory.url.path)) } // MARK: - GIFs and other files @@ -168,6 +195,64 @@ struct GBKMediaUploadProcessorTests { } } + // MARK: - Files without an extension + + /// GutenbergKit names the temp file after the multipart `filename`, which + /// the editor does not guarantee carries an extension (its native inserter + /// derives one from a URL path segment). Such a file resolves to + /// `public.data`, so the reported MIME type has to stand in for the type. + @Test func extensionlessImageIsProcessedUsingReportedMIMEType() async throws { + let settings = makeSettings() + settings.imageOptimizationEnabled = true + settings.maxImageSizeSetting = 200 + let processor = makeProcessor(settings: settings) + let url = try copyFixtureDroppingExtension("test-image-device-photo-gps.jpg") + defer { cleanUp(url) } + + let result = try await processor.processFile( + at: url, + mimeType: "image/jpeg", + filename: url.lastPathComponent + ) + + guard case .processed(let outputURL, let mimeType, _) = result else { + Issue.record("Expected a processed file") + return + } + defer { cleanUp(outputURL) } + #expect(mimeType == "image/jpeg") + #expect(max(try imageSize(at: outputURL).width, try imageSize(at: outputURL).height) == 200) + } + + /// The fallback only applies when the URL yields no type of its own — a + /// mismatched MIME type must not override what the file actually is. + @Test func fileExtensionWinsOverReportedMIMEType() async throws { + let settings = makeSettings() + settings.imageOptimizationEnabled = false + settings.removeLocationSetting = false + let processor = makeProcessor(settings: settings) + let url = try fixtureURL("test-gif.gif") + + let result = try await processor.processFile(at: url, mimeType: "image/jpeg", filename: url.lastPathComponent) + + // Classified as a GIF from the extension, not as a JPEG from the + // reported type, so it passes through instead of being re-encoded. + guard case .original = result else { + Issue.record("Expected the original file to pass through") + return + } + } + + @Test func extensionlessFileWithUnusableMIMETypeThrows() async throws { + let processor = makeProcessor(settings: makeSettings()) + let url = try copyFixtureDroppingExtension("test-image-device-photo-gps.jpg") + defer { cleanUp(url) } + + await #expect(throws: MediaURLExporter.URLExportError.self) { + try await processor.processFile(at: url, mimeType: "not-a-mime-type", filename: url.lastPathComponent) + } + } + // MARK: - Videos @Test func videoExceedingDurationLimitThrows() async throws { @@ -210,6 +295,17 @@ struct GBKMediaUploadProcessorTests { return url } + /// Copies a fixture to a temporary file with no path extension, mirroring + /// an upload whose multipart `filename` carried none. + private func copyFixtureDroppingExtension(_ filename: String) throws -> URL { + let source = try fixtureURL(filename) + let destination = FileManager.default.temporaryDirectory + .appendingPathComponent(UUID().uuidString, isDirectory: false) + try FileManager.default.copyItem(at: source, to: destination) + #expect(destination.pathExtension.isEmpty) + return destination + } + private func imageProperties(at url: URL) throws -> [CFString: Any] { let source = try #require(CGImageSourceCreateWithURL(url as CFURL, nil)) let properties = try #require(CGImageSourceCopyPropertiesAtIndex(source, 0, nil) as? [CFString: Any]) @@ -226,6 +322,10 @@ struct GBKMediaUploadProcessorTests { private func cleanUp(_ url: URL) { try? FileManager.default.removeItem(at: url) } + + private enum ProcessingError: Error { + case expectedProcessedFile + } } /// Anchor for resolving the test bundle from Swift Testing suites. diff --git a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift index 3fface018092..1d3e22ca91cc 100644 --- a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift +++ b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift @@ -15,6 +15,20 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { private let allowableFileExtensions: Set private let makeMediaSettings: @Sendable () -> MediaSettings + /// The temporary directory an export is written to. + /// + /// GutenbergKit deletes the processed file after uploading it, so exports + /// go to a temporary directory rather than the uploads directory tracked by + /// `MediaFileManager`. + /// + /// Every export gets its own directory. Destination names come from + /// `URL.incrementalFilename()`, a check-then-act `fileExists` loop with no + /// locking, and uploads are processed concurrently — one task per + /// connection — so sharing a directory lets two exports of the same source + /// name resolve to the same path and clobber each other. A per-export + /// directory removes the shared state instead of racing on it. + private let makeExportDirectory: @Sendable () -> MediaDirectory + /// Raster image types the WordPress REST API reliably accepts. Other image /// formats (e.g. HEIC) are converted to JPEG during processing, mirroring /// `ItemProviderMediaExporter`. @@ -24,13 +38,6 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { /// exporter — `processFile` returns it unchanged (see below). private static let webSafeImageTypes: Set = [.png, .jpeg, .gif] - /// A fixed ID for the temporary directory exports are written to, so every - /// upload reuses the same directory instead of creating a new one. - /// - /// Stable across launches: a directory orphaned by a crash is reused rather - /// than accumulating alongside a new one. - private static let exportDirectoryID = UUID(uuidString: "1D8A4E5C-1F3B-4E7A-9C2D-6B0F8A5E3C71")! - @MainActor convenience init(blog: Blog) { // HEIC isn't supported when uploading an image, so we filter it out, @@ -47,17 +54,20 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { init( videoDurationLimit: TimeInterval?, allowableFileExtensions: Set, - makeMediaSettings: @escaping @Sendable () -> MediaSettings = { MediaSettings() } + makeMediaSettings: @escaping @Sendable () -> MediaSettings = { MediaSettings() }, + makeExportDirectory: @escaping @Sendable () -> MediaDirectory = { .temporary(id: UUID()) } ) { self.videoDurationLimit = videoDurationLimit self.allowableFileExtensions = allowableFileExtensions self.makeMediaSettings = makeMediaSettings + self.makeExportDirectory = makeExportDirectory } // MARK: - MediaUploadDelegate func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { - let expected = try MediaURLExporter.expectedExport(with: url) + let sourceType = Self.sourceType(of: url, reportedMIMEType: mimeType) + let expected = try Self.expectedExport(of: url, type: sourceType) let settings = makeMediaSettings() switch expected { @@ -76,12 +86,10 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { } return .original case .image: - let type = url.typeIdentifier.flatMap(UTType.init) - // SVG conforms to `UTType.image`, so it lands here, but ImageIO // cannot decode or encode it: the export would fail rather than // produce a file. Upload it unchanged, like a GIF. - if type == .svg { + if sourceType == .svg { return .original } @@ -96,8 +104,8 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { // settings the same way. if !settings.imageOptimizationEnabled, !settings.removeLocationSetting, - let type, - Self.webSafeImageTypes.contains(type) + let sourceType, + Self.webSafeImageTypes.contains(sourceType) { return .original } @@ -107,67 +115,151 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { break } - let export = try await makeExporter(for: url, expected: expected, settings: settings).export() + let exportImageType = Self.exportImageType(for: expected, sourceType: sourceType) + let directory = makeExportDirectory() - guard let mimeType = export.url.typeIdentifier.flatMap(UTType.init)?.preferredMIMEType else { - throw MediaURLExporter.URLExportError.unknownFileUTI + do { + let export = try await makeExporter( + for: url, + expected: expected, + settings: settings, + exportImageType: exportImageType, + directory: directory + ) + .export() + + let mimeType = try Self.mimeType(of: export.url, exportImageType: exportImageType) + return .processed(export.url, mimeType: mimeType, filename: export.url.lastPathComponent) + } catch { + // Nothing else sweeps this directory: GutenbergKit removes only the + // file it is handed, and `MediaFileManager`'s cleanup covers the + // uploads directory alone. On the success path the directory is + // left holding the file GutenbergKit is about to upload, but a + // failure here would otherwise abandon a full-size export — and any + // directory the export already created — for the lifetime of the + // app's container. + try? FileManager.default.removeItem(at: directory.url) + throw error } - return .processed(export.url, mimeType: mimeType, filename: export.url.lastPathComponent) } // MARK: - Exporter configuration /// Builds an exporter configured from the app's Media settings, mirroring /// the option mapping in `MediaImportService`. + /// + /// Returns the concrete exporter for the branch rather than + /// `MediaURLExporter`, which re-derives the type from the path extension in + /// `exportURL` and so would reject a file classified via its reported MIME + /// type. `MediaImageExporter` reads the type from the file's contents with + /// `CGImageSourceGetType`, so it handles an extensionless image correctly. private func makeExporter( for url: URL, expected: MediaURLExporter.URLExportExpectation, - settings: MediaSettings - ) -> MediaURLExporter { - let exporter = MediaURLExporter(url: url) - // GutenbergKit deletes the processed file after uploading it, so the - // export is written to a temporary directory rather than the uploads - // directory tracked by `MediaFileManager`. - // - // Reuse one directory for every export rather than `MediaDirectory - // .temporary`, which mints a new UUID per access. GutenbergKit removes - // only the exported file, never its enclosing directories, so a fresh - // UUID each time would leave an empty `tmp//Media/` behind for - // every upload. - exporter.mediaDirectoryType = .temporary(id: Self.exportDirectoryID) - - var imageOptions = MediaImageExporter.Options() - imageOptions.maximumImageSize = maximumImageSize(from: settings) - imageOptions.stripsGeoLocationIfNeeded = settings.removeLocationSetting - imageOptions.imageCompressionQuality = settings.imageQualityForUpload.doubleValue - // Only meaningful for an image export: `exportImageType` is the - // destination type `MediaImageExporter` writes, and it also determines - // the output file extension. `exportVideo` ignores `imageOptions`, so - // setting it for a video would be misleading rather than harmless. - if case .image = expected, - let type = url.typeIdentifier.flatMap(UTType.init), - !Self.webSafeImageTypes.contains(type) - { - imageOptions.exportImageType = UTType.jpeg.identifier + settings: MediaSettings, + exportImageType: UTType?, + directory: MediaDirectory + ) -> any MediaExporter { + switch expected { + case .video: + let exporter = MediaVideoExporter(url: url) + exporter.mediaDirectoryType = directory + var options = MediaVideoExporter.Options() + options.stripsGeoLocationIfNeeded = settings.removeLocationSetting + options.exportPreset = settings.maxVideoSizeSetting.videoPreset + options.durationLimit = videoDurationLimit + exporter.options = options + return exporter + case .image, .gif, .other: + // Only images reach the exporter: `.gif` and `.other` return + // `.original` before this point. + let exporter = MediaImageExporter(url: url) + exporter.mediaDirectoryType = directory + var options = MediaImageExporter.Options() + options.maximumImageSize = maximumImageSize(from: settings) + options.stripsGeoLocationIfNeeded = settings.removeLocationSetting + options.imageCompressionQuality = settings.imageQualityForUpload.doubleValue + // `exportImageType` is the destination type `MediaImageExporter` + // writes, and it also determines the output file extension. Left + // nil, the source type is kept. + options.exportImageType = exportImageType?.identifier + exporter.options = options + return exporter } - exporter.imageOptions = imageOptions - - var videoOptions = MediaVideoExporter.Options() - videoOptions.stripsGeoLocationIfNeeded = settings.removeLocationSetting - videoOptions.exportPreset = settings.maxVideoSizeSetting.videoPreset - videoOptions.durationLimit = videoDurationLimit - exporter.videoOptions = videoOptions - - var urlOptions = MediaURLExporter.Options() - urlOptions.allowableFileExtensions = allowableFileExtensions - urlOptions.stripsGeoLocationIfNeeded = settings.removeLocationSetting - exporter.urlOptions = urlOptions - - return exporter } private func maximumImageSize(from settings: MediaSettings) -> CGFloat? { let maxUploadSize = settings.imageSizeForUpload return maxUploadSize < Int.max ? CGFloat(maxUploadSize) : nil } + + // MARK: - Type resolution + + /// The type of the file to process. + /// + /// Resolved from the file itself, falling back to the type the editor + /// reported. The URL resolves its type from the path extension alone, and + /// an upload can arrive without one — GutenbergKit names the temp file + /// after the multipart `filename`, which the editor does not guarantee + /// carries an extension (its native inserter derives one from a URL path + /// segment). Such a file resolves to the generic `public.data`, which + /// conforms to no media type, so the export would be rejected outright. + private static func sourceType(of url: URL, reportedMIMEType: String) -> UTType? { + guard let type = url.typeIdentifier.flatMap(UTType.init), type != .data else { + return UTType(mimeType: reportedMIMEType) + } + return type + } + + /// Classifies a file the way `MediaURLExporter.expectedExport(with:)` does, + /// but from an already-resolved type so the caller can supply one the URL + /// alone cannot provide. + private static func expectedExport( + of url: URL, + type: UTType? + ) throws -> MediaURLExporter.URLExportExpectation { + guard url.isFileURL else { + throw MediaURLExporter.URLExportError.invalidFileURL + } + guard let type else { + throw MediaURLExporter.URLExportError.unknownFileUTI + } + if type == .gif { + return .gif + } else if type.conforms(to: .video) || type.conforms(to: .movie) { + return .video + } else if type.conforms(to: .image) { + return .image + } else if type.conforms(to: .content) || type.conforms(to: .zip) { + return .other + } + throw MediaURLExporter.URLExportError.unsupportedFileType + } + + /// The type `MediaImageExporter` should write, or `nil` to keep the source + /// type. Only image exports convert: everything the REST API accepts as-is + /// is left alone, and the rest becomes JPEG. + private static func exportImageType( + for expected: MediaURLExporter.URLExportExpectation, + sourceType: UTType? + ) -> UTType? { + guard case .image = expected, let sourceType else { + return nil + } + return webSafeImageTypes.contains(sourceType) ? nil : .jpeg + } + + /// The MIME type of a finished export. + /// + /// An image export writes `exportImageType` when set, so that value is + /// authoritative and needs no round-trip through the output path. Only the + /// cases that keep the source type — every video, and a web-safe image — + /// fall back to reading the file back. + private static func mimeType(of url: URL, exportImageType: UTType?) throws -> String { + let type = exportImageType ?? url.typeIdentifier.flatMap(UTType.init) + guard let mimeType = type?.preferredMIMEType else { + throw MediaURLExporter.URLExportError.unknownFileUTI + } + return mimeType + } } From 8b2acf37d2c450368945ce9203f8dcc5d262180e Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Tue, 18 Aug 2026 10:37:19 -0400 Subject: [PATCH 18/29] feat: skip the temp-file copy for uploads the processor won't touch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GutenbergKit asks the delegate, from the multipart headers alone, whether it will handle a file before streaming the upload to a temp file. The default is `true`, so every upload was materialized in full even when `processFile` immediately returned it unchanged — GIFs, and documents on a site with no extension restriction to enforce. Implement the gate for exactly those cases. It only declines where `processFile` returns `.original` for any Media settings, so it stays a fast path rather than a second place the policy lives; a parity test asserts that invariant across the settings matrix. Images and videos are always claimed: what happens to them depends on settings or on the file's contents, and declining is unrecoverable because the file is never seen again. Deciding from the reported MIME type needs it normalized first. `Content-Type` may carry parameters and arbitrary casing (RFC 9110 §8.3), and GutenbergKit's multipart parser substitutes `text/plain` for a part that sent no `Content-Type` at all (RFC 7578 §4.4) — it picks the file part by the presence of a `filename` parameter, not by content type. Left as-is, `image/jpeg; charset=binary` resolves to a dynamic type that conforms to nothing, and a real photo announced as `text/plain` looks like a document. Strip parameters, lower the casing, and treat the placeholder types as absent, falling back to the filename extension. `processFile` shares the helper, which also fixes the same misreading in its extensionless-upload fallback. Record why the image passthrough leaves EXIF orientation alone: the legacy exporter's unconditional normalization predates WordPress 5.3, whose `wp_create_image_subsizes` rotates server-side for every site, self-hosted included. Baking in a rotation the server performs anyway would cost a lossy re-encode of a photo the user asked not to optimize. --- .../Media/GBKMediaUploadProcessorTests.swift | 90 +++++++++++++++ .../GBKMediaUploadProcessor.swift | 103 +++++++++++++++++- 2 files changed, 190 insertions(+), 3 deletions(-) diff --git a/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift b/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift index 01e5ba82b333..91273aab7963 100644 --- a/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift +++ b/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift @@ -253,6 +253,96 @@ struct GBKMediaUploadProcessorTests { } } + // MARK: - handlesFile + + /// The invariant the metadata gate rests on: declining a file must mean + /// `processFile` would have returned it unchanged. If this fails, the gate + /// is skipping work that `processFile` would actually have done. + @Test(arguments: [true, false], [true, false]) + func decliningAFileImpliesProcessFileWouldNotTouchIt( + optimizationEnabled: Bool, + removeLocation: Bool + ) async throws { + let settings = makeSettings() + settings.imageOptimizationEnabled = optimizationEnabled + settings.removeLocationSetting = removeLocation + settings.maxImageSizeSetting = 200 + let processor = makeProcessor(settings: settings) + + let fixtures: [(filename: String, mimeType: String)] = [ + ("test-image-device-photo-gps.jpg", "image/jpeg"), + ("iphone-photo.heic", "image/heic"), + ("test-gif.gif", "image/gif"), + ("test-video-device-gps.m4v", "video/mp4") + ] + + for fixture in fixtures { + guard !processor.handlesFile(ofType: fixture.mimeType, named: fixture.filename) else { + continue + } + let url = try fixtureURL(fixture.filename) + let result = try await processor.processFile( + at: url, + mimeType: fixture.mimeType, + filename: fixture.filename + ) + guard case .original = result else { + Issue.record("Declined \(fixture.filename) but processFile would have processed it") + return + } + } + } + + @Test func gifIsDeclinedBeforeBeingCopiedToDisk() { + let processor = makeProcessor(settings: makeSettings()) + #expect(!processor.handlesFile(ofType: "image/gif", named: "animation.gif")) + } + + @Test func imagesAndVideosAreAlwaysClaimed() { + let processor = makeProcessor(settings: makeSettings()) + #expect(processor.handlesFile(ofType: "image/jpeg", named: "photo.jpg")) + #expect(processor.handlesFile(ofType: "image/heic", named: "photo.heic")) + #expect(processor.handlesFile(ofType: "video/mp4", named: "clip.mp4")) + } + + /// A document is only worth claiming when there is a restriction to + /// enforce; otherwise `processFile` returns it unchanged. + @Test func documentsAreClaimedOnlyToEnforceAllowedExtensions() { + let unrestricted = makeProcessor(settings: makeSettings()) + #expect(!unrestricted.handlesFile(ofType: "application/pdf", named: "doc.pdf")) + + let restricted = GBKMediaUploadProcessor( + videoDurationLimit: nil, + allowableFileExtensions: ["pdf"], + makeMediaSettings: makeSettingsFactory(makeSettings()) + ) + #expect(restricted.handlesFile(ofType: "application/pdf", named: "doc.pdf")) + } + + /// A part with no `Content-Type` arrives as `text/plain`, and a real + /// `Content-Type` may carry parameters or arbitrary casing. None of those + /// may cause a photo to be mistaken for a document and declined. + @Test( + arguments: [ + "image/jpeg; charset=binary", + "IMAGE/JPEG", + "text/plain", + "application/octet-stream", + "" + ] + ) + func imagesAreClaimedWhateverTheReportedMIMEType(mimeType: String) { + let processor = makeProcessor(settings: makeSettings()) + #expect(processor.handlesFile(ofType: mimeType, named: "photo.jpg")) + } + + /// Nothing decidable from the metadata means the file is claimed, so + /// `processFile` can read the type off the bytes instead. + @Test func unrecognizableMetadataIsClaimed() { + let processor = makeProcessor(settings: makeSettings()) + #expect(processor.handlesFile(ofType: "text/plain", named: "upload")) + } + // MARK: - Videos @Test func videoExceedingDurationLimitThrows() async throws { diff --git a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift index 1d3e22ca91cc..8e5bd1090ac9 100644 --- a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift +++ b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift @@ -65,6 +65,48 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { // MARK: - MediaUploadDelegate + /// Whether the file is worth materializing for `processFile`. + /// + /// GutenbergKit calls this from the multipart headers alone, before + /// streaming the upload to a temp file. Returning `false` skips that copy + /// and forwards the original request body to WordPress unchanged, so it is + /// only correct where `processFile` would return `.original` for *any* + /// Media settings — the metadata here cannot answer anything finer. + /// + /// This is a fast path, never a second place the policy lives: every `false` + /// below mirrors a branch of `processFile` that ignores `settings`. + /// Declining is also unrecoverable — the file is never seen again — so + /// anything undecidable from metadata claims the file and decides for real + /// once the bytes are on disk. + func handlesFile(ofType mimeType: String, named filename: String) -> Bool { + // The URL the file will be written to isn't available yet, so classify + // from the reported type alone, falling back to the filename extension + // when it is a placeholder. Both are untrustworthy in ways `processFile` + // can recover from and this cannot, hence the bias toward `true`. + guard let type = Self.type(ofMIMEType: mimeType) ?? Self.type(ofExtensionIn: filename) else { + return true + } + guard let expected = try? Self.expectedExport(of: nil, type: type) else { + return true + } + switch expected { + case .gif: + // Always returned unchanged, whatever the settings. + return false + case .other: + // Claim these only to enforce the site's allowed extensions, which + // `processFile` throws on. Declining would spend a full upload on a + // file the site rejects and surface the server's error instead of + // ours. With no restriction to enforce, there is nothing to do. + return !allowableFileExtensions.isEmpty + case .image, .video: + // An image may be downscaled, stripped, or converted, and a video + // is always exported. Both depend on settings or on the file's + // contents, so decide in `processFile`. + return true + } + } + func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { let sourceType = Self.sourceType(of: url, reportedMIMEType: mimeType) let expected = try Self.expectedExport(of: url, type: sourceType) @@ -102,6 +144,17 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { // quality even though `imageSizeForUpload` leaves its dimensions // alone. That mirrors `MediaImportService`, which maps the same // settings the same way. + // + // Skipping the export also skips the exporter's unconditional EXIF + // orientation normalization, so a sideways-shot photo uploads with + // its orientation flag intact rather than rotated into its pixels. + // That is deliberate: the normalization predates WordPress 5.3, + // whose `wp_create_image_subsizes` rotates on the server for every + // site, self-hosted included. Re-encoding here to bake in a + // rotation the server performs anyway would cost a lossy pass on a + // photo the user asked not to optimize. + // + // See WordPress-iOS#12703 and core changeset 46202. if !settings.imageOptimizationEnabled, !settings.removeLocationSetting, let sourceType, @@ -206,19 +259,63 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { /// conforms to no media type, so the export would be rejected outright. private static func sourceType(of url: URL, reportedMIMEType: String) -> UTType? { guard let type = url.typeIdentifier.flatMap(UTType.init), type != .data else { - return UTType(mimeType: reportedMIMEType) + return type(ofMIMEType: reportedMIMEType) } return type } + /// The type a reported MIME type names, or `nil` when it names nothing + /// usable. + /// + /// `UTType(mimeType:)` matches the bare `type/subtype` only, so the header + /// is normalized first. Two shapes reach us that it would otherwise miss: + /// + /// - Parameters and casing: `Content-Type` may carry parameters + /// (`image/jpeg; charset=binary`) and its casing is not significant + /// (RFC 9110 §8.3). Left as-is, both resolve to a dynamic UTType that + /// conforms to nothing. + /// - Placeholders: GutenbergKit's multipart parser defaults a part with no + /// `Content-Type` to `text/plain` (RFC 7578 §4.4), and it picks the file + /// part by the presence of a `filename` parameter rather than by content + /// type — so a real image can arrive labeled `text/plain`. Treating that + /// as authoritative would classify a photo as a document. + private static func type(ofMIMEType mimeType: String) -> UTType? { + let normalized = mimeType.prefix(while: { $0 != ";" }) + .trimmingCharacters(in: .whitespaces) + .lowercased() + guard !normalized.isEmpty, !placeholderMIMETypes.contains(normalized) else { + return nil + } + return UTType(mimeType: normalized) + } + + /// MIME types that carry no information about the file. `octet-stream` is + /// the generic "unknown bytes" type; `text/plain` is what GutenbergKit's + /// multipart parser substitutes for a part that sent no `Content-Type`. + private static let placeholderMIMETypes: Set = [ + "application/octet-stream", "text/plain" + ] + + /// The type a filename's extension names, for use before the file exists. + /// `processFile` reads the type off the file itself instead. + private static func type(ofExtensionIn filename: String) -> UTType? { + let fileExtension = (filename as NSString).pathExtension.lowercased() + guard !fileExtension.isEmpty else { + return nil + } + return UTType(filenameExtension: fileExtension) + } + /// Classifies a file the way `MediaURLExporter.expectedExport(with:)` does, /// but from an already-resolved type so the caller can supply one the URL /// alone cannot provide. + /// - Parameter url: The file being classified, or `nil` when only the type + /// is known — `handlesFile` runs before the file exists. private static func expectedExport( - of url: URL, + of url: URL?, type: UTType? ) throws -> MediaURLExporter.URLExportExpectation { - guard url.isFileURL else { + if let url, !url.isFileURL { throw MediaURLExporter.URLExportError.invalidFileURL } guard let type else { From 64c2e1666146787b3c68b50f603af3638f931f33 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Tue, 25 Aug 2026 10:22:47 -0400 Subject: [PATCH 19/29] fix: stop re-checking uploads against the site's cached file types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit processFile rejected a document whose extension was missing from both MediaImportService.defaultAllowableFileExtensions and the site's Blog.allowedFileTypes, mirroring MediaURLExporter.exportURL. That check belongs to the legacy picker, which hands over arbitrary files with nothing having vetted them. Here GutenbergKit has already validated the upload in the WebView against the site's real allowedMimeTypes from /wp-block-editor/v1/settings, so anything reaching processFile has passed the authoritative check. The second gate could therefore only ever produce false rejections: it cannot catch a type the editor missed, but it can disagree with the server and refuse a file the server would have accepted. Blog.allowedFileTypes is a cached blog option that can lag the site's actual configuration, and defaultAllowableFileExtensions is a 19-entry static list carrying no text types — so a .txt or .csv on a WP.com site with a populated allowed_file_types failed with "This file type is not allowed" where it previously uploaded. Drop the check and let the server's own error surface if one is warranted. Images and videos never reached it, so nothing that gets processed changes. handlesFile's .other branch existed only to claim documents for that check, so it now declines them unconditionally, joining .gif. Documents skip the full temp-file copy GutenbergKit writes before processFile — the copy only ever handed the file straight back. This deliberately diverges from the legacy editor. Parity matters where the two paths could produce different uploads; this gate only ever accepted or refused one, so removing it cannot make the GutenbergKit path emit a different file. The convenience init's HEIC removal goes with it: it only ever narrowed the allowlist. HEIC handling is unaffected — exportImageType converts it to JPEG, as heicIsConvertedToJPEG pins. --- .../Media/GBKMediaUploadProcessorTests.swift | 44 +++++++++--------- .../GBKMediaUploadProcessor.swift | 46 +++++++------------ 2 files changed, 37 insertions(+), 53 deletions(-) diff --git a/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift b/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift index 91273aab7963..450d4e08a971 100644 --- a/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift +++ b/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift @@ -128,7 +128,6 @@ struct GBKMediaUploadProcessorTests { let directory = MediaDirectory.temporary(id: UUID()) let processor = GBKMediaUploadProcessor( videoDurationLimit: 1, - allowableFileExtensions: [], makeMediaSettings: makeSettingsFactory(makeSettings()), makeExportDirectory: { directory } ) @@ -180,18 +179,25 @@ struct GBKMediaUploadProcessorTests { } } - @Test func disallowedFileExtensionThrows() async throws { - let processor = GBKMediaUploadProcessor( - videoDurationLimit: nil, - allowableFileExtensions: ["pdf"], - makeMediaSettings: makeSettingsFactory(makeSettings()) - ) + /// The editor validates uploads against the site's real `allowedMimeTypes` + /// before they reach the delegate, so the processor does not second-guess it + /// with `Blog.allowedFileTypes` — a cached option that can lag the server and + /// could only reject a file the server would have accepted. + @Test func documentPassesThroughWhateverTheSiteAllows() async throws { + let processor = makeProcessor(settings: makeSettings()) let url = FileManager.default.temporaryDirectory.appendingPathComponent("\(UUID().uuidString).txt") try "plain text".write(to: url, atomically: true, encoding: .utf8) defer { cleanUp(url) } - await #expect(throws: MediaURLExporter.URLExportError.self) { - try await processor.processFile(at: url, mimeType: "text/plain", filename: url.lastPathComponent) + let result = try await processor.processFile( + at: url, + mimeType: "text/plain", + filename: url.lastPathComponent + ) + + guard case .original = result else { + Issue.record("Expected the original file to pass through") + return } } @@ -305,18 +311,12 @@ struct GBKMediaUploadProcessorTests { #expect(processor.handlesFile(ofType: "video/mp4", named: "clip.mp4")) } - /// A document is only worth claiming when there is a restriction to - /// enforce; otherwise `processFile` returns it unchanged. - @Test func documentsAreClaimedOnlyToEnforceAllowedExtensions() { - let unrestricted = makeProcessor(settings: makeSettings()) - #expect(!unrestricted.handlesFile(ofType: "application/pdf", named: "doc.pdf")) - - let restricted = GBKMediaUploadProcessor( - videoDurationLimit: nil, - allowableFileExtensions: ["pdf"], - makeMediaSettings: makeSettingsFactory(makeSettings()) - ) - #expect(restricted.handlesFile(ofType: "application/pdf", named: "doc.pdf")) + /// `processFile` returns every document unchanged, so claiming one would + /// only spend a full temp-file copy to hand it straight back. + @Test func documentsAreDeclinedBeforeBeingCopiedToDisk() { + let processor = makeProcessor(settings: makeSettings()) + #expect(!processor.handlesFile(ofType: "application/pdf", named: "doc.pdf")) + #expect(!processor.handlesFile(ofType: "text/csv", named: "data.csv")) } /// A part with no `Content-Type` arrives as `text/plain`, and a real @@ -348,7 +348,6 @@ struct GBKMediaUploadProcessorTests { @Test func videoExceedingDurationLimitThrows() async throws { let processor = GBKMediaUploadProcessor( videoDurationLimit: 1, - allowableFileExtensions: [], makeMediaSettings: makeSettingsFactory(makeSettings()) ) let url = try fixtureURL("test-video-device-gps.m4v") @@ -363,7 +362,6 @@ struct GBKMediaUploadProcessorTests { private func makeProcessor(settings: MediaSettings) -> GBKMediaUploadProcessor { GBKMediaUploadProcessor( videoDurationLimit: nil, - allowableFileExtensions: [], makeMediaSettings: makeSettingsFactory(settings) ) } diff --git a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift index 8e5bd1090ac9..05df56324773 100644 --- a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift +++ b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift @@ -9,10 +9,9 @@ import WordPressData /// /// Assigned to `GutenbergKit.EditorViewController.mediaUploadDelegate`, which /// holds it weakly and invokes it off the main actor, so the type is `Sendable` -/// and snapshots the `Blog`-derived values it needs at initialization. +/// and snapshots the `Blog`-derived value it needs at initialization. final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { private let videoDurationLimit: TimeInterval? - private let allowableFileExtensions: Set private let makeMediaSettings: @Sendable () -> MediaSettings /// The temporary directory an export is written to. @@ -40,25 +39,15 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { @MainActor convenience init(blog: Blog) { - // HEIC isn't supported when uploading an image, so we filter it out, - // mirroring `MediaImportService`. - var allowedFileTypes = blog.allowedFileTypes - allowedFileTypes.remove("heic") - - self.init( - videoDurationLimit: blog.videoDurationLimit, - allowableFileExtensions: allowedFileTypes - ) + self.init(videoDurationLimit: blog.videoDurationLimit) } init( videoDurationLimit: TimeInterval?, - allowableFileExtensions: Set, makeMediaSettings: @escaping @Sendable () -> MediaSettings = { MediaSettings() }, makeExportDirectory: @escaping @Sendable () -> MediaDirectory = { .temporary(id: UUID()) } ) { self.videoDurationLimit = videoDurationLimit - self.allowableFileExtensions = allowableFileExtensions self.makeMediaSettings = makeMediaSettings self.makeExportDirectory = makeExportDirectory } @@ -90,15 +79,10 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { return true } switch expected { - case .gif: - // Always returned unchanged, whatever the settings. + case .gif, .other: + // Always returned unchanged, whatever the settings: only images and + // videos are processed. return false - case .other: - // Claim these only to enforce the site's allowed extensions, which - // `processFile` throws on. Declining would spend a full upload on a - // file the site rejects and surface the server's error instead of - // ours. With no restriction to enforce, there is nothing to do. - return !allowableFileExtensions.isEmpty case .image, .video: // An image may be downscaled, stripped, or converted, and a video // is always exported. Both depend on settings or on the file's @@ -117,15 +101,17 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { // GIFs are uploaded unchanged; processing would only copy the file. return .original case .other: - // Non-media files are uploaded unchanged, but enforce the site's - // allowed file extensions, mirroring `MediaURLExporter.exportURL`. - if let fileExtension = url.typeIdentifierFileExtension, - !MediaImportService.defaultAllowableFileExtensions.contains(fileExtension), - !allowableFileExtensions.isEmpty, - !allowableFileExtensions.contains(fileExtension) - { - throw MediaURLExporter.URLExportError.unsupportedFileType - } + // Non-media files are uploaded unchanged. + // + // Deliberately narrower than `MediaURLExporter.exportURL`, which + // also rejects extensions outside the site's allowed list. That + // check belongs to the legacy picker, which hands over arbitrary + // files with nothing having vetted them. Here the editor has already + // validated the file against the site's real `allowedMimeTypes` from + // `/wp-block-editor/v1/settings` before the upload reaches us, so + // re-checking against `Blog.allowedFileTypes` — a cached option that + // can lag the server — could only ever reject a file the server + // would have accepted. return .original case .image: // SVG conforms to `UTType.image`, so it lands here, but ImageIO From 4b7f01e70474b65a6efdf3a40e9f9e7f42de7708 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Tue, 25 Aug 2026 12:03:02 -0400 Subject: [PATCH 20/29] fix: keep the editor alive past its async load in the GBK editor tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both tests built a PostGBKEditorViewController in a local, called loadViewIfNeeded(), and returned. viewDidLoad kicks off prepareEditor() on a Task, so the load reaches startUploadServer() after the test method has already returned and released the controller. Once this branch assigns mediaUploadDelegate, that is fatal. GutenbergKit holds the delegate weakly and the controller is the only owner of its GBKMediaUploadProcessor, so the delegate deallocates with the controller while the editor itself is still alive inside the Task's [weak self]. That is exactly the state GutenbergKit's precondition exists to catch: precondition(!(mediaUploadDelegateWasAssigned && mediaUploadDelegate == nil), "mediaUploadDelegate was released before the editor loaded") The crash log from CI symbolicates to it directly — EXC_BREAKPOINT in _assertionFailure, called from startUploadServer() ← loadEditor(dependencies:) ← prepareEditor() ← closure #2 in viewDidLoad(). Because it kills the test host, the damage was not contained to this suite. Swift Testing runs separate suites concurrently, so the four suites in flight died with it (reported as "Crash: WordPress at ") and EditorConfigurationTests, torn down mid-test, read a Blog it never built and failed on "Bearer token" and a stray 643603.example.com host. Six failures, one cause, none of them in the suite responsible. Route both tests through a makeEditor(blog:) helper that retains each window — and through it the controller and processor — for the suite's lifetime. That restores the production invariant: the controller outlives its own load. Only the test's scope was shorter than the async work it started. This does not reproduce locally: whether the Task reaches startUploadServer() before or after the controller deallocates is a race, and a faster simulator wins it. It failed on all three CI retries. The suite becomes a final class so the retention array can be stored; Swift Testing instantiates the type per test either way, and .serialized is kept. --- .../PostGBKEditorViewControllerTests.swift | 45 ++++++++++++------- 1 file changed, 28 insertions(+), 17 deletions(-) diff --git a/Tests/KeystoneTests/Tests/Features/Gutenberg/PostGBKEditorViewControllerTests.swift b/Tests/KeystoneTests/Tests/Features/Gutenberg/PostGBKEditorViewControllerTests.swift index 9b9125c082f2..3a7a6b9c8e65 100644 --- a/Tests/KeystoneTests/Tests/Features/Gutenberg/PostGBKEditorViewControllerTests.swift +++ b/Tests/KeystoneTests/Tests/Features/Gutenberg/PostGBKEditorViewControllerTests.swift @@ -8,12 +8,25 @@ import UIKit @MainActor @Suite(.serialized) -struct PostGBKEditorViewControllerTests { +final class PostGBKEditorViewControllerTests { - @Test("presents the site media library for GutenbergKit requests") - func presentsSiteMediaLibrary() throws { - let context = ContextManager.forTesting().mainContext - let blog = BlogBuilder(context).build() + /// Keeps every editor built here alive for the whole suite. + /// + /// `viewDidLoad` starts `prepareEditor()` asynchronously, and that work + /// reaches `startUploadServer()` after the test method has returned. The + /// editor holds `mediaUploadDelegate` weakly, so if the controller — the + /// only owner of `PostGBKEditorViewController.mediaUploadProcessor` — is + /// released at scope exit, the delegate is gone by the time the load + /// arrives and GutenbergKit trips its "released before the editor loaded" + /// precondition, crashing the test host and taking every concurrently + /// running suite with it. + /// + /// Production ownership is correct: the controller outlives its own load. + /// Only the test's scope was shorter than the async work it kicked off. + private var retainedWindows: [UIWindow] = [] + + /// Builds an editor and retains it (via its window) for the suite's lifetime. + private func makeEditor(blog: Blog) -> PostGBKEditorViewController { let viewController = PostGBKEditorViewController( postId: nil, postType: .post, @@ -26,6 +39,15 @@ struct PostGBKEditorViewControllerTests { window.rootViewController = viewController window.makeKeyAndVisible() viewController.loadViewIfNeeded() + retainedWindows.append(window) + return viewController + } + + @Test("presents the site media library for GutenbergKit requests") + func presentsSiteMediaLibrary() throws { + let context = ContextManager.forTesting().mainContext + let blog = BlogBuilder(context).build() + let viewController = makeEditor(blog: blog) let data = Data( #"{"allowedTypes":["image"],"multiple":true,"value":[],"contextId":"test"}"#.utf8 @@ -108,18 +130,7 @@ struct PostGBKEditorViewControllerTests { for blog: Blog, requesting mediaIds: [Int] ) throws -> SiteMediaPickerViewController { - let viewController = PostGBKEditorViewController( - postId: nil, - postType: .post, - title: "", - content: "", - status: "draft", - blog: blog - ) - let window = UIWindow() - window.rootViewController = viewController - window.makeKeyAndVisible() - viewController.loadViewIfNeeded() + let viewController = makeEditor(blog: blog) let value = mediaIds.map(String.init).joined(separator: ",") let data = Data( From 0219c4e040d3b3118f9b3f211a039ea1e78d4cc4 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Tue, 25 Aug 2026 12:23:29 -0400 Subject: [PATCH 21/29] fix: resolve the upload type the same way in both delegate methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `handlesFile` resolved `MIME ?? extension` while `processFile`'s `sourceType(of:)` resolves `file ?? MIME`. The two classified differently whenever the signals disagreed. An upload named `photo.jpg` carrying a mislabeled `Content-Type: application/pdf` resolved to `com.adobe.pdf`, classified `.other`, and was declined — so it reached WordPress with no downscaling and, more importantly, no GPS stripping. `processFile` would have resolved `public.jpeg` from the extension and processed it. Declining is unrecoverable: GutenbergKit forwards the original request body and the file is never seen again. The gate therefore has to resolve in the same order `processFile` does, standing in for the not-yet-written file with the filename extension and falling back to the reported type. This does not weaken the bias toward claiming a file. An unknown extension resolves to a dynamic UTType that conforms to nothing, so `expectedExport` throws and `handlesFile` still returns `true`, exactly as before. The doc comment claimed "every `false` below mirrors a branch of `processFile` that ignores `settings`", which is what this restores. --- .../Media/GBKMediaUploadProcessorTests.swift | 22 +++++++++++++++++++ .../GBKMediaUploadProcessor.swift | 13 ++++++----- 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift b/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift index 450d4e08a971..f12fb815f03a 100644 --- a/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift +++ b/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift @@ -343,6 +343,28 @@ struct GBKMediaUploadProcessorTests { #expect(processor.handlesFile(ofType: "text/plain", named: "upload")) } + /// `handlesFile` must resolve the type the way `processFile` does — the + /// extension first — or a mislabeled `Content-Type` silently declines a + /// photo that would have been downscaled and stripped. Declining is + /// unrecoverable, so the two cannot disagree. + @Test func mislabeledContentTypeDoesNotDeclineAnImage() async throws { + let settings = makeSettings() + settings.imageOptimizationEnabled = true + settings.removeLocationSetting = true + settings.maxImageSizeSetting = 200 + let processor = makeProcessor(settings: settings) + + #expect(processor.handlesFile(ofType: "application/pdf", named: "photo.jpg")) + + // And the claim is warranted: `processFile` really does process it. + let url = try fixtureURL("test-image-device-photo-gps.jpg") + let result = try await processor.processFile(at: url, mimeType: "application/pdf", filename: "photo.jpg") + guard case .processed(let outputURL, _, _) = result else { + throw ProcessingError.expectedProcessedFile + } + cleanUp(outputURL) + } + // MARK: - Videos @Test func videoExceedingDurationLimitThrows() async throws { diff --git a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift index 05df56324773..9870bff3eca1 100644 --- a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift +++ b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift @@ -68,11 +68,14 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { /// anything undecidable from metadata claims the file and decides for real /// once the bytes are on disk. func handlesFile(ofType mimeType: String, named filename: String) -> Bool { - // The URL the file will be written to isn't available yet, so classify - // from the reported type alone, falling back to the filename extension - // when it is a placeholder. Both are untrustworthy in ways `processFile` - // can recover from and this cannot, hence the bias toward `true`. - guard let type = Self.type(ofMIMEType: mimeType) ?? Self.type(ofExtensionIn: filename) else { + // The file doesn't exist yet, so stand in for it with the filename + // extension and resolve in the same order `sourceType(of:)` does: the + // file's own type first, the reported one only as a fallback. Reversing + // the two here would let a mislabeled `Content-Type` decline a file + // `processFile` would have classified — and processed — from its + // extension. Both signals are untrustworthy in ways `processFile` can + // recover from and this cannot, hence the bias toward `true`. + guard let type = Self.type(ofExtensionIn: filename) ?? Self.type(ofMIMEType: mimeType) else { return true } guard let expected = try? Self.expectedExport(of: nil, type: type) else { From f976b7bd47f6df2514a269336a650e00661a3e73 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Tue, 25 Aug 2026 12:39:14 -0400 Subject: [PATCH 22/29] fix: upload processed media under the name the editor sent `processFile` returned `export.url.lastPathComponent`, and GutenbergKit sends that verbatim as the multipart `filename`, which WordPress turns into the attachment's slug and title. That name is not the user's. GutenbergKit streams the upload to a temp file it names `-`, and `MediaImageExporter(url:)` seeds its output name from `url.lastPathComponent`, so a photo picked as `IMG_1234.HEIC` uploaded as `09314a8e-06a3-4b32-9d0d-b07c299fad5b-img_1234.jpeg`. The clean `filename` was passed into `processFile` and never used. The `.original` path was unaffected, so the same photo got a clean name with optimization off and a UUID-laden one with it on. Take the basename from the caller and only the extension from the export, since a conversion (HEIC to JPEG, MOV to MP4) still has to be reflected in the name. One consequence worth noting: the extension is normalized to the type's preferred form, so a `.jpg` upload is now named `.jpeg` even when the format does not change. The extension always matching the bytes is worth more than preserving the spelling. The existing test asserted only `hasPrefix("test-image-device-photo-gps")`, which passed because it called `processFile` with a fixture URL directly rather than through the `-` name the server actually produces. The new test routes through that name. --- .../Media/GBKMediaUploadProcessorTests.swift | 58 +++++++++++++++++++ .../GBKMediaUploadProcessor.swift | 31 +++++++++- 2 files changed, 88 insertions(+), 1 deletion(-) diff --git a/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift b/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift index f12fb815f03a..747f30ee0ccf 100644 --- a/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift +++ b/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift @@ -259,6 +259,49 @@ struct GBKMediaUploadProcessorTests { } } + // MARK: - Output naming + + /// GutenbergKit names the temp file it hands over `-`, and + /// the returned name becomes the attachment's slug and title. The name the + /// editor sent must survive processing, or the UUID ends up in both. + @Test func processedFileKeepsTheNameTheEditorSent() async throws { + let settings = makeSettings() + settings.imageOptimizationEnabled = true + settings.maxImageSizeSetting = 200 + let processor = makeProcessor(settings: settings) + let url = try copyFixture("test-image-device-photo-gps.jpg", as: "\(UUID().uuidString)-img_1234.jpg") + defer { cleanUp(url) } + + let result = try await processor.processFile(at: url, mimeType: "image/jpeg", filename: "IMG_1234.jpg") + + guard case .processed(let outputURL, _, let filename) = result else { + throw ProcessingError.expectedProcessedFile + } + defer { cleanUp(outputURL) } + // The basename is the editor's, with no trace of the UUID the temp file + // carried. The extension is the export's — `.jpg` normalizes to the + // type's preferred `.jpeg` even though the format did not change. + #expect(filename == "IMG_1234.jpeg") + } + + /// A conversion changes the bytes, so the extension has to follow them even + /// though the basename does not. + @Test func convertedFileKeepsItsNameButTakesTheExportExtension() async throws { + let settings = makeSettings() + settings.imageOptimizationEnabled = true + let processor = makeProcessor(settings: settings) + let url = try fixtureURL("iphone-photo.heic") + + let result = try await processor.processFile(at: url, mimeType: "image/heic", filename: "IMG_1234.HEIC") + + guard case .processed(let outputURL, let mimeType, let filename) = result else { + throw ProcessingError.expectedProcessedFile + } + defer { cleanUp(outputURL) } + #expect(mimeType == "image/jpeg") + #expect(filename == "IMG_1234.jpeg") + } + // MARK: - handlesFile /// The invariant the metadata gate rests on: declining a file must mean @@ -405,6 +448,21 @@ struct GBKMediaUploadProcessorTests { return url } + /// Copies a fixture to a temporary file under a different name, mirroring + /// the `-` temp file GutenbergKit hands to `processFile`. + private func copyFixture(_ filename: String, as destinationName: String) throws -> URL { + let source = try fixtureURL(filename) + let destination = FileManager.default.temporaryDirectory + .appendingPathComponent(UUID().uuidString, isDirectory: true) + .appendingPathComponent(destinationName, isDirectory: false) + try FileManager.default.createDirectory( + at: destination.deletingLastPathComponent(), + withIntermediateDirectories: true + ) + try FileManager.default.copyItem(at: source, to: destination) + return destination + } + /// Copies a fixture to a temporary file with no path extension, mirroring /// an upload whose multipart `filename` carried none. private func copyFixtureDroppingExtension(_ filename: String) throws -> URL { diff --git a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift index 9870bff3eca1..e266d7e1dfd6 100644 --- a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift +++ b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift @@ -171,7 +171,11 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { .export() let mimeType = try Self.mimeType(of: export.url, exportImageType: exportImageType) - return .processed(export.url, mimeType: mimeType, filename: export.url.lastPathComponent) + return .processed( + export.url, + mimeType: mimeType, + filename: Self.uploadFilename(original: filename, exportURL: export.url) + ) } catch { // Nothing else sweeps this directory: GutenbergKit removes only the // file it is handed, and `MediaFileManager`'s cleanup covers the @@ -185,6 +189,31 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { } } + // MARK: - Output naming + + /// The name the processed file is uploaded under. + /// + /// Keeps the name the editor sent, which is the one the user recognizes, + /// and takes only the extension from the export — a conversion (HEIC to + /// JPEG, MOV to MP4) changes it, and the extension must match the bytes. + /// + /// The export's own name is unusable here: `MediaImageExporter(url:)` seeds + /// it from `url.lastPathComponent`, and that URL is the temp file + /// GutenbergKit named `-`. Uploading that verbatim would + /// make the UUID part of the attachment's slug and title. + private static func uploadFilename(original: String, exportURL: URL) -> String { + let name = (original as NSString).lastPathComponent + let base = (name as NSString).deletingPathExtension + guard !base.isEmpty else { + return exportURL.lastPathComponent + } + let exportExtension = exportURL.pathExtension + guard !exportExtension.isEmpty else { + return base + } + return "\(base).\(exportExtension)" + } + // MARK: - Exporter configuration /// Builds an exporter configured from the app's Media settings, mirroring From 729377233ee8f05856d131f9b808c4af3fff9db0 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Tue, 25 Aug 2026 12:41:27 -0400 Subject: [PATCH 23/29] fix: write every export to one reused temporary directory `makeExportDirectory()` minted `//Media/` per export and only the failure path removed it, so every processed upload left an empty directory behind for the lifetime of the app's container. Nothing else reclaims them: GutenbergKit deletes only the file it was handed, its own `cleanOrphanedUploads` sweep scans just `GutenbergKit-uploads`, and `MediaFileManager`'s cleanup covers `.uploads`. Write every export to a single directory identified by a fixed ID. Created once and reused, it never accumulates, so no cleanup code is needed at all. The per-export directory existed to avoid a race in `incrementalFilename()`, an unlocked check-then-act `fileExists` loop, on the assumption that two concurrent exports could resolve to the same name. They cannot: GutenbergKit writes each upload to `-` and both exporters name their output after that `lastPathComponent`, so every export name is already unique before the loop is consulted. The test that covered the race passed the same source URL to every task, a shape production never produces; it now uses the prefixed names the editor actually sends. Nor is any failure-path cleanup needed. GutenbergKit removes the file it was handed on both the success and the failure path, so the only uncovered case is a crash mid-upload. Those files land in the directory the next session reuses, and iOS reclaims the temporary directory under storage pressure and across app updates. --- .../Media/GBKMediaUploadProcessorTests.swift | 48 +++++------ .../GBKMediaUploadProcessor.swift | 80 ++++++++----------- 2 files changed, 50 insertions(+), 78 deletions(-) diff --git a/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift b/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift index 747f30ee0ccf..32274c2d4260 100644 --- a/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift +++ b/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift @@ -81,23 +81,33 @@ struct GBKMediaUploadProcessorTests { #expect(filename.hasSuffix(".jpg") || filename.hasSuffix(".jpeg")) } - /// Destination names come from a check-then-act `fileExists` loop, and - /// GutenbergKit processes uploads concurrently, so exports of the same - /// source must not share a directory to race in. - @Test func concurrentExportsOfTheSameFileDoNotCollide() async throws { + /// Destination names come from a check-then-act `fileExists` loop with no + /// locking, and GutenbergKit processes uploads concurrently, so concurrent + /// exports sharing one directory must still resolve to distinct files. + /// + /// They do because GutenbergKit writes each upload to `-` + /// and the exporters name their output after that, so every export name is + /// already unique before the loop is consulted. The sources here carry that + /// prefix, as they do in production. + @Test func concurrentExportsDoNotCollide() async throws { let settings = makeSettings() settings.imageOptimizationEnabled = true settings.maxImageSizeSetting = 200 let processor = makeProcessor(settings: settings) - let url = try fixtureURL("test-image-device-photo-gps.jpg") + + let sources = try (0..<8) + .map { _ in + try copyFixture("test-image-device-photo-gps.jpg", as: "\(UUID().uuidString)-photo.jpg") + } + defer { sources.forEach(cleanUp) } let outputURLs = try await withThrowingTaskGroup(of: URL.self) { group in - for _ in 0..<8 { + for source in sources { group.addTask { let result = try await processor.processFile( - at: url, + at: source, mimeType: "image/jpeg", - filename: url.lastPathComponent + filename: "photo.jpg" ) guard case .processed(let outputURL, _, _) = result else { throw ProcessingError.expectedProcessedFile @@ -118,28 +128,6 @@ struct GBKMediaUploadProcessorTests { } } - /// A failed export must not leave its temporary directory behind: nothing - /// else sweeps it, so an abandoned export would outlive the app session. - /// - /// The video exporter throws after `makeLocalMediaURL` has already created - /// the directory, which is exactly what an implementation without the - /// failure-path cleanup would leak. - @Test func failedExportLeavesNoDirectoryBehind() async throws { - let directory = MediaDirectory.temporary(id: UUID()) - let processor = GBKMediaUploadProcessor( - videoDurationLimit: 1, - makeMediaSettings: makeSettingsFactory(makeSettings()), - makeExportDirectory: { directory } - ) - let url = try fixtureURL("test-video-device-gps.m4v") - - await #expect(throws: (any Error).self) { - try await processor.processFile(at: url, mimeType: "video/mp4", filename: url.lastPathComponent) - } - - #expect(!FileManager.default.fileExists(atPath: directory.url.path)) - } - // MARK: - GIFs and other files @Test func gifPassesThroughUntouched() async throws { diff --git a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift index e266d7e1dfd6..88a4415fed5b 100644 --- a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift +++ b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift @@ -14,20 +14,20 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { private let videoDurationLimit: TimeInterval? private let makeMediaSettings: @Sendable () -> MediaSettings - /// The temporary directory an export is written to. + /// The temporary directory exports are written to. /// - /// GutenbergKit deletes the processed file after uploading it, so exports - /// go to a temporary directory rather than the uploads directory tracked by - /// `MediaFileManager`. + /// One directory, reused: GutenbergKit deletes the file it was handed on + /// both the success and the failure path, so nothing here needs cleanup. A + /// fresh directory per export would, since nothing sweeps those. /// - /// Every export gets its own directory. Destination names come from - /// `URL.incrementalFilename()`, a check-then-act `fileExists` loop with no - /// locking, and uploads are processed concurrently — one task per - /// connection — so sharing a directory lets two exports of the same source - /// name resolve to the same path and clobber each other. A per-export - /// directory removes the shared state instead of racing on it. + /// Sharing it is safe despite `incrementalFilename()`'s unlocked + /// check-then-act loop: GutenbergKit writes each upload to + /// `-` and the exporters name their output after it, so + /// concurrent exports cannot resolve to the same name. private let makeExportDirectory: @Sendable () -> MediaDirectory + private static let exportDirectoryID = UUID(uuidString: "1D8A4E5C-1F3B-4E7A-9C2D-6B0F8A5E3C71")! + /// Raster image types the WordPress REST API reliably accepts. Other image /// formats (e.g. HEIC) are converted to JPEG during processing, mirroring /// `ItemProviderMediaExporter`. @@ -45,7 +45,9 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { init( videoDurationLimit: TimeInterval?, makeMediaSettings: @escaping @Sendable () -> MediaSettings = { MediaSettings() }, - makeExportDirectory: @escaping @Sendable () -> MediaDirectory = { .temporary(id: UUID()) } + makeExportDirectory: @escaping @Sendable () -> MediaDirectory = { + .temporary(id: GBKMediaUploadProcessor.exportDirectoryID) + } ) { self.videoDurationLimit = videoDurationLimit self.makeMediaSettings = makeMediaSettings @@ -158,49 +160,31 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { } let exportImageType = Self.exportImageType(for: expected, sourceType: sourceType) - let directory = makeExportDirectory() - - do { - let export = try await makeExporter( - for: url, - expected: expected, - settings: settings, - exportImageType: exportImageType, - directory: directory - ) - .export() + let export = try await makeExporter( + for: url, + expected: expected, + settings: settings, + exportImageType: exportImageType, + directory: makeExportDirectory() + ) + .export() - let mimeType = try Self.mimeType(of: export.url, exportImageType: exportImageType) - return .processed( - export.url, - mimeType: mimeType, - filename: Self.uploadFilename(original: filename, exportURL: export.url) - ) - } catch { - // Nothing else sweeps this directory: GutenbergKit removes only the - // file it is handed, and `MediaFileManager`'s cleanup covers the - // uploads directory alone. On the success path the directory is - // left holding the file GutenbergKit is about to upload, but a - // failure here would otherwise abandon a full-size export — and any - // directory the export already created — for the lifetime of the - // app's container. - try? FileManager.default.removeItem(at: directory.url) - throw error - } + let mimeType = try Self.mimeType(of: export.url, exportImageType: exportImageType) + return .processed( + export.url, + mimeType: mimeType, + filename: Self.uploadFilename(original: filename, exportURL: export.url) + ) } // MARK: - Output naming - /// The name the processed file is uploaded under. - /// - /// Keeps the name the editor sent, which is the one the user recognizes, - /// and takes only the extension from the export — a conversion (HEIC to - /// JPEG, MOV to MP4) changes it, and the extension must match the bytes. + /// The name the processed file is uploaded under, which WordPress turns + /// into the attachment's slug and title. /// - /// The export's own name is unusable here: `MediaImageExporter(url:)` seeds - /// it from `url.lastPathComponent`, and that URL is the temp file - /// GutenbergKit named `-`. Uploading that verbatim would - /// make the UUID part of the attachment's slug and title. + /// The editor's name, with the extension from the export because a + /// conversion changes it. The export's own name carries the UUID prefix + /// GutenbergKit gave the temp file, so it can't be used. private static func uploadFilename(original: String, exportURL: URL) -> String { let name = (original as NSString).lastPathComponent let base = (name as NSString).deletingPathExtension From 6eb2f329e7462420428a608096cc29870bc883c3 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Tue, 25 Aug 2026 12:42:50 -0400 Subject: [PATCH 24/29] perf: decline SVG uploads before copying them to disk MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `processFile` returns `.original` for SVG whatever the settings, because ImageIO can neither decode nor encode it. By the metadata gate's own rule — decline anything `processFile` returns unchanged for any settings — SVG qualifies. It was claimed anyway: `image/svg+xml` resolves to `public.svg-image`, which conforms to `UTType.image`, so the gate fell into the image branch and returned `true`. GutenbergKit then streamed the whole body to a temp file only for `processFile` to hand it straight back, which is exactly the copy the gate exists to avoid. --- .../Media/GBKMediaUploadProcessorTests.swift | 9 +++++++++ .../NewGutenberg/GBKMediaUploadProcessor.swift | 15 +++++++++++---- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift b/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift index 32274c2d4260..49ee0f1f6a0f 100644 --- a/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift +++ b/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift @@ -335,6 +335,15 @@ struct GBKMediaUploadProcessorTests { #expect(!processor.handlesFile(ofType: "image/gif", named: "animation.gif")) } + /// SVG conforms to `UTType.image`, but ImageIO cannot decode it, so + /// `processFile` returns it unchanged for any settings. Claiming it would + /// buy a full temp-file copy that is handed straight back. + @Test func svgIsDeclinedBeforeBeingCopiedToDisk() { + let processor = makeProcessor(settings: makeSettings()) + #expect(!processor.handlesFile(ofType: "image/svg+xml", named: "logo.svg")) + #expect(!processor.handlesFile(ofType: "text/plain", named: "logo.svg")) + } + @Test func imagesAndVideosAreAlwaysClaimed() { let processor = makeProcessor(settings: makeSettings()) #expect(processor.handlesFile(ofType: "image/jpeg", named: "photo.jpg")) diff --git a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift index 88a4415fed5b..8b521339166b 100644 --- a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift +++ b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift @@ -88,10 +88,17 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { // Always returned unchanged, whatever the settings: only images and // videos are processed. return false - case .image, .video: - // An image may be downscaled, stripped, or converted, and a video - // is always exported. Both depend on settings or on the file's - // contents, so decide in `processFile`. + case .image: + // SVG conforms to `UTType.image` but is returned unchanged for any + // settings, because ImageIO cannot decode it (see `processFile`). + // Declining it skips a temp-file copy that could never be used. + // + // Every other image may be downscaled, stripped, or converted + // depending on settings and on the file's contents, so decide in + // `processFile`. + return type != .svg + case .video: + // Always exported, to apply the preset and duration limit. return true } } From 1635ae3a2b017ed088d4a9ce1a31b80c66222875 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Tue, 25 Aug 2026 13:22:05 -0400 Subject: [PATCH 25/29] refactor: drop the unreachable .gif entry from webSafeImageTypes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both sites that read the set sit behind an `.image` classification: the skip-export check in `processFile` and `exportImageType`, which guards on `case .image`. `expectedExport` returns `.gif` for a GIF before it ever reaches the `.image` branch, so no GIF can reach either one. The member was therefore dead. Its doc comment reasoned carefully about why SVG was absent while carrying an entry that could not be hit, which is a misleading signal for anyone editing the set later. Rewrite the comment to say what the set is actually for — the types that reach the image branch — and name GIF and SVG together as the two web-safe formats deliberately handled earlier. No behavior change. --- .../NewGutenberg/GBKMediaUploadProcessor.swift | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift index 8b521339166b..fe6904004700 100644 --- a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift +++ b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift @@ -32,10 +32,12 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { /// formats (e.g. HEIC) are converted to JPEG during processing, mirroring /// `ItemProviderMediaExporter`. /// - /// - Note: SVG is deliberately absent. It is web-safe, but it is a vector - /// format that ImageIO cannot decode or encode, so it never reaches the - /// exporter — `processFile` returns it unchanged (see below). - private static let webSafeImageTypes: Set = [.png, .jpeg, .gif] + /// Only consulted for an `.image` export, so it lists just the types that + /// reach that branch. GIF and SVG are web-safe too but are absent: both + /// return `.original` before any of this is read — GIF from its own + /// `expectedExport` case, SVG because ImageIO can neither decode nor encode + /// it. + private static let webSafeImageTypes: Set = [.png, .jpeg] @MainActor convenience init(blog: Blog) { From 13822ba3ea040d1742e3d456ebfe0f98d1c32c38 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Tue, 25 Aug 2026 16:25:40 -0400 Subject: [PATCH 26/29] test: retain GBK editors past the async load they start MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit viewDidLoad starts the editor's load on a Task that reaches startUploadServer() after the test method has returned, where GutenbergKit asserts an assigned mediaUploadDelegate is still alive. Since this branch assigns that delegate, a controller released at scope exit leaves the editor reachable with a dead delegate and trips the precondition. That kills the test host, so the fallout lands elsewhere: concurrently running suites report "Crash: WordPress at ", and suites torn down mid-test report assertion failures against state they never set up. Six failures, none of them in the suite responsible. Only a test can reach this. PostGBKEditorViewController holds the editor and its GBKMediaUploadProcessor as strong `let`s on one object, so in the app they always die together and the load's [weak self] on the editor is already nil. Here a window is the controller's only owner, so releasing it mid-load separates the two lifetimes. There is no production path. Move retention to file scope. An earlier attempt stored the windows on the suite itself, which does not work: Swift Testing builds a fresh suite instance per test, so the array was released the moment a test returned — the very deadline being missed. This is hardening, not a build fix. The race is timing-dependent and CI is currently green with the hazard still present, so no red-to-green demonstration is possible; a passing run does not prove it is gone. The failure mode is expensive enough — a host crash that corrupts unrelated suites — to be worth closing regardless. --- .../PostGBKEditorViewControllerTests.swift | 45 +++++++++++-------- 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/Tests/KeystoneTests/Tests/Features/Gutenberg/PostGBKEditorViewControllerTests.swift b/Tests/KeystoneTests/Tests/Features/Gutenberg/PostGBKEditorViewControllerTests.swift index 3a7a6b9c8e65..ea8d4bf980f4 100644 --- a/Tests/KeystoneTests/Tests/Features/Gutenberg/PostGBKEditorViewControllerTests.swift +++ b/Tests/KeystoneTests/Tests/Features/Gutenberg/PostGBKEditorViewControllerTests.swift @@ -6,26 +6,35 @@ import UIKit @testable import WordPress @testable import WordPressData +/// Keeps every editor these tests build alive for the lifetime of the process. +/// +/// `viewDidLoad` starts the editor's load on a `Task` that reaches +/// `startUploadServer()` after the test method returns, where GutenbergKit +/// asserts an assigned `mediaUploadDelegate` is still alive. Tripping that +/// precondition crashes the test host, killing every concurrently running suite +/// — so the damage is not contained to this file. +/// +/// Only a test can reach that state. `PostGBKEditorViewController` holds the +/// editor and its `GBKMediaUploadProcessor` as strong `let`s on one object, so +/// in the app they always die together and the load's `[weak self]` is already +/// nil. Here a window is the controller's only owner, so releasing it mid-load +/// leaves the editor reachable with a dead delegate. +/// +/// Retention must outlive the *suite instance*: Swift Testing builds a fresh +/// one per test, so an instance property dies at the very deadline being +/// missed. The race is timing-dependent, so a green run does not prove the +/// hazard is gone. @MainActor -@Suite(.serialized) -final class PostGBKEditorViewControllerTests { +private enum RetainedEditors { + static var windows: [UIWindow] = [] +} - /// Keeps every editor built here alive for the whole suite. - /// - /// `viewDidLoad` starts `prepareEditor()` asynchronously, and that work - /// reaches `startUploadServer()` after the test method has returned. The - /// editor holds `mediaUploadDelegate` weakly, so if the controller — the - /// only owner of `PostGBKEditorViewController.mediaUploadProcessor` — is - /// released at scope exit, the delegate is gone by the time the load - /// arrives and GutenbergKit trips its "released before the editor loaded" - /// precondition, crashing the test host and taking every concurrently - /// running suite with it. - /// - /// Production ownership is correct: the controller outlives its own load. - /// Only the test's scope was shorter than the async work it kicked off. - private var retainedWindows: [UIWindow] = [] +@MainActor +@Suite(.serialized) +struct PostGBKEditorViewControllerTests { - /// Builds an editor and retains it (via its window) for the suite's lifetime. + /// Builds an editor, retaining it for the process's lifetime so the load it + /// starts cannot outlive its delegate. See ``RetainedEditors``. private func makeEditor(blog: Blog) -> PostGBKEditorViewController { let viewController = PostGBKEditorViewController( postId: nil, @@ -39,7 +48,7 @@ final class PostGBKEditorViewControllerTests { window.rootViewController = viewController window.makeKeyAndVisible() viewController.loadViewIfNeeded() - retainedWindows.append(window) + RetainedEditors.windows.append(window) return viewController } From b039696a6a9995a3b21d13d33b7876e2dc20e1de Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Wed, 26 Aug 2026 10:22:54 -0400 Subject: [PATCH 27/29] fix: classify uploads whose extension no UTI declares `sourceType(of:reportedMIMEType:)` fell back to the reported MIME type only when the URL resolved to exactly `public.data`. An extension nothing declares resolves instead to a *dynamic* type synthesized from it, which clears that check while conforming to no media type, so `expectedExport` threw `unsupportedFileType` and the upload failed with a 500. `photo.jfif` is the case that matters: a plain JPEG WordPress accepts, whose `Content-Type` says `image/jpeg`, uploaded fine before the editor was given a media upload delegate. Defer to the reported type for a dynamic type as well, and discard it in `type(ofExtensionIn:)` too so `handlesFile` keeps classifying the file the same way `processFile` does. --- .../Media/GBKMediaUploadProcessorTests.swift | 41 +++++++++++++++++++ .../GBKMediaUploadProcessor.swift | 35 ++++++++++++---- 2 files changed, 69 insertions(+), 7 deletions(-) diff --git a/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift b/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift index 49ee0f1f6a0f..654ce969ed9c 100644 --- a/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift +++ b/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift @@ -237,6 +237,47 @@ struct GBKMediaUploadProcessorTests { } } + /// An extension no UTI declares resolves to a *dynamic* type rather than to + /// `public.data`, so it clears a `!= .data` check while still conforming to + /// nothing. `.jfif` is a plain JPEG WordPress accepts, and the reported + /// `image/jpeg` says so, so it has to be processed like any other JPEG + /// instead of failing the upload outright. + @Test func undeclaredExtensionIsProcessedUsingReportedMIMEType() async throws { + let settings = makeSettings() + settings.imageOptimizationEnabled = true + settings.maxImageSizeSetting = 200 + let processor = makeProcessor(settings: settings) + let url = try copyFixture("test-image-device-photo-gps.jpg", as: "photo.jfif") + defer { cleanUp(url) } + // Guards the premise: if `jfif` ever gains a real declaration this test + // stops exercising the dynamic-type path. + #expect(try #require(url.typeIdentifier.flatMap(UTType.init)).isDynamic) + + let result = try await processor.processFile( + at: url, + mimeType: "image/jpeg", + filename: "photo.jfif" + ) + + guard case .processed(let outputURL, let mimeType, _) = result else { + throw ProcessingError.expectedProcessedFile + } + defer { cleanUp(outputURL) } + #expect(mimeType == "image/jpeg") + #expect(max(try imageSize(at: outputURL).width, try imageSize(at: outputURL).height) == 200) + } + + /// `handlesFile` resolves the type from the extension, which is dynamic for + /// `.jfif` too. It has to discard it the same way, or the file is claimed + /// (or declined) on a classification `processFile` does not share. + @Test func undeclaredExtensionIsClaimedFromItsReportedMIMEType() { + let processor = makeProcessor(settings: makeSettings()) + #expect(processor.handlesFile(ofType: "image/jpeg", named: "photo.jfif")) + // Nothing decidable from either signal still means claim, so + // `processFile` gets to read the bytes. + #expect(processor.handlesFile(ofType: "text/plain", named: "photo.jfif")) + } + @Test func extensionlessFileWithUnusableMIMETypeThrows() async throws { let processor = makeProcessor(settings: makeSettings()) let url = try copyFixtureDroppingExtension("test-image-device-photo-gps.jpg") diff --git a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift index fe6904004700..f9db40a726ee 100644 --- a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift +++ b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift @@ -263,18 +263,33 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { /// /// Resolved from the file itself, falling back to the type the editor /// reported. The URL resolves its type from the path extension alone, and - /// an upload can arrive without one — GutenbergKit names the temp file - /// after the multipart `filename`, which the editor does not guarantee - /// carries an extension (its native inserter derives one from a URL path - /// segment). Such a file resolves to the generic `public.data`, which - /// conforms to no media type, so the export would be rejected outright. + /// two kinds of upload defeat that: + /// + /// - No extension at all. GutenbergKit names the temp file after the + /// multipart `filename`, which the editor does not guarantee carries an + /// extension (its native inserter derives one from a URL path segment). + /// Such a file resolves to the generic `public.data`. + /// - An extension no UTI declares. `photo.jfif` is a plain JPEG WordPress + /// accepts, but nothing claims `jfif`, so it resolves to a *dynamic* type + /// synthesized from the extension (`dyn.ah62d4rv4ge80y3xmq2`). + /// + /// Neither conforms to any media type, so `expectedExport` would throw and + /// fail an upload that the reported `Content-Type` describes perfectly. + /// Both therefore defer to it — for `photo.jfif`, `image/jpeg`. private static func sourceType(of url: URL, reportedMIMEType: String) -> UTType? { - guard let type = url.typeIdentifier.flatMap(UTType.init), type != .data else { + guard let type = url.typeIdentifier.flatMap(UTType.init), !isUninformative(type) else { return type(ofMIMEType: reportedMIMEType) } return type } + /// Whether a type says nothing about the file's format and should give way + /// to the reported MIME type. See `sourceType(of:reportedMIMEType:)`. + private static func isUninformative(_ type: UTType) -> Bool { + type == .data || type.isDynamic + } + + /// The type a reported MIME type names, or `nil` when it names nothing /// usable. /// @@ -309,12 +324,18 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { /// The type a filename's extension names, for use before the file exists. /// `processFile` reads the type off the file itself instead. + /// + /// Discards a dynamic type for the same reason `sourceType` does, and to + /// stay in step with it: an extension no UTI declares must fall through to + /// the reported MIME type in both places, or `handlesFile` would classify a + /// `.jfif` from a type that conforms to nothing while `processFile` + /// classifies the same file as the JPEG it is. private static func type(ofExtensionIn filename: String) -> UTType? { let fileExtension = (filename as NSString).pathExtension.lowercased() guard !fileExtension.isEmpty else { return nil } - return UTType(filenameExtension: fileExtension) + return UTType(filenameExtension: fileExtension).flatMap { isUninformative($0) ? nil : $0 } } /// Classifies a file the way `MediaURLExporter.expectedExport(with:)` does, From ca6cda9f5c4c1c087fb9c54251818777f159763f Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Wed, 26 Aug 2026 10:27:51 -0400 Subject: [PATCH 28/29] fix: pass through video AVFoundation cannot read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The video branch exported unconditionally, but it is reached by UTType conformance to `.video` or `.movie`, which is broader than the set of containers AVFoundation opens. WebM and WMV both conform to `.movie` and are absent from `AVURLAsset.audiovisualTypes()`, so `MediaVideoExporter` failed them on `guard asset.isExportable` and the upload returned a 500. Both formats are ones WordPress accepts — `wmv` is in the app's own `MediaImportService.defaultAllowableFileExtensions` — and both uploaded fine before the editor was given a media upload delegate. Return them unchanged instead, and decline them in `handlesFile` so the temp copy is skipped too. The export preset, duration limit, and location stripping are unavailable for these files either way; failing the upload to say so would only reject media the server would have taken. Types are matched by conformance rather than identity so a subtype of a readable format is not swept up with the unreadable ones. --- .../Media/GBKMediaUploadProcessorTests.swift | 32 +++++++++++++++ .../GBKMediaUploadProcessor.swift | 39 ++++++++++++++++--- 2 files changed, 66 insertions(+), 5 deletions(-) diff --git a/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift b/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift index 654ce969ed9c..ec2417d4f651 100644 --- a/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift +++ b/Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift @@ -448,6 +448,38 @@ struct GBKMediaUploadProcessorTests { // MARK: - Videos + /// WebM and WMV conform to `UTType.movie`, so they classify as video, but + /// AVFoundation cannot open either — `MediaVideoExporter` fails them on + /// `AVURLAsset.isExportable`. Exporting them anyway would turn uploads + /// WordPress accepts into errors, so they pass through instead. + @Test(arguments: [("clip.webm", "video/webm"), ("clip.wmv", "video/x-ms-wmv")]) + func videoAVFoundationCannotReadPassesThroughUntouched( + filename: String, + mimeType: String + ) async throws { + let processor = makeProcessor(settings: makeSettings()) + // Contents are never read: the type alone decides, before any export. + let url = try copyFixture("test-video-device-gps.m4v", as: filename) + defer { cleanUp(url) } + + let result = try await processor.processFile(at: url, mimeType: mimeType, filename: filename) + + guard case .original = result else { + Issue.record("Expected \(filename) to pass through unprocessed") + return + } + // And it is declined up front, so the temp copy is skipped entirely. + #expect(!processor.handlesFile(ofType: mimeType, named: filename)) + } + + /// The formats AVFoundation does read must still be claimed and exported, + /// or the preset and duration limit stop being applied to real video. + @Test(arguments: ["clip.mp4", "clip.mov", "clip.m4v", "clip.avi"]) + func readableVideoIsStillClaimed(filename: String) { + let processor = makeProcessor(settings: makeSettings()) + #expect(processor.handlesFile(ofType: "video/mp4", named: filename)) + } + @Test func videoExceedingDurationLimitThrows() async throws { let processor = GBKMediaUploadProcessor( videoDurationLimit: 1, diff --git a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift index f9db40a726ee..12902302c6c9 100644 --- a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift +++ b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift @@ -1,3 +1,4 @@ +import AVFoundation import Foundation import GutenbergKit import UniformTypeIdentifiers @@ -100,8 +101,10 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { // `processFile`. return type != .svg case .video: - // Always exported, to apply the preset and duration limit. - return true + // Exported to apply the preset and duration limit — except for the + // containers AVFoundation cannot read, which `processFile` returns + // unchanged for any settings (see there). + return Self.isExportableVideoType(type) } } @@ -163,9 +166,16 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { return .original } case .video: - // Always process video to apply the export preset, duration - // limit, and location stripping. - break + // AVFoundation cannot open every container that conforms to + // `.movie`, and `MediaVideoExporter` rejects the ones it cannot + // read rather than passing them along. Upload those unchanged: the + // preset, duration limit, and location stripping are all + // unavailable for them, and failing the upload to say so would + // reject files WordPress accepts (`wmv` is even in the app's own + // `MediaImportService.defaultAllowableFileExtensions`). + if let sourceType, !Self.isExportableVideoType(sourceType) { + return .original + } } let exportImageType = Self.exportImageType(for: expected, sourceType: sourceType) @@ -289,6 +299,25 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { type == .data || type.isDynamic } + /// Whether AVFoundation can read the container, and so whether + /// `MediaVideoExporter` can export it at all. + /// + /// `expectedExport` routes anything conforming to `.video` or `.movie` to + /// the video branch, which is broader than what AVFoundation opens: WebM + /// and WMV both conform to `.movie` but are absent from + /// `audiovisualTypes()`, and `MediaVideoExporter` fails them on + /// `AVURLAsset.isExportable`. + /// + /// Matched by conformance rather than identity, because a file can be a + /// subtype of a listed format without appearing in the list itself: + /// DRM-wrapped MPEG-4 conforms to `public.mpeg-4` but is absent. + private static func isExportableVideoType(_ type: UTType) -> Bool { + exportableVideoTypes.contains { type.conforms(to: $0) } + } + + private static let exportableVideoTypes: Set = Set( + AVURLAsset.audiovisualTypes().compactMap { UTType($0.rawValue) } + ) /// The type a reported MIME type names, or `nil` when it names nothing /// usable. From 6526986cbffe7c26139af01d8fbf43200b8544b2 Mon Sep 17 00:00:00 2001 From: David Calhoun Date: Wed, 26 Aug 2026 12:46:38 -0400 Subject: [PATCH 29/29] docs: correct the upload validation claim in the GBK processor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment said the editor had already validated the file against the site's `allowedMimeTypes` from `/wp-block-editor/v1/settings` before the upload reached the processor. It has not: that route is provided by the Gutenberg plugin, not WordPress core, so on a site without the plugin the setting stays nil and the editor's check passes everything through. State that instead, and keep the reason the branch is still correct — a cached `Blog.allowedFileTypes` can lag the server, so rejecting from it could only refuse a file the server would accept. --- .../GBKMediaUploadProcessor.swift | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift index 12902302c6c9..be7bde2d0e21 100644 --- a/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift +++ b/WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift @@ -123,12 +123,19 @@ final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { // Deliberately narrower than `MediaURLExporter.exportURL`, which // also rejects extensions outside the site's allowed list. That // check belongs to the legacy picker, which hands over arbitrary - // files with nothing having vetted them. Here the editor has already - // validated the file against the site's real `allowedMimeTypes` from - // `/wp-block-editor/v1/settings` before the upload reaches us, so - // re-checking against `Blog.allowedFileTypes` — a cached option that - // can lag the server — could only ever reject a file the server - // would have accepted. + // files with nothing having vetted them. + // + // Nothing vets the file here either — the editor validates uploads + // against `allowedMimeTypes`, but that value reaches it only from + // `/wp-block-editor/v1/settings`, a route the Gutenberg plugin + // provides and WordPress core does not. Without it the setting stays + // nil and the check passes everything through. + // + // Re-checking here would not recover it. `Blog.allowedFileTypes` is + // a cached option that can lag the server, so rejecting from it + // could only ever refuse a file the server would have accepted. The + // server is the authority, and it rejects with a message the native + // upload relay passes back to the editor verbatim. return .original case .image: // SVG conforms to `UTType.image`, so it lands here, but ImageIO