Fix part of #60

[hpawe01]

• Tries to extract shop from the referer header, if not in query
• Makes sure, that the shop from the session is the same as the shop performing the request

[marutypes]

Just some stylistic things, but the code looks sane. I'm going to open an issue to actually test this middleware.

[marutypes]

to be a bit more idiomatic of the code style favouring immutability we have in this repo I'd maybe consider something a bit more like

const {session = {}, baseUrl, query} = request;
const {accessToken} = session;

const shop = shopFromReferrer(request.get('referrer')) || session.shop

if (accessToken) { 
  next();
  return;
}

if (shop) {
  response.redirect(`${authBaseUrl || baseUrl}/auth?shop=${shop}`);
  return;
}

response.redirect('/install');

Where we shove the regex logic into another function in scope.

function shopFromReferrer(referrer) {
  const results = referrer.match(/shop=([^&]+)/) ;
  return results && results[1]
}

[marutypes]

Also thanks so much for contributing :)

[hpawe01]

Thanks for the styling tips! I implemented them in the way you described.

[marutypes]

Looks great :)

[marutypes]

@AWaselnuk @lopert either of you have any objections?

[AWaselnuk]

Looks lovely. Thank you!

[jamiemtdwyer]

I think this is concerning to me from a security perspective – could this allow an arbitrary user to log in as a given shop if they spoof the referrer?

The code as is wouldn't even require explicit spoofing – any referring URL with the shop parameter would be able to log in as the provided shop given that this shop has previously authed and we have an access token stored.

[marutypes]

They need to have an access token in express's session, which means they've logged in before. We could also pull up the shop in question and check if the accessToken matches, which I opened an issue for. #64

[hpawe01]

@jamiemtdwyer Actually the referrer is just another way to get the shop name. If an arbitrary user could log in as a given shop by spoofing the referrer, they could do it already at the moment by providing the GET parameter shop.

So this pull request is independent of the security issue you describe. As @TheMallen points out in #64 it is not the question how we determine, which shop is requested, but if the current session token matches the requested shop.

[marutypes]

After talking to Jamie I think we actually need to can this.

We'll need to implement something similar to login_again_if_different_shop to properly handle multiple shops consistently with shopify_app.

I'm sorry. Thanks so much for the effort.

[hpawe01]

Actually I see the same problem in this rails code as that one I want to fix with this pull request: to get the requesting shop, only params[:shop] is checked. We are ignoring the referrer as well
and so I guess with shopify_app we also have the problem when accessing a shopify app through multiple shops during one browser session.

Regarding your point, that the implementation should be similar to the rails app: I am no rails expert, but with this pull request the behavior looks more similar to the rails code than before. What the rails code does is:

1. check if there is a session #L12
2. if not redirect to login #L20
3. if yes, check if the shop in the session is the same as the requesting #L30
4. if not, clean the session and redirect to login #L33
5. if yes, proceed

We do the same but in another order:

1. check if there is a session and if the shop in the session is the same as the requesting #L7
2. if yes proceed
3. if no, redirect to login / install #L12 / #L17

The only thing we don't do is to reset the session. But I can add this, if you want:

\\ ...
if (accessToken && session.shop === shop) {
  next();
  return;
}

request.session.acessToken = null;
request.session.shop = null;
\\ ...

Don't know if that works though. And it is more part of #64 than #60.

But anyhow, as I said here, I think this pull request should be merged as it doesn't decreases the security and can be easily used as basis for #64.

[marutypes]

@jamiemtdwyer does this assuage your fears?