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

implement cookies.unified option to use unified cookie #192

Conversation

maerzhase
Copy link

This PR implements a very basic solution to save authentication cookies in one single unified cookie. This can be used in scenarios where only a single cookie is allowed, e.g. firebase hosting (see #190).

I already tested this version on a deployment with cloud.run + firebase hosting and it works when the cookie is name __session. (as mentioned in their documentation)

I consider this solution the most straightforward approach I could offer—but please point me in the right directions to get his feature merged!

a single unified cookie is required in order to make server side
authentication work in firebase hosting scenarions. since firebase
hosting only allows you to use on single cookie; cookies.signed
will default to false when cookies.unified is set to true

- related to github.com/gladly-team/issues/190
@stphnthiel
Copy link

This would greatly simplify work we do for a project and for which we’re currently using next-firebase-auth Thanks for the work put into this @maerzhase !

@kmjennison
Copy link
Contributor

kmjennison commented Jun 3, 2021

While this looks like a fine workaround for people who need it, I don't think it makes sense to merge into the main package. My main concerns are:

  • Disabling signed cookies is a potential security risk for people who use withAuthUserSSR, where the cookie values are trusted without ID token verification. Edit: opened Do not allow using withAuthUserSSR when cookies are unsigned #195 to prevent developers from making this mistake.
  • The unified option is just a patch to an underlying problem, which is: this package should simply always use a single cookie and allow the user to customize its full name, not just the prefix.

@maerzhase
Copy link
Author

maerzhase commented Jun 4, 2021

@kmjennison I see your concerns. I'd love to make the unified cookie the default behaviour—but I also see this is becoming a larger work package, since how I understand the cookies module it currently only supports storing signature in a separate cookie and I am actually still not sure if signature still makes sense when its stored within the same cookie. I opened the conversation about it in an issue in the cookies module too: pillarjs/cookies#135

@kmjennison kmjennison closed this Jun 27, 2021
@maerzhase
Copy link
Author

@kmjennison a comment on my reply to

which is: this package should simply always use a single cookie and allow the user to customize its full name, not just the prefix.

would have been appreciated 👍

@kmjennison
Copy link
Contributor

@maerzhase Sorry, I didn't mean to ignore you here, just closing to move the discussion to #190. I agree that supporting a single cookie makes sense. Cookie signing still makes sense within a single value (like how JSON web tokens work)—we'd just need to use a cookies library that supports it.

@maerzhase
Copy link
Author

cool! thank's a lot! I will try to offer my support! have a good one!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants