0
\$\begingroup\$

First of all I just started learning OOP.

My priority is to make safe code. I think my code is safe, because I use GD to make a copy of the uploaded image and I use an image proxy to make sure nobody can access the uploaded file directly and it's uploaded outside of the webroot. But if I was 100% sure that it's safe I was not posting it here haha. I'm not sure about the way I check the MIME type, and about the GD.

I made some tests and it works fine, but what do you guys think? Any suggestions are welcome. Also if you have any questions about the code just ask me in the comments.

Class Img:

class Img { private static $folder, $file, $ext, $w, $h; public static function upload($file, $new_size){ self::$ext = pathinfo($file["name"], PATHINFO_EXTENSION); list(self::$w, self::$h) = ($size = getimagesize($file["tmp_name"])); if(!$size || !in_array(self::$ext, ['png', 'jpeg', 'jpg']))die(json_encode(['error' => 'Invalid file'])); if(filesize($file["tmp_name"]) > 2700000)die(json_encode(['error' => 'File is too big'])); self::$folder = dirname(__DIR__).'/../../uploads/profile/'.$_SESSION['user_folder']; self::$file = $file["tmp_name"]; $_SESSION['avatar'] = ($name = mt_rand(100, 100000).'-'.time().'.'.self::$ext); self::resize_image($name, $new_size, $new_size); $user_id = $_SESSION['user_id']; $conn = \lib\Db::instance(); $query = $conn->query("UPDATE users SET avatar = '{$name}' WHERE user_id = '{$user_id}'"); $conn = NULL; die(json_encode(['avatar' => $_SESSION['avatar']])); } private static function resize_image($name, $width, $height){ if(self::$ext === 'jpeg')self::$ext = 'jpg'; if(self::$ext === 'jpg')$img = imagecreatefromjpeg(self::$file); elseif(self::$ext === 'png')$img = imagecreatefrompng(self::$file); $ratio = max($width/self::$w, $height/self::$h); $x = (self::$w - $width/$ratio)/2; self::$h = $height/$ratio; self::$w = $width/$ratio; $new = imagecreatetruecolor($width, $height); // preserve transparency if(self::$ext === "png"): imagecolortransparent($new, imagecolorallocatealpha($new, 0, 0, 0, 127)); imagealphablending($new, false); imagesavealpha($new, true); endif; imagecopyresampled($new, $img, 0, 0, $x, 0, $width, $height, self::$w, self::$h); if(self::$ext === 'jpg')imagejpeg($new, self::$folder.$name); elseif(self::$ext === 'png')imagepng($new, self::$folder.$name); imagedestroy($new); } }

avatar_upload.php:

require_once dirname(__DIR__).'/../../vendor/autoload.php'; if(!isset($_SESSION))session_start(); if($_SERVER['REQUEST_METHOD'] != 'POST' || empty($_SESSION['user_id']))die(header('HTTP/1.1 404 Not Found')); if(!empty($_FILES['avatar']['size']))\lib\Img::upload($_FILES["avatar"], 150);
\$\endgroup\$

    1 Answer 1

    Reset to default
    1
    \$\begingroup\$
    • Fight the urge to squeeze multiple "things" into one line. Your script will be easier to read and maintain with all declarations and constructs occupying their own rightful place. Spend the extra lines, you'll be happy you did a year from now.
    • Use curly braces to encapsulate language constructs (e.g. if-else and foreach, etc.), this combined with appropriate tabbing will make your script easier to read and help to prevent mishaps when expressing conditional outcomes. Start but don't stop reading about coding standards here: https://blog.sideci.com/5-php-coding-standards-you-will-love-and-how-to-use-them-adf6a4855696
    • Instead of murdering your script with die() calls, return a consistent data type no matter the outcome. In the future, you may change how you want to display/deliver the results. Finally, convert your returned array to json and echo.
    • When writing variables into a query, you should be using a prepared statement with placeholders and bound variables.
    • Avoid single-use and unnecessary variable declarations. If you have $_SESSION['avatar'] then you don't need to declare $name.
    • Starting your session is not a conditional thing. Do it early and do it every time.
    • You might like to ask yourself why you've elected to write static methods. This is a topic worth investing some research time into. Start, but don't stop here: When should I use static methods?

    There is more to tell you, but I have run out of time.

    \$\endgroup\$
    3
    • \$\begingroup\$You said that i should use prepared statements but is it because of the variable $ext? I mean, the variable $user_id is from a column with AUTO_INCREMENT, and the $name is made with the function mt_rand(), so i don't see other reason to use prepared statements. I only use it when there's user input.\$\endgroup\$
      – mario
      CommentedJul 15, 2019 at 21:53
    • \$\begingroup\$I can't see the code where $_SESSION['user_id'] is generated. If it is at all insecure, then it becomes a potential attack vector for this script. If it may possibly contain characters that will monkeywrench your query, then it becomes a potential point of breakage. Regardless of how stable/secure you believe the SESSION data is, I recommend consistently using prepared statements to write variables into your queries. If the $_SESSION['user_id'] is an integer (and you are not going to use a prepared statement) you could use explicit casting ((int)$_SESSION['user_id']).\$\endgroup\$
      – mickmackusa
      CommentedJul 15, 2019 at 22:48
    • \$\begingroup\$in the php.ini i had set sessions to httponly and secure. The user id is generated in the database with AUTO_INCREMENT, so i think it's safe. But i have a question, do you trust the value from the variable $ext? I meant, not to check the MIME type, but to use it in the name of the file and in the sql (which i did), I made a check with the function in_array, but i don't know if it's enough to trust.\$\endgroup\$
      – mario
      CommentedJul 16, 2019 at 0:43

    Start asking to get answers

    Find the answer to your question by asking.

    Ask question

    Explore related questions

    See similar questions with these tags.