2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Is there a reason why everything is static in this class? \$\endgroup\$ – insertusernamehere Jan 11 '17 at 20:48
  • \$\begingroup\$ @insertusernamehere Yes. There will only be one request, so there is no reason to support multiple instances of a request. Even better, no two Requests are allowed. \$\endgroup\$ – Gerrit Luimstra Jan 11 '17 at 21:09
  • \$\begingroup\$ Static classes have two major downsides: They are hard to extend and not suitable for unit tests, especially since PHPUnit removed mocking/stubbing of static methods. See: When to use static vs instantiated classes, Static vs Instance Method, PHPUnit testing singleton dependencies, Unit testing static calls without refactoring the world \$\endgroup\$ – insertusernamehere Jan 11 '17 at 21:58
  • \$\begingroup\$ How do you use the class? It looks like you have to call init() before accessing anything. Can you give an example of what a usage would look like? \$\endgroup\$ – Seb Jan 12 '17 at 11:54
  • \$\begingroup\$ Why do you use htmlspecialchars on the url? You don't want html entities in your url. \$\endgroup\$ – Seb Jan 12 '17 at 11:57
2
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Once again thank you so much for a nice review. \$\endgroup\$ – Gerrit Luimstra Jan 12 '17 at 21:06
  • \$\begingroup\$ I really understand what you are after this time. I haven't thought about the Request being called concrete like this before, thank you for the insight, it makes alot of sense. I also get why my Request property should not be static and I will fix that asap. At some point you suggest validation the Request information inside the Request class, but how much harm does it do, when I move this into an external static class? I like to seperate the validation of the data with the data itself as much as possible, just like you use external Exceptions. \$\endgroup\$ – Gerrit Luimstra Jan 12 '17 at 21:06
  • \$\begingroup\$ Also, nice point on the JSONRequest, didn't think about that. I don't quite understand what you mean about the filter_input_array() function (yet), but I will look into that. All the replacements you suggested are right on spot and I will implement them. Good point on the ` $_GET` parameters, will change that. I indeed hussled the URI and URL back and forth in the framework.. Will also change that. Will implement the parse_url(); function. Can't believe I missed that. \$\endgroup\$ – Gerrit Luimstra Jan 12 '17 at 21:07
  • \$\begingroup\$ @GerritLuimstra I think it is OK to, for example, group a set of validation methods like you are trying to do with Sanitizer, though I would suggest that Sanitizer may be too a name and naming such as RequestValidation might be better to indicate these are a set of functions meant for validation in one specific domain (for the request). I honestly don't know how much validation is going to be applicable in Request though, so I might start with validation methods being in that class until such a point where I saw wider applicability for those methods in the application. \$\endgroup\$ – Mike Brant Jan 12 '17 at 21:14
  • \$\begingroup\$ Ahh makes sense. Thanks for the explanation :) \$\endgroup\$ – Gerrit Luimstra Jan 12 '17 at 21:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.