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

Default cookie setter in OAuth is not setting samesite = None #352

Open
2 tasks done
Kudze opened this issue Jul 1, 2024 · 0 comments · May be fixed by #353
Open
2 tasks done

Default cookie setter in OAuth is not setting samesite = None #352

Kudze opened this issue Jul 1, 2024 · 0 comments · May be fixed by #353

Comments

@Kudze
Copy link

Kudze commented Jul 1, 2024

Issue summary

The samesite parameter is being ommited from setcookie function in OAuth2 process: https://github.com/Shopify/shopify-api-php/blob/main/src/Auth/OAuth.php#L351, causing the default Lax mode to be selected.

This causes "Perform Token Exchange" step in https://shopify.dev/docs/apps/build/authentication-authorization/session-tokens#request-flow-using-a-session-token to fail when embedded page is loaded through an iframe (for example from Shopify admin panel), because the cookies will not be saved.

Expected behavior

The cookies should be set and OAuth should succeed in Shopify admin panel.

Actual behavior

The cookies are not set and OAuth fails in Shopify admin panel.

image

Steps to reproduce the problem

  1. Implement minimal application implementing Shopify's OAuth2 authentication (online mode = true), using this library, with default cookie setter.
  2. Install application into any Shopify store.
  3. Delete the initial customer's session. (Simulating different admin user or expired session).
  4. Try to perform "Token Exchange" from Shopify admin panel.

Reduced test case


Checklist

  • I have described this issue in a way that is actionable (if possible)
  • I have created a merge request fixing this issue.
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 a pull request may close this issue.

1 participant