fix(compose): release persistable URI permissions for attachments/inline images #203

2026-07-03T08:30:27Z

JMR-dev commented

2026-07-03 08:30:27 +00:00

(Migrated from github.com)

From the post-batch security review.

Finding (Low — privacy + robustness)

The attachment picker and inline-image picker in ComposeScreen.kt call takePersistableUriPermission on every pick, but the grant was never released (releasePersistableUriPermission was absent repo-wide). The app accumulated indefinite read access to every file/photo ever attached, and can hit the per-app persistable-grant cap — after which the take (inside runCatching, so a cap failure is silently swallowed) fails, and a later draft reopen can't reload the image.

Fix

The picked bytes are copied into the app cache at enqueue (MailRepositoryImpl.copyAttachments), so a grant is only truly needed to reload an image when a draft is reopened. New AttachmentUriGrants releases a URI's grant once nothing references it, invoked after the referencing row is gone:

MailRepositoryImpl.deleteDraft — a deleted draft can never reopen.
MailRepositoryImpl.cancelOutboxMessage — the queued copy is discarded.
SendWorker — after a send succeeds, and when a queued message is dropped because its account was removed.

releasePersistableUriPermission is wrapped in runCatching, so releasing a grant we don't actually hold (or a non-persisted URI) is a no-op.

Chosen release scope (re: "don't release a URI still referenced by another live row")

A URI is released only when no remaining draft AND no remaining outbox row references it — I track this precisely by re-reading both tables after the triggering row is deleted (DraftDao.getAll() / OutboxDao.getAll()), rather than releasing blindly. So:

Deleting one of two drafts that share a picked URI keeps the grant (the other draft can still reload it).
Sending a message composed from a draft: the draft is deleted first (by the existing performSend path), but the just-enqueued outbox row still references the URI, so the grant is kept until SendWorker actually sends — then released. No premature release, no orphaned grant.

Residual edge cases (documented, not over-engineered — Low finding)

Remove-then-discard leak: if the user attaches a file, then removes the chip (removeAttachment) and discards the compose without ever saving/sending, no row-deletion event fires for that URI, so its grant isn't released. (Attaching then discarding with the attachment still present is fine — it autosaves/saves a draft, whose later deletion releases it.)
Kill between delete and release: if the process dies between deleting a row and the release call, that grant leaks; it isn't retroactively reclaimed. Both are bounded by the OS grant cap and are strictly better than the prior "never release" behavior.
SendWorker's release path is not unit-tested (no existing WorkManager test harness); it's a direct mirror of the repository call.

Optional same-area hardening

sanitizeAttachmentName (new, in domain/model) strips path separators and ISO control chars (incl. CR/LF) from picked/received display names before they become on-disk filenames or MIME Content-Disposition filenames — closing a directory-traversal / header-injection vector. Wired into the compose picker (queryFileName) and the outbox/incoming staging (copyAttachments, attachmentFile), replacing the duplicated ad-hoc substringAfterLast logic.

Tests (JVM)

AttachmentUriGrantsTest — the pure unreferencedUris release decision (releases the unreferenced, keeps the still-referenced, dedups).
SanitizeAttachmentNameTest — CR/LF + control-char stripping, path-prefix stripping, blank fallback.
MailRepositoryGrantsTest — deleteDraft / cancelOutboxMessage hand the removed row's URIs to the releaser, after deleting the row (coVerifyOrder).

Full local preflight green (JDK 21, --max-workers=8, one at a time): assembleDebug, testDebugUnitTest, lintDebug, ktlintCheck+detekt, compileDebugAndroidTestKotlin. No Room schema change (added DAO queries only). Emulator E2E left to CI.

🤖 Generated with Claude Code

From the post-batch security review. ## Finding (Low — privacy + robustness) The attachment picker and inline-image picker in `ComposeScreen.kt` call `takePersistableUriPermission` on every pick, but the grant was **never released** (`releasePersistableUriPermission` was absent repo-wide). The app accumulated indefinite read access to every file/photo ever attached, and can hit the per-app persistable-grant cap — after which the take (inside `runCatching`, so a cap failure is silently swallowed) fails, and a later draft reopen can't reload the image. ## Fix The picked bytes are copied into the app cache at enqueue (`MailRepositoryImpl.copyAttachments`), so a grant is only *truly* needed to reload an image when a **draft** is reopened. New `AttachmentUriGrants` releases a URI's grant once nothing references it, invoked after the referencing row is gone: - `MailRepositoryImpl.deleteDraft` — a deleted draft can never reopen. - `MailRepositoryImpl.cancelOutboxMessage` — the queued copy is discarded. - `SendWorker` — after a send succeeds, and when a queued message is dropped because its account was removed. `releasePersistableUriPermission` is wrapped in `runCatching`, so releasing a grant we don't actually hold (or a non-persisted URI) is a no-op. ### Chosen release scope (re: "don't release a URI still referenced by another live row") A URI is released **only when no *remaining* draft AND no *remaining* outbox row references it** — I track this precisely by re-reading both tables after the triggering row is deleted (`DraftDao.getAll()` / `OutboxDao.getAll()`), rather than releasing blindly. So: - Deleting one of two drafts that share a picked URI keeps the grant (the other draft can still reload it). - Sending a message composed from a draft: the draft is deleted first (by the existing `performSend` path), but the just-enqueued outbox row still references the URI, so the grant is kept until `SendWorker` actually sends — then released. No premature release, no orphaned grant. ### Residual edge cases (documented, not over-engineered — Low finding) - **Remove-then-discard leak:** if the user attaches a file, then removes the chip (`removeAttachment`) and discards the compose without ever saving/sending, no row-deletion event fires for that URI, so its grant isn't released. (Attaching then discarding *with* the attachment still present is fine — it autosaves/saves a draft, whose later deletion releases it.) - **Kill between delete and release:** if the process dies between deleting a row and the release call, that grant leaks; it isn't retroactively reclaimed. Both are bounded by the OS grant cap and are strictly better than the prior "never release" behavior. - `SendWorker`'s release path is not unit-tested (no existing WorkManager test harness); it's a direct mirror of the repository call. ## Optional same-area hardening `sanitizeAttachmentName` (new, in `domain/model`) strips path separators and ISO control chars (incl. CR/LF) from picked/received display names before they become on-disk filenames or MIME `Content-Disposition` filenames — closing a directory-traversal / header-injection vector. Wired into the compose picker (`queryFileName`) and the outbox/incoming staging (`copyAttachments`, `attachmentFile`), replacing the duplicated ad-hoc `substringAfterLast` logic. ## Tests (JVM) - `AttachmentUriGrantsTest` — the pure `unreferencedUris` release decision (releases the unreferenced, keeps the still-referenced, dedups). - `SanitizeAttachmentNameTest` — CR/LF + control-char stripping, path-prefix stripping, blank fallback. - `MailRepositoryGrantsTest` — `deleteDraft` / `cancelOutboxMessage` hand the removed row's URIs to the releaser, **after** deleting the row (`coVerifyOrder`). Full local preflight green (JDK 21, `--max-workers=8`, one at a time): `assembleDebug`, `testDebugUnitTest`, `lintDebug`, `ktlintCheck`+`detekt`, `compileDebugAndroidTestKotlin`. No Room schema change (added DAO queries only). Emulator E2E left to CI. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: JMR-dev/LibreMail#203