Hospital (V2)

[The Phantom]

I need help. I don't know if it's secure enough? I took me two days to create. If anyone can help me clean it up, I would be grateful!

 

<?php

require('globals.php');

echo "<h3><u>Hospital</u></h3>";

if(!isset($_GET['st']))
{
 $_GET['st'] = 0;
}
if(!$_GET['st']) 
{ 
 $_GET['st'] = 0; 
}
$st=abs((int) $_GET['st']);
$query = $db->query("SELECT `hospital` FROM `users` WHERE `hospital` > 0");
$members = $db->num_rows($query);
$pages = ceil($members / 25); 
for($i = 1; $i <= $pages; $i++)
{
$s = ($i-1) * 25;
if($s == $st) 
{ 
echo "Pages: <strong>$i</strong> "; 
} else { 
echo "<a href='hospital.php?&st=$s'>$i</a> "; }
if($i % 25 == 0) 
{ 
echo "<br />"; 
}
}
echo "<br /><br />
<hr width='50%'>Welcome to the Hospital. Unlucky to be placed here.<br/>Medical Bill: " . money_formatter($ir['hosp_bill']) . "<hr width='50%'><br />
<hr width='85%'><table width='85%' class='table' border='0' cellspacing='1'><tr><th>Name</th><th>Level</th><th>Time</th><th>Reason</th><th>Links</th></tr>";
$q=$db->query("SELECT `u`.*, `c`.*
                    FROM `users` AS `u`
                    LEFT JOIN `clans` AS `c`
                    ON `u`.`clan`=`c`.`clanID`
                    WHERE `u`.`hospital` > 0
                    ORDER BY `u`.`hospital` 
                    DESC LIMIT $st, 25");
while($r=$db->fetch_row($q))
{
$time=$r['hospital'];
$t4=floor($time/60/24);
$t1=floor($time/60) % 24;
$t2=$time % 60;
if($t2 < 10) { $t3="0".$t2; } else { $t3=$t2; }
if($t4) { $t5="$t4 days,"; } else { $t5=""; }
if($t1) { $t1="$t1 hours,"; } else { $t1=""; }
if($t2 == 1) { $t2="1 minute"; } else { $t2="$t2 minutes"; }
echo "\n<tr><td> <a href='viewuser.php?u={$r['userid']}'>{$r['username']}</a> [{$r['userid']}]</td><td>
{$r['level']}</td><td>$t5 $t1 $t2</td><td>{$r['hospreason']}</td><td><a href='healer.php'>[Hire Healer]</td></tr>";
}
if($db->num_rows($q) == 0)
{
echo "<td colspan='5'>There is no one in hospital!</td>";
}
echo "</table><hr width='85%'><br /><hr width='50%'><a href='index.php'>>Go Home</a><hr width='50%'><br />";
$h->endpage();
?>

[G7470]

Well "secure enough" is something that is really your opinion on whether it is or not as you can go crazy with securing your code. The biggest thing you want to do though is to validate the user input (such as the $_GET variable(s)) as this is the most common security problem. You're just about there, but you're not validating whether $_GET['st'] is a number or not in order to run line 15 appropriately. So what I would do is replace lines 11-14 with this:

 

if(!$_GET['st'] || !is_numeric($_GET['st']))
{
 $_GET['st'] = 0;
}

 

For reference on the is_numeric function: http://php.net/manual/en/function.is-numeric.php

[The Phantom]

[MENTION=70485]G7470[/MENTION] - Thanks man! Just learnt something new :D

[Magictallguy]

• Added some basic formatting
  
• Changed the abs((int) [...]) to .. well, look at line 4
  
• Added a stripslashes() and htmlspecialchars() to the usernames
  
• Re-wrote both queries, stripping the call to the clans table from the second query as there was nothing using it
  
• Minor changes to the for() loop
  
• Brought it down from 61 lines of code to 45
  

 

<?php
include(__DIR__ . '/includes/globals.php');
echo '<h3><u>Hospital</u></h3>';
$st = array_key_exists('st', $_GET) && ctype_digit($_GET['st']) ? $_GET['st'] : 0;
$query = $db->query('SELECT COUNT(`userid`) FROM `users` WHERE `hospital` > 0');
$members = $db->fetch_single($query);
$pages = ceil($members / 25) + 0;
for($i = 1; $i <= $pages; ++$i) {
$s = $i * 25;
echo $s == $st ? 'Pages: <strong>' . $i . '</strong> ' : '<a href="hospital.php?&st=' . $s . '">' . $i . '</a> ';
}
?><hr width='50%' />
Welcome to the Hospital. Unlucky to be placed here.
Medical Bill: <?php echo money_formatter($ir['hosp_bill']); ?><hr width='50%' />
<hr width='85%' />
<table width='85%' class='table' border='0' cellspacing='1'>
<tr>
	<th>Name</th>
	<th>Level</th>
	<th>Time</th>
	<th>Reason</th>
	<th>Links</th>
</tr><?php
$q = $db->query("SELECT `userid`, `username`, `level`, `hospital`, `hospreason` FROM `users` WHERE `hospital` > 0 ORDER BY `hospital` DESC LIMIT " . $st . ", 25");
if (!$db->num_rows($q))
echo '<tr><td colspan="5">There is no one in hospital</td></tr>';
while ($r = $db->fetch_row($q)) {
$time = $r['hospital'];
$t4 = floor($time / 60 / 24);
$t1 = floor($time / 60) % 24;
$t2 = $time % 60;
$t3 = $t2 < 10 ? '0' . $t2 : $t2;
$t5 = $t4 ? $t4 . ' days, ' : '';
$t1 = $t1 ? $t1 . ' hours, ' : '';
$t2 = $t2 . ' minute' . ($t2 == 1 ? '' : 's');
?><tr>
	<td><a href='viewuser.php?u=<?php echo $r['userid']; ?>'><?php echo stripslashes(htmlspecialchars($r['username'])); ?></a> [<?php echo $r['userid']; ?>]</td>
	<td><?php echo number_format($r['level']); ?></td>
	<td><?php echo $t5, ' ', $t1, ' ', $t2; ?></td>
	<td><?php echo stripslashes($r['hospreason']); ?></td>
	<td><a href='healer.php'>[Hire Healer]</td>
</tr><?php
}
?></table><hr width='85%' /><hr width='50%' /><a href='index.php'>>Go Home</a><hr width='50%' /><?php
$h->endpage();

 

Well "secure enough" is something that is really your opinion on whether it is or not as you can go crazy with securing your code. The biggest thing you want to do though is to validate the user input (such as the $_GET variable(s)) as this is the most common security problem. You're just about there, but you're not validating whether $_GET['st'] is a number or not in order to run line 15 appropriately. So what I would do is replace lines 11-14 with this:

 

if(!$_GET['st'] || !is_numeric($_GET['st']))
{
 $_GET['st'] = 0;
}

 

For reference on the is_numeric function: http://php.net/manual/en/function.is-numeric.php

Would recommend using ctype_digit() over is_numeric() in this case.

is_numeric() allows negative and decimal input, ctype_digit() does not.

However, there are 2 slight drawbacks to this - is_numeric() is marginally faster, and the int/float must be passed (or typecasted) to a string.

Luckily, with it being getdata, it's automatically casted internally as a string even though a var_dump() will say otherwise

Edited December 2, 2014 by Magictallguy
Edited code, fixed a bug

[The Phantom]

Hey thanks Magictallguy! But I have a problem? All it shows is Hospital and Pages: 0 nothing else

[G7470]

[MENTION=69409]The Phantom[/MENTION] my advice to you is if you don't understand the code that was posted, then don't use it.

Yes, the number of lines was reduced, but at what cost? If you as the developer do not understand your code, then how can you fix any bugs that arise? You can't. So keep that in mind.

~G7470

[Magictallguy]

Hey thanks Magictallguy! But I have a problem? All it shows is Hospital and Pages: 0 nothing else

Original code updated. The main issue was the

return null

. I've removed that call and used a sort of hacky workaround for it.

Tested and fully working :)