0

This is driving me nuts, I am learning PHP and this problem has got me all day. I am pulling some info from a users database into a file and then using it in a form to insert into another table.

I want to do this with the users ID and username, I immediately tried $userID = $_GET['id']; which works great but does not when trying $username = $_GET['username']. To get to this form I am echoing the user ID into the url, could this be why it will only get the ID?

If I echo the username it works, so a little stumped. I guess if I echoed the username into the url it would work and ID would not, but I don't want to echo the username or both to be honest unless I have too.

<?php
ini_set('display_errors',1);
error_reporting(E_ALL);

include_once '../includes/conn.php';

if(!$user->is_loggedin()){
    $user->redirect('../users/login.php');
}


$stmt = $conn->prepare("SELECT id, username FROM users");
$stmt->execute();

$userRow=$stmt->fetch(PDO::FETCH_ASSOC);

$userID = $_GET['id'];
$username = $_GET['username'];

if(isset($_POST['submit'])){
    $category = trim($_POST['category']);
    $points = trim($_POST['points']);
    $reason = trim($_POST['reason']); 

    if($category==""){
        $error[] = "Select category."; 
    }else if($points==""){
        $error[] = "Provide points."; 
    }else if($reason==""){
        $error[] = "Provide reason.";
    }else if(strlen($reason) < 6){
        $error[] = "Reason must be at least 6 characters<br /><br />"; 
    }else{
        try{
            $sql = "INSERT INTO infractions(userid,username,category,points,reason)VALUES(?,?,?,?,?)";
            $stmt = $conn->prepare($sql);
            $stmt->execute(array($userID, $username, $category, $points, $reason));
    }
    catch(PDOException $e){
        echo $e->getMessage();
        }
    } 
 }
?>
<!DOCTYPE html>
<html lang="en">    
<head>
    <title>EpicOwl UK | CMS Users Add Infraction</title>
    <meta charset="utf-8">
    <link rel="shortcut icon" href="../images/favicon.ico" type="image/x-icon" />
    <link rel="stylesheet" type="text/css" href="../css/main.css">
</head>
<body>
<div id="header">
    <a href="index.php"><img id="logo" src="../images/logo.png" /></a>
    <div id="navigation">
        <ul>
            <a href="../index.php"><li>Home</li></a>
            <a href="../users/profile.php"><li>My Profile</li></a>
            <a href="./index.php"><li>Admin Panel</li></a>
        </ul>
    </div>
</div>
<div id="content">
<form method="$_POST"><br />
    <h2>Give <?php echo ($userRow['username']); ?> an Infraction</h2>
    <label><strong>Category:</strong></label><br />
    <select name="category">
        <option value="Select Category">Select Category</option>
        <option value="Language Used">Language Used</option>
        <option value="Breaking Rules">Breaking Rules</option>
        <option value="Double Posting">Double Posting</option>
    </select><br /><br />
    <label><strong>Number of points to award:</strong></label><br />
    <input type="text" name="points" maxlength="50" /><br /><br />
    <label><strong>Reason:</strong><label><br />
    <textarea name="reason" rows="13" cols="60" maxlength="255"></textarea><br /><br />
    <button type="submit" name="submit">Add Infraction</button><br /><br /><br />
</form>
</div>
<div id="footer">
    <p class="copyright">&copy; EpicOwl UK. All Rights Reserved.</p>
</div>
</body>
</html>
Sauced Apples
  • 1,163
  • 2
  • 14
  • 37

2 Answers2

3

It looks like you are mixing up your prepared statements and queries. The query should have placeholders where the values will be inserted by the PHP driver.

Here is one approach..

$sql = "INSERT INTO infractions(userid,username,category,points,reason)VALUES(?,?,?,?,?)";
$stmt = $conn->prepare($sql);
$stmt->execute(array($userID, $username, $category, $points, $reason));

Here's a second way...

$sql = ("INSERT INTO infractions(userid,username,category,points,reason)VALUES(:userid, :username, :category, :points, :reason)");
$stmt = $conn->prepare($sql);
$stmt->execute(array(':category'=>$category, ':points'=>$points, ':reason'=>$reason, ':userid' => $userID, ':username'=> $username));

and a third approach

$sql = ("INSERT INTO infractions(userid,username,category,points,reason)VALUES(:userid, :username, :category, :points, :reason)");
$stmt = $conn->prepare($sql);
$stmt->bindParam(":category", $category);
$stmt->bindParam(":points", $points);
$stmt->bindParam(":reason", $reason);
$stmt->bindParam(":userid", $userID);
$stmt->bindParam(":username", $username);
$stmt->execute();

I use the bindings less often so might be some issues there but I believe that is the correct usages.

Here's the PHP manual on it, http://php.net/manual/en/pdo.prepared-statements.php.

The reason for the prepared statements is to separate out user provided data from the SQL. Without the separation you can run into SQL injections, or just failed queries for example if Mr. O'brien tried to create an account/login.

SQL injection attacks are a type of injection attack, in which SQL commands are injected into data-plane input in order to effect the execution of predefined SQL commands.

https://www.owasp.org/index.php/SQL_injection

and a thread that goes into ways of preventing this

How can I prevent SQL injection in PHP?

The method attribute of the form element tells the form how the data should be transmitted. The values are post or get. You seem to be mixing these in your PHP as well.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/form

post: Corresponds to the HTTP POST method ; form data are included in the body of the form and sent to the server.

get: Corresponds to the HTTP GET method; form data are appended to the action attribute URI with a '?' as separator, and the resulting URI is sent to the server. Use this method when the form has no side-effects and contains only ASCII characters.

PHP Usages:
http://php.net/manual/en/reserved.variables.get.php
http://php.net/manual/en/reserved.variables.post.php

Community
  • 1
  • 1
chris85
  • 23,846
  • 7
  • 34
  • 51
  • 1
    Yep. I'd add to this that the original code contained SQL injection vulnerabilities, which is why parameterisation is especially needed in this case. – halfer Jul 07 '15 at 16:35
  • Thanks for the information, I have seen your first example used often so I will switch to it; thank you. Also, I am now passing the username through the URL which is fine, but using the same method as the user id `$username = $_GET['username']; is throwing Notice: Undefined index: id in but it worked before? Nothing is being inserted into the database currently. I've updated the OP to show the changes made. – Sauced Apples Jul 07 '15 at 16:42
  • What is the URL of the page being requested, does it have a `GET` value with `id`? – chris85 Jul 07 '15 at 16:44
  • 1
    This, `
    `, is wrong. It should be `GET` or `POST`. The `$_` is for accessing the values in PHP.
    – chris85 Jul 07 '15 at 16:45
  • `Add` `/addinfraction.php?id=1&username=Apples` is being shown in browser. so I was correct with `
    the first time then, before someone suggested `$_POST`? Thanks
    – Sauced Apples Jul 07 '15 at 16:49
  • Where is this `addinfraction.php` link, I'm not seeing that in the original code? The `$_POST` was for the PHP side, not HTML. I've added an update at the end of my answer. – chris85 Jul 07 '15 at 16:51
  • Scratch that, it was right before the change to form action was suggested, I just checked. Thank you for the links, a great help! – Sauced Apples Jul 07 '15 at 16:51
0

I use addinfraction.php?id=1

This is why it's not working. You have to set the username parameter in order to work. Like I said:

addinfraction.php?id=<?php echo $list['id']; ?>&username=<?php echo $list['username']; ?>

addinfraction.php does not know the username in your example, unlike id.

Parameters are separated by the character '&', like this:

?parameter1=value1&parameter2=value2&parameter3=value3 /*etc...*/
Oldskool
  • 34,211
  • 7
  • 53
  • 66
Devz
  • 563
  • 7
  • 23
  • @chris85 sorry, but since this is a real answer i thought that posting a new one was the thing to do – Devz Jul 07 '15 at 16:29