Merge pull request #73 from basecamp/flavorjones/fix-url-check

Prevent redirects to malformed URLs

Author: flavorjones

lib/google_sign_in/redirect_protector.rb

@@ -9,9 +9,12 @@ class Violation < StandardError; end
     QUALIFIED_URL_PATTERN = /\A#{URI::DEFAULT_PARSER.make_regexp}\z/
 
     def ensure_same_origin(target, source)
-      if target.blank? || (target =~ QUALIFIED_URL_PATTERN && origin_of(target) != origin_of(source))
-        raise Violation, "Redirect target #{target.inspect} does not have same origin as request (expected #{origin_of(source)})"
+      if (target =~ QUALIFIED_URL_PATTERN && origin_of(target) == origin_of(source)) ||
+         target =~ URI::DEFAULT_PARSER.regexp[:ABS_PATH]
+        return
       end
+
+      raise Violation, "Redirect target #{target.inspect} does not have same origin as request (expected #{origin_of(source)})"
     end
 
     private

test/models/redirect_protector_test.rb

@@ -8,6 +8,18 @@ class GoogleSignIn::RedirectProtectorTest < ActiveSupport::TestCase
     end
   end
 
+  test "disallows URL target that is not a valid URL" do
+    assert_raises GoogleSignIn::RedirectProtector::Violation do
+      GoogleSignIn::RedirectProtector.ensure_same_origin 'https://basecamp.com\n\r@\n\revil.com', 'https://basecamp.com'
+    end
+  end
+
+  test "disallows URL target that is blank" do
+    assert_raises GoogleSignIn::RedirectProtector::Violation do
+      GoogleSignIn::RedirectProtector.ensure_same_origin '', 'https://basecamp.com'
+    end
+  end
+
   test "disallows URL target with different port than source" do
     assert_raises GoogleSignIn::RedirectProtector::Violation do
       GoogleSignIn::RedirectProtector.ensure_same_origin 'https://basecamp.com:10443', 'https://basecamp.com'