Cphalcon: Security tokens error

[emiliodeg]

Well I think this is an error

echo $this->security->getSessionToken();
echo $this->security->getToken();
echo $this->security->getSessionToken();
echo $this->security->getToken();

//expeted output
RjlLK2k0bEZSd2hadThscnp3bWMwUT09 
bXdDMmx6cWZyMTRXdWF3SVdXc3VVUT09 
RjlLK2k0bEZSd2hadThscnp3bWMwUT09 
bXdDMmx6cWZyMTRXdWF3SVdXc3VVUT09 

//real output
RjlLK2k0bEZSd2hadThscnp3bWMwUT09 
bXdDMmx6cWZyMTRXdWF3SVdXc3VVUT09 
bXdDMmx6cWZyMTRXdWF3SVdXc3VVUT09 <<--- here getToken() rewrite getSessionToken()
bXdDMmx6cWZyMTRXdWF3SVdXc3VVUT09 

getToken function https://github.com/phalcon/cphalcon/blob/master/phalcon/security.zep#L354
getToken() should not change the value of getSessionToken() until the next request

I think this error generates the misunderstanding that makes programmers confuse us also removes the possibility of using the Validator Identical in our forms assigning the accepted value getSessionToken()

What do you think?

[sayonara90s]

I have same problem with this too. v3.0.1

I'm beginner, CSRF looks like easy in Phalcon, but not easy.

[temuri416]

Looks like you've come across the same issue:

https://forum.phalconphp.com/discussion/8093/csrf-problems-on-206#C21945

[emiliodeg]

@temuri416 yes you are right, here some other examples

//some action view in volt 
{{ dump(security.checkToken(null, null, false)) }}
{{ security.getSessionToken() }}
{{ security.getToken() }}//here sessionToken was rewrited
{{ security.getSessionToken() }}
{{ security.getToken() }}
{{ dump(security.checkToken(null, null, false)) }}

<form action="" method="post">
    <input type="text" name="{{ security.getTokenKey() }}" value="{{ security.getToken() }}">
    <input type="submit">
</form>

//ouput
boolean true
aVAza2FRMy9ydzVqdGgvdU9GQTMrQT09
YTBuUlRpMGtBTlYwc0ZVeHdJakc5UT09
YTBuUlRpMGtBTlYwc0ZVeHdJakc5UT09
YTBuUlRpMGtBTlYwc0ZVeHdJakc5UT09
boolean false 

//other example
{{ dump(security.checkToken(null, null, false)) }}
{{ dump(security.checkToken()) }}
{{ dump(security.checkToken()) }}
{{ security.getSessionToken() }}//empty!!!
{{ security.getToken() }}//new token was generated

boolean true
boolean true
boolean false
----EMPTY SESSION TOKEN----
YTBuUlRpMGtBTlYwc0ZVeHdJakc5UT09

Thats works fine at first time but an getter modifying another getter it's an strange behavior
I think we must change this getToken() behavior

[sergeyklay]

@emiliodeg Could you please provide the Phalcon version, OS, and way you initialized the Security service. I'll try to sort out

[mbrostami]

Consider that we have already a token in session e.g. aaa

1. echo $this->security->getSessionToken(); // returns STORED value "aaa"
2. echo $this->security->getToken(); // new value stores in session "bbb"
3. echo $this->security->getSessionToken(); // returns STORED value "bbb"
4. echo $this->security->getToken(); // returns generated value in step 2 "bbb"

In this scenario by default session is storing in a file that we read that, write that, and again read that. So it is expectable that after modifying file(session) content in step 2, we read the new content of file in step 3. For better understanding look at the below code:

1. echo $_SESSION['token']; // returns "aaa"
2. $generatedToken = ($generatedToken ?: $_SESSION['token'] = "bbb"); // $generatedToken is now "bbb"
     "bbb" is a random
3. echo $_SESSION['token']; // returns "bbb"
4. $generatedToken = ($generatedToken ?: $_SESSION['token'] = "ccc"); // $generatedToken is "bbb" because already generated

Same issue in the second example:

1. {{ dump(security.checkToken(null, null, false)) }} // returns true
2. {{ security.getSessionToken() }} // returns "aaa"
3. {{ security.getToken() }} // Modifies session content : "bbb"
4. {{ security.getSessionToken() }} // returns "bbb"
5. {{ security.getToken() }} // returns "bbb" already generated in step 3
6. {{ dump(security.checkToken(null, null, false)) }} // returns false because posted token is "aaa" that compares with "bbb"

And the last example:

1. {{ dump(security.checkToken(null, null, false)) }} // returns true
2. {{ dump(security.checkToken()) }} // returns true but removes token from session content(file storage)
3. {{ dump(security.checkToken()) }} // returns false because posted token is "aaa" that compares with null
4. {{ security.getSessionToken() }} // there is no token in session storage we removed that in step 2
5. {{ security.getToken() }} // new token stores in session storage, "bbb"

At last it is up to you that when and how to use these functions.

[emiliodeg]

Well it isn't a bug is a wrong way to do a simple work
session token must not be modified in current request

PHP 7.0.14
Phalcon 3.0.2
Windows 7

Sessin service config

$di['session'] = function () use ($config) {
    $session = new Files(['uniqueId' => $config->session->unique_id]);
    $session->start();
    return $session;
};

[emiliodeg]

If someone needs a more consistent operation here a small change in the logic of the security component

namespace PhalconFix;

class Security extends \Phalcon\Security
{
    private static $sessionToken = null;

    /**
     * {@inheritdoc}
     */
    public function getSessionToken()
    {
        if (self::$sessionToken === null) {
            self::$sessionToken = parent::getSessionToken();
        }
        return self::$sessionToken;
    }


    /**
     * {@inheritdoc}
     */
    public function getToken()
    {
        if (self::$sessionToken === null) {
            $this->getSessionToken(); //don't lose real session token, setup!
        }
        return parent::getToken(); //continues normally
    }
}

//services setup same as always
$di['security'] = function () {
    $security = new Security();
    $security->setWorkFactor(12);/*ohm no fluid setters....*/
    return $security;
};

//Now works like a charm
//anywhre in your code, here in volt

{{ security.getToken() }}
{{ security.getSessionToken() }} 
{{ security.getToken() }}
{{ security.getSessionToken() }}
{{ security.getToken() }}

SVpSaGd2S3E4cUV5ZXczMTZra0d5dz09
UFV1S1c0eWFxc3VOQk90RnJyTUcyQT09 
SVpSaGd2S3E4cUV5ZXczMTZra0d5dz09
UFV1S1c0eWFxc3VOQk90RnJyTUcyQT09 //nice work!!! didn't rewrite
SVpSaGd2S3E4cUV5ZXczMTZra0d5dz09

Have a nice new year!

[mbrostami]

Actually If I know your mean, you need a new feature e.g. getRequestToken() to return token for current request, because after the second call security.getToken() token has been changed and getSessionToken returns the current value of token NOT the old one.
The method getSessionToken has two different meaning,
1- get token value from session
2- get token value for current session (or current request!)
I think the first is correct so we need NFR, otherwise you are right and I suggest to name that method getRequestToken().

[emiliodeg]

Hi @mbrostami I think methods without parameters must have a only one meaning
A new feature request with getRequestToken() could be the solution but the getSessionToken functionality is "strange"

[gguridi]

I think the problem is just a misunderstanding. The method getToken not only generates a new token but sets it into the current session. This leaves the developer with the responsibility of keeping the previous token before calling this method and calling the checkToken method before that.

For me, the request token is only important during the validation (because in the post we are receiving the old one), otherwise is fine that when we call getToken again the session token changes. As soon as we regenerate the token we should start using it as the current session one.

I've added a pull request with a change proposal so the checkToken functionality doesn't use the regenerated session but uses the one that was set at the beginning if it existed.

[Jurigag]

Well since we already got a consensus about what we should do here this issue is NFR and should aim 3.1.0 with PR #12518.

[CameronHall]

This was resolved in this PR #13642.