[RTM] Store the request token as cookie instead of the session #1065
Conversation
ausi
force-pushed
the
feature/double-submit-cookie
branch
2 times, most recently
from
a0b0742
to
93b65a0
Compare
src/Csrf/MemoryTokenStorage.php
Outdated
| * | ||
| * @return array | ||
| */ | ||
| public function getSaveTokens() |
There was a problem hiding this comment.
I don't like this method name, shouldn't it be getActiveTokens if the internal variable is called $activeTokens? Also, I would prefer to name that usedTokens 😁
The method can be simplified like this:
return array_intersect_key($this->tokens, $this->activeTokens);| * @param int $cookieLifetime | ||
| * @param string $cookiePrefix | ||
| */ | ||
| public function __construct(MemoryTokenStorage $tokenStorage, int $cookieLifetime = 86400, string $cookiePrefix = 'csrf_') |
There was a problem hiding this comment.
Does this have to be a MemoryTokenStorage? Shouldn't we expect the interface?
There was a problem hiding this comment.
We call the initialize() method on it, so we cannot use the interface.
| $tokens = []; | ||
|
|
||
| foreach ($cookies as $key => $value) { | ||
| if (strncmp($key, $this->cookiePrefix, strlen($this->cookiePrefix)) === 0) { |
There was a problem hiding this comment.
strpos($this->cookiePrefix, $key) === 0 ?
There was a problem hiding this comment.
strncmp() should be way faster IMO.
src/Resources/config/listener.yml
Outdated
| - "@contao.csrf.token_storage" | ||
| tags: | ||
| - { name: kernel.event_listener, event: kernel.request, method: onKernelRequest, priority: 20 } | ||
| - { name: kernel.event_listener, event: kernel.response, method: onKernelResponse, priority: 0 } |
There was a problem hiding this comment.
you don't need to set a priority if it's 0.
ausi
changed the title
| $isSecure = $event->getRequest()->isSecure(); | ||
|
|
||
| foreach ($this->tokenStorage->getUsedTokens() as $key => $value) { | ||
| $event->getResponse()->headers->setCookie( |
There was a problem hiding this comment.
I think you should use deleteCookie if the value is null. This way Symfony handles the unset case.
There was a problem hiding this comment.
deleteCookie needs the same arguments as setCookie, except value and lifetime. Using deleteCookie would make a block of duplicate code.
There was a problem hiding this comment.
but it would move the delete logic to the header bag instead of our class, right?
There was a problem hiding this comment.
If you refer to $value === null ? 1 : $cookieLifetime as the delete logic, then yes.
leofeyer
changed the title
leofeyer
force-pushed
the
develop
branch
5 times, most recently
from
ed0a7e5
to
3dadd44
Compare
leofeyer
force-pushed
the
feature/double-submit-cookie
branch
2 times, most recently
from
0e0a15f
to
b142160
Compare
leofeyer
force-pushed
the
feature/double-submit-cookie
branch
from
b142160
to
e27ac94
Compare
Use a double submit cookie instead of storing the request token in the session of the user.