AuthShop middleware fails to authenticate if request contains other parameters than described

[marko-behm]

Expected Behavior

HMAC should contain all parameters from request, but it only contains the most common ones.

Current Behavior

HMAC validation fails if there are other than shop, hmac, timestamp, code, locale, state, id or ids parameters in request.

Failure Information

OhMyBrew\ShopifyApp\Exceptions\SignatureVerificationException Unable to verify signature.

Steps to Reproduce

Executing js script to load page within embedded application:

const redirect = AppBridge.actions.Redirect.create(app); redirect.dispatch(AppBridge.actions.Redirect.Action.APP, '{{ route('shop.billing', ['something' => 'else'], false) }}');

In this case parameter something makes the request to fail. Sometimes I need to refresh the page with F5 to make it crash.

Context

• Package Version: 10.2.1
• Laravel Version: 6.5.2
• PHP Version: 7.3.4

Failure Logs

OhMyBrew\ShopifyApp\Exceptions\SignatureVerificationException Unable to verify signature.

How To Fix This

If I edit Middleware/Authshop.php::getQueryDomain() function like this:

private function getQueryDomain(Request $request)
    {
        // Extract the referer
        $shop = $request->input('shop');
        if (!$shop) {
            return false;
        }

        $verify = $request->all();
        foreach ($verify as $key => $value) {
            if (is_array($value)) {
                $verify[$key] = '["'.implode('", "', $value).'"]';
            }
        }

        // Make sure there is no param spoofing attempt
        if (ShopifyApp::api()->verifyRequest($verify)) {
            return $shop;
        }

        throw new SignatureVerificationException('Unable to verify signature.');
    }

It validates all requests I tried it with.

[marko-behm]

And it seems that getRefererDomain() method needs to be fixed too.

This seems to fix it:

private function getRefererDomain(Request $request)
    {
        // Extract the referer
        $referer = $request->header('referer');
        if (!$referer) {
            return false;
        }

        // Get the values of the referer query params as an array
        $url = parse_url($referer, PHP_URL_QUERY);
        parse_str($url, $refererQueryParams);
        if (!$refererQueryParams) {
            return false;
        }

        // These 3 must always be present
        if (!isset($refererQueryParams['shop']) || !isset($refererQueryParams['hmac']) || !isset($refererQueryParams['timestamp'])) {
            return false;
        }

        foreach ($refererQueryParams as $key => $value) {
            if (is_array($value)) {
                $refererQueryParams[$key] = '["'.implode('", "', $value).'"]';
            }
        }

        // Make sure there is no param spoofing attempt
        if (ShopifyApp::api()->verifyRequest($refererQueryParams)) {
            return $refererQueryParams['shop'];
        }

        throw new SignatureVerificationException('Unable to verify signature.');
    }

[gnikyt]

@marko-behm Would you submit a PR?

[marko-behm]

I have no idea how that is done.

[gnikyt]

@marko-behm No worries then, I will tackle it :)

[gnikyt]

Fix is in place. Will be part of next release.

[jedimdan]

Do we need to update getHeaderDomain() too?

[Kyon147]

When it the next release happening as this is a production level issue which is breaking a lot of live apps.

Switching to "dev-master" in composer is an okay temp fix but should not really be used as a long term solution.

[StefanNeuser]

Hi @ohmybrew , today i got also a new issue by verifying the signature. I added a new parameter that comes from the shopify redirect for the hmac check. The parameter is called 'session'

What i did to get signature verification back working is: https://github.com/ohmybrew/laravel-shopify/compare/master...StefanNeuser:v10.2.1?expand=1

[eleazargan]

@ohmybrew This issue is still happening on the latest version

[jedimdan]

@eleazargan the fix has not been released yet but is in master. For now, you can consider switching to the master branch temporarily while we wait for a new release.

(Update 2020 Feb 20: This is now fixed as of 10.3.0. Please do not use the master branch in production anymore!)

[eleazargan]

For my production fix I’ve overridden the middleware with my own logic that’s causing the error. Since there are many users using the application so I had to come up with a quick solution. I understand that the package is in the midst of being refactored.

[marko-behm]

This started to happen on an app that worked nicely before. App gets into authentication loop that never ends. We were using 2019-04 version of shopify api at the moment. We fixed the code by manually overwriting the source file and are hoping to get the fixed published version of it in near future.

[Kyon147]

This started to happen on an app that worked nicely before. App gets into authentication loop that never ends. We were using 2019-04 version of shopify api at the moment. We fixed the code by manually overwriting the source file and are hoping to get the fixed published version of it in near future.

@marko-behm you probably want to move away from 2019-04 as it is being deprecated.

The fix is in master but just waiting on the proper release, understandably ohmybrew does have a life outside this package 👍

[jedimdan]

Great news everyone, @ohmybrew has just released v10.3.0 which includes the fix for this.

[tzookb]

I just upgraded to 10.3 :)

but now I get the redirect loop on auth...

I checked the cookie, and I got two:

• laravel session, that is set with samesite "none" and secured
• ugly token, but not secured and no "samesite" set

But when I alter the "ugly token" cookie, and set samesite to "none" and update to secured. Im able to login through shopify...

any ideas?

[tzookb]

@jedimdan previously I did something similar to your initial middleware fix.

So all my cookies would be set with "sameSite" rules.

But with 10.3 we now set sameSite only on the ShopSession service cookies.

Im not really sure what is the weird looking cooking... "laravel_session" cookie is easy... but what is the other one that is not secure :|

[jedimdan]

@tzookb The 10.3 update actually works the same as the middleware, in that it changes the flags globally. So all cookies from the same response will be set to SameSite None. The only difference is that unlike the middleware, only responses that involve the ShopSession will have the flags set. So there shouldn’t be a case of one cookie set and another not set if they came from the same response.

I’m not actually sure what this ugly cookie is though.

[tzookb]

I agree...

in some weird way, the middleware does store the cookie properly but with 10.3 it doesnt

will try to find what is that :|

[tzookb]

@darrynten mentioning you as I know you did the great initial work on the middleware, so maybe you have more ideas.

I decoded the "ugly cookie" and it seems like its laravel session stuff:

{"data":"a:3:{s:6:\"_token\";s:40:\"dT7bX6bY2FuoNKabdKSxoJ5L7tIctHjEXudTxvVe\";s:9:\"_previous\";a:1:{s:3:\"url\";s:30:\"http:\/\/55f086c4.ngrok.io\/login\";}s:6:\"_flash\";a:2:{s:3:\"old\";a:0:{}s:3:\"new\";a:0:{}}}","expires":1580924577}

[jedimdan]

@tzookb I just encountered something similar to what you were describing (a seemingly random cookie) and figured out what it was. That random cookie shows up when your SESSION_DRIVER is set to cookie. It's actually your entire session data in the form of a cookie. You should change the driver to something else and that cookie will be gone. For some reason, that cookie doesn't seem to honour the session flags which is why it "breaks" your app even if you have the right flags set.

[tzookb]

thanks @jedimdan

I did notice that as well, I should have posted that here 😑

my bad.

I just moved my session driver to anything else and it got resolved.