Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): support server set cookies #333

Closed
wants to merge 5 commits into from

Conversation

MathiasCiarlo
Copy link
Collaborator

@MathiasCiarlo MathiasCiarlo commented Apr 14, 2019

This fixes the issue that arises when trying to access a protected path directly(the first request to the website). The middleware tries to save the attempted, protected path to storage, so that we can redirect to the path after login. The problem was that the "auth.redirect" cookie was not set when the middleware was run server side. This PR supports cookies set server side and should fix #332, fixes #326 and fixes #134.

Now the server can set cookies in the response.
@MathiasCiarlo MathiasCiarlo changed the title Support server set cookies Support server set cookies (fixes direct navigation to protected routes) Apr 15, 2019
@karimcambridge
Copy link

I also encountered this issue!

@mhafizhasan
Copy link

+1 should be fixed

@smesterheide
Copy link

This is an elegant solution to the problem I am having with users getting logged out because the token has changed server side but is not synced with the client.

@smesterheide
Copy link

smesterheide commented Apr 22, 2019

I have found some potential problems with this PR:

  1. getCookie(setCookie()) is not working as expected
    Breaks the Storage class contract
  2. setCookie(A) && setCookie(B) overwrites A
    Can be fixed by more careful handling of Set-Cookie headers
  3. Cookie options are ignored

I need to pass several cookies back to the client, so I am thinking of setting them in nuxtServerInit instead.

@smesterheide smesterheide mentioned this pull request Apr 22, 2019
10 tasks
@MathiasCiarlo
Copy link
Collaborator Author

MathiasCiarlo commented Apr 28, 2019

I have found some potential problems with this PR:

  1. getCookie(setCookie()) is not working as expected
    Breaks the Storage class contract
  2. setCookie(A) && setCookie(B) overwrites A
    Can be fixed by more careful handling of Set-Cookie headers
  3. Cookie options are ignored

I need to pass several cookies back to the client, so I am thinking of setting them in nuxtServerInit instead.

@sambrezo Thank you for the feedback! I have fixed the overwrite issue, and added options. What do you mean by with getCookie(setCookie()) ? The setCookie function still returns the set value?

@smesterheide
Copy link

Hey @MathiasCiarlo,
that's looking good. I'm gonna test it with the rest of my app this week and let you know if it works for me. Thanks for tackeling the cookie issue!

Regarding 1) I think the issue might still persist. The problem that I see is when you call setCookie while SSR and then read the value again with getCookie you will get the cookie information from the request header and not what you have just written to the response header. Might be an edge case but all other getters of the Storage class provide some kind of persistence which is not guaranteed when he have to deal with two sets of cookies. No 3) is somewhat related in that the cookies should stay in sync when moving server to client and vice-versa. Unfortunately the cookie.options in auth-moduledo not always map onto the values we see in the cookie string. For example expires is used differently in js-cookie.
We also have to keep in mind that we cannot use HTTP enabled cookies cookies server-side because we rely on the client being able to read and write to the cookie via js-cookie. This means we cannot really protect our tokens from XSS when we move forward with this approach.

Would be nice to have one of the other contributors comment on the decision whether to support server cookies at all.

@duyleekun
Copy link

I patch-package this to use this fix immediately. Hope this get merged soon

lib/core/storage.js Outdated Show resolved Hide resolved
lib/core/storage.js Outdated Show resolved Hide resolved
@pi0 pi0 changed the title Support server set cookies (fixes direct navigation to protected routes) feat(core): support server set cookies May 23, 2019
lib/core/storage.js Outdated Show resolved Hide resolved
lib/core/storage.js Outdated Show resolved Hide resolved
@pi0 pi0 mentioned this pull request May 23, 2019
@MathiasCiarlo
Copy link
Collaborator Author

MathiasCiarlo commented May 23, 2019

I've added support for unsetting ssr cookies. The only thing remaining is to think about cookie options (link to docs). In an earlier commit, I did a simple implementation, but @sambrezo told me the options used in js-cookie are a little different than my implementation. Do we have to include options ssr, and if so, please help me out in how we can serialize js-cookie options with the current implementation :)

@pi0
Copy link
Member

pi0 commented May 24, 2019

Hi @MathiasCiarlo. I've proposed #360 to backport universal-storage improved storage into auth module. Would you please have a review on it?

I think the difference with this PR is that here we support multiple Set-Header calls. If you think it is a requirement we can combine these PRs and use getCookiesFromResponseHeader/setResponseHeaderCookies utils.

@MathiasCiarlo
Copy link
Collaborator Author

Great @pi0, I'll have a look tonight :)

@MathiasCiarlo
Copy link
Collaborator Author

@pi0 Have looked at your PR. Many nice changes, but we definitely need the multiple Set-Header calls. How can we combine the PRs? Can I raise a new PR, branched out from your branch? :)

@pi0
Copy link
Member

pi0 commented May 30, 2019

Thanks, everyone for patientnce and helping on this issue. Sever set cookie is added in v4.6.0 via #360.

@pi0 pi0 closed this May 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants