Page MenuHomePhabricator

Handle inbound email with phabricator address in the CC
Closed, ResolvedPublic

Description

It appears that the incoming mail processor correctly handles mail when the address is in the To: field, but rejects mail when it is in the Cc: field.

Upon sending a message with headers like this:

delivered-to: phabricator+D12345+public+0deadbeef991@mail.internal
received: Array
x-received: Array
from: Jane Doe <jdoe@mail.internal>
date: Tue, 18 Nov 2014 11:52:36 -0800
message-id: <77c1521bee8562922bf5159c9afd3c54@mail.internal>
subject: Re: [Request, 666 lines] D12345: Frobnicate the bazzifier
to: Joe Smith <jsmith@mail.internal>
cc: phabricator+D12345+public+0deadbeef991@mail.internal

Users report getting this bounce:

Your email to Phabricator was not processed, because an error occurred while
trying to handle it:

Phabricator can not process this mail because no application knows how to
handle it. Check that the address you sent it to is correct.

(No concrete, enabled subclass of PhabricatorMailReceiver can accept this
mail.)

Event Timeline

lfaraone created this task.Mar 6 2015, 6:36 PM
lfaraone updated the task description. (Show Details)
lfaraone raised the priority of this task from to Needs Triage.
lfaraone added projects: User Delight, Mail.
lfaraone added subscribers: lfaraone, aroravishal, charles.
chad renamed this task from Handle inbound email with phabricator address in the CC. to Handle inbound email with phabricator address in the CC.

Is this the actual use case (CC'ing a revision)? Specifically, not CC'ing something like bugs@phabricator.company.com? Is there a reason the revision address is in the CC line instead of the "To" line? Presumably, the author had to move it there explicitly, which seems a little unusual.

If mail CCs a revision, a task, and several bugs@ addresses, what is the expected behavior?

Not directly related to replies, but to creating tasks via email.

Some collegues of mine have tried to create them over email, but supplied the address in the CC, which did not work - probably for the same reason. They've since then learned to place the address for creating tasks as the "To", but it did not seem intuitive at all.

I think it's probably reasonable for us to look for a valid address anywhere in the recipient list. CC'ing bugs@ makes a lot of sense to me, in particular.

I'm just not sure what the expected behavior is if there are multiple valid addresses (I'd say: reject the mail, but maybe that's not what anyone else would expect).

And I'm just wondering if there's some deeper or more complex use case for a revision to end up in CC, since it seems that must have been intentional.

chad added a subscriber: rjkaplan.
chad added a subscriber: chad.

Actually, this seems related, but different. My bad.

rjkaplan removed a subscriber: rjkaplan.Mar 11 2015, 9:14 PM

I'm just not sure what the expected behavior is if there are multiple valid addresses (I'd say: reject the mail, but maybe that's not what anyone else would expect).

I think the best behavior here is:

  • Let all receivers try to accept every address.
  • If any receiver accepts any address, accept the mail.
  • Call each receiver once, passing it all of the addresses it accepts.
  • Receivers get to decide what they do with mail to multiple acceptable addresses.

In particular, this means:

  • Mail to events@ with bugs@ CC'd will create both a new Calendar event and a new task.
  • Mail to frontend-bug@ with backend-bugs@ CC'd will create only one task (unless the handler opts to do something different), but it will be created via "both" addresses.
  • Mail to several users could create a thread with all of them (what do we do now? Pick the first one?).

So most of the work here is changing the APIs which accept/expect one address to take a list of addresses.

Just to chime in, one use case I've seen that often accidentally repros this bug is:

  • Someone comments on a task, in Phabricator. This triggers an email to the subscribers.
  • One of the subscribers (person A) accidentally replies-to-all with a comment. This sends two emails -- the first is from:A, to:phabricator, cc:subscribers, and the second (a few minutes later) is from:phabricator to:subscribers.
  • One of the other subscribers (person B) sees this first email from A and does another reply-all with a follow-up comment. This sends an email from:B to:A cc:phabricator and the other subscribers. (Sometimes, but not always, this creates a Conpherence.)

I would like the email from B to comment on the task. Your proposal, above, sounds reasonable.

One of the subscribers (person A) accidentally replies-to-all with a comment. This sends two emails -- the first is from:A, to:phabricator, cc:subscribers, and the second (a few minutes later) is from:phabricator to:subscribers.

At least at HEAD, this is unexpected: users should not receive email generated in response to email which they were CC'd on. T7533 is a task complaining that users don't receive this email; rPd9c6e07f is the original change which silenced this email. So either we have a separate bug or something else is afoot here.

(Sometimes, but not always, this creates a Conpherence.)

Oh, you have Phabricator receiving email on the same domain that users receive personal email? We should probably do something like not accept email as Conpherence email if the address is registered to a user.

Specifically, I'd guess that the "sometimes" condition is "when a recipient's @company.com address is the same as their Phabricator username".

Our intent here is for epriestley@phabricator.mycompany.com to create a Conpherence, but our assumption is that Phabricator is not receiving mail on a "real" domain, which obviously doesn't hold up under various reasonable configurations.

jhurwitz added a project: Restricted Project.May 20 2015, 12:47 AM
liza added a subscriber: liza.Jun 5 2015, 2:06 AM

This is not blocked, although it's a moderate amount of work (4-6 hours?) and not adjacent to other work.

This just bit us again, via a different repro.

Someone replied to an audit and received an error:

Phabricator is not able to process this mail because more than one application
is willing to accept it, creating ambiguity. Mail needs to be accepted by
exactly one receiving application.
Accepting receivers: PhabricatorAuditMailReceiver,
PhabricatorCountdownMailReceiver.

I have no clue what Countdown is or why it has a receiver on this address. (I've temporarily addressed the problem by uninstalling Countdown.)

Your proposed solution ("Mail to events@ with bugs@ CC'd will create both a new Calendar event and a new task.") would also fix this.

That just looks like a bug that's only tangentially related, I think Audit and Countdown are both trying to receive mail at C... addresses.

eadler added a project: Restricted Project.Jan 9 2016, 12:36 AM
eadler edited projects, added Restricted Project; removed Restricted Project.
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Jul 4 2016, 9:16 PM

Is there any sense of when this might be worked on? We end up getting "Error Processing Mail (No Receivers)" due to Phabricator being in the CC a few times a week, and it's really annoying.

Sorry, we never have any idea. See Planning for discussion.

chad added a comment.Sep 6 2016, 4:27 PM

@jonah214 If it helps, unlikely to happen in the next 6 months given current prioritization queue. We are actively looking to hire though to widen our engineering throughput.

Thanks; that does help (and also disappoint).

eadler moved this task from Backlog to Nits on the llvm board.
lyngvi added a subscriber: lyngvi.Jan 5 2017, 4:30 PM

Patch attached which addresses this - filter and object selection done on a new getToOrCCAddresses call. This patch is based off stable branch commit 58375fa9e6db4a389fd6029ee1ad14ddb0dc9e90.

Arguable whether this should be a configurable setting. My personal opinion is that the distinction between CC and To is immaterial for the purposes of, e.g., issue tracking; but beware that I'm not deeply versed in Phabricator development or lore, so there may be things I've missed.

Some email clients (notably GMail) automatically shuffle the Phabricator address to the CC line when a user hits "reply all." The user then receives either bounce (email not honored), and Phabricator's comments aren't updated with the user replies. Yes, we could tell the user to change their email client settings, but most of our users couldn't be bothered.

alexmv added a subscriber: alexmv.May 15 2017, 2:45 PM

Phacility folks, do you have thoughts about the patch provided by @lyngvi above? Would be it be easier to review if submitted as a Diff?

Per above, see Planning. If that isn't clear, let me know how we could improve it.

Sorry, that seemed to be in response to "are you working on this?" A user has since provided a patch for the feature, which is the sort of thing which in the past you folks have generally been nice enough to review.

Are you saying that you no longer review code submissions for features?

Many of the patches you've personally contributed in the past have bypassed the bulk of the requirements under the "Rise to Prominence" clause of Contributing Code, since you have a history with the project, are a member of Community, and have generally contributed very focused patches with clear context:

https://secure.phabricator.com/book/phabcontrib/article/contributing_code/#alternatives

This is exceptionally rare -- so rare that you are possibly the individual user who has faced the lowest barrier to upstreaming things in the last year or so outside of Phacility employees. There are a handful of other users who consistently contribute high-quality, focused patches where the time spent reviewing them immediately pays for itself, and these patches get to bypass a bunch of requirements (but they generally adhere to the rules anyway).

Here, @lyngvi has no history with the project. I can't tell if they've signed a CLA since they didn't submit through Differential, so I can't look at the patch. I'd be surprised if the patch implements the behavior outlined in T7477#111380, since I expect that to be a relatively large change beyond the reach of most first-time contributors (I quoted 4-6 hours for us, above, it looks like), and the description mentions "shuffling" but the behavior I proposed would be order-independent.

If you, e.g., work with @lyngvi and can sign a CLA on behalf of the patch, you're welcome to personally submit the patch under your own mantle of renown, but I'm unlikely to bring it upstream if it doesn't implement something along the lines of the behavior in T7477#111380.

In most cases (when patches are not submitted by a well-known contributor) they're subject to the full weight of rules and barriers in Contributing Code / Planning, and I think we're pretty consistent about this.

Understood -- thanks for the clarification. One isn't always aware of how special-case one is. :)

The patch indeed does not implement all of T7477#111380 -- I'll take a deeper look at what it would take to accomplish, and perhaps submit a diff accordingly.

epriestley moved this task from Backlog to Stamps/Failover on the Mail board.Jan 27 2018, 9:57 PM

The scope on T13053 didn't end up making it very far in this direction so I think this missed the boat for now.

epriestley moved this task from Stamps/Failover to Future on the Mail board.Feb 8 2018, 6:02 PM
epriestley moved this task from Future to Soon? on the Mail board.Wed, Jan 2, 3:19 PM
epriestley triaged this task as Normal priority.