Jump to content
MakeWebGames

Recommended Posts

Posted

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();
?>
Posted

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

Posted (edited)
  • 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 by Magictallguy
Edited code, fixed a bug
Posted

[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

  • Like 1
  • 3 weeks later...
Posted
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 :)

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

×
×
  • Create New...