0

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
6
  • My first advice would be to try and split logic with presentation. Why don't you jump into a framework? Codeigniter, for instance, would do wonders to the code you just presented (both keeping it smaller and tighter).
    – Frankie
    Aug 13, 2010 at 19:10
  • 1
    Just fyi, you want to make sure you are escaping every parameter you use before sticking it into an sql command (using, for ex, mysql_real_escape_string()). Read up on 'sql injection'. Imagine if 'id' was set to '; drop database;'. Aug 13, 2010 at 19:21
  • @Frankie Im using NetBeans. I'll have a look at Codeigniter thanks
    – Jonathan
    Aug 13, 2010 at 19:21
  • @Jonny - when you edit please keep an eye out for whether or not your code is formatted properly ;) Aug 13, 2010 at 19:21
  • I wouldnt suggest frameworks and such quite yet. Its obvious he's trying to learn. Throwing in frameworks and complicating the script is just going to make things more difficult for him at this point. Aug 13, 2010 at 19:23

3 Answers 3

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.

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

3
  • i'm in two minds as to whether this is a good idea or not generally. it doesn't acctually make any difference (in my opinion) and this wouldn't acctually stop what's happening in the question. Obviously it makes for more managable code, but there shouldn't be any issue with doing the whole thing in one script. Aug 13, 2010 at 19:10
  • It's a pretty broad question ;) I focused on the title - requesting feedback - as I didn't make it that far into the question :P Aug 13, 2010 at 19:12
  • The single php page dispatching actions is fine. I'd include the actual functionality into functions or classes stored in other files. But he did say he's just starting out, so he'll get to that. Aug 13, 2010 at 19:19
0

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

}
3
  • Thanks for your input. The problem isnt the user going directly to the URL. Its when they go to offers.php?id=300 and then directly pasting offers.php?id=100&action=confirm its because the seller id is set at the start. so offer 300 will set sellerid = 4 for example and then when they go to the action=confirm page of another product directly the sellerid wont be what it should be. Now that i think of it, i think i made a mistake even setting a sellerid when i can get the sellerid from the database using the ad id!
    – Jonathan
    Aug 13, 2010 at 19:48
  • Yes, definately pull the seller id out of the database, and dont rely on the one stored as a session variable. I hadnt noticed you were doing that, but thats certainly part of the problem. Aug 13, 2010 at 20:28
  • yeah, jonny you really want to make as much of your script as dynamic as possible. never store a static ID twice if you don't have to... ie: what I mean is, if you have a product id for your transaction, always dynamically check the seller id when you need to retrieve the product. If you do it like this you reduce the risk of confusing things, and you have less places to go when you need to change your code.e Aug 14, 2010 at 17:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.