Search code examples
phpsessionfeedback

Would like feedback on my PHP programming & advice on sessions


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

Solution

  • 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: