diff --git a/forms/UploadField.php b/forms/UploadField.php index b86d2255a..e5b161da4 100644 --- a/forms/UploadField.php +++ b/forms/UploadField.php @@ -1262,16 +1262,16 @@ class UploadField extends FileField { } /** - * Determines if a specified file exists + * Check if file exists, both checking filtered filename and exact filename * - * @param SS_HTTPRequest $request + * @param string $originalFile Filename + * @return bool */ - public function fileexists(SS_HTTPRequest $request) { + protected function checkFileExists($originalFile) { // Check both original and safely filtered filename - $originalFile = $request->requestVar('filename'); $nameFilter = FileNameFilter::create(); - $filteredFile = basename($nameFilter->filter($originalFile)); + $filteredFile = $nameFilter->filter($originalFile); // Resolve expected folder name $folderName = $this->getFolderName(); @@ -1281,17 +1281,32 @@ class UploadField extends FileField { : ASSETS_PATH."/"; // check if either file exists - $exists = false; - foreach(array($originalFile, $filteredFile) as $file) { - if(file_exists($parentPath.$file)) { - $exists = true; - break; - } + return file_exists($parentPath.$originalFile) + || file_exists($parentPath.$filteredFile); + } + + /** + * Determines if a specified file exists + * + * @param SS_HTTPRequest $request + */ + public function fileexists(SS_HTTPRequest $request) { + // Assert that requested filename doesn't attempt to escape the directory + $originalFile = $request->requestVar('filename'); + if($originalFile !== basename($originalFile)) { + $return = array( + 'error' => _t('File.NOVALIDUPLOAD', 'File is not a valid upload') + ); + } else { + $return = array( + 'exists' => $this->checkFileExists($originalFile) + ); } // Encode and present response - $response = new SS_HTTPResponse(Convert::raw2json(array('exists' => $exists))); + $response = new SS_HTTPResponse(Convert::raw2json($return)); $response->addHeader('Content-Type', 'application/json'); + if (!empty($return['error'])) $response->setStatusCode(400); return $response; } diff --git a/tests/forms/uploadfield/UploadFieldTest.php b/tests/forms/uploadfield/UploadFieldTest.php index af831abf7..89dac5e0c 100644 --- a/tests/forms/uploadfield/UploadFieldTest.php +++ b/tests/forms/uploadfield/UploadFieldTest.php @@ -680,6 +680,30 @@ class UploadFieldTest extends FunctionalTest { $responseExistsData = json_decode($responseExists->getBody()); $this->assertFalse($responseExists->isError()); $this->assertTrue($responseExistsData->exists); + + // Test that files with invalid characters are rewritten safely and both report exists + // Check that uploaded files can be detected in the root + $tmpFileName = '_test___Upload___Bad.txt'; + $tmpFileNameExpected = 'test-Upload-Bad.txt'; + $response = $this->mockFileUpload('NoRelationField', $tmpFileName); + $this->assertFalse($response->isError()); + $this->assertFileExists(ASSETS_PATH . "/UploadFieldTest/$tmpFileNameExpected"); + // With original file + $responseExists = $this->mockFileExists('NoRelationField', $tmpFileName); + $responseExistsData = json_decode($responseExists->getBody()); + $this->assertFalse($responseExists->isError()); + $this->assertTrue($responseExistsData->exists); + // With rewritten file + $responseExists = $this->mockFileExists('NoRelationField', $tmpFileNameExpected); + $responseExistsData = json_decode($responseExists->getBody()); + $this->assertFalse($responseExists->isError()); + $this->assertTrue($responseExistsData->exists); + + // Test that attempts to navigate outside of the directory return false + $responseExists = $this->mockFileExists('NoRelationField', "../../../../var/private/$tmpFileName"); + $responseExistsData = json_decode($responseExists->getBody()); + $this->assertTrue($responseExists->isError()); + $this->assertContains('File is not a valid upload', $responseExists->getBody()); } protected function getMockForm() {