Download script that downloads the page itself when no ID is specified, what's wrong?

Question

I coded a script that when users want to download a file, it shows an advert first and then start the download passing the ID of the file via $_GET. Problem is that if I reach the page with no ID specified (download_file.php instead of download_file.php?id=1, for instance), the page starts the download of the page itself.

<?php
require("/membri/lostlife/mysql.php");
// Variables:
$id = $_GET["id"];
$result = mysql_query("SELECT * FROM Setting WHERE ID = $id");
$row = mysql_fetch_array($result);
$downloads = $row["Downloads"] + 1;
//
switch ($_GET["action"])
{
    case "download":
    // Download the file:
    header("Content-Type: application/zip");
    header("Content-Disposition: attachment; filename=\"$row[Filename]\"");
    readfile("/membri/lostlife/setting/$row[Filename]");
    // Update the database:
    mysql_query("UPDATE Setting SET Downloads = $downloads WHERE ID = $id");
    break;
    default:
    echo "";
    header("Refresh: 5; url=?id=$id&action=download");
}
?>

That's my code. What's wrong with it?

Post the complete code. There isn't enough here to really give an intelligent answer. For example, where does the $row variable come from? (Also, unless Filename is a constant, you'll be having issues). — dmcnelis, Mar 19 '11 at 15:19
This is why you should always develp with error reporting on and at max level, and also why you must always check that a value is defined when using it. Had you done these things, you'd have known when `$id` was defined and when it wasn't — Matteo Riva, Mar 19 '11 at 16:24
**Danger**: You are using [an **obsolete** database API](http://stackoverflow.com/q/12859942/19068) and should use a [modern replacement](http://php.net/manual/en/mysqlinfo.api.choosing.php). You are also **vulnerable to [SQL injection attacks](http://bobby-tables.com/)** that a modern API would make it easier to [defend](http://stackoverflow.com/questions/60174/best-way-to-prevent-sql-injection-in-php) yourself from. — Quentin, Jun 18 '14 at 10:33

score 0 · Answer 1 · edited Jun 18 '14 at 10:26

Also you got in your default from your switch a refresh header.. so when the action is NOT 'download' it is going to refresh to action=download.

ill would do it this way:

require("/membri/lostlife/mysql.php");

    $id = $_GET["id"];
    $action = $_GET["action"];

    // if its not empty and it is numeric(check if its a integer can be done in different ways)
    if(!empty($id) && is_numeric($id)) 
    {
        $query = mysql_query("SELECT Downloads, Filename FROM Setting WHERE ID = $id");
        $row   = mysql_fetch_assoc($query);
        $download = $row['Downloads'];
        $filename = $row[Filename];

        if($action == "downoad") {
            header("Content-Type: application/zip");
            header("Content-Disposition: attachment; filename=\"". $filename ."\"");
            readfile("/membri/lostlife/setting/". $filename);
        }
    }
    else
    {
        die("No ID found");
    };

You also updating something? what your doing know is update the download what you got from your select statement? so you don't need to update it? you do you want to count what you download?

Download script that downloads the page itself when no ID is specified, what's wrong?

1 Answers1