safeAttributes bug?

It seems that Yii does not count attributes defined in the model as safe, only attributes defined in the database.  Eg:

class User extends CActiveRecord

{

public $password_repeat;

}

CActiveRecord::safeAttributes() will not return 'password_repeat', because it is not in the database.

Shouldn't model-defined attributes be deemed safe by default?

Not quite sure if it is a good practice to add member variables, but you can override safeAttributes() and append the attributes that you want. Maybe wait to see if we have other similar requests?

Quiang, maybe you can add parameter into safeAttributes() called inclModelDefAttr='false'?

This would mean that any methods that call safeAttributes() would require the same extra parameter…

Consider this: if we add public members and then you want to remove some of them from the safe list, it would be more troublesome than appending them.

Think of it this way qiang.  What's the risk of having member variables defined as safe anyways?  Isn't the point of safeAttributes() to keep hackers from saving data that shouldn't be saved (such as perhaps a isAdmin column in the table).  But in the case of member variables, they can't be saved in the database anyways.

These are some examples of member variables I have seen:

public $tagInput; //blog example

public $rememberMe; //for user login

public $password_repeat;

public $verifyCode; //captcha

Most member variables, such as $verifyCode for captcha, there's no risk in defining them as safe, as it can't be saved in the database anyways.

You convinced me. Could you please create a ticket for this? Thanks!

I've been doing a lot of brainstorming on Yii validation and security…

First, there is a the safeAttributes() security layer.  This makes sure that fields are not saved that shouldn't be saved.  One way or another you define an array of fields that are safe (either in the model or as an argument of safeAttributes() or both).  I usually define the fields in the argument instead of via the model as I found the safe attributes vary in scenario to scenario.  For instance, I may have user creation page, and for admins, the isAdmin attribute is safe, but for regular users accessing the page, isAdmin is not safe.

The second security layer is the first argument of CModel::validate($attributes=null,$on=null)

This argument defines which attributes are validated (in most cases that means it defines which attributes are required which was the original reason for the implantation)

I found that in 95% cases you will want to pass the same value to safeAttributes() as validates(), as all fields that are required are obviously safe, and often all fields that are safe a required.

So I found myself doing a lot of this:

<?php


$fields = array('username', 'password', 'password_repeat', 'email');


$user->setAttributes($_POST['User'], $fields);


if ($user->validate($fields) && $user->save(false))


	$this->redirect(...);


That's kind of redundant…  I found another solution to defining required fields via validate()'s second argument.  For instance:

<?php


///Model


public function rules() {


	return array(


		///...


		array('username, password, password_repeat, email', 'required', 'on' => 'register'),


		array('username, password', 'required', 'on' =>  'login'),


	);


}





//controller action


$fields = array('username', 'password', 'password_repeat', 'email');


$user->setAttributes($_POST['User'], $fields);


if ($user->validate(null, 'register') && $user->save(false)) //note the second argument of validate()


   $this->redirect(...);


This way it knows the required fields through the 'on' parameter, but you still have to pass safe fields to setAttributes(), so that's even more redundant because you have to type out the attributes twice.  But I liked the idea of the required fields being defined in the model instead of the controller with scenarios.  It seems more correct as the controller should not really do that kind of thing.  I thought it might be nice if you could also define the safe attributes in the model with scenarios in the rules() member function.  Here's a set-up I thought up that I think would be nice in Yii:

<?php


//model rules


public function rules() {


	return array(


		///...


		array('username, password, password_repeat, email', 'required', 'on' => 'register'),


		array('about_me', 'safe', 'on' => 'register'),


	);


}





//controller action


$user->setAttributes($_POST['User']);


if ($user->validate('register') && $user->save(false)) //note validate()'s first argument was removed, more info below


   $this->redirect(...);


With this setup Yii would correctly know which attributes are required because of the scenario use (Yii can already do this).  What I am introducing is the 'safe' validation rule.  I would expect Yii to automatically know that all the fields defined in that rule are safe to assign massively through setAttributes(), and also the fields defined in the 'required' rule are safe (because if a attribute is required, it must be safe, right?).  Going further, Yii could even realize that if any rules are defined for a field inside of the current scenario, it must be safe.  So in the above case the 'about_me' attribute would be safe but not required, and 'username, password, password_repeat, email' are safe and required.  It would be synonymous to doing the following right now:

<?php


$fields = array('username', 'password', 'password_repeat', 'email');


$user->setAttributes($_POST['User'], array_merge($fields, array('about_me')));


if ($user->validate($fields) && $user->save(false))


	$this->redirect(...);


Only the new way is cleaner, and you don't have to define the fields in the controller, but instead in the model.  Also it is not redundant as Yii would automatically know that 'required' fields are 'safe'.  With the new way, if when I was developing a application and I decided to add a new field to the registration process, I would have to modify the model and the view, instead of the controller and the view, which makes more sense for that type of modification.

Tell me what you think, and if that makes sense…

That’s a long post.  ;) Let me try to summarize your idea and see if I get it correct.

I would call your idea scenario-based safe attributes. It's a very clever idea indeed!

First, we probably can drop safeAttributes() method and protectedAttributes().

Second, an attribute is safe if and only if it appears in a validation rule which applies to the specified scenario. The scenario-based validation tutorial has explained when a rule would apply to a scenario.

Third, with scenario-based safe attributes, the controller code can be simplified as:



<?php


$post->attributes=$_POST['Post'];


if($post->validate($scenario) && $post->save(false))


    $this->redirect(...);





// if($post->save()) 


//     is equivalent to the following


// if($post->validate($post->isNewRecord?'insert':'update') && $post->save(false))


Now the question is: should we change the signature of validate()? Do we still need to validate a specific list of attributes?

Thank you for presenting this wonderful idea! I like it.

Quote

First, we probably can drop safeAttributes() method and protectedAttributes().

Yes, I see no need for those anymore.  I remember how CakePHP had many ways to do a single thing, and that was something I found confusing.  If you leave those methods and implant my idea, you will have two ways to do the same thing, so you may want to remove those methods, mostly not to confuse people.

Quote

Second, an attribute is safe if and only if it appears in a validation rule which applies to the specified scenario. The scenario-based validation tutorial has explained when a rule would apply to a scenario.

Yes, but I think another validation type would need to be added called "safe", as in the following example:

<?php


//model rules


public function rules() {


   return array(


      ///...


      array('about_me', 'safe', 'on' => 'register'),


   );


}

The 'safe' validation rule would not really do anything but indicate an attribute as safe, in case the attribute is actually safe but the developer does not have any other rules set for it (although he probably should unless he trusts his users). Because remember, all attributes with validation rules (in the active scenario) are safe, but not all safe attributes have validation rules (as far as I can tell and I've thought about it for a while).  So the 'safe' rule will be there for those few cases.

Quote

Third, with scenario-based safe attributes, the controller code can be simplified as:


<?php


$post->attributes=$_POST['Post'];


if($post->validate($scenario) && $post->save(false))


    $this->redirect(...);





// if($post->save()) 


//     is equivalent to the following


// if($post->validate($post->isNewRecord?'insert':'update') && $post->save(false))


Now the question is: should we change the signature of validate()? Do we still need to validate a specific list of attributes?

Yes, maybe the validate() signature could be changed to simply validate($on) (or you could call the argument $scenario if that's more clear), as the $attributes argument won't be needed anymore (because it's defined in the model instead), but you could leave it in case come people prefer the old method (again though I don't recommend having multiple ways of doing something).

The save() signature could also change (similarly) from

save($runValidation, $attributes)

to

save($runValidation, $scenario)

Yes, I noticed the ‘safe’ validator and think it’s another good idea. ;)

Ok, it seems very clear now. Hope users won't shout at these BC breaking changes at 1.0.2.

Great! Looking forward to this!

Ok, it’s in. Very big change. Guess people will be mad with this change.  ;)

Awesome! Implanted just like I expected.

I’m starting to see a flaw however.  My original proposal was for Yii to know that fields with the ‘required’ rule (in the given scenario) should also be considered ‘safe’.  But then the proposal changed to fields with any rule (in the given scenario) should be considered safe.  But now I see that this is not necessarily true.  Here’s an example:

<?php


public function rules() {


	return array(


		array('username, password, email', 'length', 'max' => 50),


		array('username', 'unique', 'on' => 'register'),


		


		array('password', 'authenticatePass', 'on' => 'login'),


		array('password', 'compare', 'on' => 'register'),


		


		array('email', 'length', 'max' => 100),


		array('email', 'email'),


		


		array('username, password, password_repeat, email', 'required', 'on' => 'register'),


		array('username, password', 'required', 'on' => 'login'),


	);


}


Look at my first rule.  It makes sure those fields are not longer than 50 chars.  With the new/current set-up, those three fields will always be considered safe no matter what the scenario is set to, because that rule does not have a scenario set (I did this because that rule overlaps multiple scenarios).  They should not however always be safe.  For instance the email field is not safe in the login scenario.  I could of course split that rule into two rules, and attach different scenario attributes to them, but that would be redundant.  Also, I don't think Yii lets me set rules to have multiple scenarios right now (?), so that could further complicate things if, for instance, I needed the max length rule for a single attribute in multiple scenarios.

I see two solutions for this, of which I can not decide which is better.  The first would be for Yii to not take into account rules without an attached scenario attribute when deciding which attributes are safe.  The second is for Yii to only take into account the "required" (and of course "safe") rules when deciding which fields are safe.  I'm leaning towards the latter solution but they both seem good.

Any comments?

Besides this concern, everything is working well!  I was able to simplify my controllers a bunch with this. And I'm very happy I don't have to define arrays of attributes in my controllers anymore for security reasons (I used to also have to do this in CakePHP also BTW)

Naah, you CAN declare multiple scenarios for a single rule. Just separate those scenarios with commas. See the following line in the 'on' implementation:



			if(is_array($params['on']))


				$on=$params['on'];


			else


				$on=preg_split('/[s,]+/',$params['on'],-1,PREG_SPLIT_NO_EMPTY);


I don't want to limit to the 'required' rule because that would mean a hardcoded rule, and we probably would face requirements that other rules should also be considered.

My only concern now is that this change seems to have big impact on existing code. Just take a look at how many files I changed in order to implement this feature. But I think it is worthwhile in the long run. I don't like the previous safeAttributes and protectedAttributes design (which actually comes from RoR).

Quote

Ok, it's in. Very big change. Guess people will be mad with this change.  ;)

hehe, haven't had the time to read it completely but it looks good… just explain it well in the changelog, update the documentation (if needed) and provide a small demo (or highlight the changes, using comments, in the blog demo).

thanks jonah and qiang  :)

bye,

Giovanni.

Even with being able to specify multiple scenarios to a rule, it will still be more tedious to write the rules (and even worse to add new scenarios in) if they are all taken into account when deciding which ones are safe.

Right now I would need this to be secure:



<?php


public function rules() {


   return array(


      array('username, password', 'length', 'max' => 50, 'on' => 'register, login'),


      array('email', 'length', 'max' => 50, 'on' => 'register'),


      array('email', 'email', 'on' => 'register'),


      


      array('username, password, password_repeat, email', 'required', 'on' => 'register'),


      array('username, password', 'required', 'on' => 'login'),


   );


}


With one of the modifications I was talking about (like only taking into account rules with scenarios attached when deciding whether it is 'safe'), I could have the same effect with less code:



<?php


public function rules() {


   return array(


      array('username, password, email', 'length', 'max' => 50),


      array('email', 'email'),


      


      array('username, password, password_repeat, email', 'required', 'on' => 'register'),


      array('username, password', 'required', 'on' => 'login'),


   );


}


I can live with it however (and I still love this framework of course), and that will be my last argument.  It's your decision whether you agree.

Thanks,

Jonah

Isn't what you want the current behavior? If you don't specify 'on', the rule will be applicable to all scenarios.

Yes, the way it finds out which rules applies to which scenarios are fine, but I mean the way it decides which fields are 'safe' is not what I think is optimal.  Maybe this would be more clear:



<?php


public function rules() {


   return array(


      array('username, password, email', 'length', 'max' => 50),


      array('email', 'email'),


      array('about', 'length', 'max' => 5000),


      


      array('username, password, password_repeat, email', 'required', 'on' => 'register'),


      array('username, password', 'required', 'on' => 'login'),


      array('email', 'required', 'on' => 'update'),


      array('about', 'safe', 'on' => 'update'),


   );


}


In the login scenario, I would expect it to know the following are (and only) safe (because they are required):

username, password

In the register scenario, I would expect it to know the following are safe:

username, password, password_repeat, email

In the update scenario, I would expect it to know the following are safe:

email, about

But actually, it would decide the following are safe:

login scenario:

username, password, email, about

register scenario:

username, password, password_repeat, email, about

update scenario:

username, password, email, about

To get the correct behavior, I would need to attach 'on' parameters to my first three rules, and split the first rules into two rules, which is much more typing.

I think it would be simpler if Yii did not take into account rules without the 'on' parameters when deciding which attributes are safe, or alternatively, if it only took into account 'required' and 'safe' rules when deciding which attributes are safe.

Thank you for elaboration. I have been thinking about this newly implemented feature for several days. I see your arguments. While I also have my arguments, let's take a look at an alternative solution first and see if it is better.

This solution requires us to write a safeAttributes() method which returns the safe attribute per scenario. So for your example, the method would be:



<?php


return array(


	'login'=>array('username', 'password'),


	'register'=>array('username', 'password', 'password_repeat', 'email'),


	'update'=>array('email', 'about'),


);


In case every scenario has the same set of safe attributes, the method would be:



<?php


return array('username', 'password', 'password_repeat', 'email');


The benefits of this approach are:

  1. It doesn't break backward compatibility of 1.0.2 with 1.0.1.

  2. It is very easy to tell which attributes are safe in which scenario.

The main disadvantage is that this may require more typing. However, it is worthwhile because it makes our program safer and easier to read.

Now back to the rules() approach. When I was fixing the blog demo due to the introduction of this approach, I found I had big problem trying to decipher which attributes are safe by reading these rules. I think this situation will get even worse if there are a lot of attributes. So although we save typing, we actually introduce more possibility of security holes in our program if we are not careful enough.

What do you think?

Yes, I like your new idea better.  It's sort of like mine, except that it doesn't magically decide that 'required' fields are safe.  It still maybe could though, but maybe it doesn't have to advertise that too much in the documentation, so not to confuse people.  And instead of a 'safe' validation rule, you put the 'safe' attributes in the safeAttributes() method.  Nothing wrong with that.

Your method does seem cleaner and less confusing.  A little more typing, but not much more, but cleaner.  But you have my approval (not that you need it :P)

Since the model's are becoming much more scenario-based, i've been thinking about a new way to define rules which you might like.

<?php


//prototype


public function rules() {


	return array(


		'scenario(s)' => array(


			array('fields', 'rule-name')


			//...


		)


		//...


	)


}





//deeper example


public function rules() {


	return array(


		//login scenario


		'login' => array(


			array('password', 'authenticatePass'),


			array('username, password', 'required'),


			//...


		),


		//register scenario


		'register' => array(


			array('password', 'compare'),


			array('username', 'unique'),


			array('username, password, password_repeat, email', 'required')


		),





		//multiple scenarios possible


		'scenarioA, scenarioB' => array(


			//more rules


		),





		//these apply to all scenarios


		array('username, password', 'length', 'max' => 50),


		array('email', 'length', 'max' => 100),


		array('email', 'email'),


		


		array('attributes', 'validator', 'on'=>'validatorC'), //The old way still works also


		//Wont break backwards compatibility even


	)


}


Any thoughts on that?