From 7b5f47b17d6ed0ed496685d92813b7272b665950 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 7 May 2012 06:17:00 -0700 Subject: [PATCH] Enforce upload size limits and transport exceptions with appropriate response encoding Summary: - When a user uploads an oversized file, throw an exception. - When an uncaught exception occurs during a Conduit request, return a Conduit response. - When an uncaught exception occurs during a non-workflow Ajax request, return an Ajax response. Test Plan: - Uploaded overlarge files. - Hit an exception page with ?__ajax__=1 and ?__conduit__=1 Reviewers: btrahan, vrana, jungejason Reviewed By: btrahan CC: aran Maniphest Tasks: T875, T788 Differential Revision: https://secure.phabricator.com/D2385 --- externals/javelin | 2 +- ...AphrontDefaultApplicationConfiguration.php | 24 ++++++++++++++++++- .../default/configuration/__init__.php | 2 ++ src/aphront/request/AphrontRequest.php | 10 ++++++++ .../response/ajax/AphrontAjaxResponse.php | 5 ++++ .../PhabricatorFileDropUploadController.php | 2 +- .../upload/PhabricatorFileUploadException.php | 6 ++++- .../files/storage/file/PhabricatorFile.php | 20 +++++++++++++++- .../files/storage/file/__init__.php | 1 + 9 files changed, 67 insertions(+), 5 deletions(-) diff --git a/externals/javelin b/externals/javelin index 748eac2b2f..1cdedbfc00 160000 --- a/externals/javelin +++ b/externals/javelin @@ -1 +1 @@ -Subproject commit 748eac2b2fb3f210dc68506d2bc36f96a6b0d77f +Subproject commit 1cdedbfc00a32f3167aa3bc14dd917cf886014ff diff --git a/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php b/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php index 3f0a55a1de..ee6b5ca26b 100644 --- a/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php +++ b/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php @@ -455,10 +455,32 @@ class AphrontDefaultApplicationConfiguration } public function handleException(Exception $ex) { + $request = $this->getRequest(); + + // For Conduit requests, return a Conduit response. + if ($request->isConduit()) { + $response = new ConduitAPIResponse(); + $response->setErrorCode(get_class($ex)); + $response->setErrorInfo($ex->getMessage()); + + return id(new AphrontJSONResponse()) + ->setContent($response->toDictionary()); + } + + // For non-workflow requests, return a Ajax response. + if ($request->isAjax() && !$request->isJavelinWorkflow()) { + $response = new AphrontAjaxResponse(); + $response->setError( + array( + 'code' => get_class($ex), + 'info' => $ex->getMessage(), + )); + return $response; + } $is_serious = PhabricatorEnv::getEnvConfig('phabricator.serious-business'); - $user = $this->getRequest()->getUser(); + $user = $request->getUser(); if (!$user) { // If we hit an exception very early, we won't have a user. $user = new PhabricatorUser(); diff --git a/src/aphront/default/configuration/__init__.php b/src/aphront/default/configuration/__init__.php index c28e3a05f2..83e885d5ea 100644 --- a/src/aphront/default/configuration/__init__.php +++ b/src/aphront/default/configuration/__init__.php @@ -10,9 +10,11 @@ phutil_require_module('phabricator', 'aphront/applicationconfiguration'); phutil_require_module('phabricator', 'aphront/request'); phutil_require_module('phabricator', 'aphront/response/ajax'); phutil_require_module('phabricator', 'aphront/response/dialog'); +phutil_require_module('phabricator', 'aphront/response/json'); phutil_require_module('phabricator', 'aphront/response/webpage'); phutil_require_module('phabricator', 'applications/base/controller/404'); phutil_require_module('phabricator', 'applications/base/controller/redirect'); +phutil_require_module('phabricator', 'applications/conduit/protocol/response'); phutil_require_module('phabricator', 'applications/people/storage/user'); phutil_require_module('phabricator', 'infrastructure/env'); phutil_require_module('phabricator', 'view/control/table'); diff --git a/src/aphront/request/AphrontRequest.php b/src/aphront/request/AphrontRequest.php index 22a6714b58..79f8347184 100644 --- a/src/aphront/request/AphrontRequest.php +++ b/src/aphront/request/AphrontRequest.php @@ -24,9 +24,15 @@ */ final class AphrontRequest { + // NOTE: These magic request-type parameters are automatically included in + // certain requests (e.g., by phabricator_render_form(), JX.Request, + // JX.Workflow, and ConduitClient) and help us figure out what sort of + // response the client expects. + const TYPE_AJAX = '__ajax__'; const TYPE_FORM = '__form__'; const TYPE_CONDUIT = '__conduit__'; + const TYPE_WORKFLOW = '__wflow__'; private $host; private $path; @@ -171,6 +177,10 @@ final class AphrontRequest { return $this->getExists(self::TYPE_AJAX); } + final public function isJavelinWorkflow() { + return $this->getExists(self::TYPE_WORKFLOW); + } + final public function isConduit() { return $this->getExists(self::TYPE_CONDUIT); } diff --git a/src/aphront/response/ajax/AphrontAjaxResponse.php b/src/aphront/response/ajax/AphrontAjaxResponse.php index 2195b02aad..1e7bb01f6f 100644 --- a/src/aphront/response/ajax/AphrontAjaxResponse.php +++ b/src/aphront/response/ajax/AphrontAjaxResponse.php @@ -29,6 +29,11 @@ final class AphrontAjaxResponse extends AphrontResponse { return $this; } + public function setError($error) { + $this->error = $error; + return $this; + } + public function buildResponseString() { $response = CelerityAPI::getStaticResourceResponse(); $object = $response->buildAjaxResponse( diff --git a/src/applications/files/controller/dropupload/PhabricatorFileDropUploadController.php b/src/applications/files/controller/dropupload/PhabricatorFileDropUploadController.php index ca16749bd8..4dca060e7e 100644 --- a/src/applications/files/controller/dropupload/PhabricatorFileDropUploadController.php +++ b/src/applications/files/controller/dropupload/PhabricatorFileDropUploadController.php @@ -29,7 +29,7 @@ final class PhabricatorFileDropUploadController $data = file_get_contents('php://input'); $name = $request->getStr('name'); - $file = PhabricatorFile::newFromFileData( + $file = PhabricatorFile::newFromXHRUpload( $data, array( 'name' => $request->getStr('name'), diff --git a/src/applications/files/exception/upload/PhabricatorFileUploadException.php b/src/applications/files/exception/upload/PhabricatorFileUploadException.php index d48e2a4e36..e15ae6fb5f 100644 --- a/src/applications/files/exception/upload/PhabricatorFileUploadException.php +++ b/src/applications/files/exception/upload/PhabricatorFileUploadException.php @@ -1,7 +1,7 @@ "Unable to upload: a PHP extension stopped the upload.", + + -1000 => + "Uploaded file exceeds limit in Phabricator ". + "'storage.upload-size-limit' configuration.", ); $message = idx($map, $code, "Upload failed: unknown error."); diff --git a/src/applications/files/storage/file/PhabricatorFile.php b/src/applications/files/storage/file/PhabricatorFile.php index 59121f04fe..08b091e05b 100644 --- a/src/applications/files/storage/file/PhabricatorFile.php +++ b/src/applications/files/storage/file/PhabricatorFile.php @@ -66,6 +66,8 @@ final class PhabricatorFile extends PhabricatorFileDAO { throw new Exception("File size disagrees with uploaded size."); } + self::validateFileSize(strlen($file_data)); + return $file_data; } @@ -82,8 +84,24 @@ final class PhabricatorFile extends PhabricatorFileDAO { return self::newFromFileData($file_data, $params); } - public static function newFromFileData($data, array $params = array()) { + public static function newFromXHRUpload($data, array $params = array()) { + self::validateFileSize(strlen($data)); + return self::newFromFileData($data, $params); + } + private static function validateFileSize($size) { + $limit = PhabricatorEnv::getEnvConfig('storage.upload-size-limit'); + if (!$limit) { + return; + } + + $limit = phabricator_parse_bytes($limit); + if ($size > $limit) { + throw new PhabricatorFileUploadException(-1000); + } + } + + public static function newFromFileData($data, array $params = array()) { $selector = PhabricatorEnv::newObjectFromConfig('storage.engine-selector'); $engines = $selector->selectStorageEngines($data, $params); diff --git a/src/applications/files/storage/file/__init__.php b/src/applications/files/storage/file/__init__.php index e72e0f6ae6..efd290a2b8 100644 --- a/src/applications/files/storage/file/__init__.php +++ b/src/applications/files/storage/file/__init__.php @@ -13,6 +13,7 @@ phutil_require_module('phabricator', 'applications/phid/constants'); phutil_require_module('phabricator', 'applications/phid/storage/phid'); phutil_require_module('phabricator', 'infrastructure/env'); phutil_require_module('phabricator', 'infrastructure/util/hash'); +phutil_require_module('phabricator', 'view/utils'); phutil_require_module('phutil', 'error'); phutil_require_module('phutil', 'error/aggregate');