Request class in PHP

Question

I am implementing a Request class in my MVC framework that stores all the required information from a page request.

It surely does it's job, but I was wondering if this is secure enough. I see alot of people advising against the use of $_SERVER['REQUEST_URI'], since this can be manipulated by the end user. Have I made a good workaround, or does it still contain vulnerabilities?

class Request
{
    // contains the URL of the request
    private static $url = null;

    // contains the request type of the request : GET | POST
    private static $type = null;

    // contains the segments of the request
    private static $segments = null;

    // contains the name of the controller of the request
    private static $controller = null;
    // contains the name of the method for the controller
    private static $method = null;
    // contains additional parameters
    private static $data = null;

    /*
     * Prevent this class from being called 'non-statically'
     */
    private function __construct() {}

    /*
     * Returns the Request object, so it can be used as a dependency
     */
    public static function getRequest()
    {
        return new self;
    }

    /*
     * Stores all Request info into the class
     */
    public static function init()
    {
        self::$url = Sanitizer::sanitizeURL(rtrim(substr($_SERVER["REQUEST_URI"], 1), '/'));
        self::$type = $_SERVER['REQUEST_METHOD'];
        self::$data = (object) array();

        self::parseURL();

        $postParameters = filter_input_array(INPUT_POST);
        $cookieParameters = filter_input_array(INPUT_COOKIE);

        if(!is_null($postParameters))
        {
            foreach($postParameters as $parameter => $value)
            {
                if(!isset(self::$data->postParameters))
                {
                    self::$data->postParameters = (object) array();
                }

                self::$data->postParameters->{$parameter} = $value;
            }
        }

        if(!is_null($cookieParameters))
        {
            foreach($cookieParameters as $parameter => $value)
            {
                if(!isset(self::$data->cookieParameters))
                {
                    self::$data->cookieParameters = (object) array();
                }

                self::$data->cookieParameters->{$parameter} = $value;
            }
        }
    }

    /*
     * Grabs all the info from the requested URL
     */
    private static function parseURL()
    {
        $url = self::$url;
        self::$segments = explode('/', $url);

        self::$data->getParameters = array_values(array_diff(array_slice(self::$segments, 2), array('')));
        self::$data->controller = isset(self::$segments[0]) && !empty(self::$segments[0]) ? self::$segments[0] : 'index';
        self::$data->method = isset(self::$segments[1]) && !empty(self::$segments[1]) ? self::$segments[1] : 'index';
    }

    /*
     * Returns the requested URL
     */
    public static function getURL()
    {
        return self::$url;
    }

    /*
     * Returns the request type
     */
    public static function getType()
    {
        return self::$type;
    }

    /*
     * Returns the request data
     */
    public static function getData()
    {
        return self::$data;
    }

    /*
    * Returns the name of the controller
    */
    public static function getController()
    {
        return self::$data->controller;
    }

    /*
    * Returns the name of the method for the controller
    */
    public static function getMethod()
    {
        return self::$data->method;
    }
}

As you may have noticed, the Sanitizer class gets called in the process. Here is what that class does:

class Sanitizer
{
    private function __construct() {}

    /*
     * Sanitize a given URL
     */
    public static function sanitizeURL($url)
    {
        return htmlspecialchars(strip_tags($url));
    }

}

The URLs that will be requested should (if valid) look like this: {website}/profile/edit/20.

And this is how everything gets called in my framework (note that this might be improved later on)

<?php
class Seromium
{
    public static function boot()
    {
        require "lib/Autoloader.php";
        require "lib/Config.php";
        Config::init();
        Autoloader::init();
        Request::init();
    }
}
// boot the framework
Seromium::boot();

So once again, is this code secure enough? If not, what is wrong with it and how can it be solved?

Note: Later on in my project, I will build the Router itself, which will validate if the controllers et cetera are valid and contain the given methods.

Answer 1

Good to see the continuations of your efforts here. Some thoughts:

I would tend to think of a request as being a concrete object, capable of being passed around as a dependency to the various code in your framework that needs it vs. just having it be statically accessible.

Why?

You want to guarantee that code needing this request information has it set up in a correct state. When thinking about a router for example, you want to hand the router a valid Request object to work with. If that object cannot be properly instantiated (maybe because of bad input), you should never even get to the point in your code where the router is invoked.

If using the static approach here, rather than passing a Request to a router, you would likely end up having code in the router that tries to statically access the request info (even if the request is not in a proper state). This means you now need a bunch of guarding code in the router class to handle these edge cases where the request information is not in a valid state.

This is in contrast to say your Seromium::boot() method, where I think there is a good argument for static invocation, as the Seronium class is not going to be needed as a dependency anywhere else in your code and the class and method serve a very narrow purpose in your application (perform a one-time initialization sequence).

General purpose classes/libraries for validation (like your Sanitizer) or other not-instance-specific functionality are also potential candidates for a static interface, primarily if you want to provide some grouping around a common sets of methods or constant values. One could arguably use namespaced functions/constants for the same purpose as well.

---

I do see that you at least try to give Singleton-like concrete behavior to the Request, but really, the properties should not be static in this class. I am not really sure why Singleton is that important to establish here either. Would it be simpler to have all your instantiation logic triggered in your constructor, with exception being thrown if there is any invalid data that should prevent the object from being instantiated? Why introduce static/concrete duality?

---

I would break up the instantiation logic. I might envision a constructor and helper methods like:

public function __construct() {
    $this->setHttpRequestMethod();
    $this->setUriProperties();
    $this->setInputData();
}

protected function setHttpRequestMethod() {
    $this->httpRequestMethod = $this->validateHttpRequestMethod($_SERVER['REQUEST_METHOD']);
}

protected function validateHttpRequestMethod($input) {
    if(empty($input)) {
        throw new InvalidArgumentException('I need valid value');
    }
    switch ($input) {
        case 'GET':
        case 'POST':
        case 'PUT':
        case 'DELETE':
        case 'HEAD':
            return $input;
            break;
        default:
            throw new InvalidArgumentException('Unexpected value.');
            break;
    }
}

protected function setInputData() {
    switch ($this->httpRequestMethod) {
        case 'GET':
        case 'HEAD':
            $this->setDataFromGet();
            break;
        case 'POST':
            $this->setDataFromPost();
            break;
        case 'PUT':
            $this->setDataFromPut();
            break;
        case 'DELETE':
            // do nothing, no data to set
            break;
        default:
            throw new Exception(
                "Unmapped httpActionMethod. Value provided: '{$this->httpRequestMethod}'";
            );
    }
}

//  and so on...

Using this approach you can prevent instantiation of the Request altogether (with exception being thrown) if the input does not meet basic validation standards of this class.

Breaking this logic up in this manner also provides more flexibility to extend this base class. For example, maybe at some point, you want to handle JSON requests, so you make a JsonRequest class that inherits this class. You might, for example, only need to override methods on validating/setting input data while keeping the rest of the base class behaviors.

---

I like your use of filter_input_array(), but would suggest that you likely need to add some more meaningful validation than what you have. Obviously, validation of individual field values would probably be deferred to individual models in the MVC, but is there some other basic validation that needs to happen here? For example, if POST http action is called, but $_POST is empty you could raise an some sort of exception against the request before getting to routing step).

You also need to actually check the return from filter_input_array() to determine if the filter passed.

What if the data payload (for POST or UPDATE) is not pre-populated by PHP in $_POST because you are not dealing with form-encoded data type (like you are using application/json for example)? In these cases, filter_input_array() would not be meaningful.

---

It would probably be considered better form (more explicit) to replace this:

self::$data = (object) array();

with

self::$data = new stdClass();

---

The Request class does not seem to consider a GET request. Would there ever be parameters passed in URI on GET? If parametric data will only be passed in URI, then perhaps you need to validaate that the URI does not contain ?... or #...

---

You seem to use URL and URI interchangeably. These are not the same thing. I don't see where you are actually working with URL at all, so perhaps some variable renaming is in order.

---

Consider parse_url() or similar functionality for you URL/URI parsing. No reason to re-invent the wheel here. The only additional functionality you probably need is breaking up the URI path component into individual segments.

---

I would consider whether you should assign value to those URI path segments in this class (i.e. which segments specify controllers, actions on controller, parameters, etc.) You should leave it up to your router and controllers to assign meaning, not the class that is simply trying to encapsulate the state of the request.