Merge pull request #75 from basecamp/flavorjones/handle-invalid-paths

Gracefully handle more invalid paths

Author: flavorjones

lib/google_sign_in/redirect_protector.rb

@@ -9,20 +9,27 @@ class Violation < StandardError; end
     QUALIFIED_URL_PATTERN = /\A#{URI::DEFAULT_PARSER.make_regexp}\z/
 
     def ensure_same_origin(target, source)
-      if (target =~ QUALIFIED_URL_PATTERN && origin_of(target) == origin_of(source)) ||
-         target =~ URI::DEFAULT_PARSER.regexp[:ABS_PATH]
-        return
+      unless uri_same_origin?(target, source) || absolute_path?(target)
+        raise Violation, "Redirect target #{target.inspect} does not have same origin as request #{source.inspect}"
       end
-
-      raise Violation, "Redirect target #{target.inspect} does not have same origin as request (expected #{origin_of(source)})"
     end
 
     private
+      def uri_same_origin?(target, source)
+        target =~ QUALIFIED_URL_PATTERN && origin_of(target) == origin_of(source)
+      rescue ArgumentError, URI::Error
+        false
+      end
+
+      def absolute_path?(target)
+        target =~ URI::DEFAULT_PARSER.regexp[:ABS_PATH] && URI(target).host.nil? && !target.start_with?("//")
+      rescue ArgumentError, URI::Error
+        false
+      end
+
       def origin_of(url)
         uri = URI(url)
         "#{uri.scheme}://#{uri.host}:#{uri.port}"
-      rescue ArgumentError
-        nil
       end
   end
 end

test/controllers/callbacks_controller_test.rb

@@ -103,6 +103,48 @@ class GoogleSignIn::CallbacksControllerTest < ActionDispatch::IntegrationTest
     assert_response :bad_request
   end
 
+  test "protecting against open redirects given a malformed URI" do
+    post google_sign_in.authorization_url, params: { proceed_to: 'http://www.example.com\n\r@\n\revil.example.org/login' }
+    assert_response :redirect
+
+    get google_sign_in.callback_url(code: '4/SgCpHSVW5-Cy', state: flash[:state])
+    assert_response :bad_request
+  end
+
+  test "rejects proceed_to paths if they are relative" do
+    post google_sign_in.authorization_url, params: { proceed_to: 'login' }
+    assert_response :redirect
+
+    get google_sign_in.callback_url(code: '4/SgCpHSVW5-Cy', state: flash[:state])
+    assert_response :bad_request
+  end
+
+  test "accepts proceed_to paths if they are absolute" do
+    post google_sign_in.authorization_url, params: { proceed_to: '/login' }
+    assert_response :redirect
+
+    stub_token_for '4/SgCpHSVW5-Cy', access_token: 'ya29.GlwIBo', id_token: 'eyJhbGciOiJSUzI'
+
+    get google_sign_in.callback_url(code: '4/SgCpHSVW5-Cy', state: flash[:state])
+    assert_redirected_to 'http://www.example.com/login'
+  end
+
+  test "protecting against open redirects given a double-slash net path" do
+    post google_sign_in.authorization_url, params: { proceed_to: '//evil.example.org' }
+    assert_response :redirect
+
+    get google_sign_in.callback_url(code: '4/SgCpHSVW5-Cy', state: flash[:state])
+    assert_response :bad_request
+  end
+
+  test "protecting against open redirects given a triple-slash net path" do
+    post google_sign_in.authorization_url, params: { proceed_to: '///evil.example.org' }
+    assert_response :redirect
+
+    get google_sign_in.callback_url(code: '4/SgCpHSVW5-Cy', state: flash[:state])
+    assert_response :bad_request
+  end
+
   test "receiving no proceed_to URL" do
     get google_sign_in.callback_url(code: '4/SgCpHSVW5-Cy', state: 'invalid')
     assert_response :bad_request

test/models/redirect_protector_test.rb

@@ -44,9 +44,41 @@ class GoogleSignIn::RedirectProtectorTest < ActiveSupport::TestCase
     end
   end
 
-  test "allows path target" do
+  test "disallows relative path target" do
+    assert_raises GoogleSignIn::RedirectProtector::Violation do
+      GoogleSignIn::RedirectProtector.ensure_same_origin 'callback', 'https://basecamp.com'
+    end
+  end
+
+  test "allows absolute path target" do
     assert_nothing_raised do
       GoogleSignIn::RedirectProtector.ensure_same_origin '/callback', 'https://basecamp.com'
     end
   end
+
+  test "disallows double-slash path target" do
+    assert_raises GoogleSignIn::RedirectProtector::Violation do
+      GoogleSignIn::RedirectProtector.ensure_same_origin '//evil.example.org', 'https://basecamp.com'
+    end
+  end
+
+  test "disallows triple-slash path target" do
+    assert_raises GoogleSignIn::RedirectProtector::Violation do
+      GoogleSignIn::RedirectProtector.ensure_same_origin '///evil.example.org', 'https://basecamp.com'
+    end
+  end
+
+  test "disallows invalid paths" do
+    assert_raises GoogleSignIn::RedirectProtector::Violation do
+      GoogleSignIn::RedirectProtector.ensure_same_origin '/a path with spaces is invalid', 'https://basecamp.com'
+    end
+
+    assert_raises GoogleSignIn::RedirectProtector::Violation do
+      GoogleSignIn::RedirectProtector.ensure_same_origin '/path#with-fragment', 'https://basecamp.com'
+    end
+
+    assert_raises GoogleSignIn::RedirectProtector::Violation do
+      GoogleSignIn::RedirectProtector.ensure_same_origin '/path?with=query', 'https://basecamp.com'
+    end
+  end
 end