Summary:
Depends on D18702. Ref T13008. This replaces the old hard-coded single rate limit with multiple flexible limits, and defines two types of limits:
- Rate: reject requests if a client has completed too many requests recently.
- Connection: reject requests if a client has too many more connections than disconnections recently.
The connection limit adds +1 to the score for each connection, then adds -1 for each disconnection. So the overall number is how many open connections they have, at least approximately.
Supporting multiple limits will let us do limiting by Hostname and by remote address (e.g., a specific IP can't exceed a low limit, and all requests to a hostname can't exceed a higher limit).
Configuring the new limits looks something like this:
```
PhabricatorStartup::addRateLimit(new PhabricatorClientRateLimit())
->setLimitKey('rate')
->setClientKey($_SERVER['REMOTE_ADDR'])
->setLimit(5);
PhabricatorStartup::addRateLimit(new PhabricatorClientConnectionLimit())
->setLimitKey('conn')
->setClientKey($_SERVER['REMOTE_ADDR'])
->setLimit(2);
```
Test Plan:
- Configured limits as above.
- Made a lot of requests, got cut off by the rate limit.
- Used `curl --limit-rate -F 'data=@the_letter_m.txt' ...` to upload files really slowly. Got cut off by the connection limit. With `enable_post_data_reading` off, this correctly killed the connections //before// the uploads finished.
- I'll send this stuff to `secure` before production to give it more of a chance.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13008
Differential Revision: https://secure.phabricator.com/D18703
Summary:
Ref T13008. Depends on D18701. The overall goal here is to make turning `enable_post_data_reading` off not break things, so we can run rate limiting checks before we read file uploads.
The biggest blocker for this is that turning it off stops `$_FILES` from coming into existence.
This //appears// to mostly work. Specifically:
- Skip the `max_post_size` check when POST is off, since it's meaningless.
- Don't read or scrub $_POST at startup when POST is off.
- When we rebuild REQUEST and POST before processing requests, do multipart parsing if we need to and rebuild FILES.
- Skip the `is_uploaded_file()` check if we built FILES ourselves.
This probably breaks a couple of small things, like maybe `__profile__` and other DarkConsole triggers over POST, and probably some other weird stuff. The parsers may also need more work than they've received so far.
I also need to verify that this actually works (i.e., lets us run code without reading the request body) but I'll include that in the change where I update the actual rate limiting.
Test Plan:
- Disabled `enable_post_data_reading`.
- Uploaded a file with a vanilla upload form (project profile image).
- Uploaded a file with drag and drop.
- Used DarkConsole.
- Submitted comments.
- Created a task.
- Browsed around.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13008
Differential Revision: https://secure.phabricator.com/D18702
Summary:
Ref T12612. This updates the rate limiting code to:
- Support a customizable token, like the client's X-Forwarded-For address, rather than always using `REMOTE_ADDR`.
- Support APCu.
- Report a little more rate limiting information.
- Not reference nonexistent documentation (removed in D16403).
I'm planning to put this into production on `secure` for now and then we can deploy it more broadly if things work well.
Test Plan:
- Enabled it locally, used `ab -n 100` to hit the limit, saw the limit enforced.
- Waited a while, was allowed to browse again.
Reviewers: chad, amckinley
Reviewed By: amckinley
Maniphest Tasks: T12612
Differential Revision: https://secure.phabricator.com/D17758
Summary:
Fixes T12013. Send either "Content-Length" or enable output compression, but not both.
Prefer compression for static resources (CSS, JS, etc).
Test Plan: Ran `curl -v ...`, no longer saw responses with both compression and `Content-Length`.
Reviewers: chad, avivey
Reviewed By: avivey
Subscribers: avivey
Maniphest Tasks: T12013
Differential Revision: https://secure.phabricator.com/D17045
Summary:
Ref T11553. With some regularity, users make various configuration mistakes which we can detect by making a request to ourselves.
I use a magical header to make this request because we want to test everything else (parameters, path).
- Fixes T4854, probably. Tries to detect mod_pagespeed by looking for a header. This is a documentation-based "fix", I didn't actually install mod_pagespeed or formally test this.
- Fixes T6866. We now test for parameters (e.g., user somehow lost "QSA").
- Ref T6709. We now test that stuff is decoded exactly once (e.g., user somehow lost "B").
- Fixes T4921. We now test that Authorization survives the request.
- Fixes T2226. Adds a setup check to determine whether gzip is enabled on the web server, and attempts to enable it at the PHP level.
- Fixes `<space space newline newline space><?php` in `preamble.php`.
Test Plan: Tested all of these setup warnings, although mostly by faking them.
Reviewers: joshuaspence, chad
Reviewed By: chad
Subscribers: Korvin
Maniphest Tasks: T4854, T4921, T6709, T6866, T11553, T2226
Differential Revision: https://secure.phabricator.com/D12622
Summary:
See accompanying discussion in T11359.
As far as I can tell we aren't vulnerable, but subprocesses could be (now, or in the future). Reject any request which may have a `Proxy:` header.
This will also do a false-positive reject if `HTTP_PROXY` is defined in the environment, but this is likely a misconfiguration (cURL does not read it). I'll provide guidance on this.
Test Plan:
- Made requests using `curl -H Proxy:...`, got rejected.
- Made normal requests, got normal pages.
Reviewers: chad, avivey
Reviewed By: avivey
Differential Revision: https://secure.phabricator.com/D16318
Summary:
Ref T10604. This uses the new standalone stream reader introduced in D15483 to read request data, instead of putting the logic in PhabricatorStartup.
It also doesn't read request data until it specifically needs to. This supports, e.g., streaming Git LFS PUT requests, and streaming more types of requests in the future.
Test Plan: See D15483. Made various different types of requests and wasn't immediately able to break anything.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10604
Differential Revision: https://secure.phabricator.com/D15484
Summary:
Ref T10264. Under PHP 5.6, you are no longer allowed to use `compress.zlib://php://input` as an argument to either `fopen()` or `file_get_contents()`.
Instead, open `php://input` as a file handle, then add `zlib.inflate` as a stream wrapper. This requires some level of magic to work properly.
Test Plan:
First, I constructed a synthetic gzipped payload by typing some words into a file and using `gzcompress()` to compress it.
Then I used a `curl` command like this to make requests with it:
```
$ curl -X POST -H "Content-Length: 66" -H "Content-Type: text/plain" -H "Content-Encoding: gzip" --data-binary @payload.deflate -v http://127.0.0.1/
```
I modified Phabricator to just dump the raw request body and exit, and reproduced the issue under PHP 5.6 (no body, error in log) by brining up a micro instance in EC2 and installing php56 on it.
After this patch, it dumped the body properly instead, and PHP 5.5 also continued worked properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10264
Differential Revision: https://secure.phabricator.com/D15314
Summary:
Fixes T10228. When we receive a gzipped request (rare, but `git` may send them), decode it before providing it to the application.
This fixes the issue with proxying certain requests described in T10228.
Test Plan:
- Applied this fix in production.
- Cloned a problem repository cleanly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10228
Differential Revision: https://secure.phabricator.com/D15145
Summary: Ref T8588. It looks like something slow is happening //before// we start DarkConsole. Add some crude reporting to try to narrow it down.
Test Plan: {F743050}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8588
Differential Revision: https://secure.phabricator.com/D13956
Summary:
Ref T8424. Fixes T7114. This was envisioned as a per-request cache for reusing interpreters, but isn't a good fit for that in modern Phabricator.
In particular, it isn't loaded by the daemons, but they have equal need for per-request caching.
Since I finally need such a cache for Spaces, throw the old stuff away before I built a more modern cache.
Also resolves T7114 by dropping filtering on $_SERVER. I'm pretty sure this is the simplest fix, see D12977 for a bit more discussion.
Test Plan: Called `didFatal()` from somewhere in normal code and verified it was able to use the access log.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7114, T8424
Differential Revision: https://secure.phabricator.com/D13152
Summary: Use `__CLASS__` instead of hard-coding class names. Depends on D12605.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12806
Summary:
Caught this in the production error logs. We can end up with `argv` defined and set to an array in an nginx + php-fpm configuration.
When we later run `ExecFuture` subprocesses, they won't be able to forward the value.
The error this produces looks like this:
```
015/04/27 12:17:35 [error] 10948#0: *674 FastCGI sent in stderr: "PHP message: [2015-04-27 12:17:35] ERROR 8: Array to string conversion at [/core/lib/libphutil/src/future/exec/ExecFuture.php:667]
PHP message: arcanist(head=master, ref.master=805ae12408e8), phabricator(head=master, ref.master=8ce8a761efe9), phutil(head=master, ref.master=fccf03d48e08)
PHP message: #0 ExecFuture::isReady() called at [<phutil>/src/future/Future.php:39]
PHP message: #1 Future::resolve(NULL) called at [<phutil>/src/future/exec/ExecFuture.php:413]
PHP message: #2 ExecFuture::resolvex() called at [<phabricator>/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php:40]
PHP message: #3 DiffusionGitRawDiffQuery::executeQuery() called at [<phabricator>/src/applications/diffusion/query/rawdiff/DiffusionRawDiffQuery.php:17]
PHP message: #4 DiffusionRawDiffQuery::loadRawDiff() called at [<phabricator>/src/applications/diffusion/conduit/DiffusionRawDiffQueryConduitAPIMethod.php:56]
PHP message: #5 DiffusionRawDiffQueryConduitAPIMethod::getResult(ConduitAPIRequest) called at [<phabricator>/src/applications/diffusion/conduit/DiffusionQueryConduitAPIMethod.php:135]
PHP message: #6 DiffusionQueryConduitAPIMethod::execute(ConduitAPIRequest) called at [<phabricator>/src/applications/conduit/method/ConduitAPIMethod.php:90]
PHP message: #7 ConduitAPIMethod::executeMethod(ConduitAPIRequest) called at [<phabricator>/src/applications/conduit/call/ConduitCall.php:134]
PHP message: #8 ConduitCall::executeMethod() called at [<phabricator>/src/applications/conduit/call/ConduitCall.php:84]
PHP message: #9 ConduitCall::execute() called at [<phabricator>/src/applications/diffusion/query/DiffusionQuery.php:81]
PHP message: #10 DiffusionQuery::callConduitWithDiffusionRequest(PhabricatorUser, DiffusionGitRequest, string, array) called at [<phabricator>/src/applications/diffusion/controller/DiffusionController.php:184]
PHP message: #11 DiffusionController::callConduitWithDiffusionRequest(string, array) called at [<phabricat
```
Test Plan: I'm just going to push this to make sure it fixes things, since I can't repro it locally.
Reviewers: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12571
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:
Fixes T6044. We've had two cases (both the same install, coincidentally) where pages got hung doing too much data fetching.
When pages hang, we don't get a useful stack trace out of them, since nginx, php-fpm, or PHP eventually terminates things in a non-useful way without any diagnostic information.
The second time (the recent Macros issue) I was able to walk the install through removing limits on nginx, php-fpm, php, and eventually getting a profile by letting the page run for several minutes until the request completed. However, this install is exceptionally technically proficient and this was still a big pain for everyone, and this approach would not have worked if the page actually looped rather than just taking a long time.
Provide `debug.time-limit`, which should give us a better tool for reacting to this situation: by setting it to a small value (like 10), we'll kill the page after 10 seconds with a trace, before nginx/php-fpm/php/etc can kill it uselessly. Hopefully that will be enough information to find the issue (generally, getting a trace has been 95% of the problem in the two cases we've encountered).
Test Plan: Set this option to `3` and added a sleep loop, saw a termination after 3 seconds with a useful trace.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: csilvers, joshuaspence, epriestley
Maniphest Tasks: T6044
Differential Revision: https://secure.phabricator.com/D10465
Summary: Fixes T6011. See that task for discussion. We can detect when `memory_limit` will be the limiting factor for drag-and-drop uploads and warn administrators about it.
Test Plan: Fiddled configuration values and hit, then resolved, the issue.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6011
Differential Revision: https://secure.phabricator.com/D10413
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: This is still too rough to enable by default. We can turn it on for `secure.phabricator.com` and tweak it a bit first, and then release it in general with higher defaults and more sensible behavior in edge cases.
Test Plan: Loaded some pages.
Reviewers: btrahan, spicyj
Reviewed By: spicyj
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8736
Summary:
Fixes T3923. On `secure.phabricator.com`, we occasionally get slowed to a crawl when someone runs a security scanner against us, or 5 search bots decide to simultaneously index every line of every file in Diffusion.
Every time a user makes a request, give their IP address some points. If they get too many points in 5 minutes, start blocking their requests automatically for a while.
We give fewer points for logged in requests. We could futher refine this (more points for a 404, more points for a really slow page, etc.) but let's start simply.
Also, provide a mechanism for configuring this, and configuring the LB environment stuff at the same time (this comes up rarely, but we don't have a good answer right now).
Test Plan: Used `ab` and reloading over and over again to hit rate limits. Read documentation.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T3923
Differential Revision: https://secure.phabricator.com/D8713
Summary: See D8049. Same deal as that one, but this is in the Phabricator web stack.
Test Plan: Man oh man.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8050
Summary:
See <https://github.com/facebook/phabricator/issues/487>. If an exception is thrown too high in the stack for the main exception handling to deal with it, we currently never report a stack trace. Instead:
- Always report a stack trace to the error log.
- With developer mode, also report a stack trace to the screen.
Test Plan: Added a high-level `throw` and hit both cases. Got traces in the log and traces-under-developer-mode on screen.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8022
Summary: Ref T3684 for discussion. This could be cleaned up a bit (it would be nice to draw entropy once per request, for instance, and maybe respect CSRF_TOKEN_LENGTH more closely) but should effectively mitigate BREACH.
Test Plan: Submitted forms; submitted forms after mucking with CSRF and observed CSRF error. Verified that source now has "B@..." tokens.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3684
Differential Revision: https://secure.phabricator.com/D6686
Summary: Ensures that weird behavior from filter_input_array does not remove keys from superglobals. Should fix T3677.
Test Plan:
Checked that $_SERVER contained same number of keys before and after
filtering, and that those affected by the original bug continue to be filtered
correctly.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: zorfling, aran, Korvin, wez
Maniphest Tasks: T3677
Differential Revision: https://secure.phabricator.com/D6680
Summary:
Fixes issue where double-encoding of $_SERVER occurs when php.ini forces all input to be sanitized
Ex:
filter.default = full_special_chars
filter.default_flags = 36
Fix line length
Test Plan: Encountered issue on clean install when registring new user (phusr not defined for email verification). php.ini on that server contains above filter settings. nginx/php-fpm with recommended settings for that server block from setup guide.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D6672
Summary:
These versions are broken, but package distros seem to be picking them up. :/
Since the error you get is completely useless, fatal immediately with a useful message.
Ref T2594.
Test Plan: Faked verisions and hit the issue.
Reviewers: btrahan
Reviewed By: btrahan
CC: brennantaylor, Arijit, aran
Maniphest Tasks: T2594
Differential Revision: https://secure.phabricator.com/D6415
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: See D5874. PhutilDeferredLog's exception on `write()`/`__destruct()` is not especially important and can come at an awkward time. Instead of throwing, just emit an error message to the log.
Test Plan: Faked failed writes and saw an error message in the log instead of many terrible things everywhere.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3144
Differential Revision: https://secure.phabricator.com/D5875
Summary: Fixes T2930. We may not have an access log here yet -- I got a little too overeager with D5533.
Test Plan:
Faked gpc quotes and got a nice error instead of a blank screen:
{F39821}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2930
Differential Revision: https://secure.phabricator.com/D5630
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:
The fixed-position side nav background thing tends to make looking at print_r() output hard. Also, it breaks Ajax, etc.
- Loudly call out unexpected output on normal pages, to catch extra spaces before `<?php`, etc.
- Display unexpected output in an attractive panel on normal pages.
- Log unexpected output instead of breaking Ajax.
Test Plan:
{F32267}
Also triggered various fatals and verified they still show the right messages (no blank pages).
Reviewers: vrana, btrahan, chad
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4892
Summary: This particular misconfiguration results in a difficult-to-debug redirect loop, so stop it early.
Test Plan: Broke my rewrite rule, verified I got yelled at.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4591
Summary:
We have a bunch of code duplication now between __init_script__.php and webroot/index.php. Consoldiate these methods and move them into PhabricatorEnv.
Merge PhabricatorRequestOverseer into PhabricatorStartup.
Test Plan: Loaded page, ran script. Wiped PHABRICATOR_ENV; loaded page, ran script; got errors.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2223
Differential Revision: https://secure.phabricator.com/D4283
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
