To register for an Internet.com membership to receive newsletters and white papers, use the Register button ABOVE.
To participate in the message forums BELOW, click here
PHPBuilder.com  
 

 

Go Back   PHPBuilder.com > PHP Help > Code Critique

Code Critique Having someone critique your code is always a great way to hone the skills. Stop in and post your code to see what your peers may have done differently.

Reply
 
Thread Tools Rate Thread Display Modes
Old 12-23-2003, 08:06 PM   #1
Skull
Senior Member
 
Skull's Avatar
 
Join Date: Jul 2003
Location: Scotland
Posts: 149
Downloads

well here's a script I created for the download section on my site, it basically checks if they're logged in, if they are logged in it'll carry on, if not they'll go to the log in page. Then once thats done it'll check if the file exists, if it doesn't it'll return an error, if not it'll carry on. Then it'll check if you have access to that area, if you don't it'll return an error, if not it'll carry on.

PHP Code:
if($_COOKIE['username'] == ""){
header("Location: login.php");
}else{
$file = $_REQUEST['type']."/".$_REQUEST['area']."/".$_REQUEST['filename'];
if(
file_exists($file)){
$shortname=basename($file);
$size=filesize($file);
//set header
header("Content-Type: application/save");  
header("Content-Length: $size");
header("Content-Disposition: attachment; filename=$shortname");
header("Content-Transfer-Encoding: binary");
//start transfer
    
$dbh=mysql_connect ("localhost", "xxxxxx", "xxxxxx") or die ('I cannot connect to the database.');
    
mysql_select_db ("xxxxxxx");
$SQL = "SELECT * FROM download_users where username='".$_COOKIE['username']."'";
        
$result = @mysql_query($SQL) or die(mysql_error());
    while(
$row = @mysql_fetch_array($result)) {
    
$status = $row['status'];
    
$email = $row['email'];
    }
if((
$status == "Public")&&($_REQUEST['area'] == "private")){
echo
"<strong>Error:</strong>
You are not allowed to access this file.<br>
An E-mail has been sent to the admin informing him of this.
If you try to get into files you aren't allowed to access again your account will be terminated"
;
$message = "".$_COOKIE['username']." has attempted to download files from the private area.
$file
Its up to you what you want to do now"
;

$sender = "webmaster@ewewrestling.com";


mail("$sender", "EWE Downloads Error", $message,
     
"From: $email\r\n"
    
."Reply-To: $sender\r\n"
    
."X-Mailer: PHP/" . phpversion());
}else{
$handler=fopen("$file","r");
fpassthru($handler);


    
$date = date("l, d F Y h:i a");
        
$SQL = "INSERT INTO downloads
  (filename,size,username,date) VALUES('$shortname','$size','$username','$date')"
;
          
$result = @mysql_query($SQL) or die(mysql_error());
exit;
}
}else{
echo
"<strong>Error:</strong>
File Does Not Exist<br>
An e-mail has been sent to the admin informing him of this."
;
$message = "".$_COOKIE['username']." has attempted to download files from the download section that don't exist.
$file
Its up to you what you want to do now"
;

$sender = "webmaster@ewewrestling.com";


mail("$sender", "EWE Downloads Error", $message,
     
"From: $email\r\n"
    
."Reply-To: $sender\r\n"
    
."X-Mailer: PHP/" . phpversion());

}
}
any improvments anyone can see that can be done to it, or anything that can make the code itself shorter?
__________________
Blaine Young
http://www.linksuk.com

Last edited by Skull; 12-23-2003 at 08:08 PM.
Skull is offline   Reply With Quote
Old 12-24-2003, 01:12 AM   #2
LordShryku
kung foo code monkey
 
LordShryku's Avatar
 
Join Date: Aug 2002
Location: Occupational Hypnotherapy
Posts: 7,473
Well, I'll give it a good looking over tomorrow, the first two things that stick out to me are:

1) The indention is whacked. Could use some fixing.
2) On your insert statement, you're putting in $username, which up to this point has been reffered to as $_COOKIE['username']. I don't see it ever being defined to $username. Of course, maybe I'm just missing it.

LordShryku is offline   Reply With Quote
Old 12-24-2003, 11:42 AM   #3
Skull
Senior Member
 
Skull's Avatar
 
Join Date: Jul 2003
Location: Scotland
Posts: 149
PHP Code:
if($_COOKIE['username'] == "")
    {
        
header("Location: login.php");
    }else
        {
            
$file = $_REQUEST['type']."/".$_REQUEST['area']."/".$_REQUEST['filename'];
        
            if(
file_exists($file))
                {
                    
$shortname=basename($file);  
                    
$size=filesize($file);  
                    
                    
//set header
                    
header("Content-Type: application/save");   
                    
header("Content-Length: $size");  
                    
header("Content-Disposition: attachment; filename=$shortname");  
                    
header("Content-Transfer-Encoding: binary");  
                    
                    
//start transfer
                    
$dbh=mysql_connect ("localhost", "xxxxxx", "xxxxxx") or die ('I cannot connect to the database.');
                    
mysql_select_db ("xxxxxxx");
                    
                    
$SQL = "SELECT * FROM download_users where username='".$_COOKIE['username']."'";
                    
$result = @mysql_query($SQL) or die(mysql_error());
                    while(
$row = @mysql_fetch_array($result))
                        {  
                            
$status = $row['status'];
                            
$email = $row['email'];
                        }
                        
                    if((
$status == "Public")&&($_REQUEST['area'] == "private"))
                        {
                            echo
"<strong>Error:</strong>
                            You are not allowed to access this file.<br>
                            An E-mail has been sent to the admin informing him of this.
                            If you try to get into files you aren't allowed to access again your account will be terminated"
;
                            
$message = "".$_COOKIE['username']." has attempted to download files from the private area.
                            $file  
                            Its up to you what you want to do now"
;
                    
                            
$sender = "webmaster@ewewrestling.com";
                    
                    
                            
mail("$sender", "EWE Downloads Error", $message,
                            
"From: $email\r\n"
                            
."Reply-To: $sender\r\n"
                            
."X-Mailer: PHP/" . phpversion());
                            }
                        else
                            {
                                
$handler=fopen("$file","r");  
                                
fpassthru($handler);  
                            
                            
                                
$date = date("l, d F Y h:i a");
                                
$SQL = "INSERT INTO downloads
                                (filename,size,username,date) VALUES('$shortname','$size',"
.$_COOKIE['username'].",'$date')";
                                
$result = @mysql_query($SQL) or die(mysql_error());
                                exit;
                            }
                }else
                    {
                        echo
"<strong>Error:</strong>
                        File Does Not Exist<br>
                        An e-mail has been sent to the admin informing him of this."
;
                        
$message = "".$_COOKIE['username']." has attempted to download files from the download section that don't exist.
                        $file
                        Its up to you what you want to do now"
;
                    
                        
$sender = "webmaster@ewewrestling.com";
                    
                    
                        
mail("$sender", "EWE Downloads Error", $message,
                        
"From: $email\r\n"
                        
."Reply-To: $sender\r\n"
                        
."X-Mailer: PHP/" . phpversion());
                    
                    }  
        }
i'm not the best at indenting but I fixed it up a little
__________________
Blaine Young
http://www.linksuk.com
Skull is offline   Reply With Quote
Old 12-24-2003, 04:41 PM   #4
Jedi Legend
Member
 
Join Date: Aug 2002
Posts: 73
It's basically pointless to have login with that code. I could download from your site without registering. All I would need is to fake a cookie named username. I've never faked a cookie before, but back when I was writing a security system using cookies back in the day, I was told it's pretty easy.

In fact, I could even put someone else's username in there.
__________________
Live, learn, and die by the code!
Jedi Legend is offline   Reply With Quote
Old 12-24-2003, 05:17 PM   #5
Skull
Senior Member
 
Skull's Avatar
 
Join Date: Jul 2003
Location: Scotland
Posts: 149
so how can I get around that then? Sessions?
__________________
Blaine Young
http://www.linksuk.com
Skull is offline   Reply With Quote
Old 12-25-2003, 03:26 AM   #6
Tristan Wells
Member
 
Join Date: Oct 2003
Location: Louisiana
Posts: 38
Quote:
Originally posted by Skull
so how can I get around that then? Sessions?
Still just as risky. Sessions which aren't secured can be hacked mearly by adding the session id onto the url. And secured sessions wont work in AOL.
__________________
The girls at school call me nerdalicious.
Tristan Wells is offline   Reply With Quote
Old 12-25-2003, 11:52 AM   #7
pjleonhardt
Senior Member
 
Join Date: Sep 2003
Posts: 279
for added security you might want to hold an md5() password in a cookie, and check that against the database as well.
pjleonhardt is offline   Reply With Quote
Reply

Bookmarks


Currently Active Users Viewing This Thread: 1 (0 members and 1 guests)
 
Thread Tools
Display Modes Rate This Thread
Rate This Thread:

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is Off
HTML code is Off
Forum Jump


All times are GMT -4. The time now is 08:38 PM.






Acceptable Use Policy

internet.comMediabistrojusttechjobs.comGraphics.com

WebMediaBrands Corporate Info


Advertise | Newsletters | Feedback | Submit News

Legal Notices | Licensing | Permissions | Privacy Policy


Powered by vBulletin® Version 3.7.2
Copyright ©2000 - 2009, Jelsoft Enterprises Ltd.