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: add setting to use cross-origin cookie for "detectBrowserLocale" #853

Merged
merged 9 commits into from
Aug 23, 2020

Conversation

lucianholt97
Copy link

change locale cookie options sameSite to none
-> allow setting cookie in iFrame integration
closes #852

change locale cookie options sameSite to none
allow setting cookie in iFrame integration
@rchl
Copy link
Collaborator

rchl commented Aug 19, 2020

It doesn't seem like None a good default in general. See this https://web.dev/samesite-cookie-recipes/#how-to-implement-samesite-today

I think it would need to be an option instead so one can customize if needed.

@lucianholt97
Copy link
Author

An option would also work for me. But it is a common issue and I don‘t see a problem using SameSite=None and secure since the information in the cookie is not critical.
What is critical though is, that it doesn‘t work at all in the current configuration within an iFrame context.

@rchl
Copy link
Collaborator

rchl commented Aug 19, 2020

I don't know how None would behave (would have to check), but it would probably make the browser not read the cookie when visiting the site from external link (like a mail for example). That would mean that the cookie would be missing and user preference regarding language would not be respected.

(But as I said, I haven't experimented with None yet so not sure).

@lucianholt97
Copy link
Author

That’s not the case. Actually, none is the most universal option and won‘t restrict the application of the cookie at all. It is only important to prevent none when storing sensible information in the cookie which must not be leaked to other sites. Having none allows other domains to read the cookie, too.

@lucianholt97
Copy link
Author

But I just discovered that the linked PR is incomplete. There is still some validation in place which does not allow the none setting and throws an error.

@divine
Copy link

divine commented Aug 19, 2020

Sorry to write it here but iFrame with a url to the non-default language could be easily handled when #799 will be ready as I understand it correctly.

Instead of adding another option or changing cookie settings there is already a PR that handles your case so basically we should wait for it.

Thanks.

@lucianholt97
Copy link
Author

@divine yes, it sounds similar, but I don‘t see this option as a solution for my needs. I don‘t want to disable redirects at all, I just want it to work in an iFrame and therefor the cookie settings need to be changed.

@rchl
Copy link
Collaborator

rchl commented Aug 19, 2020

I've read up on SameSite and I think it would be fine to use SameSite=None. The cookie we are talking about here is not security or session-related so it doesn't pose a risk when sent from third-party sites.

The only thing is that SameSite=None is fairly new so if we only send this one, an older browser versions will not understand it. The good thing is that they should default to Lax then but that can trigger a warning in the developer console.

@lucianholt97
Copy link
Author

But I just discovered that the linked PR is incomplete. There is still some validation in place which does not allow the none setting and throws an error.

@rchl thank you for checking. I have to correct myself, the PR is working. I had a local issue where another package required cookie@0.3 while the cookie package only supports samesite=none from 0.4. But the deps in this repo are correct, so this was only a specific issue for me.
Therefore I look forward to you merging the PR. Thanks in advance!

@divine
Copy link

divine commented Aug 19, 2020

@divine yes, it sounds similar, but I don‘t see this option as a solution for my needs. I don‘t want to disable redirects at all, I just want it to work in an iFrame and therefor the cookie settings need to be changed.

I don't think your PR should be merged at all. Here is a reason from web.dev.

For cookies needed in a third-party context, you will need to ensure they are marked as SameSite=None; Secure. Note that you need both attributes together. If you just specify None without Secure the cookie will be rejected.

Anyways, what you're trying to achieve doesn't require cookie change, you should await another PR which is definitely what you're looking for.

Thanks!

@rchl
Copy link
Collaborator

rchl commented Aug 19, 2020

Yes, using None requires Secure.
So that probably would break some use cases.
Probably best to just provide an option.

@divine Those are separate issues at the root because here the cookie is always ignored, regardless if visiting default or non-default locale.

@divine
Copy link

divine commented Aug 19, 2020

@divine Those are separate issues at the root because here the cookie is always ignored, regardless if visiting default or non-default locale.

@rchl well, that was what I've understand from that message.

Integrate nuxt website in an iFrame with a url to the non-default language.
Due to the lax cookie setting, the browser will block the cookie and somehow nuxt-i18n ignores the url and displays the default locale

Basically, another PR could probably resolve this, that's why I'd mentioned sorry to write it here :)

Thanks!

@lucianholt97
Copy link
Author

You‘re right, secure is missing. But the issue is not only about non-default locales, its more general.
I can see that secure could break some use cases, so an option would be the best solution.

add secure option to cookie
add option crossOriginCookie to enable sameSite: none and secure: true cookie settings
pass crossOriginCookie to setLocaleCookie
add default for crossOriginCookie
add crossOriginCookie to type definition
add crossOriginCookie to docs
@lucianholt97
Copy link
Author

@rchl I added an option to enable the cross-origin cookie. I would appreciate if you can review the changes.

@rchl
Copy link
Collaborator

rchl commented Aug 20, 2020

Can you also update es/browser-language-detection.md with same (English) text?

src/templates/utils-common.js Outdated Show resolved Hide resolved
remove bool casting
update es docs for browser language detection
@lucianholt97
Copy link
Author

@rchl all done, thanks for the comments!

@divine
Copy link

divine commented Aug 23, 2020

Basically by default cookie has now secure false?

Why should be secure false added to the cookie?

Doesn't make sense.

@rchl
Copy link
Collaborator

rchl commented Aug 23, 2020

@divine secure: false is and was the default value (even if not explicitly specified). So no changes to default.

@rchl
Copy link
Collaborator

rchl commented Aug 23, 2020

@lucianholt97 thank you

@rchl rchl changed the title Change cookie settings to allow cors feat: add setting to use cross-origin cookie for "detectBrowserLocale" Aug 23, 2020
@rchl rchl merged commit e446676 into nuxt-modules:master Aug 23, 2020
@rchl
Copy link
Collaborator

rchl commented Aug 27, 2020

Note that I'm renaming this option from crossOriginCookie to cookieCrossOrigin for consistency with other options.
And also adding cookieSecure in #869

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.

Cookie not working in iFrame
3 participants