storing data about available RPG games

Question

I have been developing a little, private Blog Site to make notes/ stories of pen and paper RPG games available for my players and me.

This is my first project. As I am new to coding in PHP and MySQL I'd very much appreciate your opinions on the code I have written. I am interested in any feedback you can give me - but especially on the security of the script I have written. The input form is actually secured by a randomly created username and password authentication via .htaccess. (Side note: the website uses proper encryption for the user input including HSTS and ranks as an A+ on ssllabs.com.)

I have been reading a lot lately on script injection and SQL injection (for example on https://phpdelusions.net/pdo/sql_injection_example) and using variables to create a query was often considered unsafe. Still - in my inexperiencedness - I've made a script that is using the (static) keys of the $_POST Array to determine the database columns and placeholders for values which are used in my PDO query. Afterwards I bind each submitted value to its PDO placeholder.

I do not expect to enter any HTML Input, which is why I decided to use the stripslashes and htmlspecialchars funcions on the input values before binding it to the PDO querys and I haven't used any str_replace function to escape backtics since every value is directly bound to the designated PDO placeholder.

if  (isset ($_POST['session_submit'])) {
    $table = 'sessions';
        
    foreach ($_POST as $column=>$value){
        $column = input_behandlung_sessions($column);
        $secure_value = input_behandlung_sessions($value);
        $formular_daten[$column] = $secure_value;
    }   
    
    $kampagne = $formular_daten['kampagne'];    
    $count = $pdo->prepare('SELECT COUNT(kampagne) FROM sessions WHERE kampagne = :kampagne');
    $count->bindParam(':kampagne', $kampagne, PDO::PARAM_STR);
    $count->execute();
    $rowcount = $count->fetchColumn(0);
    $newrowcount = ++$rowcount;     
    
    unset($formular_daten['session_submit']);

    foreach ($formular_daten as $column=>$value){
        if ($value == "") unset($formular_daten[$column]);
    }

    foreach ($formular_daten as $column=>$value){
        $non_empty_columns[] = $column;
        $columns_param = ':'.$column;
        $array_columns_param[] = $columns_param;
        $to_bind[$columns_param] = $value;
    }

    $text_non_empty_columns = implode (", ", $non_empty_columns);
    $parameter_columns = implode (', ', $array_columns_param);  
       
    $sql_string = 'INSERT INTO '.$table.' (session_uid, '.$text_non_empty_columns.') VALUES ('.$newrowcount.', '.$parameter_columns.')';    
    $stmt = $pdo->prepare($sql_string);
    foreach ($to_bind as $key=>$param){     
        $stmt->bindValue($key, $param);
    }
    $stmt->execute();

    unset($_POST);
    
}   

function input_behandlung_sessions($data) {
  $data = stripslashes($data);
  $data = htmlspecialchars($data);
  return $data;
}

And here is the form used to submit the data (in case you are interested).

echo '
<form method="post" action="#v-pills-session">
      <div class="row">
          <div class="form-group col-md-2 col-sm-6"><label for="formGroupExampleInput">Kampagne: </label></div>
          <div class="col-md-4 col-sm-6"><input required="true" type="text" class="form-control" id="formGroupExampleInput" name="kampagne" placeholder="Kürzel der Kampagne eintragen"></div> 
          <div class="form-group col-md-2 col-sm-6"><label for="formGroupExampleInput">Kapitel-Nr.: </label></div>
          <div class="col-md-4 col-sm-6"><input required="true" type="text" class="form-control" id="formGroupExampleInput" name="session_story_arc" placeholder="Nummer des Kapitels eintragen"></div>
      </div>
      <div class="form-group row">
          <div class="col-md-1 col-sm-12"><label for="formGroupExampleInput">Name: </label></div>
          <div class="col-md-11 col-sm-12"><input required="true" type="text" class="form-control" id="formGroupExampleInput" name="session_name" placeholder="Hier den Namen der Session eintragen"></div>
      </div>
      <div class="form-group row">
          <div class="col-md-1 col-sm-12"<label for="formGroupExampleInput2">Spieler: </label></div>
          <div class="col-md-11 col-sm-12"><input type="text" required="true" class="form-control" id="formGroupExampleInput2" name="session_charaktere_aktiv" placeholder="Hier die Namen der aktiven Charaktere eintragen"></div>
      </div>
      <div class="form-group row">
          <div class="col-md-1 col-sm-12"><label for="formGroupExampleInput2">Quests: </label></div>
          <div class="col-md-11 col-sm-12"><input type="text" class="form-control" id="formGroupExampleInput2" name="session_quests" placeholder="Hier die Quests in dieser Session eintragen"></div>
      </div>
      <div class="form-group row">
          <div class="col-md-1 col-sm-12"><label for="formGroupExampleInput2">NPCs</label></div>
          <div class="col-md-11 col-sm-12"><input type="text" class="form-control" required="true" id="formGroupExampleInput2" name="session_npc" placeholder="Hier die Namen der NPCs aus dieser Session eintragen"></div>
      </div>
      <div class="form-group row">
          <div class="col-md-1 col-sm-12"><label for="formGroupExampleInput2">Orte: </label></div>
          <div class="col-md-11 col-sm-12"><input type="text" class="form-control" required="true" id="formGroupExampleInput2" name="session_orte" placeholder="Hier die Orte eintragen, in welchen die Abenteure unterwegs waren"></div>
      </div>
      <div class="form-group">
          <input required="true" type="submit" name="session_submit" class="form-control btn-primary" id="formGroupExampleInput2" placeholder="Submit">
      </div>
</form>';

Welcome to Code Review! Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. — Sᴀᴍ Onᴇᴌᴀ
– Sᴀᴍ Onᴇᴌᴀ ♦, Commented Nov 7, 2018 at 17:37
@SᴀᴍOnᴇᴌᴀ I hope that I do not violate another rule by answering to your comment but still - I'm sorry, I clearly seem to have lost track of some of the rules, will do better with future questions! — chainsaw42
– chainsaw42, Commented Nov 7, 2018 at 17:51
I've actually written something similar but I use SHOW COLUMNS FROM $table to generate the schema from the DB and then filter the post array by that. It gets a bit tricky when dealing with multiple tables though. — ArtisticPhoenix
– ArtisticPhoenix, Commented Nov 8, 2018 at 19:36

Sᴀᴍ Onᴇᴌᴀ · Accepted Answer · 2021-04-26 22:10:14Z

Let's look at that second loop:

foreach ($formular_daten as $column=>$value){
    if ($value == "") unset($formular_daten[$column]);
}

Some argue it is bad practice to omit the curly brackets, since anyone who needs to add a second statement when the condition is true might forget that there isn't a block here.

Additionally, array_filter() could be used to replace the entire loop:

$formular_daten = array_filter( $formular_daten , 'strlen' );

_{(credit goes to the post by anonymous)}

Additionally, instead of adding values to $formular_daten in the first foreach and then having another foreach loop to unset any value that is equal to an empty string, you could just only add values to the array that are not equal to an empty string...

foreach ($_POST as $column=>$value){
    $column = input_behandlung_sessions($column);
    $secure_value = input_behandlung_sessions($value);
    if ($secure_value) { // equivalent to !== ""
        $formular_daten[$column] = $secure_value;
    }
}

And the unset call for the session_submit could be removed by not setting it in the first loop...

unset($formular_daten['session_submit']);

So if the modified foreach above is used, that unset could be eliminated by updating the condition for setting the array value:

if ($secure_value && $column !== 'session_submit') {
    $formular_daten[$column] = $secure_value;
}

While it appears that you would need to have that loop before the check for count based on the value at $POST['kampagne], you could potentially move the code from that last foreach up into the first foreach loop, which would require looping through the data only once...

You could consider using ? placeholders instead of named placeholders - see Example 2 on the PDO prepare documentation (and compare to the example above it).

Update

I would like to speak to what @Your Common Sense mentioned:

I don't know why you call keys of the $_POST Array "static" (and what does it mean at all)

Not only can be $_POST be modified within the PHP script (because it is a superglobal variable - not a Superglobal constant), but your script can be called by techniques other than submitting the form - including but not limited to:

making a separate page that has a <form> tag with an action= attribute matching the URL corresponding to your script
making an XMLHttpRequest/AJAX request matching the URL corresponding to your script
making requests with tools like Postman

And any of those means can be used to send various values in the POST data.

Sᴀᴍ Onᴇᴌᴀ · Accepted Answer · 2018-11-08 11:24:25Z

I don't know why you call keys of the $_POST Array "static" (and what does it mean at all) but the article you linked to (which I wrote) explicitly demonstrates that $_POST Array keys are unsafe and using them in the query untreated makes your code wide open to SQL injection.

And no, your input_behandlung_sessions() is not the proper treatment. It makes anything but a secure value. Calling its output "secure" is a mockery: htmlspecialchars() has nothing to do with SQL injections, while stripslashes() is just a pointless function that does absolutely nothing useful at all.

As a result, your code is exemplary vulnerable.

You must use the solution offered in the article - a white list of predefined field names. So your code regarding the INSERT query must be something like this:

$formular_daten = []; // variables must be defined before use
$allowed = ['session_story_arc', 'session_name']; //add other fields

$fieldnames = "`session_uid`"; // a list of column names
$values = [$newrowcount]; // an array with values

foreach ($allowed as $column) { // allowed fields ONLY!
    if (!empty($_POST[$column])) { // if a column exists and not empty in a single line
        $fieldnames .= ", `$column`";
        $values[] = $_POST[$column];
    }
}
$parameter_columns = str_repeat('?,', count($values) - 1) . '?';

$sql_string = "INSERT INTO `$table` ($fieldnames) VALUES ($parameter_columns)";
$stmt = $pdo->prepare($sql_string);
$stmt->execute($values);

the code in the end is not only safe but also uses less loops to process.

On a side note, your method to get a session_uid is extremely clumsy and error prone. Depending on the purpose of this field, either make it integer auto_increment or some unpredictable unique value like one generated by session_create_id()

Calak · Accepted Answer · 2018-11-07 16:55:28Z

0

When you expect get reviewed from poeple speaking English, try to use only English in your code (and in comments).

Why go through formular_daten twice when you can check if the value is empty and, if not do the processing in a single loop?

answered Nov 7, 2018 at 16:55

Calak

2,41112 silver badges19 bronze badges

\$\begingroup\$ I changed the variables to english and deleted the forms since they were half german and I do not expect them to be relevant. Additionally I've merged the two loops like you suggested - thank you very much for your answer! \$\endgroup\$

chainsaw42
– chainsaw42

2018-11-07 17:36:17 +00:00
Commented Nov 7, 2018 at 17:36

Add a comment |

chainsaw42 · Accepted Answer · 2018-11-08 16:24:20Z

Considering the suggestions from @Sam Onela and the devastating comment of @Your Common Sense I re-read the article on SQL-Injection and looked up some basic examples on YouTube to understand the importance of the code you provided (like html-form-exploits and sql-injection). I recommend this to everyone else new to this.

The confusion regarding session_uid is understandable. The purpose of this was to be able to sort sessions of different campagins in chronological order and to be able to link quests to sessions. Therefore I changed the field to session_date and the format to "date". The quests will be linked to the session_id (which was not shown in the code, as it is an int, auto_increment column).

I changed the code suggested by @Your Common Sense a bit (a little closer to the example in the article listed in my question) since I had an easier time troubleshooting errors in the PDO statement. Thank you very much for all your input - here's the updated, hopefully better, code:

PS: Could someone point out to me why the backtics in the keys are still being escaped after specifying $allowed keys (like in the solution from phpdelusion)?

if  (isset ($_POST['session_submit'])) {
$table = 'sessions';

$allowed = ["session_story_arc", "session_name", "session_date", "session_charaktere_aktiv", "session_quests", "session_npc", "session_orte", "kampagne"];
$pdo_str_column = $pdo_str_values = $pdo_str_value = "";
$params = [];

foreach ($allowed as $key) {
    if (($_POST[$key]) != ''){
        $pdo_str_column .= "`".str_replace("`", "``", $key)."`, ";
        $pdo_str_value = ":".str_replace("`", "``", $key);
        $pdo_str_values .= ":".str_replace("`", "``", $key).", ";
        $params[$pdo_str_value] = $_POST[$key];
    }
}

$pdo_str_column = rtrim ($pdo_str_column, ", ");  
$pdo_str_values = rtrim ($pdo_str_values, ", "); 

$sql_string = 'INSERT INTO '.$table.' ('.$pdo_str_column.') VALUES ('.$pdo_str_values.')';

$stmt = $pdo->prepare($sql_string);
foreach ($params as $key=>$value) {
        $stmt->bindValue($key, $value);
}
$stmt->execute();

unset($_POST);
unset($params);

}

Stack Exchange Network

storing data about available RPG games

4 Answers 4

Update

You must log in to answer this question.

Hot Network Questions

storing data about available RPG games

4 Answers 4

Update

You must log in to answer this question.

Related

Hot Network Questions