Best practises to handle input in controller

Hello,

Let’s suppose I’ve got some AJAX Post call in some random view to a route named ‘user/flag-as-spammer’, with the argument id equals 1927.

In terms of best practises, what would be the ideal way to deal with this?

Is the following somewhat accepted? If not, what do you suggest?




<?php


     // [...]


    public function actionFlagAsSpammer() {


        if (!isset($_POST['id'] || !is_numeric($_POST['id'])) {

            return $this->redirect( Url::to(['/']) );

        }


        $user_id = (int)$_POST['id'];


        $user = User::find()->where(['id' => $id])->one();


        // [...]


    }


     // [...]


?>



I think its good and its good practice not to use $_POST data directly in query and assign it to a variable first.

As far as parsing it to int goes i would be sure of it while i am passing the data from ajax call of taking care of data types

And you are already redirecting to some page if the value is not number than no need to parse to int again

but you are the only one who can understand possible flow of your project and what could go wrong

  1. $_POST or $_GET should never be used. Use Yii::$app->request->post() and Yii::$app->request->get() instead.

  2. Instead of redirect I’d throw HTTP exception.

  3. Casting to int doesn’t make sense in this case.

Hello,

Thanks for replying.

Yes, however usually I do make sure of that in both sides - client and server.

Thanks @samdark.

  1. Any special reason why I should always use Yii::$app->request->post() instead of $_POST (and the same for $_GET)?

  2. And then you would customize the HTTP error handling, pointing it to some custom views?

  3. May I ask why not? If I want to be sure it is a numeric that is being passed to the query, how can I be sure of that?

Thanks for your attention guys.

  1. Mainly testing.

  2. Yes.

  3. You don’t need to be sure of that.

I would write something like the following:




<?php

    public function behaviors()

    {

        return [

            'verbs' => [

                'class' => VerbFilter::className(),

                'actions' => [

                    'delete' => ['post'],

                    'flag-as-spammer' => ['post'], // added for actionFlagAsSpammer

                ],

            ],

        ];

    }

    ...


    public function actionFlagAsSpammer() {

        $id = Yii::$app->request->post('id');

        if ($id === null) {

            throw new yii\web\BadRequestHttpException("The 'id' parameter is missing.");

        }

        $user = User::find()->where(['id' => $id])->one();

        if ($user === null) {

            throw new NotFoundHttpException("The requested user [$id] was not found.");

        }

        $user->spammer = true;

        $user->save(false);

        ...

    }

?>



The verb filter ensures the "flag-as-spammer" action is called with POST method. It will throw yii\web\MethodNotAllowedHttpException if the method is not POST.

http://www.yiiframework.com/doc-2.0/guide-structure-filters.html#verb-filter

Usually you don’t need to mind about the error view. Each error message of the exceptions will be displayed in the ready made error view that is a part of the application template.

http://www.yiiframework.com/doc-2.0/guide-runtime-handling-errors.html

And from the security point of view, there’s no need to worry about the value of ‘id’, because “where([‘id’ => $id])” will use the parameter binding. In fact, you can safely pass “1972; delete * from user;” as $id without fearing a sql injection. The query will only return null as $user, and that’s all that will happen.

http://www.yiiframework.com/doc-2.0/guide-db-query-builder.html

And $id contained in the error message will be properly escaped in the ready made error view. Take a look at your @app\views\site\error.php.

As a whole, you have to try to find out what the framework can do for you. Otherwise you’ll end up with re-inventing of a wheel at the very most.