Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm using this form to submit articles into the database:

<form enctype="multipart/form-data" method="post" action="add.php">

  <input name="title" type="text">
  <input name="image" type="file">
  <textarea name="text"></textarea>
  <input type="submit" name="go" value="Spremi">

</form>

include ("database.php"); 
$upload_path = "files/images/";
$prefix= date("Hi-mdY")."-";

$file_name = $HTTP_POST_FILES['image']['name'];
$file_name = str_replace(' ', '-', $file_name);
$file_name = str_replace('_', '-', $file_name);
$file_name = strtolower($file_name);

$upload_path = $upload_path . basename($prefix.$file_name);
move_uploaded_file($_FILES['image']['tmp_name'], $upload_path);

$final_file_name = ("/files/images/".$prefix.$file_name);
$date=date("Y.m.d");

$sql="INSERT INTO articles (title, image_link,  text, date) VALUES ('$_POST[title]', '$final_file_name', '$_POST[text]', '$date')"
if (mysql_query($sql)){
    echo "done";
} else {
    echo "error<br>" . mysql_error();
}

And here is the problem:

One of my friends told me that the PHP code in add.php is incorrect.

This is working for me, but can someone correct the code please.

EDIT:

Thanks guys, I've corrected the code :

<form enctype="multipart/form-data" method="post" action="">

  <input name="title" type="text">
  <input name="image" type="file">
  <textarea name="text"></textarea>
  <input type="submit" name="go" value="Submit">

</form>

<?php 
  if (isset($_POST['go'])){
    include ("database.php"); 
    $upload_path = "files/images/";
    $prefix= date("Hi-mdY")."-";

    $file_name = $_FILES['image']['name'];
    $file_name = str_replace(' ', '-', $file_name);
    $file_name = str_replace('_', '-', $file_name);
    $file_name = strtolower($file_name);

    $upload_path = $upload_path . basename($prefix.$file_name);
    move_uploaded_file($_FILES['image']['tmp_name'], $upload_path);

    $final_file_name = ("/files/images/".$prefix.$file_name);

    $title=$_POST['title'];
    $text=$_POST['text'];
    $date=date("Y.m.d");

    $title = mysql_real_escape_string($title);
    $final_file_name = mysql_real_escape_string($final_file_name);
    $text = mysql_real_escape_string($text);

    $sql="INSERT INTO articles (title, image_link,  text, date) VALUES ('$title', '$final_file_name', '$text', '$date')";
    if (mysql_query($sql)){
      echo "done";
    } else {
      echo "error<br>" . mysql_error();
    }
  }
?>

Now it's good?

share|improve this question

migrated from stackoverflow.com Aug 29 '12 at 3:11

This question came from our site for professional and enthusiast programmers.

    
I would be able to upload a script and take over your server :) –  RobertPitt Dec 27 '10 at 15:57
    
Use MySQLi as MySQL is deprecated php.net/manual/en/book.mysqli.php using prepared statements will also help secure your form php.net/manual/en/mysqli.quickstart.prepared-statements.php these type of forms are available for download if you google upload forms. You should also create a whitelist for file types allowed –  CodeX Jul 6 '14 at 12:06

1 Answer 1

There are quite few things to note in your code.

First of all, you are using deprecated $HTTP_POST_FILES where as you should use $_FILES

You are not using mysql_real_escape_string function in your query variables and therefore are vulnerable to sql injection which is caused through poor sql queries.

You are not checking whether or not the submit button was clicked, you should wrap your entire code in between:

if (isset($_POST['go'])){
  // your code
}
share|improve this answer
    
just that is incorrect? –  user554943 Dec 27 '10 at 12:43
2  
@user554943: It is correct otherwise and will work but you should also consider the points i have told above. –  Sarfraz Dec 27 '10 at 12:50

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.