fix: send from the address a message was delivered to (shared/aliased mailboxes) - #918
Open
rotterp wants to merge 3 commits into
Open
fix: send from the address a message was delivered to (shared/aliased mailboxes)#918rotterp wants to merge 3 commits into
rotterp wants to merge 3 commits into
Conversation
On a shared, group or aliased mailbox every reply, reply-all and forward
went out as `identities[0]` - the account owner - no matter which mailbox
was open. Users hit this as "it keeps sending as the wrong address and I
have to fix the From by hand every time".
Two DIFFERENT behaviours were fused behind `autoSelectReplyIdentity`:
1. pick among the user's OWN configured identities (exact address, then
`+tag`-stripped). Never rewrites `From:` - it only chooses which of
your own addresses sends.
2. the domain catch-all: rewrite `From:` to an address you have NOT
configured, sending through a same-domain identity.
(1) is what makes a shared mailbox reply from its own address, and it is
what people expect. It carries no impersonation risk, so it is now
unconditional - `findReplyIdentityId` already implemented exactly this and
was until now referenced only from tests.
(2) stays behind the setting, whose description already promises exactly
that behaviour ("for domain catch-all deliveries, rewrites the From header
to the alias"). It must stay opt-in: `ownedDomains` in `resolveReplyFrom`
treats ANY address on a domain you hold an identity on as a catch-all
candidate - including a colleague's, and including one that only appeared
in `Bcc:` - so on a multi-user domain it can put someone else's address in
your From. Enabling that for everyone would have been a regression, not a
fix.
`forward` is additionally admitted to (1). It was excluded, so forwarding a
message that arrived at a shared mailbox went out as the owner - even
though the comment above the gate already promised "Reply/forward fall
through to the recipient-based resolution below", and the neighbouring
inline-image effect in the same file groups all three modes together.
Forwards deliberately do NOT take (2) even when it is enabled: a reply
continues a thread whose participants already know the addressing, while a
forward introduces the rewritten From to a recipient the user just typed,
who has no way to tell it is not really from that person.
`handleQuickReply` in `components/mail/mail-app.tsx` is likewise narrowed
to (1). That surface is a bare text box with no From row, so a rewritten
From would be applied with nothing on screen to show it - or to correct
it. It now resolves own identities only; the full composer remains
available when the rewrite is wanted.
Tests cover: reply and forward from the delivered-to address; no rewrite
while the setting is off; no rewrite on a forward even with it on; and, as
a control, that the rewrite still happens on a reply with it on - so the
first two prove the gate rather than a broken resolver. The settings mock
in `reply-addressing.test.tsx` was flipped to the shipped default (it hard
-coded the setting on, which is why this gap had no coverage). Verified
that restoring the old gate turns exactly the three positive cases red.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ProComposeTabBody` rendered `<EmailComposer>` without `composeFromAccountEmail`; only the standard shell in `components/mail/mail-app.tsx` passed it. Without that prop `mode === 'compose'` resolves `findComposeIdentityId(identities, undefined)` to null, so in the Pro/embedded shell every new message defaulted to the account owner even with a shared folder open. That covered the whole compose surface there, not an edge: the Pro shell hoists every "show composer" intent into a tab - including `mailto:` links - and the inline composer never renders while it is active. Resolved exactly as the standard shell does, so both shells share one rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`handleStateChange` gated the email-list refresh on the PRIMARY account:
const accountChanges = change.changed[accountId];
if (accountChanges?.Email) {
await get().refreshCurrentMailbox(client);
Stalwart's SSE only pushes StateChange for the primary account, never for
a delegated/shared owner, so changes to a shared folder arrive solely via
the client's secondary poll - reported under the OWNER's accountId. With a
shared folder open, a background Email change therefore refreshed the
folder COUNTERS (`anyMailboxChanged` already reacts to every account) but
never refetched the rows, leaving the visible list stale until a manual
reload.
That is most visible when something outside the client mutates a message:
a server-side keyword patch - e.g. an operator job stamping a colour label
- or another member of the shared mailbox acting on it simply did not
appear. The counters moving while the list did not is the tell.
Uses the existing `resolveViewAccountId()` helper, which answers "which
owner account am I looking at" and resolves through `resolveActionMailboxes`
so it stays correct in the multi-account shell. It returns undefined for a
normal own-account view, leaving that path unchanged. The aggregate views
contribute shared accounts too, so they take any contributing account's
Email change; `refreshCurrentMailbox` already coalesces, which bounds the
extra refetches a busy shared mailbox can trigger.
Scope worth stating: this refreshes the first page of the open list, which
is what `refreshCurrentMailbox` fetches - rows scrolled past it are still
re-appended from local state. Nor is it instant: the shared-account poll
runs on the secondary (20s) interval and is skipped while the tab is
hidden.
Tests cover the owner-account case, the aggregate-view case, and four
controls: primary-account (unchanged), an unrelated account, a
Mailbox-only change, and an own-account view. Verified that restoring the
primary-only gate turns exactly the owner and aggregate cases red.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On a shared, group or aliased mailbox every reply, reply-all, forward and new
message went out as
identities[0]— the account owner — no matter whichmailbox was open. Users hit this as "it keeps sending as the wrong address and
I have to fix the From by hand every time".
The cause
Two different behaviours were fused behind
autoSelectReplyIdentity, whichships off:
(exact address, then
+tag-stripped). Never rewritesFrom:; it onlychooses which of your own addresses sends. This is the Thunderbird/Outlook
default.
From:, sending through a same-domain identity ([Feature]: Make the from-address field editable #246).(1) is what makes a shared mailbox reply from its own address. It carries no
impersonation risk, so this makes it unconditional.
findReplyIdentityIdalready implemented exactly this and was, until now, referenced only from tests.
(2) stays behind the setting, whose description already promises precisely that
behaviour. It has to stay opt-in:
ownedDomainstreats any address on adomain you hold an identity on as a catch-all candidate — including a
colleague's, and including one seen only in
Bcc:— so on a multi-user domainit can put someone else's address in your From. Enabling it for everyone would
have been a regression, not a fix.
What else this fixes
forwardwas excluded from the resolution entirely, so forwarding amessage that arrived at a shared mailbox went out as the owner — even though
the comment above the gate already promised "Reply/forward fall through to
the recipient-based resolution below", and the neighbouring inline-image
effect in the same file groups all three modes together. Forwards take (1)
but deliberately never (2): a reply continues a thread whose participants
know the addressing, while a forward introduces a rewritten From to a
recipient the user just typed, who cannot tell it isn't genuine.
handleQuickReplyis narrowed to (1). That surface is a bare text boxwith no From row, so a rewritten From would be applied with nothing on screen
to show it — or correct it.
ProComposeTabBodynever passedcomposeFromAccountEmail, so in thePro/embedded shell
mode === 'compose'resolved to null and every newmessage defaulted to the account owner. That covered the whole compose
surface there, since the Pro shell hoists every "show composer" intent —
mailto:links included — into a tab.handleStateChangegated the email-list refresh on the primary account.Stalwart's SSE never pushes StateChange for a delegated/shared owner, so
those changes arrive only via the secondary poll under the owner's
accountId. With a shared folder open, a background change refreshed the
folder counters (
anyMailboxChangedalready reacts to every account) butnever refetched the rows — so a server-side keyword patch, or another member
of the shared mailbox acting on it, stayed invisible until a hard reload.
Uses the existing
resolveViewAccountId()helper.Scope and limits
The refresh covers the first page of the open list, which is what
refreshCurrentMailboxfetches; rows scrolled past it are still re-appendedfrom local state. It is not instant either — the shared-account poll runs on
the secondary (20s) interval and is skipped while the tab is hidden.
Testing
16 tests across the two suites. Each fix was verified by reverting it and
confirming the intended cases go red while the controls stay green:
Controls included on purpose: the catch-all rewrite still fires on a reply
when the setting is on, so the "no rewrite" tests prove the gate rather than a
broken resolver; and Mailbox-only changes, unrelated accounts and own-account
views must not trigger a refetch.
Note the composer suite's settings mock hard-coded
autoSelectReplyIdentity: truewhile the shipped default isfalse— so the shipped configuration wasthe one nothing exercised. The mock now matches the default.
tsc --noEmitandeslintare clean. 652 tests pass across the affectedsuites; the 3
auth-store-logoutfailures are pre-existing onmain.