Summary: Since this element isn't strictly about errors, re-label as info view instead.
Test Plan: Grepped for all callsites, tested UIExamples and a few other random pages.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11867
Summary:
Ref T4340. The attack this prevents is:
- An adversary penetrates your network. They acquire one of two capabilities:
- Your server is either configured to accept both HTTP and HTTPS, and they acquire the capability to observe HTTP traffic.
- Or your server is configured to accept only HTTPS, and they acquire the capability to control DNS or routing. In this case, they start a proxy server to expose your secure service over HTTP.
- They send you a link to `http://secure.service.com` (note HTTP, not HTTPS!)
- You click it since everything looks fine and the domain is correct, not noticing that the "s" is missing.
- They read your traffic.
This is similar to attacks where `https://good.service.com` is proxied to `https://good.sorvace.com` (i.e., a similar looking domain), but can be more dangerous -- for example, the browser will send (non-SSL-only) cookies and the attacker can write cookies.
This header instructs browsers that they can never access the site over HTTP and must always use HTTPS, defusing this class of attack.
Test Plan:
- Configured HTTPS locally.
- Accessed site over HTTP (got application redirect) and HTTPS.
- Enabled HSTS.
- Accessed site over HTTPS (to set HSTS).
- Tore down HTTPS part of the server and tried to load the site over HTTP. Browser refused to load "http://" and automatically tried to load "https://". In another browser which had not received the "HSTS" header, loading over HTTP worked fine.
- Brought the HTTPS server back up, things worked fine.
- Turned off the HSTS config setting.
- Loaded a page (to set HSTS with expires 0, diabling it).
- Tore down the HTTPS part of the server again.
- Tried to load HTTP.
- Now it worked.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4340
Differential Revision: https://secure.phabricator.com/D11820
Summary: Fixes T7273. This shows a better title (like "No Such Instance") instead of a generic one ("Unhandled Exception") when the user hits an AphrontUsageException.
Test Plan: Visited a nonexistent instance, got a nice title.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7273
Differential Revision: https://secure.phabricator.com/D11771
Summary: Clean up the error view styling.
Test Plan:
Tested as many as I could find, built additional tests in UIExamples
{F280452}
{F280453}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11605
Summary:
Ref T2086. Ref T7014. With the persistent column, there is significant value in retaining chrome state through navigation events, because the user may have a lot of state in the chat window (scroll position, text selection, room juggling, partially entered text, etc). We can do this by capturing navigation events and faking them with Javascript.
(This can also improve performance, albeit slightly, and I believe there are better approaches to tackle performance any problems which exist with the chrome in many cases).
At Facebook, this system was "Photostream" in photos and then "Quickling" in general, and the technical cost of the system was //staggering//. I am loathe to pursue it again. However:
- Browsers are less junky now, and we target a smaller set of browsers. A large part of the technical cost of Quickling was the high complexity of emulating nagivation events in IE, where we needed to navigate a hidden iframe to make history entries. All desktop browsers which we might want to use this system on support the History API (although this prototype does not yet implement it).
- Javelin and Phabricator's architecture are much cleaner than Facebook's was. A large part of the technical cost of Quickling was inconsistency, inlined `onclick` handlers, and general lack of coordination and abstraction. We will have //some// of this, but "correctly written" behaviors are mostly immune to it by design, and many of Javelin's architectural decisions were influenced by desire to avoid issues we encountered building this stuff for Facebook.
- Some of the primitives which Quickling required (like loading resources over Ajax) have existed in a stable state in our codebase for a year or more, and adoption of these primitives was trivial and uneventful (vs a huge production at Facebook).
- My hubris is bolstered by recent success with WebSockets and JX.Scrollbar, both of which I would have assessed as infeasibly complex to develop in this project a few years ago.
To these points, the developer cost to prototype Photostream was several weeks; the developer cost to prototype this was a bit less than an hour. It is plausible to me that implementing and maintaining this system really will be hundreds of times less complex than it was at Facebook.
Test Plan:
My plan for this and D11497 is:
- Get them in master.
- Some secret key / relatively-hidden preference activates the column.
- Quicksand activates //only// when the column is open.
- We can use column + quicksand for a long period of time (i.e., over the course of Conpherence v2 development) and hammer out the long tail of issues.
- When it derps up, you just hide the column and you're good to go.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T2086, T7014
Differential Revision: https://secure.phabricator.com/D11507
Summary:
Ref T7019. When we receive a `git clone https://` (or `git push` on HTTP/S), and the repository is not local, proxy the request to the appropriate service.
This has scalability limits, but they are not more severe than the existing limits (T4369) and are about as abstracted as we can get them.
This doesn't fully work in a Phacility context because the commit hook does not know which instance it is running in, but that problem is not unique to HTTP.
Test Plan:
- Pushed and pulled a Git repo via proxy.
- Pulled a Git repo normally.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7019
Differential Revision: https://secure.phabricator.com/D11494
Summary:
Ref T7019. Adds a new response which can proxy an HTTP request and pass the result through.
This is grossly inefficient for the same reasons as HTTP hosting is generally inefficient right now (T4369). This stuff is fixable but not trivial.
Test Plan: Replaced home page with a proxy to `example.org`, used Charles to view headers, saw the page headers and content proxy with an X-Phabricator-Proxied header.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7019
Differential Revision: https://secure.phabricator.com/D11492
Summary: If a cookie prefix is set (as on the Phacility cluster), we end up double-namespacing cookies when trying to remove them. This can make logging out produce a cookie error.
Test Plan: Logged out locally with cookie prefix, got normal logout workflow.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11282
Summary:
Ref T2783. Ref T6706.
- Add `cluster.addresses`. This is a whitelist of CIDR blocks which define cluster hosts.
- When we recieve a request that has a cluster-based authentication token, require the cluster to be configured and require the remote address to be a cluster member before we accept it.
- This provides a general layer of security for these mechanisms.
- In particular, it means they do not work by default on unconfigured hosts.
- When cluster addresses are configured, and we receive a request //to// an address not on the list, reject it.
- This provides a general layer of security for getting the Ops side of cluster configuration correct.
- If cluster nodes have public IPs and are listening on them, we'll reject requests.
- Basically, this means that any requests which bypass the LB get rejected.
Test Plan:
- With addresses not configured, tried to make requests; rejected for using a cluster auth mechanism.
- With addresses configred wrong, tried to make requests; rejected for sending from (or to) an address outside of the cluster.
- With addresses configured correctly, made valid requests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6706, T2783
Differential Revision: https://secure.phabricator.com/D11159
Summary:
Fixes T6692. Addresses two main issues:
- The write guard would sometimes not get disposed of on exception pathways, generating an unnecessary secondary error which was just a symptom of the original root error.
- This was generally confusing and reduced the quality of reports we received because users would report the symptomatic error sometimes instead of the real error.
- Instead, reflow the handling so that we always dispose of the write guard if we create one.
- If we missed the Controller-level error page generation (normally, a nice page with full CSS, etc), we'd jump straight to Startup-level error page generation (very basic plain text).
- A large class of errors occur too early or too late to be handled by Controller-level pages, but many of these errors are not fundamental, and the plain text page is excessively severe.
- Provide a mid-level simple HTML error page for errors which can't get full CSS, but also aren't so fundamental that we have no recourse but plain text.
Test Plan:
Mid-level errors now produce an intentional-looking error page:
{F259885}
Verified that setup errors still render properly.
@chad, feel free to tweak the exception page -- I just did a rough pass on it. Like the setup error stuff, it doesn't have Celerity, so we can't use `{$colors}` and no other CSS will be loaded.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley, chad
Maniphest Tasks: T6692
Differential Revision: https://secure.phabricator.com/D11126
Summary:
Ref T2783. ConduitCall currently has logic to pick a random remote server, but this is ultimately not appropriate: we always want to send requests to a specific server. For example, we want to send repository requests to a server which has that repository locally. The repository tier is not homogenous, so we can't do this below the call level.
Make ConduitCall always-local; logic above it will select ConduitCall for an in-process request or do service selection for an off-host request via ConduitClient.
Test Plan:
- Browsed some pages using ConduitCall, everything worked.
- Grepped for removed stuff.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D10959
Summary:
Ref T5702. This is a forward-looking change which provides some very broad API improvements but does not implement them. In particular:
- Controllers no longer require `$request` to construct. This is mostly for T5702, directly, but simplifies things in general. Instead, we call `setRequest()` before using a controller. Only a small number of sites activate controllers, so this is less code overall, and more consistent with most constructors not having any parameters or effects.
- `$request` now offers `getURIData($key, ...)`. This is an alternate way of accessing `$data` which is currently only available on `willProcessRequest(array $data)`. Almost all controllers which implement this method do so in order to read one or two things out of the URI data. Instead, let them just read this data directly when processing the request.
- Introduce `handleRequest(AphrontRequest $request)` and deprecate (very softly) `processRequest()`. The majority of `processRequest()` calls begin `$request = $this->getRequest()`, which is avoided with the more practical signature.
- Provide `getViewer()` on `$request`, and a convenience `getViewer()` on `$controller`. This fixes `$viewer = $request->getUser();` into `$viewer = $request->getViewer();`, and converts the `$request + $viewer` two-liner into a single `$this->getViewer()`.
Test Plan:
- Browsed around in general.
- Hit special controllers (redirect, 404).
- Hit AuditList controller (uses new style).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5702
Differential Revision: https://secure.phabricator.com/D10698
Summary: Ref T5702. This primarily gets URI routing out of Aphront and into an Application, for consistency.
Test Plan: Loaded some pages, got static resources.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5702
Differential Revision: https://secure.phabricator.com/D10696
Summary: Ref T5702. Primarily, this gets the custom DarkConsole URI routes out of the Aphront core and into an Application, like almost all other routes.
Test Plan: Used DarkConsole.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5702
Differential Revision: https://secure.phabricator.com/D10695
Summary:
Resolves T5937. HTTPS redirects caused by `security.require-https` use a full scheme, domain and port in the URI. Consequently, this causes invocation of the new external redirect logic and prevents redirection from occurring properly when accessing the HTTP version of Phabricator that has `security.require-https` turned on.
I've also fixed the automatic slash redirection logic to add the external flag where appropriate.
Test Plan: Configured SSL on my local machine and turned on `security.require-https`. Observed the "Refusing to redirect" exception on master, while the redirect completed successfully with this patch.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5937
Differential Revision: https://secure.phabricator.com/D10318
Summary: Fixes T5798. We basically weren't using the caching mechanism. Also adds service calls for S3 stuff, and support for seeing a little info like you can for conduit.
Test Plan: uploaded a paste, looked at paste list - no s3 service calls. edited the paste, looked at paste list - no s3 service calls and edited content properly shown
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5798
Differential Revision: https://secure.phabricator.com/D10294
Summary:
Via HackerOne. Chrome (at least) interprets backslashes like forward slashes, so a redirect to "/\evil.com" is the same as a redirect to "//evil.com".
- Reject local URIs with backslashes (we never generate these).
- Fully-qualify all "Location:" redirects.
- Require external redirects to be marked explicitly.
Test Plan:
- Expanded existing test coverage.
- Verified that neither Diffusion nor Phriction can generate URIs with backslashes (they are escaped in Diffusion, and removed by slugging in Phriction).
- Logged in with Facebook (OAuth2 submits a form to the external site, and isn't affected) and Twitter (OAuth1 redirects, and is affected).
- Went through some local redirects (login, save-an-object).
- Verified file still work.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10291
Summary:
Instead of allowing all routes based on security.alternate-file-domain, now, when security.alternate-file-domain is set, and the request matches this domain, requests are validated against an explicit list. Allowed routes:
- /res/
- /file/data/
- /file/xform/
- /phame/r/
This will be redone by T5702 to be less of a hack.
Test Plan:
- browse around (incl. Phame live) to make sure there is no regression from this when security.alternate-file-domain is not used.
- check that celerity resources and files (incl. previews) are served with security.alternate-file-domain set.
- check that phame live blog is serving its css correctly with security.alternate-file-domain set.
- check that requests outside of the whitelist generate an exception for security.alternate-file-domain
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10048
Summary: Ref T5655. Some discussion in D9839. Generally speaking, `Phabricator{$name}Application` is clearer than `PhabricatorApplication{$name}`.
Test Plan:
# Pinned and uninstalled some applications.
# Applied patch and performed migrations.
# Verified that the pinned applications were still pinned and that the uninstalled applications were still uninstalled.
# Performed a sanity check on the database contents.
Reviewers: btrahan, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: hach-que, epriestley, Korvin
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9982
Summary: I'm pretty sure that `@group` annotations are useless now... see D9855. Also fixed various other minor issues.
Test Plan: Eye-ball it.
Reviewers: #blessed_reviewers, epriestley, chad
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9859
Summary: Ref T3116. If you have MFA on your account, require a code to sign a legal document.
Test Plan: Signed legal documents, got checkpointed.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T3116
Differential Revision: https://secure.phabricator.com/D9772
Summary: Applied some more linter fixes that I previously missed because my global `arc` install was out-of-date.
Test Plan: Will run `arc unit` on another host.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9443
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: We haven't needed this for like three years, so we probably won't ever need it. It's in history if we do.
Test Plan: thought long and hard
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9311
Summary:
D9153 fixed half of this, but exposed another issue, which is that we don't actually serve ".eot" and ".ttf" through Celerity right now.
Make sure we include them in the routes.
Test Plan:
- Downloaded CSS, JS, TTF, EOT, WOFF, JPG, etc., through Celerity.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9154
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 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 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: A small but appreciable number of users find flavor on buttons confusing. Remove this flavor. This retains flavor in headers, error messages, etc., which doesn't cause confusion.
Test Plan: Looked at a revision, task, paste, macro, etc.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8812
Summary:
Couple of minor cleanup things here:
- Pass handles to ApplicationTransactions when rendering their stories; this happened implicitly before but doesn't now.
- Add `?text=1` to do ad-hoc rendering of a story in text mode.
- Make Conduit skip unrenderable stories.
- Fix/modernize some text in the Commit story.
Test Plan: Rendered text versions of stories via Conduit and `?text=1`.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: zeeg, spicyj, epriestley
Differential Revision: https://secure.phabricator.com/D8793
Summary:
See <https://github.com/facebook/phabricator/pull/563>.
I think this secondary construction of a `$user` is very old, and predates subsequent changes which cause a proper user to construct earlier, so using the user on the `$request` should (I think) always work. I couldn't immediately find any cases where it does not.
Test Plan: With `debug.stop-on-redirect` set, hit various redirects, like jump-naving to T1. Got a proper stop dialog.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8718
Summary: I accidentally made these exceptionally ugly recently.
Test Plan: {F137411}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley, chad
Differential Revision: https://secure.phabricator.com/D8684
Summary:
This adds a system which basically keeps a record of recent actions, who took them, and how many "points" they were worth, like:
epriestley email.add 1 1233989813
epriestley email.add 1 1234298239
epriestley email.add 1 1238293981
We can use this to rate-limit actions by examining how many actions the user has taken in the past hour (i.e., their total score) and comparing that to an allowed limit.
One major thing I want to use this for is to limit the amount of error email we'll send to an email address. A big concern I have with sending more error email is that we'll end up in loops. We have some protections against this in headers already, but hard-limiting the system so it won't send more than a few errors to a particular address per hour should provide a reasonable secondary layer of protection.
This use case (where the "actor" needs to be an email address) is why the table uses strings + hashes instead of PHIDs. For external users, it might be appropriate to rate limit by cookies or IPs, too.
To prove it works, I rate limited adding email addresses. This is a very, very low-risk security thing where a user with an account can enumerate addresses (by checking if they get an error) and sort of spam/annoy people (by adding their address over and over again). Limiting them to 6 actions / hour should satisfy all real users while preventing these behaviors.
Test Plan:
This dialog is uggos but I'll fix that in a sec:
{F137406}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8683
Summary:
- Point them at the new Diviner.
- Make them a little less cumbersome to write.
Test Plan: Found almost all of these links in the UI and clicked them.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8553
Summary:
This is the other half of D8548. Specifically, the attack here was to set your own editor link to `javascript\n:...` and then you could XSS yourself. This isn't a hugely damaging attack, but we can be more certain by adding a whitelist here.
We already whitelist linkable protocols in remarkup (`uri.allowed-protocols`) in general.
Test Plan:
Tried to set and use valid/invalid editor URIs.
{F130883}
{F130884}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8551
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:
Fixes T4610. Open to suggestions, etc., if there's anything I'm missing.
Also:
- Moves these "system" endpoints into a real application.
- Makes `isUnlisted()` work a little more consistently.
Test Plan: Accessed `/robots.txt`, `/status/` and `/debug/`.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Maniphest Tasks: T4610
Differential Revision: https://secure.phabricator.com/D8532
Summary:
Ref T4593. Via HackerOne. An attacker can use the anchor reattachment, combined with the Facebook token workflow, combined with redirection on OAuth errors to capture access tokens. The attack works roughly like this:
- Create an OAuth application on Phabricator.
- Set the domain to `evil.com`.
- Grab the OAuth URI for it (something like `https://phabricator.com/oauthserver/auth/?redirect_uri=http://evil.com&...`).
- Add an invalid `scope` parameter (`scope=xyz`).
- Use //that// URI to build a Facebook OAuth URI (something like `https://facebook.com/oauth/?redirect_uri=http://phabricator.com/...&response_type=token`).
- After the user authorizes the application on Facebook (or instantly if they've already authorized it), they're redirected to the OAuth server, which processes the request. Since this is the 'token' workflow, it has auth information in the URL anchor/fragment.
- The OAuth server notices the `scope` error and 302's to the attacker's domain, preserving the anchor in most browsers through anchor reattachment.
- The attacker reads the anchor in JS and can do client workflow stuff.
To fix this, I've made several general changes/modernizations:
- Add a new application and make it beta. This is mostly cleanup, but also turns the server off for typical installs (it's not generally useful quite yet).
- Add a "Console" page to make it easier to navigate.
- Modernize some of the UI, since I was touching most of it anyways.
Then I've made specific security-focused changes:
- In the web-based OAuth workflow, send back a human-readable page when errors occur. I //think// this is universally correct. Previously, humans would get a blob of JSON if they entered an invalid URI, etc. This type of response is correct for the companion endpoint ("ServerTokenController") since it's called by programs, but I believe not correct for this endpoint ("AuthController") since it's used by humans. Most of this is general cleanup (give humans human-readable errors instead of JSON blobs).
- Never 302 off this endpoint automatically. Previously, a small set of errors (notably, bad `scope`) would cause a 302 with 'error'. This exposes us to anchor reattachment, and isn't generally helpful to anyone, since the requesting application did something wrong and even if it's prepared to handle the error, it can't really do anything better than we can.
- The only time we'll 'error' back now from this workflow is if a user explicitly cancels the workflow. This isn't a 302, but a normal link (the cancel button), so the anchor is lost.
- Even if the application is already approved, don't blindly 302. Instead, show the user a confirmation dialog with a 'continue' link. This is perhaps slightly less user-friendly than the straight redirect, but I think it's pretty reasonable in general, and it gives us a lot of protection against these classes of attack. This redirect is then through a link, not a 302, so the anchor is again detached.
-
Test Plan: I attempted to hit everything I touched. See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Maniphest Tasks: T4593
Differential Revision: https://secure.phabricator.com/D8517
Summary:
- Allow Celerity to map and serve WOFF files.
- Add Source Sans Pro, Source Sans Pro Bold, and the corresponding LICENSE.
- Add a `font-source-sans-pro` resource for the font.
Test Plan:
- Changed body `font-face` to `'Source Sans Pro'`.
- Added `require_celerity_resource('font-source-sans-pro')` in StandardPageView.
Works in Firefox/Chrome/Safari, at least:
{F123296}
{F123297}
{F123298}
Reviewers: btrahan, chad
Reviewed By: chad
CC: chad, aran
Differential Revision: https://secure.phabricator.com/D8430
Summary:
Currently, the linter raises `XHP29` warnings for these files because they are not abstract or final.
I guess there are two possibly solutions, either making the classes final or marking them as `@concrete-extensible`. Given that there are no subclasses of these classes in the `phabricator`, `arcanist` and `libphutil` repositories... I opted to declare the classes as final.
Test Plan:
The following linter warnings are gone:
```
>>> Lint for src/aphront/configuration/AphrontDefaultApplicationConfiguration.php:
Warning (XHP29) Class Not abstract Or final
This class is neither 'final' nor 'abstract', and does not have a
docblock marking it '@concrete-extensible'.
3 /**
4 * @group aphront
5 */
>>> 6 class AphrontDefaultApplicationConfiguration
7 extends AphrontApplicationConfiguration {
8
9 public function __construct() {
>>> Lint for src/applications/differential/mail/DifferentialReplyHandler.php:
Warning (XHP29) Class Not abstract Or final
This class is neither 'final' nor 'abstract', and does not have a
docblock marking it '@concrete-extensible'.
1 <?php
2
>>> 3 class DifferentialReplyHandler extends PhabricatorMailReplyHandler {
4
5 private $receivedMail;
6
```
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8347
Summary:
Ref T1191. I believe we only have three meaningful binary fields across all applications:
- The general cache may contain gzipped content.
- The file storage blob may contain arbitrary binary content.
- The Passphrase secret can store arbitrary binary data (although it currently never does).
This adds Lisk config for binary fields, and uses `%B` where necessary.
Test Plan:
- Added and executed unit tests.
- Forced file uploads to use MySQL, uploaded binaries.
- Disabled the CONFIG_BINARY on the file storage blob and tried again, got an appropraite failure.
- Tried to register with an account containing a G-Clef, and was stopped before the insert.
Reviewers: btrahan, arice
Reviewed By: arice
CC: arice, chad, aran
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D8316
Summary:
Ref T4324. Add a real `Application` class. Use modern UI elements.
@chad, we could use an icon :3
Test Plan: {F114477}
Reviewers: chad, btrahan
Reviewed By: chad
CC: chad, aran
Maniphest Tasks: T4324
Differential Revision: https://secure.phabricator.com/D8254
