Action parameter binding feature

Just read through the release notes and discovered it. Seems like a handy improvement, but I fear it drives people into the wrong direction, especially in combination with its documentation. The eample shows how to use the new feature (which uses GET parameters) to create content on the server. In the security section of the documentation, the point “Cross-site Request Forgery Prevention” explains why this is a bad idea. What I suggest is to use another example, that only queries for some data. Maybe you could even provide a hint why the feature shouldn’t be used to create content.

It would also be cool to use this feature together with $_POST, but I don’t have a good idea yet how to accomplish it. Maybe through naming convention of the parameters, or by using a special tag in doc comment like the SOAP feature does. But I don’t know how big the performance impact would be…

Oh! Maybe a mechanism like the model rules could be used for that purpose?

I don’t see where the provided example should allow a xss attack…

xss means injection of javascript or other code… the example shows how to cast the category parameter to an int… and the language-parameter surely won’t be printed and only used in a switch… (but thats invisible for us…)

He told about "Cross-site Request Forgery Prevention", not about XSS

You’ve seen the discussion about this feature?

http://www.yiiframework.com/forum/index.php?/topic/11410-design-of-new-action-parameter-feature/

Somehow to me this feels like a step backward. Back in the early days of PHP, when "register_globals" was on by default, this was the common way to work with GET and POST data: They where just PHP variables that where automatically filled with input data. It was combined with "gpc_order" configuration, to define in which order GET, POST and COOKIE data was mapped to PHP variables. All this was removed/disabled from PHP for good reason: It was very easy to create insecure code.

I still think it’s good to let the developer be aware of this situation. It’s your responsibility to sanitize/validate anything coming from the outside. The framework should only provide some tools to help you with this.

I think this topic not about discuss this feature :)

And action parameter binding feature free from register_globals security troubles.

Ben told about incorrect example only (which will be incorrect even without binding).

Thanks for the link, didn’t notice that before. I’m going to read through it this evening.

My main concern is, that if yii povides such a feature, people will use it. And if they’re not aware of the problems it might introduce, they will use it incorrectly. I think this is especially true for people that are just starting to learn yii (or php itself).

That’s why I think it’s important to

[list=1]

[*]correct the current example in the documentation

[*]provide same mechanism for creating content (using $_POST)

[*]mention when to use either of these

[/list]

Ok, sorry i’ve misread… but still i see no problem with the example… GET is there for retrieving data… perhaps the action-name is misleading - but for example i have a project where i need to create items inside a category… and instead of setting the categoryId to the form it’s better to use it as get-parameter since all other pages also will have that categoryId parameter.

(and when looking at the example i think the PostController just tells, that you actually create posts inside a category… you don’t create a category with a get-request)

Until the example doesn’t show how this GET-parameter is altering existing data i realy see no problem… but to make all feel better the action-name also could be renamed to actionView or something…

And to that register_globals… i think you totaly misunderstand something: register_globals is not bad cause it renames some GET parameters into variable-names… it’s bad because it renames all GET parameters to variable names… so the programmer has to set default values to each variable first to secure, that no GET-parameter introduced new values (and if people actually would have good coding style to not check nonexistent variables for their values that wouldn’t ever be a problem)

That new feature is completely different to the problem of register_globals… you will know which varaibles will get substituted by GET… so you know which variables to check… in contrast to register_globals where you just can assume which variables the user might have set…

And please describe why exactly that example should be a cross-site-request-forgery… I hope i understood your problem this time right… if not then give a scenario where malicious data can be introduced…


about the POST idea:

i think a naming-sheme would be nice… we can create prefixes like c (cookie) g (get) p (post)

so the parameters would look like

gCategory, cUserstuff, pPostform

so even you are still convinced, that this equals to register_globals such naming sheme can be immediately identified as special action-parameters (which also might help to improve readability of the code)

and just a general note about that feature: at the beginning where i started with yii and realized how the action-methods are mapped to an url my natural guess was to retrieve _GET parameters through those function-parameters… it’s not just less typing of $_GET[’’] around each parameter… it also helps with readability… you just can look at the function-header and see which GET parameters are required and which might be set

There’s no difference wether all variables are assigned or only the ones you know. The effect is the same if you forget to init that variable.

About your POST proposal: It’s not possible to automatically inject other variables into a function namespace except its arguments.

Why want you init the userinput? you want to retrieve the userinput and then overwrite it?

And of course you have to do your checks, but this is done in the example with (int)… and this also hast to be done with all GET stuff

with my example i mean you have to set the parameters with this prefix:

so function actionCreate($gCategory, $pPostform, $gLanguage = ‘en’)

and your sentence describes pretty well why i think it doesn’t harm… exactly because you know what variables getting injected


a short example where register_globals is bad: (warning bad coding style)


if($_COOKIE['user']=='verySecretStuff')

    $auth = true;

if ($auth)

    // do some cool stuff



if the attacker knows that you’re writing bad code and that you use auth as variablename, he can give you index.php?auth=1 as parameter

so you never expect to retrieve the $_GET[‘auth’] parameter…

the new feature is exactly the other way round… you hardcode all expected parameters… so you exactly know which things you have to handle… or tell me the idea behind this:


function actionDo($auth=false)

{

if($_COOKIE['user']=='verySecret')

    $auth = true;

if($auth)

  // do something

}

you are already defining, that you expect the user to set the $_GET[‘auth’]… but still coding bad… that’s definitly a programmers fault

I see your point about having defined input parameters. So you’re right it’s not really comparable. It was just that it ressembles the practice in the “old times” ;) .

Hi,

I am wondering why this feature is not implemented in "CAction->run()"? If parameter binding works in action methods of a controller then "CAction->run()" should act the same way, for sake of consistency.

Mike, I had the same initial gut reaction when I read it! ;) I was very reassured when I read the full description though, I think they’ve done a good job of simplifying things without falling into the old trap.

Yeah, maybe i shouldn’t always shout so fast. Old habits, you know. :)