Upgrade Instructions for v1.1.12 do not specify that CFileValidator::safe now is false by default

[ghost]

Upgrade Instructions for v1.1.12 do not specify that CFileValidator::safe is now false by default. This change breaks existing code.

[samdark]

What existing code exactly? It was not using input parameters anyway.

[cebe]

@samdark when someone used CFileValidator before mass assignment worked for the file field. Now as it is unsafe it does not work anymore. So it breaks existing code that did not explicitly declared file attribute as unsafe. Even if it was unsecure code it might now be broken.

[ghost]

@samdark -- In a project I am working on. The old version of Yii had safe = true, and the app I am working on was designed around this assumption.

[ghost]

@cebe -- Yes, the solution was simply to specify safe = true in the parameter list of rules().

Ahh, okay, I will check for a security hole in my code.

[cebe]

@GTcode no I ment you should have declared that attribute unsafe before it became unsafe by default. Declaring it save might be a secutiry risk as someone could send you a prepared post with a different filename.

[ghost]

@cebe Okay, thanks. I am not sure what is the best practice for handling a DB field/model attribute related to an uploaded file; we were simply using CFileValidator on the attribute where the file name was stored. But, if it is unsafe, it won't be mass assigned. So, I assume I will need to manually assign the filename to a different attribute and use a separate attribute for the CFileValidator? Perhaps a wiki or some updated docs would be helpful here? Sorry if I am missing something obvious.

[ghost]

@cebe -- Nevermind, sorry. The code I am working on has a separate upload mechanism and was using a hidden element simply to pass the filename. Looks like Qiang recommends assigning the filename non-massively in this Wiki article: http://www.yiiframework.com/wiki/2/how-to-upload-a-file-using-a-model/

[mdomba]

@GTcode ... so if everything is OK, please close the issue...

[cebe]

@mdomba @samdark sure that we should not add a hint to UPGRADE instructions?

[mdomba]

As explained above old code should not in any way rely on massive assignment as that was not the way to get the filename, so there is no problem with BC

above that even if we would add now a hint... that would just be in the trunk and release only when 1.1.13 will get out

Because of those two points I don't see the need for the hint.

[cebe]

k, so we can close this.

[ghost]

Thanks guys for the assistance.

Perhaps the title of this ticket should have been to the effect of, "Change breaks code that has design/security problems".

Nevertheless, it is still a change that should be documented in the upgrade instructions, IMO.

[rawtaz]

I agree. If the actual API is changed, then it's not BC, hence should at least be noted. I completely agree that the code shouldn't be affected if it was written correctly, but that's still a separate question :) But either way it's past 1.1.12 release so kinda too late.

[marcovtwout]

I also ran into this problem, my CForm with CMultiFileUpload was not showing the field because it became unsafe.
Fortunately I read the full changelog and remembered something about default safe setting.

The note says: "This will prevent setting attribute when no file was uploaded"

I'd like to see a -brief- description or example where this is a problem (I have went through related issues and forum posts, and still it's not very clear to me, let alone users who face this as a new problem).

Currently, I am not convinced setting unsafe as default and breaking backwards compatibility is more important than keeping the old situation. Please elaborate.

[marcovtwout]

@cebe you said "Declaring it save might be a secutiry risk as someone could send you a prepared post with a different filename.". Could you give an example of the security problem you mensioned? I don't see how safe/unsafe rules have any influence on request forgery (that's why you have csrf protection).

[cebe]

This has nothing to do with CSRF. I can take your form (e.g. for uploading my avatar on your site) and modify file field to text field and send '/etc/passwd' as the file field value. When it is declared save it will replace fileName in your db with '/etc/passwd'. When I am lucky and you stored the absolute path I will get your passwd file as avatar.

[marcovtwout]

If I understand correctly, the problem you are describing will only happen under the following circumstances:

• You are saving the model (to database)
• You are using the same attribute for uploading the file as for storing the filename.
• The validation rule for CFileValidator has allowEmpty => true

What if CFileValidator->emptyAttribute() would also set $object>$attribute = null; (regardless of the allowEmpty value)? Wouldn't that solve the security issue?

When looking for the right way to do this, I found many incorrect examples where people 'solve' this issue by setting the validator back to safe again. The reason for setting it to false is not documented in CFileValidator either. I get a nasty feeling people are unaware of this particular security issue because the documentation is lacking.

[samdark]

@marcovtwout probably it is a better fix. I'm OK for changing it back as long as the issue is fixed and there are no drawbacks.

Also I'm aware that people just changing it back to safe so probably it wasn't a good idea to fix it like that.

Do you want to work on the fix?

[marcovtwout]

Sure, I'll see if I can send a pull request today. :)

The common wiki article starting point is this one by Qiang: http://www.yiiframework.com/wiki/2/how-to-upload-a-file-using-a-model/
I think we could still use an authorative (wiki) example on how to properly deal with file uploads that are also saved (to database) and how to handle the update form.

[marcovtwout]

(it will probably be on monday)

[marcovtwout]

#3197

[ekzobrain]

Fix of this issue in 1.1.16 (attribute is set to null by default when no file uploaded) broke many things. It is a common update scenario to check for a file being uploaded (then assign new value to file attribute), and if not, leave old value as is. But this update sets old value to null and data gets lost. To avoid this one needs to set 'safe' => false to validation rules, but it is not mensioned anywhere in docs/cookbooks/etc. At least this information should be added where appropriate, if you consider this new default behavior correct.

[marcovtwout]

Like I proposed here (#1257 (comment)) a common solution should be described in the wiki article.