redirect to root during oauth#callback if attempted phishing attack (S…

…hopify#1605)

* render 404 if attempted phishing attack

* changelog

* rubocop style fixes

* root address instead of 404

* redirect to param[:host] after sanitization

* Update app/controllers/shopify_app/callback_controller.rb

Co-authored-by: Bill Klenotiz <54754550+klenotiw@users.noreply.github.com>

Co-authored-by: Bill Klenotiz <54754550+klenotiw@users.noreply.github.com>

CHANGELOG.md

@@ -9,6 +9,8 @@ Unreleased
 * Log a deprecation warning for the use of incompatible controller concerns [#1560](https://github.com/Shopify/shopify_app/pull/1560)
 * Fixes bug with expired sessions for embedded apps returning a 500 instead of 401 [#1580](https://github.com/Shopify/shopify_app/pull/1580)
 * Generator properly handles uninstall [#1597](https://github.com/Shopify/shopify_app/pull/1597)
+* Move ownership for session persistence from library to this gem [#1563](https://github.com/Shopify/shopify_app/pull/1563)
+* Patch phishing vulnerability [#1605](https://github.com/Shopify/shopify_app/pull/1605)
 * Remove `Itp` from `LoginProtection`. See the [upgrading docs](https://github.com/Shopify/shopify_app/blob/main/docs/Upgrading.md) for more information. [#1604](https://github.com/Shopify/shopify_app/pull/1604)
 
 21.2.0 (Oct 25, 2022)

app/controllers/shopify_app/callback_controller.rb

@@ -20,7 +20,7 @@ def callback
       return respond_with_user_token_flow if start_user_token_flow?(api_session)
 
       perform_post_authenticate_jobs(api_session)
-      respond_successfully if check_billing(api_session)
+      redirect_to_app if check_billing(api_session)
     end
 
     private
@@ -68,15 +68,26 @@ def update_rails_cookie(api_session, cookie)
       ShopifyApp::Logger.debug("Saving Shopify user ID to cookie")
     end
 
-    def respond_successfully
+    def redirect_to_app
       if ShopifyAPI::Context.embedded?
-        return_to = session.delete(:return_to) || ""
-        redirect_to(ShopifyAPI::Auth.embedded_app_url(params[:host]) + 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
     end
 
+    def decoded_host
+      @decoded_host ||= ShopifyAPI::Auth.embedded_app_url(params[:host])
+    end
+
+    # host param doesn't match the configured myshopify_domain
+    def deduced_phishing_attack?
+      sanitized_host = ShopifyApp::Utils.sanitize_shop_domain(URI(decoded_host).host)
+      sanitized_host.nil?
+    end
+
     def respond_with_error
       flash[:error] = I18n.t("could_not_log_in")
       redirect_to(login_url_with_optional_shop)

test/controllers/callback_controller_test.rb

@@ -30,7 +30,8 @@ class CallbackControllerTest < ActionController::TestCase
       I18n.locale = :en
       @stubbed_session = ShopifyAPI::Auth::Session.new(shop: "shop", access_token: "token")
       @stubbed_cookie = ShopifyAPI::Auth::Oauth::SessionCookie.new(value: "", expires: Time.now)
-      host = Base64.strict_encode64("#{ShopifyAPI::Context.host_name}/admin")
+      @host = "little-shoppe-of-horrors.#{ShopifyApp.configuration.myshopify_domain}"
+      host = Base64.strict_encode64(@host + "/admin")
       @callback_params = { shop: "shop", code: "code", state: "state", timestamp: "timestamp", host: host,
                            hmac: "hmac", }
       @auth_query = ShopifyAPI::Auth::Oauth::AuthQuery.new(**@callback_params)
@@ -95,6 +96,21 @@ class CallbackControllerTest < ActionController::TestCase
       get :callback, params: @callback_params
     end
 
+    test "#callback returns to root if the host in the param doesn't match configuration indicating a potential phishing attack" do
+      host = "hackerman-evil-site.com/hide-yo-wife-hide-yo-kids"
+      encoded_host = Base64.strict_encode64(host + "/admin")
+      hacker_params = @callback_params.dup
+      hacker_params[:host] = encoded_host
+      ShopifyAPI::Auth::Oauth::AuthQuery.stubs(:new).with(**hacker_params).returns(@auth_query)
+      ShopifyAPI::Auth::Oauth.expects(:validate_auth_callback).returns({
+        cookie: @stubbed_cookie,
+        session: @stubbed_session,
+      })
+
+      get :callback, params: hacker_params
+      assert_redirected_to ShopifyApp.configuration.root_url
+    end
+
     test "#callback sets the shopify_user_id in the Rails session when session is online" do
       associated_user = ShopifyAPI::Auth::AssociatedUser.new(
         id: 42,
@@ -230,17 +246,19 @@ class CallbackControllerTest < ActionController::TestCase
       ShopifyAppConfigurer.setup_context # to reset the context, as there's no attr_writer for embedded
       mock_oauth
 
+      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 "/?host=#{@callback_params[:host]}&shop=#{@callback_params[:shop]}.myshopify.com"
+      assert_redirected_to non_embedded_host
     end
 
     test "#callback redirects to the embedded app url for embedded" do
       mock_oauth
 
       get :callback, params: @callback_params # host is required for App Bridge 2.0
 
-      assert_redirected_to "https://test.host/admin/apps/key"
+      assert_redirected_to "https://#{@host}/admin/apps/key"
     end
 
     test "#callback performs install_webhook job after authentication" do