Any problems with this code?

[phreak3r]

Aside from the lack of security against SQL injection attacks, is there any other issue with this code? I cannot seem to get files to upload to the server anymore. I am being prompted with some var_dumps and the message I echoed out in the else part of the if-else statement at the bottom of this script. I have tried using isset, empty, and is_uploaded_file functions for the if-else statement at the bottom, nothing seems to work. If you remove the if-else statement, the code works, but I put the if-else statement there to prevent empty forms and missing fields from being submitted.

 

Here's a hastebin link to the script: https://hastebin.com/denorunera.xml

Edited February 17, 2018 by phreak3r

[ginerjm]

If you have a specific problem with your code (as you kind of hinted at) please post THAT part of the code HERE.

 

I, for one, don't want to click on some link that may be holding an entire script of yours or worse, something else even.

 

Happy to help you then.

[requinix]

- Technically you shouldn't try to use stuff in $_FILES until you've done the isset so those assignments would trigger notices

- You strtolower the extension so there's no point looking for MP4 or MOV.

- Need to fix that one error message that currently sticks the error code and size in

- The two else ifs will work but don't make sense

- 200MB for a thumbnail is ridiculous, especially if you don't clean up previous thumbnails

- You should probably clean up previous thumbnails

- Sure you don't want to put separators or something into the thumbnail filename?

- The is_uploaded_file checks will never work because you're using the wrong variables

- move_uploaded_file will move the file and I don't know if is_uploaded_file cares whether the file still exists so look into that

- Not accounting for the above point, the conditions for doing the INSERT query only require that the video and thumbnails be uploaded at all and that a title was provided - errors don't stop it from happening

- Keep in mind that empty("0") is true

- Overwriting the video file on subsequent uploads doesn't match up with the fact that you're inserting new videos0 records

- If you'll be putting more code into that TODO then you need to exit; after the header redirect

- I assume you can guarantee that usernames are safe for SQL - they're always only letters and numbers or something restrictive like that

- If there's multiple errors then all the error messages will appear together, though really that whole scheme should be cleaned up to be more user friendly

[mac_gyver]

some points for your code - 

 

1) i recommend that you use an array ($errors for example) to hold validation errors. you can test at any point in your logic if there are errors or not by testing if the array is not empty() or is empty(). this will allow all non-dependent validation tests to be preformed at once. after you check for upload errors (see the next two points) and validate all input data, you would use the submitted form data if the $errors array is empty(). you would display validation errors by displaying the the contents of the $errors array at the appropriate point in the html document.

 

2) if the total size of the submitted form data is greater than the post_max_size setting, both the $_POST and $_FILES arrays will be empty. you need to detect this condition first, before running the rest of the form processing code.

 

3) you need to test if the upload(s) worked, without any upload errors, before referencing any of the uploaded file information.

 

4) this test - } else if (!$allowed) { (in two places) makes no sense. $allowed is an array of the extensions from the 1st uploaded file code. for your existing logic, this should just be an } else { statement. however, there's no point in testing the file extension if the upload didn't work at all. testing any specific uploaded data values must come after you determined that the upload worked without error.

 

5)  is_uploaded_file() operates on the source uploaded file, i.e. the ['tmp_name'] element, not the ['name'] element.

 

6) you have a bunch of lines of code copying variables to other variables, repeated for each possible uploaded file, and you also copied the first uploaded file information to another variable $file, but are not using that to simplify the code. if you are going to perform the same operation multiple times, at a minimum, you should copy the data to a short variable name, then use that variable name in the rest of the code.

 

edit: it's not clear if the form and form processing code are on the same page. if they are, the form processing code needs to be above the start of the html document. this will allow you to perform the header() redirect if the form processing code was successful, to display the errors when you re-display the form, and to re-populate the $_POST form fields with the submitted values so that the user doesn't need to keep re-entering data when there are errors.

Edited February 17, 2018 by mac_gyver

[phreak3r]

If you have a specific problem with your code (as you kind of hinted at) please post THAT part of the code HERE.

 

I, for one, don't want to click on some link that may be holding an entire script of yours or worse, something else even.

 

Happy to help you then.

 Yeah, there is a lot of cleaning up I have to do. Here's the particular excerpt: https://hastebin.com/awekisanuf.bash

Edited February 18, 2018 by phreak3r

[phreak3r]

- Technically you shouldn't try to use stuff in $_FILES until you've done the isset so those assignments would trigger notices

- You strtolower the extension so there's no point looking for MP4 or MOV.

- Need to fix that one error message that currently sticks the error code and size in

- The two else ifs will work but don't make sense

- 200MB for a thumbnail is ridiculous, especially if you don't clean up previous thumbnails

- You should probably clean up previous thumbnails

- Sure you don't want to put separators or something into the thumbnail filename?

- The is_uploaded_file checks will never work because you're using the wrong variables

- move_uploaded_file will move the file and I don't know if is_uploaded_file cares whether the file still exists so look into that

- Not accounting for the above point, the conditions for doing the INSERT query only require that the video and thumbnails be uploaded at all and that a title was provided - errors don't stop it from happening

- Keep in mind that empty("0") is true

- Overwriting the video file on subsequent uploads doesn't match up with the fact that you're inserting new videos0 records

- If you'll be putting more code into that TODO then you need to exit; after the header redirect

- I assume you can guarantee that usernames are safe for SQL - they're always only letters and numbers or something restrictive like that

- If there's multiple errors then all the error messages will appear together, though really that whole scheme should be cleaned up to be more user friendly

 

-It was just a size that I guessed, it was bigger and allowed me to upload some test thumbnails during the time.

-Yeah, I am working on a system for that. But, in what context do you mean clean up the thumbnails?

-I will put separators back into the thumbnail file name.

-That particular function only accepts strings as parameters, not arrays. I tried with the array, did not work.

-I do not quite understand this one. So, even if errors are given out, you can still upload a video if you have a video, thumbnail, and title?

-Overwriting the video file? I did not know I did that..

- Yeah, it is just a test for now, but will be fixed in a matter of time.

 

Erm...Thank You!?

[phreak3r]

If you have a specific problem with your code (as you kind of hinted at) please post THAT part of the code HERE.

 

I, for one, don't want to click on some link that may be holding an entire script of yours or worse, something else even.

 

Happy to help you then.

 

Sorry, ran out of time to edit the first post...

if (is_uploaded_file($fileName) && is_uploaded_file($thumbnailImageName) && !empty($videoTitle)) {
    $sql = "INSERT into videos0 (uploader, video, thumbnail, video_title, video_desc) VALUES ('$username', '$fileDestination', '$thumbnailImageDestination', '$videoTitle', '$videoDesc')";
    $result = mysqli_query($conn, $sql);
    header('Location: /soapbox/upload.php?success');
} else {
    echo "Empty fields!";
    var_dump($file);
    var_dump($thumbnailImageFile);
    var_dump($videoTitle);
}

Edited February 21, 2018 by cyberRobot
please use [code][/code] tags