diff --git a/src/applications/auth/provider/PhabricatorAuthProvider.php b/src/applications/auth/provider/PhabricatorAuthProvider.php --- a/src/applications/auth/provider/PhabricatorAuthProvider.php +++ b/src/applications/auth/provider/PhabricatorAuthProvider.php @@ -247,13 +247,19 @@ $image_uri, array( 'name' => $name, - 'canCDN' => true, + 'viewPolicy' => PhabricatorPolicies::POLICY_NOONE, )); + if ($image_file->isViewableImage()) { + $image_file + ->setViewPolicy(PhabricatorPolicies::getMostOpenPolicy()) + ->setCanCDN(true) + ->save(); + $account->setProfileImagePHID($image_file->getPHID()); + } else { + $image_file->delete(); + } unset($unguarded); - if ($image_file) { - $account->setProfileImagePHID($image_file->getPHID()); - } } catch (Exception $ex) { // Log this but proceed, it's not especially important that we // be able to pull profile images. diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -451,24 +451,93 @@ } + /** + * Download a remote resource over HTTP and save the response body as a file. + * + * This method respects `security.outbound-blacklist`, and protects against + * HTTP redirection (by manually following "Location" headers and verifying + * each destination). It does not protect against DNS rebinding. See + * discussion in T6755. + */ public static function newFromFileDownload($uri, array $params = array()) { - PhabricatorEnv::requireValidRemoteURIForFetch( - $uri, - array( - 'http', - 'https', - )); - $timeout = 5; - list($file_data) = id(new HTTPSFuture($uri)) - ->setTimeout($timeout) - ->resolvex(); - $params = $params + array( - 'name' => basename($uri), - ); + $redirects = array(); + $current = $uri; + while (true) { + try { + if (count($redirects) > 10) { + throw new Exception( + pht('Too many redirects trying to fetch remote URI.')); + } - return self::newFromFileData($file_data, $params); + PhabricatorEnv::requireValidRemoteURIForFetch( + $current, + array( + 'http', + 'https', + )); + + list($status, $body, $headers) = id(new HTTPSFuture($current)) + ->setFollowLocation(false) + ->setTimeout($timeout) + ->resolve(); + + if ($status->isRedirect()) { + // This is an HTTP 3XX status, so look for a "Location" header. + $location = null; + foreach ($headers as $header) { + list($name, $value) = $header; + if (phutil_utf8_strtolower($name) == 'location') { + $location = $value; + break; + } + } + + // HTTP 3XX status with no "Location" header, just treat this like + // a normal HTTP error. + if ($location === null) { + throw $status; + } + + if (isset($redirects[$location])) { + throw new Exception( + pht( + 'Encountered loop while following redirects.')); + } + + $redirects[$location] = $location; + $current = $location; + // We'll fall off the bottom and go try this URI now. + } else if ($status->isError()) { + // This is something other than an HTTP 2XX or HTTP 3XX status, so + // just bail out. + throw $status; + } else { + // This is HTTP 2XX, so use the the response body to save the + // file data. + $params = $params + array( + 'name' => basename($uri), + ); + + return self::newFromFileData($body, $params); + } + } catch (Exception $ex) { + if ($redirects) { + throw new PhutilProxyException( + pht( + 'Failed to fetch remote URI "%s" after following %s redirect(s) '. + '(%s): %s', + $uri, + new PhutilNumber(count($redirects)), + implode(' > ', array_keys($redirects)), + $ex->getMessage()), + $ex); + } else { + throw $ex; + } + } + } } public static function normalizeFileName($file_name) {