search

Shopify / shopify-app-js

MIT License
280 stars 111 forks source link

Redirect to ensure session token search param behind reverse proxy #378

Closed davespanton closed 1 year ago

davespanton commented 1 year ago

Issue summary

Part of the authentication flow for embedded apps will redirect users to ensure they have a valid session token in the url search parameters ( https://github.com/Shopify/shopify-app-js/blob/main/packages/shopify-app-remix/src/auth/admin/authenticate.ts#L495 ).

This specifies a further redirect using a shopify-redirect search parameter, the url for which is taken from the current request's ( https://github.com/Shopify/shopify-app-js/blame/main/packages/shopify-app-remix/src/auth/admin/authenticate.ts#L382C37-L382C37 )

When the embedded app is being hosted behind a reverse proxy that rewrites the Host header (which may be necessary when e.g. a backend service is being hosted somewhere like Fly or Vercel), this can redirect to an invalid url. It would be preferrable to redirect to the app url specified in the config.

Expected behavior

The session token redirect flow should set the shopify-redirect search parameter using the app url specified in the application config.

As an example, this is passed by process.env.SHOPIFY_APP_URL in the shopify-app-template-remix here: https://github.com/Shopify/shopify-app-template-remix/blob/main/app/shopify.server.js#L18

Actual behavior

The redirect flow gets stuck due to the shopify-redirect search parameter pointing to a different url than the app's.

Steps to reproduce the problem

  1. Setup up a remix embedded app hosted on e.g. fly.io
  2. Setup e.g. HAProxy, with a backend configured with the .fly.dev url, and a Host header rewrite so fly.io can correctly route the request.
  3. Point the applications main domain to HAProxy.
  4. Install the application on a dev store and observe the above described behavior.
jwilkey commented 1 year ago

Agree. For those using nginx, this is how I resolved this issue:

location / {
   ...
    proxy_set_header Host $host;
   ...
}

Otherwise you will get https://localhost:PORT !== https://my.domain.com

paulomarg commented 1 year ago

Hey folks, thanks for the very detailed issue, and sorry for the delayed response. I think what you're saying makes sense, if there's a place where we're linking to the app using a full URL and the request's hostname, we can change that to use the configured value instead.

I'm sorry to ask, but it seems the links you sent earlier are stale now, so I'm having trouble finding exactly where you spotted this. Could you please confirm once again where this is happening so I can set up a PR? If you think it's quicker, please feel free to set up a PR and I'll make sure it gets merged.

Thanks!

davespanton commented 1 year ago

Hi @paulomarg,

Thanks for following up. Looks like there's been a bit of refactor since I opened the issue. I knew I should have used the commit shas in the links instead of main :grimacing:

The first link has become: https://github.com/Shopify/shopify-app-js/blob/1f4ffef/packages/shopify-app-remix/src/server/authenticate/admin/authenticate.ts#L499

and the second is now: https://github.com/Shopify/shopify-app-js/blob/1f4ffef/packages/shopify-app-remix/src/server/authenticate/admin/authenticate.ts#L386

I believe adding something like: url.hostname = config.hostName after line 505 would do the trick, and hopefully wouldn't break any other use cases.