Support for refresh token in cookie #199
Conversation
|
The whole point of a refresh token is to have a way to get the token after refreshing the page so you dont have to save the token in the localStorage or as a cookie which are both vulnerable to attacks from hackers. if you dont store the refresh token in a HttpOnly cookie its still vulnerable from attacks from hackers which would defeat the entire point of a refresh token and make the bundle useless. I say that it should be stored in a cookie by default and that there should be no other options for security reasons. |
|
Wow great work @lukacovicadam 👍 |
|
Very important feature, I can't wait to use it |
|
@markitosgv i have tested it and use the fork actually on my projects, all is functional. |
|
@lukacovicadam could you please fix style issues in order to merge it to master and make a release? thanks |
|
@markitosgv Sure, but in StyleCI diff I see marked lines, which I didn't edit or add. Should I fix it anyway? Or could you fix it like in #194 ? |
|
@markitosgv the Lukacovicadam's proposition seems fair to me, are you agree ? |
|
Shouldn't we encrypt or sign refresh token, so that hackers can't try random values without app secret? |
|
Are there any plans to merge this sometime? If I understand correctly, using HttpOnly cookie is in theory the safest way possible? So this would be a very welcome feature. |
|
I agree. Is something blocking the merge? |
|
@markitosgv @lukacovicadam Sorry guys, I don't understand how this important feature is complete but not merged due to code style issues nobody seems willing to fix ¯_(ツ)_/¯ |
| $refreshTokenString = isset($params[$tokenParameterName]) ? trim($params[$tokenParameterName]) : null; | ||
| } elseif (null !== $request->get($tokenParameterName)) { | ||
| $refreshTokenString = $request->get($tokenParameterName); | ||
| } elseif (null !== $request->cookies) { |
There was a problem hiding this comment.
| } elseif (null !== $request->cookies) { | |
| } elseif ($request->cookies->has($tokenParameterName)) { |
|
Hello @markitosgv @lukacovicadam I have created a pull request to fix the current master version (#224). If you want I could create a new PR from this branch - https://github.com/morawskim/JWTRefreshTokenBundle/tree/cookie If there are some other issues let me know. |
|
Hello, |
|
Hi @markitosgv , is there possibility to merge my PR, please? |
|
Good stuff ! It will definitely remove my hacky code :) |
| } elseif (null !== $request->get($tokenParameterName)) { | ||
| $refreshTokenString = $request->get($tokenParameterName); | ||
| } elseif (null !== $request->cookies) { | ||
| $refreshTokenString = $request->cookies->get($tokenParameterName); |
There was a problem hiding this comment.
This part needs to be rework with the new extractor implementation
|
FYI, I have opened a new PR which is basically the result of cherry-picking these changes on top of the current master, resolving the conflicts that emerge, and adapting it to the new extractor implementation. It works locally and passes CI tests. |
|
Thank you @Jayfrown . I'm closing this PR in favor of yours. |
13 participants
Hi @markitosgv
I created support for refresh token in cookie. Cookie is also automatically extracted. Cookie is by default httpOnly, thus cannot be read by JS and it's safe against XSS attack.