Shopify / shopify-app-js

MIT License
297 stars 116 forks source link

Session cookie race condition with multiple shops #451

Open joelvh opened 1 year ago

joelvh commented 1 year ago

Issue summary

In our standalone Remix app (not embedded), we use await authenticate.admin(request) to create a session (offline). This sets a cookie and the user's session has the auth token for the shop they accessed our app from.

However, if the same user accesses our app from another shop, the cookie retrieves a session for the previous shop -- thereby referencing the wrong session information and access token. The cookie also still states cookies: 'shopify_app_session=offline_previous-shop.myshopify.com;.

This same thing happens with online sessions. The authenticate.admin function doesn't ensure that the shop querystring parameter is used for session storage and retrieval, to handle the same user accessing the same app from multiple shops.

Is there a way to prevent a cookie from being set, to make these requests stateless?

15:29:09 │ remix    │ LOADER (ROOT) http://app.test/?hmac=4b1e0c2f88131f91000f87f592a2f22475000cd073ed3d8712d4478b880722b6&host=YWRtaW4oo2hvcGlmeS5jb20oo3RvcmUvb2R4LWxvY2FsLWVhdHM&shop=second-store.myshopify.com&timestamp=123456789 {
15:29:09 │ remix    │   cookies: 'shopify_app_session=offline_previous-store.myshopify.com; shopify_app_session.sig=jFJzqzEB9g/WxxVarL///iFQaOpbtdj6n2pMS8t1iFk='
15:29:09 │ remix    │ }
15:29:09 │ remix    │ LOADER (_index) http://app.test/?hmac=4b1e0c2f88131f91000f87f592a2f22475000cd073ed3d8712d4478b880722b6&host=YWRtaW4oo2hvcGlmeS5jb20oo3RvcmUvb2R4LWxvY2FsLWVhdHM&shop=second-store.myshopify.com&timestamp=123456789
15:29:09 │ remix    │ [shopify-app/INFO] Authenticating admin request
15:29:09 │ remix    │ SESSION Session {
15:29:09 │ remix    │   isOnline: false,
15:29:09 │ remix    │   shop: 'previous-store.myshopify.com',
15:29:09 │ remix    │   scope: 'read_all_orders,read_inventory,read_price_rules,read_product_listings,write_customers,write_draft_orders,write_files,write_fulfillments,write_merchant_managed_fulfillment_orders,write_order_edits,write_orders,write_products,write_script_tags,write_shipping,write_themes,write_third_party_fulfillment_orders',
15:29:09 │ remix    │   id: 'offline_previous-store.myshopify.com',
15:29:09 │ remix    │   state: 'XXXXXXXXXX',
15:29:09 │ remix    │   accessToken: 'shpca_XXXXXXXXXXXX',
15:29:09 │ remix    │   expires: undefined
15:29:09 │ remix    │ }

Expected behavior

  1. Users should be able to access the same app from different shops
  2. authenticate.admin should retrieve session data based on the shop the request originated from (e.g. respect the shop querystring parameter)

Actual behavior

  1. Users accessing the app from multiple shops resumes the initial session and ignores that links have hmac and shop parameters indicating they are accessing from a different shop

Steps to reproduce the problem

  1. Create Remix app from template
  2. Call const { session } = await authenticate.admin(request) to initiate a session when accessing the app homepage
  3. Log or output the session object to see what shop it holds information for
  4. Install the app on Shop 1 and see that session has Shop 1 data
  5. Install the app on Shop 2 and see that session still has Shop 1 data
joelvh commented 1 year ago

We are able to work around this in some cases by calling await login(request) if the shop querystring parameter is not the same as the session.shop. However, this seems like the behavior that likely ought to be built-in.

paulomarg commented 1 year ago

Hey, thanks for reporting this! Unfortunately we do need the cookie because that's how we keep track of which user is making requests in a non-embedded scenario.

That being said, I agree that the package should detect this scenario and update the cookie automatically when the user logs in with a second shop, though your suggested workaround with login is the correct way to solve this.

Since this is a little bit of an edge case and there is a workaround for it, it might be a while until we can actually fix it.

joelvh commented 1 year ago

hi @paulomarg, thanks for weighing in. Can you confirm that -- in effect -- you're saying this package assumes users only belong to one shop and won't login to the same app from different shops?

I'm asking mostly for posterity of this thread to make sure the edge case in question is clear to others. Thanks!

paulomarg commented 1 year ago

Yes, right now that is a limitation of this package, but it only affects non-embedded apps. For embedded apps, because we never store a cookie and use session tokens provided by the admin interface, this would not be an issue.

So just to set expectations here, we are going to look into ways of improving the behaviour, but there are other things that will take priority over it!

joelvh commented 1 year ago

Thanks for the clarification @paulomarg!

mike-bernard commented 11 months ago

This is also affecting my application. Thanks for the workaround @joelvh!

github-actions[bot] commented 9 months ago

We're labeling this issue as stale because there hasn't been any activity on it for 60 days. While the issue will stay open and we hope to resolve it, this helps us prioritize community requests.

You can add a comment to remove the label if it's still relevant, and we can re-evaluate it.

mannprerak2 commented 2 months ago

Any update for this?

github-actions[bot] commented 1 week ago

We're labeling this issue as stale because there hasn't been any activity on it for 60 days. While the issue will stay open and we hope to resolve it, this helps us prioritize community requests.

You can add a comment to remove the label if it's still relevant, and we can re-evaluate it.