the @see tag should be removed everywhere as it does not add anything useful.

i can remove it, but i disagree that it doesnt add anything useful: you see where the original declaration comes from, without doin any clicks in your IDE. you only see, if its an overridden method in IDEs, not who the original definer was (without doin more clicks for inspection). also on github you dont have any code assist regarding inheritance.

all those @see should be removed, they do not add anything useful (the class implements TokenStorageInterface).

Does it makes sense to mark concrete public methods as final?

I think final should only be used in very special cases when you absolutely rely on the implemented behavior and to keep the internal state of the class clean and valid, and this cannot be provided by subclasses (e.g. not being able to access private properties). Both is not the case here. Imagine someone wants to create a decorater that fixes the token ID to a certain value, using this class as a parent would not be possible with final methods.

Should be at variable setting not in constructor.

i think its personal preference, i try to do all property initialization within constructors and cleanly separate it from property declaration.

It seems to be set on properties in symfony core.

@HeahDude is right. And you should use array() instead of [].

Should be '' === $token.

Maybe '' === $token ?

missing () at the end.

Should be 0 < strlen($this->resolveToken($tokenId)).

according to symfony code style guide the reversing only applies to equality and identity checks (and their negations).

I find it much harder to understand when "zero is smaller than something" vs "something is greater than zero"

I missed it the first time :) I guess casting to bool can work too and would be more readable imo.

false === (bool) '0' or do you mean casting the strlen to bool? latter would work, but then again, i thing "the length of the string is greater than zero" is so much more explicit, especially since casting from string to something else is very adventurous in PHP.

I mean the strlen of courses :)

return (bool) strlen($this->resolveToken($tokenId));

This in fact should be '' !== $this->resolveToken($tokenId).

agree with @stloyd

Here could be '' !== $token

What's preventing you to normalize it ?

the question is: is the behavior of Cookie correct to overwrite a zero length cookie value with the value "deleted" (and a past expiration date). opinions on stackoverflow suggest using the empty string value and a past expiration date to "delete" a cookie. (in general a past expiration date should be enough, but browsers are not required to dismiss expired cookies, so in the case the browser decides to submit the cookie anyway, we must detect it as "deleted". But there is no way to distinguish between a "deleted" cookie and a cookie actually containing the value "deleted".)

Is the string "deleted" used anywhere else in the code ?

It would clash, if someone tries to setToken($tokenId, 'deleted') (basically in every use case, where the cookie value "deleted" is in the allowed value set of the cookie)

What would be the reason not to ? IIUC this is about security.

i missunderstood the httponly flag. it must be a cookie that is allowed to be read by the client side scripts, so one could use it in ajax requests headers.

IIRC http only allows reading but prevents writing, so this should be the way to go, right ?

it also disallows reading according to https://www.owasp.org/index.php/HttpOnly#What_is_HttpOnly.3F

Shouldn't it be $this->cookies->get($name, false) to be sure that's the name which is empty ? Or it doesn't matter ?

The empty string token is the non-existant token. Thats because we dont have an explicit "delete" cookie header. If you want to delete a cookie, you need to overwrite a previously existing one. Thats also why it is disallowed to pass an empty string token to setToken (and the empty token makes no sense from a security point of view anyway).

So update can be an alias to remove ? of courses not it is protected

May be you could hold the names by ids in a private array to prevent useless call to generate them.

I'm confused about what's happening when removeToken() is called, doesn't the name generation happen twice ?

the same cookie name should be generated for the same token id within the same CookieTokenStorage instance. so calling it multiple times is perfectly fine.

also the generation is simple and fast and only called very few times during CSRF-protected requests (normally 2 times, if a token exists).

Symfony/Component/Security/Csrf/EventListener/ ?

Was speculating about, but then i saw that every other kernel listener sits in the EventListener namespace of the HttpKernel component.

I really think Symfony/Component/Security/Csrf/EventListener is fine

In the TokenStorageInterface ?

managing a token storage within the request attributes is something specific to the RequestStackTokenStorage, not something a TokenStorageInterface must do in general. but you can use the CookieTokenStorage and CookieTokenStorageListener without RequestStackTokenStorage, if you handle the management yourself.

But any TokenStorage might need this constant eventually, right ?

only code that maintains (access or set) a token storage within the request attributes

I don't understand, I mean that any class implementing TokenStorageInterface might need a default storage key so it could make sense to hold this constant in the interface.

well its also not specific to TokenStorageInterface, but to code managing TokenStorageInterfaces within higher aggregates. the managing code can be a TokenStorageInterface itself (like the RequestStackTokenStorage), but doesnt need to be (like the CookieTokenStorageListener). its also just a default key, if no explicit one is passed to the managing code. each class could provide its own default key, but it would be handy, if every implementation uses the same default, especially when they are meant to be used together (like the RequestStackTokenStorage and the CookieTokenStorageListener).

It make more sense to me to move this constant in the TokenStorageInterface anyway as it could be a default key used by any object needing to identify a TokenStorageInterface instance and interact with it.

On this second round I agree with you :)

You agree on putting an own constant on CookieTokenStorageListener? That means the contant values need to be synced by hand and changed in both places.

Sorry, I've missed that duplication, it was not there at the time of my original comment there was only one implementation using it. So now I guess it really makes sense to have this constant in the interface instead, don't you think?

This return type is not needed when "void".

yep. CS fixer will remove it. I didnt cleaned up the code yet. will do, when i finish it.