Shopify / shopify_app

A Rails Engine for building Shopify Apps
MIT License
1.74k stars 685 forks source link

Missing `host` param #1781

Open flavio-b opened 5 months ago

flavio-b commented 5 months ago

Issue summary

We started sporadically seeing an error ShopifyAPI::Errors::MissingRequiredArgumentError (host argument is required), from our SplashPageController#index (it's an embedded app).

Upon further investigation, I believe it occurs like this:

  1. The merchant is logged in and navigating inside the app as usual. The session token is working as expected.
  2. Suddenly, for some reason, missing_expected_jwt? returns true, which ends up running redirect_to_splash_page.
  3. The app redirects to the splash page, but it has no host param, because the redirect comes from a page that's already inside the iframe.
  4. SplashPageController#index calls redirect_to(ShopifyAPI::Auth.embedded_app_url(params[:host]) + request.path, allow_other_host: true). Since no host is present, it throws an error.

Expected behavior

When a merchant is logged in a navigating inside the app, a session token is always present and so all authenticated routes work as expected.

Actual behavior

In some cases, missing_expected_jwt? returns true. Something down the stack from this method is causing issues with the token, or it's not being set correctly on the client.

Steps to reproduce the problem

  1. Create an app with at least 2 routes. Say a primary page and a secondary page.
  2. In the secondary page controller action, call redirect_to_splash_page, to simulate missing the JWT.
  3. Open the app and login as usual. Then, try to open the secondary page.
  4. The app will redirect to splash page, with no host params and should throw ShopifyAPI::Errors::MissingRequiredArgumentError
paulomarg commented 5 months ago

Hey, sorry to hear you're having issues, and thank you for the all the details!

I think the host being missing when we call redirect_to_splash_page is somewhat expected if there wasn't a JWT or a host search param in the request, so maybe the issue is when the navigation happens in the first place. Just so I can have a better idea of what's happening:

flavio-b commented 5 months ago

We use Turbolinks, so all (XHR) requests for a page load get intercepted and an Authorization header is added before the request is submitted.

We've also seen the missing JWT one time on a request triggered by authenticatedFetch from AppBridge utilities.

Our Turbolinks setup is pretty much copy and paste from a tutorial from Shopify, when session tokens were being introduced, and we're using the latest version of AppBridge 3.7.10 to provide authenticatedFetch on specific requests.

paulomarg commented 5 months ago

Hey, I touched base with the team, and this could be an intermittent issue that prevents App Bridge from getting the session token (used in Authorization) - if an error happens in the Admin, the token will be empty when making the request, and authenticatedFetch will return a rejected promise, so the request should be stopped at that point.

Unfortunately, that is something that can happen at times (the token is fetched from the Shopify server which can have hiccups), so it'll be up to the app to retry that request from the frontend. In that case, there's not a lot we can do from the app package side.

That being said, I've never run into this scenario myself (I imagine it happens only at scale), so I don't know if the request failing on the server side would be a problem. I'm assuming that if you cancel the request or handle the rejection it will ignore the redirect response, but I'll keep this issue open just in case we have to fix something.

Hope this helps!

flavio-b commented 5 months ago

Ok, thanks for looking into it! That makes sense.

I think there might be something that can be done at the package level to help with this error. For example, inside redirect_to_splash_page. It perhaps could check if the host is available and, if not, attempt to get it another way, since the splash page is somewhere we always expect a host to be present.

For example, something like host = params[:host] || Shop.find_by_shopify_domain(current_shopify_domain)&.host.

I don't know if that's the best or safest solution, or whether the responsibility to attempt to get the host falls on the redirect_to_splash_page method or the SplashPageController#index.

paulomarg commented 5 months ago

Yeah, that's a good point. Unfortunately, I don't think there's a ton we can do here - if there is no session token and no host / shop params, there's really no way for us to recover as the request is essentially unsigned, and cookies won't be available for embedded apps for us to pull data from the session.

I'll close this issue since there's no action we can take right now, but let me know if you spot any improvements / fixes we can make from our side and we can reopen!

flavio-b commented 5 months ago

The requests with missing JWT that we've seen so far seem to at least come with the shop param, which gives us access to current_shopify_domain, that we could use, for example to try to set the host: host = params[:host] || Shop.find_by_shopify_domain(current_shopify_domain)&.host

Though I'm not sure if that's safe. TBH, I never really understood the need for both a host and shop params being separate, since one can always derive one from the other, in code. For example, if we have host, we can extract shop. If we have shop, we can build the host.

paulomarg commented 5 months ago

The host is a separate value because it can be something other than the admin, depending on where the app is loaded. Though we can indeed work the value backwards from the shop and that would solve most cases.

I think you're right - there might be something we can do with this. Since there's a workaround, it may be a while until we can implement this, though. Thanks for working through this with me :)