Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing embedded_redirect_url redirect loop #1497

Merged
merged 1 commit into from
Sep 2, 2022

Conversation

paulomarg
Copy link
Contributor

@paulomarg paulomarg commented Sep 1, 2022

What this PR does

There are some (quite) edge case scenarios where the embedded_redirect_url config will trigger a 302 loop to itself, when reloading the app in the admin after a scope change, in production (or SSR) mode.

To avoid that issue, we will simply not redirect in that scenario, so that the request is considered valid and reaches the backend - there is an assumption here that this page does not contain sensitive information and does not handle actual data, but just blindly redirects.

Reviewer's guide to testing

  1. Create a ruby app with the CLI and follow the steps to run it in dev mode. Open the app to install it and kill the server, keeping the browser window open.
  2. Run npm run build --api-key=<apiKey>
  3. Run ngrok http 3000 on a window
  4. Run cd web in the app
  5. Run RAILS_SERVE_STATIC_FILES=1 SHOPIFY_API_KEY=<apiKey> SHOPIFY_API_SECRET=<apiSecret> SCOPES=write_products,read_orders HOST=<ngrokAddress> rails server -b 0.0.0.0 -e production -p 3000
  6. Refresh the admin page now that the scopes have changed.
  7. With the current app, you'll see the iframe redirecting internally to login and borking
  8. With this PR, it should break out of the iframe and show the grant screen

Checklist

Before submitting the PR, please consider if any of the following are needed:

  • Update CHANGELOG.md if the changes would impact users

@paulomarg paulomarg requested a review from a team September 1, 2022 20:24
@paulomarg paulomarg force-pushed the fix_embedded_url_redirect_loop branch from 19f01b8 to 0db27ad Compare September 1, 2022 20:25
@paulomarg paulomarg force-pushed the fix_embedded_url_redirect_loop branch from 0db27ad to 7b714a4 Compare September 1, 2022 20:29
@paulomarg paulomarg merged commit b6380a7 into main Sep 2, 2022
@paulomarg paulomarg deleted the fix_embedded_url_redirect_loop branch September 2, 2022 12:59
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems September 2, 2022 13:20 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants