Page MenuHomePhabricator
Create Task
Maniphest T13235

Replace "SimpleEmailService" with a subclass of "PhutilAWSFuture" that surfaces errors more clearly
Closed, ResolvedPublic
Actions

Description

We currently use an external library for SES.

This is one ~750 line file which doesn't do anything very interesting, and is awful at handling errors. In particular, the "you have not verified your From address" error is routine from SES, but not exposed to the user since the class throws a generic exception when the HTTP response is not HTTP 2XX.

It throws this:

[2019-01-14 07:55:37] EXCEPTION: (SimpleEmailServiceException) Unexpected HTTP status while making request to Amazon SES: expected 200, got 400.

...but the body is far more clear:

<ErrorResponse xmlns="http://ses.amazonaws.com/doc/2010-12-01/">
  <Error>
    <Type>Sender</Type>
    <Code>MessageRejected</Code>
    <Message>Email address is not verified. The following identities failed the check in region US-EAST-1: noreply@phacility.com, Phabricator &lt;noreply@phacility.com&gt;</Message>
  </Error>
  <RequestId>d5f1d933-1814-11e9-b4a5-234246d388a5</RequestId>
</ErrorResponse>

Revisions and Commits

Restricted Differential RevisionRestricted Diffusion Commit
Restricted Differential RevisionRestricted Diffusion Commit

Related Objects
Search...

Event Timeline

epriestley triaged this task as Low priority.Jan 14 2019, 4:00 PM
epriestley created this task.
Herald added a subscriber: amckinley. · View Herald TranscriptJan 14 2019, 4:00 PM
epriestley renamed this task from Replace "SimpleEmailService" with a subclass of "PhutilAWSFuture" to Replace "SimpleEmailService" with a subclass of "PhutilAWSFuture" that surfaces errors more clearly.Jan 14 2019, 4:01 PM
epriestley added a comment.Jan 14 2019, 4:51 PM

See also https://discourse.phabricator-community.org/t/suggestions-show-failed-email-in-ui-send-test-error-console-output/1844.

epriestley added a comment.Jan 14 2019, 4:54 PM

The replacement should also setTimeout(...), per T5969.

epriestley moved this task from Future to Infrastructure on the Mail board.Jan 14 2019, 4:55 PM
amckinley added a revision: Restricted Differential Revision.Jan 16 2019, 7:09 PM
amckinley added a comment.Jan 16 2019, 7:11 PM

When I was messing around with SMS sending last week, I put a rough cut of this together that is capable of sending an SMS using SNS, which has almost the same API as SES: D19982. I can tackle this unless you're already in the middle of it.

epriestley added a comment.Jan 16 2019, 7:35 PM

D19955 has how I think this likely breaks down, I'm not planning to touch it in this iteration since the badness, while bad, is mostly in a box (hopefully) until the next PHPMailer CVE. All yours!

amckinley added a commit: Restricted Diffusion Commit.Jan 24 2019, 12:30 AM
joshuaspence added a subscriber: joshuaspence.Feb 2 2019, 1:40 AM
arian.maykon added a subscriber: arian.maykon.Apr 22 2019, 5:15 PM
epriestley mentioned this in Dependencies.May 13 2019, 7:01 PM
epriestley mentioned this in T13570: Amazon is retiring SMTP V3 Signatures.Aug 13 2020, 4:00 PM
epriestley added a parent task: T13570: Amazon is retiring SMTP V3 Signatures.Sep 15 2020, 4:08 PM
epriestley added a revision: Restricted Differential Revision.Sep 15 2020, 4:15 PM
epriestley added a comment.Sep 15 2020, 4:19 PM

I've marked D21461 as resolving this, since the new errors surface the particulars:

[2020-09-15 09:10:59] EXCEPTION: (PhutilAWSException) AWS Request Failed
HTTP Status Code: 400
Response Body: <ErrorResponse xmlns="http://ses.amazonaws.com/doc/2010-12-01/">
  <Error>
    <Type>Sender</Type>
    <Code>MessageRejected</Code>
    <Message>Email address is not verified. The following identities failed the check in region US-WEST-2: ses@example.net, Phabricator &lt;ses@example.net&gt;</Message>
  </Error>
  <RequestId>877f232a-2715-40e1-a806-22123a866f85</RequestId>
</ErrorResponse>
 at [<arcanist>/src/future/aws/PhutilAWSFuture.php:164]
...
epriestley closed this task as Resolved by committing Restricted Diffusion Commit.Sep 15 2020, 8:03 PM
epriestley claimed this task.
epriestley added a commit: Restricted Diffusion Commit.
Phabricator · Built by Phacility