Would like feedback on my PHP programming & advice on sessions

Question

Hey I'm new to PHP so would really like your insight on how I'm programming and also I'm having trouble coming up with a solution for a session problem.

I'm making a script for a bartering and Local Trade System (LETS) and I'm currently coding the offer page where the user can view all products/services on offer and then click on a product/service to get more details. Upon clicking on the product/service they can make a bid. In a LETS system members have Time/Life Dollars where they can earn by doing trade with other people. So pretty much its alternative currency which is created out of people doing work (unlike our current fiat currency system that governments use). So if a user have Life Dollars they can make a bid to the other user who is offering their products/services.

I'm doing all this on one PHP page called offers.php. To put it simply there will be 4 pages made out of offers.php. When a user initially views the offer section (offers.php) they'll see all the offers, then they can click on an offer (offers.php?id=X) then click to make a bid (offers.php?id=X&action=makebid) and then confirm bid (offers.php?id=X&action=confirm).

Ok so the problem with my Sessions is this: The sessions work when the user starts off from offers.php?id=X up until the end. If they go the route they're suppose to there should be no problems and they wont be able to bypass my validation. However, if the user clicks on say offers.php?id=100 and then enters the URL offers.php?id=200&confirm into the browser address bar they can bypass my validation causing an offer to be entered twice(if they already made an offer). The same happens when the user goes directly to the other offers.php?etc URL but that isn't much of a problem. I would still like to correct this because I'm concerned of when a product/service page is being pasted on another website because then the sessions wont work properly. Does what I'm saying make sense? I can explain more if needed. I love programming so throw out any tips/challenges that you can. Thanks for taking the time :)

Here's my offers.php code:

<?php

require_once('startsession.php');
require_once('dbconnect.php');

if (!isset($_SESSION['user_id'])) {
    echo '<p class="login">Please <a href="login.php">log in</a> to access this page.</p>';
    exit();
}

require_once('navmenu.php');

$dbc = mysqli_connect(DB_HOST, DB_USER, DB_PASSWORD, DB_NAME);

if (isset($_GET['id']) && $_GET['action'] == 'confirm') {

    $adid = $_GET['id'];
    $userid = $_SESSION['user_id'];
    $cost = $_SESSION['cost'];
    $sellerid = $_SESSION['seller_id'];

    //Check if bid was already made
    $query = "SELECT * FROM transactions WHERE ad_id = '$adid' AND buyer_id = '$userid'";
    $data = mysqli_query($dbc, $query);
    $row = mysqli_num_rows($data);

    if ($row == 1) {
        echo '<p>Bid has been made</p>';
    } else {
        //If bid doesnt already exist insert bid
        $query = "INSERT INTO transactions (ad_id, buyer_id, seller_id, cost, status) VALUES ('$adid', '$userid', '$sellerid', '$cost', 'O')";
        $data = mysqli_query($dbc, $query);
    }
} else if (isset($_GET['id']) && $_GET['action'] == 'makeoffer') {

    $adid = $_GET['id'];
    $userid = $_SESSION['user_id'];

    //Check if bid was already made
    $query = "SELECT * FROM transactions WHERE ad_id = '$adid' AND buyer_id = '$userid'";
    $data = mysqli_query($dbc, $query);
    $row = mysqli_num_rows($data);

    if ($row == 1) {
        echo '<p>You have already made a bid on this..</p>';
    } else {
        echo '<form method="post" action="offers.php?id=' . $adid . '&action=confirm">';
        echo '<p>You are about to bid 5 Life Dollars.';
        echo '<input type="submit" value="Confirm" name="submit" /></p>';
        echo '</form>';
    }
} else if (isset($_GET['id'])) {

    $userid = $_SESSION['user_id'];

    //Get ad details
    $adid = $_GET['id'];

    $query = "SELECT * from ads WHERE id = '$adid'";
    $data = mysqli_query($dbc, $query);

    $row = mysqli_fetch_array($data);

    //echo ad details
    echo '<p>' . $row['ad_name'] . '<br>' . $row['ad_desc'] . '<br>' . 'Cost: ' . $row['timedollars']
    . ' Time Dollars . ' . '<br>';

    //Set session seller and cost
    $sellerid = $row['seller_id'];
    $_SESSION['seller_id'] = $sellerid;
    $_SESSION['cost'] = $row['timedollars'];

    //Check to see if a bid was already made
    $query = "SELECT * FROM transactions WHERE ad_id = '$adid' and buyer_id = '$userid'";
    $data = mysqli_query($dbc, $query);
    $row = mysqli_num_rows($data);

    if ($row == 0 && $userid != $sellerid) {
        echo '<a href="offers.php?id=' . $adid . '&action=makeoffer">Make Bid</a></p>';
    } else if ($row == 1) {
        echo 'Already bidded';
    }
} else {

    //Get all ads/offers
    $query = "SELECT * FROM ads WHERE ad_type = 'O'";
    $data = mysqli_query($dbc, $query);

    //echo all ads
    while ($row = mysqli_fetch_array($data)) {
        echo '<p>' . '<a href="offers.php?id=' . $row['id'] . '">' . $row['ad_name'] . '</a>' . '<br>' . $row['ad_desc'] . '</p>';
    }
}

mysqli_close($dbc);
?>

enter code here

Answer 1

I speed read your question (it needs some major editing) and the thing which jumped out at me was...

I'm doing all this on one PHP page called offers.php. To put it simply there will be 4 pages made out of offers.php. When a user initially views the offer section (offers.php) they'll see all the offers, then they can click on an offer (offers.php?id=X) then click to make a bid (offers.php?id=X&action=makebid) and then confirm bid (offers.php?id=X&action=confirm).

I would say having this much functionality in one script is a major code smell.

Why not instead have 4 PHP scripts?

• offers.php
• an-offer.php
• bid.php
• confirm.php

Addendum

Like @Thomas Clayton says, You need to use some POST requests here. You're modifying server state with GET requests which is textbook BAD in so many ways it makes me wish you were parsing HTML with RegEx instead. (which would also be bad)

Read on Wikipedia and on the w3c web site about how GET is a safe method.

Read about web sites that got buggered up because of changing state on GET requests:

• Well-Intentioned Destruction
• The Spider of Doom

Answer 2

This is complicated haha.

Ok, so one simple way of reducing the ability to "hack" your site would be to use "post" variables rather than "get" variables. That way they can't just modify the address bar, they have to post their variables.

The way that I would do it is to store the "transaction" in a database. So, you would have a table with session_id and action or something similar as columns. Then when they load up each "page" of the script instead of getting where they are from the querystring you would query the database for their session_id and get the action from there. Then everytime they complete an action you update the database to say what point they are NOW at.

Another way to to this would be to put the action into a $_SESSION variable and call that each time.

Answer 3

I definately agree about using POST. something like:

<form method=POST action='offers.php'>
<input type=hidden name=id value=100>
<input type=hidden name=action value=confirm>

</form>

BUT, keep in mind that doesnt actually prevent someone from faking a submission. It just makes it far less convenient to do so.

In the end, I'd say, if this is an action the user can do anyways, I wouldnt spend a whole lot of effort trying to prevent it. That is, if they can go about the site, find id 200, and hit confirm... well, whats the difference between that and manually typing it into the address bar? As long as its only affecting their account, its not a major security problem.

As I said down in one the comments, your bigger problem is the sql injection you are inviting by not escaping the id before you put them into your sql queries. Even if you do something like:

$id = $_GET['id'] + 0

To ensure you have a number, and not malicious text.

Another thing to consider is using a switch statement instead of if's. Do a switch based on the action. It'll be much more readable.

case ($action)
{
   'confirm':
      //do confirm stuff
      break;

   'makeoffer':
       // do offer stuff
       break;

   default:
      default stuff

}