gnikyt / laravel-shopify

A full-featured Laravel package for aiding in Shopify App development
MIT License
1.24k stars 374 forks source link

2 Different Shops with Same App in Same Browser leads to Session Sharing Issues #295

Closed darrynten closed 5 years ago

darrynten commented 5 years ago

Expected Behavior

A user should be able to have the same app installed on 2 different shops and manage the app for both shops without the session data clashing or causing conflicts.

Current Behavior

Currently all shops that have the same app open in the same browser can end up sharing the same session which can cause things like Shop A overwriting Shop B information.

I have also experienced shops getting redirected to each others myshopify.com domains while testing apps in 2 different shops.

Failure Information

The issue exhibits in an edge case, and may be causing other issues that seem to be unreproducible.

The problem exists because sessions are loaded on a domain basis, and when loading the same app from 2 different stores in the same browser can cause the stores to overwrite each others cookie because they are both actually running on the same domain (the apps domain loaded in the shopify admin iframe).

Steps to Reproduce

Make an app that has a get route as well as a post route.

  1. Launch the app in Shop A and Shop B and dump ShopifyApp::shop()->shopify_domain on the get() route. You should see the correct details because get() routes load the store from the get() variables request()->requestUri values (shop domain, HMAC)
  2. In chrome developer tools check the XXX_session value in each stores cookies for the app domain. You will see the value is the same for both stores.
  3. Submit a post to any post() route that dumps ShopifyApp::shop()->shopify_domain for both shops. You will see that both shops will have the same ShopifyApp::shop()->shopify_domain. You see the incorrect details for 1 store because post() routes load the store from session, and both shops share the same domain (and therefore session) for the app.

Remediation Advice

The only place I see the correct myshopify.com domain coming through is in the HTTP REFERER header available on affected POST requests.

This header cannot be blindly trusted as it is spoofable but the referer also has the HMAC available, so it would be safe to use the domain in the referer header only after the HMAC has been verified. (i.e. validating and loading via the referer in the same way the normal get requests get checked and loaded).

If the referer domain is verified and differs from the session domain it should be safe to prioritise the shop domain in the referer header over the one available in session on post requests.

Context

Requires that 2 shops have the same app installed, and that both shops are being managed in the same browser.

StefanNeuser commented 5 years ago

Hi @darrynten, thanks for this report and you are right, there is a issue as long you click inside the app between 2 different shops. At the moment i think the only (easy) way to get this working is to set a shop=your-shop.myshopify.com param in each request you make inside your app. The problem is, that all shops/app-installations are using the same domain/session.

@ohmybrew : i guess the package needs something like https://github.com/tymondesigns/jwt-auth instead of session based auth. Client-Based-Auth|State

What we did after reading about this issue

  1. in the blade template of our react-polaris app: <meta name="shop" content="{{ shop()->shopify_domain }}">

  2. in our app.js we set this header window.axios.defaults.headers.common['shop'] = document.head.querySelector('meta[name="shop"]').content

  3. in AuthShop.php @ohmybrew see https://github.com/ohmybrew/laravel-shopify/pull/297 $shopDomainParam = $request->get('shop') ?? $request->header('shop');

darrynten commented 5 years ago

@StefanNeuser This would mean you'd have to send the shop domain along with each post request.

If this isn't validated somehow then anyone can send any shop domain and make changes to other shop app settings.

Your suggestion would mean that each person using this package would have to do this in order to avoid the issue, which places the responsibility on each developer, instead of having the package take responsibility. This issue must be handled by the package as this is where the problem lies.

The only reliable way I've managed to check the true store domain on post requests is to process and verify the details in the HTTP_REFERER header (i.e. check the domain and the HMAC) and then prioritise that domain above others.

darrynten commented 5 years ago

@StefanNeuser your solution is spoofable.

darrynten commented 5 years ago

@StefanNeuser the proposed JWT solution would result in the same problem because even though the JWTs will be different they will all load the same session.

StefanNeuser commented 5 years ago

@darrynten its not spoofable because of

    /**
     * Checks if the package has everything it needs in session.
     *
     * @return bool
     */
    public function isValid()
    {
        // No token set or domain in session?
        $result = !empty($this->getToken(true))
            && $this->getDomain() !== null
            && $this->getDomain() == $this->shop->shopify_domain;

        return $result;
    }

It is not spoofable as long as this condition !$Session->isValid() is in AuthShop.php. There is then again executed an auth against shopify and for that you have to be logged in at shopify.

darrynten commented 5 years ago

My HTTP_REFERER header on all post requests I've tested already have the real domain as well as the HMAC

gnikyt commented 5 years ago

Ah I believe this falls into the issues present in v8 with auth stuff, which I think was solved with the last release a couple days ago.

I have installed an app on three stores, visiting each store, I can change settings/info fine as the session updates.

StefanNeuser commented 5 years ago

@ohmybrew no this issue wasnt fixed with the latest release. if you click inside the app between the different stores your backend will use the same (latest authenticated) session for three apps.

darrynten commented 4 years ago

@osiset this has become an issue again