Skip to content
This repository was archived by the owner on Jul 14, 2022. It is now read-only.

Fix part of #60 #61

Open
hpawe01 wants to merge 2 commits into
base: master
Choose a base branch
from
+13 −4
Open

Fix part of #60 #61

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1bce1fa
Fix #60
hpawe01 Mar 13, 2018
ba5dc32
Follow shopify coding style
hpawe01 Mar 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions middleware/withShop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,27 @@
module.exports = function withShop({ authBaseUrl } = {}) {
return function verifyRequest(request, response, next) {
const { query: { shop }, session, baseUrl } = request;
const { query = {}, session = {}, baseUrl } = request;
const { accessToken } = session;
const shop = getShopFromReferrer(request.get('referer')) || query.shop;

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

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

response.redirect('/install');
return;
};

function getShopFromReferrer(referrer) {
if (!referrer) {
return;
}
const result = referrer.match(/shop=([^&]+)/);
Copy link
Contributor

@jamiemtdwyer jamiemtdwyer Mar 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@marutypes marutypes Mar 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@hpawe01 hpawe01 Mar 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

return result && result[1];
}
};