Merge pull request #1887 from Shopify/fix_top_level_redirection

Fix full page redirection at top level

Author: paulomarg

CHANGELOG.md

@@ -1,6 +1,8 @@
 Unreleased
 ----------
 
+- Handle edge case where we attempted to redirect to login when already at the top level [#1887](https://github.com/Shopify/shopify_app/pull/1887)
+
 22.3.0 (July 24, 2024)
 ----------
 - Deprecate `ShopifyApp::JWTMiddleware`. And remove internal usage.  Any existing app code relying on decoded JWT contents set from `request.env` should instead include the `WithShopifyIdToken` concern and call its respective methods. [#1861](https://github.com/Shopify/shopify_app/pull/1861) [Migration Guide](/docs/Upgrading.md#v2300---removed-shopifyappjwtmiddleware)

app/views/shopify_app/shared/redirect.html.erb

@@ -6,8 +6,13 @@
   <base target="_top">
   <title>Redirecting…</title>
 
+  <%
+    is_iframe = local_assigns.key?(:is_iframe) ? is_iframe : true
+    if is_iframe
+  %>
   <meta name="shopify-api-key" content="<%= ShopifyApp.configuration.api_key %>">
   <%= javascript_include_tag "https://cdn.shopify.com/shopifycloud/app-bridge.js" %>
+  <% end %>
 
   <%= javascript_include_tag('shopify_app/redirect', crossorigin: 'anonymous', integrity: true) %>
 </head>

lib/shopify_app/controller_concerns/login_protection.rb

@@ -204,7 +204,7 @@ def fullpage_redirect_to(url)
         render(
           "shopify_app/shared/redirect",
           layout: false,
-          locals: { url: url, current_shopify_domain: current_shopify_domain },
+          locals: { url: url, current_shopify_domain: current_shopify_domain, is_iframe: embedded? },
         )
       else
         redirect_to(url)

lib/shopify_app/controller_concerns/sanitized_params.rb

@@ -33,5 +33,9 @@ def sanitized_params
         end
       end
     end
+
+    def embedded?
+      params[:embedded] == "1" || request.env["HTTP_SEC_FETCH_DEST"] == "iframe"
+    end
   end
 end

lib/shopify_app/controller_concerns/token_exchange.rb

@@ -94,10 +94,6 @@ def redirect_to_bounce_page
       )
     end
 
-    def embedded?
-      params[:embedded] == "1" || request.env["HTTP_SEC_FETCH_DEST"] == "iframe"
-    end
-
     def online_token_configured?
       ShopifyApp.configuration.online_token_configured?
     end
@@ -108,7 +104,7 @@ def fullpage_redirect_to(url)
       render(
         "shopify_app/shared/redirect",
         layout: false,
-        locals: { url: url, current_shopify_domain: current_shopify_domain },
+        locals: { url: url, current_shopify_domain: current_shopify_domain, is_iframe: embedded? },
       )
     end
   end

test/shopify_app/controller_concerns/login_protection_test.rb

@@ -509,7 +509,7 @@ class LoginProtectionControllerTest < ActionController::TestCase
     with_application_test_routes do
       example_shop = "shop.myshopify.com"
       get :redirect, params: { shop: example_shop }
-      assert_fullpage_redirected(example_shop, response)
+      assert_fullpage_redirected(example_shop, false)
     end
   end
 
@@ -519,7 +519,24 @@ class LoginProtectionControllerTest < ActionController::TestCase
       ShopifyApp::SessionRepository.expects(:load_session)
         .returns(ShopifyAPI::Auth::Session.new(shop: example_shop))
       get :redirect
-      assert_fullpage_redirected(example_shop, response)
+      assert_fullpage_redirected(example_shop, false)
+    end
+  end
+
+  test "#fullpage_redirect_to loads app bridge when embedded param is set" do
+    with_application_test_routes do
+      example_shop = "shop.myshopify.com"
+      get :redirect, params: { shop: example_shop, embedded: "1" }
+      assert_fullpage_redirected(example_shop, true)
+    end
+  end
+
+  test "#fullpage_redirect_to loads app bridge when Sec-Fetch-Dest header is present" do
+    with_application_test_routes do
+      example_shop = "shop.myshopify.com"
+      request.headers.merge!({ "Sec-Fetch-Dest" => "iframe" })
+      get :redirect, params: { shop: example_shop }
+      assert_fullpage_redirected(example_shop, true)
     end
   end
 
@@ -603,14 +620,18 @@ class LoginProtectionControllerTest < ActionController::TestCase
 
   private
 
-  def assert_fullpage_redirected(shop_domain, _response)
+  def assert_fullpage_redirected(shop_domain, expect_embedded)
     example_url = "https://example.com"
 
     assert_template("shared/redirect")
     assert_select "[id=redirection-target]", 1 do |elements|
       assert_equal "{\"myshopifyUrl\":\"https://#{shop_domain}\",\"url\":\"#{example_url}\"}",
         elements.first["data-target"]
     end
+    assert_select "script" do |elements|
+      assert elements.any? { |element| element["src"] =~ %r/\/assets\/shopify_app\/redirect-[^\.]*\.js/ }
+      assert_equal expect_embedded, elements.any? { |element| element["src"] == "https://cdn.shopify.com/shopifycloud/app-bridge.js" }
+    end
   end
 
   def with_application_test_routes