Summary: Ref T13658.
Test Plan: Static checks only, these are all reachable only by breaking your install in weird ways.
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21781
Summary: Ref T13658.
Test Plan: Static checks only, these are mostly obscure or require breaking the install in unusual ways.
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21780
Summary: Ref T13658.
Test Plan:
This is non-exhaustive.
- Looked at some (most?) of the config values in Config.
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21768
Summary: Ref T13658.
Test Plan:
This test plan is non-exhaustive.
- Used Auth to start an "another copy of Phabricator" OAuth server workflow.
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21766
Summary:
Refer to discussion on D21677#275541
Refs D21681 (arcanist changes)
Phabricator has several uses of the `--debug` flag being used with Mercurial. Use of this flag causes additional output to be added which Phabricator needs, however the behavior of `--debug` is not guaranteed to be stable, and in newer versions of Mercurial there have been additional output that has caused Phabricator to choke on parsing the output. This change removes several uses of `--debug` in favor of using `--template` with the `hg log` or `hg annotate` commands in combination with the `{p1.node}` or `{p2.node}` template format.
The use of `{p1node}` format in templates was added in [[ https://www.mercurial-scm.org/wiki/WhatsNew/Archive#Mercurial_2.4_.282012-11-01.29 | Mercurial 2.4 (2012) ]]. This format was deprecated in [[ https://www.mercurial-scm.org/wiki/WhatsNew#Mercurial_4.9_.282019-02-01.29 | Mercurial 4.9 (2019) ]] in favor of using `{p1.node}` format which is unclear when this new format was added (presumably earlier than Mercurial 4.9).
The use of `--template` with `hg annotate` is only officially supported in [[ https://www.mercurial-scm.org/wiki/Release4.6 | Mercurial 4.6 (2018) ]], though does appear to work in 4.5 but is not documented.
Since the `{p1node}` format was introduced in 2.4 this bumps the required version of `hg` to 2.4 (from 1.9). Since the `annotate --template` feature wasn't added until 4.6 (which is still fairly recent), the use of it is gated on a capability test, but still preferred for use where possible to avoid extraneous output from `--debug` flag.
Test Plan:
I verified I could do the following in a mercurial repository, while having mercurial 5.8 installed:
1. Navigate and view files in Diffusion under e.g. `/source/test-repo/`.
2. While viewing a file in Diffusion verified that I could view the blame of the file and the history/annotations looked accurate for the files I was browsing.
3. From the blame sidebar, select to view a commit which loaded and displayed changes properly.
4. View the history of the repository under e.g. `/source/test-repo/history/default/`. I verified the history looked correct and the tree-like structure showing relationship of commits also looked accurate.
I setup mercurial to run version 4.4, created a new repository, added some commits, and verified all the above behavior still works properly.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D21679
Summary: Found a few typos which could be updated.
Test Plan:
I tested the Configuration page change by navigating to `/config` and verifying the page title set in the browser as well as the page title text on the page
|Before|After|
|---|---|
|{F9013208}|{F9013210}|
|{F9013300}|{F9013301}|
I verified the Conduit error message by navigating to `/auth/start/?__conduit__=1`
{F9013289}
The CircleCI error message was not verified due to the involvement of testing with CircleCI however the change is very minor and has very little risk of impacting any functionality.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D21675
Summary:
Ref T9764. These "star" icons are unclear, inconsistent, and not friendly to colorblind users.
They date from a time long ago when the product didn't have icons.
Modernize them and make them more consistent with the similar statuses in Harbormaster.
Test Plan:
{F8545690}
{F8545691}
{F8545692}
Maniphest Tasks: T9764
Differential Revision: https://secure.phabricator.com/D21639
Summary: Ref T13624. Depends on D21578. In "sshd" subprocess contexts, use "PhutilErrorLog" to direct errors to both stderr and, if configured, a logfile on disk.
Test Plan:
- Confiugured an error log.
- Forced `ssh-auth` to fatal.
- Saw errors on stderr and in log.
Maniphest Tasks: T13624
Differential Revision: https://secure.phabricator.com/D21579
Summary: Ref T13575. Particularly with the new Apple silicon, I think there are enough domain collisions for `M1`, `M2`, `P1`, etc., to justify adding them to the default ignore list.
Test Plan: Created a mock, then wrote a comment referencing an object on the list (`M1`) and an object not on the list (`T1`). Got text and a link respectively.
Maniphest Tasks: T13575
Differential Revision: https://secure.phabricator.com/D21507
Summary:
Ref T13556. These options are very old and effectively obsoleted by "bin/phd debug [--trace]". I haven't used either option diagnostically in many years, and they aren't mentioned in the documentation.
Remove them to simplify configuration, and because "phd.trace" doesn't work anyway and likely hasn't for a long time -- it has specific issues with TTY detection (see T13556).
Test Plan: Grepped for "phd.trace" and "phd.verbose". Ran "bin/phd debug [--trace]" and saw verbose/trace output.
Maniphest Tasks: T13556
Differential Revision: https://secure.phabricator.com/D21426
Summary:
Fixes T13536. See that task for discussion.
Older versions of MySQL (roughly, prior to 8.0.19) emit "int(10)" types. Newer versions emit "int" types. Accept these as equivalent.
Test Plan: Ran `bin/storage upgrade --force` against MySQL 8.0.11 and 8.0.20. Got clean adjustment lists on both versions.
Maniphest Tasks: T13536
Differential Revision: https://secure.phabricator.com/D21265
Summary: This supports the IntelliJ IDEA editor.
Test Plan:
- Looked at the editor settings panel, saw "idea://".
- Set my editor pattern to "idea://a?b".
- (Did not actually install IntelliJ IDEA.)
Differential Revision: https://secure.phabricator.com/D21206
Summary:
See <https://hackerone.com/reports/850114>.
An attacker with administrator privileges can configure "notification.servers" to connect to internal services, either directly or with chosen parameters by selecting an attacker-controlled service and having it issue a "Location" redirect.
Generally, we allow this attack to occur. The same administrator can use an authentication provider or a VCS repository to perform the same attack, and we can't reasonably harden these workflows without breaking things that users expect to be able to do.
There's no reason this particular variation of the attack needs to be allowable, though, and the current behavior isn't consistent with how other similar things work.
- Hide the "notification.servers" configuration, which also locks it. This is similar to other modern service/server configuration.
- Don't follow redirects on these requests. Aphlict should never issue a "Location" header, so if we encounter one something is misconfigured. Declining to follow this header likely makes the issue easier to debug.
Test Plan:
- Viewed configuration in web UI.
- Configured a server that "Location: ..." redirects, got a followed redirect before and a failure afterward.
{F7365973}
Differential Revision: https://secure.phabricator.com/D21123
Summary:
Ref T13507. Now that we handle processing of "Content-Encoding: gzip" headers by default, this setup check can get a decompressed body back. Since it specifically wants a raw body back, disable this behavior.
Also, "@" a couple things which can get in the way if they fail now that error handling is more aggressive about throwing on warnings.
Test Plan: Ran setup check after other changes in T13507, got clean result.
Maniphest Tasks: T13507
Differential Revision: https://secure.phabricator.com/D21122
Summary: Ref T13507. See that task for discussion.
Test Plan: Faked different response behaviors and hit both variations of this error.
Maniphest Tasks: T13507
Differential Revision: https://secure.phabricator.com/D21116
Summary:
See <https://discourse.phabricator-community.org/t/upgrade-from-sep-30-2016/3702/>. A user performing an upgrade from 2016 to 2020 ran into an issue where this setup query is overheating.
This is likely caused by too many rows changing state during query execution, but the particulars aren't important since this setup check isn't too critical and will catch the issue eventually. It's fine to just move on if this query fails for any reason.
Test Plan: Forced the query to overheat, loaded setup issues, got overheating fatal. Applied patch, no more fatal.
Differential Revision: https://secure.phabricator.com/D21057
Summary:
See PHI1647, which asks for "vscode://" to be a configurable protocol on hosted Phacility instances.
I made the configuration editable in D21008, but this can reasonably just come upstream too.
Test Plan: Viewed config in Config, set my editor URI to `vscode://blahblah`.
Differential Revision: https://secure.phabricator.com/D21009
Summary:
Ref T13395. No library with this name loads any more, so we can't version check it.
(Ideally, the version check stuff would be more graceful when it fails now, since it's required to load "Config" after I moved it off a separate page.)
Test Plan: Loaded "Config".
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D20995
Summary:
Depends on D20933. Ref T13362. This reorganizes Config a bit and attempts to simplify it.
Subsections are now in a landing page console and groupings have been removed. We "only" have 75 values you can edit from the web UI nowadays, which is still a lot, but less overwhelming than it was in the past. And the trend is generally downward, as config is removed/simplified or moved into application settings.
This also gets rid of the "gigantic blobs of JSON in the UI".
Test Plan: Browsed all Config sections.
Maniphest Tasks: T13362
Differential Revision: https://secure.phabricator.com/D20934
Summary: Depends on D20930. Ref T13362. Put all the "Services" parts of Config in their own section.
Test Plan: Clicked through each section. This is just an organization / UI change with no significant behavioral impact.
Maniphest Tasks: T13362
Differential Revision: https://secure.phabricator.com/D20931
Summary:
Ref T13362. Config is currently doing a ton of stuff and fairly overwhelming. Separate out "Modules/Extensions" so it can live in its own section.
(This stuff is mostly useful for development and normal users rarely need to end up here.)
Test Plan: Visited seciton, clicked around. This is just a visual change.
Maniphest Tasks: T13362
Differential Revision: https://secure.phabricator.com/D20930
Summary:
Ref T13362. Some applications moved to fixed-width a while ago but I was generally unsatisfied with where they ended up and have been pushing them back to full-width.
Push Config back to full-width. Some of the subpages end up a little weird, but this provides more space to work with to make some improvements, like makign `maniphest.statuses` more legible in the UI>
Test Plan: Grepped for `setFixed(`, updated each page in `/config/`. Browsed each controller, saw workable full-width UIs.
Maniphest Tasks: T13362
Differential Revision: https://secure.phabricator.com/D20925
Summary:
Ref T13444. Repository identities have, at a minimum, some bugs where they do not update relationships properly after many types of email address changes.
It is currently very difficult to fix this once the damage is done since there's no good way to inspect or rebuild them.
Take some steps toward improving observability and providing repair tools: allow `bin/repository rebuild-identities` to effect more repairs and operate on identities more surgically.
Test Plan: Ran `bin/repository rebuild-identities` with all new flags, saw what looked like reasonable rebuilds occur.
Maniphest Tasks: T13444
Differential Revision: https://secure.phabricator.com/D20911
Summary:
Ref T13404. Except for one known issue in Multimeter, Phabricator appears to function properly in this mode. It is broadly desirable that we run in this mode; it's good on its own, and enabled by default in at least some recent MySQL.
Additionally, "ONLY_FULL_GROUP_BY" and "STRICT_ALL_TABLES" shared a setup key, so ignoring one would ignore both. Change the key so that existing ignores on "ONLY_FULL_GROUP_BY" do not mask "STRICT_ALL_TABLES" warnings.
Test Plan: Grepped for `ONLY_FULL_GROUP_BY`.
Maniphest Tasks: T13404
Differential Revision: https://secure.phabricator.com/D20791
Summary: Ref T13404. Enabling "STRICT_ALL_TABLES" is good, but if you don't want to bother it doesn't matter too much. All upstream development has been on "STRICT_ALL_TABLES" for a long time.
Test Plan: {F6847839}
Maniphest Tasks: T13404
Differential Revision: https://secure.phabricator.com/D20790
Summary:
See D20779, https://discourse.phabricator-community.org/t/3089. `bin/config set` complains about
missing config file as if it's un-writable.
Test Plan: run `bin/config set` with missing, writable, unwritable conf.json and parent dir.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D20788
Summary: Ref T13403. We currently emit a useful error message, but it's not tailored and has a stack trace. Since this is a relatively routine error and on the first-time-setup path, tailor it so it's a bit nicer.
Test Plan:
- Ran `bin/config set ...` with an unwritable "local.json".
- Ran `bin/config set ...` normally.
Maniphest Tasks: T13403
Differential Revision: https://secure.phabricator.com/D20779
Summary:
Fixes T13336.
- Prevent `--no-indexes` from being combined with `--for-replica`, since combining these options can only lead to heartbreak.
- In `--for-replica` mode, dump caches too. See discussion in T13336. It is probably "safe" to not dump these today, but fragile and not correct.
- Mark the "MarkupCache" table as having "Cache" persistence, not "Data" persistence (no need to back it up, since it can be fully regenerated from other datasources).
Test Plan: Ran `bin/storage dump` with various combinations of flags.
Maniphest Tasks: T13336
Differential Revision: https://secure.phabricator.com/D20743
Summary:
Ref T13373. When you "bin/config set x ..." a value, the success message ("Set x ...") is somewhat ambiguous and can be interpreted as "First, you need to set x..." rather than "Success, wrote x...".
Make the messaging more explicit. Also make this string more translatable.
Test Plan: Ran `bin/config set ...` with various combinations of flags, saw more clear messaging.
Maniphest Tasks: T13373
Differential Revision: https://secure.phabricator.com/D20711
Summary:
Fixes T13342. This does a few different things, although all of them seem small enough that I didn't bother splitting it up:
- Support export of "remarkup" custom fields as text. There's some argument here to export them in some kind of structure if the target is JSON, but it's hard for me to really imagine we'll live in a world some day where we really regret just exporting them as text.
- Support export of "date" custom fields as dates. This is easy except that I added `null` support.
- If you built PHP from source without "--enable-zip", as I did, you can hit the TODO in Excel exports about "ZipArchive". Since I had a reproduction case, test for "ZipArchive" and give the user a better error if it's missing.
- Add a setup check for the "zip" extension to try to avoid getting there in the first place. This is normally part of PHP so I believe users generally won't hit it, I just hit it because I built from source. See also T13232.
Test Plan:
- Added a custom "date" field. On tasks A and B, set it to null and some non-null value. Exported both tasks to Excel/JSON/text, saw null and a date, respectively.
- Added a custom "remarkup" field, exported some values, saw the values in Excel.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13342
Differential Revision: https://secure.phabricator.com/D20658
Summary: Ref T13321. The daemons no longer write PID files, so we no longer need to pass any of this stuff to them.
Test Plan: Grepped for affected symbols.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13321
Differential Revision: https://secure.phabricator.com/D20608
Summary: These instructions are fairly old and can be a little fancier and more clear in the context of modern Phabricator. Drop the reference to "HPHP", link the actual timezone list, wordsmith a little.
Test Plan: d( O_o )b
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20560
Summary: Ref T7667. Also one text fix.
Test Plan: Ran `bin/auth lock`, didn't get a replacement setup issue.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T7667
Differential Revision: https://secure.phabricator.com/D20447
Summary:
Ref T7667. On the road to locking the auth config, also clean up some minor UI issues:
* Only show the warning about not Phacility instance auth if the user isn't a manager (see next diff).
* When rendering more than one warning in the guidance, add bullets.
* I didn't like the text in the `auth.config-lock` config setting.
Test Plan: Loaded the page, saw more reasonable-looking guidance: {F6369405}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T7667
Differential Revision: https://secure.phabricator.com/D20400
Summary:
Ref T7667. Adds new flows `bin/auth lock` and `bin/auth unlock` to prevent compromised administrator accounts from doing additional damage by altering the authentication provider configuration.
Note that this currently doesn't actually do anything because we aren't checking this config key in any of the edit controllers yet.
Test Plan: Ran `lock` and `unlock`, checked for correct DB state, observed expected setup warning.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T7667
Differential Revision: https://secure.phabricator.com/D20394
Summary:
Ref T13259. In some configurations, making a request to ourselves may return a VPN/Auth response from some LB/appliance layer.
If this response begins or ends with whitespace, we currently detect it as "extra whitespace" instead of "bad response".
Instead, require that the response be nearly correct (valid JSON with some extra whitespace, instead of literally anything with some extra whitespace) to hit this specialized check. If we don't hit the specialized case, use the generic "mangled" response error, which prints the actual body so you can figure out that it's just your LB/auth thing doing what it's supposed to do.
Test Plan:
- Rigged responses to add extra whitespace, got "Extra Whitespace" (same as before).
- Rigged responses to add extra non-whitespace, got "Mangled Junk" (same as before).
- Rigged responses to add extra whitespace and extra non-whitespace, got "Mangled Junk" with a sample of the document body instead of "Extra Whitespace" (improvement).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13259
Differential Revision: https://secure.phabricator.com/D20235
Summary:
Depends on D20181. Depends on D20182. Fixes T3498. Ref T13161. My claim, at least, is that D20181 can be tweaked to be good enough to throw away this "feature" completely.
I think this feature was sort of a mistake, where the ease of access to `diff -bw` shaped behavior a very long time ago and then the train just ran a long way down the tracks in the same direction.
Test Plan: Grepped for `whitespace`, deleted almost everything. Poked around the UI a bit. I'm expecting the whitespace changes to get some more iteration this week so I not being hugely pedantic about testing this stuff exhaustively.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T3498
Differential Revision: https://secure.phabricator.com/D20185
Summary: Ref T13250. See D20149. Mostly: clarify semantics. Partly: remove magic "null" behavior.
Test Plan: Poked around, but mostly just inspection since these are pretty much one-for-one.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13250
Differential Revision: https://secure.phabricator.com/D20154
Summary:
Ref T13249. See <https://discourse.phabricator-community.org/t/configuring-the-number-of-taskmaster-daemons/2394/>.
Today, when a configuration value is "locked", we prevent //writes// to the database. However, we still perform reads. When you upgrade, we generally don't want a bunch of your configuration to change by surprise.
Some day, I'd like to stop reading locked configuration from the database. This would defuse an escalation where an attacker finds a way to write to locked configuration despite safeguards, e.g. through SQL injection or policy bypass. Today, they could write to `cluster.mailers` or similar and substantially escalate access. A better behavior would be to ignore database values for `cluster.mailers` and other locked config, so that these impermissible writes have no effect.
Doing this today would break a lot of installs, but we can warn them about it now and then make the change at a later date.
Test Plan:
- Forced a `phd.taskmasters` config value into the database.
- Saw setup warning.
- Used `bin/config delete --database phd.taskmasters` to clear the warning.
- Reviewed documentation changes.
- Reviewed `phd.taskmasters` documentation adjustment.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20159
Summary:
Depends on D20115. See <https://discourse.phabricator-community.org/t/transaction-search-endpoint-does-not-work-on-differential-diffs/2369/>.
Currently, `getApplicationTransactionCommentObject()` throws by default. Subclasses must override it to `return null` to indicate that they don't support comments.
This is silly, and leads to a bunch of code that does a `try / catch` around it, and at least some code (here, `transaction.search`) which doesn't `try / catch` and gets the wrong behavior as a result.
Just make it `return null` by default, meaning "no support for comments". Then remove the `try / catch` stuff and all the `return null` implementations.
Test Plan:
- Grepped for `getApplicationTransactionCommentObject()`, fixed each callsite / definition.
- Called `transaction.search` on a diff with transactions (i.e., not a sourced-from-commit diff).
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: jbrownEP
Differential Revision: https://secure.phabricator.com/D20121
Summary: Depends on D20114. This is on the way out, so make that explicitly clear.
Test Plan: Read setup issue and configuration option.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20115
Summary: This option no longer needs to be configured if you configure inbound mail (and that's the easiest setup approach in a lot of cases), so stop telling users they have to set it up.
Test Plan: Read documentation and configuration help.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20104
Summary: Depends on D20069. Ref T13232. This is a very, very weak dependency and we can reasonably polyfill it.
Test Plan: Grepped for `iconv` in libphutil, arcanist, and Phabricator.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13232
Differential Revision: https://secure.phabricator.com/D20070
Summary:
Ref T4369. Ref T12297. Ref T13242. Ref PHI1010. I want to take a quick look at `transaction.search` and see if there's anything quick and obvious we can do to improve performance.
On `secure`, the `__profile__` flag does not survive POST like it's supposed to: when you profile a page and then submit a form on the page, the result is supposed to be profiled. The intent is to make it easier to profile Conduit calls.
I believe this is because we're hooking the profiler, then rebuilding POST data a little later -- so `$_POST['__profile__']` isn't set yet when the profiler checks.
Move the POST rebuild a little earlier to fix this.
Also, remove the very ancient "aphront.default-application-configuration-class". I believe this was only used by Facebook to do CIDR checks against corpnet or something like that. It is poorly named and long-obsolete now, and `AphrontSite` does everything we might reasonably have wanted it to do.
Test Plan: Poked around locally without any issues. Will check if this fixes the issue on `secure`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13242, T12297, T4369
Differential Revision: https://secure.phabricator.com/D20046
