From 118b29ef96cec05b3b9a6b53fd040701faa34e30 Mon Sep 17 00:00:00 2001 From: Layne Koftinow-Mikan Date: Thu, 9 Nov 2023 11:58:46 -0500 Subject: [PATCH 1/6] Redirect embedded apps to the provided return_to if it's a fully-formed URL --- .../shopify_app/callback_controller.rb | 13 ++++++--- test/controllers/callback_controller_test.rb | 29 ++++++++++++++++--- 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/app/controllers/shopify_app/callback_controller.rb b/app/controllers/shopify_app/callback_controller.rb index 5225f5169..33ecc9305 100644 --- a/app/controllers/shopify_app/callback_controller.rb +++ b/app/controllers/shopify_app/callback_controller.rb @@ -69,15 +69,20 @@ def update_rails_cookie(api_session, cookie) end def redirect_to_app - if ShopifyAPI::Context.embedded? - return_to = "#{decoded_host}#{session.delete(:return_to)}" - return_to = ShopifyApp.configuration.root_url if deduced_phishing_attack? - redirect_to(return_to, allow_other_host: true) + if ShopifyAPI::Context.embedded? && !fully_formed_url?(session[:return_to]) + return_to = "#{decoded_host}#{session.delete(:return_to)}" + return_to = ShopifyApp.configuration.root_url if deduced_phishing_attack? + redirect_to(return_to, allow_other_host: true) else redirect_to(return_address) end end + def fully_formed_url?(return_to) + 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]) end diff --git a/test/controllers/callback_controller_test.rb b/test/controllers/callback_controller_test.rb index a5d168db7..32bade4f0 100644 --- a/test/controllers/callback_controller_test.rb +++ b/test/controllers/callback_controller_test.rb @@ -255,15 +255,27 @@ class CallbackControllerTest < ActionController::TestCase ShopifyApp.configuration.embedded_app = false ShopifyAppConfigurer.setup_context # to reset the context, as there's no attr_writer for embedded mock_oauth + session[:return_to] = "not-real.little-test-shoppe-of-horrs.com" - non_embedded_host = "not-real.little-test-shoppe-of-horrs.com" - @controller.stubs(:return_address).returns(non_embedded_host) get :callback, params: @callback_params # host is required for App Bridge 2.0 - assert_redirected_to non_embedded_host + host = CGI.escape(@callback_params[:host]) + shop = @callback_params[:shop] + ".myshopify.com" + assert_redirected_to "not-real.little-test-shoppe-of-horrs.com?host=#{host}&shop=#{shop}" end - test "#callback redirects to the embedded app url for embedded" do + test "callback redirects to the return_to for embedded app when return_to is a fully-formed URL" do + mock_oauth + session[:return_to] = "https://example.com/return_to?foo=bar" + + get :callback, params: @callback_params # host is required for App Bridge 2.0 + + host = CGI.escape(@callback_params[:host]) + shop = @callback_params[:shop] + ".myshopify.com" + assert_redirected_to "https://example.com/return_to?foo=bar&host=#{host}&shop=#{shop}" + end + + test "#callback redirects to the embedded app url when app is embedded and return_to is not provided" do mock_oauth get :callback, params: @callback_params # host is required for App Bridge 2.0 @@ -271,6 +283,15 @@ class CallbackControllerTest < ActionController::TestCase assert_redirected_to "https://#{@host}/admin/apps/key" end + test "#callback redirects to the embedded app url when app is embedded and return_to is a path" do + mock_oauth + session[:return_to] = "/return_to/path" + + get :callback, params: @callback_params # host is required for App Bridge 2.0 + + assert_redirected_to "https://#{@host}/admin/apps/key/return_to/path" + end + test "#callback performs install_webhook job after authentication" do mock_oauth From 7f5cbf0e477869206aed900770e54ac61fa8c4eb Mon Sep 17 00:00:00 2001 From: Layne Koftinow-Mikan Date: Thu, 9 Nov 2023 12:01:45 -0500 Subject: [PATCH 2/6] Fix rubocop issues --- app/controllers/shopify_app/callback_controller.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/shopify_app/callback_controller.rb b/app/controllers/shopify_app/callback_controller.rb index 33ecc9305..6c9492a77 100644 --- a/app/controllers/shopify_app/callback_controller.rb +++ b/app/controllers/shopify_app/callback_controller.rb @@ -70,9 +70,9 @@ def update_rails_cookie(api_session, cookie) def redirect_to_app if ShopifyAPI::Context.embedded? && !fully_formed_url?(session[:return_to]) - return_to = "#{decoded_host}#{session.delete(:return_to)}" - return_to = ShopifyApp.configuration.root_url if deduced_phishing_attack? - redirect_to(return_to, allow_other_host: true) + return_to = "#{decoded_host}#{session.delete(:return_to)}" + return_to = ShopifyApp.configuration.root_url if deduced_phishing_attack? + redirect_to(return_to, allow_other_host: true) else redirect_to(return_address) end From 3cf53d576ce2367d96c29f90765e13fcdc6008f7 Mon Sep 17 00:00:00 2001 From: Layne Koftinow-Mikan Date: Thu, 9 Nov 2023 12:34:10 -0500 Subject: [PATCH 3/6] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 966038c72..cfc9e90f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ Unreleased ---------- * Fixes bug with `WebhooksManager#recreate_webhooks!` where we failed to register topics in the registry[#1743](https://github.com/Shopify/shopify_app/pull/1704) +* Allow embedded apps to provide a full URL to get redirected to, rather than defaulting to Shopify Admin [#1746](https://github.com/Shopify/shopify_app/pull/1746) 21.7.0 (Oct 12, 2023) ---------- From 286da23dd969ed4921f595117123ce9178438b21 Mon Sep 17 00:00:00 2001 From: Layne Koftinow-Mikan Date: Thu, 9 Nov 2023 14:37:33 -0500 Subject: [PATCH 4/6] Refactor logic a bit --- app/controllers/shopify_app/callback_controller.rb | 14 ++++++++++---- test/controllers/callback_controller_test.rb | 4 +--- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/app/controllers/shopify_app/callback_controller.rb b/app/controllers/shopify_app/callback_controller.rb index 6c9492a77..4dc6266bd 100644 --- a/app/controllers/shopify_app/callback_controller.rb +++ b/app/controllers/shopify_app/callback_controller.rb @@ -69,10 +69,16 @@ def update_rails_cookie(api_session, cookie) end def redirect_to_app - if ShopifyAPI::Context.embedded? && !fully_formed_url?(session[:return_to]) - return_to = "#{decoded_host}#{session.delete(:return_to)}" - return_to = ShopifyApp.configuration.root_url if deduced_phishing_attack? - redirect_to(return_to, allow_other_host: true) + if ShopifyAPI::Context.embedded? + return_to = session.delete(:return_to) + redirect_to = if fully_formed_url?(return_to) + return_to + else + "#{decoded_host}#{return_to}" + end + + redirect_to = ShopifyApp.configuration.root_url if deduced_phishing_attack? + redirect_to(redirect_to, allow_other_host: true) else redirect_to(return_address) end diff --git a/test/controllers/callback_controller_test.rb b/test/controllers/callback_controller_test.rb index 32bade4f0..062589fa8 100644 --- a/test/controllers/callback_controller_test.rb +++ b/test/controllers/callback_controller_test.rb @@ -270,9 +270,7 @@ class CallbackControllerTest < ActionController::TestCase get :callback, params: @callback_params # host is required for App Bridge 2.0 - host = CGI.escape(@callback_params[:host]) - shop = @callback_params[:shop] + ".myshopify.com" - assert_redirected_to "https://example.com/return_to?foo=bar&host=#{host}&shop=#{shop}" + assert_redirected_to "https://example.com/return_to?foo=bar" end test "#callback redirects to the embedded app url when app is embedded and return_to is not provided" do From 333b1c17f9a13b8fb7dbb4e0b5ab9982b1bb7fb9 Mon Sep 17 00:00:00 2001 From: Layne Koftinow-Mikan Date: Thu, 9 Nov 2023 14:38:54 -0500 Subject: [PATCH 5/6] Run rubocop --- app/controllers/shopify_app/callback_controller.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/controllers/shopify_app/callback_controller.rb b/app/controllers/shopify_app/callback_controller.rb index 4dc6266bd..ae938e6b0 100644 --- a/app/controllers/shopify_app/callback_controller.rb +++ b/app/controllers/shopify_app/callback_controller.rb @@ -72,10 +72,10 @@ def redirect_to_app if ShopifyAPI::Context.embedded? return_to = session.delete(:return_to) redirect_to = if fully_formed_url?(return_to) - return_to - else - "#{decoded_host}#{return_to}" - end + return_to + else + "#{decoded_host}#{return_to}" + end redirect_to = ShopifyApp.configuration.root_url if deduced_phishing_attack? redirect_to(redirect_to, allow_other_host: true) From f961bab6f061dce16aae4d26d7ee595bad4b555f Mon Sep 17 00:00:00 2001 From: Layne Koftinow-Mikan Date: Thu, 9 Nov 2023 14:40:35 -0500 Subject: [PATCH 6/6] Revert changes to test --- test/controllers/callback_controller_test.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/test/controllers/callback_controller_test.rb b/test/controllers/callback_controller_test.rb index 062589fa8..daa87e3d0 100644 --- a/test/controllers/callback_controller_test.rb +++ b/test/controllers/callback_controller_test.rb @@ -255,13 +255,12 @@ class CallbackControllerTest < ActionController::TestCase ShopifyApp.configuration.embedded_app = false ShopifyAppConfigurer.setup_context # to reset the context, as there's no attr_writer for embedded mock_oauth - session[:return_to] = "not-real.little-test-shoppe-of-horrs.com" + non_embedded_host = "not-real.little-test-shoppe-of-horrs.com" + @controller.stubs(:return_address).returns(non_embedded_host) get :callback, params: @callback_params # host is required for App Bridge 2.0 - host = CGI.escape(@callback_params[:host]) - shop = @callback_params[:shop] + ".myshopify.com" - assert_redirected_to "not-real.little-test-shoppe-of-horrs.com?host=#{host}&shop=#{shop}" + assert_redirected_to non_embedded_host end test "callback redirects to the return_to for embedded app when return_to is a fully-formed URL" do