# Secure file upload for PDF only

I am trying to create a secure file upload using PHP 7+ where I only allow PDF files. I found a lot of posts on this topic on different websites but couldn't find a complete solution that ensures that no harmful files can be uploaded this way.

So far I have the following code. Can someone tell me if I am missing any important steps here or if anything should be changed or removed in my code?

(Note: I am not interested in the old x-pdf file types.)

<?php
include 'session.php';

if (empty($_FILES['files'])) { echo json_encode(['error'=>'No files found for upload.']); return; } if(!empty($_POST['csrfToken'])) {
if(hash_equals($_SESSION['csrfToken'],$_POST['csrfToken'])) {
$postData =$_POST;
$files =$_FILES['files'];
$uploadRef = preg_replace('/[^A-Za-z0-9]/', '',$_GET['uploadRef']);
$categoryId = preg_replace('/[^A-Za-z0-9]/', '',$_GET['categoryId']);
$tags = preg_replace('/[^A-Za-z0-9,]/', '',$_GET['tagsList']);
$success = null;$paths= [];
$filenames =$files['name'];

for($i=0;$i < count($filenames);$i++){
if($_FILES['file']['error'] !== UPLOAD_ERR_OK) { die('Upload failed with error ' .$_FILES['file']['error']);
}

$fileTitle =$files['name'][$i];$fileTitle = substr($fileTitle, 0 , (strrpos($fileTitle, ".")));
$fileExtensions = explode('.', basename($filenames[$i]));$fileExtension = strtolower(array_pop($fileExtensions));$ok = false;
switch($fileExtension) { case 'pdf':$ok = true;
default:
die('Unknown/not permitted file type');
}

$finfo = finfo_open(FILEINFO_MIME_TYPE);$mime = finfo_file($finfo,$_FILES['file']['tmp_name']);
$ok = false; switch($mime) {
case 'application/pdf':
$ok = true; default: die('Unknown/not permitted file type'); }$uploadId = md5(uniqid()) . '_' . $i;$target = 'uploads' . DIRECTORY_SEPARATOR . $uploadId . '.' .$fileExtension;
if(move_uploaded_file($files['tmp_name'][$i], $target)) {$success = true;
$paths[] =$target;

$conn = new mysqli($dbHost, $dbUser,$dbPw, $dbName); if($conn->connect_error) {
exit($trans['errorConnectionFailedTxt'][$lang]);
}
mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
$conn->set_charset('utf8mb4');$stmt = $conn->prepare("INSERT INTO uploads (uploadId, uploadRef, categoryId, tags, fileTitle, fileExtension) VALUES (?, ?, ?, ?, ?, ?)");$stmt->bind_param("ssssss", $uploadId,$uploadRef, $categoryId,$tags, $fileTitle,$fileExtension);
$stmt->execute();$stmt->close();
$conn->close(); } else {$success = false;
break;
}
}

if ($success === true) {$output = [];
} elseif ($success === false) {$output = ['error'=>'Error while uploading images. Contact the system administrator'];
foreach ($paths as$file) {
unlink($file); } } else {$output = ['error'=>'No files were processed.'];
}

unset($postData); echo json_encode($output);
} else {
echo json_encode('invalid CSRF token');
}
} else {
echo json_encode('no CSRF token');
}
?>

• How many header('Location...') calls are you planning to make? Sep 12 '20 at 9:36
• @mickmackusa: One. Why ? Sep 12 '20 at 10:08
• It's inside a loop. Do you mean to redirect? Sep 12 '20 at 10:13
• @mickmackusa: Thanks. Yes, I mean to redirect after successful upload, but only once (also in case of multiple files) so I may have placed this wrongly. Sep 12 '20 at 10:33
• At 7am on a Sunday I am sleeping in my bed. Sep 12 '20 at 21:07

if(!empty($_POST['csrfToken'])) { if(hash_equals($_SESSION['csrfToken'], $_POST['csrfToken'])) {  I think that this should be performed in a separate method that validates tokens. Furthermore, I think that this should end in an HTTP error (as indicated by the other answer). Above is called a "guard statement", it checks if the state or parameter is valid before the method is executed. If it isn't valid, it should terminate it. Termination can be performed locally, you definitely don't want to have to scroll through the method to find:  } else { echo json_encode('invalid CSRF token'); } } else { echo json_encode('no CSRF token'); }  at the end. Furthermore, if you simply exit the method you remove a layer of indentation making the method less complex to read. ## Unclear regular expressions $uploadRef = preg_replace('/[^A-Za-z0-9]/', '', $_GET['uploadRef']);  This should also be a method, not so much because the regular expression is hard to understand (the "what"), but it isn't clear why it is performed. ## Success? $success = null;


Success is a boolean, it should not be used as a variable with three values. Use either two variables or an enumeration. Furthermore, $success is a terrible bad name, try filesUploaded or similar. If we'd check that count($filenames) is zero beforehand we can simply set the output beforehand and skip the rest of the execution (remember the guard statements). Programming is all about limiting complexity.

die('Upload failed with error ' . $_FILES['file']['error']); }  ## checking MIME type Instead of calling finfo_open(FILEINFO_MIME_TYPE) just to get the mime-type, the mime-content-type() function could be used. $mime = mime-content-type($_FILES['file']['tmp_name'])  Additionally, the mime-type may be provided from the browser in $_FILES['files']['type'][$i] though it "not checked on the PHP side and therefore don't take its value for granted."1 ## variable $postData - really needed?

After postData is assigned, it only appears to be used in one spot- passed to unset(). It hardly seems necessary...

## variadic field name checks

While the form submitted to this script is not included one can only guess as to the fields. The script checks both $_FILES['files'] as well as $_FILES['file'] - while the former most likely allows multiple files to be uploaded, are there really two different file input fields?

## database query in loop

The code currently inserts records into the database on each iteration of the loop. Consider using only one statement to insert all records. This will minimize database connections (which can increase execution time) as well as prevent invalid data from being inserted (e.g. if first file was valid but subsequent files were not).

## regular expressions to replace characters

The regular expressions could perhaps be simplified using the character type \w though that includes the underscore characters i.e. _ but must those be removed? Also not that preg_replace() won't safely clean multi-byte strings so consider that if unicode strings need to be supported.