From 29636547e12fe56558cd1a635830df9095d73915 Mon Sep 17 00:00:00 2001 From: Nelson Date: Fri, 9 Dec 2022 10:13:12 -0500 Subject: [PATCH] redirect to root during oauth#callback if attempted phishing attack (#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 | 2 ++ .../shopify_app/callback_controller.rb | 19 +++++++++++---- test/controllers/callback_controller_test.rb | 24 ++++++++++++++++--- 3 files changed, 38 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 17dbbb5b9..d56bd5787 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/app/controllers/shopify_app/callback_controller.rb b/app/controllers/shopify_app/callback_controller.rb index c79b1737f..e20eabbd1 100644 --- a/app/controllers/shopify_app/callback_controller.rb +++ b/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) diff --git a/test/controllers/callback_controller_test.rb b/test/controllers/callback_controller_test.rb index 94b807e74..b782d7439 100644 --- a/test/controllers/callback_controller_test.rb +++ b/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,9 +246,11 @@ 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 @@ -240,7 +258,7 @@ class CallbackControllerTest < ActionController::TestCase 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