Page MenuHomePhabricator

2018 Week 45-47 Bonus Content
Closed, ResolvedPublic

Description

I'd like to prevent passwords where the password is a substring of any account or install identifier, or vice versa. (For example, if your username is alincoln, passwords alincoln, lincoln, and alincoln1 would not be permitted.) This is primarily to stop occasional reports from HackerOne that this is a critical security issue. I don't think we can really stop determined users from selecting horrendous passwords or, say, tweeting their passwords, but we can stop researchers from reporting it.

See PHI944, which has another large diff that's causing import problems.

See PHI948, which reports some especially bad/confusing timeout behavior from Macros.

See PHI939, which has a major aesthetic upgrade.

See PHI943 and PHI889. One primitive we should clearly build in the short term is an intracluster sync log, since it would likely have helped with about 5 different issues by now. This log should, particularly, make fetch failures during sync more clear.

See PHI943. The bin/repository thaw --promote operation and/or the version bumping after a write can currently misbehave in the presence of disabled nodes with larger versions. When we increment the version, we should bump it past the largest version of any node, not just any enabled node.

See PHI951. Beyond introducing a sync log, we should tighten up the timing reported by the existing pull/push logs. Notably, hookTime (time spent in commit hooks) and subprocessTime (time spent running the git/hg/etc subprocess) would have been useful in investigating this issue.

See PHI947, which would like a Herald filter which excludes personal rules owned by disabled users (these rules do not actually run, but appear in the "Active" list, which is misleading).

See PHI959, et al. We could easily add an asChunks(...) primitive to LiskMigrationIterator to give these kinds of migrations a ~100x boost, since they tend to be limited by per-query INSERT overhead (e.g., network round trip cost).

See PHI976, which is T8440.

See PHI977, which reports an overflow issue with hovercards that reference objects with long names.

See PHI970, which identifies an issue with containerPHID not being populated correctly on a subset of buildables.

See PHI958, which refines diffusion.branchquery.

See PHI975, which raises an issue with "Submit Quietly" button text in Differential being misleading when "Request Review" is in your action stack.

See PHI943. The UI for managing cluster storage doesn't visually scale very well when you have a larger number of disabled nodes. At a minimum, better sorting would be helpful.

See PHI885, which requests timeouts on git fetch during working copy construction.

See PHI980, which wants configurable permissions for creating custom forms.

See PHI916. When resolving futures during builds, we should close connections if we believe we're going to be sitting there for a while.

See PHI916. T11908 should move forward to the "migrate 700 callsites" stage, at least.

See PHI984, which wants identifiers for diffusion.commit.search.

See PHI908 (et al?) which would like a maximum filesize limit for hosted repositories and a maximum number of files a commit is permitted to touch.

See PHI911, which discusses an unintuitive resign-after-reject behavior.

Revisions and Commits

rPHU libphutil
D19811
D19823
D19815
D19800
D19783
D19782
D19781
rP Phabricator
D19839
D19831
D19830
D19829
D19827
D19834
D19826
D19832
D19837
D19836
D19833
D19835
D19838
D19840
D19828
D19825
D19824
D19817
D19813
D19822
D19816
D19814
D19812
D19807
D19810
D19809
D19806
D19805
D19803
D19808
D19802
D19801
D19799
D19798
D19793
D19780
D19779
D19778
D19776
D19777
D19775

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

See PHI970, which identifies an issue with containerPHID not being populated correctly on a subset of buildables.

General context on this one: there was a whole mess of race conditions in T13054 that have supposedly been resolved now. However, the reporting install is seeing about 1 in 10 Buildables with no containerPHID, and this seems consistent with the data on this install. These suggest that not all the races in T13054 got squashed completely.

Here's recent data from this install:

mysql> select * from harbormaster_buildable order by id desc limit 10;
+-------+--------------------------------+--------------------------------+--------------------------------+-----------------+-------------+--------------+-------------------+
| id    | phid                           | buildablePHID                  | containerPHID                  | buildableStatus | dateCreated | dateModified | isManualBuildable |
+-------+--------------------------------+--------------------------------+--------------------------------+-----------------+-------------+--------------+-------------------+
| 21149 | PHID-HMBB-e5bb6atw5lxh3ntulhxv | PHID-CMIT-mtthjvtzvzqr2c7zhszp | PHID-REPO-9a38b7db1b795ae3e348 | passed          |  1542238339 |   1542238440 |                 0 |
| 21148 | PHID-HMBB-submzocdskbq6ql6udhu | PHID-DIFF-gvehtgevefxjonnf2ne5 | PHID-DREV-5zty467rtj7frt46hgji | failed          |  1542238167 |   1542238192 |                 0 |
| 21147 | PHID-HMBB-b5zilvap362tah3uyzsi | PHID-DIFF-wg7jzrs22ujdo3c572dr | PHID-DREV-6unbxu7xc5zg6vormnpu | failed          |  1542237068 |   1542237092 |                 0 |
| 21146 | PHID-HMBB-uvmk62sz24vk52tqv74r | PHID-CMIT-d2tbdirhxjbes6yu7mlp | PHID-REPO-9a38b7db1b795ae3e348 | passed          |  1542235862 |   1542235991 |                 0 |
| 21145 | PHID-HMBB-uwetovtcyzyzg4swejdz | PHID-DIFF-yf7j5z7fzakqhzfif67t | PHID-DREV-azsqiofjrnvdp5ejbthe | passed          |  1542234423 |   1542234463 |                 0 |
| 21144 | PHID-HMBB-7e333qwfmyuhxmo4wbo5 | PHID-DIFF-4dtanjpsyyzjpr4b4wfe | PHID-DREV-l676hm5n3h3gqdmg2rad | failed          |  1542226284 |   1542226308 |                 0 |
| 21143 | PHID-HMBB-thojlifcpzle74beotsu | PHID-DIFF-vmm53lgwutw6ewiifn74 | NULL                           | failed          |  1542217547 |   1542217572 |                 0 |
| 21142 | PHID-HMBB-qeyge7ow43qsmdk7s5ux | PHID-DIFF-f3x2snpe2amzhqztdncp | PHID-DREV-soajarjsgnpsqyzoeqx2 | failed          |  1542134501 |   1542134524 |                 0 |
| 21141 | PHID-HMBB-4u3e7lj53rknsaxxz7qs | PHID-DIFF-5ppfvk4rftq7n4xsfovi | PHID-DREV-soajarjsgnpsqyzoeqx2 | failed          |  1542133990 |   1542134020 |                 0 |
| 21140 | PHID-HMBB-qtnryrinzvm5bnnoz454 | PHID-DIFF-2267cih3lv3dwc5s3dwx | NULL                           | passed          |  1542133767 |   1542133799 |                 0 |
+-------+--------------------------------+--------------------------------+--------------------------------+-----------------+-------------+--------------+-------------------+
10 rows in set (0.00 sec)

B21143 and B21140 have null values for containerPHID, but their diffs have valid containers, and both came out of me running arc diff normally, so there's no obvious reason they should be missing containers.

Both buildables received and supposedly processed container update messages:

mysql> select * from harbormaster_buildmessage where receiverPHID IN ('PHID-HMBB-thojlifcpzle74beotsu', 'PHID-HMBB-qtnryrinzvm5bnnoz454');
+-------+--------------------------------+--------------------------------+-----------+------------+-------------+--------------+
| id    | authorPHID                     | receiverPHID                   | type      | isConsumed | dateCreated | dateModified |
+-------+--------------------------------+--------------------------------+-----------+------------+-------------+--------------+
| 31554 | PHID-USER-ba8aeea1b3fe2853d6bb | PHID-HMBB-qtnryrinzvm5bnnoz454 | container |          1 |  1542133768 |   1542133768 |
| 31555 | PHID-USER-ba8aeea1b3fe2853d6bb | PHID-HMBB-qtnryrinzvm5bnnoz454 | build     |          1 |  1542133768 |   1542133769 |
| 31567 | PHID-USER-ba8aeea1b3fe2853d6bb | PHID-HMBB-thojlifcpzle74beotsu | container |          1 |  1542217549 |   1542217549 |
| 31568 | PHID-USER-ba8aeea1b3fe2853d6bb | PHID-HMBB-thojlifcpzle74beotsu | build     |          1 |  1542217549 |   1542217549 |
+-------+--------------------------------+--------------------------------+-----------+------------+-------------+--------------+
4 rows in set (0.01 sec)

This might be a mundane race: the message is sent from inside applyFinalEffects(), which is still inside the transaction. If the engine runs fast enough it may not see the new version of the Diff yet? If so, this should be trivial to reproduce by adding a sleep(15) at the end of applyFinalEffects() after sending the message.

If this is the issue, the fix is to introduce applyFinalestEffects().

the message is sent from inside applyFinalEffects(), which is still inside the transaction

Er, it's actually in applyExternalEffects(), but the two are adjacent and have the same behavior with respect to transactions so there's no practical difference:

foreach ($xactions as $xaction) {
  $this->applyExternalEffects($object, $xaction);
}

$xactions = $this->applyFinalEffects($object, $xactions);

this should be trivial to reproduce

I made this change:

diff --git a/src/applications/differential/xaction/DifferentialRevisionUpdateTransaction.php b/src/applications/differential/xaction/DifferentialRevisionUpdateTransaction.php
index bf1e6de87..f89627c68 100644
--- a/src/applications/differential/xaction/DifferentialRevisionUpdateTransaction.php
+++ b/src/applications/differential/xaction/DifferentialRevisionUpdateTransaction.php
@@ -74,6 +74,8 @@ final class DifferentialRevisionUpdateTransaction
         HarbormasterMessageType::BUILDABLE_CONTAINER,
         true);
     }
+
+    sleep(15);
   }
 
   public function getColor() {

..and I'm 3 / 3 for reproducing:

mysql> select * from harbormaster_buildable order by id desc limit 3;
+----+--------------------------------+--------------------------------+---------------+-----------------+-------------+--------------+-------------------+
| id | phid                           | buildablePHID                  | containerPHID | buildableStatus | dateCreated | dateModified | isManualBuildable |
+----+--------------------------------+--------------------------------+---------------+-----------------+-------------+--------------+-------------------+
| 19 | PHID-HMBB-mtlhmnuhrs7ga6624e4e | PHID-DIFF-cmxgewbqu373pbsiwnbo | NULL          | passed          |  1542239921 |   1542239938 |                 0 |
| 18 | PHID-HMBB-fc67iwubh73odobclmgk | PHID-DIFF-3fgzrza7oeeosid56u3o | NULL          | passed          |  1542239738 |   1542239754 |                 0 |
| 17 | PHID-HMBB-tzzyl4y5456kd2on7mrk | PHID-DIFF-owfagqd4nl7274jry2vk | NULL          | passed          |  1542239684 |   1542239701 |                 0 |
+----+--------------------------------+--------------------------------+---------------+-----------------+-------------+--------------+-------------------+
3 rows in set (0.00 sec)

So I think I'm likely on the right track. I'll introduce applyFinalistEffects() and move the message send there.

See PHI885, which requests timeouts on git fetch during working copy construction.

We currently have a similar timeout in intracluster sync. Both are roughly "what's the maximum amount of time this repository should ever take to clone?".

I'm going to move this timeout into PhabricatorRepository and use it in both cases. A future change can make it configurable if necessary.

See PHI908 (et al?) which would like a maximum filesize limit for hosted repositories.

I anticipate adding a "Limits" tab to repository management with the filesize limit (default: unlimited), copy timeout limit, and (eventually) LFS quota and limit information. I'd like to make some general UI adjustments so this may not actually make it into this release, but I'm going to take a crack at the filesize mechanics first, at least.

A possible alternative approach here is to use Herald, but I tend to not like that because it will be hard to provide useful error messages to users:

In PHI908, @epriestley wrote:

PHI750 (internal) is another similar request. I currently anticipate letting you configure a repository-level maximum file size since doing this with Herald is a bit tricky and it would be hard to raise a descriptive error (we want "File X is size Y, larger than limit Q for this repository." but Herald could only give you "One or more files in this push are too big (See Herald Rule Hxyz)" without more tailoring/magic).

epriestley renamed this task from 2018 Week 45-46 Bonus Content to 2018 Week 45-47 Bonus Content.Nov 17 2018, 1:38 AM
epriestley updated the task description. (Show Details)