Pagini - Simple to use CMS

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
User avatar
Lyrositor
Forum Newbie
Posts: 6
Joined: Mon Nov 15, 2010 5:51 pm
Location: Cyberspace, Quadrant 512

Pagini - Simple to use CMS

Post by Lyrositor »

I have recently started developing a new PHP CMS (code-named "Pagini") as a hobby of mine. It has currently reached version 0.3.1 and is available at http://sourceforge.net/projects/pagini. Being a relative newcomer to PHP, Pagini was written without ANY advanced features. Inasmuch, please don't attempt to convert me to changing the whole structure of the program - for now, Pagini will remain simple, for good or bad.
What does concern me, however, is Pagini's security. I have gotten reports on Sourceforge.net that it is easily hackable, and I know of a friend who did get hacked. So I would mostly like it if you give a general review of Pagini, with a strong emphasis on security.
Security concerns aside, I hope you enjoy what has been so far a thoroughly enjoyable project for myself!
Note: Pagini development is at a standstill for now due to lack of time. :(
Features:
- Simple, intuitive interface,
- Simple code, easy to change,
- No-nonsense page editor,
- Integrated news manager,
- Theme manager,
- User manager,
- And more to come!
User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: Pagini - Simple to use CMS

Post by Jonah Bron »

I looked through the code a bit. Is this your first attempt? The first security issue I encountered was that it never cleans input. It would take a very small amount of time to put in mysql_real_escape_string() and intval() where needed.

I also observe that the software is heavily procedural. The good thing is that it's marginally easy to understand. The bad thing is that as things get more complex (which they do, *even if the project is kept simple), the understandability goes way down very quickly. This is where OOP saves the day.

Your code syntax is nice and uniform. Although, you may want to take a look at the Zend Coding Standards. Zend code is very well formatted.

With those in mind (and other issues I may have missed), you've done a pretty good job so far.

* Yes, it's possible for a project to be both simple and complex
User avatar
Lyrositor
Forum Newbie
Posts: 6
Joined: Mon Nov 15, 2010 5:51 pm
Location: Cyberspace, Quadrant 512

Re: Pagini - Simple to use CMS

Post by Lyrositor »

Yes, this is my first time.
I know I need mysql_real_escape_string(), but I don't know where to put it.
I'd love to implement an OOP system, but I don't know of any good tutorials. Any help?
User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: Pagini - Simple to use CMS

Post by Jonah Bron »

I dunno, but you'd probably get a load tutorials by googling "php oop".

Here's a example for improving your security. This is an excerpt from /inc/content.inc.php:

Code: Select all

function content()
  {
  if(isset($_GET['id']))
    {
    $id = $_GET['id'];
    }
  else
    {
    $id = "1";
    }
  $result = mysql_query("SELECT * FROM content WHERE id='$id'"); 
This is ripe for a SQL Injection attack [wikipedia.org]. What happens if $_GET['id'] is "1'; DROP TABLE content;"? You're toast, that's what happens.

The value being injected into the query is expected to be a number, so we can clean it with intval [php.net] like so.

Code: Select all

function content()
  {
  if(isset($_GET['id']))
    {
    $id = $_GET['id'];
    }
  else
    {
    $id = "1";
    }
  $id = intval($id);
  $result = mysql_query("SELECT * FROM content WHERE id='$id'"); 
That converts it to the native integer type (thereby stripping out any extraneous characters), instead of a string.

I looked through the code more, and didn't find any place where you didn't use mysql_real_escape_string().
User avatar
Lyrositor
Forum Newbie
Posts: 6
Joined: Mon Nov 15, 2010 5:51 pm
Location: Cyberspace, Quadrant 512

Re: Pagini - Simple to use CMS

Post by Lyrositor »

Okay, I'll work on that.
But there are mysql_real_escape_string() in the admin section!
User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: Pagini - Simple to use CMS

Post by Jonah Bron »

Lyrositor wrote:But there are mysql_real_escape_string() in the admin section!
Jonah Bron wrote:I looked through the code more, and didn't find any place where you didn't use mysql_real_escape_string().
I noticed that :)
User avatar
Lyrositor
Forum Newbie
Posts: 6
Joined: Mon Nov 15, 2010 5:51 pm
Location: Cyberspace, Quadrant 512

Re: Pagini - Simple to use CMS

Post by Lyrositor »

Oh, sorry, I misinterpreted your answer. :oops:
Otherwise, any suggestions for Pagini? In terms of usability, for example, or organization...
User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: Pagini - Simple to use CMS

Post by Jonah Bron »

Okay, I'm giving your program the immense privilege of being installed on my computer and tested. 8O

First impressions. Nice interface. Error messages are displayed nicely. Under "website location", it says "without trailing slash". Instead, you should just use rtrim.

It says "don't forget to delete the install/ directory". Instead, you should have a checkbox asking if you want to delete it. Also, does it log the installation process?

On the admin/(build/manage/customize) pages, there is nothing but links to sub sections. That is an unnecessary extra click. Try to figure out how to make some sort of summary, or put one of those sub sections in the top.

Overall, you've done a pretty nice job. It's very much like Wordpress, but much much simpler. Note though, I have not tried to break anything. The breakability and level of self-repair is important. Keep going, perhaps you can get the attention of other developers interested in working on this.
User avatar
Lyrositor
Forum Newbie
Posts: 6
Joined: Mon Nov 15, 2010 5:51 pm
Location: Cyberspace, Quadrant 512

Re: Pagini - Simple to use CMS

Post by Lyrositor »

I'm considering deleting "Website location" I thought of using it like Wordpress does (my inspiration...), but so far I've gotten by using relative paths so although it's an interesting function (thanks for the tip!) I don't think I'll need to use it.

Delete /install directory? But wouldn't the user have to chown that directory to www-data then?

That menu is a placeholder. It's okay so far, but I'm really trying to figure out a way to make it better. A dropdown menu might work, but it might fit with difficulty in there. A sidebar would be perfect, except, once again, it wouldn't fit well into the overall theme. I guess I could do like Wordpress (like you suggested)... But even then, ultimately, I'd want something better.

Breakability I have not tested. So far, I've opened the puzzle box, taken out the pieces and merged a few that looked like they went together; I haven't tried forcing some to fit yet. So... eventually.

Thanks for your criticism. :D
User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: Pagini - Simple to use CMS

Post by Jonah Bron »

Lyrositor wrote:I'm considering deleting "Website location" I thought of using it like Wordpress does (my inspiration...), but so far I've gotten by using relative paths so although it's an interesting function (thanks for the tip!) I don't think I'll need to use it.
No no, I meant so the person doesn't have to make sure they don't have a trailing space on it. Just use rtrim to remove a trailing slash if there is one.
Lyrositor wrote:Delete /install directory? But wouldn't the user have to chown that directory to www-data then?
Don't think so, Wordpress manipulates files.
Lyrositor wrote:That menu is a placeholder. It's okay so far, but I'm really trying to figure out a way to make it better. A dropdown menu might work, but it might fit with difficulty in there. A sidebar would be perfect, except, once again, it wouldn't fit well into the overall theme. I guess I could do like Wordpress (like you suggested)... But even then, ultimately, I'd want something better.
I'm sure you'll think of something.

Have fun :)
Post Reply