Add UploadSourceMaterializer with policy-driven transforms - #25620
Conversation
Generated by 🚫 Danger |
🤖 Build Failure AnalysisThis build has failures. Claude has analyzed them - check the build annotations for details. |
f85702c to
fde43a0
Compare
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 33084 | |
| Version | PR #25620 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 18466f5 | |
| Installation URL | 3lnoh3qkfu9io |
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 33084 | |
| Version | PR #25620 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 18466f5 | |
| Installation URL | 7tltjlrcvs9d0 |
323dddf to
56a1ac7
Compare
56a1ac7 to
7e76794
Compare
610e467 to
0c14222
Compare
The flag is the single Remove Location setting: it strips GPS EXIF from images and selects the identifying-metadata filter for re-exported video, so the name should not imply it is image-only.
Turns an upload source into an uploadable file: image normalization, video re-export, geolocation strip, and remote-URL download, all gated by the injected MediaUploadPolicy. Images are validated, type-sniffed, converted, resized, and GPS-stripped in a single ImageIO pass that preserves the EXIF orientation tag whenever pixels are not resampled. GIF and SVG never re-encode: one shared raw-passthrough rule covers the photo library, file importer, and remote sources. Includes the filename allocator, remote downloader, materializer error type, and their unit tests.
0c14222 to
f548d7a
Compare
ProcessInfo.systemUptime measures time since device boot, so the sweep cutoff spared staging directories orphaned by an earlier run in the same boot session. Capture a per-run reference date on the first sweep instead, which the app schedules shortly after launch, before any upload is staged.
URLSession's async download hands back a temp file the caller owns and must delete. A non-2xx status or a failed move threw without removing it, leaking the file in the system temp directory on every failed remote pick.
| let location: URL | ||
| let response: URLResponse | ||
| do { | ||
| (location, response) = try await URLSession.shared.download(from: url, delegate: delegate) |
There was a problem hiding this comment.
This should probably use an ephemeral session – we can get really weird behaviour by using shared (like sending login cookies with requests when we don't want to)
There was a problem hiding this comment.
sending login cookies with requests when we don't want to
Do you mean sending WP.com cookies to other sites? I don't think URLSession would do that? It would send WP.com cookies to WP.com URLs, which seems harmless.
Nonetheless I have switched to use ephemeral session in a25f413
There was a problem hiding this comment.
For context: Automattic/Automattic-Tracks-iOS#300
| ) | ||
| throw MaterializerError.remoteDownloadFailed(underlyingError: statusError) | ||
| } | ||
|
|
There was a problem hiding this comment.
Potential fun bug – some servers will return an HTML 200 response for an image URL (for instance, to avoid hot linking). It may be worth reading the magic bytes of the top of the file to validate that it's actually an image?
There was a problem hiding this comment.
In practice, this downloading code only runs on UploadSource.RemoteURL, which is produced by "Stock Photos" from https://www.pexels.com/ (endpoint: https://public-api.wordpress.com/rest/v1/meta/external-media/pexels?search=cat). It's probably not likely that they'd return a 200 html page when the url is xxx.jpeg?
If they do, the image processing code is likely to catch the issue because the content can be decoded as an image. However, we do have code that skip image processing (like for gif). Still, I think we can trust the "Stock Photos" provider to return a sensible html response if an image url does not return the requested image?
| contentType: UTType, | ||
| stem: String | ||
| ) throws -> MaterializedUpload { | ||
| let data = try Data(contentsOf: sourceURL) |
There was a problem hiding this comment.
IIRC, we pass paths to the Rust layer for upload, so do we really need to load the entire image into memory?
| parentDir: parentDir | ||
| ) | ||
| } else if contentType.conforms(to: .image) { | ||
| let data = try Data(contentsOf: localFile) |
There was a problem hiding this comment.
Same question about whether we need to load this into memory
There was a problem hiding this comment.
We need to process the image so that the uploaded image matches user settings, like removing GPS, compression rate, etc (see MediaUploadPolicy).
| } | ||
|
|
||
| // MARK: - end-to-end | ||
|
|
There was a problem hiding this comment.
| @Test("the basename for a very long source name is writable to disk") | |
| func longNameBasenameIsWritable() throws { | |
| let dir = URL(fileURLWithPath: NSTemporaryDirectory()) | |
| .appendingPathComponent(UUID().uuidString, isDirectory: true) | |
| try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) | |
| defer { try? FileManager.default.removeItem(at: dir) } | |
| let stem = allocator.stem( | |
| preferred: String(repeating: "a", count: 300), | |
| fallbackPrefix: "Photo", | |
| date: Date() | |
| ) | |
| let dest = dir.appendingPathComponent(allocator.basename(stem: stem, ext: "jpg")) | |
| try Data("x".utf8).write(to: dest) // throws ENAMETOOLONG today → test fails | |
| } |
Claude suggested that there's no limit to the filename path component size and we should clamp it.
There was a problem hiding this comment.
I got the same comment in my local review, but decided to ignore it considering it's not likely to have a 300 characters long file name. But it's probably pretty trivia to address the concern and the one suggestedName: "../escaped" below.
| ) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Claude flagged an issue with path traversal:
| @Test("remote dispatch keeps output inside parentDir for a traversing suggestedName") | |
| func remoteDispatchContainsTraversingName() async throws { | |
| let parentDir = try makeTempDir() | |
| let sourceFile = parentDir.appendingPathComponent("download.tmp") | |
| try encodeImage(makeSolidColorImage(size: CGSize(width: 10, height: 10), color: .red), as: .jpeg) | |
| .write(to: sourceFile) | |
| let result = try await makeMaterializer(policy()) | |
| .dispatchRemoteDownload( | |
| localFile: sourceFile, | |
| contentType: .jpeg, | |
| suggestedName: "../escaped", | |
| caption: nil, | |
| parentDir: parentDir | |
| ) | |
| let parent = parentDir.standardizedFileURL.path | |
| // The finalized file must stay INSIDE parentDir. | |
| #expect( | |
| result.tempFileURL.standardizedFileURL.path.hasPrefix(parent + "/"), | |
| "materialized file escaped parentDir: \(result.tempFileURL.standardizedFileURL.path)" | |
| ) | |
| // The documented cleanup target must be parentDir itself, never its parent — | |
| // deleting it must not wipe sibling uploads in the staging root. | |
| #expect(result.stagingDirectory.standardizedFileURL.path == parent) | |
| } |
The materialized temp-file name drives the uploaded filename, so an over-long source name could overflow NAME_MAX (ENAMETOOLONG) and a name carrying path separators could escape the staging directory via appendingPathComponent. Move both guards into basename, the single point every source path funnels through, so the .remoteURL and .imagePlayground paths that skip stem are covered too: sanitize separators and NUL, then cap the stem to a byte budget on a grapheme boundary.
|
Version |
|
|
||
| #expect(!FileManager.default.fileExists(atPath: orphaned.path)) | ||
| #expect(FileManager.default.fileExists(atPath: fresh.path)) | ||
| } |
There was a problem hiding this comment.
Two failing repro tests for defects in this materializer (verified on an iOS sim — these three fail; the rest of WordPressMediaLibraryTests stays green).
1. Animated GIF flattened when the declared type isn't GIF. finalizeImage chooses raw-passthrough vs. transform from the declared type, then trusts the sniffed actualType for everything else — but never re-checks actualType against rawPassthroughImageTypes. So GIF bytes arriving as .jpg on disk (or a remote image/jpeg) get CGImageDestinationAddImageFromSource'd into a single-frame JPEG, violating the documented "GIF/SVG must never round-trip through ImageIO" invariant. One-line fix: after sniffing actualType, divert rawPassthroughImageTypes.contains(actualType) to the raw copy.
- The two GIF tests assert the animation is preserved (stays
.gif, 2 frames). If you'd rather reject a mislabeled GIF, they flip toexpectThrowsCase— the "preserve" contract is deliberate (correctly-labeled GIFs already raw-copy). - The flatten requires
convertHEICToJPEG: true(the default here) and relies on.contentTypeKeybeing extension-based; the remote variant is self-contained (no OS-typing dependency).
2. Remote image branch strands the raw download. dispatchRemoteDownload's image branch reads localFile and writes a new finalized file but never deletes the original, so the staging dir holds ~2x the bytes until it's swept. The GIF/SVG branch consumes its input via moveItem; the image branch should too. The test pins the fix to this seam (delete localFile here, not a blanket defer in materializeRemoteURL).
Suggestion inserts the three tests + two fixture helpers after the existing suite; they're red until the fixes land.
| } | |
| } | |
| // MARK: - Raw-passthrough routing and staging cleanup | |
| /// GIF/SVG "must never round-trip through ImageIO" (documented invariant). | |
| /// Passthrough routing keys on the *declared* type, but `finalizeImage` | |
| /// trusts the *sniffed* type and never re-checks it against | |
| /// `rawPassthroughImageTypes`. An animated GIF that arrives with a non-GIF | |
| /// declared type — here, saved on disk as `.jpg` (a routine messaging-app / | |
| /// re-download artifact) — is re-encoded to a single-frame JPEG, silently | |
| /// dropping the animation. The `#require` proves the input really is | |
| /// animated, so a single-frame output is unambiguously a flatten. | |
| @Test("animated GIF mislabeled .jpg is not flattened") | |
| func mislabeledFileGIFNotFlattened() async throws { | |
| let url = try writeTempFixture(makeAnimatedGIF(frameCount: 2), ext: "jpg") | |
| try #require(imageFrameCount(of: url) == 2) | |
| let result = try await makeMaterializer(policy()).materialize(source: .file(url), into: stage()) | |
| #expect(imageType(of: result.tempFileURL) == .gif) | |
| #expect(imageFrameCount(of: result.tempFileURL) == 2) | |
| } | |
| /// Self-contained variant of the same flatten through the remote-dispatch | |
| /// seam, where the declared `contentType` is whatever the caller supplies. | |
| @Test("remote GIF declared image/jpeg is not flattened") | |
| func remoteMislabeledGIFNotFlattened() async throws { | |
| let parentDir = try makeTempDir() | |
| let src = parentDir.appendingPathComponent("download.tmp") | |
| try makeAnimatedGIF(frameCount: 2).write(to: src) | |
| try #require(imageFrameCount(of: src) == 2) | |
| let result = try await makeMaterializer(policy()) | |
| .dispatchRemoteDownload( | |
| localFile: src, | |
| contentType: .jpeg, | |
| suggestedName: "clip", | |
| caption: nil, | |
| parentDir: parentDir | |
| ) | |
| #expect(imageType(of: result.tempFileURL) == .gif) | |
| #expect(imageFrameCount(of: result.tempFileURL) == 2) | |
| } | |
| /// The `.remoteURL` image branch reads the downloaded temp file and writes a | |
| /// *new* finalized file, but never deletes the original download, leaving | |
| /// both in the staging dir (~2x disk). The GIF/SVG passthrough branch | |
| /// consumes its input with `moveItem`; the image branch should too. | |
| @Test("remote image branch does not strand the raw download") | |
| func remoteImageBranchConsumesRawDownload() async throws { | |
| let parentDir = try makeTempDir() | |
| // Mirror RemoteDownloader: bytes already inside parentDir under a | |
| // URLSession-style temp name. | |
| let rawDownload = parentDir.appendingPathComponent("CFNetworkDownload_ABC.tmp") | |
| let jpeg = try encodeImage( | |
| makeSolidColorImage(size: CGSize(width: 16, height: 16), color: .red), | |
| as: .jpeg | |
| ) | |
| try jpeg.write(to: rawDownload) | |
| let result = try await makeMaterializer(policy()) | |
| .dispatchRemoteDownload( | |
| localFile: rawDownload, | |
| contentType: .jpeg, | |
| suggestedName: "photo", | |
| caption: nil, | |
| parentDir: parentDir | |
| ) | |
| #expect(FileManager.default.fileExists(atPath: result.tempFileURL.path)) | |
| #expect(!FileManager.default.fileExists(atPath: rawDownload.path)) | |
| let contents = try FileManager.default.contentsOfDirectory( | |
| at: parentDir, | |
| includingPropertiesForKeys: nil | |
| ) | |
| #expect( | |
| contents.count == 1, | |
| "staging dir holds \(contents.count) files: \(contents.map(\.lastPathComponent))" | |
| ) | |
| } | |
| /// Multi-frame animated GIF built via ImageIO, so a preserved animation | |
| /// (`CGImageSourceGetCount > 1`) can be told apart from a flattened single frame. | |
| private func makeAnimatedGIF(frameCount: Int) throws -> Data { | |
| let out = NSMutableData() | |
| guard | |
| let dst = CGImageDestinationCreateWithData(out, UTType.gif.identifier as CFString, frameCount, nil) | |
| else { throw FixtureError.encodingFailed } | |
| CGImageDestinationSetProperties( | |
| dst, | |
| [kCGImagePropertyGIFDictionary: [kCGImagePropertyGIFLoopCount: 0]] as CFDictionary | |
| ) | |
| let colors: [UIColor] = [.red, .blue, .green, .yellow] | |
| let frameProps = [kCGImagePropertyGIFDictionary: [kCGImagePropertyGIFDelayTime: 0.1]] as CFDictionary | |
| for i in 0..<frameCount { | |
| guard | |
| let cg = makeSolidColorImage(size: CGSize(width: 8, height: 8), color: colors[i % colors.count]).cgImage | |
| else { throw FixtureError.imageUnavailable } | |
| CGImageDestinationAddImage(dst, cg, frameProps) | |
| } | |
| guard CGImageDestinationFinalize(dst) else { throw FixtureError.encodingFailed } | |
| return out as Data | |
| } | |
| private func imageFrameCount(of url: URL) -> Int { | |
| guard let src = CGImageSourceCreateWithURL(url as CFURL, nil) else { return 0 } | |
| return CGImageSourceGetCount(src) | |
| } |
|
@jkmassel I have captured the three issues in the Linear project. |
4 participants
Description
The main changes is introducing
UploadSourceMaterializerwhich turns anUploadSourceinto an uploadable temp file plusMediaCreateParams, which is then uploaded to the site via wordpress-rs.