PHP page to edit a user account information and avatar

Question

I wanted to ask your opinion about my code created for acccount.php page for editing the data of each logged-in user.

I wanted to have more of an opinion on your right syntax, though maybe it could be improved.

    require('includes/config.php'); 

//if not logged in redirect to login page
if(!$user->is_logged_in()) { 
    redirect('login.php');
} 

//define page title
$title = 'Account';

//include header template
require('layout/header.php');


//submit post
if (isset($_POST['location'], $_POST['bio'], $_POST['relationship'], $_POST['interested'], $_POST['work'], $_SESSION['username'])) {
    try {
    //query update
    $stmt = $db->prepare("UPDATE members SET location = ?, bio = ?, relationship = ?, interested = ?, work = ? WHERE username = ?");
    $stmt->execute([$_POST['location'], $_POST['bio'], $_POST['relationship'], $_POST['interested'], $_POST['work'], $_SESSION['username']]);
      $message = $lang['update-profile-success']; //success message
    } catch(PDOException $e) {
    $message = $lang['update-profile-error']; //error message
    }
  }

// ******************  AVATAR  *************************
//choosing the avatar:
    if(isset($_POST['upload']))
    {

        $imgFile = $_FILES['avatar']['name'];
        $tmp_dir = $_FILES['avatar']['tmp_name'];
        $imgSize = $_FILES['avatar']['size'];

        if(empty($imgFile)){
            $message = "Please Select Image File.";
        }
        else
        {
            $upload_dir = 'avatar/'; // upload directory

            $imgExt = strtolower(pathinfo($imgFile,PATHINFO_EXTENSION)); // get image extension

            // valid image extensions
            $valid_extensions = array('jpeg', 'jpg', 'png', 'gif'); // valid extensions

            // rename uploading image
            $userpic = rand(1000,1000000).".".$imgExt;

            // allow valid image file formats
            if(in_array($imgExt, $valid_extensions)){           
                // Check file size '5MB'
                if($imgSize < 5000000)              {
                    move_uploaded_file($tmp_dir,$upload_dir.$userpic);
                }
                else{
                    $message = "Sorry, your file is too large.";
                }
            }
            else{
                $message = "Sorry, only JPG, JPEG, PNG & GIF files are allowed.";       
            }
        }

        // if no error occured, continue ....
        if(!isset($message))
        {

        $edit = $db->prepare("UPDATE members SET user_avatar = :avatar WHERE username ='" . $_SESSION['username'] . "'");
        $edit->bindParam(':avatar', $userpic);
        $row['user_avatar'] = $userpic;

            if($edit->execute())
            {
                $message = "Avatar updated...";
                header("refresh:5;account.php"); // redirects image view page after 5 seconds.
            }
            else
            {
                $message = "Error...";
            }
        }
    }


//run data form
$sql = $db->prepare("SELECT * FROM members WHERE username = ?");
$sql->execute([$_SESSION['username']]);
$row = $sql->fetch();

Answer 1

Security

Possibly Broken Authentication

if(!$user->is_logged_in()) { 
    redirect('login.php');
} 

[do important stuff]

Make sure that redirect dies. The header redirect you do use doesn't, and if the one of redirect doesn't either, an attacker could easily bypass this check, as a client does not have to follow a header redirect if they do not want to.

Possibly SQL Injection

You mostly use prepared statements, which is good. It's a bit surprising that you do not always use them though:

$edit = $db->prepare("UPDATE members SET user_avatar = :avatar WHERE username ='" . $_SESSION['username'] . "'");

Depening on your - current or future - limitations on username, this is open to SQL injection. You really should always use prepared statements, even if you think the data may be safe. If you need to think about this each time, you will eventually make a mistake.

Misc

• your indentation is off, making your code harder to read.
• you have quite a lot of comments which only describe what the code does. Most of them can simply be removed, others seem to be there for structuring your code (AVATAR, choosing the avatar, etc). Instead, add more classes/files and functions. Eg you could have a Avatar class, which has a function uploadAvatar (uploadAvatar would likely use a more generic uploadImage or uploadFile function stored in a more generic class).

• your code is quite deeply nested, making it hard to read. If you follow the advice of adding more functions, you will already greatly reduce the nesting. Additionally, return early. So if you have checks at the beginning of a function, don't do if (cond) [do the complex thing], but instead if (!cond) return; [newline] [do the complex thing] (instead of just returning, you could return true/false, an error message, throw an exception, etc.

• Error... is not a very informative error message.