Prepared statements not used in hashtagging?

[SytheAG]

Reading through the code of this addon before I install it:
For example:

 public function getHashTagByContentType($contentType, $contentId)
    {
        $hashTags = $this->_getDb()->fetchAll('
                                SELECT hashtags.*
                FROM xm_hashtags AS hashtags
                WHERE content_type = ' . $this->_getDb()->quote($contentType) . ' and content_id = ' . $this->_getDb()->quote($contentId)
                                );

        return $hashTags;
    }

Why aren't prepared statements used in code like this? The zend quote function is vastly inferior to prepared statements.


Functions like this:

    public function getRecent($limit = 25)
    {
        $hashTags = $this->_getDb()->fetchAll('
                                SELECT hashtags.*
                FROM xm_hashtags AS hashtags
                                GROUP BY content_type, content_id
                                ORDER BY date_posted DESC
                                LIMIT ' . $limit
                            );

        return $this->prepareHashTags($hashTags);
    }

Show a lack of defensive programming. The value of $limit should be an integer at all times? So why not intval($limit).

 

[SytheAG]

In some instances there is literally no santization provided:

       public function guestCanViewNodeByPostId($contentid)
    {
           $row = $this->_getDb()->fetchRow("SELECT cache_value
                                    FROM xf_permission_cache_content p
                                    INNER JOIN xf_thread t on t.node_id=p.content_id
                                    INNER JOIN xf_post po on po.thread_id=t.thread_id and post_id=".$contentid.
                                " where content_type='node' and permission_combination_id=1"
                            );

           if($row){
               if(isset($row["cache_value"])){
                   $cachearray=  unserialize($row["cache_value"]);
                   if(is_array($cachearray) && !empty($cachearray["view"]) && !empty($cachearray["viewContent"]))return true;
               }
           }
           return false;
    }

Even on inputs which are sanitized in other queries...........
($contentid) in this case

I think you should give me a partial refund because I will have to spend a few hours fixing all the security problems in this addon.

 

[Xon]

@SytheAG I inherited this add-on from someone else, but I'll look into fixing that ASAP.

I do agree that prepared statements should be used, but all the inputs are sanitized in the controllers before they hit this backend code.

 

[SytheAG]

@SytheAG I inherited this add-on from someone else, but I'll look into fixing that ASAP.

I do agree that prepared statements should be used, but all the inputs are sanitized in the controllers before they hit this backend code.

Click to shrink...

yeah I figured that out because the new code in hashtag_watch is all prepared statements.

yes please go through and do a security audit on the model code particularly.

 

[Xon]

Latest version migrates this issue, and rebuilds the _getRecentTrends() to be type-safe.