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

Respect SameSite in Auth Cookie #188

Merged
merged 1 commit into from
Feb 8, 2021
Merged

Respect SameSite in Auth Cookie #188

merged 1 commit into from
Feb 8, 2021

Conversation

madaster97
Copy link
Contributor

Description

This PR addresses issue #187 by dynamically using the config.session.cookie.sameSite cookie attribute on the auth_verification cookie ONLY when the response mode is not form_post.

Prior to this, any response mode other than form_post would set the auth_verification cookie (which contains state, nonce and PKCE values) with the None SameSite attribute, and would otherwise default to Lax.

The main use case is for non- form_post response type flows, where the RP would like the auth cookie to have a SameSite value of None or Strict, rather than the forced default of Lax.

Docs on the Setting

The language (see docs) around the SameSite field states:

Defaults to "Lax" but will be adjusted based on {@link AuthorizationParameters.response_type}.

It seems the docs above actually meant to mention response_mode, if that's the case, we should address that here as well.

From that language, I belive the intended behavior was that:

  1. The appSession cookie will always use the configured sameSite value
  2. The auth_verification cookie will:
    • use SameSite=None for form_post response types, ignoring config
    • Otherwise respect the configured sameSite attribute

Potential Security Concern

Up to this point, devs may be relying on the default of Lax for response modes other than form_post. TBD: What are the security considerations for changing that default behavior, given that:

  1. The default for config.session.cookie.sameSite is also Lax
  2. We would only ever use another SameSite value if it was intentionally set in the config, and already be in use for the appSession cookie.

References

Testing

I added tests to cover what I believe is the expected behavior (see above).

  • This change adds test coverage in the login.tests.js file (see last two tests)
    • Test that form_post response modes ignore config and use None
    • Test that other response modes respect configured SameSite attribute

Checklist

  • I have added documentation for new/changed functionality in this PR or in auth0.com/docs
    • ???: See above for potential doc change. I'd like someone to review the SameSite config docs to make sure I'm interpreting it correctly (response_type is mentioned, but should it be response_mode?).
  • All active GitHub checks for tests, formatting, and security are passing
    • ???: All tests are passing and linting looks good, but what about security testing?
  • The correct base branch is being used, if not master

@madaster97 madaster97 requested a review from a team as a code owner February 7, 2021 20:36
CHANGELOG.md Show resolved Hide resolved
@@ -16,18 +16,24 @@ const filterRoute = (method, path) => {
r.route && r.route.path === path && r.route.methods[method.toLowerCase()];
};

const fetchFromAuthCookie = (res, cookieName) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add a utility function to test, and Refactor other function to use it.

Referenced below in my new test cases.

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.

2 participants