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
Summary:
Ref T4420. This sets up the basics for modular typeahead sources. Basically, the huge `switch()` is just replaced with class-based runtime dispatch.
The only clever bit I'm doing here is with `CompositeDatasource`, which pretty much just combines the results from several other datasources. We can use this to implement some of the weird cases where we need multiple types of results, although I think I can entirely eliminate many of them entirely. It also makes top-level implementation simpler, since more logic can go inside the sources.
Sources are also application-aware, will be responsible for placeholder text, and have a slightly nicer debug view.
Test Plan: {F112859}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D8228
Summary: Ref T1139. This has some issues and glitches, but is a reasonable initial attempt that gets some of the big pieces in. We have about 5,200 strings in Phabricator.
Test Plan: {F108261}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T1139
Differential Revision: https://secure.phabricator.com/D8138
Summary: Ref T3979. Currently, the home page lives in an old application called "directory" and is informally defined. Make it a real application called "Home", with a formal definition. It isn't launchable and can't be uninstalled.
Test Plan: Loaded home, saw exact same stuff.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T3979
Differential Revision: https://secure.phabricator.com/D8074
Summary:
Ref T2380. If an install has a CDN domain configured, but does not list it as an alternate domain (which is standard/correct, but not incredibly common, see T2380), we'll currently try to set anonymous cookies on it. These will correctly fail security rules.
Instead, don't try to set these cookies.
I missed this in testing yesterday because I have a file domain, but I also have it configured as an alternate domain, which allows cookies to be set. Generally, domain management is due for some refactoring.
Test Plan: Set file domain but not as an alternate, logged out, nuked file domain cookies, reloaded page. No error after patch.
Reviewers: btrahan, csilvers
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2380
Differential Revision: https://secure.phabricator.com/D8057
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. 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: Cookie-prefix should fix phabricator instances where x.com and x.y.com have conflicting cookie names
Test Plan: Pushed branch to dev.phab.example.com, logged into phab.example.com and into dev.phab.example.com.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7979
Summary: Ref T4222. Adds the map name to Celerity resource URIs, so we can serve out of any map.
Test Plan: Poked around, verified URIs have "/phabricator/" in them now.
Reviewers: btrahan, hach-que
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7877
Summary:
Ref T4222. Currently, CelerityResourceResponse holds response resources in flat maps. Instead, specify which map resources appear in.
Also, provide `requireResource()` and `initBehavior()` APIs on the Controller and View base classes. These provide a cleaner abstraction over `require_celerity_resource()` and `Javelin::initBehavior()`, but are otherwise the same. Move a few callsites over.
Test Plan:
- Reloaded pages.
- Browsed around Differential.
Reviewers: btrahan, hach-que
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7876
Summary:
Ref T4222.
- Removes the old map and changes the CelerityResourceMap API to be entirely driven by the new map.
- The new map is about 50% smaller and organized more sensibly.
- This removes the `/pkg/` URI component. All resources are now required to have unique names, so we can tell if a resource is a package or not by looking at the name.
- Removes some junky old APIs.
- Cleans up some other APIs.
- Added some feedback for `bin/celerity map`.
- `CelerityResourceMap` is still a singleton which is inextricably bound to the Phabricator map; this will change in the future.
Test Plan:
- Reloaded pages.
- Verified packaging works by looking at generated includes.
- Forced minification on and verified it worked.
- Forced no-timestamps on and verified it worked.
- Rebuilt map.
- Ran old script and verified error message.
- Checked logs.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: chad, aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7872
Summary: Ref T4140. Provide more debugging information so we can figure out what's going on with redirect loops.
Test Plan: {F83868}
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4140
Differential Revision: https://secure.phabricator.com/D7620
Summary: Fixes T4123. If you click "Profile" on a page, we already profile all the ajax requests it generates. Do the same for "Analyze Query Plans".
Test Plan: Viewed a page with Ajax requests using "Analyze Query Plans", and not using "Analyze Query Plans".
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4123
Differential Revision: https://secure.phabricator.com/D7601
Summary: Fixes T4084. See that task for discussion.
Test Plan: Did `git clone`. My setup doesn't precisely reproduce the original issue, but hopefully @enko can confirm this is a fix.
Reviewers: btrahan, enko
Reviewed By: enko
CC: enko, aran
Maniphest Tasks: T4084
Differential Revision: https://secure.phabricator.com/D7561
Test Plan: This is one of the rare moments where unit tests for views would be useful.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7547
Summary:
I just want to make sure that this is the style we want.
It seems less readable to me in some cases.
Test Plan: Looked at DarkConsole with errors.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7533
Summary: Ref T4064. The response code here isn't normally relevant, but we can hit these via `git clone http://../`, etc., and it's clearly more correct to use HTTP 500.
Test Plan: Added a fake `throw new Exception()` and verified I got an HTTP 500 response.
Reviewers: jamesr, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4064
Differential Revision: https://secure.phabricator.com/D7507
Summary:
Mostly ripped from D7391, with some changes:
- Serve repositories at `/diffusion/X/`, with no special `/git/` or `/serve/` URI component.
- This requires a little bit of magic, but I got the magic working for Git, Mercurial and SVN, and it seems reasonable.
- I think having one URI for everything will make it easier for users to understand.
- One downside is that git will clone into `X` by default, but I think that's not a big deal, and we can work around that in the future easily enough.
- Accept HTTP requests for Git, SVN and Mercurial repositories.
- Auth logic is a little different in order to be more consistent with how other things work.
- Instead of AphrontBasicAuthResponse, added "VCSResponse". Mercurial can print strings we send it on the CLI if we're careful, so support that. I did a fair amount of digging and didn't have any luck with git or svn.
- Commands we don't know about are assumed to require "Push" capability by default.
No actual VCS data going over the wire yet.
Test Plan:
Ran a bunch of stuff like this:
$ hg clone http://local.aphront.com:8080/diffusion/P/
abort: HTTP Error 403: This repository is not available over HTTP.
...and got pretty reasonable-seeming errors in all cases. All this can do is produce errors for now.
Reviewers: hach-que, btrahan
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7417
Summary:
Ref T3675. Some of these listeners shouldn't do their thing if the viewer doesn't have access to an application (for example, users without access to Differential should not be able to "Edit Tasks"). Set the stage for that:
- Introduce `PhabricatorEventListener`, which has an application.
- Populate this for event listeners installed by applications.
- Rename the "PeopleMenu" listeners to "ActionMenu" listeners, which better describes their modern behavior.
This doesn't actually change any behaviors.
Test Plan: Viewed Maniphest, Differntial, People.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3675
Differential Revision: https://secure.phabricator.com/D7364
Summary:
Ref T603. Currently, if you're logged out and try to view some object which requires you to be logged in, the login screen is missing the application breadcrumb and just says "Login".
Add the application in context so we get the keys icon.
Test Plan: {F69255}
Reviewers: chad, btrahan, asherkin
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7303
Summary:
Ref T603. I want to let applications define new capabilities (like "can manage global rules" in Herald) and get full support for them, including reasonable error strings in the UI.
Currently, this is difficult for a couple of reasons. Partly this is just a code organization issue, which is easy to fix. The bigger thing is that we have a bunch of strings which depend on both the policy and capability, like: "You must be an administrator to view this object." "Administrator" is the policy, and "view" is the capability.
That means every new capability has to add a string for each policy, and every new policy (should we introduce any) needs to add a string for each capability. And we can't do any piecemeal "You must be a {$role} to {$action} this object" becuase it's impossible to translate.
Instead, make all the strings depend on //only// the policy, //only// the capability, or //only// the object type. This makes the dialogs read a little more strangely, but I think it's still pretty easy to understand, and it makes adding new stuff way way easier.
Also provide more context, and more useful exception messages.
Test Plan:
- See screenshots.
- Also triggered a policy exception and verified it was dramatically more useful than it used to be.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7260
Summary:
Ref T603. I had to partially revert this earlier because it accidentally blocked access to Conduit and File data for installs without "policy.allow-public", since the applications are available to "all users" but some endpoints actually need to be available even when not logged in.
This readjusts the gating in the controller to properly apply application visibility restrictions, and then adds a giant pile of unit test coverage to make sure it sticks and all the weird cases are covered.
Test Plan:
- Added and executed unit tests.
- Executed most of the tests manually, by using logged in / admin / public / disabled users.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7211
Summary:
Fixes T3887. Two issues:
- Macros were generating entirely before the render cache, so audio macros worked fine in previews and the first time the cache was populated, but not afterward.
- Instead, parse them before the cache but drop them in after the cache. Clean up all the file querying, too. This makes cached remarkup generate the correct audio beahviors.
- Safari sends an HTTP request with a "Range" header, and expects a "206 Partial Content" response. If we don't give it one, it sometimes has trouble figuring out how long a piece of audio is (mostly for longer clips? Or mostly for MP3s?). I'm not exactly sure what triggers it. The net effect is that "loop" does not work when Safari gets confused. While looping a short "quack.wav" worked fine, longer MP3s didn't loop.
- Supporting "Range" and "206 Partial Content", which is straightforward, fixes this problem.
Test Plan:
- Viewed a page with lots of different cached audio macros and lots of different uncached preview audio macros, they all rendered correctly and played audio.
- Viewed a macro with a long MP3 audio loop in Safari. Verified it looped after it completed. Used Charles to check that the server received and responded to the "Range" header correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3887
Differential Revision: https://secure.phabricator.com/D7166
Summary:
Ref T603. This could probably use a little more polish, but improve the quality of policy error messages.
- Provide as much detail as possible.
- Fix all the strings for i18n.
- Explain special rules to the user.
- Allow indirect policy filters to raise policy exceptions instead of 404s.
Test Plan: See screenshots.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7151
Summary:
Fixes T3796. When this got split out into tabs, the data endpoints were accidentally locked down. Open them up again if the setting is on.
Also, when you open/close the console we try to save the preference. Just no-op if you're logged out. Previously, you'd see the requests in DarkConsole since they failed.
Test Plan: Enabled `darkconsole.always-on` and toggled the console on and off as a logged-out user. Disabled the preference and verified it was no longer accessible.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3796
Differential Revision: https://secure.phabricator.com/D6886
Summary:
Ref T3775 (discussion here). Ref T2625.
T3775 presents two problems:
# Existing tools which linked to `/differential/active/epriestley/` (that is, put a username in the URL) can't generate search links now.
# Humans can't edit the URL anymore, either.
I think (1) is an actual issue, and this fixes it. I think (2) is pretty fluff, and this doesn't really try to fix it, although it probably improves it.
The fix for (1) is:
- Provide a helper to read a parameter containing either a list of user PHIDs or a list of usernames, so `/?users[]=PHID-USER-xyz` (from a tokenizer) and `/?users=alincoln,htaft` (from an external program) are equivalent inputs.
- Rename all the form parameters to be more digestable (`authorPHIDs` -> `authors`). Almost all of them were in this form already anyway. This just gives us `?users=alincoln` instead of `userPHIDs=alincoln`.
- Inside ApplicationSearch, if a request has no query associated with it but does have query parameters, build a query from the request instead of issuing the user's default query. Basically, this means that `/differential/` runs the default query, while `/differential/?users=x` runs a custom query.
Test Plan: {F56612}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625, T3775
Differential Revision: https://secure.phabricator.com/D6840
Summary:
Ref T3684. The URI itself is reflected in a few places. It is generally not dangerous because we only let you add random stuff to the end of it for one or two controllers (e.g., the file download controller lets you add "/whatever.jpg"), but:
- Remove it entirely in the main request, since it serves no purpose.
- Remove query parameters in Ajax requests. These are available in DarkConsole proper.
Also mask a few things in the "Request" tab; I've never used these fields when debugging or during support, and they leak quasi-sensitive information that could get screenshotted or over-the-shoulder'd.
I didn't mitgate `__metablock__` because I think the threat is so close to 0 that it's not worthwhile.
Test Plan: Used Darkconsole, examined Requests tab.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3684
Differential Revision: https://secure.phabricator.com/D6699
Summary: Fixes T3673. Supposedly we won't get any data in this case, but it seems we sometimes do. See discussion in task.
Test Plan: Used `var_dump()`, etc., to verify we short circuit out of "multipart/form-data" posts regardless of the presence of input data.
Reviewers: nmalcolm, btrahan
Reviewed By: nmalcolm
CC: aran
Maniphest Tasks: T3673
Differential Revision: https://secure.phabricator.com/D6670
Summary: The `phabricator.allowed-uris` config setting is not checked properly when trying to set cookies.
Test Plan:
Set an alternate URI, then accessed Phabricator. No longer received a secondary cookie error.
Hit the new exceptions to test them:
{F51131}
{F51132}
Reviewers: btrahan, garoevans
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6528
Summary: Fixes T3544. Depends on D6475. This was just a missing dependency combined with some questionable error handling which I'll maybe fix some day.
Test Plan: Loaded page, saw result.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3544
Differential Revision: https://secure.phabricator.com/D6476
Summary: Mostly straightforward. Also fixed a couple of error/darkconsole things.
Test Plan:
- Created poll;
- viewed poll;
- voted in poll;
- used `V6` and `{V6}` markup styles in poll.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6458
Summary:
D6278 kind of got closed and commited, this is the actual direction.
Ref T3432
Depends on D6277
Test Plan: Keep using the site
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, mbishopim3
Maniphest Tasks: T3432
Differential Revision: https://secure.phabricator.com/D6283
Summary:
If D6277 is the way to go, then this will be it's implementation.
Depends on D6277
Test Plan: Keep using the site
Reviewers: epriestley
CC: aran, Korvin
Maniphest Tasks: T3432
Differential Revision: https://secure.phabricator.com/D6278
Summary:
Ref T1536. This is extremely reachable and changes the login code to the new stuff.
Notes:
- I've hard-disabled password registration since I want installs to explicitly flip it on via config if they want it. New installs will get it by default in the future, but old installs shouldn't have their auth options change.
- Google doesn't let us change the redirect URI, so keep the old one working.
- We need to keep a bit of LDAP around for now for LDAP import.
- **Facebook:** This causes substantive changes in what login code is executed.
Test Plan:
- Logged in / logged out / registered, hit new flows.
- Logged in with google.
- Verified no password registration by default.
Reviewers: btrahan, chad
Reviewed By: chad
CC: wez, nh, aran, mbishopim3
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6222
Summary: Ref T1536. Error state is a bit gross but we need to sort that out in general.
Test Plan:
{F46549}
{F46550}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6208
Summary:
Ref T1445. Ref T1536. Although we have separate CSRF protection and have never been vulnerable to OAuth hijacking, properly implementing the "state" parameter provides a little more certainty.
Before OAuth, we set a random value on the client, and pass its hash as the "state" parameter. Upon return, validate that (a) the user has a nonempty "phcid" cookie and (b) the OAuth endpoint passed back the correct state (the hash of that cookie).
Test Plan: Logged in with all OAuth providers, which all apparently support `state`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, arice
Maniphest Tasks: T1445, T1536
Differential Revision: https://secure.phabricator.com/D6179
Summary: Ref T1536. We can safely replace the old login validation controller with this new one, and reduce code dplication while we're at it.
Test Plan: Logged in with LDAP, logged in with OAuth, logged in with username/password, did a password reset.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6178
Summary:
Provide a Content-Length header so that browsers can estimate time
remaining for file downloads.
Test Plan: Tested on our local phabricator install.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6107
Summary:
Ref T2625. Lifts almost all of the search logic out of Paste controllers and into Search.
This uses controller delegation for generalization. We use this in a few places, but don't use it very much yet. I think it's pretty reasonable as-is, but I might be able to make even more stuff free.
There are some slightly rough edges around routes, still, but I want to hit Phame and Differential (which both have multiple application search engines) before trying to generalize that.
Test Plan: Executed, browsed and managed Paste searches.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6073
Summary:
Kind of a quick look at an idea for T2184
Ref T2184
Test Plan: Make sure the site still loads
Reviewers: epriestley
CC: aran, Korvin, mbishopim3
Maniphest Tasks: T2184
Differential Revision: https://secure.phabricator.com/D6045
Summary:
Ref T2785
Looks for hosts in `conduit.servers` config and if any exist route any conduit calls through any one of the hosts.
Test Plan:
Make some curl calls to public methods (`conduit.ping`), watch the access log for two requests. Make some calls from the UI that require authentication, watch the access log a bit more.
Also ran the unit tests.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2785
Differential Revision: https://secure.phabricator.com/D5970
Summary:
I always put a `phlog()` somewhere or something fails and I have hard times figuring out which request it was.
Also fix safe HTML in panel.
Test Plan: Looked at DarkConsole with error on main page, AJAX request and both.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5784
Summary:
Ref T2787. For payment methods that allow you to add a billable method (i.e., a credit card), move all the logic into the provider. In particular:
- Providers may (Stripe, Balanced) or may not (Paypal, MtGox) allow you to add rebillable payment methods. Providers which don't allow rebillable methods will appear at checkout instead and we'll just invoice you every month if you don't use a rebillable method.
- Providers which permit creation of rebillable methods handle their own data entry, since this will be per-provider.
- "Add Payment Method" now prompts you to choose a provider. This is super ugly and barely-usable for the moment. When there's only one choice, we'll auto-select it in the future.
Test Plan: Added new Stripe payment methods; hit all the Stripe errors.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D5756
Summary:
Refs T1048; Depends on D5542, D5543, D5544 - It currently just renders multiple hovercards nicely for test purposes. More is on the way.
Mode `test`: Human test chamber.
Mode `retrieve`: For JS. Added so it would not clash with search key routing.
badassery
Test Plan:
`/search/hovercard/test/?phids[hover-T4]=PHID-TASK-g5pduvwrrwvkq5gkx736&phids[hover-T2]=PHID-TASK-gta6lzaaagziavkktima`
Verified the appearance of two tasks with correct rendering and correct ids
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1048
Differential Revision: https://secure.phabricator.com/D5545
Summary:
For some time, we've stopped the profiler twice when it was invoked by the sampling mechanism. The first time it actually stops, and we write a profile. The second time it hadn't been started, so it returns empty and we write an invalid profile.
Instead, keep track of whether it is running or not, and don't stop it a second time.
Ref T2870.
Test Plan: Set sample rate to 1-in-3, observed valid sample profiles generate.
Reviewers: btrahan, chad
CC: aran
Maniphest Tasks: T2870
Differential Revision: https://secure.phabricator.com/D5534
Summary:
We have a fair number of conditionals on the existence of the access log. Instead, always build it and just don't write it if the user doesn't want a version on disk.
Also, formalize logged-in user PHID (avoids object existence juggling) in the access log and move microseconds-since-startup to PhabricatorStartup (simplifies index.php).
Depends on D5532. Fixes T2860. Ref T2870.
Test Plan: Disabled access log, verified XHProf writes occurred correctly.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2860, T2870
Differential Revision: https://secure.phabricator.com/D5533
Summary:
Ref T2870. This resolves a few issues:
- No proper Application. Define one.
- Routes are in the default controller. Move them to the application.
- UI doesn't work on mobile.
- Overescaping in the link column.
Test Plan:
Old page:
{F38444}
New page:
{F38445}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran, AnhNhan, edward
Maniphest Tasks: T2870
Differential Revision: https://secure.phabricator.com/D5531
Summary: Modern conduit responses should never have a JSON shield. We disable it for normal responses, but uncaught exceptions hit this higher-level handler block which fails to disable the shield. Disable the shield.
Test Plan: Inspection.
Reviewers: btrahan, andrewjcg
Reviewed By: andrewjcg
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5483
Summary:
Ref T2787. This does very little so far, but makes inroads on accounts and billing. This is mostly just modeled on what Stripe looks like. The objects are:
- **Account**: Has one or more authorized users, who can make manage the account. An example might be "Phacility", and the three of us would be able to manage it. A user may be associated with more than one account (e.g., a corporate account and a personal account) but the UI tries to simplify the common case of a single account.
- **Payment Method**: Something we can get sweet sweet money from; for now, a credit card registered with Stripe. Payment methods are associated with an account.
- **Product**: A good (one time charge) or service (recurring charge). This might be "t-shirt" or "enterprise plan" or "hourly support" or whatever else.
- **Purchase**: Represents a user purchasing a Product for an Account, using a Payment Method. e.g., you bought a shirt, or started a plan, or purchased support.
- **Charge**: Actual charges against payment methods. A Purchase can create more than one charge if it's a plan, or if the first charge fails and we re-bill.
This doesn't fully account for stuff like coupons/discounts yet but they should fit into the model without any issues.
This only implements `Account`, and that only partially.
Test Plan: {F37531}
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D5435
Summary:
Safari has a weird bug with `border-radius` plus border color:
{F35865}
Move the uncolored borders to an internal div to fix this. Also tweak some positioning on icons for cards, and add a "magenta" color.
Test Plan: {F35866}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D5338
Summary:
Ref T2650. Possible fix for that issue.
- "Passthru" got renamed to "passthrough" but a site was missed.
- Don't try to post an empty comment if the text is empty but we have inlines; this avoids popping a "you can't post an empty comment" error.
Test Plan: Made an empty comment with an inline in Pholio.
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2650
Differential Revision: https://secure.phabricator.com/D5240
Summary:
Currently, there's no easy way for me to tell a user "run this code from the webserver and tell me what it says". Sometimes installs can add new .php files to, e.g., `webroot/rsrc/`, but this is setup-dependent and not universal. Generally I resort to saying "put this into index.php", but that's error prone and not acceptable on active installs.
Add a "debug" controller so I can instead say "put this into support/debug.php, then visit /debug/".
Test Plan: Visited /debug/ with and without support/debug.php files. Visited /staus/.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D5212
Summary: Deleted code which used channel. Created PhabricatorChatLogChannelQuery.php
Test Plan: By manually checking in the chatlog application.
Reviewers: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5010
Summary:
- In stack traces, a `,` should clearly be a `.`.
- In Calendar, a 'td' got swapped with a 'p' somewhere.
- In old-style transaction views, strlen() is no longer a sufficient test.
Test Plan:
- Verified stack traces render correctly.
- Verified calendar renders correctly.
- Verified Maniphest transactions with no comment no longer have a little empty div a few pixels high.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4971
Summary: Sgrepped for `"=~/</"` and manually changed every HTML.
Test Plan: This doesn't work yet but it is hopefully one of the last diffs before Phabricator will be undoubtedly HTML safe.
Reviewers: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4927
Summary:
This is pretty brutal and it adds some `phutil_safe_html()`.
But it is a big step in the right direction.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4905
Summary:
This resolves lots of double escaping.
We changed most of `phutil_render_tag(, , $s)` to `phutil_tag(, , $s)` which means that `$s` is now auto-escaped.
Also `pht()` auto escapes if it gets `PhutilSafeHTML`.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4889
Summary:
When a developer changes CSS, it is normally sufficient to reload the page to get changes to show up, because browsers revalidate resources on reload.
However, if you reload the page and then an Ajax request adds new CSS to the page, this CSS does not trigger revalidation. The developer must currently clear their cache or re-run `scripts/celerity_mapper.php webroot`, to get this request to skip cache. We rarely use CSS over Ajax right now, so this hasn't cropped up much, but Conpherence does use this and clearing the resource is a big pain.
This seems to work fine normally, but I'm worried it might break some of the extra-celerity-resources stuff Facebook is doing.
Test Plan: In development mode, changed `conpherence/message-pane.css` and saw changes reflected on reload. Verified normal page loads do not cause additional HTTP requests. This change has no effect in production mode.
Reviewers: edward, vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2428
Differential Revision: https://secure.phabricator.com/D4902
Summary:
- Separate the ideas of "requested" (explicit user request) vs "started" (user request or sampling).
- Move this code out of index.php into the XHProf stuff (general effort to make index.php smaller).
Test Plan:
Verified that profiling still works, and profiling extends to ajax requests.
Set sampling rate to 2, saw 50% samples.
Looked at database, saw sampling data populating properly.
Reviewers: vrana, nh
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4891
Summary:
Lots of killed `phutil_escape_html()`.
Done by searching for `AphrontTableView` and then `$rows` (usually) backwards.
Test Plan:
Looked at homepage.
echo id(new AphrontTableView(array(array('<'))))->render();
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4884
Summary:
Done by searching for `AphrontDialogView` and then `appendChild()`.
Also added some `pht()`.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4882
Summary: Searched for `AphrontErrorView` and then for `setTitle()`.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4880
Summary: Route all `$_SERVER['HTTP_...']` stuff through AphrontRequest (it would be nice to make this non-static, but the stack is a bit tangled right now...)
Test Plan: Verified CSRF and cascading profiling. `var_dump()`'d User-Agent and Referer and verified they are populated and returned correct values when accessed. Restarted server to trigger setup checks.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4888
Summary: If a page is profiled, add an "X-Phabricator-Profiler" header to all Ajax requests, and profile those too.
Test Plan: Profiled a page, checked Darkconsole, saw profiles for everything.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4885
Summary: Done by searching for `AphrontErrorView` and then `appendChild()`.
Test Plan:
Looked at Commit Detail.
Looked at Revision Detail.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4843
Summary: Refactor options related to verbose error reporting and forcing disk reads into a single developer option.
Test Plan: Run Phabricator with the developer-mode option set and check that errors print stack traces, static assets are always reloaded, etc.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4780
Summary:
This accomplishes three major goals:
# Fixes phutil_render_tag -> phutil_tag callsites in DarkConsole.
# Moves the Ajax request log to a new panel on the left. This panel (and the tabs panel) get scrollbars when they get large, instead of making the page constantly scroll down.
# Loads the panel content over ajax, instead of dumping it into the page body / ajax response body. I've been planning to do this for about 3 years, which is why the plugins are architected the way they are. This should make debugging easier by making response bodies not be 50%+ darkconsole stuff.
Additionally, load the plugins dynamically (the old method predates library maps and PhutilSymbolLoader).
Test Plan:
{F30675}
- Switched between requests and tabs, reloaded page, saw same tab.
- Used "analyze queries", "profile page", triggered errors.
- Verified page does not load anything by default if dark console is closed with Charles.
- Generally banged on it a bit.
Reviewers: vrana, btrahan, chad
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4692
Summary:
If you run this code:
json_encode(array('tag' => phutil_tag('div', array())));
...you get this result, because json_encode() does not call toString() on objects:
{"tag":{}}
Instead, convert such objects to their underlying strings. Javelin has support for JX.HTML and for implicit conversion (which is kind of sketchy for other reasons) but it's sort of complicated (only happens on Ajax, not behaviors) and messy (not metadata-based), so ignore it for now.
We'll need to do something similar for serialization to the database. My plan there is just to throw on any objects. The only time we put HTML in the database is cache-related and those tiny number of callsites can manually handle it.
Test Plan: Various ajax things now receive the correct data.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4684
Summary:
- Implements `javelin_tag()`, which is `javelin_render_tag()` on top of `phutil_tag()` instead of `phutil_render_tag()`.
- Manually converts all or almost all of the trivial callsites.
Test Plan:
- Site does not seem any more broken than before.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4639
Summary:
Created with spatch:
lang=diff
- phutil_render_tag
+ phutil_tag
(X, Y, '...')
Then searched for `&` and `<` in the output and replaced them.
Test Plan: Loaded homepage.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4503
Summary:
Fixes T2293.
We currently hard-require this in setup. We do not need to; we don't actually need it until we start running daemons. Move it to post-install and provide more guidance.
We could make this even easier in the future, but we'd need to special case it, since it's dangerous to let it be set to any value (if you set it to the wrong value, you can't log in). We could safely have a workflow which writes the current request URI into the database configuration, or a two-stage workflow where we set the URI and then verify it, but these both imply some special casing and complication. This should be a step forward from where we are today, regardless.
Test Plan:
Removed "phabricator.base-uri" from my configuration. Verified Phabricator still works.
Without "phabricator.base-uri" configured, logged in from multiple host names (127.0.0.1:8080, local.aphront.com:8080).
Configured "phabricator.base-uri". Verified my unblessed session no longer worked. Verified setup issue went away.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2293
Differential Revision: https://secure.phabricator.com/D4580
Summary: Just removed the link and created a new field under preferences. Now the setting is under Display Preferences.
Test Plan: Enablied/Disabled dark console to see if it works.
Reviewers: epriestley
Reviewed By: epriestley
CC: irinav, aran, Korvin
Maniphest Tasks: T2344
Differential Revision: https://secure.phabricator.com/D4549
Conflicts:
src/view/page/PhabricatorStandardPageView.php
Summary: Fixed T2349
Test Plan:
Could not visibly see version at footer any more. Appeared in the top of /config.
Does not appear as a config option in /config.
Reviewers: epriestley
CC: aran, Korvin
Maniphest Tasks: T2349
Differential Revision: https://secure.phabricator.com/D4539
Summary:
Currently, we have a "config" panel in DarkConsole. It's useful to have a table of all effective config values, but it doesn't need to be in DarkConsole. Move it to Config instead. Basically:
- You don't need to activate DarkConsole to see it anymore;
- now visible only to admins;
- respects config mask/hide;
- somewhat prettier;
- links to config edit;
- no longer ships down on every DarkConsole request with a giant table of rarely-used data.
Test Plan: Looked at the table. Looked at lack of table in darkconsole.
Reviewers: codeblock, btrahan
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4473
Summary:
Fixes the two-level nav issue introduced by D4376.
(My claim that this page is device ready in the code is something of a lie, but it's fairly close.)
(@chad, this could use an icon at some point, or you can point me at which one you want and I can take a stab at slicing it.)
Test Plan: Looked at feed; saw it not-broken. Also checked public feed (which should just merge at some point).
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4381
Summary:
In the past, we did some additional magic on `$response_string` (adding profiling headers? Or DarkConsole?), so we could not share the pathway with HTTPSink. We no longer do this; share the pathways.
Also remove error handler initialization (duplicated in PhabricatorEnv), and move $sink initialization earlier. My general goal here is to allow PhabricatorSetup to emit a normal Response object and share as much code as possible with normal pages.
Test Plan: Loaded page.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2228
Differential Revision: https://secure.phabricator.com/D4285
Summary:
We have a lot of mess to get through before we can load libphutil and enter Phabricator code properly. Move it to a dedicated class.
I'm probably going to merge PhabricatorRequestOverseer into this, although the check that lives there now is kind of weird. It also does not really need to be a pre-load check and could be handled better.
I stopped shoving stuff in here once I got to ENV stuff, I'm going to tackle that next.
Test Plan: Ran phabricator normally; introduced fatals and misconfigurations. Grepped for changed symbols.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, asherkin
Maniphest Tasks: T2223
Differential Revision: https://secure.phabricator.com/D4282
Summary:
When previewing, save drafts. When loading objects, restore drafts if they are available.
Depends on: D665
Test Plan:
- Viewed a Mock.
- Typed text into the comment box.
- Reloaded the page.
- Text still there.
- Hit submit, got my comment.
- Reloaded the page.
- Draft correctly deleted.
- Repeated for Macros.
Reviewers: btrahan, chad, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2104
Differential Revision: https://secure.phabricator.com/D4252
Summary:
Implements previews for Macros and Pholio.
(Design is nonfinal -- kind of split the difference between `diff_full_view.png`, laziness, and space concerns. Next couple diffs will add more stuff here.)
Test Plan: {F28055}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran, vrana
Maniphest Tasks: T2104
Differential Revision: https://secure.phabricator.com/D4246
