Security of articles.php

[Chris_Evans]

I've looked through this and personally can't see any security issues, I've tried plenty of ? in the browser for the $_GET[] command, can't get anything not to work. Any security suggestions anyone?

<?php
	// Initial requested files
	include("config.inc.php");

// Initial requested HTTP Post Vars
if(!$_GET['name']) {
header("location: $weburl"); }
else {
$selection = "option";
$name = $_GET['name']; }

switch ($selection) {
case option:
	if(!$_GET['page']) {
	$page = "1"; }
	else {
	$page = $_GET['page']; }

$sqlq = "SELECT name FROM articles WHERE name='$name'";
$sqlr = mysql_query($sqlq) or die(mysql_error());
$sqlc = mysql_num_rows($sqlr);

$numpage = $page+1;
if($page == $sqlc) {
$url = $weburl."articles.php?name=$name&amp;page=$page"; }
else {
$url = $weburl."articles.php?name=$name&amp;page=$numpage"; }

$hfer = "You're currently on page $page of $sqlc.
            Click <a href=\"$url\">here</a> to go to the next page.";

$sqlpageone = $page-1;
$sqlpageten = $page+9;
$sqlq = "SELECT title,article,writer,type,image,alt,keywords FROM articles
            WHERE name='$name' AND page='$page'";
$sqlr = mysql_query($sqlq) or die(mysql_error());
while($sqla = mysql_fetch_array($sqlr)) {
	$title = $sqla['title'];
	$article = $sqla['article'];
	$writer = $sqla['writer'];
	$type = $sqla['type'];
	$image = $sqla['image'];
	$alt = $sqla['alt'];
	$keywords = $sqla['keywords'];

	$imagesrc = $weburl."images/$type/".$image;

	$catlisting .= "<tr><td align=\"center\" class=\"main\">";
	$catlisting .= "<img src=\"$imagesrc\" style=\"border:0;\" alt=\"$alt\" />";
	$catlisting .= "$article";
	$catlisting .= "</td></tr>";
} mysql_free_result($sqlr);

$body = "<tr><td align=\"center\" class=\"title\">$title</td></tr>$catlisting<tr>
            <td align=\"center\" class=\"main\">$hfer</td></tr>";
}

$title = "$title";
include("./end.php");

Thanks,

Chris

[Chris_Evans]

addslashes();
strip_tags();
htmlentities();

Would these be a good idea? I forgot about the first, I usually put it in.

[mtimdog]

is_numeric on $_GET['page']

btw, what is this doing??

    $numpage = $page+1; 
    if($page == $sqlc) { 
    $url = $weburl."articles.php?name=$name&page=$page"; } 
    else { 
    $url = $weburl."articles.php?name=$name&page=$numpage"; } 

...it's been a long day and my brain can't seem to follow the logic.

Also, using die(mysql_error()) may give the hacker a sense of what your db looks like making it easier to crack...I'd suggest replacing them with Error statements when it's finished.

On a side note this

$title = $sqla['title']; 
        $article = $sqla['article']; 
        $writer = $sqla['writer']; 
        $type = $sqla['type']; 
        $image = $sqla['image']; 
        $alt = $sqla['alt']; 
        $keywords = $sqla['keywords']; 

can be replaced by
extract($sqla);

[Chris_Evans]

Hi there,
The text selects a page from the articles database by looking for the page name, it then gets the page number to go to and displays it.

Thanks for the tip! 🙂

Chris

[Chris_Evans]

How's this:

	// Initial requested files
	include("config.inc.php");

// Initial requested HTTP Post Vars
if(!$_GET['name']) {
header("location: $weburl"); }
else {
$selection = "option";
$name = addslashes($_GET['name']);
$name = strip_tags($name); }

switch ($selection) {
case option:
	if(!$_GET['page']) {
	$page = "1"; }
	else {
	$page = is_numeric($_GET['page']);
		if($page == "TRUE") {
		$page = $_GET['page']; }
		else
		{
		header("location: $weburl"); } }

$sqlq = "SELECT name FROM articles WHERE name='$name'";
$sqlr = mysql_query($sqlq) or die(mysql_error());
$sqlc = mysql_num_rows($sqlr);

$numpage = $page+1;
if($page == $sqlc) {
$url = $weburl."articles.php?name=$name&page=$page"; }
else {
$url = $weburl."articles.php?name=$name&page=$numpage"; }

$hfer = "You're currently on page $page of $sqlc. 
            Click <a href=\"$url\">here</a> to go to the next page.";

$sqlpageone = $page-1;
$sqlpageten = $page+9;
$sqlq = "SELECT title,article,writer,type,image,alt,keywords
            FROM articles WHERE name='$name' AND page='$page'";
$sqlr = mysql_query($sqlq) or die(mysql_error());
while($sqla = mysql_fetch_array($sqlr)) {
	extract($sqla);

	$imagesrc = $weburl."images/$type/".$image;

	$catlisting .= "<tr><td align=\"center\" class=\"main\">";
	$catlisting .= "<img src=\"$imagesrc\" style=\"border:0;\" alt=\"$alt\" />";
	$catlisting .= "$article";
	$catlisting .= "</td></tr>";
} mysql_free_result($sqlr);

$body = "<tr><td align=\"center\" class=\"title\">$title</td></tr>$catlisting<tr>
            <td align=\"center\" class=\"main\">$hfer</td></tr>";
}

$title = "$title";
include("./end.php");
?>

[mtimdog]

That looks good...or you could put something like

include("config.inc.php"); 

// Initial requested HTTP Post Vars 
if(!$_GET['name']) { 
header("location: $weburl"); } 
else { 
$selection = "option"; 
$name = addslashes($_GET['name']); 
$name = strip_tags($name); } 

switch ($selection) { 
case option: 
    if (is_numeric($_GET['page']){
        $page=$_GET['page'];
       //you could optionally leave out the following elseif
    }elseif(isset($_GET['page'])){
          header("location: $weburl");
    }else{  

         $page = 1;//a literal, not a string even though it
     //automatically converts it for you
    }
...

[Chris_Evans]

Ok, the completed script:

<?php
/****************************************************/
/* articles.php										*/
/****************************************************/
/* This file finds and displays articles from		*/
/* the MySQL database.								*/
/****************************************************/
/* This file is copyright of						*/
/* First Choice Junction 10 Marketing Ltd			*/
/* Under no circumstances may it be distributed		*/
/* or modified by any persons, whether licensed		*/
/* or otherwise.									*/
/****************************************************/
/* Author: Chris Evans								*/
/* konect internet									*/
/* First Choice Junction 10 Marketing Ltd			*/
/****************************************************/

// Initial requested files
include("config.inc.php");

// Initial requested HTTP Post Vars
if(!$_GET['name']) {
header("location: $weburl"); }
else {
$name = addslashes($_GET['name']);
$name = strip_tags($name); }

if(!$_GET['page']) {
$page = "1"; }
else {
	if(is_numeric($_GET['page'];) {
	$page = $_GET['page']; }
	elseif(isset($_GET['page'])) {
	header("location: $weburl"); }
	else {
	$page = 1; }

$sqlq = "SELECT name FROM articles WHERE name='$name'";
$sqlr = mysql_query($sqlq) or die(mysql_error());
$sqlc = mysql_num_rows($sqlr);

$numpage = $page+1;
if($page == $sqlc) {
$url = $weburl."articles.php?name=$name&amp;page=$page"; }
else {
$url = $weburl."articles.php?name=$name&amp;page=$numpage"; }

$hfer = "You're currently on page $page of $sqlc. Click <a href=\"$url\">here</a> to go to the next page.";

$sqlpageone = $page-1;
$sqlpageten = $page+9;
$sqlq = "SELECT title,article,writer,type,image,alt,keywords FROM articles 
WHERE name='$name' AND page='$page'";
$sqlr = mysql_query($sqlq) or die(mysql_error());
while($sqla = mysql_fetch_array($sqlr)) {
	extract($sqla);
	$imagesrc = $weburl."images/$type/".$image;

$catlisting .= "<tr><td align=\"center\" class=\"main\">";
$catlisting .= "<img src=\"$imagesrc\" style=\"border:0;\" alt=\"$alt\" />";
$catlisting .= "$article";
$catlisting .= "</td></tr>";
} mysql_free_result($sqlr);

$body = "<tr><td align=\"center\" class=\"title\">$title</td></tr>$catlisting<tr>
<td align=\"center\" class=\"main\">$hfer</td></tr>";
}

$title = "$title";
include("./end.php");
?>

That looks neater too as I've removed the case (no need for it).

Chris

[mtimdog]

One question, what does this do?
if(is_numeric($_GET['page']<img src="images/smilies/wink.gif" border="0" alt="">

it might work better as

<?php
if(is_numeric($_GET['page']){
    ?><img src="images/smilies/wink.gif" border="0" alt=""><?php
?>

[Chris_Evans]

I never wrote that, it must've been VB trying to insert the wink.gif 😉.

Chris

[seby]

less double quoting.
less use of unnecessary variables, no point in making three variables to perform one thing.

those 40+ lines of code can be cut in half.

it will speed up your code and it'll be easier to read.

[Chris_Evans]

Could you point out where I have any unecessary variables?

Chris

[seby]

eg:

$name = addslashes($_GET['name']);
$name = strip_tags($name); 

just do it right in the query string if you are only using it once.. eg

mysql_query("SELECT * FROM table WHERE column = '".addslashes($_GET['name'])."'");

from three lines to one:

$rows = mysql_num_rows(mysql_query("SELECT * FROM table WHERE blah"));

also never quote numbers.

eg:

$page = "1";

those are just my opinions. but the less variables you have its much easier to read. and in theory would run faster since it has less varibles to find values for. they are small things, but its good coding practice.

[planetsim]

Originally posted by seby
eg:

$name = addslashes($_GET['name']);
$name = strip_tags($name); 

just do it right in the query string if you are only using it once.. eg

mysql_query("SELECT * FROM table WHERE column = '".addslashes($_GET['name'])."'");

from three lines to one:

$rows = mysql_num_rows(mysql_query("SELECT * FROM table WHERE blah"));

also never quote numbers.

eg:

$page = "1";

those are just my opinions. but the less variables you have its much easier to read. and in theory would run faster since it has less varibles to find values for. they are small things, but its good coding practice. [/B]

Quoting Numbers is perfectly okay if there is a leading 0 in this case there isnt so its not neccesary.

Also im a little curious to this

mysql_query("SELECT * FROM table WHERE column = '".addslashes($_GET['name'])."'");

although its correct it should be done outside of the query. There wont be much change in speed if it were even noticable.