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

[SDK-2155] Default cookie.secure config to the protocol of baseURL #159

Merged
merged 5 commits into from
Dec 7, 2020

Conversation

adamjmcgrath
Copy link
Contributor

@adamjmcgrath adamjmcgrath commented Nov 12, 2020

Description

Default the cookie.secure config to the protocol of the baseUrl config - rather than defaulting it to the protocol of the req.url (which relies on the developer to set trust proxy when behind a proxy)

And a couple of other things that should improve the DX around this setting:

  • if you override cookie.secure to true and baseUrl is http - throw an error, because this will not work (your secure cookies won't be read over http)
  • if you override cookie.secure to false and baseUrl is https - warn the user that the cookie will not be set with the Secure attribute
  • If baseUrl is http and response_mode is form_post - warn the use that this will only work for <2min logins (have also added an FAQ for this)

References

Chrome 2 min exception for Lax+POST https://www.chromium.org/updates/same-site
fixes #145

Testing

  • This change adds test coverage for new/changed/fixed functionality

Checklist

  • I have added documentation for new/changed functionality in this PR or in auth0.com/docs
  • All active GitHub checks for tests, formatting, and security are passing
  • The correct base branch is being used, if not master

@adamjmcgrath adamjmcgrath requested a review from a team as a code owner November 12, 2020 16:00
@adamjmcgrath adamjmcgrath added the review:medium Medium review label Nov 12, 2020
Copy link
Contributor

@panva panva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, i'm leaving some text suggestions.

FAQ.md Outdated Show resolved Hide resolved
FAQ.md Outdated Show resolved Hide resolved
FAQ.md Outdated Show resolved Hide resolved
FAQ.md Outdated Show resolved Hide resolved
FAQ.md Outdated Show resolved Hide resolved
lib/config.js Outdated Show resolved Hide resolved
test/config.tests.js Outdated Show resolved Hide resolved
Co-authored-by: Filip Skokan <panva.ip@gmail.com>
@adamjmcgrath adamjmcgrath requested a review from panva December 3, 2020 17:44
@adamjmcgrath adamjmcgrath merged commit df1d82b into master Dec 7, 2020
@adamjmcgrath adamjmcgrath deleted the baseurl-cookie branch December 7, 2020 13:33
adamjmcgrath added a commit to auth0/nextjs-auth0 that referenced this pull request Dec 8, 2020
@Widcket Widcket mentioned this pull request Feb 5, 2021
FPiety0521 pushed a commit to FPiety0521/Auth0-Next.js-SDK that referenced this pull request Apr 28, 2023
redsky030428 added a commit to redsky030428/autho_next.js that referenced this pull request May 13, 2024
luckyshinystar7 pushed a commit to luckyshinystar7/nextjs-crysdyazandco that referenced this pull request Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BadRequestError: checks.state argument is missing
3 participants