-3

I have a simple php form that I use to upload and store files. It's basically working via MySQL, Apache, and php. I have upload.php which file I use to upload files in directory of the project rootdir/uploads

I am renaming the file during the uploading via php script and storing the filename in MySQL table. Here is part of the code for renaming and uploading the file:

$upload_dir = 'uploads/'; 
$file_extension = pathinfo($file_name, PATHINFO_EXTENSION); filename
$new_file_name = $docnr . '.' . $file_extension; 
$upload_path = $upload_dir . $new_file_name;

if (move_uploaded_file($file_tmp_name, $upload_path)) {...........

The form is working, and files are renamed and uploaded correctly but after this, I have download.php file which I use to download files. here is the code of download.php

`<?php
include ('dbconn.php');

$id = $_GET['id'];

$query = "SELECT * FROM documents WHERE id = ?";


$stmt = mysqli_prepare($conn, $query);

mysqli_stmt_bind_param($stmt, "i", $id);


mysqli_stmt_execute($stmt);


$result = mysqli_stmt_get_result($stmt);
$row = mysqli_fetch_assoc($result);

header('Content-Disposition: attachment; filename="'.$row['file_name'].'"');

readfile($row['file_path']);
?>
`

THE problem: when I download for example uploaded Excel file, the file is corrupted and cannot be opened anymore, but if I navigate through the browser to rootdir/uploads and download the file from there via clicking the downloaded file is not corrupted and is working fine. Tell me what I am doing wrong.

I had .htaccess file with deny from all in order to try protecting visiting /uploads via browser, I thought that this could be the problem and I deleted .htaccess but this not solved my issue.

Update: download.php code updated in order to fix security weaknesses. When I download for example .txt file via download.php and open it via notepad++ inside the download file I have this:

<br />
    <b>Warning</b>:  Undefined array key "file_path" in <b>C:\xampp\htdocs\ottodoc\download.php</b> on line <b>22</b><br />
    <br />
    <b>Fatal error</b>:  Uncaught ValueError: Path cannot be empty in C:\xampp\htdocs\ottodoc\download.php:22
    Stack trace:
    #0 C:\xampp\htdocs\ottodoc\download.php(22): readfile('')
    #1 {main}
      thrown in <b>C:\xampp\htdocs\ottodoc\download.php</b> on line <b>22</b><br />

when I open the file directly from the browser via http://localhost/ottodoc/uploads/file.txt is fine. Obviously, something is wrong with my download.php which I cannot catch.

Qbasix
  • 55
  • 7
  • Be warned that your query is widely open for SQL injection. Have a look at prepared statements to avoid getting hacked – Nico Haase May 05 '23 at 13:22
  • Also, please share the difference between the two files. Maybe it would help to set the `Content-Type` header? – Nico Haase May 05 '23 at 13:24
  • Have you looked inside the downloaded file, with a plain text editor, and compared what you see there with an uncorrupted version of the file? – KIKO Software May 05 '23 at 13:25
  • **Warning:** You are wide open to [SQL Injections](https://php.net/manual/en/security.database.sql-injection.php) and should use parameterized **prepared statements** instead of manually building your queries. They are provided by [PDO](https://php.net/manual/pdo.prepared-statements.php) or by [MySQLi](https://php.net/manual/mysqli.quickstart.prepared-statements.php). Never trust any kind of input! Even when your queries are executed only by trusted users, [you are still in risk of corrupting your data](http://bobby-tables.com/). [Escaping is not enough!](https://stackoverflow.com/q/32391315) – Dharman May 05 '23 at 13:34
  • Use error handling and error reporting in your code to see if something goes wrong in your code. – Shadow May 05 '23 at 15:15
  • Please clarify your specific problem or provide additional details to highlight exactly what you need. As it's currently written, it's hard to tell exactly what you're asking. – Community May 07 '23 at 13:40
  • 1
    The comment: _"Don't judge me for security weakness, it's not completed yet."_ is often heard. The problem with it is that it shows a mentality of: _"I'll look into security afterwards, if I think about it and can possibly still fix it."_. Often security is simply ignored. The comments about security here are therefore justified. Security should be be in the forefront of your mind, and incorporated into the code from the start. – KIKO Software May 07 '23 at 13:59
  • You have found a clear error message inside the file. It's just a guess, but I think your database table `documents` might not have a column called: `file_path`? – KIKO Software May 09 '23 at 06:23
  • yeah shame on me, in my database during upload I am storing file_name in column 'file_name' not 'file_path' (typo). I have exchanged the last row of download.php to: ```readfile($row['file_name']);``` now I am getting: ```Warning: readfile(DOC937423.txt): Failed to open stream: No such file or directory in C:\xampp\htdocs\ottodoc\download.php on line 22
    ``` I am missing something but where...     – Qbasix May 09 '23 at 06:52
  • Problem solved: I added this into the end of the code: ```$file_path = 'uploads/' . $row['file_name']; header('Content-Disposition: attachment; filename="'.$row['file_name'].'"'); readfile($file_path);``` Now everything is working fine. Thanks to all – Qbasix May 09 '23 at 07:05
  • You're welcome, nice it all worked out. – KIKO Software May 09 '23 at 07:44

1 Answers1

1

If someone is looking into this topic. Here is the fix: The full path to the file was missing:

// Specify the full path to the file
$file_path = '/path/to/uploads/' . $row['file_name'];

header('Content-Disposition: attachment; filename="'.$row['file_name'].'"');

readfile($file_path);
Qbasix
  • 55
  • 7