Summary:
This probably needs some tweaks, but the idea is to make it easier to browse and access applications without necessarily needing them to be on the homepage.
Open to feedback.
Test Plan:
(This screenshot merges "Organization", "Communication" and "Core" into a single "Core" group. We can't actually do this yet because it wrecks the homepage.)
{F160052}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5176
Differential Revision: https://secure.phabricator.com/D9297
Summary:
Ref T4398. This code hadn't been touched in a while and had a few crufty bits.
**One Time Resets**: Currently, password reset (and similar links) are valid for about 48 hours, but we always use one token to generate them (it's bound to the account). This isn't horrible, but it could be better, and it produces a lot of false positives on HackerOne.
Instead, use TemporaryTokens to make each link one-time only and good for no more than 24 hours.
**Coupling of Email Verification and One-Time Login**: Currently, one-time login links ("password reset links") are tightly bound to an email address, and using a link verifies that email address.
This is convenient for "Welcome" emails, so the user doesn't need to go through two rounds of checking email in order to login, then very their email, then actually get access to Phabricator.
However, for other types of these links (like those generated by `bin/auth recover`) there's no need to do any email verification.
Instead, make the email verification part optional, and use it on welcome links but not other types of links.
**Message Customization**: These links can come out of several workflows: welcome, password reset, username change, or `bin/auth recover`. Add a hint to the URI so the text on the page can be customized a bit to help users through the workflow.
**Reset Emails Going to Main Account Email**: Previously, we would send password reset email to the user's primary account email. However, since we verify email coming from reset links this isn't correct and could allow a user to verify an email without actually controlling it.
Since the user needs a real account in the first place this does not seem useful on its own, but might be a component in some other attack. The user might also no longer have access to their primary account, in which case this wouldn't be wrong, but would not be very useful.
Mitigate this in two ways:
- First, send to the actual email address the user entered, not the primary account email address.
- Second, don't let these links verify emails: they're just login links. This primarily makes it more difficult for an attacker to add someone else's email to their account, send them a reset link, get them to login and implicitly verify the email by not reading very carefully, and then figure out something interesting to do (there's currently no followup attack here, but allowing this does seem undesirable).
**Password Reset Without Old Password**: After a user logs in via email, we send them to the password settings panel (if passwords are enabled) with a code that lets them set a new password without knowing the old one.
Previously, this code was static and based on the email address. Instead, issue a one-time code.
**Jump Into Hisec**: Normally, when a user who has multi-factor auth on their account logs in, we prompt them for factors but don't put them in high security. You usually don't want to go do high-security stuff immediately after login, and it would be confusing and annoying if normal logins gave you a "YOU ARE IN HIGH SECURITY" alert bubble.
However, if we're taking you to the password reset screen, we //do// want to put the user in high security, since that screen requires high security. If we don't do this, the user gets two factor prompts in a row.
To accomplish this, we set a cookie when we know we're sending the user into a high security workflow. This cookie makes login finalization upgrade all the way from "partial" to "high security", instead of stopping halfway at "normal". This is safe because the user has just passed a factor check; the only reason we don't normally do this is to reduce annoyance.
**Some UI Cleanup**: Some of this was using really old UI. Modernize it a bit.
Test Plan:
- **One Time Resets**
- Used a reset link.
- Tried to reuse a reset link, got denied.
- Verified each link is different.
- **Coupling of Email Verification and One-Time Login**
- Verified that `bin/auth`, password reset, and username change links do not have an email verifying URI component.
- Tried to tack one on, got denied.
- Used the welcome email link to login + verify.
- Tried to mutate the URI to not verify, or verify something else: got denied.
- **Message Customization**
- Viewed messages on the different workflows. They seemed OK.
- **Reset Emails Going to Main Account Email**
- Sent password reset email to non-primary email.
- Received email at specified address.
- Verified it does not verify the address.
- **Password Reset Without Old Password**
- Reset password without knowledge of old one after email reset.
- Tried to do that without a key, got denied.
- Tried to reuse a key, got denied.
- **Jump Into Hisec**
- Logged in with MFA user, got factor'd, jumped directly into hisec.
- Logged in with non-MFA user, no factors, normal password reset.
- **Some UI Cleanup**
- Viewed new UI.
- **Misc**
- Created accounts, logged in with welcome link, got verified.
- Changed a username, used link to log back in.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4398
Differential Revision: https://secure.phabricator.com/D9252
Summary:
Ref T4398. We have several auth-related systems which require (or are improved by) the ability to hand out one-time codes which expire after a short period of time.
In particular, these are:
- SMS multi-factor: we need to be able to hand out one-time codes for this in order to prove the user has the phone.
- Password reset emails: we use a time-based rotating token right now, but we could improve this with a one-time token, so once you reset your password the link is dead.
- TOTP auth: we don't need to verify/invalidate keys, but can improve security by doing so.
This adds a generic one-time code storage table, and strengthens the TOTP enrollment process by using it. Specifically, you can no longer edit the enrollment form (the one with a QR code) to force your own key as the TOTP key: only keys Phabricator generated are accepted. This has no practical security impact, but generally helps raise the barrier potential attackers face.
Followup changes will use this for reset emails, then implement SMS multi-factor.
Test Plan:
- Enrolled in TOTP multi-factor auth.
- Submitted an error in the form, saw the same key presented.
- Edited the form with web tools to provide a different key, saw it reject and the server generate an alternate.
- Change the expiration to 5 seconds instead of 1 hour, submitted the form over and over again, saw it cycle the key after 5 seconds.
- Looked at the database and saw the tokens I expected.
- Ran the GC and saw all the 5-second expiry tokens get cleaned up.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4398
Differential Revision: https://secure.phabricator.com/D9217
Summary: This is useful when you're trying to onboard an entire office and you end up using the Google OAuth anyway.
Test Plan: tested locally. Maybe I should write some tests?
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9150
Summary: Did a more exhaustive grep on setIcon and found 99.9% of the icons.
Test Plan: I verified icon names on UIExamples, but unable to test some of the more complex flows visually. Mostly a read and replace.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9088
Summary: The removes the sprite sheet 'icons' and replaces it with FontAwesome fonts.
Test Plan:
- Grep for SPRITE_ICONS and replace
- Grep for sprite-icons and replace
- Grep for PhabricatorActionList and choose all new icons
- Grep for Crumbs and fix icons
- Test/Replace PHUIList Icon support
- Test/Replace ObjectList Icon support (foot, epoch, etc)
- Browse as many pages as I could get to
- Remove sprite-icons and move remarkup to own sheet
- Review this diff in Differential
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9052
Summary: Fixes T4728, first pass, Make real name optional on user accounts
Test Plan: Default real name config should be false (not required). Create new user, real name should not be required. Toggle config, real name should be required. Users with no real name should be always listed by their usernames.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4728
Differential Revision: https://secure.phabricator.com/D9027
Summary:
This plugin provides an OAuth authentication provider to authenticate users using WordPress.com Connect.
This diff corresponds to github pull request https://github.com/facebook/phabricator/pull/593/ and had its libphutil counterpart reviewed in D9004.
Test Plan: Configured WordPress.com as an authentication provider, saw it show up on the login screen, registered a new account, got expected defaults for my username/name/email/profile picture.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9019
Summary: Ref T4398. Add some documentation and use `phutil_units()`.
Test Plan:
- Established a web session.
- Established a conduit session.
- Entered and exited hisec.
- Used "Sessions" panel to examine results.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4398
Differential Revision: https://secure.phabricator.com/D8924
Summary: Ref T4398. I found a reasonable-ish LGPLv3 library for doing this, which isn't too huge or unwieldy.
Test Plan:
- Scanned QR code with Authy.
- Scanned QR code with Google Authenticator.
{F149317}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4398
Differential Revision: https://secure.phabricator.com/D8923
Summary:
Ref T4398. This prompts users for multi-factor auth on login.
Roughly, this introduces the idea of "partial" sessions, which we haven't finished constructing yet. In practice, this means the session has made it through primary auth but not through multi-factor auth. Add a workflow for bringing a partial session up to a full one.
Test Plan:
- Used Conduit.
- Logged in as multi-factor user.
- Logged in as no-factor user.
- Tried to do non-login-things with a partial session.
- Reviewed account activity logs.
{F149295}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4398
Differential Revision: https://secure.phabricator.com/D8922
Summary:
Ref T4843. This adds support to `javelin_tag()` for an `aural` attribute. When specified, `true` values mean "this content is aural-only", while `false` values mean "this content is not aural".
- I've attempted to find the best modern approaches for marking this content, but the `aural` attribute should let us change the mechanism later.
- Make the "beta" markers on application navigation visual only (see T4843). This information is of very low importance, the application navigation is accessed frequently, and the information is available on the application list.
- Partially convert the main navigation. This is mostly to test things, since I want to get more concrete feedback about approaches here.
- Add a `?__aural__=1` attribute, which renders the page with aural-only elements visible and visual-only elements colored.
Test Plan: {F146476}
Reviewers: btrahan, scp, chad
Reviewed By: chad
Subscribers: aklapper, qgil, epriestley
Maniphest Tasks: T4843
Differential Revision: https://secure.phabricator.com/D8830
Summary: Ref T4398. Prevent users from brute forcing multi-factor auth by rate limiting attempts. This slightly refines the rate limiting to allow callers to check for a rate limit without adding points, and gives users credit for successfully completing an auth workflow.
Test Plan: Tried to enter hisec with bad credentials 11 times in a row, got rate limited.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4398
Differential Revision: https://secure.phabricator.com/D8911
Summary:
Ref T4398. The major goals here is to let administrators strip auth factors in two cases:
- A user lost their phone and needs access restored to their account; or
- an install previously used an API-based factor like SMS, but want to stop supporting it (this isn't possible today).
Test Plan:
- Used `bin/auth list-factors` to show installed factors.
- Used `bin/auth strip` with various mixtures of flags to selectively choose and strip factors from accounts.
- Also ran `bin/auth refresh` to verify refreshing OAuth tokens works (small `OAuth` vs `OAuth2` tweak).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4398
Differential Revision: https://secure.phabricator.com/D8909
Summary:
Ref T4398. Allows auth factors to render and validate when prompted to take a hi-sec action.
This has a whole lot of rough edges still (see D8875) but does fundamentally work correctly.
Test Plan:
- Added two different TOTP factors to my account for EXTRA SECURITY.
- Took hisec actions with no auth factors, and with attached auth factors.
- Hit all the error/failure states of the hisec entry process.
- Verified hisec failures appear in activity logs.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4398
Differential Revision: https://secure.phabricator.com/D8886
Summary:
Ref T4398. This is still pretty rough and isn't exposed in the UI yet, but basically works. Some missing features / areas for improvement:
- Rate limiting attempts (see TODO).
- Marking tokens used after they're used once (see TODO), maybe. I can't think of ways an attacker could capture a token without also capturing a session, offhand.
- Actually turning this on (see TODO).
- This workflow is pretty wordy. It would be nice to calm it down a bit.
- But also add more help/context to help users figure out what's going on here, I think it's not very obvious if you don't already know what "TOTP" is.
- Add admin tool to strip auth factors off an account ("Help, I lost my phone and can't log in!").
- Add admin tool to show users who don't have multi-factor auth? (so you can pester them)
- Generate QR codes to make the transfer process easier (they're fairly complicated).
- Make the "entering hi-sec" workflow actually check for auth factors and use them correctly.
- Turn this on so users can use it.
- Adding SMS as an option would be nice eventually.
- Adding "password" as an option, maybe? TOTP feels fairly good to me.
I'll post a couple of screens...
Test Plan:
- Added TOTP token with Google Authenticator.
- Added TOTP token with Authy.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4398
Differential Revision: https://secure.phabricator.com/D8875
Summary:
Ref T4398. This adds a settings panel for account activity so users can review activity on their own account. Some goals are:
- Make it easier for us to develop and support auth and credential information, see T4398. This is the primary driver.
- Make it easier for users to understand and review auth and credential information (see T4842 for an example -- this isn't there yet, but builds toward it).
- Improve user confidence in security by making logging more apparent and accessible.
Minor corresponding changes:
- Entering and exiting hisec mode is now logged.
- This, sessions, and OAuth authorizations have moved to a new "Sessions and Logs" area, since "Authentication" was getting huge.
Test Plan:
- Viewed new panel.
- Viewed old UI.
- Entered/exited hisec and got prompted.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4398
Differential Revision: https://secure.phabricator.com/D8871
Summary:
Ref T4398. This is roughly a "sudo" mode, like GitHub has for accessing SSH keys, or Facebook has for managing credit cards. GitHub actually calls theirs "sudo" mode, but I think that's too technical for big parts of our audience. I've gone with "high security mode".
This doesn't actually get exposed in the UI yet (and we don't have any meaningful auth factors to prompt the user for) but the workflow works overall. I'll go through it in a comment, since I need to arrange some screenshots.
Test Plan: See guided walkthrough.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4398
Differential Revision: https://secure.phabricator.com/D8851
Summary: Throwing this up for testing, swapped out all icons in timeline for their font equivelants. Used better icons where I could as well. We should feel free to use more / be fun with the icons when possible since there is no penalty anymore.
Test Plan: I browsed many, not all, timelines in my sandbox and in IE8. Some of these were just swagged, but I'm expecting we'll do more SB testing before landing.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8827
Summary: Fixes T4755. This also includes putting in a note that Google might ToS you to use the Google+ API. Lots of code here as there was some repeated stuff between OAuth1 and OAuth2 so I made a base OAuth with less-base OAuth1 and OAuth2 inheriting from it. The JIRA provider remains an independent mess and didn't get the notes field thing.
Test Plan: looked at providers and read pretty instructions.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4755
Differential Revision: https://secure.phabricator.com/D8726
Summary: Fixes T3208. This forces us to bind+search even if there are no anonymous credentials.
Test Plan: Checked the box, saved the form. Unchecked the box, saved the form. LDAP??
Reviewers: Firehed
Reviewed By: Firehed
Subscribers: epriestley
Maniphest Tasks: T3208
Differential Revision: https://secure.phabricator.com/D8723
Summary: Fixes T4451. See also D8612.
Test Plan: Viewed panel and read text, saw it matched up with the new console.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4451
Differential Revision: https://secure.phabricator.com/D8613
Summary:
- Dialog pages currently have no titles or crumbs, and look shoddy. Add titles and crumbs.
- Dialog titles aren't always great for crumbs, add an optional "short title" for crumbs.
- `AphrontDialogResponse` is pure boilerplate. Allow controllers to just return a `DialogView` instead and get the same effect.
- Building dialogs requires a bit of boilerplate, and we generally construct them with no explicit `"action"`, which has some issues with T4593. Provide a convenience method to set the viewer and get a reasonable, explict submit URI.
Test Plan:
- Viewed dialog on its own.
- Viewed dialog as a dialog.
{F132353}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8577
Summary:
This is partly a good feature, and partly should reduce false positives on HackerOne reporting things vaguely related to this.
Allow a user to terminate login sessions from the settings panel.
Test Plan:
- Terminated a session.
- Terminated all sessions.
- Tried to terminate all sessions again.
- Logged in with two browsers, terminated the other browser's session, reloaded, got kicked out.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8556
Summary:
Fixes T3471. Specific issues:
- Add the ability to set a temporary cookie (expires when the browser closes).
- We overwrote 'phcid' on every page load. This creates some issues with browser extensions. Instead, only write it if isn't set. To counterbalance this, make it temporary.
- Make the 'next_uri' cookie temporary.
- Make the 'phreg' cookie temporary.
- Fix an issue where deleted cookies would persist after 302 (?) in some cases (this is/was 100% for me locally).
Test Plan:
- Closed my browser, reopned it, verified temporary cookies were gone.
- Logged in, authed, linked, logged out.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3471
Differential Revision: https://secure.phabricator.com/D8537
Summary:
See <https://github.com/facebook/phabricator/issues/541>.
- If a provider returns the email `""` or `"0"`, we currently don't let the user edit it and thus don't let them register.
- If a provider returns an invalid email like `"!!!"` (permitted by GitHub, e.g.), we show them a nonsense error message.
Instead:
- Pretend we didn't get an address if we get an invalid address.
- Test the address strictly against `null`.
Test Plan: Registered on Phabricator with my GitHub email set to `""` (empty string) and `"!!!"` (bang bang bang).
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: aran, epriestley
Differential Revision: https://secure.phabricator.com/D8528
Summary:
Via HackerOne. In regular expressions, "$" matches "end of input, or before terminating newline". This means that the expression `/^A$/` matches two strings: `"A"`, and `"A\n"`.
When we care about this, use `\z` instead, which matches "end of input" only.
This allowed registration of `"username\n"` and similar.
Test Plan:
- Grepped codebase for all calls to `preg_match()` / `preg_match_all()`.
- Fixed the ones where this seemed like it could have an impact.
- Added and executed unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Differential Revision: https://secure.phabricator.com/D8516
Summary:
Ref T4593. There are a variety of clever attacks against OAuth which involve changing the redirect URI to some other URI on the same domain which exhibits unexpected behavior in response to an OAuth request. The best approach to dealing with this is for providers to lock to a specific path and refuse to redirect elsewhere, but not all providers do this.
We haven't had any specific issues related to this, but the anchor issue in T4593 was only a step away.
To mitigate this in general, we can reject the OAuth2 `'code'` parameter on //every// page by default, and then whitelist it on the tiny number of controllers which should be able to receive it.
This is very coarse, kind of overkill, and has some fallout (we can't use `'code'` as a normal parameter in the application), but I think it's relatively well-contained and seems reasonable. A better approach might be to whitelist parameters on every controller (i.e., have each controller specify the parameters it can receive), but that would be a ton of work and probably cause a lot of false positives for a long time.
Since we don't use `'code'` normally anywhere (as far as I can tell), the coarseness of this approach seems reasonable.
Test Plan:
- Logged in with OAuth.
- Hit any other page with `?code=...` in the URL, got an exception.
- Grepped for `'code'` and `"code"`, and examined each use to see if it was impacted.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Maniphest Tasks: T4593
Differential Revision: https://secure.phabricator.com/D8499
Summary: Although the defaults don't require a verified email address, it's easy to lock yourself out by accident by configuring `auth.require-email-verification` or `auth.email-domains` before setting up email. Just force-verify the initial/setup account's address.
Test Plan: Went through setup on a fresh install, saw address verify.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8365
Summary: If an attacker somehow intercepts a verification URL for an email address, they can hypothetically CSRF the account owner into verifying it. What you'd do before (how do you get the link?) and after (why do you care that you tricked them into verifying) performing this attack is unclear, but in theory we should require a CSRF submission here; add one.
Test Plan: {F118691}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8351
Summary: If you copy the registration URL, then register, then load the URL again while logged out (i.e., attempt to reuse the registration URL), we try to show you a tailored error message. However, this call is not correct so we show you a not-so tailored exception instead.
Test Plan:
- Get to the registration screen.
- Save URL.
- Complete registration.
- Log out.
- Return to saved URL.
Previously, exception. Now, readable error.
{F117585}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8322
Summary:
OAuth1 doesn't have anything like the `state` parameter, and I overlooked that we need to shove one in there somewhere. Append it to the callback URI. This functions like `state` in OAuth2.
Without this, an attacker can trick a user into logging into Phabricator with an account the attacker controls.
Test Plan:
- Logged in with JIRA.
- Logged in with Twitter.
- Logged in with Facebook (an OAuth2 provider).
- Linked a Twitter account.
- Linked a Facebook account.
- Jiggered codes in URIs and verified that I got the exceptions I expected.
Reviewers: btrahan, arice
Reviewed By: arice
CC: arice, chad, aran
Differential Revision: https://secure.phabricator.com/D8318
Summary:
Via HackerOne. An attacker can bypass `auth.email-domains` by registering with an email like:
aaaaa...aaaaa@evil.com@company.com
We'll validate the full string, then insert it into the database where it will be truncated, removing the `@company.com` part. Then we'll send an email to `@evil.com`.
Instead, reject email addresses which won't fit in the table.
`STRICT_ALL_TABLES` stops this attack, I'm going to add a setup warning encouraging it.
Test Plan:
- Set `auth.email-domains` to `@company.com`.
- Registered with `aaa...aaa@evil.com@company.com`. Previously this worked, now it is rejected.
- Did a valid registration.
- Tried to add `aaa...aaaa@evil.com@company.com` as an email address. Previously this worked, now it is rejected.
- Did a valid email add.
- Added and executed unit tests.
Reviewers: btrahan, arice
Reviewed By: arice
CC: aran, chad
Differential Revision: https://secure.phabricator.com/D8308
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
Summary: This uses the slightly smaller icons. Not sure about the logout icon, will play with it more in the morning.
Test Plan: tested new nav on desktop and mobile.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8119
Summary:
Ref T3623. This is like a pre-v0, in that it doesn't have a dropdown yet.
Clicking the button takes you to a page which can serve as a right click / mobile / edit target in the long run, but is obviously not great for desktop use. I'll add the dropdown in the next iteration.
Test Plan: {F105631}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T3623
Differential Revision: https://secure.phabricator.com/D8088
Summary:
Via HackerOne, there are two related low-severity issues with this workflow:
- We don't check if you're already logged in, so an attacker can trick a victim (whether they're logged in or not) into clicking a reset link for an account the attacker controls (maybe via an invisible iframe) and log the user in under a different account.
- We don't check CSRF tokens either, so after fixing the first thing, an attacker can still trick a //logged-out// victim in the same way.
It's not really clear that doing this opens up any significant attacks afterward, but both of these behaviors aren't good.
I'll probably land this for audit in a few hours if @btrahan doesn't have a chance to take a look at it since he's probably on a plane for most of the day, I'm pretty confident it doesn't break anything.
Test Plan:
- As a logged-in user, clicked another user's password reset link and was not logged in.
- As a logged-out user, clicked a password reset link and needed to submit a form to complete the workflow.
Reviewers: btrahan
CC: chad, btrahan, aran
Differential Revision: https://secure.phabricator.com/D8079
Summary: Ref T4339. We didn't previously check `isFormPost()` on these, but now should.
Test Plan: Changed csrf token on login, got kicked out.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T4339
Differential Revision: https://secure.phabricator.com/D8051
Summary:
Fixes T3793. There's a lot of history here, see D4012, T2102. Basically, the problem is that things used to work like this:
- User is logged out and accesses `/xyz/`. After they login, we'd like to send them back to `/xyz/`, so we set a `next_uri` cookie.
- User's browser has a bunch of extensions and now makes a ton of requests for stuff that doesn't exist, like `humans.txt` and `apple-touch-icon.png`. We can't distinguish between these requests and normal requests in a general way, so we write `next_uri` cookies, overwriting the user's intent (`/xyz/`).
To fix this, we made the 404 page not set `next_uri`, in D4012. So if the browser requests `humans.txt`, we 404 with no cookie, and the `/xyz/` cookie is preserved. However, this is bad because an attacker can determine if objects exist and applications are installed, by visiting, e.g., `/T123` and seeing if they get a 404 page (resource really does not exist) or a login page (resource exists). We'd rather not leak this information.
The comment in the body text describes this in more detail.
This diff sort of tries to do the right thing most of the time: we write the cookie only if we haven't written it in the last 2 minutes. Generally, this should mean that the original request to `/xyz/` writes it, all the `humans.txt` requests don't write it, and things work like users expect. This may occasionally do the wrong thing, but it should be very rare, and we stop leaking information about applications and objects.
Test Plan: Logged out, clicked around / logged in, used Charles to verify that cookies were set in the expected way.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3793
Differential Revision: https://secure.phabricator.com/D8047
Summary: Fixes T4339. If you're anonymous, we use a digest of your session key to generate a CSRF token. Otherwise, everything works normally.
Test Plan: Logged out, logged in, tweaked CSRF in forms -- I'll add some inlines.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4339
Differential Revision: https://secure.phabricator.com/D8046
Summary:
Ref T4339. Login providers use absolute URIs, but the ones that rely on local form submits should not, because we want to include CSRF tokens where applicable.
Instead, make the default be relative URIs and turn them into absolute ones for the callback proivders.
Test Plan: Clicked, like, every login button.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4339
Differential Revision: https://secure.phabricator.com/D8045
Summary:
Ref T4339. Ref T4310. Currently, sessions look like `"afad85d675fda87a4fadd54"`, and are only issued for logged-in users. To support logged-out CSRF and (eventually) external user sessions, I made two small changes:
- First, sessions now have a "kind", which is indicated by a prefix, like `"A/ab987asdcas7dca"`. This mostly allows us to issue session queries more efficiently: we don't have to issue a query at all for anonymous sessions, and can join the correct table for user and external sessions and save a query. Generally, this gives us more debugging information and more opportunity to recover from issues in a user-friendly way, as with the "invalid session" error in this diff.
- Secondly, if you load a page and don't have a session, we give you an anonymous session. This is just a secret with no special significance.
This does not implement CSRF yet, but gives us a client secret we can use to implement it.
Test Plan:
- Logged in.
- Logged out.
- Browsed around.
- Logged in again.
- Went through link/register.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4310, T4339
Differential Revision: https://secure.phabricator.com/D8043
Summary: Ref T4339. We have more magical cookie names than we should, move them all to a central location.
Test Plan: Registered, logged in, linked account, logged out. See inlines.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4339
Differential Revision: https://secure.phabricator.com/D8041
Summary:
Fixes T4143. This mitigates the "use a botnet to slowly try to login to every user account using the passwords '1234', 'password', 'asdfasdf', ..." attack, like the one that hit GitHub.
(I also donated some money to Openwall as a thanks for compiling this wordlist.)
Test Plan:
- Tried to register with a weak password; registered with a strong password.
- Tried to set VCS password to a weak password; set VCS password to a strong password.
- Tried to change password to a weak password; changed password to a strong password.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T4143
Differential Revision: https://secure.phabricator.com/D8048
