#1
  1. No Profile Picture
    Contributing User
    Devshed Novice (500 - 999 posts)

    Join Date
    Jun 2009
    Posts
    667
    Rep Power
    6

    Review a simple script?


    If anyone is free, I don't suppose I could get anyone to review my front-end page and let me know if they've any suggestions or such, and maybe take a peek at the scripting for it and offer any input if needed? The code is to meerly offer listing of bills for the selected month. Final edit was adding in the ability to safely select bills from only 12 most recent months, and ignore older ones. Thanks for any help.

    Page: http://www.hellzoneinc.com/bills.php
    Code: http://www.hellzoneinc.com/bills.txt
  2. #2
  3. Mad Scientist
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Oct 2007
    Location
    North Yorkshire, UK
    Posts
    3,660
    Rep Power
    4123
    PHP Code:
     $month $_POST["mo"]; 
    and

    PHP Code:
    $invoices mysqli_query($con,"SELECT * FROM `invoices` WHERE MONTH(due) = " $month " AND YEAR(due) = " $year " ORDER BY `due` ASC;"); 
    opens yourself up to SQL injection

    Also

    PHP Code:
    echo "....<td align=\"center\">" $paid "</td>...."
    what's wrong with

    PHP Code:
    echo "....<td align='center'>" $paid "</td>...."
    (cleaner looking code)
    I said I didn't like ORM!!! <?php $this->model->update($this->request->resources[0])->set($this->request->getData())->getData('count'); ?>

    PDO vs mysql_* functions: Find a Migration Guide Here

    [ Xeneco - T'interweb Development ] - [ Are you a Help Vampire? ] - [ Read The manual! ] - [ W3 methods - GET, POST, etc ] - [ Web Design Hell ]
  4. #3
  5. No Profile Picture
    Contributing User
    Devshed Novice (500 - 999 posts)

    Join Date
    Jun 2009
    Posts
    667
    Rep Power
    6
    Thanks Northie. I'll toss some cleanup in there for input.
  6. #4
  7. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,930
    Rep Power
    1045
    Hi,

    "cleanup" in what sense? In your code, you use the HTML escaping function for the database input. That doesn't work, because HTML and SQL are completely different languages with a different syntax.

    So "cleanup" isn't really a good name for a function, because there is no universal way to make input safe. It depends on the language and sometimes even the context.

    It's also problematic that you only escape certain values. This is extremely error-prone, because you can easily forget values or forget to add the escaping after you've changed something. From taking a glance at the code, I really couldn't tell if it's actually secure.

    So I'd use a different approach:
    • For dynamic database queries, always use prepared statements. Isn't that the very reason to switch to MySQLi?
    • For HTML stuff, always use htmlentities() or rather htmlspecialchars(). Escape any value, regardless of whether it's actually "dangerous" at this point of time.
  8. #5
  9. No Profile Picture
    Contributing User
    Devshed Novice (500 - 999 posts)

    Join Date
    Jun 2009
    Posts
    667
    Rep Power
    6
    Well, I actually removed my htmlentities function to do my cleanup, and figured since it was a simple reference to the input, I made a shorthand If/Then to check if it is numeric. As far as the prepared statements, yes, I do need to get myself updated, but that will come in time. So, from a drop-down, the online input to be submitted should be a integer 1-12. This should avoid other input anyway, I would assume.

IMN logo majestic logo threadwatch logo seochat tools logo