7
\$\begingroup\$

I'm new to OOP and PHP. I've made an attempt at a PHP library file that handles account creations, logins, and file uploads with image resizing on the fly. It works so far. I'd like some help with the Obj Oriented part, namely making the code more efficient/succinct.

User class:

<?php
define( 'BASEPATH', realpath( dirname( __FILE__ ) ) );
define( 'AppRoot', 'http://www.eclecticdigital.com/eventfinder' );


/**
 * Class user
 */
class user {

    function createAcc() {
        $userData  = new DB();
        $userFiles = new fileHandler();

        $userID = $userData->createNewUser();

        $userpath = BASEPATH . '/users/user_' . $userID . "/";
        $userFiles->userDirectory( $userpath );

    }

    /*END CREATE ACC*/


    /**
     * @param $postVars
     * @return bool
     */
    function loginUser( $postVars ) {
        $newLogin  = new DB();
        $user_data = $newLogin->findUser( $postVars );
        $session   = new userSession( $user_data );

        return true;
    }//**END loginUsr function

    /*function userLogOut{
     1. take $userData and session info and unset session
     2. redirect to homepage
     unset($_SESSION);

    }*/

}

Render class:

<?php
/**
 * Class render
 */
class render {

    /*MAKE STYLESHEETS AND JAVASCRIPT FILES GLOBALLY AVAILABLE TO THE APP*/

    public $homePath;
    public $stylePath;
    public $javascript;
    public $header;
    public $beginHTML;
    public $headerDiv;

    /**
     *
     */
    public function __construct() {
        $this->homePath   = AppRoot;
        $this->stylePath  = $this->homePath . '/style.css';
        $this->javascript = $this->homePath . '/eventsJS.js';
        $this->beginHTML  = '<!Doctype HTML>';

        $this->headTag = '<head><title>Eventfinder</title><link href =" ' . $this->stylePath . '" type="text/CSS"
       rel="stylesheet" /> <link href="http://fonts.googleapis.com/css?family=Playball" rel="stylesheet" type="text/CSS">
       <link href="http://fonts.googleapis.com/css?family=Satisfy" rel="stylesheet" type="text/css">
        <script type="text/javascript" src=" ' . $this->javascript . ' "></script> </head>';

        $this->headerDiv = "<div id=\"header\"> Eventfinder </div>";
    }


    function index() {
        echo $this->beginHTML;
        echo $this->headTag;
        echo $this->headerDiv;

    }

    function usrLogin() {
        $newform = new html_;
        $newform->userLogin();

    }

    function createAcc() {
        $forms = new html_;
        $forms->newUserForm();

    }

}

HTML Class:

<?php
/**
 * Class html_
 */
class html_ {

    public $Type = array( 'button' => 'button', 'checkbox' => 'checkbox', '_file' => 'file', 'hidden' => 'hidden', 'radio' => 'radio', 'r_set' => 'reset', 'submit' => 'submit', '_text' => 'text' );

    public $formOpen = '<form method="post" action="tryDB.php">';
    public $formClose = "</form>";

    public $inputText = '<input \'$name\'>';

    /**
     * @param $method
     * @param $action
     * @param $formName
     */
    function openForm( $method, $action, $formName ) {
        echo "<form method=\"$method\" action=\"$action\"  name=\"$formName\">";

    }

    function newUserForm() {
        echo "<div id=accform>";
        echo "<form method=\"post\" action=\"#\" name=\"newacc\" onsubmit=\"return validateAcc(this)\"> ";

        echo "First Name : &nbsp;<input type=\"text\"  name=\"fname\"  id=\"firstname\" /> <br><br>";
        echo "Last Name  : &nbsp;<input type=\"text\"  name=\"lname\" /> <br><br>";
        echo "email   :&nbsp; <input type=\"text\"  name=\"email\" id=\"email\" /> <br><br>";
        echo "password:&nbsp; <input type=\"password\"  name=\"password\" id=\"password\" /> <br><br>";

        echo "<input type=\"submit\" value=\"create account\" name=\"create\" >";
        echo "</form>";
        echo "</div>";

        echo "<div id=\"errorsDiv\"></div>";
    }

    function userLogin() {
        echo "<div id=login> ";
        $this->openForm( 'post', '#', 'loginForm' );
        echo "email   :&nbsp; <input type=\"text\"  name=\"login\" />";
        echo "password:&nbsp; <input type=\"text\"  name=\"loginPass\" />";
        echo "<input type=\"submit\" value=\"login\" name=\"submit\" />";
        $this->formClose;
        echo "</div>";

    }
}

Filehandler class:

<?php
/**
 * Class fileHandler
 */
class fileHandler {


    /*CREATE FOLDERS FOR NEW USER'S FUTURE UPLOADS*/

    /**
     * @param $userDir
     */
    public function userDirectory( $userDir ) {
        $dir = array( $userDir, $userDir . 'images',
            $userDir . 'images/width_800',
            $userDir . 'images/width_1024',
            $userDir . 'images/width_1400',
            $userDir . 'images/width_1600',
            $userDir . 'images/width_2400' );

        for ( $i = 0; $i < count( $dir ); $i++ ) {
            mkdir( $dir[$i], 0764, true );
            chmod( $dir[$i], 0764 );
        }


    }


    /*DEFINE IMAGE RESIZE FUNCTIONS*/
    /**
     * @param $image
     * @return mixed
     */
    public function imgResize( $image ) {

        $this->userPath = $_SESSION['userFolder'];
        $imgDetails     = explode( '/', $image );
        $imgName        = $imgDetails[2];

        $size    = getimagesize( $image );
        $imgType = $size['mime'];

        $reqdWidths = array( '800', '1024', '1400', '1600', '2400' );
        $width      = $size[0];
        $height     = $size[1];

        $imgRatio = $width / $height;


        /*PUT GD 'IMAGECREATE'FUNCTIONS IN A SWITCH-CASE TO DEAL WITH MULTIPLE IMAGE TYPES */
        /**
         * @param $img
         * @return resource
         */
        function createImage( $img ) {
            $size    = getimagesize( $img );
            $typeImg = $size['mime'];

            switch ( $typeImg ) {
                case 'image/png':
                    $newImage = imagecreatefrompng( $img );

                    return $newImage;
                    break;

                case 'image/jpeg':
                    $newImage = imagecreatefromjpeg( $img );

                    return $newImage;
                    break;

                case 'image/gif' :
                    $newImage = imagecreatefromgif( $img );

                    return $newImage;
                    break;
            }
        }

        /**
         * @param $imgType
         * @param $dest
         * @param $userDestImage
         * @return bool
         */
        function finalImg( $imgType, $dest, $userDestImage ) {
            switch ( $imgType ) {
                case 'image/png':
                    $final = imagepng( $dest, $userDestImage );

                    return $final;
                    break;

                case 'image/jpeg':
                    $final = imagejpeg( $dest, $userDestImage );

                    return $final;
                    break;

                case 'image/gif' :
                    $final = imagegif( $dest, $userDestImage );

                    return $final;
                    break;
            }
        }

        $source = call_user_func( 'createImage', $image );
        for ( $i = 0; $i < count( $reqdWidths ); $i++ ) {
            echo count( $reqdWidths ) . '<br><br>';
            $userDestImage = $this->userPath . '/images/width_' . $reqdWidths[$i] . '/' . $imgName;
            $newWidth      = intVal( $reqdWidths[$i] );
            $newHeight     = intVal( $newWidth / $imgRatio );

            $dest = imagecreatetruecolor( $newWidth, $newHeight );
            echo $userDestImage;

            imagecopyresampled( $dest, $source, 0, 0, 0, 0, $newWidth, $newHeight, $width, $height );
            $imgFinal = call_user_func( 'finalImg', $imgType, $dest, $userDestImage );
        }

        return $imgFinal;
    }


    /*---function to handle file upload. Calls resize when upload is done---*/


    /**
     * @return string
     */
    function file_upload() {


        /**
         * @return string
         */
        function do_upload() {
            $userFolder = $_SESSION['userFolder'];
            echo $_FILES['upFile']['type'] . '<br>';
            $allowedFiles = array( 'image/gif', 'image/jpg', 'image/jpeg', 'image/png' );

            if ( !empty( $_FILES ) && in_array( $_FILES['upFile']['type'], $allowedFiles ) ) {
                //echo $userFolder.'<hr>';
                $tmpFldr  = $_FILES['upFile']['tmp_name'];
                $fileDest = $userFolder . '/' . $_FILES['upFile']['name'];

                if ( move_uploaded_file( $tmpFldr, $fileDest ) ) {
                    echo 'file(s) uploaded successfully';
                } else {
                    echo 'Your file failed to upload<br><br>';
                }

                return $fileDest;
            } else {
                die( 'An error occurred, please make your
                 files are of the correct   type' );
            }

        }

        //END do_upload,

        /*Perform upload return path to file location*/
        $fileLoc = do_upload();

        return $fileLoc;
    }
    //**END file_upload

}

DB class:

<?php
/**
 * Class DB
 */
class DB {

    /*Predefined Queries*/

    /**
     * @return bool|PDO
     */
    public function connect() {
        $dbConn = new PDO( 'mysql:dbname=eh15118b;host=xxxxxx', 'xxxxx', 'xxxx' );
        if ( $dbConn ) {
            echo 'connected';
        } else {
            $dbConn = false;
            echo 'failed to make a connection';
        }

        return $dbConn;
    }

    //NEW USER CREATION FUNCTION
    /**
     * @return string
     */
    public function createNewUser() {
        $find       = "Select email from users where email=:email";
        $find_param = array( ':email' => $_POST['email'] );
        $connDB = $this->connect();

        if ( $connDB ) {
            try {
                $stmt   = $connDB->prepare( $find );
                $result = $stmt->execute( $find_param );
                $row    = $stmt->fetch();
            } catch ( PDOException $e ) {
                die( 'Failed to execute Query: <hr>' . $e->getmessage() );
            }

            if ( !empty( $row ) ) {
                die ( 'There is already an account registered to the address :email .
             You can login here or reset your  password' );
            } else {
                /*SALT AND HASH FOR PASSWORD SECURITY*/
                $salt     = dechex( mt_rand( 0, 2147483678 ) ) . dechex( mt_rand( 0, 2147483678 ) );
                $password = hash( 'sha256', $_POST['password'] . $salt );

                $query = "INSERT INTO users (fname,lname, password,salt, email)
                         VALUES (:fname,:lname, :password,:salt,:email)";

                for ( $round = 0; $round < 65336; $round++ ) {
                    $password = hash( 'sha256', $password . $salt );
                }

                $query_params = array( ':fname'    => $_POST['fname'], ':lname' => $_POST['lname'],
                                       ':password' => $password, ':salt' => $salt,
                                       ':email'    => $_POST['email'] );
                try {
                    $stmt   = $connDB->prepare( $query );
                    $result = $stmt->execute( $query_params );
                } catch ( PDOException $ex ) {
                    /*THIS COULD ALSO BE SENT TO A LOG FILE*/
                    $ex->getmessage();
                }
            }
        }
        $id = $connDB->lastinsertid( 'usrID' );

        return $id;
    }

    //END CREATE NEW USER

    /**
     * @param $vars
     * @return array|null
     */
    public function findUser( $vars ) {
        $sessionVars = null;
        $Conn = $this->connect();
        if ( $Conn ) {
            $user = $vars['user'];
            $pass = $vars['pass'];

            /*FIND user in DB*/
            $usrLogin = $Conn->quote( $user );
            $usrFind  = 'Select email,password,salt,usrID from users where email=' . $usrLogin;
            $stmt     = $Conn->query( $usrFind );
            $result   = $stmt->setFetchMode( PDO::FETCH_NUM );
            $row      = $stmt->fetch();

            if ( !empty( $row ) ) {
                $salt      = $row[2];
                $checkPass = hash( 'sha256', $pass . $salt );

                for ( $round = 0; $round < 65336; $round++ ) {
                    $checkPass = hash( 'sha256', $checkPass . $salt );
                }
                //Check Password and start session
                if ( $checkPass === $row[1] ) {
                    $sessionVars = array( 'user' => $row[0], 'userID' => $row[3] );
                } else {
                    die( 'your password is incorrect.<br><br>' );
                }

            } else {
                echo 'no data in row';
            }

        } else {
            echo 'failed to connect to the Database';
        }

        return $sessionVars;
    }

    //**END FUNCTION findUser


    /**
     * @param $postVals
     */
    function createEvent( $postVals ) {
        $uploads = new fileHandler();

        $image = $uploads->file_upload();
        if ( $uploads->imgResize( $image ) ) {

            echo 'The file : ' . $image . ' was uploaded and resized successfully ';

        } else {
            echo '<HR> !!message or email to admin that an image was not resized successfully';
        }

        echo 'THE VALUES IN POST VALS ARE <BR><BR><BR>';
        print_r( $postVals );
        echo '<HR>';
        var_dump( $image );

        //prepare query to insert into events database
        $query = "INSERT INTO Events (evntName,evntVenue,evntType,evntDate,fullDesc,imgpath)
               VALUES(:evntName,:evntVenue,:evntType,:evntDate,:fullDesc,:imgpath)";

        $params = array( ':evntName'  => $postVals['eName'],
                         ':evntVenue' => $postVals['eVenue'], 'evntType' => $postVals['eType'],
                         ':evntDate'  => $postVals['eDate'], ':fullDesc' => $postVals['Desc'],
                         ':imgpath'   => $image );

        $connDB = $this->connect();
        if ( $connDB ) {
            try {

                $stmt   = $connDB->prepare( $query );
                $result = $stmt->execute( $params );

            } catch ( PDOException $err ) {
                echo $err->getMessage();

                exit;
            }
        } else {
            echo 'NO DATABASE CONNECTION';
            exit;
        }


    }
}

userSession class:

<?php
/**
 * Class userSession
 */
class userSession {

    /**
     * @param $sessionVars
     */
    public function __construct( $sessionVars ) {
        session_start();

        $_SESSION['userEmail']  = $sessionVars['user'];
        $_SESSION['userID']     = $sessionVars['userID'];
        $_SESSION['userFolder'] = 'users/user_' . $_SESSION['userID'];
        $_SESSION['sessID']     = session_id();

    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ createNewUser and findUser should be abstracted from the database class \$\endgroup\$ – Meredith Jun 3 '14 at 20:33
  • \$\begingroup\$ Hi Meredith..just so I understand correctly...which part of the createNewUser and finduser should be moved out of the DB class. Surely not the queries themselves right? \$\endgroup\$ – steveBK Jun 4 '14 at 15:29
  • 1
    \$\begingroup\$ Well, usually I have a query method in my database classes that accepts a query and an array of parameters. The database class shouldn't do any specific operations, just allow the user to interact with the database. \$\endgroup\$ – Meredith Jun 4 '14 at 16:21
7
\$\begingroup\$

Executive summary:

  • Pick a naming standard and stick to it (img != image for instance)
  • Learn how to write comment blocks ( /* description / /* @var */ is kinda weird)
  • SOLID is a good place to start (google it)
  • PSR-1 and PSR-2
  • Don't store html in strings, just don't.
  • Again, don't use a class to represent a 'template'. Php does a really good job as a templating engine <myhtml><?php echo $data ?></myhtml>
  • your database isn't really abstracted (more on this later)
  • I miss interfaces :( (why? I just like them)

So why bother commenting? - You did a good effort in separating concern - you actually wrote some comments - You are using PDO

Long part

(More text will follow, I simply don't feel like writing everything in 1 go):

FileHandler or no, fileHandler

Naming convention is everything . When using code, you should not have to think about syntax. It should all feel very natural. Your code doesn't. Some examples:

the FileHandler class is actually fileHandler. Always start your class names with a capital letter. Why? because. It's a convention, we all do it, it makes writing code easy.

Now, lets say I want to use your fileHandler class. I copy paste it into my project and initialize it:

$myAwesomeStolenFileHandler = new fileHandler();
$myAwesomeStolenFileHandler->createImage($myImage);

OW sh*@t, function not found. Huh? but it's there in the code allright. Ow, wait, it's not a method of the class, it's a function inside a method. I'm ot supposed to use it, so why not make it private? Instead of creating it everytime I call createImage. wow, thats enoying. uh ok, well, I was planning on adding a watermark but i'll wate for SteveBK to implement it I guess. Ok, well lets use the imageresizer then.

$myNotSoAwesomeStolenFileHandler->resizeImage($image);

Again, method not found.

Huh? confusion

Aaah, it's img not image. and in this class somehow it's imgResize and not resizeImage.

Ok, next try;

$myNotSoAwesomeStolenFileHandler->imgResize($image);

Note that I had to read the method itself to be able to know what @param $image actually is (is a String? is it a binary object, is it, ...)

Recap:

  • Comments aren't really helping
  • A lot of functionality is cramped into one method (and those strange function creations on the fly in a method, iieeewww)
  • naming of the methods could be a lot better.

Rewrite:

What does the name FileHandler tell me? Well, without reading the code it tells me that it's a class that aids me in handling files. I could for instance want to get a list of files. Add a file, upload one, rename it, get a FileInstance to do some extra stuff with the file itself, ... You know, the usual stuuf a FileHandler does. it would look something like this:

<?php
interface FileHandler
{
    /**
     * Define the root path for the FileHandler
     * The FileHandler should not be able
     * to access files outside this root folder
     * 
     * @param String $root the root path
     */
    public function __construct($root);

    /**
     * Returns an array of all file and directory names in the current directory
     * IF $showDirectries == false
     *  exlude the directories
     * 
     * @param  boolean $showDirectories
     * @return String[]
     */
    public function listAll($showDirectories=true);

    public function changeDirectory();

    public function getCurrentDirectory();

    public function getFile($name);
}

Yes, Id din't finish all the comments. Complains can be written to /dev/null

Interface?

Yes, I wrote an interface and not a class. why? here is the reason. An interface is a contract, It tells us programmers what a certain class/object can do if it implements this interface. This also goes the other way round, if we need an object of a certain interface. we simply type-hint for that interface and we get really nice error messages that help us in the debug proces (for instance if someone is trying to cramp a MouseHandler to your app instead of a FileHandler)

Back to the FileHandler that actually should have been a ImageInstance or ImageFile

As you see, I now wrote a piece of code that has nothing to do with your app, simply because you didn't need a FileHandler. You need a ImageResizer, or whatever you need.

Handling independent files and/or images

Our FileHandler has a method getFile() and let's for the sake of codereview and my essay I'm writing here go all the way and have it a return an object of type FileInstance.

The FileInstance

A FileInstance is a representation of a file. Lucky for us, we don't have to write this ourself, there is a really good built int class for this, the SplFileInfo class

What about my ImageResize

good question, the ImageResizer is a function/method that needs a FileInstance and then does some really cool stuff to it (like resizing). The ImageResizer needs a File or a link to a file. We us the SplFileInfo object here.

ImageResizer inteface

Here we can go for different approaches, I will go for an ImageResizer that has a resize method that resizes a given image according to a certain set of rules (only width is supported here)

<?php
interface ImageResizer
{
    /**
     * Constructor, set some defaults
     * @param int $width           the width of the image
     */
    public function __construct($width);

    /**
     * Resize the image according to a set of rules defined in the __construct
     * 
     * @param  SplFileInfo $image the image to resize
     * @param  SplFileInfo $destination the destination for the resized image
     * @return void
     * @throws MymeTypeNotSupportedException If myme_type<$image> is not supported
     * @throws DestinationFileNotWritable If !$destination->isWritable()
     */
    public function resize(SplFileInfo $image, SplFileInfo $destination);
}

And now, lets implement the sh*@t out of this interface

<?php

class MimeTypeNotSupportedException extends Exception {}
class DestinationFileNotWritableException extends Exception {}
class NotAFileException extends Exception {}
class FileNotReadableException extends Exception {}

class AwesomeImageResizer implements ImageResizer
{
    private $width;
    private $infoReader;

    /**
     * Constructor, set some defaults
     * 
     * @param int $width           the width of the image
     */
    public function __construct($width)
    {
        $this->width = $width;
        $this->infoReader = new finfo(FILEINFO_MIME_TYPE);
    }

    /**
     * Resize the image according to a set of rules defined in the __construct
     * 
     * @param  SplFileInfo $image the image to resize
     * @param  SplFileInfo $destination the destination for the resized image
     * @return void
     * @throws NotAFileException If ! $image->isFile()
     * @throws MimeTypeNotSupportedException If myme_type<$image> is not supported
     * @throws DestinationFileNotWritableException If !$destination->isWritable()
     */
    public function resize(SplFileInfo $image, SplFileInfo $destination)
    {
        #is it a file?
        if ( !$image->isFile() )
        {
            throw new NotAFileException();
        }

        #is destination writable?
        if ( !$destination->isWritable() )
        {
            throw new DestinationFileNotWritableException();
        }

        #get the mimeType
        $mimeType = $this->infoReader->file($image);

        #get the image resource
        $imageResource = $this->getImageResource();

        #let's do the fancy resizing work
        $width = imagesx($imageResource);
        $height = imagesy($imageResource)
        $newWidth = $this->width;
        $newHeight = $height * $newWidth / $width;

        $newResource = imagecreatetruecolor($newWidth, $newHeight);

        imagecopyresampled(
            $newResource,
            $imageResource,
            0, 0, 0, 0,
            $newWidth,
            $newHeight,
            $width,
            $height
        );

        #create the new image
        $newImage = $this->createImage($newResource, $mimeType);

        #write the new image to the file
        $file = $destination->openFile('w');
        $file->fwrite($newImage);
    }

    /**
     * calls the corresponding php method
     * to create a resource from a certain mimtype
     * 
     * @param  SplFileInfo $image
     * @param  string $mimeType 
     * @return resource
     * @throws MimeTypeNotSupportedException If myme_type<$image> is not supported
     */
    private function getImageResource(SplFileInfo $image, $mimeType)
    {
        //is the image readable?
        if ( !$image->isReadable() )
        {
            throw new FileNotReadableException();
        }
        switch ($mimeType) {
            case 'image/png':
                return imagecreatefrompng($image->getPathName());
            break;

            case 'image/jpeg':
                return imagecreatefromjpeg($image->getPathName());
            break;

            case 'image/gif' :
                return imagecreatefromgif($image->getPathName());
            break;

            default:
                #sorry, can't help ya
                throw new MimeTypeNotSupportedException($mimeType);
            break;
        }
    }

    private function createImage($resource, $mimeType)
    {
        switch ($mimeType) {
            case 'image/png':
                return imagepng($resource);
            break;

            case 'image/jpeg':
                return imagejpeg($resource);
            break;

            case 'image/gif' :
                return imagegif($resource);
            break;

            default:
                #sorry, can't help ya
                throw new MimeTypeNotSupportedException();
            break;
        }
    }
}

Wow, what code, so much wow.

This is however untested and only written for explanation ;)

Some usage examples:

<?php
$mediumResizer = new AwesomeImageResizer(800);

$mediumResizer->resize(
    new SplFileInfo('my/current/image.png'),
    new SplFileInfo('my/destination/image.png')
);

#and agin with error handling
try {
    $mediumResizer->resize(
        new SplFileInfo('other/current/image.png'),
        new SplFileInfo('other/destination/image.png')
    );
} catch ( NotAFileException $e )
{
    echo "Oh nows, the image isn't even a file";
    die();
} catch( FileNotReadableException $e)
{
    echo "Aaahhh, I'm blind. The file is unreadable, aaaahhhhhhhhh!";
} catch ( MimeTypeNotSupportedException $e )
{
    echo "Oh nows, ze program has no clue what to with your mime_type: " . $e->getMessage();
    die();
} catch ( DestinationFileNotWritableException $e )
{
    echo "Oh nows, the destination file doesn't want me to write to it :(";
    die();
}

But what about all those throws, and exceptions? Well, when something goes wrong, you should not echo an error message, or die();

See how I'm handling errors outside of my ImageReszier? this is good. Because let's face it. An ImageResizer that has to handle resizing of images AND error handling, that's a lot. Even for our AwesomeImageResizer, we all have a limit ;)

Ok, I hope that with writing this huuuuuuge ImageResizer I have given you a better biew into writing OO code.

Ok, it's a bit of a long read, but I'll come back soon with more text ;)

On Comments:

A good read: http://pear.php.net/manual/en/standards.sample.php Note ofcourse that this is proabably overkill. But it's good to know how far you can push it ;)

\$\endgroup\$
  • \$\begingroup\$ Hi Pinoniq...I appreciate the time you've taken to do all this. As I said, im new to both OOP and PHP and this seems a little over my head. I'm sure I'll come to understand it in time. For now though...u mentioned that my DB class is not abstracted, this seems a key thing. I'd like to see how that's wrong before moving on to other stuff like class interfaces which I haven't even read about yet. I'm still trying to come to a thorough understanding of the fundamentals.I'm just starting a PHP certification course everything you've done here will definately come in handy \$\endgroup\$ – steveBK Jun 4 '14 at 15:20
  • \$\begingroup\$ aha, I'll edit my post this evening with some more basic stuff :) \$\endgroup\$ – Pinoniq Jun 4 '14 at 15:21
1
\$\begingroup\$

Security

SQL Injection

Sometimes you use prepared statements, which is good, but other times you don't. This is vulnerable:

$usrFind  = 'Select email,password,salt,usrID from users where email=' . $usrLogin;

XSS

print_r( $postVals ); looks like a debug statement, but even debug statements should be secure. If you don't expect arrays as input, this would be a simple solution: print_r(array_map('htmlentities', $postVals));

File Upload

$_FILES['upFile']['type'] is completely user controlled, so an attacker can upload for example PHP files which would result in code execution.

What you should do is check the file extension as well as the actual file type. The functions that are generally recommended for this are pathinfo and finfo_file respectively. Ideally, you should use whitelists, not blacklists.

It is a good idea to additionally forbid execution inside the upload directory, ideally by using server configuration as well as OS configuration, but this should not be your only line of defense.

You are currently setting these rights:

mkdir( $dir[$i], 0764, true );
chmod( $dir[$i], 0764 );

This means read for anyone, read/write for the group, and read/write/execute for the owner. The problem is that the owner is the webserver, as the webserver is the one creating these files. To make the directory actually non-executable, 664 or similar would be better.

Misc

  • echoing database errors to end users is not a good idea. It simplified finding of security issues, and actually aids in some attacks such as SQL injections into INSERT, which show retrieved data in error messages.
  • you should use session_regenerate_id(true) when the state of a session changes to prevent session fixation (not much of an issue with default php.ini settings, but still).
\$\endgroup\$

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.