/    Sign up×
Community /Pin to ProfileBookmark

Hey I’m pretty new to php.

I wrote this module that will add a “like” feature to phpbb and I’m just about to let it go live (been testing on xampp for a while). I know since I am new to php that I don’t have the best understanding of security when it comes to accessing my database and I feel my current code is probably weak. How would you guys improve this?

This is my post like file that stores the like in the database when someone clicks the like button. An ajax query is called and sends the data via the url aka: url/index.php?value=(ValGeneratedByJS).. ect.

[CODE]
<?php
include “like.php”;
$val = $_GET[“value”];
$liker = $_GET[“liker”];
$likee = $_GET[“likee”];
$post = $_GET[“post”];
$topic = $_GET[“topic”];
$forum = $_GET[“forum”];
if(!empty($val) && !empty($liker) && !empty($likee) && !empty($post) && !empty($topic) && !empty($forum)) {
if($liker == $likee)
exit;
$like = new Phpbb_Like($val, $liker, $likee, $post, $topic, $forum);
$like->save(Connect());
}
[/CODE]

This is my retrieve like file it also gets the information via a url appended var: url/thisfile.php?topic=(TopicVa) also applied by JS/jQuery

[CODE]
<?php
include(“like.php”);

$topic = $_GET[“topic”];

GetLikesBy(“topic”, $topic);
[/CODE]

I think my store like method is not safe but the retrieve should be fine? Really I’m not sure though.

to post a comment
PHP

5 Comments(s)

Copy linkTweet thisAlerts:
@NogDogJan 29.2015 — Since I have no idea what Phpbb_Like::__construct() does nor what Phpbb_Like::save() does, I can't really comment on DB security -- though I'd assume (which may be dangerous?) that if it's built-in phpbb code, then it does the "normal" things any good PHP code should to to prevent SQL injection, which is main thing to be concerned about DB-wise (along with not exposing the DB user/password credentials).
Copy linkTweet thisAlerts:
@LesshardtoofindauthorJan 29.2015 — I thought only the get set pages would be the risks I guess its probably how I query the database though. PHPBB does expose the UN and PW in config.php. I have it setup on xampp right now so the UN PW and DB name are all shown in my code, but they are set as the same name that PHPBB uses so that I can just include my files at their lvl and have access to the database. Here is how I query the data.
[CODE]
public function __construct($value, $liker, $likee, $postname, $topicname, $forumname){
$this->CoreInfo = array(
"value" => $value,
"liker" => $liker,
"likee" => $likee,
"post" => $postname,
"topic" => $topicname,
"forum" => $forumname,
);
}
public function exists(){
global $db;
$con = Connect();
$sql = "SELECT * FROM Likes WHERE Value='" . $this->CoreInfo['value'] . "' AND " .
"Liker='" . $this->CoreInfo['liker'] . "' AND " .
"Likee='" . $this->CoreInfo['likee'] . "' AND " .
"Post='" . $this->CoreInfo['post'] . "' AND " .
"Topic='" . $this->CoreInfo['topic'] . "' AND " .
"Forum='" . $this->CoreInfo['forum'] . "';";
if(mysqli_select_db($con, $db)){
$fetch = $con->query($sql);
if(mysqli_num_rows($fetch) == 0)
return false;
else
return true;
}
return true;
}
public function save($con){
global $db;
$sql = "INSERT INTO Likes (Value, Liker, Likee, Post, Topic, Forum)" .
"VALUES (" . "'" . $this->CoreInfo['value'] . "'" . ", " .
"'" . $this->CoreInfo['liker'] . "'" . ", " .
"'" . $this->CoreInfo['likee'] . "'" . ", " .
"'" . $this->CoreInfo['post'] . "'" . ", " .
"'" . $this->CoreInfo['topic'] . "'" . ", " .
"'" . $this->CoreInfo['forum'] . "'" . ");";
if(mysqli_select_db($con, $db) && !$this->exists()){
if(mysqli_query($con, $sql))
echo "Like Saved";
else
echo "Like failed" . mysqli_error($con);
}
}
[/CODE]


Sorry about not including this information originally. As for assuming it prevents SQL injection like any good php.... I have no idea what that means.. Kind of why I asked lol.

Thanks
Copy linkTweet thisAlerts:
@NogDogJan 29.2015 — Since you are using mysqli (glad you're not using the old mysql extension), I'd like to see you use prepared statements with bound parameters, which would then take care of any/all worries about SQL injection. There are some (rather simple) examples here: http://php.net/manual/en/mysqli.quickstart.prepared-statements.php , but the main idea here is that you use place-holders in the query string for the variable values, then bind the actual values to them via the bind_param() method. bind_param() then handles type conversions and, more importantly, special character escaping for you.
Copy linkTweet thisAlerts:
@LesshardtoofindauthorJan 30.2015 — Thanks for the links. It took me a little bit of rooting around, but I think I got it right.

Here is the new code
[CODE]
public function save(){
$con = Connect();
$stmt = $con->prepare("INSERT INTO Likes (Value, Liker, Likee, Post, Topic, Forum)" .
"VALUES (?, ?, ?, ?, ?, ?);");
$stmt->bind_param("ssssss", $this->CoreInfo['value'],$this->CoreInfo['liker'], $this->CoreInfo['likee'], $this->CoreInfo['post'], $this->CoreInfo['topic'], $this->CoreInfo['forum']);
if(!$stmt->execute())
echo "failed to save";
else
echo "saved";
}
function AGetLikesBy($key, $keyvalue){
global $db;
$con = Connect();
$stmt = $con->prepare("SELECT * FROM Likes WHERE $key = ?");
$stmt->bind_param("s", $keyvalue);
if($stmt->execute()){
$result = $stmt->get_result();
while($row = $result->fetch_array(MYSQLI_NUM)){
foreach ($row as $value) {
echo $value . "-";
}
echo ":";
}
}
}
[/CODE]


Thanks for your help. Does this look safe now?
Copy linkTweet thisAlerts:
@rootFeb 02.2015 — Looks good but I would also add a layer of security on the $_GET inputs, you may have taken care of SQL Injection but not Code injection which happens via $_GET and $_POST arrays.

something as simple as $safe = filter_var($_GET['yourFieldName'], FILTER_SANITIZE_MAGIC_QUOTES ); for example will sanitize that input, something that you can apply to all your inputs, a list of sanitize fileters can be found here, what you NEVER DO is sanitize back in to the $_GET or $_POST arrays, these should always be considered unsafe data sources, you should sanitize in to another array of your own making or in to individual variables, your choice...
×

Success!

Help @Lesshardtoofind spread the word by sharing this article on Twitter...

Tweet This
Sign in
Forgot password?
Sign in with TwitchSign in with GithubCreate Account
about: ({
version: 0.1.9 BETA 6.17,
whats_new: community page,
up_next: more Davinci•003 tasks,
coming_soon: events calendar,
social: @webDeveloperHQ
});

legal: ({
terms: of use,
privacy: policy
});
changelog: (
version: 0.1.9,
notes: added community page

version: 0.1.8,
notes: added Davinci•003

version: 0.1.7,
notes: upvote answers to bounties

version: 0.1.6,
notes: article editor refresh
)...
recent_tips: (
tipper: @nearjob,
tipped: article
amount: 1000 SATS,

tipper: @meenaratha,
tipped: article
amount: 1000 SATS,

tipper: @meenaratha,
tipped: article
amount: 1000 SATS,
)...