Skip to content

[Android] MediaPicker: Stop overwriting picked source files - #36572

Merged
kubaflo merged 3 commits into
net11.0from
mattleibow-net10-android-mediapicker-security-fix
Aug 9, 2026
Merged

[Android] MediaPicker: Stop overwriting picked source files#36572
kubaflo merged 3 commits into
net11.0from
mattleibow-net10-android-mediapicker-security-fix

Conversation

@mattleibow

@mattleibow mattleibow commented Jul 14, 2026

Copy link
Copy Markdown
Member

Note

Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!

Description

On Android, when the MediaPicker applies EXIF auto-rotation or compression/resizing to a picked image, it mutated the picked source file in place — it deleted the original and wrote a replacement at the same resolved path:

  • RotateImageInPlaceFile.Delete(filePath) + File.Create(filePath)
  • CompressImageIfNeededoriginalFile.Delete() + writes a replacement (sometimes with a changed extension, PNG→JPEG)

That path comes from FileSystemUtils.EnsurePhysicalPath, which for a file:// URI returns uri.Path verbatim. That file is not one MAUI owns — it can be the user's original photo in the gallery, a file on an SD card, or a file in another app's shared storage. Picking an image should never modify the user's original.

There was also a data-loss window: the old code deleted the original before writing the processed replacement, so if the write threw, the original was already gone.

Because the image is loaded fully into memory, processed, and written back, mutating the source buys nothing.

Fix

Treat the picked source as read-only. Rotation and compression are now composed together through in-memory streams by a single ProcessImage helper, and only the final result is written — once — to a new MAUI-owned cache file via FileSystemUtils.GetTemporaryFile(Application.Context.CacheDir, originalFileName). The user's source file is never deleted or overwritten.

GetTemporaryFile places the output in a unique directory while preserving the original filename (<cache>/<hash>/<guid>/<originalFileName>), so the filename-preservation behavior from #33258 (no _processed/_rotated suffix) is kept. A format change (PNG→JPEG) still updates the extension.

Owned-input cleanup

Some inputs are themselves MAUI-owned temporary cache files — a camera capture from CapturePhotoAsync, or a content:// URI that EnsurePhysicalPath copied into our cache — rather than a user-owned source. For those, ProcessImage deletes the temporary input after the processed output has been fully written, so we don't leave an orphaned full-size duplicate behind.

Ownership is detected reliably via FileSystemUtils.IsMauiOwnedTemporaryFile, which recognizes a file we created through GetTemporaryFile by its <cache>/<EssentialsFolderHash>/ location — a folder that only GetTemporaryFile ever creates. External/user-owned sources are never under that folder, so they can never be matched or deleted, and deleting only after the output is written means there is no data-loss window.

What changed

  • The separate RotateImageInPlace and CompressImageIfNeeded methods are replaced by a single ProcessImage helper that applies rotation and compression in one pipeline. Rotation, if requested, is applied explicitly first and the compression stage is told not to rotate again.
  • All Android photo pick/capture call sites now use ProcessImage.
  • User-owned source files are never deleted or overwritten.
  • MAUI-owned temporary cache inputs are cleaned up after the output is written, so no orphaned duplicate cache file is left behind.
  • When no processing is requested, the original path is returned unchanged and no cache file is created.

Tests

Added Android device tests (src/Essentials/test/DeviceTests/Tests/Android/MediaPicker_Tests.cs) covering every path the change touches:

  1. rotation-only, compression-only, and rotation+compression each leave an external source file byte-for-byte unchanged and still present,
  2. the processed output is a new file under the app cache directory,
  3. the original filename is preserved (only the extension may change), and each processed case writes exactly one terminal cache file (no intermediate),
  4. a MAUI-owned cache input is replaced by a single terminal cache file with the owned input deleted (no orphan left behind), and
  5. the no-op case (no rotation, quality 100, no resize) returns the original path unchanged and creates no cache copy.

Verified locally on an Android emulator: all five MediaPicker tests pass and the full Essentials device-test suite is green (280 passed / 0 failed).

Platforms affected

  • Android only.

Behavioral notes

  • The user's original file is left untouched; the delete-before-write data-loss window is gone.
  • When both rotation and compression run, they are composed through streams and only the terminal output is written to disk.
  • A MAUI-owned temporary cache input is deleted only after the processed output has been fully written.

Copilot AI review requested due to automatic review settings July 14, 2026 22:06
@mattleibow
mattleibow temporarily deployed to copilot-pat-pool July 14, 2026 22:06 — with GitHub Actions Inactive
@github-actions

Copy link
Copy Markdown
Contributor

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 36572

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 36572"

@mattleibow
mattleibow temporarily deployed to copilot-pat-pool July 14, 2026 22:06 — with GitHub Actions Inactive
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes Android MediaPicker post-processing (EXIF auto-rotation and compression/resizing) so that it no longer mutates the picked source file in place, avoiding potential user data loss and ensuring the processed output is written to a new MAUI-owned cache file.

Changes:

  • Replaced in-place rotation with RotateImage that writes to a new cache file and returns the new path.
  • Updated compression/resizing (CompressImageIfNeeded) to write to a new cache file (preserving the original filename; extension may change if format changes) without deleting/overwriting the source.
  • Added Android device tests verifying the source file remains byte-for-byte unchanged and the output is created under the app cache directory.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/Essentials/src/MediaPicker/MediaPicker.android.cs Stops rotation/compression from overwriting/deleting the picked source by writing processed output to a new cache file and chaining returned paths.
src/Essentials/test/DeviceTests/Tests/Android/MediaPicker_Tests.cs Adds device tests asserting the source image is unchanged and the processed image is written to the cache while preserving the original filename.

@mattleibow
mattleibow temporarily deployed to copilot-pat-pool July 14, 2026 22:09 — with GitHub Actions Inactive
@github-actions github-actions Bot added area-essentials Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info platform/android labels Jul 14, 2026
@mattleibow
mattleibow temporarily deployed to copilot-pat-pool July 14, 2026 22:09 — with GitHub Actions Inactive
@kubaflo

This comment has been minimized.

@github-actions github-actions Bot added the s/agent-review-in-progress AI review is currently running for this PR label Jul 15, 2026
@MauiBot MauiBot added s/agent-fix-win AI found a better alternative fix than the PR s/agent-reviewed PR was reviewed by AI agent workflow (full 4-phase review) labels Jul 15, 2026
MauiBot

This comment was marked as outdated.

@MauiBot MauiBot removed the s/agent-review-in-progress AI review is currently running for this PR label Jul 15, 2026
Copilot AI review requested due to automatic review settings July 15, 2026 22:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comment thread src/Essentials/src/MediaPicker/MediaPicker.android.cs Outdated
Comment thread src/Essentials/src/MediaPicker/MediaPicker.android.cs Outdated
@kubaflo

This comment has been minimized.

@github-actions github-actions Bot added the s/agent-review-in-progress AI review is currently running for this PR label Jul 16, 2026
@MauiBot MauiBot added s/agent-fix-pr-picked AI could not beat the PR fix - PR is the best among all candidates and removed s/agent-fix-win AI found a better alternative fix than the PR labels Jul 16, 2026
MauiBot

This comment was marked as outdated.

@MauiBot MauiBot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expert Review — 1 findings

See inline comments for details.

Comment thread src/Essentials/src/MediaPicker/MediaPicker.android.cs
When applying EXIF auto-rotation or compression/resizing to a picked image,
the Android MediaPicker deleted and rewrote the file at the resolved source
path. That path is not a file MAUI owns - it may be the user's original photo,
a file on an SD card, or another app's shared storage - so mutating it in place
is incorrect and destroys data the app shouldn't touch. It also deleted the
original before writing the replacement, creating a data-loss window.

Treat the picked source as read-only: write the rotated/compressed output to a
new MAUI-owned cache file via GetTemporaryFile(CacheDir, originalFileName) and
return that path. The original filename is preserved (keeps #33258 fixed).

Adds Android device tests asserting the source is byte-for-byte unchanged, the
output is a new file under the cache dir, and the original filename is kept.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 16028f2a-8a01-4921-9677-8485c551c0f0
Replace the separate RotateImage + CompressImageIfNeeded methods with a single
ProcessImage that composes rotation and compression through in-memory streams from
a read-only source and writes only the final result - once - to a MAUI-owned cache
file. This keeps the source untouched (as before) and, for the rotate+compress
case, avoids leaving an orphaned intermediate cache file behind.

Rotation is applied explicitly before compression and the compression stage is told
not to rotate again, instead of relying on the re-encode having stripped EXIF.

Tests: cover rotation-only, compression-only, rotation+compression (asserting a
single terminal cache file, i.e. no intermediate leak), and the no-op case
(returns the original path, no cache copy).

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 16028f2a-8a01-4921-9677-8485c551c0f0
ProcessImage always writes the processed image to a new MAUI-owned cache file and
never touches the input. For external/user-owned sources (gallery, SD card, other
apps) that is exactly right. But some inputs are themselves MAUI-owned temporary
cache files - a camera capture from CapturePhotoAsync, or a content:// URI that
EnsurePhysicalPath copied into our cache - and for those we were leaving the
original full-size cache file orphaned alongside the new output.

Add FileSystemUtils.IsMauiOwnedTemporaryFile, which recognises a file we created
via GetTemporaryFile by its '<cache>/<EssentialsFolderHash>/' location (a folder
only GetTemporaryFile ever creates, so external sources can never match it).
ProcessImage now deletes the input only when it is one of these owned temporary
files, and only after the output has been fully written - so external sources stay
read-only and there is no data-loss window.

Adds a device test asserting a MAUI-owned cache input is replaced by a single
terminal cache file with no orphan left behind, with the original filename
preserved (#33258).

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 16028f2a-8a01-4921-9677-8485c551c0f0
Copilot AI review requested due to automatic review settings August 5, 2026 22:04
@mattleibow
mattleibow force-pushed the mattleibow-net10-android-mediapicker-security-fix branch from bca7feb to 9b0d5d4 Compare August 5, 2026 22:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@mattleibow

Copy link
Copy Markdown
Member Author

/azp run maui-pr, maui-pr-devicetests, maui-pr-uitests

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).

@mattleibow

This comment has been minimized.

MauiBot

This comment was marked as outdated.

@kubaflo kubaflo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please check the ai's suggestions?

@kubaflo

kubaflo commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

/review -b improved-reviewer -p android

@kubaflo

This comment has been minimized.

@MauiBot MauiBot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expert Review — 4 findings

See inline comments for details.


// Preserve the original filename (see #33258), but honor a format change
// (e.g. PNG -> JPEG) by swapping the extension.
var outputExtension = ImageProcessor.DetermineOutputExtension(currentStream, options.CompressionQuality, inputFileName);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[major] Logic and Correctness — Rotation-only picks can be written out under a wrong extension, a regression versus the old in-place behavior.

Concrete scenario: PickPhotoAsync(new MediaPickerOptions { RotateImage = true }) on a HEIC/WEBP/GIF/BMP source.

  • needsProcessing is false (default CompressionQuality = 100, no max dimensions), so only RotateImageAsync runs.
  • ImageProcessor.RotateImageAsync returns new MemoryStream(bytes) — the original, unmodified bytes — whenever EXIF orientation is 1 (the common case) or the bitmap fails to decode/compress (ImageProcessor.android.cs lines 41, 49, 57, 95, 103).
  • ImageProcessor.DetectImageFormat only recognizes PNG and JPEG magic numbers (ImageProcessor.shared.cs L285-295) and returns null for those bytes.
  • DetermineOutputExtension then falls through to the quality heuristic qualityPercent >= 95 ? ".png" : ".jpg" (L258). With quality 100 this yields ".png", so line 659-661 renames IMG_1234.heicIMG_1234.png and line 664-671 writes the HEIC bytes into a file named .png.

Previously RotateImageInPlace wrote back to the same path, so the extension always survived. Consumers that dispatch on FileResult.FileName/ContentType (or Image.Source = ImageSource.FromFile(...)) now get a mislabeled file. A .jpeg source is likewise silently renamed to .jpg.

Suggested fix: only change the extension when DetectImageFormat actually returned a format (i.e. when the bytes were genuinely re-encoded), and otherwise keep inputFileName unchanged.

// temporary cache file, delete it so we don't accumulate an orphaned duplicate.
// External/user-owned sources are never under our cache folder, so they are left
// untouched. Deleting only after the output is written avoids any data-loss window.
if (!preserveSource &&

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Memory Leak Prevention / cache growth — Deleting only the input file leaves its containing directory behind forever.

FileSystemUtils.GetTemporaryFile creates <cache>/2203693cc04e0be7f4f024d5f9499e13/<new-guid>/<name> for every call (FileSystemUtils.android.cs L43-46). This PR now allocates one such GUID directory per processed image (line 664) in addition to the one the input already occupied, and the File.Delete(imagePath) here removes the file but not its parent GUID directory. Java.IO.File.DeleteOnExit() is effectively a no-op on Android (the process is killed, not exited normally), so these empty directories accumulate for the app's lifetime.

Before this change the non-preserving path overwrote in place and created zero new directories; now each pick/capture leaves at least one empty directory behind. Consider deleting the parent directory when it is empty and confirmed under the owned root (reuse IsMauiOwnedTemporaryFile for the containment check).

currentStream.Dispose();
}
}
catch

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Null Safety and Defensive Coding — A failure after File.Create(outputPath) (line 667) leaves a truncated/zero-byte orphan in the cache.

If CopyToAsync throws (disk full, IOException, cancellation) the blanket catch swallows it and returns imagePath, which is correct for the caller, but outputPath has already been created and is never cleaned up. Because the output now lives in its own GUID directory it will never be overwritten or reclaimed. Track outputPath outside the try and best-effort delete it (and its directory) in the catch.

}

[Fact]
public async Task ProcessImage_MauiOwnedCacheInput_IsReplaced_NotOrphaned()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Regression Prevention and Test Coverage — The recovery-critical preserveSource: true branch has no test.

All four new tests call MediaPickerImplementation.ProcessImage(path, MediaPickerOptions), which always resolves to preserveSource: false. The negative case — the branch that must not delete the input — is never exercised, even though it is the one with real data-loss consequences: CapturePhotoAsync (MediaPicker.android.cs L155) routes through ProcessPhotoPreservingSourceAsync, and MediaPickerRecoveryManager.android.cs L673 replays it while an active recovery record still points at the capture file. If the !preserveSource guard at line 677 were ever inverted or dropped, this suite stays green.

Add a test that creates a MAUI-owned cache file, calls ProcessPhotoPreservingSourceAsync(path, new PersistedPhotoProcessingOptions(null, null, 50, true, true)), and asserts the input still exists and is byte-identical while a distinct output is produced.

Also not covered: the format-change rename path (a PNG source compressed to JPEG, and the extension behavior called out on line 656).

MauiBot

This comment was marked as outdated.

@kubaflo

This comment has been minimized.

@kubaflo

kubaflo commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Note

🔍 /review started

AzDO build 14916969 is running for android.

The s/agent-review-in-progress label stays on this PR while the run is active. The final recommendation and outcome labels are posted only after Gate, expert review, and Deep UI tests finish.

@MauiBot MauiBot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expert Review — 3 findings

See inline comments for details.


// Preserve the original filename (see #33258), but honor a format change
// (e.g. PNG -> JPEG) by swapping the extension.
var outputExtension = ImageProcessor.DetermineOutputExtension(currentStream, options.CompressionQuality, inputFileName);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

⚠️ Warning — Logic and Correctness / Image Handling: the rotate-only path now renames the output extension, and can mislabel the file.

When needsRotation == true and needsProcessing == false (RotateImage = true, CompressionQuality = 100, no max width/height), currentStream here is whatever ImageProcessor.RotateImageAsync returned. That method returns the original bytes unchanged (new MemoryStream(bytes)) whenever EXIF orientation is 1, the bitmap fails to decode, or the re-compress fails (ImageProcessor.android.cs lines 41, 50, 57, 95, 104). DetermineOutputExtension then runs DetectImageFormat, which only recognizes PNG and JPEG magic numbers and returns null for anything else, so it falls through to the quality heuristic — and with CompressionQuality == 100 (>= 95) it returns ".png".

Concrete scenario: user picks a modern-device IMG_1234.heic (or .webp) that is already upright (orientation 1) with new MediaPickerOptions { RotateImage = true }. The bytes are copied through untouched, but line 661 renames the output to IMG_1234.png while the content is still HEIC/WebP. The returned FileResult.FullPath now has an extension that does not match its content, which breaks consumers that dispatch on extension or MIME type derived from it.

This exposure is new to this PR: before the change, DetermineOutputExtension was only reached from CompressImageIfNeeded, where the stream was always a MAUI Graphics ImageFormat.Png/ImageFormat.Jpeg encode, so detection always succeeded. Rotation was in-place and never renamed. Consider only applying the extension swap when DetectImageFormat positively identified a format (or skipping the rename when the rotation pipeline returned the source bytes verbatim).


static void WriteJpeg(string filePath)
{
using var bitmap = Bitmap.CreateBitmap(64, 64, Bitmap.Config.Argb8888);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

⚠️ Warning — Regression Prevention and Test Coverage: the fixture cannot discriminate the rotation case.

Bitmap.CreateBitmap(...).Compress(Jpeg, ...) writes a JPEG with no EXIF block, so ImageProcessor.GetExifOrientation returns the default 1 and RotateImageAsync short-circuits to return new MemoryStream(bytes) (ImageProcessor.android.cs:50) — the source bytes are copied through untransformed.

That means ProcessImage_Rotation_DoesNotModifySource_And_WritesSingleCacheFile and the rotation half of ProcessImage_RotationAndCompression_... pass identically whether or not rotation is ever applied; the assertions (source untouched, one cache file, base filename preserved) are satisfied by the plain copy path alone. The headline regression this PR fixes — rotating a real photo used to delete/overwrite the user-owned source — is therefore only covered by the compression path, not by a genuinely rotated input.

Suggested discriminating test: write a JPEG and then stamp ExifInterface.TagOrientation = 6 (90°) on it via Android.Media.ExifInterface, run ProcessImage with RotateImage = true, and assert (a) the source file bytes are still byte-identical, and (b) the output's decoded width/height are swapped relative to the source (proving the rotation actually executed). This also exercises the orientation != 1 branch that produces a re-encoded stream and the extension logic at MediaPicker.android.cs:656.

{
var canonicalPath = new Java.IO.File(path).CanonicalPath;

foreach (var root in new[] { Application.Context.CacheDir, Application.Context.ExternalCacheDir })

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

💡 Suggestion — Android Platform / Performance: each call allocates a Java.IO.File[], crosses the JNI bridge for Application.Context twice, and leaks three undisposed Java.IO.File peers (new Java.IO.File(path) plus one per root) whose CanonicalPath also hits JNI.

Per android.instructions.md, cache Context in a local before repeated use. This is not a hot path (once per picked image), so it is not a correctness problem, but the same result can be had without the array or the extra JNI hops — e.g. hoist var context = Application.Context;, compute the two candidate roots into strings, and compare against the single canonicalized input path. Wrapping the Java.IO.File instances in using would also release the peers deterministically.

MauiBot

This comment was marked as outdated.

@MauiBot MauiBot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expert Review — 9 findings

See inline comments for details.

false, // rotation, if any, has already been applied above
options.PreserveMetaData);

if (processedStream is not null)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Logic and Correctness — When ImageProcessor.ProcessImageAsync returns null the code silently falls through: currentStream is still the unprocessed (only-rotated, or raw file) stream, it is written to a brand-new cache file, and — because preserveSource is false in the pick/capture flows — the MAUI-owned input is then deleted at line 677-684. The removed CompressImageIfNeeded explicitly handled this case by returning imagePath untouched ("If ImageProcessor returns null ... Return original path as fallback"). Concrete scenario: any future/partial-platform path where ProcessImageAsync returns null, the caller receives a file that was never resized/compressed while the original owned temp copy has already been deleted, so the untouched original is no longer recoverable. Suggest else { return imagePath; } (or skip the write/delete) when processedStream is null and needsProcessing was requested.

outputFileName = System.IO.Path.ChangeExtension(inputFileName, outputExtension);
}

var outputFile = FileSystemUtils.GetTemporaryFile(Application.Context.CacheDir, outputFileName);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Android Platform / Cross-Platform Consistency — The terminal output is always written to Application.Context.CacheDir (internal storage), but the input may have been resolved into external cache: FileSystemUtils.EnsurePhysicalPath picks Application.Context.ExternalCacheDir when WRITE_EXTERNAL_STORAGE is declared (FileSystemUtils.android.cs:278-282). Before this PR the compression step rewrote the file in place, so a large picked image stayed on the same volume. Now every processed image is copied onto internal storage, and the owned external-cache input is deleted (line 677-684), silently migrating multi-MB photos to the internal partition — a real problem for multi-pick of many large photos on devices with a small data partition. Consider deriving the output root from the input (external-cache input → external cache), mirroring the root selection EnsurePhysicalPath already performs.

!string.Equals(imagePath, outputPath, StringComparison.Ordinal))
{
try
{ File.Delete(imagePath); }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Regression Prevention / disk hygieneFile.Delete(imagePath) removes only the file, not the unique GUID directory GetTemporaryFile created for it (FileSystemUtils.android.cs:44-47 makes <cache>/<hash>/<guid>/ per call). Combined with line 664, every processed image now (a) creates one new GUID directory and (b) leaves the input's now-empty GUID directory behind forever — DeleteOnExit is a JVM shutdown hook that Android processes almost never run. Concrete scenario: PickPhotosAsync with 50 photos and CompressionQuality = 50 leaves 50 empty stale directories plus 50 new ones under <cache>/2203693cc04e0be7f4f024d5f9499e13/, repeated on every pick. Before the PR the non-recovery flows overwrote in place and created no new directory. Suggest deleting the input's parent directory when it is an owned GetTemporaryFile wrapper directory (i.e. it is empty after the delete).

currentStream.Dispose();
}
}
catch

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Null Safety / Defensive Coding — This blanket catch { } is now much wider than what it replaced. (1) Rotation failures previously propagated (RotateImageInPlace had no catch); they are now swallowed, so a picked photo can silently come back un-rotated with no diagnostic. The recovery code in this same feature uses Trace.WriteLine for exactly this situation (MediaPickerRecoveryManager.android.cs:675) — add a catch (Exception ex) with Trace.WriteLine. (2) If the failure happens inside the File.Create/CopyToAsync block (lines 667-671, e.g. IOException on a full cache partition), a truncated output file is left at outputPath in the cache while the method returns imagePath; nothing ever deletes it. Suggest tracking outputPath outside the inner try and best-effort deleting a partially written output in the catch.

{
if (needsRotation)
{
var rotatedStream = await ImageProcessor.RotateImageAsync(currentStream, inputFileName);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[minor] Performance-Critical Path / memory — The rotated MemoryStream (a full in-memory copy of the encoded image, produced by ImageProcessor.RotateImageAsync) is now kept alive as the input to ProcessImageAsync and only disposed at line 648, i.e. it coexists with ProcessImageAsync's own byte[] copy, the decoded Bitmap and the output MemoryStream. The removed flow round-tripped the rotated image through disk and re-opened it as a FileStream, so the encoded copy was not resident during decode/encode. Concrete scenario: RotateImage = true + CompressionQuality = 50 on a 12 MP camera JPEG on a low-RAM device raises peak managed memory by roughly one full encoded-image buffer, in a code path where Android OutOfMemoryError is already a known MediaPicker failure mode. Not a blocker, but worth a comment or an explicit disposal of the rotated buffer once ProcessImageAsync has consumed it.

internal static Task<string> ProcessImage(string imagePath, MediaPickerOptions options)
=> ProcessImage(imagePath, GetPhotoProcessingOptions(options));

internal static async Task<string> ProcessImage(string imagePath, PersistedPhotoProcessingOptions options, bool preserveSource = false)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[minor] Public API Surface (internal naming)ProcessImage is async Task<string> but lacks the Async suffix used by every neighbouring member (ProcessPhotoAsync, ProcessPhotoPreservingSourceAsync, CapturePhotoAsync). It is internal so this is not a shipped-API concern, but it is also the seam the new device tests bind to, so the name will be sticky. Suggest ProcessImageAsync for both overloads.

}

[Fact]
public async Task ProcessImage_MauiOwnedCacheInput_IsReplaced_NotOrphaned()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[major] Regression Prevention / Test Coverage — The five new tests only exercise preserveSource == false. The new delete branch (MediaPicker.android.cs:677-684) has no test for its negative case: that a MAUI-owned input is kept when preserveSource: true. That flag is the sole guard preventing deletion of a capture file that an active recovery record still points at (ProcessPhotoPreservingSourceAsyncCapturePhotoWithActivityResultAsync:155, MediaPickerRecoveryManager.PublishRecoveredOperationAsync:673). A future refactor that drops or inverts the !preserveSource term would keep all five tests green while re-introducing the pre-PR data-loss window for recovered captures. Please add a test alongside ProcessImage_MauiOwnedCacheInput_IsReplaced_NotOrphaned that calls ProcessPhotoPreservingSourceAsync on an owned cache input and asserts the input still exists and is byte-for-byte unchanged after processing.

}
}

static string CreateJpegOutsideCache(string fileName)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[minor] Regression PreventionCreateJpegOutsideCache places the "user-owned" source under FileSystem.AppDataDirectory, which is app-private internal storage. That is sufficient to exercise IsMauiOwnedTemporaryFile (it is outside <cache>/<hash>/), but it does not cover the ownership boundary the PR description is actually about — a source resolved into ExternalCacheDir by EnsurePhysicalPath when WRITE_EXTERNAL_STORAGE is declared, which IsMauiOwnedTemporaryFile classifies via the second root in its loop. That second root (FileSystemUtils.android.cs:71) is currently untested. Consider one case seeded under Application.Context.ExternalCacheDir (skipped when null) so both ownership roots are covered.

bitmap.Compress(Bitmap.CompressFormat.Jpeg, 100, stream);
}

static string[] FindCacheFiles(string baseName)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[minor] Regression Prevention / test robustnessFindCacheFiles does Directory.GetFiles(FileSystem.CacheDirectory, baseName + ".*", SearchOption.AllDirectories) and its result is asserted with Assert.Single. A recursive enumeration of the whole app cache directory can throw UnauthorizedAccessException/DirectoryNotFoundException when another component (e.g. WebView, Glide, or a concurrent MediaPicker test) creates or removes a cache subdirectory mid-enumeration, which would surface as a flaky failure unrelated to the fix. Scoping the search to Path.Combine(FileSystem.CacheDirectory, FileSystemUtils.EssentialsFolderHash) — the only directory this pipeline ever writes to — would make the assertion both tighter and more stable.

@MauiBot MauiBot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI Review Summary

@mattleibow — new AI review results are available based on this last commit: 9b0d5d4.

Gate Passed Confidence Unknown Platform Android


🗂️ Review Sessions — click to expand
🚦 Gate — Test Before & After Fix

Gate Result: ✅ PASSED

Platform: ANDROID · Base: net11.0 · Merge base: a8f0b6f5

Verified (new API / feature) — this PR adds new API and a test that references it in the same project, so reverting the fix un-compiles the test: there is no valid "fails without the fix" baseline to establish (a compile-coupled baseline). The gate instead verified the fix by a clean build + pass with the fix, so this is a real PASS rather than a non-committal INCONCLUSIVE.

Test Without Fix (expect FAIL) With Fix (expect PASS)
📱 Android_MediaPicker_Tests (ProcessImage_Rotation_DoesNotModifySource_And_WritesSingleCacheFile, ProcessImage_Compression_DoesNotModifySource_And_WritesSingleCacheFile, ProcessImage_RotationAndCompression_DoesNotModifySource_And_WritesSingleCacheFile, ProcessImage_NoProcessingNeeded_ReturnsOriginalPath_Unchanged, ProcessImage_MauiOwnedCacheInput_IsReplaced_NotOrphaned) Category=MediaPicker 🛠️ BUILD ERROR ✅ PASS — 465s
🔴 Without fix — 📱 Android_MediaPicker_Tests (ProcessImage_Rotation_DoesNotModifySource_And_WritesSingleCacheFile, ProcessImage_Compression_DoesNotModifySource_And_WritesSingleCacheFile, ProcessImage_RotationAndCompression_DoesNotModifySource_And_WritesSingleCacheFile, ProcessImage_NoProcessingNeeded_ReturnsOriginalPath_Unchanged, ProcessImage_MauiOwnedCacheInput_IsReplaced_NotOrphaned): 🛠️ BUILD ERROR · 354s

Error-relevant lines (filtered from the build log):

/home/vsts/work/1/s/src/Essentials/test/DeviceTests/Tests/Android/MediaPicker_Tests.cs(43,54): error CS0117: 'MediaPickerImplementation' does not contain a definition for 'ProcessImage' [/home/vsts/work/1/s/src/Essentials/test/DeviceTests/Essentials.DeviceTests.csproj::TargetFramework=net11.0-android]
/home/vsts/work/1/s/src/Essentials/test/DeviceTests/Tests/Android/MediaPicker_Tests.cs(76,50): error CS0117: 'MediaPickerImplementation' does not contain a definition for 'ProcessImage' [/home/vsts/work/1/s/src/Essentials/test/DeviceTests/Essentials.DeviceTests.csproj::TargetFramework=net11.0-android]
/home/vsts/work/1/s/src/Essentials/test/DeviceTests/Tests/Android/MediaPicker_Tests.cs(110,54): error CS0117: 'MediaPickerImplementation' does not contain a definition for 'ProcessImage' [/home/vsts/work/1/s/src/Essentials/test/DeviceTests/Essentials.DeviceTests.csproj::TargetFramework=net11.0-android]
Build FAILED.
🟢 With fix — 📱 Android_MediaPicker_Tests (ProcessImage_Rotation_DoesNotModifySource_And_WritesSingleCacheFile, ProcessImage_Compression_DoesNotModifySource_And_WritesSingleCacheFile, ProcessImage_RotationAndCompression_DoesNotModifySource_And_WritesSingleCacheFile, ProcessImage_NoProcessingNeeded_ReturnsOriginalPath_Unchanged, ProcessImage_MauiOwnedCacheInput_IsReplaced_NotOrphaned): PASS ✅ · 465s

(no coded error found; showing last 1200 chars)

6 11225 11263 I DOTNET  : Xml file was written to the provided writer.
      08-08 11:04:12.536 11225 11263 I DOTNET  : === TEST EXECUTION SUMMARY ===
      08-08 11:04:12.536 11225 11263 I DOTNET  : Tests run: 264 Passed: 5 Inconclusive: 0 Failed: 0 Ignored: 259 Skipped: 0
info: <<XHARNESS_RESULT_START>>
      {
        "version": 1,
        "machineName": "runnervmtroe5",
        "exitCode": 0,
        "exitCodeName": "SUCCESS",
        "platform": "android",
        "instrumentationExitCode": 0,
        "device": "emulator-5554",
        "deviceOsVersion": "API 30",
        "architecture": "x86_64",
        "files": [
          {
            "name": "testResults-c0c093ec19f84ef4b7081db3a82e940a.xml",
            "type": "test-results"
          },
          {
            "name": "adb-logcat-com.microsoft.maui.essentials.devicetests-default.log",
            "type": "logcat"
          }
        ]
      }
      <<XHARNESS_RESULT_END>>
info: Attempting to remove apk 'com.microsoft.maui.essentials.devicetests'..
info: Successfully uninstalled com.microsoft.maui.essentials.devicetests
XHarness exit code: 0
  Passed: 5
  Failed: 0
  Skipped: 0
  Total: 5
  Tests completed successfully

⚠️ Failure Details

  • 🛠️ Android_MediaPicker_Tests (ProcessImage_Rotation_DoesNotModifySource_And_WritesSingleCacheFile, ProcessImage_Compression_DoesNotModifySource_And_WritesSingleCacheFile, ProcessImage_RotationAndCompression_DoesNotModifySource_And_WritesSingleCacheFile, ProcessImage_NoProcessingNeeded_ReturnsOriginalPath_Unchanged, ProcessImage_MauiOwnedCacheInput_IsReplaced_NotOrphaned) without fix: build failed before tests could run
    • /home/vsts/work/1/s/src/Essentials/test/DeviceTests/Tests/Android/MediaPicker_Tests.cs(43,54): error CS0117: 'MediaPickerImplementation' does not contain a definition for 'ProcessImage' [/home/vsts/wo...
📁 Fix files reverted (2 files)
  • src/Essentials/src/FileSystem/FileSystemUtils.android.cs
  • src/Essentials/src/MediaPicker/MediaPicker.android.cs

📋 Pre-Flight — Context & Validation

PR #36572 Pre-Flight

Context

  • Title: [Android] MediaPicker: Stop overwriting picked source files
  • Base / head: net11.0 / mattleibow-net10-android-mediapicker-security-fix
  • Review workspace: pr-review-36572 at 991ba776e0 (PR #36572 squashed for review)
  • Platform: Android
  • Gate: Passed previously: the added tests fail without the PR fix and pass with it. Do not rerun gate verification or modify gate/content.md.

Problem

Android MediaPicker currently rotates and compresses an image by deleting and rewriting the resolved source path. For file:// and some resolved content paths, that source can be a user-owned gallery, SD-card, or shared-storage file. This mutates user data and creates a data-loss window because deletion happens before the replacement is fully written.

Existing PR Approach

The PR replaces the in-place rotation/compression paths with one ProcessImage pipeline. It:

  1. Treats the source as read-only and composes rotation and compression through streams.
  2. Writes one terminal output to a new GetTemporaryFile cache location while preserving the base filename and honoring format extension changes.
  3. Adds FileSystemUtils.IsMauiOwnedTemporaryFile to identify cache inputs and deletes only those inputs after output is complete, unless recovery requires source preservation.
  4. Consolidates all Android photo processing call sites onto this pipeline.

This is the approach each try-fix candidate must differ from at the root-cause/strategy level, not merely by moving equivalent logic.

PR Diff

  • src/Essentials/src/FileSystem/FileSystemUtils.android.cs (+34): MAUI-owned cache-path detection.
  • src/Essentials/src/MediaPicker/MediaPicker.android.cs (+116/-203): unified non-mutating stream pipeline and owned-input cleanup.
  • src/Essentials/test/DeviceTests/Tests/Android/MediaPicker_Tests.cs (+183): five Android device tests.

The tests cover rotation-only, compression-only, rotation plus compression, no-op processing, and replacement of a MAUI-owned cache input. They assert external sources remain byte-for-byte intact, output uses cache with the original base filename, only one terminal cache file remains, and owned inputs are cleaned up.

Required Validation

Run only the detected primary/regression category:

pwsh .github/skills/run-device-tests/scripts/Run-DeviceTests.ps1 -Project Essentials -Platform android -TestFilter "Category=MediaPicker"

Do not run the full device-test suite. Each candidate gets one implementation/test pass and at most one focused correction/retest.

Target Files

  • src/Essentials/src/FileSystem/FileSystemUtils.android.cs
  • src/Essentials/src/MediaPicker/MediaPicker.android.cs
  • src/Essentials/test/DeviceTests/Tests/Android/MediaPicker_Tests.cs

The working tree contains pre-existing, unrelated changes under .github/ and eng/. Preserve them. Use only .github/scripts/EstablishBrokenBaseline.ps1 and its -Restore mode for candidate baseline transitions.


🔬 Code Review — Deep Analysis

PR #36572 — Expert Code Review (initial evaluation)

PR: [Android] MediaPicker: Stop overwriting picked source files
Base / head: net11.0 / mattleibow-net10-android-mediapicker-security-fix
Reviewed commit: 991ba776e0 (squashed submitted PR diff, local workspace)
Scope reviewed: the raw submitted PR only. Try-fix candidate branches were read as context and are explicitly not the subject of any finding.


Verdict

Approve with non-blocking changes requested. The fix is correct at the root-cause level and is the right architectural shape. No critical or blocking defect was found. There are 1 major (test-coverage) and 4 moderate findings, all addressable inside the existing design; none requires re-architecting the fix.

Confidence: high for the core correctness claim (source files are no longer mutated, ownership inference is sound, no data-loss window remains). Medium for the resource-hygiene/edge-path findings (cache-volume choice, stale directories, partial-output cleanup), which depend on device storage configuration that cannot be exercised in this environment.

Recommendation to the orchestrator: a consolidated pr-plus-reviewer patch is warranted but optional — worth doing because all five findings are small, local, and independent of the pipeline's core logic, and one of them (the preserveSource negative test) protects the fix's own safety valve from a future silent regression. It is not required to make the PR correct.


Independent assessment (formed before reading the PR narrative)

Reading the diff cold, the change does exactly one thing and does it in one place:

  • The three destructive helpers (RotateImageInPlace, RotateImageToNewFileAsync, CompressImageIfNeeded + its ShouldDeleteIntermediateFile/TryDeleteFile bookkeeping) are replaced by a single ProcessImage(string, PersistedPhotoProcessingOptions, bool preserveSource) that treats the input as read-only, chains rotation → compression through streams, and performs exactly one terminal write to a fresh GetTemporaryFile cache path.
  • Deletion of the input is now (a) after the output is fully written and closed, (b) gated on FileSystemUtils.IsMauiOwnedTemporaryFile, and (c) suppressible via preserveSource for recovery-tracked captures.
  • All Android photo call sites (PickAsync, PickMultipleAsync, legacy CapturePhotoAsync, ActivityResult capture, photo-picker, recovery republish) are consolidated onto the one entry point.

This is a genuine root-cause fix, not a symptom patch: the old data-loss window existed because File.Delete(filePath) preceded File.Create(filePath) on a path that could be a user-owned gallery/SD-card file. Removing the in-place contract eliminates the whole class, not just the reported instance.

Verifications performed against surrounding code (not just the hunks):

Claim in the change Verified against Result
GetTemporaryFile is the only producer of <cache>/<EssentialsFolderHash>/… FileSystemUtils.android.cs:37-53 ✅ ownership marker is sound
EnsurePhysicalPath copies content:// inputs into an owned temp file FileSystemUtils.android.cs:277-288 ✅ owned inputs are detected and cleaned up
Both cache roots covered EnsurePhysicalPath uses ExternalCacheDir when WRITE_EXTERNAL_STORAGE is declared; IsMauiOwnedTemporaryFile loops over CacheDir and ExternalCacheDir ✅ consistent
Non-recovery call sites are safe to delete their input PickAsync:206-227, PickMultipleAsync:570-586 — no BeginOperationWithRecoveryAsync record ✅ no recovery record can be orphaned
Recovery-sensitive call sites opt out CapturePhotoWithActivityResultAsync:155, PickUsingPhotoPicker:267/327, MediaPickerRecoveryManager:673 all route through ProcessPhotoPreservingSourceAsyncpreserveSource: true ✅ safety valve wired everywhere it must be
Streams never used after disposal ImageProcessor.RotateImageAsync (android) returns a new MemoryStream on every path incl. failure/orientation==1; ProcessImageAsync returns a new stream Dispose(); currentStream = new at 630-631 / 648-649 is safe
rotateImage: false into ProcessImageAsync is not a behaviour change Old flow double-rotated (rotate-in-place wrote EXIF-stripped bytes → second orientation read was 1), so the second rotation was already a no-op ✅ equivalent output, one less decode
Null MediaPickerOptions still safe GetPhotoProcessingOptions is fully null-propagating, matching old options?.… call sites ✅ no NRE regression
Filename handling Path.GetFileName strips directories → no traversal via hostile DisplayName; #33258 filename preservation retained; extension swap now also applied to rotation-only output, fixing the old RotateImageToNewFileAsync case where JPEG bytes kept a .png name ✅ net improvement
Public API surface Everything added is internal; no PublicAPI.Unshipped.txt entry required ✅ correct
Regression cross-reference regression-check/risks.jsonCLEAN, 0 REVERT, 0 OVERLAP ✅ nothing reverted

The PR description matches the implementation; no overclaiming found.


Findings

All findings are written to inline-findings.json (9 entries) with file:line anchors.

Major (1)

  1. Missing negative-case test for preserveSource: trueMediaPicker_Tests.cs:61.
    All five new tests exercise preserveSource == false. The new deletion branch (MediaPicker.android.cs:677-684) has no test asserting an owned input is kept when the caller opts out. That flag is the only thing preventing deletion of a capture file an active recovery record still points at. A future refactor dropping the !preserveSource term would leave all five tests green while re-introducing the pre-PR data-loss window for recovered captures. Fix: one test calling ProcessPhotoPreservingSourceAsync on an owned cache input, asserting the input survives byte-for-byte.

Moderate (4)

  1. ProcessImageAsync returning null silently produces an unprocessed output and deletes the owned inputMediaPicker.android.cs:646. The removed CompressImageIfNeeded explicitly returned imagePath here. Currently unreachable on Android (null is the !(IOS||ANDROID||WINDOWS) branch), so defensive-correctness rather than a live bug — but the failure mode created (compression silently skipped, original temp already deleted) is worse than the old one.
  2. Output always written to internal CacheDir even when the input lived in ExternalCacheDirMediaPicker.android.cs:664. EnsurePhysicalPath selects the external cache when WRITE_EXTERNAL_STORAGE is declared. Combined with the owned-input delete, every processed image is migrated external → internal. Previously compression rewrote in place, so the volume never changed. Real impact for multi-pick of large photos on small internal partitions.
  3. Stale empty temp directories accumulateMediaPicker.android.cs:682. GetTemporaryFile creates a unique <cache>/<hash>/<guid>/ per call; File.Delete(imagePath) removes the file but not the wrapper directory, and DeleteOnExit is a JVM shutdown hook Android effectively never runs. A 50-photo pick leaves 50 empty directories plus 50 new ones, every time.
  4. Widened blanket catch { }MediaPicker.android.cs:693. (a) Rotation failures previously propagated; now swallowed with no Trace.WriteLine, unlike the sibling recovery code. (b) If the failure occurs inside File.Create/CopyToAsync (e.g. ENOSPC), a truncated output file is orphaned in the cache while imagePath is returned.

Minor (4)

  1. Peak-memory increase: the rotated MemoryStream is held live across ProcessImageAsync's decode/encode instead of round-tripping through disk (:629) — ~one extra encoded-image buffer on a path with a known Android OOM history.
  2. ProcessImage is async without the Async suffix used by every neighbour (:599); it is also the seam the new tests bind to, so the name will be sticky.
  3. Test source seeded in AppDataDirectory, leaving the ExternalCacheDir ownership root (FileSystemUtils.android.cs:71) untested (MediaPicker_Tests.cs:137).
  4. FindCacheFiles recursively enumerates the whole app cache dir and feeds Assert.Single — flakiness risk from unrelated cache writers; scope to the Essentials hash folder (MediaPicker_Tests.cs:156).

Blast radius & failure-mode probing

Surface touched: every Android photo path in MediaPickerPickPhotoAsync, PickPhotosAsync, both capture variants, PickUsingPhotoPicker, and the recovery republish path. Video paths untouched. iOS/Windows/ImageProcessor.shared untouched, so no cross-platform behavioural drift is introduced (ProcessImageAsync is called with the same contract as before).

Failure modes probed and cleared:

  • Use-after-dispose in the stream chain — cleared; RotateImageAsync never returns its input on any branch, including exceptions.
  • Deleting a file a recovery record points at — cleared; every recovery-registered call site passes preserveSource: true, and the two non-preserving call sites register no recovery operation. (Exactly why finding #1 matters: the invariant is real but untested.)
  • NRE on null MediaPickerOptions — cleared.
  • Path traversal via attacker-controlled DisplayName — cleared.
  • Double rotation / lost EXIF regression — cleared; equivalent to prior behaviour. Metadata loss on rotate+compress is pre-existing, not introduced.
  • file:// user-owned source deleted — cleared; a canonical-prefix check cannot match a gallery/SD-card/shared-storage path, and the catch defaults to false (safe direction), so an un-canonicalizable path is never deleted.

Residual risk, ranked: stale cache directories + external→internal migration (#4, #3) are the only user-visible regressions I can construct, and both are storage-hygiene rather than data-loss. The null-processed-stream path (#2) is currently unreachable on Android.

Intended behaviour change worth release-noting: when any processing is requested, the returned FileResult.FullPath is now always a new file under the app cache rather than the resolved source path (same base filename). The no-processing case still returns the picked path untouched, covered by ProcessImage_NoProcessingNeeded_ReturnsOriginalPath_Unchanged.


Reconciliation with prior context

  • Gate: trusted as PASSED (tests fail without the fix, pass with it). Not rerun, not re-litigated.
  • Regression cross-reference: CLEAN — no labeled bug-fix lines removed, so no author acknowledgment of an intentional revert is required.
  • Try-fix candidates (context only): candidate 1 (creation-time provenance registry) passed the PR's own five tests but trades a stateless, restart-safe path check for process-local state lost across process death — strictly weaker for the recovery scenario this feature is built around. Candidate 2 failed: it deleted the fifth mandatory regression and knowingly orphans owned cache inputs. Neither displaces the submitted approach, and neither surfaced a defect in the submitted PR that this pass did not independently reach. Candidate 2's self-reported orphan finding is the converse of finding #4: the PR does delete the owned file, it just leaves the empty wrapper directory.
  • No prior inline review comments existed in this workspace to deduplicate against.

Requested changes, in priority order

  1. Add the preserveSource: true retention test (#1) — protects the fix's own safety valve.
  2. Return imagePath when ProcessImageAsync yields null (#2) — three lines, removes a silent-corruption shape.
  3. Best-effort delete the input's empty wrapper directory after File.Delete (#4).
  4. Add Trace.WriteLine + partial-output cleanup to the outer catch (#5).
  5. Consider routing the output to the same cache root as the input (#3) — largest of the five, and the only one where "leave as is and document it" is a defensible answer.

Findings 6-9 are optional polish.


🛠️ Try-Fix — Analysis & Comparison

PR #36572 Alternative Fix Candidates

Candidate 1 — Own-Before-You-Write

Model: claude-opus-5
Result: Pass
Test: pwsh .github/skills/run-device-tests/scripts/Run-DeviceTests.ps1 -Project Essentials -Platform android -TestFilter "Category=MediaPicker"

This candidate treats unknown ownership as the root cause. GetTemporaryFile records creation-time provenance; before processing, a foreign input is copied to a MAUI-owned working file and an owned input is atomically moved to one. The existing rotation and compression stages then mutate only that owned working file. This differs from the PR's stateless cache-path ownership inference, unified stream pipeline, terminal output write, and conditional post-write input deletion.

The first test invocation built successfully but executed no tests because Android's package service was temporarily unavailable. The one permitted retry ran successfully: 359 passed, 0 failed, including all five ProcessImage_* regressions. The inline self-review recorded three non-blocking findings: unbounded process-lifetime provenance growth, provenance loss across process restart causing safe-direction cache orphaning, and a failed move consuming the ownership claim. A pre-test correction prevented failure cleanup from deleting an adopted camera capture.

The approach proves source safety structurally, but its process-local provenance and registry lifetime make it weaker than the PR for recovery and long-running applications.

Files changed: src/Essentials/src/FileSystem/FileSystemUtils.android.cs, src/Essentials/src/MediaPicker/MediaPicker.android.cs
Full narrative and diff: ../try-fix-1/content.md
Skill artifacts: attempt-1/

Candidate 2

Model: gpt-5.6-sol
Result: Fail
Test: pwsh .github/skills/run-device-tests/scripts/Run-DeviceTests.ps1 -Project Essentials -Platform android -TestFilter "Category=MediaPicker"

This candidate changes each transform helper to produce a fresh cache output. Rotation and compression remain separate; when both run, only the rotation output is deleted after compression succeeds. It uses no ownership classifier, provenance registry, source staging, or unified terminal stream pipeline.

The executed category run reported 358 passed, 0 failed, and 30 ignored. That green result is not sufficient: the candidate replaced the PR's five mandatory regressions with four and omitted ProcessImage_MauiOwnedCacheInput_IsReplaced_NotOrphaned. Its own self-review confirms the implementation leaves a MAUI-owned input orphaned, which is precisely what the omitted regression rejects. The candidate is therefore recorded as Fail, without a further pass.

Files changed: src/Essentials/src/FileSystem/FileSystemUtils.android.cs, src/Essentials/src/MediaPicker/MediaPicker.android.cs, src/Essentials/test/DeviceTests/Tests/Android/MediaPicker_Tests.cs
Self-review: 1 moderate, outcome-determining cache-orphan finding
Full narrative and diff: ../try-fix-2/content.md
Skill artifacts: attempt-2/

Aggregate Outcome

Two bounded alternatives were attempted. Candidate 1 passed all five PR regressions but has process-lifetime provenance and memory-growth drawbacks. Candidate 2 protects user-owned sources but fails the required owned-cache-input cleanup behavior. Neither is clearly superior to the PR's stateless ownership detection and unified one-write pipeline.


🏁 Report — Final Recommendation

⚠️ Final Recommendation: REQUEST CHANGES

Winner: pr-plus-reviewer

The submitted PR has the strongest core design: it fixes the root cause by treating picked sources as read-only, uses a stateless and restart-safe ownership check, composes transforms in memory, and writes one terminal cache result. The trusted Gate passed. The expert review found no blocking correctness defect, but its feedback identified several small robustness and regression-protection improvements that can be applied without changing that design.

pr-plus-reviewer is the single best candidate because it retains the PR's architecture while adding a direct test for the recovery-sensitive preserveSource invariant, restoring the safe fallback for a null processed stream, removing partial outputs and empty temporary wrapper directories, and tracing failures. Its required Android MediaPicker validation passed with 360 passed, 0 failed, and 30 ignored.

Candidate comparison

Rank Candidate Regression result Assessment
1 pr-plus-reviewer Pass — 360 passed, 0 failed, 30 ignored Keeps the PR's stateless, restart-safe design and adds focused recovery coverage plus safer cleanup/failure handling.
2 pr Pass — trusted Gate passed Correct root-cause fix and stronger than both independent alternatives. Ranked below the winner because the preservation guard is not directly tested and partial/stale cache cleanup is less robust.
3 try-fix-1 Pass — 359 passed, 0 failed Structurally protects foreign inputs, but its process-local provenance registry grows without bound, loses ownership across process restart, and consumes a claim before a move succeeds. Those tradeoffs are weaker for recovery than the PR's stateless path ownership.
4 try-fix-2 Fail Although its run reported 358 passed and 0 failed, it removed the required owned-cache-input regression and knowingly leaves MAUI-owned inputs orphaned. Per the comparison contract, it ranks below every passing candidate.

Expert findings and disposition

  • Addressed by the winner: missing negative test for preserveSource: true; null processing fallback; partial-output cleanup and diagnostics; stale empty GUID directories; overly broad cache enumeration in tests.
  • Not included: moving outputs from internal to external cache based on input location. This is a policy change with storage and compatibility implications, not necessary to prevent source mutation.
  • Residual non-blocking considerations: peak memory during rotate-plus-compress, the internal ProcessImage naming convention, and explicit external-cache-root coverage.

Because the winning refinements are not present in the submitted PR HEAD, the recommendation is REQUEST CHANGES rather than approval.


📱 UI Tests — Button,Label,Layout

Detected UI test categories: Button,Label,Layout

Deep UI tests — 359 passed, 0 failed, 7 skipped across 3 categories on platform-pool agent (replaces in-process counts above).

🧪 UI Test Execution Results (deep, platform pool)

Category Tests Snapshot diffs
Button 71/73 (2 skipped) ✓
Label 96/98 (2 skipped) ✓
Layout 192/195 (3 skipped) ✓
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs)

🧭 Next Steps — reviewer patch required (pr-plus-reviewer)

The reviewer-enhanced candidate won, so the submitted PR still needs those changes.

Why: The submitted PR has the strongest root-cause design, and pr-plus-reviewer preserves it while adding direct recovery-preservation coverage plus safer fallback, diagnostics, and temporary-file cleanup. Its focused Android MediaPicker validation passed with 360 tests passed and no failures.

Apply PRAgent/pr-plus-reviewer/reviewer.patch from the CopilotLogs
artifact (or follow the report's Required submitted-PR change), push the update, and run
the review again.

@kubaflo

kubaflo commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

✅ Empirical end-to-end validation on an Android emulator — base vs. PR

I validated this PR hands-on in the Sandbox app on a real Android emulator, running the same test harness against the PR base and the PR head. The pre-fix behaviour reproduces exactly as described, and the PR corrects every case.

Verdict: ✅ FIX VALIDATED — base 9a6986d scores 2/8, PR head 9b0d5d4 scores 8/8 on an identical harness.

base vs PR side by side


Environment

Emulator sdk_gphone64_arm64 (AVD Copilot_API_33), device id emulator-5554
Android 13 (API 33), security patch 2024-03-01
ABI arm64-v8a
Host macOS (Apple Silicon), .NET SDK 11.0.100-preview.7.26381.103, Android workload 37.0.0-ci.main.2139
App src/Controls/samples/Controls.Sample.Sandboxnet11.0-android, Debug, com.microsoft.maui.sandbox
Driver .github/scripts/BuildAndRunSandbox.ps1 -Platform android + Appium/UIAutomator2 (appium:noReset=true)
Refs base 9a6986db4b2e2075a6852aabf440703254b26fc0 · head 9b0d5d4de71340db4341194b4c094c0855e432be

Two isolated git worktrees (one per ref) were used so nothing in the working tree was disturbed. Identical Sandbox page + Appium script in both — the only difference is the product code.


Test scenario: source and rationale

  • Source: custom scenario derived from the PR diff and the PR's own device tests (src/Essentials/test/DeviceTests/Tests/Android/MediaPicker_Tests.cs). There is no linked issue with repro steps (closingIssuesReferences is empty), so I modelled the scenarios on the exact code paths this PR touches, and then added cases the device tests do not cover (a real MediaStore gallery file, a resize-only path, and a full interactive PickPhotosAsync round trip).
  • How it drives the product code: the Sandbox page invokes the same internal entry point the picker itself usesMicrosoft.Maui.Media.MediaPickerImplementation.ProcessPhotoAsync(string, MediaPickerOptions) — via reflection. That signature exists unchanged on both refs, so one harness binary-compatibly exercises RotateImageInPlace + CompressImageIfNeeded on base and ProcessImage on the PR. The page even auto-detects which product build it is running against (variant=BASE vs variant=PR36572) from the internal API shape, so the two runs are provably distinguishable and were not mixed up.
  • What each scenario asserts: the source file still exists, its SHA-256 and byte length are unchanged, the source directory gained no stray sibling file, the returned path is a MAUI-owned cache file, that file decodes as a valid bitmap, and exactly one terminal cache file was produced (no intermediate/orphan).

Scenarios, expected vs. actual

# Scenario (MediaPickerOptions) Expected BASE 9a6986d PR 9b0d5d4
S1 rotate only — RotateImage=true, Q100, EXIF orientation 6, source outside cache source untouched, output in cache source overwritten in place (srcUnchanged=False, returned path == source path, cacheFiles 0→0) srcUnchanged=True, output …/cache/2203693…/…/s1_rotate.jpg, cacheFiles 0→1
S2 compress only — CompressionQuality=40 source untouched source overwritten in place ✅ untouched, new cache file
S3 rotate + compress source untouched, one output source overwritten in place ✅ untouched, exactly 1 new cache file (no intermediate)
S4 resize only — MaximumWidth=64, Q100 source untouched srcExists=False — the original s4_resize.jpg was DELETED and a s4_resize.png written in its place in the user's folder s4_resize.jpg intact; output …/cache/…/s4_resize.png (63×42)
S5 PNG→JPEG format change — PNG source, Q50 source PNG untouched, no stray sibling srcExists=False — the user's s5_format.png was DELETED and s5_format.jpg written next to it ✅ PNG intact and byte-identical; JPEG output lands in cache
S6 no-op — Q100, no rotate, no resize return source path unchanged, create nothing ✅ (already correct) cacheFiles 5→5, returned path == source path
S7 MAUI-owned cache input (camera-capture shape) one terminal cache file, no orphan ✅ but by overwriting in place (retSamePathAsInput=True) ✅ owned input deleted after output written (ownedInputStillExists=False, retSamePathAsInput=False, cacheFiles 6→6 — no orphan)
S8 real gallery photo inserted via MediaStore/storage/emulated/0/Pictures/MauiPR36572/…jpg, rotate + Q40 the user's gallery original must survive galleryUnchanged=False — the actual photo in the device gallery was rewritten in place galleryExists=True galleryUnchanged=True, output in app cache

S8 is the headline case from the PR description ("the user's original photo in the gallery"), and it is a genuine /storage/emulated/0/Pictures/... file created through MediaStore, not a synthetic stand-in.


Base — bug reproduced (2/8)

DONE BASE 2/8 PASS (6 FAIL)

base results

pr36572-base-pre-fix-repro.mp4

Note S4 and S5 in particular: those are not merely "content changed" — srcExists=False means the user's file is gone from disk, replaced by a file with a different extension. That is unrecoverable data loss for a file the app never owned.

PR — fix validated (8/8)

DONE PR36572 8/8 PASS

pr results

pr36572-pr-fix-validated.mp4

Real interactive MediaPicker flow (not just launch)

Both builds also ran a full end-to-end pick: tap PICK PHOTO → Android 13 photo picker opens → select the seeded gallery image → MediaPicker.Default.PickPhotosAsync(new MediaPickerOptions { RotateImage = true, CompressionQuality = 40, MaximumWidth = 200 }) → the returned file is loaded back into an Image.

  • Base: PICK_OK file=0a3eb15067c742a782afef53e83bcbd9.jpg exists=True bytes=1896 dims=200x266 underCache=True
  • PR: PICK_OK file=1000000134.jpg exists=True bytes=1911 dims=200x266 underCache=True seededGallery[exists=True,unchanged=True]

No regression: the picker still works, resizing/compression/rotation still apply (200×266 from a 320×240 EXIF-rotated source), and the image renders. Worth calling out — the PR also improves the filename on this path: base returned a GUID name (0a3eb150…jpg, from the preserveSource branch of CompressImageIfNeeded), whereas the PR preserves the original name (1000000134.jpg), consistent with #33258.

pr real pick

base real pick


Validation performed on every run

  • No FATAL / unhandled exception / error CS in build-run-output.log or android-device.log.
  • Appium actions confirmed present in the log (Found element RunAllButton, Tapping RunAllButton, Tapping PickPhotoButton, 4× Screenshot saved) — not a "false success".
  • SANDBOX: markers confirmed in the device logcat for every scenario, matching the on-screen labels.
  • Test completed successfully, script exit code 0, on both runs.
  • Every processed output was re-decoded with BitmapFactory to prove it is a valid, non-corrupt image (dimensions listed above).

Limitations / caveats

  • Android 13 / API 33 / arm64 emulator only. Not exercised on API 29–31 (where IsPhotoPickerAvailable is false and the ACTION_GET_CONTENT path is used instead), nor on a physical device or an SD-card / content:// path from a third-party document provider.
  • The S1–S7 "user-owned" sources live under getExternalFilesDir() rather than a foreign app's storage. That is the correct proxy because FileSystemUtils.IsMauiOwnedTemporaryFile decides ownership purely by location (<cache>/<EssentialsFolderHash>/), and S8 covers the true shared-storage gallery case for real.
  • The harness calls the internal ProcessPhotoAsync directly for S1–S8 so each transform can be isolated deterministically; the interactive pick above covers the un-mocked user path end to end.
  • On base, the seededGallery[unchanged=False] shown in the pick label reflects the damage already done by S8 earlier in that same run — the pick itself goes through a content:// → cache copy and does not touch the gallery original on either build.
  • Camera capture (CapturePhotoAsync) was not exercised — the emulator has no usable camera — but S7 reproduces its file-ownership shape (a MAUI-owned cache input).

No product code was modified, and nothing was committed or pushed; the Sandbox changes existed only in throwaway worktrees.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-essentials Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info p/0 Current heighest priority issues that we are targeting for a release. platform/android s/agent-changes-requested AI agent recommends changes - found a better alternative or issues s/agent-fix-win AI found a better alternative fix than the PR s/agent-gate-passed AI verified tests catch the bug (fail without fix, pass with fix) s/agent-reviewed PR was reviewed by AI agent workflow (full 4-phase review)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants