Security::checkToken() returns true with missing input

[mryellow]

checkToken() is the same as issue #1912 (returns true on blank input), which makes some sense as not all methods are always protected by CSRF tokens. However means you have to first check for the $PHALCON/CSRF/KEY$ session var before hitting checkToken() if you wish to define certain controller/actions as requiring it. Otherwise checkToken() just returns true when given the missing session data with the expectation it will check everything. Some cases around building tokens into GET requests as querystrings, combined with remember-me cookie token auto-login, then hitting them with expired session. The querystring token passes as the session token is missing. Resubmitting a POST after session expiry is probably much the same.

Probably functioning as designed and easy to implement the proper flow for enforcing the protection. Figured it's worth mentioning.

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

[mryellow]

My work-around with some added code for locking down specified GET routes.
checkToken overload first checks for session token existing. If this method has been called then one is expected to exist. Lot of non-framework features here but figure it shows context and someone else might find it handy.

<?php
/**
 * Security component extension of \Phalcon\Security
 * @todo Multi key session https://github.com/ustasby/phalcon-class/blob/master/Token.php
 */

/**
 * @package Utils\Security
 */
namespace Utils\Security;

/**
 * Security class adds checks for missing token and config specified GET requests.
 */
class Security extends \Phalcon\Security
{

    /**
     * Force CSRF tokens on these controllers/actions. Most often GETs.
     *
     * @var array An array of controllers/actions i.e. array('controller'=>array('action1','action2'))
     */
    private $_forced;

    /**
     * Exempt from CSRF tokens on these controllers/actions. Most often POSTs.
     *
     * @var array An array of controllers/actions i.e. array('controller'=>array('action1','action2'))
     */
    private $_exempt;

    /**
     * Set CSRF token forced controllers/actions.
     *
     * @param string $routes An array of controllers/actions i.e. array('controller'=>array('action1','action2'))
     */
    public function setForced($routes)
    {
        if (is_array($routes)) $this->_forced = $routes;
    }

    /**
     * Set CSRF token exempt controllers/actions.
     *
     * @param string $routes An array of controllers/actions i.e. array('controller'=>array('action1','action2'))
     */
    public function setExempt($routes)
    {
        if (is_array($routes)) $this->_exempt = $routes;
    }

    /**
     * Is the controller/action forced.
     *
     * @return bool
     */
    public function isForced()
    {

        $controller = $this->getDi()->getRouter()->getControllerName();
        $action = $this->getDi()->getRouter()->getActionName();

        if (isset($this->_forced[$controller])) {
            if (in_array($action, $this->_forced[$controller])) {
                return true;
            }
        }
        return false;
    }

    /**
     * Is the controller/action exempt.
     *
     * @return bool
     */
    public function isExempt()
    {

        $controller = $this->getDi()->getRouter()->getControllerName();
        $action = $this->getDi()->getRouter()->getActionName();

        if (isset($this->_exempt[$controller])) {
            if (in_array($action, $this->_exempt[$controller])) {
                return true;
            }
        }
        return false;
    }

    /**
     * Retrieve the session token key
     *
     * @return string
     */
    public function getSessionTokenKey()
    {
        return $this->getDi()->getSession()->get('$PHALCON/CSRF/KEY$');
    }

    /**
     * Check the session token key exists
     *
     * @return bool
     */
    public function hasSessionTokenKey()
    {
        return $this->getDi()->getSession()->has('$PHALCON/CSRF/KEY$');
    }

    /**
     * Check for CSRF on POSTs and 'protected' GETs, including missing tokens from session expiry.
     *
     * @returns bool
     */
    public function checkAll()
    {

        $controller = $this->getDi()->getRouter()->getControllerName();
        $action = $this->getDi()->getRouter()->getActionName();

        // Check exempt list
        if ($this->isExempt()) return true;

        // POST request
        if ($this->getDi()->getRequest()->isPost()) {
            // Always forced on POSTs unless exempted above
            return $this->checkToken();

        // GET Request
        // Check forced list
        } else if ($this->isForced()) {
            // Retrieve the tokenKey from session if it exists
            $tokenKey = $this->getSessionTokenKey();
            return $this->checkToken($tokenKey, $this->getDi()->getRequest()->getQuery($tokenKey, 'string'));
        }

        // Always exempt on GETs unless forced.
        return true;
    }

    /**
     * Overload of token check to include failure on missing session token.
     *
     * @param string $tokenKey
     * @param string $token
     *
     * @returns bool
     */
    public function checkToken($tokenKey = NULL, $token = NULL)
    {
        // Token must be set on forced actions, expired session will be without it, see issue #15 #22 and https://github.com/phalcon/cphalcon/issues/2596
        if (!$this->hasSessionTokenKey()) return false; // No checked request will be allowed without a tokenKey in session
        return parent::checkToken($tokenKey, $token);
    }

}

[stale]

Thank you for contributing to this issue. As it has been 90 days since the last activity, we are automatically closing the issue. This is often because the request was already solved in some way and it just wasn't updated or it's no longer applicable. If that's not the case, please feel free to either reopen this issue or open a new one. We will be more than happy to look at it again! You can read more here: https://blog.phalconphp.com/post/github-closing-old-issues