Is it bad to use variable variables in php like this?

Question

For example, a simple system like mvc:

/ api / class / method rewritten to php variables using .htaccess / nginx.conf

then do something like:

<?php // Set up class + method variables $className = some_class_filter($_GET['class']); $method = some_method_filter($_GET['method']); // Check if class exists and execute if(file_exists(BASE . "/controllers/" . $className . ".class.php")) { require BASE . "/controllers/" . $className . ".class.php"; $$className = new $className(); // Execute the method $$className->$method(); } else { // Spit out some error based on the problem } ?> 

Is this a terribly bad practice? If this is bad practice, can someone explain why exactly? And if so, is there a better way to do what I do?

EDIT Essentially, the reason I use variable variables is to simplify the extension of the base system - that is, adding a new controller is nice and easy. I definitely understand the security risks that allow almost any function or class to instantiate without any filter.

"some_filter_here" may be a list of allowed controllers - a "white list", as some of them mentioned.

Answer 1

Yes, this is a pretty bad practice. Do you need a variable variable for this instance? In other words, do you need more than one class and method to instantiate in a given request? Your URI structure does not assume. If not, you can simply use:

 $object = new $className(); $object->$method(); 

Otherwise, you can do:

 $objects = array(); $objects[$className] = new $className(); $objects[$className]->$method(); 

This avoids contamination of the area with variable variables that are more difficult to track.

As for checking if your class exists in this directory, it should be a sufficient whitelist (assuming that the attacker cannot write to this directory).

EDIT . As an additional check, you might consider checking the method_exists object before calling the method.

Answer 2

Since you are writing the code "some_class_filter" and "some_method_filter", I would say that everything is in order. You also have a default or error handler that I see, so in the end I would say that everything is in order.

I believe that many MVC frameworks work anyway.

Answer 3

They are not desirable, but it is great to use them as you have.

A few pointers: your code is vulnerable when an attacker could cross your directory with $_GET parameters such as ?class=../base . If this file exists, your call to file_exists() will return true , and your application will try to include it and create an instance as a class.

A safe scenario should be to assign white letters, numbers and underscores to these parameters (if you select words with underscores, i.e. .php ).

In addition, I prefer the syntax for using call_user_func and call_user_func_array . Using these functions in your code will look like this:

 <?php $class_name = $_GET['class']; $method_name = $_GET['method']; $parameters = $_GET; unset($parameters['class'], $parameters['method']); // grabs any other $_GET parameters if (file_exists(BASE.'/controllers/'.$class_name.'.class.php')) { require BASE.'/controllers/'.$class_name.'.class.php'; $controller = new $class_name(); $response = call_user_func_array(array($controller, $action_name), $parameters); } else { header('HTTP/1.1 404 Not Found'); // ...and display an error message }