0

I am trying to allow the user to upload as many images as they want, place them all in a new directory based on the title of the form, and then write that folder name to the database and I will call it later to do an image dump.

So I have been working on this for multiple days now, and have gotten this far

if(isset($_POST['Submit']))
{  $ttitle = $_POST['ttitle'];

    $file_dir = './../img/treatments/';
    foreach( $_FILES as $file_name => $file_array ){
    $current_image=$_FILES['image']['name'][0];
    $extension = substr(strrchr($current_image, '.'), 1);
if (($extension!= "png") && ($extension != "jpg")) 
{
die('Unknown extension');
}
$time = date("fYhis");
$new_image = $time . "." . $extension;
$new_dir = mkdir("./../img/treatments/" . $ttitle, 0700);
$destination= $new_dir && $new_image;
$action = copy($_FILES['image']['tmp_name'], $destination);
    }

  $last_name = $_POST['last_name'];
  $username = $_POST['username'];
  $pass = $_POST['pass'];
  $text = $_POST['text'];
  $bio = $_POST['bio'];
  $tsub = $_POST['tsub'];
  $image = $ttitle; 
if (!$action) 
{
die('File copy failed');
}else{
echo "File copy successful";
}

and then the html form

<form method='post' enctype='multipart/form-data' action='#'>
<table width="100%" border="0" cellspacing="0" cellpadding="0"><tr><td width="40%" class="right">

Title: </td><td width="60%" class="left"><input type="text" autofocus autofocus="autofocus" required required="required" name="ttitle" maxlength="255"
  />
*</td></tr><tr><td class="right">
Subtitle:</td><td class="left"> <input type="text" required required="required" name="tsub" maxlength="255"
  />
*</td></tr><tr><td class="right">
Username: </td><td class="left"><input type="text" required required="required" name="username" maxlength="255"
  />
*</td></tr><tr><td class="right">
Password: </td><td class="left"><input type="password" required required="required" name="pass" maxlength="255"
  />
*</td></tr>
<tr><td class="right">
Confirm Password: </td><td class="left"><input type="password" required required="required" name="pass2" maxlength="255"
  />
*</td></tr><tr><td class="right">Proposed Director
</td><td class="left"><input type="text" name="dir" maxlength="255"
  />
  *</td></tr><tr><td class="right">Proposed Additional
</td><td class="left"><input type="text" name="add" maxlength="255"
  /></td></tr><tr><td class="right" valign="top">
Text: </td><td class="left" valign="top"><textarea required required="required" name="text"></textarea>
*</td></tr><tr><td class="right">
Bobblehead: </td><td class="left">
<input type="file" required required="required" name="image[]" multiple="">
*</td></tr> </table>

And now everything works with the exception of actually copying and writing the files to the new directory, I am sitting here fiddling with it but I feel like I am real close and some one probably has the key, I just can't figure it out off the top of my head. Any help would be great, thanks.

Improve this question
5
  • Format your html code in a proper. Currently html code as repeated attributes. Commented Nov 11, 2011 at 22:23
  • Well I know it has something to do with me '$destination' I am not sure how to take the $new_dir and combine it with $new_img so it will be url/folder/image.jpg Commented Nov 11, 2011 at 22:23
  • I have been doing required and required="required" both because i dont remember which one, but one of those is needed to work in firefox and the other for ie Commented Nov 11, 2011 at 22:25
  • why copy and not move_uploaded_file? Commented Nov 11, 2011 at 22:27
  • You should use move_uploaded_file(), not copy(). Commented Nov 11, 2011 at 22:28

2 Answers 2

Reset to default
3

Let's see how many ways I can list on how insanely horrible your code is:

  1. You're using a raw POST to create a directory on your server. You do not filter this post field for pathing characters, so in effect you're letting the remote user create a directory of THEIR chosing ANYWHERE on your server
  2. You're analyzing filename extensions to determine file types. Ignoring for a moment that the name parameter in the $_FILES array is the name of the file as provided for the user, you're using substring operations to get the file's extension, ignoring the pathinfo() function which does that for you. Getting back to the filename, nothing stops a malicious user from renaming "nastyvirus.exe" to "kittens.jpg" and uploading it to your server.
  3. You do not in any way/shape/form check for upload success - you simply assume everything worked perfectly and start chugging away processing the uploaded file. There's precisely ONE way for an upload to succeed, and a few trillion ways for it to fail...perhaps you should have some error checking - that's why the error paramter in the _FILES array exists.
  4. I'll give you this much: you're not using the original filename to store the file in, but you do blindly generate a new filename and then not check if you'll be overwriting a previously uploaded file. It's entirely possible that two different users will select the same title and upload two different files at the same time - your script will then overwrite one of those files with the other one.

  5. You're using copy() to move the file after upload. This is a bad idea. There is move_uploaded_file() for that purpose. Besides the obvious MOVE operation, it also has some extra security checks to ensure the file's not been tampered with on the server in the time between the upload completing and your script handling the file. copy will literally copy the file, causing there to be (for a short period) double the data on your server, which is a waste of space. And on large files, the copy operation will take quite a while. Moves within a filesystem are by comparison almost instantaneous.

  6. You say you're allowing multiple files to be uploade, but your script only ever checks for the first one [0]. If you want to handle multiple files, you'll need to do this code in a loop, something like foreach(array_keys($_FILES['image']['name']) as $i) { ... } then use [$i] instead of [0].

  7. Beyond that, you've got required and required="required" in your HTML, which is simply a waste of space. Use one or the other, but not both.
  8. You've got $new_dir && $new_file. && is not a concatenation operator in PHP, it's a boolean AND operator. You're not assigning a string to $destination, you're storing the result of that boolean and, e.g. true or false.
Improve this answer
Sign up to request clarification or add additional context in comments.

3 Comments

wow thanks for all the advice, I will make sure to take into consideration
and to be truthfully honest, I am not the original programmer on this, I just took this over a day ago because literally nothing worked, so I am trying to fix everything, theres $sql errors all over the place
Ok I got it working thanks, now I am on to not making this code look like a 4th grader did it
1

You have to iterate and copy / move_uploaded_file for each of the uploaded files, this is how you access the file $_FILES['image']['tmp_name'][0]. move_uploaded_file is best suited for this.

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.