Fixing embedded_redirect_url redirect loop

CHANGELOG.md

@@ -1,5 +1,7 @@
 Unreleased
 ----------
+
+* Fixed an issue where the `embedded_redirect_url` could lead to a redirect loop in server-side rendered (or production) apps. [#1497](https://github.com/Shopify/shopify_app/pull/1497)
 * Fixes bug where webhooks were generated with addresses instead of the [path the Ruby API](https://github.com/Shopify/shopify-api-ruby/blob/7a08ae9d96a7a85abd0113dae4eb76398cba8c64/lib/shopify_api/webhooks/registrations/http.rb#L12) is expecting [#1474](https://github.com/Shopify/shopify_app/pull/1474). The breaking change that was accidentially already shipped was that `address` attribute for webhooks should be paths not addresses with `https://` and the host name. While the `address` attribute name will still work assuming the value is a path, this name is deprecated. Please configure webhooks with the `path` attribute name instead.
 * Deduce webhook path from deprecated webhook address if initializer uses address attribute. This makes this attribute change a non-breaking change for those upgrading.
 

lib/shopify_app/controller_concerns/login_protection.rb

@@ -117,7 +117,8 @@ def redirect_to_login
         else
           referer = URI(request.referer || "/")
           path = referer.path
-          query = "#{referer.query}&#{sanitized_params.to_query}"
+          query = Rack::Utils.parse_nested_query(referer.query)
+          query = query.merge(sanitized_params).to_query
         end
         session[:return_to] = query.blank? ? path.to_s : "#{path}?#{query}"
         redirect_to(login_url_with_optional_shop)

lib/shopify_app/controller_concerns/redirect_for_embedded.rb

@@ -15,7 +15,8 @@ def embedded_param?
     end
 
     def redirect_for_embedded
-      redirect_to(redirect_uri_for_embedded)
+      # Don't actually redirect if we're already in the redirect route - we want the request to reach the FE
+      redirect_to(redirect_uri_for_embedded) unless request.path == ShopifyApp.configuration.embedded_redirect_url
     end
 
     def redirect_uri_for_embedded
@@ -26,7 +27,7 @@ def redirect_uri_for_embedded
       redirect_query_params[:host] ||= params[:host] if params[:host].present?
       redirect_uri = "#{redirect_uri}?#{redirect_query_params.to_query}" if redirect_query_params.present?
 
-      query_params = sanitized_params.except(:redirect_uri, :embedded)
+      query_params = sanitized_params.except(:redirect_uri)
       query_params[:redirectUri] = redirect_uri
 
       "#{ShopifyApp.configuration.embedded_redirect_url}?#{query_params.to_query}"

lib/shopify_app/controller_concerns/sanitized_params.rb

@@ -25,9 +25,10 @@ def sanitize_shop_param(params)
     end
 
     def sanitized_params
-      request.query_parameters.clone.tap do |query_params|
+      parameters = request.post? ? request.request_parameters : request.query_parameters
+      parameters.clone.tap do |params_copy|
         if params[:shop].is_a?(String)
-          query_params[:shop] = sanitize_shop_param(params)
+          params_copy[:shop] = sanitize_shop_param(params)
         end
       end
     end

test/controllers/sessions_controller_test.rb

@@ -383,7 +383,7 @@ def assert_redirected_to_top_level(shop_domain, expected_url = nil)
     def assert_redirected_to_embedded(shop_domain, base_embedded_url = nil)
       assert_not_nil base_embedded_url
       redirect_uri = "https://test.host/login?shop=#{shop_domain}"
-      expected_url = base_embedded_url + "?redirectUri=#{CGI.escape(redirect_uri)}" + "&shop=#{shop_domain}"
+      expected_url = base_embedded_url + "?embedded=1&redirectUri=#{CGI.escape(redirect_uri)}" + "&shop=#{shop_domain}"
 
       assert_redirected_to(expected_url)
     end