slimphp / Slim-Csrf

Slim Framework CSRF protection middleware
MIT License
338 stars 58 forks source link

Token is not deleted after validation #113

Closed Trekky12 closed 4 years ago

Trekky12 commented 4 years ago

Until version 0.8.3 the used token is removed:

        // If we're not in persistent token mode, delete the token.
        if (!$this->persistentTokenMode) {
            $this->removeFromStorage($name);
        }

(https://github.com/slimphp/Slim-Csrf/blob/0.8.3/src/Guard.php#L254-L257) and a new token is generated after validation:

// Need to regenerate a new token, as the validateToken removed the current one.
$request = $this->generateNewToken($request);

(https://github.com/slimphp/Slim-Csrf/blob/0.8.3/src/Guard.php#L152-L153)

Is there any particular reason why the token is no longer deleted in the latest version for slim 4?

l0gicgate commented 4 years ago

Can you explain to me why does this need to be automatic implied behavior? Why should the validateToken() method do this automatically? This method should be used solely to validate a token imo.

Trekky12 commented 4 years ago

In the current version 1.0.0 a valid token can be used forever (also when persistent mode is not used) and not only for one request (like previously). Is this the desired behaviour? I generally don't think this should be in the validateToken() method, but I noticed that this was the previous behaviour.

l0gicgate commented 4 years ago

@Trekky12 I think automatically deleting the token from the storage upon validation isn't the right behavior, what if you want to validate the token twice in separate places? Following single responsibility principle this method should only return whether or not the token is valid.

I think that we should probably make the removeTokenFromStorage() method public so you can manually remove the token after validating it. @adriansuter thoughts?

Trekky12 commented 4 years ago

@l0gicgate But isn't the persistent mode exactly for that? When the removeTokenFromStorage() method is public maybe a success handler which is called after successfully validation would be also useful (like the failure handler on failure). But I still believe a token should only be valid for one request when persistent mode is not used.

adriansuter commented 4 years ago

What happens, if you set the storageLimit to 1?

Alternative

Is it possible to register a shutdown function (register_shutdown_function()) which automatically removes the old token at the end of the script? That way, a developer can still use the token in multiple places, but not in the next request. Breaking change?

Trekky12 commented 4 years ago

I need multiple valid tokens but each token should be only valid once, so setting the storageLimit to 1 is no solution. A shutdown function could be a possible solution. I can't see a breaking change here.

l0gicgate commented 4 years ago

I think the most flexible thing to do here is to make the removeTokenFromStorage() method public then we offer the flexibility of validating in multiple places, then deleting the token manually.