Permalink
Browse files

CFileValidator is no longer unsafe by default

CFileValidator is no longer unsafe by default to prevent setting
arbitrary values. Instead, when no file is uploaded attribute is set to
null (marcovtwout)
  • Loading branch information...
1 parent 6b4faf2 commit db13ce2d2e24a9c7546cf0f326210a89040b7972 @marcovtwout marcovtwout committed Feb 3, 2014
Showing with 5 additions and 6 deletions.
  1. +1 −0 CHANGELOG
  2. +4 −6 framework/validators/CFileValidator.php
View
@@ -7,6 +7,7 @@ Version 1.1.15 under development
--------------------------------
- Bug #268: Fixed Active Record count error when some field name starting from 'count' (nineinchnick)
- Bug #788: createIndex is not using the recommended way to create unique indexes on Postgres (nineinchnick)
+- Bug #1257: CFileValidator is no longer unsafe by default to prevent setting arbitrary values. Instead, when no file is uploaded attribute is set to null (marcovtwout)
- Bug #2235: CPgsqlColumnSchema can't parse default value for numeric field (cebe, pavimus)
- Bug #2378: CActiveRecord::tableName() in namespaced model returned fully qualified class name (velosipedist, cebe)
- Bug #2654: Allow CDbCommand to compose queries without 'from' clause (klimov-paul)
@@ -64,6 +64,8 @@ class CFileValidator extends CValidator
/**
* @var boolean whether the attribute requires a file to be uploaded or not.
* Defaults to false, meaning a file is required to be uploaded.
+ * When no file is uploaded, the owner attribute is set to null to prevent
+ * setting arbitrary values.
*/
public $allowEmpty=false;
/**
@@ -130,12 +132,6 @@ class CFileValidator extends CValidator
* limit.
*/
public $tooMany;
- /**
- * @var boolean whether attributes listed with this validator should be considered safe for massive assignment.
- * For this validator it defaults to false.
- * @since 1.1.12
- */
- public $safe=false;
/**
* Set the attribute and then validates using {@link validateFile}.
@@ -254,11 +250,13 @@ protected function validateFile($object, $attribute, $file)
/**
* Raises an error to inform end user about blank attribute.
+ * Sets the owner attribute to null to prevent setting arbitrary values.
* @param CModel $object the object being validated
* @param string $attribute the attribute being validated
*/
protected function emptyAttribute($object, $attribute)
{
+ $object->$attribute=null;
if(!$this->allowEmpty)
{
$message=$this->message!==null?$this->message : Yii::t('yii','{attribute} cannot be blank.');

8 comments on commit db13ce2

Contributor

maxlapko replied Feb 7, 2014

I think it is wrong.
For example: I create post (title is required, upload image is not mandatory field), filling all fields.
Next step: I want update title, after saving the image field always is null - it is fatality

Owner

samdark replied Feb 7, 2014

You should not use model's real field for image uploading.

Contributor

maxlapko replied Feb 7, 2014

Why? I think it is not correct. More than 3 years it was possible to do, but not now. I think we should add additional property, $forceEmpty, if ($this->forceEmpty) $object->$attribute=null; The $this->forceEmpty = true by default

Owner

cebe replied Feb 7, 2014

This is a BC break in some way so we should rethink the change.

Contributor

marcovtwout replied Feb 10, 2014

@samdark I agree, but I also think the community requires an authorative example on this subject.

@maxlapko Could you post some example code? I wonder if it is subject to the same vurnerability this issue fixes.

@cebe It's still a very specific use case to keep backwards compatible, but if we have to, having an additional property ($forceEmpty of $setNullIfEmpty) like maxlapko suggests seems like a good solution.

Contributor

maxlapko replied Feb 10, 2014

@marcovtwout I suggested new solution for this issue, please look at #3214, my last comment

This change cost me hours of debugging, In our team we have case just like maxlapko's case mentioned above.

Owner

samdark replied Aug 23, 2017

It's clearly described in update notes: https://github.com/yiisoft/yii/blob/master/UPGRADE#L47

Please sign in to comment.