Summary: Ref T13680. Conpherence may pass values with an integer type to this layer of the stack. These are "supposed" to be strings, but just be accepting.
Test Plan:
- Wrote 100+ messages to a Conpherence room.
- Clicked "Show Older Messages".
- Before: exception, int passed to "phutil_nonempty_string()".
- After: older messages loaded.
Maniphest Tasks: T13680
Differential Revision: https://secure.phabricator.com/D21824
Summary:
Ref T13681. This was introduced in D593, never used, and doesn't make sense or have any uses in modern Phabricator.
It also does some pretty direct writes that can technically do things that at least //look// like they violate policies, so remove it.
Test Plan:
- Checked the API console, no longer saw "feed.publish".
- Grepped for "feed.publish", no hits.
Maniphest Tasks: T13681
Differential Revision: https://secure.phabricator.com/D21826
Summary:
Ref T13679. In D16983, global settings objects were given an exception to let logged-out users see them, even on installs with no "public" user role.
This exception is too broad and grants everyone all capabilities, not just "CAN_VIEW". In particular, it incorrectly grants "CAN_EDIT", so any user can edit global settings defaults.
Restrict this grant to "CAN_VIEW".
Test Plan:
- As a non-administrator, tried to edit global settings.
- Before: could.
- After: could not.
Maniphest Tasks: T13679
Differential Revision: https://secure.phabricator.com/D21811
Summary: Ref T13678. When remarkup content embeds other remarkup content, detect and degrade if the references have nesting depth greater than 1. This is a coarse cycle detector, since rendering shallow (but technically non-cycling) trees doesn't seem valuable.
Test Plan: Created various objects with self-references, saw everything degrade properly (after one level of embedding) when embedded in itself and in other contexts. See attached screenshot.
Maniphest Tasks: T13678
Differential Revision: https://secure.phabricator.com/D21810
Summary: Ref T13677. This was an accidental change in D21807: when reclaiming a resource, wait for it to be completely destroyed before allowing a lease to reclaim another resource.
Test Plan: Reverts accidental behavior change in D21807.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13677
Differential Revision: https://secure.phabricator.com/D21809
Summary:
Ref T13677. Drydock has a hard-coded and fairly arbitrary limit which prevents a resource pool from growing more than 25% at once.
This is vaguely reasonable for resources which allocate quickly, but suffocating for slower resources. It's also wholly arbitrary, and the "one per lease" limit introduced in D21807 should do a better job of covering the same ground while generally being more reasonable, expected, and predictable.
Test Plan: Ran Drydock allocations without the throttle, saw faster pool growth.
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13677
Differential Revision: https://secure.phabricator.com/D21808
Summary:
Ref T13677. Currently, one lease may cause multiple resources to allocate simultaneously if it starts allocating one, then wakes up from a yield later on and still sees no available resources.
This is never desired -- or, at least, produces desirable behavior only entirely by accident. Normally, it causes an excess of resources to allocate.
This is not a catastrophic problem: the extra resources usually get used sooner or later or cleaned up; and the total amount of badness is limited by overall resource allocation limits.
However, this behavior is also suppressed by an artificial "25% of current pool size" growth limit throttle which I intend to remove. Removing this throttle without fixing the allocator behavior could make this "too many resources" problem worse.
Change the allocator so that a lease that has started allocating a resource won't allocate another resource until the first resource leaves the "pending" state.
This also fixes some general oddness with the allocator and attempts to simplify the structure.
Test Plan:
- Ran 8 taskmasters.
- Destroyed all resources and leases.
- Leased 4 working copies.
- Saw exactly 4 resources build and lease, all simultaneously.
- Destroyed all resources and leases.
- Leased 32 working copies.
- Saw exactly 32 resources build and lease, approximately 8 at a time (limited by taskmasters).
- Destroyed all leases (but not resources).
- Leased 32 working copies, saw them satisfied by existing resources.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13677
Differential Revision: https://secure.phabricator.com/D21807
Summary:
Ref T13677. Track which resources a given lease has begun allocating or reclaiming in a more formal way, and add logging for waiting actions.
The "allocating" mechanism is new. This will replace an existing similar "reclaiming" mechanism in a future change.
Test Plan: See followup changes.
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13677
Differential Revision: https://secure.phabricator.com/D21806
Summary: Ref T13677. These flags increase the convenience of testing in a development environment.
Test Plan: Used new "--all" flags to release all resources and leases.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13677
Differential Revision: https://secure.phabricator.com/D21805
Summary:
Ref T13676. The 3-minute grace period when a resource can not be reclaimed after its leases are released currently doesn't work reliably because the Resource object usually isn't actually updated when a lease is released.
Add an additional check for recently-destroyed leases, and extend the grace period if we find any.
Test Plan:
- See T13676. Ran reproduction sequence there, observed immediate resource reclamation.
- Applied patch.
- Ran sequence again, observed repository B wait 3 minutes to reclaim a repository A resource.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13676
Differential Revision: https://secure.phabricator.com/D21803
Summary:
Ref T13676. Currently, "bin/drydock lease" just creates a lease that permits any blueprint.
To prepare for "use specific blueprint X", unify the logic between this workflow and LeaseUpdateWorker so we select only blueprints which at least have coarse compatibility (e.g., if we're leasing a host, only select enabled blueprints of classes that can allocate hosts).
Test Plan: Used `bin/drydock lease` to try to lease a nonsense type, got sensible error. Leased a host.
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13676
Differential Revision: https://secure.phabricator.com/D21801
Summary: Ref T13676. This makes it easier to create resource pressure without juggling a big pile of terminals.
Test Plan: Used `bin/drydock lease --count 5 ...` to acquire 5 leases.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13676
Differential Revision: https://secure.phabricator.com/D21800
Summary: Ref T13676. When the required "repositories.map" attribute is omitted, `bin/drydock lease` currently fatals in an unhelpful way when trying to lease a working copy.
Test Plan:
Ran `bin/drydock lease --type working-copy` with no attributes, after following steps in T13676.
```
<Allocation Failed> One or more blueprints promised a new resource, but failed when allocating: [PhutilAggregateException] All blueprints failed to allocate a suitable new resource when trying to allocate lease ("PHID-DRYL-orbtwtlinksm3xqpyhmw").
- Exception: Working copy lease is missing required attribute "repositories.map".
Attribute "repositories.map" should be a map of repository specifications.
```
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13676
Differential Revision: https://secure.phabricator.com/D21796
Summary: Ref T13676. Ref T13588. Fix some issues that prevent "bin/phd" and "bin/drydock" from executing under PHP 8.1, broadly because `null` is being passed to `strlen()`.
Test Plan: Ran `bin/phd debug task` and `bin/drydock ...` under PHP 8.1.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13676, T13588
Differential Revision: https://secure.phabricator.com/D21795
Summary:
Ref T5479. Ref T13658. This was a contributed application from the early days of Phabricator which never had customers or users in the wild. The contributor moved on from the project many years ago.
Any capabilities in this general role would look different today. It also has one or two product name literal strings, so this is as good a time as any to remove it.
This change does not remove storage; I'll issue upgrade guidance and do that separately after some time.
Test Plan: Grepped for "phragment", got no relevant hits.
Subscribers: hach-que, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13658, T5479
Differential Revision: https://secure.phabricator.com/D21793
Summary:
Ref T9530. Ref T13658. The "Releeph" application was never useful outside of Facebook and any application providing release support would not resemble it much.
It has some product name literal strings, so now is as good a time as any to get rid of it.
This application never left prototype and I'm not aware of any install in the wild that uses it (or has ever used it).
I did not destroy the database itself. I'll issue upgrade guidance and destroy the database in some future release, just in case.
Test Plan: Grepped for "releeph", found no relevant/removable hits.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13658, T9530
Differential Revision: https://secure.phabricator.com/D21792
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.
- Started a JIRA auth provider creation workflow.
- Same for LDAP.
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21779
Summary: Ref T13658.
Test Plan:
This is non-exhuastive.
- Poked around "Auth": edited a provider, toggled a provider.
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21778
Summary: Ref T13658. This one is a little silly since I used `split` to divide the list of files up, and only ended up with one file in the last partition.
Test Plan: Viewed Phabricator.
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21774
Summary: Ref T13658.
Test Plan:
This test plan is non-exhaustive.
- Ran `bin/storage databases`.
- Viewed Badges UI exmaples page.
- Used eval rule for `strings.platform.server.name`, got "Phabricator".
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21773
Summary: Ref T13658.
Test Plan:
This test plan is non-exhaustive.
- Viewed "remarkup.process" Conduit method API page.
- Viewed URIs in a Diffusion repository.
- Viewed editor protocol configuration in Settings.
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21772
Summary: Ref T13658.
Test Plan:
This test plan is non-exhaustive.
- Ran `bin/mail`.
- Uninstalled and reinstalled an application.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21770
Summary: Ref T13658
Test Plan:
This test plan is non-exhaustive.
- Looked at some of the config.
- Looked at guides.
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21769
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: Ref T13658. I used the linter in D21763 to identify these and `split` them into arbitrary groups of 10 files.
Test Plan:
This test plan is non-exhaustive, because some of these strings are difficult to reach.
- Looked at "Create Service" in Almanac.
- Used "bin/auth" to go through a one-time auth workflow (not all related strings can be hit on a single workflow).
- Started the "Generate Keypair" worfklow in "SSH Public Keys".
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21765
Summary:
Ref T13667. The Herald "content added" rule (and other similar rules) do not correctly extract content from empty commits.
When we load an empty raw diff, return an empty changed content map.
Ref T13588. Also fix some PHP8.1 null/string stuff
Test Plan:
- Ran "bin/repository reparse --publish <commit>", with an empty commit hash and a nonempty commit hash.
- Reviewed Herald transcripts for general sanity.
Maniphest Tasks: T13667, T13588
Differential Revision: https://secure.phabricator.com/D21761
Summary:
Ref T13667. When a revision's diff has no changesets (usually because Diffusion performed an automatic update with an empty commit), the UI currently tries to "min()" an empty array and fatals.
Handle this case properly.
Test Plan:
- Created a revision with a diff with no changesets ("git commit --allow-empty" + copy-paste into web UI).
- Viewed revision.
- Before: "min()" fatal.
- After: UI isn't perfect, but works without fataling.
Subscribers: cspeckmim
Maniphest Tasks: T13667
Differential Revision: https://secure.phabricator.com/D21760
Summary:
Ref T13673. After the changes in that task, we may execute "git config -l ..." as a user other than the user we used to write this temporary file.
Use "--file -" to pass the data instead, avoiding use of temporary files. This makes us agnostic to filesystem permissions.
Test Plan: Viewed a Git repository with submodules in Diffusion with "ssh.user" configured as a user relatively isolated from the webserver user.
Maniphest Tasks: T13673
Differential Revision: https://secure.phabricator.com/D21759
Summary: Ref T13588. Fix a couple of argument parsing issues here.
Test Plan: Ran "bin/auth recover" under PHP 8.1.
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21757
Summary:
See T13673. Recent versions of Git (and older versions with backported security patches) now refuse to run Git commands if the top-level repository directory is not owned by the user running the command.
Currently, we "sudo" to that user only when performing writes, so upgrading Git can aggressively break a Phabricator system by knocking out essentially all Diffusion/Conduit read pathways.
As an immediate mitigation, just "sudo" in all cases where a daemon user is available. This fixes the problem, and seems like the least-bad approach. The downside is that the web user may theoretically have fewer privileges than the daemon user and this could reduce the number of layers an attacker armed with some other Git vulnerability might have to get through to do something dangerous (e.g., perform a write on a pathway where only reads are expected), but any separation between the web and daemon accounts is essentially theoretical and has never been enforced.
Test Plan: Applied patch to impacted Phacility shard, saw Diffusion work properly again.
Differential Revision: https://secure.phabricator.com/D21756
Summary: Ref T13661. Fix up some UI and language so it's more clear that this is about disabling blog comments.
Test Plan: Viewed UIs, saw some more usable strings.
Maniphest Tasks: T13661
Differential Revision: https://secure.phabricator.com/D21755
