Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

[2.3] [HttpFoundation] UploadedFile - moved a security check from move() to isValid() #6802

Closed
wants to merge 2 commits into from

3 participants

Bilal Amarni Victor Berchet Fabien Potencier
Bilal Amarni
Q A
Bug fix? [yes]
New feature? [no]
BC breaks? [yes slightly]
Deprecations? [no]
Tests pass? [yes]
License MIT

Fixed and reopened against 2.0 as per @vicb comments in #6779.

Victor Berchet

Does getTargetFile exist in 2.0 ?

Bilal Amarni

Yes it exists in File and it was already there, I haven't changed this part of the code, only removed an elseif.

Victor Berchet

You're right, my bad ! I'd better go on week-end right now I think !

Fabien Potencier fabpot commented on the diff
...ymfony/Component/HttpFoundation/File/UploadedFile.php
((6 lines not shown))
*
* @api
*/
public function isValid()
{
- return $this->error === UPLOAD_ERR_OK;
+ $isOk = $this->error === UPLOAD_ERR_OK;
+
+ return $this->test ? $isOk : $isOk && is_uploaded_file($this->getPathname());
Fabien Potencier Owner
fabpot added a note

In case of a test, the behavior is not as before. This is definitely a BC break.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Fabien Potencier
Owner

As there is a BC break, it must go into master.

Bilal Amarni bamarni referenced this pull request
Merged

[2.3] moved a security check in HttpUploadedFile #7201

1 of 1 task complete
Bilal Amarni

I've merged this out into master and submitted a new PR, see #7201

Bilal Amarni bamarni closed this
Bilal Amarni bamarni deleted the branch
Fabien Potencier fabpot referenced this pull request from a commit
Fabien Potencier fabpot merged branch bamarni/http-uploaded-file (PR #7201)
This PR was merged into the master branch.

Discussion
----------

[2.3] moved a security check in HttpUploadedFile

closes #6802

- [x] fix the testsuite, I've only run the component suite, but it needs to be updated in other places too (according to travis)

Commits
-------

5bb44f5 [HttpFoundation] UploadedFile - moved a security check
2b4cfbd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 18, 2013
  1. Bilal Amarni
Commits on Jan 24, 2013
  1. Bilal Amarni
This page is out of date. Refresh to see the latest.
28 src/Symfony/Component/HttpFoundation/File/UploadedFile.php
View
@@ -166,13 +166,15 @@ public function getError()
/**
* Returns whether the file was uploaded successfully.
*
- * @return Boolean True if no error occurred during uploading
+ * @return Boolean True if the file has been uploaded with HTTP and no error occurred.
*
* @api
*/
public function isValid()
{
- return $this->error === UPLOAD_ERR_OK;
+ $isOk = $this->error === UPLOAD_ERR_OK;
+
+ return $this->test ? $isOk : $isOk && is_uploaded_file($this->getPathname());
Fabien Potencier Owner
fabpot added a note

In case of a test, the behavior is not as before. This is definitely a BC break.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
}
/**
@@ -183,7 +185,7 @@ public function isValid()
*
* @return File A File object representing the new file
*
- * @throws FileException if the file has not been uploaded via Http
+ * @throws FileException if, for any reason, the file could not have been moved
*
* @api
*/
@@ -192,21 +194,21 @@ public function move($directory, $name = null)
if ($this->isValid()) {
if ($this->test) {
return parent::move($directory, $name);
- } elseif (is_uploaded_file($this->getPathname())) {
- $target = $this->getTargetFile($directory, $name);
-
- if (!@move_uploaded_file($this->getPathname(), $target)) {
- $error = error_get_last();
- throw new FileException(sprintf('Could not move the file "%s" to "%s" (%s)', $this->getPathname(), $target, strip_tags($error['message'])));
- }
+ }
- @chmod($target, 0666 & ~umask());
+ $target = $this->getTargetFile($directory, $name);
- return $target;
+ if (!@move_uploaded_file($this->getPathname(), $target)) {
+ $error = error_get_last();
+ throw new FileException(sprintf('Could not move the file "%s" to "%s" (%s)', $this->getPathname(), $target, strip_tags($error['message'])));
}
+
+ @chmod($target, 0666 & ~umask());
+
+ return $target;
}
- throw new FileException(sprintf('The file "%s" has not been uploaded via Http', $this->getPathname()));
+ throw new FileException(sprintf('The file "%s" is not valid', $this->getPathname()));
}
/**
16 tests/Symfony/Tests/Component/HttpFoundation/File/UploadedFileTest.php
View
@@ -186,7 +186,8 @@ public function testIsValid()
'original.gif',
null,
filesize(__DIR__.'/Fixtures/test.gif'),
- UPLOAD_ERR_OK
+ UPLOAD_ERR_OK,
+ true
);
$this->assertTrue($file->isValid());
@@ -208,6 +209,19 @@ public function testIsInvalidOnUploadError($error)
$this->assertFalse($file->isValid());
}
+ public function testIsInvalidIfNotHttpUpload()
+ {
+ $file = new UploadedFile(
+ __DIR__.'/Fixtures/test.gif',
+ 'original.gif',
+ null,
+ filesize(__DIR__.'/Fixtures/test.gif'),
+ UPLOAD_ERR_OK
+ );
+
+ $this->assertFalse($file->isValid());
+ }
+
public function uploadedFileErrorProvider()
{
return array(
Something went wrong with that request. Please try again.