Calling variable methods to handle ajax requests

Question

I made a controller.php file to handle ajax requests, with $_POST parameters action and module

if (isset($_POST["action"])) {
    $action = strtolower($_POST["action"]);

    if ($action === "get_notification") {
        //  return session notification messages
       //...

    } elseif (isset($_POST["module"])) {
        require_once("libraries/class.module.php");
        $module = new Module;

        $moduleName = strtolower($_POST["module"]);

        //  check if module-name is valid
        if ($module->verify($moduleName)) {
            // load  $modulename class
            require_once("libraries/class.".$moduleName.".php");

            //  factory used to create instance of $moduleName
            require_once("libraries/class.factory.php");

            // note: moduleName class extends Module class
            $module = $Factory->create($moduleName);

            //  verify method of $moduleName verifies action/method parameter
            //  using method_exists and cross-checking against a permitted methods list

            if ($module->verify($action)) {
                $message = $module->$action();
                echo json_encode($message);

            } else {
                //  handle invalid requests
                echo json_encode(["0", "Invalid request received"]);
            }

        } else {
            //  handle invalid requests
            echo json_encode(["0", "Invalid request received"]);
        }

    } else {
        //  handle invalid requests
        echo json_encode(["0", "Invalid request received"]);
    }

} else {
    //  handle invalid requests
    echo json_encode(["0", "No request received"]);
}

But I read this post on Stack Overflow which advises against using variable methods. Should I opt for a switch case that checks each $action and calls corresponding method. That will result in a lot more code which was the initial reason I opted for this variable method solution.

Yew, __you shall__. Let the client (browser) send a variable as a parameter which should import and call a class can be seriously dangerous. — Gabriel Heming, May 10 '17 at 14:32
you really ought to whitelist the possible actions. That's what every framework out there (PHP or not) does. Anything else is asking for trouble. — ADyson, May 10 '17 at 14:52
@ADyson how about blacklisting the prohibited actions instead of white-listing the allowed ones :) — , May 10 '17 at 17:14
@feetnappy depends which list is larger. Chances are in most situations, the allowed list will be smaller. And if you whitelist, you know you've definitely allowed something through, but if you blacklist, you need to be 100% certain you haven't forgotten to ban something. That makes whitelisting more secure IMO. — ADyson, May 11 '17 at 08:45

score 0 · Answer 1 · edited May 23 '17 at 11:47

It looks like what you really need is a proper routing system. You could make it yourself (as described here). Or use an existing solution, like FastRoute.

As for actually using "variable methods", in general it is a bad idea. But, if those methods are at the beginning of your call-stack (executed from the bootstrap-file), they are a really practical option.

When it comes to security concerns, the only public method in your controller classes should be ones, that you expect to be called. Which mean that you can simply do:

if (method_exists($controller, $action)) {

P.S. You don't have to do strtolower() there, because in PHP the class methods are not case-sensitive (and neither are the class names themselves).

P.P.S. Look into using composer as PSR-4 autoloader

Calling variable methods to handle ajax requests

1 Answers1