Create a class based on user input

Question

It's not safe to require $input . '.php'. To then institate a class. How can I make it secure, without needing to use say a whitelist of classes that can be inistiated.

Ex 1. (bad code).

<?php

$input = $_GET['controller'];

require $input . '.php';

new $input;

?>

Answer 1

Disclaimer

I should start by saying that defining static routes in your system is secure by design whereas this answer, even though I've made efforts to mitigate security issues, should be thoroughly tested and understood before trusting its operation.

The basics

First, make sure the controller contains a valid variable name using a regular expression as taken from the manual; this weeds out obvious erroneous entries:

$controller = filter_input(INPUT_GET, FILTER_VALIDATE_REGEXP, [
    'options' => [
        'regexp' => '/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$/',
        'flags' => FILTER_NULL_ON_FAILURE,
    ]
]);

if ($controller !== null) {
    // load and use controller
    require_once("$controller.php");
    $c = new $controller();
}

Enforcing hierarchy

This works well, but what if someone tries to load an internal class instead? It might horribly fail the application.

You could introduce an abstract base class or interface that all your controllers must extend or implement:

abstract class Controller {}

// e.g. controller for '?controller=admin'
class Admin extends Controller {}

Btw, to avoid name conflicts, you could define these inside a separate namespace.

And this is how you would enforce such a hierarchy:

if ($controller !== null) {
    // load and use controller
    require_once("$controller.php");
    if (is_subclass_of($controller, 'Controller')) {
        $c = new $controller();
    }
}

I'm using is_subclass_of() to type check before instantiating the class.

Auto loading

Instead of using a require_once() in this case, you could use an auto loader instead:

// register our custom auto loader
spl_autoload_register(function($class) {
    $file = "$class.php"; // admin -> admin.class.php
    if (file_exists($file)) {
        require_once $file; // this can be changed
    }
});

This is also the place where you can normalize the class name, so that it maps better to a file name, as well as enforcing a custom namespace, e.g. "App\\$class.php".

This reduces the code by one line, but making the loading more flexible:

if ($controller !== null) {
    // check hierarchy (this will attempt auto loading)
    if (class_exists($controller) && is_subclass_of($controller, 'Controller')) {
        $c = new $controller();
    }
}

All this code assumes you have proper error handling code in place; for implementation suggestions you can look at this answer.

Answer 2

A couple of suggestions:

• Put your controller classes in its own dedicated folder, containing ONLY controller classes

• Make your filter as strict as possible eg.

  /* is $_GET['controller'] set? */
  if (!isset($_GET['controller'])) {
      // load error or default controller???
  }
  
  $loadController = $_GET['controller'];
  
  /* replace any characters NOT matching a-z or _ (whitelist approach), case insensitive */
  $loadController = preg_replace('/[^a-z_]+/i', '', $loadController);
  
  /* verify var is not empty now :) */
  if (!$loadController) {
      // load error or default controller???
  }
  
  /* if your classes are named in a certain fashion, eg. "Classname", format the incoming text to match ** NEVER TRUST USER INPUT ** */
  $loadController = ucfirst(strtolower($loadController));
  

• Check if the file exists Why not file_exists? see desc

  /* avoiding using file_exists as it also matches folders... */
  if (!is_file($myControllerClassesPath.$loadController.'.php')) {
      // load error or default controller???
  }
  

• Then require the file, and verify that the class itself exists

  require($myControllerClassesPath.$loadController.'.php');
  
  /* of course, this assumes filename === classname, adjust accordingly */
  if (!class_exists($loadController)) {
      // load error or default controller???
  }
  

• Then of course, new instance of X

  new $loadController;