9
\$\begingroup\$

I'm working on a general Requestmethod class which sanitizes and recasts the input of users in an automatic fashion. I've also tried to do array-access in cookies.

My biggest question are:

  1. Is this too much for one class alone?
  2. Is the readability/thinking right on this?

Code:

    Example: (<input type="checkbox" name="checkboxes[]" />)
    $a = new Request('POST');
    $b = $a->getArray('checkboxes');
    $b->get(0);
    $b->get(1);
    $b->get(2);

    class REQUESTCONF {
        const XSS = false;
        const SALT = 'anysalt';

        const c_expire  = 24;
        const c_path    = '/';
        const c_domain  = '';
        const c_secure  = false;
        const c_httponly = false;
    }



     /**
         * Class to get Secure Request Data
         * 
         * XSS Levels:
         * 0 / false = off
         * 1 = htmlentities
         * 2 = strip tags
         * 
         * @package Request
         */
        class Request {

            private $DATA;
            private $CURSOR = false;
            private $XSS = false;
            private $METHOD;

            /**
             * Constructor
             * 
             * @package Request
             * @param string $METHOD POST GET SESSION or COOKIE
             * @param boolean $XSS  XSS Prevent Level
             * @uses sanitize(

);
         * @return object Request
         * @access public
         */
        function __construct($METHOD,$XSS=false) {
            $this->METHOD = strtoupper($METHOD);
            $this->XSS = $XSS;

            switch ($this->METHOD) {
                case 'FILE':
                    $this->DATA = $_FILES;
                break;

                case 'GET':             
                    $this->DATA = $_GET;
                break;

                case 'POST':                
                    $this->DATA = $_POST;               
                break;

                case 'COOKIE':
                    foreach($_COOKIE as $k => $v) {
                        // hiding Notice - but no other way :'( 
                        $array = @unserialize($v);
                        if ($array && is_array($array)) {
                            $this->DATA[$k] = $array;
                        } else {
                            $this->DATA[$k] = $v;
                        }
                    }               
                break;

                case 'SESSION':
                    $this->DATA = $_SESSION;
                break;

                default:
                    trigger_error('Parameter must be a valid Request Method (GET, POST, FILE, SESSION or COOKIE)',E_USER_ERROR);
                    die();
                break;
            }

            $this->DATA = $this->sanitize($this->DATA);
            return $this;
        }

        /**
         * Destruct - clean up
         * 
         * @package Request
         * @access public
         */
        function __destruct() {
            $this->save();

            if ( $this->METHOD == 'SESSION' )
                session_regenerate_id();

            unset($this->XSS);
            unset($this->DATA);
            unset($this->CURSOR);
            unset($this->METHOD);
        }

        /**
         * Removes a Value with $key of $this->DATA
         * 
         * @package Request
         * @param string $key   Arraykey of Element in $DATA     
         * @access public
         */
        public function remove($key){
            if ( $this->CURSOR != false && isset($this->DATA[$this->CURSOR]) && is_array($this->DATA[$this->CURSOR]) ) {
                unset($this->DATA[$this->CURSOR][$key]);
            } elseif (isset($this->DATA[$key])) {
                unset($this->DATA[$key]);
            } else {
                return false;
            }
        }

        /**
         * Dumps whole $DATA
         * 
         * @package Request 
         * @access public
         */
        public function dump(){
            var_dump($this->DATA);
        }

        /**
         * Set Value in $DATA
         * 
         * @package Request
         * @param string $key   Arraykey of Element in $DATA
         * @param mixed $value  String Integer Float or Bool want to set in $DATA
         * @return boolean
         * @access public
         */
        public function set($key,$value){
            if ( $this->CURSOR != false && isset($this->DATA[$this->CURSOR]) && is_array($this->DATA[$this->CURSOR]) ) {
                $this->DATA[$this->CURSOR][$key] = $value;
                return 1;
            }

            if (is_array($value))
                return false;

            $this->DATA[$key] = $value;
            $this->setToken();

            return $this;
        }

        /**
         * Get a Value from $DATA with validation
         * 
         * @package Request
         * @uses object validateString
         * @param string $key               Arraykey of Element in $DATA
         * @param mixed $validate           Regex Rule for validation or false 
         * @return mixed $this->DATA[$var]  Contents of the Arraykey
         * @access public
         */
        public function validGet($key,$validateRegex){
            if (!$this->checkToken())
                return false;

            if ( $string = $this->get($key) ) {
                if (preg_match($validateRegex,$string))
                    return $string;
            }

            return false;
        }

        /**
         * Get Filesize when Cursor is on a FILE Request
         * 
         * @package Request 
         * @return int  Filesize in Bytes of File
         * @access public
         */
        public function getFilesize(){      
            if ($this->METHOD != 'FILE' || !$this->CURSOR)
                return false;

            return $this->DATA[$this->CURSOR]['size'];
        }

        /**
         * Get Filename when Cursor is on a FILE Request
         * 
         * @package Request 
         * @return string   Name of File
         * @access public
         */
        public function getFilename(){
            if ($this->METHOD != 'FILE' || !$this->CURSOR)
                return false;

            return $this->DATA[$this->CURSOR]['name'];
        }

        /**
         * Get MIME when Cursor is on a FILE Request
         * 
         * @package Request 
         * @return string   MIME-Type of File
         * @access public
         */
        public function getFileType(){
            if ($this->METHOD != 'FILE' || !$this->CURSOR)
                return false;

            // When File is a Picture getimagesize() mime is more secure and can handle PSDs - exif not     
            if ($img = getimagesize($this->DATA[$this->CURSOR]['tmp_name'])) {
                return $img['mime'];
            } elseif (isset($this->DATA[$this->CURSOR]['type'])) {
                return $this->DATA[$this->CURSOR]['type'];
            } else {
                return false;
            }
        }

        /**
         * Saves the File to a given destination
         * 
         * @package Request 
         * @param string $dest  Save Destination
         * @return boolean  
         * @access public
         */
        public function saveFile($dest){
            if ($this->METHOD != 'FILE' || !$this->CURSOR)
                return false;

            if (move_uploaded_file($this->DATA[$this->CURSOR]['tmp_name'],$dest)) {
                return true;
            } else {
                return false;
            }
        }

        /**
         * Sets the Cursor to a FILE Request
         * 
         * @package Request
         * @param string $key   Arraykey of Element in $DATA     
         * @access public
         */
        public function getFile($key){
            if ($this->METHOD != 'FILE')
                return false;
            //sanitize filename => no leading dot
            return $this->getArray($key);
        }
        /**
         * Get a Value from $DATA
         * 
         * @package Request
         * @param string $key               Arraykey of Element in $DATA
         * @return mixed $this->DATA[$var]  Contents of the Arraykey    
         * @access public
         */
        public function get($key){
            if (!$this->checkToken())
                return false;

            if ( $this->CURSOR != false && isset($this->DATA[$this->CURSOR][$key]))
                return $this->DATA[$this->CURSOR][$key];

            if (!isset($this->DATA[$key]))
                return false;

            return $this->DATA[$key];
        }

        /**
         * Set a Array in $DATA 
         * 
         * @package Request
         * @param array $array      Array you want to Store in $DATA
         * @return boolean true
         * @access public
         */
        public function setArray($key,$array){
            if (!is_array($array) OR $this->METHOD == 'FILE')
                return false;

            $this->DATA[$key] = $array;
            $this->setToken();
            return $this;
        }

        /**
         * Sets the Cursor to an existing arraykey from Data when its an Array
         * Otherwise set it to false and return false
         * 
         * @package Request
         * @param string $key       Key of an Array in $DATA 
         * @return object Request
         * @access public
         */
        public function getArray($key){
            if (!isset($this->DATA[$key]) || !is_array($this->DATA[$key])) {
                $this->CURSOR = false;
                return false;
            }

            $this->CURSOR = $key;
            return $this;
        }

        /* Sanitizer
         * 
         *
         * @package Request
         * @return array    sanitized and pseudotyped Array (since POST and GET is String only)
         * @access private
         */
        private function sanitize($array){
            foreach ($array as $k => $v) {
                if (is_numeric($v)) {
                    $array[$k] = $v + 0;
                    if ( is_int($v) ) {
                        $array[$k] = (int) $v;
                    } elseif ( is_float($v) ) {
                        $array[$k] = (float) $v;
                    }
                } elseif (is_bool($v)) {
                    $array[$k] = (bool) $v;
                } elseif (is_array($v)) {
                    $array[$k] = $this->sanitize($array[$k]);
                } else {
                    if ($this->XSS > 0) {
                        switch ($this->XSS) {
                            case 1:
                                $array[$k] = htmlentities(trim($v));
                            break;

                            case 2:
                                $array[$k] = strip_tags(trim($v));
                            break;
                        }
                    } else {
                        $array[$k] = (string) trim($v);
                    }
                }
            }
            return $array;
        }

        /**
         * refill the original REQUEST
         * 
         * @package Request      
         * @access public
         */ 
        public function save() {
            switch($this->METHOD) {
                case "GET" :
                    $_GET = $this->DATA;
                break;

                case "POST" :
                    $_POST = $this->DATA;
                break;

                case "SESSION" :
                    $_SESSION = $this->DATA;
                break;

                case "COOKIE" :
                    $expire = time()+3600*REQUESTCONF::c_expire;

                    foreach ($this->DATA as $K => $V) {
                        if ( is_array($V)) {
                            setcookie($K,serialize($V),
                                $expire,
                                REQUESTCONF::c_path,
                                REQUESTCONF::c_domain,
                                REQUESTCONF::c_secure,
                                REQUESTCONF::c_httponly
                            );
                        } else {
                            setcookie($K,$V,
                                $expire,
                                REQUESTCONF::c_path,
                                REQUESTCONF::c_domain,
                                REQUESTCONF::c_secure,
                                REQUESTCONF::c_httponly
                            );  
                        }
                    }
                break;
            }

            return 1;
        }

        /**
         * Generates a Token with data serializing and a given salt from Config 
         * saves it in the first level of Session or Cookie
         * 
         * @package Request
         * @access private
         */
        private function setToken() {       
            if ($this->METHOD == 'SESSION' || $this->METHOD == 'COOKIE') {

                if ( isset($this->DATA['TOKEN']))
                    unset($this->DATA['TOKEN']);

                $this->DATA['TOKEN'] = crc32(serialize($this->DATA).REQUESTCONF::SALT);
            }
        }

        /**
         * checks the inserted Tokenhash with actual Data in Session or Cookie
         * 
         * @package Request 
         * @return boolean      true on success false on fail
         * @access private
         */ 
        private function checkToken() {
            if ($this->METHOD != 'SESSION' && $this->METHOD != 'COOKIE' )
                return 1;

                if ( isset($this->DATA['TOKEN'])) {
                    $proof = $this->DATA['TOKEN'];
                    unset($this->DATA['TOKEN']);

                    if ( $proof != crc32(serialize($this->DATA).REQUESTCONF::SALT) ) {
                        return false;
                    } else {
                        $this->DATA['TOKEN'] = crc32(serialize($this->DATA).REQUESTCONF::SALT);
                        return 1;
                    }
                } else {
                    return false;
                }               
        }
    }

    function Request($method,$xss=false) {
        if (!$xss)
            $xss = REQUESTCONF::XSS;

        return new Request($method,$xss);
    }
    ?>
\$\endgroup\$

1 Answer 1

13
\$\begingroup\$

Right, let me be clear, I mean this in the nicest of ways, but I don't like this code one bit. Let me start off a couple of simple things:

Please, follow the coding standards as described by PHP-FIG

I've noticed you're (ab)using the error supressing operator (eg @unserialize). Don't. Errors and Warnings are there to help you improve on your code. If there's a warning issued, don't cover it up, fix it. Even more worrying is where I saw this being used:

foreach($_COOKIE as $k => $v)
{
    $array = @unserialize($v);
    if ($array && is_array($array))
    {
        $this->DATA[$k] = $array;
    }
    else
    {
        $this->DATA[$k] = $v;
    }
}

Now this implies you're actually setting cookies to hold serialized objects or arrays. First off, it gives me a solid clue on what your stack looks like (I know for a fact you're using PHP). Serialized arrays are easily changed client side, so there's no guarantee on data integrity. If they're serialized objects, you're in trouble... big time. That would mean you're actually sending an object to the client, containing its state, and possibly all sorts of information on your server. If I were to be a 16 year old would-be hacker, I'd feel like I just struck gold. With some trial and error, I could easily manipulate that objects state, to gain access to those parts of your app you probably rather I didn't know about.
Cookies should cosist of little slivers of, on its own, meaningless data, Sessions are where you could store serialized objects, but still: I wouldn't.

Your Request class has a public function __construct function defined. Why then, do you also define a function Request? It looks as though you're trying to catch errors if somebody omitted the new keyword, or you're trying to mimic the factory pattern. Either implement a factory, or drop the function. If it's there to catch code that doesn't construct its instances using new, then that code contains bugs: fix them, don't work around them.

Next: The REQUESTCONF class, which only contains constants (ignoring the naming issues here). These constants obviously belong to the Request object: the Request object's state is defined by it. Drop that class, which is used as a constant array anyway, and define the constants as Request constants.

You don't need to call all those unset's in the destructor. Dealing with the session is fine, anything else is just overhead (the values will be unset when the object goes out of scope anyway), which is right after the destructor returns.

Moving on to some actual code:

public function getArray($key)
{
    if (!isset($this->DATA[$key]) || !is_array($this->DATA[$key])) {
        $this->CURSOR = false;
        return false;
    }
    $this->CURSOR = $key;
    return $this;
}

This doesn't make sense, to me at least. I'd expect the return type of getArray to be either an array or null, you're returning false, or the object itself. I see what you're doing here, but the name is just begging for accidents to happen.
either implement a child of the Traversable interface (which is what you're trying to do anyway) or change the method-name.

Your first question: Is this too much for one class?
Yes, it is. You might want to take a look at existing frameworks and how they manage the Request, what objects they use and how they're tied in.

Your Request object deals with everything, from the basic GET params to sessions. They're not the same thing, and should be treated as seperate entities. You're using a class as a module, which violates the single responsability principle.
I'd suggest you write a class for each request that requires its own treatment. Take, for example, a Session class. That class should indeed implement a destructor, but a Get object shouldn't. Their constructors should be different, too, but they can both implement the same basic getter and setter.

Start off by writing an abstract RequestType class, that holds the shared methods/properties, and extend it with the various request-type classes:

abstract class RequestType
{
    protected $data = null;
    protected $writeable = true;
    abstract public function __construct();//ensure all children implement their own constructor
    public function __get($name)
    {
        if (!is_array($this->data) || !isset($this->data[$name]))
        {//or throw exception
            return null;
        }
        return $this->data[$name];
    }
    public function __set($name, $val)
    {
        if ($this->writeable === false)
        {
            throw new RuntimeException(sprintf('%s instance is Read-Only', get_class($this)));
        }
        $this->data[$name] = $value;
        return $this;
    }
}
//then
class Session extends RequestType
{
    public function __construct($id = null, $readOnly = false)
    {
        $this->writeable = !!$readOnly;
        //start session, assign to $this->data
    }
}

You get the idea... You can use the abstract class for type-hinting, and implement the Traversable interface there, too.

Your main Request object then sort of becomes a service, then:

class Request
{
    private $session = null;
    private $cookie = null;
    private $post = null;
    private $get = null;
    private $xhr = false;//isAjax()
    private $uri = null;
    //type constants
    const TYPE_COOKIE = 1; //000001
    const TYPE_SESSION = 2;//000010
    const TYPE_POST = 4;   //000100
    const TYPE_GET = 8;    //001000
    const TYPE_URI = 16;   //010000
    const TYPE_AJAX = 32;  //100000
    const TYPE_ALL = 63;   //111111
    //config constants
    const CONF_XSS = 0; //use PDO-style: array(Request::CONF_XSS => XSS_ON)
    const XSS_ON = 1;
    const XSS_OFF = 0;

    private static $objects = array(
        1  => 'Cookie',
        2  => 'Session',
        4  => 'Post',
        8  => 'Get',
        16 => 'Uri',
        32 => 'Ajax'
    );
    private $options = array(
        self::CONF_XSS => XSS_OFF,
        'default'      => 'config here'
    );
    public function __construct($type = self::TYPE_ALL, array $options = null)
    {
        if (is_array($options))
        {
            foreach($options as $conf => $value)
            {
                $this->options[$conf] = $value;
            }
        }
        if ($type === self::TYPE_ALL)
        {
            foreach(self::$objects as $type)
            {//assume setPost, setSession, ..
                $this->{'set'.$type}($this->options);
            }
            return $this;
        }
        if (($type & ($type - 1)) === 0 && ($type & self::TYPE_ALL) === $type)
        {//^^ $type is power of 2 , and it's value < 63 ===> constant exists
            $this->{'set'.self::$objects[$type]}($this->options);
        }
        return $this;
    }
}

You implement, as the constructor shows, your set<Type> methods, and some lazy-loading get<Type> methods, too, and you're good to go.
In case you're not sure what I mean by lazy-loading getters:

public function getSession()
{
    if ($this->session === null)
    {
        $this->session = new Session($this->options);//session_start is the responsability of the Session class!
    }
    return $this->session;
}
\$\endgroup\$
5
  • \$\begingroup\$ Thank you for your insights and efforts to explain me this. I didnt think of that lazyload Method you are using there - thats really handy. And i have no problems at all when you say my code smells - the reason why i post this here is good critique - which you gave me. But yeah my REQUESTCONF i use it as "Pseudo-Enum" for my Config File because its the easiest way for me to do global Configurations. \$\endgroup\$
    – wandregal
    Commented Aug 7, 2013 at 9:22
  • \$\begingroup\$ @wandregal: You're welcome. Though that REQUESTCONF did remind me of enum. Still, declaring those constants in your Request class does the same thing: Request::XSS_ON is available without there being an instance of the class, too, so why bother creating 2 classes, and 9/10 having to call the autoloader twice? \$\endgroup\$ Commented Aug 7, 2013 at 9:25
  • \$\begingroup\$ Simply because i didnt know PHP can handle that and i see now ive overcomplicated my whole Problem here. Thats also "just" a new learning experience for me because OOP PHP is pretty new for me. \$\endgroup\$
    – wandregal
    Commented Aug 7, 2013 at 9:36
  • \$\begingroup\$ @wandregal: As always, there's only one place to look: php.net: on importing, and docs on namespaces in general \$\endgroup\$ Commented Aug 7, 2013 at 9:40
  • 1
    \$\begingroup\$ @wandregal: I've added some more "critique" on your using the @ operator and where you're using that operator. I must stress that there are some rather important things to fix for you there, too. \$\endgroup\$ Commented Aug 9, 2013 at 7:01

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