Change Details

PHI979 is a specific issue where we could use better instance controls (around worker count) in the short term. An alternative approach is to hard-coded `if ($instance === 'X') { ... }` for now. `admin` could also probably use more workers, I think we get saturated by backups occasionally. See PHI857, which wants support for serial queues in repository operations. See PHI989, which notes some consistency issues with certain datasource queries, particularly when some components are empty. SSH key handling (mostly in T13123) is fairly ripe. Related are these HackerOne reports, which are primarily just informative, but solid reports: - https://hackerone.com/reports/474897 - Weak SSH keys should be detected and rejected. - https://hackerone.com/reports/475126 - The SSH key "comment" field is exposed via the API, but it turns out that a whole lot of users put their entire personal diary into the "comment" field of their "public" keys. Phacility instances don't allow administrators to manage auth providers. This is good, but it also means that administrators can't manage MFA. We should either separate these permissions or maybe automatically create a TOTP MFA provider for now? PHI896 et al. could really use at least a viability assessment on "does not block draft promotion", etc. See also PHI1093 for "build can not restart". PHI985 would like smarter block boundary detection (T11738) and "click to expand block". T13161 should generally get at least an initial pass. Change attributes (T784) could use a pass on at least the server side for PHI675, PHI1061, and the various other issues linked there. See PHI1115 for `bin/policy unlock --for / --assign`. Also, `bin/conduit call --as ...` may fail on edits while attempting cache rebuilds (`EXCEPTION: (PhabricatorDataNotAttachedException) Attempting to access attached data on PhabricatorUser (via getUserSetting()), but the data is not actually attached. Before accessing attachable data on an object, you must load and attach it.`). The left-column alignment of the `>>` Differential marker is a little weird, and we probably don't need the left-side highlighting on indentation-only lines. Adding a "Restartable: If Failed" mode to build plans seems reasonable. --- // Cleared // It would be nice to make the linter for PHP version symbol issues smarter, so it can detect that `if (function_exists('x')) { x(); }` is always safe in any PHP version. A somewhat-adjacent issue to PHI1066 that I hit recently is that more stuff has become `Throwable` in PHP7.3, and some methods need to transition from `Exception` to `Throwable`, like `phlog(...)`. Some `Throwable` throws result in a blank page today. (See T13250.) PHI1066 has some possible fallout from query string construction changes. When an instance is unsuspended, we should automatically schedule a database upgrade. Currently, unsuspending a long-suspended instance tends to lead to a case where the instance is active but missing required schema changes. D20132 has a followup for adding rate limiting to "Add Payment Method". D20127 needs a revisit to make it less bad. See <https://discourse.phabricator-community.org/t/configuring-the-number-of-taskmaster-daemons/2394/>: - `bin/config set` should warn you when a stronger database value exists, and point you toward `bin/config delete --database`. - Setup guidance should warn you about locked config with database values, so we can some day stop reading database values from locked config to improve security (e.g., prevent an attacker who bypasses the lock from inserting a value for `cluster.mailers` which we'll actually respect). PHI1073 would like to see more task graph nodes. PHI1051 wants support for overriding custom field behaviors in subtypes. See D17593, previously. This now lives in T13248. PHI1059 wants stronger edit locks. PHI912 would like auto-status ~~and auto-submit~~ for Duo. PHI774 identifies an issue which can likely be mitigated in the short term by improving the "Welcome" flow for non-password installs. PHI810 would be served by having the Differential "Reviewers" tokenizer to show user availability. Maybe every user tokenizer should do this? Per PHI810, the "warn when all reviewers are away" in `arc` should really be "warn when some reviewers are away". We can do this in a coarse way now ("Some reviewers are away: alice, bailey.") and a more nuanced way after toolsets (availaiblity details via `user.search`).

PHI979 is a specific issue where we could use better instance controls (around worker count) in the short term. An alternative approach is to hard-coded `if ($instance === 'X') { ... }` for now. `admin` could also probably use more workers, I think we get saturated by backups occasionally. See PHI857, which wants support for serial queues in repository operations. See PHI989, which notes some consistency issues with certain datasource queries, particularly when some components are empty. SSH key handling (mostly in T13123) is fairly ripe. Related are these HackerOne reports, which are primarily just informative, but solid reports: - https://hackerone.com/reports/474897 - Weak SSH keys should be detected and rejected. - https://hackerone.com/reports/475126 - The SSH key "comment" field is exposed via the API, but it turns out that a whole lot of users put their entire personal diary into the "comment" field of their "public" keys. Phacility instances don't allow administrators to manage auth providers. This is good, but it also means that administrators can't manage MFA. We should either separate these permissions or maybe automatically create a TOTP MFA provider for now? PHI985 would like smarter block boundary detection (T11738) and "click to expand block". T13161 should generally get at least an initial pass. Change attributes (T784) could use a pass on at least the server side for PHI675, PHI1061, and the various other issues linked there. See PHI1115 for `bin/policy unlock --for / --assign`. Also, `bin/conduit call --as ...` may fail on edits while attempting cache rebuilds (`EXCEPTION: (PhabricatorDataNotAttachedException) Attempting to access attached data on PhabricatorUser (via getUserSetting()), but the data is not actually attached. Before accessing attachable data on an object, you must load and attach it.`). --- // Cleared // It would be nice to make the linter for PHP version symbol issues smarter, so it can detect that `if (function_exists('x')) { x(); }` is always safe in any PHP version. A somewhat-adjacent issue to PHI1066 that I hit recently is that more stuff has become `Throwable` in PHP7.3, and some methods need to transition from `Exception` to `Throwable`, like `phlog(...)`. Some `Throwable` throws result in a blank page today. (See T13250.) PHI1066 has some possible fallout from query string construction changes. When an instance is unsuspended, we should automatically schedule a database upgrade. Currently, unsuspending a long-suspended instance tends to lead to a case where the instance is active but missing required schema changes. D20132 has a followup for adding rate limiting to "Add Payment Method". D20127 needs a revisit to make it less bad. See <https://discourse.phabricator-community.org/t/configuring-the-number-of-taskmaster-daemons/2394/>: - `bin/config set` should warn you when a stronger database value exists, and point you toward `bin/config delete --database`. - Setup guidance should warn you about locked config with database values, so we can some day stop reading database values from locked config to improve security (e.g., prevent an attacker who bypasses the lock from inserting a value for `cluster.mailers` which we'll actually respect). PHI1073 would like to see more task graph nodes. PHI1051 wants support for overriding custom field behaviors in subtypes. See D17593, previously. This now lives in T13248. PHI1059 wants stronger edit locks. PHI912 would like auto-status ~~and auto-submit~~ for Duo. PHI774 identifies an issue which can likely be mitigated in the short term by improving the "Welcome" flow for non-password installs. PHI810 would be served by having the Differential "Reviewers" tokenizer to show user availability. Maybe every user tokenizer should do this? Per PHI810, the "warn when all reviewers are away" in `arc` should really be "warn when some reviewers are away". We can do this in a coarse way now ("Some reviewers are away: alice, bailey.") and a more nuanced way after toolsets (availaiblity details via `user.search`). The left-column alignment of the `>>` Differential marker is a little weird, and we probably don't need the left-side highlighting on indentation-only lines. Adding a "Restartable: If Failed" mode to build plans seems reasonable. PHI896 et al. could really use at least a viability assessment on "does not block draft promotion", etc. See also PHI1093 for "build can not restart".

PHI979 is a specific issue where we could use better instance controls (around worker count) in the short term. An alternative approach is to hard-coded `if ($instance === 'X') { ... }` for now. `admin` could also probably use more workers, I think we get saturated by backups occasionally. See PHI857, which wants support for serial queues in repository operations. See PHI989, which notes some consistency issues with certain datasource queries, particularly when some components are empty. SSH key handling (mostly in T13123) is fairly ripe. Related are these HackerOne reports, which are primarily just informative, but solid reports: - https://hackerone.com/reports/474897 - Weak SSH keys should be detected and rejected. - https://hackerone.com/reports/475126 - The SSH key "comment" field is exposed via the API, but it turns out that a whole lot of users put their entire personal diary into the "comment" field of their "public" keys. Phacility instances don't allow administrators to manage auth providers. This is good, but it also means that administrators can't manage MFA. We should either separate these permissions or maybe automatically create a TOTP MFA provider for now? PHI896 et al. could really use at least a viability assessment on "does not block draft promotion", etc. See also PHI1093 for "build can not restart". PHI985 would like smarter block boundary detection (T11738) and "click to expand block". T13161 should generally get at least an initial pass. Change attributes (T784) could use a pass on at least the server side for PHI675, PHI1061, and the various other issues linked there. See PHI1115 for `bin/policy unlock --for / --assign`. Also, `bin/conduit call --as ...` may fail on edits while attempting cache rebuilds (`EXCEPTION: (PhabricatorDataNotAttachedException) Attempting to access attached data on PhabricatorUser (via getUserSetting()), but the data is not actually attached. Before accessing attachable data on an object, you must load and attach it.`). The left-column alignment of the `>>` Differential marker is a little weird, and we probably don't need the left-side highlighting on indentation-only lines. Adding a "Restartable: If Failed" mode to build plans seems reasonable. --- // Cleared // It would be nice to make the linter for PHP version symbol issues smarter, so it can detect that `if (function_exists('x')) { x(); }` is always safe in any PHP version. A somewhat-adjacent issue to PHI1066 that I hit recently is that more stuff has become `Throwable` in PHP7.3, and some methods need to transition from `Exception` to `Throwable`, like `phlog(...)`. Some `Throwable` throws result in a blank page today. (See T13250.) PHI1066 has some possible fallout from query string construction changes. When an instance is unsuspended, we should automatically schedule a database upgrade. Currently, unsuspending a long-suspended instance tends to lead to a case where the instance is active but missing required schema changes. D20132 has a followup for adding rate limiting to "Add Payment Method". D20127 needs a revisit to make it less bad. See <https://discourse.phabricator-community.org/t/configuring-the-number-of-taskmaster-daemons/2394/>: - `bin/config set` should warn you when a stronger database value exists, and point you toward `bin/config delete --database`. - Setup guidance should warn you about locked config with database values, so we can some day stop reading database values from locked config to improve security (e.g., prevent an attacker who bypasses the lock from inserting a value for `cluster.mailers` which we'll actually respect). PHI1073 would like to see more task graph nodes. PHI1051 wants support for overriding custom field behaviors in subtypes. See D17593, previously. This now lives in T13248. PHI1059 wants stronger edit locks. PHI912 would like auto-status ~~and auto-submit~~ for Duo. PHI774 identifies an issue which can likely be mitigated in the short term by improving the "Welcome" flow for non-password installs. PHI810 would be served by having the Differential "Reviewers" tokenizer to show user availability. Maybe every user tokenizer should do this? Per PHI810, the "warn when all reviewers are away" in `arc` should really be "warn when some reviewers are away". We can do this in a coarse way now ("Some reviewers are away: alice, bailey.") and a more nuanced way after toolsets (availaiblity details via `user.search`). The left-column alignment of the `>>` Differential marker is a little weird, and we probably don't need the left-side highlighting on indentation-only lines. Adding a "Restartable: If Failed" mode to build plans seems reasonable. PHI896 et al. could really use at least a viability assessment on "does not block draft promotion", etc. See also PHI1093 for "build can not restart".