While it's on my mind -- these messages link to /mail/whatever/123/ right now, but that page doesn't hyperlink links in the mail body, which is a fairly significant usability issue since the workflow is: click the link to go to the mail, read the mail body, copy paste the link in the mail body into your address bar.

One way to cheat on this is just to render the body as remarkup. This is wrong: we've already rendered it as remarkup to a text rendering target, and we'd be re-rendering it. However, it will basically work: link links, turn Dxxx into an object reference, etc., and probably not break too much stuff, so it's likely an upgrade over no rendering.

A less cheaty way would be to preserve the original mail body for much longer and do the remarkup rendering at the last moment, before sending the mail. Then, re-render in the web view from the raw remarkup source. But this becomes very tricky in the general case because some mail contains complex HTML elements (like inline comments) which would also need to be stored in a structured way and then rendered just-in-time. Not insurmountable but a fairly hefty chunk of work to improve a very niche workflow.

A middle-of-the-road approach is to implement a "mail-client-like renderer" which mostly just links up hyperlinks. This would fix any bad results from re-rendering remarkup, but we necessarily end up with just an okay result, similar to viewing the mail message in any third-party client. We also have to duplicate at least a little bit of hyperlink rendering code, which has a lot of tricky security behavior (e.g., see D18873, recently).

(It's possible this "mail-client-like renderer" can just be a remarkup renderer with few active rules; see also T3278.)

Oh, great: ⛄ and ☃ are totally different emoji. Great.

An alternate attack on this is to preserve the text and link for the existing "View Revision" button as metadata (or just use the object PHID, which I think we already preserve) and re-render them into the View page. I'll likely do this in the short term so this isn't quite as aggressively bad as it is otherwise.

contains complex HTML elements (like inline comments) which would also need to be stored in a structured way and then rendered just-in-time

We could also store HTML in the database and trust it blindly, of course, but I'm wildly uneasy about this from a security viewpoint. We technically do this already, but only in very narrow cases in pure readthrough caches (remarkup cache, changeset parse cache).

🐱 🐟

Vaguely related, the subject line for this mail is currently something like:

[Application] Object Updated

Normally, it would be something like:

[Application] [Closed] Txxx: blah blah

Obviously, we have to hide the title. I think we're also correct to hide the action (like [Closed]) since it will sometimes leak information (consider learning that three [Raised Priority] actions have been applied to a security task -- this is a good hint that there's something really severe in the wild). But showing the application name is fine and doesn't disclose anything.

For most objects, we could also safely show the monogram, which would make this mail a little more useful without disclosing anything. However, I think we need to adjust applications a little bit to pass it into the mail.

Hrrrm...

In T13053#235208, @epriestley wrote:

Hrrrm...

Good thing I got that top-secret message over a secure channel. Who knows what the Bad People would have one if they'd intercepted it!

When an object is updated via an "inverse edge" change (normally: when it is mentioned, or when another object is linked to it) we currently do not fire Herald rules. The corresponding block in the code has this comment:

// If we're applying inverse edge transactions, don't trigger Herald.
// From a product perspective, the current set of inverse edges (most
// often, mentions) aren't things users would expect to trigger Herald.
// From a technical perspective, objects loaded by the inverse editor may
// not have enough data to execute rules. At least for now, just stop
// Herald from executing when applying inverse edges.

With the "must encrypt" flag, this means that the "require encryption" rule never fires, so the resulting mail is sent in cleartext. This discloses the name and recipient list of the object, and additional details after the D18995 ("mail stamps") series of changes.

This pushes me toward considering the stateful model for this flag rather than the transient "if Herald says so, that mail gets flagged" model, since I think it may be an easier way forward than trying to sort out the inverse edge / Herald stuff and it's generally a sturdier model, I just didn't want to build it out if we didn't really need it for anything.

Who knows what the Bad People would have one if they'd intercepted it!

I do like how much intrigue this feature creates.

The "From" address should also probably be anonymized for flagged mail.

See PHI178 and PHI126. See T12689. When users resign from a revision, they should not receive mail about it even if they're a member of a non-resigned package or project. This requires propagating mail recipients deeper into the stack, and retaining richer recipient information. Subscriptions should be upgraded to support a stronger "unsubscribe" which can get you out of indirect subscriptions via projects/packages/etc. These probably mostly go down the same pathway.

For the "Resigned" stuff, I think the actual implementation is that we build a list of PHIDs which targets are not permitted to expand into. So if you've resigned from a revision, projects and packages can't expand into your user PHID during target resolution, regardless of why they're targets. For example, if Alice resigns but #alice-fan-club is still a subscriber, her "Resign" should be stronger than that and still prevent expansion.

However, you should still be able to subscribe directly, and if you're in the natural target list you still get mail. If you resign and then "Subscribe" again (as an individual user) you'd reasonably expect to get mail.

For "Mute", we could try to use the UNSUBSCRIBE edge to build the "no expansion" list. However, this runs into dangerous territory with sequences of actions like this:

• You're added as a subscriber by a mention.
• You unsubscribe.
• A project you're a member of is added as a reviewer.

In this case, we would not send you mail. This is probably reasonable and maybe even expected, but it's not obvious ("Why am I not getting mail?" does not lead to "Oh, because I unsubscribed 3 weeks ago." easily).

I think the only downside to a separate "Mute" action is that we end up with another action in the action list, but we can do something like this:

• Ship "Mute".
• Check if anyone's using it in 6-or-whatever months.
• Remove it or turn it into a keystroke if there's little-to-no use.

So this is a separate list which just removes targets after expansion, and essentially identical to the existing getExcludeMailRecipientPHIDs() mechanism except that the delivery/routing reason should be "muted" instead of "This message is a response to another email message...".

(Testing "secure" flag in webhooks. 🐑)