The Phantom Posted November 13, 2014 Posted November 13, 2014 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(); ?> Quote
G7470 Posted November 13, 2014 Posted November 13, 2014 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 Quote
The Phantom Posted November 13, 2014 Author Posted November 13, 2014 [MENTION=70485]G7470[/MENTION] - Thanks man! Just learnt something new :D Quote
Magictallguy Posted November 13, 2014 Posted November 13, 2014 (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 December 2, 2014 by Magictallguy Edited code, fixed a bug Quote
The Phantom Posted November 14, 2014 Author Posted November 14, 2014 Hey thanks Magictallguy! But I have a problem? All it shows is Hospital and Pages: 0 nothing else Quote
G7470 Posted November 14, 2014 Posted November 14, 2014 [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 1 Quote
Magictallguy Posted December 2, 2014 Posted December 2, 2014 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 :) Quote
Recommended Posts
Join the conversation
You can post now and register later. If you have an account, sign in now to post with your account.