Summary:
Ref T12509.
- Upgrade an old SHA1 to SHA256.
- Replace an old manually configurable HMAC key with an automatically generated one.
This is generally both simpler (less configuration) and more secure (you now get a unique value automatically).
This causes a one-time compatibility break that invalidates old "Reply-To" addresses. I'll note this in the changelog.
If you leaked a bunch of addresses, you could force a change here by mucking around with `phabricator_auth.auth_hmackey`, but AFAIK no one has ever used this value to react to any sort of security issue.
(I'll note the possibility that we might want to provide/document this "manually force HMAC keys to regenerate" stuff some day in T6994.)
Test Plan: Grepped for removed config. I'll vet this pathway more heavily in upcoming changes.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D19945
Summary:
Ref T920. This simplifies mail configuration.
The "metamta.domain" option is only used to generate Thread-ID values, and we just need something that looks like a bit like a domain in order to make GMail happy. Just use the install domain. In most cases, this is almost certainly the configured value anyway. In some cases, this may cause a one-time threading break for existing threads; I'll call this out in the changelog.
The "metamta.placeholder-to-recipient" is used to put some null value in "To:" when a mail only has CCs. This is so that if you write a local client mail rule like "when I'm in CC, burn the message in a fire" it works even if all the "to" addresses have elected not to receive the mail. Instead: just send it to an unlikely address at our own domain.
I'll add some additional handling for the possiblity that we may receive this email ourselves in the next change, but it overlaps with T7477.
Test Plan: Grepped for these configuration values.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D19942
Summary:
Ref T920. About a year ago (in 2018 Week 6, see D19003) we moved from individually configured mailers to `cluster.mailers`, primarily to support fallback across multiple mail providers.
Since this has been stable for quite a while, drop support for the older options.
Test Plan: Grepped for all removed options.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D19940
Summary: Suppress an unhelpful Almanac transaction and document the location of the secret clustering management capability. I thought maybe implementing `shouldHide` and checking for `isCreate` would work, but the binding apparently gets created before an interface is bound to it.
Test Plan: Looked at a fresh binding and didn't see "Unknown Object(??)", ran bin/diviner and saw expected output.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19917
Summary: Ref T13222. See PHI992. If you lose an entire cluster, you may want to aggressively demote it out of existence. You currently need to `xargs` your way through this. Allow `--demote <service>`, which demotes all devices in a service.
Test Plan: Demoted with `--demote <device>` and `--demote <service>`. Hit the `--promote service` error.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19850
Summary:
Depends on D19831. Ref T13216. See PHI908. Allegedly, a user copied a large repository into itself and then pushed it. Great backup strategy, but it can create headaches for administrators.
Allow a "maximum paths you can touch with one commit" limit to be configured, to make it harder for users to make this push this kind of commit by accident.
If you actually intended to do this, you can work around this by breaking your commit into pieces (or temporarily removing the limit). This isn't a security/policy sort of option, it's just a guard against silly mistakes.
Test Plan: Set limit to 2, tried to push 3 files, got rejected. Raised limit, pushed changes successfully.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19839
Summary: Depends on D19830. Ref T13216. See PHI908. See PHI750. See PHI885. Allow users to configure a filesize limit, and allow them to adjust the clone/fetch timeout.
Test Plan:
{F6021356}
- Configured a filesize limit and pushed, hit it. Made the limit larger and pushed, change went through.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19831
Summary:
Ref T13216. See PHI943. If autoscale lightning strikes all your servers at once and destroys them, the path to recovery can be unclear. You're "supposed" to:
- demote all the devices;
- disable the bindings;
- bind the new servers;
- put whatever working copies you can scrape up back on disk;
- promote one of the new servers.
However, the documentation is a bit misleading (it was sort of written with "you lost one or two devices" in mind, not "you lost every device") and demote-before-disable is unnecessary and slightly risky if servers come back online. There's also a missing guardrail before the promote step which lets you accidentally skip the demotion step and end up in a confusing state. Instead:
- Add a guard rail: when you try to promote a new server, warn if inactive devices still have versions and tell the user to demote them.
- Allow demotion of inactive devices: the order "disable, demote" is safer and more intuitive than "demote, disable" and there's no reason to require the unintuitive order.
- Make the "cluster already has leaders" message more clear.
- Make the documentation more clear.
Test Plan:
- Bound a repository to two devices.
- Wrote to A to make it a leader, then disabled it (simulating a lightning strike).
- Tried to promote B. Got a new, useful error ("demote A first").
- Demoted A (before: error about demoting inactive devices; now: works fine).
- Promoted B. This worked.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19793
Summary: Depends on D19668. Ref T13197. See PHI840. This updates the documentation to describe how drafts work in more detail.
Test Plan: Read documentation.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19669
Summary:
See PHI785. Ref T13164. In this case, an install wants to receive mail via Mailgun, but not configure it (DKIM + SPF) for outbound mail.
Allow individual mailers to be marked as not supporting inbound or outbound mail.
Test Plan:
- Added and ran unit tests.
- Went through some mail pathways locally, but I don't have every inbound/outbound configured so this isn't totally conclusive.
- Hit `bin/mail send-test` with a no-outbound mailer.
- I'll hold this until after the release cut so it can soak on `secure` for a bit.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19546
Summary:
Depends on D19529. See PHI778.
- Document the "name" constraint as deprecated. All callers are likely better served by the "query" constraint.
- Guide users toward the "query" constraint a little better.
- Document the `=` syntax.
Test Plan: Read various new documentation.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19531
Summary:
Depends on D19509. Ref T13151. See PHI725. `transaction.search` supports "constraints" and the documentation mentions it in passing, but doesn't really show how to do it, and the method is not automatically self-documenting because it isn't a "real" `*.search` method.
Add an example of how to use the `contraints` parameter to retrieve information about only the relevant transactions.
Also remove a refernece to `requestb.in`, which is now defunct.
Test Plan: Called `transaction.search` with similar parameters.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13151
Differential Revision: https://secure.phabricator.com/D19510
Summary:
See PHI251. Ref T13137.
- Replace the perplexing text box with a checkbox that explains what it does.
- Mention this feature in the documentation.
Test Plan:
- Clicked/unclicked checkbox.
- Read documentation.
- Used an existing checkbox control in Slowvote to make sure I didn't break it.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13137
Differential Revision: https://secure.phabricator.com/D19433
Summary: This word isn't the right word.
Test Plan: Reading?
Reviewers: avivey, amckinley
Reviewed By: avivey
Differential Revision: https://secure.phabricator.com/D19404
Summary:
Depends on D19356. Fixes T10883. Ref T13120.
- Add a "writable" property to the bindings, defaulting to "true" with a nice dropdown.
- When selecting hosts, allow callers to request a writable host.
- If the caller wants a writable host, only return hosts if they're writable.
- In SVN and Mercurial, we sometimes return only writable hosts when we //could// return read-only hosts, but figuring out if these request are read-only or read-write is currently tricky. Since these repositories can't really cluster yet, this shouldn't matter too much today.
Test Plan:
- Without any config changes, viewed repositories via web UI and pushed/pulled via SSH and HTTP.
- Made all nodes in the cluster read-only by disabling "writable", pulled and hit the web UI (worked), tried to push via SSH and HTTP (got errors about read-only).
- Put everything back, pulled and pushed.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13120, T10883
Differential Revision: https://secure.phabricator.com/D19357
Summary:
Depends on D19296. Ref T13110.
- Remove the "Large Changesets" documentation since we now degrade very large changesets and I don't have any evidence that anyone has ever tried to follow any of the recommendations in this document.
- Remove references to it.
- When an older revision doesn't have denormalized size information on the Revision object itself, don't render a scale element (instead of rendering a bogus one).
- Try to improve terminology consistency around "Large Change" (100-1000 files) vs "Very Large Change" (1000+ files) vs "Enormous Change" (too large to hold in memory).
Test Plan: Viewed revisions; grepped for documentation.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19298
Summary:
The current link has a redirect for a while now, from
http://fortawesome.github.io/Font-Awesome/ to https://fontawesome.com
However, since the release of Version 5, the docs no longer
match the icons that are valid for use in Phabricator, which
uses Version 4.
Update the reference to link to the same logical content as before.
Test Plan: The content now lives at <https://fontawesome.com/v4.7.0/icons/>.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D19241
Summary: Ref T13069. See PHI54. Some of this behavior isn't entirely obvious, so give users a heads up in the documentation to help warn them about what is to come.
Test Plan: Read documentation.
Maniphest Tasks: T13069
Differential Revision: https://secure.phabricator.com/D19227
Summary: Support pacts have been working well and are here to stay, so guide users toward them rather than older resources (consulting / paid prioritization).
Test Plan: Read document. Twice!
Differential Revision: https://secure.phabricator.com/D19219
Summary:
Fixes T12994. We need `MYSQLI_ASYNC` to implement client-side query timeouts, and we need MySQLi + MySQL Native Driver to get `MYSQLI_ASYNC`.
Recommend users install MySQLi and MySQL Native Driver if they don't have them. These are generally the defaults and best practice anyway, but Ubuntu makes it easy to use the older stuff.
All the cases we're currently aware of stem from `apt-get install php5-mysql` (which explicitly selects the non-native driver) so issue particular guidance about `php5-mysqlnd`.
Test Plan:
- Faked both issues locally, reviewed the text.
- Will deploy to `secure`, which currently has the non-native driver.
Maniphest Tasks: T12994
Differential Revision: https://secure.phabricator.com/D19216
Summary: Ref T13099. See PHI424. Fixes T11664. Several installs are interested in having these behaviors available in Owners by default and they aren't difficult to provide, it just makes the UI kind of messy. But I think there's enough general interest to justify it, now.
Test Plan: Created a package which owns "/" with a "With Non-Owner Author" review rule which I own. Created a revision, no package reviewer. Changed rule to "All", updated revision, got package reviewer.
Maniphest Tasks: T13099, T11664
Differential Revision: https://secure.phabricator.com/D19180
Summary: Depends on D19049. Ref T11330. Adds some documentation for webhooks.
Test Plan: Read the documentation and found it to be exceptionally accurate and helpful.
Maniphest Tasks: T11330
Differential Revision: https://secure.phabricator.com/D19050
Summary: Fixes T10189. Ref T13053. We haven't sent these headers in a very long time. Briefly mention the new stamps header instead, although I expect to integrate stamp documentation into the UI in a more cohesive way in the future.
Test Plan: Read documentation.
Maniphest Tasks: T13053, T10189
Differential Revision: https://secure.phabricator.com/D19030
Summary:
Ref T13053. Postmark support recommends testing requests against a whitelist of known remote addresses to determine request authenticity. Today, the list can be found here:
<https://postmarkapp.com/support/article/800-ips-for-firewalls>
This is potentially less robust than, e.g., HMAC verification, since they may need to add new datacenters or support IPv6 or something. Users might also have weird network topologies where everything is proxied, and this makes testing/simulating more difficult.
Allow users to configure the list so that they don't need to hack things apart if Postmark adds a new datacenter or remote addresses are unreliable for some other reason, but ship with safe defaults for today.
Test Plan:
Tried to make local requests, got kicked out. Added `0.0.0.0/0` to the list, stopped getting kicked out.
I don't have a convenient way to route real Postmark traffic to my development laptop with an authentic remote address so I haven't verified that the published remote address is legitimate, but I'll vet that in production when I go through all the other mailers.
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D19025
Summary: Depends on D19016. Ref T13053. Adds a listener for the Postmark webhook.
Test Plan:
Processed some test mail locally, at least:
{F5416053}
Reviewers: amckinley
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D19017
Summary: Depends on D19007. Ref T12677.
Test Plan: Used `bin/mail send-test ... --mailer postmark` to deliver some mail via Postmark.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12677
Differential Revision: https://secure.phabricator.com/D19009
Summary:
Depends on D19003. Ref T12677. Ref T13053. For the first time, we're requiring CLI configuration of a complex value (not just a string, integer, bool, etc) to do something fairly standard (send mail).
Users sometimes have very reasonable difficulty figuring out how to `./bin/config set key <some big JSON mess>`. Provide an easy way to handle this and make sure it gets appropriate callouts in the documentation.
(Also, hide the `cluster.mailers` value rather than just locking it, since it may have API keys or SMTP passwords.)
Test Plan: Read documentation, used old and new flags to set configuration.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13053, T12677
Differential Revision: https://secure.phabricator.com/D19004
Summary:
Depends on D19002. Ref T13053. Ref T12677. Adds a new option to allow configuration of multiple mailers.
Nothing actually uses this yet.
Test Plan: Tried to set it to various bad values, got reasonable error messages. Read documentation.
Reviewers: amckinley
Maniphest Tasks: T13053, T12677
Differential Revision: https://secure.phabricator.com/D19003
Summary: Depends on D18930. Ref T13048. Try to explain what this does and give an example since I think it's probably not very obvious from the name.
Test Plan: Read the text.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13048
Differential Revision: https://secure.phabricator.com/D18931
Summary: Depends on D18910. Ref T13043. Provides reasonable user-facing documentation about the general role and utility of this tool.
Test Plan: Read document.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18911
Summary:
See PHI234. Several issues here:
- The warning about observing a repository in Read/Write mode checks the raw I/O type, not the effective I/O type. That means we can fail to warn if other URIs are set to "Default", and "Default" is "Read/Write" in practice.
- There's just an actual typo which prevents the "Observe" version of this error from triggering properly.
Additionally, add more forceful warnings that "Observe" and "Mirror" mean that you want to //replace// a repository with another one, not that we somehow merge branches selectively. It isn't necessarily obvious that "Observe" doesn't mean "merge/union", since the reasons it can't in the general case are somewhat subtle (conflicts between refs with the same names, detecting ref deletion).
Test Plan:
Read documentation. Hit the error locally by trying to "Observe" while in Read/Write mode:
{F5302655}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18810
Summary: Most of this document is no longer relevant, since we're happy to work on prototypes if you're paying us and no longer have any meaningful free support.
Test Plan: Read document.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18719
Summary: Noticed a couple of typos in the docs, and then things got out of hand.
Test Plan:
- Stared at the words until my eyes watered and the letters began to swim on the screen.
- Consulted a dictionary.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D18693
Summary:
Ref T13000. This marks each table as either "data" (normal data), "cache" (automatically rebuilt, no need to ever dump) or "index" (can be manually rebuilt).
By default, `bin/storage dump` dumps data and index tables, but not cache tables.
With `--no-indexes`, it dumps only data tables. Indexes can be rebuilt after a restore with `bin/search index --all ...`.
Test Plan:
- Ran `--no-indexes` and normal dumps with `--trace`, verified that cache and index (former case) or cache only (latter case) tables were dumped with `--no-data`.
- Verified dump has the same number of `CREATE TABLE` statements as before the changes.
- Reviewed persistence tags in the web UI (note Ferret engine tables are "Index"):
{F5210886}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13000
Differential Revision: https://secure.phabricator.com/D18682
Summary: Ref T12819. Adds some documentation for `-term`, `~term`, `title:term`, etc.
Test Plan: Read documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18592
Summary: From Z1336, we don't currently document anywhere how the default dashboard works. I should also update the copy in the UI. Ref T12969
Test Plan: regenerate docs, read carefully
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Maniphest Tasks: T12969
Differential Revision: https://secure.phabricator.com/D18454
Summary: I wrote "free" since "test" only give us 7 days to confirm the issue, but "free" no longer exists and "test" should be good enough.
Test Plan: o_O
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18423
Summary: Rewords the document to note new location and status table.
Test Plan: Read, reread.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18387
Summary:
Ref T12922.
- Tell customers where to go at the top.
- Fix a couple minor things (e.g., don't advise users to reproduce on `secure` anymore).
Test Plan: Read carefully.
Reviewers: chad, avivey
Reviewed By: chad, avivey
Maniphest Tasks: T12922
Differential Revision: https://secure.phabricator.com/D18236
Summary:
Ref T12922.
- Remove most mentions to "Contributing Feature Requests".
- Raise the barrier to entry on code contributions.
I'm going to tweak "Bug Reports" in a followup to be more similar to "Feature Requests", but that's a slightly more involved change.
Test Plan: Read new docs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12922
Differential Revision: https://secure.phabricator.com/D18235
Summary: Fixes T12926. This exists but isn't documented. Document it after the section about webserver setup, since that's probably when you'd want to set it up.
Test Plan: Read carefully, visited `/status/`.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12926
Differential Revision: https://secure.phabricator.com/D18234
Summary: This updates the support document, specifically, scopes down feature requests, updates community links, and other wordsmithing. Unsure where to direct bug reports right now, but we'll have something soon?
Test Plan: Read carefully
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Luke081515.2, Korvin
Differential Revision: https://secure.phabricator.com/D18218
Summary:
Depending on how you perform a restore, APC (or, e.g., running daemon processes) might be poisoned with out-of-date caches.
Add a note to advise installs to restart after restoring data.
See also lengthy fishing expedition support thread.
Test Plan: Read the text.
Reviewers: chad, amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18180
Summary:
Ref T12612. This updates the rate limiting code to:
- Support a customizable token, like the client's X-Forwarded-For address, rather than always using `REMOTE_ADDR`.
- Support APCu.
- Report a little more rate limiting information.
- Not reference nonexistent documentation (removed in D16403).
I'm planning to put this into production on `secure` for now and then we can deploy it more broadly if things work well.
Test Plan:
- Enabled it locally, used `ab -n 100` to hit the limit, saw the limit enforced.
- Waited a while, was allowed to browse again.
Reviewers: chad, amckinley
Reviewed By: amckinley
Maniphest Tasks: T12612
Differential Revision: https://secure.phabricator.com/D17758
