1257 fix c file validator safe mass assignment #3197

merged 4 commits into from Feb 3, 2014


None yet

5 participants


CFileValidator is no longer unsafe by default to prevent setting arbitrary values. Instead, when no file is uploaded attribute is set to null.

samdark commented Feb 3, 2014

Are there BC breaks?

@samdark samdark self-assigned this Feb 3, 2014

Explanation of security problem and proposed solution here: #1257 (comment)

Earlier discussion about original change: #1083

samdark commented Feb 3, 2014

It seems not.

@samdark samdark merged commit e5fed90 into yiisoft:master Feb 3, 2014
samdark commented Feb 3, 2014

Thanks for handling it.


It seems a similar solution was debated some time ago in #1083

The discussion and historic decision are mostly based on the assumption people are using the same attribute for uploading and storing the file. If you use that approach, you also need to implement some custom handling in the update form, to skip updating the file and/or to delete the file.

When dealing with stored uploads, I think separating the two (upload instance and stored filename) is much more clear. Do we have any authorative example code on this subject?

samdark commented Feb 3, 2014

Qiang's wiki: http://www.yiiframework.com/wiki/2/how-to-upload-a-file-using-a-model/

Generally reusing AR field is a bad idea.

rawtaz commented Feb 3, 2014

TL;DR: For my use case (a less common one where I set things up so I don't have to add extra contorller code for handling file uploads), this works fine.

In my code this change is OK/works/doesn't interfere. I'm using an approach where I have setFile() in the model automatically making a protected $file attribute an instance of CUploadedFile when setAttributes() is run, so that I don't have to put extra code in controllers just to handle file uploads. Instead it just goes with the rest of the attributes being assigned, treated like any other form data from the controller's PoV. The afterSave() in the model is then used to save the file.

I am however not using the same attribute for storing the filename in the database (i.e. I don't store the filename in the database at all). But even if I was doing that (which is doable since there's a __toString() in CUploadedFile), I would probably use the same attribute for storing the uploaded file instance as well as to provide the filename for it, as I think introducing two separate attributes/properties just to handle file uploading and filename presentation is unclean and adds clutter (since it's all about one and the same file anyway).

If I was doing the latter, I this change would probably interfere, and I also think it's a bit uneasy when a validator that is meant to just check a file upload starts setting stuff in the model (also see #1083 (comment)). But I think the setFile() setter I mentioned earlier could easily take care of that (although by cancelling the assignment, working against the intentions of the framework), so it's probably "OK" anyway. (That is, as long as Yii makes sure to call getters/setters if they exist, instead of using property_exists() which became broken in PHP 5.3, but that's another story and something I handled by overriding CActiveRecord::{get,set}Attribute() so that my setFile() is called nicely when setAttributes() runs).

Sorry for the long writing. In part if was a good place to jot down some thoughts for future reference :P

@marcovtwout marcovtwout deleted the marcovtwout:1257-fix-CFileValidator-safe-mass-assignment branch Feb 3, 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


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

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

cebe replied Feb 7, 2014

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

@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.

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

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