1

I am using the following script. it basically checks if the name of the file being uploaded already exists, if it does it should rename it to something else and upload it. So far it doesn't work. either it renames all files or when i try to open the file via webpage it says corrupt file.

Code:

$sql="SELECT filename FROM doc_u WHERE person_id= '$pid'";  

    $result=mysql_query($sql);

    $query = mysql_query($sql) or die ("Error: ".mysql_error());

 if ($result == "")
 {
 echo "";
  }
   echo "";


    $rows = mysql_num_rows($result);

    if($rows == 0)
    {
   print("");

    }
    elseif($rows > 0)
    {
   while($row = mysql_fetch_array($query))
    {

   $existing = $row['filename'];

   print("");
   }

   }

     if ( $filename === $existing ) {
$filename = $uniqueidgenerator.strrchr($_FILES['filename']['name'], ".");

    } else {
     $filename = $_FILES['filename']['name'];;
    }

//After checking it will move the files

      if(move_uploaded_file($_FILES['filename']['tmp_name'],$upload_path . $filename))
     echo ''; 
  else
     echo ''; 
KPO
  • 890
  • 2
  • 20
  • 40

2 Answers2

2

Given you're using a database already, let me say it asplainly as possible: NEVER USE A USER-PROVIDED FILENAME. Instead, store each uploaded file by its corresponding DB record's primary key ID number (you are using auto-incrementing integers for ids, right?). Store the file's name in the database, and now you can have as many "text.txt" files as you want, because each actual file will be named "1", "53", and "207", etc...

Marc B
  • 356,200
  • 43
  • 426
  • 500
  • 1
    this is so important. if you let someone upload somejpeg.php to your server, it could validate as a jpeg, yet still have malicious code inside of itself. someone could basically take over your server using this method. – dqhendricks Aug 11 '11 at 20:48
  • ok so you are saying take the filename and store in the db and then rename the file that is being uploaded. And when you serve it to the user it will have the unique filename that we gave to it. But isn't that bad that users can't get the file by the name they uploaded... – KPO Aug 11 '11 at 20:54
  • 1
    sure, you just serve up the file via PHP and output `header('Content-disposition: attachment; filename=ORIGINAL_FILENAME_HERE');` and the user will never see that it's really called '207' on your server. – Marc B Aug 11 '11 at 21:01
  • Any how how can i fix it so that it still checks the renamed file because what if it generates an existing id.. – KPO Aug 11 '11 at 21:03
  • 1
    @KPO the alternative is they hack your system. you could set up your own file server however to make sure that they cannot execute code on your server. basically you would have a download.php?id=n script that would let them access whatever file they uploaded, and set it back to its original name. the script would use headers to set the downloaded files name, then echo the contents of the file. – dqhendricks Aug 11 '11 at 21:03
  • 1
    an auto_increment primary key field in a database should never generate a duplicate id. if it does, then you've got major problems – Marc B Aug 11 '11 at 21:04
  • 1
    @KPO, your database would have an auto-incrementing id field that you could use for the filename. this would make sure that you would never have duplicate filenames. – dqhendricks Aug 11 '11 at 21:04
  • @marc the problem with id is that its one number i need something that is at least 5 numbers and still be unique. – KPO Aug 11 '11 at 21:08
  • 1
    @KPO: why? the users will never see that number. whether "resume.doc" is named '3' or "nyancat.mp3" is called "52342352342" won't matter in the least to them. they'll only ever see 'resume.doc' and 'nyancat.mp3' – Marc B Aug 11 '11 at 21:09
  • ohhh ok. got your point. But i actually wanted to know for other things like when users to do posts the posts have ids... so to prevent duplicates for those purposes.. – KPO Aug 11 '11 at 21:12
  • how can i setup the id as the name of the file in PHP. I am not sure how to set that up... – KPO Aug 11 '11 at 21:18
  • @marc this doesn't make sense though, if the renamed file is the one being stored then how can i use the orignal document name to serve the renamed file. I did it but when i open its corrupted.. – KPO Aug 11 '11 at 21:37
1

There isn't much code to go on here. I suspect that $filename isn't initialized properly. So in your if ( $filename === $existing ) that is really saying if(undefined === undefined) which would always be true.

I don't see why the files would be corrupted. That could be a different issue entirely.

Also, watch out for your sql statement there. That is an SQL injection waiting to happen. It would be a lot better if you used prepared statements or at the very least use mysql_escape_string. This has been deprecated of course in favor of prepared statements.

A better workflow would be to insert a row in the db first with the file's meta data then rename the file using the primary key you got back. It is guaranteed to be unique always.

Jason Clawson
  • 1,027
  • 7
  • 9