Provide a setting which forces all file views to be served from an alternate
domain
Summary:
See D758, D759.
- Provide a strongly recommended setting which permits configuration of an
alternate domain.
- Lock cookies down better: set them on the exact domain, and use SSL-only if
the configuration is HTTPS.
- Prevent Phabriator from setting cookies on other domains.
This assumes D759 will land, it is not effective without that change.
Test Plan:
- Attempted to login from a different domain and was rejected.
- Logged out, logged back in normally.
- Put install in setup mode and verified it revealed a warning.
- Configured an alterate domain.
- Tried to view an image with an old URI, got a 400.
- Went to /files/ and verified links rendered to the alternate domain.
- Viewed an alternate domain file.
- Tried to view an alternate domain file without the secret key, got a 404.
Reviewers: andrewjcg, erling, aran, tuomaspelkonen, jungejason, codeblock
CC: aran
Differential Revision: 760
2011-08-01 22:24:00 -07:00
|
|
|
<?php
|
|
|
|
|
Move ALL files to serve from the alternate file domain, not just files without
"Content-Disposition: attachment"
Summary:
We currently serve some files off the primary domain (with "Content-Disposition:
attachment" + a CSRF check) and some files off the alternate domain (without
either).
This is not sufficient, because some UAs (like the iPad) ignore
"Content-Disposition: attachment". So there's an attack that goes like this:
- Alice uploads xss.html
- Alice says to Bob "hey download this file on your iPad"
- Bob clicks "Download" on Phabricator on his iPad, gets XSS'd.
NOTE: This removes the CSRF check for downloading files. The check is nice to
have but only raises the barrier to entry slightly. Between iPad / sniffing /
flash bytecode attacks, single-domain installs are simply insecure. We could
restore the check at some point in conjunction with a derived authentication
cookie (i.e., a mini-session-token which is only useful for downloading files),
but that's a lot of complexity to drop all at once.
(Because files are now authenticated only by knowing the PHID and secret key,
this also fixes the "no profile pictures in public feed while logged out"
issue.)
Test Plan: Viewed, info'd, and downloaded files
Reviewers: btrahan, arice, alok
Reviewed By: arice
CC: aran, epriestley
Maniphest Tasks: T843
Differential Revision: https://secure.phabricator.com/D1608
2012-02-14 14:52:27 -08:00
|
|
|
final class PhabricatorFileDataController extends PhabricatorFileController {
|
Provide a setting which forces all file views to be served from an alternate
domain
Summary:
See D758, D759.
- Provide a strongly recommended setting which permits configuration of an
alternate domain.
- Lock cookies down better: set them on the exact domain, and use SSL-only if
the configuration is HTTPS.
- Prevent Phabriator from setting cookies on other domains.
This assumes D759 will land, it is not effective without that change.
Test Plan:
- Attempted to login from a different domain and was rejected.
- Logged out, logged back in normally.
- Put install in setup mode and verified it revealed a warning.
- Configured an alterate domain.
- Tried to view an image with an old URI, got a 400.
- Went to /files/ and verified links rendered to the alternate domain.
- Viewed an alternate domain file.
- Tried to view an alternate domain file without the secret key, got a 404.
Reviewers: andrewjcg, erling, aran, tuomaspelkonen, jungejason, codeblock
CC: aran
Differential Revision: 760
2011-08-01 22:24:00 -07:00
|
|
|
|
|
|
|
private $phid;
|
|
|
|
private $key;
|
2015-03-13 11:30:24 -07:00
|
|
|
private $file;
|
Provide a setting which forces all file views to be served from an alternate
domain
Summary:
See D758, D759.
- Provide a strongly recommended setting which permits configuration of an
alternate domain.
- Lock cookies down better: set them on the exact domain, and use SSL-only if
the configuration is HTTPS.
- Prevent Phabriator from setting cookies on other domains.
This assumes D759 will land, it is not effective without that change.
Test Plan:
- Attempted to login from a different domain and was rejected.
- Logged out, logged back in normally.
- Put install in setup mode and verified it revealed a warning.
- Configured an alterate domain.
- Tried to view an image with an old URI, got a 400.
- Went to /files/ and verified links rendered to the alternate domain.
- Viewed an alternate domain file.
- Tried to view an alternate domain file without the secret key, got a 404.
Reviewers: andrewjcg, erling, aran, tuomaspelkonen, jungejason, codeblock
CC: aran
Differential Revision: 760
2011-08-01 22:24:00 -07:00
|
|
|
|
|
|
|
public function shouldRequireLogin() {
|
|
|
|
return false;
|
|
|
|
}
|
|
|
|
|
2017-11-27 17:53:58 -08:00
|
|
|
public function shouldAllowPartialSessions() {
|
|
|
|
return true;
|
|
|
|
}
|
|
|
|
|
2015-07-27 09:41:53 -07:00
|
|
|
public function handleRequest(AphrontRequest $request) {
|
|
|
|
$viewer = $request->getViewer();
|
|
|
|
$this->phid = $request->getURIData('phid');
|
|
|
|
$this->key = $request->getURIData('key');
|
Provide a setting which forces all file views to be served from an alternate
domain
Summary:
See D758, D759.
- Provide a strongly recommended setting which permits configuration of an
alternate domain.
- Lock cookies down better: set them on the exact domain, and use SSL-only if
the configuration is HTTPS.
- Prevent Phabriator from setting cookies on other domains.
This assumes D759 will land, it is not effective without that change.
Test Plan:
- Attempted to login from a different domain and was rejected.
- Logged out, logged back in normally.
- Put install in setup mode and verified it revealed a warning.
- Configured an alterate domain.
- Tried to view an image with an old URI, got a 400.
- Went to /files/ and verified links rendered to the alternate domain.
- Viewed an alternate domain file.
- Tried to view an alternate domain file without the secret key, got a 404.
Reviewers: andrewjcg, erling, aran, tuomaspelkonen, jungejason, codeblock
CC: aran
Differential Revision: 760
2011-08-01 22:24:00 -07:00
|
|
|
|
Move ALL files to serve from the alternate file domain, not just files without
"Content-Disposition: attachment"
Summary:
We currently serve some files off the primary domain (with "Content-Disposition:
attachment" + a CSRF check) and some files off the alternate domain (without
either).
This is not sufficient, because some UAs (like the iPad) ignore
"Content-Disposition: attachment". So there's an attack that goes like this:
- Alice uploads xss.html
- Alice says to Bob "hey download this file on your iPad"
- Bob clicks "Download" on Phabricator on his iPad, gets XSS'd.
NOTE: This removes the CSRF check for downloading files. The check is nice to
have but only raises the barrier to entry slightly. Between iPad / sniffing /
flash bytecode attacks, single-domain installs are simply insecure. We could
restore the check at some point in conjunction with a derived authentication
cookie (i.e., a mini-session-token which is only useful for downloading files),
but that's a lot of complexity to drop all at once.
(Because files are now authenticated only by knowing the PHID and secret key,
this also fixes the "no profile pictures in public feed while logged out"
issue.)
Test Plan: Viewed, info'd, and downloaded files
Reviewers: btrahan, arice, alok
Reviewed By: arice
CC: aran, epriestley
Maniphest Tasks: T843
Differential Revision: https://secure.phabricator.com/D1608
2012-02-14 14:52:27 -08:00
|
|
|
$alt = PhabricatorEnv::getEnvConfig('security.alternate-file-domain');
|
2014-08-11 07:08:58 -07:00
|
|
|
$base_uri = PhabricatorEnv::getEnvConfig('phabricator.base-uri');
|
|
|
|
$alt_uri = new PhutilURI($alt);
|
|
|
|
$alt_domain = $alt_uri->getDomain();
|
|
|
|
$req_domain = $request->getHost();
|
|
|
|
$main_domain = id(new PhutilURI($base_uri))->getDomain();
|
|
|
|
|
Never generate file download forms which point to the CDN domain, tighten "form-action" CSP
Summary:
Depends on D19155. Ref T13094. Ref T4340.
We can't currently implement a strict `form-action 'self'` content security policy because some file downloads rely on a `<form />` which sometimes POSTs to the CDN domain.
Broadly, stop generating these forms. We just redirect instead, and show an interstitial confirm dialog if no CDN domain is configured. This makes the UX for installs with no CDN domain a little worse and the UX for everyone else better.
Then, implement the stricter Content-Security-Policy.
This also removes extra confirm dialogs for downloading Harbormaster build logs and data exports.
Test Plan:
- Went through the plain data export, data export with bulk jobs, ssh key generation, calendar ICS download, Diffusion data, Paste data, Harbormaster log data, and normal file data download workflows with a CDN domain.
- Went through all those workflows again without a CDN domain.
- Grepped for affected symbols (`getCDNURI()`, `getDownloadURI()`).
- Added an evil form to a page, tried to submit it, was rejected.
- Went through the ReCaptcha and Stripe flows again to see if they're submitting any forms.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13094, T4340
Differential Revision: https://secure.phabricator.com/D19156
2018-02-28 15:22:10 -08:00
|
|
|
$request_kind = $request->getURIData('kind');
|
|
|
|
$is_download = ($request_kind === 'download');
|
|
|
|
|
Don't require one-time tokens to view file resources
Summary:
Ref T10262. This removes one-time tokens and makes file data responses always-cacheable (for 30 days).
The URI will stop working once any attached object changes its view policy, or the file view policy itself changes.
Files with `canCDN` (totally public data like profile images, CSS, JS, etc) use "cache-control: public" so they can be CDN'd.
Files without `canCDN` use "cache-control: private" so they won't be cached by the CDN. They could still be cached by a misbehaving local cache, but if you don't want your users seeing one anothers' secret files you should configure your local network properly.
Our "Cache-Control" headers were also from 1999 or something, update them to be more modern/sane. I can't find any evidence that any browser has done the wrong thing with this simpler ruleset in the last ~10 years.
Test Plan:
- Configured alternate file domain.
- Viewed site: stuff worked.
- Accessed a file on primary domain, got redirected to alternate domain.
- Verified proper cache headers for `canCDN` (public) and non-`canCDN` (private) files.
- Uploaded a file to a task, edited task policy, verified it scrambled the old URI.
- Reloaded task, new URI generated transparently.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10262
Differential Revision: https://secure.phabricator.com/D15642
2016-04-06 13:06:34 -07:00
|
|
|
if (!strlen($alt) || $main_domain == $alt_domain) {
|
|
|
|
// No alternate domain.
|
|
|
|
$should_redirect = false;
|
|
|
|
$is_alternate_domain = false;
|
2014-08-11 07:08:58 -07:00
|
|
|
} else if ($req_domain != $alt_domain) {
|
Don't require one-time tokens to view file resources
Summary:
Ref T10262. This removes one-time tokens and makes file data responses always-cacheable (for 30 days).
The URI will stop working once any attached object changes its view policy, or the file view policy itself changes.
Files with `canCDN` (totally public data like profile images, CSS, JS, etc) use "cache-control: public" so they can be CDN'd.
Files without `canCDN` use "cache-control: private" so they won't be cached by the CDN. They could still be cached by a misbehaving local cache, but if you don't want your users seeing one anothers' secret files you should configure your local network properly.
Our "Cache-Control" headers were also from 1999 or something, update them to be more modern/sane. I can't find any evidence that any browser has done the wrong thing with this simpler ruleset in the last ~10 years.
Test Plan:
- Configured alternate file domain.
- Viewed site: stuff worked.
- Accessed a file on primary domain, got redirected to alternate domain.
- Verified proper cache headers for `canCDN` (public) and non-`canCDN` (private) files.
- Uploaded a file to a task, edited task policy, verified it scrambled the old URI.
- Reloaded task, new URI generated transparently.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10262
Differential Revision: https://secure.phabricator.com/D15642
2016-04-06 13:06:34 -07:00
|
|
|
// Alternate domain, but this request is on the main domain.
|
|
|
|
$should_redirect = true;
|
|
|
|
$is_alternate_domain = false;
|
|
|
|
} else {
|
|
|
|
// Alternate domain, and on the alternate domain.
|
|
|
|
$should_redirect = false;
|
|
|
|
$is_alternate_domain = true;
|
|
|
|
}
|
2014-08-11 07:08:58 -07:00
|
|
|
|
2016-04-22 04:40:17 -07:00
|
|
|
$response = $this->loadFile();
|
Don't require one-time tokens to view file resources
Summary:
Ref T10262. This removes one-time tokens and makes file data responses always-cacheable (for 30 days).
The URI will stop working once any attached object changes its view policy, or the file view policy itself changes.
Files with `canCDN` (totally public data like profile images, CSS, JS, etc) use "cache-control: public" so they can be CDN'd.
Files without `canCDN` use "cache-control: private" so they won't be cached by the CDN. They could still be cached by a misbehaving local cache, but if you don't want your users seeing one anothers' secret files you should configure your local network properly.
Our "Cache-Control" headers were also from 1999 or something, update them to be more modern/sane. I can't find any evidence that any browser has done the wrong thing with this simpler ruleset in the last ~10 years.
Test Plan:
- Configured alternate file domain.
- Viewed site: stuff worked.
- Accessed a file on primary domain, got redirected to alternate domain.
- Verified proper cache headers for `canCDN` (public) and non-`canCDN` (private) files.
- Uploaded a file to a task, edited task policy, verified it scrambled the old URI.
- Reloaded task, new URI generated transparently.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10262
Differential Revision: https://secure.phabricator.com/D15642
2016-04-06 13:06:34 -07:00
|
|
|
if ($response) {
|
|
|
|
return $response;
|
|
|
|
}
|
2014-08-11 07:08:58 -07:00
|
|
|
|
Don't require one-time tokens to view file resources
Summary:
Ref T10262. This removes one-time tokens and makes file data responses always-cacheable (for 30 days).
The URI will stop working once any attached object changes its view policy, or the file view policy itself changes.
Files with `canCDN` (totally public data like profile images, CSS, JS, etc) use "cache-control: public" so they can be CDN'd.
Files without `canCDN` use "cache-control: private" so they won't be cached by the CDN. They could still be cached by a misbehaving local cache, but if you don't want your users seeing one anothers' secret files you should configure your local network properly.
Our "Cache-Control" headers were also from 1999 or something, update them to be more modern/sane. I can't find any evidence that any browser has done the wrong thing with this simpler ruleset in the last ~10 years.
Test Plan:
- Configured alternate file domain.
- Viewed site: stuff worked.
- Accessed a file on primary domain, got redirected to alternate domain.
- Verified proper cache headers for `canCDN` (public) and non-`canCDN` (private) files.
- Uploaded a file to a task, edited task policy, verified it scrambled the old URI.
- Reloaded task, new URI generated transparently.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10262
Differential Revision: https://secure.phabricator.com/D15642
2016-04-06 13:06:34 -07:00
|
|
|
$file = $this->getFile();
|
2015-03-01 12:12:45 -08:00
|
|
|
|
Don't require one-time tokens to view file resources
Summary:
Ref T10262. This removes one-time tokens and makes file data responses always-cacheable (for 30 days).
The URI will stop working once any attached object changes its view policy, or the file view policy itself changes.
Files with `canCDN` (totally public data like profile images, CSS, JS, etc) use "cache-control: public" so they can be CDN'd.
Files without `canCDN` use "cache-control: private" so they won't be cached by the CDN. They could still be cached by a misbehaving local cache, but if you don't want your users seeing one anothers' secret files you should configure your local network properly.
Our "Cache-Control" headers were also from 1999 or something, update them to be more modern/sane. I can't find any evidence that any browser has done the wrong thing with this simpler ruleset in the last ~10 years.
Test Plan:
- Configured alternate file domain.
- Viewed site: stuff worked.
- Accessed a file on primary domain, got redirected to alternate domain.
- Verified proper cache headers for `canCDN` (public) and non-`canCDN` (private) files.
- Uploaded a file to a task, edited task policy, verified it scrambled the old URI.
- Reloaded task, new URI generated transparently.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10262
Differential Revision: https://secure.phabricator.com/D15642
2016-04-06 13:06:34 -07:00
|
|
|
if ($should_redirect) {
|
2012-08-24 12:25:51 -07:00
|
|
|
return id(new AphrontRedirectResponse())
|
2014-08-18 14:11:06 -07:00
|
|
|
->setIsExternal(true)
|
Never generate file download forms which point to the CDN domain, tighten "form-action" CSP
Summary:
Depends on D19155. Ref T13094. Ref T4340.
We can't currently implement a strict `form-action 'self'` content security policy because some file downloads rely on a `<form />` which sometimes POSTs to the CDN domain.
Broadly, stop generating these forms. We just redirect instead, and show an interstitial confirm dialog if no CDN domain is configured. This makes the UX for installs with no CDN domain a little worse and the UX for everyone else better.
Then, implement the stricter Content-Security-Policy.
This also removes extra confirm dialogs for downloading Harbormaster build logs and data exports.
Test Plan:
- Went through the plain data export, data export with bulk jobs, ssh key generation, calendar ICS download, Diffusion data, Paste data, Harbormaster log data, and normal file data download workflows with a CDN domain.
- Went through all those workflows again without a CDN domain.
- Grepped for affected symbols (`getCDNURI()`, `getDownloadURI()`).
- Added an evil form to a page, tried to submit it, was rejected.
- Went through the ReCaptcha and Stripe flows again to see if they're submitting any forms.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13094, T4340
Differential Revision: https://secure.phabricator.com/D19156
2018-02-28 15:22:10 -08:00
|
|
|
->setURI($file->getCDNURI($request_kind));
|
Provide a setting which forces all file views to be served from an alternate
domain
Summary:
See D758, D759.
- Provide a strongly recommended setting which permits configuration of an
alternate domain.
- Lock cookies down better: set them on the exact domain, and use SSL-only if
the configuration is HTTPS.
- Prevent Phabriator from setting cookies on other domains.
This assumes D759 will land, it is not effective without that change.
Test Plan:
- Attempted to login from a different domain and was rejected.
- Logged out, logged back in normally.
- Put install in setup mode and verified it revealed a warning.
- Configured an alterate domain.
- Tried to view an image with an old URI, got a 400.
- Went to /files/ and verified links rendered to the alternate domain.
- Viewed an alternate domain file.
- Tried to view an alternate domain file without the secret key, got a 404.
Reviewers: andrewjcg, erling, aran, tuomaspelkonen, jungejason, codeblock
CC: aran
Differential Revision: 760
2011-08-01 22:24:00 -07:00
|
|
|
}
|
|
|
|
|
|
|
|
$response = new AphrontFileResponse();
|
Don't require one-time tokens to view file resources
Summary:
Ref T10262. This removes one-time tokens and makes file data responses always-cacheable (for 30 days).
The URI will stop working once any attached object changes its view policy, or the file view policy itself changes.
Files with `canCDN` (totally public data like profile images, CSS, JS, etc) use "cache-control: public" so they can be CDN'd.
Files without `canCDN` use "cache-control: private" so they won't be cached by the CDN. They could still be cached by a misbehaving local cache, but if you don't want your users seeing one anothers' secret files you should configure your local network properly.
Our "Cache-Control" headers were also from 1999 or something, update them to be more modern/sane. I can't find any evidence that any browser has done the wrong thing with this simpler ruleset in the last ~10 years.
Test Plan:
- Configured alternate file domain.
- Viewed site: stuff worked.
- Accessed a file on primary domain, got redirected to alternate domain.
- Verified proper cache headers for `canCDN` (public) and non-`canCDN` (private) files.
- Uploaded a file to a task, edited task policy, verified it scrambled the old URI.
- Reloaded task, new URI generated transparently.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10262
Differential Revision: https://secure.phabricator.com/D15642
2016-04-06 13:06:34 -07:00
|
|
|
$response->setCacheDurationInSeconds(60 * 60 * 24 * 30);
|
|
|
|
$response->setCanCDN($file->getCanCDN());
|
Provide a setting which forces all file views to be served from an alternate
domain
Summary:
See D758, D759.
- Provide a strongly recommended setting which permits configuration of an
alternate domain.
- Lock cookies down better: set them on the exact domain, and use SSL-only if
the configuration is HTTPS.
- Prevent Phabriator from setting cookies on other domains.
This assumes D759 will land, it is not effective without that change.
Test Plan:
- Attempted to login from a different domain and was rejected.
- Logged out, logged back in normally.
- Put install in setup mode and verified it revealed a warning.
- Configured an alterate domain.
- Tried to view an image with an old URI, got a 400.
- Went to /files/ and verified links rendered to the alternate domain.
- Viewed an alternate domain file.
- Tried to view an alternate domain file without the secret key, got a 404.
Reviewers: andrewjcg, erling, aran, tuomaspelkonen, jungejason, codeblock
CC: aran
Differential Revision: 760
2011-08-01 22:24:00 -07:00
|
|
|
|
2015-03-14 08:29:12 -07:00
|
|
|
$begin = null;
|
|
|
|
$end = null;
|
|
|
|
|
Fix two issues with audio macros
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
2013-09-28 15:32:48 -07:00
|
|
|
// NOTE: It's important to accept "Range" requests when playing audio.
|
|
|
|
// If we don't, Safari has difficulty figuring out how long sounds are
|
|
|
|
// and glitches when trying to loop them. In particular, Safari sends
|
|
|
|
// an initial request for bytes 0-1 of the audio file, and things go south
|
|
|
|
// if we can't respond with a 206 Partial Content.
|
|
|
|
$range = $request->getHTTPHeader('range');
|
2017-04-18 13:17:39 -07:00
|
|
|
if (strlen($range)) {
|
|
|
|
list($begin, $end) = $response->parseHTTPRange($range);
|
Fix two issues with audio macros
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
2013-09-28 15:32:48 -07:00
|
|
|
}
|
|
|
|
|
2018-03-23 04:50:48 -07:00
|
|
|
if (!$file->isViewableInBrowser()) {
|
|
|
|
$is_download = true;
|
|
|
|
}
|
|
|
|
|
2016-04-07 08:45:56 -07:00
|
|
|
$request_type = $request->getHTTPHeader('X-Phabricator-Request-Type');
|
|
|
|
$is_lfs = ($request_type == 'git-lfs');
|
|
|
|
|
2018-03-23 04:50:48 -07:00
|
|
|
if (!$is_download) {
|
Move ALL files to serve from the alternate file domain, not just files without
"Content-Disposition: attachment"
Summary:
We currently serve some files off the primary domain (with "Content-Disposition:
attachment" + a CSRF check) and some files off the alternate domain (without
either).
This is not sufficient, because some UAs (like the iPad) ignore
"Content-Disposition: attachment". So there's an attack that goes like this:
- Alice uploads xss.html
- Alice says to Bob "hey download this file on your iPad"
- Bob clicks "Download" on Phabricator on his iPad, gets XSS'd.
NOTE: This removes the CSRF check for downloading files. The check is nice to
have but only raises the barrier to entry slightly. Between iPad / sniffing /
flash bytecode attacks, single-domain installs are simply insecure. We could
restore the check at some point in conjunction with a derived authentication
cookie (i.e., a mini-session-token which is only useful for downloading files),
but that's a lot of complexity to drop all at once.
(Because files are now authenticated only by knowing the PHID and secret key,
this also fixes the "no profile pictures in public feed while logged out"
issue.)
Test Plan: Viewed, info'd, and downloaded files
Reviewers: btrahan, arice, alok
Reviewed By: arice
CC: aran, epriestley
Maniphest Tasks: T843
Differential Revision: https://secure.phabricator.com/D1608
2012-02-14 14:52:27 -08:00
|
|
|
$response->setMimeType($file->getViewableMimeType());
|
|
|
|
} else {
|
2017-04-04 09:06:00 -07:00
|
|
|
$is_post = $request->isHTTPPost();
|
2018-05-01 07:08:45 -07:00
|
|
|
$is_public = !$viewer->isLoggedIn();
|
2014-08-18 14:11:06 -07:00
|
|
|
|
Never generate file download forms which point to the CDN domain, tighten "form-action" CSP
Summary:
Depends on D19155. Ref T13094. Ref T4340.
We can't currently implement a strict `form-action 'self'` content security policy because some file downloads rely on a `<form />` which sometimes POSTs to the CDN domain.
Broadly, stop generating these forms. We just redirect instead, and show an interstitial confirm dialog if no CDN domain is configured. This makes the UX for installs with no CDN domain a little worse and the UX for everyone else better.
Then, implement the stricter Content-Security-Policy.
This also removes extra confirm dialogs for downloading Harbormaster build logs and data exports.
Test Plan:
- Went through the plain data export, data export with bulk jobs, ssh key generation, calendar ICS download, Diffusion data, Paste data, Harbormaster log data, and normal file data download workflows with a CDN domain.
- Went through all those workflows again without a CDN domain.
- Grepped for affected symbols (`getCDNURI()`, `getDownloadURI()`).
- Added an evil form to a page, tried to submit it, was rejected.
- Went through the ReCaptcha and Stripe flows again to see if they're submitting any forms.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13094, T4340
Differential Revision: https://secure.phabricator.com/D19156
2018-02-28 15:22:10 -08:00
|
|
|
// NOTE: Require POST to download files from the primary domain. If the
|
|
|
|
// request is not a POST request but arrives on the primary domain, we
|
|
|
|
// render a confirmation dialog. For discussion, see T13094.
|
2017-04-04 09:06:00 -07:00
|
|
|
|
2018-05-01 07:08:45 -07:00
|
|
|
// There are two exceptions to this rule:
|
|
|
|
|
|
|
|
// Git LFS requests can download with GET. This is safe (Git LFS won't
|
|
|
|
// execute files it downloads) and necessary to support Git LFS.
|
|
|
|
|
|
|
|
// Requests with no credentials may also download with GET. This
|
|
|
|
// primarily supports downloading files with `arc download` or other
|
|
|
|
// API clients. This is only "mostly" safe: if you aren't logged in, you
|
|
|
|
// are likely immune to XSS and CSRF. However, an attacker may still be
|
|
|
|
// able to set cookies on this domain (for example, to fixate your
|
|
|
|
// session). For now, we accept these risks because users running
|
|
|
|
// Phabricator in this mode are knowingly accepting a security risk
|
|
|
|
// against setup advice, and there's significant value in having
|
|
|
|
// API development against test and production installs work the same
|
|
|
|
// way.
|
|
|
|
|
|
|
|
$is_safe = ($is_alternate_domain || $is_post || $is_lfs || $is_public);
|
2017-04-04 09:06:00 -07:00
|
|
|
if (!$is_safe) {
|
Never generate file download forms which point to the CDN domain, tighten "form-action" CSP
Summary:
Depends on D19155. Ref T13094. Ref T4340.
We can't currently implement a strict `form-action 'self'` content security policy because some file downloads rely on a `<form />` which sometimes POSTs to the CDN domain.
Broadly, stop generating these forms. We just redirect instead, and show an interstitial confirm dialog if no CDN domain is configured. This makes the UX for installs with no CDN domain a little worse and the UX for everyone else better.
Then, implement the stricter Content-Security-Policy.
This also removes extra confirm dialogs for downloading Harbormaster build logs and data exports.
Test Plan:
- Went through the plain data export, data export with bulk jobs, ssh key generation, calendar ICS download, Diffusion data, Paste data, Harbormaster log data, and normal file data download workflows with a CDN domain.
- Went through all those workflows again without a CDN domain.
- Grepped for affected symbols (`getCDNURI()`, `getDownloadURI()`).
- Added an evil form to a page, tried to submit it, was rejected.
- Went through the ReCaptcha and Stripe flows again to see if they're submitting any forms.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13094, T4340
Differential Revision: https://secure.phabricator.com/D19156
2018-02-28 15:22:10 -08:00
|
|
|
return $this->newDialog()
|
|
|
|
->setSubmitURI($file->getDownloadURI())
|
|
|
|
->setTitle(pht('Download File'))
|
|
|
|
->appendParagraph(
|
|
|
|
pht(
|
|
|
|
'Download file %s (%s)?',
|
|
|
|
phutil_tag('strong', array(), $file->getName()),
|
|
|
|
phutil_format_bytes($file->getByteSize())))
|
|
|
|
->addCancelButton($file->getURI())
|
|
|
|
->addSubmitButton(pht('Download File'));
|
Move ALL files to serve from the alternate file domain, not just files without
"Content-Disposition: attachment"
Summary:
We currently serve some files off the primary domain (with "Content-Disposition:
attachment" + a CSRF check) and some files off the alternate domain (without
either).
This is not sufficient, because some UAs (like the iPad) ignore
"Content-Disposition: attachment". So there's an attack that goes like this:
- Alice uploads xss.html
- Alice says to Bob "hey download this file on your iPad"
- Bob clicks "Download" on Phabricator on his iPad, gets XSS'd.
NOTE: This removes the CSRF check for downloading files. The check is nice to
have but only raises the barrier to entry slightly. Between iPad / sniffing /
flash bytecode attacks, single-domain installs are simply insecure. We could
restore the check at some point in conjunction with a derived authentication
cookie (i.e., a mini-session-token which is only useful for downloading files),
but that's a lot of complexity to drop all at once.
(Because files are now authenticated only by knowing the PHID and secret key,
this also fixes the "no profile pictures in public feed while logged out"
issue.)
Test Plan: Viewed, info'd, and downloaded files
Reviewers: btrahan, arice, alok
Reviewed By: arice
CC: aran, epriestley
Maniphest Tasks: T843
Differential Revision: https://secure.phabricator.com/D1608
2012-02-14 14:52:27 -08:00
|
|
|
}
|
2017-04-04 09:06:00 -07:00
|
|
|
|
Move ALL files to serve from the alternate file domain, not just files without
"Content-Disposition: attachment"
Summary:
We currently serve some files off the primary domain (with "Content-Disposition:
attachment" + a CSRF check) and some files off the alternate domain (without
either).
This is not sufficient, because some UAs (like the iPad) ignore
"Content-Disposition: attachment". So there's an attack that goes like this:
- Alice uploads xss.html
- Alice says to Bob "hey download this file on your iPad"
- Bob clicks "Download" on Phabricator on his iPad, gets XSS'd.
NOTE: This removes the CSRF check for downloading files. The check is nice to
have but only raises the barrier to entry slightly. Between iPad / sniffing /
flash bytecode attacks, single-domain installs are simply insecure. We could
restore the check at some point in conjunction with a derived authentication
cookie (i.e., a mini-session-token which is only useful for downloading files),
but that's a lot of complexity to drop all at once.
(Because files are now authenticated only by knowing the PHID and secret key,
this also fixes the "no profile pictures in public feed while logged out"
issue.)
Test Plan: Viewed, info'd, and downloaded files
Reviewers: btrahan, arice, alok
Reviewed By: arice
CC: aran, epriestley
Maniphest Tasks: T843
Differential Revision: https://secure.phabricator.com/D1608
2012-02-14 14:52:27 -08:00
|
|
|
$response->setMimeType($file->getMimeType());
|
|
|
|
$response->setDownload($file->getName());
|
|
|
|
}
|
|
|
|
|
2015-03-14 08:29:12 -07:00
|
|
|
$iterator = $file->getFileDataIterator($begin, $end);
|
|
|
|
|
|
|
|
$response->setContentLength($file->getByteSize());
|
|
|
|
$response->setContentIterator($iterator);
|
|
|
|
|
2018-03-23 04:50:48 -07:00
|
|
|
// In Chrome, we must permit this domain in "object-src" CSP when serving a
|
|
|
|
// PDF or the browser will refuse to render it.
|
|
|
|
if (!$is_download && $file->isPDF()) {
|
|
|
|
$request_uri = id(clone $request->getAbsoluteRequestURI())
|
|
|
|
->setPath(null)
|
|
|
|
->setFragment(null)
|
2019-02-12 13:18:32 -08:00
|
|
|
->removeAllQueryParams();
|
2018-03-23 04:50:48 -07:00
|
|
|
|
|
|
|
$response->addContentSecurityPolicyURI(
|
|
|
|
'object-src',
|
|
|
|
(string)$request_uri);
|
|
|
|
}
|
|
|
|
|
Provide a setting which forces all file views to be served from an alternate
domain
Summary:
See D758, D759.
- Provide a strongly recommended setting which permits configuration of an
alternate domain.
- Lock cookies down better: set them on the exact domain, and use SSL-only if
the configuration is HTTPS.
- Prevent Phabriator from setting cookies on other domains.
This assumes D759 will land, it is not effective without that change.
Test Plan:
- Attempted to login from a different domain and was rejected.
- Logged out, logged back in normally.
- Put install in setup mode and verified it revealed a warning.
- Configured an alterate domain.
- Tried to view an image with an old URI, got a 400.
- Went to /files/ and verified links rendered to the alternate domain.
- Viewed an alternate domain file.
- Tried to view an alternate domain file without the secret key, got a 404.
Reviewers: andrewjcg, erling, aran, tuomaspelkonen, jungejason, codeblock
CC: aran
Differential Revision: 760
2011-08-01 22:24:00 -07:00
|
|
|
return $response;
|
|
|
|
}
|
2015-03-01 12:12:45 -08:00
|
|
|
|
2016-04-22 04:40:17 -07:00
|
|
|
private function loadFile() {
|
|
|
|
// Access to files is provided by knowledge of a per-file secret key in
|
|
|
|
// the URI. Knowledge of this secret is sufficient to retrieve the file.
|
|
|
|
|
|
|
|
// For some requests, we also have a valid viewer. However, for many
|
|
|
|
// requests (like alternate domain requests or Git LFS requests) we will
|
|
|
|
// not. Even if we do have a valid viewer, use the omnipotent viewer to
|
|
|
|
// make this logic simpler and more consistent.
|
|
|
|
|
|
|
|
// Beyond making the policy check itself more consistent, this also makes
|
2017-10-09 10:48:01 -07:00
|
|
|
// sure we're consistent about returning HTTP 404 on bad requests instead
|
2016-04-22 04:40:17 -07:00
|
|
|
// of serving HTTP 200 with a login page, which can mislead some clients.
|
|
|
|
|
|
|
|
$viewer = PhabricatorUser::getOmnipotentUser();
|
|
|
|
|
2015-03-13 11:30:24 -07:00
|
|
|
$file = id(new PhabricatorFileQuery())
|
|
|
|
->setViewer($viewer)
|
|
|
|
->withPHIDs(array($this->phid))
|
2017-04-18 11:07:24 -07:00
|
|
|
->withIsDeleted(false)
|
2015-03-13 11:30:24 -07:00
|
|
|
->executeOne();
|
|
|
|
|
|
|
|
if (!$file) {
|
|
|
|
return new Aphront404Response();
|
|
|
|
}
|
|
|
|
|
2016-04-06 16:19:03 -07:00
|
|
|
// We may be on the CDN domain, so we need to use a fully-qualified URI
|
|
|
|
// here to make sure we end up back on the main domain.
|
|
|
|
$info_uri = PhabricatorEnv::getURI($file->getInfoURI());
|
|
|
|
|
|
|
|
|
2015-03-13 11:30:24 -07:00
|
|
|
if (!$file->validateSecretKey($this->key)) {
|
2016-04-06 16:19:03 -07:00
|
|
|
$dialog = $this->newDialog()
|
|
|
|
->setTitle(pht('Invalid Authorization'))
|
|
|
|
->appendParagraph(
|
|
|
|
pht(
|
|
|
|
'The link you followed to access this file is no longer '.
|
|
|
|
'valid. The visibility of the file may have changed after '.
|
|
|
|
'the link was generated.'))
|
|
|
|
->appendParagraph(
|
|
|
|
pht(
|
|
|
|
'You can continue to the file detail page to get more '.
|
|
|
|
'information and attempt to access the file.'))
|
|
|
|
->addCancelButton($info_uri, pht('Continue'));
|
|
|
|
|
|
|
|
return id(new AphrontDialogResponse())
|
|
|
|
->setDialog($dialog)
|
|
|
|
->setHTTPResponseCode(404);
|
2015-03-13 11:30:24 -07:00
|
|
|
}
|
|
|
|
|
|
|
|
if ($file->getIsPartial()) {
|
2016-04-06 16:19:03 -07:00
|
|
|
$dialog = $this->newDialog()
|
2015-03-13 11:30:24 -07:00
|
|
|
->setTitle(pht('Partial Upload'))
|
|
|
|
->appendParagraph(
|
|
|
|
pht(
|
|
|
|
'This file has only been partially uploaded. It must be '.
|
|
|
|
'uploaded completely before you can download it.'))
|
2016-04-06 16:19:03 -07:00
|
|
|
->appendParagraph(
|
|
|
|
pht(
|
|
|
|
'You can continue to the file detail page to monitor the '.
|
|
|
|
'upload progress of the file.'))
|
|
|
|
->addCancelButton($info_uri, pht('Continue'));
|
|
|
|
|
|
|
|
return id(new AphrontDialogResponse())
|
|
|
|
->setDialog($dialog)
|
|
|
|
->setHTTPResponseCode(404);
|
2015-03-13 11:30:24 -07:00
|
|
|
}
|
|
|
|
|
|
|
|
$this->file = $file;
|
|
|
|
|
|
|
|
return null;
|
|
|
|
}
|
|
|
|
|
|
|
|
private function getFile() {
|
|
|
|
if (!$this->file) {
|
2015-05-14 07:53:52 +10:00
|
|
|
throw new PhutilInvalidStateException('loadFile');
|
2015-03-13 11:30:24 -07:00
|
|
|
}
|
|
|
|
return $this->file;
|
|
|
|
}
|
|
|
|
|
Provide a setting which forces all file views to be served from an alternate
domain
Summary:
See D758, D759.
- Provide a strongly recommended setting which permits configuration of an
alternate domain.
- Lock cookies down better: set them on the exact domain, and use SSL-only if
the configuration is HTTPS.
- Prevent Phabriator from setting cookies on other domains.
This assumes D759 will land, it is not effective without that change.
Test Plan:
- Attempted to login from a different domain and was rejected.
- Logged out, logged back in normally.
- Put install in setup mode and verified it revealed a warning.
- Configured an alterate domain.
- Tried to view an image with an old URI, got a 400.
- Went to /files/ and verified links rendered to the alternate domain.
- Viewed an alternate domain file.
- Tried to view an alternate domain file without the secret key, got a 404.
Reviewers: andrewjcg, erling, aran, tuomaspelkonen, jungejason, codeblock
CC: aran
Differential Revision: 760
2011-08-01 22:24:00 -07:00
|
|
|
}
|