Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Ok, so I started to build my own CMS. You can see Aleksandar Golubovic's Blog,and I need your help. Please, look at my code, and tell me is that safe? You can download current version from Twitter Bootstrap CMS. If you want to install it, just read instructions.

So, please, tell me if I get some errors, or some of your opinions, instructions... I really need help.

EDIT:

I have many functions like:

function DeletePost()
{
if (isset($_GET['action']))
{
    if ($_GET['action']==='delete-post' && !isset($_GET['id']))
    {
        $qry=mysql_query("SELECT * FROM posts ORDER BY id DESC");
        while ($arr=mysql_fetch_array($qry))
        {
        echo '<div class="span8 offset1"><p><h3>'.$arr['post_title'].'</h3> <a href="javascript:;" title="Delete This Post" class="delete" id="'.$arr['id'].'">Delete this post</a></p><p>'.$arr['post_content'].'</p></div>';
        }
    }
    if ($_GET['action']==='delete-post' && isset($_GET['id']))
    {

            $del=$_GET['id'];
            $qry=mysql_query("DELETE FROM posts WHERE id='$del'") or die ("ERROR!!!");
            echo "This post has been successfull deleted.";


    }
}
}

And this function is included in some page. So my question is: Is this method safe? Note: All functions is separeted pages using .htaccess

share|improve this question
    
This site is for code review only. IE: You post a piece of code and a question concerning the posted code and we review it and offer help on what you should do. This is not a website review board, etc. Perhaps try webapps.stackexchange.com or stack overflow? –  jsanc623 Feb 4 '13 at 15:49
add comment

1 Answer

up vote 0 down vote accepted

Well looking at this as a code review of the code you posted i can recommend a couple of things;

Security

The major flaw at the moment is the possibility for SQL injection. You put the $_GET values directly into the query without validation or escaping.

PHP.net has a page on SQL injection: http://php.net/manual/en/security.database.sql-injection.php

Deprecated code

It is strongly recommend not to use the mysq_* functions. !!!

These functions are deprecated and will recieve no more support and will be removed in an upcoming version of PHP. See http://php.net/manual/en/function.mysql-connect.php and look at the big red box. This is potentially a huge security hazzard in the future as well.

User Content / Access levels

Currently you are not demonstrating any check on access level. If i am to guess your actions i could delete the entire content of your website without logging in!

Database connection

You seem to open the sql connection somewhere else. Please verify for yourself that you also close the connection when request ends, avoiding lingering MySQL connections.

Misc

Also, look into the SOLID principles and PSR-2 codestyle, your current code does not match it. These are best practices and provide a good base strategy for your coding. Also, making it more likely the open source community would accept/adopt your code.

Good luck in further development of your CMS!

share|improve this answer
    
Where can one find information regarding psr-2 codestyle that you speak of? –  chrisjlee Feb 4 '13 at 19:46
    
github.com/php-fig/fig-standards/blob/master/accepted/… . Please note, PSR-2 extends PSR-1 (link in above page) which in turn extends PSR-0 –  mvbrakel Feb 4 '13 at 20:00
    
Thanks!!! This is the best answer I can get! I dont have enough points to vote for your answer, but as soon as I get some, I will vote! –  Aleksandar Feb 4 '13 at 21:10
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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