Shopify / shopify-app-js

MIT License
262 stars 101 forks source link

No documentation for custom apps #719

Open david-256 opened 5 months ago

david-256 commented 5 months ago

Overview/summary

I want my Shopify app to be only installable by a certain merchant. I already set the distribution method to "custom distribution" within the partner dashboard.

If someone opens the domain on which my app is running in the browser, this person can install the app on his own store. This is unexpected behavior, since I specified within the Shopify partner dashboard that only a specific shop should be allowed to install my app.

I suppose the problem is that the Shopify config file shopify.server.ts by default is configured for distribution via the app store. I suppose that the following changes need to be carried out on my site:

What Shopify can improve

My main problem is that I found no documentation on how to adjust the shopify.server.ts file for a custom app.

I don't know the difference between AppDistribution.ShopifyAdmin and AppDistribution.SingleMerchant. There is no documentation as doc comment within the code.

I don't know the effects of setting customShopDomains. There is no documentation as doc comment within the code.

Could you please clarify what I should do and also adjust the docs on https://shopify.dev as well as within the code.

Further information

I use the Shopify Remix app template.

In my app, the package @shopify/shopify-app-remix has the version 2.6.1.

Here is an excerpt from the default shopify.server.ts file (which I am currently using):

const shopify = shopifyApp({
  apiKey: process.env.SHOPIFY_API_KEY,
  apiSecretKey: process.env.SHOPIFY_API_SECRET || "",
  apiVersion: LATEST_API_VERSION,
  scopes: process.env.SCOPES?.split(","),
  appUrl: process.env.SHOPIFY_APP_URL || "",
  authPathPrefix: "/auth",
  sessionStorage: new PrismaSessionStorage(prisma),
  distribution: AppDistribution.AppStore,
  restResources,
  webhooks: {
    APP_UNINSTALLED: {
      deliveryMethod: DeliveryMethod.Http,
      callbackUrl: "/webhooks",
    },
  },
  hooks: {
    afterAuth: async ({ session }) => {
      shopify.registerWebhooks({ session });
    },
  },
  future: {
    v3_webhookAdminContext: true,
    v3_authenticatePublic: true,
    unstable_newEmbeddedAuthStrategy: true,
  },
  ...(process.env.SHOP_CUSTOM_DOMAIN
    ? { customShopDomains: [process.env.SHOP_CUSTOM_DOMAIN] }
    : {}),
});
paulomarg commented 5 months ago

Hey, thanks for bringing this up. We're planning on going over all of our documentation from a custom app lens and revamp it, and this is great feedback, thank you! We'll keep this in mind when doing that and improve our docs!

In the meantime, some info on custom app distribution (full info in the docs):

Therefore, I think the solution for you is:

Hope this helps!

lukeclifton commented 5 months ago

Hey. I have just come across this same confusion and had to hunt around for a while to find this issue.

+1 for better docs around this!!

Also, for me the terms ShopifyAdmin and SingleMerchant are very confusing. The description for ShopifyAdmin distribution type sounds like it should be for SingleMerchant type.

lukeclifton commented 5 months ago

Also, when using SingleMerchant do we still need to set SHOP_CUSTOM_DOMAIN env var?

lukeclifton commented 5 months ago

Seeing something strange here...

I have set distribution to SingleMerchant in shopify.server.ts and set distribution type to "custom" in app settings however, I am still able to install the app on any Shopify store? Am I missing something?

paulomarg commented 5 months ago

Hey @lukeclifton - yeah, I agree the types of custom apps can be a bit confusing. Just to clarify: SingleMerchant apps work like app store apps as far as authentication goes.

You still need to go through the process of generating access tokens via OAuth (or token exchange) because they can be installed on multiple stores for Plus merchants, so they're not always tied to a single store, but to a single merchant. ShopifyAdmin apps are tied to a single store because they're created inside the context of that store's admin.

I think you have a good point in that these names are confusing. I wonder if we should change our distribution enum to have values:

That feels like it might be clearer because it would match the docs.

I have set distribution to SingleMerchant in shopify.server.ts and set distribution type to "custom" in app settings however, I am still able to install the app on any Shopify store? Am I missing something?

When you set your app to custom distribution, you'd pick a store to install it to, right? Are you still able to login to other stores after doing that? IIRC Shopify should block you from authenticating with other stores.

lukeclifton commented 4 months ago

Hey @paulomarg Thanks for coming back so quick!

Yeah I much prefer the use of terms "Public" and "Custom" as these are terms we already use and are familiar with.

The term "ShopifyAdmin" still throws me off to be honest. Using the old term "Private" seems way more obvious but perhaps something like "Manual" is an idea? Or "SingleStore" if your sticking with "SingleMerchant".

When you set your app to custom distribution, you'd pick a store to install it to, right? Are you still able to login to other stores after doing that? IIRC Shopify should block you from authenticating with other stores.

Yes, to confirm I am setting app to custom distribution and setting a store to install too which cannot be changed as expected. Then when I try and install on a completely different store owned by a different partner account I am still able to install the app without any restrictions. 😕

I remember when Shopify blocked this also and that's what I am expecting. Is this a bug? If not, how do we lock custom apps down to just a single merchant?

paulomarg commented 4 months ago

We'll look into the naming when we make the changes for custom apps, thanks!

As for the bug, I've reported it to the team and they're looking into it - it may be that you can install your custom app in multiple "Non-transferable" stores, but Shopify will still block installs for merchants other than the one you assigned when you configured the app.

lukeclifton commented 4 months ago

OK thanks for looking into it. Yeah I think the store I tested with was a "Non-transferable" however I see this as a issue. You shouldn't be able to install a custom app for another merchant regards if its a dev store or not. Seems a bit of a security flaw if the app is not meant to be public facing.

So currently we will need to implement some custom backend auth logic to restrict install to only whitelisted store domains?

paulomarg commented 4 months ago

I followed up with the team and they're looking into it, but one thing to notice is that you can only install such apps if you have the link to begin with, and this would only have any impact on development stores (that have several restrictions compared to "regular" stores), which should mitigate that risk.

Thanks for bringing that up :)

david-256 commented 4 months ago

Hey @paulomarg, please check out my HackerOne report with the report id 2445815. Unfortunately, it has been categorized as a functional problem, but I believe it is a security issue. In this report, I have written down a step-by-step guide to install a custom app of a different merchant without having access to the installation link. I hope this issue gets fixed.

lukeclifton commented 4 months ago

Yeah with the current Remix app template, you could install an app without having a Shopify app install URL. You would just need to know the app URL and then you could go to the /auth route and add a Shopify domain.

Here's a real world example: A developer is building an internal management app for an enterprise merchant. This app connects to external business related API's and should never be able to be accessed by someone external to the company.

If a developer builds such a custom app and uses the distribution type SingleMerchant they may think this is safe, however in reality someone with knowledge of the app URL, could install the app on a development store and access something they shouldn't be able to see.

paulomarg commented 4 months ago

Fair points, thank you folks. I'm forwarding that to the team that manages this area :)

lukeclifton commented 4 months ago

@paulomarg What do you think to adding a "store domain whitelist" config feature to the app template to overcome this issue?

zzooeeyy commented 4 months ago

Hello!

We’re sorry to hear about your frustrations regarding app installations. Our team has reviewed this issue and found that allowing apps to be installed on other development stores provides greater flexibility and benefit for app developers. As for the potential security concern that was pointed out, we have restrictions in place to ensure custom apps can only be installed on other non-transferrable development stores, and apps should ensure access and sensitive data is gated for a particular session. We agree that it’s definitely causing confusion, so we will update the documentation and wording on this.

Thanks!

github-actions[bot] commented 2 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.