Download script only works once

[jezuk]

Hi All,

 

This is my first post here so I hope someone doesn't mind lending me a hand with the following code.  I am very new to PHP so feel free to point out obvious mistakes and bad practice!

 

The idea is to download a file using a URL in the form of http://somewhere.com/download.php?id=somenumber

 

Before a user can download a file they must sign in and a session variable 'permissions' is set, the download script then checks the permissions and if allowed the user will download the file.

 

The logic seems to be working fine, once the user has signed in the file can be downloaded fine first time, however if you try to download the same file again (or any other you have permission to) the script says "You do not have permission...."

 

I think it may have something to do with resetting arrays, and I have tried a few workarounds but no luck yet.  Hopefully someone can spot a mistake!

 

Please let me know if have not provided enough background information to see what the problem is.

 

Any help would be greatly appreciated.

 

Jez

 

<?php require_once('connections/mseis.php'); ?>
<?php
/* This code assumes that users may have mulitple permissions but files will only have one set of permissions
Therefore set the lowest permission level ('public') on files which should be available to all and grant 'op' to higher ranking users */

session_start();

if (!isset($_GET['id']))
{
echo "No ID specified";
die;
}

mysql_select_db($database_mseis, $mseis);

$selectSQL = sprintf("SELECT * FROM downloads WHERE id = %s",
						GetSQLValueString($_GET['id'], "text"));

$result = mysql_query($selectSQL, $mseis) or die(mysql_error());

	if(mysql_num_rows($result) == 1)
	{
		$row = mysql_fetch_assoc($result);
		$permissions = explode(",", $_SESSION['permissions']);

		$granted = false;

		foreach ($permissions as $value)
		{
			if ($row["permissions"] == $value or $row["permissions"] == "public")
			{
				/*Permission granted*/
				$granted = true;
			}

			if ($granted == true)
			{
				/* Start download */
				$file = $row["file"];
				$dir="downloads/";
				$file = $dir . $file;
				header("Content-type: application/force-download");
				header("Content-Transfer-Encoding: Binary");
				header("Content-length: ".filesize($file));
				header("Content-disposition: attachment; filename=\"" . basename($file) . "\"");
				readfile("$file"); 
			}

			else
			{
				/* No permission */
				echo "You do not have permission to download this file";
			}
		}
	}
	else
	{
		/*Not found*/
		echo "File not found";
	}

?>

[mrMarcus]

a couple things i can see:

 

1. $_GET['id'] being an numeric, use %d instead of %s in your query to declare it of numeric value.  strengthens your SQL.  in which case you would then change "text" to "int" in your GetSQLValueString argument.

 

EDIT: is $_GET['id'] numeric?

 

2. where are you defining $_SESSION['permissions']?

 

3. setting $granted = true; and then writing a condition to handle the indefinitely set $granted is redundant:

 

<?php
if ($row["permissions"] == $value or $row["permissions"] == "public")
            {
               /*Permission granted*/

               /* Start download */
               $file = $row["file"];
               $dir="downloads/";
               $file = $dir . $file;
               header("Content-type: application/force-download");
               header("Content-Transfer-Encoding: Binary");
               header("Content-length: ".filesize($file));
               header("Content-disposition: attachment; filename=\"" . basename($file) . "\"");
               readfile("$file"); 
            }
            
            else
            {
               /* No permission */
               echo "You do not have permission to download this file";
            }
?>

 

simplified by just taking that condition out.

 

EDIT: can we see how you set the permissions, please?

[Adam]

I can't see any reason why this would happen *within this code*. Perhaps when you navigate away from the page other code is being run that is affecting the permissions session variable? Have you tried dumping the session data to see what's there when the download fails?

 

For example:

 

            else
            {
               /* No permission */
               echo "You do not have permission to download this file";
               print_r($_SESSION);
            }

 

Edit: Also to build on mrMarcus' suggestions, you could remove the loop altogether:

 

<?php require_once('connections/mseis.php'); ?>
<?php
/* This code assumes that users may have mulitple permissions but files will only have one set of permissions
Therefore set the lowest permission level ('public') on files which should be available to all and grant 'op' to higher ranking users */

session_start();

if (!isset($_GET['id']))
{
echo "No ID specified";
die;
}

mysql_select_db($database_mseis, $mseis);

$selectSQL = sprintf("SELECT * FROM downloads WHERE id = %s",
                     GetSQLValueString($_GET['id'], "text"));

$result = mysql_query($selectSQL, $mseis) or die(mysql_error());
      
      if(mysql_num_rows($result) == 1)
      {
         $row = mysql_fetch_assoc($result);

         if (strpos($_SESSION["permissions"], $row["permissions"]) !== false || $row["permissions"] == "public")
         {
             /* Start download */
             $file = $row["file"];
             $dir="downloads/";
             $file = $dir . $file;
             header("Content-type: application/force-download");
             header("Content-Transfer-Encoding: Binary");
             header("Content-length: ".filesize($file));
             header("Content-disposition: attachment; filename=\"" . basename($file) . "\"");
             readfile("$file"); 
         }
         else
         {
             /* No permission */
             echo "You do not have permission to download this file";
         }
      }
      else
      {
         /*Not found*/
         echo "File not found";
      }

?>

[jezuk]

Thanks for the speedy replies, unfortunately its not solved yet....

 

1. $_GET['id'] being an numeric, use %d instead of %s in your query to declare it of numeric value.  strengthens your SQL.  in which case you would then change "text" to "int" in your GetSQLValueString argument.

Thanks for pointing that out I've made that change, yes 'id' is an integer.

 

2. where are you defining $_SESSION['permissions']?

Extract from login.php:

<?php
$row = mysql_fetch_assoc($result);
		session_register("permissions");
		session_register ("loggedin");
		$_SESSION['loggedin'] = true;
		$_SESSION['permissions'] = $row['permissions'];
?>

The permissions are comma seperated eg. op,upload,admin in a MySQL table

 

3. setting $granted = true; and then writing a condition to handle the indefinitely set $granted is redundant:

Noted and deleted thanks!

 

Have you tried dumping the session data to see what's there when the download fails?

Is there any kind of debugger? Or do you mean just echo to the screen?

 

Thanks again

 

[jezuk]

Thanks for the update MrAdam, I didn't see your edit before I posted!

 

I will try that and see *fingers crossed*

[Adam]

Yeah sorry, was pretty late there. The second code I provided won't fix your problem though, was merely a suggestion...

 

I am very new to PHP so feel free to point out obvious mistakes and bad practice!

 

However if you try dumping out the data, it'll hopefully point out where the problem is.

[mrMarcus]

can you post: connections/mseis.php as well as login.php.  i see you're using session_register() mixed with $_SESSION, so i'm think there might be a few other issues at hand.

[Adam]

Combining $_SESSION with session_register() wouldn't cause any troubles, it's just session_register() is deprecated as of PHP5.3.

 

Edit: Combining $_SESSION with session_register() wouldn't cause any troubles in the way he's using it **

[jezuk]

I have compared the session variables with a working and broken download, it seems something is clearing out the permissions array the second time round.

 

Working:

Array ( [permissions] => op,upload [loggedin] => 1 )

 

Broken:

Array ( [permissions] => Array ( [0] => Array ) [loggedin] => 1 )

 

I will try and work out how this is happening, maybe mrMarcus is on to something about using session_register().

 

Here are the other php files being used:

 

connections/mseis.php:

<?php
# FileName="Connection_php_mysql.htm"
# Type="MYSQL"
# HTTP="true"
$hostname_mseis = "localhost";
$database_mseis = "mseis";
$username_mseis = "webserver";
$password_mseis = "anon";
$mseis = mysql_pconnect($hostname_mseis, $username_mseis, $password_mseis) or trigger_error(mysql_error(),E_USER_ERROR); 


if (!function_exists("GetSQLValueString")) {
function GetSQLValueString($theValue, $theType, $theDefinedValue = "", $theNotDefinedValue = "") 
{
  $theValue = get_magic_quotes_gpc() ? stripslashes($theValue) : $theValue;

  $theValue = function_exists("mysql_real_escape_string") ? mysql_real_escape_string($theValue) : mysql_escape_string($theValue);

  switch ($theType) {
    case "text":
      $theValue = ($theValue != "") ? "'" . $theValue . "'" : "NULL";
      break;    
    case "long":
    case "int":
      $theValue = ($theValue != "") ? intval($theValue) : "NULL";
      break;
    case "double":
      $theValue = ($theValue != "") ? "'" . doubleval($theValue) . "'" : "NULL";
      break;
    case "date":
      $theValue = ($theValue != "") ? "'" . $theValue . "'" : "NULL";
      break;
    case "defined":
      $theValue = ($theValue != "") ? $theDefinedValue : $theNotDefinedValue;
      break;
  }
  return $theValue;
}
}
?>

 

login.php:

<?php require_once('connections/mseis.php'); ?>
<?php

function isLocalURL($url) { /* XSS hijack protection */

$urlParts = parse_url($url); 
if(isset($urlParts['scheme']) && $urlParts['scheme'] != 'http') 
return false; 
if(isset($urlParts['host']) && $urlParts['host'] != 'localhost') /* CHANGE THIS WHEN GOING LIVE */
return false; 

return true;
}

session_start();

    if (!isset($_SESSION['loggedin']) or $_SESSION['loggedin'] == false)
{
	if (isset($_POST['action']) and $_POST['action'] = "login")
	{
	mysql_select_db($database_mseis, $mseis);
	$selectSQL = sprintf("SELECT * FROM webusers WHERE username = %s AND password = %s",
						GetSQLValueString($_POST['username'], "text"),
						GetSQLValueString(md5($_POST['password']), "text"));


	$result = mysql_query($selectSQL, $mseis) or die(mysql_error());

	if(mysql_num_rows($result) == 1)
	{
		/* Successful login */

		$row = mysql_fetch_assoc($result);
		session_register("permissions");
		session_register ("loggedin");
		$_SESSION['loggedin'] = true;
		$_SESSION['permissions'] = $row['permissions'];

		if(isset($_GET['redir']) and isLocalURL($_GET['redir']))
		{
			 header("Location: " . $_GET['redir']);
		}
		else
		{
			header("Location: index.php");
		}

	}
else
	{
		/* Failed login */
		$failedlogin = true;
	}		

	}	
}
else
{
	/* Already logged in */
	if(isset($_GET['redir']) and isLocalURL($_GET['redir']))
	{
		 header("Location: " . $_GET['redir']);
	}
	else
	{
		header("Location: index.php");
	}
}
?>

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />

<title>Mseis</title>

<link href="layout.css" rel="stylesheet" type="text/css" />
<!--[if IE]>
<style type="text/css"> 
/* place css fixes for all versions of IE in this conditional comment */
.twoColHybLtHdr #sidebar1 { padding-top: 30px; }
.twoColHybLtHdr #mainContent { zoom: 1; padding-top: 15px; }
/* the above proprietary zoom property gives IE the hasLayout it may need to avoid several bugs */
</style>
<![endif]--></head>

<body class="twoColHybLtHdr">

<div id="container">
  <div id="header">
<?php include("includes/header.php"); ?>
  <!-- end #header --></div>
  <div id="sidebar1" align="center">
<?php include("includes/menu.php"); ?>
    <!-- end #sidebar1 -->
  </div>
  <div id="mainContent">
<h1>Resources Login</h1>
    <form name="login" method="post" action="<?php echo htmlentities($_SERVER['PHP_SELF']);
if (isset($_GET['redir']))
{
echo "?redir=" . urlencode($_GET['redir']);
}
?>">
  <fieldset>
  <legend>Please enter your login details</legend>
   <ol>
  <?php
  if ($failedlogin == true)
  {
  echo "<li><span class=\"failed\">Login failed</span></li>";
  }
  ?>

  <li>
  <label for="UserID">Username</label>
  <input type="text" name="username" id="username">
  </li>
  <li>
  <label for="Password">Password</label>
  <input type="password" name="password" id="password">
  </li>
  </ol>
  </fieldset>
  <fieldset class="submit">
  <input type="submit" name="Submit" id="Submit" value="Submit" class="btn" onmouseover="this.className='btn btnhov'" onmouseout="this.className='btn'">
</fieldset>
<input type="hidden" name="action" value="login">
</form>

<script type="text/javascript" language="JavaScript">
document.forms['login'].elements['username'].focus();
</script>

<!-- end #mainContent --></div>
<!-- This clearing element should immediately follow the #mainContent div in order to force the #container div to contain all child floats -->
<br class="clearfloat" />
<div id="footer">
<?php include("includes/footer.php"); ?>
  <!-- end #footer --></div>
<!-- end #container --></div>
</body>
</html>

[Adam]

It's worth giving it a shot! Try removing the session_register() calls.. May as well remove them anyway to be honest!

 

What would make me think against that though is that you're not going back to login.php in between downloads -- are you? So for it to work the first time must mean it's registered the session correctly.

[jezuk]

Thank you so much guys, it seems to be working fine now 

 

I think the problem was hitting the back button takes you to login.php?redir=resources.php, so the login script was running again first and clearing the permissions variable (although I thought I'd avoided this).  Removing session_register seems to make everything work (I didn't realise it was depreciated).

 

I'm very grateful for your time and effort, and I've ended up with tidier looking code thanks to both your suggestions.

 

All the best

 

Jez

[Adam]

Glad it's working, mrMarcus was right then. I'll not give session_register() advice anymore

 

It's actually deprecated as of PHP5.3 - the last 'major version' - I dare say not yours.