Allows setting a domain-wide cookie in the oauth flow #905
Conversation
…ienotfound errors
|
Forgot to add that this PR also changes the default path when setting the auth cookie. Currently the auth cookie is valid only on exactly the same domain and the same path that started the OAuth flow, which works great when creating an app using Shopify's CLI but fails on other cases. An example where the current config fails:
The PR changes the default behavior by making the auth cookie available on the root path but at the same time it allows setting a specific domain where to make the cookie readable, but if not domain is set then the current behavior of using the initial domain is kept. |
|
Nice work! |
|
Awesome work. I hope an admin merges this soon. |
|
Yes please let's merge it 🙇🏽♂️ |
|
What will it take to get this merged? |
|
Thanks very much the PR @cmelendez ! Really appreciate you taking the time to open this. I'm going to get this reviewed by app sec and back to you. |
| @@ -86,9 +86,10 @@ export function begin(config: ConfigInterface) { | |||
|
|
|||
| await cookies.setAndSign(STATE_COOKIE_NAME, state, { | |||
| expires: new Date(Date.now() + 60000), | |||
| sameSite: 'lax', | |||
| sameSite: 'none', | |||
There was a problem hiding this comment.
Is this required for the domain to work? OAuth should only rely on top-level redirects, and as per the MDN docs:
Lax
Means that the cookie is not sent on cross-site requests, such as on requests to load images or frames, but is sent when a user is navigating to the origin site from an external site (for example, when following a link).
I believe it should still work if we use lax. I ask because using none carries some risk for CSRF. If it is strictly necessary, I think we should only set it to none if cookieDomain is set to help mitigate that risk.
| secure: true, | ||
| path: callbackPath, | ||
| path: '/', | ||
| domain: config.cookieDomain, |
There was a problem hiding this comment.
I think we need to fully skip domain if the setting isn't present, otherwise we'll end up with this Set-Cookie string:
Set-Cookie:
shopify_app_state=<state>;sameSite=lax; secure=true; path=/auth/callback; domain=undefined;
…ameSite=lax. See this PR for other people who have ran into this issue Shopify#905
WHY are these changes introduced?
Some OAuth flows require a domain-wide cookie instead of the usual behavior that only works within the starting domain. For example, if you start the OAuth flow in
api.example.comand then redirect the user toapp.example.comto finish it, you'll get a CookieNotFound error (check out issue #686).WHAT is this pull request doing?
Allows to set an optional
cookieDomainparam when callingshopifyApi.Type of change
Checklist
yarn changesetto create a draft changelog entry (do NOT update theCHANGELOG.mdfile manually)