MainActivity is exported="true" (launcher / mailto: / share). The per-message notification intent is explicit and carries no manifest intent-filter, but a hostile app can still craft an explicit intent at the exported component with action = ACTION_OPEN_MESSAGE + a message id and drive the reader to an arbitrary cached message. (Impact is limited — it only makes our app render an already-cached message in-app, and app-lock gates it — hence "below the cut". No data exfiltration.)
Implemented the maintainer's second option — "trust only the app's own PendingIntent":
openMessage(...) now attaches an unforgeable sender-token PendingIntent. A PendingIntent's creator package is stamped by the system and cannot be forged: only this app can mint one whose getCreatorPackage() is our package. The token is immutable and never sent (a package-scoped broadcast to no receiver), so it adds no new attack surface.
messageId(context, intent) yields the id only when that token is present and was created by us; otherwise it returns null. A foreign caller carries no token (or one attributed to its own package), so its intent is ignored.
The rejection path logs PII-free via AppLog.w (no message id, which is PII).
Why this can't be done more simply: notification taps don't go through startActivityForResult, so getCallingPackage() is null even for our own legitimate flow; and getReferrer() is caller-overridable (EXTRA_REFERRER), so it isn't a trustworthy check. The nested sender-token PendingIntent is the standard robust way to verify "this intent was built by us". Change is contained to the existing NotificationIntents build/parse contract plus the single MainActivity call site — mailto:/share are deliberately left open (that's the whole point of an email client).
Test:NotificationIntentsTest gains open_message_intent_without_our_sender_token_is_ignored (a same-action, token-less intent is rejected while the genuine one still resolves). Existing round-trip cases move to the new messageId(context, intent) signature. This is an instrumented test because the behaviour hinges on PendingIntent.getCreatorPackage(), which needs the real framework — matching the existing NotificationIntentsTest, which is instrumented for the same reason (real Intent/Uri/PendingIntent). It compiles locally (compileDebugAndroidTestKotlin) and runs in CI's E2E matrix.
Nit 2 — MailNotifier: messageId.hashCode() notification id can collide — DEFERRED (not fixed here), with reasoning
The finding is technically valid but I did not apply the suggested "persisted per-account counter" in this below-cut cleanup, because forcing it in would be disproportionate and would trade away a documented property for a negligible gain:
No stateless function can be collision-free. Notification ids are 32-bit Int and message ids are arbitrary strings — by pigeonhole, any String → Int mapping has collisions. So only a stateful (persisted) scheme can literally satisfy "collision-free"; no small tweak to the current hashCode() derivation qualifies, and shipping a cosmetic non-fix would be worse than leaving it.
The practical probability is ~1e-8 per notification. A collision only silently replaces a notification when a new message's id happens to equal a currently-displayed, unacknowledged notification's id. That active set is tiny (typically well under ~50), so per-event P ≈ 50 / 2^32 ≈ 1.2e-8 — i.e. on the order of 0.1% of users might ever see a single dropped notification, and the mail itself is still in the app.
The suggested fix is disproportionate/risky for a P4 below-cut nit.MailNotifier is currently a clean, dependency-free @Singleton (just a Context). A persisted per-account counter means either a Room migration (the natural home is the per-account settings table — the exact "storage/migration" category we treat with extra care) or a new DataStore/prefs seam, plus threading async persisted I/O into the notification-posting path and making notifyNewMailsuspend. That's a lot of new failure surface for a ~1e-8 issue.
A naive persisted counter also changes documented semantics. The current id is deliberately stable per message ("a later batch never overwrites an earlier, still-unacknowledged one"); a monotonic counter is stable per notify-event, not per message, so re-notifying the same message would duplicate rather than update in place. (In practice re-notify ~never happens — MailSyncer filters on existingIds and serialises syncs — so this isn't an observed regression, but it does abandon the property the code documents.) An in-memory counter would be strictly worse than the status quo (it restarts low after process death and would collide with still-showing pre-death notifications).
Recommendation: if this is worth pursuing, do it as its own tracked change (not folded into a nits-cleanup PR): a per-account monotonic counter persisted in a small dedicated DataStore (avoids a Room migration), assigned once per notify-batch (single edit, not one write per message), skipping the account's summary id. Happy to take that as a separate ticket if you'd like it prioritised over "accept as-is".
Gate
Local fast gate green (assembleDebug + testDebugUnitTest + jacocoTestCoverageVerification (floor 0.84 held) + compileDebugAndroidTestKotlin + lintDebug + ktlintCheck + detekt) — BUILD SUCCESSFUL. The instrumented NotificationIntentsTest compiles locally and runs in CI's matrix.
Closes #307 — Phase-3 domain/platform below-cut review nits.
Two nits were listed. **Nit 1 is fixed** (with a test); **Nit 2 is intentionally deferred** with the reasoning below rather than forced.
---
## Nit 1 — `MainActivity`/`NotificationIntents`: exported activity consumed `ACTION_OPEN_MESSAGE` without caller verification — **FIXED**
`MainActivity` is `exported="true"` (launcher / `mailto:` / share). The per-message notification intent is explicit and carries no manifest intent-filter, but a hostile app can still craft an explicit intent at the exported component with `action = ACTION_OPEN_MESSAGE` + a message id and drive the reader to an arbitrary cached message. (Impact is limited — it only makes *our* app render an already-cached message in-app, and app-lock gates it — hence "below the cut". No data exfiltration.)
Implemented the maintainer's second option — **"trust only the app's own PendingIntent"**:
- `openMessage(...)` now attaches an **unforgeable sender-token `PendingIntent`**. A `PendingIntent`'s creator package is stamped by the system and cannot be forged: only this app can mint one whose `getCreatorPackage()` is our package. The token is immutable and never sent (a package-scoped broadcast to no receiver), so it adds no new attack surface.
- `messageId(context, intent)` yields the id **only** when that token is present *and* was created by us; otherwise it returns `null`. A foreign caller carries no token (or one attributed to its own package), so its intent is ignored.
- The rejection path logs PII-free via `AppLog.w` (no message id, which is PII).
Why this can't be done more simply: notification taps don't go through `startActivityForResult`, so `getCallingPackage()` is `null` even for our own legitimate flow; and `getReferrer()` is caller-overridable (`EXTRA_REFERRER`), so it isn't a trustworthy check. The nested sender-token `PendingIntent` is the standard robust way to verify "this intent was built by us". Change is contained to the existing `NotificationIntents` build/parse contract plus the single `MainActivity` call site — `mailto:`/share are deliberately left open (that's the whole point of an email client).
**Test:** `NotificationIntentsTest` gains `open_message_intent_without_our_sender_token_is_ignored` (a same-action, token-less intent is rejected while the genuine one still resolves). Existing round-trip cases move to the new `messageId(context, intent)` signature. This is an instrumented test because the behaviour hinges on `PendingIntent.getCreatorPackage()`, which needs the real framework — matching the existing `NotificationIntentsTest`, which is instrumented for the same reason (real `Intent`/`Uri`/`PendingIntent`). It compiles locally (`compileDebugAndroidTestKotlin`) and runs in CI's E2E matrix.
---
## Nit 2 — `MailNotifier`: `messageId.hashCode()` notification id can collide — **DEFERRED (not fixed here), with reasoning**
The finding is technically valid but I did **not** apply the suggested "persisted per-account counter" in this below-cut cleanup, because forcing it in would be disproportionate and would trade away a documented property for a negligible gain:
1. **No *stateless* function can be collision-free.** Notification ids are 32-bit `Int` and message ids are arbitrary strings — by pigeonhole, any `String → Int` mapping has collisions. So only a *stateful* (persisted) scheme can literally satisfy "collision-free"; no small tweak to the current `hashCode()` derivation qualifies, and shipping a cosmetic non-fix would be worse than leaving it.
2. **The practical probability is ~1e-8 per notification.** A collision only silently replaces a notification when a *new* message's id happens to equal a *currently-displayed, unacknowledged* notification's id. That active set is tiny (typically well under ~50), so per-event P ≈ 50 / 2^32 ≈ 1.2e-8 — i.e. on the order of 0.1% of users might *ever* see a single dropped notification, and the mail itself is still in the app.
3. **The suggested fix is disproportionate/risky for a P4 below-cut nit.** `MailNotifier` is currently a clean, dependency-free `@Singleton` (just a `Context`). A persisted per-account counter means either a **Room migration** (the natural home is the per-account settings table — the exact "storage/migration" category we treat with extra care) or a new DataStore/prefs seam, plus threading async persisted I/O into the notification-posting path and making `notifyNewMail` `suspend`. That's a lot of new failure surface for a ~1e-8 issue.
4. **A naive persisted counter also changes documented semantics.** The current id is deliberately *stable per message* ("a later batch never overwrites an earlier, still-unacknowledged one"); a monotonic counter is stable per *notify-event*, not per message, so re-notifying the same message would duplicate rather than update in place. (In practice re-notify ~never happens — `MailSyncer` filters on `existingIds` and serialises syncs — so this isn't an *observed* regression, but it does abandon the property the code documents.) An in-memory counter would be strictly *worse* than the status quo (it restarts low after process death and would collide with still-showing pre-death notifications).
**Recommendation:** if this is worth pursuing, do it as its own tracked change (not folded into a nits-cleanup PR): a per-account monotonic counter persisted in a small dedicated DataStore (avoids a Room migration), assigned once per notify-*batch* (single edit, not one write per message), skipping the account's summary id. Happy to take that as a separate ticket if you'd like it prioritised over "accept as-is".
---
## Gate
Local fast gate green (`assembleDebug` + `testDebugUnitTest` + `jacocoTestCoverageVerification` (floor 0.84 held) + `compileDebugAndroidTestKotlin` + `lintDebug` + `ktlintCheck` + `detekt`) — `BUILD SUCCESSFUL`. The instrumented `NotificationIntentsTest` compiles locally and runs in CI's matrix.
The merge conditions cannot be satisfied due to failing checks
Hint
You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio queue comment.
Requeued — the merge queue status continues in this comment ↓.
<!---
DO NOT EDIT
-*- Mergify Payload -*-
{"version": 1, "state": "dequeued", "queue_rule_name": "default", "queued_at": "2026-07-08T12:40:15.093972+00:00", "estimated_time_of_merge": null, "speculative_check_pr": null, "required_conditions": []}
-*- Mergify Payload End -*-
-->
# Merge Queue Status
- ✅ **Entered queue** — `2026-07-08 12:40 UTC` · Rule: `default` · triggered by merge protections
- ❌ **Checks failed** · in-place
- 🚫 **Left the queue** — `2026-07-08 13:52 UTC` · at `1ee6366bec9259a1864721519295e1d5e7262f38`
This pull request spent **1 hour 12 minutes 20 seconds** in the queue, with no time running CI.
<details>
<summary><strong>Waiting for</strong></summary>
- [ ] `check-success = CI passed`
- [ ] any of: [🛡 GitHub branch protection]
- [ ] `check-neutral = CI passed`
- [ ] `check-skipped = CI passed`
- [ ] `check-success = CI passed`
- [ ] any of: [🛡 GitHub repository ruleset rule `main`]
- [ ] `check-neutral = @github-actions/CI passed`
- [ ] `check-skipped = @github-actions/CI passed`
- [ ] `check-success = @github-actions/CI passed`
</details>
<details>
<summary>All conditions</summary>
- [ ] `check-success = CI passed`
- [ ] any of [🛡 GitHub branch protection]:
- [ ] `check-neutral = CI passed`
- [ ] `check-skipped = CI passed`
- [ ] `check-success = CI passed`
- [ ] any of [🛡 GitHub repository ruleset rule `main`]:
- [ ] `check-neutral = @github-actions/CI passed`
- [ ] `check-skipped = @github-actions/CI passed`
- [ ] `check-success = @github-actions/CI passed`
- `-conflict`
- [X] #434
- `-draft`
- [X] #434
- [X] `base = main`
- `github-review-approved` [🛡 GitHub repository ruleset rule `main`]
- [X] #434
- `label != broken`
- [X] #434
- [X] any of [🛡 GitHub branch protection]:
- [X] `check-success = Debug build`
- [ ] `check-neutral = Debug build`
- [ ] `check-skipped = Debug build`
- [X] any of [🛡 GitHub branch protection]:
- [X] `check-success = Unit tests`
- [ ] `check-neutral = Unit tests`
- [ ] `check-skipped = Unit tests`
</details>
## Reason
The merge conditions cannot be satisfied due to failing checks
## Hint
You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a `@mergifyio queue` comment.
Requeued — the merge queue status continues in [this comment ↓](https://github.com/JMR-dev/LibreMail/pull/434#issuecomment-4918157647).
❌ The pull request has been removed from the queue default
The merge conditions cannot be satisfied due to failing checks.
You can take a look at Mergify Merge Queue check runs for more details about the failure.
> queue
#### ❌ The pull request has been removed from the queue `default`
<details>
The merge conditions cannot be satisfied due to failing checks.
You can take a look at `Mergify Merge Queue` check runs for more details about the failure.
</details>
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #307 — Phase-3 domain/platform below-cut review nits.
Two nits were listed. Nit 1 is fixed (with a test); Nit 2 is intentionally deferred with the reasoning below rather than forced.
Nit 1 —
MainActivity/NotificationIntents: exported activity consumedACTION_OPEN_MESSAGEwithout caller verification — FIXEDMainActivityisexported="true"(launcher /mailto:/ share). The per-message notification intent is explicit and carries no manifest intent-filter, but a hostile app can still craft an explicit intent at the exported component withaction = ACTION_OPEN_MESSAGE+ a message id and drive the reader to an arbitrary cached message. (Impact is limited — it only makes our app render an already-cached message in-app, and app-lock gates it — hence "below the cut". No data exfiltration.)Implemented the maintainer's second option — "trust only the app's own PendingIntent":
openMessage(...)now attaches an unforgeable sender-tokenPendingIntent. APendingIntent's creator package is stamped by the system and cannot be forged: only this app can mint one whosegetCreatorPackage()is our package. The token is immutable and never sent (a package-scoped broadcast to no receiver), so it adds no new attack surface.messageId(context, intent)yields the id only when that token is present and was created by us; otherwise it returnsnull. A foreign caller carries no token (or one attributed to its own package), so its intent is ignored.AppLog.w(no message id, which is PII).Why this can't be done more simply: notification taps don't go through
startActivityForResult, sogetCallingPackage()isnulleven for our own legitimate flow; andgetReferrer()is caller-overridable (EXTRA_REFERRER), so it isn't a trustworthy check. The nested sender-tokenPendingIntentis the standard robust way to verify "this intent was built by us". Change is contained to the existingNotificationIntentsbuild/parse contract plus the singleMainActivitycall site —mailto:/share are deliberately left open (that's the whole point of an email client).Test:
NotificationIntentsTestgainsopen_message_intent_without_our_sender_token_is_ignored(a same-action, token-less intent is rejected while the genuine one still resolves). Existing round-trip cases move to the newmessageId(context, intent)signature. This is an instrumented test because the behaviour hinges onPendingIntent.getCreatorPackage(), which needs the real framework — matching the existingNotificationIntentsTest, which is instrumented for the same reason (realIntent/Uri/PendingIntent). It compiles locally (compileDebugAndroidTestKotlin) and runs in CI's E2E matrix.Nit 2 —
MailNotifier:messageId.hashCode()notification id can collide — DEFERRED (not fixed here), with reasoningThe finding is technically valid but I did not apply the suggested "persisted per-account counter" in this below-cut cleanup, because forcing it in would be disproportionate and would trade away a documented property for a negligible gain:
No stateless function can be collision-free. Notification ids are 32-bit
Intand message ids are arbitrary strings — by pigeonhole, anyString → Intmapping has collisions. So only a stateful (persisted) scheme can literally satisfy "collision-free"; no small tweak to the currenthashCode()derivation qualifies, and shipping a cosmetic non-fix would be worse than leaving it.The practical probability is ~1e-8 per notification. A collision only silently replaces a notification when a new message's id happens to equal a currently-displayed, unacknowledged notification's id. That active set is tiny (typically well under ~50), so per-event P ≈ 50 / 2^32 ≈ 1.2e-8 — i.e. on the order of 0.1% of users might ever see a single dropped notification, and the mail itself is still in the app.
The suggested fix is disproportionate/risky for a P4 below-cut nit.
MailNotifieris currently a clean, dependency-free@Singleton(just aContext). A persisted per-account counter means either a Room migration (the natural home is the per-account settings table — the exact "storage/migration" category we treat with extra care) or a new DataStore/prefs seam, plus threading async persisted I/O into the notification-posting path and makingnotifyNewMailsuspend. That's a lot of new failure surface for a ~1e-8 issue.A naive persisted counter also changes documented semantics. The current id is deliberately stable per message ("a later batch never overwrites an earlier, still-unacknowledged one"); a monotonic counter is stable per notify-event, not per message, so re-notifying the same message would duplicate rather than update in place. (In practice re-notify ~never happens —
MailSyncerfilters onexistingIdsand serialises syncs — so this isn't an observed regression, but it does abandon the property the code documents.) An in-memory counter would be strictly worse than the status quo (it restarts low after process death and would collide with still-showing pre-death notifications).Recommendation: if this is worth pursuing, do it as its own tracked change (not folded into a nits-cleanup PR): a per-account monotonic counter persisted in a small dedicated DataStore (avoids a Room migration), assigned once per notify-batch (single edit, not one write per message), skipping the account's summary id. Happy to take that as a separate ticket if you'd like it prioritised over "accept as-is".
Gate
Local fast gate green (
assembleDebug+testDebugUnitTest+jacocoTestCoverageVerification(floor 0.84 held) +compileDebugAndroidTestKotlin+lintDebug+ktlintCheck+detekt) —BUILD SUCCESSFUL. The instrumentedNotificationIntentsTestcompiles locally and runs in CI's matrix.Merge Queue Status
2026-07-08 12:40 UTC· Rule:default· triggered by merge protections2026-07-08 13:52 UTC· at1ee6366bec9259a1864721519295e1d5e7262f38This pull request spent 1 hour 12 minutes 20 seconds in the queue, with no time running CI.
Waiting for
check-success = CI passedcheck-neutral = CI passedcheck-skipped = CI passedcheck-success = CI passedmain]check-neutral = @github-actions/CI passedcheck-skipped = @github-actions/CI passedcheck-success = @github-actions/CI passedAll conditions
check-success = CI passedcheck-neutral = CI passedcheck-skipped = CI passedcheck-success = CI passedmain]:check-neutral = @github-actions/CI passedcheck-skipped = @github-actions/CI passedcheck-success = @github-actions/CI passed-conflict-draftbase = maingithub-review-approved[🛡 GitHub repository ruleset rulemain]label != brokencheck-success = Debug buildcheck-neutral = Debug buildcheck-skipped = Debug buildcheck-success = Unit testscheck-neutral = Unit testscheck-skipped = Unit testsReason
The merge conditions cannot be satisfied due to failing checks
Hint
You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a
@mergifyio queuecomment.Requeued — the merge queue status continues in this comment ↓.
Merge Queue Status
Required conditions to enter a queue
-closed[📌 queue requirement]-conflict[📌 queue requirement]-draft[📌 queue requirement]-mergify-configuration-changedcheck-success = Configuration changedcheck-neutral = Mergify Merge Protectionscheck-skipped = Mergify Merge Protectionscheck-success = Mergify Merge Protectionsdefault]:-conflict-draftbase = maincheck-success = CI passedgithub-review-approved[🛡 GitHub repository ruleset rulemain]label != brokencheck-success = Debug buildcheck-neutral = Debug buildcheck-skipped = Debug buildcheck-success = Unit testscheck-neutral = Unit testscheck-skipped = Unit testscheck-success = CI passedcheck-neutral = CI passedcheck-skipped = CI passedmain]:check-success = @github-actions/CI passedcheck-neutral = @github-actions/CI passedcheck-skipped = @github-actions/CI passed@Mergifyio refresh
✅ Pull request refreshed
❌ The pull request has been removed from the queue
defaultThe merge conditions cannot be satisfied due to failing checks.
You can take a look at
Mergify Merge Queuecheck runs for more details about the failure.@Mergifyio requeue
Merge Queue Status
2026-07-08 19:04 UTC· Rule:default· triggered by @JMR-dev with the@mergifyio queuecommand2026-07-08 19:45 UTC· at1ee6366bec9259a1864721519295e1d5e7262f38· mergeThis pull request spent 41 minutes 12 seconds in the queue, including 27 minutes 14 seconds running CI.
Required conditions to merge
-conflict-draftbase = maincheck-success = CI passedgithub-review-approved[🛡 GitHub repository ruleset rulemain]label != brokencheck-success = Debug buildcheck-neutral = Debug buildcheck-skipped = Debug buildcheck-success = Unit testscheck-neutral = Unit testscheck-skipped = Unit testscheck-success = CI passedcheck-neutral = CI passedcheck-skipped = CI passedmain]:check-success = @github-actions/CI passedcheck-neutral = @github-actions/CI passedcheck-skipped = @github-actions/CI passed