Redirect embedded apps to the provided return_to if it's a fully-formed URL #1746
Conversation
| @@ -78,6 +78,11 @@ def redirect_to_app | |||
| end | |||
| end | |||
|
|
|||
| def fully_formed_url?(return_to) | |||
| uri = Addressable::URI.parse(return_to) | |||
| uri.present? && uri.scheme.present? && uri.host.present? | |||
There was a problem hiding this comment.
Outlined here what the scheme and host refer to: https://github.com/sporkmonger/addressable#example-usage
| uri = Addressable::URI.parse(return_to) | ||
| uri.present? && uri.scheme.present? && uri.host.present? | ||
| end | ||
|
|
||
| def decoded_host | ||
| @decoded_host ||= ShopifyAPI::Auth.embedded_app_url(params[:host]) |
There was a problem hiding this comment.
The other thing this this embedded_app_url does is append the Context's API key to the path. I'm doing some research of the context of what this is needed for. It sounds like your app is fine without it but I'm curious if others would
| @@ -69,7 +69,7 @@ def update_rails_cookie(api_session, cookie) | |||
| end | |||
|
|
|||
| def redirect_to_app | |||
| if ShopifyAPI::Context.embedded? | |||
| if ShopifyAPI::Context.embedded? && !fully_formed_url?(session[:return_to]) | |||
There was a problem hiding this comment.
While I see what this is doing now because I have the context, it isn't super clear as to the intent of the feature. Maybe we add another return_to local override that says return_to = return_url if full_formed_url?. Something similar to line 74's return_to override.
There was a problem hiding this comment.
Makes sense, I've refactored a bit so it's hopefully more clear. Let me know what you think
|
@rondanino that was going to my next question, I figure that changes get batched into new releases and once that release goes out we'll bump the gem in Shop Server @nelsonwittwer what's the release process generally like for minor changes like this? |
What this PR does
Fixes https://github.com/Shopify/shop-server/issues/65307.
We have a flow in Shop where we're able to install the Shop channel through an internal admin tool called Shop Internal. This is generally used for testing in Spin and production. In previous versions of this gem, the installation flow brought us back to Shop Internal at the end. However, now the gem assumes that embedded app installations should always get redirected to Shopify Admin. This fixes that issue by redirecting to the provided
return_tofor embedded apps if it's a fully-formed URL (ie. not just a path within Shopify Admin).Reviewer's guide to testing
I tested both our internal installation flow and our regular installation flow and both are working. The internal installation flow brings us back to Shop Server, and the regular one brings us to the Admin.
Internal flow
spin up shop-servercode --add $(bundle show shopify_app)to import theshopify_appgem codedev candMerchant::ShopRecord.destroy_allRegular flow
dev candMerchant::ShopRecord.destroy_allThings to focus on
That this won't have any adverse effects for other apps.
Checklist
Before submitting the PR, please consider if any of the following are needed:
CHANGELOG.mdif the changes would impact usersREADME.md, if appropriate./docs, if necessary