- Notifications
You must be signed in to change notification settings - Fork384
Allows setting a domain-wide cookie in the oauth flow#905
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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 thesame domain and thesame 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. |
arabovs commentedJun 11, 2023
Nice work! |
rodrigogsqquid commentedJun 20, 2023
Awesome work. I hope an admin merges this soon. |
gusbueno commentedJun 22, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Yes please let's merge it 🙇🏽♂️ |
jagthedrummer commentedAug 29, 2023
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. |
paulomarg left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hey folks, thanks for waiting while we got this reviewed.
I think we can take this PR, but there are a couple of things I think we need to tweak!
| awaitcookies.setAndSign(STATE_COOKIE_NAME,state,{ | ||
| expires:newDate(Date.now()+60000), | ||
| sameSite:'lax', | ||
| sameSite:'none', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this required for the domain to work? OAuth should only rely on top-level redirects, and as perthe 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 uselax. I ask because usingnone carries some risk for CSRF. If it is strictly necessary, I think we should only set it tonone ifcookieDomain is set to help mitigate that risk.
| secure:true, | ||
| path:callbackPath, | ||
| path:'/', | ||
| domain:config.cookieDomain, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we need to fully skipdomain if the setting isn't present, otherwise we'll end up with thisSet-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 issueShopify#905
Cody-traded-clothes left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
cool
Uh oh!
There was an error while loading.Please reload this page.
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)