Merge pull request #913 from Shopify/fix-open-redirect

Return to relative paths only

app/controllers/shopify_app/sessions_controller.rb

@@ -125,7 +125,7 @@ def validate_shop_presence
     end
 
     def copy_return_to_param_to_session
-      session[:return_to] = params[:return_to] if params[:return_to]
+      session[:return_to] = RedirectSafely.make_safe(params[:return_to], '/') if params[:return_to]
     end
 
     def render_invalid_shop_error

lib/shopify_app.rb

@@ -4,6 +4,7 @@
 # deps
 require 'shopify_api'
 require 'omniauth-shopify-oauth2'
+require 'redirect_safely'
 
 module ShopifyApp
   def self.rails6?

lib/shopify_app/controller_concerns/login_protection.rb

@@ -139,7 +139,7 @@ def login_url_params(top_level:)
       query_params = {}
       query_params[:shop] = sanitized_params[:shop] if params[:shop].present?
 
-      return_to = session[:return_to] || params[:return_to]
+      return_to = RedirectSafely.make_safe(session[:return_to] || params[:return_to], nil)
 
       if return_to.present? && return_to_param_required?
         query_params[:return_to] = return_to

shopify_app.gemspec

@@ -18,6 +18,7 @@ Gem::Specification.new do |s|
   s.add_runtime_dependency('shopify_api', '~> 9.0.2')
   s.add_runtime_dependency('omniauth-shopify-oauth2', '~> 2.2.2')
   s.add_runtime_dependency('jwt', '~> 2.2.1')
+  s.add_runtime_dependency('redirect_safely', '~> 1.0')
 
   s.add_development_dependency('rake')
   s.add_development_dependency('byebug')

test/controllers/sessions_controller_test.rb

@@ -83,6 +83,16 @@ class SessionsControllerTest < ActionController::TestCase
       assert_redirected_to_top_level(shopify_domain)
     end
 
+    test '#new stores root path when return_to url is absolute' do
+      get :new, params: { shop: 'my-shop', return_to: '//example.com' }
+      assert_equal '/', session[:return_to]
+    end
+
+    test '#new stores only relative path for return_to in the session' do
+      get :new, params: { shop: 'my-shop', return_to: '/page' }
+      assert_equal '/page', session[:return_to]
+    end
+
     test '#new sets the top_level_oauth cookie if a valid shop param exists and user agent supports cookie partitioning' do
       request.env['HTTP_USER_AGENT'] = 'Version/12.0 Safari'
       get :new, params: { shop: 'my-shop' }