Summary: See detailed discussion in T1543.
Test Plan:
- Enabled multiplexing.
- Set user A to "enable Re".
- Created a task owned by user B with user A cc'd.
- Verified A got no "Re:" before this patch.
- Applied patch.
- Verified A got "Re:" after this patch.
Reviewers: nh, btrahan, vrana
Reviewed By: nh
CC: aran
Maniphest Tasks: T1543
Differential Revision: https://secure.phabricator.com/D3031
Summary: This keeps people in the correct To or CC field on multiplexed messages.
Test Plan:
with multiplexing on, checked that I received an email with me in the CC
field instead of the To field for a diff I'm CC'd on.
Reviewers: epriestley, jungejason, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2999
Summary:
See T931. LLVM users would also prefer simpler mail, and have similarly harsh opinions about the current state of affairs.
Allow installs to disable the hint blocks if they don't want them.
Test Plan: Sent myself mail with these settings on/off, got mail with/without the blocks.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T931
Differential Revision: https://secure.phabricator.com/D2968
Summary: This is a minor quality-of-life improvement to prevent D2968 from being as nasty as it is.
Test Plan: Ran unit tests; generated Differential, Maniphest and Diffusion emails and verified the bodies looked sensible.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T931
Differential Revision: https://secure.phabricator.com/D2986
Summary:
- Assigning $cc_phids clobbers the correct value assigned on line 80.
- Remove stack trace noise, the trace is always meaningless and well-known.
- Actually show the original body.
Test Plan: Piped mail to the mail receiver and verified the errors didn't CC revision CCs, no longer had traces, and included the original raw text body.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2969
Summary: People want to create filters checking if they are in CC which is almost impossible with multiplexing in Outlook.
Test Plan: Sent e-mail with multiple CCs, verified headers.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2829
Summary:
Works this way:
- Select users' language with multiplexing.
- Select default language otherwise (it can be different from current user's language).
- Build body and subject for each user individually.
- Set the original language after sending the mails.
Test Plan:
- Comment on a diff of user with custom translation.
- Set default to a custom translation. Comment on a diff of user with default translation.
- Set default to a default translation. Comment on a diff of user with default translation.
Repeat with/without multiplexing.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1139
Differential Revision: https://secure.phabricator.com/D2774
Summary:
It seems that Outlook and Mail.app mostly ignores the threading headers and thread primarily by subject.
They are also very picky about the Re: part in the header.
I guess that's because users of these clients often hit Reply when they want to create a new message to the sender of an e-mail.
We need both of these applications to work with the same setting because we don't use multiplexing to prevent sending multiple e-mails to people in lists.
I also believe that the default behavior should just work in most setups.
I've tried several different combinations of putting "Re:" and none of them seems to always work in both clients.
This diff at least adds more abstraction to the code which should prevent copy/paste errors (two fixed by this diff!).
Test Plan: Sent several e-mails with varying subject, verified that they look as before in Outlook and Mail.app.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2709
Summary:
- `kill_init.php` said "Moving 1000 files" - I hope that this is not some limit in `FileFinder`.
- [src/infrastructure/celerity] `git mv utils.php map.php; git mv api/utils.php api.php`
- Comment `phutil_libraries` in `.arcconfig` and run `arc liberate`.
NOTE: `arc diff` timed out so I'm pushing it without review.
Test Plan:
/D1234
Browsed around, especially in `applications/repository/worker/commitchangeparser` and `applications/` in general.
Auditors: epriestley
Maniphest Tasks: T1103
Summary: They were only displaying seconds. I found a function in viewutils.php that allowed for single-unit precision formatting, but I wanted more, so I wrote another function to allow more detail.
Test Plan: [site]/mail, and watch it work. It's a new function, so it shouldn't break anything else.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1296
Differential Revision: https://secure.phabricator.com/D2616
Summary:
It's currently possible to configure Phabricator to send mail to some address it recognizes as relating to an object.
When we receive mail from Phabricator, drop it unconditionally.
Test Plan: Wrote two emails, one with the header and one without. Piped them to `mail_handler.php`, one was dropped immediately.
Reviewers: btrahan, nh, mikaaay, jungejason
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2529
Summary:
Since user emails aren't in the user table, we had to do extra data fetching
for handles, and the emails are only used in MetaMTA, so we move the email
code into MetaMTA and remove it from handles.
Test Plan: send test emails
Reviewers: jungejason, vrana, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2494
Summary:
- Move email to a separate table.
- Migrate existing email to new storage.
- Allow users to add and remove email addresses.
- Allow users to verify email addresses.
- Allow users to change their primary email address.
- Convert all the registration/reset/login code to understand these changes.
- There are a few security considerations here but I think I've addressed them. Principally, it is important to never let a user acquire a verified email address they don't actually own. We ensure this by tightening the scoping of token generation rules to be (user, email) specific.
- This should have essentially zero impact on Facebook, but may require some minor changes in the registration code -- I don't exactly remember how it is set up.
Not included here (next steps):
- Allow configuration to restrict email to certain domains.
- Allow configuration to require validated email.
Test Plan:
This is a fairly extensive, difficult-to-test change.
- From "Email Addresses" interface:
- Added new email (verified email verifications sent).
- Changed primary email (verified old/new notificactions sent).
- Resent verification emails (verified they sent).
- Removed email.
- Tried to add already-owned email.
- Created new users with "accountadmin". Edited existing users with "accountadmin".
- Created new users with "add_user.php".
- Created new users with web interface.
- Clicked welcome email link, verified it verified email.
- Reset password.
- Linked/unlinked oauth accounts.
- Logged in with oauth account.
- Logged in with email.
- Registered with Oauth account.
- Tried to register with OAuth account with duplicate email.
- Verified errors for email verification with bad tokens, etc.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1184
Differential Revision: https://secure.phabricator.com/D2393
Summary: "Committed" is SVN-specific language, and confusing in Git and Mercurial. Use neutral language instead.
Test Plan: Inspection.
Reviewers: btrahan, Makinde, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T909
Differential Revision: https://secure.phabricator.com/D2087
Summary: This appears to sometimes be effective (for MS clients), and we've seen it in the wild on inbound mail.
Test Plan: Sent myself some mail, verified it had the right header.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T571
Differential Revision: https://secure.phabricator.com/D2241
Summary: See rP23f25edd97f052ff4c1c5d8c4be962b4da149bca.
Test Plan: RAN LINT AND UNIT TESTS. VERIFIED THERE ARE NO SYNTAX ERRORS.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2240
Summary:
- Add an explicit multiplexing option, and enable it by default. This is necessary for Mail.app to coexist with other clients ("Re:" breaks outlook at the very least, and generally sucks in the common case), and allows users with flexible clients to enable subject variance.
- Add an option for subject line variance. Default to not varying the subject, so mail no longer says [Committed], [Closed], etc. This is so the defaults thread correctly in Gmail (not entirely sure this actually works).
- Add a preference to enable subject line variance.
- Unless all mail is multiplexed, don't enable or respect the "Re" or "vary subject" preferences. These are currently shown and respected in non-multiplex cases, which creates inconsistent results.
NOTE: @jungejason @nh @vrana This changes the default behavior (from non-multiplexing to multiplexing), and might break Facebook's integration. You should be able to keep the same behavior by setting the options appropriately, although if you can get the new defaults working they're probably better.
Test Plan:
Send mail from Maniphest, Differential and Audit. Updated preferences. Enabled/disabled multiplexing. Things seem OK?
NOTE: I haven't actually been able to repro the Gmail threading issue so I'm not totally sure what's going on there, maybe it started respecting "Re:" (or always has), but @cpiro and @20after4 both reported it independently. This fixes a bunch of bugs in any case and gives us more conservative set of defaults.
I'll see if I can buff out the Gmail story a bit but every client is basically a giant black box of mystery. :/
Reviewers: btrahan, vrana, jungejason, nh
Reviewed By: btrahan
CC: cpiro, 20after4, aran
Maniphest Tasks: T1097, T847
Differential Revision: https://secure.phabricator.com/D2206
Summary: Sometimes we get a lowercase "Meddelelse" in Danish outlook. Relax the patterns since the risk of hitting false positives here is essentially nonexistant.
Test Plan: Unit tests.
Reviewers: davidreuss, btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2205
Summary: See discussion in T789. Covered the obvious cases, at least. We can refine this as we get a larger sample size.
Test Plan: Unit test coverage.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T789
Differential Revision: https://secure.phabricator.com/D2154
Summary: NOTE: This is not produced by a script so there might be errors. Please review carefully.
Test Plan: Browse around.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2091
Summary: Added a regex to remove the text
Test Plan: Tested a few messages, from mail application them gmail, both seemed fine, will add unit tests
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2078
Summary: Last of the big final patches. Left a few debatable classes (12 out of about 400) that I'll deal with individually eventually.
Test Plan: Ran testEverythingImplemented.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1881
Summary:
These are all unambiguously unextensible. Issues I hit:
- Maniphest Change/Diff controllers, just consolidated them.
- Some search controllers incorrectly extend from "Search" but should extend from "SearchBase". This has no runtime effects.
- D1836 introduced a closure, which we don't handle correctly (somewhat on purpose; we target PHP 5.2). See T962.
Test Plan: Ran "testEverythingImplemented" unit test to identify classes extending from `final` classes. Resolved issues.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1843
Summary:
They end up in "CCs:" fields where they can't be parsed.
Not bothering to migrate since I think only Dropbox has hit this.
Also improved another error condition's handling.
Test Plan: Tried to save a mailing list with spaces and commas in the name.
Reviewers: btrahan, Makinde
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T947
Differential Revision: https://secure.phabricator.com/D1813
Summary:
Show the retry count in the meta mta view (in addition to the list of
messages) - I find this info useful when I'm trying to debug what's going on
with mail failures.
Task ID: #
Blame Rev:
Test Plan:
loaded /mail/view/NNNNN/ and saw the retry count
Revert Plan:
Tags:
Reviewers: epriestley, btrahan, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1782
Summary:
Added a query option of status for the MetaMTA list controller. There currently
isn't a ui for accessing this.
Task ID: #
Blame Rev:
Test Plan:
loaded /mail/, /mail/?status=queued, /mail?phid=PHID...&status=...
each request returned a sane list of data
Revert Plan:
Tags:
Reviewers: epriestley, btrahan, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1784
Summary:
This will allow sending mail to be done by task workers. See T750.
Task ID: #
Blame Rev:
Test Plan:
- started taskmaster daemon in test env
- used "send new test message" feature in MetMTA (with send now unchecked)
- confirmed receipt of 1 email
- repeated 2 & 3 with send now checked
Revert Plan:
Tags:
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T388, T750
Differential Revision: https://secure.phabricator.com/D1723
Summary:
See T926. If you want to write a mail rule that, e.g., captures Differential
mail but ignores people replying to it, it's kind of tricky right now. You can
use the 'X-Mail-Transport-Agent' header but that's not obvious and it's not
necessarily stable.
Add a nice, obvious "X-Phabricator-Sent-This-Message" header.
Test Plan: Sent myself some mail, verified the header appeared.
Reviewers: vrana, btrahan, fugalh, jungejason
Reviewed By: jungejason
CC: aran, epriestley
Maniphest Tasks: T926
Differential Revision: https://secure.phabricator.com/D1732
Summary:
When users submit an audit, send email to relevant parties informing them.
Allow email to be replied to. Just basic support so far; no "!raise" stuff and
no threading with the Herald commit notification.
Test Plan: Made comments, got email. Replied to email, got comments.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1698
Summary:
The mailing list page in MetaMTA only showed the first 100
sorted by ID, so it made it seem like lists were missing. Changed it to
do paging and short by name, so it has some user-understandable order.
Test Plan:
- Go to /mail/lists/
- Step through pager, confirm ordering.
Reviewers: nh, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1670
Summary:
A few similar requests have come in across several tools and use cases that I
think this does a reasonable job of resolving.
We currently send one email for each update an object receives, but these aren't
always appreciated:
- Asana does post-commit review via Differential, so the "committed" mails are
useless.
- Quora wants to make project category edits to bugs without spamming people
attached to them.
- Some users in general are very sensitive to email volumes, and this gives us
a good way to reduce the volumes without incurring the complexity of
delayed-send-batching.
The technical mechanism is basically:
- Mail may optionally have "mail tags", which indicate content in the mail
(e.g., "maniphest-priority, maniphest-cc, maniphest-comment" for a mail which
contains a priority change, a CC change, and a comment).
- If a mail has tags, remove any recipients who have opted out of all the
tags.
- Some tags can't be opted out of via the UI, so this ensures that important
email is still delivered (e.g., cc + assign + comment is always delivered
because you can't opt out of "assign" or "comment").
Test Plan:
- Disabled all mail tags in the web UI.
- Used test console to send myself mail with an opt-outable tag, it was
immediately dropped.
- Used test console to send myself mail with an opt-outable tag and a custom
tag, it was delivered.
- Made Differential updates affecting CCs with and without comments, got
appropriate delivery.
- Made Maniphest updates affecting project, priority and CCs with and without
comments, got appropriate delivery.
- Verified mail headers in all cases.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley, moskov
Maniphest Tasks: T616, T855
Differential Revision: https://secure.phabricator.com/D1635
Summary:
This got caught in the crossfire when we admin-only'd the whole MetaMTA tool. It
should not be admin only.
(Generally, we should probably separate this out better at some point.)
Test Plan: Hit /mail/sendgrid/ as a logged-out, non-admin user (like SendGrid
does).
Reviewers: s, btrahan
Reviewed By: s
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1588
your own actions
Summary:
- Mail.app on Lion has cumbersome threading rules, see T782. Add an option to
stick "Re: " in front of all threaded mail so it behaves. This is horrible, but
apparently the least-horrible option.
- While I was in there, I added an option for T228.
Test Plan:
- Sent a bunch of threaded and unthreaded mail with varous "Re:" settings,
seemed to get "Re:" in the right places.
- Disabled email about my stuff, created a task with just me, got voided mail,
added a CC, got mail to just the CC.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, mkjones
Maniphest Tasks: T228, T782
Differential Revision: https://secure.phabricator.com/D1448
Summary: There are lots of callsites to $changeset->getFilename() so it seemed
easier to rename getFileName() to getFilename() even if it includes database
change. Plus I think that getFilename() is better.
Test Plan:
Alter database.
Open revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1437
Summary:
I locked this down a little bit recently, but make
double-extra-super-sure that we aren't sending the user anywhere suspicious or
open-redirecty. This also locks down protocol-relative URIs (//evil.com/path)
although I don't think any browsers do bad stuff with them in this context, and
header injection URIs (although I don't think any of the modern PHP runtimes are
vulnerable).
Test Plan:
- Ran tests.
- Hit redirect page with valid and invalid next URIs; was punted to / for
invalid ones and to the right place for valid ones.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: arice, aran, epriestley, btrahan
Differential Revision: https://secure.phabricator.com/D1369
Summary:
- Add some captions to make it more clear what these fields mean.
- Require "name", since tokenizers use it exclusively.
- Limit URI to allowed protocols, since admins can currently XSS users by
entering a "javascript:" URI and then tricking the user into clicking the
mailing list name. This exploit is dumb, but technically privilege escallation.
Test Plan:
- Created a new mailing list.
- Edited a mailing list.
- Tested URI: valid, invalid, omitted.
- Tested name: valid, omitted.
Reviewers: btrahan, jungejason, davidreuss
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: https://secure.phabricator.com/D1365
Summary:
We have a debug interface for sending various sorts of email, but normal users
don't really need to use it. In particular, they can:
- Send arbitrary email to other users;
- Discover other users' email addresses fairly easily (CC everyone);
- Send arbitrary email to arbitrary addresses in conjunction with "Mailing
Lists"
In fact, normal users don't need to get to the MetaMTA web interface at all and
it has some somewhat-sensitive things beacuse it has a lot of detailed
information about mail. For instance, users can look at mail records to discover
things like password reset links and per-user object email addresses.
We should smooth out the UI here but I think I can do something about T21 fairly
soon and cover it then.
Test Plan:
Went to /mail/ with a non-admin, got 404'd. Went to /mail/ with an
admin, everything works, got a red admin header.
Reviewers: jungejason, btrahan
Reviewed By: btrahan
CC: aran, btrahan, jungejason
Maniphest Tasks: T718
Differential Revision: https://secure.phabricator.com/D1292
Summary:
It used to be more useful for daemons to spew random debugging information, but
features like "phd debug" and some fixes to error reporting like D1101 provide
better ways to debug, test, develop and diagnose daemons.
- Stop writing "." every time MetaMTA sends a message.
- Stop spewing the entire IRC protocol from the IRC bot unless in debug mode.
- Stop writing GC daemon log entries about collecting daemon logs (DURRR)
unless in debug mode.
Test Plan: Ran daemons in debug and non-debug modes, got expected level of
noisiness.
Reviewers: jungejason, nh, btrahan
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: https://secure.phabricator.com/D1268
Summary:
- For context, see T547. This is the last (maybe?) in a series of diffs that
moves us off raw sha1() calls in order to make it easier to audit the codebase
for correct use of hash functions.
- This breaks CSRF tokens. Any open forms will generate an error when
submitted, so maybe upgrade off-peak.
- We now generate HMAC mail keys but accept MAC or HMAC. In a few months, we
can remove the MAC version.
- The only remaining callsite is Conduit. We can't use HMAC since Arcanist
would need to know the key. {T550} provides a better solution to this, anyway.
Test Plan:
- Verified CSRF tokens generate properly.
- Manually changed CSRF to an incorrect value and got an error.
- Verified mail generates with a new mail hash.
- Verified Phabricator accepts both old and new mail hashes.
- Verified Phabricator rejects bad mail hashes.
- Checked user log, things look OK.
Reviewers: btrahan, jungejason, benmathews
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Maniphest Tasks: T547
Differential Revision: 1237
