From 7b714a4a7a98057a6fdd11fb55612853870fefa4 Mon Sep 17 00:00:00 2001 From: Paulo Margarido <64600052+paulomarg@users.noreply.github.com> Date: Thu, 1 Sep 2022 16:29:17 -0400 Subject: [PATCH] Fixing embedded_redirect_url redirect loop --- CHANGELOG.md | 2 ++ lib/shopify_app/controller_concerns/login_protection.rb | 3 ++- lib/shopify_app/controller_concerns/redirect_for_embedded.rb | 5 +++-- lib/shopify_app/controller_concerns/sanitized_params.rb | 5 +++-- test/controllers/sessions_controller_test.rb | 2 +- 5 files changed, 11 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ca8ffb593..de774d5e3 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/lib/shopify_app/controller_concerns/login_protection.rb b/lib/shopify_app/controller_concerns/login_protection.rb index d0a41c70d..f7cccf017 100644 --- a/lib/shopify_app/controller_concerns/login_protection.rb +++ b/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) diff --git a/lib/shopify_app/controller_concerns/redirect_for_embedded.rb b/lib/shopify_app/controller_concerns/redirect_for_embedded.rb index 63ca63d41..88704f79d 100644 --- a/lib/shopify_app/controller_concerns/redirect_for_embedded.rb +++ b/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}" diff --git a/lib/shopify_app/controller_concerns/sanitized_params.rb b/lib/shopify_app/controller_concerns/sanitized_params.rb index b1b737559..6b01b8d18 100644 --- a/lib/shopify_app/controller_concerns/sanitized_params.rb +++ b/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 diff --git a/test/controllers/sessions_controller_test.rb b/test/controllers/sessions_controller_test.rb index 5bad04db5..3ba4c2381 100644 --- a/test/controllers/sessions_controller_test.rb +++ b/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