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 T13588. This configuration value may not be set.
Also fix an issue in `bin/storage` and whatever else I hit between now and this diff actually uploading.
Also fix a MySQLi report mode difference, beginning in PHP 8.1.
Also update a bunch of "static" property usage in Lisk.
Test Plan: Ran `bin/files ...` locally under PHP 8.1.
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21744
Summary:
Ref T13627. If a lock fails, the connection may be returned to the pool, even if the connection is an external connection. Under old versions of MySQL, connection reuse can release other locks on the same connection.
Don't return external connections to the pool.
This issue was introduced in D21369.
Test Plan: Added a failing test and made it pass.
Maniphest Tasks: T13627
Differential Revision: https://secure.phabricator.com/D21585
Summary:
Ref T13627. MySQL versions older than 5.7 release held locks when a new lock is acquired.
Prevent acquisition of a second lock to prevent this.
Test Plan: Added a failing unit test, made it pass.
Maniphest Tasks: T13627
Differential Revision: https://secure.phabricator.com/D21586
Summary: Ref T13627. This makes the API harder to misuse: setting an external connection on a held lock isn't a meaningful operation. Prevent it.
Test Plan: Added a failing test, made it pass.
Maniphest Tasks: T13627
Differential Revision: https://secure.phabricator.com/D21584
Summary:
Ref T13627. Currently, global locks always return connections (even external connections) to the connection pool when unlocked.
This code is obviously buggy: `isExternalConnection` is set to false immediately before it is tested. This bug has existed since this code was introduced, in D15792.
- Instead of storing a flag, store the actual connection.
- Don't clear it when unlocking.
- Don't return external connections to the pool.
Test Plan:
- Added a failing test, made it pass.
Maniphest Tasks: T13627
Differential Revision: https://secure.phabricator.com/D21583
Summary:
See PHI1794, which describes a connection exhaustion issue with a large number of webhook tasks in queue.
The "GlobalLock" mechanism manages a separate connection pool from the main pool, and webhook workers immediately try to grab a webhook lock with a 0-second wait when they start. So far, this is fine.
Prior to this change, good connections which fail to acqiure a lock are discarded. This can lead to connection exhaustion as the worker rapidly cycles through lock attempts: the connections will remain open for at least 60 seconds (since D16389) in an effort to avoid outbound port exhaustion, but they're effectively orphaned because they aren't part of the main pool and aren't part of the lock pool. We're basically leaking a connection every time we fail to lock.
Failing to lock doesn't mean we need to discard the connection: it's a completely suitable connection for reuse. Instead of dropping it on the floor, put it into the lock pool.
Test Plan:
- Used "bin/webhook call ... --count 10000 --background" to queue a large number of webhook calls against a slow ("sleep(15);") webhook.
- Used "bin/phd launch 32 taskmaster" to start taskmasters.
- Observed MySQL connection behavior:
- Before change: 2048 configured connections immediately exhausted.
- After change: connections stable at ~160ish.
- Ran queue for a while, saw expected single-threaded calls to webhook.
Differential Revision: https://secure.phabricator.com/D21369
Summary:
Fixes T13392. If you have 17 load balancers in sequence, Phabricator will receive requests with at least 17 "X-Forwarded-For" components in the header.
We want to select the 17th-from-last element, since prior elements are not trustworthy.
This currently isn't very easy/obvious, and you have to add a kind of sketchy piece of custom code to `preamble.php` to do any "X-Forwarded-For" parsing. Make handling this correctly easier.
Test Plan:
- Ran unit tests.
- Configured my local `preamble.php` to call `preamble_trust_x_forwarded_for_header(4)`, then made `/debug/` dump the header and the final value of `REMOTE_ADDR`.
```
$ curl http://local.phacility.com/debug/
<pre>
HTTP_X_FORWARDED_FOR =
FINAL REMOTE_ADDR = 127.0.0.1
</pre>
```
```
$ curl -H 'X-Forwarded-For: 1.1.1.1, 2.2.2.2, 3.3.3.3, 4.4.4.4, 5.5.5.5, 6.6.6.6' http://local.phacility.com/debug/
<pre>
HTTP_X_FORWARDED_FOR = 1.1.1.1, 2.2.2.2, 3.3.3.3, 4.4.4.4, 5.5.5.5, 6.6.6.6
FINAL REMOTE_ADDR = 3.3.3.3
</pre>
```
```
$ curl -H 'X-Forwarded-For: 5.5.5.5, 6.6.6.6' http://local.phacility.com/debug/
<pre>
HTTP_X_FORWARDED_FOR = 5.5.5.5, 6.6.6.6
FINAL REMOTE_ADDR = 5.5.5.5
</pre>
```
Maniphest Tasks: T13392
Differential Revision: https://secure.phabricator.com/D20785
Summary:
Ref T13244. See D20080. Rather than randomly jittering service calls, we can give each host a "metronome" that ticks every 60 seconds to get load to spread out after one cycle.
For example, web001 ticks (and makes a service call) when the second hand points at 0:17, web002 at 0:43, web003 at 0:04, etc.
For now I'm just planning to seed the metronomes randomly based on hostname, but we could conceivably give each host an assigned offset some day if we want perfectly smooth service call rates.
Test Plan: Ran unit tests.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20087
Summary:
Ref T13222. Ref T13225. We store a digest of the session key in the session table (not the session key itself) so that users with access to this table can't easily steal sessions by just setting their cookies to values from the table.
Users with access to the database can //probably// do plenty of other bad stuff (e.g., T13134 mentions digesting Conduit tokens) but there's very little cost to storing digests instead of live tokens.
We currently digest session keys with HMAC-SHA1. This is fine, but HMAC-SHA256 is better. Upgrade:
- Always write new digests.
- We still match sessions with either digest.
- When we read a session with an old digest, upgrade it to a new digest.
In a few months we can throw away the old code. When we do, installs that skip upgrades for a long time may suffer a one-time logout, but I'll note this in the changelog.
We could avoid this by storing `hmac256(hmac1(key))` instead and re-hashing in a migration, but I think the cost of a one-time logout for some tiny subset of users is very low, and worth keeping things simpler in the long run.
Test Plan:
- Hit a page with an old session, got a session upgrade.
- Reviewed sessions in Settings.
- Reviewed user logs.
- Logged out.
- Logged in.
- Terminated other sessions individually.
- Terminated all other sessions.
- Spot checked session table for general sanity.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13225, T13222
Differential Revision: https://secure.phabricator.com/D19883
Summary:
Ref T13202. See PHI889. If the lock log is enabled, we can try to offer more details about lock holders.
When we fail to acquire a lock:
- check for recent acquisitions and suggest that this is a bottleneck issue;
- if there are no recent acquisitions, check for the last acquisition and print details about it (what process, how long ago, whether or not we believe it was released).
Test Plan:
- Enabled the lock log.
- Changed the lock wait time to 1 second.
- Added a `sleep(10)` after grabbing the lock.
- In one window, ran a Conduit call or a `git fetch`.
- In another window, ran another operation.
- Got useful/sensible errors for both ssh and web lock holders, for example:
> PhutilProxyException: Failed to acquire read lock after waiting 1 second(s). You may be able to retry later. (This lock was most recently acquired by a process (pid=12609, host=orbital-3.local, sapi=apache2handler, controller=PhabricatorConduitAPIController, method=diffusion.rawdiffquery) 3 second(s) ago. There is no record of this lock being released.)
> PhutilProxyException: Failed to acquire read lock after waiting 1 second(s). You may be able to retry later. (This lock was most recently acquired by a process (pid=65251, host=orbital-3.local, sapi=cli, argv=/Users/epriestley/dev/core/lib/phabricator/bin/ssh-exec --phabricator-ssh-device local.phacility.net --phabricator-ssh-key 2) 2 second(s) ago. There is no record of this lock being released.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13202
Differential Revision: https://secure.phabricator.com/D19702
Summary: Depends on D19173. Ref T13096. Adds an optional, disabled-by-default lock log to make it easier to figure out what is acquiring and holding locks.
Test Plan: Ran `bin/lock log --enable`, `--disable`, `--name`, etc. Saw sensible-looking output with log enabled and daemons restarted. Saw no additional output with log disabled and daemons restarted.
Maniphest Tasks: T13096
Differential Revision: https://secure.phabricator.com/D19174
Summary:
Ref T13096. Currently, we do a fair amount of clever digesting and string manipulation to build lock names which are less than 64 characters long while still being reasonably readable.
Instead, do more of this automatically. This will let lock acquisition become simpler and make it more possible to build a useful lock log.
Test Plan: Ran `bin/repository update`, saw a reasonable lock acquire and release.
Maniphest Tasks: T13096
Differential Revision: https://secure.phabricator.com/D19173
Summary:
Ref T13045. See that task for discussion.
This replaces `digestForIndex()` with a "clever" algorithm in `digestForAnchor()`. The new digest is the same as `digestForIndex()` except when the original output was "." or "_". In those cases, a replacement character is selected based on entropy accumulated by the digest function as it iterates through the string.
Test Plan: Added unit tests.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13045
Differential Revision: https://secure.phabricator.com/D18909
Summary:
Ref T13043. In D18898 I moved VCS passwords to the new shared infrastructure.
Before account passwords can move, we need to make two changes:
- For legacy reasons, VCS passwords and Account passwords have different "digest" algorithms. Both are more complicated than they should be, but we can't easily fix it without breaking existing passwords. Add a `PasswordHashInterface` so that objects which can have passwords hashes can implement custom digest logic for each password type.
- Account passwords have a dedicated external salt (`PhabricatorUser->passwordSalt`). This is a generally reasonable thing to support (since not all hashers are self-salting) and we need to keep it around so existing passwords still work. Add salt support to `AuthPassword` and make it generate/regenerate when passwords are updated.
Then add a nice story about password digestion.
Test Plan: Ran migrations. Used an existing VCS password; changed VCS password. Tried to use a revoked password. Unit tests still pass. Grepped for callers to legacy `PhabricatorHash::digestPassword()`, found none.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18900
Summary: Noticed a couple of typos in the docs, and then things got out of hand.
Test Plan:
- Stared at the words until my eyes watered and the letters began to swim on the screen.
- Consulted a dictionary.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D18693
Summary: Ref T12509. This encourages code to move away from HMAC+SHA1 by making the method name more obviously undesirable.
Test Plan: `grep`, browsed around.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D17632
Summary:
Ref T12509. This adds support for HMAC+SHA256 (instead of HMAC+SHA1). Although HMAC+SHA1 is not currently broken in any sense, SHA1 has a well-known collision and it's good to look at moving away from HMAC+SHA1.
The new mechanism also automatically generates and stores HMAC keys.
Currently, HMAC keys largely use a per-install constant defined in `security.hmac-key`. In theory this can be changed, but in practice essentially no install changes it.
We generally (in fact, always, I think?) don't use HMAC digests in a way where it matters that this key is well-known, but it's slightly better if this key is unique per class of use cases. Principally, if use cases have unique HMAC keys they are generally less vulnerable to precomputation attacks where an attacker might generate a large number of HMAC hashes of well-known values and use them in a nefarious way. The actual threat here is probably close to nonexistent, but we can harden against it without much extra effort.
Beyond that, this isn't something users should really have to think about or bother configuring.
Test Plan:
- Added unit tests.
- Used `bin/files integrity` to verify, strip, and recompute hashes.
- Tampered with a generated HMAC key, verified it invalidated hashes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D17630
Summary:
Ref T12444. A few issues:
- `x % (y - z)` doesn't generate values in the full range: the largest value is never generated. Instead, use `x % (1 + y - z)`.
- `digestToRange(1, count)` never generates 0. After fixing the first bug, it could generate `count`. The range of the arrays is `0..(count-1)`, inclusive. Generate the correct range instead.
- `unpack('L', ...)` can unpack a negative number on a 32-bit system. Use `& 0x7FFFFFFF` to mask off the sign bit so the result is always a positive integer.
- FileFinder might return arbitrary keys, but we rely on sequential keys (0, 1, 2, ...)
Test Plan:
- Used `bin/people profileimage ... --force` to regenerate images.
- Added some debugging to verify that the math seemed to be working.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12444
Differential Revision: https://secure.phabricator.com/D17543
Summary: Ref T10319. This swaps the default in the Picture Chooser to allow picking of the custom unique avatar. We're currently going with 100k unique possibilities. The logic roughly hashes a user name and picks an image pack, color, and border. Based on that, we select the first character of their username, or fall back to Psyduck if not [a-z][0-9].
Test Plan:
Set the following usernames from ProfilePicture as a test: chad, epriestley, sally, 007, _cat_, -doggie-.
{F3453979}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10319
Differential Revision: https://secure.phabricator.com/D17430
Summary:
Ref T10860. This allows us to recover if the connection to the database is lost during a push.
If we lose the connection to the master database during a push, we would previously freeze the repository. This is very safe, but not very operator-friendly since you have to go manually unfreeze it.
We don't need to be quite this aggressive about freezing things. The repository state is still consistent after we've "upgraded" the lock by setting `isWriting = 1`, so we're actually fine even if we lost the global lock.
Instead of just freezing the repository immediately, sit there in a loop waiting for the master to come back up for a few minutes. If it recovers, we can release the lock and everything will be OK again.
Basically, the changes are:
- If we can't release the lock at first, sit in a loop trying really hard to release it for a while.
- Add a unique lock identifier so we can be certain we're only releasing //our// lock no matter what else is going on.
- Do the version reads on the same connection holding the lock, so we can be sure we haven't lost the lock before we do that read.
Test Plan:
- Added a `sleep(10)` after accepting the write but before releasing the lock so I could run `mysqld stop` and force this issue to occur.
- Pushed like this:
```
$ echo D >> record && git commit -am D && git push
[master 707ecc3] D
1 file changed, 1 insertion(+)
# Push received by "local001.phacility.net", forwarding to cluster host.
# Waiting up to 120 second(s) for a cluster write lock...
# Acquired write lock immediately.
# Waiting up to 120 second(s) for a cluster read lock on "local001.phacility.net"...
# Acquired read lock immediately.
# Device "local001.phacility.net" is already a cluster leader and does not need to be synchronized.
# Ready to receive on cluster host "local001.phacility.net".
Counting objects: 3, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (2/2), done.
Writing objects: 100% (3/3), 254 bytes | 0 bytes/s, done.
Total 3 (delta 1), reused 0 (delta 0)
BEGIN SLEEP
```
- Here, I stopped `mysqld` from the CLI in another terminal window.
```
END SLEEP
# CRITICAL. Failed to release cluster write lock!
# The connection to the master database was lost while receiving the write.
# This process will spend 300 more second(s) attempting to recover, then give up.
```
- Here, I started `mysqld` again.
```
# RECOVERED. Link to master database was restored.
# Released cluster write lock.
To ssh://local@localvault.phacility.com/diffusion/26/locktopia.git
2cbf87c..707ecc3 master -> master
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10860
Differential Revision: https://secure.phabricator.com/D15792
Summary: Ref T8509. We currently give you a fairly obtuse error when trying to name a project something like "!!". The error is correct, but not as helpful as it could be. Give users a more specific, more helpful error.
Test Plan: {F1042883}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8509
Differential Revision: https://secure.phabricator.com/D14872
Summary: Fixes T9715. Adds a MySQL-based lock to ensure that schema migrations are not applied on multiple hosts simultaneously.
Test Plan: Ran `./bin/storage upgrade` concurrently. One invocation was successful whilst the other hit a `PhutilLockException`.
Reviewers: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T9715
Differential Revision: https://secure.phabricator.com/D14463
Summary: Fixes T9573. This incorrectly affected Phriction. I could restore it for only projects, but you didn't like the rule very much anyway and I don't feel strongly about it.
Test Plan: Unit tests.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9573
Differential Revision: https://secure.phabricator.com/D14287
Summary:
Ref T9551. We currently use the same logic for generating project hashtags and Phriction slugs, but should be a little more conservative with project hashtags.
Stop them from generating with stuff that won't parse in a "Reviewers:" field or generally in commments (commas, colons, etc).
Test Plan:
Created a bunch of projects with nonsense in them and saw them generate pretty reasonable hashtags.
{F873456}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9551
Differential Revision: https://secure.phabricator.com/D14261
Summary:
Ref T9551. To set things up:
- Name a project `aa bb`. This will have the tag `aa_bb`.
- Try to visit `/tag/aa%20bb`.
Here's what happens now:
- You get an Aphront redirect error as it tries to add the trailing `/`. Add `phutil_escape_uri()` so that works again.
- Then, you 404, even though this tag is reasonably equivalent to the real project tag and could be redirected. Add a fallback to lookup, resolve, and redirect if we can find a hit for the tag.
This also fixes stuff like `/tag/AA_BB/`.
Test Plan: Visited URIs like `/tag/aa%20bb`, `/tag/aa%20bb/`, `/tag/Aa_bB/`, etc. None of them worked before and now they all do.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9551
Differential Revision: https://secure.phabricator.com/D14260
Summary: See D14025. In all cases where we compare hashes, use strict, constant-time comparisons.
Test Plan: Logged in, logged out, added TOTP, ran Conduit, terminated sessions, submitted forms, changed password. Tweaked CSRF token, got rejected.
Reviewers: chad
Reviewed By: chad
Subscribers: chenxiruanhai
Differential Revision: https://secure.phabricator.com/D14026
Summary: Use `PhutilClassMapQuery` where appropriate.
Test Plan: Browsed around the UI to verify things seemed somewhat working.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13429
Summary: All classes should extend from some other class. See D13275 for some explanation.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13283
Summary: Use `__CLASS__` instead of hard-coding class names. Depends on D12605.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12806
Summary:
Fixes T6958. Ref T7484.
- When we collide on a lock in `bin/repository update`, explain what that means.
- GlobalLock currently uses a "lock name" which is different from the lock's actual name. Don't do this. There's a small chance this fixes T7484, but I don't have high hopes.
Test Plan: Ran `bin/repository update X` in two windows really fast, got the new message in one of them.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6958, T7484
Differential Revision: https://secure.phabricator.com/D12574
Summary:
Fixes T7484. There's a bunch of spooky mystery here but the current behavior can probably cause problems in at least some situations.
Also moves a couple callsigns to monograms (see T4245).
Test Plan:
- Faked a short lock length to hit the exception.
- Updated normally.
- Grepped for other use sites, none seemed suspicious or likely to overflow the lock length.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7484
Differential Revision: https://secure.phabricator.com/D12263
Summary:
Fixes T5492. I figured this would be easier to just fix than write a guide for; it actually took me an hour, but I spent like 75% of that futzing with my editor.
- The Move controller currently accepts either a slug or an ID. I can't find any callsites which pass a slug, and this doesn't make sense. Pretty sure this was copy/pasted from Edit or something. Only accept IDs.
- Slightly modernize the Move controller (newDialog(), handleRequest(), $viewer).
- When the user enters a bad slug, warn them that we're going to fix it for them and let them accept or reject the changes.
- Don't prefill the edit note (this feels inconsistent/unusual).
- On the form, label the input "Path" instead of "URI".
- Show the old path, to help remind the user what the input should look like.
- When a user tries to do a no-op move, show a more tailored message.
- When the user tries to do an overwriting move, explain how they can fix it.
- When normalizing a slug like `/question/???/mark/`, make it normalize to `/question/_/mark`.
Test Plan:
- Tried to move a document to itself.
- Tried to overwrite a document.
- Did a bad-path move, accepted corrected path.
- Did a good-path move.
- Did a path move with a weird component like `/???/`.
- Added and executed unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5492
Differential Revision: https://secure.phabricator.com/D10838
Summary:
Fixes T5934. If you hash a password with, e.g., bcrypt, and then lose the bcrypt hasher for some reason, we currently fatal when trying to figure out if we can upgrade.
Instead, detect that the current hasher implementation has vanished and let the user reset their password (for account passwords) or choose a new one (for VCS passwords)>
Test Plan:
Account password:
- Artifically disabled bcrypt hasher.
- Viewed password panel, saw warnings about missing hasher.
- Used password reset workflow to change password, saw iterated MD5 hashed password get set.
- Enabled bcrypt hasher again.
- Saw upgrade warning.
- Upgraded password to bcrypt.
VCS password:
- Artificially disabled bcrypt hasher.
- Viewed password panel, saw warnings about missing hasher.
- Reset password.
- Saw iterated md5 password.
- Reenabled bcrypt.
- Upgraded to bcrypt.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5934
Differential Revision: https://secure.phabricator.com/D10325
Summary: Ran `arc lint --apply-patches --everything` over rP, mainly to change double quotes to single quotes where appropriate. These changes also validate that the `ArcanistXHPASTLinter::LINT_DOUBLE_QUOTE` rule is working as expected.
Test Plan: Eyeballed it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9431
Summary:
PHP 5.5 specifies constant PASSWORD_BCRYPT should be used in password_hash()
instead of CRYPT_BLOWFISH. Using CRYPT_BLOWFISH is not supported in either PHP
or HHVM. This constant breaks Username / Password authentication.
Test Plan:
Login using Username/Password with bcrypt hash. Before applying the patch,
No matter what password entered, it will always fail authentication. After this
patch, user should be able to login with bcrypt hash.
Reviewers: btrahan, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8808
Summary: Fixes T4614. These don't do anything bad or dangerous, but generate unusable pages.
Test Plan:
- Added and executed unit tests.
- Tried to create pages like `/../`, `/begin/../end/`, etc.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: aran, epriestley
Maniphest Tasks: T4614
Differential Revision: https://secure.phabricator.com/D8535
Summary: Ref T4587. Add an option to automatically generate a keypair, associate the public key, and save the private key.
Test Plan: Generated some keypairs. Hit error conditions, etc.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Maniphest Tasks: T4587
Differential Revision: https://secure.phabricator.com/D8513
Summary:
There are quite a few tests in Arcanist, libphutil and Phabricator that do something similar to `$this->assertEqual(false, ...)` or `$this->assertEqual(true, ...)`.
This is unnecessarily verbose and it would be cleaner if we had `assertFalse` and `assertTrue` methods.
Test Plan: I contemplated adding a unit test for the `getCallerInfo` method but wasn't sure if it was required / where it should live.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8460
Summary:
Fixes T4463. When your VCS or account password is not set, we test it for upgrade anyway. This doesn't make sense and throws shortly into the process because the empty hash isn't parseable.
Instead, only show upgrade prompts when the password exists.
Test Plan:
- Added a password to an existing account with no password via password reset.
- Added a VCS password to an existing account with no VCS password.
- Observed no fatals / nonsense behaviors.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T4463
Differential Revision: https://secure.phabricator.com/D8282
Summary: Fixes T4443. Plug VCS passwords into the shared key stretching. They don't use any real stretching now (I anticipated doing something like T4443 eventually) so we can just migrate them into stretching all at once.
Test Plan:
- Viewed VCS settings.
- Used VCS password after migration.
- Set VCS password.
- Upgraded VCS password by using it.
- Used VCS password some more.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4443
Differential Revision: https://secure.phabricator.com/D8272
Summary:
Ref T4443. In addition to performing upgrades from, e.g., md5 -> bcrypt, also allow sidegrades from, e.g., bcrypt(cost=11) to bcrypt(cost=12). This allows us to, for example, bump the cost function every 18 months and stay on par with Moore's law, on average.
I'm also allowing "upgrades" which technically reduce cost, but this seems like the right thing to do (i.e., generally migrate password storage so it's all uniform, on average).
Test Plan:
- Fiddled the bcrypt cost function and saw appropriate upgrade UI, and upgraded passwords upon password change.
- Passwords still worked.
- Around cost=13 or 14 things start getting noticibly slow, so bcrypt does actually work. Such wow.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4443
Differential Revision: https://secure.phabricator.com/D8271
Summary:
Ref T4443.
- Add a `password_hash()`-based bcrypt hasher if `password_hash()` is available.
- When a user logs in using a password, upgrade their password to the strongest available hash format.
- On the password settings page:
- Warn the user if their password uses any algorithm other than the strongest one.
- Show the algorithm the password uses.
- Show the best available algorithm.
Test Plan: As an md5 user, viewed password settings page and saw a warning. Logged out. Logged in, got upgraded, no more warning. Changed password, verified database rehash. Logged out, logged in.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4443
Differential Revision: https://secure.phabricator.com/D8270
Summary:
Ref T4443. Make hashing algorithms pluggable and extensible so we can deal with the attendant complexities more easily.
This moves "Iterated MD5" to a modular implementation, and adds a tiny bit of hack-glue so we don't need to migrate the DB in this patch. I'll migrate in the next patch, then add bcrypt.
Test Plan:
- Verified that the same stuff gets stored in the DB (i.e., no functional changes):
- Logged into an old password account.
- Changed password.
- Registered a new account.
- Changed password.
- Switched back to master.
- Logged in / out, changed password.
- Switched back, logged in.
- Ran unit tests (they aren't super extensive, but cover some of the basics).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, kofalt
Maniphest Tasks: T4443
Differential Revision: https://secure.phabricator.com/D8268
