Yeah, should be restarted with my custom dumb script:

Ah I see, did you manually go to /diffusion/create/ or did we miss a link somewhere?

Did you restart phabricator?

I think this isn't really too bad. The major issue is that we can't use stylesheets in email because Gmail doesn't support them, so we need to emit markup with inline style attributes.

This is now live in HEAD of master, and deployed to secure. Here's phabricator/ on this install:

I'm going to call this resolved now -- there are still substantial rough edges with this and the rest of T10748, but we're running on the new code and everything else should mostly be UI stuff. T10922 has general guidance and I'll open an errata task to track cleanup shortly.

D15837 appears to have deployed cleanly here.

(☞ﾟヮﾟ)☞ Prioritized ☜(ﾟヮﾟ☜)

Basically this is a "yes, but first let's build glorious INFRASTRUCTURE", as most things tend to be on this project.

And I think the mail notifications are also largely reasonable, I just don't want to hack this one mail notification in and call it a day since this I think this should be approached more broadly (covering all high-security actions, not just SSH) and deeply (SMS, mail, transactions).

My only idea would be converting actions surrounding SSH key management to transactions, which you're not explicitly stating here, but if we were to do that we would be able to use internal infa we have that reacts to transactions, and send email when a key is added.

We haven't yet enabled, but will be enabling MFA this week.

The best way to describe this request from our perspective is "third party requirement", which falls under your definition of a policy compliance change, mostly.

The concern here is Arcanist already solves this problem, and if you really can't use arcanist, then T5000 solves this problem. Beyond that, this feels like a band-aid and not a proper fix to the issue. There should be automatic, programmatic means of doing this. Having users attach after the fact still relies on them to do this step manually, which is unlikely to be 100% a success.

arcanist is not a practical tool for some of us (we use hg and patch queues), so we do the annotation manually. Is arcanist considered a must-use for phabricator?

Why are users manually adding this information. It's automatic if you use Arcanist/Differential.

I don't understand...there's no requirement per se for a commit to have a diff attached so the commits come through with no diff attached. Usually this happens in error because the use doesn't enter the diff id correctly or misspells "Differential Revision" or complete forget to add it in, etc. So the diff is there and was properly reviewed but the association hasn't been made.

How are you getting commits into the system that have been reviewed but don't have a revision in the commit message. Manually adding these is functionally a step backwards

Some things we can do easily here:

In T10920#173847, @eadler wrote:

2. It would help if you could articulate the "benefit in having commits and diffs cross referenced outside of just autoclosing the diff". See https://secure.phabricator.com/book/phabcontrib/article/describing_problems/

While I agree it would be nice to have some control over this in the UI here are two comments:

1. This can be done via the CLI: ./bin/differential attach-commit
2. It would help if you could articulate the "benefit in having commits and diffs cross referenced outside of just autoclosing the diff". See https://secure.phabricator.com/book/phabcontrib/article/describing_problems/

I've run into this problem with my puppet repositories. A common puppet workflow is to use every branch as a puppet environment.
This way you can test a large change on a subset of production servers before applying it to the production branch itself.

Although it's not a default setting,

proxy_set_header X-Forwarded-Proto $scheme;

is a common configuration option when using nginx to terminate SSL.

In particular, for anyone hitting this, is your use case anything other than something in the vein of "my users publish temporary/development changes to branches in the remote"? If we implement GitHub and let them dump their garbage into personal forks which are isolated from a Herald perspective, would that resolve the issue?

I suppose we can trust X-Forwarded-Proto for the purposes of raising a setup issue, although I'm not sure if that would help here, since I think the header usually won't be set if you use nginx to terminate SSL instead of, say, an ELB.

@epriestley you nailed it.

What about my suggestion regarding evaluating commit newness on a per-herald rule basis, based on which refs would match the herald rule?

We're extremely unlikely to pursue any changes where 99% of the time it works great and 1% of the time a user shows up here wanting to kill us, so arguments in the vein of "it would work for me and other reasonable users who know what they're doing", while likely true, has very weak persuasive power.

You can see which fields are copied in Edit Task → Configure Form → Using HTTP Parameters and scrolling down to "Template Objects". On this install, these fields are currently copied:

A branch or tag can be moved from a very old commit to a new commit, and then re-run rules on a large amount of repository history.

I suspect you may not be setting $_SERVER['HTTPS'] so Phabricator believes you are connecting over HTTP. It looks for a client using the http protocol, fails to find one (only https is available), and does not emit a websocket setup instruction.

A branch or tag can be moved from a very old commit to a new commit, and then re-run rules on a large amount of repository history.

Broadly, there are a class of "security" changes which are really policy compliance changes. For example, it's common for large enterprises to have a password requirement like this:

• I git pulled the 3 phabricator repos from the stable branch.

@cspeckmim Right, that was an edge case indeed, but even very active tasks can rapidly pick up a huge number of subscribers/tokens/etc (something I'm seeing on my install). I'd personally aim for a collapsed-but-expandable '[+] 16 users subscribed, 7 tokens awarded', but this is definitely getting deep into design territory.

I don't think you need to do anything new for new refs.

Can you give precise reproduction steps, please? Specifically:

Do you have concrete examples? I believe we only pull over subscribers and tags.

In T6522#173738, @reardencode wrote:

for (commit in commits_between(ref.old_commit, ref.new_commit)) {

Well the custom fields were probably poor examples...

I've generally been hesitant about making Phabricator send these emails because I'm not sure how valuable they are in practice, and a lot of users hate email (see T9161). If an attacker gains access to an instance even briefly, I think it's likely much of the damage is done before most users will be able to respond (in particular, a targeted attack can virtually guarantee you receive the email at a time when you are likely to be asleep). By its nature, this mitigation is a reactive one with a potentially long reaction time.

Thank you for the quick fix! As for the fate of the misplaced tasks, don't worry, I'll deal with them.

Please open a new test instance on phacility for playing around. This is a real, live, production instance.

http://googlе.com

How about:

Grmbl. I cannot indeed reproduce it neither with your example, I'll investigate more what's wrong with my (other) setup, that still doesn't work. I'll reopen if/when I find out what's happening. Sorry for the noise.

Per F1256292, presuming resolved until more details surface about what's up.

Does copy not cover this in field definition?

Grmbl. I cannot indeed reproduce it neither with your example, I'll investigate more what's wrong with my (other) setup, that still doesn't work. Sorry about the hassle, feel free to close this issue in the meantime.

You can set a global default in...

At least the default seems better, since they can change it in cases where it's not correct.