We have moved our forum to GitHub Discussions. For questions about Phalcon v3/v4/v5 you can visit here and for Phalcon v6 here.

Is this safe?

Can someone tell me if this code is safe?

if(!empty($tags)){

            $tags = explode(',',$tags);
            $tags = array_unique($tags);

            foreach($tags as $tag){
                if (!ctype_alnum($tag)) {
                    break;
                }
                $sql[] = '("'.$blogid.'", "'.$postid.'", "'.$tag.'")';   
            }

            if(count($sql) > 0){
                $this->db->query('INSERT INTO blogs_posts_tags (blogid, postid, tag) VALUES '.implode(",", $sql));
            }

        }
edited Mar '16

Although you have a ctype_alnum check, concatenating values into raw SQL commands is inherently unsafe.

You'd be better off using placeholders:

if(!empty($tags)) {
    $tags = array_unique(explode(',',$tags));
    $n = count($tags);
    if($n>0) {
      $sql = "INSERT INTO blogs_posts_tags (blogid, postid, tag) VALUES ";
      $params = [];
      for($i=0; $i<$n; $i++) {
          $sql.= "(?,?,?),";
          $params[] = $blogid;
          $params[] = $postid;
          $params[] = $tags[$i];
      }
      $sql = substr($sql,0,-1);
      $this->db->query($sql, $params);
    }
}


77.7k
Accepted
answer

This is a slightly improved version by using named placeholders, so blog and post id's wont be repeated:

if(!empty($tags)) {
    $tags = array_unique(explode(',',$tags));
    $n = count($tags);
    if($n>0) {
      $sql = "INSERT INTO blogs_posts_tags (blogid, postid, tag) VALUES ";
      $params = [
        'blog' => $blogid,
        'post' => $postid,
      ];
      for($i=0; $i<$n; $i++) {
          $sql.= "(:blog,:post,:tag".$i."),";
          $params['tag'.$i] = $tags[$i];
      }
      $sql = substr($sql,0,-1);
      $this->db->query($sql, $params);
    }
}


5.7k

Another option, if using Phalcon Models would be to do something like this:

/** 
* Get the blog post if you haven't already.
* This assumes that you have a model called "BlogsPosts" with a hasMany relationship to the BlogsPostsTags Model
**/
$blogPost = BlogsPosts::findFirstByBlogPostId($postid);

if(!$blogPost){
    // Couldn't find the record, do something here...
    // Maybe return something?
}

// If tags exist, lets add them
if(!empty($tags) ){

    // Create an empty array that will hold the tags
    $tagsArray = [];

    // Loop through each tag and add them to the array
    foreach($tags as $key => $tag)
        $tagsArray["{$key}"] = new BlogsPostsTags();
        $tagsArray["{$key}"]->setTag($tag);
    }

    // This will set the tags for the post.
    $blogPost->blogsPostsTags = $tagsArray;

}   
// ... Do whatever else may need to be done. 

// Now save
if(!$blogPost->save()){
    foreach($blogPost->getMessages() as $message){
        // Handle your error messages however you wish. Here is an example using the phalcon flash service
        $this->flash->error($message->getMessage());
    }
}

After looking at the OP code, it seems that a "blog post" will more than likely have a "blog id" since it will probably be linked to a blog entry, storing both the BlogID and PostID with each tag may be redundant and unnecessary. You may be able to normalize the structure so that each tag only has a "post id" along with the tag (and maybe a unique identifier depending on your table structure)

I strongly recommend using Steven's method of using Models. Even if it seems like a lot of effort, using models greatly reduces the number of security bugs possible and will make everything else a lot simpler too.

I dont seem to get Stevens code to work

I have two tables

BlogsPosts BlogsPostsTags

But in the code you are just looping the tags, and setting the array, without doing anything with it. I dont see any save() function for the tags and why do you have a setTag() function, what should that function be doing ?

What im trying to do is to create one post and then loop all the tags and save each tag inside the BlogsPostsTags..

Thanks for trying to help me out, it really means a lot :-)!

Models are mapped to db tables, that's why save() does all the work. Check out the docs: https://docs.phalcon.io/en/latest/reference/models.html

The only drawback with this is that you can't execute a single insert query for all you tags, like you did in your example. I use models in my projects, but when multiple rows needs to be inserted I fall back to raw SQL for the sake of performance.

But I need to insert multiple rows at once.. That's why I should use your example Lajos Bencz.

And, i do use models for every other request, but I can't insert multiple rows with models, so like you said, i need to use raw SQL