IDP-634 Use cookies not session to store token #154
Conversation
|
I'm a little concerned about this approach, because if we're setting it in a cookie that means it's getting passed back and forth with every web request to that host. We might have already been doing that (we probably were... it's the user's access token, after all), but does this mean we will now be including it twice in the request? I'm concerned that there may be leftover code that will make debugging harder if we just switch this from sessionStorage to a cookie: there are probably other code changes needed in order to update our overall approach to be based on storing (and passing) this in a cookie, rather than however our code was doing so before. Don't get me wrong: This is a great start. It just might not be complete. |
| init() | ||
|
|
||
| export default { | ||
| key: () => get('key'), | ||
| authzHeader: () => `${get('token_type')} ${apiToken()}`, | ||
| reset, | ||
| accessToken: () => get('access_token'), | ||
| setAccessToken: (t) => sessionStorage.setItem('access_token', t), | ||
| setAccessToken: (t) => Cookies.set('access_token', t, { expires: 7, path: '/' }), |
There was a problem hiding this comment.
| setAccessToken: (t) => Cookies.set('access_token', t, { expires: 7, path: '/' }), | |
| setAccessToken: (t) => Cookies.set('access_token', t, { expires: 7, path: '/', sameSite: 'strict' }), |
As long as this doesn't prevent api and ui from talking. It works on cover
| } | ||
|
|
||
| function set(key, defaultValue = '') { | ||
| sessionStorage.setItem(key, get(key) || defaultValue) | ||
| Cookies.set(key, get(key) || defaultValue, { expires: 7, path: '/' }) |
There was a problem hiding this comment.
| Cookies.set(key, get(key) || defaultValue, { expires: 7, path: '/' }) | |
| Cookies.set(key, get(key) || defaultValue, { expires: 7, path: '/', sameSite: 'strict' }) |
| sessionStorage.removeItem('token_type') | ||
| sessionStorage.removeItem('access_token') | ||
| Cookies.remove('token_type') | ||
| Cookies.remove('access_token') |
There was a problem hiding this comment.
we should include the same path and sameSite as when we set them.
There was a problem hiding this comment.
Ahh, since('/') is the default we don't really need it.
| } | ||
|
|
||
| function reset() { | ||
| sessionStorage.removeItem('token_type') | ||
| sessionStorage.removeItem('access_token') | ||
| Cookies.remove('token_type') |
There was a problem hiding this comment.
| Cookies.remove('token_type') | |
| Cookies.remove('token_type', { sameSite: 'strict' }) |
For cookies to be (relatively) secure from xss, I think they need to be set by the server and httpOnly. That would mean moving this code over to the server side. Does that seem right or is there something I'm missing. That's what I got from reading this article anyways. |
Yes, that sounds correct. And bonus points for citing Jeff Atwood. 😁 |
|
Thanks for the bonus points. Sveltekit is using httpOnly cookies which is
nice, but I don't quite know how to do it in php/go yet.
…On Fri, May 17, 2024 at 4:40 AM forevermatt ***@***.***> wrote:
For cookies to be (relatively) secure from xss, I think they need to be
set by the server and httpOnly. That would mean moving this code over to
the server side. Does that seem right or is there something I'm missing.
That's what I got from reading this article
<https://blog.codinghorror.com/protecting-your-cookies-httponly/> anyways.
Yes, that sounds correct. And bonus points for citing Jeff Atwood. 😁
—
Reply to this email directly, view it on GitHub
<#154 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQ34VP3ECXYTKAVPTIYGBZLZCUABBAVCNFSM6AAAAABHWLMRIGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJWGAYDCMJVGU>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
--
[image: glyph1 and logo] <https://www.sil.org> *Michael Wilson*
Web Developer
GTIS ITSE
SIL International <https://www.sil.org>
***@***.***
*M:* 319-244-8359
*O:* 0493380960
Australian Central Time
|
|
So what's the next step for this? Was there work being done to try to switch to setting the cookie server-side? (I might be combining separate issues in my mind...) |
Changed