Shopify / shopify-app-template-remix

345 stars 145 forks source link

Remix Auth: Make sure redirecting to the bounce page without a tunnel works in Chrome #46

Open byrichardpowell opened 1 year ago

byrichardpowell commented 1 year ago

We currently have this code:

  private redirectToBouncePage(url: URL): void {
    const { api, config } = this;

    // TODO this is to work around a remix bug
    url.protocol = `${api.config.hostScheme}:`;

    const params = new URLSearchParams(url.search);
    params.set("shopify-reload", url.href);

    // TODO Make sure this works on chrome without a tunnel (weird HTTPS redirect issue)
    throw redirect(`${config.auth.sessionTokenPath}?${params.toString()}`);
  }

I think I remember that this code did not work if:

  1. You use a tunnel provided by the CLI
  2. You use Chrome

Let's make sure we fix that if it's not working

paulomarg commented 1 year ago

It seems that this happens because the admin loads with Upgrade-Insecure-Requests: 1, which forces the iframe to load over https even if the app is running on localhost. How can we work around that?

See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/upgrade-insecure-requests

paulomarg commented 1 year ago

Another update: it seems that this breaks existing apps which worked before. I believe we've recently started returning that header from the admin, so I'm not sure if we'll be able to fix this. I'm trying to track down why that change was made to see if there is a path around it.

UPDATE: this has actually always happened when a document request returned a redirect, we just never did it in any other template because we can only grab the token with the id_token search param, which wasn't there before.

I've followed up with the appsec team, as per the next comment.

spy-v2[bot] commented 1 year ago

Paulo Margarido [2023-05-25 02:20PM UTC]
Hi friends! We're working on a new remix app template, and we ran into an interesting scenario: with the introduction of the id_token search param, we can now run into a pretty niche issue when running on localhost, and I wanted to get your thoughts on whether there is a solution. Including details in :thread2:

:thread: Slack Thread
[Slack Thread](https://shopify.slack.com/archives/C8RPMFBN2/p1685024428013969?thread_ts=1685024428.013969&cid=C8RPMFBN2)
**[Paulo Margarido](https://github.com/paulomarg)** [2023-05-25 02:20PM UTC]
Hi friends! We're working on a new remix app template, and we ran into an interesting scenario: with the introduction of the `id_token` search param, we can now run into a pretty niche issue when running on `localhost`, and I wanted to get your thoughts on whether there is a solution. Including details in :thread2:
**[Paulo Margarido](https://github.com/paulomarg)** [2023-05-25 02:21PM UTC]
If an app doesn't have the beta flag enabled to include that search param, the remix template will use App Bridge Next to set it up by redirecting the iframe doc request to a special path This all works fine when the app is running on a tunnel, because the template returns a `302` response pointing to `/`, and it gets loaded and fetches the token
**[Paulo Margarido](https://github.com/paulomarg)** [2023-05-25 02:22PM UTC]
The problem when running on `localhost` is that this `302` in the iframe follows the CSP directives set by the admin, currently: ``` child-src 'self' https://* shopify-pos://* blob:;connect-src 'self' blob: wss://* https://* [https://bugsnag-mtl.shopifycloud.com:4900/js](https://bugsnag-mtl.shopifycloud.com:4900/js) [hcaptcha.com](http://hcaptcha.com) *.[hcaptcha.com;default-src](http://hcaptcha.com;default-src) 'self' data: blob: https://* shopify-pos://*;frame-src [app.shopify.com](http://app.shopify.com) *.[shopifyapps.com](http://shopifyapps.com) *.[myshopify.com](http://myshopify.com) *.[myshopify.com](http://myshopify.com) https://* shopify-pos://* [hcaptcha.com](http://hcaptcha.com) *.[hcaptcha.com](http://hcaptcha.com) blob: [http://localhost](http://localhost):*;media-src 'self' data: blob: [https://videos.shopifycdn.com](https://videos.shopifycdn.com) [https://cdn.shopify.com/videos/](https://cdn.shopify.com/videos/) [https://cdn.shopify.com/shopifycloud/web/assets/v1/](https://cdn.shopify.com/shopifycloud/web/assets/v1/) [https://almond-sandpiper-6593.twil.io/assets/;script-src](https://almond-sandpiper-6593.twil.io/assets/;script-src) 'self' 'unsafe-inline' 'unsafe-eval' [cdn.shopify.com](http://cdn.shopify.com) [cdn.shopifycdn.net](http://cdn.shopifycdn.net) *.[shopifycs.com](http://shopifycs.com) [www.google-analytics.com](http://www.google-analytics.com) [stats.g.doubleclick.net](http://stats.g.doubleclick.net) [app.shopify.com](http://app.shopify.com) [app.myshopify.com](http://app.myshopify.com) [c.paypal.com](http://c.paypal.com) [www.paypal.com](http://www.paypal.com) [appcenter.intuit.com](http://appcenter.intuit.com) [mpsnare.iesnare.com](http://mpsnare.iesnare.com) [api.stripe.com](http://api.stripe.com) [maps.googleapis.com](http://maps.googleapis.com) [js.braintreegateway.com](http://js.braintreegateway.com) [songbird.cardinalcommerce.com](http://songbird.cardinalcommerce.com) [hcaptcha.com](http://hcaptcha.com) *.[hcaptcha.com](http://hcaptcha.com) [www.youtube.com](http://www.youtube.com) [s.ytimg.com;style-src](http://s.ytimg.com;style-src) 'self' 'unsafe-inline' data: blob: https://*;upgrade-insecure-requests;worker-src 'self' blob: ``` The problem here is that `upgrade-insecure-requests` directive at the very end
**[Paulo Margarido](https://github.com/paulomarg)** [2023-05-25 02:25PM UTC]
When running on `localhost`, that directive seems to be forcing chrome to redirect to `[https://localhost](https://localhost)...` which naturally fails. I don't know exactly why localhost isn't being considered a secure location, but it essentially prevents us from using the "bounce" page on localhost in the remix template. Like I said, this is pretty niche and we can live without it for now, but _if_ it were possible to set the header up so localhost requests don't get bumped to `https`, that'd be perfect
**[Zack Deveau](https://github.com/fresh-eggs)** [2023-05-26 08:19PM UTC]
shared this in a couple channels
**[Paulo Margarido](https://github.com/paulomarg)** [2023-05-26 08:36PM UTC]
awesome, I appreciate that Zack!

Created originally at 2023-05-29 06:15PM UTC by Paulo Margarido

paulomarg commented 1 year ago

Marking this as blocked because it seems to be an issue with either chrome or the way admin sets up the headers, so there's not a lot we can do.