-2

So I am trying to create this form however, I am not able to enter the data into the table. Whenever I click on continue button, nothing really happens. I don't get an error or anything. and when I check my database, the values have not been added. Idk how its happening. I spent hours trying to understand what the issue is but i am not able to identify it and it got to the point where I felt hopeless and decided to post this question so please help me. Here's my code:

<?php

$conn = new mysqli("localhost", "root", "", "KFC");
if(isset($_POST['Submit_btn']))
{
    session_start();
    $RID= $_POST['R_ID'];
    $Name= $_POST['Name'];
    $S_Description=($_POST['S_Description']);
    $L_Description=($_POST['L_Description']);
    $Nutritional_value=($_POST['Nutritional_value']);
    $Cost=($_POST['Cost']);
    $TypeOfMeal=($_POST['TypeOfMeal']);
    $SubmittedBy=($_POST['SubmittedBy']);
    $sql= "INSERT INTO `recipe`(`R_ID`, `Name`, `S_Description`, `L_Description`, `Nutritional_value`, `Cost`, `TypeOfMeal`, `SubmittedBy`) VALUES ($RID,$Name,$S_Description,$L_Description,$Nutritional_value,$Cost,$TypeOfMeal,$SubmittedBy)";
    mysqli_query($conn,$sql);

}
mysqli_close($conn);
?>
<html>
<body>
<div id="bodyContent">
<h1> Registration </h1>
</div>
<form method="post">
<table>
<tr>
<td>R_ID: </td>
<td>
<input type="text" name="R_ID" class="textInput">
</td>
</tr>

<tr>
<td>Name: </td>
<td>
<input type="text" name="Name" class="textInput">
</td>
</tr>

<tr>
<td>Small Description: </td>
<td>
<input type="text" name="S_Description" class="textInput">
</td>
</tr>

<tr>
<td>Long Description: </td>
<td>
<input type="text" name="L_Description" class="textInput">
</td>
</tr>

<tr>
<td>Nutritional value: </td>
<td>
<input type="text" name="Nutritional_value" class="textInput">
</td>
</tr>

<tr>
<td>Cost: </td>
<td>
<input type="number" name="Cost" class="textInput">
</td>
</tr>

<tr>
<td>Type Of Meal: </td>
<td>
<input type="text" name="TypeOfMeal" class="textInput">
</td>
</tr>

<tr>
<td>UserID: </td>
<td>
<input type="Number" name="SubmittedBy" class="textInput">
</td>
</tr>

<tr>
<td> </td>
<td>
<input type="submit" name="Submit_btn" value="Continue">
</td>
</tr>

</table>
</form>
</body>
</html>
  • 2
    **Your code is vulnerable to SQL injection and will be hacked** even if [you are escaping inputs!](https://stackoverflow.com/a/5741264/2595450) Use [Prepared Statements](http://php.net/manual/en/mysqli.quickstart.prepared-statements.php) instead. – Spoody Apr 07 '18 at 23:07
  • 2
    "I don't get an error or anything" -- you're not checking for an error. If the query fails, `mysqli_query()` will return `false` and you can get more information about **why** it failed by looking at the result of `mysqli_error()`. – rickdenhaan Apr 07 '18 at 23:08
  • 1
    Also, why are you adding parentheses around `$_POST` variables? – Spoody Apr 07 '18 at 23:09
  • 1
    "I don't get an error or anything" i don't see error_reporting enabled with in your code.. – Raymond Nijland Apr 07 '18 at 23:09
  • 1
    do you make it a habit to get your solutions and run off? – Funk Forty Niner Apr 08 '18 at 00:29
  • @FunkFortyNiner Off all the questions I posted including this one, I didn't get any answer that solved my problem. I do not want to take advantage of people by using their solution and then running off. Do not assume idiotic things about me without knowing what you are talking about. Thank u. – buffyBTVS96 Apr 08 '18 at 01:26

2 Answers2

0

Ok a few things:

$SubmittedBy=($_POST['SubmittedBy']);

you don't need the parenthesis around the $_POST. But if you're going to use the data from it especially in a SQL but not only you should sanitize the input. Here is an example with mysqli_real_escape_string so it would look like this:

$SubmittedBy=mysqli_real_escape_string($conn, $_POST['SubmittedBy']);

There are other methods like PDO::quote or using prepared statements, you might want to have a look at those too.

Next up your sql inserts the values without quotes:

$sql= "INSERT INTO `recipe`(`R_ID`, `Name`, `S_Description`, `L_Description`, `Nutritional_value`, `Cost`, `TypeOfMeal`, `SubmittedBy`) VALUES ('$RID','$Name','$S_Description','$L_Description','$Nutritional_value','$Cost','$TypeOfMeal','$SubmittedBy')";

Edited: Thanks to Funk Forty Niner for pointing out the missing $conn !

Paul G Mihai
  • 226
  • 1
  • 6
  • so i used mysqli_error() and it showed me the following message :- Unknown column 'sid' in 'field list' I don't understand what it is referring to.. I didnt use the column sid.. Dont even have it – buffyBTVS96 Apr 07 '18 at 23:22
  • 1
    `mysqli_real_escape_string` [doesn't protect from SQL injections](https://stackoverflow.com/a/5741264/2595450) – Spoody Apr 07 '18 at 23:29
  • @MehdiBounya please show me how you can crack my sql considering that I used single quotes together with mysqli_real_escape_string – Paul G Mihai Apr 07 '18 at 23:48
  • 1
    actually @Paul the escape function won't even run at all; it's missing the connection's argument. So, you were really only 1/2 right kind'ish. – Funk Forty Niner Apr 07 '18 at 23:49
  • @Chris abusing prepared statements as sql sanitizers creates unnecessary overhead. They should be only used if the sql is being used repeatedly and only the parameters change! If you use it once you generate unnecessary communication with the mysql server and sql cache that will never be used a second time. – Paul G Mihai Apr 07 '18 at 23:51
  • 2
    I'm with everyone here about using a prepared statement. There are a lot more advantages than just helping to ward off an sql injection. With PDO, for one; named placeholders are so much easier to keep track of. – Funk Forty Niner Apr 07 '18 at 23:58
  • @Chris It is an abuse in that prepared statements are not built for sanitazing purposes but for repeated use, so to spare the SQL parser & connection of processing and transmitting the same stuff over and over. The overhead may be small but it quickly adds up when you do loads of unique queries. – Paul G Mihai Apr 08 '18 at 00:17
  • @FunkFortyNiner I do agree the named parameter binding is nice, while better then having a bunch of '???'s. I'm not against using prepared statemetns... at all, they are the more robust solution for sure. If there would be a way to not have 2 interactions with the server and the cache overhead I would always use it, no questions asked. As it is right now I use them cautiously however. – Paul G Mihai Apr 08 '18 at 00:35
  • Agreed @Paul and yeah, that's what I had in mind earlier about getting lost into a bunch of `?`'s and `iisssssiiisssissssissssssdssssbssssii`'s *haha!* ;-) ok, maybe I went a tad overboard ¯_(ツ)_/¯ – Funk Forty Niner Apr 08 '18 at 00:38
-1

Try replacing with this

<?php
// This prevents SQL injection threat
$RID = mysqli_real_escape_string($conn, $RID);
$Name = mysqli_real_escape_string($conn, $Name);
$S_Description = mysqli_real_escape_string($conn, $S_Description);
$L_Description = mysqli_real_escape_string($conn, $L_Description);
$Nutritional_value = mysqli_real_escape_string($conn, $Nutritional_value);
$Cost = mysqli_real_escape_string($conn, $Cost);
$TypeOfMeal = mysqli_real_escape_string($conn, $TypeOfMeal);
$SubmittedBy = mysqli_real_escape_string($conn, $SubmittedBy);

$sql= "INSERT INTO `recipe`(`R_ID`, `Name`, `S_Description`, `L_Description`, `Nutritional_value`, `Cost`, `TypeOfMeal`, `SubmittedBy`) VALUES ('$RID','$Name','$S_Description','$L_Description','$Nutritional_value','$Cost','$TypeOfMeal','$SubmittedBy')";

But more importantly, activate error displaying in your DEV environment, but remove it when you will host your website (PROD environment):

<?php
ini_set('display_errors', '1');
error_reporting(E_ALL);
Ermac
  • 1,181
  • 1
  • 8
  • 12
  • 2
    Please recommend parameter binding, not arbitrary use of `mysqli_real_escape_string()`. Without knowing the types of those columns there's no way to know whether that function will do the right thing. – ChrisGPT was on strike Apr 07 '18 at 23:30
  • Well it should always work even if the type is a number since we are in Mysql, using `NUMBER = 1` or `NUMBER = '1'` is the same... Show me a case when it won't work I'm curious. – Ermac Apr 07 '18 at 23:33
  • See https://stackoverflow.com/a/5741264/354577. The _only_ way to reliably protect against SQL injection is to use parameter binding. – ChrisGPT was on strike Apr 07 '18 at 23:34
  • And by the way, those two statements aren't the same in any sane database. MySQL isn't sane; it shouldn't be doing type coercion. – ChrisGPT was on strike Apr 07 '18 at 23:35
  • "in Mysql, using NUMBER = 1 or NUMBER = '1' is the same... Show me a case when it won't work I'm curious" Ok you asked for it. If NUMBER is a int datatype and indexed and if you use `WHERE NUMBER = '1'` MySQL is unenable to use the index on column NUMBER. – Raymond Nijland Apr 07 '18 at 23:35
  • I have wrapped the variables in single quotes, so what you showed isn't applied to my code... – Ermac Apr 07 '18 at 23:36
  • I'm not going to waste any more time arguing with you about this. But you should check out http://sqlmap.org/. There are _way more ways to SQL inject that you imagine_, and your solution _does not protect against all of them_, full stop. – ChrisGPT was on strike Apr 07 '18 at 23:40
  • @Raymond Nijland Well Ok good to know, but do we index INT column types? They should be fast to search don't know anyway, but anyway PDO is the recommended solution to deal with databases in PHP but here I'm following with the code of the user and since mysqli is still in use with PHP 7 we can still use it, we all started with `mysql_connect()`. – Ermac Apr 07 '18 at 23:42
  • Besides `mysqli_real_escape_string` is only "safe" when you set the client's charset like the documentation says. "Security: the default character set The character set must be set either at the server level, or with the API function mysqli_set_charset() for it to affect mysqli_real_escape_string()." – Raymond Nijland Apr 07 '18 at 23:42
  • I know I said I wasn't going to waste any more time here, but here's another good resource: https://phpdelusions.net/sql_injection – ChrisGPT was on strike Apr 07 '18 at 23:47
  • @RaymondNijland Your point is valid, how could I change the code to take into account your note? – Ermac Apr 08 '18 at 00:14