Empty .txt is downloaded

[afterfx]

Hello guys, i do not have very good PHP knowledge so go easy on me

 

So I am building this site, where users can create files (for now only text files)

 

Creating file is done and works, now I want to add function to download the file, my code so far

<?php
error_reporting(E_ALL);
$file = $_GET['file'];
if(file_exists('users/'.$_COOKIE['username'].'/files'. '/' .$file)) {
	header('Content-Type: text/plain');
	$content = file_get_contents('users/' . $_COOKIE['username'] . '/files' . '/' .$file);
	//var_dump($content);
	//die();
	header('Content-Disposition: attachment; filename="'.$file.'"');
	readfile($file);
} else {
	echo "File does not exists!";
}

?> 

I have 5 files 1.txt 2.txt 3.txt ... 5.txt.
Download part works alright, but the file downloaded is blank
If i var_dump the file i get the following : string(4) "Five" for the 5.txt and string(3) "Two" for the 2.txt

 

I know that the code is awful to look but I am not that experienced in coding.
Am I missing some headers?

[requinix]

I see you're doing users/$username/files/$file in that one if condition, then again when you get $content. That's the full path to the file, right? You aren't doing that with the readfile...

 

There's a much, much more significant problem with your code: I can use it to download any file I want from your server, and all I have to do is change the ?file= in the URL. Consider what would happen if I did

?file=../../../script.php

That means your code would look for

users/$username/files/../../../script.php -> script.php

?file isn't the only part that's risky, either. Cookies can be edited by the user, so when you put the username in one I can change it to whatever value I want. Besides using that to mess around with the file path, I bet I could use it to any username I wanted.

 

Here's what you need to do:

 

1. Fix the username problem. Store your data in sessions instead of in cookies, because session data cannot be edited by the user. You'll need to fix this on the rest of your site too.

2. Make sure that the ?file is a safe value. For example, you can make sure that it's just a filename by using basename. This is also something that you'll probably have to change in other places.

3. You also need to make sure that the ?file is even present in the first place. If not your code will do some odd things. Use isset for that.

 

Unrelated,

4. You should not need to change error_reporting in your scripts. Set it globally with your PHP configuration (however that works) and you'll never have to think about it again.

 

After all those changes, your script should look more like

<?php

session_start(); // required to use session data

if (isset($_GET['file'])) { // make sure ?file is present
	$file = basename($_GET['file']); // strip everything except the final filename portion
	$path = 'users/' . $_SESSION['username'] . '/files/' . $file; // create a variable for this instead of repeating it everywhere
	if (is_file($path)) { // file_exists checks if the $path exists *even if it's not a file*. use is_file instead
		header('Content-Type: text/plain');
		header('Content-Disposition: attachment; filename="' . $file '"');
		readfile($path);
		exit; // stop executing code. header + readfile + exit often come as a trio with download scripts
	} else {
		echo 'File does not exist!';
	}
} else {
	echo 'File name missing!';
}