For example, a simple mvc type system:
/api/class/method rewritten into php variables using .htaccess/nginx.conf
then doing 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 horribly bad practice? If it is bad practice, can someone explain exactly why? And if so, is there a better way to do what i'm doing?
EDIT Essentially the reason i'm using variable variables is to make it simple to expand the core system - ie - adding in a new controller is nice and simple. I definitely understand the security risks of allowing essentially any function or class to be instantiated without some kind of filter.
The 'some_filter_here' could be a list of controllers that are allowed - whitelist as some here have mentioned.
They're not desirable, but it's fine to use them how you have.
A couple of pointers, though: your code does have a vulnerability where an attacker could traverse your directory with
$_GET
parameters like?class=../base
. If that file exists, yourfile_exists()
call will returntrue
and your application will attempt to include it and instantiate it as a class.The safe scenario would be to whitelist those parameters to be letters, numbers and underscores only (if you separate words with underscores, i.e.
.php
).Also, I prefer the syntax of using
call_user_func
andcall_user_func_array
. Using these functions in your code would look as follows:Since you're writing the "some_class_filter" and "some_method_filter" code, I'd say it's OK. You also have a error or default handler I see, so in the end, I'd say it's alright.
I believe many MVC frameworks operate in a similar fashion anyway.
Yes, this is rather bad practise. Do you need a variable variable for that instance? In other words, do you need more than one class & method to be instantiated in a given request? Your URI structure suggests not. If not, you could just use:
Otherwise, you might want to do:
This avoids polluting the scope with variable variables, which are harder to track.
As far as the existence checks for your class in a given directory, this should be a sufficient whitelist (presuming an attacker cannot write to that directory).
EDIT: As a further check, you may want to consider checking
method_exists
on the object before calling the method.