2
\$\begingroup\$

I'd like to know if this code is usable, because I'm not sure, thank you. The code is used to upload images to a predefined folder for users.

if(isset($_FILES['avatar']) AND !empty($_FILES['avatar']['name'])) {
$tailleMax = 6844685465456456;
$extensionsValides = array('jpg', 'jpeg', 'gif', 'png');

if($_FILES['avatar']['size'] <= $tailleMax) {

   $extensionUpload = strtolower(substr(strrchr($_FILES['avatar']['name'], '.'), 1));
   
   if(in_array($extensionUpload, $extensionsValides)) {

      $resultat = move_uploaded_file($_FILES['avatar']['tmp_name'], 'uploads/'.$_SESSION['id'].'.'.$extensionUpload);
       
      if($resultat) {

   

     echo "success";
             
          } else {

            echo "Erreur durant l'importation de votre photo de profil extension";
          }
       } else {
           
        echo "Votre photo de profil doit être au format jpg, jpeg, gif ou png";
       }
    } else {
        
       echo "Votre photo de profil ne doit pas dépasser 2Mo";
    }
 }else{
     echo ':/';
 }
\$\endgroup\$
2
\$\begingroup\$

Just a few remarks:

I am wondering how that magic value of 6844685465456456 was determined :) I don't think that is equivalent to 2 Mb.


You need to use proper indentation, especially when you have nested control blocks - readability is important. Lack of readability here can result in a greater likelihood of logic errors.


This code is not helpful at all:

 }else{
     echo ':/';
 }

It doesn't tell the user what the problem is. Don't you hate those forms that tell you 'invalid input, try again' but don't actually tell you the problem and leave you guessing ?


This code (borrowed from mickmackusa):

if (!move_uploaded_file($_FILES['avatar']['tmp_name'], "uploads/{$_SESSION['id']}.{$extensionUpload}")) {
    exit("Erreur durant l'importation de votre photo de profil extension");
}

You are showing an error message to the user but it should be treated like an exception. This code could fail for several reasons like:

  • invalid file name
  • disk full/not mounted
  • etc

But at this point it's not the user fault. It's a malfunction in your application or a server issue. There is nothing the user can do. So I would handle the exception and send an alert. If your application is broken you'll want to be notified and fix it asap, before someone reaches out to you.


This code is not foolproof:

$resultat = move_uploaded_file($_FILES['avatar']['tmp_name'], 'uploads/'.$_SESSION['id'].'.'.$extensionUpload);

You are expecting that the session variable ID will be set. It's perfectly possible that the user has the page opened already and the session expires after some time. When submitting the form the session variable could be unset, unless you have more code elsewhere (an include perhaps) that verifies that the session is active. If there is no active session you'll probably redirect the user to the login page.

So the resulting file name could be incorrect and not what you expected (eg: .jpg). It could be one reason why move_uploaded_file would fail in your script. In itself it doesn't present an obvious security risk though but it is a bug.

\$\endgroup\$
0
\$\begingroup\$
  1. if(isset($_FILES['avatar']) AND !empty($_FILES['avatar']['name'])) { can more simply be: if(!empty($_FILES['avatar']['name'])) { because if the latter is true, then the former is true as well.

  2. $tailleMax, $extensionsValides, $resultat are "single-use variables". In the majority of cases, I prefer not to declare single-use variables because they end up needlessly bloating my scripts. That said, there are times when they are advisable:

    1. When the value is somewhat mysterious in what it is for.
    2. When the declaration prevents writing a line of code which is excessively long.

    If you think you may use the first two variables elsewhere in your project, then it would be sensible to store then in a config file for ease of use and maintenance.

  3. There is A LOT of debate on the web about how to best determine the extension / mime type of an imported file and cleanse it of any malicious code. On Code Review and Information Security you may research as much as you like. After spending a couple hours reading heaps of posts all over Stack Exchange about how everything can be spoofed by the user and how using the GD extension to recreate the uploaded image, I'm actually not game enough to post a claim on what is best / most secure. If you merely want the portion of the filename after the latest dot, then there are several ways to isolate that with fewer function calls.

  4. As a personal preference, I like to write all of my failing conditions earlier in my script and reserve the successful outcome(s) for the later in the script. I don't know if you need to exit() or return for your script, but using these process ending calls will prevent your script for using "arrow head" tabbing (an antipattern). In other words, this will keep more of your code "left" and require less horizontal scrolling in your IDE.

Here's an untested version of what I am suggesting that assumes that you are doing something like passing a string response back to an ajax call:

if (empty($_FILES['avatar']['name'])) {
    exit('No importation');
}
if ($_FILES['avatar']['size'] > 6844685465456456) {
    exit('Votre photo de profil ne doit pas dépasser 2Mo');
}
$extensionUpload = strtolower(pathinfo($_FILES['avatar']['name'], PATHINFO_EXTENSION));
if (!in_array($extensionUpload, ['jpg', 'jpeg', 'gif', 'png'])) {
    exit('Votre photo de profil doit être au format jpg, jpeg, gif ou png');
}
if (!move_uploaded_file($_FILES['avatar']['tmp_name'], "uploads/{$_SESSION['id']}.{$extensionUpload}")) {
    exit("Erreur durant l'importation de votre photo de profil extension");
}
exit('success');
\$\endgroup\$
2
  • \$\begingroup\$ Thank's for your message \$\endgroup\$ – Elk Delf Jun 28 '20 at 17:53
  • \$\begingroup\$ Please never thank in a comment. \$\endgroup\$ – mickmackusa Jun 29 '20 at 20:05

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.