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.

Details

Commits
D19839 / rP9bfe558587aa: Add a "touched paths" limit to repositories, limiting the maximum number of…
D19831 / rPc86c5749bafb: Make the repository "Filesize Limit" and "Clone/Fetch Timeout" configurable in…
D19830 / rPc6fc05ee0941: Pull Git filesize logic into a separate LowLevel query and use more Iterators
D19829 / rPfd12b37d16f6: Modularize Repository transactions
D19827 / rPc25d2a399d68: Separate the repository management UI into sections
D19834 / rPc457d23a1d30: Tailor the "no reviewers on this revision" warnings to handle the case where…
D19826 / rPbb369c7b711f: Convert the "Repository Management" UI to a full-width, Phortune-style UI
D19832 / rP2e8a5e843f95: Recover when cookies are disabled in Firefox and accessing localStorage throws
D19837 / rP88189f723f05: Make a Feed query construction less clever/sneaky for new qsprintf() semantics
D19836 / rP8b550ce2cd8f: Don't allow the middle mouse button to start an inline comment
D19833 / rP5343b1f898d4: Remove defunct "metamta.herald.show-hints" Config option
D19835 / rP9473f60a362f: Allow "Abandoned" revisions to be commandeered
D19838 / rPbcc90d8c6b8c: Fix an off-by-one error affecting mail rendering of inlines on the final line…
D19840 / rP97e7ef0f015c: When the last rejecting reviewer resigns from a revision, return it to "Needs…
D19828 / rP03f249baf349: Remove rendering support for very old Repository transactions
D19825 / rP45e93c8f1d1a: Expose "identifiers" as a query constraint for Commit search
D19811 / rPHU35d0ec2dfa59: Keep the new "%P" query conversion out of the service call profiler by…
D19824 / rP43cf4edfb16b: When waiting for long-running Harbormaster futures to resolve, close idle…
D19823 / rPHU4641a84dadf1: Define "idle" connections that are safe to close (no transactions, no held…
D19817 / rPa0d4b6da4b60: Support (but do not actually enable) a maximum file size limit for Git…
D19813 / rPab14f49ef87d: On the Diffusion cluster status page, improve device sort order
D19822 / rP9481b9eff111: Allow "Can Configure Application" permissions to be configured
D19816 / rPcb033673b6eb: Unify intracluster sync and Drydock working copy construction timeouts as a…
D19814 / rP933462b4873b: Continue cleaning up queries in the wake of changes to "%Q"
D19812 / rP49483bdb4823: Use "%P" to protect session key hashes in SessionEngine queries from DarkConsole
D19807 / rPb2e91d220539: Move the "container updated" message for Buildables that build Diffs outside of…
D19815 / rPHUe90d5edc1567: When a command is killed by a timeout, make the human-readable error more clear
D19810 / rP44c32839a68c: When you "Request Review" of a draft revision, change the button text from…
D19809 / rPec452e548a0d: Improve text overflow behavior for hovercards with (for example) long package…
D19806 / rP533e4e13b35a: Add a `bin/herald test ...` for doing test runs via the CLI
D19805 / rP8a8123c9db3b: Replace the primary "Disabled/Enabled" Herald Rule filter with…
D19803 / rPbbd292b9b34e: Modernize the Herald rule search engine
D19808 / rPea6d2afa8685: Fix flickering tooltips in Chrome when the tip container overlaps the…
D19802 / rP96f9b0917e21: Improve performance of two recent commit migrations
D19801 / rP86fd2041484f: Fix all query warnings in "arc unit --everything"
D19800 / rPHU0e6ee5937ca5: Add "%Z" (Raw Query) and "%LK" (List of Columns for Keys) to qsprintf()
D19783 / rPHUbe5a87463ac5: Support %LA (AND), %LO (OR) and %LQ (comma) conversions for qsprintf() to…
D19782 / rPHUf842247de41a: Support %P (Password or Secret) in qsprintf()
D19781 / rPHUe1e8d3ba95b2: Make qsprintf() return an object, not a string, to support %P and hardening of…
D19799 / rP315d857a8ad6: Add a basic web UI for intracluster sync logs
D19798 / rP1d7c960531be: Put push log "hookWait" to data export and add all wait values to UI
D19793 / rP2a7ac8e3882f: Make "bin/repository thaw" workflow more clear when devices are disabled
D19780 / rPb12e92e6e2dd: Add timing information for commit hooks to push logs
D19779 / rP966db4d38ec8: Add an intracluster synchronization log for cluster repositories
D19778 / rPe09d29fb1a3e: Clean up the workflow for some post-push logging code
D19776 / rP1f6a4cfffe58: Prevent users from selecting excessively bad passwords based on their username…
D19777 / rPc206b066df38: When `{meme ...}` embed has no text, just use the raw file data unmodified
D19775 / rPbbfc860c6359: Improve aesthetics of commit hook rejection message

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
epriestley updated the task description. (Show Details)Nov 14 2018, 11:39 PM

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.

epriestley updated the task description. (Show Details)Nov 15 2018, 12:20 PM
epriestley updated the task description. (Show Details)Nov 15 2018, 1:33 PM
epriestley updated the task description. (Show Details)Nov 15 2018, 1:44 PM

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)
epriestley updated the task description. (Show Details)Sat, Nov 17, 6:38 PM
epriestley updated the task description. (Show Details)Wed, Nov 21, 4:22 PM
epriestley updated the task description. (Show Details)Wed, Nov 21, 4:39 PM
epriestley updated the task description. (Show Details)Wed, Nov 21, 5:03 PM
epriestley updated the task description. (Show Details)Wed, Nov 21, 5:08 PM
epriestley updated the task description. (Show Details)Wed, Nov 21, 5:19 PM
epriestley updated the task description. (Show Details)
epriestley updated the task description. (Show Details)Thu, Nov 22, 1:18 AM
epriestley updated the task description. (Show Details)Thu, Nov 22, 1:51 AM
epriestley updated the task description. (Show Details)Mon, Nov 26, 1:53 PM
epriestley updated the task description. (Show Details)Mon, Nov 26, 2:06 PM
epriestley updated the task description. (Show Details)
epriestley closed this task as Resolved.Tue, Nov 27, 9:34 PM